Repository navigation
Conversation
3dbe7c9 to
8047e97
Compare
- Protocol 8: ProcessInfo with pty/pipe groups, create_pty, get, list { mode },
required features; drop migrate(), the features fallback and the rename
feature check (UnsupportedError).
- Keep daemon request errors (kind "request") apart from connect/startup
failures; PersistentPty.get uses get { id } and maps unknown ids and pipe
processes to NotFoundError.
- daemon.wait on its own connection with no timeout; interrupting closes it.
- Pipe process API for Shell: createPipe, waitPipe, getPipe, signalPipe,
setPipeMetadata, terminatePipe, listPipes.
- Fake daemon speaks v8 including pipe requests.
8047e97 to
3050c9b
Compare
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
Works as described for interrupts. On the base, interrupting a running shell call gave the model Tool execution interrupted with no output. With this change the model gets the output tail, the saved-file notice and the shell ID, the command runs in the 0.5.0 daemon and is stopped. Before/after screenshots below. Core and server tests and typecheck:tsc pass. I did not drive the planned-restart handoff end to end; only the fake-daemon tests cover it.
Public API: the shell tool result's metadata gains an optional interrupted field, and the persistent-pty rename route no longer returns PtyFeatureUnsupportedError (the error is still declared in the protocol, so nothing breaks for clients). The daemon protocol bump to v8 is internal. Backwards compatible.
Concerns (inline): what happens to daemon commands when a Location is evicted or reloaded without a server restart, and the timing-based sleeps in shell-daemon.test.ts.
Fixes #39565
Related: #36348, #51828, #57977
| // the daemon's commands back. Once closed, this one leaves their timers, stops and reporting to it, and only | ||
| // still answers its own waiters. | ||
| let closed = false | ||
| yield* lifecycle.awaitShutdown.pipe( |
There was a problem hiding this comment.
Could you check what happens when a Location shuts down without a server restart (idle eviction like #51828, or invalidate)? Once closed is set, stop() returns early (L556), and teardown only detaches daemon commands. So the background job's onInterrupt → shell.stop(id, "killed") does nothing. As far as I can tell, Job.settle then marks the job cancelled, because the registry isn't closed and we're not detaching. The process keeps running in the daemon, with no timeout.
The next boot of that Location takes it back in recover(), but only SessionRestart calls ShellTool.reattach, so no job watches it and the session never hears how it ended. The 7-day sweep also skips running processes. Either stop daemon commands on a Location close that isn't a handoff, or reattach their jobs when the replacement Location boots.
There was a problem hiding this comment.
Confirmed, and fixed in 923b926. The scenario was reachable through a Location reload, vcs.init / debug.location.evict (plain invalidate), and narrowly through idle eviction. A closed Location now keeps watching the daemon commands it started (exit, timeout, stop), so the job finishes normally and the session is notified; only a server shutdown with a prepared handoff detaches. stop() works after close again, so jobs.cancel / cancelSession stop the command. The replacement still takes the commands back and is the only one that publishes events and applies retention; both read the deadline and stop reason from the daemon, so they can't disagree. notifyWhenDone no longer lives in the plugin's Location scope. New test in tool-shell: a background job survives a reload, cancelling one sends SIGTERM, the other completes and notifies.
There was a problem hiding this comment.
The fix in 923b926 covers the scenario I raised. I read the diff and ran the tests in packages/core. I didn't change anything in the checkout; the bun.lock modification was already there and I left it alone.
- Finalizer (
shell.ts~L244): daemon commands are skipped unlessdetaching.isStopping()is true. A reload or eviction therefore leaves their wait and timeout fibers running. Those fibers come fromrunForkWith(context), not the Location scope, and they only needPersistentPty, which is a global node. The transport stays usable after the Location closes. stop(): theclosedearly return is gone, sojobs.cancel/cancelSessionon the old Location now send SIGTERM.- Two watchers: the old Location and its replacement both read
stopReasonandtimeoutDeadlineback throughlatest(), at timer expiry and on exit, so they report the same status. If both happen to stop the same command, it gets two SIGTERMs, which does no harm. notifyWhenDone:forkDetachtakes it out of the plugin's Location scope, so the session is still notified after the Location closes.- Tests:
test/tool-shell.test.ts(63 pass, 23 skip, including the new reload test), plusshell,shell-daemon,detachingandjob(37 pass, 1 skip). All of these ran on this machine's default path; I didn't run them against a real v8 daemon binary.
I don't see anything blocking.
| expect(env.daemon.ops).not.toContain("terminate") | ||
| expect(env.signals(1)).toEqual([]) | ||
| expect(env.signals(4)).toEqual(["SIGTERM"]) | ||
| yield* Effect.sleep(150) |
There was a problem hiding this comment.
These tests use real-clock sleeps (Effect.sleep(20/50/100/150)) against 50–100 ms command timeouts to decide whether a timer did or didn't fire (also L219, L299–L326). On a loaded CI machine that will flake. Could they use TestClock (already imported), or wait on the fake daemon's signals/ops instead of fixed delays?
There was a problem hiding this comment.
Fixed in 0d93fd7. No real-clock sleeps remain in shell-daemon.test.ts: timers run on TestClock, and fake-daemon socket I/O is awaited with an until(condition) helper. The restart test sets an expired deadline directly instead of sleeping past it. Also fixed an EPIPE in the fake daemon and a retention race that showed up under load; 11 runs under 2x CPU load pass. The real-daemon test now runs by default against @opencode/pty 0.5.0.
There was a problem hiding this comment.
Checked against pr/head (0d93fd7). This resolves my concern.
shell-daemon.test.tsno longer has anyEffect.sleepor bareBun.sleep. Timeouts and the SIGTERM→SIGKILL grace period are driven byTestClock.adjust(L165–170, L285, L317, L346). Everything else waits on fake-daemon state throughuntil().until()(fixture/shell-daemon.ts:107) polls in real time on purpose, since the test clock can't drive socket replies. It gives up after 2,000 attempts of about 2 ms each, so a slow machine waits longer instead of failing.- The restart test sets
timeoutDeadline: Date.now() - 1(L243). That matches the code:shell.ts:745restores timers fromdeadline - Date.now()on the wall clock, so an expired deadline fires at once with no clock involved.
One small thing: if process 3 is ever missing, if (expiring) at L243 skips the setup silently. shell.wait(before.expiring.id) would then hang until the test times out instead of failing with a clear message. Asserting it exists would be clearer, but it isn't a flake risk.
I didn't run the suite. The shared checkout's working tree is on an older commit with a modified bun.lock, so I didn't touch it, and I made no changes.
There was a problem hiding this comment.
Both earlier points are addressed. A Location that closes while the server keeps running now keeps watching its daemon commands. Timeouts and stops are read back from the daemon's metadata, and the background notifier no longer stops with the Location, so the session still hears how a command ended. The new tool-shell test covers this across a Location reload. The shell-daemon tests now use TestClock and the fake daemon's ops instead of real sleeps.
Re-checked at the new head: interrupting a shell call still gives the model the output tail, the saved-file notice and the shell ID. Core shell/job/session tests and typecheck:tsc pass. The public API impact is the same as in my earlier review. Nothing new to raise.
…ad of at shutdown
There was a problem hiding this comment.
Both earlier points are still addressed in this head. Daemon commands now live in a server-wide registry (Shell.daemonNode), so a Location closing no longer takes them down, and background-job notices run in the server's scope through daemon.watch. The shell-daemon tests use TestClock and the fake daemon's ops.
Re-checked with the simulated model: interrupting a running shell call gives the model the output, the saved-file notice and the shell ID (the base gives Tool execution interrupted with no content). The TUI now shows that result too; before/after screenshots below. Core and server tests and typecheck:tsc pass.
Public API: the shell tool's structured output gains an optional killed string, and its metadata gains killed: true when set. An interrupted result now has interrupted: true and no longer has shellID in its metadata; the text names the shell ID. The persistent-pty rename route no longer returns PtyFeatureUnsupportedError. All backwards compatible.
The latest commits also reformat generate.ts, schedule.ts and artifact.ts, which aren't part of this change; those hunks can be dropped.
Issue for this PR
Closes #
Type of change
What does this PR do?
Runs OpenCode's local shell commands in the
opencode-ptydaemon (protocol v8, daemon PR #61875) so they survive a planned server restart, and stops interrupts from throwing away a command's output.Uses
@opencode/pty0.5.0 (release), built from #61875 and released in #62007. Protocol 8 is breaking: on upgrade, a running v7 daemon's handoff is discarded and persistent terminals start fresh once.Transport (
persistent-pty/):renamefeature check,migrate()and other v7 compatibility.createPipe,waitPipe,getPipe,signalPipe,setPipeMetadata,terminatePipe,listPipes). Errors are split:UnavailableError(request never sent, safe to fall back) vsRequestError(never re-run).Shell commands:
sh_X.outfile. The server reads the file directly for the UI and the model; no output goes through the socket or server memory. Falls back to today's spawner only if the daemon can't be reached.Shell.stop: SIGTERM, 3 s, SIGKILL, keeping the command and its file. Timeouts use it.remove, the 25-command limit and the 7-day sweepterminatethe daemon process before deleting its file.Interrupt fix (applies to both paths):
Planned restart:
Job.settleno longer turns a teardown interrupt into "cancelled", so background shell jobs keep their row instead of sending "Cancelled" to the model.Not handled: user-run
!cmdcommands aren't recovered after a restart.How did you verify your code works?
bun run typecheck:tscin core and server. Core tests (shell, shell tool, job, session runner/step, session execution/restart, location activity, persistent-pty, plus newshell-daemonanddetachingtests): 476 pass with a v8 daemon binary, and the same suite passes without one (spawner fallback). Session tests were run with an isolatedHOME. Server process, persistent-pty, job and message tests pass, including the persistent-pty smoke tests against the published 0.5.0 binary. Most shell tool tests run on the spawner, with a parity set on both paths; daemon tests use the in-memory fake daemon, plus one opt-in test against the real binary.Checklist
Added by the opencode bot (AI-generated).
Fixes #39565
Related: #36348, #51828, #57977
Public API: adds optional
killedandinterruptedto shell tool result metadata, dropsshellIDfrom interrupted results; persistent-pty rename no longer returns PtyFeatureUnsupportedError (backwards compatible)