test(e2e): isolate probe worker pools - #1147
Closed
NekoPunch (orangeCatDeveloper) wants to merge 1 commit into
Closed
test(e2e): isolate probe worker pools#1147NekoPunch (orangeCatDeveloper) wants to merge 1 commit into
NekoPunch (orangeCatDeveloper) wants to merge 1 commit into
Conversation
Aditya Shantanu (aditya-shantanu)
added a commit
to aditya-shantanu/substrate
that referenced
this pull request
Aug 24, 2026
Adopted from agent-substrate#1147 by @orangeCatDeveloper to unblock CI velocity. Fixes the TestActorIdentity_AfterRestore_IsOwnID_NotGolden flake (26 of the last 80 failed runs): identity, egressmitm and imagevolume suites shared one workload:probe pool label, so selection crossed suite boundaries and a saturated pool dialed workers that were not there. Co-authored-by: NekoPunch <engineer.jyao@gmail.com>
Collaborator
NekoPunch (orangeCatDeveloper)
force-pushed
the
fix/e2e-probe-pool-isolation
branch
from
August 25, 2026 01:56
ef1e1ff to
3ced570
Compare
Probe suites shared one worker selector, so one suite could delete workers still used by another. Give each fixture its own pool identity to keep parallel teardown isolated.
NekoPunch (orangeCatDeveloper)
force-pushed
the
fix/e2e-probe-pool-isolation
branch
from
August 25, 2026 03:32
3ced570 to
48ce424
Compare
NekoPunch (orangeCatDeveloper)
changed the title
yufan-su
pushed a commit
that referenced
this pull request
Aug 25, 2026
…relay) (#1160) Fixes #675, fixes #1100, fixes #1146; addresses the CI flake in #1106. ## Why this PR Flaky tests are the single biggest drag on this repo's velocity right now: the three flakes fixed here account for the majority of red CI runs over the last 7 days (identity: 35 failures, parking: 32, relay: 9 — from the flake dashboard's cross-PR analysis of ~630 runs). Every red run costs a contributor a rebase-and-rerun cycle and costs reviewers signal. **This PR consolidates the three root-caused, in-flight fixes into one change to get CI green now and unblock the community — the goal is velocity, not authorship.** ## Credit where it's due All three fixes were root-caused and written by others; this PR adopts them onto latest main with their tests, unchanged in substance. Each commit carries a `Co-authored-by` trailer: | Commit | Original PR | Author | Root cause | |---|---|---|---| | e2e: give each probe fixture its own worker pool | #1147 | @orangeCatDeveloper | identity/egressmitm/imagevolume suites share one `workload: probe` pool label; cross-suite selection under concurrent suite processes dials workers that are not there | | atenet: never cancel an in-flight resume at the park budget | #991 | @omeryahud | the park budget doubled as the ResumeActor RPC deadline; a mid-restore cancel strands a RESUMING actor on a live worker | | atunnel: close the relay's both ends before returning | #1101 | @orangeCatDeveloper | the relay closed both ends from a `context.AfterFunc` goroutine the test never waits for | @Stevenjin8's #1107 correctly diagnosed the ateom readiness race in #1106; the control-plane readiness gap it targets remains real and open — this PR only removes the e2e-fixture contention that makes it fire constantly in CI. If maintainers prefer to land the original PRs individually instead, closing this one is completely fine — the point is that the fixes land somewhere, soon. ## Evidence the flakes are actually fixed **TestRelayIngressCancellationClosesBothSides (unit, `-race`):** - Unpatched main, `-count=3000`: **83 failures (2.8%)** — matches the 2.9% observed across 308 CI runs this week - This branch, `-count=10000`: **0 failures** **TestRequestParking (park-budget cancellation):** - The new `InFlightAttemptRunsToCompletion` and `LateRetryableErrorIsBudgetExhaustion` unit tests (from #991) encode the exact failure mode from #675 and pass under `go test -race -count=100 ./cmd/atenet/internal/router/ingress/` - The pre-fix behavior (budget cancelling the in-flight RPC) is deterministically reproduced by the old test it replaces **TestActorIdentity_AfterRestore_IsOwnID_NotGolden (probe pool isolation):** - Not reproducible outside CI (needs concurrent suite processes on a contended kind node), so verified statically: `${FIXTURE_SUFFIX}` is always `-<suite>` (internal/e2e/sandbox.go:189,201 — never empty), `probe-sized` already uses its own label, and no other manifest or selector references `workload: probe`. #1147's CI data shows all three failure signatures (missing `ateom.sock`, `runsc restore` killed, router 502/503) trace to cross-suite pool sharing; per-suite labels make the selector suite-local by construction - The definitive check is this PR's own CI plus the flake dashboard's 7-day window after merge — I will report the post-merge rates on #1106 Also run: `go build ./...`, `go vet` and the full `-race` suites of both touched packages — all green. ## What this PR deliberately does NOT fix `TestActorEgressHTTPS` (#1050, 4.6% this week, below the 5% flake threshold) has no root-caused fix yet — the 503 `upstream connect error` path needs investigation in a live cluster. #1103 (@orangeCatDeveloper) tightens the related `TestActorArbitraryPortAccess` assertion so those 503s stop passing silently; it should land after #1050's cause is fixed, or it converts hidden flakiness into visible red. ## Update (post-CI investigation) The first e2e runs failed on `TestRequestParking/ParkThenServed` (micro-VM lane). Investigation showed this is the **pre-existing dominant mode** of #675 — identical failures in main-era runs 32305728993 / 32397291519 / 32487958077 — not a regression: on micro-VM, `SuspendActor` returns before the snapshot upload completes, so the worker legitimately isn't free within the 5s park budget and the router's 503 is correct behavior. #991 fixes the *other* (mid-restore cancellation/stranding) mode. Commit d637690 makes the subtest retry while the worker is still freeing; a stranded worker still fails every attempt, so the regression stays pinned. **Additional validation:** - CI e2e-test now **passes both lanes** (run 32754691017) - Local kind cluster built from this branch: parking suite **10/10 consecutive passes**; identity + egressmitm + imagevolume run **concurrently** (the exact contention behind the identity flake) × 3 iterations — **9/9 suite passes** --------- Co-authored-by: Aditya Shantanu <aditya-shantanu@users.noreply.github.com> Co-authored-by: NekoPunch <engineer.jyao@gmail.com> Co-authored-by: Omer Yahud <oyahud@nvidia.com>
NekoPunch (orangeCatDeveloper)
deleted the
fix/e2e-probe-pool-isolation
branch
August 25, 2026 18:59
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
2 participants
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.
Fixes #1146
TestActorIdentity_AfterRestore_IsOwnID_NotGoldenis the most frequent failure in CI — 26 of the last 80 failed runs — and fails three different ways: a dial to a worker'sateom.sockthat is not there,runsc restore`: signal: killed, and a router 502/503.All three are one bug.
probe.yaml.tmplgives every suite its own namespace, ActorTemplate and snapshot prefix through${FIXTURE_SUFFIX}, but the pool label and theworkerSelectorare the constantworkload: probe. Selection is by label, not namespace, soidentity,egressmitmandimagevolumeshare one 9-worker pool, and the first suite to tear its namespace down deletes workers the others are still running actors on. The victim's symptom is decided only by where it happened to be: between calls it cannot dial, mid-restore ateom's handler context is cancelled andexec.CommandContextSIGKILLs runsc, and after routing is up the router answers 5xx.Putting the suffix on both labels restores the isolation the rest of the template already has —
parking_test.go:190does the same withdemo: <namespace>.capabilities.yaml.tmplandprobe-sized.yaml.tmplcarry constant labels too, but each has a single caller today, so they cannot collide and are left alone.Two identical 4-vCPU VMs, run concurrently, differing only in whether these two labels carry the suffix. A round is the full e2e suite; a hit is any of the three signatures:
The unfixed arm also carries CI's own evidence — an identity actor running on an imagevolume worker: