allthingssecurity opened a new pull request, #29586: URL: https://github.com/apache/camel/pull/29586
# Description [CAMEL-26816](https://issues.apache.org/jira/browse/CAMEL-26816) `JdbcOrphanLockAwareIdempotentRepository` used the inherited `confirm()`, which only returns true, and its row has no "processed" state (`processorName, messageId, createdAt`). So a processed and confirmed key was treated like an in-flight lease: - while its instance ran, the key stayed in the keep-alive set (removed only by `delete`), and the keep-alive task refreshed its `createdAt` on every round; the set and each keep-alive batch grew with every processed message; - when the instance stopped (rolling restart, scale-in, crash), the row was no longer refreshed, and once it was older than `lockMaxAgeMillis` another instance took the key over: `add()` returned true for a message that was already processed. With the Idempotent Consumer EIP (eager, the default), the file consumer with `idempotent=true` / `noop=true`, or `readLock=idempotent` with the default `readLockRemoveOnCommit=false`, a duplicate that reached another instance later was processed again. The JIRA left the direction to the maintainers (confirm marks the row as processed, or the repository is documented as in-flight locks only). This PR takes the first one, without a schema change: documenting it as locks only would leave the keep-alive set and batch growing with every processed key, and a state column would need a schema migration of every existing table. - `confirm()` sets the `createdAt` of the key to null (new `confirmQuery` property; `tableName` applies to its default, a custom `confirmQuery` is kept) and stops keeping the key alive. In a transaction (its own, or the caller's), the key leaves the keep-alive set after the commit, so a confirm that rolls back keeps the lease alive; and an `add` of the same key earlier in that transaction no longer keeps it alive after the commit (as `delete` already did); - the orphan check counts a null `createdAt` as present (`createdAt >= ? OR createdAt IS NULL`), and the takeover (`createdAt < ?`) never matches it, so a confirmed key stays a duplicate, as in `JdbcMessageIdRepository`; - the keep-alive does not update a row whose `createdAt` is null, so a keep-alive that runs while a key is confirmed does not make it a lease again; - when the key cannot be confirmed, it stays a lock that is kept alive while its instance runs, as before, and `confirm()` returns false (it returned true before) instead of throwing: no such row, a failure of the database (one WARN, then DEBUG), or a `createdAt` column that does not store null. The last one is checked after the update (one more `SELECT COUNT` in the same transaction): a MySQL or MariaDB `TIMESTAMP` column with `explicit_defaults_for_timestamp=OFF` (the default of MySQL 5.7 and older MariaDB, kept by the column after a server upgrade) silently stores the current time for null, and a key confirmed that way would expire while its instance runs; - `confirmQuery` set to null keeps confirmed keys as locks (the way back to the old behaviour). A key that is added and not confirmed is still a lock that another instance takes over when its instance stops, as before. No caller uses the return value of `confirm()`. Why null and not a timestamp in the far future: a far-future value does not fit every column type (a MySQL `TIMESTAMP` ends in 2038), while the table that the repository creates (`createdAt TIMESTAMP`) allows null on PostgreSQL, Oracle, DB2, Derby, H2 and MySQL 8 (with its default `explicit_defaults_for_timestamp=ON`). The 4.24 upgrade guide names the columns that do not store null, the housekeeping by `createdAt` (it does not delete confirmed keys), custom tables, `readLockRemoveOnRollback=false`, and rolling upgrades (Camel versions before 4.14.6 and 4.18.2 take a confirmed key over at once; later ones do not, but their `contains` reports it as absent). Cost: a confirm is now one transaction with an UPDATE and a SELECT, where it did nothing before; in exchange the keep-alive batch no longer grows with every processed key. Docs: the Orphan Lock section of `sql-component.adoc` describes a confirmed key and the new `confirmQuery` (catalog copy regenerated), and the class javadoc says that inside a caller's transaction a failed confirm marks that transaction as rollback only (as a failed add or remove does). Still taken over, named in the upgrade guide: keys written before the upgrade (they keep their timestamp), and keys that are only added and never confirmed, such as by the file consumer with `idempotentEager=false` (`GenericFileOnCompletion` calls `add()` there and not `confirm()`); the latter is a follow-up for camel-file (I can file the JIRA). Tests, new `JdbcOrphanLockAwareIdempotentRepositoryConfirmTest` (two repository instances on one H2 database, as two nodes; the passing of time is an UPDATE that makes every lease older than the lock max age): - `testConfirmedKeyStaysADuplicateAfterItsNodeStops`: the case of the JIRA; - `testLeaseThatIsNotConfirmedIsTakenOverAfterItsNodeStops`: control, passes on main too; - `testConfirmedKeyIsNoLongerKeptAlive`: the key leaves the keep-alive set, and a keep-alive does not touch it; - `testKeyAddedAndConfirmedInOneTransactionIsNotKeptAlive`; - `testConfirmThatRollsBackKeepsTheLeaseAlive`: control (passes on main, where confirm does nothing); it guards the removal from the keep-alive after the commit; - `testConfirmOfAnUnknownKey`; - `testRemovedKeyCanBeAddedAgain`: control, passes on main too; - `testConfirmWithAColumnThatDoesNotStoreNull` (a confirm query that stores the current time, as MySQL does), `testConfirmWithANotNullColumn`: the key is not confirmed and stays a lock that is kept alive, without an exception; - `testNoConfirmQueryKeepsConfirmedKeysAsLocks`: the way back; - `testConfirmWithACustomTableName`: the confirm query follows `tableName`; - `testIdempotentConsumerSkipsADuplicateAfterTheNodeThatProcessedItStopped`: the Idempotent Consumer EIP on two CamelContexts. Against main, 5 of the first 8 tests fail (the two controls and the rollback case pass), as run before the failure-path tests were added; 2 of those use the new `confirmQuery` property, so the final test class does not compile against main, and against a first version of this change, which did not check that null was stored and let a database failure through, 3 of the failure-path tests fail (the custom table name passes there). With the change the 12 pass, 240/240 over 20 runs in one JVM; the existing orphan-lock tests passed 20 runs each with the first version (concurrency 20/20, keep-alive 60/60, transaction 140/140) and pass with the final one, as do the idempotent tests of camel-sql (on fresh main: camel-sql 379 run, 0 failures, without `SqlFunctionDataSourceTest`). `SqlFunctionDataSourceTest` cannot install its embedded MariaDB in my sandbox (2 errors with the first version); it does not use the repository. Upgrade guide: a 4.24 entry, as a confirmed key is no longer taken over after its instance stops. Main only (a behaviour change). # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it): [CAMEL-26816](https://issues.apache.org/jira/browse/CAMEL-26816). # Apache Camel coding standards and style - [x] I checked that each commit in the pull request has a meaningful subject line and body. - [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes. (I built the core modules and camel-sql with the formatter and import-sort plugins and ran the camel-sql tests; no generated file changes. I did not run the full root build.) # AI-assisted contributions - [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used. This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a `Co-Authored-By` trailer. _Claude Code on behalf of allthingssecurity_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
