feat(desktop): schedule runtime auto updates#468
Conversation
Greptile SummaryThis PR introduces the packaged-only background runtime auto-updater: a 30s/30min schedule that checks for newer runtime versions, stages them to disk, and notifies renderers via IPC — with a carefully guarded rollback path that prevents re-stage loops when a bad version is marked.
Confidence Score: 5/5Safe to merge; the auto-update and rollback logic is sound and well-tested against a real tmpdir. The rollback gating, version comparison, pointer self-repair, and IPC broadcast paths are all exercised by the test suite. The single-flight inFlight guard correctly prevents concurrent extracts, and broadcastToAllRenderers handles destroyed windows gracefully. The only open item is a build-time validation gap in stage-cli.mjs where the runtime version is written without asserting valid semver — readBundledVersion() recovers correctly at runtime so no user-facing breakage occurs. packages/desktop/scripts/stage-cli.mjs — missing semver validation on the version read from package.json before it is embedded in the staged artifact.
|
| Filename | Overview |
|---|---|
| packages/desktop/scripts/stage-cli.mjs | Reads root package.json for runtime version and embeds it into staged cli/package.json; version is not validated as semver before writing, though readBundledVersion() in runtime-auto-update.ts gracefully handles the invalid/missing case at runtime |
| packages/desktop/src/main.ts | Wires autoUpdate into orchestrator and app lifecycle; broadcastToAllRenderers is a function declaration (hoisted) so passing it to createRuntimeAutoUpdate before its textual definition is safe; stop() called correctly before orchestrator shutdown |
| packages/desktop/src/preload.ts | Adds onUpdateStaged and onRuntimeRolledBack subscriptions using a well-designed subscribe helper that wraps listeners to prevent cross-renderer listener removal |
| packages/desktop/src/runtime-auto-update.ts | Core auto-update module; rollback gating on markBadVersion success is correct and well-tested; loadOverride() self-repairs stale/corrupt pointers; inFlight single-flight guard prevents concurrent extracts |
| packages/desktop/test/runtime-auto-update.test.ts | Comprehensive test coverage using a real tmpdir; exercises rollback gating, pointer self-repair, stale pointer cleanup, corrupt JSON, and the inFlight guard |
Sequence Diagram
sequenceDiagram
participant Main as main.ts
participant AU as RuntimeAutoUpdate
participant Store as runtime-store
participant Update as runtime-update
participant Orch as RuntimeOrchestrator
participant Renderer as Renderer
Main->>AU: createRuntimeAutoUpdate(deps)
AU->>Store: cleanupPartials(userData)
Main->>Orch: new RuntimeOrchestrator(resolveCliEntryOverride, onCliEntryOverrideFailed)
Main->>AU: app ready - scheduleChecks()
AU-->>AU: setTimeout(runCheck, 30s)
AU-->>AU: setInterval(runCheck, 30min)
alt update staged
AU->>Store: loadOverride() self-repair stale pointer
AU->>Store: readPointer() currentVersion
AU->>Update: checkAndStageLatestRuntime(currentVersion)
Update-->>AU: staged, version
AU->>Renderer: broadcast runtime:update-staged
end
alt staged runtime fails startup
Orch->>AU: onCliEntryOverrideFailed(reason, cliEntry)
AU->>Store: markBadVersion(failedVersion)
AU->>Store: removeVersionDir(failedVersion)
AU->>Store: clearPointer() if marked AND pointer unchanged
AU->>Renderer: broadcast runtime:rolled-back
end
Main->>AU: before-quit stop()
AU-->>AU: clearTimeout + clearInterval
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/scripts/stage-cli.mjs:19-21
The `version` field is extracted from `package.json` without validating it is a valid semver string before it is written into `cli/package.json`. If the root `package.json` ever has a pre-release tag or placeholder value that is not strict semver, `JSON.stringify` will write it as-is (e.g. `"version": "1.0.0-beta.1+sha.abc"`). `readBundledVersion()` will then reject it as invalid semver and silently fall back to `shellVersion`, which is the correct recovery — but the logged message `(runtime undefined)` or a non-canonical version string in the staged artifact would be confusing. Stripping the value to its semver-clean form (or at least asserting it is valid) at build time is safer.
```suggestion
const rawRuntimeVersion = JSON.parse(
readFileSync(resolve(repoRoot, "package.json"), "utf8"),
).version;
if (typeof rawRuntimeVersion !== "string" || !rawRuntimeVersion.match(/^\d+\.\d+\.\d+/)) {
fail(`package.json version is not a valid semver string: ${JSON.stringify(rawRuntimeVersion)}`);
}
const runtimeVersion = rawRuntimeVersion;
```
Reviews (2): Last reviewed commit: "feat(desktop): schedule runtime auto upd..." | Re-trigger Greptile
| scheduleChecks() { | ||
| firstTimer = setTimeout(() => void runCheck(), FIRST_CHECK_DELAY_MS); | ||
| firstTimer.unref(); | ||
| interval = setInterval(() => void runCheck(), CHECK_INTERVAL_MS); | ||
| interval.unref(); | ||
| }, |
There was a problem hiding this comment.
The
scheduleChecks() method overwrites firstTimer and interval without clearing the previous references. If called a second time (e.g., during integration-test setup or an unexpected double app.ready sequence), the first set of timers becomes unreachable and cannot be stopped by stop(), leaving two concurrent polling loops running. A simple guard prevents this.
| scheduleChecks() { | |
| firstTimer = setTimeout(() => void runCheck(), FIRST_CHECK_DELAY_MS); | |
| firstTimer.unref(); | |
| interval = setInterval(() => void runCheck(), CHECK_INTERVAL_MS); | |
| interval.unref(); | |
| }, | |
| scheduleChecks() { | |
| if (firstTimer !== null || interval !== null) return; | |
| firstTimer = setTimeout(() => void runCheck(), FIRST_CHECK_DELAY_MS); | |
| firstTimer.unref(); | |
| interval = setInterval(() => void runCheck(), CHECK_INTERVAL_MS); | |
| interval.unref(); | |
| }, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/desktop/src/runtime-auto-update.ts
Line: 212-217
Comment:
The `scheduleChecks()` method overwrites `firstTimer` and `interval` without clearing the previous references. If called a second time (e.g., during integration-test setup or an unexpected double `app.ready` sequence), the first set of timers becomes unreachable and cannot be stopped by `stop()`, leaving two concurrent polling loops running. A simple guard prevents this.
```suggestion
scheduleChecks() {
if (firstTimer !== null || interval !== null) return;
firstTimer = setTimeout(() => void runCheck(), FIRST_CHECK_DELAY_MS);
firstTimer.unref();
interval = setInterval(() => void runCheck(), CHECK_INTERVAL_MS);
interval.unref();
},
```
How can I resolve this? If you propose a fix, please make it concise.| deps.broadcast("runtime:rolled-back", failedVersion ?? null); | ||
| }; | ||
|
|
||
| const runCheck = async (): Promise<void> => { | ||
| // Single-flight: a slow extract racing the periodic interval | ||
| // would otherwise re-enter pacote.extract on the same partial. | ||
| if (inFlight) return; | ||
| inFlight = true; | ||
| try { | ||
| // Side effect: drops a stale-or-broken pointer so the | ||
| // version gate below sees an accurate `max(pointer, bundled)`. | ||
| loadOverride(); | ||
| const ptr = readPointer(deps.userData); | ||
| const currentVersion = | ||
| ptr && semver.gt(ptr.version, bundledVersion) | ||
| ? ptr.version | ||
| : bundledVersion; | ||
| const outcome = await checkAndStageLatestRuntime({ | ||
| userData: deps.userData, | ||
| currentVersion, | ||
| nativeDepsSource, | ||
| }); | ||
| if (outcome.kind === "staged") { | ||
| console.log( | ||
| `[desktop] Staged kanban@${outcome.version} — restart to apply.`, | ||
| ); | ||
| deps.broadcast("runtime:update-staged", outcome.version); |
There was a problem hiding this comment.
IPC channel names duplicated across trust boundary
"runtime:update-staged" and "runtime:rolled-back" are hardcoded in runtime-auto-update.ts (the sender) and again in preload.ts (the subscriber). A future rename or typo correction that updates only one side silently breaks the notification path with no TypeScript or lint error to catch it. Centralising these into a shared constants module (e.g. ipc-channels.ts) would make any mismatch a compile error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/desktop/src/runtime-auto-update.ts
Line: 167-193
Comment:
**IPC channel names duplicated across trust boundary**
`"runtime:update-staged"` and `"runtime:rolled-back"` are hardcoded in `runtime-auto-update.ts` (the sender) and again in `preload.ts` (the subscriber). A future rename or typo correction that updates only one side silently breaks the notification path with no TypeScript or lint error to catch it. Centralising these into a shared constants module (e.g. `ipc-channels.ts`) would make any mismatch a compile error.
How can I resolve this? If you propose a fix, please make it concise.
Split from #440.
This final slice enables the packaged-only background updater: