fix: defer lifecycle closes during active runs#900
Conversation
📝 WalkthroughWalkthroughThis PR introduces deferred lifecycle closes for local instance disposal. When a run is active and disposal is requested in maintenance mode, the close is deferred until the run completes. A new ChangesDeferred Lifecycle Closes for Local Instance Disposal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
app
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/components/dialog-connect-provider-source.test.ts, packages/app/src/components/dialog-connect-provider.tsx, packages/app/src/components/settings-providers.tsx, packages/app/src/context/global-sync/client-action-source.test.ts, packages/app/src/i18n/en.ts, packages/app/src/i18n/zh.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a deferred disposal mechanism for project instances, ensuring that resources are only cleaned up once active runs are idle. It updates the InstanceStore and Instance classes to support maintenance and force modes, adds tracking for active runs in lifecycle-provenance, and integrates these changes into the server routes and UI toast notifications. The review feedback highlights critical issues regarding state consistency: specifically, the options object is lost during recursive deferred disposal calls, and the internal directories tracking set becomes stale when disposal operations are deferred because it lacks a completion callback to trigger the cleanup. Additionally, the aggregation of disposal results in disposeAllLoadedInstances is noted as lossy, potentially omitting affected directory keys in multi-instance scenarios.
Perf delta summaryComparator: pass
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@packages/opencode/src/project/instance-store.ts`:
- Around line 251-256: The deferred disposal Promise created by
whenAllRunsIdle(...).then(() =>
Effect.runPromise(disposeEntryNow(...))).finally(releaseClose) is
fire-and-forget (void completed) so any rejection from
Effect.runPromise(disposeEntryNow(...)) is swallowed; update the code that
builds completed (the variable created from whenAllRunsIdle, calling
disposeEntryNow and finally releaseClose) to attach a .catch handler that logs
or otherwise handles the error (use the existing logging mechanism or
ctx.logger) and ensure releaseClose still runs; reference whenAllRunsIdle,
disposeEntryNow, releaseClose, completed and ctx.directory when adding the catch
so failures during deferred disposal are not unhandled.
- Around line 280-284: The deferred reload is fire-and-forget and can produce
unhandled rejections; wrap the Promise returned by
whenAllRunsIdle(...).then(...) (the chain invoking
Effect.runPromise(reload(input, reason, { mode: "force"
})).finally(releaseDeferredClose)) with a .catch handler to surface/log errors
and still ensure releaseDeferredClose runs; in other words, attach .catch(...)
to the chain that calls reload (referencing whenAllRunsIdle, reload, and
releaseDeferredClose) so any rejection is handled consistently with the change
you made for disposeEntry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 265b82e7-d186-4443-be40-037c98497e69
⛔ Files ignored due to path filters (1)
packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (15)
packages/app/src/components/dialog-connect-provider-source.test.tspackages/app/src/components/dialog-connect-provider.tsxpackages/app/src/components/settings-providers.tsxpackages/app/src/context/global-sync/client-action-source.test.tspackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/opencode/src/config/config.tspackages/opencode/src/project/instance-runtime.tspackages/opencode/src/project/instance-store.tspackages/opencode/src/project/instance.tspackages/opencode/src/server/instance/global.tspackages/opencode/src/session/lifecycle-provenance.tspackages/opencode/src/session/run-state.tspackages/opencode/test/project/instance-store.test.tspackages/opencode/test/session/run-state.test.ts
1 participant
Summary
Fixes active assistant runs being interrupted by local lifecycle maintenance closes.
disposeAll, config invalidation, reload, dispose) until affected directories are idle.global.disposefrombooleanto a structuredcompleted/deferredresult and updates the generated SDK type.Why
Issue #898 showed that provider connect, config refresh, or instance reload could close
InstanceStatewhile an assistant turn was still running. BecauseSessionRunStatelived under that disposable state, the run finalizer cancelled the active runner and surfaced as misleading failed tool cards.The fix moves the lifecycle decision to the central instance close seam: normal maintenance waits for active runs; force cleanup still works.
Related Issue
Closes #898
Human Review Status
Approved by maintainer
Review Focus
global.disposesemantics are acceptable for provider connect/disconnect and config refresh.Risk Notes
global.disposeresponse shape changed frombooleanto{ status, lifecycleActionID, affectedDirectoryKeys }; generated SDK v2 types were updated.app-shellsnap was run as the available visual smoke target.How To Verify
Screenshots or Recordings
bun run snap app-shellpassed. The changed deferred provider toast branch is state-dependent and covered by source-boundary tests rather than a dedicated snap fixture in this PR.Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
New Features
Refactor
Tests