fix(restore): simplify persistence and harden cross-platform cleanup#602
Conversation
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29415142365 Artifacts expire in 7 days.
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29416879658 Artifacts expire in 7 days.
|
Gatekeeper review: BLOCKReviewed the entire PR #602 at head Blocking findings
Required before PASS
Validation observed
|
Gatekeeper review: BLOCKThis review supersedes the earlier gatekeeper report and evaluates the latest PR state at Blocking findings
Required implementation direction
Validation status
Passing tests/builds do not cover the interleavings above. The PR should not merge until these blockers are fixed and regression tests exercise the exact races. |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29451033896 Artifacts expire in 7 days.
|
Ponytail review: CHANGES REQUIREDReviewed the entire PR at Merge blockers
Smallest better implementationThe cross-host protocol is doing more work than necessary. Keep both existing host-local elections, but replace the new mutex/claim sequence with one complete temporary ownership directory atomically renamed to the fixed owner path. Publish participants atomically, never delete the owner during ordinary primary release, and retire a stale owner directory only after identity validation and confirming no live participants. The owner UUID itself can name participants/retirements. This removes Optional deletions
Required focused tests
Validation observed
The green suites do not cover the crash/interleaving cases above. The PR should not merge yet. |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29454203385 Artifacts expire in 7 days.
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29459180943 Artifacts expire in 7 days.
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29483622027 Artifacts expire in 7 days.
|
Gatekeeper review: CHANGES REQUIREDReview of the full 1. High: simultaneous stale-owner recovery can leave every host permanently secondaryBoth hosts publish their participant before attempting stale-owner retirement. Retirement is then refused whenever another live participant exists. If Electron and Tauri start together after a primary crash, each sees the other participant, both refuse retirement, and neither retries or promotes later.
Impact: both clients skip restoration and reject all shared-state writes for their lifetime. Add a barrier-controlled stale-owner test where both hosts publish before retirement and assert exactly one primary. 2. High: an outgoing Tauri renderer can commit after authority rotatesThe Tauri commands validate the renderer token before entering
Reproduction: claim token A, block an A save before replacement, claim token B during navigation, then release A. A can still replace or clear B's state. Fence mutations with an access generation revalidated under the write lock and immediately before replacement; add deterministic save/clear versus token-rotation tests. 3. High: a non-authoritative workspace stop can erase a restored tab and unsent draft
After a successful restore, type a draft, let it persist, then let the workspace process stop unexpectedly. The next capture overwrites the prior snapshot without that workspace or draft. Add coverage for a settled restored result followed by a non-authoritative stop. 4. High: delayed HTTP message hydration can roll back newer SSE stateMessage-load epochs protect deletion, eviction, and competing HTTP loads, but SSE message/part updates do not advance the epoch. A delayed HTTP response therefore remains current and
Interleave a pending 5. High: concurrent Electron restarts can spawn an untracked CLI server
Serialize/coalesce start, stop, and restart, and reject stale starts after shutdown begins. Add concurrent restart and restart-versus-quit tests. 6. High: Tauri Windows session-end can force-exit before native state is flushed
A slow CLI stop can therefore guarantee that the internal forced exit occurs before the final native snapshot/window write. Flush and release client state before waiting for CLI shutdown, and cover a blocked CLI plus dirty state. 7. Medium: shared window bounds mix inner and outer dimensionsTauri stores
Define one shared geometry contract and add a cross-host round-trip test, including non-1.0 DPI. Coverage gateThe current PR checks package successfully, but do not execute the modified Electron native, UI Node, server lifecycle, or Rust regression suites. Add required CI jobs for these paths; compilation alone does not exercise the races above. Verdict: request changes. The shared-state and lifecycle paths can still lose restore data, suppress persistence for an entire run, or leave an unmanaged server. |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29497579789 Artifacts expire in 7 days.
|
|
Consolidated gatekeeper review: CHANGES REQUIRED This review supersedes the measured and exhaustive reviews and removes their duplicates. Scope: full Consensus findings independently confirmed by both reviews
Additional High findings from the exhaustive review
Additional Medium findings
Validation Electron native 92 passed; Tauri Rust 65 passed; focused server suites up to 64 passed; focused clean-head UI codec/merge 50 passed with 2 skipped; typechecks, Recursive review budget 1 root orchestrator, 10 direct leads, 90 descendants. The host plugin reached Final decision: CHANGES REQUIRED |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29514984984 Artifacts expire in 7 days.
|
Restore follow-up: contingency diagnostic ledgerThis note preserves verified alternatives from the recursive investigation in case the Windows artifact at Reproduced primary failure
Verified fallback risks
Missing regression coverage
Validation
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29531269244 Artifacts expire in 7 days.
|
|
Temporary test-only commit: |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29533701164 Artifacts expire in 7 days.
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29534477076 Artifacts expire in 7 days.
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29537850487 Artifacts expire in 7 days.
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29539162767 Artifacts expire in 7 days.
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29540132194 Artifacts expire in 7 days.
|
Consolidate the desktop restore lifecycle and its regression coverage while preserving primary-process ownership, renderer authorization, active drafts, attachments, session tombstones, and cancellation semantics. Bound workspace launches end to end, retain cleanup ownership after unexpected exits, and use identity- and token-guarded portable POSIX process-group cleanup. This avoids the macOS all-process ps timeout without introducing an unverified PID fallback, with real Darwin integration tests for CI. Harden packaged Electron origins, transactional Tauri navigation authority, and bounded Windows session-end shutdown. Validation covers Electron, Tauri, UI, and server focused suites plus all TypeScript typechecks.
Restore workspace tabs without treating transient startup stops as user removals, hydrate saved session ancestry and metadata across worktree workspaces, and preserve scroll snapshots through cache eviction and late native seeding. Keep Electron zoom consistent for native menu, keyboard, and wheel input across navigation and restart. Bound desktop shutdown, start CLI termination alongside state flushing, and use verified Windows process-tree escalation to avoid minute-long exits. Treat Electron as a secondary client when a live Tauri marker already owns restoration, while retaining an independent CLI and manual workspace creation. Cancellation ownership is request-scoped so reused workspace launches cannot remove or retain the wrong instance. Validated with UI and Electron typechecks, server typecheck, 67 Electron native tests, 54 focused restore tests, independent targeted reviews, and successful server/UI/Electron builds.
Prevent late restore completion and cleanup races from resurrecting explicitly closed workspaces. Preserve message scroll while invalidating evicted session hydration, retain scoped request correlation for shared launches, and retry failed cancellation cleanup safely. Add confirmed graceful desktop shutdown with concurrent workspace cleanup and final process-tree enforcement. Introduce a shared Electron/Tauri cross-host election with atomic publication, identity-guarded stale recovery, cohort participants, legacy marker checks, and Node/Rust interoperability coverage. Validated with focused UI, server, Electron, and Tauri suites; UI/Electron/server typechecks; cargo check and focused rustfmt; and UI, Electron, and server production builds.
Guard asynchronous session search, parent hydration, and message loading with deletion authority and per-session request epochs so stale responses cannot resurrect deleted or evicted state. Make Electron shutdown identity-safe and bounded: do not veto or force-exit Windows session end, require the graceful completion handshake, capture immutable descendant identities, reject PID reuse, and confirm process disappearance after enforcement. Serialize Electron/Tauri cross-host owner and participant transitions with a shared atomic lock so simultaneous startup elects one primary and participant publication cannot cross cohort release. Validated with Electron native tests, focused UI and server suites, Tauri client-state tests, UI/Electron/server typechecks, cargo check, and focused Rust formatting.
Replace the cross-host mutex and claim protocol with atomically published owner directories and participants. This removes crash-wedged intermediate states, preserves live cohorts, handles upgraded and legacy markers, and drops the sha2 dependency. Keep workspace cancellations until their matching creation settles and retry pre-response restore cancellation without relying on SSE delivery. Inline one-use cache eviction logic and remove unused aliases. Delete the unreachable Electron utility-process supervisor path. The existing identity-guarded shutdown remains; Windows crash-only descendant containment is intentionally left as a documented residual rather than adding a PowerShell Job Object supervisor. Validated with Electron native tests and build, focused UI/server suites, Tauri client-state tests, all affected typechecks, cargo check, and focused Rust formatting.
Persist Electron and Tauri restore data in one cross-host envelope, migrate legacy host files deterministically, and prevent cleared state from being resurrected. Keep atomic replacements on the destination filesystem and normalize Tauri window bounds for HiDPI handoffs. Delay capture until native flush listeners are ready, preserve the saved active tab and session IDs during startup, accept legacy unversioned snapshots, and hydrate incomplete session metadata without overwriting newer updates. Validated with 89 Electron native tests, 69 focused UI tests, 36 Tauri client-state tests, UI/Electron typechecks, cargo check, rustfmt, and the Electron production build.
Recover stale cross-host ownership deterministically when Electron and Tauri start together, and fence Tauri renderer mutations against authority rotation through the final atomic replacement. Preserve restored tabs and unsent drafts after non-authoritative workspace stops, reject stale HTTP message hydration after newer SSE updates, and serialize Electron CLI lifecycle operations so restart cannot race shutdown or spawn an untracked server. Flush Tauri client state before stopping the CLI, align shared window geometry on outer position plus content size, and discard incompatible legacy native bounds during migration. Validated with 92 Electron native tests, 71 focused UI tests, 38 Tauri client-state tests, 7 Tauri shutdown tests, all affected typechecks, cargo check, focused rustfmt, and the Electron production build.
Preserve inactive workspace identities, selected drafts, attachments, and late prompt hydration so Electron and Tauri restore every tab instead of only the globally active one. Fence renderer and message hydration races, harden cross-host stale-owner recovery, serialize shutdown after state capture, and make Windows process cleanup use consistent identities and refreshed descendant trees. Distinguish explicit workspace deletion from transient stops, prevent request ownership reactivation, and retire migrated legacy state without shadowing future envelopes. Add PR behavioral checks and regression coverage across UI persistence, Electron lifecycle/process cleanup, server workspace shutdown, and Tauri client state. Validated with 93 Electron tests, 53 focused UI tests, 23 server tests, 65 Rust tests, all TypeScript typechecks, cargo check, targeted rustfmt, and the production UI build.
Continue restoring saved workspace state when the initial session-list request fails transiently. The direct saved-session hydration path can recover after reconnect, preserving selection, runtime markers, scroll position, and the original tab order instead of appending the unresolved workspace after the active tab. This behavior is shared by Electron and Tauri. Validated with the UI typecheck, production build, 65 restoration and reconciliation tests, and 13 instance runtime authority tests.
Start applying the loaded snapshot immediately, matching the Tauri behavior before desktop state was shared with Electron. Flush listeners are still registered eagerly, but their asynchronous setup no longer delays tab ordering and session hydration behind workspace startup. The focused source regression check now verifies both listeners remain registered without blocking restoration. UI typecheck and the targeted test pass.
This reverts commit 532aefa.
Restore-created workspaces previously mounted in parallel. Solid could invoke Virtua refs before document adoption, causing ResizeObserver initialization to throw after the workspace entered the UI but before its restore binding committed. The affected tabs then lost saved ordering, session selection, idle markers, scroll snapshots, and generation recovery. Create missing workspaces sequentially while preserving parallel hydration for existing workspaces and SideCars. Budget the startup deadline per serialized workspace and bound failed-creation cleanup so one stalled cancellation cannot block later tabs. Validated with a packaged Tauri release against a three-workspace snapshot, confirming exact tab order, active tab, per-workspace active sessions, idle markers, scroll snapshot counts, and generation recovery. Added focused regression coverage and ran UI typecheck and restore tests.
New Electron, Tauri, and UI client state now starts with restoration disabled while preserving an explicitly persisted enabled preference. Parsing malformed or incomplete envelopes follows the same safe default across desktop hosts. Tests that exercise persistence explicitly enable restoration, and the rebased interruption registry uses its public session-id view instead of stale internal maps. Validation: UI and Electron typecheck passed; focused Rust tests reached 36/38 before the final fixture-only correction.
71ebdd3 to
f0a9fc3
Compare
Move the server cancellation retry into a UI-independent module so the regression test can run under Node without loading Solid client-only notification code. The instance store keeps correlation cleanup while delegating the retry itself to the tested helper. Validation: typecheck passed; expanded UI restoration suite passed 84 tests with one pre-existing skip.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29573189195 Artifacts expire in 7 days.
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29574059387 Artifacts expire in 7 days.
|
Do not mount the project-opening home while a saved session with tabs is still restoring. This removes the timing-dependent home flash whenever the runtime tab list is temporarily empty. Persist an explicit homeActive marker so closing from the project home remains restorable without treating transient empty-tab gaps as user intent. Validation: desktop typecheck passed; expanded UI restoration suite passed 86 tests with one pre-existing skip.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29578576318 Artifacts expire in 7 days.
|
There was a problem hiding this comment.
Gatekeeper review
-
P1, blocking - packages/ui/src/App.tsx:633-643, packages/ui/src/lib/hooks/use-app-session-restore.ts:232
Restoring homeActive=true while appTabs() is still empty mounts two FolderSelectionView instances: the empty-app home and the full-screen home overlay. Both register global keyboard/drop handlers, so one drop can trigger duplicate workspace creation. Only one home instance should mount while the tab list is empty.
-
P3 - .github/workflows/pr-build.yml:88-93
app-session-restore-gate.test.ts is not included in the workflow explicit UI test list, so this regression check does not run in PR CI.
The optional homeActive codec change is backward-compatible. Focused restoration tests and desktop typecheck pass.
Keep the empty-state home and the tab overlay mutually exclusive while restoring an explicitly saved home view. This prevents duplicate global keyboard and drop handlers from issuing repeated workspace actions. Run the restore-gate regression test in PR CI so the startup rendering contract remains covered. Validation: desktop typecheck passed; focused restoration suite passed 56 tests.
|
Gatekeeper follow-up: blocking duplicate-home finding fixed in d57449b. Empty-state home and tab overlay are now mutually exclusive, and the restore-gate regression test is included in PR CI. Validation: desktop typecheck and 56 focused restoration tests pass. |
|
@shantur LGTM |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29580381412 Artifacts expire in 7 days.
|
Create missing workspaces concurrently while serializing UI commits so restored tabs keep their saved order without paying the network latency sequentially. Fence stale workspace-list and lifecycle events, preserve complete reopen state, and harden Electron, Tauri, and server process containment so cancellation and shutdown cannot leak or revive workspaces. Require the expanded UI, server, Electron, Linux Rust, and Windows Rust checks before artifact publication. Validated with TypeScript typecheck, focused restore and shutdown suites, 99 Electron tests, and 76 Rust tests.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29591185038 Artifacts expire in 7 days. |
Keep ordinary workspace creation outside restore cancellation ownership and retire graceful cross-host ownership cohorts atomically so surviving secondary hosts cannot block later persistence failover. Make shutdown status terminal, preserve the Tauri Windows graceful control channel, and move Electron tree enforcement off the main thread with handle-bound Windows identity checks and hard end-to-end deadlines. Windows session end now fails open only after a bounded primary release attempt. Prepare clean Tauri test resources on Linux and Windows, run Windows PATH behavior on Windows, and make the Linux runtime harness observe exited processes. Regression coverage exercises ownership, repeated handoffs, PID reuse, malformed snapshots, hung enforcement, and session-end deadlines.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29599169737 Artifacts expire in 7 days. |
Pin the simulated runtime platform and return valid unrelated process snapshots after the owned child exits so Linux can prove cleanup instead of waiting on an invalid empty probe. Align PID-reuse expectations with identity-safe cleanup and use a host-valid folder for simulated Windows launches. Verified with the runtime suite under both Linux Node in WSL and native Windows.
|
@shantur LGTM |
Normalize the native GetProcessTimes value to the microsecond precision exposed by CIM before comparing it with the captured creation identity. This keeps handle-bound PID reuse protection strict while allowing owned CLI processes to be terminated. Add real Windows child-process coverage proving valid CIM identities terminate and sub-microsecond mismatches remain protected. Electron typecheck and all 117 native tests pass.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29603542842 Artifacts expire in 7 days.
|
Enable restore for new or missing desktop preferences in both Electron and Tauri while preserving an explicitly disabled setting and keeping unsupported future state envelopes protected. Clarify the localized startup setting so users know that unsent messages, window position, and zoom are restored. Add focused coverage for the new default and explicit opt-out behavior. Validation: rustfmt --check passed for the touched Rust files; git diff --check passed. Full test execution was skipped at user request.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29608098000 Artifacts expire in 7 days.
|
2 participants
Summary
Platform hardening
Scope
Validation