Uh oh!
There was an error while loading. Please reload this page.
Preserve provider bindings when stopping sessions - #2125
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:560da15384
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const reaped = yield* providerService | ||
| .stopSession({ | ||
| threadId: binding.threadId, | ||
| preserveBinding: true, |
There was a problem hiding this comment.
Calling providerService.stopSession with preserveBinding: true during idle reaping leaves a persisted resume binding even after the runtime session is closed, but processSessionStopRequested only performs a destructive provider stop when thread.session.status !== "stopped" (apps/server/src/orchestration/Layers/ProviderCommandReactor.ts:714-716). In the common reaped state (session already marked stopped), an explicit thread.session.stop command no longer removes that binding, so a later turn can still resume via the persisted cursor (ProviderService.startSession reuses persisted binding state). This makes explicit stop non-destructive for already-reaped threads and breaks the stated manual-stop semantics for that scenario.
Useful? React with 👍 / 👎.
ApprovabilityVerdict: Needs human review This PR changes session lifecycle behavior by removing the ability to delete provider bindings on stop. Two unresolved review comments identify that the implementation contradicts the stated intent - manual stops now preserve bindings when they should be destructive, allowing unintended session resumption. You can customize Macroscope's approvability policy. Learn more. |
juliusmarminge
commented
Apr 17, 2026
reading this now i don't remember why we ever remove the provider binding 🤔 |
- Keep persisted provider bindings on stop/reap - Remove obsolete `preserveBinding` stop flag Co-authored-by: codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 938a082. Configure here.
| runtimePayload: { | ||
| activeTurnId: null, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Manual stop preserves binding, contradicting stated destructive intent
High Severity
stopSession now always upserts the binding with status: "stopped", but the PR intent is to only preserve bindings for idle-reaper stops while keeping manual/explicit stops destructive. The preserveBinding conditional mentioned in the PR description was never implemented — ProviderStopSessionInput still only has threadId, and the remove method was completely deleted from ProviderSessionDirectory. After a manual stop, the next sendTurn will recover the same provider thread via the persisted resumeCursor instead of starting fresh.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 938a082. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
- Keep an existing thread bound to its provider after reap - Add regression coverage for first-turn and post-bind provider changes
Uh oh!
There was an error while loading. Please reload this page.
The `remove` method was removed from `ProviderSessionDirectoryShape` in pingdotgg#2125 (Preserve provider bindings when stopping sessions). The test mock still referenced it, causing a typecheck failure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Brings in upstream changes since d22c6f5: - Claude Opus 4.5 built-in model (pingdotgg#2143) - Claude Opus 4.7 built-in model (pingdotgg#2072) - opencode provider support (pingdotgg#1758) - Claude process leak fix & session reaper (pingdotgg#2042) - Configurable project grouping (pingdotgg#2055) - Node-native TypeScript for desktop/server (pingdotgg#2098) - Clean up invalid pending approval projections (pingdotgg#2106) - Thread status in command palette (pingdotgg#2107) - Backend startup readiness coordination (pingdotgg#2133) - Preserve provider bindings on session stop (pingdotgg#2125) - Allow deleting non-empty projects (pingdotgg#1264) - Various Windows fixes and CI improvements Conflict resolutions: - model.ts: kept claudeAgent opus-4-7 default, added opencode entry - settings.ts: adopted upstream sidebarProjectGroupingMode/Overrides - ClaudeProvider.ts: adopted upstream Opus 4.7 effort config - Sidebar.tsx: adopted upstream project grouping approach - package.json: kept custom desktop-only build filter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lastobelus
commented
Apr 21, 2026
ouch, this caused a lot of pain in the last few days. I have to go apologize to several codex threads. |
…orphaned-session stop **1. One card per file for multi-file patches** Multi-file tool calls (apply_patch with N file markers, Claude MultiEdit, etc.) previously rendered as a single collapsed "N files changed — Show full diff" card. That UX forced the user to expand the aggregate just to see any one file's diff. `MessagesTimeline.logic.ts` now splits a file-change entry with N `diffPreviews` hunks into N individual file-change rows. Each row reuses the existing FileChangeCard single-hunk render path, so users see inline diff previews for every file without expanding. Row ids are suffixed with `#<index>` to keep each row's identity stable across updates. **2. Stop button no longer deadlocks on orphaned provider sessions** User hit this repro: thread shows "Working" for hours, clicking the red stop button emits `ProviderValidationError: Cannot recover thread '<id>' because no provider resume state is persisted` into the logs, and the UI stays stuck. Root cause: - MarCode's `stopSession` deletes the provider binding (commit 8940163, diverges from upstream pingdotgg#2125) — so after a crash/restart the binding can exist with status="stopped" or with resumeCursor cleared. - `ProviderService.interruptTurn` used `allowRecovery: true`, which tried to RESURRECT the dead session via `recoverSessionForThread` just to kill it. If resume state is missing → validation error bubbles out → interrupt fails → thread.session.status stays "running" → UI stays "Working". Fixes in two layers: - `ProviderService.interruptTurn` now uses `allowRecovery: false` and skips the adapter call when `isActive === false`. You can't interrupt a session that doesn't exist; there's nothing to kill. - `ProviderCommandReactor.processTurnInterruptRequested` wraps the provider interrupt in `Effect.catchCause`. If the provider call fails (or, now more rarely, the adapter is no-op because recovery is off), the reactor dispatches a `thread.session.set` with status="interrupted" and `activeTurnId: null`, so the UI unblocks from "Working" regardless of what the provider does. Also appends a visible failure activity explaining "session was orphaned; force-stopped. Send a new message to resume." **Verification** - Typecheck: all 9 packages clean. - @marcode/web: 1074 tests pass (incl. 14 MessagesTimeline.logic). - @marcode/server: 995 pass, 5 skipped (incl. 24 ProviderCommandReactor, 26 ProviderService). - Browser: 161 pass, 6 skipped.
Co-authored-by: Julius Marminge <julius0216@outlook.com> Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com> Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com> Co-authored-by: codex <codex@users.noreply.github.com>
Closes#2121
Summary
Why
Idle reaping currently uses the same destructive stop path as an explicit manual stop. For the Codex provider, that drops the resume cursor and forces the next turn onto a fresh Codex thread instead of resuming the previous provider thread.
This change keeps the resume binding for idle cleanup only, so a later send can recover the same provider thread while leaving explicit stop semantics unchanged.
Verification
bun fmtbun x vitest run apps/server/src/provider/Layers/ProviderService.test.ts apps/server/src/provider/Layers/ProviderSessionReaper.test.tsbun lintbun typecheckNotes
bun lintpasses with existing repo warnings outside this patchNote
Medium Risk
Changes provider session lifecycle/persistence semantics (stop no longer unbinds), which can affect routing and recovery paths across providers and restarts; coverage is improved but behavior changes are system-wide.
Overview
Provider session stopping is changed from a destructive unbind to a soft stop:
ProviderServiceLive.stopSessionnowupserts the thread binding withstatus: "stopped"(and clearsactiveTurnId) instead of deleting it, keeping the persisted provider + resume cursor so a latersendTurncan recover/resume the same provider thread.ProviderSessionDirectoryremoves theremove(threadId)API entirely, and tests are updated/added to verify stopped bindings remain persisted and that routing can resume after a stop.ProviderCommandReactorsession restart logic is tightened to always prefer the thread-bound provider and to only dropresumeCursoron model-change restarts, with test reordering/adjustments around restart-failure and provider-switch rejection.Reviewed by Cursor Bugbot for commit b33fd1f. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Preserve provider bindings when stopping sessions instead of deleting them
ProviderSessionDirectoryremoves theremovemethod; stopped sessions are now persisted withstatus: 'stopped'andactiveTurnId: nullrather than deleted from the directory.ProviderService.stopSessionupserts the binding on stop, so a subsequentsendTurncan resume from the correctresumeCursorand provider without re-binding from scratch.ProviderCommandReactorno longer restarts a session solely due to a provider change; it always uses the thread's bound provider and preservesresumeCursorunless a runtime mode or model-change condition requires a restart.Macroscope summarized b33fd1f.