Skip to content

test(proxy-stress): dial 127.0.0.1 for the test proxy's upstream instead of resolving 'localhost' - #34049

Merged
dylan-conway merged 3 commits into
mainfrom
farm/01a36cd4/proxy-stress-localhost-dial
Jul 13, 2026
Merged

dylan-conway merged 3 commits into
mainfrom
farm/01a36cd4/proxy-stress-localhost-dial

Conversation

@robobun

@robobun robobun commented Jul 12, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

test/js/bun/http/proxy-stress-matrix.test.ts went red on darwin 26 aarch64 in build 72283:

error: expect(received).toBe(expected)
Expected: 200
Received: 204
✗ method matrix > OPTIONS via https-proxy → http-origin
334 pass / 1 fail

The runner tagged it "crash reported" (the intentional crashes from run-crash-handler.test.ts earlier in the shard, as diagnosed in #33984), which skipped the automatic retry.

Cause

Nothing in this test file ever emits 204, and the adversarial origin's buildResponse defaults to 200 for every cell of the method matrix. The 204 came from outside the test process.

createAdversarialOrigin binds to 127.0.0.1 but hands out a http(s)://localhost:${port} URL (so that Host / SNI / checkServerIdentity assertions see a hostname). The test proxy parses the absolute-form target, pulls host = "localhost", and dials net.connect(port, "localhost"). On the darwin boxes localhost resolves to [::1, 127.0.0.1] with ::1 first, and autoSelectFamily tries addresses in that order:

$ ssh darwin-test-arm64-3 "bun -e '...dns.lookup(\"localhost\",{all:true},...)'"
[{"address":"::1","family":6},{"address":"127.0.0.1","family":4}]

IPv4 and IPv6 ephemeral-port spaces are independent, so an unrelated process on the bare-metal CI host (another buildkite agent, a system daemon) can be listening on [::1]:P at the exact moment our origin holds 127.0.0.1:P. Verified directly on darwin-test-arm64-3: when both [::1]:P (returns 204) and 127.0.0.1:P (returns 200) are bound, net.connect(P, "localhost") connects to ::1 and reads back 204. On linux the same script reads 200.

This is the second half of the mechanism #33984 ("the proxy's net.connect(port, 'localhost') goes through autoSelectFamily, adding async hops") reduced but didn't eliminate: #33984 shared the proxy pair to cut ephemeral-port churn; each test still creates a fresh origin, and each origin dial still resolves localhost. Introduced by #32635.

Fix

Normalize host = "localhost" to "127.0.0.1" in createAdversarialProxy's upstream dial. Every origin in the suite binds 127.0.0.1; the IPv6-literal tests in proxy-stress-protocol.test.ts pass [::1] explicitly and are unaffected. The origin URL (and therefore the client's Host header, CONNECT target, SNI, and the checkServerIdentity host argument) remains localhost, so none of the hostname-shape assertions in proxy-stress-adversarial.test.ts / proxy-stress-headers.test.ts change.

Verification

bun bd test test/js/bun/http/proxy-stress-matrix.test.ts        # 335 pass (debug+ASAN)
bun bd test test/js/bun/http/proxy-stress-adversarial.test.ts \
            test/js/bun/http/proxy-stress-headers.test.ts \
            test/js/bun/http/proxy-stress-protocol.test.ts      # 329 pass
bun bd test test/js/bun/http/proxy-stress-errors.test.ts \
            test/js/bun/http/proxy-stress-lifecycle.test.ts \
            test/js/bun/http/proxy-stress-concurrent.test.ts    # 177 pass

15 consecutive release runs of proxy-stress-matrix.test.ts, all 335/335.


no test proof · iteration 1 · docs-only change; test-proof not applicable

…xy's upstream connect

On darwin, localhost resolves to [::1, 127.0.0.1] and net.connect with
autoSelectFamily tries ::1 first. The adversarial origins bind 127.0.0.1
only, so when an unrelated process on the CI host happens to be listening
on [::1]:P at the same ephemeral port, the test proxy connects to that
instead of the intended origin. Observed as a stray 204 on
'OPTIONS via https-proxy -> http-origin' in build 72283 on darwin 26
aarch64.

The origin URL remains http(s)://localhost:P so Host header, CONNECT
target, SNI, and checkServerIdentity assertions are unchanged.
@robobun

robobun commented Jul 12, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:26 AM PT - Jul 12th, 2026

❌ @robobun, your commit 637f1dd has 4 failures in Build #72305 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 34049

That installs a local version of the PR into your bun-34049 executable, so you can run:

bun-34049 --bun

@coderabbitai

coderabbitai Bot commented Jul 12, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

Proxy host normalization

Layer / File(s) Summary
Normalize upstream host before connection
test/js/bun/http/proxy-stress-helpers.ts
createAdversarialProxy strips IPv6 brackets and maps localhost to 127.0.0.1 before calling net.connect.

Possibly related PRs

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and accurately describes the upstream dialing change from localhost to 127.0.0.1.
Description check ✅ Passed The description covers the problem, cause, fix, and verification, which satisfies the template's intent despite different headings.

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — one-line test-helper fix normalizing localhost → 127.0.0.1 in the proxy's upstream dial to eliminate the darwin ::1-first flake.

What was reviewed:

  • Confirmed createAdversarialOrigin binds 127.0.0.1 while emitting a localhost URL, so the normalization is correct and the Host/SNI/CONNECT-target assertions in the adversarial/headers suites still see localhost.
  • IPv6-literal tests in proxy-stress-protocol.test.ts pass [::1] explicitly; bracket-stripping happens before this check, so they bypass it.
  • The sibling inline proxy in proxy-stress-memory-fixture.ts was checked — its origin is Bun.serve (not bound to 127.0.0.1-only) and it asserts RSS not status codes, so the same failure mode doesn't apply.
Extended reasoning...

Overview

Single-line behavioral change plus a 7-line explanatory comment in test/js/bun/http/proxy-stress-helpers.ts, a test-only helper module. The change adds if (host === "localhost") host = "127.0.0.1"; immediately before the adversarial proxy's net.connect(port, host) upstream dial. No production/runtime code is touched.

Security risks

None. This is test infrastructure that spins up loopback proxies/origins for stress-testing the HTTP client. The normalization narrows the dial target from a resolved hostname to a fixed loopback literal — strictly reducing surface, not adding any.

Level of scrutiny

Low. Test-helper flake fix with an exceptionally thorough root-cause analysis (dns.lookup ordering on darwin, independent v4/v6 ephemeral-port spaces, verified repro on the actual CI host). The fix is placed at the shared helper so every consumer of createAdversarialProxy benefits, which is exactly where the CLAUDE.md guidance says flake fixes belong. All six dependent test files (841 tests) were re-run and pass.

Other factors

  • Verified the placement is correct: it comes after the IPv6-bracket strip, so [::1] targets from proxy-stress-protocol.test.ts are already ::1 at this point and skip the rewrite.
  • The origin URL construction at proxy-stress-helpers.ts:674 still emits localhost, so the hostname-shape assertions at proxy-stress-adversarial.test.ts:146/176/584/586 and proxy-stress-headers.test.ts:244 are unaffected — only the proxy's internal socket dial changes.
  • Finder agents flagged the inline proxy in proxy-stress-memory-fixture.ts as having the same pattern; verifiers ruled it out because that fixture's origin is a Bun.serve bound to all interfaces (not 127.0.0.1-only) and the fixture asserts RSS growth rather than response status, so a stray ::1 connection can't produce the same failure.
  • The comment is 7 lines, over the repo's 3-line guideline, but it documents a non-obvious CI-host-specific mechanism with a build reference — the kind of durable non-obvious content the guideline explicitly permits.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — one-line test-helper fix normalizing the proxy's upstream dial from localhost to 127.0.0.1.

What was reviewed:

  • Confirmed every origin in the proxy-stress suite (helper + hand-rolled ones in protocol/headers/errors/adversarial/concurrent) binds 127.0.0.1, so the rewrite is always correct.
  • Verified the IPv6-literal tests pass [::1] explicitly and the bracket-strip runs before this check, so they're unaffected.
  • Checked that all localhost assertions in the suite target Host / CONNECT target / checkServerIdentity — derived from the origin URL, not the dial address, so unchanged.
Extended reasoning...

Overview

Single-line change in test/js/bun/http/proxy-stress-helpers.ts: inside createAdversarialProxy, after parsing the upstream host from the CONNECT target / absolute-form URL and stripping IPv6 brackets, rewrite host === "localhost" to "127.0.0.1" before calling net.connect(port, host). Test infrastructure only; no runtime code touched.

Security risks

None. This is test-only code adjusting which loopback address a test proxy dials. No auth, crypto, permissions, or user-facing surface.

Level of scrutiny

Low. It's a CI-flake fix in a test helper with an unusually thorough root-cause writeup (darwin resolves localhost → [::1, 127.0.0.1] with ::1 first; v4/v6 ephemeral-port spaces are independent, so an unrelated [::1]:P listener can hijack the connection). The fix is the obvious minimal one and matches what createAdversarialOrigin actually binds.

Other factors

I grepped all proxy-stress-*.test.ts consumers: every .listen(...) call binds 127.0.0.1, so the PR's claim that "every origin in the suite binds 127.0.0.1" holds — the rewrite can't misdirect any test. The [::1] tests in proxy-stress-protocol.test.ts pass an explicit IPv6 literal, which hits the bracket-strip branch and skips this new check. All localhost string assertions in the suite inspect the Host header, CONNECT target, or checkServerIdentity host argument — those come from the origin URL (still localhost), not from the proxy's internal net.connect dial address. The comment is 3 lines, within the repo's stated limit. The author ran all seven proxy-stress test files plus 15 consecutive release runs of the previously-flaky file.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — test-helper-only flake fix mapping localhost → 127.0.0.1 in the proxy's upstream dial.

What was reviewed:

  • Confirmed createAdversarialOrigin binds 127.0.0.1 while advertising localhost, so the rewrite targets the actually-bound address.
  • Checked the IPv6-literal tests in proxy-stress-protocol.test.ts pass [::1] explicitly; bracket-stripping runs first so they don't hit this branch.
  • Verified the rewrite is scoped to the net.connect dial only — the origin URL / Host header / CONNECT target / SNI still see localhost, so hostname assertions elsewhere are unaffected.
Extended reasoning...

Overview

One-line change to test/js/bun/http/proxy-stress-helpers.ts: inside createAdversarialProxy, after resolving the upstream host from the CONNECT target or absolute-form URL, host === "localhost" is normalized to "127.0.0.1" before net.connect(port, host). A 3-line comment explains why. No production/runtime code is touched.

Security risks

None. This is test infrastructure only; the helper is a hand-rolled proxy used exclusively by the proxy-stress-*.test.ts suite. The change narrows which loopback address the test proxy dials — it cannot affect shipped behavior, auth, or data handling.

Level of scrutiny

Low. Test-helper flake fix with a well-diagnosed root cause (darwin resolves localhost to ::1 first via autoSelectFamily; v4/v6 ephemeral-port spaces are independent, so an unrelated [::1]:P listener on the shared CI host can shadow the 127.0.0.1:P origin). The origin already binds 127.0.0.1 explicitly, so dialing that literal is strictly more correct than resolving localhost.

Other factors

  • Placement is correct: the check follows the existing IPv6 bracket-strip, so [::1] targets become ::1 and skip this branch — confirmed the IPv6-literal tests at proxy-stress-protocol.test.ts:277-298 use [::1] explicitly.
  • The rewrite only affects the net.connect argument. createAdversarialOrigin still returns a localhost URL, so the client-side Host header, CONNECT authority, SNI, and checkServerIdentity inputs are unchanged; hostname-shape assertions in the adversarial/headers suites remain valid.
  • No CODEOWNERS entry covers this path.
  • PR description shows all six proxy-stress test files passing plus 15 consecutive release runs of the previously-flaky matrix file.

@robobun

robobun commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator Author

The diff is ready. Across build 72301 and the retrigger 72305, every test/js/bun/http/proxy-stress-*.test.ts file (the only consumers of the changed helper) passed on all lanes, including darwin 26 aarch64 where the original flake fired.

Remaining red is unrelated to this change:

  • test/napi/napi.test.ts "has the right lifetime" GC-timing on Windows x64-baseline (72301)
  • test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js SIGABRT on linux x64-asan (72305)
  • bake/dev-and-prod.test.ts, zlib/leak.test.ts, test-repl-close.js, bun-install-proxy.test.ts, 30205.test.ts, spawn.test.ts, bun-add.test.ts in the "flaky" annotation (also seen on main)

None of those files import proxy-stress-helpers.ts; this diff is a single-line change inside createAdversarialProxy and touches no runtime code.

@dylan-conway
dylan-conway merged commit a59a9c3 into main Jul 13, 2026
74 of 77 checks passed
@dylan-conway
dylan-conway deleted the farm/01a36cd4/proxy-stress-localhost-dial branch July 13, 2026 15:58
Jarred-Sumner pushed a commit that referenced this pull request Jul 24, 2026
…'t steal it (#35269)

## Problem

`test/js/bun/http/proxy-stress-errors.test.ts` went red in [build
78599](https://buildkite.com/bun/bun/builds/78599) (debian 13 x64),
[build 78501](https://buildkite.com/bun/bun/builds/78501) (debian 13
aarch64), and [build 77839](https://buildkite.com/bun/bun/builds/77839)
(alpine 3.23 x64). Each sighting is a different "upstream unreachable
via proxy" or "proxy authentication" subtest getting the wrong result:

| build | test | expected | got |
|---|---|---|---|
| 77839 | `http-proxy, absolute-form upstream refused → 502` | 502 | 200
|
| 77839 | `https-proxy → http-origin: wrong auth → 403` | origin not
reached | `origin.requests.length == 1` |
| 78501 | `https-proxy, CONNECT upstream refused → 502` | 502 | 407 |
| 78599 | `http-proxy, absolute-form upstream refused → 502` | 502 |
ECONNRESET |

Each build's annotation also carries `1 crashes reported during this
test`; those traces are `panic: Failed to start File Watcher: EAGAIN`
from `src/jsc/hot_reloader.rs`, which this test cannot reach (it spawns
no subprocess, uses no watcher). They are the shard-level
crash-attribution issue already diagnosed in #33984/#34049 and only
serve to suppress the automatic retry, promoting the flake to a hard
fail.

## Cause

`deadPort()` binds `127.0.0.1:0`, closes the server, and returns the
port for the caller to dial expecting `ECONNREFUSED`. The doc comment
says "nothing reuses it in the microseconds before the caller dials",
which is false under `test.concurrent`: the file's 53 concurrent tests
each issue one to three `listen(0, "127.0.0.1")` calls of their own, and
the kernel's ephemeral allocator happily satisfies one of them with the
port `deadPort()` just released. Mechanically:

```
$ bun -e '... oldDeadPort() then 20× listen(0), 500 trials ...'
old deadPort: port grabbed by subsequent listen(0) in 3 / 500 trials
```

That explains every observed value: 200 is a concurrent
`createAdversarialOrigin` answering; 407 is a concurrent
`createAdversarialProxy({ tls: true, auth })` answering (the test
proxy's upstream TCP connect succeeds, it replies 200 Connection
Established, the client's inner TLS handshake completes against the auth
proxy, which then 407s the GET); ECONNRESET is a concurrent TLS server
receiving plaintext absolute-form bytes; and `origin.requests.length ==
1` is this test's origin being another test's "dead" port.

Introduced by #32635 (the file has always paired `deadPort()` with
`test.concurrent`); same family as the ephemeral-port races
#33975/#33984/#34049 fixed in the sibling files.

## Fix

Hold the port as the local side of a live TCP connection for the
lifetime of the test: bound, so `listen(0)` will not be handed it, and
not listening, so `connect()` to it still gets `ECONNREFUSED`.
`deadPort()` now returns a disposable and the three call sites become
`using dead = await deadPort()`.

```
$ bun -e '... newDeadPort() held, 2000× listen(0) ...'
listen(0) collisions with held port out of 2000: 0
connect to held port: error:ECONNREFUSED
```

## Verification

```
# before (release bun, flake-prone subset -t "unreachable|authentication|unsupported proxy scheme")
6 / 200 runs had a failure

# after (same command)
0 / 200 runs had a failure

# full file, 100 release runs: stable
# bun bd (debug+ASAN), 10 runs: 53 pass / 0 fail / 132 expect() each
```

All six sibling `proxy-stress-*.test.ts` files (788 tests) pass
unchanged.

<!-- robobun:evidence:begin -->

---

**[stamp-90s]** gate passed · iteration 1 · 2 files touched

<details><summary>passes on PR (with fix)</summary>

```console
Test-only change.

Debug/ASAN (expected pass):
$ bun bd test 'test/js/bun/http/proxy-stress-errors.test.ts'
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/bun/http/proxy-stress-errors.test.ts
bun test v1.4.0 (1e135bf)

test/js/bun/http/proxy-stress-errors.test.ts:
(pass) CONNECT failure status > http-proxy CONNECT → 400 is surfaced as-is [608.07ms]
(pass) CONNECT failure status > http-proxy CONNECT → 403 is surfaced as-is [357.19ms]
(pass) CONNECT failure status > http-proxy CONNECT → 407 is surfaced as-is [353.15ms]
(pass) CONNECT failure status > http-proxy CONNECT → 500 is surfaced as-is [443.44ms]
(pass) CONNECT failure status > http-proxy CONNECT → 502 is surfaced as-is [445.88ms]
(pass) CONNECT failure status > http-proxy CONNECT → 503 is surfaced as-is [281.49ms]
(pass) CONNECT failure status > http-proxy CONNECT → 504 is surfaced as-is [279.92ms]
(pass) CONNECT failure status > https-proxy CONNECT → 400 is surfaced as-is [341.27ms]
(pass) CONNECT failure status > https-proxy CONNECT → 403 is surfaced as-is [316.94ms]
(pass) CONNECT failure status > https-proxy CONNECT → 407 is surfaced as-is [316.58ms]
(pass) CONNECT failure status > https-proxy CONNECT → 500 is surfaced as-is [347.12ms]
(pass) CONNECT failure status > https-proxy CONNECT → 502 is surfaced as-is [351.66ms]
(pass) CONNECT failure status > https-proxy CONNECT → 503 is surfaced as-is [386.69ms]
(pass) CONNECT failure status > https-proxy CONNECT → 504 is surfaced as-is [369.56ms]
(pass) CONNECT failure status > CONNECT → 301 with Location is not followed [386.12ms]
(pass) CONNECT failure status > CONNECT → 302 with Location is not followed [575.16ms]
(pass) proxy unreachable > proxy port refused, http origin [449.75ms]
(pass) proxy unreachable > proxy port refused, https origin [437.50ms]
(pass) CONNECT failure status > https-proxy CONNECT → 101 fails even when the request asked to upgrade [614.67ms]
(pass) CONNECT failure status > http-proxy CONNECT → 101 fails even when the request asked to upgrade [718.76ms]

... (truncated)
Exit: 0
```

</details>

<details><summary>diff hotspot</summary>

```
test/js/bun/http/proxy-stress-errors.test.ts | 12 +++----
 test/js/bun/http/proxy-stress-helpers.ts     | 49 +++++++++++++++++++++-------
 2 files changed, 43 insertions(+), 18 deletions(-)
```

</details>

**gate history** · 2 passed · 0 rejected · iteration 1

<details><summary>evidence per changed file</summary>

```
file                                          reads  edits  tests
test/js/bun/http/proxy-stress-errors.test.ts      1      3      0
test/js/bun/http/proxy-stress-helpers.ts          2      4      0
```

</details>

<!-- robobun:evidence:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants