fix(runtime): report an inconclusive port probe as unknown, not as exit

[marcusrbrown]

Two follow-ups from the #1519 review. Both are cases where an unknown answer resolved to the optimistic one, on paths built specifically to be pessimistic.

The probe reported exited when it meant unknown

waitForServerQuiescence polls the OpenCode child's port until connections are refused, and the socket timeout resolved finish(false) — where false means port closed, which the caller reads as quiesced and returns on immediately.

So a connect that neither completed nor was refused, the exact case the timeout was added for, reported that the child had exited. That suppresses the warning telling an operator their checkpoint may have raced a live writer.

Everything else in this design resolves the other way: verifyDatabaseUsable returns usable for an unrecognized throw, isStructuralCorruptionError returns false unless SQLite positively says otherwise. This was the one place "I don't know" became "it's fine" — the same polarity class as the issue that blocked round four of #1519.

Now returns true: an inconclusive attempt keeps polling, and only the real 5s deadline produces quiesced: false.

Testing it required making the socket injectable, since a real one either connects or is refused and never reaches the timeout branch. A fake socket drives it directly rather than depending on a black-holed address behaving consistently in CI.

A failed download left a partial database nothing would examine

A mid-transfer failure in the S3 adapter left a partial opencode.db on disk. mainDbRestored stays false, restore reports a miss and falls through to the Actions cache; if that also misses — the normal case for a repository leaning on S3 — the repair block is skipped entirely and a truncated database reaches bootstrap unprobed.

The obvious cleanup condition (mainDbRestored === false && failed > 0 at the restore layer) is unsafe, and this is worth stating because it was my first instinct: syncSessionsFromStore increments failed for any key in the batch. An unrelated sidecar failure satisfies it while opencode.db was never attempted — and on a self-hosted runner with a pre-existing local database, that deletes live session state the failed download never touched.

Fixed at the write site instead. download() unlinks localPath only when the pipeline itself throws, meaning the stream was open and a partial write actually happened. A rejection before the pipeline is reached never touches the path, so an existing file is provably safe — pinned by its own test.

Verification

Both fixes were run against their pre-fix shapes and fail there: the polarity test with expected false to be true, the partial-download test with promise resolved \"undefined\" instead of rejecting.

Gates: types 0, lint 0, dist/ rebuilt. Full suite green — runtime 675, action 1710, workspace-agent 208, harness 317, gateway 3280, evals 170.

[fro-bot]

Verdict: PASS

Both fixes are correct, minimal, and resolve unknowns in the direction the surrounding design already resolves them. I verified the changes against source, re-ran the affected suites, confirmed check-types is clean across all four workspace packages, and confirmed the committed dist/ actually carries both changes (not just a rebuild churn).

Probe polarity (packages/runtime/src/agent/server.ts:81) — socket.setTimeout(timeoutMs, () => finish(true)) is right. The socket timeout exists for a connect that neither completes nor is refused; resolving false there meant "port closed," which waitForServerQuiescence reads as quiesced and returns on immediately (server.ts:134). Resolving true makes an inconclusive attempt a no-op for the loop, and the 5s deadline at server.ts:138 is what produces quiesced: false. The only path that can now claim the child exited is a positive error.

The destroy() → removeAllListeners() reorder is also an improvement over the prior order, not just a comment: a synchronous error emitted during teardown now still lands on an attached listener, where it hits the settled guard instead of Node's uncaught-exception path. destroy() with no error argument doesn't emit error asynchronously, so the new ordering has no counterpart risk.

Partial download (packages/runtime/src/object-store/s3-adapter.ts:183-198) — fixing at the write site rather than at the restore layer is the right call, and the PR body's reasoning for rejecting mainDbRestored === false && failed > 0 is sound: syncSessionsFromStore (content-sync.ts:149-166) increments failed per key with no attribution back to opencode.db, so that predicate would delete live local state a sidecar failure never touched. Scoping cleanup to a pipeline() rejection keeps the blast radius to a file this call opened.

Security: no new exposure. download()'s localPath is validated by validation.ts and resolved inside the storage root before content-sync.ts:149 ever calls it, so the new unlink can't be steered outside the root by a hostile key.

Public API: isPortOpen and QuiescenceProbeSocket are newly exported from the module but are not re-exported from packages/runtime/src/agent/index.ts (which still exports only bootstrapOpenCodeServer, ensureOpenCodeAvailable, and the two types). No package surface change.

Verification I ran locally:

• vitest run packages/runtime/src/agent/server.test.ts packages/runtime/src/object-store/s3-adapter.test.ts — 54 passed
• vitest run src/services/cache/restore-save-flow.test.ts — 9 passed
• bun run check-types — runtime, action, harness, gateway all exit 0
• dist/artifact-D0_f4uS9.js carries both changes: the probe minifies to o.setTimeout(n,()=>s(!0)) with o.destroy(),o.removeAllListeners(), and the download to catch(e){throw await Gn.unlink(i).catch(()=>{}),e}. The dist/ rebuild is real, not incidental.

Blocking issues

None.

Non-blocking concerns

1. The cleanup comment claims more than the code guarantees — s3-adapter.ts:191-195

The comment asserts the unlink can only reach "a file this exact pipeline() call just opened for writing." That holds for the mid-transfer case, but not for an open failure. If createWriteStream(localPath) fails to open the destination at all — localPath is a directory, or the existing file is EACCES for this process — pipeline() rejects before truncating, and the cleanup then unlinks a file this attempt never wrote a byte to. unlink needs write permission on the directory, not the file, so a read-mode-restricted pre-existing file in a writable storage root is removable.

Practically near-zero here (the storage root is created by this process, and the recovery is a clean miss rather than data loss in the paths that matter), so I'd leave the behavior alone. But the comment is the durable artifact a future reader will trust when deciding whether this cleanup is safe to widen — worth narrowing the claim to "a GetObject/network rejection above this line never reaches here" and dropping the stronger "only a file this call opened."

2. Shutdown worst case is now the full 5s, and that trade isn't quantified anywhere

Correct and intended — but the practical effect of the polarity flip is that a genuinely inconclusive probe changes shutdown() from returning in ~one poll interval (wrongly) to burning DEFAULT_SHUTDOWN_QUIESCE_TIMEOUT_MS = 5000ms (honestly). One server per run, so it's bounded at +5s per run and not a real concern. Since the comment at server.ts:56-59 already explains the mechanism, a parenthetical on the cost ("up to the full 5s deadline instead of one poll interval") would make the trade legible to whoever next tunes these constants.

3. Windows: the unlink can silently no-op — s3-adapter.ts:196

pipeline() destroys the write stream on rejection, but the fd close is asynchronous; on Windows an unlink racing an open handle fails EBUSY/EPERM and is swallowed by .catch(() => {}), leaving exactly the partial file the fix targets. No workflow in this repo uses a Windows runner, so this is theoretical for first-party CI — noting it only because the action is consumable by third parties who might.

Missing tests

1. The polarity is pinned in isPortOpen but not at its call site.

server.test.ts:433 pins that the timeout branch resolves true, which is the actual bug. But waitForServerQuiescence calls isPortOpen(hostname, port, pollIntervalMs) at server.ts:133 without an injector, so nothing pins that an inconclusive probe keeps the loop polling. A regression that inverts the read at the call site — if (stillOpen) return {quiesced: true} — passes every test in this PR, and reintroduces the exact bug being fixed one line away from where it was fixed. Threading the same optional connect parameter through waitForServerQuiescence and asserting "N inconclusive probes then a deadline yields quiesced: false, never true" would close this cheaply and pin the invariant end to end.

2. No coverage for the open-failure case in download().

The two new tests cover mid-transfer failure (s3-adapter.test.ts:314) and pre-response rejection (:330), which is a good pair. The third shape — the GetObject succeeds but createWriteStream fails to open — is the one that would falsify the comment in concern #1. A test writing a directory (or a mode-0o444 file) at localPath and asserting whatever behavior you decide is correct would pin it.

Risk assessment: LOW

Regression likelihood is low: both changes are narrow, both are pinned by tests that fail against their pre-fix shapes, and both move behavior from optimistic to pessimistic on paths already designed to be pessimistic — the failure mode of getting this wrong is a slower shutdown or a redundant delete, not corrupted or lost state.

Blast radius is small and asymmetric in the right direction. The probe change affects only bootstrapOpenCodeServer(...).shutdown (server.ts:238-241) and can at worst add 5s to a run; the S3 change affects only the failure branch of download(), and its worst case (deleting an already-truncated file) is strictly better than the status quo of a partial opencode.db reaching bootstrap unprobed. No security exposure — path validation happens upstream and is unchanged. dist/ is verified in sync with source, so no stale-bundle risk.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	33601132062
Cache	hit
Session	ses_f9f16554cffedtLaSePnFxpvBQ

[marcusrbrown]

All three closed at 4fcc0eae2.

The missing test was the real finding, and it bites. You were right that the polarity was pinned inside isPortOpen but not where it is read — inverting the call site to if (stillOpen) return {quiesced: true} passed every test in the PR, reintroducing the bug one line from where it was fixed. The optional connect parameter now threads through waitForServerQuiescence (defaulted, so the production call site is unchanged), and a test drives several poll cycles of nothing-but-timeouts asserting the only reachable outcome is quiesced: false at the deadline. Confirmed against the inverted read: expected { quiesced: true } to deeply equal { quiesced: false }.

The comment now separates guaranteed from merely rare. Your open-failure case is right — if createWriteStream cannot open the destination at all, pipeline() rejects before truncating and the unlink still fires against a file this attempt never wrote to, since unlink needs permission on the containing directory rather than the target. Behavior unchanged as agreed; the claim no longer overstates. That is the fifth comment across this work asserting a property its own code did not fully hold, which is why it was worth a round rather than a note.

Cost of the polarity flip recorded at the constant: an inconclusive probe now rides out the full 5s instead of returning after one poll interval — bounded at one server per run, intended, and no longer something the next person tuning those constants has to rederive.

Skipped the Windows EBUSY note as agreed; no Windows runner exists in this repo's matrix.

Gates: types 0, lint 0, dist/ rebuilt. Runtime 675 → 676. One further environmental flake surfaced under full-suite load (packages/gateway/src/program.test.ts), passing in isolation and on an immediate re-run; recording it rather than adding it to the known-flake list, since I have not confirmed it against a pristine worktree the way I did the other three.

[fro-bot]

Verdict: PASS

Re-review after 4fcc0eae2. All three findings from the prior round are closed, and the one that mattered — the untested call-site polarity — is closed properly rather than papered over. I re-verified the source, ran the affected suites (including repeat runs to probe the new test's timing sensitivity), confirmed check-types is clean across all four workspace packages, and confirmed the committed dist/ carries the new signature, not just the previous round's changes.

The call-site gap is genuinely closed. waitForServerQuiescence now takes the same optional connect injector (server.ts:112) and forwards it to isPortOpen at server.ts:139, so the loop's own read of that return value is directly assertable. The new test (server.test.ts, "never reports quiesced: true from repeated inconclusive probes") drives a connect() that fires nothing but setTimeout, and asserts the deadline is the only thing that can end the wait. The regression I described — if (stillOpen) return {quiesced: true} — now fails against it: alwaysInconclusive → isPortOpen resolves true → the inverted read returns {quiesced: true} where the test demands {quiesced: false}. The invariant is pinned end to end.

The microtask ordering in the fake is correct, which is worth stating since it is load-bearing and non-obvious: queueMicrotask(fake.fireTimeout) is scheduled inside connect(), but isPortOpen's promise executor is synchronous, so socket.setTimeout has assigned timeoutCallback before the microtask drains. The fake can't silently no-op into a false pass.

The connect parameter is additive and defaulted, so the production call site at server.ts:246 is unchanged, and neither waitForServerQuiescence nor QuiescenceProbeSocket is re-exported from packages/runtime/src/agent/index.ts (still only bootstrapOpenCodeServer, ensureOpenCodeAvailable, and the two types). No package surface change.

The comment now separates guaranteed from rare. s3-adapter.ts:186-199 explicitly names the createWriteStream open-failure case, states that pipeline() rejects before truncating, and records that unlink needs permission on the containing directory rather than the target — so the claim no longer outruns what the code holds. Behavior unchanged, as agreed. Cost recorded at server.ts:59-64: an inconclusive probe rides out the full DEFAULT_SHUTDOWN_QUIESCE_TIMEOUT_MS instead of returning after one poll interval, bounded at one server per run. That is exactly the note the next person tuning those constants needs.

Verification I ran:

• vitest run packages/runtime/src/agent/server.test.ts packages/runtime/src/object-store/s3-adapter.test.ts src/services/cache/restore-save-flow.test.ts — 64 passed
• server.test.ts alone, 5 consecutive runs — 21 passed each time (see non-blocking #1 for why I checked)
• bun run check-types — runtime, action, harness, gateway all exit 0
• dist/artifact-BzJocZnW.js carries the new signature, not a stale bundle: pl(e,t=5e3,n=100,r=dl) is the four-arg waitForServerQuiescence with the defaulted injector, and it forwards through as fl(i,a,n,r). The probe still minifies to once(\error`,()=>s(!1))with the timeout resolving!0, and the download to unlink(i).catch(()=>{})`.

Blocking issues

None.

Non-blocking concerns

1. The new call-site test has a small real-timer flake surface — server.test.ts, "never reports quiesced: true from repeated inconclusive probes"

waitForServerQuiescence('http://127.0.0.1:4096', 60, 10, alwaysInconclusive) runs on real timers with a 60ms budget and a 10ms poll interval, then asserts connectCalls > 1. The do/while checks Date.now() < deadline after the first iteration, so a ≥60ms stall during the first delay(10) — a contended runner with parallel vitest workers is exactly where that happens — exits the loop with connectCalls === 1 and fails the assertion.

Importantly this is asymmetric in the safe direction: the primary assertion (toEqual({quiesced: false})) is stall-immune, since a stalled loop still can't produce quiesced: true. The worst case is a spurious red, never a wrong green, so the invariant this test exists to pin is not at risk. Five consecutive runs were clean here. If it ever does flake, the cheap fix is to drop the wall-clock coupling — assert on a call count the fake controls (e.g. resolve the deadline after N recorded probes) rather than on elapsed time.

2. A test title describes a weaker property than the test pins — s3-adapter.test.ts:330

"does not touch a pre-existing local file at a different path when the GetObjectCommand call itself rejects" — but the pre-existing file is written to localPath, the same path the download targets. That's what makes the test meaningful: it pins that a pre-pipeline rejection leaves the exact file the cleanup would otherwise delete untouched. A file at a different path being safe is trivially true and worth nothing. The title undersells the assertion, and a future reader trimming tests could reasonably mistake it for redundant coverage. Body comment is accurate; only the title is off.

Missing tests

1. The createWriteStream open-failure case is still uncovered.

Noted for completeness rather than as a new ask — you addressed it by narrowing the comment rather than adding a test, which I agreed to. The gap is that s3-adapter.ts:193-198 now documents a behavior (unlink fires against a file this attempt never wrote to) that nothing pins, so a future change to the try/catch boundary could silently contradict the comment. Low value relative to cost; the two shapes that matter (mid-transfer, pre-response) are both covered, and the documented case degrades to a clean miss.

Risk assessment: LOW

Unchanged from the prior round, and slightly better. The second commit is test and comment only — the sole production change is an additive, defaulted parameter on waitForServerQuiescence whose production call site passes nothing, so runtime behavior is byte-identical to the commit already reviewed. The polarity invariant is now pinned at both the function and its call site, which removes the specific one-line regression that would have passed the previous test set. dist/ is verified in sync with source, so no stale-bundle risk. Blast radius stays bounded at bootstrapOpenCodeServer(...).shutdown (at worst +5s on one server per run) and the failure branch of download(). No security exposure: path validation is upstream and unchanged.

The gateway program.test.ts flake you recorded is outside this diff and doesn't bear on the verdict; recording it as unconfirmed rather than adding it to the known-flake list is the right call.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	33603530975
Cache	hit
Session	ses_f9efb338bffeTEZOdTrKH2OWCW

[marcusrbrown]

Both closed at 200892198.

The timer coupling was real. connectCalls > 1 against a 60ms budget with a 10ms interval is exactly the shape you described — the do/while checks the deadline only after the first iteration, so one stalled delay() exits with a single probe. Widened to a 500ms budget with a 5ms interval so a stall cannot end the loop before a second probe, and recorded in the comment why the asymmetry you noted matters: the quiesced: false assertion is stall-immune, so the headroom only removes a spurious red and can never mask a wrong green.

The test title undersold its own assertion. "at a different path" would be trivially true and worth nothing; the file is at the same path the download targets, which is the whole point — a pre-pipeline rejection leaves untouched the exact file the cleanup would otherwise delete. Retitled to match what the body pins, since a future reader trimming tests was the realistic risk.

On the uncovered createWriteStream open-failure case: agreed on your own framing, and leaving it. The two shapes that matter are covered, and the documented case degrades to a clean miss. Worth noting the residual honestly though — the comment now describes a behavior nothing pins, so a future change to the try/catch boundary could contradict it silently. That is a real gap, just not one worth a fixture that has to make createWriteStream fail to open.

Gates: types 0, lint 0, 55 tests green across both touched files.

[fro-bot]

Verdict: PASS

Third review pass, after 20089219. The new commit is test and comment only — I diffed it against 4fcc0eae and confirmed the production surface is byte-identical to the commit already reviewed twice, including dist/ (git diff --stat 4fcc0eae..HEAD -- dist is empty, which is exactly right for a test-only change). Both prior rounds' non-blocking notes are closed. I re-verified the full diff against the merge base rather than trusting the earlier passes.

Both polarity fixes hold at source. isPortOpen resolves true on its own socket timeout (server.ts:86), and waitForServerQuiescence forwards the injector at server.ts:139 so the loop's read of that value is directly assertable. The invariant is double-pinned: inverting finish(true) → finish(false) fails isPortOpen's own polarity test, and inverting the call-site read to if (stillOpen) return {quiesced: true} fails the loop test. Neither regression can slip through the current suite.

Public API is unchanged. isPortOpen, QuiescenceProbeSocket, and waitForServerQuiescence are module-level exports but none is re-exported from packages/runtime/src/agent/index.ts — I read the full export list and grepped the workspace for external callers (grep -rn 'isPortOpen\|QuiescenceProbeSocket\|waitForServerQuiescence' src packages outside agent/server) and found zero. The new connect parameter is additive and defaulted, so server.ts:246 is untouched. No consumer-visible change.

Security: no new exposure. The unlink target is localPath, validated and root-resolved upstream in validation.ts before content-sync.ts ever calls download(). The new cleanup cannot be steered outside the storage root by a hostile key.

Verification I ran on the merge ref:

• vitest run on the three touched test files — 64 passed
• vitest run packages/runtime — 676 passed, 36 files
• server.test.ts alone, 3 consecutive runs — 21 passed each, stable
• bun run check-types — runtime, action, harness, gateway all exit 0
• bun run lint — 0 errors (8 pre-existing gateway warnings, unrelated to this diff); dist:check-hidden-unicode clean; 388 md links OK
• dist/artifact-BzJocZnW.js in sync with source: .setTimeout(n,()=>s(!0)), destroy(),o.removeAllListeners(), unlink(i).catch(()=>{}), and function pl(e,t=5e3,n=100,r=dl) — the four-arg waitForServerQuiescence with the defaulted injector

Blocking issues

None.

Non-blocking concerns

1. The commit subject overstates what the change did — 20089219

"decouple the quiescence test from the wall clock" is not what landed. The test is still wall-clock coupled: connectCalls > 1 still requires the first delay(5) to complete inside the 500ms budget, and the do/while still checks Date.now() < deadline only after iteration one. What changed is the margin — 60ms/10ms → 500ms/5ms, roughly 100x headroom. That is a fine fix and I agree the residual risk is negligible, but the flake surface is reduced, not removed, and the commit body's own reasoning ("so a stall cannot end the loop before a second probe") claims an impossibility the code only makes improbable.

Worth naming because it is the same class as the two findings this PR already spent a round on: the s3-adapter comment that claimed more than the code held, and the test title that claimed less. This one is a level up, in the commit message. The in-code comment at the call site is accurate (it says "headroom," not "decoupled"), so only the subject line drifts. Actual decoupling — resolving the deadline after N fake-recorded probes rather than after elapsed time — remains the cheap option if it ever does flake.

2. That test now dominates the file's runtime

server.test.ts spends ~500ms of its ~740ms total in this one case, since the loop genuinely runs its full budget across ~100 iterations. Absolutely trivial (sub-second, one file), but it is the direct cost of fixing the flake with headroom instead of with a fake-controlled deadline, and it is the second reason the counted-probes approach would have been strictly better.

3. Silent truncation is the one shape of this bug class still open — s3-adapter.ts:184

The fix covers a pipeline() rejection. It does not cover an S3 body that ends early without erroring: pipeline() resolves, no cleanup fires, ok(undefined) is returned, and a short opencode.db reaches bootstrap — the exact outcome the PR set out to prevent, reached by a different door. In practice Node's HTTP handler raises ERR_STREAM_PREMATURE_CLOSE on a truncated response, so this is mostly theoretical today. But response.ContentLength is right there on the GetObjectCommandOutput and is never compared against what was written, so the guarantee rests entirely on the transport layer rather than on anything this adapter checks. Out of scope for this PR; noting it as the remaining gap in the class, not as an ask.

4. The new assertions sit ahead of an unguarded holder.close() — restore-save-flow.test.ts:481-485

expect(store.upload).not.toHaveBeenCalled() and expect(store.objects.size).toBe(0) are inserted before holder.exec('COMMIT'); holder.close(), which is not in a finally. If either new assertion throws, an EXCLUSIVE-locked DatabaseSync handle leaks past the test. Pre-existing convention — save-ordering.test.ts:106 and checkpoint.test.ts:171 have the identical shape — so this is not a regression the PR introduced, just two more ways to reach the leak. Not worth changing here; worth a try/finally if that block is ever touched again.

Separately: store.objects.size is trivially 0 given createInMemoryStoreAdapter() is constructed fresh in this test and upload was already asserted uncalled. It is not redundant — it catches a write reaching the store through a path other than upload — but it is close, and the value is narrower than the comment suggests.

Missing tests

1. The createWriteStream open-failure case remains uncovered — s3-adapter.ts:193-199

Carried forward from round one, and I still agree with the resolution: you closed it by narrowing the comment rather than adding a test. The residual gap is that the comment now documents a behavior (unlink firing against a file this attempt never wrote to) that nothing pins, so a future change to the try/catch boundary could silently contradict it. The two shapes that matter — mid-transfer and pre-response — are both covered, and the documented case degrades to a clean miss. Low value relative to cost.

Nothing new. The three shapes worth pinning in this diff (isPortOpen polarity, the call-site read of it, and the two download() failure boundaries) are all covered, and each fails against its pre-fix shape.

Risk assessment: LOW

Lower than either prior round, because the only change since the last PASS is test text and one test's timing constants. Production behavior is byte-identical to 4fcc0eae, which was itself identical to 86abf65d except for an additive defaulted parameter that no production call site passes. dist/ is verified in sync and correctly untouched by the test-only commit, so there is no stale-bundle risk.

Blast radius is unchanged and asymmetric in the right direction. The probe change reaches only bootstrapOpenCodeServer(...).shutdown (server.ts:244-247) and its worst case is +5s once per run. The S3 change reaches only the pipeline() failure branch of download(), and its worst case — unlinking an already-truncated file, or in the documented rare case a file this attempt could not open — is a clean subsequent miss rather than the status quo of a partial opencode.db reaching bootstrap unprobed. No security exposure: path validation is upstream and unchanged, and no package export surface moved.

The one live regression vector left is a flaky red on the new call-site test under extreme runner contention, which fails safe by construction — a stalled loop still cannot produce quiesced: true, so the invariant the test exists to protect is never at risk from its own flake.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	33670831312
Cache	hit
Session	ses_f9c7e1a7cffeT1cTgy8FniLP51

[fro-bot]

Verdict: PASS

Fourth pass, after 5b25bc45. That commit is a merge of main into the branch and nothing else — git diff 20089219..HEAD is three files under docs/solutions/, and git diff 20089219..HEAD -- dist is empty. The production surface is byte-identical to the commit already approved, and the merge base is now main's tip, so this is fully rebased with no semantic conflict to inspect.

I re-derived the review from the diff against the merge base rather than trusting the three prior passes, and independently traced the consumer side of both changes.

Both polarity fixes hold, and the blast radius claim checks out at the consumer. isPortOpen resolves true on its own socket timeout (server.ts:86) and waitForServerQuiescence forwards the injector at server.ts:139. I followed ShutdownResult to its only consumer: cleanup.ts:119-132 warns on quiesced: false and proceeds. So the cost of the flip really is "up to +5s once per run, plus a warning that is now correctly emitted" — nothing gates on it, no phase is skipped. The comment at server.ts:59-64 is accurate.

The orphan-sidecar question the S3 fix could have opened is already closed elsewhere. Unlinking a partial opencode.db leaves mainDbRestored: false while sidecars from the same listing (content-sync.ts:135-164, lexicographic order puts opencode.db first) may still land. A stale -wal beside a main-restored db from the Actions-cache fallback would be the dangerous shape — but restore.ts:224-226 runs deleteDownloadedObjectStoreWal in a finally on every object-store path, including the miss. The residual -shm orphan is inert without a WAL, and it is not new: the pre-fix behavior (partial db left on disk) produced the same sidecar set.

Public API unchanged. isPortOpen, QuiescenceProbeSocket, and the new connect parameter are module-level only — packages/runtime/src/agent/index.ts:51-52 still exports bootstrapOpenCodeServer, ensureOpenCodeAvailable, OpenCodeServerHandle, ShutdownResult. A workspace grep outside agent/server* returns zero callers, so no positional-argument caller can be broken by the added parameter.

Security: no new exposure. The unlink target is localPath, produced by validateDownloadPath in content-sync.ts:137 before download() is ever reached, so the new cleanup cannot be steered outside the storage root by a hostile key.

Verification I ran on the merge ref:

• vitest run on the three touched files — 64 passed
• vitest run packages/runtime — 676 passed, 36 files
• server.test.ts alone, 3 consecutive runs — 21 passed each, ~725ms tests every time, no timing variance
• bun run check-types — runtime, action, harness, gateway all exit 0
• bun run lint — 0 errors (8 pre-existing gateway no-non-null-assertion warnings, none in this diff); dist:check-hidden-unicode clean; 394 md links OK
• dist/artifact-BzJocZnW.js in sync: setTimeout(n,()=>s(!0)) and unlink(i).catch(()=>{}) both present

Blocking issues

None.

Non-blocking concerns

1. pollIntervalMs: 0 silently disables the stall guard this comment now depends on — server.ts:139, new

waitForServerQuiescence passes pollIntervalMs directly as isPortOpen's timeoutMs, which becomes socket.setTimeout(timeoutMs, ...) at server.ts:86. Node documents net.Socket.setTimeout(0) as disabling the idle timeout — the callback stays registered as a 'timeout' listener and simply never fires. So waitForServerQuiescence(url, 5000, 0) reverts to exactly the pre-timeout behavior the comment at server.ts:47-51 says the timeout exists to prevent: a black-holed connect never settles, and the do/while stalls forever past its own timeoutMs, because the deadline is only checked between iterations and the loop never gets back there.

Not reachable today — the sole production call site (server.ts:246) takes both defaults, and DEFAULT_SHUTDOWN_QUIESCE_POLL_INTERVAL_MS is 100. What makes it worth naming is the asymmetry this PR introduced: server.ts:126-135 now guards port in code specifically so an invariant isn't "left as an assumption about what callers happen to pass," and one line below that block, the parameter that determines whether the stall guard exists at all is unvalidated. 0 is a plausible thing for a future caller to pass meaning "poll as fast as possible."

Cheapest fix is at the call: isPortOpen(hostname, port, Math.max(1, pollIntervalMs), connect), or fold pollIntervalMs into the existing guard block. Either is a one-liner.

2. The fake socket cannot catch that class of regression — server.test.ts, createFakeSocket

setTimeout: (_ms, onTimeout) => { timeoutCallback = onTimeout } discards the duration, so fireTimeout() fires regardless of what was passed. That is the right shape for pinning polarity, but it means no test in this file — including the new call-site one — observes the timeoutMs value at all. If you take concern #1, recording _ms into the returned handle makes the fix assertable for free.

3. That test still dominates the file's runtime. ~500ms of server.test.ts's ~725ms, since the loop genuinely runs its 500ms budget across ~100 iterations. Measured identically across three runs, so the round-three flake is gone — this is just the price of buying stability with headroom instead of a fake-controlled deadline. Trivial in absolute terms; noting it only because it's the same trade concern #2 would let you revisit.

4. Carried forward, unchanged and still agreed: the unguarded holder.close() the new restore-save-flow.test.ts:469-471 assertions sit ahead of (pre-existing convention, save-ordering.test.ts and checkpoint.test.ts share it), and silent truncation — a body that ends early without erroring resolves pipeline(), so response.ContentLength is never checked against bytes written. Both out of scope here.

Missing tests

1. The createWriteStream open-failure case remains uncovered — s3-adapter.ts:193-199. Carried from round one; I still agree with closing it by narrowing the comment rather than adding a fixture. The residual is that the comment documents a behavior nothing pins.

2. If you take concern #1, pin it. A waitForServerQuiescence(url, 50, 0, alwaysInconclusive) case asserting it returns {quiesced: false} rather than hanging would close both #1 and #2 in one test — but it needs the fake to honor _ms first, otherwise it passes vacuously.

Nothing else. The three shapes that matter in this diff — isPortOpen's polarity, the loop's read of it, and both download() failure boundaries — are each pinned by a test that fails against its pre-fix shape.

Risk assessment: LOW

Lowest of the four rounds. The only delta since the last approval is a docs-only merge from main; production code and dist/ are unchanged, and dist/ is verified in sync with source, so there is no stale-bundle risk.

Blast radius is bounded and asymmetric in the right direction, now confirmed at the consumer rather than inferred. The probe change reaches only bootstrapOpenCodeServer(...).shutdown, whose result cleanup.ts:122 uses solely to emit a warning — worst case +5s once per run in the cleanup phase. The S3 change reaches only the pipeline() failure branch of download(), and its worst case is a clean subsequent miss rather than the status quo of a partial opencode.db reaching bootstrap unprobed; the -wal pairing hazard that a partial-main-db miss could have created is already handled by restore.ts's finally. No security exposure: path validation is upstream and unchanged, and no package export surface moved.

The one latent hang vector is concern #1, and it is unreachable from any current call site.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	33675233839
Cache	hit
Session	ses_f9c5641ecffeAlFDQ2K3dXr6Hd