Uh oh!
There was an error while loading. Please reload this page.
fix(hub): throw when restarting a session with a closed stream - #164
Open
dvcolomban wants to merge 2 commits into
Open
fix(hub): throw when restarting a session with a closed stream#164dvcolomban wants to merge 2 commits into
dvcolomban wants to merge 2 commits into
Conversation
✅ Deploy Preview for devfra ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a usability bug in the hub terminals lifecycle by making restart() fail loudly when a terminal session’s output stream has already been closed (after natural exit or terminate()), which cannot be recovered in-place due to the one-shot ReadableStream controller.
Changes:
- Throw new
DF8206fromstartChildProcess()/startPtySession()sessionrestart()whenstreamClosedis set (instead of silently no-op’ing). - Add
DF8206diagnostic definition and update tests to assert the rejection behavior. - Document the new error in public types JSDoc and add
docs/errors/DF8206.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/hub/src/types/terminals.ts | Documents that restart() throws DF8206 once the stream has closed. |
| packages/hub/src/node/host-terminals.ts | Changes restart() to throw DF8206 when the session’s output stream is already closed. |
| packages/hub/src/node/diagnostics.ts | Adds the new DF8206 coded diagnostic with a recovery hint. |
| packages/hub/src/node/tests/host-terminals.test.ts | Updates assertions to expect restart() to reject with DF8206 after exit/terminate. |
| docs/errors/DF8206.md | Adds the error reference page for DF8206. |
Suppressed comments (1)
packages/hub/src/types/terminals.ts:144
- Same as above:
remove()is ambiguous here (it is not a method on the session). Prefer pointing callers atctx.terminals.remove(session)explicitly.
/** Throws `DF8206` once the session's output stream has closed (after a natural exit or `terminate()`) — `remove()` it and start a fresh session instead. */
restart: () => Promise<void>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
restart() silently returned once a startChildProcess()/startPtySession() session's streamClosed flag was set — after a natural process exit or terminate(). That flag guards a single-use ReadableStream controller which cannot be reopened, so restarting in place genuinely cannot work; the defect was that a caller had no way to distinguish success from a no-op (hub:terminals:restart resolves either way). Throw a new DF8206 diagnostic instead, pointing callers at remove(session) + a fresh start*() with a new id. This is a behaviour change on a case devframes#148 (four days ago) deliberately pinned as a silent no-op — the two host-terminals tests that pinned it are updated to assert the rejection while keeping their original assertions (stream stays closed / status stays 'stopped'): only the silence changes. Not reusing DF8205 (its fix text describes restartable: false, which would misdescribe a spent stream) and not flipping `restartable` in closeStream() (a different concept — "lifecycle owned elsewhere" vs. "stream spent"). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
dvcolombanforce-pushed
the
dvcol/fix-terminal-restart-closed-stream
branch
from
August 5, 2026 09:29
124e909 to
e56dd62CompareAddress review feedback on the wording only, no behaviour change: remove() is a method on ctx.terminals, not on the session, so both restart() JSDoc lines said it wrongly; DF8206's fix text now prefixes the calls with ctx.terminals. the way DF8202/DF8204 do and spells out the two recovery steps instead of contracting them; the docs page's Fix section is reworded as a sentence. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
restart()on astartChildProcess()/startPtySession()session silently returned once itsstreamClosedflag was set — after a natural exit, or afterterminate(). That flag guards a single-useReadableStreamcontroller that can't be reopened, so restarting in that state genuinely can't work. The bug isn't the refusal, it's that it's silent:hub:terminals:restartresolves either way, so a caller can't tell "restarted" from "did nothing."This throws a new
DF8206instead, pointing at the actual recovery:ctx.terminals.remove(session)then a freshstart*()with a new id. Covers the RPC path for free sincehub:terminals:restartjust awaitssession.restart().Why not DF8205
Its
fixtext says "it was registered withrestartable: false" — that'd misdescribe a session whose stream is just spent.restartableis a different concept (lifecycle owned elsewhere) thathost-terminals.tsnever even mutates.Behavior change, on purpose
host-terminals.test.tshad two tests explicitly pinning the no-op (does not restart a terminated child-process session,keeps status stopped when restart() is called after the process exited) from #148, four days ago. Both are updated here to assert theDF8206rejection instead, keeping their original assertions (stream stays closed, status stays'stopped') — only the silence changes. I think that's worth doing even though it revises recently-pinned behavior: an undiscoverable no-op on a public API is a footgun no matter how recently it shipped.Blast radius looks like zero — nothing outside this repo I can find calls
restart()or readsrestartable.Also added a one-line JSDoc on both public
restart(): Promise<void>declarations documenting the throw — no snapshot impact,tsnapistrips JSDoc.Tests
pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build— all green (1054 tests).