-
Notifications
You must be signed in to change notification settings - Fork 5
fix: resolve stale pending tool state during session sync #381
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
947e62a
1bd9e58
5d4a9e2
1ad1335
cbffa53
d164a95
92b2e2a
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { createContext, useContext, onCleanup, batch, type ParentProps } from "solid-js" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { createStore, reconcile, produce } from "solid-js/store" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { Session, Message, Part, Provider } from "../sdk/client" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { Session, Message, Part, Provider, TextPart } from "../sdk/client" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useSDK } from "./sdk" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useServer } from "./server" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { createSSEParser } from "../utils/sse" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -58,6 +58,95 @@ function sortParts(parts: Part[]): Part[] { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return [...withId, ...withoutId] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function toolEnd(part: Extract<Part, { type: "tool" }>): number { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const state = part.state | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (state.status === "completed") return state.time.end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (state.status === "error") return state.time.end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function toolStart(part: Extract<Part, { type: "tool" }>): number { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const state = part.state | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (state.status === "running") return state.time.start | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (state.status === "completed") return state.time.start | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (state.status === "error") return state.time.start | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function toolRank(part: Extract<Part, { type: "tool" }>): number { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const state = part.state | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (state.status === "pending") return 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (state.status === "running") return 2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 3 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function mergePart(existing: Part, synced: Part): Part { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (existing.type !== "tool") { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (synced.type !== "tool") { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const getTimeValue = (part: Part): number => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (part.type === "text" || part.type === "reasoning") { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const t = (part as TextPart).time | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return t?.end ?? t?.start ?? 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (part.type === "retry") { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return (part as { time?: { created?: number } }).time?.created ?? 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const existingEnd = getTimeValue(existing) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const syncedEnd = getTimeValue(synced) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (existingEnd > syncedEnd) return existing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (syncedEnd > existingEnd) return synced | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return existing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return synced | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (synced.type !== "tool") return synced | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+83
to
+104
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function mergePart(existing: Part, synced: Part): Part { | |
| if (existing.type !== "tool") return synced | |
| if (synced.type !== "tool") return synced | |
| function partTimeValue(part: Part, key: "start" | "end"): number | undefined { | |
| const value = (part as { time?: { start?: unknown; end?: unknown } }).time?.[key] | |
| return typeof value === "number" ? value : undefined | |
| } | |
| function mergeNonToolPart(existing: Part, synced: Part): Part { | |
| const existingEnd = partTimeValue(existing, "end") | |
| const syncedEnd = partTimeValue(synced, "end") | |
| if (existingEnd !== undefined && syncedEnd !== undefined) { | |
| if (existingEnd > syncedEnd) return existing | |
| if (syncedEnd > existingEnd) return synced | |
| } | |
| const existingStart = partTimeValue(existing, "start") | |
| const syncedStart = partTimeValue(synced, "start") | |
| if (existingStart !== undefined && syncedStart !== undefined) { | |
| if (existingStart > syncedStart) return existing | |
| if (syncedStart > existingStart) return synced | |
| } | |
| if (existing.id && synced.id && existing.id === synced.id) return existing | |
| return synced | |
| } | |
| function mergePart(existing: Part, synced: Part): Part { | |
| if (existing.type !== "tool" || synced.type !== "tool") { | |
| return mergeNonToolPart(existing, synced) | |
| } |
Copilot
AI
Apr 20, 2026
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.
mergePart() currently returns synced for all non-tool parts, which means a full-session sync can overwrite newer local/SSE part updates for the same part.id (contrary to the issue acceptance criterion about not regressing newer SSE state). Consider extending the freshness comparison to non-tool parts using their time fields (e.g., end/start/created), or defaulting to existing unless synced is provably newer.
Copilot
AI
Apr 20, 2026
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.
mergeMessage() only tracks existing parts that have an id (existingWithId). Any locally-added parts without an id (which this file explicitly handles in message.part.updated) will be dropped during a full-session sync merge if they aren’t present in the synced payload. If id-less parts are expected, they should be preserved similarly to how you preserve id-bearing parts (e.g., append existing parts with no id after merging, or incorporate them into the merge output explicitly).
Copilot
AI
Apr 20, 2026
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.
mergeMessage() always uses info: synced.info, which can regress message metadata if newer message.updated SSE events arrived after the sync request started (same race as parts). Consider preferring the info object with the newer time.completed (or other monotonic fields) when both sides refer to the same message, instead of always taking synced.info.
Copilot
AI
Apr 20, 2026
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.
mergeMessage() always takes synced.info. Elsewhere (message.updated) you already guard against out-of-order updates by preferring the Message with newer time.completed. To truly “preserve newer SSE updates” during syncSession(), consider merging info with a similar freshness check (at least for assistant messages) so a stale sync payload can’t overwrite newer local info fields.
Copilot
AI
Apr 20, 2026
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.
mergeMessage() always sets info: synced.info, which can overwrite newer message info already received via SSE (e.g., a newer time.completed, token/cost updates, or errors) and violates the intent to preserve newer local state during full-session sync. Consider merging existing.info vs synced.info using a freshness signal (like time.completed/time.created) similar to the logic used in the message.updated handler, and keep the newer one.
Copilot
AI
Apr 20, 2026
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.
mergeMessage() always returns info: synced.info, which can overwrite newer message info already applied via SSE (e.g. time.completed/error) if the syncSession() request started before the SSE update and returns a stale snapshot. Consider merging existing.info vs synced.info with the same “prefer newer time.completed” logic used in the message.updated handler, so full sync can’t regress message completion/error state.
Copilot
AI
Apr 20, 2026
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.
isNewer() reads part.time.completed, but Part.time objects in the SDK don’t have a completed field (messages do; parts use start/end/created depending on type). This should be updated to compare the correct timestamps (e.g., time.end ?? time.start for text/reasoning, time.created for retry, and/or tool state.time.*), otherwise this won’t typecheck and won’t correctly protect against out-of-order updates.
| function isNewer(a: Part, b: Part): boolean { | |
| const aEnd = a.time?.completed ?? a.time?.start | |
| const bEnd = b.time?.completed ?? b.time?.start | |
| if (!aEnd) return false | |
| if (!bEnd) return true | |
| return aEnd > bEnd | |
| function toTimestamp(value: unknown): number | undefined { | |
| if (typeof value === "number") return Number.isNaN(value) ? undefined : value | |
| if (typeof value === "string") { | |
| const parsed = Date.parse(value) | |
| return Number.isNaN(parsed) ? undefined : parsed | |
| } | |
| if (value instanceof Date) { | |
| const parsed = value.getTime() | |
| return Number.isNaN(parsed) ? undefined : parsed | |
| } | |
| return undefined | |
| } | |
| function getPartTimestamp(part: Part): number | undefined { | |
| const time = part.time as { end?: unknown; start?: unknown; created?: unknown } | undefined | |
| const directTimestamp = | |
| toTimestamp(time?.end) ?? toTimestamp(time?.start) ?? toTimestamp(time?.created) | |
| if (directTimestamp !== undefined) return directTimestamp | |
| const stateTime = ( | |
| part as Part & { | |
| state?: { | |
| time?: { | |
| completed?: unknown | |
| end?: unknown | |
| start?: unknown | |
| created?: unknown | |
| } | |
| } | |
| } | |
| ).state?.time | |
| return ( | |
| toTimestamp(stateTime?.completed) ?? | |
| toTimestamp(stateTime?.end) ?? | |
| toTimestamp(stateTime?.start) ?? | |
| toTimestamp(stateTime?.created) | |
| ) | |
| } | |
| function isNewer(a: Part, b: Part): boolean { | |
| const aTime = getPartTimestamp(a) | |
| const bTime = getPartTimestamp(b) | |
| if (aTime === undefined) return false | |
| if (bTime === undefined) return true | |
| return aTime > bTime |
Copilot
AI
Apr 20, 2026
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.
isNewer() references ToolPart["state"], but ToolPart is not imported/defined in this module, so this won’t typecheck. Import ToolPart from ../sdk/client (like other SDK types) or rewrite the narrowing to use Extract<Part, { type: "tool" }>/the existing helper functions (toolStart/toolEnd) without needing ToolPart.
Copilot
AI
Apr 20, 2026
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.
isNewer() reads aState.time/bState.time directly, but ToolStatePending has no time field in the generated SDK types. With strict: true this should fail typechecking and can also lead to incorrect comparisons. Narrow on state.status !== "pending" (or use a type guard like "time" in state) before reading time, and compute a comparable timestamp from end ?? start only when the field exists.
Copilot
AI
Apr 20, 2026
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.
In the message.part.updated handler, the part store update uses isNewer(existingPart, part) to ignore stale tool-part updates, but the corresponding update of store.message[...] does not. Since most UI code reads msg.parts from sync.messages(), an out-of-order stale tool update (e.g. reverting completed back to pending) can still regress the visible state even if the part store ignored it. Apply the same freshness guard when replacing parts inside msgs.map(...) to keep the two stores consistent and prevent stale tool state from reappearing.
| const newParts = partIdx === -1 ? [...m.parts, part] : m.parts.map((p, pi) => (pi === partIdx ? part : p)) | |
| return { ...m, parts: newParts } | |
| if (partIdx === -1) return { ...m, parts: [...m.parts, part] } | |
| const existingPart = m.parts[partIdx] | |
| if (!existingPart.id || isNewer(existingPart, part)) return m | |
| return { ...m, parts: m.parts.map((p, pi) => (pi === partIdx ? part : p)) } |
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.
mergePart()defaults to returningsyncedwhen the compared freshness signals are equal (e.g.,existingEnd === syncedEnd), which can overwrite newer in-memory/SSE state when freshness can't be established. Consider preferringexistingon ties (or adding a tie-breaker) to avoid regressing local state during sync merges.