Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
This fixes the idle re-subscribe loop. I ran the #54114 reproduction from source with the data directory inside the config directory. On the base, <project>/.agents/skills was torn down and re-subscribed 25 times in 30 s of idling. With this change it subscribes once. Idle CPU drops to the same level as a control run with separate data and config directories. The new test fails on the base source and passes here. test/config (228 pass) and the packages/core typecheck are clean.
The public API is unchanged.
The new key-diffing mirrors Config.reconcileWatches, and the keys match the ones watch() uses, so I have no blocking concerns. Note that config.changes() still runs a full rescan and skill reload on every event under the config directory, with no debounce. This change removes the watcher teardown that kept the loop going, not the extra reloads, so #53393 is only partly addressed. The open #53764 (debounce, and skip unchanged compatibility roots) covers that part and complements this change.
Related: #53393
f8186b4 to
a0d19f7
Compare
|
Closing this to stay within GitHub's 3-open-pull-request limit for contributors without write access, which currently blocks every new pull request here. The branch remains on my fork, so I can reopen it on request. |
Issue for this PR
Closes #54114
Type of change
What does this PR do?
opencode.config.compatibilityrebuilt its watches every time it saw a change.refresh()opened withFiberMap.clear(watches)and then re-subscribed each compatibility directory, which defeats theonlyIfMissing: trueon theFiberMap.runinsidewatch(): an event reaching the 100 ms debounce tore down every watch and attached them again, and attaching a subscription publishes again, so the debounce was handed a fresh event to consume. On a project with./.agents/skillsthe service settled into that cycle —watcher subscribefor that path grew without bound while no file changed, pinning the service at ~37% CPU and filling the log.The change reconciles instead of clearing: build the plan of directories that should be watched,
FiberMap.removethe keys that dropped out of it, and subscribe only the keys that are not already present. A directory whose resolved path has not changed keeps its watch, so there is no re-subscription to publish an event and the cycle has nothing left to sustain it.I took this shape from the two places in the codebase that already work this way —
Config.reconcileWatchesinpackages/core/src/config.ts, andConfigSkillPlugin.watchDirectory.refresh()keeps itslock.withPermit, so the plan is not raced.One honest limit: I could not reproduce the reporter's trigger here — it needs Linux inotify and, per the triage thread, the data directory coinciding with the config directory. What I verified is the mechanism the report named, watches being cleared and rebuilt per event, and that it is gone.
How did you verify your code works?
Added
keeps a compatibility watch instead of rebuilding it on every eventtopackages/core/test/config/skill.test.ts. It starts the plugin under the existingWatcher.Testlayer, which records everysubscribecall, asserts the compatibility directory is watched once, emits a single update beneath that directory, and asserts the directory is still subscribed exactly once. That last assertion fails on the unmodified source — the re-subscription this report is about is precisely what it catches — and passes with the change.bun test test/config/skill.test.ts→ 11 pass / 0 failbun test test/config→ 209 pass / 19 fail, and the same 19 fail on the unmodified tree ate31136bdbd(they are config-discovery tests that pick up a real document in this environment), so this adds no failuretsgo -b tsconfig.json tsconfig.tests.jsoninpackages/core→ exit 0oxlinton both changed files → 0 warnings / 0 errorsScreenshots / recordings
If this is a UI change, please include a screenshot or recording.
Checklist
Added by the opencode bot (AI-generated).
Related: #53393