test-tonic: fail fast with stderr when cargo dies instead of a 150s hook timeout

[robobun]

test/js/third_party/grpc-js/test-tonic.test.ts went red on the darwin-aarch64-15.1 lane in build 71849:

error: rustup could not choose a version of cargo to run, because one wasn't specified explicitly, and no default is configured.

✗ test tonic server > (unnamed) [150000.00ms]
  ^ a beforeEach/afterEach hook timed out for this test.
TypeError: undefined is not an object (evaluating 'server.kill')

Cause

startServer() read stdout chunk-by-chunk; when the process exits before printing Listening on, the reader returns {done: true}, the loop breaks, and await promise waits on a promise that is never settled. The 150s is the hook timeout, not a diagnosable error, and afterAll then throws on server.kill() with server still undefined.

The trigger on that agent was an infra change: the host agent on darwin-test-arm64-1 was reconfigured on Jul 9 to run as a user with /opt/rust/bin in PATH but no RUSTUP_HOME/CARGO_HOME in its profile, so the rustup shim resolved but had no toolchain. That has been fixed on the box (the agent user now has the same .profile exports the previous user had).

Fix

This PR keeps the existing skipIf(!cargoBin || ...) unchanged (no new skipping) and only hardens startServer():

• Accumulate stdout chunks and parse the address only once a newline-terminated Listening on <addr> line is present, so a chunk boundary mid-line cannot yield a partial address.
• If stdout closes without that line, throw tonic server exited (<code>) before reporting an address: followed by the captured stderr and exit code, instead of awaiting an unresolved promise until the hook timeout.
• Pipe and concurrently drain stderr so a chatty compile cannot fill the pipe buffer and wedge the child.
• Forward RUSTUP_TOOLCHAIN alongside RUSTUP_HOME/CARGO_HOME for agents that pin via env.
• Null-guard and await server?.kill() in afterAll.

Verification

With RUSTUP_HOME pointed at an empty dir (reproduces the original CI state):

• before: beforeAll hangs to the hook timeout, then afterAll throws undefined is not an object (evaluating 'server.kill')
• after: beforeAll throws in ~14s with tonic server exited (1) before reporting an address: followed by the rustup could not choose a version of cargo to run... message

With a working toolchain the tonic server compiles, Listening on 127.0.0.1:<port> is parsed, and the flow-control test runs as before.

Test-only change; no src/ diff.

---

no test proof · iteration 4 · Platform-specific test-only change; deferring to CI.

[robobun]

^{Updated 12:04 PM PT - Jul 15th, 2026}

⏳ @robobun, your commit 9b00b06 is still building in Build #73331, but has 2 failures so far (All Failures):

• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64-baseline
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64
• test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js - SIGABRT on 🐧 13 x64-asan

[coderabbitai]

Walkthrough

Changes

Tonic test startup

Layer / File(s)	Summary
Cargo availability and teardown test/js/third_party/grpc-js/test-tonic.test.ts	The test probes cargo using the configured Rust environment, skips when cargo is unusable, and conditionally kills the server during teardown.
Server process startup and readiness test/js/third_party/grpc-js/test-tonic.test.ts	Server stderr is captured and drained while stdout is buffered until the listening address appears; early exits now include captured diagnostics and cleanup.

🚥 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 clearly matches the main change: failing fast with stderr instead of waiting for a 150s hook timeout.
Description check	✅ Passed	The description covers what changed and how it was verified, even though it uses Cause/Fix/Verification instead of the template headings.

---

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

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/third_party/grpc-js/test-tonic.test.ts`:
- Around line 39-43: Condense the explanatory comment immediately above the
cargo/toolchain probe to no more than three lines, preserving the key details
about Bun.which finding the rustup shim, missing default toolchains, and probing
with the same environment and outside-repository working directory.
- Around line 146-147: Update the afterAll teardown hook to await completion of
the child server shutdown and temporary-directory cleanup before returning. Use
the server teardown mechanism around server?.kill() and make the hook
asynchronous while preserving safe handling when no server exists.
- Around line 121-126: Update the readiness parsing in the server-startup helper
around the “Listening on” check to buffer incoming stdout chunks until a
complete newline-terminated readiness line is available. Only then extract and
trim the address, preserving the existing killServer return behavior and
avoiding undefined or partial addresses from split chunks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 97b1314a-56d8-479d-a4f2-ed3f141d7478

📥 Commits

Reviewing files that changed from the base of the PR and between 9657f37 and 29d54ce.

📒 Files selected for processing (1)

• test/js/third_party/grpc-js/test-tonic.test.ts

[claude]

LGTM — test-only hardening of the tonic suite's cargo gate and startup readiness loop.

Checked that cargoWorks short-circuits on !cargoBin before spawning, and that the cargoBin! assertion is guarded by describe.skipIf(!cargoWorks ...).
Verified the fixture prints via println!("Listening on {}", addr) so the newline-terminated parse in the new read loop is sound.
Confirmed stderr.text() is started before the stdout read loop, so a piped stderr can't fill and wedge the child on the happy path even though the promise is only awaited on failure.

Extended reasoning...

Overview

Single-file, test-only change to test/js/third_party/grpc-js/test-tonic.test.ts. It replaces the Bun.which("cargo") presence check with a cargo --version probe (same stripped env, outside-repo cwd) so the suite skips when the rustup shim has no default toolchain; rewrites the stdout readiness loop to accumulate chunks, wait for a newline-terminated Listening on <addr> line, and throw with captured stderr + exit code if the child dies first; pipes and concurrently drains stderr; and null-guards/awaits server?.kill() in afterAll. RUSTUP_TOOLCHAIN is now forwarded and the env object is deduplicated between the probe and cargo run.

Security risks

None. No src/ changes, no new external inputs. The only new spawn is cargo --version with an explicit env allowlist and ignored stdio.

Level of scrutiny

Low. This is CI test infrastructure for a third-party integration test that already conditionally skipped when cargo was absent; the PR just tightens the skip predicate and turns a 150s hook-timeout into either a fast skip or a diagnosable throw. Worst case is a false skip on an agent where cargo works — strictly better than the current 150s red on every PR sharded to that lane.

Other factors

All three CodeRabbit review comments (comment length, buffer-to-newline, await teardown) were addressed in ca3add8 and confirmed resolved. I cross-checked fixtures/tonic-server/src/main.rs:33 — it uses println!, so the \n-terminated parse is correct. The stderrPromise is unawaited on the happy path but is actively draining the pipe, and --quiet keeps its volume bounded. No CODEOWNERS entry covers this path. The bug hunting system found nothing.

[robobun]

Updated in 9b00b06 (rebased onto main; diff unchanged) after the discussion with @dylan-conway: the skipIf is back to the original !cargoBin || !releases[release] (no new skipping). What remains is the startServer() hang fix: when cargo exits before printing Listening on, beforeAll now throws with the captured stderr and exit code in seconds instead of timing out at 150s, and afterAll awaits and null-guards the kill.

The underlying break on darwin-aarch64-15.1-1 was the agent user missing the RUSTUP_HOME=/opt/rust / CARGO_HOME=/opt/rust profile exports after the Jul 9 reconfig; /Users/ciadmin/.profile has been written on that box to match the previous user's, so cargo resolves a toolchain there again.

CI on 9b00b06 (build 73331) finished: test-tonic.test.ts is green on every lane, including :darwin: 14 aarch64 on agent darwin-aarch64-15.1-1 (the lane that was timing out in build 71849). The remaining red is unrelated main breaks already being handled elsewhere:

• test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js SIGABRT on :linux: 13 x64-asan (see process: defer termination across lazy PropertyCallback builders #33966 / worker_threads: don't abort when terminate() interrupts a lazy property builder #33418)
• test/js/node/test/parallel/test-net-connect-memleak.js on :alpine: 3.23 x64 / x64-baseline

Diff is test-only and ready to merge.

[claude]

LGTM on the code — test-only hardening of startServer() that turns the 150s hook timeout into a fast, diagnosable failure. The one inline nit is about the PR title/description going stale after df96134 (it now fails fast rather than skips on an unusable toolchain); worth updating before squash-merge so the commit subject matches behavior.

Extended reasoning...

Overview

Single-file, test-only change to test/js/third_party/grpc-js/test-tonic.test.ts. The startServer() helper is rewritten to: forward RUSTUP_TOOLCHAIN; pipe and concurrently drain stderr via server.stderr.text(); accumulate stdout chunks and only return once the full newline-terminated Listening on <addr> line is present; and, if stdout closes without that line, await [stderrPromise, server.exited] and throw with the captured stderr and exit code. afterAll is now async and null-guards server?.kill(). No src/ changes.

Security risks

None. This is CI test scaffolding for a third-party gRPC interop test. No auth, crypto, or user-facing surface is touched; the only new env read (RUSTUP_TOOLCHAIN) is forwarded verbatim to a child cargo process, same as the existing CARGO_HOME/RUSTUP_HOME forwarding.

Level of scrutiny

Low. Test-infrastructure change that strictly improves failure diagnostics. The rewrite directly follows the repo's own testing guidance in CLAUDE.md: buffer raw stdout chunks to the protocol's framing before asserting, drain pipes concurrently so a chatty compile can't deadlock the child, and wire every failure path to throw rather than await a never-settled promise. I traced the new loop: done is checked after the marker scan so a final chunk that completes the line is still parsed; on the success path the un-awaited stderrPromise keeps draining in the background until the server is killed in afterAll, which is the intended behavior; on the error path server.exited is awaited before killServer() re-awaits it, which is a harmless no-op.

Other factors

All three CodeRabbit threads (comment length, wait-for-full-line, await teardown) were addressed in ca3add8 and confirmed resolved. CI on ca3add8 was green on the previously-failing darwin-aarch64-15.1 lane. df96134 only removes the cargoWorks probe added in 29d54ce, reverting the gate to the original skipIf(!cargoBin || !releases[release]) — a strict reduction relative to the CI-verified revision. The one open item is the inline nit: after df96134 the misconfigured-toolchain case fails fast rather than skips, so the PR title and first Fix/Verification bullets are stale. That's a metadata fix, not a code defect, and the inline comment spells out the suggested retitle.

[claude]

LGTM — my earlier note about the stale title/description has been addressed, and the code is unchanged since then.

What was reviewed:

• stdout buffering waits for the newline-terminated Listening on <addr> line (verified println! in tonic-server/src/main.rs:33 emits the trailing \n).
• stdout-closed path awaits [stderrPromise, server.exited] then throws with the captured stderr — no more never-settled promise.
• stderr: "pipe" is drained concurrently via .text(); afterAll null-guards and awaits kill().

Extended reasoning...

Overview

Test-only change to test/js/third_party/grpc-js/test-tonic.test.ts hardening startServer() so that when the spawned cargo run exits without printing the readiness line, beforeAll throws immediately with the captured stderr and exit code instead of hanging on a never-settled promise until the 150s hook timeout. Also: forwards RUSTUP_TOOLCHAIN, switches stderr from inherit to a concurrently-drained pipe, buffers stdout to a newline-terminated Listening on line before parsing the address, and null-guards/awaits server?.kill() in afterAll. The skipIf(!cargoBin || !releases[release]) gate is unchanged. No src/ changes.

Security risks

None. This is CI test harness plumbing for a third-party gRPC integration test; no user-facing code, no auth/crypto/permissions, no new external inputs beyond forwarding an existing env var to a child process.

Level of scrutiny

Low. Single test file, ~30 lines of diff, mechanical fix for a diagnosed hang. The success path is behaviorally identical (returns {address, kill} once the server prints its address); only the failure path changes from hang → diagnostic throw. I confirmed the fixture at tonic-server/src/main.rs:33 uses println!("Listening on {}", addr), so the trailing newline the parser now requires is always present.

Other factors

All three CodeRabbit findings (buffer-to-newline, await teardown, comment length) were addressed in follow-up commits and marked resolved. My prior review's only concern was that the PR title/description were stale after the cargoWorks probe was reverted; the author has since retitled the PR and rewritten the Fix/Verification sections to match the current behavior (fail-fast, not skip). The bug-hunting system found nothing. No outstanding reviewer comments remain.

[robobun]

The agent-user change on darwin-test-arm64-1 surfaced one more issue: /opt/rust/registry is still owned by administrator and isn't group-writable, so once a new transitive dep (http-body-util 0.1.4, released 2026-07-13) needed downloading, cargo hit Permission denied. #34291 points the test's CARGO_HOME at its own cache dir so it no longer depends on the system cargo home being writable.