Repository navigation
test(reactor): a record completes on the terminal signal, not per item - #329
Conversation
A user function returning a publisher that emits nothing - Mono.empty(), a
Mono<Void> from Mono.fromRunnable(..), a Flux whose filter removed everything -
left its record in flight forever, and the consumer silently stopped once
maxConcurrency slots were held. No exception, no failed record, nothing in the
log: it just stopped committing and stopped calling the user function.
react() subscribed with Flux#subscribe(Consumer, Consumer), which takes onNext
and onError. There is no completion argument, so onComplete(..) - the method
that calls onUserFunctionSuccess and adds the record to the controller's mailbox
- was only ever reached by an emitted item. It now takes the three-argument
form, with completion on the completion signal and the item consumer left to the
user (items were already traced by the doOnNext above it).
BEHAVIOUR CHANGE, worth a release note: a publisher emitting n items previously
ran the success path n times and now runs it once, at completion. That is the
correct semantic and it is what the Mutiny engine has always done - the two
engines are offered as interchangeable, so they must agree on whether "produced
no value" means "succeeded".
The n-item case turned out to be broken too, which an earlier reading had
recorded the other way ("committed all five records cleanly", with
over-completion left as an untested hypothesis). Measured on a MockConsumer with
Flux.just(1, 2, 3), five records and maxConcurrency 4: before, 15 "Reactor
success" signals for 5 records, an entirely empty commit history, and
numberRecordsOutForProcessing pinned at 1; after, 5 signals, offset 5 committed,
in-flight 0. Each item handed the same WorkContainer to the controller again and
each pass re-ran onSuccessResult, so the in-flight counter the engine throttles
against was corrupted in both directions.
Tests: ReactorEmptyPublisherTest covers the empty Mono, Mono<Void>, empty Flux,
a single-item control, the multi-item case (asserting in-flight returns to zero,
which is what catches over-completion - the commit alone cannot see it), and an
erroring publisher as a second control, green before and after, holding the fix
to Reactor's guarantee that onError and onComplete are mutually exclusive.
Every assertion is on TWO numbers, because the committed offset alone cannot
tell a stall from a slow run. The user-function invocation count can: when it
pins at exactly maxConcurrency, completed records are not being released. The
suite therefore runs with maxConcurrency deliberately below the record count -
at the module's usual cap of 1000 the bug looks like nothing worse than a
missing commit. ReactorUnitTestBase gained overridable maxConcurrency and commit
mode so this reuses the existing harness rather than forking it.
Red before the fix: 5 of 6 failed, each on timeout with invocations pinned at 4
of 5. Green after: all 6 in 3.1s. Core (386) and reactor (20) unit suites pass.
No other instance of the defect class - a reactive subscribe whose terminal
action is wired to the wrong signal - anywhere in the tree. MutinyProcessor was
already correct, Vert.x uses Future#onSuccess and is structurally immune, and
every other .subscribe( hit is a topic subscription with no completion signal to
mis-wire.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MedgsxqrM8vjSt5ncuAo8g
… value Records the defect class behind the Reactor empty-publisher stall so the next engine adapter does not reinvent it: a record is done when its publisher finishes, whether or not it produced anything, and exactly once however many values it produced. Carries the two things the fix commit cannot: the diagnostic tell (count user-function invocations, not just commits - a missing commit cannot tell a stall from a slow run, an invocation count pinned at maxConcurrency can), and the sweep of every .subscribe( in the tree with the reason each candidate was dismissed, including why Vert.x is structurally immune rather than merely correct today. It also corrects an earlier reading of the multi-item case, which recorded that a publisher emitting n items "committed all five records cleanly" and left over-completion as an untested hypothesis. It does not hold - the before/after measurements are in the table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MedgsxqrM8vjSt5ncuAo8g
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
|
[superseded - a quarantined test changed outcome] 🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its Superseded by a newer quarantine lane report. |
One conflict, in ReactorProcessor#react. Master landed the same fix first as #342 (1cebce8), and then wrapped the terminal callbacks in the MDC scope from #197; that hunk is a strict superset of this branch's, so the merge takes master's subscribe block and keeps this branch's javadoc paragraph on completion semantics. What remains branch-only after the merge is the javadoc, ReactorEmptyPublisherTest with its ReactorUnitTestBase overrides, and the docs/solutions write-up. Reactor unit suite green on the merged tree: 19 tests, 0 failures. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@codex review Branch is freshly merged with master. Master already carries the same one-line fix (#342), so what this PR adds on top is the javadoc paragraph on completion semantics, |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its No quarantined test changed outcome since the previous push. Updated for |
🟢 Throughput — OKThis branch measured about 11% slower than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 10 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: 7a8a7f0, 2e2705a, 28c6be6, c813942, 558fcfb, 5ae7b34, 10ed71c, 75b48bb, e028296, 8df9cc7 Since the previous push: ratio 0.936 -> 0.886, share 1.743 -> 1.772, rate 70990 -> 64254 (-9.5%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #329 +/- ##
============================================
+ Coverage 81.57% 82.02% +0.44%
- Complexity 1462 1470 +8
============================================
Files 95 95
Lines 5118 5118
Branches 498 498
============================================
+ Hits 4175 4198 +23
+ Misses 745 725 -20
+ Partials 198 195 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@claude review this |
|
Claude finished @astubbs's task in 3m 18s —— View job Review complete
Note on method: the sandbox this review runs in blocks What this PR actually changesI confirmed via
FindingsNo correctness, security, or maintainability issues found in this PR's own diff. The counter-increment placement in the test lambdas (inside the publisher-building code, not inside emitted items) correctly measures "user code was invoked" once per dispatched record regardless of how many items the returned publisher emits, which is what the tests need to distinguish a stall from progress. Codecov confirms full coverage of the modified lines and all tests green. One process nitpick, non-blocking: the PR is still marked draft and has no linked issue number in the commit subject — worth flipping to ready-for-review and deciding whether this defect warrants a tracked issue before merge, per the repo's issue-reference convention, though that's a judgment call for the author rather than a defect in the change itself. |
astubbs
changed the title
On the issue number: there is no issue to cite. Flipping draft to ready is the author's call and is left for them. |
Description
The Reactor engine's fix for the empty-publisher stall is already on master. This PR is the test
suite that pins it, the javadoc that promises it, and the write-up that records the defect class.
A user function returning a publisher that emits nothing -
Mono.empty(), aMono<Void>fromMono.fromRunnable(..), aFluxwhose filter removed everything - left its record in flight forever.In-flight filled to
maxConcurrency, back-pressure halted selection, and the consumer stopped with noexception, no failed record and nothing in the log.
ReactorProcessor#reactsubscribed withFlux#subscribe(Consumer, Consumer), which takes onNext and onError, soonComplete(..)was only everreached by an emitted item - and a publisher emitting n items ran the success path n times.
This branch found and fixed that independently. master then landed the same three-argument
subscribeas part of #342, wrapped in the MDC propagation from#205, together with one regression test in
ReactorPCTestfor the emptyMono. After merging master, what this PR delivers on top is:ReactorEmptyPublisherTest- six cases, each asserting two numbers, because the committedoffset alone cannot tell a stall from a slow run: the user-function invocation count pins at exactly
maxConcurrencywhen completed records are not being released. EmptyMono,Mono<Void>from arunnable, empty
Flux, a single-item control, the multi-item case (asserting in-flight returns tozero, which is what catches over-completion - a commit cannot see it), and an erroring publisher as a
second control, holding the fix to Reactor's guarantee that
onErrorandonCompleteare mutuallyexclusive. Both controls were green before the fix, so a change that broke completion outright could
not pass as success. The suite runs with
maxConcurrencydeliberately below the record count; at themodule's usual cap the defect looks like nothing worse than a missing commit.
ReactorUnitTestBasegains overridablegetMaxConcurrency()andgetCommitMode()so the suitereuses the existing harness rather than forking it.
ReactorProcessor#reactjavadoc now states the contract: a record succeeds when its publishercompletes and fails when it errors; the items in between are the user's, and the engine neither counts
nor inspects them.
docs/solutions/logic-errors/reactive-completion-wired-to-onnext-2026-08-22.md- the defectclass (a reactive subscribe whose terminal action is wired to the wrong signal), the invocation-count
tell, and the sweep, so the next engine adapter does not reinvent it.
The measurements the test design rests on
MockConsumer, five records,maxConcurrency4, no broker, before the fix:Mono.empty()Mono.just(1)Uniresolving to null"Invoked 4 of 5" is the stall. Mutiny is the counterexample that made this a Reactor defect rather
than an
ExternalEngineone:MutinyProcessor#onRecordalready subscribed with an explicit completioncallback.
The multi-item case was broken too, and an earlier investigation had recorded the opposite. With
Flux.just(1, 2, 3), fix stashed then restored, everything else identical:Reactor successsignals for 5 recordsnumberRecordsOutForProcessingat restDefect-class sweep - one instance, none others
Every
.subscribe(in the tree, checked for arity and which signal carries the terminal action:MutinyProcessoris three-arg and correct;VertxParallelEoSStreamProcessorusesFuture#onSuccess,which fires once even on a null result, so it is structurally immune; the remaining hits are topic
subscriptions with no completion signal to mis-wire.
Not settled
The path by which pre-fix duplicate completions were partly absorbed - all 15 reach
onComplete,yet in-flight settles at 1 rather than -10. The corrupt end state is asserted against; the absorption
path is not.
Checklist
docs/solutions/write-up, and thereact(..)javadoc now states the completioncontract
docs/features/- no feature; the behaviourchange shipped with perf(core): give external engines the pipeline buffer, and the dispatch ceiling that bounds it #342
ReactorEmptyPublisherTestand theReactorUnitTestBaseoverrides it needsdocs/inflight/working note - N/A - single-branch work with nothingghcannot show; thedurable knowledge went straight to
docs/solutions/ce-simplifyandce-code-reviewlocally - N/A -@codex reviewon the PR found no majorissues on the merged head, and
@claude review thisis requested on the PR instead