fix(provider): list only committed sessions#4191
Conversation
github-actions
Bot
added
vouch:unvouched
|
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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const commitMcpSession = Effect.fn("ProviderService.commitMcpSession")(function* ( | ||
| prepared: PreparedMcpSession, | ||
| ) { | ||
| if ( | ||
| prepared.previous && | ||
| prepared.previous.providerSessionId !== prepared.current?.providerSessionId | ||
| ) { | ||
| yield* McpSessionRegistry.revokeActiveMcpProviderSession(prepared.previous.providerSessionId); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🟡 Medium Layers/ProviderService.ts:257
When issueUncommittedMcpCredential returns undefined, prepareMcpSession keeps the previous MCP config installed (current is undefined), but commitMcpSession then revokes prepared.previous.providerSessionId because prepared.current?.providerSessionId is also undefined. A successful session start or recovery therefore leaves McpProviderSession pointing at a credential that has already been revoked, breaking subsequent MCP access whenever credential issuance is unavailable. The guard in commitMcpSession should skip revocation when current is undefined, not just when the session ids differ.
const commitMcpSession = Effect.fn("ProviderService.commitMcpSession")(function* (
prepared: PreparedMcpSession,
) {
- if (
- prepared.previous &&
- prepared.previous.providerSessionId !== prepared.current?.providerSessionId
- ) {
+ if (prepared.current && prepared.previous &&
+ prepared.previous.providerSessionId !== prepared.current.providerSessionId
+ ) {
yield* McpSessionRegistry.revokeActiveMcpProviderSession(prepared.previous.providerSessionId);
}
});🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/ProviderService.ts around lines 257-266:
When `issueUncommittedMcpCredential` returns `undefined`, `prepareMcpSession` keeps the previous MCP config installed (`current` is `undefined`), but `commitMcpSession` then revokes `prepared.previous.providerSessionId` because `prepared.current?.providerSessionId` is also `undefined`. A successful session start or recovery therefore leaves `McpProviderSession` pointing at a credential that has already been revoked, breaking subsequent MCP access whenever credential issuance is unavailable. The guard in `commitMcpSession` should skip revocation when `current` is `undefined`, not just when the session ids differ.
There was a problem hiding this comment.
same as the thread on #4190, fixed here in 91f004ef8.
the adapter now sees no stale config when credential issuance is unavailable, and rollback still restores the previous config on failure.
| "provider.kind": routed.adapter.provider, | ||
| "provider.thread_id": input.threadId, | ||
| }); | ||
| yield* advanceThreadGeneration(input.threadId); |
There was a problem hiding this comment.
🟡 Medium Layers/ProviderService.ts:1057
getThreadLock permanently inserts every threadId into threadLocks, and neither this map nor threadGenerations is ever pruned after stopSession, runStopAll, or failed session starts. A long-running server accumulates a semaphore and generation entry for every thread it has ever seen, causing unbounded memory growth. Consider removing both entries for a thread when its session is stopped or fails to bind.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/ProviderService.ts around line 1057:
`getThreadLock` permanently inserts every `threadId` into `threadLocks`, and neither this map nor `threadGenerations` is ever pruned after `stopSession`, `runStopAll`, or failed session starts. A long-running server accumulates a semaphore and generation entry for every thread it has ever seen, causing unbounded memory growth. Consider removing both entries for a thread when its session is stopped or fails to bind.
There was a problem hiding this comment.
same concern as the thread on #4190, replied there.
listSessions is another possible queued borrower, so deleting the lock here has the same split-lock risk. gonna keep bounded cleanup out of this stack.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
1 participant
What Changed
reconcile adapter sessions against persisted ownership bindings, and return only active sessions owned by the committed provider instance.
listing retries transitional snapshots under the per-thread ownership lock, and fails loudly when bindings can't be read.
Why
adapters can briefly expose a replacement child before its ownership binding commits. treating adapter state alone as authoritative makes unbound, stale, or wrong-instance sessions look active.
so listing now follows the same ownership boundary as lifecycle transitions. reconciliation under the lock lets us distinguish an in-progress transition from corrupt state without exposing the uncommitted child.
Stack
Checklist
I included before/after screenshots for any UI changesI included a video for animation/interaction changesNote
Fix
listSessionsto return only sessions matching their committed bindingProviderServiceto serialize session operations per-thread using per-thread semaphores and a generation counter, making session start, stop, and recovery transactional.listSessionsnow reconciles active sessions against persisted bindings under a thread lock instead of throwing on mismatches, filtering out sessions that are not owned by the current binding.startSessiongains anactiveSession: 'reuse' | 'replace'option;ProviderCommandReactordefaults new starts to'reuse'and restarts to'replace'.📊 Macroscope summarized fd0b53d. 4 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.