Skip to content

fix(tui): skip instance disposal on SIGUSR2 reload when config is unchanged - #48990

Closed
khughitt wants to merge 3 commits into
anomalyco:devfrom
khughitt:fix-sigusr2-conditional-dispose
Closed

khughitt wants to merge 3 commits into
anomalyco:devfrom
khughitt:fix-sigusr2-conditional-dispose

Conversation

@khughitt

@khughitt khughitt commented Sep 14, 2026

Copy link
Copy Markdown

Issue for this PR

Closes #42621

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Theme switchers send SIGUSR2 to refresh the TUI. The worker previously disposed all instances on every signal, aborting active model streams and tools even when configuration was unchanged.

Reload now compares global config and fingerprints project config, OPENCODE_CONFIG, and discovered agent/mode/command/plugin files. Fingerprints follow symlinks and expand file references. Comparisons preserve permission-rule order while ignoring harmless JSON key reorders. The baseline is captured before bootstrap consumes config, and failed reads conservatively trigger disposal without leaving instance loads pending. Theme files and loader-added schema metadata do not trigger disposal. The configUpdate route is unchanged.

How did you verify your code works?

  • Added regressions for permission precedence, referenced files, symlinked directories, fingerprint defects, and edits during bootstrap; confirmed they failed before the fixes.
  • Also covered loader-added schema metadata and unreadable files; existing theme-only and cached-config recovery tests remain.
  • bun run test test/server test/config test/project: 638 passed, 6 skipped, 0 failed. bun run typecheck and all 30 pre-push workspace typechecks passed; formatting passed; oxlint reported no errors and only two pre-existing return-style warnings.

Screenshots / recordings

N/A — no UI change.

Checklist

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

…hanged

The TUI forwards SIGUSR2 to the worker's reload RPC, which invalidated
the global config cache and unconditionally disposed every instance,
aborting all in-flight sessions. Theme switchers (Omarchy, Noctalia
hooks, ...) signal on every wallpaper change, so a periodic rotation
kills active agent work every time.

Themes are discovered from themes/*.json and are not part of the global
config, so a theme-only signal never needs an instance rebuild. Reload
now re-reads the global config and disposes instances only when it
actually changed; the configUpdate route behavior is unaffected.
@github-actions github-actions Bot added needs:compliance This means the issue will auto-close after 2 hours. needs:issue labels Sep 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

@github-actions

Copy link
Copy Markdown
Contributor

The following comment was made by an LLM, it may be inaccurate:

Related PR found:

@github-actions github-actions Bot removed needs:compliance This means the issue will auto-close after 2 hours. needs:issue labels Sep 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for updating your PR! It now meets our contributing guidelines. 👍

Review follow-ups:

- Read failures can no longer wedge reload: the cached read runs through
  catchCause and invalidate() always executes. An unreadable config is
  treated as changed (the previous unconditional-dispose behavior), so a
  malformed file neither skips invalidation nor pins the cache.
- The gate now also covers per-instance on-disk inputs (project config,
  agent/command/mode/plugin files, OPENCODE_CONFIG) via a content
  fingerprint captured at instance boot, so editing those and signaling
  still reloads the instance. Theme files stay excluded.
- Config comparison is key-order insensitive (canonicalEquals), so pure
  key reordering no longer counts as a change.
@khughitt

Copy link
Copy Markdown
Author

Addressed all three findings in 1fcafcb (pushed to this branch):

P1 — malformed config wedging reload. Verified the premise: ConfigParse.jsonc throws, which surfaces through the cached loadGlobal as a defect that orElseSucceed doesn't cover, and my original ordering read getGlobal() before invalidate(). The gate now reads via catchCause (covers defects, logs a warning), always runs invalidate(), and treats an unreadable config as changed — the previous unconditional-dispose behavior — so a bad file can neither skip invalidation nor pin the cache. global-lifecycle.test.ts covers failure → disposal with invalidation, then normal gating after the file is fixed.

P2 — project/custom config inputs. The gate now covers per-instance inputs, not just the global config. ConfigFingerprint.hashInstanceInputs(directory, worktree) hashes exactly what the instance loader reads from disk: the project opencode.json/jsonc chain, every config directory's agent/mode/command/plugin files (mirroring the loader, including its .opencode filter for the json files), and the OPENCODE_CONFIG override. InstanceStore captures the fingerprint at boot and exposes configChanged(), which re-hashes loaded instances and reports drift; a missing or unreadable baseline counts as changed. Theme files are deliberately excluded — they're rendering-only, and are what the signal is actually about. Covered by a configChanged integration test in instance.test.ts and unit tests in fingerprint.test.ts.

P2 — key reorder false positive. Config comparison now uses canonicalEquals (order-insensitive structural equality), and the fingerprint canonicalizes parsed JSON before hashing, so pure key reordering no longer disposes or registers as drift. Both levels have tests.

One related note: #42622 (defer disposal until sessions idle) is a complementary approach — this PR gates whether to dispose, that one would gate when. If both land, the gate here still avoids needless rebuilds for theme-only signals.

Verification: bun run typecheck clean; bun test test/server test/config test/project → 629 pass, 0 fail; oxlint clean on touched files.

@khughitt

Copy link
Copy Markdown
Author

Closing in favor of #49162. Review showed the fingerprint gate could not track everything the loaders read (managed config, auth, skills, tools, and permission-rule ordering), so any miss would leave stale state running. #49162 keeps the unconditional reload and instead defers disposal until sessions are idle, which fixes the reported abort for every reload trigger.

@khughitt khughitt closed this Sep 15, 2026
Sign up for free to 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

1 participant