-
Notifications
You must be signed in to change notification settings - Fork 879
fix(web): clear all per-session state when archiving or removing a session #984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f4781e2
40defb6
134e896
02266ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@moonshot-ai/kimi-code": patch | ||
| --- | ||
|
|
||
| Clear all per-session state when a session is archived or removed, so archived sessions no longer leave orphaned data behind. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -442,6 +442,40 @@ function removeSessionMessages(sessionId: string): void { | |
| rawState.messagesBySession = rest; | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Session teardown — single place that wipes a session and all its per-session | ||
| // 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 { | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When this runs after Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 02266ed —
Comment on lines
+451
to
+454
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When archiving a running session, this only sends Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| removeSession(sessionId); | ||
| removeSessionMessages(sessionId); | ||
|
Comment on lines
+450
to
+456
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the session being archived has already been opened, the WebSocket connection remains subscribed here because Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 134e896 — |
||
| delete rawState.approvalsBySession[sessionId]; | ||
| delete rawState.questionsBySession[sessionId]; | ||
| delete rawState.tasksBySession[sessionId]; | ||
| delete rawState.goalBySession[sessionId]; | ||
| delete rawState.gitStatusBySession[sessionId]; | ||
| delete rawState.lastSeqBySession[sessionId]; | ||
| delete rawState.compactionBySession[sessionId]; | ||
| delete rawState.messagesLoadingMoreBySession[sessionId]; | ||
| delete rawState.messagesHasMoreBySession[sessionId]; | ||
| delete rawState.messagesLoadMoreErrorBySession[sessionId]; | ||
| delete epochBySession[sessionId]; | ||
| sessionsKnownEmpty.delete(sessionId); | ||
| // In-flight / queued prompt state: drop these too so a queued follow-up | ||
| // can't be submitted to a session that was just archived when its turn later | ||
| // goes idle (onSessionIdle drains queuedBySession[sid] without re-checking | ||
| // that the session still exists). | ||
| inFlightPromptSessions.delete(sessionId); | ||
| delete rawState.queuedBySession[sessionId]; | ||
| delete rawState.promptIdBySession[sessionId]; | ||
| delete rawState.sendingBySession[sessionId]; | ||
| } | ||
|
|
||
| // Models + Providers reactive state and helpers live in | ||
| // ./client/useModelProviderState. It is instantiated below (after the | ||
| // `activity` computed it depends on) as `modelProvider`. | ||
|
|
@@ -894,20 +928,7 @@ function goalErrorMessage(err: unknown): string | undefined { | |
| } | ||
|
|
||
| async function handleSessionNotFound(sessionId: string): Promise<void> { | ||
| removeSession(sessionId); | ||
| removeSessionMessages(sessionId); | ||
| delete rawState.approvalsBySession[sessionId]; | ||
| delete rawState.questionsBySession[sessionId]; | ||
| delete rawState.tasksBySession[sessionId]; | ||
| delete rawState.goalBySession[sessionId]; | ||
| delete rawState.gitStatusBySession[sessionId]; | ||
| delete rawState.lastSeqBySession[sessionId]; | ||
| delete rawState.compactionBySession[sessionId]; | ||
| delete rawState.messagesLoadingMoreBySession[sessionId]; | ||
| delete rawState.messagesHasMoreBySession[sessionId]; | ||
| delete rawState.messagesLoadMoreErrorBySession[sessionId]; | ||
| delete epochBySession[sessionId]; | ||
| sessionsKnownEmpty.delete(sessionId); | ||
| forgetSession(sessionId); | ||
|
|
||
| if (rawState.activeSessionId !== sessionId) return; | ||
|
|
||
|
|
@@ -1835,7 +1856,7 @@ const workspaceState = useWorkspaceState(rawState, { | |
| updateSession, | ||
| upsertSessionFront, | ||
| appendSession, | ||
| removeSession, | ||
| forgetSession, | ||
| setActiveSessionId, | ||
| updateSessionMessages, | ||
| nextOptimisticMsgId, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a session is archived while it has an in-flight turn and queued follow-up prompts, this teardown leaves
rawState.queuedBySessionandinFlightPromptSessionsintact. When the archived session later emits its idle status,onSessionIdlestill drainsrawState.queuedBySession[sid]and callssubmitPromptInternal(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 40defb6 —
forgetSessionnow also clearsinFlightPromptSessions,queuedBySession,promptIdBySessionandsendingBySessionfor 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.