Repository navigation
feat(desktop): capture app and server heap snapshots into one folder - #58259
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
There was a problem hiding this comment.
The design is sound, but I could only partly verify it. Against a real opencode serve from this branch, with auth on, captureServer fetched the server's heap dump and moved it into the capture folder. The new tests pass and gui-extensions type-checks cleanly. I couldn't run the palette command, the window and main snapshots or the manifest without the Electron app; a screenshot of the toast and the resulting folder in the PR already show that part.
Public API is unchanged: this uses the existing /api/debug/heap-dump endpoint and adds an internal debug.heap Ipc between the desktop's main process and its windows.
One small edge case: if the Downloads folder doesn't exist, the whole capture fails with a generic error (see inline).
| ) | ||
|
|
||
| // Snapshots hold prompts, file contents and credentials. | ||
| await mkdir(directory, { mode: 0o700 }) |
There was a problem hiding this comment.
app.getPath("downloads") can point to a folder that doesn't exist yet (e.g. minimal Linux setups with no ~/Downloads). Without recursive: true, this mkdir fails with ENOENT and the user only sees "Could not capture heap snapshots". mkdir(directory, { recursive: true, mode: 0o700 }) avoids that, and the folder name is already unique.
There was a problem hiding this comment.
The missing-Downloads point is fixed: the folder is now created with recursive: true. The other new commits look sound and I have nothing new to flag: the command always captures the desktop's own server, the server's snapshot is only renamed into the folder (never copied), and windows are captured one at a time. The updated tests pass and gui-extensions type-checks cleanly. I still couldn't run the Electron side. Public API is unchanged.
7023be4 to
51d97e2
Compare
There was a problem hiding this comment.
The missing-Downloads point is fixed (recursive: true). The new commits work against a real opencode serve from this branch: ?directory= with an absolute path writes there and creates missing folders, a relative path gets a 400, and opencode debug heap-dump --directory works with absolute and relative paths. The server, CLI and desktop heap tests pass, and tsc -b is clean in cli, server and gui-extensions. I couldn't run the Electron side.
Public API: two additions, both backwards compatible (requests without directory behave as before, and older servers ignore it):
POST /api/debug/heap-dumptakes an optionaldirectoryquery parameter (absolute, server-local, created if missing). A relative path returns 400InvalidRequestError, and its 500 error message now includes the filesystem's reason.opencode debug heap-dump --directory, and an optional{ directory }fordebug.heapDump()in the clients.
One new bug: an absolute --directory with a trailing slash wrongly prints 'The server ignored --directory' (see inline).
| : cause, | ||
| }) | ||
| // Servers released before `directory` existed ignore it and write into their log directory. | ||
| if (directory !== undefined && path.dirname(snapshot.path) !== path.normalize(directory)) |
There was a problem hiding this comment.
path.normalize keeps a trailing separator, so --directory /tmp/snaps/ compares /tmp/snaps with /tmp/snaps/ and prints "The server ignored --directory…" even though the server wrote there. I reproduced this against a server from this branch. Resolving absolute paths too (path.resolve(value), which drops the trailing slash) or comparing with path.resolve on both sides avoids it. Only a POSIX path sent to a remote server from Windows needs to skip resolve, and path.posix.normalize won't strip the slash there either.
There was a problem hiding this comment.
Fixed in a8db41f. The CLI now checks the result the way the server builds it: it joins the directory and file name with the path style of the server's machine (win32 for a drive or UNC path, POSIX otherwise). That accepts a trailing separator and keeps a POSIX path for a remote server intact on Windows. The desktop side uses the same join. The CLI test now passes --directory
/\ and fails without the fix.There was a problem hiding this comment.
The trailing-slash point is fixed: against a real opencode serve from this branch, --directory /tmp/…/d/ writes there without the wrong warning, and plain absolute and relative directories still work. The new comparison in the CLI and desktop is sound, and I have nothing new to flag. The CLI and desktop heap tests pass, and tsc -b is clean in cli and gui-extensions. I couldn't run the Electron side. Public API is unchanged since my last review.
A desktop command palette command, Capture heap snapshots, writes snapshots of the desktop app and its local server into one folder. To get the server's snapshot there without moving a large file,
opencode debug heap-dumpand its endpoint now take an optional directory.CLI:
--directorySIGUSR1.--serverstays intact on Windows.POST /api/debug/heap-dump?directory=…. A request without it, likeopencode api post /api/debug/heap-dumpor the previous CLI, works as before. A relativedirectoryis rejected withInvalidRequestError(400).Failed to write heap snapshot: EEXIST: file already exists, mkdir '/home/me/snapshots', instead of only pointing at the server log.The server ignored --directory. Update and restart it to choose where snapshots go.Desktop: one folder for the whole app
sequenceDiagram participant W as window (debug renderer) participant M as main (debug/main.ts) participant S as desktop server (sidecar) W->>M: HeapSnapshots.write() M->>M: mkdir folder (0700) par M->>S: POST /api/debug/heap-dump?directory=folder S-->>M: { path, pid } once the file is complete and M->>W: webContents.takeHeapSnapshot(file), one window at a time end M->>M: v8.writeHeapSnapshot(main), last M->>M: manifest.json M-->>W: { directory, complete }sidecar). Remote servers (SSH, WSL, paired machines) get no request; useopencode debug heap-dumpthere.directorykeeps the file in its log directory, and the manifest records that path.file,pathorerrorentry in the manifest, and the toast says when something failed.app.getAppMetrics()(metricsMemoryUnit: "KiB"), and main'sprocess.memoryUsage(). A process that grows while its JS heap does not leaks native memory.0700and the manifest0600. Downloads is created when missing.browser.heaptool covers them) and Web Workers.debugGUI extension. NewHeapSnapshotsIpc and a main entry. The command registers throughcreateKeyed(ctx.uses.heap, …), so it shows only while main answers, never on the web. One capture runs at a time.New copy is English only; translations land separately. The OpenAPI documents include only the heap-dump change; unrelated drift from
devis left out. Docs:troubleshooting.mdxandcli/commands.mdxcover--directoryand the desktop command, and theopencode-devskill explains both.Added by the opencode bot (AI-generated).
Related: #56058, #36677
Public API: adds an optional
directoryquery parameter to POST /api/debug/heap-dump,--directorytoopencode debug heap-dump, and{ directory }to the clients'debug.heapDump()(backwards compatible)