papinifrancesco commented on issue #2630:
URL: https://github.com/apache/activemq/issues/2630#issuecomment-5959474384
I have a fix for this and would like to check the approach with you before
opening a PR. @cshannon, since this builds on AMQ-9698, your view would be
especially useful.
**Approach.** Implement `recoverExpired()` for the JDBC store, so `Topic`
uses it for JDBC as well as KahaDB and no longer falls back to `doBrowse()`.
Per eligible durable subscription, `JDBCTopicMessageStore.recoverExpired()`
runs two plain-SQL queries:
1. read the subscription's `LAST_ACKED_ID` and the id of the first pending
message that is not expired (or is part of a prepared XA transaction);
2. load the expired messages between those two ids, at most
`maxExpirePageSize` of them (`setMaxRows`).
Nothing in it is specific to PostgreSQL. A topic browse (JMX,
`StatisticsBroker`) expires through the same path on JDBC.
**A second bug, found while writing it.** The JDBC store records a durable
subscription's acks as a high-water mark (`UPDATE ACTIVEMQ_ACKS SET
LAST_ACKED_ID=?`), so acking an expired message also acks every earlier message
of that subscription. Today, when a message with a TTL expires behind one
without a TTL, the expiry task (or a browse) acks the expired one and the
earlier message is lost:
1. offline durable subscription, JDBC store;
2. send A with no TTL, then B with a 500 ms TTL;
3. wait for the expiry task: B is expired, `LAST_ACKED_ID` jumps to B's id;
4. restart the broker and reconnect the subscription: A is never delivered.
The same test passes on KahaDB, which tracks acks per message. That is why
the fix stops at the first non-expired message: it only expires the run of
expired messages directly after `LAST_ACKED_ID` (per priority with prioritized
messages). The visible difference from KahaDB is that expired messages queued
behind a non-expiring one stay in the store until that one is consumed; they
are still dropped at dispatch. Would you like this filed as a separate issue?
**Testing.**
- New unit tests on Derby for both bugs:
- `recoverExpired` itself: stopping at the first non-expired message, the
page limit, priorities, a prepared XA transaction;
- an end-to-end test with a broker restart, for both the expiry task and
the browse path;
- `JDBCPersistenceAdapterExpiredMessageTest` updated to the new method.
- The existing JDBC, expiry and durable-subscription tests show no
regressions.
- On PostgreSQL, the reproduction repository now also runs patched legs:
https://github.com/papinifrancesco/activemq-jdbc-topic-expiry-oom/actions/runs/37039551631.
6.3.2 and 6.2.10 with the patch survive the 200,000-message backlog. With 30 s
TTLs, the expiry task acks 400 messages per run with no out-of-memory error,
while unpatched 6.3.2 with the same TTLs runs out of memory.
**Left out:** a manual browse of a JDBC topic still reads the whole result
set on PostgreSQL. Only an explicit JMX or statistics browse triggers it, never
the periodic task, so I kept it out of this change.
**Disclosure:** the patch, the tests and the reproduction were written with
an AI assistant (Claude Code, Anthropic). The test results above come from the
runs described there: the unit tests locally, the PostgreSQL cases in the
linked CI run. In line with the ASF guidance on generative tooling, the commits
will carry a `Co-authored-by` trailer naming the tool.
If the approach is fine, I'll open a PR against `main`; the same change
applies to 6.3.x, and on 6.2.x with a whitespace-only difference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
