fix(runtime-host): lazily adopt legacy Session Connection identity - #4627
fix(runtime-host): lazily adopt legacy Session Connection identity#4627bug-superman wants to merge 1 commit into
Conversation
0fe755d to
b494457
Compare
Generated-by: OpenAI Codex
b494457 to
410468d
Compare
|
Generative tooling disclosure: OpenAI Codex contributed materially to this change; final review and ownership remain with the human contributor. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The direction is right and the identity store stays single-owner: llmConnectionId is still only written by HostSessionCatalogCoordinator and the revision inheritance path. I verified the Host has one SessionAdmissionGate instance shared by the catalog, root and message coordinators, that moving the sessionCatalog construction earlier does not use anything defined after it, and that the six readSessionHeader call sites in message-coordinator.ts that do not pass a lease only read isArchived, so they are not a gap. Three things to fix before merge, plus one scope question.
P2: the wiring has no test. All five new tests call coordinator.adoptLegacySessionConnectionIdentity(...) directly; nothing exercises RootTurnCoordinator or HostMessageCoordinator, so the user-visible outcome the PR claims (a legacy Session can send again) is not covered. Two things make that riskier than usual. The adopter is an optional trailing constructor parameter, so every existing RootTurnCoordinator construction silently keeps the old behavior and a wiring mistake cannot turn a test red. And a lease that crosses task contexts makes SessionAdmissionGate throw (Cannot reuse a Session admission lease from another task, Session admission lease was not issued by this gate), which would surface as a hard error on the normal send path. I could not verify by running whether the lease passed at root-turn-coordinator.ts:2213 is always in the issuing context, so that is an evidence gap on my side. Please add one end to end test: an unlocked legacy Session that is refused on main and sends here, asserting llmConnectionId is persisted.
P3: readSessionHeader now writes to the store (root-turn-coordinator.ts:510, contract at message-coordinator.ts:197), while its name and interface still say read. Consider renaming it or lifting the adoption out into its own call.
P3: prepareSessionHeader is called at nine sites, each one a readHeaderSnapshot immediately followed by a prepareSessionHeader. Folding both into a single private readAdmittedHeader(sessionId, lease) puts each call site back to one line and makes it impossible for a future entry point to miss the step.
Scope question, not a finding: connectionLocked is set as soon as a user message lands (packages/storage/src/sqlite-session-metadata-store.ts:1494), so this only unblocks legacy Sessions that never sent anything. Every pre-upgrade Session with history stays blocked and needs the user to explicitly reselect a Connection. The Safety section explains why locked Sessions cannot be guessed, and I agree with that, but the title and summary read as if all legacy Sessions are covered. Please state the actual scope, and confirm the reselect path is reachable in the UI for a Session that is currently reporting the legacy unavailable reason.
On CI: gh pr checks reports no checks on this branch, and the only workflow run at this head is CI sitting at action_required. Nothing has been verified by CI at 410468db.
| if (!connection) return undefined; | ||
| let committed: SessionHeaderSnapshot; | ||
| try { | ||
| committed = await this.#stores.updateHeaderVersioned( |
There was a problem hiding this comment.
P2: the CAS write happens before the revalidation below, and a failed revalidation does not undo it. If the catalog changes between the snapshot on line 260 and resolveExecutionConnection on line 275, or the two simply disagree, the method returns undefined (caller sees the pre-write header and fails closed for this execution) while the store now holds a llmConnectionId that never passed revalidation. On the next admission the first guard on line 254 sees a defined id and skips adoption entirely, so the Session executes with that binding and nothing ever clears it. Credentials cannot cross, since bound never falls back to slug, but it is still a committed wrong state, and the new test at session-catalog-coordinator.test.ts:1144 asserts the bad id stays persisted. Smallest fix: revalidate first and commit only on a resolvable identity. #3860 asks for revalidation before execution, which validate-then-write also satisfies, with one fewer residual state.
| ) { | ||
| return undefined; | ||
| } | ||
| const connection = (await this.#runtimePolicy.connectionCatalog.getSnapshot()).connections.find( |
There was a problem hiding this comment.
P2: this is a second slug to identity resolver. resolveExecutionConnection({ kind: 'catalog_slug', connectionSlug }) already does exactly this lookup, inside the runtime policy lane and with the retired and enabled checks, at packages/storage/src/runtime-policy/coordinator.ts:826. Resolving here off a raw catalog snapshot is what lets the two disagree, which is precisely the case the post-commit revalidation then has to fail closed on. Going through the existing resolver and taking connection.connectionId off the ready result removes this duplication and lets the revalidation block on lines 275 to 282 disappear.
|
|
||
| /** | ||
| * Compatibility migration for pre-identity Sessions that have not yet | ||
| * committed a user turn. The write is revision/CAS guarded and the resulting |
There was a problem hiding this comment.
P3: the comment says "have not yet committed a user turn" but the code branches on connectionLocked (line 256). They are equivalent today because the store sets the lock on the first user message, but the comment should name the field it actually reads, otherwise it becomes wrong the moment connectionLocked picks up another meaning.
3 participants
Closes #3860.
Summary
Safety
Bound execution continues to resolve by immutable
connectionIdand verifies the stored slug. Missing IDs and ID/slug mismatches never fall back to a same-slug catalog entry, so deleted/reused Connections cannot cross credentials. OAuth reconciliation remains ID-bound through the existing resolver.A pre-upgrade legacy Session has no deletion tombstone or historical identity evidence. If its original Connection was deleted and the slug reused before this upgrade, the original identity cannot be reconstructed; locked legacy Sessions therefore remain blocked instead of being silently rebound.
Verification
npm --workspace @maka/runtime-host run typecheck(Node 24)npm --workspace @maka/runtime-host run build(Node 24)@maka/uitype errors after core/storage/runtime/runtime-host compile successfully.