Repository navigation
Fix startup update reinstall loop with a durable attempt marker - #1378
Merged
Merged
Conversation
…arker (#1367) Telemetry shows single devices re-running the same startup install up to 246 times for one version. The loop-breaker marker (lastStartupUpdateAttemptVersion) is the last settings write before the app quits into the installer, so settings.json.bak is always one write behind it - and readFileSafe restored .bak over the primary after ANY read error, including transient AV/indexer locks. On interference-prone machines the marker kept getting erased while the pending-update marker survived, so the same version reinstalled on every boot. safe-file: retry transient EPERM/EACCES/EBUSY on reads and sync renames; only restore .bak when the primary is genuinely missing or empty, serving .bak without restoring when the primary is merely locked; expose a bakFallbacks diagnostic counter. startup-attempt-marker (new): a tiny sidecar file in configDir() mirrors the attempt marker with no .bak machinery to roll it back, a read-back verification after writing, and a tri-state read so an unreadable (locked) marker is never mistaken for an absent one. updater: the loop-breaker consults both marker homes; an unavailable sidecar fails the install decision closed (marker_unavailable); the sidecar must be verifiably durable before any marker is written or the install proceeds (marker_not_durable skips install without blocking future retries); stale sidecars are cleared once the target version is running; startup_install telemetry carries bakFallbacks to correlate loops with filesystem interference. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019fd4f6-8904-746f-a85d-9280a17b564b
📝 WalkthroughWalkthroughThe change adds retry-aware safe-file operations, tri-state read results, durable startup-attempt markers, guarded settings and installation writes, and startup-install loop prevention with diagnostics. ChangesStartup update reliability
Sequence Diagram(s)sequenceDiagram
participant Updater
participant StartupAttemptMarker
participant SafeFile
participant Telemetry
Updater->>StartupAttemptMarker: readStartupAttemptMarker()
StartupAttemptMarker->>SafeFile: read marker with retry
SafeFile-->>StartupAttemptMarker: marker content or read state
StartupAttemptMarker-->>Updater: present, absent, or unavailable
Updater->>StartupAttemptMarker: recordStartupAttempt(version)
StartupAttemptMarker->>SafeFile: write and read back marker
SafeFile-->>StartupAttemptMarker: persistence result
Updater->>Telemetry: report marker and fallback diagnostics
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Comments describe current behavior concisely instead of narrating the pre-fix history; restore the bakPath locals in readFileSafe and readFileSafeAsync instead of repeating the string concatenation. No behavior change. Amp-Thread-ID: https://ampcode.com/threads/T-019fd4f6-8904-746f-a85d-9280a17b564b Co-authored-by: Amp <amp@ampcode.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/lib/safe-file.test.ts`:
- Around line 98-150: Extend src/main/lib/safe-file.test.ts:98-150 by capturing
getSafeFileDiagnostics().bakFallbacks before a .bak fallback read and asserting
it increases by one, accounting for the process-wide monotonic counter. In
src/main/lib/updater.test.ts:789-837, add bakFallbacks: expect.any(Number) to at
least one startup_install_skipped assertion and one startup_install assertion;
update only these tests and preserve their existing assertions.
In `@src/main/lib/safe-file.ts`:
- Around line 129-145: Update readFileSafe and its load() caller to distinguish
an unreadable file from a genuinely missing file: when the primary exists but
readFileWithRetrySync cannot read it and no backup is available, return or
propagate an unreadable result instead of null. Ensure load() fails closed for
unreadable settings so save() cannot treat the file as empty and overwrite it,
while preserving null/empty handling for truly absent files.
In `@src/main/lib/startup-attempt-marker.test.ts`:
- Around line 93-100: Add a companion test alongside the existing
persistence-failure test that allows recordStartupAttempt to write the marker
successfully but forces its read-back to return unavailable, then assert it
returns false. Exercise the verification/read-back branch in
recordStartupAttempt rather than the writeFileSafe failure path, preserving the
fail-closed behavior.
In `@src/main/lib/startup-attempt-marker.ts`:
- Around line 69-78: Update recordStartupAttempt and its marker-write path to
use durable storage semantics rather than treating readStartupAttemptMarker as
proof of persistence. Ensure the marker file and replacement directory are
flushed before returning success, while keeping the existing fast write behavior
for unrelated settings paths through an explicit opt-in or marker-specific
durable option.
In `@src/main/lib/updater.ts`:
- Around line 894-899: Update evaluateStartupInstall and its callers to accept
and reuse the existing StartupAttemptMarkerRead result instead of reading the
startup marker again; at the boot call site, perform a second read only if
clearStartupAttemptMarker actually removed the marker. Import
StartupAttemptMarkerRead with the existing marker types and update the
hasPendingStartupUpdate() caller consistently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 01e38da6-4c10-4a3a-88ee-4428c66f64cd
📒 Files selected for processing (6)
src/main/lib/safe-file.test.tssrc/main/lib/safe-file.tssrc/main/lib/startup-attempt-marker.test.tssrc/main/lib/startup-attempt-marker.tssrc/main/lib/updater.test.tssrc/main/lib/updater.ts
…marker, single sidecar read - readFileSafe/readFileSafeAsync now return tri-state outcomes so callers can tell an unreadable (locked) file apart from a missing one; settings.set() and installations mutators fail closed instead of overwriting an intact file with reconstructed defaults - the startup attempt marker write is durable (fsync before rename, plus best-effort parent dir fsync) so a mid-update power cut cannot erase it - writeFileSafe/writeFileSafeAsync take a SafeWriteOptions object - applyPendingUpdateOnStartup reads the sidecar once and passes it into evaluateStartupInstall, halving worst-case blocking on locked markers - pin the bakFallbacks diagnostic end to end and the marker read-back fail-closed branch in tests Amp-Thread-ID: https://ampcode.com/threads/T-019fd4f6-8904-746f-a85d-9280a17b564b Co-authored-by: Amp <amp@ampcode.com>
MaanilVerma
approved these changes
Aug 6, 2026
…op-marker-durability
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/installations.ts (1)
91-115: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winLet a newer explicit
useSharedModelswin over the legacy forcedtrue.Step 2 states the rule plainly: on a mixed-schema record the per-folder flags are newer, so they win. Step 1 does not follow the same rule.
...restis spread first, thenuseSharedModels: trueoverwrites any explicit value the record already carried. A mixed record that holds bothuseSharedPathsanduseSharedModels: falseloses the user's choice, and the next write persists the flipped value to disk.Mixed records are rare, but the migration is sticky — one downgrade dance and shared models waltz back on uninvited.
🐛 Proposed fix to honor the newer flag
if ('useSharedPaths' in rec) { const legacy = rec.useSharedPaths as boolean | undefined const { useSharedPaths: _drop, ...rest } = rec rec = { ...rest, - useSharedModels: true, + // A mixed-schema record may already carry the newer flag; it wins. + useSharedModels: typeof rest.useSharedModels === 'boolean' ? rest.useSharedModels : true, useSharedInputOutput: typeof legacy === 'boolean' ? legacy : true } as InstallationRecord }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/installations.ts` around lines 91 - 115, Update migrateRecord so the legacy useSharedPaths migration does not overwrite an existing explicit useSharedModels value. Preserve a boolean useSharedModels from the original record when present, and only default it to true when absent; keep the existing useSharedInputOutput migration behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/installations.ts`:
- Around line 179-205: Update loadOutcome() so JSON.parse failures return
unreadable: true rather than an empty readable result, allowing load() to retain
its read-only fallback while loadForWrite() throws before saving. Update
loadForWrite()’s documentation and error message to cover both unreadable files
and corrupt JSON, and leave settings.ts behavior unchanged unless the existing
design confirms it should also fail closed.
- Line 241: Update register() startup calls to seedDefaults(), both
ensureExists() invocations, and enforceCloudName() so each promise is awaited or
has attached error handling; ensure loadForWrite() rejections are handled
without unhandled promise rejections, while preserving the existing _queue
recovery behavior.
In `@src/main/lib/safe-file.test.ts`:
- Around line 173-186: Make fs spy cleanup deterministic in
src/main/lib/safe-file.test.ts: add one suite-level afterEach that calls
vi.restoreAllMocks(), covering the readFileSync spy at lines 173-186, the
promises.readFile spy at lines 236-248, and the fsyncSync spy at lines 44-55.
Remove the explicit vi.restoreAllMocks() call at line 224 because the shared
cleanup replaces it.
In `@src/main/lib/safe-file.ts`:
- Line 237: Replace the synchronous fsyncDirBestEffort call in
writeFileSafeAsync’s durable rename path with an awaited async counterpart. Add
fsyncDirBestEffortAsync beside fsyncDirBestEffort using asynchronous directory
open, sync, and close operations while preserving best-effort error handling,
then await it after the rename.
---
Outside diff comments:
In `@src/main/installations.ts`:
- Around line 91-115: Update migrateRecord so the legacy useSharedPaths
migration does not overwrite an existing explicit useSharedModels value.
Preserve a boolean useSharedModels from the original record when present, and
only default it to true when absent; keep the existing useSharedInputOutput
migration behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 73e3e800-11b6-46c5-a993-eae0bc0290fb
📒 Files selected for processing (8)
src/main/installations.tssrc/main/lib/safe-file.test.tssrc/main/lib/safe-file.tssrc/main/lib/startup-attempt-marker.test.tssrc/main/lib/startup-attempt-marker.tssrc/main/lib/updater.test.tssrc/main/lib/updater.tssrc/main/settings.ts
…for a locked primary readFileSafe/Async now tag backup data served for an unreadable primary (primaryUnreadable), and settings/installations refuse to persist while that holds - saving state derived from the stale backup would overwrite the newer locked primary once the lock clears. Also dedupes the sync and async fallback policy into one helper and pins that the durable temp-file fsync happens before the publishing rename. Amp-Thread-ID: https://ampcode.com/threads/T-019fd4f6-8904-746f-a85d-9280a17b564b Co-authored-by: Amp <amp@ampcode.com>
….json, handle startup mutation rejections, async dir fsync - A readable-but-corrupt installations.json now flags the load outcome unreadable, so loadForWrite() rejects instead of letting save() replace every record with a list built from nothing. - register() startup mutations (seedDefaults, ensureExists x2, enforceCloudName) attach .catch handlers so a fail-closed rejection logs a warning instead of becoming an unhandled rejection. - writeFileSafeAsync uses an async directory fsync so a durable async write cannot block the main-process event loop. Amp-Thread-ID: https://ampcode.com/threads/T-019fd4f6-8904-746f-a85d-9280a17b564b Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/settings.ts`:
- Around line 409-418: The loadOutcome JSON.parse failure path must fail closed:
in its catch block, set unreadable to true so corrupt primary settings are never
overwritten by normalization or set(). Add a corrupt-primary test alongside the
locked-primary test, covering both automatic loading and set() preservation, and
ensure no rewrite occurs for corrupt data.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6788e5d3-895e-4d1f-ade3-b32506dd7acd
📒 Files selected for processing (7)
src/main/installations.test.tssrc/main/installations.tssrc/main/lib/ipc/index.tssrc/main/lib/safe-file.test.tssrc/main/lib/safe-file.tssrc/main/settings.test.tssrc/main/settings.ts
Comment on lines
+409
to
+418
| function loadOutcome(): { settings: Settings; unreadable: boolean } { | ||
| maybeSeedFromEnv() | ||
| let parsed: Record<string, unknown> | null = null | ||
| const raw = readFileSafe(dataPath) | ||
| if (raw) { | ||
| let unreadable = false | ||
| const read = readFileSafe(dataPath) | ||
| if (read.kind === 'unreadable') { | ||
| return { settings: { ...defaults }, unreadable: true } | ||
| } | ||
| if (read.kind === 'data') { | ||
| unreadable = read.primaryUnreadable === true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail closed after a settings JSON parse failure.
If JSON.parse(read.data) throws at Lines 419-425, unreadable remains false. set() can then save defaults over the corrupt settings.json. Mark this outcome unreadable in the catch block, so both automatic normalization writes and set() preserve the file. Add a corrupt-primary test beside the locked-primary test. Corrupt data must not get a rewrite delight.
Proposed fix
} catch (err) {
console.warn('Settings: failed to parse settings JSON:', (err as Error).message)
+ return { settings: { ...defaults }, unreadable: true }
}Also applies to: 577-578, 615-623
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/settings.ts` around lines 409 - 418, The loadOutcome JSON.parse
failure path must fail closed: in its catch block, set unreadable to true so
corrupt primary settings are never overwritten by normalization or set(). Add a
corrupt-primary test alongside the locked-primary test, covering both automatic
loading and set() preservation, and ensure no rewrite occurs for corrupt data.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hopefully addresses #1367
Problem
Telemetry shows single devices re-running the same startup install up to 246 times for one target version (22 devices with >5 attempts on 1.0.29 alone, recurring on every release since). The existing loop-breaker (
lastStartupUpdateAttemptVersion) works for most machines (6.5kloop_breakerskips across 2.8k devices) but is clearly not surviving between launches on the looping machines.The marker is the last settings write before the app quits into the installer, so
settings.json.bakis always one write behind it.readFileSaferestored.bakover the primary after ANY read error - including transient AV/indexer locks (EPERM/EACCES/EBUSY). On interference-prone machines that rollback erased the attempt marker while keeping the pending-update marker, so the same version reinstalled on every boot, indefinitely.Fix
safe-file.ts: retry transient lock errors on reads (primary and.bak) and sync renames. Restore.bakover the primary only when the primary is genuinely missing or empty; a merely locked primary is served from.bakWITHOUT restoring. Adds abakFallbacksdiagnostic counter.startup-attempt-marker.ts(new): durable sidecar marker file inconfigDir()- no.bakmachinery to roll it back, read-back verification after writing, and a tri-state read (present/absent/unavailable) so an unreadable (locked) marker is never mistaken for an absent one.updater.ts: the loop-breaker consults both marker homes; the verified sidecar must be durable on disk before any marker is written or the install proceeds. Fail-closed skips (marker_unavailable,marker_not_durable) open the UI normally instead of looping; the explicit pill install is unaffected. Stale sidecars are cleared once the target version is running.Telemetry to confirm the root cause in the field
The rollback mechanism is telemetry-inferred, not yet directly observed, so this PR also instruments the diagnosis:
startup_install_skippedwithreason: loop_breakernow carriessource: settings | sidecar. Asidecarhit means the settings marker was erased between launches - direct field evidence of the rollback. If loops continue with neither firing, the cause is elsewhere (updater cache errors are the next suspect).startup_installand skip events carrybakFallbacksto correlate looping devices with.bak-served reads.Tests
New
safe-file.test.ts(13) andstartup-attempt-marker.test.ts(8) pin the retry/no-restore semantics and marker durability.updater.test.tsadds sidecar-only loop-break, both fail-closed paths (asserting no marker is written so the next launch retries), newer-version override of a failed attempt, and pill-install bypass. Full suite: 226 files / 3695 tests green; typecheck and lint clean.