From af967c6e47e3b3e215ae3c4434eb5bcb27a0e7cb Mon Sep 17 00:00:00 2001 From: danusha2345 Date: Fri, 30 Oct 2026 17:47:37 -0500 Subject: [PATCH] fix(graph): a path search keeps one predecessor per node, so it stays linear findPath's breadth-first search copied the whole path so far into every queue entry and dequeued with Array.shift(). A long path cost time and heap quadratic in its length, and a queued frontier held one copy per entry. The search now keeps one predecessor per discovered node in a map, walks a queue of ids with a head index, and builds the path once at the end. It keeps the enqueue-once set from PR #2016. Targets are fetched once each even when parallel edges point at the same node. Red, green and A/B: - Red on main: a 400-hop chain walks 81,001 array elements, against under 4,000 now, and a fan-in batch asks for the same node twice. No timing in either test. - Same paths: 400 seeded pairs each on excalidraw, gin and flask gave the same node and edge sequence on both arms (1,200 of 1,200). - A 20,000-hop chain: 1,007 ms and 47 MB of heap on main, 30 ms and 13 MB here. The test added in PR #2016 now counts node lookups per id instead of patching Array.prototype.push, because the queue no longer holds objects with a path. Its expectation is unchanged. Original PR #1583 by @danusha2345. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + __tests__/graph.test.ts | 78 +++++++++++++++++++++++++++++++++++------ src/graph/traversal.ts | 55 +++++++++++++++-------------- 3 files changed, 98 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a731bf9ab..ef2a1d801d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -128,6 +128,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - Running `codegraph query`, `explore`, `node`, `callers`, `callees`, `impact`, `context`, `files` or `affected` while `codegraph index` rebuilds a project no longer makes the rebuild fail with "database is locked", and neither does the context hook Claude Code runs on its own when you ask about your code: like `codegraph status`, they now only read the index and say when a rebuild is running, and the hook stays quiet until the rebuild is done. They still answer from an index an older CodeGraph version built. Thanks @sealad886 for the report. (#2913) - Opening a project whose index needs a schema update no longer fails when several CodeGraph processes open it at the same moment, like agent sessions starting right after an upgrade, or the MCP server and a git hook: one of them updates the index while the others wait for it and then carry on. Before, the ones that lost the race applied the same update again, and failed with "UNIQUE constraint failed" or "database is locked". Thanks @inth3shadows for the report and @johnhldiver-beep for the first fix. (#2980) - A qualified name given to `codegraph callers`, `callees` or `impact`, or to `codegraph_node`, like `Format.Price`, now names only the symbol whose scope is spelled that way, so its answer no longer merges in `LegacyFormat.Price` or any other class whose name only ends with `Format`, nor a class that just sits in a file named `Format.cs`. In C#, `BasketService.AddItemToBasket` no longer also selects the `IBasketService` interface's method; ask for `IBasketService.AddItemToBasket` for that one. A file or folder still names the module in Rust, Python and Go, and a PHP namespace can be written with its own backslashes, like `Billing\Format::price`. No re-index needed. Thanks @Gobakabana for the report and @yang0228. (#2908) +- Finding the path between two symbols no longer slows down sharply, or grows its memory use, when the path is long or the graph is densely connected. Thanks @danusha2345. - `codegraph callers` and the `codegraph_callers` tool now list each caller at the lines where it calls the symbol, every one of them, like `src/report.ts:9, 10` for a function that calls it twice, and a function that calls itself among its own callers, marked as calling itself. Before, a caller showed only the line its own declaration starts on, a second call in the same caller didn't show at all, and a recursive call was left out, though each is a line to edit when the symbol changes. `codegraph_node` lists its callers the same way, `codegraph callees` and `codegraph_callees` include a function that calls itself, and `--json` gives every caller its call-site lines. A caller with many calls shows the first few lines and how many more there are. Thanks @Gobakabana for the report and @johnhldiver-beep for the first fix. (#2909) - `codegraph affected --filter` now matches its glob against whole names: `tests/**/*.py` also finds the tests directly inside `tests/`, including a changed test file itself, `tests/*.py` no longer counts a changed `tests/x.py.bak` as a test, and `e2e/*.spec.ts` no longer matches a folder that only ends in `e2e`, like `xe2e/`. A pattern with a folder in it still matches that folder at any depth, so `e2e/*.spec.ts` keeps finding `apps/web/e2e/login.spec.ts`; start it with `/` to match from the project root only. Thanks @Jseca666. - `codegraph init` and `codegraph index` no longer report a file you deleted as one that "could not be read" while git still lists it, which it does until the deletion is staged: such a file is now skipped, and so is a symlink that points at nothing. A file that is there but can't be read is still reported. Thanks @xianzhi-xzw for the report and the first fix. (#2904) diff --git a/__tests__/graph.test.ts b/__tests__/graph.test.ts index f5e07d39d7..105268952b 100644 --- a/__tests__/graph.test.ts +++ b/__tests__/graph.test.ts @@ -553,6 +553,65 @@ function tGraph(nodes: Node[], edges: Edge[]): GraphTraverser { } describe('Traversal edge-completeness & limits (#1086–#1090)', () => { + it('findPath keeps shortest-path order without duplicate frontier entries', () => { + const nodes = ['A', 'B', 'C', 'D', 'E'].map((id) => tNode(id)); + const edges: Edge[] = [ + { source: 'A', target: 'B', kind: 'calls', line: 1 }, + { source: 'A', target: 'B', kind: 'references', line: 2 }, + { source: 'A', target: 'C', kind: 'calls', line: 3 }, + { source: 'B', target: 'D', kind: 'calls', line: 4 }, + { source: 'C', target: 'D', kind: 'calls', line: 5 }, + { source: 'D', target: 'E', kind: 'calls', line: 6 }, + ]; + const byId = new Map(nodes.map((n) => [n.id, n])); + const batches: string[][] = []; + const q = { + getNodeById: (id: string) => byId.get(id) ?? null, + getNodesByIds: (ids: readonly string[]) => { + batches.push([...ids]); + expect(new Set(ids).size).toBe(ids.length); + return new Map(ids.flatMap((id) => { + const node = byId.get(id); + return node ? [[id, node] as const] : []; + })); + }, + getOutgoingEdges: (source: string) => edges.filter((e) => e.source === source), + }; + + const path = new GraphTraverser(q as never).findPath('A', 'E'); + expect(path?.map((step) => step.node.id)).toEqual(['A', 'B', 'D', 'E']); + expect(path?.map((step) => step.edge?.line ?? null)).toEqual([null, 1, 4, 6]); + expect(batches[0]).toEqual(['B', 'C']); + }); + + it('findPath keeps one predecessor per node instead of copying the path into the queue', () => { + const depth = 400; + const nodes = Array.from({ length: depth + 1 }, (_, i) => tNode(`n${i}`)); + const edges: Edge[] = nodes.slice(1).map((node, i) => ({ source: `n${i}`, target: node.id, kind: 'calls' })); + const byId = new Map(nodes.map((n) => [n.id, n])); + const q = { + getNodeById: (id: string) => byId.get(id) ?? null, + getNodesByIds: (ids: readonly string[]) => new Map(ids.map((id) => [id, byId.get(id)!] as const)), + getOutgoingEdges: (source: string) => edges.filter((e) => e.source === source), + }; + + // Count the elements array iteration walks, so no timing is involved. A + // queue entry that copies its path walks 1 + 2 + ... + depth of them. + const iterate = Array.prototype[Symbol.iterator]; + let walked = 0; + let found: ReturnType; + try { + Array.prototype[Symbol.iterator] = function* (this: unknown[]) { + for (let i = 0; i < this.length; i++) { walked++; yield this[i]; } + } as never; + found = new GraphTraverser(q as never).findPath('n0', `n${depth}`); + } finally { + Array.prototype[Symbol.iterator] = iterate; + } + expect(found).toHaveLength(depth + 1); + expect(walked).toBeLessThan(depth * 10); + }); + it('traverseBFS keeps every parallel edge to the same target (#1090)', () => { // A reaches B via both `calls` and `references` — two distinct edges. const edges: Edge[] = [ @@ -704,22 +763,21 @@ describe('findPath enqueue-once (#1359)', () => { for (const b of layerB) edges.push({ source: b, target: 'end', kind: 'calls' }); seed(ids, edges); - // Count actual queue insertions, without timing thresholds or replacing SQLite. + // Count target lookups: a node is looked up only when it is first + // discovered and enqueued, so a repeat would mean a duplicate queue entry. + // No timing thresholds, and SQLite stays real. const counts = new Map(); - const push = Array.prototype.push; + const queries = cg['queries']; + const getNodesByIds = queries.getNodesByIds; let result: ReturnType; try { - Array.prototype.push = function (...items) { - for (const item of items) { - if (item && typeof item.nodeId === 'string' && Array.isArray(item.path)) { - counts.set(item.nodeId, (counts.get(item.nodeId) ?? 0) + 1); - } - } - return Reflect.apply(push, this, items); + queries.getNodesByIds = function (ids) { + for (const id of ids) counts.set(id, (counts.get(id) ?? 0) + 1); + return getNodesByIds.call(this, ids); }; result = cg.findPath('start', 'end', ['calls']); } finally { - Array.prototype.push = push; + queries.getNodesByIds = getNodesByIds; } expect(result?.map((step) => step.node.id)).toEqual(['start', 'a0', 'b0', 'end']); expect(result?.slice(1).every((step) => step.edge?.kind === 'calls')).toBe(true); diff --git a/src/graph/traversal.ts b/src/graph/traversal.ts index d4016345d3..3630ed3df4 100644 --- a/src/graph/traversal.ts +++ b/src/graph/traversal.ts @@ -713,25 +713,31 @@ export class GraphTraverser { return null; } - // BFS to find shortest path - const visited = new Set(); + // BFS to find the shortest path. Keep one predecessor per discovered node + // instead of copying the full path into every queue entry; use a head index + // so dequeues stay O(1), and mark on enqueue so converging edges cannot + // multiply queued work. const enqueued = new Set([fromId]); - const queue: Array<{ nodeId: string; path: Array<{ node: Node; edge: Edge | null }> }> = [ - { nodeId: fromId, path: [{ node: fromNode, edge: null }] }, - ]; + const parents = new Map(); + const queue: string[] = [fromId]; + let head = 0; - while (queue.length > 0) { - const { nodeId, path } = queue.shift()!; + while (head < queue.length) { + const nodeId = queue[head++]!; if (nodeId === toId) { + const path: Array<{ node: Node; edge: Edge | null }> = []; + let currentId = toId; + while (currentId !== fromId) { + const step = parents.get(currentId)!; + path.push({ node: step.node, edge: step.edge }); + currentId = step.parentId; + } + path.push({ node: fromNode, edge: null }); + path.reverse(); return path; } - if (visited.has(nodeId)) { - continue; - } - visited.add(nodeId); - // Get outgoing edges const outgoingEdges = this.queries.getOutgoingEdges( nodeId, @@ -739,23 +745,20 @@ export class GraphTraverser { ); if (outgoingEdges.length === 0) continue; - // Batch-fetch only targets not yet visited or queued. - const wantIds = outgoingEdges - .map((e) => e.target) - .filter((id) => !visited.has(id) && !enqueued.has(id)); + // Batch-fetch only undiscovered targets, once each even when parallel + // edges point at the same node. + const wantIds = [...new Set( + outgoingEdges.map((e) => e.target).filter((id) => !enqueued.has(id)) + )]; const nextNodes = wantIds.length > 0 ? this.queries.getNodesByIds(wantIds) : new Map(); for (const edge of outgoingEdges) { - if (!visited.has(edge.target) && !enqueued.has(edge.target)) { - const nextNode = nextNodes.get(edge.target); - if (nextNode) { - enqueued.add(edge.target); - queue.push({ - nodeId: edge.target, - path: [...path, { node: nextNode, edge }], - }); - } - } + if (enqueued.has(edge.target)) continue; + const nextNode = nextNodes.get(edge.target); + if (!nextNode) continue; + enqueued.add(edge.target); + parents.set(edge.target, { parentId: nodeId, node: nextNode, edge }); + queue.push(edge.target); } }