Conversation
SIGUSR2 asks the TUI worker to reload config, which disposes every instance. Instance disposal cancels the session runners that instance owns, so a signal that lands while the model is streaming interrupts the run. Desktop environments send this signal on theme changes - Omarchy's omarchy-theme-set runs `killall -SIGUSR2 opencode` - so switching themes mid-run aborts the in-flight request. Theme refresh does not depend on the worker reload: the TUI re-detects the terminal palette and re-scans theme files from its own SIGUSR2 handler. Wait for every instance to have no busy session before invalidating config and disposing, and coalesce signals that arrive while waiting, so the reload is deferred rather than dropped.
Fold the idle wait, config invalidation, and disposal into a single reloadWhenSessionsIdle effect so the worker only coalesces overlapping signals and the whole path can be exercised in a test. Cover both the immediate reload when no session is busy and the deferred one, asserting on global.disposed and instance identity rather than on the wait helper.
InstanceStore.list snapshotted the cache before awaiting each entry's boot, so an instance that loaded while an earlier one was still booting was missing from the idle check but present in the disposal that followed. Re-snapshot after the await and repeat until the set is stable. Add a gated-bootstrap regression test for the case.
holny
left a comment
holny
left a comment
There was a problem hiding this comment.
Ran the suites this touches locally at 3b4d836: test/server/global-lifecycle.test.ts + test/project/instance.test.ts, 13/13 pass, typecheck clean. (The full test/project + test/server run is too slow to finish in one go on my machine, so I stuck to the touched files.)
Things I checked and am happy with:
- The idle predicate is sound: set deletes the map entry when the status goes idle (status.ts:44-48), so SessionStatus.list() never carries idle residue — size > 0 means a live run and the poll can't spin forever. Busy is set at the top of each step (processor.ts:653) and idle only on run end or halt, so retries and permission waits stay counted; I couldn't find a mid-run window the 250ms poll could slip through.
- InstanceStore.list()'s re-snapshot loop converges: each iteration awaits the current snapshot's deferreds, so a set that changed under you gets re-collected with forward progress instead of spinning on the same entries.
- The promise dedupe in worker.reload is right for the SIGUSR2 storm case, and the .finally gap can't be hit from an RPC that arrives on a later macrotask.
Two things I'd ask about:
-
The reload now blocks until every session in every instance is idle, with no bound and no trace. For the desktop theme trigger that means a theme change can silently wait out a long agent run. Worth at least a log line when it defers, and maybe a cap or fallback if you don't want this open-ended — otherwise "my theme didn't change" reports are going to be hard to diagnose.
-
list() filters failed entries out (exits.filter(Exit.isSuccess)), so a caller can't tell "never loaded" from "failed to load". That's fine for this use since a failed instance has no live sessions, but it's a new entry point on InstanceStore and a line on the type or a doc comment would save the next caller the surprise.
Agreed on the boundaries you already noted, for what it's worth — the check→dispose gap is narrow and the alternative was aborting the run, so deferring looks like the right trade.
Log once when a reload starts waiting on busy sessions, with the count, and again when it proceeds, so a config change that seems ignored can be traced to a run still in progress. Note on InstanceStore.list that it awaits booting entries and omits ones whose boot failed.
|
Thanks for the careful read. Both addressed in the latest commit:
|
Why this matters
EvidenceAgainst tip
Confirms scope — no expand. Author’s unbounded-wait rationale noted. Ask (design, light)
Happy to help land as-is. |
4 participants
Issue for this PR
Closes #42621
Type of change
What does this PR do?
Desktop theme switchers (Omarchy, Noctalia) send SIGUSR2 to refresh the TUI. The worker reloads config on that signal, and the reload disposes every instance. Disposal cancels the session runners the instance owns, so a signal that lands mid-run aborts the model stream or shell command in flight.
The reload now waits until no instance has a busy or retrying session before invalidating config and disposing, so it is deferred rather than dropped. Signals that arrive while one is pending join it.
This revives #42622 by @matjam, which took the same approach and was closed by the automated cleanup rather than on review. The first commit is theirs, rebased onto dev. The second folds the wait, invalidation, and disposal into one
reloadWhenSessionsIdleeffect so the whole path is covered by tests. The third fixes a gap in the newInstanceStore.list: it snapshotted the cache before awaiting each entry's boot, so an instance that loaded while an earlier one was still booting was missed by the idle check but included in the disposal that followed. It now re-snapshots after the await and repeats until the set is stable.Known limits, unchanged from before:
I opened #48990 earlier with a different approach (skip the dispose when a fingerprint of config inputs is unchanged). Review showed the fingerprint could not track everything the loaders read, so I am closing it in favor of this fix.
How did you verify your code works?
test/server/global-lifecycle.test.tsexercises the reload effect end to end: with no busy session it disposes and emitsglobal.disposed; with a busy session it holds, leaves the instance loaded, then reloads after the session goes idle; with a gated bootstrap, a session that starts on a second instance while the first is still booting also holds the reload. Each was written first and failed before its change.bun run test test/project test/server: 390 passed, 3 skipped, 0 failed.bun typecheckinpackages/opencodepasses; prettier check passes on touched files.Screenshots / recordings
N/A — no UI change.
Checklist