From b536579b3211657d21569552a5b7f95f8ce4afc0 Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Thu, 5 Nov 2026 18:37:19 -0500 Subject: [PATCH] fix(graph): impact reads a node's edges in one unlimited query again (#3248) #3125 gave impact traversal a node and edge budget, and fetched each visited node's incoming edges as `kind IN () ... LIMIT ?`. Both halves slowed the read the walk makes once per visited node: the kind list searches the target index once per kind, and the LIMIT under the read's ORDER BY made it slower still. getImpactRadius took 2-3x as long for the same subgraph. The walk now reads `WHERE target = ? AND kind != 'contains'` with no LIMIT, through a cached statement, and the slice applies the budget as before. A container's members are read without a LIMIT too, for the same reason. A contains edge still never takes a budget slot or marks the result truncated, and the output is unchanged: the same nodes, edges, order and truncation as main on 20,000 random graphs and 500 traversals of codegraph's own index. Fixes #3248 --- __tests__/graph.test.ts | 73 +++++++++++++++++++++++++++++++++++++++++ src/db/queries.ts | 19 +++++++++++ src/graph/traversal.ts | 12 +++---- 3 files changed, 98 insertions(+), 6 deletions(-) diff --git a/__tests__/graph.test.ts b/__tests__/graph.test.ts index 7edd0570f..a2be09b3a 100644 --- a/__tests__/graph.test.ts +++ b/__tests__/graph.test.ts @@ -361,6 +361,20 @@ export { main }; // sibling method and explode impact for a leaf symbol. (#536) expect(impact.nodes.has(derived!.id)).toBe(false); }); + + it('reads incoming edges without contains, in the same order as getIncomingEdges (#3248)', () => { + const queries = (cg as unknown as { queries: import('../src/db/queries').QueryBuilder }).queries; + const all = queries.getAllNodes(); + let withContains = 0; + for (const node of all) { + const incoming = queries.getIncomingEdges(node.id); + const expected = incoming.filter((e) => e.kind !== 'contains'); + if (expected.length !== incoming.length) withContains++; + expect(queries.getIncomingEdgesExceptContains(node.id)).toEqual(expected); + } + // The fixture exercises the filter: members have an incoming contains edge. + expect(withContains).toBeGreaterThan(0); + }); }); describe('findPath()', () => { @@ -549,6 +563,8 @@ function tGraph(nodes: Node[], edges: Edge[]): GraphTraverser { edges.filter((e) => e.source === source && (!kinds || kinds.includes(e.kind))), getIncomingEdges: (target: string, kinds?: string[]) => edges.filter((e) => e.target === target && (!kinds || kinds.includes(e.kind))), + getIncomingEdgesExceptContains: (target: string) => + edges.filter((e) => e.target === target && e.kind !== 'contains'), }; return new GraphTraverser(q as never); } @@ -741,6 +757,63 @@ describe('Traversal edge-completeness & limits (#1086–#1090)', () => { expect(sub.edges.every((edge) => sub.nodes.has(edge.source) && sub.nodes.has(edge.target))).toBe(true); }); + it('getImpactRadius reads a node\'s edges without a SQL LIMIT, and budgets after dropping contains (#3248)', () => { + // K is a class that contains A. B calls A, C calls B. + const nodes = [tNode('A'), tNode('B'), tNode('C'), tNode('K', 'class')]; + const edges: Edge[] = [ + { source: 'K', target: 'A', kind: 'contains', line: 1 }, + { source: 'B', target: 'A', kind: 'calls', line: 2 }, + { source: 'C', target: 'B', kind: 'calls', line: 3 }, + ]; + const byId = new Map(nodes.map((n) => [n.id, n])); + const limits: unknown[] = []; + const q = { + getNodeById: (id: string) => byId.get(id) ?? null, + getNodesByIds: (ids: readonly string[]) => + new Map(ids.flatMap((id) => (byId.has(id) ? [[id, byId.get(id)!] as const] : []))), + // Every read behaves like its SQL: a kind list filters, a limit caps. + getOutgoingEdges: (source: string, kinds?: string[], provenance?: string, limit?: number) => { + limits.push(limit); + const rows = edges.filter((e) => e.source === source && (!kinds || kinds.includes(e.kind))); + return limit === undefined ? rows : rows.slice(0, limit); + }, + getIncomingEdges: (target: string, kinds?: string[], limit?: number) => { + limits.push(limit); + const rows = edges.filter((e) => e.target === target && (!kinds || kinds.includes(e.kind))); + return limit === undefined ? rows : rows.slice(0, limit); + }, + getIncomingEdgesExceptContains: (...args: unknown[]) => { + // Any argument past the target would be a new bound on the read. + limits.push(args.length > 1 ? args[1] : undefined); + return edges.filter((e) => e.target === args[0] && e.kind !== 'contains'); + }, + }; + const traverser = new GraphTraverser(q as never); + + const sub = traverser.getImpactRadius('A', 3); + expect([...sub.nodes.keys()].sort()).toEqual(['A', 'B', 'C']); + expect(sub.edges.some((e) => e.kind === 'contains')).toBe(false); + expect(sub.truncated).toBeUndefined(); + // The walk reads once per visited node, and a LIMIT under the reads' + // ORDER BY made each one markedly slower, so the budget is applied after + // the read instead. K is a container, so impact on it reads its members. + traverser.getImpactRadius('K', 2); + traverser.getImpactRadius('K', 2, { maxEdges: 1 }); + expect(limits.length).toBeGreaterThan(0); + expect(limits.every((limit) => limit === undefined)).toBe(true); + + // The contains edge is dropped before the budget applies, so it neither + // takes A's one edge slot nor marks the result truncated. + const tight = traverser.getImpactRadius('A', 1, { maxEdges: 1 }); + expect(tight.edges.map((e) => `${e.source}>${e.target}`)).toEqual(['B>A']); + expect(tight.truncated).toBeUndefined(); + // A second dependent of A is over that budget, and says so. + edges.push({ source: 'C', target: 'A', kind: 'calls', line: 4 }); + const over = traverser.getImpactRadius('A', 1, { maxEdges: 1 }); + expect(over.edges).toHaveLength(1); + expect(over.truncated).toBe(true); + }); + it('surfaces impact truncation explicitly in MCP output', () => { const formatted = (new ToolHandler(null) as any).formatImpact('A', { nodes: new Map([['A', tNode('A')]]), diff --git a/src/db/queries.ts b/src/db/queries.ts index aff563119..6647075d8 100644 --- a/src/db/queries.ts +++ b/src/db/queries.ts @@ -483,6 +483,7 @@ export class QueryBuilder { deleteEdgesByTarget?: SqliteStatement; getEdgesBySource?: SqliteStatement; getEdgesByTarget?: SqliteStatement; + getDependentEdgesByTarget?: SqliteStatement; getUnresolvedFromNode?: SqliteStatement; getUnresolvedInFile?: SqliteStatement; insertFile?: SqliteStatement; @@ -2768,6 +2769,24 @@ export class QueryBuilder { return rows.map(rowToEdge); } + /** + * Incoming edges to a node except `contains`: the edges that make their + * source depend on the target, in {@link getIncomingEdges} order. + * + * Impact traversal runs this once per visited node, so it stays one index + * search with no LIMIT. A kind list searches the index once per kind, and a + * LIMIT under this ORDER BY made each read markedly slower (#3248). + */ + getIncomingEdgesExceptContains(targetId: string): Edge[] { + if (!this.stmts.getDependentEdgesByTarget) { + this.stmts.getDependentEdgesByTarget = this.db.prepare( + "SELECT * FROM edges WHERE target = ? AND kind != 'contains' ORDER BY kind, source, line, col" + ); + } + const rows = this.stmts.getDependentEdgesByTarget.all(targetId) as EdgeRow[]; + return rows.map(rowToEdge); + } + /** * Get incoming edges to a node. Kind must precede opaque source IDs: * file IDs sort before function IDs, so source-first ordering lets imports diff --git a/src/graph/traversal.ts b/src/graph/traversal.ts index 280ee59c8..43496af30 100644 --- a/src/graph/traversal.ts +++ b/src/graph/traversal.ts @@ -4,7 +4,7 @@ * BFS and DFS traversal for the code knowledge graph. */ -import { Node, Edge, Subgraph, TraversalOptions, EdgeKind, ImpactOptions, EDGE_KINDS } from '../types'; +import { Node, Edge, Subgraph, TraversalOptions, EdgeKind, ImpactOptions } from '../types'; import { QueryBuilder } from '../db/queries'; /** @@ -21,7 +21,6 @@ const DEFAULT_OPTIONS: Required = { export const DEFAULT_IMPACT_MAX_NODES = 10_000; export const DEFAULT_IMPACT_MAX_EDGES = 50_000; -const IMPACT_INCOMING_KINDS = EDGE_KINDS.filter((kind) => kind !== 'contains'); /** * Result of a single traversal step @@ -663,7 +662,8 @@ export class GraphTraverser { const containerKinds = new Set(['class', 'interface', 'struct', 'union', 'trait', 'protocol', 'module', 'enum']); if (containerKinds.has(focalNode.kind)) { const remaining = budget.maxEdges - edges.length; - const fetched = this.queries.getOutgoingEdges(nodeId, ['contains'], undefined, remaining + 1); + // No SQL LIMIT here either: the slice applies the budget (#3248). + const fetched = this.queries.getOutgoingEdges(nodeId, ['contains']); if (fetched.length > remaining) budget.truncated = true; const containsEdges = fetched.slice(0, remaining); if (containsEdges.length > 0) { @@ -692,9 +692,9 @@ export class GraphTraverser { // them, so following it upward would climb to the parent class and then // re-expand every sibling member — exploding impact for a leaf symbol. (#536) const remaining = budget.maxEdges - edges.length; - // Fetch one extra row so hitting the SQL LIMIT is distinguishable from an - // exact-size result and can be surfaced as explicit truncation. - const fetched = this.queries.getIncomingEdges(nodeId, IMPACT_INCOMING_KINDS, remaining + 1); + // The whole list, then the slice applies the budget: a SQL LIMIT made this + // per-node read markedly slower (#3248). + const fetched = this.queries.getIncomingEdgesExceptContains(nodeId); if (fetched.length > remaining) budget.truncated = true; const incomingEdges = fetched.slice(0, remaining); if (incomingEdges.length === 0) return;