Compare commits
2 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
 agent_coder
|
96faa28220 | ||
|
agent_coder
|
654ba9f249 |
@@ -1,140 +0,0 @@
|
||||
/**
|
||||
* gitmost #401 — regression test for the connect-vs-unload race in
|
||||
* @hocuspocus/server 3.4.4 (patched via patches/@hocuspocus__server@3.4.4.patch).
|
||||
*
|
||||
* The race (unpatched): when the last client disconnects, storeDocumentHooks'
|
||||
* `finally` schedules an async `unloadDocument`. That unload runs its
|
||||
* `beforeUnloadDocument` hooks asynchronously and, meanwhile, records an
|
||||
* in-flight promise in `this.unloadingDocuments`. In the original 3.4.4
|
||||
* `createDocument`, a NEW connection arriving in that window falls straight
|
||||
* through to the `loadingDocuments`/`documents` checks — it never consults
|
||||
* `unloadingDocuments`. So the new connection can start loading (or reuse) a
|
||||
* document while the old instance is still being torn down; the re-check inside
|
||||
* unload (`shouldUnloadDocument`, which sees 0 connections because async auth
|
||||
* hooks have not registered the new connection yet) then deletes/destroys the
|
||||
* doc out from under the freshly-connected client → orphaned Document → later
|
||||
* redis-sync takes the "doc not loaded" path → sync never completes → the
|
||||
* provider hangs until its ~25s timeout.
|
||||
*
|
||||
* The patch: `createDocument` first awaits any in-flight
|
||||
* `unloadingDocuments.get(name)` before proceeding. Once that settles, the
|
||||
* decision is deterministic — either the doc was fully unloaded (gone from
|
||||
* `documents`, so a clean fresh load) or the unload aborted (healthy doc still
|
||||
* in `documents`, reused). The new connection can never hand-shake onto an
|
||||
* about-to-be-destroyed Document.
|
||||
*
|
||||
* These tests exercise the REAL patched `Hocuspocus.createDocument` (the class
|
||||
* is directly constructible) by seeding `unloadingDocuments` with a controllable
|
||||
* in-flight unload and observing that createDocument waits for it.
|
||||
*/
|
||||
import { Hocuspocus } from '@hocuspocus/server';
|
||||
|
||||
// A promise we can resolve on demand, to model an unload that is mid-flight.
|
||||
function deferred<T = void>() {
|
||||
let resolve!: (v: T) => void;
|
||||
const promise = new Promise<T>((r) => {
|
||||
resolve = r;
|
||||
});
|
||||
return { promise, resolve };
|
||||
}
|
||||
|
||||
describe('gitmost #401 — hocuspocus createDocument awaits in-flight unload', () => {
|
||||
it('does NOT start loading a new doc until the in-flight unload settles, then loads fresh', async () => {
|
||||
const hp = new Hocuspocus();
|
||||
const name = 'page.race';
|
||||
|
||||
// Observe loadDocument: on the unpatched code it is invoked synchronously
|
||||
// within createDocument (before the unload settles); on the patched code it
|
||||
// must be deferred until unloadingDocuments resolves.
|
||||
const freshDoc = { name, __fresh: true } as any;
|
||||
const loadSpy = jest
|
||||
.spyOn(hp as any, 'loadDocument')
|
||||
.mockResolvedValue(freshDoc);
|
||||
|
||||
// Model an unload in progress: an entry sits in unloadingDocuments and, when
|
||||
// it completes, it removes the doc from `documents` (a real full unload).
|
||||
const unload = deferred();
|
||||
(hp as any).documents.set(name, { name, __dying: true });
|
||||
(hp as any).unloadingDocuments.set(
|
||||
name,
|
||||
unload.promise.then(() => {
|
||||
(hp as any).documents.delete(name);
|
||||
}),
|
||||
);
|
||||
|
||||
// Kick off a new connection's createDocument but do not await it yet.
|
||||
const createPromise = (hp as any).createDocument(
|
||||
name,
|
||||
{},
|
||||
'socket-1',
|
||||
{ isAuthenticated: true, readOnly: false },
|
||||
{},
|
||||
);
|
||||
|
||||
// Let all currently-schedulable microtasks run. The patched createDocument is
|
||||
// now parked on `await unloadingDocuments.get(name)`, so loadDocument must
|
||||
// NOT have been called yet, and it must NOT have returned the dying doc.
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
expect(loadSpy).not.toHaveBeenCalled();
|
||||
|
||||
// The unload completes (doc removed from `documents`).
|
||||
unload.resolve();
|
||||
|
||||
// createDocument now proceeds: sees no existing doc → fresh load.
|
||||
const doc = await createPromise;
|
||||
expect(loadSpy).toHaveBeenCalledTimes(1);
|
||||
expect(doc).toBe(freshDoc);
|
||||
// The freshly-loaded doc is the one registered — never the dying instance.
|
||||
expect((hp as any).documents.get(name)).toBe(freshDoc);
|
||||
});
|
||||
|
||||
it('reuses the live doc when the in-flight unload aborts (doc left in documents)', async () => {
|
||||
const hp = new Hocuspocus();
|
||||
const name = 'page.abort';
|
||||
|
||||
const loadSpy = jest.spyOn(hp as any, 'loadDocument');
|
||||
|
||||
// Model an unload that ABORTS (e.g. a new connection reappeared before the
|
||||
// sync re-check): it settles WITHOUT deleting the doc from `documents`.
|
||||
const unload = deferred();
|
||||
const liveDoc = { name, __live: true } as any;
|
||||
(hp as any).documents.set(name, liveDoc);
|
||||
(hp as any).unloadingDocuments.set(name, unload.promise); // no-op unload
|
||||
|
||||
const createPromise = (hp as any).createDocument(
|
||||
name,
|
||||
{},
|
||||
'socket-2',
|
||||
{ isAuthenticated: true, readOnly: false },
|
||||
{},
|
||||
);
|
||||
|
||||
unload.resolve();
|
||||
const doc = await createPromise;
|
||||
|
||||
// The still-present live doc is reused; no fresh load happened.
|
||||
expect(doc).toBe(liveDoc);
|
||||
expect(loadSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('no in-flight unload → behaves normally (fresh load)', async () => {
|
||||
const hp = new Hocuspocus();
|
||||
const name = 'page.normal';
|
||||
const freshDoc = { name } as any;
|
||||
const loadSpy = jest
|
||||
.spyOn(hp as any, 'loadDocument')
|
||||
.mockResolvedValue(freshDoc);
|
||||
|
||||
const doc = await (hp as any).createDocument(
|
||||
name,
|
||||
{},
|
||||
'socket-3',
|
||||
{ isAuthenticated: true, readOnly: false },
|
||||
{},
|
||||
);
|
||||
|
||||
expect(loadSpy).toHaveBeenCalledTimes(1);
|
||||
expect(doc).toBe(freshDoc);
|
||||
});
|
||||
});
|
||||
@@ -1,130 +0,0 @@
|
||||
/**
|
||||
* gitmost #401 fix 2 — onLoadDocument applies the DB state directly into the
|
||||
* hook's target document and returns undefined (instead of building a NEW Y.Doc
|
||||
* and returning it, which made hocuspocus re-encode+apply the whole state a
|
||||
* SECOND time on every cold load).
|
||||
*
|
||||
* These tests assert:
|
||||
* - the hook mutates `data.document` in place so its content equals the DB doc,
|
||||
* - onLoadDocument returns undefined (so hocuspocus keeps the mutated doc and
|
||||
* does NOT run its own applyUpdate(encodeStateAsUpdate(...)) merge),
|
||||
* - both the raw-ydoc branch and the json→ydoc conversion branch behave so.
|
||||
*
|
||||
* Returning undefined is the observable signal that the double-encode is gone
|
||||
* (the old code returned a new Y.Doc, which made hocuspocus re-encode+apply the
|
||||
* state a second time); we assert that contract rather than counting internal
|
||||
* encode calls, which is brittle given the encodes inside toYdoc and the test's
|
||||
* own `expected` fixtures.
|
||||
*/
|
||||
import * as Y from 'yjs';
|
||||
import { Document } from '@hocuspocus/server';
|
||||
import { TiptapTransformer } from '@hocuspocus/transformer';
|
||||
import { PersistenceExtension } from './persistence.extension';
|
||||
import { tiptapExtensions } from '../collaboration.util';
|
||||
|
||||
// A fresh hocuspocus Document (extends Y.Doc, adds isEmpty()) as hocuspocus
|
||||
// hands to onLoadDocument on a cold load.
|
||||
const freshDoc = () => new Document(`page.${PAGE_ID}`, {});
|
||||
|
||||
const PAGE_ID = '550e8400-e29b-41d4-a716-446655440000';
|
||||
|
||||
const doc = (text: string) => ({
|
||||
type: 'doc',
|
||||
content: [{ type: 'paragraph', content: [{ type: 'text', text }] }],
|
||||
});
|
||||
|
||||
const jsonOf = (ydoc: Y.Doc) =>
|
||||
TiptapTransformer.fromYdoc(ydoc, 'default');
|
||||
|
||||
describe('PersistenceExtension.onLoadDocument — #401 fix 2 (apply-into-hook-doc)', () => {
|
||||
let ext: PersistenceExtension;
|
||||
let pageRepo: { findById: jest.Mock };
|
||||
|
||||
beforeEach(() => {
|
||||
pageRepo = { findById: jest.fn() };
|
||||
ext = new PersistenceExtension(
|
||||
pageRepo as any,
|
||||
{} as any,
|
||||
{} as any,
|
||||
{} as any,
|
||||
{} as any,
|
||||
{} as any,
|
||||
{} as any,
|
||||
{} as any,
|
||||
);
|
||||
jest.spyOn(ext['logger'], 'debug').mockImplementation(() => undefined);
|
||||
jest.spyOn(ext['logger'], 'warn').mockImplementation(() => undefined);
|
||||
});
|
||||
|
||||
const load = (document: Document) =>
|
||||
ext.onLoadDocument({ documentName: `page.${PAGE_ID}`, document } as any);
|
||||
|
||||
it('raw ydoc branch: mutates the hook doc to the DB state and returns undefined', async () => {
|
||||
// Source doc representing the persisted ydoc state.
|
||||
const source = TiptapTransformer.toYdoc(
|
||||
doc('DB CONTENT'),
|
||||
'default',
|
||||
tiptapExtensions,
|
||||
);
|
||||
const dbState = Buffer.from(Y.encodeStateAsUpdate(source));
|
||||
pageRepo.findById.mockResolvedValue({ id: PAGE_ID, ydoc: dbState });
|
||||
|
||||
// The hook target is a fresh empty doc (as hocuspocus supplies on cold load).
|
||||
const target = freshDoc();
|
||||
const result = await load(target);
|
||||
|
||||
// Return undefined so hocuspocus keeps `target` as-is (no second merge).
|
||||
expect(result).toBeUndefined();
|
||||
// The hook document now carries the DB content.
|
||||
expect(jsonOf(target)).toEqual(jsonOf(source));
|
||||
});
|
||||
|
||||
it('json→ydoc branch: converts page.content into the hook doc and returns undefined', async () => {
|
||||
pageRepo.findById.mockResolvedValue({
|
||||
id: PAGE_ID,
|
||||
ydoc: null,
|
||||
content: doc('JSON CONTENT'),
|
||||
});
|
||||
|
||||
const target = freshDoc();
|
||||
const result = await load(target);
|
||||
|
||||
// Returning undefined is what keeps hocuspocus from re-encoding+applying the
|
||||
// state a second time (the old code returned the doc, forcing that extra
|
||||
// encode). We assert the observable contract here — the return value and the
|
||||
// resulting content — rather than counting internal encode calls, which is
|
||||
// brittle: toYdoc and the `expected` build below both encode too.
|
||||
expect(result).toBeUndefined();
|
||||
|
||||
// The converted content landed in the hook document.
|
||||
const expected = TiptapTransformer.toYdoc(
|
||||
doc('JSON CONTENT'),
|
||||
'default',
|
||||
tiptapExtensions,
|
||||
);
|
||||
expect(jsonOf(target)).toEqual(jsonOf(expected));
|
||||
});
|
||||
|
||||
it('live doc already non-empty: early return, no DB read', async () => {
|
||||
// A hocuspocus Document carrying live content (isEmpty('default') === false).
|
||||
const target = freshDoc();
|
||||
const live = TiptapTransformer.toYdoc(
|
||||
doc('LIVE'),
|
||||
'default',
|
||||
tiptapExtensions,
|
||||
);
|
||||
Y.applyUpdate(target, Y.encodeStateAsUpdate(live));
|
||||
|
||||
const result = await load(target);
|
||||
expect(result).toBeUndefined();
|
||||
expect(pageRepo.findById).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('no persisted state: leaves the fresh empty doc untouched, returns undefined', async () => {
|
||||
pageRepo.findById.mockResolvedValue({ id: PAGE_ID, ydoc: null, content: null });
|
||||
const target = freshDoc();
|
||||
const result = await load(target);
|
||||
expect(result).toBeUndefined();
|
||||
expect(target.isEmpty('default')).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -171,21 +171,15 @@ export class PersistenceExtension implements Extension {
|
||||
return;
|
||||
}
|
||||
|
||||
// #401 fix 2 — apply the DB state DIRECTLY into the hook's target document
|
||||
// (`document` === `data.document`) and return undefined. When onLoadDocument
|
||||
// returns undefined, hocuspocus keeps the mutated hook document as-is; only
|
||||
// when the hook RETURNS a Y.Doc does hocuspocus re-`applyUpdate(document,
|
||||
// encodeStateAsUpdate(returned))` — a second full encode+apply of the whole
|
||||
// (e.g. 315KB) state on every cold load. Mutating in place performs a single
|
||||
// apply and avoids the throwaway `new Y.Doc()` allocation.
|
||||
if (page.ydoc) {
|
||||
this.logger.debug(`ydoc loaded from db: ${pageId}`);
|
||||
|
||||
const doc = new Y.Doc();
|
||||
const dbState = new Uint8Array(page.ydoc);
|
||||
|
||||
Y.applyUpdate(document, dbState);
|
||||
Y.applyUpdate(doc, dbState);
|
||||
observeCollabLoad(dbState.length, (performance.now() - startedAt) / 1000);
|
||||
return;
|
||||
return doc;
|
||||
}
|
||||
|
||||
// if no ydoc state in db convert json in page.content to Ydoc.
|
||||
@@ -198,23 +192,18 @@ export class PersistenceExtension implements Extension {
|
||||
tiptapExtensions,
|
||||
);
|
||||
|
||||
// Encode the converted doc ONCE, reuse the bytes for both the size label
|
||||
// and the single apply into the hook document (previously this encode's
|
||||
// result was returned and hocuspocus re-encoded+applied it a second time).
|
||||
// Reuse this single encode for the size label (do NOT add a second one).
|
||||
const encoded = Y.encodeStateAsUpdate(ydoc);
|
||||
Y.applyUpdate(document, encoded);
|
||||
observeCollabLoad(
|
||||
encoded.byteLength,
|
||||
(performance.now() - startedAt) / 1000,
|
||||
);
|
||||
return;
|
||||
return ydoc;
|
||||
}
|
||||
|
||||
// No persisted state: the hook document is already a fresh empty Y.Doc, so
|
||||
// leave it untouched and return undefined (no re-encode of an empty doc).
|
||||
this.logger.debug(`creating fresh ydoc: ${pageId}`);
|
||||
observeCollabLoad(0, (performance.now() - startedAt) / 1000);
|
||||
return;
|
||||
return new Y.Doc();
|
||||
}
|
||||
|
||||
async onStoreDocument(data: onStoreDocumentPayload) {
|
||||
|
||||
@@ -148,6 +148,53 @@ describe('assistantParts', () => {
|
||||
expect(toolPart).not.toHaveProperty('output');
|
||||
});
|
||||
|
||||
it('replays the REAL error text for a THROWN tool (tool-error part)', () => {
|
||||
const steps = [
|
||||
{
|
||||
text: '',
|
||||
toolCalls: [
|
||||
{ toolCallId: 'c7', toolName: 'editPageText', input: { id: 'p1' } },
|
||||
],
|
||||
// A thrown tool is a `tool-error` content part; toolResults holds only
|
||||
// successes and stays empty for this call.
|
||||
toolResults: [],
|
||||
content: [
|
||||
{
|
||||
type: 'tool-error',
|
||||
toolCallId: 'c7',
|
||||
toolName: 'editPageText',
|
||||
input: { id: 'p1' },
|
||||
error: new Error('page is locked'),
|
||||
},
|
||||
],
|
||||
},
|
||||
];
|
||||
const parts = assistantParts(steps, '') as AnyPart[];
|
||||
const toolPart = parts.find((p) => p.type === 'tool-editPageText');
|
||||
expect(toolPart).toBeDefined();
|
||||
expect(toolPart!.state).toBe('output-error');
|
||||
// The REAL error is replayed, NOT the 'Tool call did not complete.' placeholder.
|
||||
expect(toolPart!.errorText).toBe('page is locked');
|
||||
expect(toolPart).not.toHaveProperty('output');
|
||||
});
|
||||
|
||||
it('keeps the placeholder ONLY for a call with neither result nor tool-error', () => {
|
||||
const steps = [
|
||||
{
|
||||
text: '',
|
||||
toolCalls: [
|
||||
{ toolCallId: 'c8', toolName: 'insertNode', input: { node: {} } },
|
||||
],
|
||||
toolResults: [],
|
||||
content: [], // aborted mid-step: no result AND no tool-error
|
||||
},
|
||||
];
|
||||
const parts = assistantParts(steps, '') as AnyPart[];
|
||||
const toolPart = parts.find((p) => p.type === 'tool-insertNode');
|
||||
expect(toolPart!.state).toBe('output-error');
|
||||
expect(toolPart!.errorText).toBe('Tool call did not complete.');
|
||||
});
|
||||
|
||||
it('skips malformed tool-calls (missing toolName or toolCallId)', () => {
|
||||
const steps = [
|
||||
{
|
||||
@@ -195,6 +242,45 @@ describe('serializeSteps', () => {
|
||||
expect(trace[0]).toEqual({ toolName: 'getPage', input: { id: 'p1' } });
|
||||
expect(trace[1]).toEqual({ toolName: 'getPage', output: { title: 'T' } });
|
||||
});
|
||||
|
||||
it('records a THROWN tool failure (tool-error part) with its error message', () => {
|
||||
const trace = serializeSteps([
|
||||
{
|
||||
toolCalls: [{ toolName: 'editPageText', input: { id: 'p1' } }],
|
||||
toolResults: [],
|
||||
content: [
|
||||
{
|
||||
type: 'tool-error',
|
||||
toolName: 'editPageText',
|
||||
error: new Error('page is locked'),
|
||||
},
|
||||
],
|
||||
},
|
||||
]) as Array<Record<string, unknown>>;
|
||||
// The call element is followed by a paired error element (mirroring how a
|
||||
// successful result is appended), so the failure survives in the trace.
|
||||
expect(trace).toHaveLength(2);
|
||||
expect(trace[0]).toEqual({ toolName: 'editPageText', input: { id: 'p1' } });
|
||||
expect(trace[1]).toEqual({
|
||||
toolName: 'editPageText',
|
||||
error: 'page is locked',
|
||||
});
|
||||
});
|
||||
|
||||
it('truncates a very long tool-error message to the tool-output limit', () => {
|
||||
const long = 'x'.repeat(5000);
|
||||
const trace = serializeSteps([
|
||||
{
|
||||
toolCalls: [{ toolName: 'editPageText', input: {} }],
|
||||
toolResults: [],
|
||||
content: [{ type: 'tool-error', toolName: 'editPageText', error: long }],
|
||||
},
|
||||
]) as Array<Record<string, unknown>>;
|
||||
const errorText = trace[1].error as string;
|
||||
// Truncated (not the full 5000 chars) and carries the omission marker.
|
||||
expect(errorText.length).toBeLessThan(long.length);
|
||||
expect(errorText).toContain('chars omitted');
|
||||
});
|
||||
});
|
||||
|
||||
describe('rowToUiMessage', () => {
|
||||
|
||||
@@ -1637,6 +1637,17 @@ type StepLike = {
|
||||
toolName?: string;
|
||||
output?: unknown;
|
||||
}>;
|
||||
// ai@6.0.134: a tool that THREW surfaces as a `tool-error` content part
|
||||
// ({ type:'tool-error', toolCallId, toolName, input, error }), NOT as a
|
||||
// `toolResults` entry (which holds only successes). Read from here so failed
|
||||
// calls are persisted with their real error instead of being dropped.
|
||||
content?: ReadonlyArray<{
|
||||
type?: string;
|
||||
toolCallId?: string;
|
||||
toolName?: string;
|
||||
input?: unknown;
|
||||
error?: unknown;
|
||||
}>;
|
||||
};
|
||||
|
||||
/**
|
||||
@@ -1739,6 +1750,26 @@ function compactValue(value: unknown, depth: number): unknown {
|
||||
return value;
|
||||
}
|
||||
|
||||
/**
|
||||
* Extract a bounded string message from a `tool-error` part's `error` field for
|
||||
* persistence and history replay. The field may be an `Error`, a string, or an
|
||||
* arbitrary object, so pull a message robustly. The result is passed through
|
||||
* `compactValue` so a very long error honors the SAME truncation limits the file
|
||||
* already applies to tool outputs (no new limit is introduced here).
|
||||
*/
|
||||
function normalizeToolError(error: unknown): string {
|
||||
const message =
|
||||
error instanceof Error
|
||||
? error.message
|
||||
: typeof error === 'string'
|
||||
? error
|
||||
: error != null &&
|
||||
typeof (error as { message?: unknown }).message === 'string'
|
||||
? (error as { message: string }).message
|
||||
: String(error);
|
||||
return compactValue(message, 0) as string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Rebuild the FULL UIMessage `parts` for an assistant turn from the SDK steps,
|
||||
* so multi-turn history replays prior tool-calls/results to the model (not just
|
||||
@@ -1771,6 +1802,14 @@ export function assistantParts(
|
||||
for (const r of step.toolResults ?? []) {
|
||||
if (r.toolCallId) resultsById.set(r.toolCallId, r.output);
|
||||
}
|
||||
// Index this step's THROWN tool failures (ai@6 `tool-error` content parts)
|
||||
// by tool call id, so a call that failed replays with its real error text.
|
||||
const errorsById = new Map<string, unknown>();
|
||||
for (const part of step.content ?? []) {
|
||||
if (part.type === 'tool-error' && part.toolCallId) {
|
||||
errorsById.set(part.toolCallId, part.error);
|
||||
}
|
||||
}
|
||||
for (const call of step.toolCalls ?? []) {
|
||||
if (!call.toolName || !call.toolCallId) continue;
|
||||
const hasResult = resultsById.has(call.toolCallId);
|
||||
@@ -1783,9 +1822,21 @@ export function assistantParts(
|
||||
input: call.input,
|
||||
output: compactToolOutput(resultsById.get(call.toolCallId)),
|
||||
});
|
||||
} else if (errorsById.has(call.toolCallId)) {
|
||||
// The tool THREW: replay the REAL error so the model on the next turn
|
||||
// knows WHY the call failed (and does not blindly repeat it). An
|
||||
// output-error round-trips through convertToModelMessages as a balanced
|
||||
// tool-call + tool-result, keeping the rebuilt history valid.
|
||||
parts.push({
|
||||
type: `tool-${call.toolName}`,
|
||||
toolCallId: call.toolCallId,
|
||||
state: 'output-error',
|
||||
input: call.input,
|
||||
errorText: normalizeToolError(errorsById.get(call.toolCallId)),
|
||||
});
|
||||
} else {
|
||||
// No paired result (e.g. aborted mid-step). Persisting a bare
|
||||
// tool-call (input-available) would replay as an unpaired call and
|
||||
// No paired result AND no tool-error (e.g. aborted mid-step). Persisting
|
||||
// a bare tool-call (input-available) would replay as an unpaired call and
|
||||
// throw MissingToolResultsError on the next turn (convertToModelMessages
|
||||
// emits no tool-result for it). Emit a SYNTHETIC paired result instead:
|
||||
// an output-error round-trips through convertToModelMessages as a
|
||||
@@ -2021,10 +2072,19 @@ export function serializeSteps(
|
||||
steps: ReadonlyArray<{
|
||||
toolCalls?: ReadonlyArray<{ toolName?: string; input?: unknown }>;
|
||||
toolResults?: ReadonlyArray<{ toolName?: string; output?: unknown }>;
|
||||
content?: ReadonlyArray<{
|
||||
type?: string;
|
||||
toolName?: string;
|
||||
error?: unknown;
|
||||
}>;
|
||||
}>,
|
||||
): unknown {
|
||||
const calls: Array<{ toolName?: string; input?: unknown; output?: unknown }> =
|
||||
[];
|
||||
const calls: Array<{
|
||||
toolName?: string;
|
||||
input?: unknown;
|
||||
output?: unknown;
|
||||
error?: string;
|
||||
}> = [];
|
||||
for (const step of steps ?? []) {
|
||||
for (const call of step.toolCalls ?? []) {
|
||||
calls.push({ toolName: call.toolName, input: call.input });
|
||||
@@ -2032,6 +2092,18 @@ export function serializeSteps(
|
||||
for (const r of step.toolResults ?? []) {
|
||||
calls.push({ toolName: r.toolName, output: compactToolOutput(r.output) });
|
||||
}
|
||||
// ai@6 surfaces a THROWN tool failure as a `tool-error` content part, NOT as
|
||||
// a `toolResults` entry. Record it as its own paired element (mirroring how a
|
||||
// successful result is appended) so the failure and its reason survive in the
|
||||
// trace instead of leaving an orphaned call with no result.
|
||||
for (const part of step.content ?? []) {
|
||||
if (part.type === 'tool-error') {
|
||||
calls.push({
|
||||
toolName: part.toolName,
|
||||
error: normalizeToolError(part.error),
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
return calls.length > 0 ? calls : null;
|
||||
}
|
||||
|
||||
+77
-39
@@ -13,12 +13,14 @@ Read the **Gotchas** section before you trust any error count.
|
||||
- Agent chats live in Postgres, DB `docmost`, tables `ai_chat_*`.
|
||||
- Each tool invocation is stored as **two** array elements (a `tool-call` part and
|
||||
a `tool-result` part), so naive counting double-counts.
|
||||
- **A tool that *throws* writes no result part at all.** Its error text is nowhere
|
||||
in the DB — not in `tool_calls`, `content`, or `metadata`. It is shown live in
|
||||
the UI only. So `isError` / `success=false` scans under-report by design.
|
||||
- To find where agents fail you need **three** sources: (1) soft-failure markers in
|
||||
`tool_calls`, (2) the orphan-gap proxy for thrown errors, (3) server logs / the
|
||||
live UI for the actual error text.
|
||||
- **A tool that *throws* writes no result part.** Since the #407 fix its error is
|
||||
persisted as a dedicated `{toolName, error}` element in `tool_calls` (queryable +
|
||||
replayed to the model). **Rows written before #407 still drop it** — the error is
|
||||
nowhere in the DB and shows only in the live UI. So `isError` / `success=false`
|
||||
scans under-report by design, and pre-#407 thrown errors are invisible.
|
||||
- To find where agents fail: (1) soft-failure markers in `tool_calls`, (2) the new
|
||||
`error` field for thrown errors (new rows) / the orphan-gap proxy (old rows),
|
||||
(3) server logs / the live UI for full stack traces beyond the truncated message.
|
||||
|
||||
## Where the data lives
|
||||
|
||||
@@ -61,13 +63,17 @@ index 0: { "toolName": "getPage", "input": { "pageId": "…" } } ← tool-ca
|
||||
index 1: { "toolName": "getPage", "output": { … } } ← tool-result (has output, NO input)
|
||||
```
|
||||
|
||||
The **only** keys that ever appear on an element are `toolName`, `input`, `output`.
|
||||
There is no `state`, no `errorText`, no `type`. Consequences:
|
||||
The keys that appear on an element are `toolName`, `input`, `output`, and — for a
|
||||
**thrown** failure on rows written after the #407 fix — `error` (the tool's error
|
||||
message; see the "Hard failures" section below). There is no `state`, no `errorText`,
|
||||
no `type`. On pre-#407 rows a thrown failure has NO paired result element at all
|
||||
(silent orphan). Consequences:
|
||||
|
||||
1. **Real invocation count = elements that have `output`.** Counting every element
|
||||
double-counts (you get ~2× and a spurious "~50% of every tool has no output").
|
||||
2. **Pairing:** a successful call = a `tool-call` part followed by its `tool-result`
|
||||
part. Both carry `toolName`, so you can group by tool on either.
|
||||
1. **Real invocation count = elements that have `output` or `error`.** Counting every
|
||||
element double-counts (you get ~2× and a spurious "~50% of every tool has no output").
|
||||
2. **Pairing:** a call = a `tool-call` part followed by its result part. A success
|
||||
carries `output`; a thrown failure (post-#407) carries `error` instead. Both carry
|
||||
`toolName`, so you can group by tool on either.
|
||||
|
||||
## The two classes of failure (and which the DB can see)
|
||||
|
||||
@@ -85,25 +91,37 @@ These are visible in the `tool-result` `output`. The marker differs per tool:
|
||||
Note `editPageText` returns `failed: []` on success — filtering on the *presence*
|
||||
of the key gives false positives; filter on **non-empty**.
|
||||
|
||||
### 2. Hard failures — tool THREW → NOT PERSISTED ❌ (the trap)
|
||||
### 2. Hard failures — tool THREW → NOW PERSISTED ✅ (since the #407 fix)
|
||||
|
||||
When a tool throws (the classic one is `patchNode` / `insertNode` / `tableUpdateCell`
|
||||
→ `Failed to encode document to Yjs (fromJSON): Unknown node type: undefined`), the
|
||||
runtime writes **no `tool-result` part**. The orphaned `tool-call` part stays, but
|
||||
the error text is **nowhere in the DB**. It is streamed to the UI live and (until
|
||||
rotation) to server logs — that is it.
|
||||
runtime still writes **no `tool-result` part** — the failure is an ai@6 `tool-error`
|
||||
content part instead. **Since the #407 fix, that error is persisted**: `serializeSteps`
|
||||
appends a dedicated element `{toolName, error: "<message>"}` right after the failed
|
||||
call, mirroring how a successful `{toolName, output}` element is appended. So a thrown
|
||||
error now leaves a queryable `error` field carrying its (truncated) reason, and the
|
||||
same real text is replayed to the model on the next turn (an `output-error` part with
|
||||
the real `errorText`, no longer the `'Tool call did not complete.'` placeholder).
|
||||
|
||||
So any query like `count(*) FILTER (WHERE output.success = false)` will happily
|
||||
return **0** for `patchNode` even when the chat is visibly full of red failures.
|
||||
That is survivorship bias, not reliability.
|
||||
**Cutover caveat — old rows keep the old blind shape.** Rows written **before** this
|
||||
change have the two-part shape (`call` + `output` only) and simply **drop** thrown
|
||||
errors, leaving a silent **orphan** (a `call` with no `output` *and* no `error`). Rows
|
||||
written **after** the fix additionally carry the `error` element. So:
|
||||
|
||||
The only DB-side proxy for a thrown error is an **orphan**: a `tool-call` part with
|
||||
no matching `tool-result`. Caveat: orphans also appear when a run is **aborted**
|
||||
mid-flight (server restart), so a high-volume tool (`createComment`, `searchInPage`,
|
||||
`Search_web_search`) shows orphans from aborts, not from real errors. Treat the
|
||||
orphan gap as an *upper bound* on hard errors, and cross-check the tool: a gap on a
|
||||
structural editor (`patchNode`, `insertNode`, `updatePageJson`, `transformPage`) is
|
||||
almost certainly a thrown Yjs-encode error; a gap on `createComment` is mostly aborts.
|
||||
- **New rows:** query the `error` field directly (see the hard-error query below) — no
|
||||
orphan heuristic needed for thrown failures.
|
||||
- **Old rows (pre-#407):** the only DB-side proxy is still an **orphan**: a `tool-call`
|
||||
part with no matching `tool-result` *and* no `error`. Orphans also appear when a run
|
||||
is **aborted** mid-flight (server restart), so a high-volume tool (`createComment`,
|
||||
`searchInPage`, `Search_web_search`) shows orphans from aborts, not real errors on
|
||||
old rows. Treat the orphan gap as an *upper bound*, and cross-check the tool: a gap on
|
||||
a structural editor (`patchNode`, `insertNode`, `updatePageJson`, `transformPage`) is
|
||||
almost certainly a thrown Yjs-encode error; a gap on `createComment` is mostly aborts.
|
||||
|
||||
A note on the aborted-call fallback: a call with **neither** a result **nor** a
|
||||
`tool-error` (genuinely interrupted mid-step) still replays with the
|
||||
`'Tool call did not complete.'` placeholder and persists as an orphan — that path is
|
||||
unchanged, and is distinct from a real thrown error, which now carries `error`.
|
||||
|
||||
### 3. Run-level failures → `ai_chat_runs`
|
||||
|
||||
@@ -164,14 +182,28 @@ WHERE jsonb_typeof(o->'failed') = 'array'
|
||||
GROUP BY 1 ORDER BY 2 DESC;
|
||||
```
|
||||
|
||||
**Hard-error proxy — orphan gap per tool, WITH a spread column** (call parts minus
|
||||
result parts, plus how many distinct chats the gap is spread across):
|
||||
**Hard errors — persisted `error` field per tool (NEW rows, since #407)** — thrown
|
||||
tool failures now carry their real reason, so query them directly:
|
||||
|
||||
```sql
|
||||
SELECT elem->>'toolName' AS tool, count(*) AS thrown_errors,
|
||||
min(elem->>'error') AS sample_error
|
||||
FROM ai_chat_messages m, jsonb_array_elements(m.tool_calls) elem
|
||||
WHERE jsonb_typeof(m.tool_calls) = 'array' AND elem ? 'error'
|
||||
GROUP BY 1 ORDER BY 2 DESC;
|
||||
```
|
||||
|
||||
**Hard-error proxy for OLD rows (pre-#407) — orphan gap per tool, WITH a spread column**
|
||||
(call parts minus result parts, plus how many distinct chats the gap is spread across).
|
||||
This covers rows written before thrown errors were persisted; on new rows a thrown
|
||||
failure now has its own `error` element (use the query above) and an orphan means only
|
||||
a genuinely aborted mid-step call:
|
||||
|
||||
```sql
|
||||
WITH parts AS (
|
||||
SELECT m.chat_id, elem->>'toolName' AS tool,
|
||||
(elem ? 'input' AND NOT (elem ? 'output')) AS is_call,
|
||||
(elem ? 'output') AS is_result
|
||||
(elem ? 'output' OR elem ? 'error') AS is_result
|
||||
FROM ai_chat_messages m, jsonb_array_elements(m.tool_calls) elem
|
||||
WHERE jsonb_typeof(m.tool_calls) = 'array' AND m.role = 'assistant'
|
||||
),
|
||||
@@ -188,8 +220,12 @@ HAVING sum(gap) FILTER (WHERE gap > 0) > 0
|
||||
ORDER BY missing_results DESC;
|
||||
```
|
||||
|
||||
**`missing_results` mixes thrown errors AND aborted/interrupted runs — you cannot
|
||||
split them from `output` alone** (a positional "what follows the orphan" heuristic
|
||||
The `is_result` predicate counts an `error` element as a paired result too, so on new
|
||||
rows a persisted thrown error no longer inflates the orphan gap; a remaining gap is an
|
||||
aborted/interrupted call.
|
||||
|
||||
**On OLD rows, `missing_results` mixes thrown errors AND aborted/interrupted runs — you
|
||||
cannot split them from `output` alone** (a positional "what follows the orphan" heuristic
|
||||
breaks on parallel tool batches, which persist as `call,call,…,result,result`). Use
|
||||
`chats_spread` to disambiguate:
|
||||
|
||||
@@ -244,18 +280,20 @@ docker compose -p gitmost logs -f --tail=100 # whole stack
|
||||
```
|
||||
|
||||
Logging is `json-file`, `max-size=10m max-file=5` → ~50 MB retained, then rotated,
|
||||
and **wiped on container recreate**. So thrown-tool error text is only reliably
|
||||
caught **in real time** (or in the live chat UI, which renders the failed part with
|
||||
its message). There is no durable, queryable store of hard tool errors today — if you
|
||||
need one, that is a feature to add (persist `output-error` parts, or emit a
|
||||
`tool_calls_total{tool,status}` metric to VictoriaMetrics).
|
||||
and **wiped on container recreate**. Since the #407 fix, thrown-tool error text is
|
||||
**persisted in the `error` field** of `tool_calls` (see the hard-error query above), so
|
||||
you no longer depend on live logs for it. Logs/live UI remain useful for **pre-#407
|
||||
rows** (whose thrown errors were dropped) and for full stack traces beyond the
|
||||
truncated stored message. A per-tool `tool_calls_total{tool,status}` metric to
|
||||
VictoriaMetrics is still a possible future add for aggregate dashboards.
|
||||
|
||||
## Gotchas checklist
|
||||
|
||||
- [ ] Counting every `tool_calls` element → **2× overcount**. Count elements with `output`.
|
||||
- [ ] `isError` / `success=false` ≈ 0 does **not** mean "no errors" — thrown errors aren't persisted.
|
||||
- [ ] Counting every `tool_calls` element → **overcount**. Count `output` elements; add `error` elements for thrown failures (new rows), but don't count both as invocations.
|
||||
- [ ] `isError` / `success=false` ≈ 0 does **not** mean "no errors" — thrown errors are a separate `error` element (new rows) or dropped entirely (pre-#407 rows).
|
||||
- [ ] Thrown errors persist only on rows written **after the #407 fix** — pre-#407 rows still drop them (orphan only). Mind the cutover when trending over time.
|
||||
- [ ] `editPageText.failed` is `[]` on success — test for **non-empty**, not presence.
|
||||
- [ ] Orphan gap mixes thrown errors **and** aborted runs — split by tool before concluding.
|
||||
- [ ] Orphan gap on OLD rows mixes thrown errors **and** aborted runs — split by tool. On NEW rows a thrown error is its own `error` element, so a gap ≈ aborted call.
|
||||
- [ ] `aborted` runs = server restarts, `failed` runs = provider overload — not agent mistakes.
|
||||
- [ ] Never dump a raw `tool_calls` cell — it can be hundreds of KB.
|
||||
- [ ] Logs are ephemeral (≤50 MB, wiped on recreate) — grab hard-error text live.
|
||||
|
||||
+1
-2
@@ -97,8 +97,7 @@
|
||||
"patchedDependencies": {
|
||||
"scimmy@1.3.5": "patches/scimmy@1.3.5.patch",
|
||||
"yjs@13.6.30": "patches/yjs@13.6.30.patch",
|
||||
"ai@6.0.134": "patches/ai@6.0.134.patch",
|
||||
"@hocuspocus/server@3.4.4": "patches/@hocuspocus__server@3.4.4.patch"
|
||||
"ai@6.0.134": "patches/ai@6.0.134.patch"
|
||||
},
|
||||
"overrides": {
|
||||
"prosemirror-changeset": "2.4.0",
|
||||
|
||||
@@ -1,62 +0,0 @@
|
||||
diff --git a/dist/hocuspocus-server.cjs b/dist/hocuspocus-server.cjs
|
||||
index b24ff6d091c32f733089eeaa47b03f7b37cf5964..f003af304fc751b7edc1aee17f3651282d70666a 100644
|
||||
--- a/dist/hocuspocus-server.cjs
|
||||
+++ b/dist/hocuspocus-server.cjs
|
||||
@@ -2426,6 +2426,26 @@ class Hocuspocus {
|
||||
* Create a new document by the given request
|
||||
*/
|
||||
async createDocument(documentName, request, socketId, connection, context) {
|
||||
+ // PATCH(gitmost #401): close the connect-vs-unload race. When the last
|
||||
+ // client disconnects, storeDocumentHooks' finally schedules an unload;
|
||||
+ // unloadDocument runs its beforeUnloadDocument hooks asynchronously and
|
||||
+ // records an in-flight promise in `unloadingDocuments`. A NEW connection
|
||||
+ // arriving in that window would otherwise fall straight through to the
|
||||
+ // `documents`/`loadingDocuments` checks and could either reuse a doc that
|
||||
+ // is about to be destroyed, or start loading a fresh doc concurrently
|
||||
+ // with the destroy. Awaiting the in-flight unload first makes the decision
|
||||
+ // deterministic: once it settles, either the doc was fully unloaded
|
||||
+ // (removed from `documents`, so we do a clean fresh load below) or the
|
||||
+ // unload aborted because work/connections reappeared (the healthy doc is
|
||||
+ // still in `documents`, so we reuse it). Either way the new connection can
|
||||
+ // never hand-shake onto an orphaned, about-to-be-destroyed Document.
|
||||
+ const existingUnloadingDoc = this.unloadingDocuments.get(documentName);
|
||||
+ if (existingUnloadingDoc) {
|
||||
+ // Wait for the in-flight unload to settle so we never hand-shake onto a dying
|
||||
+ // Document. Swallow a rejected unload — fall through to a fresh load, matching
|
||||
+ // pre-patch behavior (the doc is already removed from `documents` by then).
|
||||
+ try { await existingUnloadingDoc; } catch { /* unload rejected — fresh load */ }
|
||||
+ }
|
||||
const existingLoadingDoc = this.loadingDocuments.get(documentName);
|
||||
if (existingLoadingDoc) {
|
||||
return existingLoadingDoc;
|
||||
diff --git a/dist/hocuspocus-server.esm.js b/dist/hocuspocus-server.esm.js
|
||||
index 1f4dd80244e899128e2c4e5dad8eab7cfc1cbad6..8c2411747bba27fb9486e1df81678a14e41e884e 100644
|
||||
--- a/dist/hocuspocus-server.esm.js
|
||||
+++ b/dist/hocuspocus-server.esm.js
|
||||
@@ -2406,6 +2406,26 @@ class Hocuspocus {
|
||||
* Create a new document by the given request
|
||||
*/
|
||||
async createDocument(documentName, request, socketId, connection, context) {
|
||||
+ // PATCH(gitmost #401): close the connect-vs-unload race. When the last
|
||||
+ // client disconnects, storeDocumentHooks' finally schedules an unload;
|
||||
+ // unloadDocument runs its beforeUnloadDocument hooks asynchronously and
|
||||
+ // records an in-flight promise in `unloadingDocuments`. A NEW connection
|
||||
+ // arriving in that window would otherwise fall straight through to the
|
||||
+ // `documents`/`loadingDocuments` checks and could either reuse a doc that
|
||||
+ // is about to be destroyed, or start loading a fresh doc concurrently
|
||||
+ // with the destroy. Awaiting the in-flight unload first makes the decision
|
||||
+ // deterministic: once it settles, either the doc was fully unloaded
|
||||
+ // (removed from `documents`, so we do a clean fresh load below) or the
|
||||
+ // unload aborted because work/connections reappeared (the healthy doc is
|
||||
+ // still in `documents`, so we reuse it). Either way the new connection can
|
||||
+ // never hand-shake onto an orphaned, about-to-be-destroyed Document.
|
||||
+ const existingUnloadingDoc = this.unloadingDocuments.get(documentName);
|
||||
+ if (existingUnloadingDoc) {
|
||||
+ // Wait for the in-flight unload to settle so we never hand-shake onto a dying
|
||||
+ // Document. Swallow a rejected unload — fall through to a fresh load, matching
|
||||
+ // pre-patch behavior (the doc is already removed from `documents` by then).
|
||||
+ try { await existingUnloadingDoc; } catch { /* unload rejected — fresh load */ }
|
||||
+ }
|
||||
const existingLoadingDoc = this.loadingDocuments.get(documentName);
|
||||
if (existingLoadingDoc) {
|
||||
return existingLoadingDoc;
|
||||
Generated
+2
-5
@@ -44,9 +44,6 @@ overrides:
|
||||
ip-address: 10.1.1
|
||||
|
||||
patchedDependencies:
|
||||
'@hocuspocus/server@3.4.4':
|
||||
hash: d8dc66e5ec3b9d23a876b979f493b6aa901fd2d965be54729495da5136296a42
|
||||
path: patches/@hocuspocus__server@3.4.4.patch
|
||||
ai@6.0.134:
|
||||
hash: f60bfc3357e01e1f3978c6c40fdd65aeb33fefaad7179cde8676465b6c5ff4d9
|
||||
path: patches/ai@6.0.134.patch
|
||||
@@ -78,7 +75,7 @@ importers:
|
||||
version: 3.4.4(y-protocols@1.0.6(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810))
|
||||
'@hocuspocus/server':
|
||||
specifier: 3.4.4
|
||||
version: 3.4.4(patch_hash=d8dc66e5ec3b9d23a876b979f493b6aa901fd2d965be54729495da5136296a42)(y-protocols@1.0.6(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810))
|
||||
version: 3.4.4(y-protocols@1.0.6(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810))
|
||||
'@hocuspocus/transformer':
|
||||
specifier: 3.4.4
|
||||
version: 3.4.4(@tiptap/core@3.20.4(@tiptap/pm@3.20.4))(@tiptap/pm@3.20.4)(y-prosemirror@1.3.7(prosemirror-model@1.25.1)(prosemirror-state@1.4.3)(prosemirror-view@1.40.0)(y-protocols@1.0.6(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810))
|
||||
@@ -13058,7 +13055,7 @@ snapshots:
|
||||
- bufferutil
|
||||
- utf-8-validate
|
||||
|
||||
'@hocuspocus/server@3.4.4(patch_hash=d8dc66e5ec3b9d23a876b979f493b6aa901fd2d965be54729495da5136296a42)(y-protocols@1.0.6(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810))':
|
||||
'@hocuspocus/server@3.4.4(y-protocols@1.0.6(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810))':
|
||||
dependencies:
|
||||
'@hocuspocus/common': 3.4.4
|
||||
async-lock: 1.4.1
|
||||
|
||||
Reference in New Issue
Block a user