Skip to content

test(e2e): require 502 for rejected ports - #1103

Open
NekoPunch (orangeCatDeveloper) wants to merge 1 commit into
agent-substrate:mainfrom
orangeCatDeveloper:fix/arbitraryport-require-502
Open

test(e2e): require 502 for rejected ports#1103
NekoPunch (orangeCatDeveloper) wants to merge 1 commit into
agent-substrate:mainfrom
orangeCatDeveloper:fix/arbitraryport-require-502

Conversation

@orangeCatDeveloper

Copy link
Copy Markdown
Contributor

Fixes #1102

TestActorArbitraryPortAccess/unlisted_port_rejected passes when the worker is unreachable, which is the one situation it most needs to catch.

Root cause

The subtest asserts only that the response is not 200:

if resp.StatusCode == http.StatusOK {
    t.Fatalf("...unexpectedly returned HTTP 200; body: %s", body)
}

Three different outcomes satisfy that, and only one of them is the behaviour under test:

Response What happened Correct?
502 bad gateway atunnel dialled the unlisted port and failed yes
503 upstream connect error Envoy never reached the worker (#1050) no — nothing was rejected
504 actor ... request timed out the router's own ext_proc timed out no

Six recent pr-workflow runs logged the 503 as a pass; correctly there is the test's own word:

tunneled request to an unlisted port correctly returned HTTP 503;
  body: upstream connect error or disconnect/reset before headers. reset reason: connection timeout

In run 32200086998 the other three tests #1050 names failed while this one passed, on the same worker in the same run.

Fix

Require 502. That is what atunnel's ErrorHandler returns when its dial to the unlisted port fails, and it is the only outcome that means the rejection actually happened. Checking the code rather than matching Envoy's error text also covers the 504 and does not depend on wording that can change between Envoy versions.

The read-error branch above still returns early and counts as a pass. That path is not what #1050 produces, so it is left alone rather than widened without evidence.

This makes CI redder until #1050 is fixed: a failure that used to be silent now shows up. That is the point — while the assertion stays loose, TestActorArbitraryPortAccess cannot tell anyone whether #1050 has been fixed.

Before / after

Injected #1050's exact failure — SYN from the router to the worker pods dropped, control plane and established connections untouched, atenet-router restarted first so Envoy has no pooled connection. Both arms received the same 503 at 5.01s, matching OriginalDstCluster's 5s connect_timeout:

before  --- PASS: TestActorArbitraryPortAccess/unlisted_port_rejected (5.01s)
            tunneled request to an unlisted port correctly returned HTTP 503;
              body: upstream connect error or disconnect/reset before headers. reset reason: connection timeout

after   --- FAIL: TestActorArbitraryPortAccess/unlisted_port_rejected (5.01s)
            tunneled request to an unlisted port returned HTTP 503, want 502 from atunnel's failed dial;
              body: upstream connect error or disconnect/reset before headers. reset reason: connection timeout

Healthy path, no injection, three consecutive runs:

rc=0  all subtests PASS  ...correctly returned HTTP 502; body: bad gateway
  • Tests pass
  • Appropriate changes to documentation are included in the PR

@orangeCatDeveloper
NekoPunch (orangeCatDeveloper) force-pushed the fix/arbitraryport-require-502 branch 2 times, most recently from 2fddd1e to 198c1fd Compare August 25, 2026 03:32
@orangeCatDeveloper NekoPunch (orangeCatDeveloper) changed the title e2e: require 502 from the unlisted-port rejection test(e2e): require 502 for rejected ports Aug 25, 2026
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>
@orangeCatDeveloper
NekoPunch (orangeCatDeveloper) force-pushed the fix/arbitraryport-require-502 branch 3 times, most recently from e198565 to e87e416 Compare August 29, 2026 19:05
Accepting any non-200 response let unrelated 503 and 504 failures pass as a correct port rejection. Require the router response that proves the policy was enforced.
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.

TestActorArbitraryPortAccess passes when the worker is unreachable, masking #1050

2 participants