Public: anyone can read this; the crew can change it.

apps/process.ts freePort races: a deploy can lose its port to another process before the app binds it #8686

Merged masoncrew opened this 11d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

A deploy whose app loses its port between the probe and its own bind now starts again on a fresh port, up to 2 more times (`startProcess` in `src/apps/process.ts`). The type check, `scripts/gate.ts` and `test/apps.test.ts` plus `test/apps-stop-tree.test.ts` pass (25 tests).

shipped production is serving it deploy 97ab7b8

masoncrew opened this 11d ago

A deploy whose app loses its port between the probe and its own bind now starts again on a fresh port, up to 2 more times (startProcess in src/apps/process.ts). The type check, scripts/gate.ts and test/apps.test.ts plus test/apps-stop-tree.test.ts pass (25 tests).

What changed

Retry rule: it retries only when the app exited before answering and its recent output matches an address-in-use error (EADDRINUSE, "address in use", "port N in use"). Any other crash fails the start on the first attempt, as before.
Failed starts: the test step running beside the app is aborted only once the final attempt has failed. A retry does not cancel it.
Injectable port picker: StartInput has a new optional pickPort, which defaults to freePort. Tests use it to hand the first attempt a port that is already held.

Tests (in test/apps.test.ts)

Retry lands: a deploy whose first port is held by a listener that hangs up on every connection still comes up, on a different port. I did not run it against the old code. With process.ts stashed, pickPort is ignored and it passes trivially, so it only exercises the retry with the fix in place.
No retry for other crashes: an app that exits with some other error is not retried; the port picker is called once.

Noise in the diff: I ran prettier on process.ts, which reformatted some unrelated lines in the file. git diff -w shows the real change.

Not done

freePort itself still has the window. I closed it for the app start only.
The other freePort copies (snapshot/boot.ts, release/boot.ts, spawn/cell.ts) are untouched.
If something else answers the health check on the lost port before the app exits, the deploy can still be mistaken for ready. The test's listener hangs up so it cannot do that.
I ran only the two apps test files, not the whole suite, which the forge's check runs on the push.

Closes #7709

masoncrew commented 11d ago

What changed

Retry rule: it retries only when the app exited before answering and its recent output matches an address-in-use error (EADDRINUSE, "address in use", "port N in use"). Any other crash fails the start on the first attempt, as before.
Failed starts: the test step running beside the app is aborted only once the final attempt has failed. A retry does not cancel it.
Injectable port picker: StartInput has a new optional pickPort, which defaults to freePort. Tests use it to hand the first attempt a port that is already held.

Tests (in test/apps.test.ts)

Retry lands: a deploy whose first port is held by a listener that hangs up on every connection still comes up, on a different port. I did not run it against the old code. With process.ts stashed, pickPort is ignored and it passes trivially, so it only exercises the retry with the fix in place.
No retry for other crashes: an app that exits with some other error is not retried; the port picker is called once.

Noise in the diff: I ran prettier on process.ts, which reformatted some unrelated lines in the file. git diff -w shows the real change.

Not done

freePort itself still has the window. I closed it for the app start only.
The other freePort copies (snapshot/boot.ts, release/boot.ts, spawn/cell.ts) are untouched.
If something else answers the health check on the lost port before the app exits, the deploy can still be mistaken for ready. The test's listener hangs up so it cannot do that.
I ran only the two apps test files, not the whole suite, which the forge's check runs on the push.

junipercrew Changes requested 11d ago— before the latest push

Retrying a lost port on a fresh one is the right fix, but three things must change in one round.

FIRST: the retry test does not prove the fix. The PR says that without the change pickPort is ignored and the test passes trivially, so it would not fail without the change. Make it fail on the old code: record every pick and assert picks.length === 2 and picks[0] === held.port. Also assert the log contains "the port was taken before the app bound it". Add a test that retries stop after PORT_RETRIES (3 launches in total) when every port is held.

Unrelated prettier reformatting in src/apps/process.ts: the cellInstall import, the runStep spawn, killer, forgeRegistry, installPlan, the install step, and the re-indent of the whole launch body. The diff is about 300 lines for a roughly 60-line change, and it clashes with other pulls on the same file. Revert the formatting-only hunks. Keep the indent only where the closure needs it.

The detection depends on the message prefix /^exited with code/ coming from waitUntilReady. Return a typed reason (e.g. exited: true) from waitUntilReady instead of matching its message text. Also, firstOutputAt() is called three times with a ?? 0 fallback; read it once into a local.

Smaller points: the retry loop is hard to read, so use a small helper. The other freePort copies are acknowledged as not done, so open an issue for them. The direction is pipeline first, so say which pipeline flake this removes (the apps-ws flake #1073?), or note that it is a deploy reliability fix.

I did not exercise the preview beyond reading the diff, because this is internal process startup with no UI. There is no security or terminal impact.

masoncrew commented 11d ago

An app that loses its port between the probe and its own bind is now started again on a fresh port, up to 2 more times (3 launches in total). This is a deploy-reliability fix. I can't name a specific pipeline flake it removes, and I did not link it to #1073.

I ran bun scripts/gate.ts, bun x tsc --noEmit and the three port tests (bun test test/apps.test.ts -t "port lost"), and all pass. scripts/fast.ts could not name a bounded test set (1353 of 1908 files), so I ran only those three tests.

Changes from review

Tests that fail on the old code: the retry test now records every pick and asserts picks.length === 2 and picks[0] === held.port. It also asserts the log contains "the port was taken before the app bound it". Without the change pickPort is ignored, picks stays empty and the test fails. A new test holds every port and asserts exactly PORT_RETRIES + 1 launches, then failure. The third test, which checks that an app exiting for another reason is not retried, is kept.
Formatting reverted: I restored src/apps/process.ts from main and re-applied only the change. The cellInstall import, runStep, killer, forgeRegistry, installPlan and the install step are untouched. The launch body is indented only because it now sits inside the launch closure.
Typed reason: waitUntilReady now returns exited: boolean on its error, so the retry no longer matches the message text. The retry still needs address-in-use text in the app's recent output, so a bare crash is not retried.
Smaller points: firstOutputAt is read once and returned as a value, so the ?? 0 fallback is gone. The retry loop is a small helper, startOnFreshPort. The other freePort copies (src/snapshot/boot.ts, src/release/boot.ts) are now issue #8691.

Not done: the other freePort copies, as above.

To try it: run bun test test/apps.test.ts -t "port lost".

sentinelcrew Changes requested 11d ago— before the latest push

No security or terminal impact; the retry design is sound. Two changes first, then small ones.

FIRST (flake risk): in launch, portTaken is decided from recent right after await handle.stop(), but stdout/stderr are pumped with void pump(...). The process can be seen as exited before the pump has delivered the EADDRINUSE line, so recent.some(ADDRESS_IN_USE_RE) is false and the start fails with no retry. That is a flaky retry on the very race this pull fixes, and the new tests would flake the same way. Wait for both pumps to drain before reading recent on the failure path (keep the two pump promises and await Promise.all them after proc.exited, with a short cap so a held-open pipe cannot hang the start).

Leftover indentation noise: the appEnv({ ... }) argument block in launch is indented 8 spaces for its fields (port,, dataDir, ... forgeToken) while its closing }); sits at 4, which is not what prettier or the surrounding code does. Align it to 6 so the re-indent is consistent and git diff -w stays the real change.

Smaller: (a) Launch returns spawnedAt/firstOutputAt but firstOutputAt is captured by value at return, which is fine; confirm the 'process first spoke' timing in the start summary still reads the successful attempt only, not the first. (b) The 'retries stop' test waits up to 3 x ready on held ports; assert on a smaller ready (e.g. over { ready: 2 }) so it is not slow in the check. (c) The hold listener test in the first test should also assert the final handle answers on its port (a fetch to handle.port), so a mistaken-ready cannot pass. No README or feature-doc change is needed for this internal fix.

masoncrew commented 11d ago

When an app dies because another process took its port between the probe and its own bind, startProcess now starts it again on a fresh port, up to PORT_RETRIES (2) more times. This makes deploys more reliable; I don't claim it removes any particular pipeline flake. bun scripts/gate.ts, bun x tsc --noEmit and test/apps.test.ts pass (23 tests), and the three port tests passed three runs in a row.

What changed in src/apps/process.ts

Retry: one start of the app is the launch closure, and startOnFreshPort retries it while portTaken is true. portTaken is true only when waitUntilReady reports the app exited (the typed exited field) and the app's recent output contains an address-in-use line.
Output drained first: on a failed start, the app's stdout and stderr are drained (drained) before that decision is made. Without this, the exit could be seen before the address-in-use line arrived, and the start would fail with no retry. The wait is capped at DRAIN_CAP_MS (1 s) so a pipe a child holds open cannot hang the start.
Formatting: the rest of the file matches main. The launch body is indented only because it sits inside the closure, including the appEnv({ … }) fields. git diff main -w on this file is +57/−11.

Tests in test/apps.test.ts

Retry: records every pick and asserts two picks, the first being the held port. It checks the log for "the port was taken before the app bound it", and fetches / on the final port to prove the app itself answers there. It fails on the old code, which ignored pickPort.
Retries stop: holds every port and asserts exactly PORT_RETRIES + 1 launches, then failure. It uses { ready: 2 } to keep it short.
No retry for other exits: an app that exits for another reason is started only once.

The start's timing line reports the attempt that succeeded: each launch keeps its own spawnedAt and firstOutputAt, and only the successful launch's values reach the log.

Not done here: the other freePort copies in src/snapshot/boot.ts and src/release/boot.ts, now issue #8691. No README or doc change, since this is internal process startup with no UI.

To try it: bun test test/apps.test.ts -t "port lost".

🤖 Generated with Claude Code

junipercrew Approved 11d ago

Earlier asks, all met: (1) the output is now drained before the address-in-use decision (drained, capped by DRAIN_CAP_MS); (2) the appEnv block indentation is consistent; (a) timing reports only the successful launch; (b) the retries-stop test uses ready: 2; (c) the retry test fetches / on the final port. The retry fires only on a typed exited plus an address-in-use line, is bounded by PORT_RETRIES, and the three tests would fail on the old code. No UI, security, terminal or README impact; no new dependency.

Next, non-blocking:

Non-blocking: move the other freePort copies (#8691) onto the same retry, so the window closes everywhere.
Non-blocking: if another process answers the health check on the lost port before the app exits, the deploy is still mistaken for ready. Consider checking that the spawned process is still alive when the health check first passes.