Repository navigation
fix(runner): complete the WebSocket close handshake on attach - #1569
Merged
Merged
Conversation
Collaborator
Author
|
/reviewed c26103b |
Contributor
Author review acknowledgmentThe PR author acknowledged this commit. @DefeatMan: after reading the current diff, post this as a new PR comment: Unacknowledged PRs are converted to draft. After this check passes, click Ready for review when you want reviews. |
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe runner now performs bounded WebSocket close handshakes for attach teardown and terminal startup failures. Readers drain frames after cancellation without extending the deadline. Tests cover peer responses, silent peers, and active peers. ChangesWebSocket teardown
Priority: ⬆️ High Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: High Sequence Diagram(s)sequenceDiagram
participant Client
participant AttachHandler
participant Reader
Client->>AttachHandler: Attach WebSocket
AttachHandler->>Client: Send exit and Close frames
AttachHandler->>Reader: Cancel attach processing
Reader-->>Client: Drain client frames
Client-->>Reader: Send Close response
Reader-->>AttachHandler: Report reader completion
AttachHandler->>AttachHandler: Close connection after response or timeout
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The runner now preserves fast-command output by completing a bounded WebSocket close handshake. The change is ready to merge with no concrete blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
DefeatMan
marked this pull request as ready for review
September 21, 2026 07:41
DefeatMan
added a commit
that referenced
this pull request
Sep 21, 2026
`codecov/patch` became a required check on 2026-09-20, but no coverage upload has ever covered `apps/runner`. Codecov answers a PR that changes only Go files there with `Empty Upload — Testable files changed` and fails, so every such PR is blocked on a check it cannot satisfy. PR #1569 is the first one to hit it. Three things were missing, and only the three together help. The `go` filter listed `sdks/go` and `libgvproxy-sys` but not `apps/runner`, so the Go job never ran for a runner-only change; fixing the upload alone would have changed nothing. `coverage:go` measured the SDK and the gvproxy bridge but not the runner, so there was no profile to send. And the upload step named two profiles, so a third would have been dropped. `coverage-empty` needs no edit: it is conditioned on every suite being `false`, so a runner change now makes `go` true and skips it. The runner joins the existing Go job rather than getting its own: it links libboxlite.a through cgo exactly as `sdks/go` does, and that job already builds it, so a separate job would repeat a multi-minute Rust build. `apps/api` has its own job because it needs Postgres and Redis. The filter gains the assertions it never had. workflow-checks.test.ts loads the real workflow rules, so both defects are table rows. `apps/runner/cmd/runner/main.go` expecting `['go']` fails with `[]` before the filter edit. And whatever the filter selects on has to reach the workflow that runs the filter, so `apps/go.work` and its sum are asserted against `push.paths` too — the Go equivalents of the `apps/package.json` and `apps/yarn.lock` already listed there. Running the runner in CI for the first time exposed three `pkg/boxlite` integration tests that build a real runtime: a hosted runner has `/dev/kvm` but denies the test user access, and a hosted mac cannot check Hypervisor.framework, so `NewClient` fails with `code=13` on every platform. Each of the three already skips on the same prerequisite one step later, at its `Create`; the guard simply was not on the construction that precedes it, and on macOS HVF that step never failed. They now skip there too, with the neighbouring wording. Where the runtime does start they still run — all three pass locally — and pointed at an unusable HomeDir the new branch reports SKIP, so the guard neither over- nor under-fires. That same run showed a second failure the local one could not: `go: no such tool "covdata"`, once per runner package without tests. CI installs Go 1.24 while `apps/go.work` asks for 1.25.4, so GOTOOLCHAIN fetched another toolchain mid-job and coverage could not run under it. A plain `go build` survives that, which is why building the runner in CI has worked all along and only collecting coverage exposed it. The installed version now matches, in `ci-config.json` and in the setup-go default that jobs without a config dependency use, and a test asserts the installed toolchain satisfies every tracked `go.mod` and `go.work` — it reports `apps/go.work requires go 1.25.4, but CI installs 1.24` before the bump. `make coverage:go` writes `target/coverage/runner.out` beside the two existing profiles. Its two other failures are in legs this change does not touch and are local to macOS: CI last exercised them at a043fb0 (run 35522733629, Go Tests green on all three platforms) and has skipped the Go job on every main run since, which is CI certifying those paths unchanged. The full `apps/infra` suite is 648/648.
DefeatMan
added a commit
that referenced
this pull request
Sep 21, 2026
`codecov/patch` became a required check on 2026-09-20, but no coverage upload has ever covered `apps/runner`. Codecov answers a PR that changes only Go files there with `Empty Upload — Testable files changed` and fails, so every such PR is blocked on a check it cannot satisfy. PR #1569 is the first one to hit it. Three things were missing, and only the three together help. The `go` filter listed `sdks/go` and `libgvproxy-sys` but not `apps/runner`, so the Go job never ran for a runner-only change; fixing the upload alone would have changed nothing. `coverage:go` measured the SDK and the gvproxy bridge but not the runner, so there was no profile to send. And the upload step named two profiles, so a third would have been dropped. `coverage-empty` needs no edit: it is conditioned on every suite being `false`, so a runner change now makes `go` true and skips it. The runner joins the existing Go job rather than getting its own: it links libboxlite.a through cgo exactly as `sdks/go` does, and that job already builds it, so a separate job would repeat a multi-minute Rust build. `apps/api` has its own job because it needs Postgres and Redis. The filter gains the assertions it never had. workflow-checks.test.ts loads the real workflow rules, so both defects are table rows. `apps/runner/cmd/runner/main.go` expecting `['go']` fails with `[]` before the filter edit. And whatever the filter selects on has to reach the workflow that runs the filter, so `apps/go.work` and its sum are asserted against `push.paths` too — the Go equivalents of the `apps/package.json` and `apps/yarn.lock` already listed there. Running the runner in CI for the first time exposed three `pkg/boxlite` integration tests that build a real runtime: a hosted runner has `/dev/kvm` but denies the test user access, and a hosted mac cannot check Hypervisor.framework, so `NewClient` fails with `code=13` on every platform. Each of the three already skips on the same prerequisite one step later, at its `Create`; the guard simply was not on the construction that precedes it, and on macOS HVF that step never failed. They now route through one helper that skips only on the runtime's own ErrUnsupported — the code both a KVM-denied Linux runner and a mac that cannot check Hypervisor.framework return — and fails on anything else, so a constructor regression cannot hide in a green skip. Pointed at an unusable HomeDir, which reports ErrStorage, the test fails; where the runtime does start all three still run and pass. That same run showed a second failure the local one could not: `go: no such tool "covdata"`, once per runner package without tests. CI installs Go 1.24 while `apps/go.work` asks for 1.25.4, so GOTOOLCHAIN fetched another toolchain mid-job and coverage could not run under it. A plain `go build` survives that, which is why building the runner in CI has worked all along and only collecting coverage exposed it. The installed version now matches, in `ci-config.json` and in the setup-go default that jobs without a config dependency use, and a test asserts the installed toolchain satisfies every tracked `go.mod` and `go.work` — it reports `apps/go.work requires go 1.25.4, but CI installs 1.24` before the bump. `make coverage:go` writes `target/coverage/runner.out` beside the two existing profiles. Its two other failures are in legs this change does not touch and are local to macOS: CI last exercised them at a043fb0 (run 35522733629, Go Tests green on all three platforms) and has skipped the Go job on every main run since, which is CI certifying those paths unchanged. The full `apps/infra` suite is 648/648.
zombee0
pushed a commit
to zombee0/boxlite
that referenced
this pull request
Sep 21, 2026
## Summary `codecov/patch` became required on 2026-09-20, but nothing uploads `apps/runner` coverage, so Codecov fails a runner-only PR with `Empty Upload — Testable files changed` (boxlite-ai#1569 first). Running it in CI exposed two more: three `pkg/boxlite` tests need virtualization a hosted runner lacks, and CI installed Go 1.24 against a 1.25.4 workspace, so GOTOOLCHAIN fetched a toolchain without `covdata`. **Blast radius: every Go job now installs 1.25.4.** ## Call graph Before ``` changes/go filter (workflow · .github/workflows/test.yml:176) ← BUG: no apps/runner, so the Go job never runs, and coverage:go measures only sdks/go + gvproxy ``` After ``` setup-go (config · .github/ci-config.json:10) — 1.25.4, no mid-job fetch └─ changes/go (workflow · .github/workflows/test.yml:179) — + apps/runner/**, apps/go.work* └─ coverage:go (make · make/coverage.mk:73) — + runner.out ``` ## Verification Each defect is an assertion that fails without its fix: the filter row (`['go']` vs `[]`), the push-path rows, `apps/go.work requires go 1.25.4, but CI installs 1.24`. Risks: Codecov's module-path-to-`apps/runner/` mapping is unverifiable locally; those three skip on hosted CI, but only on `ErrUnsupported` — any other constructor error still fails. Pre-existing, left alone: their `Create` guards skip on any error; `setup-common.sh:135` says Go 1.24; `apps/proxy` shares the gap. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
silverzki
approved these changes
Sep 21, 2026
silverzki
left a comment
silverzki
left a comment
Collaborator
There was a problem hiding this comment.
don‘t forget src/cli/src/commands/serve/handlers/executions.rs share the same shape
zombee0
reviewed
Sep 22, 2026
zombee0
left a comment
zombee0
left a comment
Contributor
There was a problem hiding this comment.
LGTM,
as silver commented, there are three calls use the same pattern, can we consolidate them into a single place?
Member
|
@boxliteai review and comment like Linus |
Contributor
|
@DorianZheng Reviewed The fix is correct: the reader remains alive after cancellation, the server waits for the peer’s Close, and the five-second bound prevents a hostile client from retaining the attach slot indefinitely. The terminal failure path follows the same rule. Tests cover silent, responsive, and chatty peers. No blocking findings. I ran 📦 Ran in an isolated BoxLite microVM · reply with a mention to follow up |
DorianZheng
enabled auto-merge
September 22, 2026 08:56
DorianZheng
previously approved these changes
Sep 22, 2026
An exec whose command exits inside one round trip lost its output to a 502. `boxlite exec <box> -- echo hello` failed 19 of 28 runs against the GCP dev stack; `sleep 1` and `sleep 3` never failed, so the failure tracks how long the WebSocket lives and not request rate or spacing. The process starts on the POST, so a fast command is already finished when the client attaches. The runner replays the backlog, writes the exit frame, writes Close and drops the TCP connection — the whole session in about 20ms, while the balancer is still relaying the 101 to a client one round trip away. Its request log calls those attaches `502 websocket_handshake_failed` and shows two inbound attaches per failing exec against the client's one, which is its retry; Cloud Run records a clean 101 for every one of them, so nothing below the balancer reports an error. RFC 6455 §5.5.1 makes the close a handshake: an endpoint drops the TCP connection once it has both sent and received a Close. Waiting for the peer's answer keeps the connection alive past the upgrade, bounded by wsPeerCloseWait for a peer that neither answers nor drops. Three parts make that hold. Every path that sends a Close then waits — the failure and panic exits used to close in the deferred cleanup and drop immediately, and a cancelled loop is not always a dead peer. The reader drains rather than returning: past cancellation it keeps reading and answers nothing, because §5.5.1 forbids a data frame after a Close and handleControlFrame's error replies are data frames, while returning on the first frame would hand a client with stdin still in flight exactly the teardown this avoids. And proxy.go's terminal upgrade closed the same way on its StartExecution failure branch, so it shares the wait. Rejected: retrying the attach client-side, which re-runs the same race on every attempt; and delaying the close by a fixed interval, which pays latency on every exec and tunes a constant to one balancer's behaviour rather than fixing the protocol. The runner README described the old teardown as current — the pong handler pushing the read deadline out unconditionally, and a call graph ending at the Close — so it moves with the code. Confirmed end to end after rolling the dev fleet: `echo hello` failures went from 19/28 to 0/32, the attach lives ~1.27s rather than 0.021s, and the balancer's retry is gone because there is nothing left to retry.
DefeatMan
force-pushed
the
fix/runner-exit-handshake
branch
from
September 22, 2026 08:58
c26103b to
754a636
Compare
auto-merge was automatically disabled
September 22, 2026 08:58
Pull request was converted to draft
Collaborator
Author
|
/reviewed 754a636 |
DefeatMan
marked this pull request as ready for review
September 22, 2026 09:04
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
DefeatMan
enabled auto-merge
September 22, 2026 09:26
DefeatMan
added a commit
to DefeatMan/boxlite
that referenced
this pull request
Sep 22, 2026
A client still uploading stdin when its command exits lost the exit code and saw a broken pipe instead. `boxlite exec <box> < bigfile` is the shape that hits it: the upload is still in flight when the process ends. The writer sends the exit frame and its Close, and the teardown then aborted the reader at once. That reader is also what drains the socket — its own comment says it "keeps draining so the writer can flush and close" — so whatever the client had in flight stayed unread, and closing a socket with unread data makes the kernel send RST rather than FIN. An RST discards the sender's own queued bytes, and the exit frame was among them. RFC 6455 §5.5.1 makes the close a handshake: an endpoint drops the TCP connection once it has both sent and received a Close. The reader now gets that chance — it already breaks on `Message::Close`, so it ends by itself when the peer answers, and keeps draining until then. The bound covers a peer that neither answers nor drops; tungstenite replies to a Close as soon as it reads one, so the common case costs a round trip. Rejected: draining the socket in the teardown instead, which duplicates the reader's loop and races it; and swallowing the write error client-side, which hides a lost exit code rather than delivering it. Waiting on the reader means the wait itself drives that handle to completion, so only a handle this teardown aborted may be joined afterwards — the file's own NOTE warns about the second poll, and joining unconditionally panics with "JoinHandle polled after completion" before `mark_disconnected` runs, wedging the attach slot that `upgrade_to_attach_session` exists to release. A third test covers that path: it answers the Close, then asserts the slot can be claimed again. Only a Close that actually went out earns the wait: the writer reports whether its send succeeded, because every other way out of its loop is a failed send that opened no handshake, and waiting on one of those would delay `mark_disconnected` while the reader kept taking stdin. That branch has no test — forcing a send to fail also breaks the reader, which then wins the select, so a test for it would assert on a race. The reproducer reads once the teardown is under way but still inside the bound, so the frame has to survive in the socket rather than being caught in flight. Without the fix it reports `Broken pipe (os error 32)` and no exit code. Its control runs the same delayed read with nothing in flight and passes either way, which is what pins the cause on the unread stdin rather than on timing. Same defect class as the runner's attach (boxlite-ai#1569), surfacing differently: the runner needs a load balancer in front to turn it into a 502, while `serve` is reached directly and loses the exit code on its own. `README.md`'s teardown sketch described the old abort-the-other behaviour and moves with the code.
TestBoxliteExecAttach_StdinAndExit checked stdin delivery after the exec had exited — past the point where the server has sent its Close and the reader only drains, so whether the frame still reaches stdin depends on which goroutine runs first. Linux loses that race often: 6 and 11 failures in two 50-run batches of the attach loop under golang:1.25, 0 in 50 with the assertion moved. macOS wins it nearly every time, which is why CI is red on both Linux arches and green on darwin-arm64. Production loses nothing either way — AttachWriteStdin refuses the write once the exec is done. Assert right after the frame is written instead, the only point where delivery is defined. The comment above the drain in readClientFrames read as though in-flight stdin were still handled. Say what happens to those frames and why neither cancellation cause loses anything by it.
coverage:go measures the whole runner module with -coverpkg=./..., so swaggo's generated docs package lands in the report as permanent 0% for a file whose own header says DO NOT EDIT. Codecov's report for 754a636 lists the module's files under apps/runner/ rather than under its import path, which is what the glob matches. Nothing else in the module is ignored. cmd/runner looked like pure entry point, but main.go carries the migration store and work-dir precedence chains with no tests under cmd/, and `ignore` also drops those lines from the 90% patch gate — so they stay measured and visible instead.
Collaborator
Author
|
/reviewed 33f2352 |
DefeatMan
marked this pull request as ready for review
September 22, 2026 11:23
Collaborator
Author
|
Follow-up commits on this branch.
Linux failed 6–11 of 50 runs, macOS 0 of 50. It now asserts while the session is live. Production behaviour is unchanged.
|
DorianZheng
enabled auto-merge
September 22, 2026 11:37
DorianZheng
approved these changes
Sep 22, 2026
DefeatMan
added a commit
to DefeatMan/boxlite
that referenced
this pull request
Sep 22, 2026
Running this suite against api.dev.boxlite.ai found three defects in what this branch had just added, and three areas the cloud path never covered. The sweep selected by idle time alone: a report against dev listed five of a colleague's POL-599 boxes as candidates. Boxes the cases create are now named `e2e-<random>` and only that prefix is swept; `--any-name` is the opt-out and an empty prefix is refused. The sweep also waits for a box to stop being served, and only a 404 says it is gone. Completing one lifecycle window beside a caller's other one could build a pair the SDK rejects, so the SDK door applies both or neither. A hand-built REST body meets no such rule and is completed and named. The workflow now refuses a stage outside dev/prod and a non-https API_URL. The Node tunnel driver only stopped its four boxes; it removes them now. Ported from the local suite, none of it reachable there before: the box's main command, secrets, and a read-only mount refusal. Tunnel and preview become xfail(strict) — since boxlite-ai#1370 no SDK caller can create a public box, and raw REST with the nested shape shows that is the client's fault, not the server's. test_harness_guards.py covers the suite's own rules without a stage, each one run first against the defect it reproduces. What is not verified is recorded in apps/e2e/README.md: the volume cases skip on dev, whose key holds no volume permission, and short-exec stdout is dropped there until boxlite-ai#1569 lands.
DefeatMan
added a commit
to DefeatMan/boxlite
that referenced
this pull request
Sep 24, 2026
Running this suite against api.dev.boxlite.ai found three defects in what this branch had just added, and three areas the cloud path never covered. The sweep selected by idle time alone: a report against dev listed five of a colleague's POL-599 boxes as candidates. Boxes the cases create are now named `e2e-<random>` and only that prefix is swept; `--any-name` is the opt-out and an empty prefix is refused. The sweep also waits for a box to stop being served, and only a 404 says it is gone. Completing one lifecycle window beside a caller's other one could build a pair the SDK rejects, so the SDK door applies both or neither. A hand-built REST body meets no such rule and is completed and named. The workflow now refuses a stage outside dev/prod and a non-https API_URL. The Node tunnel driver only stopped its four boxes; it removes them now. Ported from the local suite, none of it reachable there before: the box's main command, secrets, and a read-only mount refusal. Tunnel and preview become xfail(strict) — since boxlite-ai#1370 no SDK caller can create a public box, and raw REST with the nested shape shows that is the client's fault, not the server's. test_harness_guards.py covers the suite's own rules without a stage, each one run first against the defect it reproduces. What is not verified is recorded in apps/e2e/README.md: the volume cases skip on dev, whose key holds no volume permission, and short-exec stdout is dropped there until boxlite-ai#1569 lands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
boxlite exec <box> -- echo hellolost its output to a 502 on 19/28 runs on the GCP dev stack;sleep 1never failed. A fast command has already finished when the client attaches, so the runner wrote Close and dropped TCP in ~20 ms — while the balancer was still relaying the101, logged502 websocket_handshake_failed.RFC 6455 §5.5.1 makes the close a handshake: drop TCP only once a Close is both sent and received. The reader now drains past cancellation rather than returning.
proxy.go's terminal upgrade shares the wait.Call graph
Before
After
Verification
Two-side: production reverted, tests kept — three reproducers fail in 0.00 s on
EOF; restored, all pass.apps/runnergreen under-race. Dev fleet: 0/32.boxlite serveshares the shape, without a balancer — unfixed.Fixes #1546
🤖 Generated with Claude Code
Summary by CodeRabbit