Repository navigation
test(replication): keep the fetch seam's e2e tests clear of replica hints - #246
Conversation
…ints Two e2e test files, `fetch_local_write_guard` and `fetch_responsibility_recheck`, host a chunk on a holder and then push its key straight into the target's fetch queue through the test-only `enqueue_fetch_for_test`, in five places. From the moment the holder stores the chunk it is advertisable, and a change to the holder's closest peers starts a neighbour-sync round at once instead of waiting out the 10 to 20 minute interval. On a 12-node testnet that is still settling, a replica hint can therefore put the key into the target's pending verification before the test enqueues it. `enqueue_fetch` then refuses the key as already tracked, and the test fails at setup. CI hit this three times, each the only failure in its run: "control candidate must enqueue" in `already_held_key_is_not_fetched_again` on Windows on 2026-10-01 and on Ubuntu on 2026-10-07, and in `stale_fetch_candidate_is_declined_at_download_time` on Ubuntu on 2026-10-07. Accepting the refusal in the tests would not be enough. The hint's entry has one holder behind it and no paid-list entry, so it does not reach a quorum and the key is not fetched; such a test would only time out later. So the seam now drops a pending-verification entry for the key before enqueuing it, and the key enters the fetch queue as it would have without the hint. A key already in the fetch queue or in flight, or a full queue, is still refused. If a verification cycle is asking about the key when the entry goes, it skips the key when it evaluates the answers; one that had already evaluated it could only queue it again on a verified outcome, which such a key does not reach. A hint can also land after the seam's candidate has resolved, since the chunk stays advertisable. The wait loops used `fetch_pipeline_contains_for_test`, which counts pending verification, so that entry could hold a loop to its deadline: an inconclusive verification round defers it by `verification_request_timeout`, 15 s, against windows of 12 s and 15 s. The loops now use a new `fetch_queued_or_in_flight_for_test`, which leaves pending verification out; the old hook stays as it was. A seam candidate carries no verification retry metadata, so it is never requeued for verification, and leaving the fetch queue and the in-flight set is the end of it. The hooks are compiled only for tests and the `test-utils` feature, so node behaviour does not change. `ensure_pending_verify` in `fetch_local_write_guard.rs` handles the same race at the pending-verification seam.
dirvine
left a comment
There was a problem hiding this comment.
Reviewed WithAutonomi/ant-node at a33a9cb. No blocking code findings. Platform CI is still pending; this is not a full-green sign-off.
The coordinator and two independent review seats (including OpenRouter z-ai/glm-5.2) agree on that verdict. No blocking dissent.
Why the fix is sound
- The test helper removes the pending hint and enqueues the fetch under one write lock. A hint cannot enter between those operations. Existing pending-entry cleanup also removes its bookkeeping.
- The new wait check covers the fetch queue and in-flight set. These test-injected candidates have no verification-retry metadata, so they cannot return to pending verification. A later replica hint is separate work and should not extend their wait.
- Storage, served-byte and positive-control assertions remain intact. The changes in src are test/test-utils gated; normal production behaviour is unchanged.
Verification
- Local macOS: all 1,249 library tests passed; all 54 scheduling tests also passed in a focused run.
- Local focused e2e: all eight fetch tests passed, including the three affected tests, with --test-threads=1.
- cargo fmt --check and git diff --check passed.
- A broader local e2e run reached the command's 600-second timeout before completion. It is incomplete, not a passing full-suite result.
- At posting, Ubuntu/macOS/Windows Test jobs remain pending. All other reported checks passed, including builds, Clippy, security audit, no-logging tests, storage-filesystem checks and the WebRTC devnet.
Optional follow-up
A small deterministic regression test could pin both race cases: pending hint before test injection, and pending-only state after fetch completion. It could also assert pending-only=false, queued=true and in-flight=true for the new helper. The review seats differ only on how valuable this extra unit coverage is; neither considers it a blocker.
The original random failure was not reproduced against the base in this review. Passing this focused run and tracing the race support the fix, but do not prove every source of CI flakiness is gone. Let the remaining platform CI finish before merge.
dirvine
left a comment
There was a problem hiding this comment.
Approved at a33a9cb. The previously completed review panel, including GLM-5.2, found no blocking issues. Verified that the reviewed head is unchanged and all 20 CI checks passed, including Ubuntu, macOS and Windows tests (the separate Claude check was skipped). The outstanding CI gate is satisfied. No required changes.
Two e2e tests that push a key straight into a node's fetch queue have failed intermittently on CI, and a third shares the cause: a replica hint that reaches the queue before the test does. This PR makes the test-only fetch seam, and the hook the tests watch it with, unaffected by that hint. No default-feature or production node behaviour changes.
Linear issue
Closes V2-1457
Risk tier
The changed code is compiled only for this crate's tests and the
test-utilsfeature, so no production node behaviour changes.Compatibility
test-utils). NewReplicationEngine::fetch_queued_or_in_flight_for_testandReplicationQueues::fetch_queued_or_in_flight;fetch_pipeline_contains_for_testis unchanged.enqueue_fetch_for_testkeeps its signature and now drops a pending-verification entry for the key before enqueuing it.Semver impact
Test evidence
already_held_key_is_not_fetched_againon Windows on 2026-10-01 (feat(node): add opt-in local health endpoint (V2-1380) #244) and on Ubuntu on 2026-10-07 (fix(pointer): serve the record round 1 bound, however many updates follow #238), andstale_fetch_candidate_is_declined_at_download_timeon Ubuntu on 2026-10-07 (fix(pointer): serve the record round 1 bound, however many updates follow #238, rerun).enqueue_fetch_for_testfor its key. The chunk is advertisable from the moment it is stored, and a change to the holder's closest peers starts a neighbour-sync round at once. A replica hint then puts the key into the target's pending verification first, andenqueue_fetchrefuses it as already tracked. With one holder and no paid-list entry the hint's entry does not reach a quorum, so tolerating the refusal in the tests would only move the failure to a timeout: a first version did that, and with a hint forced first both controls timed out waiting for the chunk.enqueue_pending_verify_for_test(key, holder), what a hint does) injected before all five direct enqueues in the three tests: withmain's seam all three tests fail at their first enqueue ("held-key candidate must enqueue", "candidate must enqueue", "stale candidate must enqueue"), refused the way CI saw; with this change all three pass. The third test,write_blocked_node_neither_probes_nor_dials, has not failed this way on CI, so for it this run is the only evidence. A hint injected one second after the stale and write-blocked enqueues did not fail either the old hook or the new one locally, so that run does not tell them apart. By the code, a failed quorum drops the hint's entry at once, while an inconclusive round defers it byverification_request_timeout, 15 s, as long as the write-blocked window and longer than the 12 s stale one; the new hook does not count that entry.mainat70fe777: the three tests passed three times in a row at0b4ca5band twice more at heada33a9cb, which only adds back the unchanged old hook;cargo test --lib --features test-utils1,249 passed; the fulle2esuite at0b4ca5b115 passed, 3 ignored as onmain; at head,cargo clippy --all-targets --all-features -- -D warnings,cargo fmt --check,cargo docwith-D warnings, a-D warningscheck without default features, and a Rust 1.95cargo check --all-targets --all-features --lockedare clean.New dependency
none
ADR
n/a
Mitigation / rollback
Revert. Only test code changes.