fix(chat): scope stop requests to one stream (#15497)

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
WenxiandClaude Opus 5.5 authored and GitHub committed 2026-10-05 17:54:28 +00:00
1 parent 1a894fcf47
commit 26185be857
25 files changed
+282 -263

No files matched your search

+1 -1
View File
@@ -25,7 +25,7 @@ Port the Onyx **web chat experience** to the Onyx **mobile app** (React Native 0
- **Packet protocol**: wrapper `{ placement:{turn_index,tab_index?,sub_turn_index?,model_index?}, obj:{type,...} }`. 40+ packet types (pure TS) in `web/src/app/app/services/streamingModels.ts`. **Minimal core needs only** `MESSAGE_START/DELTA/END`, `STOP`, `ERROR`, `MessageResponseIDInfo`, `CreateChatSessionID`.
- **Pure-TS, React-free**: `web/src/app/app/services/messageTree.ts` (`upsertMessages`, `getLatestMessageChain`, `buildImmediateMessages`, …). Core types in `web/src/app/app/interfaces.ts`.
- **React-coupled** (must be reimplemented in RN): packet→display transform `…/timeline/hooks/usePacketProcessor.ts`; markdown `web/src/components/chat/MinimalMarkdown.tsx` (react-markdown + rehype); scroll/toolbar/editing UI.
- Sessions: `POST /api/chat/create-chat-session {persona_id(=0 default), description, project_id}` → `{chat_session_id}`. Detail returns `messages[]` + `packets[][]` (replay) + `current_run`. Resume in-flight: `GET /api/chat/chat-session/{id}/resume-stream?cursor=n`. Stop: `POST /api/chat/stop-chat-session/{id}`.
- Sessions: `POST /api/chat/create-chat-session {persona_id(=0 default), description, project_id}` → `{chat_session_id}`. Detail returns `messages[]` + `packets[][]` (replay) + `current_stream`. Resume in-flight: `GET /api/chat/chat-session/{id}/resume-stream?cursor=n`. Stop: `POST /api/chat/stop-chat-session/{id}`.
- **Agents**: `GET /api/persona` → `MinimalAgent[]` (`id,name,description,tools[],starter_messages,icon_name,uploaded_image_id,builtin_persona,is_public,is_featured,display_priority`). Selection is **implicit** — session carries `persona_id`; `sendMessage` has **no** agent param. Types `web/src/lib/agents/types.ts`. Default persona id=0.
- **Projects**: `GET /api/user/projects` → `Project[] {id,name,instructions,chat_sessions[]}`; `ChatSession.project_id`. Files many-to-many. Upload `POST /api/user/projects/file/upload` (multipart, field `files`, `project_id?`, `temp_id_map?`) → `{user_files: ProjectFile[], rejected_files[]}`. `ProjectFile {id,file_id,name,status,chat_file_type,token_count,…}`; `UserFileStatus` enum. Link/unlink `POST`/`DELETE /api/user/projects/{pid}/files/{fid}`. Service `web/src/app/app/projects/projectsService.ts`; state `web/src/providers/ProjectsContext.tsx` (optimistic temp_id, **3s status polling**).
- **Attachments**: `FileDescriptor {id, type:ChatFileType, name?, user_file_id?}`; `ChatFileType` enum (`image/document/plain_text/tabular/user_knowledge`). Pure helper `projectFilesToFileDescriptors()` `web/src/app/app/services/fileUtils.ts`. Sent message carries `file_descriptors[]`. **Send is gated** until uploaded files are indexed (`token_count != null`). Image preview via `GET /api/chat/file/{file_id}`.
+2 -2
View File
@@ -141,10 +141,10 @@ PR 3 is the spine; PR 4/5/6 and all of PR 9 fan out from it independently. PR 7
- **Depends on:** PR 3.
- **Feature-flag state:** N/A.
- **Tests on merge:** RN Testing Library — resume re-attaches to a mocked live run; pagination loads older mocked pages; guard prevents cross-session writes.
- **Before you start (grill on):** `resume-stream` cursor semantics + `current_run` shape; how to detect a live run on open. Auto-name endpoint + trigger timing. Which endpoint/params page older messages.
- **Before you start (grill on):** `resume-stream` cursor semantics + `current_stream` shape; how to detect a live run on open. Auto-name endpoint + trigger timing. Which endpoint/params page older messages.
- **Drift checkpoint:** Confirm resume is still wanted for v1 (it's polish; could defer if scope tightens).
- **As built (2026-07-01, "grilled before coding" — scope tightened to resume + auto-name):** The grill collapsed the "history pagination" line item. **Within-session message pagination was DROPPED** — web has none (it loads the whole `get-chat-session` snapshot and lets virtualization handle long chats; there is no older-messages endpoint to page against), and mobile already gets the full conversation in one call + FlashList already virtualizes. Implementing `onStartReached` would have been a mobile-only divergence, so `MessageList.tsx` is **unchanged**. The **session-list** cursor (sidebar recents, built in PR 1) keeps its `before=time_updated` cursor as-is — the owner chose to leave the rare same-timestamp tie rather than add a `(time_updated, id)` compound cursor to the shared backend route (the chat port stays **backend-free**; comment updated in `api/chat/sessions.ts`). So PR 4 = **resume in-flight run + auto-name** only (~190 source LOC + tests, under the ~450 est).
- **Resume (faithful web port):** new `hooks/useChatSessionController.ts` — a module-scope `runResumeStream` (survives re-renders, like PR 3's `runChatStream`) + a module `resumingRuns: Set<number>` dedupe. The hook is a **read-only observer** (`useQuery` `enabled:false`) of the `get-chat-session` snapshot that `useChatController` fetches (same query key → React Query dedups; no second network call). When the snapshot's `current_run.run_id` maps to an **assistant** node (single-model only — a multi-model run_id is the user message and fails the node-type check, matching web), it re-attaches: `resumeChatMessage(sessionId, cursor=0, signal)` (full buffer replay + live tail), ~50 ms batched flush, a `stillCurrent = currentSessionId===sessionId && !aborted` guard (drops writes once the user switches away). **Heartbeats pass through on the resume path** (web does the same via `resumeStream`, unlike send) so the loop re-checks focus during quiet phases and releases the connection promptly on navigate-away; they're excluded from render via `isHeartbeat`. On finish/error (**404 = nothing to resume**, swallowed by the bare catch) it settles from `get-chat-session` (`processRawChatHistory` → `hydrateSession`) and `setQueryData`s the fresh snapshot so a remount can't re-resume a finished run. **abortController is reused** (not a second field): send and resume are mutually exclusive per session (resume only fires on cold-open hydration, guarded by `data.abortController == null`), so PR 3's existing `stop()` aborts a resume for free. The settle-block guards `hydrateSession`/`setQueryData`/controller-release on **`stillOurs()` — the resume's own `AbortController` object used as an ownership token held through the settle await** — so a send that races in (and even completes) during the `get-chat-session` fetch is not clobbered by the now-stale snapshot (a completed send leaves a `null` controller, which ≠ the resume's token).
- **Resume (faithful web port):** new `hooks/useChatSessionController.ts` — a module-scope `runResumeStream` (survives re-renders, like PR 3's `runChatStream`) + a module `resumingStreams: Set<number>` dedupe. The hook is a **read-only observer** (`useQuery` `enabled:false`) of the `get-chat-session` snapshot that `useChatController` fetches (same query key → React Query dedups; no second network call). When the snapshot's `current_stream.stream_id` maps to an **assistant** node (single-model only — a multi-model stream_id is the user message and fails the node-type check, matching web), it re-attaches: `resumeChatMessage(sessionId, cursor=0, signal)` (full buffer replay + live tail), ~50 ms batched flush, a `stillCurrent = currentSessionId===sessionId && !aborted` guard (drops writes once the user switches away). **Heartbeats pass through on the resume path** (web does the same via `resumeStream`, unlike send) so the loop re-checks focus during quiet phases and releases the connection promptly on navigate-away; they're excluded from render via `isHeartbeat`. On finish/error (**404 = nothing to resume**, swallowed by the bare catch) it settles from `get-chat-session` (`processRawChatHistory` → `hydrateSession`) and `setQueryData`s the fresh snapshot so a remount can't re-resume a finished run. **abortController is reused** (not a second field): send and resume are mutually exclusive per session (resume only fires on cold-open hydration, guarded by `data.abortController == null`), so PR 3's existing `stop()` aborts a resume for free. The settle-block guards `hydrateSession`/`setQueryData`/controller-release on **`stillOurs()` — the resume's own `AbortController` object used as an ownership token held through the settle await** — so a send that races in (and even completes) during the `get-chat-session` fetch is not clobbered by the now-stale snapshot (a completed send leaves a `null` controller, which ≠ the resume's token).
- **Auto-name:** `runChatStream` now takes an `AutoNameContext | null` (set only when `sessionId == null` at submit — a brand-new session). In its `finally`, when the run produced an answer (`!signal.aborted && sawStreaming && !hadError`), it fires `nameNewSession`: a 200 ms delay (mirrors web's `handleNewSessionNaming` — backend needs a beat to persist the row), then `renameChatSession` → `PUT /chat/rename-chat-session {name:null}` (backend LLM-generates the title), then invalidates `chatSessions` so the sidebar + the header title (which reads from `useChatSessions()`) update and the new chat appears. `api/chat/sessions.ts` gains `renameChatSession`; `stream.ts` refactors the NDJSON reader into a shared `readNdjson` used by both send + resume, adds `resumeChatMessage` (GET, bearer) + `StreamHttpError`.
- **Divergences from web (per WEB-PARITY PRINCIPLE):** (1) no within-session message pagination (neither does web — parity preserved); (2) auto-name fires only after a **new** session's first answer — web additionally has a "catch-up" rename on the 2nd message if the first naming failed (`useChatSessionController.ts` `newMessageHistory.length >= 2 && !description`); mobile defers that minor recovery path; (3) mobile reuses the single `abortController` for resume (web uses a dedicated one) — safe because the two are mutually exclusive per session on mobile.
- **Files:** `hooks/useChatSessionController.ts` (new), `hooks/__tests__/useChatSessionController.test.tsx` (new, 9 tests); modified `hooks/useChatController.ts` (auto-name), `api/chat/stream.ts` (`resumeChatMessage` + `readNdjson` refactor + `keepHeartbeats` + exported `isHeartbeat`), `api/chat/sessions.ts` (`renameChatSession` + cursor comment), `components/chat/ChatConversation.tsx` (mounts the resume hook), `hooks/__tests__/useChatController.test.tsx` (+3 auto-name tests). **Web untouched; no backend change.** Verified: mobile `tsc` clean, lint clean (2 pre-existing `_layout.tsx` warnings), **141 jest pass** (12 new). **Two adversarial multi-agent reviews** (each: 3 dimensions × independent verification). Review 1 (19 findings) surfaced 2 confirmed-real bugs, FIXED — (1) the settle-refetch could clobber a send that completed during its await → fixed with the `stillOurs()` ownership-token guard above; (2) heartbeats were filtered on the resume path, dropping web's quiet-phase liveness re-check → fixed with `keepHeartbeats` passthrough (a stop-during-resume `setQueryData` "uncertain" was resolved by the same guard). Review 2 (post-fix) confirmed **both fixes correct (0 fix-correctness findings)** and the comment cut behavior-neutral; its only actionable items were code-quality nits — applied: dropped an unused `readNdjson(signal)` param, tightened the auto-name comment (in-band error packets still name, web-faithfully), and added the observer post-mount-reactivity test. **A GitHub-bot review round** (Greptile + cubic, 4 P2 nits) followed: `authHeaders` no longer sends `Content-Type` on the bodyless resume GET (added only on the send POST); `FLUSH_INTERVAL_MS` extracted to `chat/constants.ts` (shared by both stream hooks); the observer `queryFn` uses `skipToken` instead of a non-null assertion; and the resume catch now logs non-404 failures (`StreamHttpError` reintroduced so the expected 404 stays quiet). **143 jest pass.** On-device resume/auto-name against a live backend is the remaining manual check.