Skip to content

Persist profile state in SQLite with a durable background writer - #22612

Open
OrcaWin wants to merge 31 commits into
mainfrom
OrcaWin/np-async-profile-persistence
Open

OrcaWin wants to merge 31 commits into
mainfrom
OrcaWin/np-async-profile-persistence

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 212 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​24344 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1232 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​23112
Prod 233 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​13872 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1896 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​11976

ELI5

Orca saves migrated profiles to SQLite in a background worker. Terminal creation, removal and automation changes wait for the save to succeed, so reported success survives a restart. Existing JSON profiles migrate automatically on capable runtimes; JSON remains available for import, export, recovery and admitted legacy runtimes.

Stack: #22612 → #22635 (bundled Bun) → ordinary JSON-backend retirement.

What Changed

  • Save separately validated profile domains in transactions. Ordinary edits update changed domains; full checkpoints, exports and backups use a consistent revision. Preserve unknown fields and sealed secrets.
  • Move database writes and backups into workers. Batch compatible queued saves while keeping durable mutations, exports, cancellation and later edits ordered. A canceled caller stops waiting without killing an admitted write or disabling subsequent saving.
  • Make terminal bindings, retirement, SSH leases and automation acknowledgements follow durable saves. Known failures roll back the matching change; unknown commit outcomes stop further writes pending recovery. A late terminal exit cannot remove its replacement.
  • Keep plain profile switches writable through relaunch cleanup and the final SSH-detach checkpoint. Known maintenance failures may re-admit unchanged storage and complete the final save; changed storage and uncertain outcomes remain protected.
  • Use revision checks and recovery records for migration, backup restoration and cross-profile moves. Startup replays moves involving its active profile; rollback refuses unresolved moves before changing either participant. CLI journal inspection does not import replay/startup dependencies.
  • Publish current compatibility JSON on clean final shutdown and maintenance, off the ordinary save path. Recover damaged legacy JSON from its backup ring. Release profile admission after successful teardown, durably publish ownership records, and reclaim proven previous-boot POSIX owners. Reboot reclamation requires matching host and machine evidence; cloned machine IDs cannot authorize taking another host’s owner record.
  • Show pre-ready desktop failures instead of silently exiting. Distinguish newer database schemas from corruption, and explain unsupported publication filesystems without weakening atomic no-overwrite publication. Background and serve launches report errors through logs.

Why

Whole-profile JSON serialization and rewriting scale with the complete profile for every save. SQLite transactions let ordinary writes scale with the changed domains. Workers move database disk work off the main thread, while explicit acknowledgements preserve correctness.

The existing database interface isolates the driver. An ORM would add another representation without replacing the revision checks or recovery protocols spanning files and databases, so those protocols remain explicit.

Linked Issue

No linked issue. Runtime portability is stacked separately in #22635.

Visual Proof

N/A — persistence and lifecycle changes; no rendered interface changes. Startup refusal is covered through the native error-dialog boundary without showing test windows.

Testing

Current head: 7306dd2. Rebased onto main at 5c45337. Complete PR CI passed, including both desktop and Docker SSH end-to-end lanes.

Gate Current evidence
Base-branch persistence, profile, automation and startup suites 1,334 passed, 8 platform/environment skips across 147 files before rebase; the rebased head now passes the complete PR CI run.
End-to-end on current head Changed desktop specs: 52 passed, 2 explicit skips requiring an unavailable older packaged executable. Docker SSH: 47 passed across watcher isolation, terminal parking/readiness and remaining SSH specs. No retries.
Static checks Full normal typechecks passed after rebase; 42 focused restart/admission/identity tests passed (one platform skip). Changed-code quality, formatting and commit hooks passed on the reviewed combined tree.
Cancellation/finalization Real worker regressions verify abandoned waiters, ordered final checkpoints, known maintenance failures, and preserved fencing after unknown outcomes.
Profile switching Actual IPC handler preserves pre-relaunch writes and final SSH detach, including quit during the switch checkpoint.
Recovery/startup Future schemas stay untouched; unsupported-link fault injection preserves the source and permits retry; malformed journals remain protected; owner publication faults clean up only this admission.
Independent review Three parallel reviews, cross-review of fixes, and negative controls reproducing the confirmed failures. Mobile retirement claims were checked against existing durable-close/replacement tests.
ORCA_BACKGROUND_LAUNCH=1 pnpm test src/main/persistence/profile-state src/main/persistence/loading-store src/main/orca-profiles src/main/automations
pnpm tc
pnpm run check:code-quality:changed

Build CLI before Electron because both write to out/main. Agent-launched application checks use background mode.

  • Local behavior and automated regressions exercised.
  • Authority, migration, transactions, worker faults, shutdown, backup/recovery and rollback covered.

Review

Suggested order: authority admission/migration; worker protocol and Store ordering; terminal/SSH/automation durability; maintenance/recovery; failure-injection tests.

The latest external review identified four reachable P1 issues, all fixed: caller cancellation, pre-ready startup failures, profile-switch final state, and final saving after known maintenance failure. The reported managed SSH upgrade failure applies to deployment functions with no production callers. That path still refuses an unproven owner; connecting it requires an ownership migration, not merely trusting a process name.

The reported mobile generation and committed-close failures do not match the current safeguards: retirement compares Map.get without recreating generations; missing retired rows remain accepted; replacement incarnations remain protected. Schema versions 1/2 were unreleased intermediate formats; a future released schema bump must add its own startup upgrade admission.

Agent skill upstream boundary

  • Not applicable; no upstream skill-installer resources copied or translated.

Notes

  • Filesystems without hard links remain unsupported for atomic database publication. Startup now gives actionable instructions while preserving the original data and recovery files.
  • Windows reused PIDs and unverifiable legacy ownership records remain conservatively protected, with explicit manual remediation. New POSIX records can establish previous-boot exit using matching host, machine and boot identity; invalid Linux machine IDs are rejected.
  • Forced/timed-out shutdown cannot guarantee current downgrade JSON. A worker's own unknown-commit timeout remains a reason to stop writes; a caller's canceled wait is no longer one.
  • No automatic choice between divergent JSON and SQLite was added. Recovery preserves both copies and directs users to inspection/rollback commands.
  • JSON import/export/recovery remains. Bun supplies the headless runtime without requiring users to install Node 24. No new required public RPC fields or stream opcodes are introduced.
  • Tracking, design, research and measurement documents remain local.

Checklist

  • Small PR — intentionally substantial: complete persistence cutover and durability callers.
  • Explained behavior, mechanism and alternatives.
  • Visual proof is N/A with reason.
  • Independently reviewed for correctness and performance.
  • macOS/Linux/Windows, SSH, folder workspaces and mixed versions considered.
  • Local validation recorded; complete PR CI passed above.

@OrcaWin
OrcaWin marked this pull request as ready for review September 24, 2026 06:49

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — the writer/revision protocol, migration admission, secret retention and backup/export paths all held up under a dedicated pass. A few rough edges inline.

Reviewed changes — this run read the full 392-file diff and checked it against the writer protocol, storage admission, secrets, backups, durability awaits, automation-runs storage, runtime reconciliation and packaging wiring.

  • SQLite profile-state authority — one profile-state.db per profile behind the existing Store interface; domain payload rows plus per-row automation runs with a rebuilt projection, preserving normalization, encryption and ownership.
  • Background writer worker — live SQL runs in a worker; the main thread serializes commands, fences exact revisions and latches the connection on any indeterminate ack, so a committed write can never be misreported as a recoverable failure.
  • Durable mutation API — PTY binding, SSH lease, terminal retirement and the codex ledger now await runDurableMutation, with rollback that preserves newer bindings, secret changes and concurrent presentation edits.
  • Migration / admission / recovery — fail-closed classification, schema/identity/hash/revision validation, atomic empty-profile publication, versioned exports and rotating backups.
  • Offline CLI, orcad/desktop packaging — two new worker entries and unpacked CLI sidecars wired through the build and plain-Node guard.

ℹ️ A valid legacy JSON profile beside any stray SQLite sidecar refuses startup

classifyProfileStateStorage counts profile-state.db-wal / -shm / -journal as database evidence, so a profile with intact orca-data.json plus an orphaned sidecar (or a zero-byte/corrupt .db) classifies as both and is routed into SQLite admission instead of the JSON it still has in hand. Admission then fails closed and the user is told to recover from a backup even though the primary JSON is valid. This is deliberate ("SQLite presence is authority evidence") and pinned by treats an orphaned SQLite %s sidecar as authority evidence with or without JSON, but it is the one way this change can strand a user with recoverable data.

Technical details
# Stray SQLite sidecar blocks a valid JSON profile

## Affected sites
- `src/main/persistence/profile-state/profile-state-storage-classification.ts:20-30` — existence of any `profileStateDatabaseFiles` entry drives `both`/`sqlite-only`.
- `src/main/persistence/profile-state/profile-state-authority-bootstrap.ts:83-99` — `both` → `readAcceptedState` → `openProfileStateDatabaseReadOnly`, which throws `Profile state database does not exist` when only a sidecar survives, then becomes `ProfileStateRecoveryRequiredError`.
- `src/main/persistence/profile-state/profile-state-recovery-required.ts:22-29` — no JSON fallback once database-family files exist.

## Required outcome
- Decide and document whether a sidecar without a primary database should refuse startup outright or be treated as ignorable residue when the primary JSON is independently valid.

## Open questions for the human
- Is refusing startup the intended tradeoff for the "JSON remains for deliberate recovery" contract, and is there a user-facing path to clear an orphaned sidecar without a manual delete?

ℹ️ Nitpicks

  • src/main/persistence/profile-state/profile-state-recovery-copy.ts:23 — on non-Darwin the function always does a direct copyFileSync to the final target, so the exported API is not crash-atomic. Both current callers stage the target and validate/publish later, so this is only a hazard for a future caller.
  • src/main/persistence/profile-state/profile-state-database.ts:104-227 — quick_check (and full schema verification) runs on every open, including read-only opens in history GC / recovery; consider whether the per-open cost is acceptable for very large databases.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/ipc/pty/pane/stable-owner.ts Outdated
Comment thread config/build-plugins/plain-node-entry-guard.ts
Comment thread src/main/persistence/profile-state/profile-state-backup-rotation.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in this delta. The single commit since the last review is test hardening plus a few small production tweaks; the previously opened inline threads are untouched by it and remain open, so this is a comment review rather than an approval.

Reviewed changes — reviewed b672ccaace (the only commit since the prior pullfrog review at 26675c1d16), its surrounding call sites, and reran the affected tests.

  • Lazy startup milestones — logStartupMilestone now accepts () => details and snapshots t before invoking it; the loading store drops its duplicate logPersistenceStartupMilestone helper and uses the shared function.
  • Recovery-launch diagnostics — failure errors now carry bounded child output (exitCode, signal, timedOut, outputTruncated, and the trailing 4096 stderr characters) on RuntimeClientError.data.
  • Background-launch policy — startup recovery-dialog suppression now routes through isBackgroundLaunch() rather than a raw ORCA_BACKGROUND_LAUNCH comparison.
  • Schema cleanup — automation-runs table SQL is inlined into profile-state-database-schema.ts and the createProfileStateAutomationRunsTablesSql helper is removed, with no dangling references.
  • Bundle-parity guard — the CLI/main bundle parity test moves to config/scripts and reads the exported electronViteConfig input map, asserting exact source paths instead of key presence alone.
  • Test hardening — durable Store fixture reused in the pty respawn and remote retirement tests, retirement onPtyExit awaited, launchUntilExit captures pre-renderer startup refusals from process creation, and raw Linux dev Electron gets the sandbox opt-out.

Locally verified the new/changed tests pass: config/scripts/cli-main-module-bundle-parity.test.ts (2), src/main/startup/startup-diagnostics.test.ts (4), src/cli/runtime/profile-state-recovery-launch.test.ts (13), and src/main/persistence-loading-store-extraction.test.ts (10).

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues in this delta.

Reviewed changes — reviewed 5ef25a7ecc (the only commit since the prior pullfrog review at b672ccaace), traced its call sites in SshRelaySession, and reran the affected suite — including a check that the new regression test fails against the pre-fix logic.

  • Live passthrough routing — quarantineReattachData now owns the livePassthrough case and streams the frame when ownsPtyRecoveryAttempt(payload.id, pending) holds, instead of the provider onData handler short-circuiting to acceptPtyData while recovery-transfer frames were queued into pending.liveData and never drained.
  • Regression coverage — the new keeps source output contiguous while the recovered pane binding waits for disk case drives a frame through the source-activation transferToRecovery sink while persistPtyBinding is deferred, asserting acceptOutputData receives [4, 8, 12] and the recovery lease commits only after the binding resolves. Reverting the livePassthrough branch makes it fail ([4]), so it exercises the real fix.
  • Test tidy-up — emitSourceFrame gained data/sink parameters so the existing cases reuse the helper.

The three prior-review threads (plain-node-entry-guard.ts, stable-owner.ts, profile-state-backup-rotation.ts) anchor paths this commit does not touch, so they remain open and this is a comment review rather than an approval.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues in this delta.

Reviewed changes — reviewed 52481a66ab (the only commit since the 5ef25a7ecc review), traced its call sites and the retirement fence, and reran the affected suites.

  • Durable-binding retirement on spawn-time exit — a new registerPersistedPtySpawn wraps runtime.registerPty(...); when registration throws agent_session_exited_during_start and the runtime holds an earned exited verdict, it awaits retirePersistedStablePaneOwner(...) before rethrowing, so a terminal that dies during the durable persistPtyBinding write no longer leaves a persisted binding that would resurrect the closed pane.
  • Both spawn commit paths now use it — commitRuntimePtySpawn and commitPtyIpcSpawn await the helper in place of a direct registerPty call; the runtime path also destructures ctx.hostSessionBinding into locals (no behavior change).
  • Coverage — pty-spawn-exit-durability.test.ts (8 cases) exercises runtime and IPC controllers on local and SSH connections, proves a replacement binding (new and same ptyId, new incarnation) survives while retirement waits, retains the binding when loss of contact supplies no process-exit proof, and holds the rejection until SQLite cleanup completes.

The fence in retirePersistedStablePaneOwner matches on current.ptyId/current.incarnationId, so a replacement incarnation wins; the unverifiable case correctly leaves the binding alone. Affected suites rerun green (pty-spawn-exit-durability 8, spawn-commit-pty-size, spawn-reattach-size-cache, orca-runtime-terminal-retirement, spawn-commit-ssh-lease-cardinality 29). The three prior-review threads anchor paths this commit does not touch, so they remain open and this is a comment review rather than an approval.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues in this delta.

Reviewed changes — reviewed 30664b6b66 (the only commit since the prior pullfrog review at 52481a66ab), traced every registerPersistedPtySpawn call site, and reran the affected suites.

  • Synchronous successful registration — registerPersistedPtySpawn is no longer async; on success it calls runtime.registerPty(...) and returns undefined synchronously, so callers no longer yield a microtask before the remaining spawn publication. The confirmed-exit retirement path returns retirePersistedStablePaneOwner(...).then(() => { throw error }) (Promise<never>), while the non-retire throw paths still throw synchronously.
  • Callers updated — commitPtyIpcSpawn and both commitRuntimePtySpawn sites now await only the returned rejection promise, which is behaviorally equivalent to the old await on the retirement path and removes the success-path yield.
  • Telemetry extraction — the spawn agent_started block moved verbatim into recordPtySpawnTelemetry (spawn-telemetry.ts); both call sites are unchanged in effect.
  • Tests — the unverifiable case now asserts a synchronous throw, the retirement case consumes the returned promise, and a new case pins the synchronous success return (it fails against the pre-change async version).

Verified locally: pty-spawn-exit-durability (9), spawn-commit-pty-size (2), spawn-reattach-size-cache (5), and spawn-commit-ssh-lease-cardinality (8) all pass. The three prior-review threads (plain-node-entry-guard.ts, stable-owner.ts, profile-state-backup-rotation.ts) anchor paths this commit does not touch, so they remain open and this is a comment review rather than an approval.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found in this delta.

Reviewed changes — reviewed 1d6e677a1a (the only commit since the prior pullfrog review at 30664b6b66), traced the binding write path and the crash-hydration reconnect plan, and confirmed the new regression test fails against the pre-fix source.

  • Activity recorded with the binding — applyPtyBinding now appends the binding's worktree to session.activeWorktreeIdsOnShutdown when that list is present and omits it, so an acknowledged spawn or reattach survives a crash before the renderer writes its own activity snapshot; an absent list keeps the existing derive-from-tabs semantics.
  • Fast lane stays strict — evaluatePtyBindingFastLane adds an inactive_worktree miss under the same condition, so an otherwise-matching durable reattach takes the write path and repairs the missing activity instead of being skipped.
  • Rollback — the fieldwise rollback restores the activity list on a known write failure; the durability test asserts the in-memory session returns to its pre-write value.
  • Regression coverage — pty-binding-async-durability.test.ts pins that activity lands only after the SQLite transaction commits and that a same-binding reattach repairs the list exactly once; the new terminal-binding-crash-hydration.test.ts drives applyPtyBinding → hydrate → reconnectPersistedTerminals for repo and folder workspaces, with and without a pre-existing tab row. Reverting the production change fails all four cases.

The three prior-review threads remain open on files this commit does not touch, so this is a comment review rather than an approval. Local runs of the three affected suites pass (pty-binding-fast-lane 4, pty-binding-async-durability 11, terminal-binding-crash-hydration 4).

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@OrcaWin

OrcaWin commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

Reviewed the additional observations from the initial review as well:

  • SQLite files or orphaned sidecars remain authority evidence even when legacy JSON exists. Automatically using that JSON could discard newer acknowledged data. Startup preserves the evidence and requires explicit recovery. The selected JSON-export rollback validates its input before quarantining database/WAL files and replacing the live JSON; bootstrap and CLI recovery tests pass (40 tests).
  • Recovery copy targets are staging files. Both callers already own validation, fsync and publication; the copy API now states that contract directly. Adding another atomic-copy layer would duplicate the existing publication boundary.
  • Retained read-only admission and integrity checks. Earlier local measurements put the repeated scan at roughly 3–13 ms and schema verification around 0.2 ms. The steady writer reuses its connection, and backup validation runs in a worker. Earlier writable/query-only admission experiments could recover a hot journal before validation, violating preservation of damaged-state evidence. These checks also do not replace the separate payload-hash validation.

All three inline threads are addressed in ac4a8c0. Full local typecheck, CLI/Electron builds and the changed-code quality gate pass. The Bun PR is being rebased onto this correction for a combined ad-hoc build.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — reviewed ac4a8c0bc3 (the only commit since the prior pullfrog review at 1d6e677a1a), traced the new durability branch through runDurableMutation, the guard/parity wiring and the backup-rotation lifecycle, and reran the affected suites. This commit retires all three previously open threads.

  • Gated the retirement fast path on durability — retirePersistedStablePaneOwner now returns persist: 'if-dirty' when the pane is already absent in memory, and runDurableMutation skips the write only when lastDurableWriteGeneration >= writeGeneration. A renderer removal still waiting for its debounced flush is therefore forced to disk before retirement is acknowledged, while an already-durable removal keeps the no-write fast path.
  • Consolidated the CLI plain-Node guard — the eleven sidecar inputs plus the agent-hooks, codex and keychain entries now share one exported CLI_MAIN_ENTRY_NAMES list used by both the Rollup inputs and the guard; a parity test asserts every src/main/* module the CLI imports is in GUARDED_ENTRY_NAMES, and a per-entry test drives direct, static and dynamic Electron requires.
  • Tightened backup-rotation idleness — assertIdle() now checks the whole pending operation instead of the copying flag, so synchronous quarantine is fail-fast across queueing, snapshot copy and retention pruning.
  • Added regression coverage — pty-retirement-async-durability.test.ts uses real SQLite to pin delayed acknowledgement, failure/retry and the already-durable no-write path for local and SSH owners; the backup suite adds the queued-before-start and blocked-prune intervals.

Locally verified: reverting the stable-owner.ts line to persist: false fails 4 of the 6 new retirement cases (both hosts, both delayed-ack cases); with the fix, pty-retirement-async-durability (6), profile-state-backup-rotation (9), plain-node-entry-guard (26) and cli-main-module-bundle-parity (3) all pass.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — reviewed the two commits since the prior pullfrog review at ac4a8c0bc3: 42f874d341 (dedup refactor) and 60cc397e89 (drain admitted terminal mutations, restore failed retirements), traced the retirement path through runDurableMutation and the maintenance drain, and confirmed the new regression suite fails against the pre-fix admission check.

  • Admitted mutations survive finalization — runDurableMutation now sets a durableMutationPhase for the duration of its synchronous mutate/rollback callbacks, and setWorkspaceSession treats that as permission to publish a snapshot that was admitted before maintenance began. The relaxation is scoped to the callback (the phase resets before the disk await), writesFrozen still throws unconditionally, and the before-unload stage path still passes false, so no new renderer snapshot can slip in.
  • Failed retirements restore without rebasing — on a failed retirement write, the fieldwise rollback publishes the merged session directly instead of routing through setLocalWorkspaceSession/setHostWorkspaceSession, whose runtime-authored-field rebasing would re-apply the retirement the rollback is undoing. The merged object derives from the live session, so newer edits are preserved.
  • Refactor is behavior-preserving — isRecord is imported from profile-state-document-validation.ts (identical body) and readAgentHookSettingsFromJson runs both branches through the shared readAgentHookSettingsFromSettingsValue coercion.
  • Regression coverage — profile-state-pty-retirement-finalization.test.ts (18) pins retirement admission across running/maintenance/freeze/final on local and SSH, a failed-write rollback that keeps a newer activeTabId, and phase reset after throwing callbacks.

Verified locally: reverting the local admission change fails 3 cases (maintenance/freeze/final); all 18 pass with it. The three previously opened threads remain resolved.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@OrcaWin
OrcaWin force-pushed the OrcaWin/np-async-profile-persistence branch 2 times, most recently from 9622ea0 to 1b2e701 Compare September 25, 2026 02:50

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — re-reviewed the two commits added since the prior pullfrog review at 60cc397e89 (both rewritten by a later rebase onto main@bf40d35b0b; the earlier docs commit and the behavior-preserving deployment refactor have no behavioral surface): ba791bb4cf (profile maintenance recovery + shutdown compatibility) and 1b2e70166d (durable automation writes). Read both commit diffs end to end, traced the maintenance/durability call sites, and ran the new suites: automation-run-writer (7), automation-worker-durability (18), headless-dispatch-durability (4), profile-state-maintenance-recovery (7), profile-state-maintenance-compatibility (3), profile-state-worker-coordination (14), profile-state-backup-temporary-files (2) and profile-state-access (26) — all pass.

  • Recoverable profile maintenance — beginProfileStateMaintenance now wraps the pause in a canResumeFailedMaintenance catch that re-proves the authority writable and unfreezes on a known write failure, and freezes/closes on an indeterminate or already-frozen state; pauseProfileState drains backup work, flushes without the caller's signal so an already-dispatched commit keeps a known outcome, publishes the JSON compatibility export, then freezes.
  • Owner identity beyond hostname — new profile-state-access-identity.ts records kernel boot identity and process-start identity, so ownerExited reclaims a reused PID only when the boot matches and the start identity differs, and treats an unavailable identity as unverifiable.
  • Orphaned backup cleanup + cancellation — backup temp files carry a boot/namespace owner scope; removeAbandonedProfileStateBackupFiles removes only old files whose owner PID is proven exited, and runProfileStateBackupWorker accepts an abort signal and removes its temp files after the worker exits.
  • Fatal mutation latch — a throwing mutate/rollback callback latches fatalMutationError, freezes writes and closes the authority; every later sync/async write rethrows it instead of persisting a partial edit.
  • Shutdown compatibility — normal quit runs flushFinalOrThrowAsync({ exportJsonCompatibility: true }) and releases profile admission only after the writer closes; orcad keeps admission when runtime teardown fails; the update-quit cleanup budget for required exports is 90s.
  • Durable automation writes — every automation run/definition write awaits flushPendingOrThrowAsync before announcing or returning, and requestAutomationDispatch rechecks the definition, destination and run status after the acknowledgement wait, cancelling on stop or removal.

ℹ️ Every normal quit now serializes the full profile to JSON

main-process-quit.ts previously gated exportJsonCompatibility on an in-progress update; it now passes true unconditionally, so a migrated (SQLite-only) profile writes a whole-profile JSON compatibility export on every quit. That is a deliberate downgrade-compatibility choice, pinned by the renamed exports a normal quit for an older build after sessions and settings changed test, but it is a new per-quit cost that scales with profile size and narrows the "migrated profiles write only to SQLite" framing in the description. Worth confirming the tradeoff is intended for large profiles.

Technical details
# Per-quit full JSON export

## Affected sites
- `src/main/startup/main-process-quit.ts:203-216` — `flushFinalOrThrowAsync({ exportJsonCompatibility: true })` on every `will-quit`.
- `src/main/startup/main-window-core-services.ts:129-132` — the update-quit hook now relies on the compatibility export to carry the recovery export too, so the per-quit and per-update paths both serialize the whole profile.

## Required outcome
- Confirm (or gate) that a whole-profile JSON serialization on every ordinary quit is intended; if the compatibility window only matters around updates, the prior `updateQuitInProgress` gate kept normal quits JSON-free.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — reviewed the one commit added since the prior pullfrog review at 1b2e70166d: 799dd38db0 (reconcile manual automation requests interrupted before launch). Read the commit diff and the reconciliation flow, and ran run-completion-watcher (10) and retained-run-reconciliation (5) — both pass.

  • Stranded pending manual runs are closed out — reconcileRetainedRuns now also stages pending runs whose trigger === 'manual', and describeStrandedAutomationRun gained a pending branch returning "Orca stopped before this manual run could launch." A pending manual run left by a dead process cannot be re-dispatched in a new session, and resolveRunTerminal returns null without a terminalPaneKey, so the run is staged and stranded after the surface-settle grace.
  • Scheduled pending runs stay retryable — the filter excludes scheduled pending runs, and the test pins that pendingScheduled remains pending while pendingManual becomes dispatch_failed.

The change matches the existing dispatching handling: the stale captured run object never resolves a pane, so the reconciler correctly waits for the terminal surface to report ready before stranding. No concerns.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — reviewed 04ba64fce1 (the only commit since the prior pullfrog review at 799dd38db0), traced both terminal-close refusal paths through runDurableMutation, and confirmed the two new regression cases fail against the pre-fix source.

  • Expected refusals no longer freeze profile persistence — commitHeadlessTerminalTabRetirement (terminal_pane_owner_changed, workspace_session_unavailable, terminal_tab_pinned, tab_not_found) and clearWorktreeTerminalResumeRecords (terminal_close_incomplete) now return { value: new Error(...), persist: false } instead of throwing inside the mutation callback. A throw there runs stopAfterFailedPrimaryStateMutation, which latches fatalMutationError and freezes every later write, so a merely refused tab close used to permanently stop profile saving. The returned Error is re-thrown, so callers still observe the refusal.
  • Contract documented — runDurableMutation now states the rule: expected refusals return persist: false, thrown callbacks are treated as partial-state corruption.
  • Regression coverage — the pinned-close case and a new worktree-teardown case assert flushPendingOrThrowAsync() resolves after the refusal. Both fail pre-fix with Cannot flush while persistence is finalized; at head acknowledged-terminal-tab-retirement (13) and orca-runtime-terminal-retirement-host-partition (8) pass.

The change matches the refusal idiom already used by pty-binding-persistence.ts and ssh-pty-consumer-recovery.ts, and no other terminal-close refusal path still throws inside a durable mutation.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — reviewed the two commits added since the prior pullfrog review at 04ba64fce1: 9c87748d75 (remove compatibility exports after clean profile shutdown) and 3268a67bfe (observe SQLite authority in restart and startup journeys). Both are E2E-test-only; read the commit diffs end to end and checked every assertion they add or move.

  • SQLite-authority reads in the parked-scrollback journey — readOnDiskPartitions now reads committed state through readPersistedProfileState (opens profile-state.db read-only under a runtime admission) instead of globbing retained orca-data.json compatibility exports, so the load-bearing runtimePartitionHoldsCapture / localPartitionDidNotKeepCapture assertions read the same authority production restores from. Runtime admissions are shared participants, so reading while the paired app is still live remains valid.
  • Per-profile seeding in the launch-budget journey — each baseline/populated sample now gets a fresh session and has plugins seeded before its first migration into SQLite, replacing the shared session that re-seeded an already-migrated profile (which would have had no orca-data.json to edit); the inert-plugin marker assertion moved inside the loop so it runs per launch.
  • Clean-shutdown compatibility assertions — the terminal-restart journey now asserts the compatibility JSON contains the created automation after a clean close, and refreshes/deletes the JSON mirrors only after the clean-maintenance export, keeping the later defaultJson/targetJson-absent assertions that prove a SQL-only launch.

No inline comments: the delta has no production code, and no assertion was loosened or made vacuous.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The new snapshot coalescing can silently drop a getter-only in-memory edit while an awaiting durability flush reports success. Reproduced against e5f44ea87d; details inline.

Reviewed changes — reviewed the two commits since the prior pullfrog review at 3268a67bfe: fffcfdbbcf (coalesce queued durability snapshots) and e5f44ea87d (keep the writer usable after export preparation failures). Read the write/flush/worker paths around the change and ran the new and affected suites.

  • Queued snapshot batching — enqueueWrite now reuses an in-flight queuedSnapshot batch, folding in skipIfClean and the latest pendingSnapshotFileWork and returning that batch's completion; enqueuePrimaryStateOperation clears the queued batch so a durable mutation, export or checkpoint starts a new one, so concurrent flushes share a single worker capture.
  • Preparation failures stay recoverable — a new ProfileStateExportPreparationError marks failures raised before SQLite acceptance or canonical JSON is touched, so the worker no longer latches stopping for export-compatibility/close on such a failure, while an indeterminate underlying cause still surfaces through the recursive outcome derivation.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/persistence/loading-store/primary-state-writes.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — reviewed ca5ab03788 ("fix: preserve getter edits across coalesced profile flushes"), the only commit since the prior pullfrog review at e5f44ea87d. Traced the merge branch through writeProfileStateInWorker and prepareSelectiveProfileStateWrite, checked every enqueueWrite caller, and confirmed the new regression cases fail against the pre-fix source.

  • Merged explicit flushes now force a full capture — the queued batch's capture carries a fullCheckpoint flag, and queued.capture.fullCheckpoint ||= !queued.capture.skipIfClean runs after the existing skipIfClean &&= upgrade. Any batch that absorbs a non-skipIfClean durability waiter — explicit-joins-debounce or debounce-joins-explicit — nulls dirtyProfileStateDomains immediately before capture, so the untracked getter-only edit (getWorkspaceSession().activeTabId = …, which marks no domain dirty) is serialized instead of being dropped by a selective write.
  • Debounce-only batches stay selective — two debounced saves that coalesce keep skipIfClean true, so the full-capture trigger does not fire; the new case asserts writeCompleteSerializedDomains is never called.
  • The dirty-set reset reads the batch, not the call — primary-state-writes.ts:205 now consults capture.fullCheckpoint, so a merge that lands after the batch was enqueued still upgrades the capture. Non-batchable writes (final checkpoints, signal-bearing flushes) are unaffected: they cannot merge and their capture.fullCheckpoint mirrors options.fullCheckpoint.
  • Regression coverage — the batching suite adds both merge orderings for the getter-only edit, the explicit-then-debounce case, and the debounce-only selective check; the 21-waiter coordination regression now pins exactly one full serialization across two captures. Removing the fullCheckpoint ||= line fails five cases (both getter orderings, explicit-then-debounce, the shared-failure test that moved to writeCompleteSerializedDomains, and the full-capture assertion), so the tests exercise the real fix.

Verified locally: profile-state-write-batching (11) and profile-state-worker-coordination (14) pass at head; pnpm tc is green.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one narrow locking edge case inline.

Reviewed changes — re-reviewed the single commit added since the prior pullfrog review at ca5ab03788: 353afda6e3 (preserve profile saving through cancellation and recovery failures). Read the commit diff end to end, traced the durability/lifecycle call sites, and ran the new and changed suites (11 files, 134 tests pass).

  • Caller cancellation stops only the waiter — enqueueWrite drops the signal abort listener that called profileStateAuthority.abort() and returns waitForPromiseWithSignal(completion, signal); an abandoned waiter rejects while the admitted write keeps its place in runtime.pendingWrite, so the final checkpoint still joins it.
  • Quit survives a known maintenance failure — flushFinalOrThrowAsync awaits pendingProfileMaintenance, swallows a known failure only when the store was re-admitted, then drains and runs the final full checkpoint before closing; canResumeFailedMaintenance no longer treats quitFlushStarted as preventing resume.
  • Profile switch stays writable through relaunch — the switch handler uses flushActiveProfileBeforeRelaunch (plain flush, no maintenance pause), so the live source writer lets SSH-detach and other cleanup writes reach the source profile's final checkpoint.
  • Cross-profile move journal — move intent types/validation move to profile-project-move-record.ts; startup replays only intents involving the active profile, and offline rollback refuses while a move is unresolved.
  • Startup diagnostics — newer schemas raise newer-schema (no rollback instructions), unsupported hard-link publication raises publication-unavailable with actionable text, and pre-ready desktop launch failures now show a native error dialog.
  • Owner reclamation — new machine identity plus directory fsyncs, and a published owner is removed when a later durable publish step fails.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/persistence/profile-state/profile-state-access-owner.ts Outdated
m4air added 10 commits September 25, 2026 00:36
Share fully checked streaming reads across snapshots and backup/recovery,
and produce checkpoint strings through the existing secret serializer.
Copy and move only changed domains with fenced writes and compatible
versioned crash-recovery intents. Preserve schema, hashes and durability.
…ifetimes

Preserve surviving legacy backups during admission, publish immutable exports
with the actual snapshot revision, and reject malformed inactive JSON roots.
Keep import-only Stores isolated from live alias/listener ownership and freeze
them before normalization can schedule work.

Fence worker admission when close starts. Roll back failed PTY bindings through
unrelated edits while retaining valid surfaces and newer sibling ownership.
Remove duplicate restart notification, parser and deferred resolver plumbing.

Add regression coverage for each recovered race and lifetime boundary.
@OrcaWin
OrcaWin force-pushed the OrcaWin/np-async-profile-persistence branch from 353afda to 7306dd2 Compare September 25, 2026 07:37

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — re-reviewed the two commits added since the prior pullfrog review at 353afda6e3 (the branch was rebased, so that head is now 345e165fc8 with identical content): 29b06b3263 (keep cloned host identities from reclaiming live profile owners) and 7306dd23 (await durable state in the pane restart test harness). Read both commit diffs end to end, traced ownerExited against every reclamation case, and ran the affected suites.

  • Cross-host machine-id collisions now fail closed — ownerExited gains sameHost = owner.host === hostname(); the foreign-host guard becomes !sameBoot && !sameHost, and the reboot shortcut now requires sameHost && sameMachine instead of sameMachine alone. A second host sharing a cloned machine id and a profile directory can no longer reclaim a live owner through a differing boot identity, while a same-host reboot with a matching machine identity still proves exit. This directly addresses the prior inline finding.
  • Placeholder Linux machine ids are rejected — /etc/machine-id is only accepted when it matches ^[a-f0-9]{32}$ and is not all zeros, so the uninitialized placeholder value can no longer be treated as a stable machine identity.
  • Regression coverage — the new profile-state-access-identity.test.ts pins the Linux validation cases, and the new "does not reclaim another host with a cloned machine identity" case runs with and without a readable current boot identity. Both fail against the pre-fix shortcut (which returned true from the matching machine id). The wall-clock test now calls reclaimExitedOwner directly so Linux platform emulation does not perturb host publication/fsync flags; it still asserts unverifiable with the owner retained.
  • Test-only harness line — pty-pane-restart-replace.test.ts adds a runDurableMutation fake that forwards to its callback. No production surface.

Verified locally: profile-state-access (32), profile-state-access-identity (5) and pty-pane-restart-replace (6) pass. All prior Pullfrog threads are resolved.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant