fix(graph): pack symbol-graph upserts by bytes and surface persist failures - #92
Conversation
…ilures buildCodeGraph reported success while the symbol graph silently held nothing on repos containing large source files, leaving codebase_impact answering "0 callers" for every symbol. saveFilePayloads batched a fixed 50 points per Qdrant request with no regard for their size. A per-file payload scales with the source file (a ~900 KB Java file yields a ~6 MB point), so a handful of large files in one batch pushed the request body past Qdrant's service.max_request_size_mb ceiling (32 MiB by default) and the server rejected the whole batch with HTTP 400. Reproduced against Qdrant v1.17.0: six ~6 MB payloads in one request return "Payload error: JSON payload (36001578 bytes) is larger than allowed (limit: 33554432 bytes)". Pack each request up to a byte budget as well as the existing point cap, so ordinary repos batch exactly as before and large files simply take more requests. Point ids and payload shapes are untouched: the stored data is byte-identical, verified by writing the same workload on both sides and diffing every persisted point. A point that cannot fit even alone now throws SymbolGraphPointTooLargeError naming the file or shard and the server knob that raises the limit, instead of being dropped or producing an anonymous 400. Name shards, the one structure that grows unbounded with repo size, are size-checked for the same reason. The failure was also invisible twice over: the Qdrant client puts the server's real explanation in err.data.status.error while its message is the bare HTTP status text, and the catch logged only that message, did not rethrow, and let the build record an unqualified success. describeQdrantError now recovers the real reason (through a cause chain), the build records it on GraphBuildCompleted.symbolGraphError, and codebase_graph_status reports that symbol-level tools are incomplete until a rebuild succeeds. Refs #89
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSymbol-graph persistence now batches Qdrant requests by byte budget and point count, rejects oversized points explicitly, and records enriched persistence failures in graph-build status and ChangesSymbol Graph Persistence Reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant doRebuildGraph
participant SymbolGraphStore
participant Qdrant
participant codebase_graph_status
doRebuildGraph->>SymbolGraphStore: persist symbol graph
SymbolGraphStore->>Qdrant: send byte-bounded upserts
Qdrant-->>SymbolGraphStore: return success or detailed error
SymbolGraphStore-->>doRebuildGraph: return persistence result
doRebuildGraph->>codebase_graph_status: record symbolGraphError when needed
codebase_graph_status-->>doRebuildGraph: report graph status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/symbol-graph-upsert-budget.test.ts (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the always-
false-branch conditional type.
CapturedUpsert["points"] extends never ? never : { points: CapturedUpsert["points"] }always resolves to{ points: CapturedUpsert["points"] }since the array type is nevernever. Can be simplified without behavior change.♻️ Proposed simplification
-const mockUpsert = vi.fn(async (collName: string, body: CapturedUpsert["points"] extends never ? never : { points: CapturedUpsert["points"] }) => { +const mockUpsert = vi.fn(async (collName: string, body: { points: CapturedUpsert["points"] }) => { upserts.push({ collName, points: body.points }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/symbol-graph-upsert-budget.test.ts` around lines 16 - 18, Update the mockUpsert parameter type in mockUpsert to remove the redundant CapturedUpsert["points"] extends never conditional and use the equivalent object type containing points directly, preserving the existing upserts.push behavior.src/services/symbol-graph-store.ts (1)
160-176: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFlush pending batch before throwing on an oversized point.
When a point exceeds
QDRANT_MAX_REQUEST_BYTES(line 163-165), the function throws immediately without flushing whatever is already queued inbatch. Those already-validated, budget-compliant points are then silently discarded instead of being persisted, even though they were ready to send.♻️ Proposed fix
const bytes = pointBytes(point); if (bytes > QDRANT_MAX_REQUEST_BYTES) { + await flush(); throw new SymbolGraphPointTooLargeError(describe(i), bytes); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/symbol-graph-store.ts` around lines 160 - 176, Update the point-processing loop around pointBytes and SymbolGraphPointTooLargeError to flush any non-empty pending batch before throwing for a point exceeding QDRANT_MAX_REQUEST_BYTES. Preserve the existing error and index/size details, and keep the final flush behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/services/symbol-graph-store.ts`:
- Around line 160-176: Update the point-processing loop around pointBytes and
SymbolGraphPointTooLargeError to flush any non-empty pending batch before
throwing for a point exceeding QDRANT_MAX_REQUEST_BYTES. Preserve the existing
error and index/size details, and keep the final flush behavior unchanged.
In `@tests/unit/symbol-graph-upsert-budget.test.ts`:
- Around line 16-18: Update the mockUpsert parameter type in mockUpsert to
remove the redundant CapturedUpsert["points"] extends never conditional and use
the equivalent object type containing points directly, preserving the existing
upserts.push behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90226b44-c205-4e68-bfa5-2d0a32444591
📒 Files selected for processing (8)
DEVELOPER.mdsrc/constants.tssrc/services/code-graph.tssrc/services/qdrant.tssrc/services/symbol-graph-store.tssrc/tools/graph-tools.tstests/unit/qdrant-error-wrapping.test.tstests/unit/symbol-graph-upsert-budget.test.ts
…on-oversize choice Addresses both CodeRabbit nitpicks on #92. The mock's parameter type carried a conditional that always resolved to the same object type. The upsert loop's decision not to flush a queued batch before throwing is deliberate, so the reason is now stated where the throw happens rather than left to be rediscovered: the throw aborts persistSymbolGraph, no metadata is written, and the next successful build rewrites every payload, so flushing would only add points nothing reads to a collection already known to be incomplete.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@DEVELOPER.md`:
- Line 886: Update the documentation text to identify doRebuildGraph as the
function that saves the file-import graph and records
GraphBuildCompleted.symbolGraphError via describeQdrantError; do not refer to
buildCodeGraph for this flow, since it only performs per-file parsing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5777377-f80c-4117-9642-a1e3b146407c
📒 Files selected for processing (8)
DEVELOPER.mdsrc/constants.tssrc/services/code-graph.tssrc/services/qdrant.tssrc/services/symbol-graph-store.tssrc/tools/graph-tools.tstests/unit/qdrant-error-wrapping.test.tstests/unit/symbol-graph-upsert-budget.test.ts
…ror recorder buildCodeGraph only parses files; the persist call, its catch, and the GraphBuildCompleted record all live in doRebuildGraph. Caught in review.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/code-graph.ts (1)
234-263: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
symbolGraphErroris silently cleared by skip-symbol-graph rebuilds, undermining the "incomplete until successful rebuild" guarantee.
symbolGraphError(line 234) is re-initialized toundefinedon every call and only ever set insideif (!opts.skipSymbolGraph). Per DEVELOPER.md,rebuildGraph(path, { skipSymbolGraph: true })is the fully-wired watcher/Phase F path, triggered on ordinary file changes. When that path runs after a prior symbol-graph persistence failure, line 263 overwriteslastGraphBuildCompletedwithsymbolGraphError: undefined, socodebase_graph_statusreports a clean build again — even though the symbol graph was never actually rebuilt or fixed. This defeats the PR's stated goal of not reporting incomplete builds as successful "until a successful rebuild."Preserve the prior failure across skip-symbol-graph calls instead of resetting it unconditionally:
🐛 Proposed fix
// Build & persist symbol graph (resolution + sharded persistence) — unless // the caller asked to skip it (Phase F watcher path). - let symbolGraphError: string | undefined; + // A skip-symbol-graph rebuild doesn't touch the symbol graph at all, so it + // must not clear a previously-recorded failure — only an actual attempt + // (success or new failure) below should change this. + let symbolGraphError: string | undefined = opts.skipSymbolGraph + ? lastGraphBuildCompleted.get(resolvedPath)?.symbolGraphError + : undefined; if (!opts.skipSymbolGraph) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/code-graph.ts` around lines 234 - 263, Preserve an existing symbol-graph failure when rebuildGraph is called with opts.skipSymbolGraph enabled. Initialize symbolGraphError from the prior lastGraphBuildCompleted entry for resolvedPath, and only clear or replace it after a non-skipped symbol-graph rebuild succeeds or fails; keep the existing error recording behavior in the catch block and ensure skip-only rebuilds do not report a clean status.
🧹 Nitpick comments (2)
src/services/symbol-graph-store.ts (1)
144-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBatching logic verified against test scenarios — correct.
Hand-traced the byte-budget/point-count loop against the "splits by BYTES", "oversized-but-writable", and "keeps historic 50-points" test cases; batch boundaries match expected output in all three.
Minor nit:
UPSERT_MAX_POINTS(line 184) is referenced insideupsertWithinBudget(line 174) before its declaration. Safe today (function bodies only evaluate free variables when called), but consider moving the constant above the function for readability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/symbol-graph-store.ts` around lines 144 - 185, Move the UPSERT_MAX_POINTS constant above the upsertWithinBudget function so its declaration appears before the function that references it, without changing the batching logic or constant value.src/constants.ts (1)
115-115: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCeiling is hardcoded to Qdrant's default, not the deployed server's actual limit.
The comment correctly notes self-hosted deployments may set
service.max_request_size_mbdifferently, butQDRANT_MAX_REQUEST_BYTES(and thereforeQDRANT_UPSERT_BUDGET_BYTES) is a fixed default. If an operator lowers the server limit (or a proxy imposes a stricter one), this fix still produces requests that exceed it and reproduces issue#89at a different threshold. Consider deriving this from an env var (e.g.QDRANT_MAX_REQUEST_SIZE_MB), consistent with the existingQDRANT_URL/QDRANT_HOSTenv-driven config pattern insrc/services/qdrant.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/constants.ts` at line 115, Update QDRANT_MAX_REQUEST_BYTES to derive from an environment-configured request-size limit, using the existing QDRANT_URL/QDRANT_HOST configuration pattern in the qdrant service and preserving the current default when the variable is absent or invalid. Ensure QDRANT_UPSERT_BUDGET_BYTES continues to use the derived byte limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/services/code-graph.ts`:
- Around line 234-263: Preserve an existing symbol-graph failure when
rebuildGraph is called with opts.skipSymbolGraph enabled. Initialize
symbolGraphError from the prior lastGraphBuildCompleted entry for resolvedPath,
and only clear or replace it after a non-skipped symbol-graph rebuild succeeds
or fails; keep the existing error recording behavior in the catch block and
ensure skip-only rebuilds do not report a clean status.
---
Nitpick comments:
In `@src/constants.ts`:
- Line 115: Update QDRANT_MAX_REQUEST_BYTES to derive from an
environment-configured request-size limit, using the existing
QDRANT_URL/QDRANT_HOST configuration pattern in the qdrant service and
preserving the current default when the variable is absent or invalid. Ensure
QDRANT_UPSERT_BUDGET_BYTES continues to use the derived byte limit.
In `@src/services/symbol-graph-store.ts`:
- Around line 144-185: Move the UPSERT_MAX_POINTS constant above the
upsertWithinBudget function so its declaration appears before the function that
references it, without changing the batching logic or constant value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 762f38e9-e849-4d75-8202-34b2ff5167d6
📒 Files selected for processing (8)
DEVELOPER.mdsrc/constants.tssrc/services/code-graph.tssrc/services/qdrant.tssrc/services/symbol-graph-store.tssrc/tools/graph-tools.tstests/unit/qdrant-error-wrapping.test.tstests/unit/symbol-graph-upsert-budget.test.ts
…ebuilds A build that passes skipSymbolGraph never attempts symbol-graph persistence, so it has no standing to declare the symbol graph healthy — but it still wrote the completion record, clearing symbolGraphError. indexer.ts drives exactly that on the watcher path (rebuildGraph with skipSymbolGraph: useIncremental), so a single edited file after a failed persist reported a clean build while the symbol graph was still broken, which is the reporting gap this change set out to close. Carry the recorded failure forward when the symbol graph is skipped; only an actual successful persist clears it. Covered by a test that fails without the carry-forward, plus cases pinning the unwrapped server reason and the clear-on- success path. Caught in review of #92.
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 12 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/code-graph.ts (1)
234-291: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTotal rebuild failure silently clears a previously recorded
symbolGraphError.The outer
catch(lines 278-286) overwriteslastGraphBuildCompletedwithoutsymbolGraphError, dropping any prior symbol-graph failure. If this build fails before reaching the symbol-graph phase (e.g. transient Qdrant outage duringsaveGraphData), and a subsequent incremental rebuild runs withskipSymbolGraph: true, it readssymbolGraphErrorasundefined(line 261) and silently reports the symbol graph as healthy even though it was never actually fixed — exactly the false-success reporting this PR sets out to prevent.🐛 Proposed fix
} catch (err) { const message = err instanceof Error ? err.message : String(err); progress.error = message; lastGraphBuildCompleted.set(resolvedPath, { completedAt: Date.now(), durationMs: Date.now() - progress.startedAt, filesProcessed: progress.filesProcessed, filesSkipped: progress.filesSkipped, nodesCreated: 0, edgesCreated: 0, error: message, + symbolGraphError: lastGraphBuildCompleted.get(resolvedPath)?.symbolGraphError, }); throw err;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/code-graph.ts` around lines 234 - 291, Preserve the previously recorded symbolGraphError in the outer failure handler of the build flow. When updating lastGraphBuildCompleted in the catch block, carry forward the value from the existing record for resolvedPath unless the current build has explicitly established a new symbol-graph error; keep the existing error field and failure metadata unchanged.
🧹 Nitpick comments (2)
DEVELOPER.md (1)
1190-1199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
codebase_graph_statusreference doc doesn't mention the new symbol-graph-failure line.The "If ready" bullet doesn't note the new
Symbol graph FAILED to persist: ...warning added insrc/tools/graph-tools.ts(lines 317-325).📝 Proposed fix
If ready: Status READY with node/edge count, last built time, cache status, last build duration, and files skipped when non-zero + (plus a "Symbol graph FAILED to persist" warning when the last symbol-graph persist failed)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DEVELOPER.md` around lines 1190 - 1199, Update the codebase_graph_status “If ready” documentation to mention that the response may include the warning “Symbol graph FAILED to persist: ...”, matching the behavior implemented in the graph status tool.tests/unit/symbol-graph-upsert-budget.test.ts (1)
112-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
saveFilePayloadandsaveReverseShardsize guards.This suite tests the new
assertPointFitsguard viasaveNameShard, butsaveFilePayload(singular) andsaveReverseShardgained the identical guard (src/services/symbol-graph-store.tslines 266-273, 398-404) with no equivalent test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/symbol-graph-upsert-budget.test.ts` around lines 112 - 133, Add tests covering the size guards in saveFilePayload and saveReverseShard, mirroring the oversized-point rejection and zero-upsert assertions already used for saveNameShard. Verify each rejects with SymbolGraphPointTooLargeError and includes the relevant file or reverse-shard identifier in the error message, while preserving existing success-path coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/services/code-graph.ts`:
- Around line 234-291: Preserve the previously recorded symbolGraphError in the
outer failure handler of the build flow. When updating lastGraphBuildCompleted
in the catch block, carry forward the value from the existing record for
resolvedPath unless the current build has explicitly established a new
symbol-graph error; keep the existing error field and failure metadata
unchanged.
---
Nitpick comments:
In `@DEVELOPER.md`:
- Around line 1190-1199: Update the codebase_graph_status “If ready”
documentation to mention that the response may include the warning “Symbol graph
FAILED to persist: ...”, matching the behavior implemented in the graph status
tool.
In `@tests/unit/symbol-graph-upsert-budget.test.ts`:
- Around line 112-133: Add tests covering the size guards in saveFilePayload and
saveReverseShard, mirroring the oversized-point rejection and zero-upsert
assertions already used for saveNameShard. Verify each rejects with
SymbolGraphPointTooLargeError and includes the relevant file or reverse-shard
identifier in the error message, while preserving existing success-path
coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e50e29e-3094-4bb9-9c72-2350d6eb1519
📒 Files selected for processing (9)
DEVELOPER.mdsrc/constants.tssrc/services/code-graph.tssrc/services/qdrant.tssrc/services/symbol-graph-store.tssrc/tools/graph-tools.tstests/unit/graph-symbol-error-record.test.tstests/unit/qdrant-error-wrapping.test.tstests/unit/symbol-graph-upsert-budget.test.ts
…ure too The skipSymbolGraph carry-forward closed one route to false success; the outer catch was the other. A build that dies before the symbol-graph phase (a transient Qdrant outage during saveGraphData, say) rewrote the completion record without symbolGraphError, erasing the failure the previous build recorded. The next incremental build then carried that blank forward and reported the symbol graph healthy while it was still broken. Preserve the recorded failure there as well, read back from the map since the catch sits outside the scope of the try's local. Covered by a test that drives a rejected saveGraphData and asserts both the build error and the retained symbol-graph reason; it fails without the fix. Caught in review of #92.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/symbol-graph-upsert-budget.test.ts (1)
1-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid coverage of the byte-budget batching contract.
Batching, oversized-point-of-its-own, and hard-failure paths are all exercised and match the traced implementation.
One minor gap:
saveReverseShard's oversized-shard path (mirroring thesaveNameShardtest at Line 112) isn't directly tested, though it shares the sameassertPointFitsguard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/symbol-graph-upsert-budget.test.ts` around lines 1 - 134, Add a unit test covering saveReverseShard’s oversized-shard failure path, mirroring the existing saveNameShard test and using a payload exceeding QDRANT_MAX_REQUEST_BYTES. Assert that it rejects with SymbolGraphPointTooLargeError, identifies the reverse shard, and performs no upserts.src/tools/graph-tools.ts (1)
343-352: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSymbol-level query tools don't surface
symbolGraphError, so a stale/incomplete cache can still answer silently.Only
codebase_graph_statuswarns aboutlastBuild.symbolGraphError.codebase_impact,codebase_flow, andcodebase_symbolanswer straight fromgetSymbolGraphCachewithout checking it, so if the latest symbol-graph persist failed (e.g. a single oversized point/shard) but an older successful cache is still present, these tools would keep answering with stale data and no indication anything is wrong — the same "0 callers, no explanation" pattern issue#89is about, just for the narrower case the byte-budget batching in this PR doesn't eliminate.Consider prepending the same caveat used in status when
getLastGraphBuildCompleted(resolved)?.symbolGraphErroris set.Also applies to: 370-375, 429-437
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/graph-tools.ts` around lines 343 - 352, The symbol-level query tools do not report a recent symbol-graph persistence error when using an older cache. In the handlers for codebase_impact, codebase_flow, and codebase_symbol, call getLastGraphBuildCompleted with the resolved project context and prepend the same caveat used by codebase_graph_status when symbolGraphError is set, while preserving the existing query results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/tools/graph-tools.ts`:
- Around line 343-352: The symbol-level query tools do not report a recent
symbol-graph persistence error when using an older cache. In the handlers for
codebase_impact, codebase_flow, and codebase_symbol, call
getLastGraphBuildCompleted with the resolved project context and prepend the
same caveat used by codebase_graph_status when symbolGraphError is set, while
preserving the existing query results.
In `@tests/unit/symbol-graph-upsert-budget.test.ts`:
- Around line 1-134: Add a unit test covering saveReverseShard’s oversized-shard
failure path, mirroring the existing saveNameShard test and using a payload
exceeding QDRANT_MAX_REQUEST_BYTES. Assert that it rejects with
SymbolGraphPointTooLargeError, identifies the reverse shard, and performs no
upserts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbce55ce-a9eb-4587-b8b0-51450d3c2d94
📒 Files selected for processing (9)
DEVELOPER.mdsrc/constants.tssrc/services/code-graph.tssrc/services/qdrant.tssrc/services/symbol-graph-store.tssrc/tools/graph-tools.tstests/unit/graph-symbol-error-record.test.tstests/unit/qdrant-error-wrapping.test.tstests/unit/symbol-graph-upsert-budget.test.ts
…o persist codebase_graph_status reported a failed symbol-graph persist, but the tools that actually answer from it did not. codebase_impact, codebase_flow, codebase_symbol and codebase_symbols read whatever cache survived, so after a failed persist they answer from stale or absent data with nothing on screen to say why, which is the report behind #89 in the narrower case where an older cache is still present. Wrap the dispatch and prepend the recorded failure for those four tools. Also pin saveReverseShard's oversized-shard guard, which shared assertPointFits with the name-shard case but was reached by a separate call path and untested. Both raised in review of #92.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/code-graph.ts`:
- Around line 243-261: Update the incremental watcher flow around
updateChangedFilesSymbolGraph() so Phase F failures are captured and propagated
into symbolGraphError before the fallback full rebuild records
GraphBuildCompleted. Preserve the failure detail from indexer.ts rather than
leaving the field empty, ensuring the subsequent lastGraphBuildCompleted status
retains shard or payload errors even when the fallback rebuild succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ac85e83-37cc-4581-b2c7-8d549861e810
📒 Files selected for processing (9)
DEVELOPER.mdsrc/constants.tssrc/services/code-graph.tssrc/services/qdrant.tssrc/services/symbol-graph-store.tssrc/tools/graph-tools.tstests/unit/graph-symbol-error-record.test.tstests/unit/qdrant-error-wrapping.test.tstests/unit/symbol-graph-upsert-budget.test.ts
1 participant
Fixes #89.
The bug
On repos with large source files,
codebase_graph_buildreported success while the symbol graph silently held nothing, socodebase_impactanswered "0 callers" for every symbol. Three separate defects stacked up.1. Root cause: upserts were batched by count, not size.
saveFilePayloadssent a fixed 50 points per Qdrant request regardless of how big they were. A per-file payload scales with the source file (a ~900 KB Java file yields a ~6 MB point), so a few large files in one batch pushed the body past Qdrant'sservice.max_request_size_mbceiling (32 MiB by default) and the server rejected the entire batch.Reproduced against real Qdrant v1.17.0, six ~6 MB payloads in one request:
2. The real reason was thrown away. The Qdrant client puts the server's explanation in
err.data.status.errorwhile itsmessageis only the HTTP status text, so the log saidBad Requestand nothing else.3. The failure was recorded as a success. The catch logged, did not rethrow, and the build then wrote an unqualified completion record. The persist also writes payloads, then shards, then metadata last with no rollback, so a mid-way failure leaves committed points but no metadata, which is what makes impact return 0 rather than error.
The fix
SymbolGraphPointTooLargeErrornaming the file or shard and the server knob that raises the limit, rather than being dropped. Name shards, the one structure that grows unbounded with repo size, are size-checked for the same reason.describeQdrantErrordigs the server's explanation out oferr.data.status.error(through acausechain), the build records it onGraphBuildCompleted.symbolGraphError, andcodebase_graph_statusreports that symbol-level tools are incomplete until a rebuild succeeds.Backward compatibility (verified three ways)
mainand on this branch against a real Qdrant, dumped every persisted point with vectors and payloads, and diffed: identical. Nothing to migrate, existing graphs keep working.saveFilePayloadskeeps its signature;symbolGraphErroris an optional additive field; the build still returns and saves the file-import graph when the symbol half fails, exactly as before.Deliberately not done
Splitting an oversized shard across multiple points would change the on-disk layout, so it is out of scope here; such a shard now fails with a named, actionable error instead. Happy to do it as a follow-up if you want it.
Testing
npx tsc --noEmitclean,npm run lint(biome) clean,npm run buildsucceedsdescribeQdrantError(message/cause/no-duplicate/fallback)Summary by CodeRabbit
codebase_graph_status, including structured failure reasons.