From 12d0acbe06995ddc5662673d1540ce6c6bc765e7 Mon Sep 17 00:00:00 2001 From: adevwithpurpose Date: Thu, 20 Aug 2026 23:34:32 +0500 Subject: [PATCH] fix(conversations): bound reconnect walk + memoize turn hashes (#7847) (#10800) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- open-sse/services/conversationTracker.ts | 117 +++++++++-- ...conversationTracker-reconnect-7847.test.ts | 183 ++++++++++++++++++ 2 files changed, 282 insertions(+), 18 deletions(-) create mode 100644 tests/unit/conversationTracker-reconnect-7847.test.ts diff --git a/open-sse/services/conversationTracker.ts b/open-sse/services/conversationTracker.ts index fadc726fc4..e24489a000 100644 --- a/open-sse/services/conversationTracker.ts +++ b/open-sse/services/conversationTracker.ts @@ -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 }; } diff --git a/tests/unit/conversationTracker-reconnect-7847.test.ts b/tests/unit/conversationTracker-reconnect-7847.test.ts new file mode 100644 index 0000000000..00686f37ac --- /dev/null +++ b/tests/unit/conversationTracker-reconnect-7847.test.ts @@ -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(anchors); + const parentsWithChildren = new Set(); + 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> = [{ 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); +});