Run swath's real listing policies against a fixture in virtual time - #52
Merged
Conversation
…ecision tapes per mechanism Three things the policy layer needs from the model and kernel before it can be wired, each with a consequence if it is skipped. A page's client-side cost is not one quantity. Measurement of a real client found it split across stages that behave differently under load: the fetch worker's own conversion work (parallel, per page), the durability commit (one serial writer every page waits on), the output sink (another serial stage whose service rate is a genuine ceiling), and, for a columnar sink, an encode pool that runs off the page's critical path. CompositeClientCost charges the first three in series on the page's own timeline and the fourth in parallel, which is the order the engine does them in; charging them in parallel instead would let a simulated fleet emit pages faster than a real client can absorb, which is the impossible strategy a client-cost term exists to rule out. Two stages have a mean several times their median, so those are sampled from their measured quantiles rather than averaged, through the client-cost tape. Timeouts and windows are inputs. The adaptive controller's growth pace, timeout windows, shed window bounds, latency-baseline decay and valve pace join the budgets a scenario declares, as do the transient-retry ceilings and the backoff between retries. A fixed duration only means something relative to the latencies it bounds, and that ratio is exactly what a scaled-down real-time reproduction loses. The decision streams split per mechanism, not per purpose. Ranking two variants of one mechanism is what a sweep is for, and a shared decision tape makes that ranking a function of every other mechanism's draw count. Fleet-wide instruments draw on a reserved actor id of their own, distinct from the no-actor id, and a draw taken outside any event body is now refused rather than minting a tape nobody owns. The latency SPI gains an occupancy-aware overload so a store whose latency rises with the number of calls in flight can be stated at all, and the event cap is restated as events dispatched, invalidated ones included.
The simulator's kernel could drive a store; it could not drive a policy. This adds the executor half of the policy seam, so swath's actual decision code -- the seed planner, the owner-side split governor, the thief's victim selection and pivot cascade, the idle-steal pacing -- runs against a ground-truth fixture on a virtual clock, at a fixed seed, and reports what it did. Nothing is reimplemented except the adaptive-concurrency controller, which the seam defines as a port for a reason: it is the most timing-coupled code in the engine, and carving it out from under its concurrent callers would have been a larger risk than writing an equivalent whose every signal carries its own timestamp. That one is a reimplementation and is treated as one -- reviewed against the controller's documented guarantees, pinned by shape tests at the exact window boundaries, and a change to either is a change to both. The ordering is the substance. One event body is one atomic region, so the page commit -- trim to the current bound, advance the cursor, fold the page into the density digest, run the owner-split decision -- is one body, which is the region the engine holds the worker's lock across. A steal is deliberately the opposite: the victim's cursor is read in one body, the probes resolve in later ones, and the proposal is re-validated against the victim as it stands at the end. So a simulated steal can lose the race a real steal loses, and a test fails if the re-validation is removed. A timeout that pre-empts an in-flight call has to retire that call's response without cancellation, which the kernel does not have. Where the completion instant is known at issue the executor schedules one event or the other and a timeout costs nothing extra; where it is not, both are armed and the loser is counted rather than absorbed, because it is charged against the run's event budget like any other event. Four keyspace fixtures shaped like real buckets -- a date-partitioned archive, a hash-fanned corpus, one object per directory, and a single dense flat leaf -- complete end to end under the real policies, emitting every key exactly once.
… run costs A sweep is the point of the simulator, and reusing an open handle across its legs is the only way it is affordable -- opening a large fixture costs more than a run does. That reuse invites two mistakes, both of which produce plausible numbers rather than errors, so both are closed here: a leg gets a freshly supplied client-cost model (a stateful one left mid-service by a leg that hit a ceiling would carry that queue into the next), and store meters are reported per leg as a delta rather than as the cumulative reading every leg after the first would see. The invariants are restated with policies wired. Where the policy declines to act -- one worker, nothing to steal from, owner-side splitting ablated off -- the arithmetic is exact and asserted as an equality. Where it acts, scaling is monotonic and deliberately not proportional; eight workers finish a flat-leaf fixture in 4.4 s of modelled time against one worker's 12.0 s, and a test asserting eight times faster would be asserting a bug. The adversarial store fixture lands with them: a store whose latency rises with the number of calls in flight, which is the only thing that exercises the adaptive controller's latency-freeze rung, together with the control run proving an ordinary store of the same shape never freezes. The freeze holds growth without ever lowering the target and without stalling the run, and the fixture says so rather than only checking that a counter fired. The ambient-source guard now also scans the engine classes a policy run executes, not just this module's own sources: an ambient clock read in the decision path would break the determinism claim exactly as one here would, while being invisible to a check that only looked here. One read is allowed and named -- a range's own creation instant, for a diagnostic drain-rate estimate that no decision reads -- and the allowance is exact, so a second one fails the test. Measured, on one development box: a policy run dispatches ~15.6 kernel events per modelled store call at ~117 us per call including page materialisation, so a 150,000-call run is ~2.3M events and tens of seconds rather than the minutes that would put sweeping out of reach. The bench that produces it is opt-in, and the numbers are in the module README with the two caveats that travel with them.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 45 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 (15)
📝 WalkthroughWalkthroughAdds a virtual-time policy executor with adaptive concurrency simulation, composite client-cost and occupancy-aware latency models, immutable scenario/result APIs, multi-leg sweeps, generated fixtures, ordering documentation, and extensive tests for execution, determinism, retries, timeouts, splitting, and concurrency behavior. ChangesPolicy simulation stack
Estimated code review effort: 5 (Critical) | ~120 minutes 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: 9
🧹 Nitpick comments (2)
swath-sim/src/main/java/io/varve/swath/sim/executor/SimSweep.java (1)
94-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
TimeUnitrather than qualifying it inline.Every other type in this file is imported at the top. As per coding guidelines, "Match surrounding code style, naming, and structure".
♻️ Proposed change
- values.merge(name + ".total_nanos", timer.totalTime(java.util.concurrent.TimeUnit.NANOSECONDS), - Double::sum); + values.merge(name + ".total_nanos", timer.totalTime(TimeUnit.NANOSECONDS), Double::sum);plus
import java.util.concurrent.TimeUnit;alongside the otherjava.utilimports.🤖 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/SimSweep.java` around lines 94 - 95, Import java.util.concurrent.TimeUnit with the other imports in SimSweep, then update the timer.totalTime call in the values.merge statement to use the imported TimeUnit.NANOSECONDS instead of the fully qualified name.Source: Coding guidelines
swath-sim/src/main/java/io/varve/swath/sim/executor/SimExecutor.java (1)
592-608: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew decision paths emit ad-hoc counter strings instead of the engagement seam.
STEAL.in_flight_denied,STEAL.paced_denied,OWNER_SPLIT.self_abortedand friends are inline literals, while every policy-returned path goes throughrecordEngagements. As per coding guidelines, every new algorithm path — including pacing and backoff branches — must emit arecordStealReason(category, reason)engagement counter, and duplicated magic strings should be avoided. Consider routing these through the same category/reason constants the engine uses.Also applies to: 534-534, 555-555
🤖 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 592 - 608, Route the new denial and abort branches in SimExecutor, including the visible steal-in-flight and pacing paths plus the referenced OWNER_SPLIT paths, through recordStealReason(category, reason) instead of inline ctx.count strings. Reuse the engine’s existing category and reason constants, and remove duplicated magic counter names while preserving each branch’s current control flow and parking behavior.Source: Coding guidelines
🤖 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-sim/src/main/java/io/varve/swath/sim/executor/SimConcurrencyPolicy.java`:
- Around line 193-204: Replace all zero-valued timestamp sentinels in
SimConcurrencyPolicy with explicit initialization state or Long.MIN_VALUE,
covering transientWindowStartNanos, shedWindowStartNanos, and lastThrottleNanos.
Update rollShedWindowIfElapsed, onTransientTimeout, multiplicativeDecrease,
onSuccess, and maybeShed so legitimate events at t=0 follow normal window,
threshold, cooldown, and fired-flag behavior.
In `@swath-sim/src/main/java/io/varve/swath/sim/executor/SimExecutor.java`:
- Line 9: Remove the unused EngineToggles import from SimExecutor, since the
implementation only accesses scenario.toggles() and does not reference the
imported type. Run the swath-sim Spotless formatter to verify the format gate
passes.
- Around line 711-731: Update probeRetry to reuse the same capped retry handler
as retryCall instead of unconditionally calling finishSteal on timeout. Pass the
incremented attempt number when scheduling the retry so probeRetry continues
through the declared probeAttemptRetryCap and only fails after the cap is
reached; preserve the existing timeout accounting and probe-specific recording.
In `@swath-sim/src/main/java/io/varve/swath/sim/executor/SimListingView.java`:
- Around line 72-87: Update the page and probeNonEmpty read paths in
SimListingView so a null startAfter is replaced with scanPrefix and the initial
scan is inclusive; preserve the existing exclusive behavior for non-null
cursors. Apply the same anchoring consistently to both methods so leading ranges
cannot read keys outside the configured prefix.
In `@swath-sim/src/main/java/io/varve/swath/sim/model/ClientCostModel.java`:
- Line 40: Update the counter contract defined by ClientCostModel and all
permitted implementations— IidClientCost, ContendedClientCost, and
CompositeClientCost—so counters remain comparable across all three cost forms.
Replace any wording or contract assumptions that refer only to two forms while
preserving the existing counter behavior.
In `@swath-sim/src/main/java/io/varve/swath/sim/model/CompositeClientCost.java`:
- Around line 156-161: Update the offload submission in the client-cost flow
around offloadServer.submit so queued offload completions do not keep the kernel
active after the sink completes. Treat offload work as detached/background
accounting or make run completion use the sink boundary, ensuring offload events
cannot extend virtualNanos() or trigger MAX_DURATION/EVENT_CAP.
In
`@swath-sim/src/main/java/io/varve/swath/sim/model/OccupancyScaledLatencyModel.java`:
- Around line 68-70: Update the latency calculation in
OccupancyScaledLatencyModel’s drawNanos method to prevent perInFlightNanos
multiplied by inFlight from overflowing before applying the ceiling. Compute the
remaining ceiling after drawn, compare the occupancy increment against it before
multiplication, and return ceilingNanos when the increment would exceed the
remaining capacity; otherwise perform the multiplication and preserve the
existing capped result.
In `@swath-sim/src/main/java/io/varve/swath/sim/model/SampledClientCostTerm.java`:
- Around line 73-84: Update the quantile-position validation in
SampledClientCostTerm to reject non-finite fractions, including NaN, before
applying the existing (0, 1) range and ascending checks. Preserve the current
validation and exception behavior for finite values.
In `@swath-sim/src/test/java/io/varve/swath/sim/fixture/ListingFixtureStore.java`:
- Around line 39-46: Update the ListingFixtureStore constructor to defensively
copy every byte[] before storing the keys, while retaining the existing
unsigned-order validation. Ensure the copied key arrays are used for the
immutable keys field so callers cannot mutate the fixture’s sorted-key invariant
after construction.
---
Nitpick comments:
In `@swath-sim/src/main/java/io/varve/swath/sim/executor/SimExecutor.java`:
- Around line 592-608: Route the new denial and abort branches in SimExecutor,
including the visible steal-in-flight and pacing paths plus the referenced
OWNER_SPLIT paths, through recordStealReason(category, reason) instead of inline
ctx.count strings. Reuse the engine’s existing category and reason constants,
and remove duplicated magic counter names while preserving each branch’s current
control flow and parking behavior.
In `@swath-sim/src/main/java/io/varve/swath/sim/executor/SimSweep.java`:
- Around line 94-95: Import java.util.concurrent.TimeUnit with the other imports
in SimSweep, then update the timer.totalTime call in the values.merge statement
to use the imported TimeUnit.NANOSECONDS instead of the fully qualified name.
🪄 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: c851e725-d58e-4fa7-99ac-ff19e0fe58db
📒 Files selected for processing (41)
swath-sim/README.mdswath-sim/docs/executor-ordering.mdswath-sim/src/main/java/io/varve/swath/sim/executor/PolicyRunResult.javaswath-sim/src/main/java/io/varve/swath/sim/executor/PolicyScenario.javaswath-sim/src/main/java/io/varve/swath/sim/executor/SimConcurrencyPolicy.javaswath-sim/src/main/java/io/varve/swath/sim/executor/SimExecutor.javaswath-sim/src/main/java/io/varve/swath/sim/executor/SimListingView.javaswath-sim/src/main/java/io/varve/swath/sim/executor/SimNodeLedger.javaswath-sim/src/main/java/io/varve/swath/sim/executor/SimSweep.javaswath-sim/src/main/java/io/varve/swath/sim/kernel/SimKernel.javaswath-sim/src/main/java/io/varve/swath/sim/kernel/SimRng.javaswath-sim/src/main/java/io/varve/swath/sim/kernel/SimRngStream.javaswath-sim/src/main/java/io/varve/swath/sim/kernel/SimRunResult.javaswath-sim/src/main/java/io/varve/swath/sim/model/ClientCostModel.javaswath-sim/src/main/java/io/varve/swath/sim/model/CompositeClientCost.javaswath-sim/src/main/java/io/varve/swath/sim/model/EngineTimeBudgets.javaswath-sim/src/main/java/io/varve/swath/sim/model/LatencyModel.javaswath-sim/src/main/java/io/varve/swath/sim/model/MeasuredClientCost.javaswath-sim/src/main/java/io/varve/swath/sim/model/OccupancyScaledLatencyModel.javaswath-sim/src/main/java/io/varve/swath/sim/model/SampledClientCostTerm.javaswath-sim/src/test/java/io/varve/swath/sim/SimAmbientSourceGuardTest.javaswath-sim/src/test/java/io/varve/swath/sim/driver/ClientCostFormsTest.javaswath-sim/src/test/java/io/varve/swath/sim/driver/ConcurrencyScalingTest.javaswath-sim/src/test/java/io/varve/swath/sim/driver/EventLogDeterminismTest.javaswath-sim/src/test/java/io/varve/swath/sim/driver/ExactModeInvariantsTest.javaswath-sim/src/test/java/io/varve/swath/sim/driver/KeyListStore.javaswath-sim/src/test/java/io/varve/swath/sim/executor/ConcurrencyPoisonTest.javaswath-sim/src/test/java/io/varve/swath/sim/executor/ContendedStoreTimeoutTest.javaswath-sim/src/test/java/io/varve/swath/sim/executor/PolicyInvariantsTest.javaswath-sim/src/test/java/io/varve/swath/sim/executor/PolicyRunBudgetBenchTest.javaswath-sim/src/test/java/io/varve/swath/sim/executor/PolicyRunEndToEndTest.javaswath-sim/src/test/java/io/varve/swath/sim/executor/PolicyRunFixtures.javaswath-sim/src/test/java/io/varve/swath/sim/executor/SimConcurrencyPolicyTest.javaswath-sim/src/test/java/io/varve/swath/sim/executor/SimSweepTest.javaswath-sim/src/test/java/io/varve/swath/sim/executor/SnapshotToCasFootraceTest.javaswath-sim/src/test/java/io/varve/swath/sim/fixture/KeyspaceFixtures.javaswath-sim/src/test/java/io/varve/swath/sim/fixture/ListingFixtureStore.javaswath-sim/src/test/java/io/varve/swath/sim/kernel/SimKernelTest.javaswath-sim/src/test/java/io/varve/swath/sim/model/CompositeClientCostTest.javaswath-sim/src/test/java/io/varve/swath/sim/model/EngineTimeBudgetsTest.javaswath-sim/src/test/java/io/varve/swath/sim/model/SampledClientCostTermTest.java
💤 Files with no reviewable changes (1)
- swath-sim/src/test/java/io/varve/swath/sim/driver/KeyListStore.java
Comment on lines
+72
to
+87
| Page page(byte[] startAfter, int maxKeys) { | ||
| List<byte[]> keys = read(startAfter, false, scanCeiling, maxKeys); | ||
| return new Page(keys, keys.size() == maxKeys); | ||
| } | ||
|
|
||
| /** | ||
| * The one-key speculative probe: is there a key after {@code startAfter} that is still at or below | ||
| * {@code hi}? An open {@code hi} accepts any key the probe finds. | ||
| */ | ||
| boolean probeNonEmpty(byte[] startAfter, byte[] hi) { | ||
| List<byte[]> keys = read(startAfter, false, scanCeiling, 1); | ||
| if (keys.isEmpty()) { | ||
| return false; | ||
| } | ||
| return hi == null || KeyBytes.compareUnsigned(keys.getFirst(), hi) <= 0; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
page(null, …) escapes the scan prefix.
read(startAfter, false, scanCeiling, maxKeys) passes from == null straight through, so the very first page of a range whose lo is null starts at the store's first key rather than at scanPrefix. SimExecutor.seedRanges creates exactly that range (byte[] lo = null; for the leading seed), so any fixture with a non-empty scanPrefix will list and emit keys below the prefix. Contrast rollup, which correctly anchors at prefix with fromInclusive = true.
probeNonEmpty has the same null-startAfter path, though callers pass a pivot today.
🐛 Anchor the null cursor at the scan prefix
Page page(byte[] startAfter, int maxKeys) {
- List<byte[]> keys = read(startAfter, false, scanCeiling, maxKeys);
+ List<byte[]> keys = startAfter == null
+ ? read(scanPrefix, true, scanCeiling, maxKeys)
+ : read(startAfter, false, scanCeiling, maxKeys);
return new Page(keys, keys.size() == maxKeys);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Page page(byte[] startAfter, int maxKeys) { | |
| List<byte[]> keys = read(startAfter, false, scanCeiling, maxKeys); | |
| return new Page(keys, keys.size() == maxKeys); | |
| } | |
| /** | |
| * The one-key speculative probe: is there a key after {@code startAfter} that is still at or below | |
| * {@code hi}? An open {@code hi} accepts any key the probe finds. | |
| */ | |
| boolean probeNonEmpty(byte[] startAfter, byte[] hi) { | |
| List<byte[]> keys = read(startAfter, false, scanCeiling, 1); | |
| if (keys.isEmpty()) { | |
| return false; | |
| } | |
| return hi == null || KeyBytes.compareUnsigned(keys.getFirst(), hi) <= 0; | |
| } | |
| Page page(byte[] startAfter, int maxKeys) { | |
| List<byte[]> keys = startAfter == null | |
| ? read(scanPrefix, true, scanCeiling, maxKeys) | |
| : read(startAfter, false, scanCeiling, maxKeys); | |
| return new Page(keys, keys.size() == maxKeys); | |
| } | |
| /** | |
| * The one-key speculative probe: is there a key after {`@code` startAfter} that is still at or below | |
| * {`@code` hi}? An open {`@code` hi} accepts any key the probe finds. | |
| */ | |
| boolean probeNonEmpty(byte[] startAfter, byte[] hi) { | |
| List<byte[]> keys = read(startAfter, false, scanCeiling, 1); | |
| if (keys.isEmpty()) { | |
| return false; | |
| } | |
| return hi == null || KeyBytes.compareUnsigned(keys.getFirst(), hi) <= 0; | |
| } |
🤖 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/SimListingView.java`
around lines 72 - 87, Update the page and probeNonEmpty read paths in
SimListingView so a null startAfter is replaced with scanPrefix and the initial
scan is inclusive; preserve the existing exclusive behavior for non-null
cursors. Apply the same anchoring consistently to both methods so leading ranges
cannot read keys outside the configured prefix.
| * being assumed. | ||
| */ | ||
| public sealed interface ClientCostModel permits IidClientCost, ContendedClientCost { | ||
| public sealed interface ClientCostModel permits IidClientCost, ContendedClientCost, CompositeClientCost { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the counter contract for all three cost forms.
Permitting CompositeClientCost makes the later “across the two forms” wording stale; counters must remain comparable across every implementation.
🤖 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/model/ClientCostModel.java` at
line 40, Update the counter contract defined by ClientCostModel and all
permitted implementations— IidClientCost, ContendedClientCost, and
CompositeClientCost—so counters remain comparable across all three cost forms.
Replace any wording or contract assumptions that refer only to two forms while
preserving the existing counter behavior.
Comment on lines
+156
to
+161
| long offloadNanos = offload.drawNanos(keys, committed.rng(SimRngStream.CLIENT_COST)); | ||
| committed.count(NANOS_COUNTER, offloadNanos); | ||
| offloadServer.submit(committed, offloadNanos, encoded -> { | ||
| }); | ||
| } | ||
| sinkServer.submit(committed, sinkNanos, onComplete); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep offload draining out of run completion.
These queued offload completions remain in the kernel queue after the sink completes, so they extend virtualNanos() and can turn an otherwise finished listing into MAX_DURATION or EVENT_CAP. Model this as detached/background accounting, or define completion at the sink boundary rather than kernel quiescence.
🤖 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/model/CompositeClientCost.java`
around lines 156 - 161, Update the offload submission in the client-cost flow
around offloadServer.submit so queued offload completions do not keep the kernel
active after the sink completes. Treat offload work as detached/background
accounting or make run completion use the sink boundary, ensuring offload events
cannot extend virtualNanos() or trigger MAX_DURATION/EVENT_CAP.
Comment on lines
+68
to
+70
| long drawn = base.drawNanos(callClass, rng); | ||
| long inflated = drawn + perInFlightNanos * Math.max(0, inFlight); | ||
| return Math.min(ceilingNanos, inflated); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Saturate before occupancy arithmetic overflows.
perInFlightNanos * inFlight can wrap before the ceiling is applied, producing a negative latency and failing scheduling. Compare against the remaining ceiling before multiplying, and return ceilingNanos when the increment would exceed it.
🤖 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/model/OccupancyScaledLatencyModel.java`
around lines 68 - 70, Update the latency calculation in
OccupancyScaledLatencyModel’s drawNanos method to prevent perInFlightNanos
multiplied by inFlight from overflowing before applying the ceiling. Compute the
remaining ceiling after drawn, compare the occupancy increment against it before
multiplication, and return ceilingNanos when the increment would exceed the
remaining capacity; otherwise perform the multiplication and preserve the
existing capped result.
Comment on lines
+73
to
+84
| for (int i = 0; i < fractions.length; i++) { | ||
| if (fractions[i] <= 0.0 || fractions[i] >= 1.0) { | ||
| throw new IllegalArgumentException("quantile positions must lie strictly inside (0, 1), got " | ||
| + fractions[i]); | ||
| } | ||
| if (nanos[i] < 0) { | ||
| throw new IllegalArgumentException("a quantile value must be >= 0, got " + nanos[i]); | ||
| } | ||
| if (i > 0 && (fractions[i] <= fractions[i - 1] || nanos[i] < nanos[i - 1])) { | ||
| throw new IllegalArgumentException("a quantile ladder must ascend in both position and " | ||
| + "value; position " + i + " does not"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-finite quantile positions.
NaN passes both the range and ascending checks, despite violating the documented (0, 1) contract. It then yields invalid interpolation behavior.
Proposed fix
- if (fractions[i] <= 0.0 || fractions[i] >= 1.0) {
+ if (!Double.isFinite(fractions[i])
+ || fractions[i] <= 0.0 || fractions[i] >= 1.0) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (int i = 0; i < fractions.length; i++) { | |
| if (fractions[i] <= 0.0 || fractions[i] >= 1.0) { | |
| throw new IllegalArgumentException("quantile positions must lie strictly inside (0, 1), got " | |
| + fractions[i]); | |
| } | |
| if (nanos[i] < 0) { | |
| throw new IllegalArgumentException("a quantile value must be >= 0, got " + nanos[i]); | |
| } | |
| if (i > 0 && (fractions[i] <= fractions[i - 1] || nanos[i] < nanos[i - 1])) { | |
| throw new IllegalArgumentException("a quantile ladder must ascend in both position and " | |
| + "value; position " + i + " does not"); | |
| } | |
| for (int i = 0; i < fractions.length; i++) { | |
| if (!Double.isFinite(fractions[i]) | |
| || fractions[i] <= 0.0 || fractions[i] >= 1.0) { | |
| throw new IllegalArgumentException("quantile positions must lie strictly inside (0, 1), got " | |
| fractions[i]); | |
| } | |
| if (nanos[i] < 0) { | |
| throw new IllegalArgumentException("a quantile value must be >= 0, got " + nanos[i]); | |
| } | |
| if (i > 0 && (fractions[i] <= fractions[i - 1] || nanos[i] < nanos[i - 1])) { | |
| throw new IllegalArgumentException("a quantile ladder must ascend in both position and " | |
| "value; position " + i + " does not"); | |
| } |
🤖 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/model/SampledClientCostTerm.java`
around lines 73 - 84, Update the quantile-position validation in
SampledClientCostTerm to reject non-finite fractions, including NaN, before
applying the existing (0, 1) range and ascending checks. Preserve the current
validation and exception behavior for finite values.
Comment on lines
+39
to
+46
| public ListingFixtureStore(List<byte[]> keys) { | ||
| for (int i = 1; i < keys.size(); i++) { | ||
| if (Arrays.compareUnsigned(keys.get(i - 1), keys.get(i)) >= 0) { | ||
| throw new IllegalArgumentException("fixture keys must ascend in unsigned byte order; " | ||
| + "entry " + i + " does not"); | ||
| } | ||
| } | ||
| this.keys = List.copyOf(keys); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Defensively copy key bytes at construction.
List.copyOf(keys) retains each mutable byte[]. A caller can mutate a key after sortedness validation, breaking the fixture’s fixed/sorted invariant and invalidating binary-search range results.
Proposed fix
- this.keys = List.copyOf(keys);
+ this.keys = keys.stream()
+ .map(key -> Arrays.copyOf(key, key.length))
+ .toList();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public ListingFixtureStore(List<byte[]> keys) { | |
| for (int i = 1; i < keys.size(); i++) { | |
| if (Arrays.compareUnsigned(keys.get(i - 1), keys.get(i)) >= 0) { | |
| throw new IllegalArgumentException("fixture keys must ascend in unsigned byte order; " | |
| + "entry " + i + " does not"); | |
| } | |
| } | |
| this.keys = List.copyOf(keys); | |
| public ListingFixtureStore(List<byte[]> keys) { | |
| for (int i = 1; i < keys.size(); i++) { | |
| if (Arrays.compareUnsigned(keys.get(i - 1), keys.get(i)) >= 0) { | |
| throw new IllegalArgumentException("fixture keys must ascend in unsigned byte order; " | |
| "entry " + i + " does not"); | |
| } | |
| } | |
| this.keys = keys.stream() | |
| .map(key -> Arrays.copyOf(key, key.length)) | |
| .toList(); |
🤖 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/test/java/io/varve/swath/sim/fixture/ListingFixtureStore.java`
around lines 39 - 46, Update the ListingFixtureStore constructor to defensively
copy every byte[] before storing the keys, while retaining the existing
unsigned-order validation. Ensure the copied key arrays are used for the
immutable keys field so callers cannot mutate the fixture’s sorted-key invariant
after construction.
…e record around it The blocking one: the shipped controller uses zero as "this has never happened" for its pacing timestamps, which works only because it reads a clock counting from process start, where zero is unreachably far in the past. A virtual run starts at zero. Reusing that sentinel inverted its meaning -- the first success of every run was paced out, and the latency-freeze valve, paced at thirty seconds, could never open inside a run shorter than thirty virtual seconds, which is the length of exactly the runs that exercise it. Every timestamp now carries an explicit unarmed value that is never confused with an instant, and two tests drive the first signals at virtual zero; both fail if the sentinel is reverted. The rest are accuracy of the record rather than behaviour, except where noted: - The executor named the wrong disclosed widening. It models the right one -- the per-victim futility cooldown read in the unlocked pool pass and consumed after selection -- and the fleet-wide pacing window is not a widening at all, its arithmetic having moved behind the seam unchanged. The doc now says so, and names the third widening the code already models: the zero-fan-out streak landing one step late. - The owner-split view now carries the raw, pre-toggle density ratio its own contract asks for, read through the one public accessor that returns it untouched. It was equivalent only because the governor happens to re-apply the toggle idempotently, which is not a property to rely on. - Counters take the engine's own names, so a cross-instrument comparison cannot read a silent zero where a real run has a number. - A growth step now hands out as many page-fetch slots as it released, rather than one per completion, which could let the fleet lag its own target on a run where completions are rare; and a call the client timed out on keeps occupying the store until the store finishes it, which is the one regime an occupancy-sensitive latency model exists for. - What a fetch does when its transient retries run out is a declared input with the shipped default, because the two dispositions end a run differently and a timeout-heavy leg ending stuck is the bounded one's meaning, not an artefact. - The sampled term's truncation bias is disclosed in both directions with the computed figure (+8.8% on the worker term's drawn mean), and pinned by a test; the measured constants cite the span and band each came from. - The tiling invariant is asserted as intervals rather than as a total, since a gap and an overlap of equal size cancel in a count. - The cost measurement gained a second size point, which showed events per call is not invariant: park timers track idleness rather than calls, so the figure falls as a fixture grows. The extrapolation is weakened accordingly, and the byte-identity claim is qualified to traces that fit in memory with the reason the rolling digest stays deferred.
…ition, trace formatting Five small ones, none behavioural except the last. The latency baseline's decay boundary was the one place still doing arithmetic against a field initialised to the unarmed sentinel. It is unreachable -- the first sample arms it on the way past -- but the class claims every read checks first, and a claim with one exception is a claim a later change can quietly break, so it checks first. An inline comment still described the fleet-wide idle-steal pacing check as the extraction's widened shape, contradicting the class javadoc and the ordering doc that were corrected around it. That arithmetic moved behind the seam unchanged and is still consulted under one monitor; it is not a widening. A run record printed its stop reason without the disposition that produced it, which is exactly the pair a reader needs together: a timeout-heavy leg that ends stuck under the bounded disposition is that disposition's own meaning, and indistinguishable from a defect without it. The per-page trace entry hex-encodes two keys, and it did so even with the trace disabled, which is how a sweep runs. It is now behind the recording check -- the only trace site here worth guarding, since the others format nothing -- and the bytes are identical when the trace is on. Every measured constant now says why that point of its band was taken: the uncontended end where the model produces contention itself and taking a contended figure would count it twice, the midpoint where the band is a spread across arms with no trend to prefer an end. The columnar dispatch constant moves to its band's midpoint accordingly.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
The simulator could drive a store; it could not drive a policy. This wires swath's
real listing policies into it — the seed planner, the owner-side split governor,
the thief's victim selection and pivot cascade, the idle-steal pacing — so a run
answers "what would this policy have done on this bucket, and how long would it have
taken", reproducibly, at a fixed seed, in virtual time.
Nothing is reimplemented except the adaptive-concurrency controller, which the policy
seam defines as a port rather than an extraction for good reason: it is the most
timing-coupled code in the engine, and carving it out from under its concurrent
callers would have been a larger risk than writing an equivalent whose every signal
carries its own timestamp. That one is treated as a reimplementation — reviewed
against the controller's own documented guarantees, pinned by shape tests at the exact
window boundaries, and a change to either is a change to both.
The ordering is the substance
One event body is one atomic region, so the page commit — trim to the current bound,
advance the cursor, fold the page into the density digest, run the owner-split
decision — is one body, which is exactly the region the engine holds a worker's lock
across. A steal is deliberately the opposite: the victim's cursor is read in one body,
the probes resolve in later ones, and the proposal is re-validated against the victim
as it stands at the end. So a simulated steal can lose the race a real steal loses,
and a test fails if the re-validation is removed. Full ordering contract, including the
two disclosed timing widenings the executor models:
swath-sim/docs/executor-ordering.md.What a page costs the client
Charging a page one number is the mistake this avoids. Measurement found the per-page
cost split across stages that behave differently under load — the worker's own
conversion work (parallel), the durability commit (one serial writer every page waits
on), the sink (another serial stage whose service rate is a real ceiling), plus a
columnar sink's encode pool (parallel, off the critical path). The composite charges
the first three in series and the fourth in parallel, because that is the order the
engine does them in; the other way round would let a simulated fleet emit pages faster
than a real client can absorb. Two stages have a mean several times their median, so
those are sampled from measured quantiles rather than averaged.
Timeouts, and what a cancelled timer costs
The kernel has no cancellation. Where a call's completion instant is known at issue,
the executor schedules the response or the timeout, so a timeout costs no extra
event. Where it is not — a modelled store with a queue — both are armed and the loser
is counted (
events.stale) rather than absorbed, because it is charged against therun's event budget like everything else.
Results
Four keyspace fixtures shaped like real buckets complete end to end under the real
policies, emitting every key exactly once, with the phase shape the design predicts
(seed, fan-out, steal tail) and the policy paths firing: far-ahead and flat-leaf and
adaptive-structure pivots, owner-side carves with the reflection clamp and lift,
demand gating, futility pacing, and steals that legitimately fail.
Invariants hold with policies wired: where the policy declines to act the arithmetic
is exact (asserted as an equality), and where it acts scaling is monotonic and
deliberately not proportional — eight workers finish a flat-leaf fixture in 4.4 s of
modelled time against one worker's 12.0 s.
Measured cost, one development box: ~15.6 kernel events per modelled store call at
~117 µs per call including page materialisation, so a 150,000-call run is ~2.3M events
and tens of seconds rather than the minutes that would put sweeping out of reach.
A run record carries what it must be read against — which store served it, which cost
term it used and how far that term can be trusted, which budgets it declared — because
a duration quoted without them is not a result.
No result here is a validation. Agreement with the replay instrument or with a
live run is a separate exercise with its own gate; this produces plausible phase shapes
and closed-form-checkable behaviour, and says so.
Summary by CodeRabbit
New Features
Documentation