feat(desktop): add runtime store#464
Conversation
Greptile SummaryThis PR introduces the on-disk
Confidence Score: 5/5Safe to merge; the new runtime-store module is well-structured with thorough validation and tests, and no new blocking defects were found. All changed code is additive (new module + tests), the canonical-path enforcement in readPointer/writePointer is solid, and test coverage is broad. The one new observation — isBadVersion silently returning false for invalid input — is non-blocking since callers in this slice always validate versions before calling it. packages/desktop/src/runtime-store.ts — atomicWrite, cleanupPartials, and markBadVersion have edge-case gaps flagged in open review threads.
|
| Filename | Overview |
|---|---|
| packages/desktop/src/runtime-store.ts | New module implementing the on-disk runtime pointer, bad-version registry, and partial-cleanup helpers; well-structured with strict path validation, but atomicWrite has an orphaned-temp risk and cleanupPartials has a TOCTOU gap flagged in review threads. |
| packages/desktop/test/runtime-store.test.ts | Comprehensive unit tests covering pointer round-trips, canonical-path enforcement, bad-version CRUD, and partial/version cleanup; good coverage of path-traversal edge cases. |
| packages/desktop/package.json | Added semver ^7.6.3 as a runtime dependency and @types/semver ^7.5.8 as a devDependency; placement is correct. |
| packages/desktop/package-lock.json | Lockfile updated to hoist semver 7.7.4 to top-level (satisfies ^7.6.3) and scope the former top-level semver 6.3.1 under @electron/get; deduplication is correct. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Boot / Updater] -->|readPointer| B{current.json\nexists & valid?}
B -- No --> C[Use bundled runtime]
B -- Yes --> D{cliEntry file\nexists on disk?}
D -- No --> C
D -- Yes --> E{isBadVersion?}
E -- Yes --> C
E -- No --> F[Spawn staged runtime]
F -- crash --> G[markBadVersion\n+ clearPointer]
G --> C
H[Updater / Stager] -->|writePointer| I[atomicWrite\ntmp to rename\ncurrent.json]
H -->|stage new version| J[Extract to\nversions/v.partial/]
J --> K[rename to\nversions/v/]
K --> I
L[Startup] -->|cleanupPartials| M[Remove *.partial dirs\nbest-effort]
L -->|removeVersionDir| N[rm versions/v/]
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/desktop/src/runtime-store.ts:205-207
The `isBadVersion` function silently returns `false` when given a non-semver `version` string rather than propagating an error. This means a caller that accidentally passes an invalid version string (e.g., a build tag like `"nightly-20250507"`) would receive `false` — indistinguishable from "this version is known good" — and could proceed to stage or launch a version that was never actually checked. Throwing (or at minimum returning a distinct sentinel) would surface the mistake at the call site rather than silently allowing it.
```suggestion
export function isBadVersion(userData: string, version: string): boolean {
if (!isSemver(version)) {
throw new Error(`runtime-store: invalid semver: ${version}`);
}
return readBadVersions(userData).includes(version);
}
```
Reviews (2): Last reviewed commit: "feat(desktop): add runtime store" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function cleanupPartials(userData: string): void { | ||
| const versions = path.join(root(userData), "versions"); | ||
| if (!existsSync(versions)) return; | ||
| for (const e of readdirSync(versions, { withFileTypes: true })) { | ||
| if (e.isDirectory() && e.name.endsWith(".partial")) { | ||
| rmSync(path.join(versions, e.name), { recursive: true, force: true }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
cleanupPartials can throw despite "best-effort" label
The existsSync guard and the readdirSync call are not atomic. If the versions directory is removed by an external agent (system cleanup, antivirus, parallel test run) between the two calls, readdirSync throws an uncaught ENOENT that propagates to the caller — exactly the failure mode that "Best-effort" is supposed to suppress. A try-catch around the loop body (or the whole function) would match the documented intent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/desktop/src/runtime-store.ts
Line: 172-180
Comment:
**`cleanupPartials` can throw despite "best-effort" label**
The `existsSync` guard and the `readdirSync` call are not atomic. If the `versions` directory is removed by an external agent (system cleanup, antivirus, parallel test run) between the two calls, `readdirSync` throws an uncaught `ENOENT` that propagates to the caller — exactly the failure mode that "Best-effort" is supposed to suppress. A try-catch around the loop body (or the whole function) would match the documented intent.
How can I resolve this? If you propose a fix, please make it concise.| function atomicWrite(target: string, body: string): void { | ||
| mkdirSync(path.dirname(target), { recursive: true }); | ||
| const tmp = `${target}.${process.pid}.${Date.now()}.tmp`; | ||
| writeFileSync(tmp, body); | ||
| renameSync(tmp, target); | ||
| } |
There was a problem hiding this comment.
.tmp file if renameSync throws
writeFileSync succeeds, writing a full temp file to disk, and then if renameSync fails (e.g. Windows antivirus lock, EXDEV on a bind-mount), the temp file is never cleaned up. Accumulated .tmp files live next to current.json and bad-versions.json indefinitely. A try/catch on renameSync that deletes the temp file before re-throwing would keep the store tidy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/desktop/src/runtime-store.ts
Line: 86-91
Comment:
**Orphaned `.tmp` file if `renameSync` throws**
`writeFileSync` succeeds, writing a full temp file to disk, and then if `renameSync` fails (e.g. Windows antivirus lock, EXDEV on a bind-mount), the temp file is never cleaned up. Accumulated `.tmp` files live next to `current.json` and `bad-versions.json` indefinitely. A `try/catch` on `renameSync` that deletes the temp file before re-throwing would keep the store tidy.
How can I resolve this? If you propose a fix, please make it concise.| export function markBadVersion(userData: string, version: string): void { | ||
| if (!isSemver(version)) { | ||
| throw new Error(`runtime-store: invalid semver: ${version}`); | ||
| } | ||
| const set = new Set(readBadVersions(userData)); | ||
| set.add(version); | ||
| atomicWrite( | ||
| badVersionsPath(userData), | ||
| `${JSON.stringify([...set].sort(semver.compare))}\n`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
markBadVersion is not application-level atomic
atomicWrite makes the final rename atomic at the filesystem level, but the read-then-write cycle is not. If two callers mark different versions bad concurrently (e.g. a crash-reporter and a background worker both detect a failing version), the second writer's Set is seeded only from its own read — it never sees the first writer's addition — so renameSync silently overwrites and loses the first entry. The existing comment for bad-version pruning acknowledges this area is informal, but the lost-update window is worth calling out explicitly since the comment doesn't mention it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/desktop/src/runtime-store.ts
Line: 209-219
Comment:
**Read-modify-write in `markBadVersion` is not application-level atomic**
`atomicWrite` makes the final rename atomic at the filesystem level, but the read-then-write cycle is not. If two callers mark different versions bad concurrently (e.g. a crash-reporter and a background worker both detect a failing version), the second writer's `Set` is seeded only from its own read — it never sees the first writer's addition — so `renameSync` silently overwrites and loses the first entry. The existing comment for bad-version pruning acknowledges this area is informal, but the lost-update window is worth calling out explicitly since the comment doesn't mention it.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Split from #440.
This PR adds the packaged desktop runtime store primitives only:
Kept as the first slice so follow-up updater/orchestrator PRs can depend on these helpers.