Repository navigation
test(chaos) confluentinc#857: test-log silos, a scenario-wide recovery diagnostic, and unattended experiment runners - #381
Merged
Conversation
…d the config name collision Extracted from #29, where this was built to make an overnight soak readable. It has no dependency on that PR's fix, so it lands ahead of it. The core unit profile gains seven sifted streams under ${pc.log.dir:-target/pc-logs}: errors, warnings, probes, harness, and PC's commit, lifecycle and state channels. They are routed as COPIES - default additivity is left alone - so console output is unchanged and nothing that reads it breaks. No appender declares a level of its own, so -Dpc.log.level still governs the whole harness exactly as before. The integration profile is renamed to logback-integration-test.xml. Both files used to copy to target/test-classes/logback-test.xml, so which one configured a run depended on classpath order: a silent coin flip, and the sort of thing that makes an instrumented result untrustworthy without announcing itself. They now have distinct names and the integration profile is selected explicitly with -Dlogback.configurationFile. docs/logging.md owns the why, including how to prove which profile actually loaded. That is the check worth having: surefire:test does not reprocess test resources, so an edited config can sit in src/ never reaching target/test-classes, and a run against the old one reads exactly like a change that had no effect.
…very scenario, and correct what it claims Extracted from #29. -Dchaos.diagnoseStallRecovery=true answers the one question a gating run structurally cannot: when a probe violation fires, does whatever it caught ever recover, or is it wedged forever? A gating run destroys that evidence at the moment of detection, because failFast aborts the wait on the first violation - so "unbounded" and "merely slow" are indistinguishable from the data it leaves behind. The capability lived on AbstractRevokeUnderWorkScenario. It moves to ChaosScenarioBase so that any scenario can be asked the same question, rather than ChaosChurnStormIT growing a drifted copy of it. It cannot turn a red run green: assertScenarioSlos still asserts the probe's violations are empty whichever way the wait ended, and with the property absent the gating configuration is byte-for-byte unchanged. It also corrects a claim that had gone stale. ChaosChurnStormIT's javadoc and its runtime warning both told the reader the diagnostic had never engaged on this scenario, and that any result was therefore a NEW finding. It has since engaged: the backlog DRAINED on all six firings collected on 2026-08-28, which is what demoted the asynchronous stall to a timing proxy rather than a distinct defect. Both now say so - the message existed to stop an operator re-deriving a known result, and had come to cause exactly that. Both now cite the class's own "Calibration status" javadoc instead of a docs/inflight/ note. Those notes are deleted when their work lands, so shipped code citing one is a citation with an expiry date, and nothing goes red when it expires.
Extracted from #29. Each drives a test that already exists on master, so they run here unchanged. They exist because the answers they produce cost CPU rather than attention: each one runs unattended, tallies its own outcome, and prints a verdict a reader can act on without reconstructing the run. hunt-async-stall-answer.sh runs ChaosChurnStormIT until the diagnostic FIRES, not merely until it engages - the distinction that made earlier hunts return nothing confirm-async-drain.sh collects further firings to test whether the drain repeats, rather than hunting fresh seeds audit-stall-detector-silence.sh asks whether every failure had a detector behind it measure-large-instances-failure-rate.sh measures a rate largeNumberOfInstances has never had sweep-large-instances-scale.sh sweeps -Dperf.scale to ask whether a residual moves with capacity batch-857-experiments.sh drives the outstanding set as one unattended run One runner stays behind on #29: soak-deadlock-probe.sh drives Rebalance857CommitSyncDeadlockProbeIT, which is that PR's own instrument and does not exist on master.
Dependency ReviewThe following issues were found:
License Issues.github/workflows/experiments.yml
OpenSSF Scorecard
Scanned Files
|
Merged
4 of 6 tasks
✅ 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 |
🧪🔒 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 |
astubbs
added a commit
that referenced
this pull request
Aug 31, 2026
…its dependencies leave `feats/overnight-torture-harness` is merged into this branch rather than given its own PR. It contained this branch in its entirety, so a PR from it would have shown the whole 857 diff to review a two-file addition - a rung that had not earned one. It is now a pointer, deletable once this lands. The test infrastructure it actually depends on - the seven-stream log silos and the scenario-wide stall-recovery diagnostic - left in the other direction, to #381, cut fresh from master. The harness needed observability, not a deadlock fix, and stacking it here was chronology rather than dependency. The handoff's "Where things sit" section said neither of those things, so it now does.
astubbs
added a commit
that referenced
this pull request
Aug 31, 2026
… read after this merges The previous commit described the branch layout in the present tense, on a note that lives in docs/inflight/ - so every sentence in it was a claim this merge falsifies, and check-branch-self-reference.sh failed it for exactly that. Rewritten in post-merge terms and marked checked. It also records what the previous version did not: the harness's real dependency was never the deadlock fix. It drives only chaos scenarios that predate the 857 work and needs `pc.log.dir` from the log silos, both of which are now in #381. What tied it here was purpose, not code.
|
astubbs
changed the title
astubbs
added a commit
that referenced
this pull request
Aug 31, 2026
Second merge of `feats/overnight-torture-harness`, picking up the review pass on the harness, the overnight soak's sightings, and a correction to docs/investigating.md. WHAT ARRIVES The harness had five paths to a confident wrong verdict and one to no verdict at all, all now fixed and covered by bin/test-torture-overnight.sh - its self-test passes here. Two of them close items this branch had recorded as open: `loss=` counted matching log LINES, so "LEDGER_LOSS: 37 produced records never consumed" reported one rather than thirty-seven, and the drain verdict could read a wedge as a recovery in two separate ways. The jstack fix is the one that matters most for an unattended night: it had no deadline, inside a watchdog whose whole subject is processes that will not respond, so a single unresponsive JVM could consume the rest of the run and yield nothing. The soak also produced something no previous sighting could - a RATE, with a control arm nobody had to build: NO_PROGRESS killed roughly one ChaosChurnStormIT cycle in twenty on an idle desktop while the other four chaos scenarios ran the night without a failure. Cycle 111 is flagged as the best lead in the file, on the GATING liveness claim rather than a demoted one. Cycle 16 independently confirms what this branch reopened yesterday: a genuine detector-quiet failure, swept for any detector-shaped token rather than for NO_PROGRESS alone. The detector-silence task on this branch was reopened on exactly that case, from the torture run, and this is the same finding checked by someone else's method. THE ONE CONFLICT, AND WHY NEITHER SIDE WON `handoff-overnight-torture-harness.md` was renamed to `branch-` on that branch - correctly, since the directory's table gives `branch-` to work sitting on a branch with no PR and `handoff-` names a status rather than an area. This branch had meanwhile rewritten its "Where things sit" section, so git reported modify/delete. Resolved by carrying this side's content into their renamed file rather than taking either whole. Their explanation of the dependency is better than the one it replaces and is kept: cut from master the harness still RUNS, which is the trap, because `-Dpc.log.dir` is accepted and ignored there and the morning tarball is empty rather than absent. What this side adds is that the dependency has since moved - the log silos and the diagnostic lift were extracted to #381 and cut from master, so the harness's real dependency is that work and not the deadlock fix. VERIFIED bin/check-all.sh green, test-compile clean, bin/test-torture-overnight.sh green, and no reference anywhere in the tree to the pre-rename filename. The two auto-merged files were checked for losses rather than assumed: both gained content, and this branch's concurrency label on the churn-storm note and its "Designing a liveness check" section in docs/investigating.md both survive alongside the arriving correction.
astubbs
added a commit
that referenced
this pull request
Aug 31, 2026
…f failure it is The two red CI lanes on this branch had no diagnosis for weeks, because gh run view --log-failed returns a truncated log for those jobs - it ends mid-run with no error line and no failsafe summary, which reads exactly like a job that failed for no reason. Fetched through the run-logs archive instead, the failures are TransactionAndCommitModeTest .testTransactionalDefaultMaxPoll and MultiInstanceHighVolumeTest.multiInstance, both on "all keys processed within time". That message is the problem this commit is about. It is the same sentence whether the run was crawling or wedged, and those have opposite causes - contention on the machine versus a defect in the code under test. This repo's rule is to establish which before touching a test that fails under load, and the message gave a reader nothing to establish it with. MAIN CODE: describeProgress() PC now describes its own state in one line, and reports BOTH ENDS deliberately. workRemaining() alone cannot tell "nothing is finishing" from "nothing is happening" - an instance inside a slow user function reads as a flat line while entirely busy, and a wedged one reads identically. Outstanding work paired with records-out-for-processing separates them: work outstanding AND records out means occupied; work outstanding and nothing out means not trying. Public deliberately, because the callers who need it are outside the package - integration tests and applications diagnosing a stall - and a diagnostic that needs reflection to reach does not get used at the moment it is needed. The javadoc says plainly that the out-for-processing figure is read without a fence and is a diagnostic estimate never to branch on, citing the note that owns that defect. TEST CODE: ProgressTracker classifies rather than reports Extended the existing shared helper rather than adding a parallel one. A timeout now carries a verdict: STILL ADVANCING at the deadline (too slow, not stopped) or FLAT for N seconds (it stopped rather than ran out of time), with elapsed time and the consumer's own describeProgress() line beside it. The discriminator is progress since progress was last SEEN, not progress overall - a run that races to a high count and then freezes has still made a lot of progress, and judging on the total reads that as healthy. That is the same inversion the torture harness's drain verdict had to be fixed for, arrived at independently here. withDiagnostic() is opt-in and the verdict says so when it is absent, rather than silently omitting the half that only the instance can see. THE HYPOTHESIS THIS SERVES, WRITTEN BEFORE THE EXPERIMENT docs/inflight/test-857-branch-red-lanes-cause-unestablished.md. Ambient CI load is ruled out - both lanes are green on #381 and #352 on the same infrastructure - and the tryLock fix cannot cause the transactional failure, since the AB-BA cycle cannot close in that mode. The candidate is cluster 2, ThreadConfinedConsumer and ConsumerOwnership, which wraps the consumer in every mode and is the part of this branch with no dedicated evidence. Against it stands a clean ambient probe and a documented flake family this test belongs to. The note states the prediction and, more importantly, the void condition: if neither arm reproduces locally the experiment says nothing, and a green pair must not be reported as "the branch is fine". Recorded before running so the result cannot be fitted to it.
…ster-based baseline while doing it The required Performance Tests check gates on wall-clock deadlines, so it cannot say how MUCH slower a tree got - only "slower than the bound, on this runner, today". Red-timing-lane and busy-runner are the same signal, which is how a several-fold transactional regression sat in that check for weeks reading as flakiness. The lane detected it; the lane's OUTPUT could not express it. ThroughputReport emits one PC-THROUGHPUT line per performance run, on passing runs as well as failing ones, at warn so the test log profiles cannot filter it, with key=value fields so a collector need not parse prose. bin/performance-test.sh greps them into a summary, prints it at the end of the lane, and says so loudly when it finds none - "no test emitted" and "no regression" otherwise look identical. Reporting on GREEN runs is the half that is easy to drop and should not be: a number recorded only when something fails cannot establish what normal looks like, and a future recalibration has nothing to read. WHY IT LANDS HERE RATHER THAN ON THE 857 BRANCH Two reasons, and the second is the operator's and is the better one. It belongs here by theme: this PR is the test-observability extraction, and a throughput reporter is observability. Putting it on #29 was inconsistent - the rest of the set was already extracted here. And it produces a BASELINE, which is the thing the measurement has been missing. The performance lane does not run on master, so a rate measured on #29 has had nothing to be compared against; the regression there had to be established by hand, on a laptop, against a purpose-built control worktree. This branch is cut from master and carries no engine change, so its own lane run is a master-tree measurement - the baseline arrives with the instrument rather than after it, and every later run on a dependent branch has something to be read against.
…d, so a degrading flake is a number
This suite has a long-running flake family whose members rotate between runs under parallel load,
and every member fails the same way: it times out. A pass/fail result cannot tell a test that
finished comfortably from one that scraped in with a fraction of a second to spare - those are the
same green. So "the flake got worse" has only ever been an impression, argued from how often
people saw red, and the argument restarts whenever the runner or the fork count changes.
A test that normally uses a third of its deadline and starts using nearly all of it is degrading
BEFORE it flakes. That is now visible one run at a time:
PC-DEADLINE-HEADROOM test=pausedConsumerStillLongPollsForNothing() elapsedMs=1541
deadlineMs=20000 deadlineUsedPercent=7 outcome=PASSED
Emitted by the ambient probe, which already attaches to every broker integration test - so one
change instruments the whole family rather than six bespoke wirings, and the six named members
wait in six different shapes that would each have needed their own.
On passes as well as failures, at warn so the profiles cannot filter it, key=value so a collector
need not parse prose. It gates nothing: the healthy fraction differs per test, nobody has measured
the spread, and a bound guessed before the spread is known is the mistake this repo has already
written up under timing bounds used as correctness gates. Collect first.
It also gives the quarantine discipline something to weigh. An entry backed by a headroom trend is
evidence; a sighting count is a story about attention.
FOUND BY RUNNING IT, WHICH IS THE POINT
The first version emitted nothing at all. It resolved the deadline from the test CLASS only, and
the test tried declares @timeout on the METHOD - which overrides the class-level one and is
invisible to a class lookup. It returned quietly, indistinguishable from a healthy silence.
That gap is one this branch's own ChaosScenarioBase javadoc already names, in as many words, and I
walked into it anyway. The resolver now goes method-then-class, the order JUnit itself uses, and
the comment says why so the next reader does not narrow it back.
Silent by design in one case only: a test with no @timeout anywhere has no ceiling, so there is no
headroom to express, and inventing a denominator would be worse than saying nothing.
astubbs
added a commit
that referenced
this pull request
Aug 31, 2026
…and migrate the #119 answer somewhere it survives Two jobs, and the second is the one that would have cost something. AUDITED: the "Still open" list was written 2026-08-19 and three of its items have since been done * The review deferral's PRECONDITION IS MET. It was deferred until #204, #31, #57 and #267 landed - all four have, master has been merged in, and the deadlock fix was re-verified afterwards on a four-cell control across both assignors. Review is now the right next step rather than a wasted cycle. Two NEW parents have appeared since that order was written - #381 and #393 - and the note says explicitly that neither is a reason to defer again, because neither changes this tree. * The roadmap edit is done, and the constraint the item stated is respected: it was not to be edited early in a way that asserts something not yet true, and the new wording says the fix is measured and unmerged, which is what is true now. * The merge strategy is recommended and the squash message written, in the scratchpad rather than the PR body. MIGRATED: the #119 answer now lives in the family ledger That answer existed only in this PR's own note, which is DELETED when the PR lands - so the established answer to "does #119 close with this?" would have gone with it, and the next person would have re-derived it from the same sightings. It is now in bug-857-family.md, which is on master and is the durable owner of this family's accounting. The answer, briefly: no, because #119 is a symptom bucket rather than a defect. The AB-BA cycle the fix breaks cannot close in PERIODIC_TRANSACTIONAL_PRODUCER at all - its second edge lives in a committer built only for the consumer-commit modes - which leaves the unbounded transactional revoke wait, #44, the only issue upstream ever labelled a verified bug, untouched. Closing #119 here would retire the remainder silently, which is the specific damage the merge checklist names. One mechanical note for whoever edits that ledger next: it carries a file-wide post-merge checked-begin/checked-end block, so a new section inside it must NOT bring its own pair. A nested pair breaks the outer one and the gate then flags lines far above the edit, which is a confusing way to find out.
astubbs
added a commit
that referenced
this pull request
Aug 31, 2026
…efect, and the extraction already ruled one suspect out This note predicted that the Performance Tests failure shared a cause with the Integration Tests one - the stale pause cache - on the reasoning that the mechanism is commit-mode-independent. The lane has now run with that fix in place and still fails, so the prediction is REFUTED. Recorded as a refuted prediction rather than replaced by the new number, because a prediction that did not hold is worth as much as one that did and vanishes if only the outcome is written down. The measurement, same test and identical configuration: #381 processed=3000000/3000000 elapsedMs=42024 71387 rec/s PASS #29 processed=2191095/3000000 elapsedMs=60554 36184 rec/s FAILED #381 is test-scope and cut from master, which makes it the closest thing to a master baseline the lane can produce - it does not run on master. Getting that baseline by riding the reporter through on a PR the failing one already depends on was the point of the extraction, and it arrived in time to stop a silent halving from merging. Two samples on one side against one on the other, all on hosted runners: enough to attribute, not enough to treat the ratio as precise. AND ONE SUSPECT ELIMINATED FOR NOTHING #393's performance lane passes, and its thread-confinement refactor is also in the failing tree. So the consumer-ownership work is not the cause, established without running an experiment. The general form is worth more than the result: a branch cut out of a suspect tree is a control arm for whatever stayed behind. Check whether the decomposition already gave you one before designing a bisect. The cheapest untried ablation is named - the revoke fork's two log lines were raised to INFO, and per-loop INFO logging in a three-million-record run is the right shape to cost this much throughput. The coherence check and the pause-observation fields are on the same list. One term at a time, same lane, control arm. Numbers here are anchored to the runs that produced them and written in the past tense, not as standing properties, so they read as evidence rather than as current state that nothing will update.
… runners, and the count that silently broke their tally
Simplify pass over this PR's own work - the six unattended runners landed with five copies of the
maven invocation, four of the failsafe-report classifier and three of the violation-trajectory
parsing. bin/lib/quarantine-common.sh exists because that same shape had already been paid for once
("previously duplicated ~8x across scripts + workflows with drift; Source this, do not copy from
it"), and the drift had already started here: confirm-async-drain.sh carries a fixed
trajectory-parsing bug that its two sibling copies never received.
bin/lib/chaos-experiment-common.sh is now the single home for the maven command line, the JDK pin,
the failsafe outcome classifier, the detector-verdict classifier and the trajectory parsing.
THE COUNT WAS WRONG, AND ONLY IN THE CASE THE AUDIT EXISTS TO FIND
`grep -c` PRINTS 0 and EXITS 1 when nothing matches, so `$(grep -c ... || echo 0)` captures two
lines - "0\n0". Reproduced against the scripts as they stood: the run row then splits across three
lines of the tally and bash prints `[: 0 0: integer expression expected`, and both happen only when
NO detector matched - the LEDGER-ONLY miss case, the whole question the audit was written to answer.
Five call sites had it, across audit-stall-detector-silence.sh and batch-857-experiments.sh.
pc_count_matches replaces them and is the only counting route in the set.
Verified by controlled comparison rather than by reading: a stubbed mvnw and a canned failsafe
report drove each runner at HEAD and after the change, across four scenarios (failed-with-detector,
failed-with-no-detector, passed, executed-nothing), diffing the tallies.
measure-large-instances-failure-rate.sh, sweep-large-instances-scale.sh and
hunt-async-stall-answer.sh came out byte-identical in every scenario. The two deliberate differences
are both in audit-stall-detector-silence.sh and both are the fix landing:
- failed-with-no-detector: HEAD emits a three-line broken row plus a bash error; now one row.
- executed-nothing: HEAD said "PASSED - not a data point" for a run that never ran a test. It now
says DID-NOT-RUN, which is the discipline that script's own header states and its four siblings
already applied. Its "passed" also lowercases, to the one vocabulary the shared classifier uses.
TWO SMALLER THINGS THE SHARED HOME MADE CHEAP
The personal JDK path stopped being a silent fallback. All six carried
`${JAVA_HOME:-/Users/astubbs/.sdkman/...}`, which off that machine is worse than doing nothing: it
suppresses the wrapper's own detection and fails minutes later inside maven, in a message about
something else. It now honours JAVA_HOME, falls back to the SDKMAN candidate only when the binary is
actually there, and SAYS so when it is not. Same for the two scripts that hardcoded the worktree root
to an absolute home directory - derived from git now.
SpotBugs and forbiddenapis are skipped alongside the copyright and jacoco skips that were already
there. Both bind to `verify` in the root pom, so each of the tens of iterations these loops perform
was re-running a full-effort analysis over main AND test classes, with three plugin packs, against a
source tree that had not changed - only a system property had. No runner reads maven's exit code,
so suppressing a verify-phase gate cannot change what any of them reports.
Verification: bin/check-all.sh 15/15; shellcheck clean at the gate's severity floor; the equivalence
harness above.
… wait Simplify pass over this PR's own work. MultiInstanceHighVolumeTest called ThroughputReport twice with near-identical five-argument calls, and the two had already diverged: the timeout path named `outcome=FAILED` and the completion path carried no `outcome` field at all. A collector could only tell a green run from a schema change by the ABSENCE of a key, which is the reading a key=value line exists to remove - and reporting on green runs is the half this instrument exists for, so it is the half that must be unambiguous. One private reportThroughput(expected, waitStarted, outcome) now serves both, so the field sets cannot drift again, and a green run says `outcome=PASSED` the way the ambient probe's sibling PC-DEADLINE-HEADROOM line already does. The PC-THROUGHPUT marker itself is untouched - bin/performance-test.sh greps for it and outside collectors read these lines, so the token and the key=value shape stay exactly as they were. The only change to what is emitted is one added field on the passing line. Also moves the ThroughputReport import into the bz.stub group it belongs to; it had been inserted into the middle of the java.time block.
…header
Simplify pass over this PR's own work.
The seven new sifted appenders carried a copy of their encoder pattern each - two distinct layouts
spread over seven literals. That is how one silo quietly ends up formatted differently from its
siblings and stops answering the same grep as the rest. They are now SILO_PATTERN and
SILO_PATTERN_WITH_STACK, defined once beside pc.log.dir, with the reason the two differ (a wider
logger field and %ex, because an error line without its exception is not an error report) stated
where the difference lives rather than being inferred from two strings.
logback-integration-test.xml carried its copyright notice twice - the documentation block and then
a second bare comment holding nothing else. One header, as everywhere else in the tree.
Verified rather than assumed, which is the check docs/logging.md itself asks for: a real unit run
under -Dlogback.statusListenerClass confirmed logback-test.xml loaded, created all seven silo files,
and rendered both layouts - %logger{24} in the concern streams and %logger{36} in warnings.log -
with no literal ${SILO_PATTERN} anywhere in the output. bin/check-test-log-config.sh still passes.
…g one path across forks
TWO DEFECTS, ONE FILE, THE SECOND FOUND WHILE PROVING THE FIRST
The reported one: the seven FileAppenders in logback-test.xml are plain, non-prudent appenders on
fixed paths, and the ci profile runs surefire at forkCount=1C with reuseForks. One JVM per core
therefore held a handle on each of the same seven files. A non-prudent FileAppender is not safe for
that, and the two streams promising EVERY error and EVERY warning for the whole run are precisely
the ones that lose writes.
Fixed by giving each fork its own directory from the surefire argLine, not by <prudent>: prudent
buys one shared path at the cost of file-locked unbuffered writes on all seven appenders on every
line. Read across them with `cat target/pc-logs/fork-*/errors.log`.
Failsafe deliberately does NOT get this. It declares no forkCount, so it runs at the default of one,
and the bin/ runners read a flat <their -Dpc.log.dir>/probes.log that a subdirectory would break.
THE ONE FOUND ON THE WAY
The fix did nothing, and that was the discovery. The property reached the fork - the debug log shows
-Dpc.log.dir=target/pc-logs/fork-1 on the command line - and the files still landed flat.
The config declared:
<property name="pc.log.dir" value="${pc.log.dir:-target/pc-logs}"/>
a definition that refers to itself. Logback resolves the self-reference to the default rather than to
the system property, so -Dpc.log.dir has silently done nothing since the silos were added, and the
four bin/ runners passing a per-iteration directory were all appending into one shared set of files.
Confirmed with a control arm rather than by reasoning: property on the command line, files flat;
rename the local to siloDir, same command, files in fork-1. One term changed, outcome flipped.
The failure mode is the one this repo keeps paying for - an instrument that is not wired up reports
nothing and looks like a clean result. Nothing goes red if the two names converge again, so
docs/logging.md now says to keep them different and why.
…'s failsafe report Every runner classifies an iteration by grepping failsafe-reports/TEST-*.xml after the maven run, and nothing removed those reports first. An iteration that died before failsafe - a compile break, a bad -Dit.test, a dependency resolution failure - was classified from the file iteration i-1 left behind. That makes the DID-NOT-RUN verdict unreachable in exactly the case it exists for. The stale report still says tests="1", so a broken tree reports the last good run indefinitely, and it fails toward "everything is fine" - the direction that hides a problem rather than raising one. These scripts advertise the discipline it defeats: a run that did not execute the test is NOT a data point. One rm -f before each run, in the shared runners so no caller can forget it. bin/chaos-test.sh met the neighbouring version of this - every rep writes the same TEST-<class>.xml - and solved it by archiving; here the previous iteration's file has already been read by the time the next one starts, so deleting is enough. Also corrected: the audit script's header listed an `unclassified` verdict the classifier has never emitted, and named `ledger-only` where it emits LEDGER-ONLY-MISS-CASE. A phantom bucket is worse than a missing row - a reader who never sees a run land in it concludes the corpus was clean rather than that the bucket does not exist. The header now says there are exactly three verdicts, why there is no fourth, and that nothing checks it against the classifier.
…ed-looking doc is hiding WHAT IS RECORDED AT THE CODE, BECAUSE THAT IS WHERE IT WILL BE REDISCOVERED logback-integration-test.xml claims levels are property-driven; the root logger and the bz.stub.parallelconsumer pin are bare literals, so -Dpc.log.level does not reach PC's own loggers in that profile. The comment says why the obvious one-line repair fails: making root property-driven alone achieves nothing, because the package pin overrides root for every PC logger and only third-party noise would move. The two pins have to be decided together, and the file's header records that hardcoded levels here once produced a 126MB job log. Nothing goes red either way - the level gate scopes itself to the library modules' unit profiles. ChaosScenarioBase's @timeout resolver is class-only where AmbientProbeExtension's is method-then-class. That difference is a decision, not a drifted copy - no chaos scenario declares a method-level @timeout - and it is now recorded on the narrower one so nobody unifies them without seeing what it costs, including that Optional.or is Java 9+ and this module compiles to Java 8. A FLAKE THAT IS ALREADY WRITTEN UP AS FIXED Running the core unit suite surfaced two DIFFERENT failures under one test name, and they are kept apart in the ledger because conflating them would waste the next person's time: parameter [1] timing out on the ordered-offsets assertion, reproducible 1 run in 3; and parameter [3] failing its own input-data sanity check with an empty latch list, seen once in a 533-test run and not reproduced in five attempts since. The [1] failure is symptom-for-symptom the one in solutions/test-flakiness/assert-the-commit-frontier-not-the-tick-path.md, whose status says the fix was UNMERGED. It merged the same day the doc was written, and the frontier assertion it introduced is in the tree - but the failing path still takes the old ordered-list description, which KafkaTestUtils still offers as a default from two call sites. So this is a stale resolution rather than a new flake, and that is the more useful reading: a document saying "fixed" is the reason nobody reopened it. Recorded, not diagnosed - a sighting is written down before the branch that saw it merges, because the evidence expires with the logs.
5 tasks done
…ses solved it separately
THE DRIFT, FOUND BY ASKING WHETHER THESE SCRIPTS ARE A SECOND TEST SYSTEM
They are not - the tests are Java and both harnesses drive the same suite. But there were two
harnesses, and they had independently met the same hazard and answered it differently.
With more than one repetition, every rep writes the SAME TEST-<class>.xml filenames, so anything
reading them afterwards describes only the last one. bin/chaos-test.sh archives each finished rep;
the experiment runners delete before each run. Neither knew about the other. A third caller would
have met it a third time, and the failure is silent in both directions - archive with the permissive
glob and every earlier rep is re-archived and renumbered under the newest; summarise with the strict
glob and every archived rep vanishes from the count.
BOTH POLICIES ARE CORRECT, SO NEITHER IS REMOVED
The hazard is now stated once, in bin/lib/chaos-reports-common.sh, with both answers beside each
other so the next caller CHOOSES rather than rediscovers:
chaos_archive_rep KEEP - the CI gate's reports are still going to be read, by its summary and
by the workflow's artifact upload, so a rep discarded is a finding lost.
chaos_clear_reports DELETE - an experiment has already tallied the previous iteration, and needs
an iteration that never reached failsafe to be visibly absent rather than
silently classified from its predecessor's file.
The two globs are named for the same reason: they differ by one leading slash and picking wrong is
silent.
AND THE SELF-TEST THAT DID NOT EXIST
bin/chaos-test.sh is called by two workflows and had no self-test at all, so the extraction had
nothing holding its behaviour still. bin/test-chaos-reports-common.sh now covers the globs, the
archive policy, that a second archive pass does not cascade, and that clearing is scoped to one test
name and does not touch the archives.
Its negative control is the part that matters: asserting the strict glob SKIPS an archived report
would pass vacuously on a fixture that has none, so the permissive glob is asserted to FIND the same
file in the same fixture. If a later change makes the fixture stop distinguishing them, that
assertion fails and says so.
The library defines no set options and has no side effects at source time - it is sourced both by a
caller under `set -euo pipefail` and by callers under bare `set -u`, which the experiment runners use
deliberately because a failing iteration is their data.
…as grow bin/ grows and never shrinks. The prefixes already carry most of the grouping - check-, test-, ci- and build- account for the large majority - and the residue is the problem: one-off drivers that answered one empirical question and then stayed forever, because nothing ever says a script is done. Two rules, and neither is enforceable, which is why they are written where an agent editing bin/ will be handed them. An experiment driver takes the `exp-` prefix. The test is what the script is FOR, not what it runs: a header stating a question with a stopping condition is an instrument; something you would re-run after any change to the code it exercises is a tool and keeps an ordinary name. Deliberately a prefix and not a subdirectory. Script paths here are cited from pom.xml, workflow YAML, javadoc and docs; nothing checks those citations, and AGENTS.md already records a move that left six stale pointers behind. A prefix buys the same legibility for none of that risk. When the question is answered, the method moves to docs/solutions/ and the script goes. The durable value of an experiment is how it was settled - the control arm, the trap that voided the first attempt, the number - and that is a write-up this repo already knows how to keep. An executable nobody will run again is not a record; it is a file everyone scrolls past and no reader can tell from a live tool. The rule states its own two exclusions, because both are easy to get wrong: it does not license deleting a driver whose question is still open, and it does not apply to a tool that merely happens to have been written during one investigation.
…nstead of ls-only THE MEASUREMENT THAT PROMPTED THIS Six runners had landed referenced by no doc, no workflow, and no other script. Fifteen of the seventy-three scripts in bin/ are named nowhere at all - but seven of those are self-tests that check-all.sh globs, so they are found by construction. Strip those and the genuinely unreachable set is the six experiment runners plus two build helpers. So the gap is not "bin/ has no index". It is that the scripts nothing globs also have nothing naming them, and an agent that needs one writes a seventh instead - which is how five copies of the same maven invocation got here in the first place. WHAT EACH PIECE DOES docs/testing.md gains an "Experiment runners" section: a table of which QUESTION each script answers and whether that question is still open, plus the rule for choosing between a runner and the lanes above it. Every other section of that file describes a lane that reports pass or fail; these are instruments that report a measurement, and the distinction is the useful part. A rate needs N runs - one run gives a pass or a fail, which is not a rate, and no lane here aggregates across runs. bin/AGENTS.md gains the discovery half of the lifecycle rule it was missing. It already said when a script is finished; it said nothing about how one is found while alive. A row is owed only where no mechanism sweeps the file - check-* and test-* are globbed, so naming them elsewhere is a copy that goes stale. .github/workflows/experiments.yml offers each runner as a dispatch choice on the high-CPU box, which is where a ten-iteration batch belongs. Its header answers "why not on every push like everything else" in three parts: a rate measured once is not a rate and nothing aggregates across pushes; the batch is a runner-hour per push for a number nobody asked for on that commit; and gating on it would need a threshold nobody has the spread to choose - the manufactured-evidence trap this repo has already written up. One experiment is also scheduled, alone: the largeNumberOfInstances rate, whose question is open and whose answer has not moved since a 2026-01 upstream report. A rate nothing samples stays unmeasured. TWO THINGS THIS COMMIT ADMITS RATHER THAN HIDES The artifact-collection step originally guessed two of the six /tmp paths wrong - it would have uploaded nothing while reporting success. Corrected against the scripts, and the step now warns loudly when it finds no output, because an empty artifact reads as an empty result. The first table row cites no tracking note. The note exists on the branch these runners came from and not on master, and a link into another branch's file is a broken reference to everyone else. The row says so and says when to add the link.
…cheduled run it would have broken The lifecycle rule landed a commit ago and the scripts it describes still had ordinary names, so nothing in bin/ actually told a reader which of its seventy-odd scripts are instruments with a question and a stopping condition. The prefix is the rule made visible in `ls`. Five are a mechanical prefix. batch-857-experiments.sh becomes exp-batch-857.sh rather than exp-batch-857-experiments.sh, because the prefix already says "experiment" and the word twice reads as a different thing than it is. WHAT THE REFERENCE SWEEP CAUGHT Only three files named these scripts, which is the same finding as the discovery commit from the other side: they were nearly unreferenced because nothing routed to them. The sweep for leftovers found one, and it mattered: the workflow's SCHEDULED run falls back to a literal experiment name when there are no dispatch inputs, and that literal was not covered by the options list or the case arms. Left alone it would have failed validation with "Unknown experiment" every Sunday - a scheduled job that never runs is the silent-no-op shape this repo keeps paying for, and the weekly sample is the entire reason that schedule exists. This is why the sweep is run over the whole tree with the old names, rather than trusting that editing the obvious three files was enough. Grep for the string, not for the places you expect it. Already-pushed commit bodies still cite the old names. They are a historical record of what the files were called when those commits were written and are left alone; what matters is that nothing in the tree points at a name that no longer exists, and check-file-refs.sh now agrees.
…s so Recorded, not fixed - the decision is the maintainer's, and the finding is cheap enough to park. bin/build-without-tests.sh and bin/build-parallel-consumer-core-without-tests.sh are seven lines each. Between them they pin JDK 13 where the project requires 17, call system mvn where AGENTS.md requires the wrapper, resolve JAVA_HOME through the same /usr/libexec/java_home call this repo has already been bitten by, run on macOS only, and omit set -euo pipefail so the failed lookup does not stop the build either. They are pre-fork upstream files - introduced by 2aa4da9, still carrying Confluent's 2020-2022 header - so JDK 13 was plausible when they were written. Nothing has run them since and nothing told anyone they had stopped being true, which is the actual defect: a build script pinning a wrong JDK is worse than an absent one, because someone eventually runs it and debugs the fallout somewhere else entirely. Found by asking which scripts in bin/ are referenced from no doc, no workflow and no other script. These two were the only non-self-test answers - the rest of that list is self-tests, which check-all.sh globs and so finds by construction. The note names what already replaces them (bin/build.sh takes the same arguments and works), and the two things worth weighing before deleting: they are upstream files, so removal is a deliberate fork divergence rather than a tidy-up; and keeping them for provenance means fixing the JDK pin and the maven call in the same change, since a kept script that cannot work is the state the note exists to end.
… script nobody has claimed #200 deletes bin/build-parallel-consumer-core-without-tests.sh in e7b1dda, open and unmerged. The note now says so at the top so nobody redoes it, and narrows to its surviving sibling, bin/build-without-tests.sh. IT FOUND A DEFECT THIS NOTE MISSED, AND IT IS THE SHARPER ONE That script ran a bare `mvn -pl parallel-consumer-core` with no `-am`, which cannot succeed against this tree at all - the enforcer's ReactorModuleConvergence rule fails it outright. So it was not merely mis-pinned to JDK 13; it could never have run. Recorded here because the finding is about the invocation shape rather than that one file, and because I reproduced it independently the same day without recognising it: an unrelated `-pl parallel-consumer-core` without `-am` failed with exactly that enforcer message while I was checking something else, and I treated it as my own typo. The surviving script builds the whole reactor and does not have that fault, which is exactly why it needs its own decision rather than inheriting one. TWO ROUTES, SAME PAIR, SAME DAY A discoverability audit here (which scripts does no doc, workflow or script reference?) and a defect-class sweep there (where else does a bare `-pl` appear?) arrived at the same two files from opposite directions. Kept in the note rather than tidied away: it is the argument for doing the sweep at merge prep, and it means the survivor is not an oversight by either party but a file whose fate simply has not been decided. The note also now carries the ordering, since deleting is the likely answer: #200 merges first and takes the sibling with it. Deleting this one on another branch beforehand buys a conflict in the same directory for nothing.
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fcd8655cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…it did not measure
A codex review of this branch found eight ways one of the new unattended instruments
reports an answer it never established, plus three contract slips on the in-flight note.
They are one family: every one of them turns "did not measure" or "measured something
else" into a clean-looking result, which is the reading that cannot be re-checked because
nothing about it looks wrong.
CANNOT RUN, REPORTED AS RAN
- The two CROSS-TREE runners read sibling worktrees (.claude/worktrees/pr29, pre-344) that
actions/checkout does not create, and both were offered as workflow_dispatch choices.
exp-confirm-async-drain wrote a MISSING TREE row and exited 0; exp-batch-857 had no check
at all, so maven failed per iteration and every absent report classified as DID-NOT-RUN.
Either way: green job, artifact with no data, indistinguishable from a clean result. They
now refuse to start without their trees (pc_require_tree, exit 2 - bin/'s "cannot run",
deliberately not a pass column), and neither is a dispatch choice any more. Provisioning
arbitrary historical worktrees on a runner is not an alternative: those are specific
branches at specific commits, chosen by hand for one comparison.
- The workflow's `iterations` input was documented as self-validating - "anything else
fails the script's own seq". It does not. The runners set `set -u` and deliberately not
`-e`, so `for i in $(seq 1 ten)` writes to stderr, expands to nothing, never enters the
loop and exits 0. The job then went green having run no test. Validated in the workflow
now, and the comment that asserted the opposite - which is why nobody wrote the check -
is gone.
A DETECTOR FIRED, RECORDED AS DETECTOR SILENCE
- pc_detector_verdict's "other probe" list was written from memory: it omitted DRAIN_OVERDUE
and PROBE_DEGRADED and listed LEDGER_KEY_ORDER, which ProgressProbe does not raise. An
unmatched violation fell through to LEDGER-ONLY-MISS-CASE - the verdict meaning every
detector stayed silent - so a run a detector DID catch was counted as a miss, in the audit
whose entire result is a count of misses. The list is completed against ProgressProbe's
violate() calls, matching is anchored on the "VIOLATION: " announcement (the bare-name
match also hit the ambient observer's autopsy bullets, for a probe that was not gating),
and a new UNNAMED-PROBE bucket catches the next omission instead of letting it corrupt
the headline. This is not the phantom `unclassified` row the audit's header retired: that
one was listed and never emitted; this one is emitted.
A DIFFERENT MECHANISM, ANSWERED UNDER THIS QUESTION'S NAME
- pc_first_violation reads `violations=<n>`, ProgressProbe's AGGREGATE count, so
INSTANCE_STALL, ZOMBIE_MEMBER, DRAIN_OVERDUE or PROBE_DEGRADED each make it non-empty.
All three runners that hunt the ASYNC stall stopped on it and wrote up whatever
trajectory followed as that stall's - a wrong answer, filed confidently, in the one file
anybody reads. New pc_signal_fired gates each of them on NO_PROGRESS itself; the
aggregate is still recorded beside it, because a run another detector caught first is
worth seeing.
A WEDGE THAT CLIMBED, CALLED DRAINED
- exp-confirm-async-drain declared DRAINED whenever the last consumption reading exceeded
the first. A run that advances once after the violation and then wedges short of the
backlog satisfies that - and it is exactly the partial-progress wedge the experiment
exists to find, so the reading that called it DRAINED is the reading that reports the bug
as absent. The verdict is now the scenario's own done=true; a run that advanced without
completing gets its own PARTIAL-THEN-STOPPED. pc_consumed_bounds carries the denominator
and the done flag, pc_trajectory_after_violation carries them per row, and every place
that said "climbing means DRAINED" says to read the last row instead.
THE WRONG OUTCOME, AND A PARTIAL ONE
- AmbientProbeExtension emitted PC-DEADLINE-HEADROOM from afterTestExecution, which runs
before @AfterEach methods and AfterEachCallbacks - either of which fails the test. A test
whose method passed and whose teardown failed was labelled outcome=PASSED inside a
failing run: not a missing value in a key=value line, a wrong one. The clock still stops
at the end of the method (teardown must not count as time the test spent); the label now
comes from the TestWatcher phase, which runs after all teardown and is told which way the
test went. Aborted and disabled tests stay silent - failsafe reports them as skipped, not
as a duration. Three unit tests pin the split.
- MultiInstanceHighVolumeTest reported throughput from catch (ConditionTimeoutException).
Awaitility's failFast arm exits through TerminalFailureException, an unrelated sibling, so
the one failure with a named cause ("PC died") was the one that reported nothing - and
bin/performance-test.sh printed NONE FOUND for a run that had just measured a processor
death. Reported and rethrown unchanged on that path too. A sweep for the same shape found
two more instances, both in files this branch does not touch, recorded in
docs/inflight/test-awaitility-failfast-skips-the-failure-diagnostic.md rather than folded
in here; VeryLargeMessageVolumeTest and VertxConcurrencyIT were checked and dismissed.
- bin/performance-test.sh's summary is headed "per performance test" and only one of the
four @tag("performance") classes calls ThroughputReport, so one figure read as coverage of
the lane. It now names the classes that ran and emitted nothing. Both lists are derived -
from the failsafe reports the run just wrote and from the summary's own test= field -
because a hardcoded roster is the copy that goes stale, silently, the day someone adds a
fifth.
THE IN-FLIGHT NOTE
- Renamed bug- to ci-: docs/inflight/AGENTS.md reserves bug- for defects in PRODUCT code and
assigns build and harness tooling to ci-. Renamed now, before citations accumulate.
- Added inflight-state: parked. The prose said parked and the tag did not, and absent means
open - so a deliberately deferred item was being presented among active work.
- Removed the command-answerable counts the same contract forbids. One of them was already
wrong: "breaks three of this project's stated build rules" sat above a list of five.
Not done, and left for the maintainer: the review also argued that this branch's own
ChaosChurnStormIT calibration record ("drained on all six firings", "already answered")
retires exp-confirm-async-drain and exp-hunt-async-stall-answer outright, per bin/AGENTS.md
"a script that answered its question is finished". The contradiction was real and is fixed -
both headers and the docs/testing.md rows now state what is established and what would still
be new - but retiring a live instrument is the judgement bin/AGENTS.md explicitly leaves to
whoever knows what the experiment found.
Owner
Author
|
@claude review this |
|
Claude finished @astubbs's task in 4m 24s —— View job Review
Scope note: this is the test-observability half of #29 — log silos, the recovery diagnostic, and the Verified: the Codex round's fixes are actually correct, not just claimed
One residual nit (non-blocking)
On the two threads left open for the maintainer
SummaryNo blocking issues. The one item above is a comment/behavior mismatch worth a one-line fix but doesn't affect correctness — the log-silo design (copies, additivity preserved) means the classifiers still see everything they need from the raw log. Everything the Codex round flagged has been fixed correctly, not just marked fixed, based on independent re-reading of the resulting code and tests. |
…ually read Both runners' headers claimed they classify from the siloed probes.log "so 'did it fire' is a small file instead of a grep of tens of thousands of lines". They never did: pc_first_violation, pc_signal_fired and pc_detector_verdict are all handed $log, the maven console redirect. exp-audit-stall-detector-silence.sh even records the probes.log path in every tally row and then classifies from somewhere else. The code is right and the comments were wrong, which is the direction that matters here: the two things these scripts read live in DIFFERENT silos. The "VIOLATION: NO_PROGRESS" announcement is ProgressProbe, routed to probes.log; the "[diagnose] ... consumed=n/m ... done=" trajectory is ChaosScenarioBase, routed to harness.log. The run log is the only place both appear together, and it has them because the silos are routed as copies with additivity left alone. Reading the silo would mean splitting the classifier across two files for no gain. So the headers now say what happens and why, and name the silo directory as what it is - the small file for whoever opens the run afterwards. The pom comment that justified keeping failsafe off the per-fork subdirectory said the runners "read" that flat path; corrected to "record", which is still a reason to keep it flat. Found by an automated review, which called it non-blocking and was right: no behaviour changes here. A comment that describes code that is not there is still a config lie, and this repo has a rule about those because nothing goes red when one drifts.
Owner
Author
|
Thanks - the residual nit was real, and fixed in You are right that the classifiers were handed So both headers now say what the code does and why, and name the silo directory as what it really Everything else in your review is verification rather than a finding, so nothing else changed. The |
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…y duplicate at merge, and the cleanup nobody comes back for #381 renames every experiment runner to an exp- prefix and lands first. This branch still carries the originals, so the merge sees six NEW PATHS rather than six conflicts - git reports nothing, and master ends up holding both copies of each. They then diverge, because only one of the two is the one anybody edits. That is the shape worth recording: not a conflict to resolve but an absence of one. The table names each pair so the deletion is mechanical rather than a judgement call at the worst moment, and says which side wins - the exp- copies, because the docs/testing.md choosing table, bin/AGENTS.md's lifecycle rule and .github/workflows/experiments.yml all name those, while the un-prefixed ones are referenced by nothing once #381 lands. bin/performance-test.sh is modified on both sides, so it will conflict properly; that one is the easy case. It also says to diff before deleting rather than assume the pairs are identical, since this branch may carry a fix that never crossed. AND THE QUESTION THAT ONLY GETS ASKED HERE A second task asks which of the six deserve to exist at all, because this PR answers some of the questions they were built to ask. bin/AGENTS.md already carries the rule - an experiment whose question is settled has its method written up in docs/solutions/ and its script deleted - and the merge that answers them is the one moment the judgement is cheap. Nobody comes back for it later, which is precisely why bin/ has only ever grown. The entry names the candidates one at a time rather than as a batch, including which are safe: the detector-silence audit stays because its question is reopened, the two async-drain runners are the same question at two confidence levels and collapse into one write-up if it is settled, and the batch runner is a snapshot of one day's outstanding work. This branch's own scripts get the same test and not a free pass - the deterministic soak probe is a regression instrument worth keeping, the overnight torture harness was built for a soak that has already reported.
…nd write up what they found
A review caught this branch contradicting itself, and it was right. ChaosChurnStormIT's calibration
record says the backlog drained on all six firings collected and marks the question "already
answered, do not re-derive" - while two runners still advertised that same question as open and
dispatchable. bin/AGENTS.md, added on this branch, says an experiment whose question is settled has
its method written up in docs/solutions/ and its script deleted. That rule now applies to its own
author.
The write-up lands first, because it is the part that has to outlive the scripts, and their headers
carried more method than the code did:
- MORE FIRINGS BEAT MORE SEEDS. The first attempt to firm up a single-firing result hunted for a
second seed and found none in eight tries. Wrong instrument: the known seed already fires most
runs, so the cheap axis was repetitions of the known trigger. A seed hunt answers "can this
happen elsewhere"; a firing count answers "what does it do when it happens", and only the second
question was open.
- A STOPPING CONDITION MUST BE THE ANSWER, NEVER THE ENGAGEMENT. An earlier run stopped as soon as
the diagnostic switched on and reported no answer at all. Engagement proves the wiring; it is not
an outcome.
- THE CROSS-TREE TRAP that voids the comparison: on a tree predating the diagnostic's lift into
ChaosScenarioBase the flag is accepted and does nothing, so those runs fail with errors=1 while
emitting no telemetry - and a grep for violations reads that silence as "did not fire".
The discriminator survives too, in the write-up and in the class javadoc: a run that drains
re-derives a known result, a run that stays flat is the finding.
WHAT ELSE HAD TO MOVE
Deleting a script is the easy half. The roster in the shared library, the choosing table in
docs/testing.md, the dispatch workflow's options AND its case arm, and a comment in the batch runner
citing a header that no longer exists all named them. The workflow is the one worth noting: its case
arm still accepted a runner the options list had already stopped offering, so the inconsistency
predates this commit and would have outlived the file it referred to.
docs/testing.md keeps a row rather than losing one - it now records the question as RETIRED and
points at the answer, because a table that simply drops a row tells the next reader nothing about
where the instrument went.
Prompted by review rather than by anyone revisiting the scripts, which is the general lesson: an
instrument that outlives its question does not look retired, it looks live, and the next person
spends a chaos run re-deriving a known result.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…e traps it predicted All three merge-time hazards this branch recorded in advance fired exactly as written. Recording how each was resolved, because the notes said what to do and the reasoning is worth keeping. 1. SIX SCRIPTS DUPLICATED SILENTLY, AND GIT REPORTED NOTHING #381 renamed every experiment runner to an exp- prefix and landed first. This branch still carried the originals, so the merge saw six NEW PATHS rather than six conflicts and produced both copies of each. Zero conflicts reported. The originals are deleted. They were diffed first rather than assumed identical, which mattered: the pairs differ by 33 to 93 lines, and the exp- copies are the improved lineage - they source the shared library the originals never had. Two of the six had no exp- twin at all, because #381 RETIRED them once their question was answered; re-adding them here would have resurrected instruments that master had deliberately buried. 2. getAssignmentSize - deleted by the dependency, still called here A compile error, which is the good case. Resolved as the note prescribed, NOT by reverting the deletion: the surviving caller is a failure-path diagnostic invoked once per dump, so the per-poll cost that justified removing the cache does not apply to it. The public getter is gone and the field is dropped from the dump, with the reasoning left at the site so the next reader does not restore a mirror of state Kafka already owns. 3. getConsumerClass did NOT come back Master's side won, which is the outcome the note demanded. It could never have worked - Object.getClass() on a wrapper - so restoring it during conflict resolution would have produced a check that always takes the not-a-known-driver path: defended on paper, undefended in fact. HOW THE THIRTEEN CONFLICTS WERE RESOLVED Ten resolved to master, each verified as a superset or successor rather than taken on faith: master's ConsumerManager carries the pause-cache entry refresh with its "deliberately BEFORE pollingBroker" reasoning intact, its commitRequested field, and none of the three deleted members; the test files extend the shared base a review pass extracted; the logback config carries the siloDir fix, where this branch still had the self-referencing property that silently ignored -Dpc.log.dir. One was a genuine merge. AmbientProbeExtension had real work on BOTH sides - the deadline-headroom reporter on master, the unfireable-detector caveat here - so both were kept. The first splice missed the caveat's private helper and the compiler caught it. ChaosChurnStormIT took master's corrected calibration text: this branch still claimed the recovery diagnostic had never engaged, while master records six firings that all drained. Keeping this side would have re-introduced a claim its own author had already retracted.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
… tasks were describing a week that had ended Prompted by a reader asking whether "PR #29 is still BLOCKED, and that is still correct" was rubbish. It was. That paragraph named #57, #322 and #267 as open "checked 2026-08-27". All three are merged - and two landed on 2026-08-26, the day BEFORE the check claims to have run, so it was wrong when written rather than merely stale. It is left visible with the correction attached, because a status line nobody notices is what produced it. FOUR MORE IN THE LIVE TASK LIST, WHICH IS WHAT A READER ACTS ON Task 2 asked for a second firing to confirm the async drain demotion. Answered: the calibration record now carries six firings that all drained, and the two runners built to ask it were retired on #381 with their method written up - so a reader following that task would have gone looking for deleted scripts. Task 5 said "the two unreproduced field reports". #177 closed on 2026-09-01, so it is one - and that issue's own triage already names a likely cause, which outranks waiting on #352. The roadmap edit and the compounding run were both listed as owed; both are done. Task 4 carried the "only issue upstream ever labelled a verified bug" claim for the third time today. It is false, it was corrected across the tree this morning, and it keeps reappearing in files that merged afterwards - which is the durable lesson: a sweep is complete only against the tree as it stood when it ran. ALSO, FROM A REVIEW COMMENT .gitignore's `target/pc-logs/` is removed. The reviewer asked whether it should be `target/`; it should be neither - line 125 already ignores `target`, and `git check-ignore` confirms the path is ignored identically with and without the addition. This PR no longer touches .gitignore. The wider vet found the rest sound: the other "still open" phrasings refer to open QUESTIONS or are self-aware about not being the source of truth, and the three refactoring-backlog classes named as not extending the shared base still do not extend it.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
… number measures the product PREDICTION, recorded before CI runs it: MultiInstanceHighVolumeTest returns to roughly 71,000 records/second in the full lane. If it does not, JVM sharing is not the mechanism and this commit should be reverted rather than built on. WHAT IS ESTABLISHED The same test, same tree, four conditions: alone, lane of one .................................. 73,722 PASS CI lane WITHOUT MultiInstanceRebalanceTest (#381) ... 71,387 PASS CI lane WITH it, incl. the 21-instance test ......... 39,684 FAIL CI lane WITH it, that test disabled ................. 44,992 FAIL LOCAL full lane, identical composition .............. 72,498 PASS So the degradation is real, correlates exactly with those classes being in the lane, and is CI-only. Disabling the heaviest of them recovered about 5,000 records/second and no more, which is what says the mechanism is not that one test but the shared VM. WHAT IS NOT ESTABLISHED, AND THE COMMIT SAYS SO The local arm REFUTES the simple version of the model: on a development machine the full lane passes at 72,498, statistically identical to running alone. There is enough headroom here to absorb what a GitHub runner cannot. That makes this an environment-dependent effect and means no local experiment can verify the fix - the thing being fixed does not reproduce locally. CI is the instrument. THE FLAG NAME IS THE INTERESTING PART The user property is `reuseForks`, not `failsafe.reuseForks`. Confirmed with help:describe against the plugin, which prints "User property: reuseForks". The qualified form - the obvious thing to write - is accepted and does nothing, exactly like -DforkCount versus -Dsurefire.forkCount, which this repo's own pom already warns about, and exactly like -Dchaos.commitMode, which was accepted and silently ignored for weeks while a harness reported transactional coverage it never had. That is three instances of one defect class in this investigation alone, and the general form is worth more than the fix: a property name that is wrong is not rejected, it is obeyed by nobody.
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
Ten commits, no Java overlap - master's core work (#257's produce-lock handover, #393's thread-confined consumer) is nowhere near the offsets package this branch changes. All three conflicts were in docs, and all three were two ledgers being appended to from both sides on the same day. WHAT THE RESOLUTION KEPT Both flake ledgers gained entries from master AND from this branch on 2026-09-01, so every conflict resolution keeps both sides rather than choosing one: - test-load-tightness-flakes.md: master's inFlightMessagesCommittedIfProcessed- DuringShutdown[1] paragraph and this branch's committedOffsetRemoved[1] latest recurrence are about different tests and neither supersedes the other. - test-untracked-ci-flakes.md: both sides added a processInKeyOrder row for sightings on the same day from different branches. Merged into one entry rather than two, because two rows under one test name is exactly the conflation master's own section warns against. This branch's four sightings are folded in as a sub-section of master's [3] bullet, where they turn "seen once, not reproduced" into a control-armed contention finding - and master's isolated-run result stops reading as a contradiction, since isolation removing the failure is what the contention reading predicts. - refactoring.md auto-merged: master deleted the produce-lock double-release OPEN QUESTION entry because #257 answered it. TWO REPAIRS MADE WHILE IN THERE Master's table had a blank line between two rows, which markdown renders as two tables with the second missing its header. Closed up. The JStream entry said "delete this entry when #116 lands" - a marker docs/inflight/AGENTS.md explicitly forbids on master, because the merge is the moment nobody is looking here. Replaced with the migrate-rather-than-delete outcome that doc names, and the attestation block it opened is now closed rather than running into the next section's. INHERITED AND READ #403 makes Node the default for new bin/ scripts and deletes check-shell-sigpipe.sh; #382 rewrites the hook guards to derive identity from the command rather than the session; a new core AGENTS.md binds @GuardedBy to any field change in the engine. This branch adds no scripts and changes no shared field, so none of the three change what it does - recorded because a green build is not evidence the ground under a design held still. AND ONE FIX THE MERGE COULD NOT BE COMMITTED WITHOUT #403's new bin/check-source-patterns.mjs refused this merge commit, reporting seven of #381's experiment runners as new shell scripts and advising --no-verify. They are master's, already grandfathered there, and this branch never touched them. The cause is that its `added-files` scope is the union of committed and staged additions against the merge base - and while a merge is staged but not yet committed, HEAD is still the pre-merge tip, so every file master added since the branch was cut is an addition against that base. It corrects itself the moment the merge commit exists, which is why nothing on master has seen it: it fires only in the window where the fix has to be applied. Membership of origin/master settles it without needing to know about merges at all - a path already there is not new to this repository, whoever staged it - so the subtraction lives in bin/lib/added-files.mjs as a pure function of three lists. That placement is what lets it be tested by control pair like every rule in the table, instead of standing up a fixture repository, which is the reason the runner had no test to break. The four must-NOT-match cases are red-proven against the pre-fix implementation. Carried here rather than split out because the alternative was committing this merge with --no-verify, and a gate that fires on somebody else's merged work teaches exactly that bypass.
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
…ture The merge of master compiled to nothing on this branch: git took THIS side's import block and MASTER's method bodies, and they disagree. #203 (merged into this branch) replaced the raw logback ListAppender/Logger/ILoggingEvent boilerplate in this class with the shared LogCapture helper; master's #381 independently added three headroom tests written in the old style. Textually clean, semantically contradictory - the imports were gone and the bodies still called the classes. Resolved toward the helper rather than by restoring the imports. Restoring them would put a second, raw capture idiom back into a class #203 had just finished unifying, which is the duplication AGENTS.md's reuse-test-utilities rule exists to stop, and a reviewer would have had no way to see it was reintroduced by a merge. headroomLines() now reads LogCapture.events(), and each test scopes its read inside the try-with-resources - the pattern the rest of the class already uses. VERIFIED WITH A CONTROL ARM, because the ported tests failed at first and the tempting reading was that the port broke them. It did not. Whole class, no -Pci: master's own unmodified version fails the SAME three tests the same way, in a detached worktree at 000de5d with none of this branch applied - they attach to a JVM-shared logger with no isolation and read each other's lines under concurrent mode. Under -Pci, which is the lane CI runs, master and this branch both pass 28/28. So the port is behaviour-preserving and the class-level failure is a pre-existing master condition, recorded rather than fixed here.
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
…ture The merge of master compiled to nothing: git took THIS side's import block and MASTER's method bodies, and they disagree. This branch replaced the raw logback ListAppender/Logger/ILoggingEvent boilerplate in AmbientProbeExtensionTest with the shared LogCapture helper; master's #381 independently added three headroom tests written in the old style. Textually clean, semantically contradictory - the imports were gone and the bodies still called the classes. Nothing in the merge reported a conflict. Resolved toward the helper rather than by restoring the imports. Restoring them would put a second, raw capture idiom back into the class this branch had just finished unifying, which is the duplication AGENTS.md's reuse-test-utilities rule exists to stop, and a reviewer would have had no way to see it was reintroduced by a merge rather than written that way. headroomLines() now reads LogCapture.events(), and each test scopes its read inside the try-with-resources - the pattern the rest of the class already uses. VERIFIED WITH A CONTROL ARM, because the ported tests failed at first and the tempting reading was that the port broke them. It did not. Whole class, no -Pci: master's own unmodified version fails the SAME three tests the same way, in a detached worktree at 000de5d with none of this branch applied - they attach to a JVM-shared logger with no isolation and read each other's lines under concurrent mode. Under -Pci, the lane CI runs, master and this branch both pass 28/28. So the port is behaviour-preserving, and the class-level failure is a pre-existing master condition - recorded, not fixed here.
5 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
…am-memory-leak Master gained 30 commits since b2e6c19. One conflict, and it is the interesting kind: both sides were fixing the SAME wrong sentence, on different axes. `AmbientProbeExtension.buildAutopsy` used to print "the fault is likely in the test itself, not consumer-group progress" whenever the probe saw nothing. Both sides had already agreed that claim is unearned; they disagreed about why. - This branch (aba0330) found it printing under `ContainerLaunchException: Container startup failed for image confluentinc/cp-kafka:7.9.0` - the group never formed, so the detectors had nothing to SAMPLE. Fixed in prose, and left a TODO(refactor) asking for the distinction to be made mechanically. - Master (#381) asked whether the detectors had TIME to fire, and answered it with code: `appendUnfireableDetectors` resolves the test's own `@Timeout` and names every detector whose bound is at or past that ceiling. Nothing-to-sample and no-time-to-fire are two different reasons a clean probe means nothing, so taking either side alone would have re-armed a wrong verdict on the other axis. Resolved by keeping both: the weaker leading claim this branch established ("nothing in consumer-group progress explains this failure"), master's conditional and its `appendUnfireableDetectors` call, and the container/Docker caveat. Master's "IF YOU TRUST THIS TEST" clause is dropped rather than merged - it qualified the "fault is in the test" claim, which no longer appears. `AmbientProbeExtensionTest` asserts only on the `probe clean - no rebalance dwell...` prefix, unchanged on both sides. `docs/testing.md` auto-merged with both caveats intact and complementary, and its closing sentence still describes what the code now prints. The TODO(refactor) stays: master answered detector REACH, not the "never sampled a group" vs "sampled and saw nothing" distinction it asks for. Also carried in, and this PR now owns retiring it: #207 landed a ledger entry in `docs/inflight/test-untracked-ci-flakes.md` recording `JStreamParallelEoSStreamProcessorTest.testConsumeAndProduce` and `.testFlatMapProduce` failing TOGETHER on produced-record count. That is this PR's defect observed rather than inferred, and the entry says explicitly not to diagnose it separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extracted from #29, cut fresh from master so it carries none of that
PR's fix. It closes no issue on its own: this is the test-observability half of the
confluentinc#857 investigation, which is useful to everything and blocked behind
nothing. The overnight torture harness needs it, and today can only reach it by stacking on a PR
whose subject is a deadlock fix.
Description
Test log silos. The core unit profile gains sifted streams under
${pc.log.dir:-target/pc-logs}- errors, warnings, probes, harness, and PC's commit, lifecycle andstate channels. They are routed as copies (default additivity is left alone), so console output
is unchanged and nothing that reads it breaks. No appender declares a level of its own, so
-Dpc.log.levelstill governs the whole harness exactly as before.The integration profile is renamed to
logback-integration-test.xml. Both files previouslycopied to
target/test-classes/logback-test.xml, so which one configured a run depended onclasspath order - a silent coin flip, and the kind of thing that makes an instrumented result
untrustworthy without announcing itself. They now have distinct names and the integration profile is
selected explicitly with
-Dlogback.configurationFile.docs/logging.mdowns the why, including how to prove which profile actually loaded. That checkearns its place:
surefire:testdoes not reprocess test resources, so an edited config can sit insrc/never reachingtarget/test-classes, and a run against the stale one reads exactly like achange that had no effect.
-Dpc.log.dirhad never worked, and nothing said so. The config declared<property name="pc.log.dir" value="${pc.log.dir:-target/pc-logs}"/>- a self-reference, whichlogback resolves to the default rather than to the system property, so every run wrote to
target/pc-logshowever it was invoked. The local property is nowsiloDir; the flag it reads isstill
pc.log.dir. Proved by passing the flag on the fork command line and watching the files land.Each surefire fork gets its own directory. Seven non-prudent
FileAppenders sharing one path atforkCount=1Cis several JVMs writing the same files, so surefire'sargLinenow appendsfork-${surefire.forkNumber}.${pc.log.dir}stays a property so a caller can still redirect theroot.
The stall-recovery diagnostic moves to
ChaosScenarioBase.-Dchaos.diagnoseStallRecovery=trueanswers the one question a gating run structurally cannot: when a probe violation fires, does
whatever it caught ever recover, or is it wedged forever? A gating run destroys that evidence at the
moment of detection, because
failFastaborts the wait on the first violation - so "unbounded" and"merely slow" are indistinguishable from the data it leaves behind. The capability lived on
AbstractRevokeUnderWorkScenario; it moves to the base so any scenario can be asked the samequestion, rather than
ChaosChurnStormITgrowing a drifted copy of it.It cannot turn a red run green.
assertScenarioSlosstill asserts the probe's violations are emptywhichever way the wait ended, and with the property absent the gating configuration is byte-for-byte
unchanged.
Unattended experiment runners in
bin/, each driving a test that already exists on master.They produce answers that cost CPU rather than attention: each runs unattended, tallies its own
outcome, and prints a verdict a reader can act on without reconstructing the run. Three things come
with them, because six of these had arrived referenced by no doc, no workflow and no other script:
exp-prefix and a lifecycle rule inbin/AGENTS.md- a script whose header states aquestion with a stopping condition is an instrument, and when the question is answered the method
moves to
docs/solutions/and the script goes.bin/grows and never shrinks because nothing eversays a script is done;
docs/testing.md- which question each runner answers, whether thatquestion is still open, and when to reach for a runner rather than a lane. An
exp-script owes arow before it merges, since nothing globs these the way
check-all.shglobs the gates;.github/workflows/experiments.yml- dispatch-only on the high-CPU runner, plus one weeklyschedule for the single rate with a standing open question. Nothing here runs on push and nothing
gates; the header carries why gating on a rate would need a threshold nobody has the spread to
choose.
bin/lib/chaos-reports-common.shis one home for the failsafe-report globs and the tworep-collision policies these share with
bin/chaos-test.sh: KEEP for CI, whose reports are stillgoing to be read, and DELETE for the experiments, where an iteration that died before failsafe must
be visibly absent rather than silently classified from its predecessor's file. Covered by
bin/test-chaos-reports-common.sh.One correction carried in
ChaosChurnStormIT's javadoc and its runtime warning both told the reader the recoverydiagnostic had never engaged on this scenario, and that any result was therefore a NEW finding. It
has since engaged: the backlog drained on every firing collected on 2026-08-28, which is what
demoted the asynchronous stall to a timing proxy rather than a distinct defect. A message written to
stop an operator re-deriving a known result had come to cause exactly that. Both now say so.
Both also now cite the class's own
Calibration statusjavadoc rather than adocs/inflight/note.Those notes are deleted when their work lands, so shipped code citing one is a citation with an
expiry date - and nothing goes red when it expires.
Deliberately left on #29
bin/soak-deadlock-probe.sh- drives that PR's ownRebalance857CommitSyncDeadlockProbeIT, whichdoes not exist on master
AGENTS.mdrule - it documentsConsumerManager.tryClaimOwnership, which is that PR's consumer-ownership workdocs/investigating.mdpointer toprove-the-problem-exists-before-writing-the-fix.md- thatsolution doc is not on master yet
docs/testing.mdhunks about W5 calibration andKeyOrderLedgerscopeThe codex review round
An automated review of this branch found eight ways one of these instruments reports an answer it
never established, plus three
docs/inflight/contract slips. One family, all fixed incd3208c5d: two cross-tree runners were dispatch choices although CI never creates the siblingworktrees they read (green job, empty artifact); the workflow's
iterationsinput was documented asself-validating and is not; the detector classifier's signal list omitted two signals, so a run a
detector did catch was counted as a detector miss; three runners stopped on the probe's aggregate
violation count and would have written another mechanism's trajectory up under the async stall's
name; a partial-progress wedge was labelled
DRAINED; the deadline-headroom line saidoutcome=PASSEDinside a run a failing@AfterEachhad made red; the throughput figure was skippedon Awaitility's
failFastexit; and a one-figure summary headed "per performance test" covered oneof four. Each is answered in its own review thread.
Two threads are deliberately left open for the maintainer: whether
bin/performance-test.shshouldgain the three missing
ThroughputReportemitters rather than only naming the tests that emitnothing, and whether the two async-drain runners are now answered and should be retired to
docs/solutions/- whichbin/AGENTS.mdsays is a judgement made at merge by whoever knows what theexperiment found.
Verification
bin/check-all.sh- every gate passes./mvnw -pl parallel-consumer-core -am test-compilegreen on Temurin 17.0.18; the chaos classesand both logback profiles land in
target/test-classesunder distinct namestarget/pc-logs, with logback's statuslistener confirming which profile loaded
Checklist
docs/logging.mdadded and routed fromAGENTS.md;docs/investigating.mdgains the liveness-check method
docs/features/@claude review thison the PR instead, which is the route that can openinline review threads