allthingssecurity opened a new pull request, #29664: URL: https://github.com/apache/camel/pull/29664
# Description [CAMEL-26768](https://issues.apache.org/jira/browse/CAMEL-26768), part of [CAMEL-26630](https://issues.apache.org/jira/browse/CAMEL-26630) (exchange sanitizer), on the `camel-sanitizer` module of [CAMEL-26714](https://issues.apache.org/jira/browse/CAMEL-26714) (#29409). Builds on the on completion listener SPI of #29436 ([CAMEL-26742](https://issues.apache.org/jira/browse/CAMEL-26742)), on [CAMEL-26741](https://issues.apache.org/jira/browse/CAMEL-26741) (an `Error` thrown by a plain `Processor` fails the exchange) and on R1 "still pending" ([CAMEL-26751](https://issues.apache.org/jira/browse/CAMEL-26751)), all merged: one commit on main. Step 3 of the "Order of work" of `design/exchange-sanitizer.adoc`, second part: rule R2, "each consumer commit runs exactly once, for the unit of work that owns it". Nothing of R3 or R4, no dev console. R1 is unchanged. ## What it adds A *commit* is an on completion that an exchange holds when it gets the unit of work created for it, such as the file consumer's `GenericFileOnCompletion` (it moves into the unit of work then). On completions added later, such as a route `onCompletion` or the tracer's, are not commits. R2 follows each add of a commit, as the `Synchronization` Javadoc states the contract, and reports four new kinds of `Violation` (`getRule()` returns `R2`): | Kind | When | Reported | |---|---|---| | `COMMIT_RUN_TWICE` | a commit runs again for one add (a unit of work done twice; a commit run where it was handed over from and again by the copy it went to) | before the second run, with the thread and stack of both runs | | `COMMIT_HANDED_OVER_BY_OTHER_EXCHANGE` | an exchange that uses the unit of work of another exchange hands its commit over (CAMEL-26576) | at the handover, before the commit runs with the outcome of the copy, naming the step that runs on that thread (such as the `to("seda:...")` of the part) | | `COMMIT_HANDED_OVER_TWICE` | a commit is handed over again from a unit of work it already left (CAMEL-26569) | at the second handover, with the stacks of both | | `COMMIT_NEVER_RUN` | the exchange of its unit of work is done and it was neither run, handed over nor taken out; or the context stops while it waits in a copy that was never routed (a `seda` queue without a consumer) | at the done event of that exchange, or when the context stops | ```text Exchange sanitizer: commit handed over by an exchange its unit of work was not created for (R2) commit org.apache.camel.component.file.GenericFileOnCompletion of exchange 232F75255D490A3-0000000000000001, route route10, consumer org.apache.camel.component.file.FileConsumer, endpoint file://target/data/... at step to19 of route route10, processor org.apache.camel.processor.SendProcessor, endpoint seda://line (org.apache.camel.component.seda.SedaEndpoint) its unit of work was created for exchange 232F75255D490A3-0000000000000001, and exchange ...-0000000000000004 handed it over to exchange ...-0000000000000005, so it would run with the outcome of exchange ...-0000000000000005 handed over: thread Camel (camel-1) thread #2 - Split at org.apache.camel.support.AbstractExchange.handoverCompletions(AbstractExchange.java:...) ... ``` (the report of the validation run with the CAMEL-26576 fix reverted, below). - **`SanitizerOnCompletionListener`** on the SPI of #29436, and **`Commits`**, the records. They keep no exchange: a weak map of the units of work (each holding the id of the exchange it was created for: the first exchange given a unit of work is the one it was created for, at every call site), and the on completions are held weakly, by identity, one record per add (one on completion object can be on many exchanges). The commits that wait in exchanges without a unit of work (copies in a seda queue) and those taken out to be run by someone else are bounded (10 000; past it the oldest taken out goes first, then the oldest waiting; one WARN and the report count them). One lock, held briefly; the stacks are taken outside it. - **Exchange done, one notifier for both.** R1 "still pending" (CAMEL-26751) already adds an `EventNotifier` for exchange completed and failed only, to drop the pending calls of an exchange that is done. R2 needs the same event to find the commits that never ran, so there is still one notifier (`SanitizerContextServicePlugin.ExchangeDoneNotifier`), and it calls one method, `Sanitizer.exchangeDone(Exchange)`, which does both: R2's never-run check and the still pending clean-up. It is added when either is on (`Sanitizer.needsExchangeDone()`): with the default `callback,commit` and the default `pendingTimeout` of 60s, with `checks=callback` (still pending only), with `checks=commit` (R2 only), and with `pendingTimeout=0` while R2 is on. `SimpleEventNotifierEventsTest` already has the change of CAMEL-26751. - **Still pending needs R1.** The still pending warning follows the callbacks of R1, so with `checks=commit` only it is off (no table of pending calls), whatever `pendingTimeout` says; the option docs and the camel-main Javadoc say so. - **The step.** The step listener of R1 also tells R2 which step runs on a thread (only when R2 is on), so a handover report names the step of the exchange that handed over, or of the commit's exchange. - **Option.** `camel.sanitizer.checks` gets `commit`; the default is `callback,commit` (camel-main, generated metadata, `main.adoc`, catalog copies). `checks=commit` alone keeps the step listener (it still wraps and counts the steps, and tells R2 the step). - **Docs**: an R2 section in `sanitizer.adoc` (kinds, report, what is not checked, where a never-run at stop is charged, the shared notifier); `design/exchange-sanitizer.adoc`: steps 1, 2, 3 and 6 built, and the R2 changes below. - **Tests of CAMEL-26751** adapted to the shared notifier: `SanitizerActivationTest` (one notifier with both on, with `checks=callback`, with `pendingTimeout=0` while R2 is on; none with `checks=callback` and `pendingTimeout=0`; no table of pending calls with `checks=commit`), `SanitizerPendingCallsTest` (`exchangeDone` takes the exchange), `SanitizerStillPendingTest` (runs with R2 on). - **A test of #29436** (`OnCompletionListenerTest`): with the CI switch the sanitizer's own listener is installed, so two tests compare only their own listeners, and `testCopyNotQueuedGivesItBack`, which leaves a copy in a seda queue without a consumer on purpose, says `@ExpectSanitizerViolation(COMMIT_NEVER_RUN)`. ## Where this differs from the design - **Item 3 of R2, "two handovers through the same unit of work from two threads that overlap in time, seen with a watch window"**, is checked by its outcome instead: a commit handed over twice (or run twice, or never). Since CAMEL-26569 the unit of work takes each on completion out under its lock, so an overlap alone is not a violation (parallel parts with `shareUnitOfWork` hand over their own on completions at the same time on main), and a watch window would be R3's sampling. The design text says so. - **What is not checked**, now written in the design and the docs: a custom `UnitOfWork` that does not tell what it hands over (the default `handoverSynchronizationWithResult` returns `null`) or does not report its adds is not checked for a commit handed over twice or never run; commits taken out as a list (`handoverCompletions()`, as seda does with `multipleConsumers` and `purgeQueue`) are checked for running once only, matched by their on completion alone, so one object shared by many exchanges can be matched to another exchange's record; an on completion added to a unit of work after it exists (a processor that adds it, as `SplitParallelShareUnitOfWorkHandoverTest` does for its "consumer") is not a commit. - **Limits of ownership by unit of work** (found in our pre-review of this PR): a copy that keeps the owner's exchange id (Loop `copy=true`, RoutingSlip, FailOver) counts as the owner, so a handover by it is not reported; a part that hands over a commit it was given itself (a `pollEnrich` resource with its own unit of work) would be reported. Neither shows in the runs below. ## Validation (design, "Validation") **On main, after #29525 and #29528 merged.** This commit was rebuilt on main once both had merged (#29528 had gained the soft bound and the striped table of pending calls since R2 was first built on it); the only conflict was the description of `pendingTimeout` (camel-main Javadoc, `main.adoc`, the generated metadata, `sanitizer.adoc` and the catalog copies), where both sentences are kept: "0 turns the warning off, as does a value of checks without callback" and the note about the parts of a parallel split. The sanitizer tests of camel-core (121, with `SanitizerStillPendingTest`, `SanitizerPendingCallsTest`, `SanitizerActivationTest` and the R2 classes), camel-management (`ManagedSanitizerTest`), camel-main (10) and camel-test-main-junit6 (4), with `OnCompletionListener*Test` and `SimpleEventNotifierEventsTest`: 136/136 without the CI switch and 136/136 with it. 20 times in one JVM: the R2 classes, the still pending classes, `OnCompletionListenerTest` and `SimpleEventNotifierEvent sTest` 1940/1940 without the switch, and 1620/1620 with it (without `SanitizerActivationTest`, which turns the sanitizer on and off itself). The whole camel-core suite with the switch: 9203 tests, 8 failures, 1 error (`ValidatorExternalResourceTest`, network), 46 skipped. The 8 failures are R1 reports, not R2: the `VirtualMachineError` cases of `ProcessorThrowsErrorTest` (5) and `BeanMethodThrowsErrorTest` (3), added by CAMEL-26741 (#29525, #29575), throw the `Error` on after the callback by design, and R1 reports "process threw after calling the callback" for them; the same 8 fail on main without this commit (the two classes run with the switch). A follow-up marks them as expected (`@ExpectSanitizerViolation`). R2 reports only in `SanitizerCommitTest` and `OnCompletionListenerTest` (both expected); the other report files are R1 reports of tests that break the rule on purpose (and the test classes that `CamelSanitizerExtensionTest` runs on purpose). Sections 1 to 5 were measured on R2 before (R2 alone on main, then on main with #29525 and #29528 as branches); the code of R2 is the same, only joined with the still pending check. **1. The real bugs (CAMEL-26576, CAMEL-26569).** Both are fixed by one commit on main, 62cf348fd7ec (#29203). On a local validation branch from this commit (deleted after the runs), the fix was reverted in two steps, keeping the SPI calls, and the tests run with the CI switch (`-Psanitizer`): - *CAMEL-26576 reverted* (parts of `shareUnitOfWork` get `setUnitOfWork` again): `ShareUnitOfWorkSedaHandoverTest` fails 10 of 12 on its own; R2 reports `COMMIT_HANDED_OVER_BY_OTHER_EXCHANGE` in 9 of them, each at the handover, at the `to("seda:line")` step of the part (split, nested split, multicast; the recipient list has no step for its recipient, as the design says), for the file consumer's `GenericFileOnCompletion` and for the test's commit, before the commit runs with the seda copy. The tenth, `testOnCompletionAddedToTheUnitOfWorkStaysWithTheParent`, adds its on completion to the unit of work directly, so it is not a commit. - *Both reverted* (the pre-CAMEL-26569 iterator handover too): the stress probe of CAMEL-26569 (parallel split with `shareUnitOfWork`, every part to seda, a commit added before routing), six runs, 8100 exchanges: every commit that ran twice (5) was reported `COMMIT_HANDED_OVER_TWICE` at the second handover, before it ran again; every exchange whose part handover completed was reported `COMMIT_HANDED_OVER_BY_OTHER_EXCHANGE`. Two commits never ran: in both, the race had corrupted the list of the unit of work (`DefaultUnitOfWork.done` threw `NegativeArraySizeException: -1`, which Camel logs at WARN), so the done event never came and R2 could not see it. `SplitParallelShareUnitOfWorkHandoverTest` (fails 3 of 3 on its own) gets no R2 report: its "consumer" on completion is added by a processor after the unit of work exists, so it is not a commit. **2. User code.** `SanitizerCommitTest`: a custom consumer (`BrokenUserCode$DoneTwiceConsumer`) that creates the unit of work itself and calls `done` after the processing and again in `finally` fails its test with `COMMIT_RUN_TWICE`, naming the commit class (`DoneTwiceConsumer$MessageCommit`), the consumer class and both stacks at `DoneTwiceConsumer.receive`. Also: a copy of the exchange that hands the commit over is reported at its step; a commit taken out of the unit of work without the exchange never runs; a commit in a seda queue without a consumer is reported when the context stops. **3. Quiet on main.** On R2 alone on main (before the rebuild), the whole camel-core suite with the CI switch: **9107 tests, 0 failures, 1 error** (46 skipped): `ValidatorExternalResourceTest`, which loads an XSD from raw.githubusercontent.com and timed out on the network here, as it does without the sanitizer. Every report file of the run is marked expected, apart from the R1 extension's own test classes that `CamelSanitizerExtensionTest` runs on purpose. An earlier run, before the `OnCompletionListenerTest` change above, had exactly 3 failures, from the sanitizer's own listener and the copy left in the queue on purpose; with the change they pass, 33/33 with and without the switch. R2 reports appear only in `SanitizerCommitTest` (expected, annotated) and `OnCompletionListenerTest.testCopyNotQueuedGivesItBack`. `SanitizerCommitFileTest` runs a real file consumer (`move=done`) through a split, a split with `shareUnitOfWork`, an aggregator, a route `onCompletion`, a `wireTap` and a `pollEnrich` whose resource is a seda copy holding the commit of another file, each to seda: no report, each file moved once. These runs found no new bug in Camel. They do not cover every place that hands over a commit: the `TopicLoadBalancer` and `Enricher` cases were not exercised by the camel-core suite (see Follow-ups). **4. Model.** A TLA+ model, kept outside this PR: the records of `Commits` against the real moves and runs of one commit, with the handover, the copy's routing or give-back, the owner's done and the context stop on different threads, each change and its listener event as separate steps. With correct code, a part that hands over (CAMEL-26576), the racy handover (CAMEL-26569), a unit of work done twice, and all three together: no false report, no missed violation, each kind sound and complete (largest 3464 states). A mutant (the copy's unit of work does not take the records of its list) gives a false report; witnesses reach a quiet handover, a quiet give-back and each of the four reports. **5. Tests.** `SanitizerCommitsTest` (the records, 18, including 8 threads × 2000 commits), `SanitizerCommitTest` (10), `SanitizerCommitFileTest` (6), `SanitizerActivationTest` (checks): 20 times in one JVM without and with the CI switch, together with `OnCompletionListenerTest` and `SimpleEventNotifierEventsTest`: 1200/1200 without the switch and 940/940 with it. Negative controls, each a mutant of `Commits`: no owner check fails 2 tests, no never-run 2, no second run 6, the copy's records not moved 3, no handed-over-twice 1. The sanitizer tests of camel-core, camel-management, camel-main and camel-test-main-junit6 pass on this commit; formatter and import sort ran in the build; the generated files are as the package plugin writes them and the catalog copies are byte-identical. ## Questions for the maintainers - **Cost with the sanitizer on.** Each unit of work, on completion added, run and exchange done takes the lock of the records once; the design's JMH run with the sanitizer on (a commit and a seda handover, 1 and N threads) is not done yet. Before merge, or with the dev console step? - **Never run at context stop** reports a copy left in a seda queue without a consumer (and a resequencer's buffer). That is what the design asks, but tests that stop with such a copy on purpose need `@ExpectSanitizerViolation`. Keep it a violation, or make it a warning like R1 "still pending"? - **Exchange done.** Would an end-of-unit-of-work event on the on completion listener be better than the event notifier (it would not turn on the exchange events)? The design chose the notifier. ## Follow-ups (not in this PR) - The `-Psanitizer` runs of camel-file and camel-ftp, now that R2 is on by default. - Mark the `VirtualMachineError` cases of `ProcessorThrowsErrorTest` and `BeanMethodThrowsErrorTest` as expected R1 violations, so the camel-core suite passes with the switch again. - Tests that exercise the handover in `TopicLoadBalancer` and `Enricher` with the sanitizer on; the camel-core suite does not reach them. # 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-26768](https://issues.apache.org/jira/browse/CAMEL-26768). # 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, the base components, camel-sanitizer, camel-management, camel-main and camel-test-main-junit6 with the formatter and import-sort plugins and the package plugin; the camel-main metadata and `main.adoc` and the sanitizer JSON are generated, and the catalog copies match them. 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]
