GeezerVison is showing github.com — Fix vendor prefetch breaker-test flake and outage over-requests by mikolalysenko · Pull Request #510 · SocketDev/socket-patch
Conversation
Problem
api::vendor_prefetch::tests::an_outage_mid_list_opens_the_breaker_at_the_same_package (socket-patch-core unit test) failed in test (macos-latest) on CI run 36878455088 (job 110423824031, branch agent/fix-yarn-berry-hosted-auth-leak, SHA 0c461dcd). That branch doesn't touch the prefetch:
panicked at crates/socket-patch-core/src/api/vendor_prefetch.rs:894:9:
assertion `left == right` failed: a mid-list outage must not be amplified by the plan either
left: 16
right: 12
(4675 passed, 1 failed)
A single failure in test turns the whole CI run red (about 100 jobs and about 215 job-minutes per attempt, with the windows/macos test legs taking 18–25 min), and yarn-classic-matrix, cargo-vex-matrix and others wait on it with needs: test.
Local reproduction (unchanged main, debug build, 4 vCPU): 0/200 when idle; 11/640 and 4/640 with 16 copies running in parallel plus 8 busy-loop CPU hogs. The failures were 14, 15 and 16 requests against 12, the same signature as CI.
Root cause
The test depends on timing, and the timing-independent version of the same scenario exposes a real over-request bug in the prefetch.
- The test's race. The doc comment says "the task here is running ahead of a loop still on the granted packages". Nothing enforces that. It holds only if package 3's 503 retry ladder lands in the prefetch task before the mock server releases package 1's grant (a 20 ms delay). Under load the ladder is slower, so the loop reaches packages 1 and 2 first. With the barrier not yet set, the widened reach admits packages 5–7. That is the documented "loop caught up with the window" cost, which the strict assertion doesn't allow for.
- The bug the race was hiding. I made the doc's premise deterministic: before each call, the loop waits until the task has seen the failure it is about to meet, the way the real loop pauses to write between packages. Under that pacing the plan sends 21 requests where serial sends 12, every time. Here is why. The loop consumes package 3's failure and calls package 4.
Lookahead::arrive then lifts the barrier, but the reach is still 6, a size earned by answers that came before the outage. So packages 4, 5, 6 and 7 all start their retry ladders at once, against a service the loop's own breaker is one failure from giving up on. The module docs promise this case ("an outage part-way down a list … costs what the serial loop paid, as long as the task is running ahead of the loop") costs nothing extra. On a real outage, that means about three extra 3-attempt ladders hit the patch service per outage.
Fix
crates/socket-patch-core/src/api/vendor_prefetch.rs:
Lookahead::arrive: when the loop moves past the failed position and the barrier lifts, the reach drops to 1. Only the loop's own position may be requested until a good answer reopens the reach (next_reach(1) is SLOW_START). Outcomes are unchanged, since every decision is still made at consumption time. The only thing removed is speculative requests against a failing service. The module and field docs are updated to say so.
- Test: a new
run_paced helper waits on the lookahead's own moved notifier (30 s safety timeout) instead of on wall-clock delays. The test's request-count assertion is unchanged (planned == serial == 12), and so are its outcome and breaker-count assertions. Only the scheduling assumption they rested on now holds by construction.
No test removed, no #[ignore], no timeout raised, no retry added.
Proof
- Without the fix, the paced test fails 5/5 (
left: 18/19/21/21/21, right: 12), so it now pins the bug.
- With the fix, the paced test passed 0 failures in 1600 runs (16 parallel copies plus 8 CPU hogs). Before, the same stress gave 4–11 failures per 640. The whole
api::vendor_prefetch module (24 tests) gave 0 failures in 200 loaded runs.
cargo test -p socket-patch-core --lib: 4715 passed. Four tests fail identically on unchanged main in this container because it runs as uid 0: relax_loop_must_not_traverse_symlinked_root, an_unremovable_hidden_lock_keeps_every_store_entry, wire_write_failure_maps_error_and_leaves_lock_untouched and wire_failure_rolls_back_already_written_files all expect permission errors.
rustfmt --check is clean on the touched file. cargo clippy -p socket-patch-core --all-targets -- -D warnings reports nothing in vendor_prefetch.rs. The errors it does report are in maven_repo.rs, nuget_feed.rs, yarn_berry_lock.rs and bun_binary.rs, and they are the same on unchanged main with this toolchain.
Where the tests run
Nothing removed or moved. The test still runs in ci.yml's test (3 OSes), test-release and coverage jobs.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WDqizZLqhYTcaakGpnyv1r
Generated by Claude Code
Note Medium Risk
Changes when the prefetch task may hit the vendor service during outages; observable outcomes and breaker behavior at consumption are unchanged, but request volume and timing against a failing host differ in the post-failure window.
Overview
Vendor prefetch no longer fires extra grant/retry traffic after the serial loop consumes a mid-list availability failure. When the loop moves past the failed position and the lookahead barrier clears, Lookahead::arrive now resets reach to 1, so only the loop’s current package may be speculated until the service answers well again—instead of keeping a pre-outage window that started retry ladders on several following packages.
Docs for the module and Lookahead describe that tightened bound. The flaky an_outage_mid_list_opens_the_breaker_at_the_same_package test uses a new run_paced helper that waits on lookahead state (failure landed) before each fetch, so the “prefetch ahead of the loop” assumption holds without relying on mock delays under CPU load.
Reviewed by Cursor Bugbot for commit 2f72a2a. Configure here.
Generated by Claude Code
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
an_outage_mid_list_opens_the_breaker_at_the_same_package failed on
macOS CI (16 requests vs 12) and fails ~1% locally under load. The
test assumed the prefetch task lands the outage's first failure before
the loop leaves the granted packages, but that depended on the mock
server's 10-30ms delays beating a retry ladder.
Making that assumption deterministic (the loop waits, as the real loop
does while it writes) exposed a real amplification: once the loop
consumed the failure and moved on, the barrier lifted with the reach
still sized by the answers from before the outage, so the task fired
up to three extra retry ladders (21 requests vs serial's 12) at a
service the loop's breaker was one failure from giving up on.
Lifting the barrier now drops the reach to one, so only the loop's own
position is requested until the service answers well again. The test
paces the loop on the lookahead instead of on wall-clock delays.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01WDqizZLqhYTcaakGpnyv1r
cursor[bot] reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 2f72a2a. Configure here.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
[agent] Ready for review at 2f72a2a.
- CI: 97/97 non-skipped checks green.
- Bugbot: reviewed 2f72a2a (23:17Z) — no issues found.
- Mergeable against
main, no unresolved review threads.
- Slack announcement pending: the burn-down agent has no Slack send tool this run.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Reviewed 2f72a2a39114. Ready to merge from this code review; no changes requested.
No blocking findings. The reach reset happens before the failure barrier is lifted, and the new notifier-based pacing pins the intended test ordering without changing the request-count or breaker assertions.
Validation: Ran cargo test --locked -p socket-patch-core --lib api::vendor_prefetch -- --test-threads=4: 24 passed. Reviewed the surrounding admission, failure, and consumption paths; current CI checks pass.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Labels
ci-janitor
Opened by the CI janitor routine (flakes, redundant tests, CI perf)
Ready for review
Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|