fix(sdk): stop connect() from waiting on READY for a PAUSING sandbox - #550
fix(sdk): stop connect() from waiting on READY for a PAUSING sandbox#550AprilNEA wants to merge 1 commit into
Conversation
A PAUSING sandbox's next stop is PAUSED — never READY — and no lifecycle event marks that edge, so routing PAUSING into the readiness wait parked connect() forever on a keepalive-fed Events stream. Poll the checkpoint out (500 ms Inspect cadence), then route on the settled state: PAUSED resumes as before, READY returns, terminal states throw. TypeScript twin of the Python fix (78e0491 on the sdk-python branch), where this routing was first reported and corrected.
Greptile SummaryThis PR corrects TypeScript SDK connection routing for sandboxes observed in the PAUSING state.
Confidence Score: 5/5The PR appears safe to merge, with the PAUSING state now routed through the lifecycle transition it can actually reach. The polling loop exits on every non-PAUSING result and then reuses the existing handling for resumable, ready, starting, terminal, and error states; the regression test verifies the intended re-inspection and single-resume behavior.
|
| Filename | Overview |
|---|---|
| sdk/typescript/src/sandbox.ts | Replaces the unreachable PAUSING readiness-event wait with repeated inspection and existing settled-state routing; no actionable defect was identified. |
| sdk/typescript/test/connect.test.ts | Adds focused mock-transport coverage demonstrating that PAUSED sandboxes resume and PAUSING sandboxes are re-inspected before one resume call. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[connect: Inspect sandbox] --> B{State}
B -->|PAUSING| C[Wait 500 ms]
C --> D[Inspect again]
D --> B
B -->|PAUSED| E[Resume sandbox]
B -->|READY or RUNNING| F[Return Sandbox handle]
B -->|STARTING| G[Wait for readiness event]
B -->|Terminal or unknown| H[Throw SandboxStateError]
E --> F
G --> F
Reviews (1): Last reviewed commit: "fix(sdk): stop connect() from waiting on..." | Re-trigger Greptile
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ The routing fix is right. Three rough edges — one of them a test assertion that does not check what its comment says.
Reviewed changes — the TypeScript half of the shared connect() PAUSING routing bug, reviewed against the merged Python twin on feat/sdk-python and against the daemon's current sandbox surface.
PAUSINGno longer falls into the readiness-event wait —sdk/typescript/src/sandbox.tsdropscase SandboxStateProto.PAUSING:from theSTARTINGfallthrough, soconnect()can no longer park on anEventsstream waiting for a READY that a pausing sandbox will never reach.- A settle poll replaces it — a
while (info.state === PAUSING)loop re-Inspects everyPAUSE_SETTLE_POLL_MS(500), then routes on the settled state through the existingswitch.PAUSED → resume(),READY/RUNNING → return, terminal →SandboxStateError. - New
test/connect.test.ts— aMockLifecycleover connect-escreateRouterTransportservesInspectfrom a state script and countsResumecalls; two cases cover the PAUSED baseline and the PAUSING→PAUSED regression.
I ran the suite locally (npx vitest run test/connect.test.ts — 2 passed) and confirmed the PAUSING case does fail when the fix is reverted, so it is a real guard. I also confirmed foxts living under devDependencies is fine: bunchee externalizes only dependencies/peerDependencies and inlines the rest, and foxts/noop already relied on that.
ℹ️ The PAUSING branch is unreachable today, and the fix has a twin that will drift
Daemon-side Pause/Resume are contract-only stubs returning unimplemented (app/arcbox-api/src/connect/control.rs:120-140, CORE-21), and the guest agent's event mapper emits no pausing/paused kinds (guest/arcbox-agent/src/sandbox/convert.rs:122-133, whose sibling state_filter notes "the manager cannot produce them yet"). No live daemon can return PAUSING from Inspect, so the mock test is the only guard this path will have until CORE-21 lands — worth stating in the PR so nobody later assumes e2e covers it. Separately, this is a line-for-line twin of the already-merged Python fix, including the two comments flagged inline below; corrections agreed here need to land in feat/sdk-python too or the twin claim goes stale.
Technical details
# Cross-SDK parity and post-CORE-21 follow-up
## Affected sites
- `sdk/typescript/src/sandbox.ts:81-85`, `:184-193` — twin of
`sdk/python/src/arcbox/_async/sandbox.py` (`_PAUSE_SETTLE_POLL_SECONDS`,
the `while info.state == SANDBOX_STATE_PAUSING` loop). Identical wording,
identical shape.
- `sdk/typescript/test/connect.test.ts:60-71` — twin of
`sdk/python/tests/test_connect.py::test_connect_settles_a_pausing_sandbox_then_resumes`,
carrying the same non-informative `daemon.states` assertion.
## Required outcome
- Whatever wording/assertion changes land here also land on `feat/sdk-python`,
so the two SDKs stay a true mirror.
- The polling design is revisited once CORE-21 makes the daemon emit
`SANDBOX_EVENT_KIND_PAUSED` — at that point the event wait becomes viable
and the poll can go away.
## Open questions for the human
- Is `feat/sdk-python` still open (so a follow-up commit can ride along), or
already merged and needing its own PR?
- Should a tracking note reference CORE-21 from the SDK so the poll is not
left behind indefinitely?Claude Opus ｜ 𝕏
| * How often {@link ArcBox.connect} re-inspects a PAUSING sandbox. No | ||
| * lifecycle event marks the PAUSING→PAUSED edge, so it is polled out. |
There was a problem hiding this comment.
The proto contradicts this: SANDBOX_EVENT_KIND_PAUSED = 10 is declared as "Checkpoint complete; runtime resources released" (sandbox.proto:160-161) — an event that marks exactly this edge. What is actually true is that the daemon does not emit it yet (Pause/Resume are CORE-21 stubs). As written the comment tells a future reader that polling is permanently necessary, which is the kind of claim nobody revisits once CORE-21 lands.
| * How often {@link ArcBox.connect} re-inspects a PAUSING sandbox. No | |
| * lifecycle event marks the PAUSING→PAUSED edge, so it is polled out. | |
| * How often {@link ArcBox.connect} re-inspects a PAUSING sandbox. The | |
| * daemon does not emit `SANDBOX_EVENT_KIND_PAUSED` yet (Pause/Resume are | |
| * CORE-21 stubs), so the checkpoint is polled out instead. |
| while (info.state === SandboxStateProto.PAUSING) { | ||
| // eslint-disable-next-line no-await-in-loop -- sequential by design: each re-inspect waits out the poll interval | ||
| await wait(PAUSE_SETTLE_POLL_MS); | ||
| // eslint-disable-next-line no-await-in-loop -- sequential by design: the settled state decides the route | ||
| info = await this.#client.inspect({ id }, unaryOptions(this.#ctx)); | ||
| } |
There was a problem hiding this comment.
Nothing bounds this loop: no deadline, no attempt cap, no AbortSignal. unaryOptions() bounds each individual Inspect, not the loop, so a daemon that keeps reporting PAUSING (checkpoint wedged, snapshot stuck) trades one indefinite wait for another — the caller still has no way out. It is strictly better than the old park (it makes progress, and it does exit if Inspect itself errors), and the Python twin has the same shape, so this is a suggestion rather than a defect — but the PR's own thesis is "stop waiting forever", which this only half achieves.
| // The checkpoint was polled out (a second Inspect ran) and exactly | ||
| // one Resume followed — not a readiness-event wait that never ends. | ||
| expect(daemon.states).toEqual([SandboxStateProto.READY]); |
There was a problem hiding this comment.
This assertion cannot prove what the comment claims. resume sets this.states = [SandboxStateProto.READY] unconditionally (line 40), so [READY] is the post-state whether or not a second Inspect ever ran — I confirmed by adding the identical assertion to the PAUSED baseline test above, where no polling happens at all, and it passed. Count Inspect calls on the mock instead; the resumes assertion on line 70 is the one carrying real signal here.
Technical details
# `daemon.states` does not witness the settle poll
## Affected sites
- `sdk/typescript/test/connect.test.ts:38-42` — the `resume` handler resets
`this.states = [READY]` unconditionally, erasing whatever the script had
consumed.
- `sdk/typescript/test/connect.test.ts:67-69` — the assertion and the comment
that claims it proves "a second Inspect ran".
- `sdk/python/tests/test_connect.py` — the twin carries the identical
assertion and comment.
## Required outcome
- The PAUSING test must fail if `connect()` stops re-inspecting, for a reason
that names the poll rather than incidentally tripping over an unimplemented
mock method.
## Suggested approach
Add an `inspects = 0` counter to `MockLifecycle`, increment it in the
`inspect` handler, and assert `expect(daemon.inspects).toBe(2)` in the PAUSING
case (and `toBe(1)` in the PAUSED baseline, which pins the negative). That
also removes the current reliance on the mock having no `events` handler:
today a reverted fix fails with `[unimplemented] SandboxService.Events`, not
with anything describing the actual regression.1 participant
Summary
A PAUSING sandbox's next stop is PAUSED — never READY — and no lifecycle event marks the PAUSING→PAUSED edge. The TS SDK routed PAUSING into the readiness-event wait, which parked
connect()forever on a keepalive-fed Events stream.Fix: poll the checkpoint out via Inspect (500 ms cadence), then route on the settled state — PAUSED resumes as before, READY/RUNNING returns, terminal states throw
SandboxStateError. STARTING keeps the event wait.This is the TypeScript twin of the Python fix (78e0491 on
feat/sdk-python), where the shared routing bug was first reported on review and fixed; that commit noted the TS SDK inherits it and would be fixed separately — this PR.Test
test/connect.test.tsmirrors the Pythontest_connect.py: a mock daemon (connect-escreateRouterTransport) serves Inspect from a state script and records Resume calls.resumes a PAUSED sandbox— baseline routing.settles a PAUSING sandbox to PAUSED, then resumes— the regression: asserts the checkpoint was polled out (a second Inspect consumed the script) and exactly one Resume followed, instead of a readiness-event wait that can never end.Gates
npm run lint✅npm run format:check✅npm test✅ (38 passed, 1 e2e skipped)npx tsc --noEmit✅npm run build✅