test(ingester): make RFC0008.8 latency test deterministic via virtual clock - #197
Merged
jensholdgaard merged 2 commits intoJun 14, 2026
Merged
Conversation
… clock `rfc0008_8_p99_latency_tracks_batch_window` asserted a strict 3-way monotonic ordering of three *wall-clock* P99 measurements (`p99(10) < p99(50) < p99(150)`). That ordering is non-deterministic: on a loaded or instrumented runner the per-flush fixed overhead dominates the smallest window and any sample can spike, flipping the order. It flaked the required `cargo test` job on #193 (`[164, 491, 311]`) and the `continue-on-error` coverage job on #196 (`[482, 53, 154]`, llvm-cov instrumentation inflating the 10 ms window to 482 ms). Rewrite it to run under a paused virtual clock (`#[tokio::test(start_paused = true)]`, enabled by tokio's `test-util` dev feature). tokio auto-advances to the next pending timer when the runtime is idle, so the only time that elapses is the coordinator's own `tokio::time::sleep(window)`; the real fsync (offloaded to `spawn_blocking`) runs in wall-clock time but does not advance the virtual clock. Measuring with `tokio::time::Instant` therefore yields the commit's batch wait exactly, with zero jitter. This strengthens the contract rather than weakening it (§6.2): a batch of commits fired together all ride one window, so ack latency *equals* the configured window and scales 1:1 across the spec's real `{10, 100, 1000}` ms values (now free of wall-clock cost — the prior `{10, 50, 150}` deviation is gone). A per-record-fsync impl would ack at ≈ 0 regardless of the window. Batching itself (`appends_per_sync ≫ 1`) stays pinned counter-exactly by the sibling test. Approach chosen by the maintainer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRFC0008.8's batched-fsync test is rewritten from a P99 wall-clock latency check with ±30% tolerance to a deterministic assertion under a paused Tokio virtual clock, where each commit's ack latency must equal the configured ChangesRFC0008.8 Deterministic Batch-Window Test
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR rewrites the RFC0008.8 ingester latency test to eliminate flakiness by running it under Tokio’s paused virtual clock, making the “batch window dominates” timing assertion deterministic (and independent of CI jitter/coverage instrumentation).
Changes:
- Replace the wall-clock P99-based latency test with a virtual-time-based test that asserts deterministic batch-window waiting behavior.
- Update the RFC0008.8 batched-fsync test documentation and implementation to use
tokio::time::Instant. - Enable Tokio’s
test-utilfeature inourios-ingesterdev-dependencies to support#[tokio::test(start_paused = true)].
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/ourios-ingester/tests/rfc0008_8_batched_fsync.rs | Replaces flaky wall-clock P99 latency sampling with a deterministic virtual-clock assertion for batch-window wait time. |
| crates/ourios-ingester/Cargo.toml | Adds Tokio test-util feature in dev-dependencies to enable start_paused in tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot flagged a spec/test mismatch on #197: the RFC0008.8 §5 scenario still described measuring wall-clock P99 ack latency over a 10 s sample within a ±30 % tolerance, but the test now asserts exact ack-latency equality under a paused virtual clock. Update the scenario (and the §5 summary) to the virtual-clock formulation: a batch of commits fired together under a paused clock each ack at exactly the configured window (deterministic, no jitter) — the window dominates, not per-record fsync. The contract is unchanged (window dominates, appends_per_sync ≫ 1, §3.4 holds); only the measurement method is restated, superseding the non-deterministic wall-clock-P99 formulation. Approach approved by the maintainer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Problem
rfc0008_8_p99_latency_tracks_batch_windowwas flaky onmain. It asserted a strict 3-way monotonic ordering of three wall-clock P99s (p99(10) < p99(50) < p99(150)), which is non-deterministic — the per-flush fixed overhead dominates the smallest window and any sample can spike, flipping the order:cargo testjob:[164, 491, 311]ms → failedcontinue-on-error):[482, 53, 154]ms → failed (llvm-cov instrumentation inflated the 10 ms window to 482 ms)The coverage job's red is what surfaced as "CI failing on main" (the required job currently passes; the coverage job fails reliably under instrumentation).
Fix (maintainer-chosen: deterministic virtual clock)
Run the test under a paused virtual clock (
#[tokio::test(start_paused = true)], enabled by tokio'stest-utildev feature). tokio auto-advances to the next pending timer when the runtime is idle, so the only time that elapses is the coordinator's owntokio::time::sleep(window). The real fsync (offloaded tospawn_blocking) runs in wall-clock time but does not advance the virtual clock, so measuring withtokio::time::Instantyields the commit's batch wait exactly — no scheduler/instrumentation jitter.Invariants (CLAUDE.md §3 / §6.2)
This strengthens the contract, it does not weaken it (§6.2 — tests are specifications):
{10, 100, 1000}ms values (the prior{10, 50, 150}"documented deviation" is gone — virtual time is free).appends_per_sync ≫ 1) stays pinned counter-exactly by the sibling test.Verification
Ran the rewritten test 6× locally — deterministic (exact equality holds each time), ~0.02 s. Full
ourios-ingestertest crate,cargo fmt --check, andcargo clippy --all-targets --all-features -D warningsall clean.🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Documentation
Chores