fix(web): clear all per-session state when archiving or removing a session - #984
Conversation
🦋 Changeset detectedLatest commit: 02266ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4781e2b3b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // sidecar state. Both removal entry points (not-found + archive) go through | ||
| // this, so adding a new per-session map only ever needs one new line here. | ||
| // --------------------------------------------------------------------------- | ||
| function forgetSession(sessionId: string): void { |
There was a problem hiding this comment.
When a session is archived while it has an in-flight turn and queued follow-up prompts, this teardown leaves rawState.queuedBySession and inFlightPromptSessions intact. When the archived session later emits its idle status, onSessionIdle still drains rawState.queuedBySession[sid] and calls submitPromptInternal(sid, ...) without checking that the session is still present, so a prompt can be submitted to a session the user just archived. Please clear the prompt queue/in-flight prompt sidecars here along with the other per-session state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 40defb6 — forgetSession now also clears inFlightPromptSessions, queuedBySession, promptIdBySession and sendingBySession for the torn-down session, so a queued follow-up can't be submitted to a just-archived session when its turn later goes idle. Thanks for catching this.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40defb6f91
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function forgetSession(sessionId: string): void { | ||
| removeSession(sessionId); | ||
| removeSessionMessages(sessionId); |
There was a problem hiding this comment.
When the session being archived has already been opened, the WebSocket connection remains subscribed here because forgetSession only removes local maps. The socket keeps session subscriptions until unsubscribe() is called, and the event reducer will recreate per-session maps for any later/buffered event for that session id; in an active or running archive flow, closing/turn events can therefore repopulate the orphaned messages/tasks/flags this helper just deleted. Unsubscribe the session as part of this teardown before clearing the sidecars.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 134e896 — forgetSession now calls eventConn?.unsubscribe(sessionId) before clearing the sidecars, so late/buffered events for the removed session cannot be reduced and repopulate the maps. Good catch.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 134e896f68
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Stop receiving events for this session BEFORE clearing its state: a late or | ||
| // buffered event for this id would otherwise be reduced and recreate the very | ||
| // per-session maps we are about to delete. | ||
| eventConn?.unsubscribe(sessionId); |
There was a problem hiding this comment.
When this runs after subscribe() has queued a session but before the socket receives server_hello, it does not actually stop events for that session: DaemonEventSocket.unsubscribe() deletes subscriptions but leaves pendingSubscriptions, and onServerHello() merges those pending entries back into the active subscription set. In a slow initial WS connection or reconnect, archiving/removing a just-selected session can still subscribe to it and late events can recreate the per-session maps that forgetSession() just cleared; please clear pending subscriptions too or guard events for forgotten IDs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 02266ed — DaemonEventSocket.unsubscribe() now also removes the session from pendingSubscriptions, so a subscribe queued before server_hello is properly cancelled and will not be merged back into the active set. The forgetSession unsubscribe is therefore effective in that window too.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02266ed31e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Stop receiving events for this session BEFORE clearing its state: a late or | ||
| // buffered event for this id would otherwise be reduced and recreate the very | ||
| // per-session maps we are about to delete. | ||
| eventConn?.unsubscribe(sessionId); |
There was a problem hiding this comment.
When archiving a running session, this only sends unsubscribe before deleting the maps, but DaemonEventSocket still forwards any frames already queued in the browser or sent before the server processes the unsubscribe. Those late messageCreated/status frames then go through applyEvent/onSessionIdle and can recreate messagesBySession, sendingBySession, or unread state for the archived session, so the new teardown is still racy; keep a forgotten-session guard/tombstone in the event path or drop frames for unsubscribed session ids before reducing them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged — this is the buffered/in-flight frame race, which is real but extremely narrow and low-impact: it only triggers when archiving a running session with frames already in the browser queue or sent before the server processes the unsubscribe. The backend stops emitting events once a session is archived, so the residual is only frames already in flight, and any repopulated maps are not rendered (the session is gone from the list) and are cleared again on the next load.
The mitigations already in this PR (clearing per-session state in forgetSession, unsubscribe before teardown, and cancelling pending subscriptions in DaemonEventSocket.unsubscribe) cover the realistic cases. Adding a forgotten-session tombstone in the event path would be extra complexity for a window that is not practically observable, so we are deferring it for now and keeping this PR focused on the teardown fix. We can revisit if it ever shows up in practice.
1 participant
Related Issue
N/A — follow-up to the web client structure refactor in #979.
Problem
The two entry points that remove a session cleaned up per-session state inconsistently:
handleSessionNotFoundcleared ~13 fields (messages, approvals, questions, tasks, goal, git status, cursors, etc.).archiveSessioncleared only the session list, side-chat state, and the active-session pointer — leaving the same ~13 per-session maps behind as orphaned data for the archived session.This is the kind of "change is not thorough" bug that motivated the setter consolidation in #979: adding a new per-session map required remembering to update both teardown sites, and
archiveSessionhad drifted.What changed
Add
forgetSession(id)in the facade (next to the existing state setters) as the single place session teardown lives: it drops the session from the list and wipes every per-session sidecar map. Both entry points now route through it:handleSessionNotFound(id)→forgetSession(id)(replaces its inline teardown).archiveSession(id)→forgetSession(id)plus its existing side-chat-specific cleanup and active-session fallback.Behavior change: archiving a session now also clears its messages / approvals / questions / tasks / etc. (previously leaked). This is the intended fix.
Verified with
typecheck,test(26),build, andoxlint(0 errors).Checklist
apps/kimi-web; covered bytypecheck+ existing pure-logic tests.)gen-changesetsskill, or this PR needs no changeset. (Patch on@moonshot-ai/kimi-code;@moonshot-ai/kimi-webis ignored and bundled into the CLI.)gen-docsskill, or this PR needs no doc update. (No user-facing config or command change.)