feat: persist desktop provider sessions - #49
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds two-launch desktop persistence smoke coverage, isolated Electron userData handling, lifecycle-aware window reopening, provider crash recovery, conditional idle runtime cleanup, and Windows/macOS CI workflow enforcement. ChangesDesktop persistence and runtime recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant SmokeScript
participant Electron
participant SmokeFixture
participant SQLite
CI->>SmokeScript: run persistence smoke
SmokeScript->>SmokeFixture: seed fixture
SmokeScript->>Electron: launch and await readiness
SmokeScript->>SmokeFixture: arm persisted runtime
SmokeScript->>Electron: force-stop process tree
SmokeScript->>SmokeScript: launch second instance
SmokeScript->>SmokeFixture: assert recovered state
SmokeFixture->>SQLite: read durable bindings
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/server/src/provider/providerLifecycleCoordinator.ts (1)
107-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
runConditionalgeneration-preservation logic looks correct. advance() is lazy and rollback only fires after a commit, so a failing operation that never advanced leaves the current generation untouched.Optional: the failure-rollback body (Lines 134-138) duplicates the identical block in
run(Lines 95-99). Consider extracting a smallrestorePreviousGeneration(threadId, previousGeneration)helper to keep the two lifecycle paths in sync.As per coding guidelines: "avoid duplicating logic across files and prefer reusable modules over local shortcuts."
🤖 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 `@apps/server/src/provider/providerLifecycleCoordinator.ts` around lines 107 - 144, Extract the duplicated generation-restoration logic from runConditional and run into a shared restorePreviousGeneration helper, accepting threadId and previousGeneration. Replace both inline rollback blocks with this helper while preserving the existing delete-versus-set behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@apps/server/src/provider/providerLifecycleCoordinator.ts`:
- Around line 107-144: Extract the duplicated generation-restoration logic from
runConditional and run into a shared restorePreviousGeneration helper, accepting
threadId and previousGeneration. Replace both inline rollback blocks with this
helper while preserving the existing delete-versus-set behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d0ee220-9e04-46d3-8294-0cd702163715
📒 Files selected for processing (22)
.github/workflows/ci.ymlapps/desktop/package.jsonapps/desktop/scripts/session-persistence-smoke.mjsapps/desktop/scripts/smoke-test-lifecycle.mjsapps/desktop/scripts/smoke-test.test.mjsapps/desktop/src/desktopUserDataProfile.test.tsapps/desktop/src/desktopUserDataProfile.tsapps/desktop/src/desktopWindowLifecycle.test.tsapps/desktop/src/desktopWindowLifecycle.tsapps/desktop/src/main.tsapps/desktop/turbo.jsoncapps/server/integration/desktopPersistenceSmokeFixture.test.tsapps/server/integration/desktopPersistenceSmokeFixture.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/Layers/ProviderSessionReaper.test.tsapps/server/src/provider/Layers/ProviderSessionReaper.tsapps/server/src/provider/Services/ProviderService.tsapps/server/src/provider/providerLifecycleCoordinator.tspackage.jsonscripts/lib/workflow-contracts.test.tsscripts/lib/workflow-contracts.ts
140956f to
92e1a3b
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
087214b to
dec624a
Compare
1 participant
Summary
Persistence boundary
A normal window close keeps the desktop process, backend, and provider runtimes alive. An explicit quit, update/relaunch, force termination, or reboot reconstructs sessions from durable runtime cursor and payload state on the next launch.
A hard process or power loss cannot preserve an in-memory active process or unfinished turn. Startup recovery deliberately normalizes orphaned starting/running bindings to stopped before cursor-backed resume.
Validation
git diff origin/main --checkpassedThe required formatting, lint, typecheck, platform build, and release-smoke gates are left to the repository's protected CI workflow.
Summary by cubic
Persist provider sessions in the desktop app: keep the backend and provider runtimes warm after the last window closes, and restore them on relaunch. Relaunch reuses the existing backend and focuses an existing window or defers creating a new one until the backend is ready; CI runs a two-launch persistence smoke with isolated
userData, enforces absoluteSYNARA_DESKTOP_PERSISTENCE_SMOKE_USER_DATA, tightens readiness/timeouts and output capture, and confirms full process-tree cleanup.New Features
userData; adds an offline server fixture that seeds/arms durable state backed by production SQLite.Refactors
stopRuntimeSessionIfIdleand generation-aware conditional lifecycle leases to avoid races across foreground/background work.userDataoverride with a clear log prefix; tightened smoke harness isolation/validation; workflow contracts require the CI persistence step and OS-specific homes; improved Windows process-tree confirmation with explicittaskkillrace verification.Written for commit 0185b22. Summary will update on new commits.
Summary by CodeRabbit