Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions apps/desktop/src/app/chat/composer/hooks/use-composer-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ interface UseComposerQueueArgs {
queueEditRef: RefObject<QueueEditState | null>
queueSessionKey: ChatBarProps['queueSessionKey']
sessionId: string | null | undefined
/** Stored session id for the active session — used to stamp queue entries. */
storedSessionId: string | null | undefined
}

/**
Expand All @@ -57,7 +59,8 @@ export function useComposerQueue({
onSubmit,
queueEditRef,
queueSessionKey,
sessionId
sessionId,
storedSessionId
}: UseComposerQueueArgs) {
const { t } = useI18n()

Expand Down Expand Up @@ -168,7 +171,12 @@ export function useComposerQueue({
return false
}

if (!enqueueQueuedPrompt(activeQueueSessionKey, { text, attachments })) {
if (!enqueueQueuedPrompt(activeQueueSessionKey, {
text,
attachments,
sourceRuntimeId: sessionId ?? undefined,
sourceStoredId: storedSessionId ?? undefined
})) {
return false
}

Expand All @@ -177,10 +185,13 @@ export function useComposerQueue({
triggerHaptic('selection')

return true
}, [activeQueueSessionKey, attachments, clearDraft, draftRef])
}, [activeQueueSessionKey, attachments, clearDraft, draftRef, sessionId, storedSessionId])

// All queue drain paths share one lock + send-then-remove sequence.
// `pickEntry` lets each caller choose head, by-id, or skip-edited.
// Each entry carries its source session ids so the drain always targets
// the session that owned the entry at enqueue time — never the
// currently-active session if the user switched in between (Race 5).
const runDrain = useCallback(
async (pickEntry: (entries: QueuedPromptEntry[]) => QueuedPromptEntry | undefined): Promise<boolean> => {
if (drainingQueueRef.current || !activeQueueSessionKey) {
Expand All @@ -193,19 +204,32 @@ export function useComposerQueue({
return false
}

// Determine which session key owns this entry. If the entry has a
// sourceRuntimeId that differs from the current activeQueueSessionKey,
// the user switched sessions — use the entry's source key for removal
// and pass the source ids to onSubmit so it targets the right session.
const entrySessionKey = entry.sourceRuntimeId && entry.sourceRuntimeId !== activeQueueSessionKey
? entry.sourceRuntimeId
: activeQueueSessionKey

drainingQueueRef.current = true

try {
const accepted = await Promise.resolve(
onSubmit(entry.text, { attachments: entry.attachments, fromQueue: true })
onSubmit(entry.text, {
attachments: entry.attachments,
fromQueue: true,
targetRuntimeId: entry.sourceRuntimeId,
targetStoredId: entry.sourceStoredId
})
)

if (accepted === false) {
return false
}

drainFailuresRef.current.delete(entry.id)
removeQueuedPrompt(activeQueueSessionKey, entry.id)
removeQueuedPrompt(entrySessionKey, entry.id)
resetBrowseState(sessionId)

return true
Expand Down
3 changes: 2 additions & 1 deletion apps/desktop/src/app/chat/composer/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ export function ChatBar({
onSubmit,
queueEditRef,
queueSessionKey,
sessionId
sessionId,
storedSessionId: queueSessionKey
})

const statusStackVisible = queuedPrompts.length > 0 || statusPresent
Expand Down
2 changes: 1 addition & 1 deletion apps/desktop/src/app/chat/composer/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export interface ChatBarProps {
onSteer?: (text: string) => Promise<boolean> | boolean
onSubmit: (
value: string,
options?: { attachments?: ComposerAttachment[]; fromQueue?: boolean }
options?: { attachments?: ComposerAttachment[]; fromQueue?: boolean; targetRuntimeId?: string; targetStoredId?: string }
) => Promise<boolean> | boolean
onTranscribeAudio?: (audio: Blob) => Promise<string>
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,13 @@ export function useGatewayEventHandler(deps: GatewayEventDeps) {
}
}

if (sessionId && hasStatePatch) {
// Only apply per-session state patches when we have a real session_id.
// Without explicitSid, sessionId falls back to activeSessionIdRef.current
// — patching state with that fallback could misattribute a background
// session's model/cwd/branch onto the active session during a switch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a hook-level regression test for this guard. The current gateway-events.test.ts only permits unscoped session.info; it does not verify that an unscoped state patch cannot call updateSessionState for the active session, while an explicit session ID still can.

// The `apply` guard above already prevents global setters; this closes
// the per-session state-patch path (Race 3).
if (explicitSid && sessionId && hasStatePatch) {
updateSessionState(sessionId, state => ({
...state,
...statePatch,
Expand Down
60 changes: 42 additions & 18 deletions apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,14 @@ export function useSubmitPrompt(deps: SubmitPromptDeps) {

// One submit in flight per session — drop any concurrent re-fire so a
// stalled turn can't stack the same prompt into multiple real turns.
const submitLockKey = startingStoredSessionId || startingActiveSessionId || '__pending_new__'
// When draining a queued prompt, key the lock on the TARGET session
// (the one that owns the queue entry), not the currently-active one.
const submitLockKey =
options?.targetStoredId ||
options?.targetRuntimeId ||
startingStoredSessionId ||
startingActiveSessionId ||
'__pending_new__'

if (_submitInFlight.has(submitLockKey)) {
return false
Expand Down Expand Up @@ -226,7 +233,10 @@ export function useSubmitPrompt(deps: SubmitPromptDeps) {
setAwaitingResponse(true)
clearNotifications()

let sessionId: null | string = activeSessionId
// When draining a queued prompt, target the source session instead of the
// currently-active one. This prevents a queued prompt from firing into the
// wrong session after a session switch (Race 5).
let sessionId: null | string = options?.targetRuntimeId || activeSessionId

if (sessionId) {
seedOptimistic(sessionId)
Expand Down Expand Up @@ -331,22 +341,36 @@ export function useSubmitPrompt(deps: SubmitPromptDeps) {
// backend loop (#55578 symptom d) rejects the submit even though
// the stored session is fine — resume + retry instead of erroring
// out and losing the session binding.
const resumed = await requestGateway<{ session_id: string }>('session.resume', {
session_id: startingStoredSessionId,
source: 'desktop'
})

if (sessionContextDrifted()) {
return abortForSessionSwitch(sessionId)
}

const recoveredId = resumed?.session_id

if (recoveredId) {
activeSessionIdRef.current = recoveredId
await withSessionBusyRetry(() =>
requestGateway('prompt.submit', { session_id: recoveredId, text }, PROMPT_SUBMIT_REQUEST_TIMEOUT_MS)
)
// Use the queued prompt's source stored id when available so the
// resume targets the original session, not the active one.
const storedIdForResume = options?.targetStoredId || startingStoredSessionId

if (storedIdForResume) {
const resumed = await requestGateway<{ session_id: string }>('session.resume', {
session_id: storedIdForResume,
source: 'desktop'
})

if (sessionContextDrifted()) {
return abortForSessionSwitch(sessionId)
}

const recoveredId = resumed?.session_id

if (recoveredId) {
// Update the ref only if we're targeting the active session.
// For background-session drains, don't clobber the active ref.
if (!options?.targetRuntimeId || options.targetRuntimeId === activeSessionIdRef.current) {
activeSessionIdRef.current = recoveredId
}

sessionId = recoveredId
await withSessionBusyRetry(() =>
requestGateway('prompt.submit', { session_id: recoveredId, text }, PROMPT_SUBMIT_REQUEST_TIMEOUT_MS)
)
} else {
submitErr = firstErr
}
} else {
submitErr = firstErr
}
Expand Down
11 changes: 11 additions & 0 deletions apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,4 +226,15 @@ export function visibleUserIndexAtOrdinal(messages: readonly ChatMessage[], targ
export interface SubmitTextOptions {
attachments?: ComposerAttachment[]
fromQueue?: boolean
/**
* When draining a queued prompt, the source session's runtime id. If the
* user switched sessions between enqueue and drain, this ensures the prompt
* submits to the original session instead of the currently-active one.
*/
targetRuntimeId?: string
/**
* The stored session id paired with targetRuntimeId, used for session.resume
* retry when the runtime id has expired.
*/
targetStoredId?: string
}
14 changes: 14 additions & 0 deletions apps/desktop/src/lib/gateway-events.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,18 @@ describe('gateway event routing', () => {
expect(gatewayEventRequiresSessionId('session.info')).toBe(false)
expect(gatewayEventRequiresSessionId(undefined)).toBe(false)
})

it('documents the Race 3 fix: session.info state patches require explicitSid', () => {
// gatewayEventRequiresSessionId('session.info') is still false — unscoped
// session.info events are NOT dropped (they carry the active turn's state).
// The Race 3 fix is in gateway-event.ts: the per-session state-patch
// (updateSessionState) is guarded by `explicitSid` so an unscoped
// session.info can't misattribute a background session's model/cwd/branch
// onto the active session during a switch.
//
// This test documents the invariant: the function-level gate stays open
// for session.info, but the state-patch path in the event handler checks
// explicitSid separately.
expect(gatewayEventRequiresSessionId('session.info')).toBe(false)
})
})
66 changes: 66 additions & 0 deletions apps/desktop/src/store/composer-queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,3 +168,69 @@ describe('shouldAutoDrain', () => {
expect(shouldAutoDrain({ isBusy: false, queueLength: 0 })).toBe(false)
})
})

describe('Race 5: source session binding', () => {
beforeEach(() => {
window.localStorage.removeItem(QUEUE_STORAGE_KEY)
$queuedPromptsBySession.set({})
})

it('stamps sourceRuntimeId and sourceStoredId onto queued entries', () => {
const entry = enqueueQueuedPrompt(SESSION_KEY, {
attachments: [],
text: 'race 5 test',
sourceRuntimeId: 'rt-session-a',
sourceStoredId: 'stored-session-a'
})

expect(entry).not.toBeNull()
expect(entry?.sourceRuntimeId).toBe('rt-session-a')
expect(entry?.sourceStoredId).toBe('stored-session-a')

const queue = getQueuedPrompts(SESSION_KEY)
expect(queue[0]?.sourceRuntimeId).toBe('rt-session-a')
expect(queue[0]?.sourceStoredId).toBe('stored-session-a')
})

it('allows entries without source session ids (backward compatible)', () => {
const entry = enqueueQueuedPrompt(SESSION_KEY, {
attachments: [],
text: 'legacy entry'
})

expect(entry).not.toBeNull()
expect(entry?.sourceRuntimeId).toBeUndefined()
expect(entry?.sourceStoredId).toBeUndefined()
})

it('persists source session ids into local storage', () => {
enqueueQueuedPrompt(SESSION_KEY, {
attachments: [],
text: 'persist source ids',
sourceRuntimeId: 'rt-persist',
sourceStoredId: 'stored-persist'
})

const raw = window.localStorage.getItem(QUEUE_STORAGE_KEY)
expect(raw).toBeTruthy()

const parsed = JSON.parse(String(raw)) as Record<string, { sourceRuntimeId?: string; sourceStoredId?: string; text: string }[]>
expect(parsed[SESSION_KEY]?.[0]?.sourceRuntimeId).toBe('rt-persist')
expect(parsed[SESSION_KEY]?.[0]?.sourceStoredId).toBe('stored-persist')
})

it('survives migration with source ids intact', () => {
enqueueQueuedPrompt('rt-old', {
attachments: [],
text: 'migrate me',
sourceRuntimeId: 'rt-original',
sourceStoredId: 'stored-original'
})

migrateQueuedPrompts('rt-old', 'rt-new')

const queue = getQueuedPrompts('rt-new')
expect(queue[0]?.sourceRuntimeId).toBe('rt-original')
expect(queue[0]?.sourceStoredId).toBe('stored-original')
})
})
22 changes: 20 additions & 2 deletions apps/desktop/src/store/composer-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,17 @@ export interface QueuedPromptEntry {
text: string
attachments: ComposerAttachment[]
queuedAt: number
/**
* The runtime session id that was active when this entry was enqueued.
* Used to bind the drain submit to the source session so a queued prompt
* never fires into a different session after a session switch (Race 5).
*/
sourceRuntimeId?: string
/**
* The stored session id that was active when this entry was enqueued.
* Paired with sourceRuntimeId for session.resume retry paths.
*/
sourceStoredId?: string
}

type QueueState = Record<string, QueuedPromptEntry[]>
Expand Down Expand Up @@ -80,7 +91,12 @@ export const getQueuedPrompts = (key: string | null | undefined): QueuedPromptEn

export const enqueueQueuedPrompt = (
key: string | null | undefined,
payload: { text: string; attachments: ComposerAttachment[] }
payload: {
text: string
attachments: ComposerAttachment[]
sourceRuntimeId?: string
sourceStoredId?: string
}
): null | QueuedPromptEntry => {
const sid = sidOf(key)

Expand All @@ -92,7 +108,9 @@ export const enqueueQueuedPrompt = (
id: nextId(),
text: payload.text,
attachments: cloneAttachments(payload.attachments),
queuedAt: Date.now()
queuedAt: Date.now(),
sourceRuntimeId: payload.sourceRuntimeId,
sourceStoredId: payload.sourceStoredId
}

writeSession(sid, [...queueFor(sid), entry])
Expand Down
Loading