Skip to content

fix(opencode): defer config reload until sessions are idle - #42622

Open
matjam wants to merge 1 commit into
anomalyco:devfrom
matjam:reload-when-idle
Open

fix(opencode): defer config reload until sessions are idle#42622
matjam wants to merge 1 commit into
anomalyco:devfrom
matjam:reload-when-idle

Conversation

@matjam

Copy link
Copy Markdown

Issue for this PR

Closes#42621

Type of change

  • Bug fix

What does this PR do?

SIGUSR2 makes the TUI worker reload config, and that reload disposes every instance. Instance disposal cancels the session runners the instance owns, so a signal that lands mid-run kills whatever is in flight. Desktop environments send SIGUSR2 on theme changes (Omarchy runs killall -SIGUSR2 opencode), so switching themes while opencode is working aborts the request.

The reload now waits until no instance has a busy session before invalidating config and disposing, so the reload is deferred rather than dropped. Signals that arrive while waiting join the pending reload instead of stacking.

Themes still update immediately — that path never went through the worker. The TUI re-detects the terminal palette and re-scans theme files from its own SIGUSR2 handler.

InstanceStore.list() is new; the wait needs it to check SessionStatus per instance.

How did you verify your code works?

Manually, before and after, using a shell command so no model call is involved:

  1. bun dev, then !sleep 45, then kill -SIGUSR2 <tui pid> from another terminal.
  2. On dev: the run aborts with "User aborted the command", and the log shows disposing all instances ~5ms after the signal.
  3. On this branch: the run finishes normally, and disposing all instances appears once the session goes idle — so the config reload still happens, just later.

Tests:

  • test/server/global-lifecycle.test.ts (new): awaitSessionsIdle returns while idle, and blocks until a busy session goes idle. Checked that it fails if the busy predicate is broken.
  • test/project/instance.test.ts: coverage for InstanceStore.list().
  • bun typecheck clean in packages/opencode; bun test test/server/httpapi-global.test.ts test/project/ passes.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

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.
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

Scope: config reload (SIGUSR2 from desktop theme changes, config writes) no longer disposes instances mid-run: the reload RPC now waits for all sessions across all loaded instances to go idle before invalidating config, and concurrent reload signals coalesce onto one in-flight promise.

  • The coalescing is correct — late signals join the pending reload, finally clears the slot so a subsequent signal after completion starts a fresh reload rather than being dropped.
  • InstanceStore.list() filtering to successfully-loaded instances via Effect.exit is the right shape (a half-booting instance has no sessions to protect).
  • Unbounded wait concern: if a session never goes idle (hung stream, wedged tool), the reload is deferred forever with no logging and no escape hatch. Since SIGUSR2 reloads are user-triggered, at minimum log "reload deferred, N sessions active" each poll interval (or once), and consider a max-deferral that forces the reload after some generous bound. Silent infinite deferral will be reported as "config changes stopped applying."
  • Polling every 250ms via SessionStatus.list() per instance is fine at this scale; an event-driven wake on session-status change would be cleaner but isn't necessary.
  • Semantics note: this makes reload deferred, not dropped — good choice over cancelling runs, and the comment explaining why disposal cancels runners is valuable context.
  • Tests cover list() and the idle-wait path; suggest adding a test for the coalescing behavior (two overlapping reload calls → single invalidate) since that's easy to regress.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SIGUSR2 theme reload interrupts an in-flight session

2 participants

@matjam@Enough1122