Repository navigation
fix(core): reclaim background shell notifications after plugin rebuild - #56780
reynold-hu wants to merge 2 commits into
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
This fixes the reported case. I ran the #56729 reproduction (the PR's test) on the base branch and with the PR. On the base, the rebuild case gets no notification and leaves the background marker behind. With the PR, exactly one notification arrives and the marker is cleared. tool-shell, job, session-execution and the new test pass, and packages/core typecheck is clean.
Public API: unchanged.
Concerns:
- Every location reclaims every location's backgrounds.
Jobis a global node andpendingBackgroundscans all markers, but the shell plugin is activated once per location. So whenever any location activates the shell plugin, including its first activation, it starts a waiter on every running background shell in the process, even ones whose own observer is still alive. I confirmed this with two locations: B's reclaim picked up A's running shell. Delivery is still deduplicated by notification ID, but the reclaim waiter and the live observer race on the same job. If the reclaim wins, the session gets a notification without the shell output metadata (exit code, truncation, ...), becausereclaimBackgroundnever passesoutputtoShellResult.notification. See the inline comments. - Approach. This copies the notification logic a third time (
notifyWhenDone,recoverShellinsession/execution/restart.ts, and nowreclaimBackground). RunningnotifyWhenDonein a scope that lives as long as the Job/Shell service, rather than the plugin activation scope, would fix the bug where it starts and needs no reclaim pass. If you keep the reclaim, limit it to markers whose observer was actually interrupted, at least to the current location's sessions. - The test is the bot's reproduction pasted almost verbatim, about 160 lines of copied setup, a fixed sleep and debug output. See the inline comment.
| // A rebuilt activation closes the scope that owned the previous notification observers. Reclaim | ||
| // persisted backgrounds owned by this process so their completion still reaches the Session. | ||
| // Jobs from a previous process are unknown here and stay for restart recovery. | ||
| yield* reclaimBackgrounds(jobs, sessions).pipe(Effect.forkIn(scope, { startImmediately: true })) |
There was a problem hiding this comment.
This runs on every activation of the shell plugin in every location, including the first one. Since Job is global, pendingBackground returns markers from all locations. A newly opened location B will therefore start waiting on location A's running background shells, whose observer was never interrupted, and race it to deliver the notification. I reproduced the extra waiter with two locations. Running notifyWhenDone in a longer-lived scope (Job/Shell service or the location layer) instead of the activation scope would avoid the reclaim and the race entirely. Otherwise, could the reclaim be limited to markers whose observer is known to be gone?
| jobID: background.id, | ||
| shellID: recovery.shellID, | ||
| command: recovery.command, | ||
| state: info.status, |
There was a problem hiding this comment.
Unlike notifyWhenDone, this doesn't pass output, so the notification has no ShellResult.metadata (exit code, truncation, etc.). The inbox keeps the first admission for an ID, so when this path wins the race against a live observer, the session's notification loses that metadata, and nothing reports it. This logic also duplicates notifyWhenDone and recoverShell in session/execution/restart.ts. A shared helper, or not needing the reclaim at all, would keep them consistent.
| yield* plugins.activate(rebuild ? [shellGen] : [prefix, shellGen]) | ||
|
|
||
| const result = yield* jobs.wait({ id: shellID }) | ||
| yield* Effect.promise(() => Bun.sleep(1500)) |
There was a problem hiding this comment.
This test is the reproduction script copied nearly verbatim: about 160 lines of setup from tool-shell.test.ts, the // Repro for header, console.log of the state, as any casts and a fixed Bun.sleep(1500) (timing-sensitive on slow CI). Could it be cut down to one focused case in tool-shell.test.ts, reusing its existing layers and withSession? Rebuild while a background shell runs, then wait for the notification event (e.g. take the first matching InboxEnqueued) instead of sleeping, and drop the debug output.
|
Addressed the review in 27ce8ed:
|
27ce8ed to
80d6122
Compare
Issue for this PR
Closes #56729
Type of change
What does this PR do?
ShellToolforks its completion observer into the plugin activation scope. WhenPlugin.activaterebuilds the shell plugin suffix, that scope closes and the observer is interrupted, so a background shell that finishes afterwards never notifies its Session: the Job and its persisted background marker survive, but the Session receives nothing and the marker is never acknowledged.The shell plugin now reclaims persisted shell backgrounds when it is activated:
Jobs from a previous process are unknown to this process, so they are left for the existing restart recovery path. Delivery stays deduplicated by the persisted notification ID, so a reclaim that races a live observer still produces one notification, and
completeBackgroundis idempotent. The observer itself stays owned by the shell tool; this only covers the lifetime gap between activations.How did you verify your code works?
packages/core/test/shell-plugin-rebuild.test.tsfollows the reproduction in the issue: activate[audit.prefix, opencode.tool.shell], start a background shell, re-activate with only the shell plugin, then release the process. The rebuilt case now admits one notification and clears the marker; the unchanged-activation control still passes. On the parent commit the rebuilt case fails withnotices: 0andpending: 1.bun test test/tool-shell.test.ts test/job.test.ts test/session-execution.test.ts test/shell-plugin-rebuild.test.ts: 97 pass, 63 skip, 0 fail.packages/coretypecheck passes;test/config/shell.test.tsand the shell-scan suites (328 tests) pass.Screenshots / recordings
No UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #51828, #50751