Skip to content

fix: cancel disconnected exec sessions and clean up docs - #14

Merged
CMGS merged 4 commits into
mainfrom
review/whole-repo-2026-09-21
Sep 21, 2026
Merged

CMGS merged 4 commits into
mainfrom
review/whole-repo-2026-09-21

Conversation

@CMGS

@CMGS CMGS commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Whole-repo review round, 2026-09-21. Every production file read in full; the host→guest exec flow (cocoon vm exec → hybrid vsock → agent → child → framed reply), the reseed path and the Windows transport were traced end to end by a reader agent that ran eleven probes (framing edges, 1 MiB output, non-zero exit, ctx kill, mid-stream connection drop, 200 KiB stdin, 16 concurrent sessions, a real reseed with both RNG ioctls in a privileged Linux container); a docs lens verified every claim on all seven pages; a tests lens read the five test files.

Commits

  • 07a2edf review: two comments claim less than they did
  • 6be436f review: remove dead code and align documentation
  • 5d41245 fix: cancel active exec sessions on disconnect
  • 89aa3b6 test: the disconnect test takes the started and ready frames in either order

Findings

# where claim outcome
1 agent/sysproc_linux.go the comment promised background workers never outlive the session; the process-group kill runs on cancel only, a daemonized grandchild survives a normal exit by design (a probe showed sleep 6 & alive after a clean exit) comment narrowed to the cancel path
2 agent/reseed_linux.go the comment promised the entropy left no heap copy; the scanner buffer keeps the base64 form comment narrowed; zeroing the scanner buffer would be over-defense at the agent's own privilege level
3 cmd/transport_windows.go LocalAddr reports the host CID for the guest's own endpoint; reaches two log lines only noted, no change
4 wire contract vs cocoonv2 nine frame types, no version field, unknown fields decode away, unknown frame types ignored except as the first frame; cocoonv2 pins v0.2.3 == the round's base correct, no skew; 5d41245 changes no frame
5 agent/agent.go handleConn (found by the owner after the round) the stdin reader returned after MsgStdinClose and, on a clean EOF, returned without cancelling the child, so a client that vanished after closing stdin (or while a quiet child ran) left the child running until it exited or the server context ended; the round's read recorded that the reader exits, not that the child outlives the client, and the disconnect probe dropped the connection only while output was flowing fixed 5d41245: the reader drains the connection after stdin closes and cancels the exec context whenever it exits; TestExecDisconnectCancelsQuietChild fails on the pre-fix code (both subtests time out) and passes on HEAD; the exec-flow lens measured the same on archives of both trees (a quiet child whose client vanished after closing stdin is alive 5 s later on the old tree, reaped at once on the new one) and re-ran its nine exec probes unchanged. Host consumers (cocoon vm exec through this repo's client package) never half-close, so an EOF at the agent means the client is gone
6 client/client.go Run, packaging/install-cocoon-agent.ps1 a post-loop sawExit branch the read loop could never reach, and an unused $binPath variable; the LOC lens reported no dead code cut in 6be436f

Docs: all seven pages verified against source in the round; 6be436f/5d41245 then reshaped README.md (highlights, related projects, Makefile targets, all checked against the Makefile) and the architecture page's error and StdinClose bullets, both checked against handleConn (an undecodable first frame is logged unless it is a plain EOF and the connection closed without a frame; MsgStdinClose ends input, not the session).

Design notes (owner decision, no code)

  • A client that half-closes its write side and then waits for MsgExit now loses its child: since 5d41245 an EOF on the connection cancels the session. No shipped caller does this (the client package only closes the whole connection, on context cancel), and the architecture page's new StdinClose sentence states the input side; if the guarantee is worth spelling out, one clause on that bullet ("closing the connection ends the session and the command") does it.

Cut-list (report only)

The owner's 6be436f cut the two dead items the LOC lens missed (sawExit, $binPath); nothing else is on the list — the standing rejections (Winsock send()==0 guard, Message.PID, the two vsock reject loops, processController, len(argv)==0) stand.

LOC (same counting on both ends)

  • origin/main=7a8a406: prod=1506 test=853 comments=92 blanks=197 effective=1217
  • HEAD=89aa3b6: prod=1495 test=949 comments=86 blanks=197 effective=1212 test_funcs=30 (was 28)

Per commit: 07a2edf review 2/2 (comments +1/−1); 6be436f review 0/14 (comments +0/−6); 5d41245 fix 10/7 · 83/0 (comments +4/−4); 89aa3b6 test 0/0 · 17/8.

Gates

All gates re-run on HEAD 89aa3b6 (and on 5d41245 before the test commit):

  • GOWORK=off make fmt-check: rc=0; golangci-lint on GOOS=darwin, linux and windows: 0 issues; GOOS=windows go vet: ok.
  • asl ./... on darwin and linux: 0 findings.
  • go test -race -count=1 ./...: green (agent, client on darwin).
  • TestExecDisconnectCancelsQuietChild run against the pre-fix agent/agent.go: both subtests fail (5 s timeouts), so the test is red before the fix and green after; at HEAD it passes -race -count=30.
  • linux/arm64 golang:1.27 container: go build ./... && go vet ./... && go test -race -count=1 ./... green (agent, client, cmd) on 5d41245 and 89aa3b6.

Codex

Round 1, thread 01a0c25f-d040-70c0-9e66-fe627417b63d, read-only sandbox, on HEAD 07a2edf; the prompt carried the discipline (no contrived scenarios, no over-design, idempotent fixes, no over-defense). Final message, verbatim:

No blockers: this diff converges.

Round 2 on 5d41245 (the owner's two commits, same thread): the production fix judged sound; one finding on the new test, verbatim:

[P3] agent/exec_cancel_test.go:56 — TestExecDisconnectCancelsQuietChild fails when cmd.Start's stdout copier forwards "ready" before runExec sends MsgStarted; their order is unsynchronized — accept both frames in either order before disconnecting.

Confirmed from runExec (cmd.Stdout is a writer, so the copier Start spawns races the MsgStarted encode) and fixed by 89aa3b6.

Round 3 on 89aa3b6, verbatim:

No blockers: this diff converges.

Follow-ups

None filed.

The process-group kill runs on cancel only, so a daemonized grandchild
survives a normal exit by design; clearing the decoded entropy leaves the
scanner's copy, so the comment no longer promises a heap free of it.
@CMGS CMGS changed the title Whole-repo review round 2026-09-21 rewrite comment Sep 21, 2026
Keep reading the connection after stdin closes and cancel the child when the reader exits, including clean EOF. Add regressions for quiet children with open and closed stdin while the server context remains live.
@CMGS CMGS changed the title rewrite comment fix: cancel disconnected exec sessions and clean up docs Sep 21, 2026
…r order

cmd.Stdout is a writer, so the copier Start spawns can forward the
child's first output before runExec encodes MsgStarted; the test
asserted a fixed order the code does not promise.
@CMGS
CMGS merged commit e42b38e into main Sep 21, 2026
2 checks passed
@CMGS
CMGS deleted the review/whole-repo-2026-09-21 branch September 21, 2026 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant