Skip to content

fix(server): cancel an attach whose client leaves before the pty is ready - #3

Merged
devswha merged 2 commits into
mainfrom
fix/attach-cleanup
Sep 19, 2026
Merged

devswha merged 2 commits into
mainfrom
fix/attach-cleanup

Conversation

@devswha

@devswha devswha commented Sep 19, 2026 •

Copy link
Copy Markdown
Owner

Problem

The attach handler added the client to the attachment and to client.data.attached only after await ensureAttachment(...). A frame from the same socket that arrived during that await found nothing to clean up. Bun doesn't serialize an async message handler per socket.

  • Disconnect mid-attach: close walked an empty attached set, so the closed socket was added to attachment.clients afterwards. It was never removed, and the pty outlived every live client.
  • Detach mid-attach: PaneTerminal sends detach(old) and then attach(new) on every pane switch, so this is the more common trigger. The detach was a no-op, and the client was added to the old pane anyway. It kept receiving that pane's bytes, and the pty stayed alive, until the socket closed.

Fix

  • The pane goes into client.data.attached before the await. detach and close both clear it, and the attach checks it when it resumes. If the attach fails, the entry is removed again.
  • A cancelled attach calls releaseUnclaimed. It closes the attachment only if it's still the current one, has no clients, and no other client has the pane in attached.
  • That last check matters because of the pending-creation sharing from fix(server): share one pty when clients attach the same pane concurrently #2: a live client may be waiting on the same creation and not have resumed yet. Closing eagerly would leave it with a dead record and a blank terminal.

Follow-up commit: stale claims after a pty exits

With the intent scan in place, an entry left behind in attached counts as a live claim. When a pty exits while its herdr pane survives (e.g. herdr restarts), closeAttachment deleted the record but left the pane in each member's attached. Those members are on the "terminal ended" screen. Their stale entries then blocked releaseUnclaimed from closing a later cancelled attach on that pane, so the pty kept running with zero clients. It did recover on the member's next pane switch or disconnect. closeAttachment now removes the pane from its members' attached.

No contract test pins this: when a cancelled creation has no other joiner, a client can't observe when it has finished. An ad-hoc probe checked it instead. It kills the attach under a watching client, then attaches and detaches the pane in one tick, and waits 3s. With the change 0 attach processes remain; without it, 2 remain.

Tests

  • WebSocket client leaving mid-attach has one test for each trigger, and each runs on its own workspace. The oracle is the leaked resource itself: once the last live client is gone, no herdr terminal attach <terminal_id> process may remain (pgrep -f, bounded wait). Each test first asserts that the process count is above 0, so the pgrep pattern can't pass vacuously.
  • The observer-first test moved into its own describe with its own pane. It no longer relies on the previous roles test having released the shared qaPaneId. The test body is unchanged.
Check Result
Mutation: server/index.ts fix reverted both new tests fail 3/3 (attach … still running 5000ms after its last client left)
Mutation: only the intent scan in releaseUnclaimed removed both new tests fail 3/3 (stayer pty-data not received), so the eager-close trap is pinned
bun test server/api.contract.test.ts ×4 24 pass / 0 fail each run
bun test 65 pass / 0 fail, 9 files
bun run typecheck / bun run build clean
Failed lookup (w9999:p9999 ×2 racing, then again) three pane_not_found frames, no hang
Leftovers after runs no attach/sidecar processes, no herdr-web-ui-test* workspaces

The two new tests take about 1.5–2s each, because herdr terminal attach takes about 1.2s to exit after the sidecar kills it (measured separately). pushes pane-status … sometimes runs about 4.2s instead of 1.7s. That happens on main too: its status retry loop occasionally takes an extra round. This PR doesn't change it.

README: test count 63 → 65, and the coverage sentence now lists this case.

🤖 Generated with Claude Code

devswha and others added 2 commits September 19, 2026 14:23
…eady

The attach handler added the client to the attachment only after
awaiting the terminal lookup. A detach or close that arrived during that
await found nothing to remove, so the client was added afterwards
anyway. The pty then outlived its last live client: a closed socket
stayed in the set forever, and a detached client kept receiving the
pane's bytes until it disconnected. The detach case is what switching
panes quickly in the sidebar does (detach old, attach new).

The pane is now recorded in `attached` before the await, so detach and
close clear it, and the attach checks it on resume. A cancelled attach
closes the attachment only when no other client still wants the pane.
A joiner of the same creation may not have resumed yet, and closing
eagerly would hand it a dead record.

Tests: two contract tests (disconnect, detach) check that no `herdr
terminal attach` process survives the last live client. With the fix
reverted both fail 3/3; with the intent check removed both fail 3/3
because the remaining client never receives data. The observer-first
test moves to its own pane so it no longer relies on the previous test
having released the shared one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
releaseUnclaimed treats a pane in any client's `attached` set as a live
claim. When a pty exits while its herdr pane lives on, closeAttachment
deleted the record but left the pane in each member's set. Those members
sit on the "terminal ended" screen, and their stale entries then vetoed
closing a later attach on that pane that was cancelled mid-creation, so
that pty kept running with no clients.

closeAttachment now removes the pane from its members' sets. A probe
(kill the attach under a watching client, then attach and detach the
pane in one tick) leaves 0 attach processes with this change and 2
without it. No contract test pins it: nothing observable marks the
moment a cancelled creation with no other joiner has finished.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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