Make the simulator honest about serial tails: consume the engine gate chain, fix the probe:page ratio, add per-decision gate visibility - #69
Merged
Conversation
The aggregate OWNER_SPLIT.*/NO_VICTIM.* counters say a gate fired, never which range it blocked or on what readings -- so a serial tail that never splits cannot be attributed from a run's artifacts. Add two --trace event kinds that carry exactly that, additive to the v:1 schema: - owner_split_decision, once per OwnerSplitGovernor.decide() past the open-frontier early-out (blocked OR carved), with the terminal gate's reason plus est / pages_since_last_self_split / outstanding / worker_count / far_ahead_fraction / density_ratio / keys_emitted. - victim_scan, once per ThiefPolicy.selectVictim() pass, with the per-cause skip tallies, the winning estimate and the refusal discriminator -- aggregate per scan, never per candidate. Both policies stay pure: the readings ride back on the decision (OwnerSplitGateInputs on Skip/Carve, VictimScan on Selected/NoVictim) and the executors that already hold the TraceSink emit the events, behind the same enabled() guard every other call site uses. The gate chain short-circuits, so an input a lower gate would have read is reported as a NaN not-computed sentinel, serialized -- like every non-finite number -- as JSON null so a trace line stays strictly parseable. No gate logic, ordering or counter changes. The decision-trace goldens gain one victim_scan entry per recorded steal attempt; no existing recorded value moves.
…or mirrors The simulator installed a sensing variant by reproducing victim selection and the owner-split gate chain in this module with one substitution -- where the estimate comes from -- because the engine's policies had no seam to install an estimator through. They have had one since #68, so the mirrors were two copies kept faithful by a test rather than a mechanism. The sim's RemainingWorkEstimator now EXTENDS the engine's (the estRemaining contract was already identical; what the simulator adds is the two degeneracy probes its sensor counters ask), so a variant is handed straight to the engine's own OwnerSplitGovernor and ThiefPolicy. Both construction paths collapse into one: CURRENT passes null and leaves the engine on its own WINDOW default, which is byte-for-byte the shipped path it already took. RateAnchoredArm delegates classify() too, so a simulated run on the promoted arm emits the same SENSING_OWNER.*/SENSING_STEAL.* rows a deployed one does. The SENSING_ROUTE.* counters are re-keyed off whether an estimator was installed rather than off which mirror class was: the question they answered dies with the mirrors, and the one that replaces it -- which sensor steered this table's leg -- is what a race record needs. The parity test's mirror-vs-engine comparisons are vacuous now, so the same batteries drive the ENGINE chain under all eight arms instead: decisions complete and stable per arm, CURRENT pinned against a chain nobody steered (the no-regression pin on the collapse), and -- new -- every arm required to expose the observed-mass floor's structural zero, a large estimate blocked because min(1, densityRatio) <= f leaves no reachable child tail. That last check is the class the mirror-parity test structurally could not make.
A counter says a gate fired n times; it cannot say which reading made it fire, which is the only question worth asking when a simulated run and a real one disagree. `-Dswath.sim.gate-dump=<path>` writes one TSV row per `OwnerSplitGovernor.decide` past the open-frontier early-out, and one per `ThiefPolicy.selectVictim` pass at `<path>.scans.tsv`. The columns are the engine's own `OwnerSplitGateInputs` and `VictimScan` payloads -- what it already emits as its `owner_split_decision` and `victim_scan` trace events -- so the two sides diff row for row against a replay-server trace of the same listing. Each decision row also carries the decided range's `lo`/`cursor_to`/`hi`: sim node ids and a replay run's node ids are different id spaces, so a tail range is matched by keys. A write-only observer: a dumping run takes exactly the decisions it takes without the dump, and with the property unset no row is formatted and no key rendered. Instants come from the run's own virtual clock. Both files are opened CREATE_NEW and every IO failure fails the run -- an artifact that silently truncated would have its missing rows read as a finding.
… ratio The simulator priced a one-key probe at 35 ms against a 110 ms page — 0.32 — while the store those instruments answer questions about was measured at 121 ms against 223 ms (0.54, nara-1950-census live/replay profile, 2026-07-28). A thief spends a cascade of probes placing a pivot and loses if the victim drained past it meanwhile, so that ratio times the probes an attempt issues IS the race window in owner pages: about two at the live ratio, under one at 0.32. Everything else in the world model was measured faithful — parks, the fleet's single attempt slot, futility pacing, the pivot cascade — so this one ratio is why a simulated thief kept winning races the engine loses 100% of on a wide-flat tail, and why the sim reported a real bucket carved into ~900 ranges where the engine stalls at ~385. PolicyRunFixtures gains LIVE_S3_LATENCY (223 ms page / 121 ms pivot probe / 223 ms delimited probe, flat — the structure probe's measured 2 ms/cp term needs a SimListingView change and is disclosed as a non-goal), and the three instruments whose answers are quoted as "what the engine would do on this bucket" run at it: RealListingRunTest, the corpus sweep, SingleLegRunTest. The synthetic benches stay at MEASURED_TAIL_LATENCY, whose javadoc now says what it is — a bench regime, not the live store's ratio — because their pinned tables were taken there and a bench number states its regime. ProbeToPageRatioTailTest pins the mechanism on the module's own nara-shaped fixture (the leaf-concentrated bench at a full 1,000-key page, eight workers, the promoted arm, four seeds): the heavy leaf drains as one range that emits every one of its 400,000 keys itself, 668 steal attempts against it produce one child that carries no keys, proposal loss reads 0.9938, the run publishes 18 splits and spends 49% of itself serial. The same fixture, seeds and arm at the bench ratio fail every one of those thresholds (victim share 0.40-0.57, 1,001 keys stolen off it, loss 0.76-0.82, 39-61 splits, serial 0.09-0.21), which is the falsification the leg exists for. E-11's window-in-pages threshold is reported as not-computable rather than approximated: a run records steal.split for a race the thief wins and nothing for the scan that opened an attempt, so there is no per-attempt bracket to take a median over without new executor plumbing. The occupancy walk that finds which range held the fleet moves to SimTrace, shared with the real-listing decomposition rather than copied — two walks over the same claim/complete stream that disagreed would be describing two different runs. The README's claim that race loss is a property of the keyspace and not of the declared timings is corrected: the two regimes it generalised from both price probes at 0.32, so what they held fixed was the ratio.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds ChangesDecision trace instrumentation
Simulator integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
swath-sim/src/main/java/io/varve/swath/sim/executor/SimExecutor.java (1)
398-415: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
gateDump.close()failure infinallycan mask the run's real failure.If
kernel.run()throws andgateDump.close()(in thefinally) also throwsUncheckedIOException, the close failure replaces the original exception per Javafinallysemantics — the opposite of the comment's stated intent that a failing run's dump is "most worth reading" for diagnosis. The original failure reason is lost.🐛 Proposed fix: preserve the primary failure, suppress the close failure
- SimRunResult result; - try { - result = kernel.run(); - } finally { - // Closed on the failing path too: a run that died holding half its dump is the run whose - // dump is most worth reading. - if (gateDump != null) { - gateDump.close(); - } - } + SimRunResult result; + RuntimeException primary = null; + try { + result = kernel.run(); + } catch (RuntimeException failure) { + primary = failure; + throw failure; + } finally { + // Closed on the failing path too: a run that died holding half its dump is the run whose + // dump is most worth reading. A close failure must not mask the run's own failure. + if (gateDump != null) { + try { + gateDump.close(); + } catch (RuntimeException closing) { + if (primary != null) { + primary.addSuppressed(closing); + } else { + throw closing; + } + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@swath-sim/src/main/java/io/varve/swath/sim/executor/SimExecutor.java` around lines 398 - 415, Update execute() so a gateDump.close() failure cannot replace the exception from kernel.run(). Preserve the primary failure and attach any close-time UncheckedIOException as a suppressed exception; when kernel.run() succeeds, continue propagating a close failure normally. Keep the existing cleanup behavior and gateDump null check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@swath-core/src/main/java/io/varve/swath/observability/NoopTraceSink.java`:
- Around line 37-46: Add an in-method comment to both empty overrides,
ownerSplitDecision and victimScan, stating that TraceSink.NONE intentionally
discards all trace events; leave their no-op behavior unchanged.
In
`@swath-core/src/test/java/io/varve/swath/engine/WorkStealingScanConstructorDefaultsTest.java`:
- Around line 432-441: Document the intentional no-op implementations of
ownerSplitDecision and victimScan in the test sink, stating that it only
observes lifecycle events. Add the documentation directly to these overrides
without changing their signatures or behavior.
In `@swath-sim/src/main/java/io/varve/swath/sim/executor/SimGateDump.java`:
- Around line 176-193: Update appendKey to decode key bytes with a strict UTF-8
decoder that reports malformed or unmappable input instead of replacing it with
U+FFFD. Preserve the existing tab/newline rejection and successful text output
for valid UTF-8 keys, while propagating a clear failure for invalid byte
sequences consistent with SimGateDump’s fail-loudly contract.
---
Outside diff comments:
In `@swath-sim/src/main/java/io/varve/swath/sim/executor/SimExecutor.java`:
- Around line 398-415: Update execute() so a gateDump.close() failure cannot
replace the exception from kernel.run(). Preserve the primary failure and attach
any close-time UncheckedIOException as a suppressed exception; when kernel.run()
succeeds, continue propagating a close failure normally. Keep the existing
cleanup behavior and gateDump null check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e219e3ca-86e7-4800-b252-7c46f0c9f305
📒 Files selected for processing (49)
docs/internals/metrics-internals.mddocs/ops/dev/decision-trace-goldens.mdswath-core/src/main/java/io/varve/swath/engine/OwnerSelfSplit.javaswath-core/src/main/java/io/varve/swath/engine/Thief.javaswath-core/src/main/java/io/varve/swath/engine/WorkStealingScan.javaswath-core/src/main/java/io/varve/swath/engine/policy/Carve.javaswath-core/src/main/java/io/varve/swath/engine/policy/NoVictim.javaswath-core/src/main/java/io/varve/swath/engine/policy/OwnerSplitDecision.javaswath-core/src/main/java/io/varve/swath/engine/policy/OwnerSplitGateInputs.javaswath-core/src/main/java/io/varve/swath/engine/policy/OwnerSplitGovernor.javaswath-core/src/main/java/io/varve/swath/engine/policy/Selected.javaswath-core/src/main/java/io/varve/swath/engine/policy/Selection.javaswath-core/src/main/java/io/varve/swath/engine/policy/Skip.javaswath-core/src/main/java/io/varve/swath/engine/policy/ThiefPolicy.javaswath-core/src/main/java/io/varve/swath/engine/policy/VictimScan.javaswath-core/src/main/java/io/varve/swath/observability/JsonlTraceSink.javaswath-core/src/main/java/io/varve/swath/observability/NoopTraceSink.javaswath-core/src/main/java/io/varve/swath/observability/TraceSink.javaswath-core/src/test/java/io/varve/swath/engine/DecisionTraceGoldenTest.javaswath-core/src/test/java/io/varve/swath/engine/RecordingTraceSink.javaswath-core/src/test/java/io/varve/swath/engine/WorkStealingScanConstructorDefaultsTest.javaswath-core/src/test/java/io/varve/swath/engine/WorkStealingScanTraceTest.javaswath-core/src/test/java/io/varve/swath/engine/policy/OwnerSplitGovernorTest.javaswath-core/src/test/java/io/varve/swath/engine/policy/ThiefPolicySelectionTest.javaswath-core/src/test/java/io/varve/swath/observability/TraceSinkTest.javaswath-core/src/test/resources/goldens/decision-trace/deep-narrow.jsonlswath-core/src/test/resources/goldens/decision-trace/explosion-1to1.jsonlswath-core/src/test/resources/goldens/decision-trace/flat-wide.jsonlswath-core/src/test/resources/goldens/decision-trace/partition-key-value.jsonlswath-core/src/test/resources/goldens/decision-trace/thief-cascade-mechanisms.jsonlswath-core/src/test/resources/goldens/decision-trace/thief-edge-cases.jsonlswath-sim/README.mdswath-sim/build.gradle.ktsswath-sim/src/main/java/io/varve/swath/sim/executor/EstimatorOwnerSplitPolicy.javaswath-sim/src/main/java/io/varve/swath/sim/executor/EstimatorStealPolicy.javaswath-sim/src/main/java/io/varve/swath/sim/executor/RateAnchoredArm.javaswath-sim/src/main/java/io/varve/swath/sim/executor/RemainingWorkEstimator.javaswath-sim/src/main/java/io/varve/swath/sim/executor/SimExecutor.javaswath-sim/src/main/java/io/varve/swath/sim/executor/SimGateDump.javaswath-sim/src/test/java/io/varve/swath/sim/executor/CorpusSweep.javaswath-sim/src/test/java/io/varve/swath/sim/executor/PolicyRunFixtures.javaswath-sim/src/test/java/io/varve/swath/sim/executor/ProbeToPageRatioTailTest.javaswath-sim/src/test/java/io/varve/swath/sim/executor/RealListingRunTest.javaswath-sim/src/test/java/io/varve/swath/sim/executor/SensingEstimatorTest.javaswath-sim/src/test/java/io/varve/swath/sim/executor/SensingRaceProtocol.javaswath-sim/src/test/java/io/varve/swath/sim/executor/SensingVariantParityTest.javaswath-sim/src/test/java/io/varve/swath/sim/executor/SimGateDumpTest.javaswath-sim/src/test/java/io/varve/swath/sim/executor/SimTrace.javaswath-sim/src/test/java/io/varve/swath/sim/executor/SingleLegRunTest.java
💤 Files with no reviewable changes (2)
- swath-sim/src/main/java/io/varve/swath/sim/executor/EstimatorOwnerSplitPolicy.java
- swath-sim/src/main/java/io/varve/swath/sim/executor/EstimatorStealPolicy.java
…8 keys in the gate dump Sonar's new-code quality gate flagged the two newly added TraceSink no-op overrides (ownerSplitDecision/victimScan) in NoopTraceSink and the constructor- defaults test's RecordingTraceSink, since new empty method bodies need an in-method justification comment even though the surrounding pre-existing no-ops (predating the gate) already establish this pattern implicitly via a class-level comment. Add the same in-method comment style to both. SimGateDump.appendKey used new String(key, UTF_8), which silently substitutes U+FFFD for malformed byte sequences instead of failing -- undermining the dump's stated purpose of byte-exact diffing against a replay trace, and inconsistent with the class's fail-loudly-or-not-at-all contract (it already refuses tab/CR/LF for exactly this reason). Decode with a strict CharsetDecoder that reports malformed/unmappable input and throw an IllegalStateException with the hex-encoded key instead. Add a regression test covering a malformed-UTF-8 key.
This was referenced Jul 28, 2026
Merged
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.
Makes the simulator honest about serial tails, and gives both instruments per-decision gate visibility.
Why
The sensing port's live A/B left one divergence: the sim promised a full cure of nara-1950-census's ~5.5M-key serial tail (~850 owner children); the real engine — live and on the replay server — stalls at ~385 splits with the tail intact. Racing tail cures on a simulator that invents cures would produce validated-but-fake fixes, so this branch closes the honesty gap first.
What
--trace): two new JSONL events —owner_split_decision(terminal gate reason plus the inputs the chain read: est, pages-since-carve, outstanding, far-ahead fraction, density ratio) andvictim_scan(per-scan skip tallies + refusal discriminator). Policies stay pure; executors emit. Goldens regenerated added-only; metrics-internals §7 updated.EstimatorOwnerSplitPolicy/EstimatorStealPolicyare deleted;SimExecutorconstructs the engine's ownOwnerSplitGovernor/ThiefPolicyfor every sensing arm through the estimator seam (null → WINDOW for CURRENT, bit-identical by construction). The mirror-drift class is structurally gone. Parity test rebuilt: per-arm battery determinism, CURRENT-vs-unsteered identity, and a per-arm structural-zero exposure pin at the observed-mass floor.swath.sim.gate-dump): opt-in per-run TSVs of every owner-split decision's inputs and every victim scan, for diffing sim runs against replay traces. Plus a property-driven single-leg run harness.ProbeToPageRatioTailTestpins the honest-stall mechanism at all four protocol seeds and fails against the old profile.Evidence chain
Replay reproduction → per-decision tail diagnosis (owner blocked 5,326/5,326 by the reflect floor's zeroed window term; thief loses every evaluated pivot race) → mirror deletion (fantasy persisted ⇒ world model indicted) → per-decision input subtraction (density model exonerated) → attempt-anatomy measurement with a two-point latency-scale fit (probe:page ratio isolated as the sole gap; live window ≈ 2.0 owner pages) → ratio fix → acceptance sweep: promoted-arm rows now land at 373–429 splits against the engine truth of 383–399, ~99% proposal loss, serial ~0.95. CURRENT now collapses at 4/4 seeds, matching the live 4/4 persistence (the old 2/4 "coin flip" was an artifact of the wrong ratio).
Every commit was independently reviewed (SHIP each). Engine behavior is unchanged on all default paths: the swath-core changes are trace-only observability; goldens changed added-only.
Summary by CodeRabbit
New Features
victim_scantrace events with victim-selection counts, outcomes, and estimates.owner_split_decisionevents with gate outcomes and decision metrics.nullvalues.Documentation
Tests