Repository navigation
feat(cli): Complete Local session IDs and labels - #1781
Conversation
The Local session, envelope, and event commands took their session as free text. Sessions live only in the daemon's memory and `sentry __complete` makes no network requests, so the daemon now mirrors each session's ID, label, state, and expiry to an owner-only `local-sessions.json` next to its discovery state, rewritten when a session is created, closed, or expires and removed on shutdown. Completion reads it, ignores it once the daemon's PID has exited, and suggests session IDs (described by label and state) and the labels only one session has. Co-Authored-By: BYK <126780+BYK@users.noreply.github.com>
Co-Authored-By: BYK <126780+BYK@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
| let snapshotWrites = Promise.resolve(); | ||
| const snapshotSessions = (sessions: readonly LocalSession[]) => { | ||
| snapshotWrites = snapshotWrites | ||
| .then(() => writeLocalSessionSnapshot(directory, process.pid, sessions)) |
There was a problem hiding this comment.
Why are we not using new Promise(..) pattern like normal people? 😀 (and don't tell me it's because you're a bot!)
There was a problem hiding this comment.
Fair 😄 There's no promise to construct here: the chain only existed to keep async writes in order. In c898089 the snapshot is written synchronously instead (writeFileSync + renameSync, still atomic and 0600). The file is tiny and the daemon only writes it when a session is created, closed, or expires, so each write finishes before the next one starts and there's nothing left to serialize or await on shutdown.
| "id" | "label" | "state" | "expiresAt" | ||
| >; | ||
|
|
||
| function snapshotPath(directory: string): string { |
There was a problem hiding this comment.
Shouldn't we just use Sentry CLI data directory for this?
There was a problem hiding this comment.
It already lives there: both callers passed getConfigDir(), the directory that holds cli.db and local-control-plane.json. In c898089 the module resolves getConfigDir() itself, so the directory parameter is gone and the location can't drift. If you meant storing the sessions in cli.db (like completion_cache) rather than a file, I kept it a file because the daemon would otherwise need SQLite writes alongside every other CLI process. Happy to switch if you prefer that.
| [...sessions.values()] | ||
| .map(toPublicSession) |
There was a problem hiding this comment.
I think this is one of those rare cases where Array.from() might be better as it already accepts a map function?
There was a problem hiding this comment.
Done in c898089: Array.from(sessions.values(), toPublicSession).sort(…).
| } | ||
| return [...completions] | ||
| .filter(([value]) => value.startsWith(partial)) | ||
| .map(([value, description]) => ({ value, description })); |
There was a problem hiding this comment.
Like my earlier comment, I wonder if doing Array.from().filter() would work better? Though in this case it swaps the .filter and .map order.
There was a problem hiding this comment.
Done in c898089: Array.from(completions, ([value, description]) => ({ value, description })).filter(({ value }) => value.startsWith(partial)). Mapping first only builds a few extra small objects for this list size.
Address review: the snapshot resolves the CLI config directory itself instead of taking it as a parameter, and writes synchronously, so no promise chain is needed to keep writes in order. Use `Array.from` with a map function where it replaces a spread and `.map`.
| }; | ||
| await mkdir(directory, { recursive: true, mode: 0o700 }); | ||
| const path = snapshotPath(directory); | ||
| mkdirSync(getConfigDir(), { recursive: true, mode: 0o700 }); |
There was a problem hiding this comment.
Maybe we should also have an ensureConfigDir() which creates the dir and returns its path now?
There was a problem hiding this comment.
Good call: db/index.ts already had a private ensureConfigDir(). In ca00c9a it is exported and returns the directory, and it replaces the hand-rolled getConfigDir() + mkdir(…, 0o700) in the session snapshot writer, ensureLocalDaemon, and runLocalDaemonForeground.
Address review: export `ensureConfigDir()` from `db/index.ts`, returning the directory it creates, and use it where the Local daemon and session snapshot created the config directory by hand.
Follow-up to #1759.
local session view|close|reset|retention,local envelope list|view, andlocal event list|viewnow complete the session: IDs of the running daemon's sessions, described by label and state (web (open)), then labels that only one session has (the commands accept those too).Where the sessions come from. Sessions exist only in the daemon's memory, and
sentry __completemakes no network calls. So the daemon now keeps a smalllocal-sessions.jsonnext to its discovery state. It is written atomically with owner-only permissions, the same way aslocal-control-plane.json. The daemon rewrites it when a session is created, closed, or expires, and removes it on shutdown. It holds onlyid,label,state, andexpiresAt. The serializer picks those fields explicitly, so capabilities, cwd, and telemetry never reach the file. The completer:A stale entry can only suggest a session that the command then reports as not found.
I chose this over calling the daemon's
/sessionsendpoint from the completer. That would put a loopback request on every TAB press, add a timeout to tune, and make the completer read the control token. The snapshot keeps the fast path file-only. The cost is one small file write per session lifecycle change. Activity updates don't trigger a write, because every ingested envelope records activity.What's not covered. The second word of
local envelope view <session> <envelope>/local event view <session> <event>still completes nothing. Envelope and event IDs exist only in the daemon's retention buffers. Mirroring them would mean a disk write on every ingested envelope, plus decoding for events, on the daemon's hot path. Labels with whitespace are also left out of the suggestions, because they don't stay one shell word.Open question: should the snapshot also carry
cwd, so the description can tell apart same-named sessions from different directories? I left it out to keep the file minimal.via @BYK.
--
View Session in Sentry