Add config hot reload and targeted invalidation#9
Conversation
|
@codex review this PR |
There was a problem hiding this comment.
Pull Request Overview
This PR implements config hot reload functionality with targeted invalidation for OpenCode, allowing configuration changes to be applied without full server restarts. The implementation introduces a feature-flagged pipeline that computes config diffs and selectively invalidates only the affected subsystems (provider, MCP, LSP, plugin, etc.).
Key changes:
- New state management infrastructure with string-based registration and selective invalidation via
State.register()andState.invalidate() - Safe config persistence with file locking, backup/restore, and JSONC comment preservation using
jsonc-parser - Event-driven invalidation system that publishes
config.updatedevents and applies targeted refreshes based on diff analysis
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/config-spec.md | Documents PATCH /config endpoint behavior, feature flags, and client workflows |
| packages/opencode/test/config/write.test.ts | Tests JSONC comment preservation and incremental edit validation |
| packages/opencode/test/config/hot-reload.test.ts | Comprehensive hot reload tests covering project/global scopes, event publishing, and targeted invalidation |
| packages/opencode/test/config/config.test.ts | Updated config tests for new API signature and plugin resolution |
| packages/opencode/src/config/lock.ts | File locking mechanism with timeout-based acquisition |
| packages/opencode/src/config/backup.ts | Backup/restore utilities for atomic config updates |
| packages/opencode/src/config/write.ts | JSONC writer using jsonc-parser for comment-preserving incremental updates |
| packages/opencode/src/config/persist.ts | Main persistence logic with locking, diffing, validation, and cache invalidation |
| packages/opencode/src/config/diff.ts | Config diff computation for detecting changed sections |
| packages/opencode/src/config/error.ts | Typed error definitions for config operations |
| packages/opencode/src/config/invalidation.ts | Targeted invalidation handlers that map diff sections to subsystem refreshes |
| packages/opencode/src/config/hot-reload.ts | Feature flag helper for checking hot reload enablement |
| packages/opencode/src/config/global-file.ts | Global config file path resolution |
| packages/opencode/src/config/config.ts | Updated Config.update() to new signature, added event definitions, and plugin path resolution |
| packages/opencode/src/server/server.ts | Enhanced PATCH /config with scope parameter, event publishing, and toast enrichment |
| packages/opencode/src/project/state.ts | New State.register() and State.invalidate() APIs for named state management |
| packages/opencode/src/project/instance.ts | Added Instance.invalidate() and Instance.forEach() helpers |
| packages/opencode/src/project/bootstrap.ts | Wired ConfigInvalidation.setup() into initialization |
| packages/opencode/src/tool/registry.ts | Converted to State.register() for targeted invalidation |
| packages/opencode/src/provider/provider.ts | Converted to State.register() for targeted invalidation |
| packages/opencode/src/plugin/index.ts | Converted to State.register() with cleanup dispose handler |
| packages/opencode/src/permission/index.ts | Converted to State.register() for targeted invalidation |
| packages/opencode/src/mcp/index.ts | Converted to State.register() for targeted invalidation |
| packages/opencode/src/lsp/index.ts | Converted to State.register() for targeted invalidation |
| packages/opencode/src/format/index.ts | Converted to State.register() for targeted invalidation |
| packages/opencode/src/file/watcher.ts | Converted to State.register() for targeted invalidation |
| packages/opencode/src/command/index.ts | Converted to State.register() for targeted invalidation |
| packages/opencode/src/agent/agent.ts | Converted to State.register() for targeted invalidation |
| IMPLEMENTATION_SUMMARY.md | Comprehensive implementation documentation with architecture decisions and migration guide |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let releaseFn: () => void | ||
| const lockPromise = new Promise<void>((resolve) => { | ||
| releaseFn = resolve | ||
| }) | ||
|
|
||
| fileLocks.set(normalized, lockPromise) | ||
|
|
||
| return () => { | ||
| fileLocks.delete(normalized) | ||
| releaseFn!() |
There was a problem hiding this comment.
The releaseFn! usage on line 42 uses a non-null assertion operator, but releaseFn is not guaranteed to be defined if this function is called before the Promise callback executes. While the current logic ensures this won't happen, it would be safer to initialize releaseFn or use a pattern that avoids the assertion:
let releaseFn: (() => void) | undefined
const lockPromise = new Promise<void>((resolve) => {
releaseFn = resolve
})
fileLocks.set(normalized, lockPromise)
return () => {
fileLocks.delete(normalized)
if (releaseFn) releaseFn()
}| if (result.autoshare === true && !result.share) { | ||
| result.share = "auto" | ||
| } | ||
|
|
||
| // Handle migration from autoshare to share field | ||
| if (result.autoshare === true && !result.share) { | ||
| result.share = "auto" | ||
| } |
There was a problem hiding this comment.
Duplicate code: Lines 181-187 are identical to lines 185-187. The second check for autoshare === true is redundant and should be removed.
| if (waited > 5000 && waited < 5100) { | ||
| log.warn("lock acquisition taking longer than expected", { | ||
| filepath: normalized, | ||
| waited, | ||
| }) |
There was a problem hiding this comment.
[nitpick] The magic numbers 5000 and 5100 on lines 19-23 for the warning threshold should be extracted as named constants for better readability:
const LOCK_WARNING_THRESHOLD_MS = 5000
const LOCK_WARNING_WINDOW_MS = 100
if (waited > LOCK_WARNING_THRESHOLD_MS && waited < LOCK_WARNING_THRESHOLD_MS + LOCK_WARNING_WINDOW_MS) {|
|
||
| function rememberConfigUpdate(directory: string, scope: "project" | "global", sections: string[]) { | ||
| LastConfigUpdate.set(directory, { scope, sections, at: Date.now() }) | ||
| // best-effort cleanup of stale entries | ||
| for (const [key, value] of LastConfigUpdate) { | ||
| if (Date.now() - value.at > 60_000) LastConfigUpdate.delete(key) |
There was a problem hiding this comment.
[nitpick] The magic number 60000 (60 seconds) on line 88 should be extracted as a named constant:
const CONFIG_UPDATE_MEMORY_TTL_MS = 60_000
if (Date.now() - value.at > CONFIG_UPDATE_MEMORY_TTL_MS) LastConfigUpdate.delete(key)| function rememberConfigUpdate(directory: string, scope: "project" | "global", sections: string[]) { | |
| LastConfigUpdate.set(directory, { scope, sections, at: Date.now() }) | |
| // best-effort cleanup of stale entries | |
| for (const [key, value] of LastConfigUpdate) { | |
| if (Date.now() - value.at > 60_000) LastConfigUpdate.delete(key) | |
| const CONFIG_UPDATE_MEMORY_TTL_MS = 60_000 | |
| function rememberConfigUpdate(directory: string, scope: "project" | "global", sections: string[]) { | |
| LastConfigUpdate.set(directory, { scope, sections, at: Date.now() }) | |
| // best-effort cleanup of stale entries | |
| for (const [key, value] of LastConfigUpdate) { | |
| if (Date.now() - value.at > CONFIG_UPDATE_MEMORY_TTL_MS) LastConfigUpdate.delete(key) |
| if (match) { | ||
| const scope = (match[1] as string).toLowerCase() as "global" | "project" | ||
| const now = Date.now() | ||
| const isFresh = (ts: number) => now - ts < 10_000 |
There was a problem hiding this comment.
[nitpick] The magic number 10000 (10 seconds) on line 1708 should be extracted as a named constant for better maintainability:
const TOAST_FRESHNESS_WINDOW_MS = 10_000
const isFresh = (ts: number) => now - ts < TOAST_FRESHNESS_WINDOW_MS| const isFresh = (ts: number) => now - ts < 10_000 | |
| const TOAST_FRESHNESS_WINDOW_MS = 10_000 | |
| const isFresh = (ts: number) => now - ts < TOAST_FRESHNESS_WINDOW_MS |
| interface LockOptions { | ||
| timeout?: number | ||
| } | ||
|
|
||
| export async function acquireLock(filepath: string, options?: LockOptions): Promise<() => void> { | ||
| const normalized = path.normalize(filepath) | ||
| const timeout = options?.timeout ?? 30000 |
There was a problem hiding this comment.
[nitpick] The timeout value on line 13 is a magic number (30000ms). Consider extracting it to a named constant for better maintainability:
const DEFAULT_LOCK_TIMEOUT_MS = 30000
const timeout = options?.timeout ?? DEFAULT_LOCK_TIMEOUT_MS| interface LockOptions { | |
| timeout?: number | |
| } | |
| export async function acquireLock(filepath: string, options?: LockOptions): Promise<() => void> { | |
| const normalized = path.normalize(filepath) | |
| const timeout = options?.timeout ?? 30000 | |
| const DEFAULT_LOCK_TIMEOUT_MS = 30000 | |
| interface LockOptions { | |
| timeout?: number | |
| } | |
| export async function acquireLock(filepath: string, options?: LockOptions): Promise<() => void> { | |
| const normalized = path.normalize(filepath) | |
| const timeout = options?.timeout ?? DEFAULT_LOCK_TIMEOUT_MS |
| tasks.push(task) | ||
| } | ||
| entries.delete(key) | ||
| recordsByKey.delete(key) |
There was a problem hiding this comment.
The cleanup operation in line 142 has a bug. It deletes the wrong key - it should delete key but instead deletes entries.delete(key). The correct line should be:
recordsByKey.delete(key)Currently, entries.delete(key) will return a boolean (true/false), and that boolean is then used as the key to delete from recordsByKey, which is incorrect.
| throw new Error(`Lock timeout: could not acquire lock for ${normalized} after ${waited}ms`) | ||
| } | ||
|
|
||
| await fileLocks.get(normalized) |
There was a problem hiding this comment.
The lock acquisition loop uses a busy-wait pattern without any delay between checks, which could cause high CPU usage. Consider adding a small delay (e.g., await new Promise(resolve => setTimeout(resolve, 10))) between iterations to reduce CPU consumption while waiting for the lock.
| await fileLocks.get(normalized) | |
| await fileLocks.get(normalized) | |
| await new Promise(resolve => setTimeout(resolve, 10)) |
| @@ -0,0 +1,178 @@ | |||
| import path from "path" | |||
| import os from "os" | |||
There was a problem hiding this comment.
Unused import os.
| import os from "os" |
| const log = Log.create({ service: "config.write" }) | ||
|
|
There was a problem hiding this comment.
Unused variable log.
| const log = Log.create({ service: "config.write" }) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| validator("json", Config.Info), | ||
| async (c) => { | ||
| const config = c.req.valid("json") | ||
| await Config.update(config) | ||
| return c.json(config) | ||
| const scope = (c.req.query("scope") as "project" | "global" | undefined) ?? "project" | ||
| const directory = Instance.directory | ||
|
|
||
| const result = await Config.update({ | ||
| scope, | ||
| update: config, | ||
| directory, | ||
| }) | ||
|
|
||
| const publishDiff = result.diffForPublish | ||
| const hotReloadEnabled = isConfigHotReloadEnabled() | ||
| const sections = Object.keys(publishDiff).filter((k) => (publishDiff as any)[k] === true) |
There was a problem hiding this comment.
Previously the PATCH /config endpoint disposed the instance after writing so changes took effect immediately. The new handler only publishes invalidation events when OPENCODE_CONFIG_HOT_RELOAD is true; otherwise it just calls Config.update and returns the response. Config.update now skips any State.invalidate calls when hot reload is disabled, so with the flag unset (the documented default) the running process keeps using the old configuration until it is manually restarted. This silently regresses the existing behavior and makes config edits appear successful while they are ignored. Consider invoking the legacy full dispose path when hot reload is disabled.
Useful? React with 👍 / 👎.
Summary
PATCH /config, which is not currently used by the OpenCode TUI and uses the legacy config.json file format, currently force a full restart for every edit. This branch introduces a feature-flagged hot reload pipeline with safe persistence so we only invalidate the subsystems touched by each diff.
Changes
Features
config.updatedevents, and provide diffs for downstream targets so edits can be applied without restarts.State.registerAPI into the server/event bus so a diff only reinitializes the provider/MCP/LSP components that actually changed.Fixes
Refactoring
State.invalidate("agent")during targeted refreshes instead of disposing the entire instance.config.updatedevents, respect project vs global scope, and to rely on the new persistence primitives instead of ad hoc file IO.Breaking Changes
Config.updatenow requires an object input{ scope, update, directory }and returns{ before, after, diff, filepath }; any callers must update to the new signature./configsupports ascopequery parameter and now responds with the merged config plus diff metadata, so API consumers expecting the previous shape must adjust.Testing
bun test packages/opencode/test/config/write.test.ts packages/opencode/test/config/hot-reload.test.tsbun test packages/opencode/test/config/config.test.tsConfiguration
OPENCODE_CONFIG_HOT_RELOAD=trueto enable targeted invalidation; leave unset to retain full dispose behavior.OPENCODE_FULL_DISPOSE_ON_CONFIG_UPDATE=trueto force legacy restarts andOPENCODE_CONFIG_INVALIDATION_LOG_DIFF=truefor verbose diff logging.Migration
Config.updateor PATCH/configto pass the new parameters and to consume the new response structure.