-
Notifications
You must be signed in to change notification settings - Fork 272
[Fix] Commands stay Running when user closes their terminal #1363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
zoomote
wants to merge
22
commits into
main
Choose a base branch
from
fix/terminal-close-completion-2lvnwknm9glyx
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
bc493e6
fix(terminal): finalize commands when terminal closes
roomote b15588a
test(terminal): model close lifecycle
roomote 8ed11e8
test(terminal): cover close lifecycle branches
roomote 95f8446
test(terminal): verify wait cleanup edges
roomote 837bda6
test(terminal): keep registry assertion typed
roomote 9ba943d
test(terminal): assert exact process identity
roomote 5efce7c
test(terminal): tighten close regression cleanup
roomote 2f5bafe
refactor(ci): consolidate lifecycle model command
roomote a6f0c75
fix(terminal): settle all startup waits on close
roomote 7740263
test(terminal): cover close cleanup invariants
roomote 4afbc32
test(terminal): prove close finalization idempotency
roomote 1157313
refactor(terminal): remove redundant close state
roomote d96bd86
fix(terminal): finalize superseded active processes
roomote b932d84
refactor(terminal): centralize process tracking
roomote 8266582
test(terminal): verify process tracking cleanup
roomote 7ee1ebd
test(terminal): reject truncated lifecycle exploration
roomote abea7b8
fix(terminal): finalize failed processes once
roomote 6739d29
test(terminal): prove error cleanup ownership
roomote 39f7230
test(terminal): cover stream error cleanup
roomote ea98003
fix(terminal): preserve current process ownership
roomote 7686231
test(terminal): assert completion ownership
roomote 0fba582
test(terminal): assert completion owner state
roomote File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,251 @@ | ||
| type Phase = "idle" | "waiting" | "running" | "completed" | "failed" | "closed" | ||
| type Action = "run" | "wait" | "track-process" | "activate" | "output" | "end" | "error" | "error-superseded" | "close" | ||
|
|
||
| interface ModelState { | ||
| phase: Phase | ||
| processAttached: boolean | ||
| commandSubmitted: boolean | ||
| completionCount: number | ||
| output: string | ||
| deliveredOutput: string | ||
| iteratorReleased: boolean | ||
| waitsCreated: number | ||
| pendingWaits: number | ||
| settledWaits: number | ||
| processesCreated: number | ||
| trackedProcesses: number | ||
| settledProcesses: number | ||
| } | ||
|
|
||
| interface TraceStep { | ||
| action: Action | "initial" | ||
| state: ModelState | ||
| } | ||
|
|
||
| const actions: Action[] = [ | ||
| "run", | ||
| "wait", | ||
| "track-process", | ||
| "activate", | ||
| "output", | ||
| "end", | ||
| "error", | ||
| "error-superseded", | ||
| "close", | ||
| ] | ||
| const MAX_DEPTH = 9 | ||
| const MAX_STATES = 500 | ||
|
|
||
| function initialState(): ModelState { | ||
| return { | ||
| phase: "idle", | ||
| processAttached: false, | ||
| commandSubmitted: false, | ||
| completionCount: 0, | ||
| output: "", | ||
| deliveredOutput: "", | ||
| iteratorReleased: false, | ||
| waitsCreated: 0, | ||
| pendingWaits: 0, | ||
| settledWaits: 0, | ||
| processesCreated: 0, | ||
| trackedProcesses: 0, | ||
| settledProcesses: 0, | ||
| } | ||
| } | ||
|
|
||
| function complete(state: ModelState, phase: "completed" | "closed"): ModelState { | ||
| return { | ||
| ...state, | ||
| phase, | ||
| processAttached: false, | ||
| completionCount: state.processAttached ? state.completionCount + 1 : state.completionCount, | ||
| deliveredOutput: state.output, | ||
| iteratorReleased: state.iteratorReleased || state.phase === "running", | ||
| } | ||
| } | ||
|
|
||
| function transition(state: ModelState, action: Action): ModelState { | ||
| switch (action) { | ||
| case "run": | ||
| return state.phase === "idle" ? { ...state, phase: "waiting", processAttached: true } : state | ||
| case "wait": | ||
| return state.phase !== "closed" && state.waitsCreated < 2 | ||
| ? { ...state, waitsCreated: state.waitsCreated + 1, pendingWaits: state.pendingWaits + 1 } | ||
| : state | ||
| case "track-process": | ||
| return state.phase !== "closed" && state.processesCreated < 2 | ||
| ? { | ||
| ...state, | ||
| processesCreated: state.processesCreated + 1, | ||
| trackedProcesses: state.trackedProcesses + 1, | ||
| } | ||
| : state | ||
| case "activate": | ||
| return state.phase === "waiting" | ||
| ? { | ||
| ...state, | ||
| phase: "running", | ||
| commandSubmitted: true, | ||
| pendingWaits: 0, | ||
| settledWaits: state.settledWaits + state.pendingWaits, | ||
| } | ||
| : state | ||
| case "output": | ||
| return state.phase === "running" && state.output === "" ? { ...state, output: "chunk" } : state | ||
| case "end": | ||
| return state.phase === "waiting" || state.phase === "running" ? complete(state, "completed") : state | ||
| case "error": | ||
| return state.phase === "waiting" || state.phase === "running" | ||
| ? { | ||
| ...state, | ||
| phase: "failed", | ||
| processAttached: false, | ||
| iteratorReleased: state.iteratorReleased || state.phase === "running", | ||
| pendingWaits: 0, | ||
| settledWaits: state.settledWaits + state.pendingWaits, | ||
| trackedProcesses: Math.max(0, state.trackedProcesses - 1), | ||
| settledProcesses: state.settledProcesses + (state.trackedProcesses > 0 ? 1 : 0), | ||
| } | ||
| : state | ||
| case "error-superseded": | ||
| return state.processAttached && state.trackedProcesses >= 2 | ||
| ? { | ||
| ...state, | ||
| trackedProcesses: state.trackedProcesses - 1, | ||
| settledProcesses: state.settledProcesses + 1, | ||
| } | ||
| : state | ||
| case "close": | ||
| return state.phase === "closed" | ||
| ? state | ||
| : { | ||
| ...complete(state, "closed"), | ||
| pendingWaits: 0, | ||
| settledWaits: state.settledWaits + state.pendingWaits, | ||
| trackedProcesses: 0, | ||
| settledProcesses: state.settledProcesses + state.trackedProcesses, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function violations(state: ModelState): string[] { | ||
| const result: string[] = [] | ||
| if (state.completionCount > 1) result.push("a command completed more than once") | ||
| if (state.phase === "closed" && state.processAttached) result.push("a closed terminal retained its process") | ||
| if (state.phase === "closed" && state.pendingWaits !== 0) result.push("a closed terminal retained pending waits") | ||
| if (state.phase === "closed" && state.trackedProcesses !== 0) { | ||
| result.push("a closed terminal retained tracked processes") | ||
| } | ||
| if (state.settledWaits > state.waitsCreated) result.push("more waits settled than were created") | ||
| if (state.settledProcesses > state.processesCreated) result.push("more processes settled than were created") | ||
| if (state.phase === "closed" && state.commandSubmitted && !state.iteratorReleased) { | ||
| result.push("closing a submitted command did not release its stream iterator") | ||
| } | ||
| if ((state.phase === "completed" || state.phase === "closed") && state.deliveredOutput !== state.output) { | ||
| result.push("completion did not deliver all buffered output") | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| function formatCounterexample(message: string, trace: TraceStep[]): string { | ||
| return [ | ||
| `Terminal lifecycle invariant failed: ${message}`, | ||
| `Bounds: depth=${MAX_DEPTH}, states=${MAX_STATES}`, | ||
| ...trace.map((step, index) => `${index}. ${step.action}: ${JSON.stringify(step.state)}`), | ||
| ].join("\n") | ||
| } | ||
|
|
||
| const landmarks = { | ||
| "waiting-close-without-submit": (trace: TraceStep[]) => | ||
| trace.some((step) => step.action === "run") && | ||
| trace.at(-1)?.action === "close" && | ||
| trace.at(-1)?.state.commandSubmitted === false && | ||
| trace.at(-1)?.state.completionCount === 1, | ||
| "running-close-after-output": (trace: TraceStep[]) => | ||
| trace.some((step) => step.action === "output") && | ||
| trace.at(-1)?.action === "close" && | ||
| trace.at(-1)?.state.deliveredOutput === "chunk" && | ||
| trace.at(-1)?.state.iteratorReleased === true, | ||
| "end-then-close": (trace: TraceStep[]) => | ||
| trace.some((step) => step.action === "end") && | ||
| trace.at(-1)?.action === "close" && | ||
| trace.at(-1)?.state.completionCount === 1, | ||
| "error-then-close": (trace: TraceStep[]) => | ||
| trace.some((step) => step.action === "error") && | ||
| trace.at(-1)?.action === "close" && | ||
| trace.at(-1)?.state.processAttached === false && | ||
| trace.at(-1)?.state.completionCount === 0, | ||
| "superseded-error-preserves-owner": (trace: TraceStep[]) => | ||
| trace.at(-1)?.action === "error-superseded" && | ||
| trace.at(-1)?.state.processAttached === true && | ||
| trace.at(-1)?.state.phase === "running" && | ||
| trace.at(-1)?.state.trackedProcesses === 1, | ||
| "duplicate-close": (trace: TraceStep[]) => trace.filter((step) => step.action === "close").length >= 2, | ||
| "concurrent-waits-close": (trace: TraceStep[]) => | ||
| trace.at(-1)?.action === "close" && | ||
| trace.at(-1)?.state.waitsCreated === 2 && | ||
| trace.at(-1)?.state.pendingWaits === 0 && | ||
| trace.at(-1)?.state.settledWaits === 2, | ||
| "superseded-process-close": (trace: TraceStep[]) => | ||
| trace.at(-1)?.action === "close" && | ||
| trace.at(-1)?.state.processesCreated === 2 && | ||
| trace.at(-1)?.state.trackedProcesses === 0 && | ||
| trace.at(-1)?.state.settledProcesses === 2, | ||
| } satisfies Record<string, (trace: TraceStep[]) => boolean> | ||
|
|
||
| const start = initialState() | ||
| const queue: Array<{ state: ModelState; trace: TraceStep[] }> = [ | ||
| { state: start, trace: [{ action: "initial", state: start }] }, | ||
| ] | ||
| const visited = new Set([JSON.stringify(start)]) | ||
| const reachedActions = new Set<Action>() | ||
| const reachedLandmarks = new Set<string>() | ||
| const frontier: ModelState[] = [] | ||
|
|
||
| for (let index = 0; index < queue.length; index++) { | ||
| const node = queue[index]! | ||
| const stateViolations = violations(node.state) | ||
| if (stateViolations.length) throw new Error(formatCounterexample(stateViolations.join("; "), node.trace)) | ||
| for (const [name, predicate] of Object.entries(landmarks)) { | ||
| if (predicate(node.trace)) reachedLandmarks.add(name) | ||
| } | ||
| if (node.trace.length - 1 === MAX_DEPTH) { | ||
| frontier.push(node.state) | ||
| continue | ||
| } | ||
|
|
||
| for (const action of actions) { | ||
| const next = transition(node.state, action) | ||
| const trace = [...node.trace, { action, state: next }] | ||
| for (const [name, predicate] of Object.entries(landmarks)) { | ||
| if (predicate(trace)) reachedLandmarks.add(name) | ||
| } | ||
| if (next === node.state) continue | ||
| reachedActions.add(action) | ||
| const key = JSON.stringify(next) | ||
| if (visited.has(key)) continue | ||
| visited.add(key) | ||
| queue.push({ state: next, trace }) | ||
| if (visited.size > MAX_STATES) throw new Error(`Terminal lifecycle exceeded its ${MAX_STATES}-state budget`) | ||
| } | ||
| } | ||
|
|
||
| const unexploredSuccessor = frontier | ||
| .flatMap((state) => actions.map((action) => ({ action, next: transition(state, action) }))) | ||
| .find(({ next }) => !visited.has(JSON.stringify(next))) | ||
| if (unexploredSuccessor) { | ||
| throw new Error( | ||
| `Terminal lifecycle exploration reached depth ${MAX_DEPTH} with an unseen successor (${unexploredSuccessor.action}); increase the depth bound`, | ||
| ) | ||
| } | ||
|
|
||
| const missingActions = actions.filter((action) => !reachedActions.has(action)) | ||
| if (missingActions.length) throw new Error(`Terminal lifecycle has unreachable actions: ${missingActions.join(", ")}`) | ||
| const missingLandmarks = Object.keys(landmarks).filter((name) => !reachedLandmarks.has(name)) | ||
| if (missingLandmarks.length) | ||
| throw new Error(`Terminal lifecycle has unreachable landmarks: ${missingLandmarks.join(", ")}`) | ||
|
|
||
| console.log( | ||
| `Terminal lifecycle model check passed: ${visited.size} reachable states, ${actions.length}/${actions.length} actions reachable, ${Object.keys(landmarks).length}/${Object.keys(landmarks).length} landmarks reached, depth <= ${MAX_DEPTH}`, | ||
| ) | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bind current-process registration and release to the model transitions.
Terminal.runCommandconstructsTerminalProcess, whose constructor immediately callstrackProcess; completion and error paths callreleaseProcess. Inscripts/check-terminal-lifecycle.ts,runonly setsprocessAttached, andendclears that flag without settling a process.track-processis also reachable without a current process. The model can therefore pass without representing production registration or normal release. Updaterunandendto register and settle the current process atomically, and restricttrack-processto adding a superseded process after a current process exists.🤖 Prompt for AI Agents