Repository navigation
fix(core) astubbs#422: keep an explicit commit interval that equals the default - #427
Conversation
What changed for a user: in PERIODIC_TRANSACTIONAL_PRODUCER mode an interval you set explicitly is now always kept, including commitInterval(ParallelConsumerOptions.DEFAULT_COMMIT_INTERVAL). Before, that value was silently replaced with the 100ms transactional default - fifty times the broker load configured, with no log line - because "unset" was inferred from the value. An unset interval still resolves to 100ms under transactions and to Kafka's five seconds otherwise; nothing else changes. Diagnosis: the field was @Builder.Default = DEFAULT_COMMIT_INTERVAL and transactionsValidation() tested getCommitInterval() == DEFAULT_COMMIT_INTERVAL, so passing the constant was indistinguishable from passing nothing. Found while proving claim C5 of the transactional register (#262); the identity idiom appears nowhere else in any module's src/main. Fix: drop @Builder.Default so the builder leaves the field null, and resolve null in getCommitInterval() from the commit mode. The getter is the one path every reader takes - the engine's getTimeBetweenCommits(), toString(), the C5 proof, and any caller reading options before constructing a processor - so a null can never escape on any call order, which resolving inside validate() alone could not promise. validate() no longer touches the interval; two resolution points would drift. The getter has to stay total, because it replaces one that could not throw. Resolving through isUsingTransactionCommitMode() made getCommitInterval() throw NPE for a commitMode built as an explicit null - a misconfiguration validate() already rejects, but one that reading options was never able to trip. The mode test is therefore constant-first, PERIODIC_TRANSACTIONAL_PRODUCER.equals( commitMode), which returns Kafka's default exactly as the generated accessor did and leaves the rejection where it was. A second defect falls out with it. validate() used to bake the resolved value into the field, so toBuilder() carried that value forward: re-building a validated transactional options object with a non-transactional commit mode kept the 100ms interval, which was never anybody's configuration. Now the field round-trips as null and the copy re-resolves against its own commit mode. Rejected: equals instead of ==, which the retired note docs/inflight/bug-commit-interval-identity-check.md had suggested. A user who writes Duration.ofSeconds(5) gets a fresh object, identity is false and their value is kept today - the right outcome. Under equals every explicit five seconds would be reduced to 100ms, turning a narrow defect into a broad one. That note is deleted here; its content lives in this message, the field javadoc and the tests. Tests: ParallelConsumerOptionsTest gains the explicit half of the truth table - an explicit five seconds, the explicit default constant, and clearing the interval back to unset through the deprecated setter, which is the only path that reaches the null branch after construction. The unset half is already covered by the claim-C5 arms and is deliberately not restated. explicitDefaultConstantInTransactionalModeIsKept was observed red before the fix: "value of: getCommitInterval() / expected: PT5S / but was : PT0.1S", and it was the only arm that failed - which is also the evidence that the note's equals-based diagnosis was inverted, since the explicit-five-seconds arm was green on both sides. Negative control observed on the new code: making getCommitInterval() return DEFAULT_COMMIT_INTERVAL for the unset case regardless of the commit mode - one term, everything else identical - failed exactly the two arms that read an unset interval under transactions, both "expected: PT0.1S but was : PT5S", while the kept-explicit arms and the non-transactional control stayed green. Reverted. The C5 arm's pre-validate() read is kept and inverted rather than deleted: two reads either side of validate() are what make "no call order can expose an unresolved value" an assertion instead of a javadoc claim. The register entry cites these tests, carries the new control, and no longer records the defect as open. Also repairs the dangling javadoc link to AbstractParallelEoSStreamProcessor#KAFKA_DEFAULT_AUTO_COMMIT_FREQUENCY, a constant that no longer exists anywhere in the tree, and gives both DEFAULT_COMMIT_INTERVAL constants javadoc saying when each applies.
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)
|
[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 Updated for Superseded by a newer quarantine lane report. |
|
🟢 Throughput — OKThis branch measured about 3% faster 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: 2e2705a, 28c6be6, c813942, 558fcfb, 5ae7b34, 10ed71c, 75b48bb, e028296, 8df9cc7, 88f5774 Since the previous push: ratio 0.979 -> 1.029, share 1.669 -> 1.596, rate 67153 -> 68288 (+1.7%). 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 #427 +/- ##
============================================
+ Coverage 81.54% 82.08% +0.54%
- Complexity 1461 1471 +10
============================================
Files 95 95
Lines 5103 5118 +15
Branches 498 498
============================================
+ Hits 4161 4201 +40
+ Misses 745 723 -22
+ Partials 197 194 -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:
|
The three transactional cases differed only in the interval and repeated the same builder chain - a producer, the commit mode, build, validate - which is what jscpd counted as a new clone on the PR. validatedTransactionalOptions takes the interval, null meaning unset, which is the distinction the whole truth table is about. No assertion changes: every message and expected duration is the one that was observed red, and the arms still call validate() before reading the getter.
[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 Since the previous push: Updated for Superseded by a newer quarantine lane report. |
|
Predecessor: #425 (closed, superseded by this PR) reached the same diagnosis independently from #424, a duplicate of #422. Its review of the nullable-field shape found the |
…ere docs/solutions/ search will find it The fix's diagnosis - the retired inflight note had the defect's direction backwards, and equals() would have been the regression it feared - lives in the commit body, the field javadoc and the tests. Nothing in docs/solutions/ carried it, so a future session grepping "commitInterval" in the solutions corpus would find nothing and could re-derive the wrong conclusion. Ported from the superseded parallel fix of the same defect, with its "actual fix" section rewritten to this branch's shape: resolution from the commit mode in the getter, validate() no longer writing the field, and the toBuilder() carry-forward that closes as a result. Frontmatter matches its logic-errors neighbours. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
|
@claude review this |
|
@codex review |
|
Claude finished @astubbs's task in 5m 59s —— View job Review:
|
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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 |
|
@claude review this |
|
Claude finished @astubbs's task in 6m 16s —— View job Review:
|
…on that would 404 on master Two review findings from the automated round on #427. The citation. The ported write-up opened with `git show d3d9e7b:docs/inflight/bug-commit-interval-identity-check.md`. That hash is not reachable from master, nor from this branch - it exists only on `fix/803-bound-transactional-revoke-wait`, an unrelated branch carrying a same-content copy of the note under a different hash. A reader on master following it gets `fatal: invalid object name`, and nothing catches that: `bin/check-file-refs.sh` gates the path half of a citation, never the revision half. It came across verbatim from the superseded #425 without being re-pointed at this branch's history. Replaced with `10ed71c9e`, this branch's own base commit - reachable from master, carries the file, carries the `commitInternalHasNotBeenSet` anchor, and already used for this exact citation in `docs/plans/2026-08-07-001-test-transactional-eos-battle-test-plan.md`. The parenthetical now says what the hash is, so the next reader does not mistake it for the authoring commit. Same-defect-class check: every commit hash cited by any file this PR changes was resolved and tested for master-reachability. `d3d9e7bea` was the only unreachable one; `b36ad9428`, `c429d8b6` and the two `10ed71c9e` citations all resolve. The test. The PR claimed a second defect closed - `validate()` baked the resolved interval into the field, so `toBuilder()` carried 100ms into a copy re-built as non-transactional - but no test asserted it; the closure was structural and unwitnessed, in a suite whose whole point is that documented guarantees are machine-checked. `ParallelConsumerOptionsTest#anUnsetIntervalRoundTripsThroughToBuilderInsteadOfBeingBakedInByValidation` asserts the round trip, and also that validating the copy leaves the original alone. Negative control observed, one term changed and everything else identical: restoring the bake-in (`this.commitInterval = getCommitInterval();` at the head of `transactionsValidation()`) failed exactly this one arm, `expected: PT5S but was : PT0.1S` - the second defect's own signature - while the other eight stayed green. Reverted; main code is untouched by this commit. No `@ProvesClaim`: C5 is "transactional mode reduces the default to 100ms", which this does not test. The register's evidence prose stays accurate rather than being churned. Declined from the same round: the observation that `docs/inflight/issue-response-422.md`'s "four-way truth table" names only three test methods. The four are the cross product of explicit/unset against transactional/not, and the two unset arms are named in the following clause as living in `TransactionalBulkCommitTest` (#transactionalModeWithNoExplicitCommitIntervalResolvesTo100ms, #nonTransactionalModeKeepsTheFiveSecondDefault). The prose is correct as written.
Review round addressed -
|
|
To use Codex here, create an environment for this repo. |
Picks up #430, which excludes CVE-2026-82596 on org.latencyutils:LatencyUtils:2.0.3. That advisory is master state, not this branch's - nothing here touches a pom - and it was the sole entry in the `deps: whole-tree CVE scan` table on this PR, so the merge is what clears the check rather than any change to the fix. Master brings one commit touching pom.xml and docs/inflight/deps-cve-backlog.md; this branch touches neither, so there is nothing to reconcile.
Taking master turned `repo: hygiene` red here, and the gate is right rather than confused. `docs/inflight/deps-cve-backlog.md` arrived with #430 recording that CVE-2026-82596 "went red on every open PR the morning it was published (#427, #428, #429)", and `bin/check-branch-self-reference.sh` fires on any note naming the PR whose state is about to change - which, on this branch, is #428. The sentence needs no rewrite: it is past tense about the morning of 2026-09-03 and reads correctly once this merges. What was missing is the attestation the gate exists to force, so it is added on the line rather than above it, which would have split the paragraph. This is the intended use of the marker, not a way round the gate: the gate scopes to whichever branch is merging precisely so the PR being named is the one asked to confirm. #427 and #429 are named in the same sentence and will hit the identical failure when they take master; whoever merges first gives master the marker and the other two inherit it.
…merge-correct Merging master brought in #430's backlog entry, which names the three PRs the advisory went red on - this one among them. `bin/check-branch-self-reference.sh` reads its `#NNN` arm over whole files, not just lines the branch added, so a line master wrote about #427 fails on #427's own branch. That is the gate working as designed: it fires only on the state about to change, and mine is. Marked rather than rewritten, which is the judgement the marker exists to record. The sentence is a dated record in the past tense - "It went red on every open PR the morning it was published" - and it names PRs, which the gate's own header calls permanent and fine to cite forever. What rots is a present-tense claim about a branch, and there is none here. It reads identically after any of the three merge. The attesting comment sits inside the checked block on purpose: it names #427 itself, so outside the block it tripped the same arm on its own first run. #428 and #429 will hit this identically when they merge master. This lands the marker once, on the first of the three, so the other two inherit it rather than each attesting the same line.
…merge-correct What changed for a user: nothing - a one-line marker in an inflight note. Merging master for #430's CVE exclusion brought in a sentence in docs/inflight/deps-cve-backlog.md naming the three PRs the advisory went red on, this one among them. check-branch-self-reference.sh reads whole files for the PR-number arm, so an inherited line naming #429 fails on this branch even though #430 wrote it - the gate only ever looks at the branch about to change state, and after this merge that is this one. The sentence is already written in post-merge terms: it is past tense about the morning of 2026-09-03, and it stays true once this PR lands. That is precisely the case the gate's header calls permanently fine - "PRs are permanent, and citing one is fine forever"; what rots is a deleted branch or a present-tense claim, and this is neither. So the attestation is the correct response rather than a rewrite, and it is added inline so the paragraph is not split. #427 and #428 inherit the same line and will hit the same gate on their own merge of master. The marker is not branch-specific, so whichever lands first carries it for the others.
…ng branches 34dde7f attested the LatencyUtils entry with the block form (`checked-begin`/`checked-end`) plus a comment explaining the judgement. #428 and #429 had already reached the same conclusion about the same inherited line and used the inline form - the marker appended to the end of line 199 - so three branches were about to land three different renderings of one attestation and conflict with each other over it. The gate accepts both, so this is not a correctness fix; it is choosing the form that merges. The file is now byte-identical to the copy on both sibling branches, verified by diffing against `origin/fix/168-commit-error-line-keeps-identifiers`, so whichever of the three lands first the other two see no conflict here at all. The reasoning the block comment carried is not lost - it is in 34dde7f's body, which is where prose reasoning belongs and which the release-note generator reads. In short: the sentence is a dated past-tense record naming PRs, the gate's own header calls citing a PR permanent and fine, and what rots is a present-tense claim about a branch, of which there is none here. Marked rather than rewritten, deliberately.
Check state after the master merge - and the one red that is not this PR's
Refuted along the way, reported because it looked decisive: I first took So this reads as a measurement artefact in Everything else is green - 30 checks pass, including Chaos Pain Suite, Integration Tests, Lincheck, |
…rval-unset-detection
…rval-unset-detection # Conflicts: # docs/inflight/pr-blockers-and-collisions.md
Closes #422
Description
In transactional commit mode,
commitInterval(ParallelConsumerOptions.DEFAULT_COMMIT_INTERVAL)wastreated as "not set" and silently replaced with 100ms - fifty times the broker load configured, with
no log line - because
transactionsValidation()inferred "unset" by reference identity against the@Builder.Defaultconstant. This PR stops inferring it from the value: the builder leaves the fieldnull, and
getCommitInterval()resolves null from the commit mode (100ms underPERIODIC_TRANSACTIONAL_PRODUCER, Kafka's five seconds otherwise). Any value a user sets is kept,whatever it equals.
validate()no longer touches the interval.Why not
equals- the retired in-flight note had suggested it. An explicitDuration.ofSeconds(5)is a fresh object, so identity is already false and the value is already kept today; under
equalsevery explicit five seconds would be reduced to 100ms, turning a narrow defect into a broad one.
ParallelConsumerOptionsTest#explicitFiveSecondsInTransactionalModeIsKeptguards against it, and itwas green both before and after the fix - which is the evidence the note's diagnosis was inverted.
Why the getter, not
validate()- no main-code reader runs beforevalidate()(the only externalreader is
AbstractParallelEoSStreamProcessor#getTimeBetweenCommits, after construction), but thegetter is public API on an
@InterfaceStability.Evolvingclass and the existing claim-C5 proof readsit before
validate(). Resolving in the getter is the one shape in which a null can never escape onany call order, and it keeps a single resolution point - two would drift. The visible consequence: an
options object built for transactional mode with no interval set now reports 100ms straight away,
where it used to report five seconds until a processor was constructed. What the running processor
commits is unchanged; only the pre-
validate()read moves, to the value it was always going to use.The getter also has to stay total, since it replaces one that could not throw.
commitModebuilt asan explicit null is a misconfiguration
validate()rejects, so the mode test is written constant-first(
PERIODIC_TRANSACTIONAL_PRODUCER.equals(commitMode)) rather than throughisUsingTransactionCommitMode(): reading options has never been able to fail, and this change mustnot be the one that starts it.
ParallelConsumerOptionsTest#aNullCommitModeIsRejectedByValidationRatherThanByTheGetterpins both halves.
A second defect falls out with it.
validate()used to bake the resolved value into the field, sotoBuilder()carried it forward: re-building a validated transactional options object with anon-transactional commit mode kept the 100ms interval, which was never anybody's configuration. The
field now round-trips as null and the copy re-resolves against its own commit mode.
ParallelConsumerOptionsTest#anUnsetIntervalRoundTripsThroughToBuilderInsteadOfBeingBakedInByValidationpins it, so the second defect's closure is asserted rather than merely structural; its negative control -
restoring the bake-in at the head of
transactionsValidation(), one term - failed exactly that arm,expected: PT5S but was : PT0.1S, while the other eight stayed green.Evidence.
ParallelConsumerOptionsTest#explicitDefaultConstantInTransactionalModeIsKeptwasobserved red before the fix (
expected: PT5S but was : PT0.1S) and was the only one of the new armsto fail. Negative control observed on the new code: making
getCommitInterval()returnDEFAULT_COMMIT_INTERVALfor the unset case regardless of the commit mode - one term, everythingelse identical - failed exactly the two arms that read an unset interval under transactions, both
expected: PT0.1S but was : PT5S, while the kept-explicit arms and the non-transactional controlstayed green. Reverted.
Same-defect-class check:
grep -rnE '== *DEFAULT_|DEFAULT_[A-Z_]+ *==' --include='*.java' */src/mainover every module returned exactly one hit before this change - the line removed here - and nothing
after it. No other option in any module is compared to its own default by identity. Also checked and
dismissed: every other
@Builder.Defaultfield onParallelConsumerOptions, none of which has a"did the user set this?" gate at all, and the deprecated
setCommitInterval, which writes the fielddirectly and so produces an explicit value by construction.
Prior art:
bin/inflight.mjs prior-art commitInterval DEFAULT_COMMIT_INTERVAL transactionsValidationreturned the battle-test plan
docs/plans/2026-08-07-001-test-transactional-eos-battle-test-plan.md(which lists this defect as open), the in-flight note retired here, and the claim-C5 commits; merged
PRs touching
ParallelConsumerOptionsreturned only the package rename, so nothing has changedtransactionsValidationsince the identity check was written;gh issue list --state allreturnedonly #422; no open PR touches
ParallelConsumerOptions#commitInterval.Record kept in touch:
docs/inflight/bug-commit-interval-identity-check.mdis deleted, with itscontent migrated to the commit body, the field javadoc and the tests first; the
pr-blockers-and-collisions.md"decisions waiting on a human" bullet and the README "known gap"paragraph are retired; the register's C5 entry cites the new tests, carries a fresh negative control
observed against the new code, and no longer records the defect as open; the dated battle-test plan
keeps its claim and gets a history pointer to the retired note.
docs/inflight/issue-response-422.mdis the drafted issue reply the in-flight rules require - not posted, and it deliberately outlives this
PR. Also repairs a dangling javadoc link:
AbstractParallelEoSStreamProcessor#KAFKA_DEFAULT_AUTO_COMMIT_FREQUENCYno longer exists anywhere in the tree. The durable write-up,
docs/solutions/logic-errors/commit-interval-identity-check-was-narrower-and-inverted-2026-09-02.md, was ported from the superseded #425 so the inverted-diagnosis finding survives indocs/solutions/search.Checklist
src/docs/README_TEMPLATE.adocwithREADME.adocregenerated by the whelk plugin (never hand-edited),docs/features/commit-interval.yamldocs/features/- the existingcommit-interval.yamlboundary was corrected; this is a defect fix, not a new featureParallelConsumerOptionsTest(one observed red before the fix) including the deprecated setter's clear-back-to-unset path, the C5 arm's pre-validate read inverted rather than deleted, and the register entrydocs/inflight/working note (pr-/branch-) started at the PR's first commit - N/A - nothing here thatghcannot show; the PR retires abug-note and adds theissue-response-422.mddraft the rules requirece-simplifyandce-code-reviewlocally - simplify found that two of the four originally drafted test arms duplicated existing claim-C5 arms inTransactionalBulkCommitTestexactly, so both were dropped and the register citation trimmed to match. Review found two real test gaps and both were closed: the deprecated setter's clear-back-to-unset path had no coverage at all, and the C5 arm's deleted pre-validate read left the "no call order can expose an unresolved value" claim unasserted, so it was inverted and restored rather than dropped. The negative control was then re-observed against the final test set. Correctness and project-standards passes returned nothing; the review ran locally rather than through a cross-model peer.