fix(session): clear stale pending interactions - #748
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR addresses the bug where session-scoped pending interactions (questions, permissions, blockers) outlive their owning sessions, causing repeated 404 errors during app bootstrap. The fix implements cleanup across the session lifecycle, adds filtering to route endpoints, and hardens bootstrap against missing sessions. ChangesDangling session cleanup and lifecycle integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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
harness
![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/context/global-sync/bootstrap.test.ts, packages/app/src/context/global-sync/bootstrap.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.
Perf delta summaryComparator: pass
|
![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 cleanup mechanism for pending interactions—including permissions, questions, and blockers—to ensure they are terminated when sessions are deleted, archived, or identified as dangling. Key changes include the addition of a clearSession method to core services, the implementation of a SessionLiveness utility for session validation, and updates to the error middleware to suppress logging for NotFoundError. The reviewer recommended refactoring the liveness-based pruning logic into a centralized pruneDangling helper within the SessionLiveness namespace to reduce code duplication and maintain consistency across the API routes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/opencode/src/server/instance/question.ts (1)
87-100: ⚡ Quick winPrefer
Effect.genas the top-level composition in this route flow.This works, but the new route logic is composed via outer
.pipe(Effect.flatMap(...))instead of a singleEffect.gen(...)block, which drifts from repo convention.As per coding guidelines:
Use Effect.gen(function* () { ... }) for Effect composition.🤖 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 `@packages/opencode/src/server/instance/question.ts` around lines 87 - 100, The current route composes effects using svc.list().pipe(Effect.flatMap(...)) instead of using a single Effect.gen; refactor the block to make Effect.gen the top-level composition: call Effect.gen(function* () { const items = yield* svc.list(); const active = SessionLiveness.activeSessionIDs(items.map(i => i.sessionID)); for (const sessionID of items.filter(i => !active.has(i.sessionID)).map(i => i.sessionID)) { yield* svc.clearSession(sessionID, "dangling_session"); } return items.filter(i => active.has(i.sessionID)); }) so you keep the same logic but replace the outer Effect.flatMap(...) with one Effect.gen(...) using svc.list, SessionLiveness.activeSessionIDs, and svc.clearSession.packages/opencode/src/server/instance/permission.ts (1)
110-123: ⚡ Quick winUse a single
Effect.gencomposition here to match project Effect style.The route logic is currently structured as
.pipe(Effect.flatMap(...)); please collapse this intoEffect.gen(...)composition for consistency with the repo rule.As per coding guidelines:
Use Effect.gen(function* () { ... }) for Effect composition.🤖 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 `@packages/opencode/src/server/instance/permission.ts` around lines 110 - 123, Replace the current svc.list().pipe(Effect.flatMap(...)) composition with a single Effect.gen generator: yield the list with const items = yield* svc.list(), compute active via SessionLiveness.activeSessionIDs(items.map(...)), build inactiveSessionIDs, loop over them and yield* svc.clearSession(sessionID, "dangling_session") for each, and finally return items.filter(item => active.has(item.sessionID)); this keeps the same behavior but uses Effect.gen for all Effect composition instead of Effect.flatMap.packages/opencode/src/server/instance/blocker.ts (1)
43-56: ⚡ Quick winAlign route composition to top-level
Effect.geninstead of.pipe(Effect.flatMap(...)).Behavior is fine, but this new block should follow the repo’s preferred Effect composition style.
As per coding guidelines:
Use Effect.gen(function* () { ... }) for Effect composition.🤖 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 `@packages/opencode/src/server/instance/blocker.ts` around lines 43 - 56, Replace the current .pipe(Effect.flatMap(...)) composition with a top-level Effect.gen: call svc.list() inside an Effect.gen(function* () { ... }) and perform the session liveness calculation and loop there (use SessionLiveness.activeSessionIDs on the yielded items), call svc.clearSession(sessionID, "dangling_session") for each inactive sessionID, and return the filtered active items; specifically remove the use of Effect.flatMap and move the logic that builds inactiveSessionIDs, iterates sessionID, and invokes svc.clearSession into the body of the top-level Effect.gen so the effect composition uses Effect.gen rather than .pipe(Effect.flatMap(...)).
🤖 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/test/question/question.test.ts`:
- Around line 184-186: The immediate assertion on blocker registration is racy:
after await waitForPending() the blocker created by the question may still be
one tick behind, so change the direct expect(await
listBlockers()).toHaveLength(1) into a wait-based assertion that polls
listBlockers until it returns length 1 (with a short timeout). Locate the code
using waitForPending() and listBlockers() in the test and replace the direct
expect with a retry/wait loop (or call your test-suite's waitFor helper) that
repeatedly calls listBlockers() and asserts length === 1 before failing.
In `@packages/opencode/test/server/middleware.test.ts`:
- Around line 71-102: Tests in middleware.test.ts currently assert against the
entire log file (using readLogFile()) which can include unrelated pre-existing
lines; change both tests to capture the log contents immediately after
Log.init() (or record a baseline via readLogFile()) and then read the log file
again after the request, compute the delta/new lines produced by the request,
and run the expect assertions against that delta only (referencing
readLogFile(), Log.init(), ErrorMiddleware and the "/missing" and "/boom"
request flows) so the assertions only consider request-local log output.
---
Nitpick comments:
In `@packages/opencode/src/server/instance/blocker.ts`:
- Around line 43-56: Replace the current .pipe(Effect.flatMap(...)) composition
with a top-level Effect.gen: call svc.list() inside an Effect.gen(function* () {
... }) and perform the session liveness calculation and loop there (use
SessionLiveness.activeSessionIDs on the yielded items), call
svc.clearSession(sessionID, "dangling_session") for each inactive sessionID, and
return the filtered active items; specifically remove the use of Effect.flatMap
and move the logic that builds inactiveSessionIDs, iterates sessionID, and
invokes svc.clearSession into the body of the top-level Effect.gen so the effect
composition uses Effect.gen rather than .pipe(Effect.flatMap(...)).
In `@packages/opencode/src/server/instance/permission.ts`:
- Around line 110-123: Replace the current svc.list().pipe(Effect.flatMap(...))
composition with a single Effect.gen generator: yield the list with const items
= yield* svc.list(), compute active via
SessionLiveness.activeSessionIDs(items.map(...)), build inactiveSessionIDs, loop
over them and yield* svc.clearSession(sessionID, "dangling_session") for each,
and finally return items.filter(item => active.has(item.sessionID)); this keeps
the same behavior but uses Effect.gen for all Effect composition instead of
Effect.flatMap.
In `@packages/opencode/src/server/instance/question.ts`:
- Around line 87-100: The current route composes effects using
svc.list().pipe(Effect.flatMap(...)) instead of using a single Effect.gen;
refactor the block to make Effect.gen the top-level composition: call
Effect.gen(function* () { const items = yield* svc.list(); const active =
SessionLiveness.activeSessionIDs(items.map(i => i.sessionID)); for (const
sessionID of items.filter(i => !active.has(i.sessionID)).map(i => i.sessionID))
{ yield* svc.clearSession(sessionID, "dangling_session"); } return
items.filter(i => active.has(i.sessionID)); }) so you keep the same logic but
replace the outer Effect.flatMap(...) with one Effect.gen(...) using svc.list,
SessionLiveness.activeSessionIDs, and svc.clearSession.
🪄 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: 66c1910f-0784-42f8-9bf7-341f75aa9c22
📒 Files selected for processing (16)
packages/app/src/context/global-sync/bootstrap.test.tspackages/app/src/context/global-sync/bootstrap.tspackages/opencode/src/permission/index.tspackages/opencode/src/question/index.tspackages/opencode/src/server/instance/blocker.tspackages/opencode/src/server/instance/permission.tspackages/opencode/src/server/instance/question.tspackages/opencode/src/server/middleware.tspackages/opencode/src/session/blocker.tspackages/opencode/src/session/liveness.tspackages/opencode/src/session/session.tspackages/opencode/test/permission-cleanup.test.tspackages/opencode/test/question/question.test.tspackages/opencode/test/server/middleware.test.tspackages/opencode/test/server/pending-interaction-routes.test.tspackages/opencode/test/session/pending-interaction-lifecycle.test.ts
github-actions
Bot
added
ci
Astro-Han
removed
the
task
github-actions
Bot
added
the
task
11 tasks
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Astro-Han
added a commit
that referenced
this pull request
May 19, 2026
* docs(github): tighten PR and task templates against agent misuse PR #748 surfaced a template ambiguity: the labeler bot auto-applied routing/priority labels but not the required type label, the agent rewrote the checklist text to "Label bot should apply type/routing/ priority labels; no manual labels were specified per maintainer instruction" instead of adding the missing `bug` label, and the human maintainer had to add `bug` minutes later before validation ran. Five rounds of crosscheck (Claude Opus + Codex) found and addressed: - Label checklist conflated bot-applied vs author-applied labels and had an "or I requested maintainer labeling" escape hatch. - All checklist items were first-person past-tense assertions with no explicit immutability guard; the agent treated the lines as editable. - Human Review Status was a free-text contract with a loose "or not required" escape hatch. - How To Verify example block had no machine-spottable marker; an agent could leave the example verbatim. - Task template Execution mode options had overlapping prose and uneven parallelism. The new PR template splits labels into three explicit author/bot rows, adds a top-of-file policy comment plus a "How to use this checklist" blockquote, converts Human Review Status into a strict three-option enum, adds a `replace-before-submit` HTML sentinel to the example block, and tags genuinely conditional items with **(conditional)**. The new Task template Execution mode rewrites each option as an explicit declarative directive ("the agent must not... until..." / "the agent must post the plan as an issue comment..." / "the agent makes the requested changes... must not push directly to dev"). Round-5 crosscheck returned "None" for both code findings and design alternatives from both reviewers. Remaining design-level work (CI lint enforcement, issue-template `area` dropdown restructure) is deferred to a follow-up PR. * ci(labeler): route all .github/ changes to ci, not only workflows Templates and issue forms under .github/ITS_TEMPLATE/ and the top-level PR template are CI/process infrastructure that should route the same way as workflow files. Without this, a PR that only touches .github/pull_request_template.md or .github/ISSUE_TEMPLATE/*.yml receives no routing label from the labeler bot, and the manual override is stripped by sync-labels on the next pr-triage run. The task rule keeps the narrower .github/workflows/** scope: task is a type label, and per the updated PR template, type labels are author-applied, not bot-applied. Workflow PRs retain the existing task auto-application as a historical convenience; this can be revisited in a follow-up if the inconsistency proves confusing. * ci(labeler): keep .github/workflows/** explicit in ci rule Restore the explicit .github/workflows/** glob alongside the broader .github/** glob. The pr-triage contract test at packages/opencode/test/github/pr-triage-workflow.test.ts:57 uses literal-string matching to assert that workflow files are routed to the ci label, so dropping the workflows glob broke the test even though .github/** subsumes it semantically. Keeping both globs preserves the test contract and documents the intent that workflow files are first-class CI infra while other .github/ files (templates, labeler.yml itself, dependabot config) are CI process.
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NotFoundErrorresponses as serverERROR failedentries.Why
Installed builds could surface stale in-memory pending interaction state for a session that no longer exists, causing repeated session warm-up 404s and uncaught promise errors during bootstrap. The backend now owns the lifecycle invariant, while the app bootstrap keeps a narrow race fallback.
Related Issue
Closes #744
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
list()remains raw for tests/internal use, while HTTP list routes only return active non-archived sessions.Risk Notes
How To Verify
Screenshots or Recordings
Not required; this is backend/bootstrap lifecycle behavior with no visible UI surface change.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
New Features
Bug Fixes
Tests