allthingssecurity opened a new pull request, #29409: URL: https://github.com/apache/camel/pull/29409
# Description [CAMEL-26714](https://issues.apache.org/jira/browse/CAMEL-26714), part of [CAMEL-26630](https://issues.apache.org/jira/browse/CAMEL-26630) (exchange sanitizer), built on the step listener SPI of [CAMEL-26688](https://issues.apache.org/jira/browse/CAMEL-26688) (#29374). Step 2 of the "Order of work" of `design/exchange-sanitizer.adoc`: the module `camel-sanitizer`, its plugin and options, rule R1 without "still pending", the report in the log, the JUnit extension and the CI switch. Nothing from the later steps: no on completion SPI, no R2, R3 or R4, no dev console or `camel get sanitizer`, no "still pending". ## What it adds - **`components/camel-sanitizer`** (next to `camel-chaos`, same layout and switch). `SanitizerContextServicePlugin` decides when the context initializes, before the routes are created: on with profile `dev`, `camel.sanitizer.enabled=true`, or for a context built on a thread the JUnit extension marked; never with profile `prod` (checked first); with no profile it stays off unless enabled. When on, it installs its `StepListener` and logs a WARN that it is on (for a context tied to a test, once per JVM and then DEBUG). With pooled exchanges it stays off and logs a WARN, as the design says. `Sanitizer.get(camelContext)` gives the violations, the counts per kind and the report. - **Rule R1**: for each call of the processor of a step (each attempt, also each redelivery), the callback is called exactly once and `process` does not throw. Reported when it happens, at WARN: - `COMPLETED_TWICE`: at the second call of the callback, before it continues the exchange, with the thread and stack of the first call (kept from when it happened) and of the second; - `THREW_BEFORE_CALLBACK`: `process` threw and the callback had not been called, with the exception and its stack; a processor that throws after it called its callback is reported too, as `THREW_AFTER_CALLBACK`; - `CALLBACK_AFTER_THROW`: the callback came after `process` threw. The record of a call is the state that camel-core keeps in the callback of the call (the design's "no table to clean up"): ids, names, thread names and stacks, never the exchange. Its events are applied under the lock of the record, so within one call the order the record sees decides the kind, not whether a violation is reported (see the TLA+ model below, which covers one call). An exception thrown on through outer steps is reported once, at the innermost step: the call of an enclosing step ignores it, as if that call had not thrown (a thread-local of the last reported throw, cleared at the next step call of that thread). Each report names the exchange, route, step and the class of the processor, looking through the `DelegateAsyncProcessor`/`DelegateSyncProcessor` Camel puts around a processor; for a step that sends to an endpoint (`SendProcessor`) it adds the endpoint, URI sanitized and class, as the broken producer is behind it (its frames are in the stacks). It ends with the usual fix for its kind and holds no body or header. After 1000 violations in a context the others are counted only. - **The report on shutdown**: when the context stops, if there was a violation, the counts per kind and one line per violation at WARN (each full report was logged when it happened), the full reports again at DEBUG; after 1000 violations in a context the others are counted only. - **Options** (`camel.sanitizer.*`, read from the properties component; a `SanitizerConfigurationProperties` group in camel-main, as `ChaosConfigurationProperties`, with the generated configurer and metadata): `enabled`, `checks` (default `callback`, the only check of this version; an unknown check stops the start), `stackDepth` (default 32; 0 keeps only thread names), `reportOnShutdown` (default true). None is `insecure`. - **JUnit extension** (`org.apache.camel.sanitizer.junit`, JUnit Jupiter API only, optional dependency compiled against the JUnit 5 API so it also runs on JUnit 6): `@CamelSanitizer` (meta-annotated with `@ExtendWith(CamelSanitizerExtension.class)`), `@ExpectSanitizerViolation` (repeatable) and `SanitizerViolationError`. The extension opens a session per test class and marks the thread in `beforeAll` and `beforeEach`; the plugin reads the mark on the thread that builds the context (`ContextServicePlugin.load`), so the context is tied to the class. A violation fails the test that runs when it happens, after it ran, with the first violation and the reports of up to ten (as a suppressed exception when the test fails on its own); one charged between tests or after the last fails the class. When the class ends its contexts are untied; a violation of an untied context is charged to the classes running then. Nothing is set for the whole JVM. Every violation is also written to `target/cam el-sanitizer/<test class>.txt`. `SanitizerTestSession` is public only for the extension and says it is internal, not an API. - **CI switch**: the module registers the extension for JUnit's automatic detection (`META-INF/services/org.junit.jupiter.api.extension.Extension`), so the recipe of the design works with no change to the tests (detected automatically for a class without `@CamelSanitizer`, the extension acts only with `camel.sanitizer.enabled=true`, so a project that has JUnit detect its own extensions does not switch it on by accident); in the Camel build, `mvn test -Psanitizer` in `core/camel-core` sets the same system properties with `<systemPropertyVariables>` and no reruns. - camel-core depends on `camel-sanitizer` in test scope (the design: the module has no tests of its own, they are in camel-core), the docs page and nav, the catalog and BOM entries, and the design marks steps 1 and 2 done (plus the stray line break Claus noted on #29296). ## Where the design left a choice (kept minimal), or this PR differs - **A fourth kind, `THREW_AFTER_CALLBACK`.** The design lists "a processor that throws after it called its callback is reported too" under "threw before calling the callback"; a name that says "before" would be false, so it has its own constant. - **Only the options R1 uses.** `overlapSampling`, `overlapWindow` and `pendingTimeout` come with R3 and "still pending"; `checks` accepts `callback` only for now. `@CamelSanitizer` has no attributes yet (its `checks` and `overlapSampling` come with R2/R3). - **`@ExpectSanitizerViolation` takes one or more kinds**, any of which meets it. A processor that throws and calls its callback on another thread produces either `THREW_BEFORE_CALLBACK` and `CALLBACK_AFTER_THROW`, or `THREW_AFTER_CALLBACK`, depending on which the record sees first; the tests of the fixes run that race on 50 exchanges. When the contexts of the class stayed off on purpose (profile `prod`, `camel.sanitizer.enabled=false`, pooled exchanges, as the `*PooledTest` subclasses use) and none installed the sanitizer, an expected violation is not required. - **Naming the processor of a user.** The step listener SPI passes the route and node ids, not the processor. Camel's own processors are found by id (`Route.filter`); a processor of a user is not `IdAware`, so the plugin reads it from the step's definition (`ProcessDefinition.getProcessor()`) when the routes are added (`LifecycleStrategy.onRoutesAdd`), not when a violation is reported (reading the model then could wait on a route being removed). That is why the module depends on `camel-core-model` too (as camel-chaos does), not only on camel-api and camel-support. - No `camel run --sanitizer` (step 5); when a `camel.sanitizer.*` option is set (or `sanitizerConfig()` used) and the sanitizer would be on, camel-main logs a WARN if the JAR is missing, as for chaos (`MainSanitizerWithoutJarTest`). The Maven profile `sanitizer` is in camel-core only; the design also puts it in the async components, whose suites matter for R3, so it comes with R3. - An `Error` (such as an `AssertionError` in a test) thrown by a plain `Processor` passes through Camel's `DelegateSyncProcessor` after it called the callback, so it is reported as `THREW_AFTER_CALLBACK`; the docs say so. - The tests of one class must run one at a time (JUnit's default); classes may run in parallel. - The design's Validation step "together with the others": `ManagedSanitizerTest` (camel-management, which takes camel-sanitizer in test scope) runs the sanitizer with a `StepHook` that fails the first attempt of a step (the SPI camel-chaos uses; camel-chaos itself is not a dependency of camel-management), JMX, the tracer and the backlog debugger: each still works, the step the hook failed is not a call, and a processor that throws is reported and counted by JMX. ## Validation (design, "Validation") **1. The real bugs.** The step listener SPI exists only on main, so "the commit before the fix" is done as: in the worktree of this branch (at an earlier state, 19c8454, before the review changes; those changed how a report names the processor and how the log is capped, not when R1 reports), the main-code part of each fix is reverted (tests kept, not committed), and the fix's own regression test runs with the CI switch on (`-Psanitizer`, or the system properties for the JMS modules, which get `camel-sanitizer` in test scope on that branch only). | Bug | Fix reverted (main code only) | The fix's test with the sanitizer on | Reports | |---|---|---|---| | CAMEL-25095 camel-jms | `JmsProducer`, `ReplyManager`, `ReplyManagerSupport` (29cb79b) | `JmsInOutSendFailureCallbackTest`: 4 of 5 fail | **completed twice** at the `to` step of each of the 4 (first call on the reply-manager thread or the `JmsReplyManagerOnTimeout` thread, second call in `JmsProducer.process` on the caller, or the other way round in the commit-failure test) | | CAMEL-25122 camel-sjms | `SjmsProducer`, `ReplyManager`, `ReplyManagerSupport` (21a37f4) | `InOutSendFailureCallbackTest`: 3 of 3 fail | **completed twice** at `to1`, `to6`, `to8` (reply thread or `JmsReplyManagerOnTimeout` thread first, `SjmsProducer.processInOut` second, or the other way round) | | CAMEL-26430 | `RedeliveryErrorHandler` (8c78228, with 26445 and 26446 reverted too, as they came after it) | `RedeliveryProcessorThrowsTest`: 11 of 13 fail (hang) | **threw before calling the callback** at the throwing step in 12 tests; the control whose processor calls back before it throws: threw after calling the callback; one also **callback after the processor threw** | | CAMEL-26445 | `RedeliveryErrorHandler` (e373d74, with 26446 reverted too) | `SimpleTaskProcessorThrowsTest`: 9 of 14 fail | **threw before calling the callback** in 11 tests, three of them also **callback after the processor threw**; completed twice in the test whose processor calls back twice | | CAMEL-26446 | `RedeliveryErrorHandler` (5551dee) | `RedeliveryFirstDeliveryProcessorThrowsTest`: 17 of 20 fail | **threw before calling the callback** in 16 tests, and **callback after the processor threw** in the three whose processor calls back after the throw; completed twice in the test that calls back twice | | CAMEL-26461 | `NoErrorHandler` (0c158cb) | `NoErrorHandlerProcessorThrowsTest`: 9 of 16 fail | **threw before calling the callback** in 10 tests (three also callback after the throw); completed twice in the test that calls back twice | | CAMEL-26464 | the three lines of `DefaultChannel.initChannel` that switch the guard on for a step without an error handler (a manual revert: the `CamelInternalProcessor` part of 9a17198 does not revert cleanly, since CAMEL-26601 builds on it, and is dormant without those lines) | `StepWithoutErrorHandlerProcessorThrowsTest`: 13 of 21 fail | **threw before calling the callback** in 17 tests, at the step inside `doTry`, `doCatch`, `doFinally`, `onException`, a multicast output and the others | Controls: the same two JMS tests on main with the sanitizer on pass with no report (3/3 and 5/5), so the double completions above are the bugs, not the sanitizer. The core fix tests on main (fixes in place) report the same throws, because those tests use processors that throw on purpose: R1 checks the processor, not whether Camel copes with it; they now carry `@ExpectSanitizerViolation` (see 3.). Not seen, as the design says: the tests where the processor that throws is a recipient of a recipient list (`recipientList*` in `NoErrorHandlerProcessorThrowsTest`, CAMEL-26601). A recipient is not a step; the recipient list step that never completes is "still pending", step 6 of the order of work. **2. User code** (all fail with a report that names the step and the class): - camel-core, `ContextTestSupport` with `@CamelSanitizer` (`SanitizerUserCodeTest`): an `AsyncProcessor` that completes from a timeout thread and again from a late reply thread (the design's `PriceLookup`); one that throws; one that throws after its callback; the producer of a custom endpoint that completes twice, reported at its `to` step with the endpoint and the producer's frame; one that throws and calls back later; an `AggregationStrategy` of the aggregate EIP that throws an `Error` (an assertion), which the EIPs do not catch, so the aggregator throws from `process`: reported at the `aggregate` step with the strategy's frame first in the stack. A strategy that throws a `RuntimeException` is caught by the splitter, which fails the exchange: no rule is broken and nothing is reported. - `CamelSanitizerExtensionTest` runs test classes of its own through the JUnit launcher: a test whose route breaks R1 fails with `SanitizerViolationError` and the report; a test that fails on its own keeps the report as suppressed; a met expectation passes and a missing one fails; a second callback after the tests fails the class; each invocation of a `@ParameterizedTest` and `@RepeatedTest` is charged on its own; the thread is unmarked after the class. - camel-test-main-junit6: `SanitizerCamelTestSupportTest` (`CamelTestSupport`: callback twice, a throw, the aggregation strategy) and `SanitizerCamelMainTest` (`@CamelMainTest`, whose extension builds the context in its `beforeEach`). - Spring Boot: left out; camel-spring-boot is another repository and not available offline here. - `SanitizerRecordTest` (the record of a call under two callbacks and a throw racing on three threads, 200 rounds: each violation reported exactly once; the 1000-violation bound; the shutdown summary), `SanitizerActivationTest` (switch, profile, mark, options, pooled exchanges, stop and start), `SanitizerUntiedContextTest` (sessions) and `SanitizerConfigurationPropertiesTest` (camel-main). A processor given by reference (`process("ref")`, the `ref` of XML/YAML routes) is named too. **3. Quiet on main.** `mvn test -Psanitizer` in camel-core (the CI switch: automatic detection of the extension, `camel.sanitizer.enabled=true`, no reruns), the **whole camel-core suite**: 8993 tests in 3116 classes, 4 minutes (JDK 21, offline reactor of the core modules, 2 parallel classes). The sanitizer reported violations only in tests that break R1 on purpose, and no false report and no new bug: - the tests of the seven fixes above (`RedeliveryProcessorThrowsTest`, `SimpleTaskProcessorThrowsTest`, `RedeliveryFirstDeliveryProcessorThrowsTest`, `NoErrorHandlerProcessorThrowsTest`, `StepWithoutErrorHandlerProcessorThrowsTest` and their support classes) and two tests of the step listener SPI whose processors throw or call back twice: 76 test methods now carry `@ExpectSanitizerViolation` with the kinds the run reported (the `*PooledTest` subclasses run with the sanitizer off, so their inherited expectations are not required); - `StepListenerTest` counted the installed listeners: it now compares only its own two; `StepListenerNotInstalledTest` is skipped (an assumption) when a listener is installed, as with the CI switch; - in the first run, `SanitizerActivationTest` of this PR started contexts not tied to a test, whose violations were charged to two task tests running in parallel, as the extension documents for untied contexts: those contexts are tied to sessions of their own now, and the one test of untied contexts is `@Isolated`. After these changes, the whole camel-core suite again (on the final commit): with the CI switch **9001 tests, 0 failures, 0 errors** (46 skipped, one of them the assumption above), and without it (camel-sanitizer on the test classpath but off) **9001 tests, 0 failures, 0 errors** (45 skipped). In the CI run every violation in the `target/camel-sanitizer/` files is marked `(expected)`, except those of the test classes that `CamelSanitizerExtensionTest` runs to fail on purpose. The targeted classes plus camel-management (`ManagedSanitizerTest`, `ManagedStepListenerTest`), camel-main and camel-test-main-junit6 without the CI switch: 0 failures, after a fix of one assertion of the new `SanitizerRecordTest` (it counted three violations per round, but a throw after the first callback makes two). The new camel-core sanitizer tests 20 times in one JVM, without and with the CI switch: 580/580 each. (In the first run `ValidatorExternalResourceTest` failed because it reads an XSD from GitHub and the network was down then; it passed in the last run and reports nothing.) The netty, netty-http, vertx-http and JMS request/reply suites of the design's step 4 are for R3; only the two JMS tests above ran here. **4. The side table's model.** `SanitizerR1.tla` (kept with the hunting models, not in this PR, as the design says of models): one call of a step whose processor is any program — 0 to 2 callbacks on the calling thread before it returns or throws, one more on each of two other threads it started (overlapping the throw), and a throw or not; each handler split where the code is (the synchronized block of the record, then the report). TLC (`-workers 2`): **789 distinct states, no error** for: no false report at any moment (each report backed by what the program did, and a callback made before the throw on the same thread is never reported as after it), no missed violation when all threads are done (exact counts per kind), and termination. Non-vacuity: each kind of report, the overlap (a callback taking the lock between the throw's lock and its report) and a quiet correct call are reachable (each witness invariant is violated, as intended); and a mutant that reads and writes the call back count in two steps (no lock) is caught (`NoMissedViolation` violated: a missed "completed twice"). **5. Cost.** The order of work puts the JMH runs at step 1 (no listener: within noise of main, one no-op listener about 34 ns per step, #29374) and at step 3 (the on completion listener). This PR changes no camel-core main code, so the routes without the sanitizer are as before; it claims no number for the sanitizer on (the design: "No number is claimed before that run"). Its main cost is a stack walk of up to `stackDepth` frames at the first callback of each call; `stackDepth=0` avoids it. The design's Validation step "Cost" also lists a run with the sanitizer on (with and without stacks); the order of work does not put it at this step, so it is not in this PR — say if you want it before merge. ## Follow-ups (not in this PR) - A scheduled or `workflow_dispatch` CI job that runs `mvn test -Psanitizer` in camel-core, so the expectations on the fix tests do not rot. - The `sanitizer` profile in the async component suites, and their runs, with R3. # 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-26714](https://issues.apache.org/jira/browse/CAMEL-26714). # 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, camel-main, camel-sanitizer and camel-test-main-junit6 with the formatter and import-sort plugins and the package plugin of this branch; the camel-main metadata and `main.adoc` 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]
