Skip to content

fix: harden daemon and large-index recovery paths - #1562

Draft
danusha2345 wants to merge 2 commits into
colbymchenry:mainfrom
danusha2345:fix/upstream-bug-reports-1553-1560
Draft

fix: harden daemon and large-index recovery paths#1562
danusha2345 wants to merge 2 commits into
colbymchenry:mainfrom
danusha2345:fix/upstream-bug-reports-1553-1560

Conversation

@danusha2345

Copy link
Copy Markdown

Summary

  • verify daemon identity through the socket hello before trusting or signaling a recorded PID, prune phantom daemon listings, and let codegraph unlock clean stale daemon artifacts
  • bound pathological large-file parse deadlines, persist skipped and failed file records, and restore every secondary index dropped by an interrupted bulk load
  • avoid V8 argument-limit and RegExp code-space failures on dense C/C++ corpora
  • restrict JSX child synthesis to JavaScript-family files, including mixed-language repositories

Root causes and impact

Several recovery paths treated process liveness, file rejection, and interrupted bulk-load state as stronger evidence than they actually were. PID reuse could block daemon startup or target an unrelated process; rejected files could be retried forever; a killed bulk load could reopen without its lookup indexes; and corpus-controlled collection sizes could cross V8 runtime limits.

The changes make those paths bounded and durable while preserving explicit timeout overrides and deterministic file-order storage.

The stale PID/artifact half of #1553 is addressed here. The reported React Native startup OOM does not yet have a proven root cause and is deliberately not claimed as fixed by this PR.

Fixes #1555
Fixes #1556
Fixes #1557
Fixes #1558
Fixes #1559
Fixes #1560

Validation

  • npm test — 3081 passed, 9 skipped
  • focused regression suite — 151 passed
  • npx tsc --noEmit
  • npm run build
  • live shared-daemon restart and MCP retrieval probe on this repository — 0 timeouts
  • CodeGraph index state complete, with 0 pending changes and 0 pending references

@codegraph-impact codegraph-impact Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeGraph review

Overall risk: 🟡 Low — The #1553 daemon-identity handshake and lock-clearing rewrite — exactly the code meant to stop a stale lock from wedging every future session — has zero test coverage on its CLI/startup wiring (startDaemonProcess…

Verify in the running product (4 checks)

  • cli — On a project with a stale codegraph.lock and no running daemon, run codegraph unlock and confirm it now reports removing stale lock artifacts (not just the indexing lock), then run codegraph index and confirm it starts cleanly.
  • cli — Index a folder containing one file larger than the size limit, then run codegraph sync twice in a row and check the second run reports no files added/changed for that file.
  • cli — In a repo containing both C/C++ files with a string like "" and separate JavaScript/TypeScript files, run codegraph explore or codegraph callers on the C function and confirm no call edge to any JS component is reported.
  • cli — Force-kill a running codegraph daemon (kill -9), then immediately start a new session/command in that project and confirm a new daemon starts within a few seconds rather than reporting 'another daemon already holds the lock' indefinitely.

Worth double-checking

  • Daemon lock takeover/unlock wiring (#1553)
  • indexFileWithContent size_exceeded persistence (#1557)
  • reactJsxChildEdges internal language filter (#1560)
What to look for in each
  • Daemon lock takeover/unlock wiring (#1553) — startDaemonProcess, clearStaleDaemonArtifacts, clearStaleDaemonLock, and main are all flagged NO TEST REACHES THIS. Only the lower-level probeDaemonIdentity/listVerifiedDaemons/stopDaemonAt registry functions are unit-tested (tests/daemon-registry.test.ts). Manually exercise a real daemon takeover (kill -9 a daemon, immediately start a new session) and codegraph unlock against both a live and a PID-reused-but-dead lock before merging.
  • indexFileWithContent size_exceeded persistence (#1557) — indexFileWithContent now calls storeExtractionResult for oversized files (src/extraction/index.ts:2231-2241) instead of returning without storing, but the graph shows no test reaches this method directly (only the bulk indexAll and orchestrator.sync paths are covered by large-corpus-regressions.test.ts and sync.test.ts). Verify the single-file re-index path (MCP/editor "index this file" flow) also persists the size_exceeded record and doesn't get retried forever.
  • reactJsxChildEdges internal language filter (#1560) — The graph reports no test reaches reactJsxChildEdges itself — the two new tests assert on absence of jsx-render edges from outside, which also passes if the pass never runs at all for the parent's language. Confirm with a mixed JS+C file (not just separate files) that the per-node JS_FAMILY.includes(n.language) filter, not just the pass-level has(...JS_FAMILY) gate, is what's excluding the C nodes.
Findings — 1 low
Severity Finding Where
🟡 Low Secondary-index self-heal now runs unconditionally on every database open src/db/index.ts:148 — see inline comment
Business rules — 1 honored · 11 not applicable
Status Rule Note
✔ Honored Changelog and release contract New CHANGELOG entries are added under the existing Fixes section in user-facing prose (command names and behavior, no internal file/function names), matching the release contract.
— Not applicable Impact radius (blast radius) This diff does not touch getImpactRadius or src/graph/traversal.ts.
— Not applicable Risk policy: surface, don't score getNodeMetrics and the deterministic-not-scored risk policy are untouched by this diff.
— Not applicable Test selection (affected tests) The diff modifies src/bin/codegraph.ts only for the daemon and unlock commands; the affected test-selection traversal is unchanged.
— Not applicable Surfaces The init method itself is untouched; the diff modifies sync's internal recovery logic without changing the CodeGraph class's public surface or contract.
— Not applicable Installer contract The diff does not touch installer behavior or tests/installer-targets.test.ts.
— Not applicable Agent interface rules The MCP initialize handshake and agent-facing guidance are untouched by this diff.
— Not applicable Server instructions are the single source of truth src/mcp/server-instructions.ts is not part of this diff.
— Not applicable Call budget by repo size src/mcp/tools.ts and the explore call budget are untouched by this diff.
— Not applicable Source strings must exclude interpolated template literals The diff's changes to src/db/index.ts add healBulkSecondaryIndexes; extractSourceStrings and template-literal handling are not touched.
— Not applicable Index This is a documentation index entry, not a testable requirement of this diff.
— Not applicable SQLite backend The diff does not change the SQLite backend selection or WAL-mode handling; it only adds an idempotent index-recreation step inside the existing open() flow.
Full assessment

This PR bundles six independent bug fixes: daemon PID-reuse safety via a socket-identity handshake (#1553), a bounded large-file parse timeout (#1555), crash-recovered secondary-index/index-state restoration (#1556), persisted skip-reasons for oversized/failed files so sync stops retrying them (#1557), an argument-limit fix in bulk unresolved-ref loading plus bounded regex caches (#1558/#1559), and a JSX-edge synthesis pass that now gates on the containing node's own language instead of the whole project's (#1560). The core algorithmic pieces (resolveParseBudgetMs, the index-state fix, the FTS/secondary-index rebuild, the JSX language gate) are directly exercised by new unit tests and match their expected values. The main gap is that the daemon lock-takeover/unlock code path — main, startDaemonProcess, clearStaleDaemonArtifacts, clearStaleDaemonLock — is reached by no test per the code graph, despite being the exact mechanism the changelog says used to "permanently wedge" sessions.

Blast radius: 156 files affected beyond the diff · 756 symbols · 117 test files selected

Tests to run:

  • __tests__/adaptive-explore-sizing.test.ts
  • __tests__/android-res-exclusion.test.ts
  • __tests__/arkts-resolution.test.ts
  • __tests__/batched-ref-cleanup.test.ts
  • __tests__/c-fnptr-kernel-sweep.test.ts
  • __tests__/c-fnptr-synthesizer.test.ts
  • __tests__/celery-dispatch-synthesizer.test.ts
  • __tests__/cfml-inheritance-resolution.test.ts
  • __tests__/cfml-receiver-inference.test.ts
  • __tests__/cli-affected-paths.test.ts
  • __tests__/cli-no-color.test.ts
  • __tests__/cli-node-command.test.ts
  • __tests__/cli-query-command.test.ts
  • __tests__/closure-collection-synthesizer.test.ts
  • __tests__/concurrent-locking.test.ts
  • __tests__/config-secret-redaction.test.ts
  • __tests__/context-ranking.test.ts
  • __tests__/context.test.ts
  • __tests__/cooperative-yield.test.ts
  • __tests__/daemon-bind-failure.test.ts

Full report

Comment thread src/db/index.ts
@danusha2345

Copy link
Copy Markdown
Author

Review follow-up (commit 5e1e2b1):

  • Added end-to-end codegraph unlock coverage for stale indexing plus daemon artifacts, followed by a successful index; a matching live daemon is also proven to remain untouched.
  • Added the single-file indexFiles oversized-file path and verified the persisted size_exceeded record makes the next sync a no-op.
  • Strengthened the mixed C/JavaScript regression so a real JavaScript jsx-render edge proves the pass ran while no edge originates from the C file.
  • Added a real daemon SIGKILL test followed by simulated OS PID reuse; a replacement daemon starts and the unrelated live PID is never signaled.
  • Gated secondary-index recovery behind a sqlite_master count so healthy opens perform no schema read or index DDL.

Validation on the final commit: npm run build; focused review suite 66/66; full suite 3086 passed, 9 skipped; git diff --check. The locally linked build was restarted through a real MCP client: warm explore 393 ms, follow-up 173 ms, 0/1 timeouts, complete index with 0 pending changes and 0 pending refs.

@codegraph-impact codegraph-impact Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeGraph review

Overall risk: 🟠 Medium — New socket-handshake identity check now gates every daemon takeover and stale-lock cleanup, yet the code graph traces no test into clearStaleDaemonLock or startDaemonProcess.

Verify in the running product (5 checks)

  • cli — Simulate a daemon lock naming a reused PID (write .codegraph/daemon.pid with a live but unrelated PID and no listening socket), then run codegraph unlock <dir> and codegraph index.
  • cli — With that same phantom daemon.pid state, run codegraph daemons (the daemon picker/list command).
  • cli — Index a project containing a single large (~900KB–1MB) data-only C header with no meaningful symbols and time the run.
  • cli — Add a file over the size limit, run codegraph index, then run codegraph sync twice more without touching the file, watching output/status for that file.
  • cli — In a project with both a .c file containing a string literal like "" and a real .jsx component named Foo, index the project and inspect the graph/explore output for a call edge from the C function to Foo.

Worth double-checking

  • Daemon identity safety coverage
  • healBulkSecondaryIndexes guard
  • index_state recovery correctness
What to look for in each
  • Daemon identity safety coverage — clearStaleDaemonLock, startDaemonProcess, and clearStaleDaemonArtifacts — the functions implementing the socket-handshake PID-reuse fix (#1553) — are flagged by the code graph as reached by no test, even though cli-unlock.test.ts and mcp-daemon.test.ts appear to exercise takeover/unlock via spawned subprocesses. Confirm those tests actually cover the allowLivePid clear path and the artifact-cleanup path, since a bug here could either wedge every future session or fail to protect an unrelated live process.
  • healBulkSecondaryIndexes guard — The new healBulkSecondaryIndexes() call added to DatabaseConnection.open() (src/db/index.ts) throws schema.sql: index ${idx} not found for crash recovery with no try/catch around the call site in open(). Verify every name in BULK_PARSE_INDEX_NAMES/BULK_REF_INDEX_NAMES/BULK_EDGE_INDEX_NAMES matches an exact CREATE INDEX IF NOT EXISTS in schema.sql — a mismatch would break DatabaseConnection.open() for every project, not just crash recovery, since open() is called on nearly every CLI/MCP/daemon path.
  • index_state recovery correctness — sync() now unconditionally flips index_state from 'indexing' to 'complete' whenever a full (no-paths) sync reaches the end of its try block (src/index.ts). Confirm this can't mark a genuinely incomplete recovery as complete — e.g. a full sync that only touches a subset of files while pending refs elsewhere remain unresolved.
Business rules — 1 honored · 11 not applicable
Status Rule Note
✔ Honored Changelog and release contract New CHANGELOG.md entries are added under the existing Fixes heading with user-facing prose (commands, languages, behavior) and no internal function/file names.
— Not applicable Impact radius (blast radius) getImpactRadius and src/graph/traversal.ts are not touched by this change.
— Not applicable Risk policy: surface, don't score getNodeMetrics and the risk-surfacing contract are untouched; this diff adds fixes, not a scoring mechanism.
— Not applicable Test selection (affected tests) The diff modifies src/bin/codegraph.ts's daemon-list and unlock commands only; the affected test-selection logic is unchanged.
— Not applicable Surfaces CodeGraph.init is unmodified; only sync() gained new logic, so the documented surfaces list is unaffected.
— Not applicable Installer contract tests/installer-targets.test.ts and installer behavior are untouched by this diff.
— Not applicable Agent interface rules MCP initialize and agent-facing instructions are unmodified.
— Not applicable Server instructions are the single source of truth src/mcp/server-instructions.ts is untouched; no guidance was duplicated or edited.
— Not applicable Call budget by repo size src/mcp/tools.ts and getExploreBudget are untouched by this diff.
— Not applicable Source strings must exclude interpolated template literals src/db/index.ts changes only add healBulkSecondaryIndexes; extractSourceStrings and template-literal handling are unmodified.
— Not applicable Index This is a documentation index entry, not an enforceable requirement of the change.
— Not applicable SQLite backend The sole node:sqlite backend and WAL mode selection are unchanged; the new index-healing code only recreates secondary indexes, not the backend.
Full assessment

This PR bundles seven independent hardening fixes: daemon PID-reuse safety via a socket-handshake identity probe (#1553), a capped large-file parse timeout (#1555), crash-recovery index/state healing (#1556), persisted skipped-file records so sync stops retrying them (#1557), a V8 argument-limit fix in unresolved-ref loading (#1558), a bounded regex cache in C function-pointer analysis (#1559), and a JS-family language gate for JSX edge synthesis (#1560). Each fix ships with a matching new or extended test. The main residual concern is that the exact functions implementing the safety-critical daemon takeover/cleanup logic (clearStaleDaemonLock, startDaemonProcess, clearStaleDaemonArtifacts) show no traced test coverage in the code graph, and DatabaseConnection.open() now runs an unguarded index-recreation step on every open across the whole product.

Blast radius: 155 files affected beyond the diff · 760 symbols · 118 test files selected

Tests to run:

  • __tests__/adaptive-explore-sizing.test.ts
  • __tests__/android-res-exclusion.test.ts
  • __tests__/arkts-resolution.test.ts
  • __tests__/batched-ref-cleanup.test.ts
  • __tests__/c-fnptr-kernel-sweep.test.ts
  • __tests__/c-fnptr-synthesizer.test.ts
  • __tests__/celery-dispatch-synthesizer.test.ts
  • __tests__/cfml-inheritance-resolution.test.ts
  • __tests__/cfml-receiver-inference.test.ts
  • __tests__/cli-affected-paths.test.ts
  • __tests__/cli-no-color.test.ts
  • __tests__/cli-node-command.test.ts
  • __tests__/cli-query-command.test.ts
  • __tests__/cli-unlock.test.ts
  • __tests__/closure-collection-synthesizer.test.ts
  • __tests__/concurrent-locking.test.ts
  • __tests__/config-secret-redaction.test.ts
  • __tests__/context-ranking.test.ts
  • __tests__/context.test.ts
  • __tests__/cooperative-yield.test.ts

Full report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant