Repository navigation
fix(session): show "Server restarted" after a restart and keep declined turns stopped - #58260
kitlangton wants to merge 5 commits into
Conversation
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
I recommend merging this, with two small concerns below. Before this change, declining a permission was recorded as a server shutdown, so the declined turn resumed on the next boot. I reproduced that on the base: your new permission-restart.test.ts fails there for both a top-level session and a background child, and passes with this PR. The nearby core, client and session-ui tests pass (303 + 35 + 63), and core, session-ui and client type-check cleanly. I didn't drive the desktop or web UI. Before/after screenshots of the "Server restarted" divider and the meta label would help.
Public API: no schema, route, config or durable-event changes. One behaviour change: after a declined permission, session.execution.interrupted now carries reason: "user" instead of "shutdown". It's backwards compatible, since both values already existed. The new metadata.interruptionReason field on assistant messages is only set in projections.
Concerns (details inline):
- Any interrupt without a reason while the server is still running now counts as deliberate. That releases the claim and cancels the session's background jobs. This is right for declines, but it's worth confirming for the mismatched-location interrupt in
context.ts. - The subprocess test is heavy for what it checks.
| const outcome = terminal(exit, reason) | ||
| // Permission declines interrupt the runner while its owner is still alive. | ||
| // Only owner-scope teardown establishes shutdown; explicit user stops win. | ||
| const outcome = terminal(exit, reason ?? (scope.state._tag === "Closed" ? "shutdown" : "user")) |
There was a problem hiding this comment.
With this, every interrupt that has no reason and happens while the owner scope is still open becomes "user". That releases the claim and runs jobs.cancel(sessionID). Declines are the intended case, but other places also interrupt without a reason. For example, SessionContext.select calls Effect.interrupt when the session's location doesn't match (context.ts:130). Before, that path kept the claim. Now it cancels the session's background jobs. Is that intended? If not, an explicit stop reason from the decline path (like stopReason("user")) would avoid guessing from the scope state.
| await using directory = await tmpdir("permission-restart-") | ||
| const path = `${directory.path}/decline.db` | ||
| const run = async (mode: "decline" | "recover") => { | ||
| const child = Bun.spawn([process.execPath, `${import.meta.dir}/fixture/permission-restart.ts`, mode, path, kind], { |
There was a problem hiding this comment.
This test spawns two real processes, depends on a 192-line fixture and a 20s timeout, and requires stderr to be completely empty (expect(stderr, stdout).toBe("")). Any stray warning or log on stderr would fail it for reasons unrelated to this change. session-execution.test.ts already covers claim release on a deliberate stop. Could this be reduced to one in-process case (decline, then run resumeSuspendedSessions against the same database and assert there is no restart notice and no new request), or at least stop requiring stderr to be empty?
34e161d to
105add5
Compare
There was a problem hiding this comment.
I recommend merging. The new commits are a rebase onto dev with small conflict fixes, and the logic is the same as before. I ran the reproduction script from #54180 against both checkouts. On the base, after a permission was rejected and the server restarted, the declined turn resumed with "The server restarted while you were working…". With this PR there was no new model request and no restart message. On the new head, the core, client and session-ui tests near the change pass, and running tsc directly is clean for those packages.
The public API is the same as in my last review. Neither of my earlier points (the scope-state check in execution.ts and the subprocess test) is addressed by these commits, so both threads stay open.
105add5 to
17a0f65
Compare
There was a problem hiding this comment.
I recommend merging; nothing new to raise. The new commits only touch session-ui: blank lines in projection.ts, one assertion rewritten in the timeline test, and a reorganised RestartNotice story. The rest of the change is the same as when I last reviewed it. The session-ui rows-current tests pass and running tsc directly on session-ui is clean. I didn't re-run the #54180 reproduction because no runtime code changed since it last passed. The public API is the same as in my earlier review.
Neither of my earlier points (the scope-state check in execution.ts and the subprocess test) is addressed, so both threads stay open.
17a0f65 to
efc61e5
Compare
There was a problem hiding this comment.
I recommend merging; nothing new to raise. The branch was rebased onto current dev again. Compared with what I last reviewed, the only differences are a dropped formatting-only edit to subagent-completion.ts and a test import updated to @opencode/ai/effect. I re-ran the #54180 reproduction. On the base, a rejected permission turn resumed after the restart; with this PR there was no new model request. The four core tests the description lists pass on the new head (74 tests), and running tsc directly on core is clean. The public API impact is the same as in my earlier review.
Neither of my earlier points (the scope-state check in execution.ts and the subprocess test) is addressed, so both threads stay open.
There was a problem hiding this comment.
I recommend merging; nothing new to raise. Thread 1 is fixed. A declined permission now ends the step with an explicit marker (SessionRunner.declined), and only that marker counts as a deliberate stop. Every other interrupt without a reason, including the location-mismatch one in context.ts, is treated as a shutdown again, as on dev. Thread 2 stays open: the new graceful-shutdown test reuses the same subprocess setup and still requires stderr to be empty.
I re-ran the #54180 reproduction. On the base, the declined turn resumed after the restart; with this PR there was no new model request. 309 core tests pass, including the two new tests for a permission still pending at shutdown, and running tsc directly on core is clean. The public API impact is the same as in my earlier review.
a1b4eb7 to
8ec9ecd
Compare
kitlangton
changed the title
There was a problem hiding this comment.
I recommend merging; nothing new to raise. The new head is another rebase onto dev, plus a commit that fixes the restart-notice spec's import after the stories package was renamed. Compared with what I last reviewed, only conflict fixes changed, and the new import path matches the other specs. One leftover in session-timeline-row.tsx restores an older form of the outcomeNoticeKey(...) call; both forms map state to the same notice key, so behaviour doesn't change.
I re-ran the #54180 reproduction. On the base, the declined turn resumed after the restart; with this PR there was no new model request. The core, client and session-ui tests near the change pass, and running tsc directly on those packages is clean. The public API impact is the same as in my earlier review. The earlier subprocess-test thread stays open.
After a server restart, the cut-off reply now says "Server restarted", and a permission you declined no longer starts running again.
Before, a restart and a Stop press both showed "Interrupted", and declining a permission was recorded as a shutdown, so that turn came back to life after the next restart. A reply you stopped yourself still says "Interrupted".
🔍 Origin: reported in #54180
❌ Before
✅ After
📊 Proof
Rendered from the real timeline in a component story. Before, then after:
permission-restart.test.tsdeclines a permission for real, then runs startup recovery in a separate process, for a top-level session and a background child. Neither resumes. A second test shuts down for real with a permission pending: the claim survives and the session resumes once. Not checked live in the desktop app.👀 Read:
execution.ts(terminal) andrunner/step.ts. Only the decline path adds the "declined" mark, so every other interrupt keeps the old shutdown default; getting that wrong would stop real restarts from resuming.session.execution.interruptednow reportsreason: "user"for declines, where it said"shutdown". No schema or durable-event changes. The rest is routine.The restart mark lives only in the projected read model · rebased onto current
dev, resolving conflicts in the timeline label and two testsChecks run
packages/core:bun test test/permission-restart.test.ts test/session-projector.test.ts test/session-execution.test.ts test/job.test.ts: 82 passpackages/client:bun test test/solid-data.test.ts: 38 passpackages/session-ui:bun test src/timeline/rows-current.test.ts: 64 pass;bun run typecheck: cleanbun run check(lint + typecheck, all packages): passesAdded by the opencode bot (AI-generated).
Fixes #54180
Related: #36347
Public API: session.execution.interrupted now reports reason "user" (was "shutdown") for declined permissions; no schema changes (backwards compatible)