From ccf33e8b982432499f2df87e20b9c32fa5b64174 Mon Sep 17 00:00:00 2001 From: Arvin Xu Date: Tue, 2 Jun 2026 13:23:19 +0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix(agent-runtime):=20classify?= =?UTF-8?q?=20topic/agent/session=20FK=20violations=20as=20ConversationPar?= =?UTF-8?q?entMissing=20(#15408)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a user deletes a topic (or agent/session/thread) while an agent operation is still running, the assistant/tool-message INSERT fails with a Postgres 23503 foreign_key_violation on the corresponding `messages` FK. The persist-error guard only recognised the `messages_parent_id_messages_id_fk` self-FK, so every other reference deletion slipped through as a raw `Failed query: insert into "messages"` 500 — surfacing to the user as a driver/SQL error and polluting the error dashboard as DatabasePersistError noise (one of the longest-standing top error categories). Generalise `isParentMessageMissingError` → `isMidOperationReferenceMissingError` to match a 23503 violation on any of the mid-operation-deletable `messages` references (parent / quota message, topic, agent, session, thread). These all mean "the referenced context was deleted mid-flight" — a lost race against the user, not a runtime failure — so they are normalised to the typed, user-side `ConversationParentMissing` error like the parent case already was. Out-of-scope FKs (e.g. `messages_user_id_users_id_fk`, other tables) stay real failures. Co-authored-by: Claude Opus 4.8 --- .../modules/AgentRuntime/RuntimeExecutors.ts | 8 ++-- .../__tests__/messagePersistErrors.test.ts | 45 +++++++++++++------ .../AgentRuntime/messagePersistErrors.ts | 44 +++++++++++++----- 3 files changed, 70 insertions(+), 27 deletions(-) diff --git a/src/server/modules/AgentRuntime/RuntimeExecutors.ts b/src/server/modules/AgentRuntime/RuntimeExecutors.ts index f843f75af6..3c3e570dc8 100644 --- a/src/server/modules/AgentRuntime/RuntimeExecutors.ts +++ b/src/server/modules/AgentRuntime/RuntimeExecutors.ts @@ -96,7 +96,7 @@ import { formatErrorEventData } from './formatErrorEventData'; import { classifyLLMError, type LLMErrorKind } from './llmErrorClassification'; import { createConversationParentMissingError, - isParentMessageMissingError, + isMidOperationReferenceMissingError, isPersistFatal, markPersistFatal, } from './messagePersistErrors'; @@ -2189,7 +2189,7 @@ export const createRuntimeExecutors = ( // Normalize BEFORE publishing so clients (which treat `error` stream // events as terminal and surface `event.data.error` directly) see the // typed business error, not the raw SQL / driver text. - const fatal = isParentMessageMissingError(error) + const fatal = isMidOperationReferenceMissingError(error) ? createConversationParentMissingError(payload.parentMessageId, error) : error instanceof Error ? error @@ -2696,7 +2696,7 @@ export const createRuntimeExecutors = ( // events as terminal and surface `event.data.error` directly, so // a raw SQL error here would leak driver text to the user before // the ConversationParentMissing throw is consumed. See . - const fatal = isParentMessageMissingError(error) + const fatal = isMidOperationReferenceMissingError(error) ? createConversationParentMissingError(parentMessageId, error) : error instanceof Error ? error @@ -3440,7 +3440,7 @@ export const createRuntimeExecutors = ( ); // Normalize BEFORE publishing so clients surface the typed business // error instead of the raw driver text (see review). - const fatal = isParentMessageMissingError(error) + const fatal = isMidOperationReferenceMissingError(error) ? createConversationParentMissingError(parentMessageId, error) : error instanceof Error ? error diff --git a/src/server/modules/AgentRuntime/__tests__/messagePersistErrors.test.ts b/src/server/modules/AgentRuntime/__tests__/messagePersistErrors.test.ts index 92ee1107ac..7b2526ba53 100644 --- a/src/server/modules/AgentRuntime/__tests__/messagePersistErrors.test.ts +++ b/src/server/modules/AgentRuntime/__tests__/messagePersistErrors.test.ts @@ -2,42 +2,61 @@ import { describe, expect, it } from 'vitest'; import { createConversationParentMissingError, - isParentMessageMissingError, + isMidOperationReferenceMissingError, isPersistFatal, markPersistFatal, } from '../messagePersistErrors'; -describe('isParentMessageMissingError', () => { +describe('isMidOperationReferenceMissingError', () => { it('matches the drizzle + postgres-js error shape (FK via .cause)', () => { const error: any = new Error('Failed query: insert into messages ...'); error.cause = { code: '23503', constraint: 'messages_parent_id_messages_id_fk' }; - expect(isParentMessageMissingError(error)).toBe(true); + expect(isMidOperationReferenceMissingError(error)).toBe(true); }); it('matches top-level code/constraint_name variants', () => { const error: any = new Error('x'); error.code = '23503'; error.constraint_name = 'messages_parent_id_messages_id_fk'; - expect(isParentMessageMissingError(error)).toBe(true); + expect(isMidOperationReferenceMissingError(error)).toBe(true); }); - it('does not match other FK violations (different constraint)', () => { + it.each([ + 'messages_parent_id_messages_id_fk', + 'messages_quota_id_messages_id_fk', + 'messages_topic_id_topics_id_fk', + 'messages_agent_id_agents_id_fk', + 'messages_session_id_sessions_id_fk', + 'messages_thread_id_threads_id_fk', + ])('matches every mid-operation-deletable reference FK: %s', (constraint) => { const error: any = new Error('x'); - error.cause = { code: '23503', constraint: 'messages_topic_id_topics_id_fk' }; - expect(isParentMessageMissingError(error)).toBe(false); + error.cause = { code: '23503', constraint }; + expect(isMidOperationReferenceMissingError(error)).toBe(true); + }); + + it('does not match FK violations on out-of-scope constraints', () => { + // user-account deletion / non-messages tables stay real failures + const userFk: any = new Error('x'); + userFk.cause = { code: '23503', constraint: 'messages_user_id_users_id_fk' }; + expect(isMidOperationReferenceMissingError(userFk)).toBe(false); + + const otherTable: any = new Error('x'); + otherTable.cause = { code: '23503', constraint: 'files_user_id_users_id_fk' }; + expect(isMidOperationReferenceMissingError(otherTable)).toBe(false); }); it('does not match non-FK pg errors', () => { const error: any = new Error('x'); error.cause = { code: '23505', constraint: 'messages_parent_id_messages_id_fk' }; - expect(isParentMessageMissingError(error)).toBe(false); + expect(isMidOperationReferenceMissingError(error)).toBe(false); }); - it('handles null / non-object safely', () => { - expect(isParentMessageMissingError(null)).toBe(false); - expect(isParentMessageMissingError(undefined)).toBe(false); - expect(isParentMessageMissingError('string-error')).toBe(false); - expect(isParentMessageMissingError(42)).toBe(false); + it('handles null / non-object / missing-constraint safely', () => { + expect(isMidOperationReferenceMissingError(null)).toBe(false); + expect(isMidOperationReferenceMissingError(undefined)).toBe(false); + expect(isMidOperationReferenceMissingError('string-error')).toBe(false); + expect(isMidOperationReferenceMissingError(42)).toBe(false); + expect(isMidOperationReferenceMissingError({ code: '23503' })).toBe(false); }); }); diff --git a/src/server/modules/AgentRuntime/messagePersistErrors.ts b/src/server/modules/AgentRuntime/messagePersistErrors.ts index 38a46959a5..cd0424934e 100644 --- a/src/server/modules/AgentRuntime/messagePersistErrors.ts +++ b/src/server/modules/AgentRuntime/messagePersistErrors.ts @@ -8,11 +8,25 @@ import { AgentRuntimeErrorType } from '@lobechat/types'; const PG_FOREIGN_KEY_VIOLATION = '23503'; /** - * Constraint name drizzle generates for the `messages.parent_id` self-FK. - * Hard-coded because we only use it as a signature — no need to reflect over - * the schema at runtime. + * Constraint names drizzle generates for the `messages` foreign keys that point + * at rows a user can delete *while an operation is still running* — the parent / + * quota message, the topic, the agent, the session, the thread. When any of + * these rows is removed mid-flight, the assistant/tool-message INSERT fails with + * a 23503 foreign_key_violation. That's a lost race against the user, not a + * runtime bug. + * + * Hard-coded because we only use them as signatures — no need to reflect over + * the schema at runtime. An entry for a constraint that doesn't exist is a + * harmless no-op. */ -const MESSAGES_PARENT_FK_CONSTRAINT = 'messages_parent_id_messages_id_fk'; +const MID_OPERATION_DELETABLE_FK_CONSTRAINTS = new Set([ + 'messages_parent_id_messages_id_fk', // parent message deleted + 'messages_quota_id_messages_id_fk', // quota (root) message deleted + 'messages_topic_id_topics_id_fk', // topic deleted + 'messages_agent_id_agents_id_fk', // agent deleted + 'messages_session_id_sessions_id_fk', // session deleted + 'messages_thread_id_threads_id_fk', // thread deleted +]); /** * Internal property the runtime uses to mark a thrown error as coming from @@ -23,15 +37,21 @@ const MESSAGES_PARENT_FK_CONSTRAINT = 'messages_parent_id_messages_id_fk'; export const PERSIST_FATAL_MARKER = 'persistFatal'; /** - * Detect whether an error returned by `messageModel.create` is a `parent_id` - * FK violation — meaning the parent message no longer exists. Most commonly - * caused by the parent being deleted concurrently with agent execution - * (see ). + * Detect whether an error returned by `messageModel.create` is a foreign-key + * violation on one of the mid-operation-deletable `messages` references — the + * parent / quota message, topic, agent, session or thread no longer exists, + * almost always because the user deleted that context concurrently with agent + * execution. + * + * Previously this only matched the `parent_id` self-FK, so topic / agent / etc. + * deletions slipped through as raw `Failed query: insert into "messages"` 500s + * (DatabasePersistError noise on the dashboard) instead of the typed user-side + * error. * * `drizzle` + `postgres-js` wrap the raw PG error as `.cause`, so the check * looks at both the top level and the cause. */ -export const isParentMessageMissingError = (error: unknown): boolean => { +export const isMidOperationReferenceMissingError = (error: unknown): boolean => { if (!error || typeof error !== 'object') return false; const err = error as any; const code = err?.code ?? err?.cause?.code; @@ -40,7 +60,11 @@ export const isParentMessageMissingError = (error: unknown): boolean => { err?.constraint ?? err?.cause?.constraint_name ?? err?.cause?.constraint; - return code === PG_FOREIGN_KEY_VIOLATION && constraint === MESSAGES_PARENT_FK_CONSTRAINT; + return ( + code === PG_FOREIGN_KEY_VIOLATION && + typeof constraint === 'string' && + MID_OPERATION_DELETABLE_FK_CONSTRAINTS.has(constraint) + ); }; /**