Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
liudongyan13701205717-source
left a comment
liudongyan13701205717-source
left a comment
There was a problem hiding this comment.
Thanks for reviving this — the design reads well and the opt-in default is the right call for a change this broad.
What I verified
I checked out the branch, wired up a worktree with the new packages/core modules resolvable, and ran the new tests.
packages/opencode/test/server/instance-eviction.test.ts— 8 pass / 0 fail. The activity guards (runners, idle statuses, background jobs, detached PTYs) and the sweep lifecycle (warn-on-invalid, no-sweep-when-unset, stop-on-scope-close) all behave as described.packages/opencode/test/project/instance.test.ts— 22 pass / 4 fail. All 4 failures are one cause, detailed in the inline comments.packages/core/test/pty/activity.test.ts— 1 pass / 1 fail, same cause (its symlink-replacement case).
The one blocking finding is that all of those failures are EPERM: operation not permitted, symlink ... — fs.symlink needs SeCreateSymbolicLinkPrivilege on Windows, and .github/workflows/test.yml runs the unit matrix on blacksmith-4vcpu-windows-2025. The inline comments point at the repo's existing process.platform !== "win32" guard, which fixes all four. Nothing else about the change appears affected.
Two notes, neither blocking
Staleness. git merge-base --is-ancestor upstream/dev HEAD fails, so GitHub is right that this needs a rebase. However the overlap with dev's movement is empty — the 25 files touched here are untouched upstream since the PR's merge-base. I cherry-picked the commit onto current dev in a scratch worktree and it applied cleanly, so the rebase itself should be trivial. My guess is that mergeable_state: blocked is about artifacts/-directory bookkeeping rather than real conflicts, but I could not confirm that from outside.
InstanceActivity is process-global. The states map in packages/core/src/instance-activity.ts is module-level, keyed by absolute directory. That is consistent with how the previous InstanceStore cache was scoped, but it does mean the accounting crosses workspaces and accounts within one server process. Worth a sentence in the design notes if that is intended — particularly forget(), which currently only walks its own key.
Happy to follow up if useful.
| }), | ||
| ) | ||
|
|
||
| it.effect("keeps entry activity stable when its pathname is replaced", () => |
There was a problem hiding this comment.
Verified locally on Windows: this test fails with EPERM: operation not permitted, syscall: 'symlink'. On Windows fs.symlink requires SeCreateSymbolicLinkPrivilege, which CI runners do not have.
.github/workflows/test.yml runs the unit matrix on blacksmith-4vcpu-windows-2025 as well as Linux, so this will fail the Windows job.
The repo already has a convention for this (packages/opencode/test/project/worktree.test.ts:19):
const wintest = process.platform !== "win32" ? it.instance : it.instance.skipOnly the tests that actually call symlink need it — the other 3 symlink sites in this file pass on Windows. I ran the full file on this branch: 22 pass / 4 fail, and all 4 failures are this single cause.
There was a problem hiding this comment.
Good catch, thanks. Fixed in 628b20a: the four tests here that create symlinks (374, 408, 440, 492) plus the symlink case in packages/core/test/pty/activity.test.ts now skip on Windows, same as the other symlink tests in the repo (snapshot.test.ts, watcher.test.ts, location-mutation.test.ts).
| }), | ||
| ) | ||
|
|
||
| it.effect("keeps a remembered symlink alias stable for the entry lifetime", () => |
There was a problem hiding this comment.
Same Windows issue as above — this test calls symlink and needs the wintest guard. I measured this one failing with EPERM on blacksmith-equivalent Windows.
Note the other two symlink tests in this file (keeps entry activity stable when its pathname is replaced, keeps a remembered symlink alias stable for the entry lifetime) pass — they only compare pathnames and don't create links. So the guard is needed on exactly these four:
- 374
- 408
- 440
- 492
| }), | ||
| ) | ||
|
|
||
| it.live("keeps waiting alias loads on the frozen key across tombstone cleanup", () => |
There was a problem hiding this comment.
Same EPERM-on-Windows cause. Verified locally on Windows 11: symlink at test/project/instance.test.ts:416 fails and this test reports fail.
Suggest guarding with the existing wintest convention, or skipping just the two tests in this describe block.
| }), | ||
| ) | ||
|
|
||
| it.live("releases an interrupted alias reservation", () => |
There was a problem hiding this comment.
Fourth and last EPERM site. Verified locally on Windows: EPERM: operation not permitted, symlink .../replacement -> .../owner at instance.test.ts:390 in the original (non-rebased) version.
With the wintest guard applied to these four tests the file should go green on both CI legs. On Linux nothing changes.
Separately, I rebased the commit onto current dev in a scratch worktree — cherry-pick applies cleanly with zero conflicts on all 25 files, so mergeable_state: blocked looks like a GitHub hiccup rather than a real conflict.
598144a to
628b20a
Compare
|
Thanks for the review @liudongyan13701205717-source.
|
|
This branch has been upgraded to V2. We're no longer taking PRs for V1. If you think this is still relevant, please port it over and open it for V2. — from 𝕺𝖕𝖊𝖓𝕮𝖔𝖉𝖊 |
Issue for this PR
Closes #33720
Type of change
What does this PR do?
Revives #42631 by @kernel-oops, which the stale-PR bot closed on Sep 14 without a review. It is the same commit, cherry-picked onto current
devwithout conflicts; Marc stays the author. Marc, happy to close this if you'd rather reopen yours.opencode servekeeps one instance per directory forever, each with its own MCP servers. With five local MCP servers and a dashboard that opens a session per worktree, I hit 63 MCP processes (5.7 GB) an hour after boot, and 19 GB of swap after a few days. This adds opt-in idle disposal viaOPENCODE_EXPERIMENTAL_INSTANCE_IDLE_TIMEOUT_MS(minimum one hour, unset means off); requests, session runs, background jobs and PTYs hold the instance open. See #42631 for the design notes.How did you verify your code works?
dev(146 pass, 0 fail).Screenshots / recordings
N/A, server-only change.
Checklist