fix(server-presence): clear session active on explicit stop disconnect - #223
fix(server-presence): clear session active on explicit stop disconnect#223danljungstrom wants to merge 4 commits into
Conversation
An accepted stop kills the runner, but the resulting publisher disconnect only recorded observer loss and left `active` set. Archive is gated on `active === false`, so it returned 409 session-active for the full 10-minute presence fence while the client retry budget is 75s, making archive-after-stop impossible to complete from the UI. Record intent only when a runner accepts a stop, and treat the disconnect that follows as the termination it is. A superseded fence still protects a successor publisher, and a failed stop records nothing so an incidental disconnect cannot end a live session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Greptile SummaryThe PR records accepted session-stop intent and uses it to close the publisher’s presence fence on disconnect, then immediately broadcasts the inactive lifecycle state.
Confidence Score: 4/5The cross-instance stop-intent handoff needs to be fixed before merging because multi-replica servers can still leave stopped sessions active until the timeout. Accepted stop intent is stored only in the RPC caller’s memory, while Redis may place the runner publisher and its disconnect handler on another process that cannot observe that intent. Files Needing Attention: apps/server/sources/app/api/socket/rpcHandler.ts; apps/server/sources/app/presence/sessionPublisherPresence.ts
|
| Filename | Overview |
|---|---|
| apps/server/sources/app/api/socket/rpcHandler.ts | Records intent after an accepted stop response, but does so on the RPC caller’s process even when the runner is remote. |
| apps/server/sources/app/presence/sessionPublisherPresence.ts | Adds fenced explicit-stop closure with TTL cleanup, while process-local intent storage does not support multi-instance routing. |
| apps/server/sources/app/api/socket.ts | Publishes inactive lifecycle state when an explicit-stop disconnect successfully closes the publisher fence. |
| apps/server/sources/app/api/socket/rpcHandler.integration.spec.ts | Verifies that an unreachable runner does not create explicit-stop intent. |
| apps/server/sources/app/presence/sessionPublisherPresence.sqlite.integration.spec.ts | Covers same-process fenced closure and participant fanout but not cross-instance intent propagation. |
Sequence Diagram
sequenceDiagram
participant UI as UI socket / instance A
participant A as Server instance A
participant R as Redis RPC registry
participant B as Server instance B
participant D as Runner publisher
UI->>A: killSession RPC
A->>R: Route RPC
R->>B: Resolve runner socket
B->>D: Forward killSession
D-->>A: accepted response
A->>A: Store stop intent in local Map
D--xB: Disconnect
B->>B: Local Map has no intent
B->>B: Record observer loss only
Note over B: Session remains active until timeout
Reviews (1): Last reviewed commit: "fix(server-presence): clear session acti..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const acceptedStopSessionId = sessionScopedStopSessionId ?? explicitMachineStopRequest?.sessionId ?? null; | ||
| if (acceptedStopSessionId && isAcceptedStopResponse(targetResponse)) { | ||
| ctx.sessionPublisherPresence?.markExplicitStopRequested({ | ||
| sessionId: acceptedStopSessionId, | ||
| }); |
There was a problem hiding this comment.
If the requesting client and runner publisher are connected to different server instances, the accepted RPC records intent only in the caller instance's process-local presence map. The runner's instance cannot consume that intent when its publisher disconnects, so the session remains active until the presence timeout and archive-after-stop continues to fail.
Knowledge Base Used: Happier Server (apps/server)
There was a problem hiding this comment.
Confirmed and fixed in dd6eb49. This was the correct read of the defect: the intent lived in explicitStopRequestedAtBySessionId, a process-local Map in createSessionPublisherPresence, so with redisRegistry.enabled the accepting RPC and the runner's publisher socket could be served by different instances and the disconnect handler never saw it. It degraded safely — no wrong closes — but delivered no improvement on multi-replica, which is where the archive-409 actually hurts.
The intent now lives on Session.stopRequestedAt (new nullable column, one migration per provider). markExplicitStopRequested writes the timestamp; forgetDisconnectedPublisher reads and clears it inside the transaction it already opens, so the consume and the close commit together. closeBindingAtFence was split into an in-transaction owner plus a thin inTx wrapper so the disconnect path reuses it without nesting a second transaction. The TTL is unchanged in meaning and is now a comparison against the persisted timestamp, which also retires the unbounded-map sweep.
Test: intent marked on one createSessionPublisherPresence instance is consumed by a second instance sharing the database. RED verified against the map — expected 'applied' to be 'closed'.
Two things the fix surfaced, both since fixed:
- The intent is keyed by session, so a successor publisher that registered before the stopped one disconnected could consume its predecessor's intent — its own fence is current, so nothing rejected the close. Reproduced (
expected 'closed' not to be 'closed') and fixed in 41fcedc by havingregisterOnceclearstopRequestedAtin the update it already performs. - The write had to be awaited in
rpcHandler, since the disconnect that reads it can arrive as soon as the runner starts tearing down.
One honest limit on the evidence: the cross-instance behaviour is proven at the storage boundary by two presence instances sharing one database, not by two server processes with Redis actually enabled. Called out in the PR's "Not verified" section.
WalkthroughThe stop-session RPC path records accepted explicit stop requests. Publisher disconnect handling consumes matching requests, closes the publisher, and publishes an inactive lifecycle update with closure metadata and participant cursors. ChangesExplicit session stop lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant rpcHandler
participant runner
participant sessionPublisherPresence
participant registeredPublisher
participant socketDisconnectHandler
participant sessionLifecycle
rpcHandler->>runner: Forward stop request
runner-->>rpcHandler: Return accepted response
rpcHandler->>sessionPublisherPresence: markExplicitStopRequested
sessionPublisherPresence->>sessionPublisherPresence: Persist stopRequestedAt
registeredPublisher-->>socketDisconnectHandler: Disconnect
socketDisconnectHandler->>sessionPublisherPresence: Consume matching request
sessionPublisherPresence->>registeredPublisher: Close at committed fence
registeredPublisher-->>socketDisconnectHandler: Return closed result and cursors
socketDisconnectHandler->>sessionLifecycle: Publish inactive lifecycle update
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/server/sources/app/api/socket/rpcHandler.integration.spec.ts`:
- Around line 667-674: Update the rpcHandler test fixture to use the socket,
listener maps, context, and sessionPublisherPresence types expected by
rpcHandler instead of broad as any casts. Reuse or create a typed RPC handler
fixture and typed Map instances, keeping at most one narrowly scoped, documented
boundary cast only if required by the test harness.
In `@apps/server/sources/app/api/socket/rpcHandler.ts`:
- Around line 381-389: Update all three successful forwarding paths around
forwardTargetResponse so the response is always forwarded and records accepted
stop intent even when no acknowledgement callback is supplied; invoke callback
only when it exists. Add a regression test covering an accepted stop without a
callback and verify it fails before the production change, then passes after it.
In `@apps/server/sources/app/presence/sessionPublisherPresence.ts`:
- Around line 111-135: Replace the session-only explicit stop tracking in
markExplicitStopRequested and consumeExplicitStopRequest with a one-shot intent
bound to the target publisher identity and committed fence, stored or routed to
that publisher’s owning Socket.IO instance. Consume it only when that exact
publisher disconnects, preventing successor publishers from inheriting stale
stop intent. Add coverage for Redis-routed stops and successor replacement
before the original publisher disconnects.
🪄 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: 8bf92beb-475c-4cfa-bdb5-37c5d22c70ce
📒 Files selected for processing (5)
apps/server/sources/app/api/socket.tsapps/server/sources/app/api/socket/rpcHandler.integration.spec.tsapps/server/sources/app/api/socket/rpcHandler.tsapps/server/sources/app/presence/sessionPublisherPresence.sqlite.integration.spec.tsapps/server/sources/app/presence/sessionPublisherPresence.ts
The intent lived in a process-local Map inside `createSessionPublisherPresence`. With `redisRegistry.enabled` the stop RPC and the runner's publisher socket can be served by different instances, so the disconnect handler never saw the intent and the fix degraded to the pre-existing behaviour: archive stays 409 `session-active` for the full 10-minute presence fence. Move the intent to `Session.stopRequestedAt`. `markExplicitStopRequested` writes the timestamp; `forgetDisconnectedPublisher` reads and clears it inside the transaction it already opens, so the consume and the close commit together. `closeBindingAtFence` is split into an in-transaction owner plus a wrapper so the disconnect path reuses it without nesting a second transaction. The TTL is unchanged in meaning and is now a comparison against the persisted timestamp, which also retires the unbounded-map sweep. The intent write is awaited in `rpcHandler` because it is now durable: the publisher disconnect that reads it can arrive as soon as the runner begins tearing down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er registers `stopRequestedAt` is keyed by session, not by publisher, so a stop that never killed its runner stayed readable after a successor took the session over. The successor's own incidental disconnect then consumed it and closed a session whose runner never agreed to stop — its fence was current, so nothing else rejected the close. A registering publisher is a new binding, so clear the intent in the update `registerOnce` already performs. No extra query, and the fence check keeps covering the opposite direction (a predecessor's stale fence). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/server/sources/app/presence/sessionPublisherPresence.ts (1)
560-579: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the explicit-stop consume-and-close block to reduce nesting.
This block adds two more nested levels inside an already deep
forgetDisconnectedPublisherclosure (arrow function →serializecallback →try→inTxcallback →if (explicitStopRequested)→if (closed.status === "closed")). Extract the stop-intent consumption and conditional close into a small named helper function, similar to howcloseBindingAtFenceInTxwas extracted fromcloseBindingAtFence. This keeps the disconnect handler flatter and makes the explicit-stop path independently testable.As per coding guidelines, "Avoid deeply nested code; refactor into separate functions when nesting exceeds 3 levels."
🤖 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 `@apps/server/sources/app/presence/sessionPublisherPresence.ts` around lines 560 - 579, Extract the explicit-stop consumption and conditional binding closure from forgetDisconnectedPublisher into a small named helper, using consumeExplicitStopRequestInTx and closeBindingAtFenceInTx with the existing session, binding, fence, timestamp, and mutation-id inputs. Have the helper return the closed result when closure succeeds and preserve the current fall-through behavior when no stop is consumed or the binding is already not closed; replace the nested block with a call to this helper.Source: Coding guidelines
🤖 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 `@apps/server/sources/app/presence/sessionPublisherPresence.ts`:
- Around line 105-123: Bind durable stop intents to the publisher fence that
accepted them, not only the session timestamp: persist a stop-request fence from
captureExplicitMachineStop’s target.committedFence, and update
consumeExplicitStopRequestInTx to require registration.committedFence to match
before consuming or closing the session. Propagate the schema and call-site
changes through register/forget handling, and add an integration test in
sessionPublisherPresence.sqlite.integration.spec.ts covering a successor
registration followed by an unrelated disconnect.
---
Nitpick comments:
In `@apps/server/sources/app/presence/sessionPublisherPresence.ts`:
- Around line 560-579: Extract the explicit-stop consumption and conditional
binding closure from forgetDisconnectedPublisher into a small named helper,
using consumeExplicitStopRequestInTx and closeBindingAtFenceInTx with the
existing session, binding, fence, timestamp, and mutation-id inputs. Have the
helper return the closed result when closure succeeds and preserve the current
fall-through behavior when no stop is consumed or the binding is already not
closed; replace the nested block with a call to this helper.
🪄 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: 425b2f47-4934-4a4f-8f3f-72d06d796f41
📒 Files selected for processing (9)
apps/server/prisma/migrations/20260802220000_add_session_stop_requested_at/migration.sqlapps/server/prisma/mysql/migrations/20260802220000_add_session_stop_requested_at/migration.sqlapps/server/prisma/mysql/schema.prismaapps/server/prisma/schema.prismaapps/server/prisma/sqlite/migrations/20260802220000_add_session_stop_requested_at/migration.sqlapps/server/prisma/sqlite/schema.prismaapps/server/sources/app/api/socket/rpcHandler.tsapps/server/sources/app/presence/sessionPublisherPresence.sqlite.integration.spec.tsapps/server/sources/app/presence/sessionPublisherPresence.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/sources/app/api/socket/rpcHandler.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/server/sources/app/presence/sessionPublisherPresence.sqlite.integration.spec.ts (1)
556-557: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact disconnect status instead of a negative match.
expect(disconnected.status).not.toBe("closed")only excludes one value. IfforgetDisconnectedPublisherregresses and returns an unrelated status such as"rejected"or"superseded"for the wrong reason, this assertion still passes. Assert the specific expected status (for example"forgotten", or whatever the non-closing outcome is named) so the test fails when the code takes an unexpected path.Proposed tightening
- expect(disconnected.status).not.toBe("closed"); + expect(disconnected.status).toBe("forgotten"); // replace with the actual non-closing status literal🤖 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 `@apps/server/sources/app/presence/sessionPublisherPresence.sqlite.integration.spec.ts` around lines 556 - 557, Update the assertion for forgetDisconnectedPublisher in the session presence integration test to require the exact expected non-closing status, such as "forgotten" or the established equivalent, instead of merely asserting that disconnected.status is not "closed".
🤖 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.
Nitpick comments:
In
`@apps/server/sources/app/presence/sessionPublisherPresence.sqlite.integration.spec.ts`:
- Around line 556-557: Update the assertion for forgetDisconnectedPublisher in
the session presence integration test to require the exact expected non-closing
status, such as "forgotten" or the established equivalent, instead of merely
asserting that disconnected.status is not "closed".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b4bf45a9-1bdb-4745-bb10-b8d4eebd9129
📒 Files selected for processing (2)
apps/server/sources/app/presence/sessionPublisherPresence.sqlite.integration.spec.tsapps/server/sources/app/presence/sessionPublisherPresence.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/sources/app/presence/sessionPublisherPresence.ts
…ledgement `forwardTargetResponse` owns explicit-stop intent recording and machine-scoped stop finalization, but all three forward sites invoked it only inside `if (callback)`. Socket.IO permits a caller to emit without an acknowledgement, and such a stop was forwarded to the runner and accepted while the server recorded nothing — leaving the session `active` for the full presence fence. Evaluate the forwarded response on every successful forward and call back only when the caller supplied one. Note that `callback?.(await forward(...))` does not fix this: optional-call short-circuiting skips argument evaluation, so the lifecycle never runs. The result is bound to a local first. Not reachable from first-party clients today — every stop path uses `emitWithAck` — so this closes the contract rather than an observed incident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/sources/app/api/socket/rpcHandler.ts (1)
379-395: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftClose the accepted-response and disconnect race.
markExplicitStopRequestedsetsstopRequestedAtafterforwardWithAckresolves, whileforgetDisconnectedPublisherreads that field in its disconnect transaction. If a publisher disconnect starts after the target sends an accepted response but before the intent write commits, the disconnect can miss the explicit-stop correlation and leave the session inactive only after the presence fence. Coordinate an accepted-stop response with disconnect consumption, then add a regression test that races accepted response, intent write, and publisher disconnect.🤖 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 `@apps/server/sources/app/api/socket/rpcHandler.ts` around lines 379 - 395, Update forwardTargetResponse and the publisher-presence disconnect handling so an accepted stop response is coordinated with markExplicitStopRequested before forgetDisconnectedPublisher consumes disconnect state, preventing the disconnect transaction from observing stale intent. Preserve correlation only for accepted stops, and add a regression test that races the accepted response, intent write, and publisher disconnect to verify the session is finalized correctly.
🧹 Nitpick comments (2)
apps/server/sources/app/api/socket/rpcHandler.integration.spec.ts (2)
689-718: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse four-space indentation in this server test.
The added block uses two spaces per indentation level. Reformat it with four spaces.
As per coding guidelines, files under
apps/server/sources/**/*.{ts,tsx}use 4 spaces for indentation.🤖 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 `@apps/server/sources/app/api/socket/rpcHandler.integration.spec.ts` around lines 689 - 718, Reformat the added test block in the `records an accepted stop even when the caller sends no acknowledgement callback` test to use four spaces for every indentation level, including nested callbacks, object literals, and assertions; do not change its behavior.Source: Coding guidelines
689-718: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover all forwarding branches in the no-ack regression.
This test exercises only the single-process listener-map path. The production change also covers the fallback path at Lines [532-536] and the Redis path at Lines [632-633]. Add equivalent no-ack cases for those branches, or parameterize the fixture, and assert that
markExplicitStopRequestedruns in each branch.🤖 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 `@apps/server/sources/app/api/socket/rpcHandler.integration.spec.ts` around lines 689 - 718, Extend the no-acknowledgement regression coverage for the forwarding logic to include the fallback listener path and Redis path in addition to the listener-map case. Reuse or parameterize the existing fixture around rpcHandler and assert that each branch invokes markExplicitStopRequested with the session ID after the runner accepts the stop.
🤖 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.
Outside diff comments:
In `@apps/server/sources/app/api/socket/rpcHandler.ts`:
- Around line 379-395: Update forwardTargetResponse and the publisher-presence
disconnect handling so an accepted stop response is coordinated with
markExplicitStopRequested before forgetDisconnectedPublisher consumes disconnect
state, preventing the disconnect transaction from observing stale intent.
Preserve correlation only for accepted stops, and add a regression test that
races the accepted response, intent write, and publisher disconnect to verify
the session is finalized correctly.
---
Nitpick comments:
In `@apps/server/sources/app/api/socket/rpcHandler.integration.spec.ts`:
- Around line 689-718: Reformat the added test block in the `records an accepted
stop even when the caller sends no acknowledgement callback` test to use four
spaces for every indentation level, including nested callbacks, object literals,
and assertions; do not change its behavior.
- Around line 689-718: Extend the no-acknowledgement regression coverage for the
forwarding logic to include the fallback listener path and Redis path in
addition to the listener-map case. Reuse or parameterize the existing fixture
around rpcHandler and assert that each branch invokes markExplicitStopRequested
with the session ID after the runner accepts the stop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7eb05681-51fe-4bc7-b2ae-31609f915d9e
📒 Files selected for processing (2)
apps/server/sources/app/api/socket/rpcHandler.integration.spec.tsapps/server/sources/app/api/socket/rpcHandler.ts
1 participant
Problem
Archiving a session from the UI fails, and the session "goes inactive on its own" ~10 minutes later.
POST /v2/sessions/:id/archiveis gated onactive === false(registerSessionArchiveRoutes.ts:56). An accepted stop kills the runner, but the runner dies by hard disconnect, andforgetDisconnectedPublisherdeliberately only records observer loss — it leavesactiveset. The only thing that clears it is the presence sweep, on a 10-minute fence (timeout.ts:18).The client retries archive for 75 seconds (
sessionStopArchiveFlow.ts, 200ms apart). 75s ≪ 600s, so the client always gives up first and archive-after-stop cannot complete from the UI.Observed on a self-hosted dev server: 3,899 × HTTP 409
session-activein 24h, in bursts of 330–670 per session — one burst per attempt, each burst a retry loop that expires. Sessions then flip to inactive minutes later when the sweep fires.Machine-scoped stop already finalized correctly via
captureExplicitMachineStop→finalizeExplicitMachineStop; the session-scoped<sessionId>:killSessionpath had no server-side lifecycle handling at all. That asymmetry is the bug.Change
stopped/requested, or a legacy{success:true}ack). A transport error, refusal, orRPC_METHOD_NOT_AVAILABLErecords nothing.Session.stopRequestedAtrather than in process memory. WithredisRegistry.enabledthe accepting RPC and the runner's publisher socket can be served by different instances, so a process-local intent never reaches the disconnect that resolves it.closeBindingAtFenceinstead of only recording observer loss, soactiveclears in seconds.active: falseon that close so clients learn immediately rather than at fence expiry.forwardTargetResponseonly insideif (callback), so an RPC emitted without an acknowledgement was forwarded and accepted while the server recorded nothing — and skipped the machine-scoped stop finalize, which predates this PR.forgetDisconnectedPublisherreads and clears the intent inside the transaction it already opens, so the consume and the close commit together.closeBindingAtFenceis split into an in-transaction owner plus a thininTxwrapper, letting the disconnect path reuse it without nesting a second transaction. The intent write is awaited inrpcHandlerbecause it is durable now — the disconnect that reads it can arrive as soon as the runner begins tearing down.Safety properties:
Migration
One new nullable column,
Session.stopRequestedAt, with a migration per provider (prisma/migrations,prisma/mysql/migrations,prisma/sqlite/migrations). Additive and backward-compatible: no backfill, no default, and a server running older code ignores the column.Tests
sessionPublisherPresence.sqlite.integration.spec.tsexpected 'applied' to be 'closed') with the behaviour reverted.expected 'applied' to be 'closed') against the in-memory map.expected 'closed' not to be 'closed') before registration cleared the intent.rpcHandler.integration.spec.tsexpected "spy" to not be called at all, but actually been called 1 times).markExplicitStopRequestedcalls: 0). Worth noting for reviewers:callback?.(await forwardTargetResponse(response))does not fix this — optional-call short-circuiting skips argument evaluation, so the test stayed red until the result was bound to a local first.yarn test)yarn test:integration)yarn build)test:migration:inventoryNot verified
No end-to-end run against a live server, and no multi-process run with Redis actually enabled — the cross-instance behaviour is proven at the storage boundary by two presence instances sharing one database, not by two server processes. Every layer is verified at its own boundary, but that a real UI stop traverses these exact call sites is inference from the code, not observation. Worth exercising on a dev deploy before release.
The no-acknowledgement forward path is fixed but was not reachable from any first-party client — every client stop path uses
emitWithAck. It closes a contract gap rather than an observed incident.Unrelated to this branch:
apps/servertest:integrationhas 11 pre-existing failures across threesessionUpdateHandler.*.integration.spec.tsfiles. Reproduced onorigin/devwith these sources reverted, so they are not caused by this PR.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Note
Clear session active state immediately on explicit stop disconnect
Sessiontable via a newstopRequestedAtcolumn, written when a runner accepts akillSessionor stop RPC call in rpcHandler.ts.sessionPublisherPresencein sessionPublisherPresence.ts immediately closes the session and publishes an inactive lifecycle update with participant cursors, bypassing the normal presence timeout fence.stopRequestedAtso incidental disconnects on restarted sessions are not treated as explicit stops.Macroscope summarized c486557.
Summary by CodeRabbit