Skip to content

fix(setup): a failed setup-state write is restored and reported (#1250 rows 6, 13) - #1403

Merged
Juliusolsson05 merged 5 commits into
mainfrom
fix/setup-state-write-failures
Sep 27, 2026
Merged

Juliusolsson05 merged 5 commits into
mainfrom
fix/setup-state-write-failures

Conversation

@Juliusolsson05

@Juliusolsson05 Juliusolsson05 commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner

Refs #1250 (rows 6 and 13). #1250 is a batch issue, so the other rows stay open.

Problem

A failed setup-state write (a full disk, or a read-only or permission-broken state directory) was invisible:

  • saveSetupState set the in-memory cache BEFORE the write and never restored it on rejection. Main then behaved as if the change were saved, and silently reverted on the next launch.
  • Row 6: the provider enablement toggle and reset had no catch (called via void), so an unhandled rejection with nothing shown. The OpenCode usage-source row showed the raw IPC error.message, which can carry a filesystem path (q22).
  • Row 13: setCliUpdateBehavior had no rejection handler, and CliUpdateBehaviorRow had nowhere to say it failed.

Change

  • Main (setupState.ts): each save is an update function applied at write time to the last state known on disk (updateSetupState).
    • The cache is that state plus the pending updates, so a failed save drops out of memory and out of every later write.
    • Provider toggles update one key.
  • providerEnablement.mutate: rejects only when the write fails. A refresh failure after a landed write resolves against the last good detection. A rejected detection probe no longer sticks in flight. Refreshes are ordered, so an older one never overwrites a newer one. Published snapshots are built from the durable state, so a toggle still in flight, or one that failed, is never broadcast.
  • checkPrerequisites: its tool-path write-back is best effort. It is a cache of the probe, and its failure no longer makes a saved answer (skip, acknowledgment, manual path, Install, reset) reject. When the write-back fails, the toolchain applies the probed paths in memory, so "found at X" is also what a launch uses.
  • SetupGate (same file): manual-path, Install, skip, acknowledgment and check failures use fixed text. The panel answer's failure is a toast after the panel closes.
  • Renderer: each row catches and shows one fixed sentence, "Couldn't save this change. Nothing was changed." (role="alert"). No IPC or filesystem text reaches the UI.
    • setCliUpdateBehavior returns its promise.

Plan: docs/plans/2026-09-27-setup-state-write-failures.md.

Tests (all red on main or on the round-0 head 6707e7cf)

  • setupState.test.ts (new, real filesystem):
    • a directory at setup.json, and a one-shot rename fault, make writes fail;
    • a single failure, two queued failures, a failure followed by a good save, a good save followed by a failure, and two provider toggles.
  • providerEnablement.test.ts (new): a reset whose re-probe fails after the write, the stuck in-flight probe, the last-good-detection fallback, refresh ordering, and no publication of a failing toggle (through another toggle's refresh or an overlapping read).
  • prerequisites.test.ts (new): the real check and toolchain; a launch uses what the check found whether or not the write-back lands.
  • src/main/ipc/setup.test.ts (new): the real setup handlers and check. A saved answer resolves when only the check's write-back fails; a failing answer write rejects.
  • setupState.test.ts: also the shared first read, and a throwing update.
  • firstRun.renderer.test.tsx: the skip-failure toast, and the manual-path and Install failure text.
  • Renderer rows: a rejecting enablement toggle, a reset (on its own render), a usage-source set and a CLI-update behaviour each show the fixed sentence and no raw text, and a later success clears it.

Verification

  • NODE_ENV=test npx vitest run src/main/setup src/main/ipc/setup.test.ts src/renderer/src/features/{setup,cli-updates,providers,settings}: 116/117 at machine load 40. The failure is ProviderEnablementRow's first test, unchanged from main, timing out on its in-test cold import under load; reviewers saw the same. It was 112/112 on the previous head at normal load.
  • Review round 1 and verification: the dispositions are in the PR comments.
  • rm -rf .tsc-out; npx tsc -b: clean.
  • The app was not launched.

🤖 Generated with Claude Code

Juliusolsson05 and others added 2 commits September 27, 2026 03:13
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rows 6, 13)

saveSetupState assigned its cache before the write and never restored it, so a
failed write left main acting on an unsaved value until a restart reverted it.
The provider enablement and CLI update behaviour rows swallowed the rejection
(or showed raw IPC text); they now say, in fixed words, that nothing changed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Juliusolsson05 Juliusolsson05 added type:bug Something works wrong class:C3-silent-failure The app knows it failed and does not say sev:P3 Minor labels Sep 27, 2026
… changes (#1403 review round 1)

Saves are update functions applied at write time to the last state known
on disk, so a failed save can neither linger in memory nor ride along with
a later write. Provider toggles update one key; a refresh failure after a
landed write no longer claims nothing changed; a rejected detection probe
no longer sticks in flight. SetupGate's write failures are said in fixed
words, the panel answer after the close.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Review disposition, round 1 (head 9370170d)

Verdicts: a FIX-BEFORE-MERGE, b FIX-BEFORE-MERGE, c MERGE-READY with minors.

Finding Disposition
a1 / c1: two queued saves both fail, and the cache is left at the first one's unwritten value Fixed by redesign. Saves are update functions applied at write time to the last state known on disk (updateSetupState). The cache is that state plus pending updates, so a failed update drops out. Test: keeps the durable state when two queued saves both fail.
a2 / b2: a failed save is carried to disk by the next queued save while the row says "Nothing was changed" Fixed by the same design, which withdraws the plan's cache === snapshot ruling. Tests: never lets a later save carry a failed one to disk (the reviewers' timing) and does not persist a failed provider toggle with a later one. Provider toggles now update one key instead of a whole map built off the optimistic cache.
a3 / b1: a reset persists, re-detection fails, and the row says nothing changed Fixed. mutate rejects only when the write fails; a refresh failure after a landed write resolves against the last good detection. Found on the way: a rejected probe stayed in flight for good; it is now cleared in finally. Test: providerEnablement.test.ts (new), red on 6707e7cf both with the whole file reverted and with only the finally reverted.
b3: SetupGate shows raw IPC text or hides write failures Fixed (same file, same write path). A manual path shows the fixed sentence. A skip or acknowledgment failure is said as a toast after the panel's #1047 close. A check failure shows fixed text, with the raw error in the console. Tests in firstRun.renderer.test.tsx, both red on the old SetupGate.
b4 / c2: test gaps (reset alert masked, clearing unpinned, cache === snapshot guard unpinned) Fixed. Reset is tested on its own render; clearing after a later success is pinned in all three rows. b's four surviving mutations are each killed now. The guard no longer exists; its case, an earlier good save kept when a later one fails, has its own test.
c3: the body's test count Fixed in the body.

Verification:

  • rm -rf .tsc-out; npx tsc -b: clean.
  • NODE_ENV=test npx vitest run src/main/setup src/renderer/src/features/{setup,cli-updates,providers,settings}: 104/104 before the tsc-only fix; src/main/setup 17/17 after it.

🤖 Generated with Claude Code

… fixed install text (#1403 verification)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Review disposition, verification pass (head 144b2f69)

Verdicts: a FIX-BEFORE-MERGE, b FIX-BEFORE-MERGE, c MERGE-READY. All round-1 findings were verified fixed; the new findings are below.

Finding Disposition
b1: setup answers (skip, acknowledgment, manual path) were saved, then reported unsaved when the check's tool-path write-back failed Fixed at the root. checkPrerequisites persists its probe as a best-effort cache (warned), and returns the probe's answer either way. This also covers Install and a provider reset. New src/main/ipc/setup.test.ts drives the real handlers, setup state and check: each answer resolves with the answer on disk, and a failing answer write still rejects. It fails with the old prerequisites.ts.
b2: Install rendered the raw rejection Fixed: "Could not install ." The test fails on the old SetupGate.
a1: an older provider refresh could overwrite and broadcast a newer one Fixed: refreshes are numbered, and a stale one answers with the newer snapshot. The test holds the first row's credential probe; the cache and the last broadcast keep Claude off. It fails without the ordering.
a/b/c survivors: the shared first read, and the last-good-detection fallback Pinned. A held first read finishing after a save no longer resets the durable baseline (fails with loading =). A Claude-only detection plus a failed reset probe shows Codex not installed (fails with new Set(ALL) and with cachedDetected).
c (suspicion): a throwing update stays pending Closed: the update leaves pending on every path. Test added.

Verification:

  • rm -rf .tsc-out; npx tsc -b: clean.
  • src/main/setup, src/main/ipc/setup.test.ts and the setup, cli-updates, providers and settings renderer features: 112/112 before the last fix; src/main/setup plus the IPC test: 25/25 after it.

🤖 Generated with Claude Code

…uses unsaved probed paths (#1403 recheck)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Review disposition, recheck (head 6c274204)

Verdicts: a FIX-BEFORE-MERGE, b FIX-BEFORE-MERGE (c MERGE-READY at verification). Every earlier finding and survivor was confirmed fixed; the new findings are below.

Finding Disposition
b, a2: a provider snapshot published a toggle whose write then failed. The refresh read the optimistic cache while another toggle was in flight, whether from a second toggle's refresh or an overlapping get. Fixed: published snapshots are built from the durable state (loadDurableSetupState). Each refresh runs after its own write lands. Two real-write tests (a successful toggle beside a failing one, and an overlapping read) both fail with the optimistic read.
a1: a failed write-back left the toolchain on the persisted, possibly dead, path while the check said "found at X" Fixed: refreshToolchainFromState(unsaved) applies the probed paths in memory when they could not be persisted. New prerequisites.test.ts (real check, toolchain and setup state) fails without it.
a (survivor): the refreshToolchainFromState call was unpinned Killed by both prerequisites.test.ts cases.

Verification:

  • npx tsc -b: clean.
  • Focused suites: 116/117. The one failure is ProviderEnablementRow "renders one row per provider kind", which is unchanged from main. It times out on the first in-test dynamic import at machine load 40. Alone, under the same load, the first two tests time out at about 5 s and the rest pass in milliseconds. Reviewers a and c reported the same load behaviour in earlier passes. CI decides.

Rounds: this is the recheck after verification. A final focused check by a and b follows.

🤖 Generated with Claude Code

@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Final check (a, b) at 6c274204: both MERGE-READY, with no findings. Their real-write probes confirm two things. A pending toggle whose write fails is never published, whether through another toggle's refresh or an overlapping read. A failed tool-path write-back leaves the disk unchanged while this process launches the path the check just found.

Per q104 the review loop is closed here: round 1, verification, recheck and final check; no further rounds. The PR waits for exact-head CI and the manager gate.

🤖 Generated with Claude Code

@Juliusolsson05
Juliusolsson05 merged commit 1205a20 into main Sep 27, 2026
2 checks passed
Juliusolsson05 added a commit that referenced this pull request Sep 27, 2026
@Juliusolsson05
Juliusolsson05 deleted the fix/setup-state-write-failures branch September 27, 2026 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

class:C3-silent-failure The app knows it failed and does not say sev:P3 Minor type:bug Something works wrong

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant