fix(lifecycle): use PTY-based renderer for lifecycle terminal output#1425
Conversation
The migration to PTY-based lifecycle execution lost the SIGTERM→SIGKILL escalation when stopping a run process. After 8 seconds, the fallback kill now correctly sends SIGKILL instead of a redundant SIGTERM.
|
@ckafrouni is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR replaces Key changes:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| src/main/services/ptyManager.ts | Adds startLifecyclePty and startLifecycleSpawnFallback. The PTY path uses '-ilc' shell flags causing login/interactive startup noise in lifecycle output; fallback path correctly omits these flags. Lifecycle PTYs are registered in the shared ptys map with kind:'local', making them indistinguishable from interactive terminals to existing consumers. |
| src/main/services/TaskLifecycleService.ts | Cleanly migrates from ChildProcess to LifecyclePtyHandle throughout. The new run-guard (existence in runPtys only) is correct because entries are removed on exit. The waitForPtyExit helper correctly registers onExit before stopRun to avoid race conditions, and the SIGTERM→SIGKILL escalation is properly gated on identity equality. |
| src/renderer/components/LifecycleTerminalView.tsx | New read-only xterm.js terminal component for lifecycle output. Terminal init effect uses empty deps (intentional) but captures stale pre-async font values causing a brief font reflow on mount. Incremental append optimization via startsWith is correct. ResizeObserver and event listeners are properly cleaned up. |
| src/renderer/components/TaskTerminalPanel.tsx | Replaces <pre> tag with LifecycleTerminalView; correctly keys on task id + phase to force terminal recreation on selection change. The lifecycleLogContent useMemo is a straightforward extraction of existing logic. |
| src/test/main/TaskLifecycleService.test.ts | Tests are cleanly migrated from child_process mocks to LifecyclePtyHandle mocks. The mock factory provides emitData/emitExit/emitError helpers for fine-grained event control. All 9 tests correctly reflect the new PTY-based implementation. |
Sequence Diagram
sequenceDiagram
participant TLS as TaskLifecycleService
participant PM as ptyManager
participant PTY as node-pty / spawn fallback
participant UI as LifecycleTerminalView
Note over TLS,UI: startRun / runFinite (setup|teardown)
TLS->>PM: startLifecyclePty({ id, command, cwd, env })
alt node-pty available
PM->>PTY: pty.spawn(shell, ['-ilc', command], opts)
PM-->>PM: ptys.set(id, { proc, kind:'local' })
else EMDASH_DISABLE_PTY=1 or require fails
PM->>PTY: spawn(command, { shell:true, detached:true })
end
PM-->>TLS: LifecyclePtyHandle { pid, onData, onExit, onError, kill }
TLS->>TLS: runPtys.set(taskId, handle)
PTY-->>TLS: onData(line) → emitLifecycleEvent('line')
TLS-->>UI: IPC line event → content string grows
UI->>UI: useEffect([content]): startsWith check
alt content appends previous
UI->>UI: terminal.write(appended)
else content reset
UI->>UI: terminal.reset() + terminal.write(content)
end
Note over TLS,PTY: stopRun (SIGTERM → SIGKILL escalation)
TLS->>TLS: stopIntents.add(taskId)
TLS->>PM: handle.kill() [SIGTERM]
TLS->>TLS: setTimeout(8s) → handle.kill('SIGKILL')
PTY-->>TLS: onExit(code) → runPtys.delete(taskId)
Note over TLS,PTY: runTeardown waits for run to exit
TLS->>TLS: waitForPtyExit(handle, isTracked, 10s)
TLS->>TLS: stopRun(taskId)
PTY-->>TLS: onExit fires → finish() resolves
TLS->>TLS: runFinite('teardown', …)
Last reviewed commit: 5b771c9
| maybeProc.on?.('error', emitError); | ||
| } catch {} | ||
|
|
||
| ptys.set(id, { id, proc, cwd, kind: 'local', cols: 120, rows: 32 }); |
There was a problem hiding this comment.
Lifecycle PTYs added to the shared ptys map without a cleanup guard
ptys.set(id, ...) registers this lifecycle PTY in the same global map used for interactive terminal PTYs. The kill() closure and the onExit handler both call ptys.delete(id), so the entry is normally removed.
However, if a caller constructs the LifecyclePtyHandle but never calls any of the registered callbacks (e.g., startLifecyclePty is called but the returned handle is immediately discarded via clearTask calling ptys.delete from the kill() closure), the ptys map is still eventually cleaned. This is fine.
The subtle concern is that lifecycle PTY entries (lifecycle-run-*, lifecycle-setup-*, etc.) are now visible to exported helpers like getPty(id), getPtyKind(id), and hasPty(id). If any consumer iterates or checks for these IDs in a way that assumes all entries are interactive terminals, it may behave unexpectedly. Adding a comment or a distinct kind value (e.g., kind: 'lifecycle') would make the distinction explicit and enable guarding in those helpers.
|
One more thing: Lifecycle PTYs registered are now registered global ptys map, before they were just subprocesses, so not registered. The id format is "lifecycle-run-{taskId}", different than the agent pty. Should i just not have them registered in the global pty list, or add a "lifecycle" kind to the pty map? |
When EMDASH_DISABLE_PTY=1 is set or node-pty fails to load, lifecycle scripts now gracefully degrade to a child_process.spawn-based implementation instead of hard-failing. Also fixes the onExit signal type to match node-pty's actual typings.
The ptys map entries already have a kind field typed as 'local' | 'ssh'. I'm not sure how you guys already differentiate between the main agent terminals, and the ones in the sidebar, I'm guessing it is purely id based ?
What I have now is
I don't know if you guys wan't to differentiate them even more, by adding another property or kind just for lifecycles. |
|
Thanks, @ckafrouni! Just resolved a merge conflict and formatted the code. |
|
Heyy bud, thank you. Sorry you had to take care of the merge conflict for me. But glad it worked out ! |
Fixes #1304
Summary
child_process.spawn()withnode-ptyfor lifecycle scripts (setup/run/teardown) so output includes proper ANSI colors and formattingLifecycleTerminalViewcomponent that renders lifecycle output in a read-only xterm.js terminal instead of a plain<pre>tagstopRunTest plan
stopRunkills a stubborn process after 8s via SIGKILL escalationpnpm exec vitest run src/test/main/TaskLifecycleService.test.ts— all 9 tests pass