Repository navigation
Fix/orcabot bug hunt - #246
Merged
Merged
Conversation
…validation 500s) #1 [sandbox] Orphaned PTY/zombie leak on terminal delete. PTY.Close() SIGKILL'd the process but only reaped (cmd.Wait) for pool slots, so non-pool PTYs (desktop / any non-egress deploy) leaked a zombie every delete. Now reap unconditionally and kill the whole process group (negative pid) so agent children (node/chromium) die too. (pty.go; sandbox revision -> main-v35-pty-close-reap) #2 [controlplane] Duplicate sessions/PTYs on concurrent create. The reuse check is check-then-act and races; 3 concurrent creates → 3 PTYs. Added a per-item in-flight gate that serializes creates within the isolate (createSession wraps createSessionImpl). #3 [controlplane] POST /dashboards/:id/items with invalid JSON → 500. Now 400. #4 [controlplane] attach with malformed body / unknown provider → 500. Wrapped json parse + provider validation + attachIntegration in try/catch → 4xx, never 500. #5 [controlplane] POST item with missing/invalid type → 201 (junk item). Now validated against the item-type union → 400; valid types still 201. Verified end-to-end via the orcabot CLI + /debug/exec: - #1 create→delete: process GONE, zombies=0 after churn. - #2 3 concurrent creates → exactly 1 session. - #3/#5 invalid JSON / missing / bogus type → 400; valid note → 201. - #4 malformed body / bogus / valid-no-account → 400/403/404, no 500. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
#7 [HIGH] deleteDashboard never stopped its sessions, so DELETE /dashboards/:id orphaned live PTYs (shells + agent children) in the sandbox. Now stops all active/creating sessions (→ kills their PTYs via stopSession, which reaps after the #1 fix) before deleting, mirroring deleteItem. Verified: dashboard delete reaps the PTY (no leak, no zombie). #6 [MED] Ghost sessions: when a PTY died (crash/OOM/agent exit/VM restart) the control plane kept status='active' forever (ls showed 'running', attach 404'd, "stuck connecting"). The WS proxy now reconciles: on a 404 from the sandbox it marks the session stopped (by pty_id), so listings reflect reality and the client recreates instead of hanging. Verified: kill PTY → attach → session→stopped, terminal shows 'idle' not 'running'. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
#1 gets fast Go unit tests; the stateful/cross-component bugs (#2-#7) get a live integration smoke, because the control-plane mock-D1 harness can't represent them (it ignores UPDATE...SET and mis-maps session inserts — it's documented as "full integration tests should use a real D1"). - sandbox/internal/pty/pty_test.go: - TestPTYCloseReapsProcess — Close() kills AND reaps (no zombie); guards #1's unconditional reap (was pool-only). - TestPTYCloseKillsProcessGroup — Close() kills the process group so in-group children die too (guards the group-kill; job-control/setsid escapees noted). - desktop/tests/regression-smoke.sh: drives the live stack via the orcabot CLI + /debug/exec and asserts #1-#7 stay fixed, plus invariants (path-traversal blocked, broker-secret output redaction). 12 checks, exits non-zero on any failure. Not in the fast PR lane (needs the desktop VM) — run locally / nightly. Verified: go test ./internal/pty green; smoke 12/12 passing. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Addresses two findings from the fix/desktop-on-mac review: - P3 (image.rs): the staging stamp used seconds-resolution mtime + size, so a rebuild landing in the same wall-clock second as the previous one (fast SKIP_KERNEL iterations; raw sandbox.img keeps a constant size) could be treated as "unchanged" and silently boot a stale image. Use nanosecond mtime. (A content hash would be more robust but hashing a multi-GB image every launch is too slow.) - P2 (macos.rs): replace the hard-coded "8787:8787" reverse-port-forward with a named CONTROLPLANE_PORT const + a doc explaining why it's pinned on both sides (secure per-boot port delivery to the guest isn't possible yet). NOT fixed — P1 was a FALSE POSITIVE. The review (and my own first code-read) assumed the guest boots the minimal init in build-images.sh, which lacks the reverse control-plane bridge. But at runtime the Debian rootfs's systemd is PID 1 and runs /etc/rc.local (the MININIT is vestigial/dead). rc.local already starts `socat TCP-LISTEN:8787 -> VSOCK-CONNECT:2:8787` and sets CONTROLPLANE_URL. Verified live on the VM: rc-local.service runs, the reverse socat is listening, guest->control-plane /health returns 200, and the server has CONTROLPLANE_URL=http://127.0.0.1:8787. So no init change is needed. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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
1 participant
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.
No description provided.