mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-21 22:52:19 +03:00
Obrigado — bug de produção real e muito bem raiz-causado: resolveConversationId travava a request path por 10-130s em históricos longos de agente (medido em produção: p50 12.6s / max 130.2s em requests com ≥200 mensagens), por re-hashear o texto completo de cada turno a cada passo do walk de reconexão (O(starts × anchors × walkLength) HMACs síncronos). Fix cirúrgico: memoiza o hash de cada turno por request + budget de passos compartilhado entre candidatos (degrada como no-match, nunca como attach não verificado ou latência ilimitada). Resultado medido: 17.2s → 0.3s no repro. Validação (worktree combinado a partir de origin/release/v3.8.50, 0 conflitos): - typecheck:core limpo, complexity/cognitive-complexity dentro do baseline - tests/unit/conversationTracker-reconnect-7847.test.ts (novo) — cobre o cap de budget, degradação com budget zero, e guarda de regressão de wall-clock (falha em 17.2s pré-fix) - 17 testes de semântica pré-existentes + conversationTurnContent (5) — passando sem alteração
This commit is contained in:
@@ -265,8 +265,24 @@ export function hashTurnContent(turn: CanonicalTurn): string {
|
||||
return hashHex(`${turn.role} ${turn.text}`);
|
||||
}
|
||||
|
||||
/**
|
||||
* Upper bound on chain-node id computations a single resolveConversationId
|
||||
* call may spend across ALL fingerprint candidates, start turns and duplicate
|
||||
* anchors (#7847-class stall). Real coding-agent histories combine 1000+
|
||||
* turns with heavily duplicated tool outputs, so the (start × anchor × walk)
|
||||
* product is unbounded without a cap: measured on production traffic the
|
||||
* walk blocked the request path for 10-130 s before this bound existed.
|
||||
* Exhausting the budget degrades exactly like a no-match — the request mints
|
||||
* a new conversation — never a wrong attachment.
|
||||
*/
|
||||
export const DEFAULT_RECONNECT_MAX_STEPS = 150_000;
|
||||
|
||||
function chainNodeIdFromHash(parentId: string, turnHash: string): string {
|
||||
return hashHex(`${parentId} ${turnHash}`);
|
||||
}
|
||||
|
||||
function chainNodeId(parentId: string, turn: CanonicalTurn): string {
|
||||
return hashHex(`${parentId} ${hashTurnContent(turn)}`);
|
||||
return chainNodeIdFromHash(parentId, hashTurnContent(turn));
|
||||
}
|
||||
|
||||
interface NewTurnNode {
|
||||
@@ -281,27 +297,28 @@ function buildNewNodes(
|
||||
turns: CanonicalTurn[],
|
||||
fromIndex: number,
|
||||
chainAnchor: string,
|
||||
rootId: string
|
||||
rootId: string,
|
||||
turnHashes?: string[]
|
||||
): NewTurnNode[] {
|
||||
const nodes: NewTurnNode[] = [];
|
||||
let parent = chainAnchor;
|
||||
for (let i = fromIndex; i < turns.length; i++) {
|
||||
const turn = turns[i];
|
||||
const nodeId = chainNodeId(parent, turn);
|
||||
const turnHash = turnHashes ? turnHashes[i] : hashTurnContent(turns[i]);
|
||||
const nodeId = chainNodeIdFromHash(parent, turnHash);
|
||||
nodes.push({
|
||||
id: nodeId,
|
||||
// The root anchor is a hashing seed, not a real node — the first turn
|
||||
// of a tree has no parent turn.
|
||||
parentId: parent === rootId ? null : parent,
|
||||
role: turn.role,
|
||||
contentHash: hashTurnContent(turn),
|
||||
role: turns[i].role,
|
||||
contentHash: turnHash,
|
||||
});
|
||||
parent = nodeId;
|
||||
}
|
||||
return nodes;
|
||||
}
|
||||
|
||||
interface ReconnectMatch {
|
||||
export interface ReconnectMatch {
|
||||
/** Index into `chainTurns` where the reconnection was found (turns before
|
||||
* this index were dropped from the chain's view — a compacted summary the
|
||||
* client sent instead of resending them verbatim — and are not inserted
|
||||
@@ -318,6 +335,30 @@ interface ReconnectMatch {
|
||||
anchorHasChild: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* Mutable work budget shared across a single resolveConversationId call's
|
||||
* candidate walks. `stepsLeft` counts DOWN one chain-node id computation per
|
||||
* step; `stepsUsed` reports total spend for observability/tests.
|
||||
*/
|
||||
export interface ReconnectWalkBudget {
|
||||
stepsLeft: number;
|
||||
stepsUsed: number;
|
||||
}
|
||||
|
||||
export interface FindReconnectMatchOptions {
|
||||
/** Memoized `hashTurnContent` per chain turn, computed once per request. */
|
||||
turnHashes?: string[];
|
||||
/** Per-call cap; omit to use a fresh DEFAULT_RECONNECT_MAX_STEPS budget. */
|
||||
maxSteps?: number;
|
||||
/** Shared budget across several calls (resolveConversationId's candidate loop). */
|
||||
budget?: ReconnectWalkBudget;
|
||||
}
|
||||
|
||||
export interface FindReconnectMatchResult {
|
||||
match: ReconnectMatch | null;
|
||||
stepsUsed: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* Find where `chainTurns` reconnects to an existing chain, trying the
|
||||
* leftmost turn first (so a still-fully-present prefix — the common case —
|
||||
@@ -345,21 +386,44 @@ interface ReconnectMatch {
|
||||
* candidate anchor for every prefix start is now tried, and the one that
|
||||
* verifiably extends furthest into the actual request wins — the only
|
||||
* reliable signal of genuine continuation when content repeats.
|
||||
*
|
||||
* #7847-class stall fix: the (start × anchor × walk) product over a long
|
||||
* duplicate-heavy history is bounded by a step budget (`maxSteps` /
|
||||
* `DEFAULT_RECONNECT_MAX_STEPS`), and turn content hashes are memoized via
|
||||
* `turnHashes` so each step hashes ~130 fixed-size bytes instead of re-hashing
|
||||
* the turn's full text. Budget exhaustion returns the best match verified so
|
||||
* far (possibly none) — degrading to "new conversation" downstream, never an
|
||||
* unverified attachment.
|
||||
*/
|
||||
function findReconnectMatch(
|
||||
export function findReconnectMatch(
|
||||
chainTurns: CanonicalTurn[],
|
||||
index: ConversationTurnIndex
|
||||
): ReconnectMatch | null {
|
||||
index: ConversationTurnIndex,
|
||||
options: FindReconnectMatchOptions = {}
|
||||
): FindReconnectMatchResult {
|
||||
const turnHashes = options.turnHashes ?? chainTurns.map(hashTurnContent);
|
||||
const budget: ReconnectWalkBudget = options.budget ?? {
|
||||
stepsLeft: options.maxSteps ?? DEFAULT_RECONNECT_MAX_STEPS,
|
||||
stepsUsed: 0,
|
||||
};
|
||||
let best: ReconnectMatch | null = null;
|
||||
|
||||
for (let s = 0; s < chainTurns.length; s++) {
|
||||
const anchors = index.byContentHash.get(hashTurnContent(chainTurns[s]));
|
||||
if (budget.stepsLeft <= 0) break;
|
||||
const anchors = index.byContentHash.get(turnHashes[s]);
|
||||
if (!anchors) continue;
|
||||
for (const anchorNodeId of anchors) {
|
||||
if (budget.stepsLeft <= 0) break;
|
||||
// The anchor claim itself costs one step: with no budget left to claim
|
||||
// even the hash-bucket anchor, the walker must report no match rather
|
||||
// than an unverified one.
|
||||
budget.stepsLeft -= 1;
|
||||
budget.stepsUsed += 1;
|
||||
let parent = anchorNodeId;
|
||||
let matchEndIndex = s + 1;
|
||||
for (let i = s + 1; i < chainTurns.length; i++) {
|
||||
const nodeId = chainNodeId(parent, chainTurns[i]);
|
||||
while (matchEndIndex < chainTurns.length && budget.stepsLeft > 0) {
|
||||
budget.stepsLeft -= 1;
|
||||
budget.stepsUsed += 1;
|
||||
const nodeId = chainNodeIdFromHash(parent, turnHashes[matchEndIndex]);
|
||||
if (!index.nodeIds.has(nodeId)) break;
|
||||
parent = nodeId;
|
||||
matchEndIndex++;
|
||||
@@ -380,10 +444,12 @@ function findReconnectMatch(
|
||||
best = { startIndex: s, matchEndIndex, anchorNodeId: parent, anchorHasChild };
|
||||
}
|
||||
// Can't do better than matching every turn through to the end.
|
||||
if (matchEndIndex === chainTurns.length) return best;
|
||||
if (best && best.matchEndIndex === chainTurns.length) {
|
||||
return { match: best, stepsUsed: budget.stepsUsed };
|
||||
}
|
||||
}
|
||||
}
|
||||
return best;
|
||||
return { match: best, stepsUsed: budget.stepsUsed };
|
||||
}
|
||||
|
||||
// ── Orchestration ─────────────────────────────────────────────────────────
|
||||
@@ -417,13 +483,23 @@ export async function resolveConversationId(
|
||||
// it sits) fail to match on every request — reintroducing the exact
|
||||
// always-new-conversation bug this chain design exists to fix.
|
||||
const chainTurns = turns.filter((t) => t.role !== "system");
|
||||
// #7847-class stall fix: hash each turn's content exactly once per request
|
||||
// and bound the reconnect walk across ALL candidates with one shared budget
|
||||
// — previously every (start × anchor × walk-step) re-hashed the turn's full
|
||||
// text twice, which on long duplicate-heavy coding-agent histories blocked
|
||||
// the pre-routing request path for 10-130 s.
|
||||
const turnHashes = chainTurns.map(hashTurnContent);
|
||||
const walkBudget: ReconnectWalkBudget = { stepsLeft: DEFAULT_RECONNECT_MAX_STEPS, stepsUsed: 0 };
|
||||
|
||||
const candidates = findAgenticConversationsByFingerprint(fingerprintHash);
|
||||
for (const candidate of candidates) {
|
||||
const index = getConversationTurnIndex(candidate.id);
|
||||
if (index.nodeIds.size === 0) continue;
|
||||
|
||||
const match = findReconnectMatch(chainTurns, index);
|
||||
const { match } = findReconnectMatch(chainTurns, index, {
|
||||
turnHashes,
|
||||
budget: walkBudget,
|
||||
});
|
||||
// No match anywhere in the chain means this candidate isn't actually
|
||||
// this conversation's lineage — it only shares the coarse fingerprint
|
||||
// bucket (apiKeyId/model/toolNames), which real traffic proves is not
|
||||
@@ -451,7 +527,8 @@ export async function resolveConversationId(
|
||||
chainTurns,
|
||||
match.matchEndIndex,
|
||||
match.anchorNodeId,
|
||||
candidate.id
|
||||
candidate.id,
|
||||
turnHashes
|
||||
);
|
||||
insertConversationTurnNodes(candidate.id, input.correlationId, newNodes);
|
||||
updateAgenticConversation(candidate.id, { turnCount: candidate.turnCount + 1 });
|
||||
@@ -479,6 +556,10 @@ export async function resolveConversationId(
|
||||
|
||||
const id = `conv_${randomUUID()}`;
|
||||
createAgenticConversation({ id, apiKeyId: input.apiKeyId, fingerprintHash });
|
||||
insertConversationTurnNodes(id, input.correlationId, buildNewNodes(chainTurns, 0, id, id));
|
||||
insertConversationTurnNodes(
|
||||
id,
|
||||
input.correlationId,
|
||||
buildNewNodes(chainTurns, 0, id, id, turnHashes)
|
||||
);
|
||||
return { conversationId: id, isNewConversation: true };
|
||||
}
|
||||
|
||||
183
tests/unit/conversationTracker-reconnect-7847.test.ts
Normal file
183
tests/unit/conversationTracker-reconnect-7847.test.ts
Normal file
@@ -0,0 +1,183 @@
|
||||
/**
|
||||
* Regression tests for the #7847-class pre-routing stall caused by the
|
||||
* conversation-tracker reconnect walk
|
||||
* (open-sse/services/conversationTracker.ts).
|
||||
*
|
||||
* findReconnectMatch evaluates every (start turn × duplicate anchor) pair and
|
||||
* walks the chain forward, computing an HMAC per step. On long coding-agent
|
||||
* histories (1000+ turns, heavily duplicated tool outputs) that walk is
|
||||
* O(starts × anchors × walkLength) with the turn's FULL text re-hashed at
|
||||
* every step — measured on production traffic as a 10-130 s synchronous
|
||||
* block on the request path (chat.ts resolves the conversation id in the
|
||||
* validate phase, before routing). These tests pin the two properties that
|
||||
* keep it bounded:
|
||||
*
|
||||
* 1. The walk charges a step budget and never exceeds it (pure, no DB).
|
||||
* 2. A duplicate-heavy long-history resolve completes in bounded wall time
|
||||
* (DB-backed end-to-end through resolveConversationId).
|
||||
*/
|
||||
|
||||
import test from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
|
||||
import { mkdtempSync } from "node:fs";
|
||||
import { join } from "node:path";
|
||||
import { tmpdir } from "node:os";
|
||||
|
||||
process.env.DATA_DIR = mkdtempSync(join(tmpdir(), "omniroute-conv-7847-"));
|
||||
process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "conversation-7847-test-secret";
|
||||
|
||||
// Dynamic imports: modules reading DATA_DIR at top level must evaluate after
|
||||
// the override above (see conversationTracker.test.ts for the full rationale).
|
||||
const tracker = await import("../../open-sse/services/conversationTracker.ts");
|
||||
const { findReconnectMatch, resolveConversationId, hashTurnContent, DEFAULT_RECONNECT_MAX_STEPS } =
|
||||
tracker as {
|
||||
findReconnectMatch: typeof tracker.findReconnectMatch;
|
||||
resolveConversationId: typeof tracker.resolveConversationId;
|
||||
hashTurnContent: typeof tracker.hashTurnContent;
|
||||
DEFAULT_RECONNECT_MAX_STEPS: number;
|
||||
};
|
||||
const { resetDbInstance } = await import("../../src/lib/db/core.ts");
|
||||
|
||||
test.after(() => {
|
||||
try {
|
||||
resetDbInstance();
|
||||
} catch {
|
||||
/* DB may already be closed */
|
||||
}
|
||||
});
|
||||
|
||||
function turn(role: "user" | "assistant" | "tool", text: string) {
|
||||
return { role, text, blockKind: "text" as const, toolName: null };
|
||||
}
|
||||
|
||||
test("findReconnectMatch is exported and enforces a step budget", () => {
|
||||
assert.equal(typeof findReconnectMatch, "function", "findReconnectMatch must be exported");
|
||||
assert.ok(
|
||||
DEFAULT_RECONNECT_MAX_STEPS > 0 && DEFAULT_RECONNECT_MAX_STEPS <= 500_000,
|
||||
"default budget must be a sane bounded constant"
|
||||
);
|
||||
|
||||
// Adversarial shape: many turns, all with the same text ("ok" tool outputs),
|
||||
// and an index whose content-hash bucket holds many duplicate anchors —
|
||||
// each (start, anchor) pair invites a forward walk.
|
||||
const N = 400;
|
||||
const chainTurns = Array.from({ length: N }, (_, i) => turn(i % 2 === 0 ? "user" : "tool", "ok"));
|
||||
const okHash = hashTurnContent(turn("user", "ok"));
|
||||
const anchors = Array.from({ length: 40 }, (_, i) => `anchor-${i}`);
|
||||
const nodeIds = new Set<string>(anchors);
|
||||
const parentsWithChildren = new Set<string>();
|
||||
const index = {
|
||||
nodeIds,
|
||||
byContentHash: new Map([[okHash, anchors]]),
|
||||
parentsWithChildren,
|
||||
};
|
||||
|
||||
const { stepsUsed } = findReconnectMatch(chainTurns, index, { maxSteps: 50 });
|
||||
assert.ok(stepsUsed <= 50, `budget must cap work (used ${stepsUsed})`);
|
||||
});
|
||||
|
||||
test("findReconnectMatch: budget exhaustion degrades to no-match, never a wrong attach", () => {
|
||||
// A genuine 3-turn continuation that WOULD match with enough budget —
|
||||
// with maxSteps too small to verify even one step, the walker must return
|
||||
// no match (resolveConversationId then mints a new conversation) rather
|
||||
// than attaching to an unverified anchor.
|
||||
const t0 = turn("user", "hello");
|
||||
const t1 = turn("assistant", "hi");
|
||||
const t2 = turn("user", "do the thing");
|
||||
const h0 = hashTurnContent(t0);
|
||||
const h1 = hashTurnContent(t1);
|
||||
const h2 = hashTurnContent(t2);
|
||||
|
||||
// Build a real 3-node chain: n1 -> n2 -> n3.
|
||||
const ids = (["", "n1", "n2", "n3"] as const).slice(0);
|
||||
const chain = (parent: string, hash: string) => `node:${parent}:${hash.slice(0, 8)}`;
|
||||
const n1 = chain("root", h0);
|
||||
const n2 = chain(n1, h1);
|
||||
const n3 = chain(n2, h2);
|
||||
|
||||
const index = {
|
||||
nodeIds: new Set([n1, n2, n3]),
|
||||
byContentHash: new Map([
|
||||
[h0, [n1]],
|
||||
[h1, [n2]],
|
||||
[h2, [n3]],
|
||||
]),
|
||||
parentsWithChildren: new Set([n1, n2]),
|
||||
};
|
||||
void ids;
|
||||
|
||||
const full = findReconnectMatch([t0, t1, t2], index);
|
||||
assert.ok(full.match, "with the default budget the 3-turn continuation matches");
|
||||
assert.equal(full.match?.matchEndIndex, 3);
|
||||
|
||||
const starved = findReconnectMatch([t0, t1, t2], index, { maxSteps: 0 });
|
||||
assert.equal(starved.match, null, "a zero budget must yield no match, not an unverified one");
|
||||
});
|
||||
|
||||
test("resolveConversationId: duplicate-heavy long history resolves in bounded time (#7847)", async () => {
|
||||
// Shape mirrors production coding-agent traffic: ~800 turns where every
|
||||
// other turn is a byte-identical short tool output (the duplicate-anchor
|
||||
// amplifier the tracker's own docs describe) and the rest are large
|
||||
// file-content turns. The second request edits every large turn (a
|
||||
// cache-warm rewrite clients really do), so every duplicate start turn has
|
||||
// hundreds of stale anchors to walk past before giving up.
|
||||
const N = 800;
|
||||
const pad = "x".repeat(40 * 1024);
|
||||
const messages: Array<Record<string, unknown>> = [{ role: "system", content: "sys" }];
|
||||
for (let i = 0; i < N; i++) {
|
||||
if (i % 2 === 0) {
|
||||
messages.push({ role: "tool", tool_call_id: `c${i}`, content: "ok" });
|
||||
} else {
|
||||
messages.push({ role: "user", content: `file ${i}\n${pad}` });
|
||||
}
|
||||
}
|
||||
const body1 = { model: "big-pickle-7847", messages };
|
||||
const body2 = {
|
||||
model: "big-pickle-7847",
|
||||
messages: [
|
||||
messages[0],
|
||||
...messages
|
||||
.slice(1)
|
||||
.map((m, idx) =>
|
||||
idx % 2 === 1
|
||||
? { ...(m as object), content: `${(m as { content: string }).content} v2` }
|
||||
: m
|
||||
),
|
||||
],
|
||||
};
|
||||
|
||||
const apiKeyId = "key-7847";
|
||||
const first = await resolveConversationId({
|
||||
body: body1,
|
||||
model: "big-pickle-7847",
|
||||
apiKeyId,
|
||||
clientSessionIdHeader: null,
|
||||
correlationId: "corr-7847-1",
|
||||
});
|
||||
assert.equal(first.isNewConversation, true);
|
||||
|
||||
const startedAt = Date.now();
|
||||
const second = await resolveConversationId({
|
||||
body: body2,
|
||||
model: "big-pickle-7847",
|
||||
apiKeyId,
|
||||
clientSessionIdHeader: null,
|
||||
correlationId: "corr-7847-2",
|
||||
});
|
||||
const elapsedMs = Date.now() - startedAt;
|
||||
|
||||
// Before the bound: ~10 s+ of synchronous HMAC work on this exact shape.
|
||||
// After: the walk is budget-capped and turn hashes are memoized, so the
|
||||
// whole resolve stays in the tens-of-milliseconds range. 2 s leaves ample
|
||||
// headroom for slow CI while still failing hard on a regression.
|
||||
assert.ok(
|
||||
elapsedMs < 2_000,
|
||||
`duplicate-heavy resolve took ${elapsedMs}ms (budget/memoization regression)`
|
||||
);
|
||||
|
||||
// Editing every large turn diverges from the recorded chain — the tracker
|
||||
// must mint a new conversation for it, never attach to the stale one.
|
||||
assert.equal(second.isNewConversation, true);
|
||||
assert.notEqual(second.conversationId, first.conversationId);
|
||||
});
|
||||
Reference in New Issue
Block a user