Skip to content

test(replication): keep the fetch seam's e2e tests clear of replica hints - #246

Merged
grumbach merged 1 commit into
WithAutonomi:mainfrom
grumbach:test/fetch-control-hint-race
Oct 9, 2026
Merged

grumbach merged 1 commit into
WithAutonomi:mainfrom
grumbach:test/fetch-control-hint-race

Conversation

@grumbach

@grumbach grumbach commented Oct 7, 2026

Copy link
Copy Markdown
Member

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

  • T0 — docs / tooling / CI / pure UX-output. Repo CI only.
  • T1 — client-only, no network-facing behavior change. CI + prod compat smoke.
  • T2 — node/client logic with behavioral surface, no protocol/format/economics change. Dev testnet + ADR.
  • T3 — protocol / storage format / payments / routing. T2 evidence + adversarial testing.

The changed code is compiled only for this crate's tests and the test-utils feature, so no production node behaviour changes.

Compatibility

  • Wire: none.
  • Storage: none.
  • API: additive, test-only (test-utils). New ReplicationEngine::fetch_queued_or_in_flight_for_test and ReplicationQueues::fetch_queued_or_in_flight; fetch_pipeline_contains_for_test is unchanged. enqueue_fetch_for_test keeps its signature and now drops a pending-verification entry for the key before enqueuing it.

Semver impact

  • breaking
  • feature
  • fix

Test evidence

  • What failed on CI: "control candidate must enqueue", each time the only failure in its run. already_held_key_is_not_fetched_again on 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), and stale_fetch_candidate_is_declined_at_download_time on Ubuntu on 2026-10-07 (fix(pointer): serve the record round 1 bound, however many updates follow #238, rerun).
  • Why: the tests host a chunk on a holder, then call enqueue_fetch_for_test for 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, and enqueue_fetch refuses 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.
  • The fix, in two parts: the seam drops a pending-verification entry a hint created before it enqueues, and the wait loops watch only the fetch queue and the in-flight set, so a hint that lands after the candidate resolved cannot hold them. A seam candidate carries no verification retry metadata, so it is never requeued for verification, and leaving those two stages is the end of it.
  • Forced race, locally on macOS with Rust 1.99.0. A simulated hint (enqueue_pending_verify_for_test(key, holder), what a hint does) injected before all five direct enqueues in the three tests: with main'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 by verification_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.
  • Unforced, on main at 70fe777: the three tests passed three times in a row at 0b4ca5b and twice more at head a33a9cb, which only adds back the unchanged old hook; cargo test --lib --features test-utils 1,249 passed; the full e2e suite at 0b4ca5b 115 passed, 3 ignored as on main; at head, cargo clippy --all-targets --all-features -- -D warnings, cargo fmt --check, cargo doc with -D warnings, a -D warnings check without default features, and a Rust 1.95 cargo check --all-targets --all-features --locked are clean.

New dependency

none

ADR

n/a

Mitigation / rollback

Revert. Only test code changes.

…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 dirvine left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 dirvine left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@grumbach
grumbach merged commit d07913d into WithAutonomi:main Oct 9, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants