From 489b4749c725cc446df7245fa611d77dab9c301e Mon Sep 17 00:00:00 2001 From: Colby Mchenry Date: Mon, 5 Oct 2026 14:41:59 +0000 Subject: [PATCH 1/5] fix(vbnet,csharp): index every Structure member; walk property bodies and VB initializers (#2345) VB.NET's grammar tags each member of a `Structure` as its own `body` field, so extractAggregate's field lookup saw only the first member. SCrawler's UserMedia indexed none of its members, and a structure that opened with a nested enum indexed only the enum. Aggregates now resolve their body the way classes and enums do (resolveBody, which VB maps to the declaration itself). Objective-C, the only other struct language with that hook, is unchanged. Property bodies were never walked: the propertyTypes branch set skipChildren after extractProperty, so calls, instantiations and reads in a VB `Get`/`Set` block, `= initializer` or `As New`, and in a C# accessor body or `=> expr`, were lost. - propertyBodies() names the parts of a property that run code (VB: the whole declaration, as its methods are walked; C#: each accessor's body and an arrow value), walked with the property on the stack. The candidates-only fn-ref scan skips the walked subtrees by node id, so a function value is captured once, from the property. C# `= initializer`s stay unwalked, as C# field initializers are. - The C# kernel mirrors it (property_bodies, scan skip list). The parity suite and a new micro pass; a serilog sweep is 209/214 byte-identical with 0 diffs (5 deferred for parse errors). - VB field declarators (`= expr`, `As New T`) and Custom Event AddHandler/RemoveHandler/RaiseEvent blocks are walked as their field or event. Before/after (nodes / edges): SCrawler 9,950 -> 10,427 / 14,336 -> 15,092; staxrip 13,440 -> 13,644 / 24,120 -> 28,002, most of it its `Property X As New NumParam With {...}` settings; serilog edges 6,642 -> 6,654. All 23 removed edges are the same call sites re-resolved by VB's name-guessing resolver now that struct members are candidates: 10 wrong edges dropped, 9 correct ones now guessed wrong, 4 wrong either way. Designer -> Add/Size edge counts are unchanged. EXTRACTION_VERSION 27 -> 28. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 + __tests__/csharp-property-accessors.test.ts | 91 +++++++++ __tests__/kernel-csharp-parity.test.ts | 20 +- __tests__/vbnet-member-bodies.test.ts | 209 ++++++++++++++++++++ codegraph-kernel/src/csharp.rs | 80 ++++++-- docs/design/csharp-kernel-port-checklist.md | 22 ++- src/extraction/extraction-version.ts | 2 +- src/extraction/languages/vbnet.ts | 9 +- src/extraction/tree-sitter.ts | 62 ++++-- 9 files changed, 459 insertions(+), 42 deletions(-) create mode 100644 __tests__/csharp-property-accessors.test.ts create mode 100644 __tests__/vbnet-member-bodies.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 06ca319d98..44144e1606 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,12 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixes + +- In VB.NET, every member of a `Structure` is now indexed, including its fields, properties, methods, constructors and nested enums. Before, only the first member was, so the rest could not be found and their callers looked empty. +- In VB.NET and C#, what a property's `Get` and `Set` code calls, creates and reads now belongs to that property, as do C#'s `get => …` accessors and `=> …` property bodies. Before, it was dropped, so a method used only from a property looked unused. +- In VB.NET, a field or property initializer like `= Compute()` or `As New List(Of Order)` now links what it calls and creates, and so do a `Custom Event`'s `AddHandler`, `RemoveHandler` and `RaiseEvent` blocks. Re-index VB.NET and C# projects after upgrading. + ## [1.6.2] - 2026-10-03 diff --git a/__tests__/csharp-property-accessors.test.ts b/__tests__/csharp-property-accessors.test.ts new file mode 100644 index 0000000000..b77d8549d6 --- /dev/null +++ b/__tests__/csharp-property-accessors.test.ts @@ -0,0 +1,91 @@ +/** + * C# property accessor bodies belong to the property. `get { … }`, + * `set { … }`, `get => …` and an expression-bodied `=> …` property used to + * be skipped by both extractors, so the calls, instantiations and static + * reads written there were lost and a getter's callee looked unused. + * + * Runs against the native kernel (when built) and the wasm extractor, which + * must agree. + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { CodeGraph } from '../src'; +import type { Node } from '../src/types'; + +const FILES: Record = { + 'Helper.cs': `public static class Helper { + public static int Compute(int n) => n; + public static void Store(int n) {} + public static int Arrow() => 1; + public static int GetA() => 1; + public static void SetA(int v) {} +} +public class Widget {} +public static class Defaults { public const string Name = "x"; } +`, + 'Box.cs': `public class Box { + private int _x; + public int Value { + get { return Helper.Compute(_x); } + set { Helper.Store(value); _x = value; } + } + public int Arrow => Helper.Arrow(); + public int Both { get => Helper.GetA(); set => Helper.SetA(value); } + public Widget Made { get { return new Widget(); } } + public string Label { get { return Defaults.Name; } } + private void Handle(int v) { } + public System.Action Handler { get { return Pick(Handle); } } +} +`, +}; + +describe('C# property accessor bodies', () => { + let root = ''; + let cg: CodeGraph | undefined; + let kernel: string | undefined; + + beforeEach(() => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-cs-accessors-')); + for (const [rel, content] of Object.entries(FILES)) fs.writeFileSync(path.join(root, rel), content); + kernel = process.env.CODEGRAPH_KERNEL; + }); + + afterEach(() => { + cg?.close(); + cg = undefined; + fs.rmSync(root, { recursive: true, force: true }); + if (kernel === undefined) delete process.env.CODEGRAPH_KERNEL; + else process.env.CODEGRAPH_KERNEL = kernel; + }); + + it.each(['default', 'wasm'])('are walked as the property (%s)', async (backend) => { + if (backend === 'wasm') process.env.CODEGRAPH_KERNEL = '0'; + else delete process.env.CODEGRAPH_KERNEL; + cg = await CodeGraph.init(root, { index: true }); + const graph = cg; + const member = (qualifiedName: string): Node => { + const node = graph.getNodesInFile('Box.cs').find((n) => n.qualifiedName === qualifiedName); + expect(node, qualifiedName).toBeDefined(); + return node!; + }; + const targets = (node: Node, kind: string): string[] => + graph + .getOutgoingEdgesFrom([node.id]) + .filter((e) => e.kind === kind) + .map((e) => graph.getNode(e.target)!.qualifiedName) + .sort(); + + expect(targets(member('Box::Value'), 'calls')).toEqual(['Helper::Compute', 'Helper::Store']); + expect(targets(member('Box::Arrow'), 'calls')).toEqual(['Helper::Arrow']); + expect(targets(member('Box::Both'), 'calls')).toEqual(['Helper::GetA', 'Helper::SetA']); + expect(targets(member('Box::Made'), 'instantiates')).toEqual(['Widget']); + expect(targets(member('Box::Label'), 'references')).toContain('Defaults'); + // A method passed as a value inside an accessor is the property's + // reference, captured once — not the class's as well. + expect(targets(member('Box::Handler'), 'references')).toContain('Box::Handle'); + expect(targets(member('Box'), 'references')).not.toContain('Box::Handle'); + expect(targets(member('Box'), 'calls')).toEqual([]); + }); +}); diff --git a/__tests__/kernel-csharp-parity.test.ts b/__tests__/kernel-csharp-parity.test.ts index f26f5072df..3d09c19d98 100644 --- a/__tests__/kernel-csharp-parity.test.ts +++ b/__tests__/kernel-csharp-parity.test.ts @@ -8,7 +8,8 @@ * * - Torture.cs — block namespace + nested/second-namespace quirks, * base_list shapes, records, properties (incl. the bare-identifier - * signature loss and never-walked accessor bodies), fields/constants, + * signature loss, accessor bodies walked as the property and + * never-walked initializers), fields/constants, * events/operators/indexer/destructor (no nodes, calls → class), ctor * initializer hole, explicit interface impl, local functions, the call * zoo (raw member-access texts, chained re-encode, `(myDel)(x)` conv, @@ -167,6 +168,23 @@ describe.skipIf(!kernelBuilt)('kernel C# extraction parity', () => { source: 'public record Empty;\n', minNodes: 2, }, + { + // Accessor bodies and `=> expr` are walked as the property (calls, + // instantiations, static reads, fn-ref candidates); an `= initializer` + // is only scanned for candidates, attributed to the class. + name: 'property bodies are walked as the property; initializers only scanned', + source: [ + 'public class C {', + ' void H(int v) { }', + ' public Action P { get { return Make(H); } set { Register(H); } }', + ' public int Q => Run(H);', + ' public Action R { get; } = Wrap(H);', + ' public int S { get => Calc.Max; init => Store(new Widget()); }', + '}', + '', + ].join('\n'), + minNodes: 7, + }, ]; for (const m of MICROS) { diff --git a/__tests__/vbnet-member-bodies.test.ts b/__tests__/vbnet-member-bodies.test.ts new file mode 100644 index 0000000000..8c280a5b55 --- /dev/null +++ b/__tests__/vbnet-member-bodies.test.ts @@ -0,0 +1,209 @@ +/** + * VB.NET member bodies the extractor used to skip: + * + * - A `Structure` lists each member as its own `body` field, so reading the + * field returned only the first member: SCrawler's `UserMedia` indexed its + * nested `States` enum (declared first) and nothing else, and a structure + * that opened with a field indexed no members at all. + * - A property's `Get` / `Set` blocks, its `= initializer` and its + * `As New T` were never walked, nor a `Custom Event`'s accessors. + * - A field's `= initializer` and `As New T` were never walked. + * + * The calls, instantiations and reads in those places belong to the member + * that declares them. + */ +import { describe, it, expect, afterAll, beforeAll } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { CodeGraph } from '../src'; +import type { Node } from '../src/types'; + +let root = ''; +let cg: CodeGraph; + +beforeAll(async () => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-vb-bodies-')); + const files: Record = { + 'Helper.vb': `Public Class Helper + Public Sub New(ByVal n As Integer) + End Sub + Public Shared Function Compute(ByVal n As Integer) As Integer + Return n + End Function + Public Shared Sub Store(ByVal n As Integer) + End Sub + Public Shared Function Seed() As Integer + Return 1 + End Function + Public Shared Function Format(ByVal o As Object) As String + Return "" + End Function + Public Shared Sub Log(ByVal s As String) + End Sub + Public Shared Sub Attach(ByVal h As EventHandler) + End Sub +End Class +`, + 'Widget.vb': `Public Class Widget +End Class +`, + 'UserMedia.vb': `Public Structure UserMedia + Public State As States + Private Count As Integer + Public Enum States + Unknown + Downloaded + End Enum + Public ReadOnly Property Name As String + Get + Return Helper.Format(State) + End Get + End Property + Public Sub New(ByVal s As States) + State = s + Helper.Log("created") + End Sub + Public Function IsDone() As Boolean + Return State = States.Downloaded + End Function +End Structure +`, + 'Box.vb': `Public Class Box + Private _x As Integer + Private _items As New Widget + Private _helper As Helper = New Helper(1) + Private Shared ReadOnly Def As Integer = Helper.Compute(3), Other As Integer = Helper.Seed() + Public Property Value As Integer + Get + Return Helper.Compute(_x) + End Get + Set(ByVal v As Integer) + Helper.Store(v) + _x = v + End Set + End Property + Public Property Auto As Integer = Helper.Seed() + Public ReadOnly Property Lst As New Widget + Public Custom Event Changed As EventHandler + AddHandler(ByVal value As EventHandler) + Helper.Attach(value) + End AddHandler + RemoveHandler(ByVal value As EventHandler) + End RemoveHandler + RaiseEvent(ByVal sender As Object, ByVal e As EventArgs) + End RaiseEvent + End Event +End Class +`, + // The 1.6.2 receiver gate must hold in the newly walked places too: + // `Me.Panel.Controls.Add(…)` and `New System.Drawing.Size(…)` name no + // project member. + 'Collections/DataColorCollection.vb': `Friend Class DataColorCollection + Friend Sub Add(ByVal Item As Object) + End Sub +End Class +`, + 'Editors/UsersInfoForm.vb': `Friend Class UsersInfoForm + Private Enum EComparers + Name + Size + End Enum +End Class +`, + 'MainForm.vb': `Public Class MainForm + Private ReadOnly DefaultSize As System.Drawing.Size = New System.Drawing.Size(184, 25) + Public ReadOnly Property Ready As Boolean + Get + Me.Panel.Controls.Add(Me.Button1) + Return True + End Get + End Property +End Class +`, + }; + for (const [rel, content] of Object.entries(files)) { + fs.mkdirSync(path.dirname(path.join(root, rel)), { recursive: true }); + fs.writeFileSync(path.join(root, rel), content); + } + cg = await CodeGraph.init(root, { index: true }); +}); + +afterAll(() => { + cg?.close(); + if (root) fs.rmSync(root, { recursive: true, force: true }); +}); + +function member(file: string, qualifiedName: string): Node { + const node = cg.getNodesInFile(file).find((n) => n.qualifiedName === qualifiedName); + expect(node, `${qualifiedName} in ${file}`).toBeDefined(); + return node!; +} + +/** `kind` edges leaving `node`, as target qualified names. */ +function targets(node: Node, kind: string): string[] { + return cg + .getOutgoingEdgesFrom([node.id]) + .filter((e) => e.kind === kind) + .map((e) => cg.getNode(e.target)!.qualifiedName) + .sort(); +} + +describe('VB.NET member bodies', () => { + it('indexes every member of a Structure, not just the first', () => { + const struct = member('UserMedia.vb', 'UserMedia'); + expect(struct.kind).toBe('struct'); + const contained = cg + .getOutgoingEdgesFrom([struct.id]) + .filter((e) => e.kind === 'contains') + .map((e) => cg.getNode(e.target)!) + .map((n) => `${n.kind} ${n.qualifiedName}`) + .sort(); + expect(contained).toEqual([ + 'enum UserMedia::States', + 'field UserMedia::Count', + 'field UserMedia::State', + 'method UserMedia::IsDone', + 'method UserMedia::New', + 'property UserMedia::Name', + ]); + expect(member('UserMedia.vb', 'UserMedia::States::Downloaded').kind).toBe('enum_member'); + expect(targets(member('UserMedia.vb', 'UserMedia::New'), 'calls')).toEqual(['Helper::Log']); + }); + + it("attributes a property's Get and Set blocks to the property", () => { + expect(targets(member('Box.vb', 'Box::Value'), 'calls')).toEqual(['Helper::Compute', 'Helper::Store']); + expect(targets(member('UserMedia.vb', 'UserMedia::Name'), 'calls')).toEqual(['Helper::Format']); + }); + + it("walks a property's initializer and its As New", () => { + expect(targets(member('Box.vb', 'Box::Auto'), 'calls')).toEqual(['Helper::Seed']); + expect(targets(member('Box.vb', 'Box::Lst'), 'instantiates')).toEqual(['Widget']); + }); + + it("walks each field's initializer and its As New, per declarator", () => { + expect(targets(member('Box.vb', 'Box::Def'), 'calls')).toEqual(['Helper::Compute']); + expect(targets(member('Box.vb', 'Box::Other'), 'calls')).toEqual(['Helper::Seed']); + expect(targets(member('Box.vb', 'Box::_items'), 'instantiates')).toEqual(['Widget']); + expect(targets(member('Box.vb', 'Box::_helper'), 'instantiates')).toEqual(['Helper']); + }); + + it("attributes a Custom Event's accessors to the event", () => { + expect(targets(member('Box.vb', 'Box::Changed'), 'calls')).toEqual(['Helper::Attach']); + }); + + it('keeps the class out of what its members do', () => { + expect(targets(member('Box.vb', 'Box'), 'calls')).toEqual([]); + expect(targets(member('Box.vb', 'Box'), 'instantiates')).toEqual([]); + }); + + it('still links nothing through a receiver no name resolves', () => { + const ids = cg.getNodesInFile('MainForm.vb').map((n) => n.id); + const linked = cg + .getOutgoingEdgesFrom(ids) + .filter((e) => e.kind !== 'contains') + .map((e) => cg.getNode(e.target)!.qualifiedName); + expect(linked).not.toContain('DataColorCollection::Add'); + expect(linked).not.toContain('UsersInfoForm::EComparers::Size'); + }); +}); diff --git a/codegraph-kernel/src/csharp.rs b/codegraph-kernel/src/csharp.rs index 7d481662a7..04e20ca5a5 100644 --- a/codegraph-kernel/src/csharp.rs +++ b/codegraph-kernel/src/csharp.rs @@ -5,7 +5,7 @@ //! path, bug-for-bug, verified by scripts/kernel-parity.mjs and the full-index //! dump-diff gate. The authoritative quirk list is //! docs/design/csharp-kernel-port-checklist.md — including every deliberate -//! emission hole (property/accessor bodies, constructor initializers, +//! emission hole (field/property initializers, constructor initializers, //! delegates/events/operators/indexers, top-level locals) and garbage ref //! (`(repo)` primary-ctor extends, `: byte` enum extends, `nameof` calls) //! this file preserves on purpose. Positions in UTF-16 code units. Files whose @@ -572,21 +572,36 @@ impl<'t> Walker<'t> { self.extract_enum(node); skip_children = true; } else if kind == "property_declaration" && self.inside_class_like() { - // Property accessor/expression bodies are NEVER walked (calls - // inside are lost by design) — candidates-only scan. - self.extract_property(node); - self.scan_fn_ref_subtree(node, 0); + // The code a property runs — its accessor bodies and `=> expr` — + // is walked with the property on the stack (propertyBodies). The + // candidates-only scan covers the rest (an `= initializer`), + // skipping what the body walk captured. + let walked: Vec = match self.extract_property(node) { + Some((row, name)) => { + let bodies = property_bodies(node); + if !bodies.is_empty() { + self.stack.push(Scope { row, kind: "property", name }); + for body in &bodies { + self.visit_function_body(*body); + } + self.stack.pop(); + } + bodies.iter().map(|b| b.id()).collect() + } + None => Vec::new(), + }; + self.scan_fn_ref_subtree(node, 0, &walked); skip_children = true; } else if kind == "field_declaration" && self.inside_class_like() { self.extract_field(node); - self.scan_fn_ref_subtree(node, 0); + self.scan_fn_ref_subtree(node, 0, &[]); skip_children = true; } else if kind == "local_declaration_statement" && !self.inside_class_like() { // Top-level statements: extractVariable's generic fallback finds no // direct identifier/variable_declarator children (C# nests them in // variable_declaration) → ZERO nodes, zero refs. Candidates only. self.extract_variable(node); - self.scan_fn_ref_subtree(node, 0); + self.scan_fn_ref_subtree(node, 0, &[]); skip_children = true; } else if kind == "using_directive" { self.extract_import(node); @@ -789,9 +804,9 @@ impl<'t> Walker<'t> { } /// extractProperty (1986) — property_declaration only (dispatch-gated to - /// class-like scopes). Accessor bodies and `=>` value clauses are never - /// walked; type refs DO come from the `type` field. - fn extract_property(&mut self, node: Node<'t>) { + /// class-like scopes). Type refs come from the `type` field; the caller + /// walks the bodies (property_bodies) with the returned row on the stack. + fn extract_property(&mut self, node: Node<'t>) -> Option<(u32, String)> { let docstring = preceding_docstring(node, self.src); let visibility = Some(self.visibility_of(node)); let is_static = Some(self.is_static(node)); // ?? false — always concrete @@ -804,10 +819,10 @@ impl<'t> Walker<'t> { .filter_map(|i| node.named_child(i)) .find(|c| c.kind() == "identifier") }); - let Some(name_node) = name_node else { return }; + let name_node = name_node?; let name = self.text(name_node).to_string(); if name.is_empty() { - return; + return None; } // Generic scan (isTsJsField=false): FIRST namedChild that isn't a @@ -842,11 +857,10 @@ impl<'t> Walker<'t> { &name, node, Extra { docstring, signature: Some(signature), visibility, is_static, ..Extra::default() }, - ); - if let Some(row) = row { - // decorators: none for C#; then the csharp type-ref path. - self.extract_csharp_type_refs(node, row); - } + )?; + // decorators: none for C#; then the csharp type-ref path. + self.extract_csharp_type_refs(node, row); + Some((row, name)) } /// extractField (2046) — field_declaration; each declarator becomes a @@ -1468,11 +1482,15 @@ impl<'t> Walker<'t> { }); } - fn scan_fn_ref_subtree(&mut self, node: Node<'t>, depth: u32) { + fn scan_fn_ref_subtree(&mut self, node: Node<'t>, depth: u32, walked: &[usize]) { stack_guard!(); if depth > 12 { return; } + // Subtrees the body walker has already been through. + if walked.contains(&node.id()) { + return; + } // functionTypes is EMPTY for C#; the literal halt list applies — // lambda_expression IS C#'s lambda, so initializer lambdas stop the // scan; anonymous_method_expression is NOT listed and scans through. @@ -1487,7 +1505,7 @@ impl<'t> Walker<'t> { self.maybe_capture_fn_refs(node); for i in 0..node.named_child_count() { if let Some(c) = node.named_child(i) { - self.scan_fn_ref_subtree(c, depth + 1); + self.scan_fn_ref_subtree(c, depth + 1, walked); } } } @@ -1624,6 +1642,30 @@ impl<'t> Walker<'t> { } } +/// propertyBodies (tree-sitter.ts) — the parts of a property that run code: +/// each accessor's `body` (a block or `=> expr`) and an expression-bodied +/// property's `=> …` value. An `= initializer` value is not a body. +fn property_bodies(node: Node) -> Vec { + let mut bodies = Vec::new(); + if let Some(accessors) = node.child_by_field_name("accessors") { + for i in 0..accessors.named_child_count() { + let Some(accessor) = accessors.named_child(i) else { continue }; + if accessor.kind() != "accessor_declaration" { + continue; + } + if let Some(body) = accessor.child_by_field_name("body") { + bodies.push(body); + } + } + } + if let Some(value) = node.child_by_field_name("value") { + if value.kind() == "arrow_expression_clause" { + bodies.push(value); + } + } + bodies +} + fn find_anonymous_class_body(node: Node) -> Option { for i in 0..node.named_child_count() { if let Some(child) = node.named_child(i) { diff --git a/docs/design/csharp-kernel-port-checklist.md b/docs/design/csharp-kernel-port-checklist.md index 41894cb067..e817968c95 100644 --- a/docs/design/csharp-kernel-port-checklist.md +++ b/docs/design/csharp-kernel-port-checklist.md @@ -234,7 +234,7 @@ all PRESERVE): | `enum_declaration` | enumTypes:1064 → extractEnum:1914 | body `enum_member_declaration_list` required (bodiless → no node); extractInheritance sees `base_list` → **the underlying type `: byte` emits an `extends` ref named `byte`** (quirk, §inheritance); `enum_member_declaration` children → extractEnumMembers:1958 — `name` field path: ONE `enum_member` node per member, positioned at the member node (attributes included in its span), values/attributes ignored; non-member children (preproc_*, comment) → visitNode (no-op) | | `method_declaration` | methodTypes:1027 → extractMethod:1737 | classifyMethodNode absent → always extractMethod. Gate 1747 passes via class-like (a method_declaration outside a type does not occur in non-erroring C# — top-level `void M(){}` parses as local_function_statement, probed); bodyless interface/partial signatures mint nodes with no body walk; **expression-bodied methods have `body: arrow_expression_clause` (a real body FIELD, probed) → walked** | | `constructor_declaration` | methodTypes → extractMethod | name field = the class-name identifier → **method node named like the class**; returnType undefined; **`constructor_initializer` (`: base(args)` / `: this(args)`) is a sibling of the body field → NEVER walked → calls inside initializer args are LOST** (probed); expression-bodied ctor body = arrow_expression_clause → walked | -| `property_declaration` (inside class-like) | propertyTypes:1075 → extractProperty:1986 | property node + scanFnRefSubtree (capture-only) + skipChildren → **accessor bodies (`get { … }`, `get => …`) and the `=> expr` value clause are NEVER walked — calls inside property getters/setters/expression bodies emit NOTHING** (only fn-ref candidates). §property below | +| `property_declaration` (inside class-like) | propertyTypes:1075 → extractProperty:1986 | property node, then **propertyBodies — each accessor's `body` (`get { … }`, `set => …`) and an expression-bodied `=> expr` `value:` — walked by visitFunctionBody with the property pushed** (calls, instantiates, static reads and fn-ref candidates attribute to the property; changed 2026-10-04, previously never walked), then scanFnRefSubtree (capture-only, attributed to the class) over the rest — **the `= initializer` stays unwalked** — skipping the walked bodies, + skipChildren. §property below | | `field_declaration` (inside class-like) | fieldTypes:1084 → extractField:2046 | field/constant nodes per declarator + scanFnRefSubtree + skipChildren → **field initializers emit no calls/instantiates/static-member refs** (fn-ref candidates only). §field below | | `local_declaration_statement` | variableTypes:1098 (only reachable at top level — global statements; body locals go through visitFunctionBody instead) | not class-like → extractVariable:2538 → **generic fallback (2863-2881) finds no direct `identifier`/`variable_declarator` children (the declarator nests inside `variable_declaration`, probed) → ZERO nodes minted**; isClassScopeConstantAssignment (1508) needs node.type `assignment` → never true. skipChildren=true + scanFnRefSubtree → **a top-level `var builder = WebApplication.CreateBuilder(args);` produces NO node, NO calls ref, NO instantiates** — only fn-ref candidates. PRESERVE | | `using_directive` | importTypes:1209 → extractImport:3170 | hook (§config) → import node + ONE generic `imports` ref {fromNodeId: nodeStack top (namespace node if present, else file), referenceName: moduleName, line/col of the directive}; **no per-binding emitter** (the TS/py/rust/php/ruby ladder at 3197-3234 excludes csharp) | @@ -305,15 +305,22 @@ for C#) or bare `name`. QUIRKS (probed, PRESERVE): - An expression-bodied property (`public int Computed => MaxItems + 1;`) has children [modifier, predefined_type, identifier, **arrow_expression_clause (`value:` field)**] → typeNode = predefined_type → signature `"int Computed"`; - the arrow clause is NEVER walked (calls inside lost). -- `{ get; } = new();` initializers: the `value:` implicit_object_creation and - the accessor_list are both skipped/excluded → no refs, no instantiates. + the arrow clause is a property body (walked as the property, below). +- `{ get; } = new();` initializers: the `value:` implicit_object_creation is + NOT a body → no refs, no instantiates (candidates-only scan, attributed to + the class); the accessor_list is excluded from the type scan. Then extractDecoratorsFor (no-op) and **extractTypeAnnotations (2037) → extractCsharpTypeRefs** — the `type` field IS walked for refs (so `public List Items` emits references `List` + `Foo` even though the signature -kept the raw text). Return value feeds no body walk (the classifyMethodNode -initializer-walk path at 1031-1047 is TS-only). +kept the raw text). The returned node is pushed while propertyBodies +(tree-sitter.ts; csharp.rs `property_bodies`) are walked: every +`accessor_declaration`'s `body` field (block or arrow_expression_clause, in +accessor order), then the property's `value:` when it is an +arrow_expression_clause. The fn-ref scan that follows skips those subtrees +by node id, so a candidate is captured once — from the property when it sits +in a body, from the class when it sits in an initializer. (The +classifyMethodNode initializer-walk path at 1031-1047 is TS-only.) ### extractField (2046) — field_declaration @@ -758,7 +765,8 @@ AspNetCore refs / Program.cs / Startup.cs / controller-source scan. + multi-declarator + instance fields (signatures `Type name`); `protected internal` (→ protected); property shapes: predefined-type, bare-identifier type (signature loses type), generic type, expression-bodied - (`=>` calls LOST), `{ get; } = new();`, accessor bodies with calls (LOST); + (`=>` calls → the property), `{ get; } = new();` (initializer LOST), + accessor bodies with calls (→ the property); event_field_declaration + event_declaration with add/remove bodies (no nodes; accessor calls → class); operator + conversion operator + indexer + destructor (no nodes; body calls → class); constructor with diff --git a/src/extraction/extraction-version.ts b/src/extraction/extraction-version.ts index 3691e95f0a..e41b5f6149 100644 --- a/src/extraction/extraction-version.ts +++ b/src/extraction/extraction-version.ts @@ -21,4 +21,4 @@ * turns the re-index hint into noise — keep it honest (see CLAUDE.md, "Honesty * in the product is load-bearing"). */ -export const EXTRACTION_VERSION = 27; +export const EXTRACTION_VERSION = 28; diff --git a/src/extraction/languages/vbnet.ts b/src/extraction/languages/vbnet.ts index b2ec4aab50..e8502b2e30 100644 --- a/src/extraction/languages/vbnet.ts +++ b/src/extraction/languages/vbnet.ts @@ -116,8 +116,13 @@ export const vbnetExtractor: LanguageExtractor = { // findable declaration (WinForms/WPF code is built around them). if (node.type === 'event_declaration' || node.type === 'custom_event_declaration') { const nameNode = node.childForFieldName('name'); - if (nameNode) { - ctx.createNode('field', getNodeText(nameNode, ctx.source), node); + const event = nameNode ? ctx.createNode('field', getNodeText(nameNode, ctx.source), node) : null; + // A Custom Event's AddHandler / RemoveHandler / RaiseEvent blocks run + // code, which is the event's. + if (event && node.type === 'custom_event_declaration') { + ctx.pushScope(event.id); + ctx.visitFunctionBody(node, event.id); + ctx.popScope(); } return true; } diff --git a/src/extraction/tree-sitter.ts b/src/extraction/tree-sitter.ts index 5c27523acc..9a10862475 100644 --- a/src/extraction/tree-sitter.ts +++ b/src/extraction/tree-sitter.ts @@ -733,8 +733,10 @@ export class TreeSitterExtractor { * nested function definitions: their bodies are walked — and their * candidates attributed — by extractFunction's own body walk. */ - private scanFnRefSubtree(node: SyntaxNode, depth: number): void { + private scanFnRefSubtree(node: SyntaxNode, depth: number, walked?: ReadonlySet): void { if (!this.fnRefSpec || depth > 12) return; + // Subtrees the body walker has already been through. + if (walked?.has(node.id)) return; const nodeType = node.type; if (depth > 0 && ( this.extractor?.functionTypes.includes(nodeType) || @@ -749,7 +751,7 @@ export class TreeSitterExtractor { this.maybeCaptureFnRefs(node, nodeType); for (let i = 0; i < node.namedChildCount; i++) { const child = node.namedChild(i); - if (child) this.scanFnRefSubtree(child, depth + 1); + if (child) this.scanFnRefSubtree(child, depth + 1, walked); } } @@ -1279,11 +1281,19 @@ export class TreeSitterExtractor { } // Check for class properties (e.g. C# property_declaration) else if (this.extractor.propertyTypes?.includes(nodeType) && this.isInsideClassLikeNode()) { - this.extractProperty(node); - // Property initializers aren't walked — scan for function-as-value - // candidates (#756): Scala `val table = Seq(targetCb)` in an object, - // Kotlin `val cb = ::handler` class properties. - this.scanFnRefSubtree(node, 0); + const propNode = this.extractProperty(node); + // The code a property runs is the property's: its calls, + // instantiations and reads attribute to it, as a method's do. + const bodies = propNode ? this.propertyBodies(node) : []; + if (propNode && bodies.length > 0) { + this.nodeStack.push(propNode.id); + for (const body of bodies) this.visitFunctionBody(body, propNode.id); + this.nodeStack.pop(); + } + // Whatever the body walk didn't cover (a C# `= initializer`, any other + // language's whole declaration) is scanned for function-as-value + // candidates (#756); the bodies captured their own. + this.scanFnRefSubtree(node, 0, new Set(bodies.map((b) => b.id))); skipChildren = true; } // Check for class fields (e.g. Java field_declaration, C# field_declaration) @@ -2113,7 +2123,12 @@ export class TreeSitterExtractor { // (#1093) because the two defaults differ — a bodiless CLASS is kept // unless a language opts into skipping, a bodiless STRUCT is skipped // unless a language opts into keeping. - const body = getChildByField(node, this.extractor.bodyField); + // + // resolveBody first, as for classes and enums: a VB.NET Structure tags + // every member as its own `body` field, so the field alone yields only + // the first member. + const body = this.extractor.resolveBody?.(node, this.extractor.bodyField) + ?? getChildByField(node, this.extractor.bodyField); if (!body && node.type !== 'record_declaration' && !this.extractor.allowBodilessStruct) return; @@ -2304,6 +2319,28 @@ export class TreeSitterExtractor { return propNode; } + /** + * The parts of a property declaration that run code, for the body walker. + * VB.NET writes its `Get` / `Set` blocks, `= initializer` and `As New T` + * as children of the declaration itself, so the declaration is walked + * whole, the way its methods are (resolveBody). C# runs code in each + * accessor's body (`get { … }`, `set => …`) and in an expression-bodied + * property's `=> …`; its `= initializer`, like a field's, stays unwalked. + * Mirrored in the kernel (csharp.rs property_bodies). + */ + private propertyBodies(node: SyntaxNode): SyntaxNode[] { + if (this.language === 'vbnet') return [node]; + if (this.language !== 'csharp') return []; + const bodies: SyntaxNode[] = []; + for (const accessor of getChildByField(node, 'accessors')?.namedChildren ?? []) { + const body = accessor.type === 'accessor_declaration' ? getChildByField(accessor, 'body') : null; + if (body) bodies.push(body); + } + const value = getChildByField(node, 'value'); + if (value?.type === 'arrow_expression_clause') bodies.push(value); + return bodies; + } + /** * Extract a class field declaration (e.g. Java field_declaration, C# field_declaration). * Extracts each declarator as a 'field' kind node inside the owning class. @@ -2408,10 +2445,11 @@ export class TreeSitterExtractor { // candidates, so a lambda / method reference / anonymous class in // `private final Runnable r = () -> target();` contributed NO call // edge at all and `target` looked callerless. Keyed on the `value` - // FIELD, which only Java's `variable_declarator` carries — C#, - // VB.NET and PHP spell their initializer differently and are - // deliberately untouched here. - const valueNode = getChildByField(decl, 'value'); + // FIELD, which only Java's `variable_declarator` carries. VB.NET + // writes `= expr` (the declarator's `initializer`) or `As New T(…)` + // (inside its as_clause), so its whole declarator is walked. C# + // and PHP spell their initializer differently and are untouched. + const valueNode = this.language === 'vbnet' ? decl : getChildByField(decl, 'value'); if (valueNode) { this.nodeStack.push(fieldNode.id); this.visitFunctionBody(valueNode, fieldNode.id); From 511d86e94dc9f26e74e32741477b6ffaae1695a7 Mon Sep 17 00:00:00 2001 From: Colby Mchenry Date: Mon, 5 Oct 2026 14:45:10 +0000 Subject: [PATCH 2/5] fix(mcp): replace a daemon left running by an older install (#2335) (#2343) A daemon keeps running the code it started with. The 1.6.1 daemon in #2335 outlived an upgrade, kept loading grammars from the install the upgrade removed and wrote every file it re-indexed as empty, while every session from the new install served itself in-process, read-only, because the old daemon held the writer lock. Daemons from before this change cannot notice an upgrade, so the launcher acts: - A launcher that finds a daemon of an older plain X.Y.Z release stops it the way `codegraph daemon` does (daemon.pid and the socket hello must agree; SIGTERM, SIGKILL only if it still answers) and starts one from its own install. It finds the daemon by its hello on the launcher's own socket, or by the project lock when it listens elsewhere: on Windows, daemons before #2278 named their pipe after the root as typed. A daemon of the same, a newer, a prerelease or an unknown version is never touched, so two installs cannot take turns stopping each other's. - The writer slot never falls free on the way. The launcher swaps writer.pid to itself (mode `handover`) before the signal, clears the old daemon's leftovers while it holds the slot, and the daemon it spawns takes the slot over (CODEGRAPH_DAEMON_HANDOVER). Sessions of the old daemon that fall back in-process find the slot held and serve reads only, instead of claiming it with the removed install's code. A daemon that does not exit gets the slot back, and a successor outwaits a racing candidate that took daemon.pid first. Validated on Windows with real npm upgrades from the published 1.6.1 and 1.6.2 to this build packed as 1.6.3, with the old session calling throughout. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + __tests__/daemon-older-version.test.ts | 716 +++++++++++++++++++++++++ __tests__/fixtures/older-daemon.cjs | 36 ++ src/mcp/daemon-registry.ts | 173 +++++- src/mcp/daemon.ts | 19 +- src/mcp/index.ts | 144 ++++- src/mcp/project-lifecycle.ts | 3 +- src/mcp/proxy.ts | 24 +- src/mcp/version.ts | 25 +- src/mcp/writer-lock.ts | 38 ++ 10 files changed, 1120 insertions(+), 59 deletions(-) create mode 100644 __tests__/daemon-older-version.test.ts create mode 100644 __tests__/fixtures/older-daemon.cjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 44144e1606..32db6b23ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - In VB.NET, every member of a `Structure` is now indexed, including its fields, properties, methods, constructors and nested enums. Before, only the first member was, so the rest could not be found and their callers looked empty. - In VB.NET and C#, what a property's `Get` and `Set` code calls, creates and reads now belongs to that property, as do C#'s `get => …` accessors and `=> …` property bodies. Before, it was dropped, so a method used only from a property looked unused. - In VB.NET, a field or property initializer like `= Compute()` or `As New List(Of Order)` now links what it calls and creates, and so do a `Custom Event`'s `AddHandler`, `RemoveHandler` and `RaiseEvent` blocks. Re-index VB.NET and C# projects after upgrading. +- Upgrading CodeGraph while an agent session is open no longer leaves the old version's background server in charge of your project: the first session started from the new install stops it and starts a current one in its place, even while sessions opened before the upgrade are still running. That old server could no longer load the language parsers the upgrade removed, so it saved every file it re-indexed with no symbols, while new sessions could only read the index beside it without keeping it up to date. A background server from a newer install is never stopped, and sessions opened before the upgrade keep the old version until you restart them. Thanks @lipchey for the report. (#2335) ## [1.6.2] - 2026-10-03 diff --git a/__tests__/daemon-older-version.test.ts b/__tests__/daemon-older-version.test.ts new file mode 100644 index 0000000000..188cd6cf82 --- /dev/null +++ b/__tests__/daemon-older-version.test.ts @@ -0,0 +1,716 @@ +/** + * A launcher replaces a daemon from an older install (#2335). + * + * A daemon keeps running the code it started with. The 1.6.1 daemon in #2335 + * outlived an upgrade to 1.6.2, went on loading grammars from the install the + * upgrade removed, and wrote every file its watcher re-indexed as empty. A + * launcher that found it could only serve its own session in-process, read-only: + * the old daemon held the project's writer lock and file watcher, so no daemon + * from the new install could start. Daemons from before the fix cannot notice + * the upgrade themselves, so the launcher acts: a daemon of an OLDER release is + * stopped the way `codegraph daemon` stops one (identity-checked SIGTERM, a + * graceful shutdown on POSIX) and one from the launcher's own install takes its + * place. The launcher hears an older daemon's hello on its own socket, or — + * when it listens elsewhere (on Windows, daemons before #2278 named their pipe + * after the root as typed) — finds it by the project lock. A daemon of the + * same, a newer or an unknown version is never touched, so two installed + * versions cannot fight over the daemon. + * + * The end-to-end cases run real daemons of other versions: this build's dist/ + * linked into a temp "install" whose package.json carries another version. A + * daemon resolves its version, and every grammar it loads, from its own files, + * so that install's daemon advertises that version in its lock and its hello. + */ +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import { ChildProcessWithoutNullStreams, execFileSync, spawn } from 'child_process'; +import { once } from 'events'; +import * as fs from 'fs'; +import * as net from 'net'; +import * as os from 'os'; +import * as path from 'path'; +import { CodeGraph } from '../src'; +import { encodeLockInfo, getDaemonPidPath, getDaemonSocketPath } from '../src/mcp/daemon-paths'; +import { isProcessAlive, stopOlderDaemon } from '../src/mcp/daemon-registry'; +import { connectWithHello } from '../src/mcp/proxy'; +import { CodeGraphPackageVersion, isOlderRelease } from '../src/mcp/version'; +import { getWriterPidPath, readWriterLock, swapWriterLock } from '../src/mcp/writer-lock'; +import { WASM_RUNTIME_FLAGS } from '../src/extraction/wasm-runtime-flags'; +import { recordSpawns, removeSpawnLog, settleLosingCandidates } from './daemon-candidates'; + +const BIN = path.resolve(__dirname, '../dist/bin/codegraph.js'); +const DIST = path.resolve(__dirname, '../dist'); +const NODE_MODULES = path.resolve(__dirname, '../node_modules'); +const OLDER_DAEMON = path.resolve(__dirname, 'fixtures/older-daemon.cjs'); + +/** Plain releases always below / above any version this repo will carry. */ +const OLDER = '0.0.1'; +const NEWER = '999.0.0'; + +describe('isOlderRelease', () => { + it('orders plain releases numerically', () => { + expect(isOlderRelease('1.6.1', '1.6.2')).toBe(true); + expect(isOlderRelease('1.6.9', '1.6.10')).toBe(true); + expect(isOlderRelease('1.9.0', '1.10.0')).toBe(true); + expect(isOlderRelease('0.99.99', '1.0.0')).toBe(true); + expect(isOlderRelease('1.6.2', '1.6.2')).toBe(false); + expect(isOlderRelease('1.6.3', '1.6.2')).toBe(false); + expect(isOlderRelease('2.0.0', '1.99.99')).toBe(false); + }); + + it('never calls a prerelease, a build or an unknown version older, in either position', () => { + for (const odd of ['0.0.0-unknown', '0.0.0-mismatch', '1.6.1-beta.1', '1.6.1+local', 'v1.6.1', 'unknown', 'test', '', '1.6']) { + expect(isOlderRelease(odd, '1.6.2'), odd).toBe(false); + expect(isOlderRelease('1.6.1', odd), odd).toBe(false); + } + }); + + it('lets at most one of two versions replace the other', () => { + const versions = ['0.9.0', '1.6.1', '1.6.2', '1.6.10', '1.7.0', '2.0.0', '1.6.2-rc.1', '0.0.0-unknown']; + for (const a of versions) { + for (const b of versions) { + expect(isOlderRelease(a, b) && isOlderRelease(b, a), `${a} / ${b}`).toBe(false); + } + } + }); +}); + +/** A path a test server can listen on: a named pipe on Windows, a socket file elsewhere. */ +function listenPath(dir: string, name: string): string { + return process.platform === 'win32' + ? `\\\\.\\pipe\\${path.basename(dir)}-${name}` + : path.join(dir, `${name}.sock`); +} + +async function listen(server: net.Server, socketPath: string): Promise { + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(socketPath, resolve); + }); +} + +describe('connectWithHello', () => { + let dir: string; + let server: net.Server | null = null; + + beforeEach(() => { dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-hello-')); }); + afterEach(async () => { + if (server) await new Promise((resolve) => server!.close(() => resolve())); + server = null; + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it.each([ + [OLDER, 'older-version', false], + [NEWER, 'version-mismatch', true], + ['0.0.0-unknown', 'version-mismatch', true], + ])('reports a %s daemon as %s', async (version, expected, logged) => { + const socketPath = listenPath(dir, 'daemon'); + server = net.createServer((socket) => { + socket.write(JSON.stringify({ codegraph: version, pid: process.pid, socketPath, protocol: 1 }) + '\n'); + }); + await listen(server, socketPath); + const stderr = vi.spyOn(process.stderr, 'write').mockImplementation(() => true); + try { + expect(await connectWithHello(socketPath, '1.6.2')).toBe(expected); + // An older daemon is the launcher's to report; it is not a mismatch to serve around. + expect(stderr.mock.calls.some(([line]) => String(line).includes('differs from ours'))).toBe(logged); + } finally { + stderr.mockRestore(); + } + }); +}); + +describe('swapWriterLock', () => { + let root: string; + beforeEach(() => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-writer-swap-')); + fs.mkdirSync(path.join(root, '.codegraph')); + }); + afterEach(() => { fs.rmSync(root, { recursive: true, force: true }); }); + + const record = (pid: number, mode: string) => ({ pid, mode, startedAt: 1, ready: false }); + + it('replaces the record of the given pid, and only that record', () => { + const file = getWriterPidPath(root); + fs.writeFileSync(file, JSON.stringify(record(111, 'daemon')) + '\n'); + expect(swapWriterLock(root, 222, record(333, 'handover'))).toBe(false); + expect(readWriterLock(root)).toMatchObject({ pid: 111, mode: 'daemon' }); + expect(swapWriterLock(root, 111, record(333, 'handover'))).toBe(true); + expect(readWriterLock(root)).toMatchObject({ pid: 333, mode: 'handover' }); + // A slot nobody holds is not handed over: it is acquired the usual way. + fs.rmSync(file); + expect(swapWriterLock(root, 333, record(444, 'daemon'))).toBe(false); + expect(fs.existsSync(file)).toBe(false); + expect(fs.readdirSync(path.join(root, '.codegraph'))).toEqual([]); + }); +}); + +/** A live process to stand in for a daemon; signals reach it, never the test. */ +function startDetachedProcess(): number { + const source = [ + "const { spawn } = require('child_process');", + "const child = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { detached: true, stdio: 'ignore' });", + 'child.unref();', + 'console.log(child.pid);', + ].join(' '); + const pid = Number(execFileSync(process.execPath, ['-e', source], { encoding: 'utf8' }).trim()); + if (!Number.isInteger(pid) || pid <= 0) throw new Error('could not start daemon fixture'); + return pid; +} + +async function deadPid(): Promise { + const child = spawn(process.execPath, ['-e', 'process.exit(0)']); + await once(child, 'exit'); + await new Promise((r) => setTimeout(r, 50)); + return child.pid!; +} + +describe('stopOlderDaemon', () => { + let root: string; + let pidPath: string; + let socketPath: string; + let server: net.Server | null; + let pids: number[]; + + beforeEach(() => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-older-daemon-')); + fs.mkdirSync(path.join(root, '.codegraph')); + pidPath = getDaemonPidPath(root); + socketPath = getDaemonSocketPath(root); + server = null; + pids = []; + }); + + afterEach(async () => { + vi.restoreAllMocks(); + for (const pid of pids) if (isProcessAlive(pid)) process.kill(pid, 'SIGKILL'); + if (server) await new Promise((resolve) => server!.close(() => resolve())); + fs.rmSync(root, { recursive: true, force: true }); + }); + + /** A daemon of `version`: its lock, and a socket answering the hello as `helloPid`. */ + async function fakeDaemon(version: string, helloPid?: number): Promise<{ pid: number; lock: string }> { + const pid = startDetachedProcess(); + pids.push(pid); + server = net.createServer((socket) => { + socket.end(JSON.stringify({ protocol: 1, codegraph: version, pid: helloPid ?? pid, socketPath }) + '\n'); + }); + await listen(server, socketPath); + const lock = encodeLockInfo({ pid, version, socketPath, startedAt: Date.now() }); + fs.writeFileSync(pidPath, lock); + return { pid, lock }; + } + + /** Signals sent to `pid`, other than the signal-0 liveness probe. */ + function signalsTo(kill: ReturnType, pid: number): unknown[] { + return kill.mock.calls.filter(([target, signal]) => target === pid && signal !== 0).map(([, signal]) => signal); + } + + /** The writer record, decoded. */ + const writerRecord = (): any => { + try { return JSON.parse(fs.readFileSync(getWriterPidPath(root), 'utf8')); } catch { return null; } + }; + + it('stops a verified daemon of an older release and clears its lock', async () => { + const { pid } = await fakeDaemon('1.6.1'); + const result = await stopOlderDaemon(root, '1.6.2'); + expect(result).toMatchObject({ pid, outcome: 'term', version: '1.6.1' }); + expect(isProcessAlive(pid)).toBe(false); + expect(fs.existsSync(pidPath)).toBe(false); + // Kept for the daemon the caller starts next. + expect(writerRecord()).toMatchObject({ pid: process.pid, mode: 'handover' }); + }, 15000); + + it('holds the writer slot from before the signal, so it is never free or stale', async () => { + const { pid } = await fakeDaemon('1.6.1'); + fs.writeFileSync(getWriterPidPath(root), JSON.stringify({ pid, mode: 'daemon', startedAt: 1, ready: true }) + '\n'); + let atSignal: unknown = null; + const originalKill = process.kill.bind(process); + vi.spyOn(process, 'kill').mockImplementation((target, signal) => { + if (target === pid && signal === 'SIGTERM') atSignal = writerRecord(); + return originalKill(target, signal); + }); + expect(await stopOlderDaemon(root, '1.6.2')).toMatchObject({ pid, outcome: 'term' }); + // A session of the old daemon serving itself the moment it went found the + // slot held, and so served reads only. + expect(atSignal).toMatchObject({ pid: process.pid, mode: 'handover' }); + expect(writerRecord()).toMatchObject({ pid: process.pid, mode: 'handover' }); + expect(fs.existsSync(pidPath)).toBe(false); + }, 15000); + + it('gives the writer slot back to an older daemon that does not exit', async () => { + const { pid, lock } = await fakeDaemon('1.6.1'); + const record = { pid, mode: 'daemon', startedAt: 1, ready: true }; + fs.writeFileSync(getWriterPidPath(root), JSON.stringify(record) + '\n'); + // The signal reaches nothing: the daemon closes its socket, as its own + // shutdown does first, and then never exits. + const originalKill = process.kill.bind(process); + vi.spyOn(process, 'kill').mockImplementation((target, signal) => { + if (target === pid && signal === 'SIGTERM') { server?.close(); return true; } + return originalKill(target, signal); + }); + expect(await stopOlderDaemon(root, '1.6.2', { shutdownGraceMs: 300 })).toMatchObject({ pid, outcome: 'still-running' }); + expect(writerRecord()).toEqual(record); + expect(fs.readFileSync(pidPath, 'utf8')).toBe(lock); + expect(isProcessAlive(pid)).toBe(true); + }, 15000); + + it('leaves the daemon to another launcher already holding the slot to replace it', async () => { + const { pid, lock } = await fakeDaemon('1.6.1'); + const other = startDetachedProcess(); + pids.push(other); + const claim = { pid: other, mode: 'handover', startedAt: 1, ready: false }; + fs.writeFileSync(getWriterPidPath(root), JSON.stringify(claim) + '\n'); + const kill = vi.spyOn(process, 'kill'); + expect(await stopOlderDaemon(root, '1.6.2')).toBeNull(); + expect(signalsTo(kill, pid)).toEqual([]); + expect(writerRecord()).toEqual(claim); + expect(fs.readFileSync(pidPath, 'utf8')).toBe(lock); + }); + + it.each(['1.6.2', '1.6.3', '2.0.0', '1.6.1-beta.1', '0.0.0-unknown', 'test'])( + 'never signals a daemon of version %s', + async (version) => { + const { pid, lock } = await fakeDaemon(version); + const kill = vi.spyOn(process, 'kill'); + expect(await stopOlderDaemon(root, '1.6.2')).toBeNull(); + expect(signalsTo(kill, pid)).toEqual([]); + expect(isProcessAlive(pid)).toBe(true); + expect(fs.readFileSync(pidPath, 'utf8')).toBe(lock); + }, + ); + + it('leaves an older lock alone when its socket answers for another process (#1553)', async () => { + const { pid, lock } = await fakeDaemon('1.6.1', process.pid); + const kill = vi.spyOn(process, 'kill'); + expect(await stopOlderDaemon(root, '1.6.2')).toMatchObject({ pid, outcome: 'unverified' }); + expect(signalsTo(kill, pid)).toEqual([]); + expect(signalsTo(kill, process.pid)).toEqual([]); + expect(isProcessAlive(pid)).toBe(true); + expect(fs.readFileSync(pidPath, 'utf8')).toBe(lock); + }); + + it('reports an older daemon that already exited, leaving its lock to its successor', async () => { + const lock = encodeLockInfo({ pid: await deadPid(), version: '1.6.1', socketPath, startedAt: Date.now() }); + fs.writeFileSync(pidPath, lock); + expect(await stopOlderDaemon(root, '1.6.2')).toMatchObject({ outcome: 'not-running', version: '1.6.1' }); + expect(fs.readFileSync(pidPath, 'utf8')).toBe(lock); + }); + + it('has nothing to stop without a lock, and leaves a listening socket alone', async () => { + server = net.createServer((socket) => { + socket.end(JSON.stringify({ protocol: 1, codegraph: '1.6.1', pid: process.pid, socketPath }) + '\n'); + }); + await listen(server, socketPath); + expect(await stopOlderDaemon(root, '1.6.2')).toBeNull(); + expect(await connectWithHello(socketPath, '1.6.2')).toBe('older-version'); + }); + + it('does not act on an unreadable lock', async () => { + fs.mkdirSync(pidPath); + expect(await stopOlderDaemon(root, '1.6.2')).toEqual({ root, pid: null, outcome: 'unverified' }); + }); +}); + +/** + * A CodeGraph install of `version`: this build's dist/, hard-linked file by + * file (copied where a link is refused), next to a package.json of that version + * and a link to this checkout's node_modules. + */ +function makeInstall(version: string): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), `cg-install-${version}-`)); + const linkTree = (from: string, to: string): void => { + fs.mkdirSync(to, { recursive: true }); + for (const entry of fs.readdirSync(from, { withFileTypes: true })) { + const source = path.join(from, entry.name); + const target = path.join(to, entry.name); + if (entry.isDirectory()) linkTree(source, target); + else { + try { fs.linkSync(source, target); } catch { fs.copyFileSync(source, target); } + } + } + }; + linkTree(DIST, path.join(dir, 'dist')); + fs.symlinkSync(NODE_MODULES, path.join(dir, 'node_modules'), 'junction'); + fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ name: '@colbymchenry/codegraph', version }) + '\n'); + return dir; +} + +interface Session { + child: ChildProcessWithoutNullStreams; + stdout: string[]; + stderr: string[]; +} + +function lines(stream: NodeJS.ReadableStream, into: string[]): void { + let buffer = ''; + stream.on('data', (chunk: Buffer) => { + buffer += chunk.toString('utf8'); + let idx: number; + while ((idx = buffer.indexOf('\n')) !== -1) { + into.push(buffer.slice(0, idx)); + buffer = buffer.slice(idx + 1); + } + }); +} + +function findResponse(stdout: string[], id: number): any | null { + for (const line of stdout) { + try { + const parsed = JSON.parse(line); + if (parsed && parsed.id === id && (parsed.result !== undefined || parsed.error !== undefined)) return parsed; + } catch { /* not JSON */ } + } + return null; +} + +function waitFor(predicate: () => T | undefined | null | false, timeoutMs: number, label: string): Promise { + return new Promise((resolve, reject) => { + const started = Date.now(); + const tick = (): void => { + let value: T | undefined | null | false; + try { value = predicate(); } catch (e) { reject(e); return; } + if (value) { resolve(value as T); return; } + if (Date.now() - started > timeoutMs) { reject(new Error(`Timed out after ${timeoutMs}ms waiting for: ${label}`)); return; } + setTimeout(tick, 25); + }; + tick(); + }); +} + +function isAlive(pid: number): boolean { + try { process.kill(pid, 0); return true; } catch { return false; } +} + +async function waitProcessExit(pid: number, timeoutMs: number): Promise { + return waitFor(() => !isAlive(pid), timeoutMs, `pid ${pid} to exit`).then(() => true, () => false); +} + +function readJson(file: string): any | null { + try { return JSON.parse(fs.readFileSync(file, 'utf8')); } catch { return null; } +} + +describe('a launcher meeting a daemon of another version (#2335)', () => { + let olderInstall: string; + let newerInstall: string; + let tempDir: string; + let realRoot: string; + const sessions: Session[] = []; + const daemonPids = new Set(); + + beforeAll(() => { + olderInstall = makeInstall(OLDER); + newerInstall = makeInstall(NEWER); + }); + + afterAll(async () => { + for (const dir of [olderInstall, newerInstall]) { + if (!dir) continue; + // The link first: removing an install must never reach this checkout's modules. + try { fs.unlinkSync(path.join(dir, 'node_modules')); } catch { /* not created */ } + await fs.promises.rm(dir, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + } + }); + + beforeEach(async () => { + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-older-version-')); + fs.writeFileSync(path.join(tempDir, 'app.ts'), 'export function appMain() { return 1; }\n'); + const cg = await CodeGraph.init(tempDir); + try { await cg.indexAll(); } finally { cg.close(); } + realRoot = fs.realpathSync(tempDir); + }); + + afterEach(async () => { + // Every launcher first (with the runtime flags there is no relaunch child), + // then the losing daemon candidates, then the daemons themselves — and only + // then the fixture: on Windows a live process inside it blocks the removal. + await Promise.all(sessions.map(async ({ child }) => { + if (child.exitCode !== null || child.signalCode !== null) return; + const exited = once(child, 'exit'); + child.kill('SIGKILL'); + await exited; + })); + sessions.length = 0; + const lockPid = (): number | null => readJson(path.join(realRoot, '.codegraph', 'daemon.pid'))?.pid ?? null; + await settleLosingCandidates(tempDir, lockPid); + const holder = lockPid(); + if (holder) daemonPids.add(holder); + for (const pid of daemonPids) { + if (pid === process.pid || !isAlive(pid)) continue; + try { process.kill(pid, 'SIGKILL'); } catch { /* raced to exit */ } + await waitProcessExit(pid, 5000); + } + daemonPids.clear(); + removeSpawnLog(tempDir); + await fs.promises.rm(tempDir, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + }, 45_000); + + /** An agent session: `install`'s launcher, `serve --mcp` in the project. */ + function startSession(install: string | null, env: NodeJS.ProcessEnv = {}): Session { + const bin = install ? path.join(install, 'dist', 'bin', 'codegraph.js') : BIN; + const recorder = recordSpawns(tempDir); + const child = spawn(process.execPath, [...WASM_RUNTIME_FLAGS, ...recorder.args, bin, 'serve', '--mcp'], { + cwd: tempDir, + stdio: ['pipe', 'pipe', 'pipe'], + env: { + ...process.env, + CODEGRAPH_MCP_LOG_ATTACH: '1', + CODEGRAPH_DAEMON_IDLE_TIMEOUT_MS: '30000', + ...recorder.env, + ...env, + }, + }) as ChildProcessWithoutNullStreams; + child.on('error', () => { /* ignore */ }); + child.stdin.on('error', () => { /* ignore */ }); + const session: Session = { child, stdout: [], stderr: [] }; + lines(child.stdout, session.stdout); + lines(child.stderr, session.stderr); + sessions.push(session); + send(session, { + jsonrpc: '2.0', id: 1, method: 'initialize', + params: { protocolVersion: '2024-11-05', capabilities: {}, clientInfo: { name: 'test', version: '0.0.0' }, rootUri: `file://${tempDir}` }, + }); + return session; + } + + function send(session: Session, msg: unknown): void { + try { session.child.stdin.write(JSON.stringify(msg) + '\n'); } catch { /* gone */ } + } + + async function status(session: Session, id: number, label: string): Promise { + send(session, { jsonrpc: '2.0', id, method: 'tools/call', params: { name: 'codegraph_status', arguments: {} } }); + const reply = await waitFor(() => findResponse(session.stdout, id), 20000, label).catch((err: Error) => { + throw new Error(`${err.message}\n--- stderr ---\n${session.stderr.join('\n')}\n--- daemon.log ---\n${daemonLog()}`); + }); + expect(reply.error, label).toBeUndefined(); + expect(reply.result?.isError, label).not.toBe(true); + expect(JSON.stringify(reply.result), label).toContain('CodeGraph Status'); + return reply; + } + + const attachedLine = (session: Session): string | undefined => + session.stderr.find((l) => l.includes('Attached to shared daemon')); + const daemonLock = (): any => readJson(path.join(realRoot, '.codegraph', 'daemon.pid')); + const writerLock = (): any => readJson(path.join(realRoot, '.codegraph', 'writer.pid')); + const daemonLog = (): string => { + try { return fs.readFileSync(path.join(realRoot, '.codegraph', 'daemon.log'), 'utf8'); } catch { return ''; } + }; + + /** A session of `install` attached to a daemon of its own, which owns the project. */ + async function sessionWithDaemon(install: string, version: string, env: NodeJS.ProcessEnv = {}): Promise<{ session: Session; pid: number }> { + const session = startSession(install, env); + await waitFor(() => attachedLine(session), 20000, `the ${version} session to attach`); + // Attached means listening, not initialized: let the engine open the + // database before anything stops this daemon. + await status(session, 2, `a ${version} tool call`); + const lock = daemonLock(); + expect(lock).toMatchObject({ version }); + expect(writerLock()).toMatchObject({ pid: lock.pid, mode: 'daemon' }); + daemonPids.add(lock.pid); + return { session, pid: lock.pid }; + } + + /** A daemon process for the project, started the way a launcher starts one. */ + function startDaemon(env: NodeJS.ProcessEnv): { child: ChildProcessWithoutNullStreams; log: string[] } { + const child = spawn(process.execPath, [...WASM_RUNTIME_FLAGS, BIN, 'serve', '--mcp', '--path', realRoot], { + cwd: tempDir, + stdio: ['pipe', 'pipe', 'pipe'], + env: { ...process.env, CODEGRAPH_DAEMON_INTERNAL: '1', CODEGRAPH_DAEMON_IDLE_TIMEOUT_MS: '30000', ...env }, + }) as ChildProcessWithoutNullStreams; + child.on('error', () => { /* ignore */ }); + child.stdout.resume(); + const log: string[] = []; + lines(child.stderr, log); + daemonPids.add(child.pid!); + return { child, log }; + } + + /** The slot as a launcher that just stopped an older daemon holds it: this test process stands in. */ + function holdWriterSlotForSuccessor(): string { + const claim = JSON.stringify({ pid: process.pid, mode: 'handover', startedAt: Date.now(), ready: false }) + '\n'; + fs.writeFileSync(path.join(realRoot, '.codegraph', 'writer.pid'), claim); + return claim; + } + + it('a daemon takes over the writer slot held for it, and only that one', async () => { + const claim = holdWriterSlotForSuccessor(); + + // A daemon the slot is not held for finds it held, as by any live writer. + const stranger = startDaemon({}); + const [strangerCode] = await once(stranger.child, 'exit'); + expect(strangerCode).toBe(1); + expect(stranger.log.join('\n')).toContain(`writer lock held by PID ${process.pid} (handover mode)`); + expect(fs.readFileSync(path.join(realRoot, '.codegraph', 'writer.pid'), 'utf8')).toBe(claim); + + const successor = startDaemon({ CODEGRAPH_DAEMON_HANDOVER: String(process.pid) }); + await waitFor( + () => daemonLock()?.pid === successor.child.pid && successor.log.some((l) => l.includes('Listening on')), + 20000, 'the successor to listen', + ); + expect(writerLock()).toMatchObject({ pid: successor.child.pid, mode: 'daemon' }); + expect(successor.log.join('\n')).toContain(`Took over the writer lock from launcher pid ${process.pid}`); + }, 60000); + + it('a daemon the slot is held for outwaits a candidate still starting in its way', async () => { + holdWriterSlotForSuccessor(); + // A racing launcher's candidate holds the daemon lock, still starting; it + // cannot get the writer slot, so it will give up. + const candidate = startDetachedProcess(); + daemonPids.add(candidate); + const pidPath = path.join(realRoot, '.codegraph', 'daemon.pid'); + const candidateLock = encodeLockInfo({ + pid: candidate, version: CodeGraphPackageVersion, socketPath: getDaemonSocketPath(realRoot), startedAt: Date.now(), + }); + fs.writeFileSync(pidPath, candidateLock); + + // Any other daemon yields to it at once. + const yielding = startDaemon({}); + const [yieldingCode] = await once(yielding.child, 'exit'); + expect(yieldingCode).toBe(0); + expect(yielding.log.join('\n')).toContain(`Another daemon (pid ${candidate}) already holds the lock`); + + const successor = startDaemon({ CODEGRAPH_DAEMON_HANDOVER: String(process.pid) }); + await waitFor( + () => successor.log.some((l) => l.includes(`Waiting for daemon candidate pid ${candidate}`)), + 20000, 'the successor to wait for the candidate', + ); + expect(successor.child.exitCode).toBeNull(); + expect(fs.readFileSync(pidPath, 'utf8')).toBe(candidateLock); + // The candidate gives up: its lock goes, then so does it. + fs.rmSync(pidPath); + process.kill(candidate, 'SIGKILL'); + await waitFor( + () => daemonLock()?.pid === successor.child.pid && successor.log.some((l) => l.includes('Listening on')), + 20000, 'the successor to listen', + ); + expect(writerLock()).toMatchObject({ pid: successor.child.pid, mode: 'daemon' }); + }, 60000); + + it('keeps the older daemon\'s sessions read-only through the replacement, even mid-call', async () => { + const before = await sessionWithDaemon(olderInstall, OLDER); + // The older session calls every 20ms throughout, so it serves itself + // in-process the instant its daemon goes. Finding the writer slot free + // then, it would claim it — with the code of the install being replaced. + let next = 100; + const calling = setInterval(() => { + send(before.session, { jsonrpc: '2.0', id: next++, method: 'tools/call', params: { name: 'codegraph_status', arguments: {} } }); + }, 20); + let after: Session; + try { + after = startSession(null); + await waitFor(() => attachedLine(after), 30000, 'the new session to attach'); + await new Promise((r) => setTimeout(r, 500)); + } finally { + clearInterval(calling); + } + const lock = daemonLock(); + daemonPids.add(lock.pid); + expect(lock.version).toBe(CodeGraphPackageVersion); + expect(writerLock()).toMatchObject({ pid: lock.pid, mode: 'daemon' }); + await status(after, 2, 'a tool call through the new daemon'); + + const last = next - 1; + await waitFor(() => findResponse(before.session.stdout, last), 20000, 'the older session to answer every call'); + for (let id = 100; id <= last; id++) expect(findResponse(before.session.stdout, id), `call ${id}`).toBeTruthy(); + const stderr = before.session.stderr.join('\n'); + expect(stderr).toContain('Serving reads in-process without auto-sync'); + expect(stderr).not.toContain('File watcher active'); + expect(writerLock()).toMatchObject({ pid: lock.pid, mode: 'daemon' }); + }, 120000); + + it('stops a daemon from an older install and starts one from its own', async () => { + const before = await sessionWithDaemon(olderInstall, OLDER); + + const after = startSession(null); + const stopped = await waitFor( + () => after.stderr.find((l) => l.includes('[CodeGraph MCP] Stopped')), + 30000, 'the older daemon to be stopped', + ); + expect(stopped).toContain(`the CodeGraph ${OLDER} daemon (pid ${before.pid})`); + expect(stopped).toContain(`from this install (${CodeGraphPackageVersion})`); + expect(await waitProcessExit(before.pid, 10000)).toBe(true); + + const attached = await waitFor(() => attachedLine(after), 20000, 'the new session to attach'); + const lock = daemonLock(); + daemonPids.add(lock.pid); + expect(lock.version).toBe(CodeGraphPackageVersion); + expect(lock.pid).not.toBe(before.pid); + expect(attached).toContain(`(pid ${lock.pid}, v${CodeGraphPackageVersion})`); + await status(after, 2, 'a tool call through the new daemon'); + // The replacement owns updates for the project: writer lock and watcher, + // handed to it by the launcher that held the slot through the stop. + expect(writerLock()).toMatchObject({ pid: lock.pid, mode: 'daemon' }); + expect(daemonLog()).toContain(`Took over the writer lock from launcher pid ${after.child.pid}`); + if (process.platform !== 'win32') { + // SIGTERM ran the old daemon's own shutdown. (On Windows it is + // TerminateProcess, and the stop clears what is left.) + expect(daemonLog()).toContain(`Shutting down (SIGTERM`); + } + + // The session from before the upgrade keeps answering, read-only, and + // leaves the new daemon its writer lock. + await waitFor( + () => before.session.stderr.some((l) => l.includes('Shared daemon connection lost')), + 10000, 'the older session to lose its daemon', + ); + await status(before.session, 3, 'a tool call in the older session'); + expect(before.session.stderr.some((l) => l.includes('Serving reads in-process without auto-sync'))).toBe(true); + expect(writerLock()).toMatchObject({ pid: lock.pid, mode: 'daemon' }); + expect(daemonLock()).toEqual(lock); + expect(isAlive(lock.pid)).toBe(true); + }, 120000); + + it('finds an older daemon its probe cannot reach by the project lock, and stops it', async () => { + // On Windows a 1.6.1 daemon named its pipe after the root as typed (#2278), + // so a newer launcher's probe never hears its hello; only its lock says + // where it listens. Stand one up on a socket no launcher probes. + const socketDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-sock-')); + try { + const socketPath = listenPath(socketDir, 'older'); + const older = spawn(process.execPath, [OLDER_DAEMON, realRoot, OLDER, socketPath], { detached: true, stdio: 'ignore' }); + older.unref(); + daemonPids.add(older.pid!); + await waitFor(() => daemonLock()?.pid === older.pid, 10000, 'the older daemon to hold the project'); + expect(writerLock()).toMatchObject({ pid: older.pid, mode: 'daemon' }); + + const session = startSession(null); + const stopped = await waitFor( + () => session.stderr.find((l) => l.includes('[CodeGraph MCP] Stopped')), + 30000, 'the older daemon to be stopped', + ); + expect(stopped).toContain(`the CodeGraph ${OLDER} daemon (pid ${older.pid})`); + expect(await waitProcessExit(older.pid!, 10000)).toBe(true); + const attached = await waitFor(() => attachedLine(session), 20000, 'the session to attach'); + const lock = daemonLock(); + daemonPids.add(lock.pid); + expect(lock.version).toBe(CodeGraphPackageVersion); + expect(attached).toContain(`(pid ${lock.pid}, v${CodeGraphPackageVersion})`); + expect(writerLock()).toMatchObject({ pid: lock.pid, mode: 'daemon' }); + expect(daemonLog()).toContain(`Took over the writer lock from launcher pid ${session.child.pid}`); + await status(session, 2, 'a tool call through the new daemon'); + } finally { + fs.rmSync(socketDir, { recursive: true, force: true }); + } + }, 90000); + + it('never stops a daemon from a newer install', async () => { + const retry = { CODEGRAPH_DAEMON_RETRY_MS: '300', CODEGRAPH_DAEMON_RETRY_MAX_MS: '600' }; + const newer = await sessionWithDaemon(newerInstall, NEWER); + const lock = daemonLock(); + + const current = startSession(null, retry); + // The first probe and the retries that follow all find the newer daemon. + await waitFor( + () => current.stderr.filter((l) => l.includes(`version (${NEWER}) differs from ours`)).length >= 3, + 20000, 'repeated probes of the newer daemon', + ); + await status(current, 2, 'a tool call served in-process'); + expect(current.stderr.some((l) => l.includes('Serving reads in-process without auto-sync'))).toBe(true); + expect(current.stderr.some((l) => l.includes('[CodeGraph MCP] Stopped'))).toBe(false); + + expect(isAlive(newer.pid)).toBe(true); + expect(daemonLock()).toEqual(lock); + expect(writerLock()).toMatchObject({ pid: newer.pid, mode: 'daemon' }); + await status(newer.session, 3, 'a tool call through the newer daemon'); + }, 90000); +}); diff --git a/__tests__/fixtures/older-daemon.cjs b/__tests__/fixtures/older-daemon.cjs new file mode 100644 index 0000000000..a6535f4d9b --- /dev/null +++ b/__tests__/fixtures/older-daemon.cjs @@ -0,0 +1,36 @@ +/** + * A daemon of an older release, listening where a current launcher does not + * look (#2335). On Windows, daemons before #2278 named their pipe after the + * project root as typed, so a newer launcher's probe never reaches them and + * only the project lock says where they are. Like a real daemon it holds the + * project's daemon and writer locks and answers the hello; on SIGTERM it lets + * go of whichever of the two locks still names it and exits, the shape of a + * real daemon's shutdown. (On Windows SIGTERM is TerminateProcess, and the + * stop clears what is left.) + * + * node older-daemon.cjs + */ +const fs = require('fs'); +const net = require('net'); +const path = require('path'); + +const [root, version, socketPath] = process.argv.slice(2); +const dir = path.join(root, '.codegraph'); +const writeLock = (name, record) => fs.writeFileSync(path.join(dir, name), JSON.stringify(record) + '\n'); + +const server = net.createServer((socket) => { + socket.end(JSON.stringify({ codegraph: version, pid: process.pid, socketPath, protocol: 1 }) + '\n'); +}); +server.listen(socketPath, () => { + writeLock('writer.pid', { pid: process.pid, mode: 'daemon', startedAt: Date.now(), ready: true }); + writeLock('daemon.pid', { pid: process.pid, version, socketPath, startedAt: Date.now() }); +}); +process.on('SIGTERM', () => { + for (const name of ['writer.pid', 'daemon.pid']) { + const file = path.join(dir, name); + try { + if (JSON.parse(fs.readFileSync(file, 'utf8')).pid === process.pid) fs.unlinkSync(file); + } catch { /* already gone */ } + } + server.close(() => process.exit(0)); +}); diff --git a/src/mcp/daemon-registry.ts b/src/mcp/daemon-registry.ts index aabb79139b..a9e5860116 100644 --- a/src/mcp/daemon-registry.ts +++ b/src/mcp/daemon-registry.ts @@ -31,7 +31,14 @@ import { probeDaemonIdentity, type DaemonLockInfo, } from './daemon-paths'; -import { readWriterLock, releaseWriterLock, tryAcquireWriterLock } from './writer-lock'; +import { + readWriterLock, + releaseWriterLock, + swapWriterLock, + tryAcquireWriterLock, + type WriterLockInfo, +} from './writer-lock'; +import { isOlderRelease } from './version'; import { WORKER_START_SETTLE_MS } from '../worker-teardown'; export interface DaemonRecord { @@ -155,7 +162,6 @@ function cleanupDaemonArtifacts( root: string, expectedLockContents: string | null, ): boolean { - const pidPath = getDaemonPidPath(root); // A daemon owns writer.pid before binding or relocating its socket. Claiming // the writer slot therefore freezes every legitimate daemon artifact writer // while we compare the inspected lock snapshot and clean it up. @@ -164,33 +170,39 @@ function cleanupDaemonArtifacts( if (claim.kind === 'taken') return false; try { - if (expectedLockContents === null) { - if (fs.existsSync(pidPath)) return false; - } else { - try { - if (fs.readFileSync(pidPath, 'utf8') !== expectedLockContents) return false; - } catch { - return false; - } - } - // POSIX sockets are real files; Windows named pipes vanish with the process. - // Sweep every candidate before releasing daemon.pid, so no successor can - // acquire the lock and bind a socket that this cleanup then removes. - if (process.platform !== 'win32') { - for (const candidate of getDaemonSocketCandidates(root)) { - try { fs.unlinkSync(candidate); } catch { /* gone */ } - } - } - deregisterDaemon(root); - try { fs.unlinkSync(pidPath); } catch (err) { - if ((err as NodeJS.ErrnoException).code !== 'ENOENT') return false; - } - return true; + return removeDaemonArtifacts(root, expectedLockContents); } finally { releaseWriterLock(root); } } +/** The removal behind {@link cleanupDaemonArtifacts}, for a caller already holding the writer slot. */ +function removeDaemonArtifacts(root: string, expectedLockContents: string | null): boolean { + const pidPath = getDaemonPidPath(root); + if (expectedLockContents === null) { + if (fs.existsSync(pidPath)) return false; + } else { + try { + if (fs.readFileSync(pidPath, 'utf8') !== expectedLockContents) return false; + } catch { + return false; + } + } + // POSIX sockets are real files; Windows named pipes vanish with the process. + // Sweep every candidate before releasing daemon.pid, so no successor can + // acquire the lock and bind a socket that this cleanup then removes. + if (process.platform !== 'win32') { + for (const candidate of getDaemonSocketCandidates(root)) { + try { fs.unlinkSync(candidate); } catch { /* gone */ } + } + } + deregisterDaemon(root); + try { fs.unlinkSync(pidPath); } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') return false; + } + return true; +} + /** Remove daemon artifacts only when no matching daemon answers the socket hello. */ export async function clearStaleDaemonArtifacts(root: string): Promise { const pidPath = getDaemonPidPath(root); @@ -240,6 +252,8 @@ export interface StopResult { pid: number | null; /** 'term' graceful, 'kill' force, 'still-running' refused, 'not-running' stale, 'no-daemon' absent, 'unverified' preserved. */ outcome: 'term' | 'kill' | 'still-running' | 'not-running' | 'no-daemon' | 'unverified'; + /** The daemon's version as its lock recorded it, when the stop got that far. */ + version?: string; } /** @@ -292,13 +306,27 @@ export async function stopDaemonAt( const removed = cleanupDaemonArtifacts(root, lockContents); return { root, pid, outcome: removed ? 'not-running' : 'unverified' }; } + return stopVerifiedDaemon(root, identity, lockContents, options.shutdownGraceMs); +} +/** + * The signalling half of a stop, for a daemon whose socket hello just proved + * `identity` against the lock read as `lockContents`: SIGTERM, wait, SIGKILL + * only one that still answers, then sweep its artifacts. + */ +async function stopVerifiedDaemon( + root: string, + identity: DaemonLockInfo, + lockContents: string | null, + shutdownGraceMs = DAEMON_SHUTDOWN_GRACE_MS, +): Promise { + const { pid, version } = identity; // Identity probing awaits I/O: never act on a superseded ownership record. const sameLock = (): boolean => { try { return fs.readFileSync(getDaemonPidPath(root), 'utf8') === lockContents; } catch { return lockContents === null; } }; - if (!sameLock()) return { root, pid, outcome: 'unverified' }; + if (!sameLock()) return { root, pid, outcome: 'unverified', version }; // POSIX: SIGTERM runs the daemon's graceful shutdown. Windows: TerminateProcess // (no graceful path), so we always sweep artifacts ourselves below. @@ -309,7 +337,7 @@ export async function stopDaemonAt( if (sameLock() && await probeDaemonIdentity(identity) && sameLock()) { try { process.kill(pid, 'SIGKILL'); } catch { /* raced to exit */ } if (!(await waitForDeath(pid, 2000))) { - return { root, pid, outcome: 'still-running' }; + return { root, pid, outcome: 'still-running', version }; } outcome = 'kill'; } else { @@ -321,14 +349,101 @@ export async function stopDaemonAt( // Its identity can't be re-proven without the socket, so signal nothing // more; just wait, bounded, for the PID to go. One that outlives the // wait is reported, as before. - if (!(await waitForDeath(pid, options.shutdownGraceMs ?? DAEMON_SHUTDOWN_GRACE_MS))) { - return { root, pid, outcome: 'still-running' }; + if (!(await waitForDeath(pid, shutdownGraceMs))) { + return { root, pid, outcome: 'still-running', version }; } } } // Compares the lock with the one we signalled, so a successor's is kept. cleanupDaemonArtifacts(root, lockContents); - return { root, pid, outcome }; + return { root, pid, outcome, version }; +} + +/** + * Stop the daemon serving `root` when it runs an older CodeGraph release than + * `version`, so the caller can start one from its own install (#2335). A + * daemon keeps running the code it started with: one that outlived an upgrade + * goes on loading grammars from files the upgrade removed, while it holds the + * project's writer lock and file watcher. Only a daemon whose lock records an + * older release ({@link isOlderRelease}) and whose socket hello confirms that + * lock is signalled, exactly as {@link stopDaemonAt} would; one of the same, a + * newer or an unknown version is never touched, so two installs cannot take + * turns stopping each other's daemon. + * + * The project's writer slot never falls free on the way: before the signal, + * this process takes it over from the old daemon (mode `handover`), and the + * caller hands it on to the daemon it starts, which takes it over in turn + * ({@link swapWriterLock}). The old daemon's sessions serve themselves + * in-process the moment it goes, and one that found the slot free would claim + * it as their writer — with the code of an install the upgrade removed — and + * keep the new daemon from starting. When the old daemon is not stopped, the + * slot goes back to it. + * + * Resolves null when the lock names no older daemon (there is none, or another + * launcher already replaced it) or the slot could not be taken (another + * launcher holds it to replace the same daemon). Otherwise says what became of + * the daemon: `term` or `kill` when it was stopped, and this process now holds + * the slot for its successor (release it with {@link releaseWriterLock} once + * that one has taken over or failed); `not-running` when it had already exited + * (its successor clears the stale lock); `unverified` or `still-running` when + * it is still there. + */ +export async function stopOlderDaemon( + root: string, + version: string, + options: { shutdownGraceMs?: number } = {}, +): Promise { + let lockContents: string; + try { + lockContents = fs.readFileSync(getDaemonPidPath(root), 'utf8'); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') return null; + return { root, pid: null, outcome: 'unverified' }; + } + const identity = decodeLockInfo(lockContents); + if (!identity) return { root, pid: null, outcome: 'unverified' }; + if (!isOlderRelease(identity.version, version)) return null; + const { pid } = identity; + if (!isProcessAlive(pid)) return { root, pid, outcome: 'not-running', version: identity.version }; + // The hello must name this pid and version (#1553): a reused pid is no daemon. + if (!canProbeDaemonIdentity(identity) || !await probeDaemonIdentity(identity)) { + return { root, pid, outcome: 'unverified', version: identity.version }; + } + const slot = takeWriterSlotFrom(root, pid); + if (!slot) return null; + const result = await stopVerifiedDaemon(root, identity, lockContents, options.shutdownGraceMs); + if (result.outcome === 'term' || result.outcome === 'kill') { + // The stop's own sweep needs the slot this process now holds: clear what + // the old daemon left (all of it on Windows, where the stop is + // TerminateProcess), so its successor starts on a clean lock. + removeDaemonArtifacts(root, lockContents); + } else { + slot.giveBack(); + } + return result; +} + +/** + * Take the project's writer slot from the daemon `pid` this process is about + * to stop (see {@link stopOlderDaemon}), whether the record is that daemon's, + * stale, or absent. Returns how to give it back to that daemon, or null when + * the slot could not be taken: another live process holds it (another + * launcher replacing the same daemon), or the swap lost a race. + */ +function takeWriterSlotFrom(root: string, pid: number): { giveBack(): void } | null { + const previous = readWriterLock(root); + if (previous && previous.pid !== pid && previous.pid !== process.pid && isProcessAlive(previous.pid)) return null; + const claim: WriterLockInfo = { pid: process.pid, mode: 'handover', startedAt: Date.now(), ready: false }; + const held = previous + ? swapWriterLock(root, previous.pid, claim) + : tryAcquireWriterLock(root, 'handover').kind === 'acquired'; + if (!held) return null; + return { + giveBack: () => { + if (previous?.pid === pid) swapWriterLock(root, process.pid, previous); + else releaseWriterLock(root); + }, + }; } /** Stop every registered, live daemon. */ diff --git a/src/mcp/daemon.ts b/src/mcp/daemon.ts index 5f44ff6e85..73a4969598 100644 --- a/src/mcp/daemon.ts +++ b/src/mcp/daemon.ts @@ -57,9 +57,11 @@ import { import { CodeGraphPackageVersion } from './version'; import { releaseWriterLock, + swapWriterLock, tryAcquireWriterLock, assertNoRebuild, writerLockHeldMessage, + type WriterLockInfo, } from './writer-lock'; import { registerDaemon, deregisterDaemon } from './daemon-registry'; @@ -187,15 +189,18 @@ export class Daemon { private stopping = false; private socketPath: string; private pidPath: string; + /** The launcher holding the writer slot for this daemon, when it replaced an older one (#2335). */ + private handoverFrom: number | null; constructor( private projectRoot: string, - opts: { idleTimeoutMs?: number; maxIdleMs?: number } = {}, + opts: { idleTimeoutMs?: number; maxIdleMs?: number; handoverFrom?: number | null } = {}, ) { this.socketPath = getDaemonSocketPath(projectRoot); this.pidPath = getDaemonPidPath(projectRoot); this.idleTimeoutMs = opts.idleTimeoutMs ?? resolveIdleTimeoutMs(); this.maxIdleMs = opts.maxIdleMs ?? resolveMaxIdleMs(); + this.handoverFrom = opts.handoverFrom ?? null; // Daemon mode serves many concurrent clients on one event loop, so off-load // read-tool dispatch to a worker pool — otherwise concurrent explores // serialize and starve the MCP transport (clients time out). Direct mode @@ -214,7 +219,17 @@ export class Daemon { // #1740: claim the project writer lock before opening/watching so a // concurrent direct-mode serve --mcp cannot start a second watcher. assertNoRebuild(this.projectRoot); - const writer = tryAcquireWriterLock(this.projectRoot, 'daemon'); + let writer = tryAcquireWriterLock(this.projectRoot, 'daemon'); + // The launcher that stopped an older daemon has held the slot for us since + // (#2335); take it over without letting it fall free. + if (writer.kind === 'taken' && this.handoverFrom !== null && + writer.existing?.pid === this.handoverFrom && writer.existing.mode === 'handover') { + const info: WriterLockInfo = { pid: process.pid, mode: 'daemon', startedAt: Date.now(), ready: false }; + if (swapWriterLock(this.projectRoot, this.handoverFrom, info)) { + writer = { kind: 'acquired', pidPath: writer.pidPath, info }; + process.stderr.write(`[CodeGraph daemon] Took over the writer lock from launcher pid ${this.handoverFrom}, which stopped an older daemon.\n`); + } + } if (writer.kind === 'taken') { const msg = writerLockHeldMessage(writer.existing, writer.pidPath); process.stderr.write(`[CodeGraph daemon] ${msg}\n`); diff --git a/src/mcp/index.ts b/src/mcp/index.ts index 824132d629..ac7aa1d0ad 100644 --- a/src/mcp/index.ts +++ b/src/mcp/index.ts @@ -47,8 +47,9 @@ import { isProcessAlive, tryAcquireDaemonLock, } from './daemon'; -import { clearStaleDaemonArtifacts } from './daemon-registry'; +import { clearStaleDaemonArtifacts, stopOlderDaemon } from './daemon-registry'; import { connectWithHello, runLocalHandshakeProxy } from './proxy'; +import { CodeGraphPackageVersion } from './version'; import { readWriterLock, assertNoRebuild, @@ -83,6 +84,13 @@ const DIRECT_QUERY_POOL_MAX = 2; */ const DAEMON_INTERNAL_ENV = 'CODEGRAPH_DAEMON_INTERNAL'; +/** + * Env var naming the launcher that holds the project's writer slot for the + * daemon it spawns, after stopping an older one (#2335). That daemon takes the + * slot over instead of finding it held. + */ +const DAEMON_HANDOVER_ENV = 'CODEGRAPH_DAEMON_HANDOVER'; + /** * Retries for the detached daemon arbitrating the O_EXCL lock against a racing * sibling. Tiny — the lock resolves on the first round in practice; the retries @@ -91,6 +99,13 @@ const DAEMON_INTERNAL_ENV = 'CODEGRAPH_DAEMON_INTERNAL'; const TAKEOVER_MAX_RETRIES = 5; const TAKEOVER_RETRY_DELAY_MS = 100; +/** + * How long a daemon the writer slot is held for (#2335) waits out a racing + * candidate that took the daemon lock first. That candidate cannot get the + * slot, so it leaves; this bounds a wait that never ends. + */ +const HANDOVER_CANDIDATE_WAIT_MS = 5_000; + /** * A fallback that serves reads without a watcher or a writer lock (#1963). * Say so on stderr: otherwise a session that quietly stopped syncing looks @@ -243,9 +258,10 @@ function resolveDaemonRoot(explicitPath: string | null): string | null { * `process.execArgv` (carries `--liftoff-only`, so the daemon never re-execs) * and `process.argv[1]` (this script). The spawned process self-arbitrates the * O_EXCL lock, so racing launchers may each spawn one — losers exit and every - * launcher proxies through the single winner. + * launcher proxies through the single winner. `handover` tells the daemon this + * launcher holds the writer slot for it (see {@link DAEMON_HANDOVER_ENV}). */ -function spawnDetachedDaemon(root: string): void { +function spawnDetachedDaemon(root: string, handover = false): void { const scriptPath = process.argv[1]; if (!scriptPath) { // No resolvable CLI entry point to re-invoke — let the caller fall back to @@ -267,6 +283,8 @@ function spawnDetachedDaemon(root: string): void { // where a long-dead session's host pid would trigger spurious shutdowns. const env: NodeJS.ProcessEnv = { ...process.env, [DAEMON_INTERNAL_ENV]: '1' }; delete env[HOST_PPID_ENV]; + if (handover) env[DAEMON_HANDOVER_ENV] = String(process.pid); + else delete env[DAEMON_HANDOVER_ENV]; const child = spawn( process.execPath, [...process.execArgv, scriptPath, 'serve', '--mcp', '--path', root], @@ -286,6 +304,76 @@ function spawnDetachedDaemon(root: string): void { } } +/** + * How long a launcher replacing an older daemon waits for one that is partway + * through its shutdown, past the stop's own 3s wait. Tool calls are held + * meanwhile, so it is short: a daemon that takes longer leaves the session + * served in-process until the proxy's next retry (#2277) finds it gone. + */ +const OLDER_DAEMON_SHUTDOWN_GRACE_MS = 5_000; + +/** + * Stop the daemon serving `root` from an older install so the caller can start + * one from this install (#2335). A daemon keeps the code it started with: one + * that outlived an upgrade went on loading grammars from the removed install + * and wrote every file it re-indexed as empty, and as the project's writer it + * kept every newer session from syncing. {@link stopOlderDaemon} finds it by + * the project lock, verifies it, and never stops one of the same, a newer or an + * unknown version. `answered` says an older daemon answered this launcher's + * own probe. Resolves `stopped` (this launcher now holds the project's writer + * slot for the daemon it starts next), `none` (no older daemon holds the + * project), or `kept` (one does, unverified or still shutting down). + */ +async function replaceOlderDaemon(root: string, answered: boolean): Promise<'stopped' | 'none' | 'kept'> { + const result = await stopOlderDaemon(root, CodeGraphPackageVersion, { + shutdownGraceMs: OLDER_DAEMON_SHUTDOWN_GRACE_MS, + }); + // No older daemon in the lock (none, or already replaced), or it had exited. + if (!result || result.outcome === 'not-running') return 'none'; + const daemon = result.version + ? `the CodeGraph ${result.version} daemon (pid ${result.pid})` + : 'an older CodeGraph daemon'; + if (result.outcome === 'term' || result.outcome === 'kill') { + process.stderr.write( + `[CodeGraph MCP] Stopped ${daemon} serving this project; starting one from this install (${CodeGraphPackageVersion}).\n` + ); + return 'stopped'; + } + if (result.outcome === 'still-running') { + process.stderr.write(`[CodeGraph MCP] Asked ${daemon} serving this project to stop, but it has not exited yet.\n`); + } else if (answered) { + process.stderr.write( + `[CodeGraph MCP] Found ${daemon} serving this project but could not verify it, so left it running; serving this session in-process.\n` + ); + } + return 'kept'; +} + +/** + * Wait out the daemon candidate `pid` holding the daemon lock while launcher + * `handoverFrom` holds the writer slot for this daemon (#2335): the candidate + * cannot get the slot, so it releases the lock and exits. Resolves true once + * `pid` no longer holds the lock; false at once when no slot is held for this + * daemon, and false when that stops being so or the wait runs out. + */ +async function outwaitCandidate(root: string, handoverFrom: number | null, pid: number): Promise { + const heldForUs = (): boolean => { + const writer = readWriterLock(root); + return handoverFrom !== null && writer?.pid === handoverFrom && writer.mode === 'handover'; + }; + if (!heldForUs()) return false; + process.stderr.write(`[CodeGraph daemon] Waiting for daemon candidate pid ${pid} to give up the lock; the writer lock is held for this daemon.\n`); + const deadline = Date.now() + HANDOVER_CANDIDATE_WAIT_MS; + while (Date.now() < deadline) { + if (!heldForUs()) return false; + let holder: number | undefined; + try { holder = decodeLockInfo(fs.readFileSync(getDaemonPidPath(root), 'utf8'))?.pid; } catch { holder = undefined; } + if (holder !== pid || !isProcessAlive(pid)) return true; + await sleep(25); + } + return false; +} + /** * MCP Server for CodeGraph * @@ -495,11 +583,15 @@ export class MCPServer { // kills/restarts can be placed in time (#1431 — the log was undatable). timestampStderrLines(); const root = resolveDaemonRoot(this.projectPath) ?? this.projectPath ?? process.cwd(); + // Read once and dropped, so nothing this daemon spawns inherits it. + const handoverPid = Number(process.env[DAEMON_HANDOVER_ENV]); + const handoverFrom = Number.isInteger(handoverPid) && handoverPid > 0 ? handoverPid : null; + delete process.env[DAEMON_HANDOVER_ENV]; for (let attempt = 0; attempt < TAKEOVER_MAX_RETRIES; attempt++) { const lock = tryAcquireDaemonLock(root); if (lock.kind === 'acquired') { - const daemon = new Daemon(root); + const daemon = new Daemon(root, { handoverFrom }); await daemon.start(); this.daemon = daemon; this.mode = 'daemon'; @@ -529,6 +621,9 @@ export class MCPServer { stillStarting || await probeDaemonIdentity(existing) ) { + // A daemon the writer slot is held for outwaits a candidate still + // starting in its way, which cannot get the slot and leaves (#2335). + if (stillStarting && await outwaitCandidate(root, handoverFrom, existing.pid)) continue; process.stderr.write( `[CodeGraph daemon] Another daemon (pid ${existing.pid}) already holds the lock; exiting.\n` ); @@ -560,7 +655,8 @@ export class MCPServer { /** * Proxy mode (the common case). Serve the MCP handshake LOCALLY for instant * tool registration, forwarding tool calls to the shared daemon — which is - * connected in the background (probed, then spawned + polled if absent) so the + * connected in the background (probed, then spawned + polled if absent; one of + * an older release is stopped first, #2335) so the * handshake never waits ~600ms on it. Runs until the host disconnects; the * proxy falls back to an in-process engine if the daemon never binds, so this * never wedges a session. @@ -577,9 +673,8 @@ export class MCPServer { for (const candidate of candidates) { const s = await connectWithHello(candidate); // A wrong-version daemon IS up — definitive; propagate so the caller - // serves in-process instead of spawning + polling for 6s. Don't keep - // probing fallbacks past it. - if (s === 'version-mismatch') return s; + // replaces it (an older release) or serves in-process instead of + // spawning + polling for 6s. Don't keep probing fallbacks past it. if (s) return s; } return null; @@ -588,16 +683,31 @@ export class MCPServer { // Fast path: a daemon may already be listening (on either candidate). const probe = await connectAnyCandidate(); if (probe === 'version-mismatch') return null; // definitive — serve in-process, don't poll for 6s - if (probe) return probe; - // None reachable — spawn one (detached) and poll for its bind. - spawnDetachedDaemon(root); - for (let attempt = 0; attempt < DAEMON_CONNECT_MAX_RETRIES; attempt++) { - await sleep(DAEMON_CONNECT_RETRY_DELAY_MS); - const s = await connectAnyCandidate(); - if (s === 'version-mismatch') return null; - if (s) return s; + if (probe && probe !== 'older-version') return probe; + // A daemon of an older release answered — or none did, while the project + // lock may still name one on a socket we don't probe (on Windows, daemons + // before #2278 named their pipe after the root as typed). Either way it + // makes way for one from this install (#2335). + const answered = probe === 'older-version'; + const older = await replaceOlderDaemon(root, answered); + if (older === 'kept' && answered) return null; + // None reachable — spawn one (detached) and poll for its bind. After a + // replacement this launcher holds the writer slot for it meanwhile. + try { + spawnDetachedDaemon(root, older === 'stopped'); + for (let attempt = 0; attempt < DAEMON_CONNECT_MAX_RETRIES; attempt++) { + await sleep(DAEMON_CONNECT_RETRY_DELAY_MS); + const s = await connectAnyCandidate(); + // Another version answering now, older or not, waits for the next + // retry: one replacement per attempt, so this can never loop. + if (s === 'older-version' || s === 'version-mismatch') return null; + if (s) return s; + } + return null; // never bound — the proxy serves this session in-process + } finally { + // The new daemon has taken the slot over, or never came; a no-op unless still ours. + if (older === 'stopped') releaseWriterLock(root); } - return null; // never bound — the proxy serves this session in-process }; await runLocalHandshakeProxy({ getDaemonSocket, makeEngine: () => makeFallbackEngine(root), root }); } diff --git a/src/mcp/project-lifecycle.ts b/src/mcp/project-lifecycle.ts index b7a52c1a5b..1a634815eb 100644 --- a/src/mcp/project-lifecycle.ts +++ b/src/mcp/project-lifecycle.ts @@ -118,7 +118,8 @@ async function activate(root: string, project: Project): Promise { if (writer.existing?.mode === 'daemon') { for (const candidate of getDaemonSocketCandidates(root)) { const socket = await connectWithHello(candidate); - if (!socket || socket === 'version-mismatch') continue; + // A daemon of another version, older or not, is left to the project's own launchers. + if (!socket || typeof socket === 'string') continue; if (project.refs === 0) { socket.destroy(); return; } project.socket = socket; socket.once('close', () => { diff --git a/src/mcp/proxy.ts b/src/mcp/proxy.ts index 52e6b77fd5..c0d9f6cafb 100644 --- a/src/mcp/proxy.ts +++ b/src/mcp/proxy.ts @@ -26,7 +26,7 @@ import { EARLY_PPID } from './early-ppid'; import { supervisionLostReason } from './ppid-watchdog'; import { armStartupHandshakeTimeout } from './startup-handshake'; import { treatStdinFailureAsShutdown } from './stdin-teardown'; -import { CodeGraphPackageVersion } from './version'; +import { CodeGraphPackageVersion, isOlderRelease } from './version'; import { SERVER_INFO, PROTOCOL_VERSION, initializeInstructions } from './session'; import { SERVER_INSTRUCTIONS } from './server-instructions'; import { getStaticTools } from './tools'; @@ -148,14 +148,17 @@ export async function runProxy( /** * Connect to a daemon at `socketPath` and verify its hello (exact version match). - * Returns the live socket (hello already consumed) or null if unreachable / stale - * / version-mismatched. Unlike {@link runProxy} it does NOT pipe — the caller - * owns the socket. Used by the local-handshake proxy's background connect. + * Returns the live socket (hello already consumed), null if unreachable / stale, + * or — for a daemon of another version — `'older-version'` when it runs an older + * release ({@link isOlderRelease}; the launcher replaces it, #2335) and + * `'version-mismatch'` for any other version, which is left alone. Unlike + * {@link runProxy} it does NOT pipe — the caller owns the socket. Used by the + * local-handshake proxy's background connect. */ export async function connectWithHello( socketPath: string, expectedVersion: string = CodeGraphPackageVersion, -): Promise { +): Promise { if (process.platform !== 'win32' && !fs.existsSync(socketPath)) return null; const socket = net.createConnection(socketPath); socket.setEncoding('utf8'); @@ -176,12 +179,14 @@ export async function connectWithHello( } if (hello.codegraph !== expectedVersion) { // A daemon IS up but it's the wrong version — definitive, not a "not yet". - // Don't poll; the caller serves in-process so we never run stale-vs-new. + socket.destroy(); + // An older release is the launcher's to replace (#2335); it says what it did. + if (isOlderRelease(hello.codegraph, expectedVersion)) return 'older-version'; + // Any other: don't poll; the caller serves in-process so we never run stale-vs-new. process.stderr.write( `[CodeGraph MCP] Found a daemon on ${socketPath} but version (${hello.codegraph}) ` + `differs from ours (${expectedVersion}); serving this session in-process.\n` ); - socket.destroy(); return 'version-mismatch'; } logAttachedDaemon(socketPath, hello); @@ -212,8 +217,9 @@ type JsonRpc = Record; /** Dependencies the local-handshake proxy needs, injected by MCPServer (which * owns the daemon-spawn machinery and the engine factory). */ export interface LocalHandshakeDeps { - /** Probe → spawn → retry → hello-verify; resolves a connected daemon socket, - * or null when the daemon path is genuinely unavailable (→ in-process fallback). */ + /** Probe (replacing a daemon of an older release, #2335) → spawn → retry → + * hello-verify; resolves a connected daemon socket, or null when the daemon + * path is genuinely unavailable (→ in-process fallback). */ getDaemonSocket(): Promise; /** Lazily create an in-process engine — used only while the daemon is * unreachable, preserving the "a broken daemon never wedges a session" diff --git a/src/mcp/version.ts b/src/mcp/version.ts index cef1b7834a..b6c919f0f0 100644 --- a/src/mcp/version.ts +++ b/src/mcp/version.ts @@ -4,7 +4,8 @@ * The version string is the rendezvous datum between cooperating daemon and * proxy processes: the daemon advertises its version in the hello line, and * the proxy refuses to share IPC across a mismatch (falls back to direct - * mode). Keeping the resolution in one place avoids drift between the CLI + * mode), or replaces a daemon of an older release ({@link isOlderRelease}). + * Keeping the resolution in one place avoids drift between the CLI * `--version` output (which reads `package.json` directly) and the daemon * handshake. * @@ -34,3 +35,25 @@ function readPackageVersion(): string { } export const CodeGraphPackageVersion = readPackageVersion(); + +/** + * Whether `version` is a CodeGraph release older than `than` — the test a + * launcher applies before it replaces a running daemon (#2335). Only plain + * `MAJOR.MINOR.PATCH` releases compare: a prerelease, a build suffix or the + * "0.0.0-unknown" sentinel is never older. A daemon is replaced only by a + * strictly newer release, so two installed versions can never take turns + * stopping each other's daemon. + */ +export function isOlderRelease(version: string, than: string): boolean { + const a = parseRelease(version); + const b = parseRelease(than); + if (!a || !b) return false; + if (a[0] !== b[0]) return a[0] < b[0]; + if (a[1] !== b[1]) return a[1] < b[1]; + return a[2] < b[2]; +} + +function parseRelease(version: string): [number, number, number] | null { + const m = /^(\d+)\.(\d+)\.(\d+)$/.exec(version); + return m ? [Number(m[1]), Number(m[2]), Number(m[3])] : null; +} diff --git a/src/mcp/writer-lock.ts b/src/mcp/writer-lock.ts index 9acbdb9a46..7785cde674 100644 --- a/src/mcp/writer-lock.ts +++ b/src/mcp/writer-lock.ts @@ -165,6 +165,44 @@ export function markWriterReady(projectRoot: string): void { } } +/** Waits between retries of a writer-record swap refused by Windows (another process has the file open). */ +const SWAP_RETRY_DELAYS_MS = [25, 50, 100, 200]; + +/** + * Replace the writer record of `fromPid` with `next`, and only that record, so + * the slot passes between two processes without ever falling free (#2335). A + * stopped daemon's sessions serve themselves in-process the moment it goes, and + * one that found the slot free or stale would claim it as their own writer. + * Compare and rename are separate steps, so callers check the result: whether + * the record was replaced. Never throws. + */ +export function swapWriterLock(projectRoot: string, fromPid: number, next: WriterLockInfo): boolean { + const pidPath = getWriterPidPath(projectRoot); + const tmp = `${pidPath}.${process.pid}.swap.tmp`; + try { + if (readWriterLock(projectRoot)?.pid !== fromPid) return false; + fs.writeFileSync(tmp, encode(next), { mode: 0o600 }); + for (let attempt = 0; ; attempt++) { + try { + fs.renameSync(tmp, pidPath); + return true; + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + const delay = SWAP_RETRY_DELAYS_MS[attempt]; + if (process.platform !== 'win32' || !['EPERM', 'EACCES', 'EBUSY'].includes(code ?? '') || delay === undefined) { + return false; + } + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, delay); + if (readWriterLock(projectRoot)?.pid !== fromPid) return false; + } + } + } catch { + return false; + } finally { + try { fs.unlinkSync(tmp); } catch { /* renamed, or never written */ } + } +} + /** Release if we still own the lock (pid match). */ export function releaseWriterLock(projectRoot: string, lockName: 'writer.pid' | 'rebuild.pid' = 'writer.pid'): void { const pidPath = getWriterPidPath(projectRoot, lockName); From dea076fd1e9fa3e236e93fa915bb0feebe73579d Mon Sep 17 00:00:00 2001 From: Colby Mchenry Date: Mon, 5 Oct 2026 15:08:14 +0000 Subject: [PATCH 3/5] fix(mcp,extraction): exit a daemon whose install changed; never store a grammar-load failure (#2335, #2336) (#2346) * fix(mcp,extraction): a daemon whose install changed exits; grammar-load failures are never stored (#2335, #2336) A daemon that outlived an upgrade loaded grammars lazily from an install that no longer existed, and stored every file its watcher re-indexed as a zero-node row under the file's new content hash: no hash-based sync ever revisited those rows, and `codegraph status` called the index up to date. - The daemon checks its own package.json every 30 s (CODEGRAPH_DAEMON_INSTALL_CHECK_MS, 0 turns it off) and exits once the file is gone or carries another version, so the next session starts a daemon from the current install. - A result whose grammar failed to load (`parser_error`) is never stored: the file keeps its previous data and the next sync or index retries it, and a row an older engine stored that way is re-indexed even though its hash matches. The tag-based CFML path reports a missing grammar with the same code. - A launcher whose own install was deleted no longer crashes on the spawn error of the daemon it tried to start. - `codegraph status` and `status --json` (filesNeedingReindex, filesWithParseErrors) report indexed files whose symbols are missing although their content is current (#2336), and `files --json` carries each file's recorded errors. Co-Authored-By: Claude Opus 5.5 * fix(mcp): a session whose own install changed never claims the writer lock (#2335) A daemon now exits once its install is upgraded or removed, and nobody holds the writer slot for a successor at that point. Its sessions run the same replaced code and serve themselves in-process the moment it goes; one that found the slot free claimed it as its writer even though its engine could not load (a real npm upgrade: "Cannot find module './sqlite-adapter'"), and so kept a daemon from the current install from starting. Such a session now falls back read-only and says to restart it. Also adds the CHANGELOG entries for the install check and grammar-load failures (#2335) and for the status report (#2336). Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 2 + __tests__/daemon-install-check.test.ts | 83 ++++++++++++ __tests__/daemon-older-version.test.ts | 45 +++++++ __tests__/grammar-load-failure.test.ts | 167 +++++++++++++++++++++++++ __tests__/status-index-health.test.ts | 159 +++++++++++++++++++++++ src/bin/codegraph.ts | 49 +++++++- src/db/queries.ts | 12 ++ src/extraction/cfml-extractor.ts | 4 +- src/extraction/grammars.ts | 17 ++- src/extraction/index.ts | 40 ++++-- src/index.ts | 30 +++++ src/mcp/daemon.ts | 72 ++++++++++- src/mcp/index.ts | 17 ++- src/mcp/version.ts | 6 +- src/types.ts | 18 +++ 15 files changed, 706 insertions(+), 15 deletions(-) create mode 100644 __tests__/daemon-install-check.test.ts create mode 100644 __tests__/grammar-load-failure.test.ts create mode 100644 __tests__/status-index-health.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 32db6b23ab..f280890b4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - In VB.NET and C#, what a property's `Get` and `Set` code calls, creates and reads now belongs to that property, as do C#'s `get => …` accessors and `=> …` property bodies. Before, it was dropped, so a method used only from a property looked unused. - In VB.NET, a field or property initializer like `= Compute()` or `As New List(Of Order)` now links what it calls and creates, and so do a `Custom Event`'s `AddHandler`, `RemoveHandler` and `RaiseEvent` blocks. Re-index VB.NET and C# projects after upgrading. - Upgrading CodeGraph while an agent session is open no longer leaves the old version's background server in charge of your project: the first session started from the new install stops it and starts a current one in its place, even while sessions opened before the upgrade are still running. That old server could no longer load the language parsers the upgrade removed, so it saved every file it re-indexed with no symbols, while new sessions could only read the index beside it without keeping it up to date. A background server from a newer install is never stopped, and sessions opened before the upgrade keep the old version until you restart them. Thanks @lipchey for the report. (#2335) +- A file is no longer saved with no symbols when its language parser can't be loaded, which is what happened to every file a background server re-indexed after an upgrade removed its install: the file keeps what it had and is indexed again once the parser loads, and files an earlier version emptied this way are re-indexed by the next sync. A background server also exits on its own once its install is upgraded or removed, so the next session starts one from the current install. Thanks @lipchey for the report. (#2335) +- `codegraph status` no longer says the index is up to date while indexed files are missing their symbols: it now names files the parser couldn't read and files stored without their symbols (which `codegraph sync` repairs), `status --json` counts both, and `codegraph files --json` lists each file's recorded errors. Thanks @lipchey for the report. (#2336) ## [1.6.2] - 2026-10-03 diff --git a/__tests__/daemon-install-check.test.ts b/__tests__/daemon-install-check.test.ts new file mode 100644 index 0000000000..614afb6e4a --- /dev/null +++ b/__tests__/daemon-install-check.test.ts @@ -0,0 +1,83 @@ +/** + * A daemon whose install was upgraded or deleted underneath it exits (#2335). + * + * The daemon loads grammars (and the native kernel, and worker scripts) lazily + * from the directory it was started from. An upgrade replaces or deletes that + * directory while the daemon keeps running, so every later load fails; the + * 1.6.1 daemon in #2335 went on re-indexing files as empty for as long as it + * lived. It now checks its own package.json periodically and makes way for a + * daemon started from the current install. The check is pure and the Daemon's + * reaction is tested with it injected; neither starts a real daemon. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { Daemon, installChangedReason } from '../src/mcp/daemon'; + +describe('installChangedReason', () => { + let dir: string; + let pkg: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-install-check-')); + pkg = path.join(dir, 'package.json'); + fs.writeFileSync(pkg, JSON.stringify({ name: '@colbymchenry/codegraph', version: '1.6.2' })); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('is null while the install still carries the running version', () => { + expect(installChangedReason(pkg, '1.6.2')).toBeNull(); + }); + + it('reports an install upgraded in place', () => { + fs.writeFileSync(pkg, JSON.stringify({ version: '1.6.3' })); + expect(installChangedReason(pkg, '1.6.2')).toMatch(/replaced by v1\.6\.3/); + }); + + it('reports an install that was deleted, file or whole directory', () => { + fs.rmSync(pkg); + expect(installChangedReason(pkg, '1.6.2')).toMatch(/removed/); + fs.rmSync(dir, { recursive: true, force: true }); + expect(installChangedReason(pkg, '1.6.2')).toMatch(/removed/); + }); + + it('waits for a package.json caught mid-write instead of exiting on it', () => { + fs.writeFileSync(pkg, '{"name": "@colbymchenry/codeg'); + expect(installChangedReason(pkg, '1.6.2')).toBeNull(); + fs.writeFileSync(pkg, ''); + expect(installChangedReason(pkg, '1.6.2')).toBeNull(); + }); +}); + +describe('Daemon.checkInstall', () => { + // A Daemon is safe to construct (nothing binds until start()); stop() is + // replaced so a positive check can't tear down the test process. + const makeDaemon = () => { + const d = new Daemon('/tmp/codegraph-install-check-unit-test', { idleTimeoutMs: 0 }) as any; + d.stop = vi.fn(async () => {}); + return d; + }; + + it('stops the daemon when its install changed', () => { + const d = makeDaemon(); + expect(d.checkInstall(() => 'Install removed')).toBe(true); + expect(d.stop).toHaveBeenCalledWith('install changed'); + }); + + it('keeps running while the install is unchanged', () => { + const d = makeDaemon(); + expect(d.checkInstall(() => null)).toBe(false); + expect(d.stop).not.toHaveBeenCalled(); + }); + + it('does nothing once the daemon is already stopping', () => { + const d = makeDaemon(); + d.stopping = true; + expect(d.checkInstall(() => 'Install removed')).toBe(false); + expect(d.stop).not.toHaveBeenCalled(); + }); +}); diff --git a/__tests__/daemon-older-version.test.ts b/__tests__/daemon-older-version.test.ts index 188cd6cf82..b29cbba3c6 100644 --- a/__tests__/daemon-older-version.test.ts +++ b/__tests__/daemon-older-version.test.ts @@ -713,4 +713,49 @@ describe('a launcher meeting a daemon of another version (#2335)', () => { expect(writerLock()).toMatchObject({ pid: newer.pid, mode: 'daemon' }); await status(newer.session, 3, 'a tool call through the newer daemon'); }, 90000); + + it('a session whose own install was upgraded serves reads only once its daemon exits', async () => { + // An upgrade in place: the daemon notices its package.json changed and + // exits (its install check), and its session falls back in-process. As + // the project's writer that session — running code the upgrade replaced — + // would only keep a daemon from the new install from starting. + const install = makeInstall('0.0.2'); + try { + const before = await sessionWithDaemon(install, '0.0.2', { + CODEGRAPH_DAEMON_INSTALL_CHECK_MS: '200', + CODEGRAPH_DAEMON_RETRY_MS: '0', + }); + fs.writeFileSync(path.join(install, 'package.json'), JSON.stringify({ name: '@colbymchenry/codegraph', version: '0.0.3' }) + '\n'); + expect(await waitProcessExit(before.pid, 10000)).toBe(true); + expect(daemonLog()).toContain('Install replaced by v0.0.3'); + await waitFor( + () => before.session.stderr.some((l) => l.includes('Shared daemon connection lost')), + 10000, 'the session to lose its daemon', + ); + + await status(before.session, 3, 'a tool call after the daemon exited'); + const stderr = before.session.stderr.join('\n'); + expect(stderr).toContain("this session's CodeGraph install was upgraded or removed"); + expect(stderr).not.toContain('File watcher active'); + expect(writerLock()).toBeNull(); + + // So a session from the current install gets a daemon straight away. + const after = startSession(null); + await waitFor(() => attachedLine(after), 20000, 'the new session to attach'); + const lock = daemonLock(); + daemonPids.add(lock.pid); + expect(writerLock()).toMatchObject({ pid: lock.pid, mode: 'daemon' }); + } finally { + // On Windows a process still running from the install blocks its removal. + for (const { child } of sessions) { + if (child.exitCode === null && child.signalCode === null) { + const exited = once(child, 'exit'); + child.kill('SIGKILL'); + await exited; + } + } + try { fs.unlinkSync(path.join(install, 'node_modules')); } catch { /* not created */ } + await fs.promises.rm(install, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + } + }, 90000); }); diff --git a/__tests__/grammar-load-failure.test.ts b/__tests__/grammar-load-failure.test.ts new file mode 100644 index 0000000000..1f285f0fd4 --- /dev/null +++ b/__tests__/grammar-load-failure.test.ts @@ -0,0 +1,167 @@ +/** + * A grammar that fails to load is an environment failure, not a parse result + * (#2335). + * + * A long-running daemon loads tree-sitter grammars lazily from its install + * directory. When an upgrade deletes that directory underneath it, every later + * grammar load fails (ENOENT) and the extractor answers `parser_error` with no + * nodes. That used to be STORED: the file's good symbols were replaced by a + * zero-node row carrying the new content hash, so `sync` never revisited it + * and `status` stayed green. Now nothing is stored for such a result — the + * file keeps its previous index data and the next sync or index retries it — + * and rows an older engine already wrote that way are re-indexed by `sync`. + * + * The failure is simulated at `getParser` (what a failed load leaves behind: + * no parser for the language), with the native kernel switched off so every + * file goes through the grammar path regardless of whether a kernel is built. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { CodeGraph } from '../src'; +import type { Language } from '../src/types'; + +const { failing } = vi.hoisted(() => ({ failing: new Set() })); + +vi.mock('../src/extraction/grammars', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + getParser: (language: Language) => (failing.has(language) ? null : actual.getParser(language)), + }; +}); + +const MATH_TS = + 'export function add(a: number, b: number): number {\n' + + ' return a + b;\n' + + '}\n' + + 'export class Calculator {\n' + + ' total = 0;\n' + + ' plus(n: number): this { this.total = add(this.total, n); return this; }\n' + + '}\n'; + +const VIEW_TS = + "import { Calculator } from './math';\n" + + 'export function total(n: number): number {\n' + + ' return new Calculator().plus(n).total;\n' + + '}\n'; + +const SUB_TS = '\nexport function sub(a: number, b: number): number {\n return a - b;\n}\n'; + +function names(cg: CodeGraph, file: string): string[] { + return cg.getNodesInFile(file).map((n) => n.name); +} + +describe('grammar load failures are never stored (#2335)', () => { + let dir: string; + let cg: CodeGraph; + let kernelEnv: string | undefined; + + beforeEach(() => { + kernelEnv = process.env.CODEGRAPH_KERNEL; + process.env.CODEGRAPH_KERNEL = '0'; + failing.clear(); + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-grammar-load-failure-')); + fs.mkdirSync(path.join(dir, 'src')); + fs.writeFileSync(path.join(dir, 'src', 'math.ts'), MATH_TS); + fs.writeFileSync(path.join(dir, 'src', 'view.ts'), VIEW_TS); + }); + + afterEach(() => { + failing.clear(); + cg?.destroy(); + if (kernelEnv === undefined) delete process.env.CODEGRAPH_KERNEL; + else process.env.CODEGRAPH_KERNEL = kernelEnv; + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('sync keeps the previous symbols of a file whose grammar failed to load, then retries it', async () => { + cg = await CodeGraph.init(dir); + await cg.indexAll(); + const before = cg.getFile('src/math.ts')!; + expect(before.nodeCount).toBeGreaterThan(1); + expect(names(cg, 'src/math.ts')).toEqual(expect.arrayContaining(['add', 'Calculator'])); + + // The upgrade removed the grammar; the watcher then re-indexes an edit. + failing.add('typescript'); + fs.appendFileSync(path.join(dir, 'src', 'math.ts'), SUB_TS); + const failed = await cg.sync(); + + expect(failed.failedFilePaths).toContain('src/math.ts'); + const kept = cg.getFile('src/math.ts')!; + expect(kept.nodeCount).toBe(before.nodeCount); + expect(kept.contentHash).toBe(before.contentHash); + expect(kept.errors ?? []).toEqual([]); + expect(names(cg, 'src/math.ts')).toEqual(expect.arrayContaining(['add', 'Calculator'])); + + // Grammar available again (a fresh process from the new install): the + // next sync sees the file as still changed and indexes the edit. + failing.clear(); + const retried = await cg.sync(); + expect(retried.filesModified).toBe(1); + expect(names(cg, 'src/math.ts')).toEqual(expect.arrayContaining(['add', 'Calculator', 'sub'])); + expect(cg.getFile('src/math.ts')!.nodeCount).toBeGreaterThan(before.nodeCount); + }); + + it('a full index stores nothing for files whose grammar failed to load, and sync adds them later', async () => { + failing.add('typescript'); + cg = await CodeGraph.init(dir); + const result = await cg.indexAll(); + + expect(result.filesErrored).toBe(2); + expect(result.errors.filter((e) => e.code === 'parser_error')).toHaveLength(2); + expect(cg.getFile('src/math.ts')).toBeNull(); + expect(cg.getFile('src/view.ts')).toBeNull(); + + failing.clear(); + const synced = await cg.sync(); + expect(synced.filesAdded).toBe(2); + expect(names(cg, 'src/math.ts')).toEqual(expect.arrayContaining(['add', 'Calculator'])); + expect(names(cg, 'src/view.ts')).toContain('total'); + }); + + it('sync re-indexes rows an older engine stored while the grammar was missing', async () => { + cg = await CodeGraph.init(dir); + await cg.indexAll(); + const before = cg.getFile('src/math.ts')!; + + // What v1.6.2 wrote for a file it re-indexed with no grammar: no nodes, + // the parser error, and the CURRENT content hash — so a hash-based + // reconcile considers it up to date forever. + const db = (cg as unknown as { db: { getDb(): { prepare(sql: string): { run(...args: unknown[]): unknown } } } }).db.getDb(); + db.prepare('DELETE FROM nodes WHERE file_path = ?').run('src/math.ts'); + db.prepare('UPDATE files SET node_count = 0, errors = ? WHERE path = ?').run( + JSON.stringify([{ message: 'Failed to get parser for language: typescript', filePath: 'src/math.ts', severity: 'error', code: 'parser_error' }]), + 'src/math.ts' + ); + expect(cg.getFile('src/math.ts')!.nodeCount).toBe(0); + + await cg.sync(); + + const healed = cg.getFile('src/math.ts')!; + expect(healed.nodeCount).toBe(before.nodeCount); + expect(healed.errors ?? []).toEqual([]); + expect(names(cg, 'src/math.ts')).toEqual(expect.arrayContaining(['add', 'Calculator'])); + }); + + it('a component whose script grammar failed keeps its previous symbols too', async () => { + fs.writeFileSync( + path.join(dir, 'src', 'Counter.svelte'), + '\n\n' + ); + cg = await CodeGraph.init(dir); + await cg.indexAll(); + const before = cg.getFile('src/Counter.svelte')!; + expect(names(cg, 'src/Counter.svelte')).toContain('increment'); + + failing.add('typescript'); + fs.appendFileSync(path.join(dir, 'src', 'Counter.svelte'), '

edited

\n'); + await cg.sync(); + + // The component node alone would have been stored before: the script's + // symbols gone, with a nonzero node count hiding it. + expect(cg.getFile('src/Counter.svelte')!.nodeCount).toBe(before.nodeCount); + expect(names(cg, 'src/Counter.svelte')).toContain('increment'); + }); +}); diff --git a/__tests__/status-index-health.test.ts b/__tests__/status-index-health.test.ts new file mode 100644 index 0000000000..b94562c8eb --- /dev/null +++ b/__tests__/status-index-health.test.ts @@ -0,0 +1,159 @@ +/** + * `codegraph status` must not call an index "up to date" while indexed files + * are missing their symbols (#2336, #2335). + * + * "Up to date" used to mean only "every file's content hash matches the + * index". Two kinds of file pass that check with nothing usable in the graph: + * + * - a file the parser could not read — `export type * from` is valid + * TypeScript 5.0 the bundled grammar has no rule for, so the file is stored + * with a parse error and no symbols (#2336); + * - a row stored without its symbols — by an engine whose grammar failed to + * load (#2335), or the #1541 wipe — which `codegraph sync` re-indexes. + * + * Both are counted in `status` (human and `--json`), and `files --json` carries + * each file's recorded errors. Exercised against the built CLI so the output + * and the JSON field names are what users and scripts actually see. + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { CodeGraph } from '../src'; + +const BIN = path.resolve(__dirname, '../dist/bin/codegraph.js'); + +function spawn(cwd: string, args: string[]) { + return spawnSync(process.execPath, [BIN, ...args], { + cwd, + encoding: 'utf-8', + timeout: 30_000, + env: { + ...process.env, + CODEGRAPH_NO_DAEMON: '1', + CODEGRAPH_TELEMETRY: '0', + CODEGRAPH_NO_UPDATE_CHECK: '1', + NO_COLOR: '1', + }, + }); +} + +function run(cwd: string, args: string[]): { status: number | null; out: string } { + const result = spawn(cwd, args); + return { status: result.status, out: (result.stdout ?? '') + (result.stderr ?? '') }; +} + +function runJson(cwd: string, args: string[]): any { + const result = spawn(cwd, args); + expect(result.status, (result.stdout ?? '') + (result.stderr ?? '')).toBe(0); + return JSON.parse(result.stdout); +} + +describe('status reports files indexed without their symbols (#2336, #2335)', () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-status-health-')); + fs.mkdirSync(path.join(dir, 'src', 'constants'), { recursive: true }); + fs.mkdirSync(path.join(dir, 'src', 'types'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'constants', 'index.ts'), + 'export const MAX_ITEMS = 10;\nexport function clamp(n: number): number { return Math.min(n, MAX_ITEMS); }\n' + ); + fs.writeFileSync( + path.join(dir, 'src', 'types', 'database.ts'), + 'export interface Row { id: number; name: string }\nexport type RowId = Row["id"];\n' + ); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + async function index(): Promise { + const cg = CodeGraph.initSync(dir); + await cg.indexAll(); + cg.destroy(); + } + + it('a clean index is up to date, with zero counts and empty error lists', async () => { + await index(); + + const human = run(dir, ['status']); + expect(human.status, human.out).toBe(0); + expect(human.out).toContain('Index is up to date'); + + const json = runJson(dir, ['status', '--json']); + expect(json.index.filesNeedingReindex).toBe(0); + expect(json.index.filesWithParseErrors).toBe(0); + + const files = runJson(dir, ['files', '--json']); + expect(files).toHaveLength(2); + for (const f of files) expect(f.errors).toEqual([]); + }); + + it('a file the parser could not read keeps status from reporting "up to date"', async () => { + fs.writeFileSync( + path.join(dir, 'src', 'index.ts'), + 'export * from "./constants/index.js";\nexport type * from "./types/database.js";\n' + ); + await index(); + + const human = run(dir, ['status']); + expect(human.status, human.out).toBe(0); + expect(human.out).not.toContain('Index is up to date'); + expect(human.out).toMatch(/1 file could not be parsed/); + expect(human.out).toContain('src/index.ts'); + + const json = runJson(dir, ['status', '--json']); + expect(json.index.filesWithParseErrors).toBe(1); + expect(json.index.filesNeedingReindex).toBe(0); + + const files = runJson(dir, ['files', '--json']); + const broken = files.find((f: any) => f.path === 'src/index.ts'); + expect(broken.errors).toHaveLength(1); + expect(broken.errors[0]).toMatchObject({ severity: 'warning', code: 'parse_error' }); + expect(broken.errors[0].message).toContain('parse produced no symbols'); + expect(files.find((f: any) => f.path === 'src/constants/index.ts').errors).toEqual([]); + }); + + it('rows stored without their symbols ask for a sync, and the sync repairs them', async () => { + await index(); + + // A row v1.6.2 wrote with its grammar missing (#2335), and a #1541 wipe: + // both carry the current content hash, so the hash check calls them current. + const { DatabaseSync } = require('node:sqlite'); + const db = new DatabaseSync(path.join(dir, '.codegraph', 'codegraph.db')); + db.prepare('DELETE FROM nodes WHERE file_path IN (?, ?)').run('src/constants/index.ts', 'src/types/database.ts'); + db.prepare('UPDATE files SET node_count = 0, errors = ? WHERE path = ?').run( + JSON.stringify([{ message: 'Failed to get parser for language: typescript', severity: 'error', code: 'parser_error' }]), + 'src/constants/index.ts' + ); + db.prepare('UPDATE files SET node_count = 0 WHERE path = ?').run('src/types/database.ts'); + db.close(); + + const human = run(dir, ['status']); + expect(human.status, human.out).toBe(0); + expect(human.out).not.toContain('Index is up to date'); + expect(human.out).toMatch(/2 files are missing their symbols/); + expect(human.out).toContain('codegraph sync'); + + const json = runJson(dir, ['status', '--json']); + expect(json.index.filesNeedingReindex).toBe(2); + expect(json.index.filesWithParseErrors).toBe(0); + expect(json.pendingChanges).toEqual({ added: 0, modified: 0, removed: 0 }); + + const synced = run(dir, ['sync']); + expect(synced.status, synced.out).toBe(0); + + const after = runJson(dir, ['status', '--json']); + expect(after.index.filesNeedingReindex).toBe(0); + expect(run(dir, ['status']).out).toContain('Index is up to date'); + const files = runJson(dir, ['files', '--json']); + for (const f of files) { + expect(f.nodeCount).toBeGreaterThan(1); + expect(f.errors).toEqual([]); + } + }); +}); diff --git a/src/bin/codegraph.ts b/src/bin/codegraph.ts index e5f8eacfc6..de33c1a47e 100644 --- a/src/bin/codegraph.ts +++ b/src/bin/codegraph.ts @@ -1056,6 +1056,33 @@ program } }); +/** + * The `status` warnings for indexed files whose symbols are missing although + * their content is current (#2335, #2336): one line per group, saying what is + * wrong, what to do, and naming up to three of the files. + */ +function describeFilesMissingSymbols(health: { needsReindex: string[]; parseErrors: string[] }): string[] { + const dash = getGlyphs().dash; + const sample = (paths: string[]): string => + paths.slice(0, 3).join(', ') + (paths.length > 3 ? ` (+${formatNumber(paths.length - 3)} more)` : ''); + const lines: string[] = []; + const reindex = health.needsReindex.length; + if (reindex > 0) { + lines.push( + `${formatNumber(reindex)} ${reindex === 1 ? 'file is missing its' : 'files are missing their'} symbols ${dash} ` + + `run "codegraph sync" to re-index ${reindex === 1 ? 'it' : 'them'}: ${sample(health.needsReindex)}` + ); + } + const unparsed = health.parseErrors.length; + if (unparsed > 0) { + lines.push( + `${formatNumber(unparsed)} ${unparsed === 1 ? 'file could not be parsed, so its symbols are' : 'files could not be parsed, so their symbols are'} ` + + `missing: ${sample(health.parseErrors)} ${dash} "codegraph files --json" shows the errors` + ); + } + return lines; +} + /** * codegraph status [path] */ @@ -1103,6 +1130,9 @@ program // Zero on a healthy index; non-zero at rest means a resolution pass was // interrupted, so some files' call edges are missing (#1187). const pendingRefs = cg.getPendingReferenceCount(); + // Files whose content is current but whose symbols are missing — the + // content-hash check behind "up to date" cannot see them (#2336). + const health = cg.getIndexHealth(); // JSON output mode if (options.json) { @@ -1143,6 +1173,12 @@ program // interrupted resolution pass left edges missing; the next // sync sweeps them (#1187). pendingRefs, + // Files stored without their symbols (e.g. while their grammar + // could not load, #2335); the next sync re-indexes them. + filesNeedingReindex: health.needsReindex.length, + // Files with a recorded parse error and no symbols from it + // (#2336); unchanged until the file or the parser changes. + filesWithParseErrors: health.parseErrors.length, }, })); cg.destroy(); @@ -1235,7 +1271,10 @@ program console.log(` Removed: ${changes.removed.length} files`); } info('Run "codegraph sync" to update the index'); - } else { + } + const missingSymbols = describeFilesMissingSymbols(health); + for (const line of missingSymbols) warn(line); + if (totalChanges === 0 && missingSymbols.length === 0) { success('Index is up to date'); } console.log(); @@ -1751,6 +1790,14 @@ program language: f.language, nodeCount: f.nodeCount, size: f.size, + // What extraction recorded for the file — a parse error, a skip + // reason — so a health check needn't read the database (#2336). + errors: (f.errors ?? []).map((e) => ({ + severity: e.severity, + code: e.code, + message: e.message, + line: e.line, + })), })); console.log(JSON.stringify(output, null, 2)); cg.destroy(); diff --git a/src/db/queries.ts b/src/db/queries.ts index a12e7c2643..704cfd5bcd 100644 --- a/src/db/queries.ts +++ b/src/db/queries.ts @@ -3253,6 +3253,18 @@ export class QueryBuilder { return rows.map(rowToFileRecord); } + /** + * Files stored with no nodes or with recorded extraction errors — the only + * rows that can be missing their symbols (`CodeGraph.getIndexHealth`). A + * healthy index returns few or none, so this stays cheap on a large one. + */ + getFilesWithoutNodesOrWithErrors(): FileRecord[] { + const rows = this.db + .prepare('SELECT * FROM files WHERE node_count = 0 OR errors IS NOT NULL ORDER BY path') + .all() as FileRow[]; + return rows.map(rowToFileRecord); + } + /** * Most recent index timestamp (ms since epoch) across all tracked files, or * null when nothing is indexed yet. One indexed aggregate, no per-row scan. (#329) diff --git a/src/extraction/cfml-extractor.ts b/src/extraction/cfml-extractor.ts index efd2ea6ea6..a1a668706a 100644 --- a/src/extraction/cfml-extractor.ts +++ b/src/extraction/cfml-extractor.ts @@ -147,10 +147,12 @@ export class CfmlExtractor { private extractTagBased(): void { const parser = getParser('cfml'); if (!parser) { + // Same code as TreeSitterExtractor's missing parser, so a grammar that + // failed to load is never stored as this file's index data (#2335). this.errors.push({ message: 'cfml grammar not loaded', severity: 'error', - code: 'unsupported_language', + code: 'parser_error', }); return; } diff --git a/src/extraction/grammars.ts b/src/extraction/grammars.ts index 6a05b96c0d..0a12e61d7d 100644 --- a/src/extraction/grammars.ts +++ b/src/extraction/grammars.ts @@ -9,7 +9,7 @@ import * as path from 'path'; import * as fsp from 'fs/promises'; import { Parser, Language as WasmLanguage } from 'web-tree-sitter'; -import { Language } from '../types'; +import { ExtractionError, Language } from '../types'; export type GrammarLanguage = Exclude; @@ -828,6 +828,21 @@ export function getUnavailableGrammarErrors(): Partial> return out; } +/** + * Whether these extraction errors say the file was never parsed because its + * grammar was not available (`parser_error`, recorded by the extractor when + * `getParser` has nothing for the language). + * + * That is a fact about the running process, not about the file: a daemon whose + * install was upgraded or deleted underneath it fails every lazy grammar load + * (#2335). Such a result must never replace a file's index data, and a row an + * older engine stored that way must be re-indexed — not treated as current + * because its content hash still matches. + */ +export function hasGrammarLoadFailure(errors: readonly ExtractionError[] | undefined): boolean { + return !!errors && errors.some((e) => e.code === 'parser_error'); +} + /** * Get language display name */ diff --git a/src/extraction/index.ts b/src/extraction/index.ts index 0ca3145be2..ecb946eafb 100644 --- a/src/extraction/index.ts +++ b/src/extraction/index.ts @@ -25,7 +25,7 @@ import { ParseWorkerPool, resolveParsePoolSize, resolveParseTimeoutMs } from './ import { StoreWriter, StoreBundle, finalizeStoreBundle } from './store-writer'; import { materializeKernelResult } from './kernel'; import { detectGeneratedFile } from './generated-detection'; -import { detectLanguage, isSourceFile, isLanguageSupported, isFileLevelOnlyLanguage, initGrammars, loadGrammarsForLanguages, readGrammarWasmBytes, isMpegTransportStream, hasMpegTsExtension, MPEG_TS_SNIFF_BYTES } from './grammars'; +import { detectLanguage, isSourceFile, isLanguageSupported, isFileLevelOnlyLanguage, initGrammars, loadGrammarsForLanguages, readGrammarWasmBytes, isMpegTransportStream, hasMpegTsExtension, MPEG_TS_SNIFF_BYTES, hasGrammarLoadFailure } from './grammars'; import { loadExtensionOverrides, loadIncludeIgnoredPatterns, loadExcludePatterns, loadIncludePatterns, PROJECT_CONFIG_FILENAME } from '../project-config'; import { isCodeGraphDataDir } from '../directory'; import { logDebug, logWarn } from '../errors'; @@ -2289,11 +2289,17 @@ export class ExtractionOrchestrator { const nodeCount = result.kernelCounts?.nodes ?? result.nodes.length; const edgeCount = result.kernelCounts?.edges ?? result.edges.length; + // A file whose grammar failed to load was never parsed (#2335). + const grammarUnavailable = hasGrammarLoadFailure(result.errors); + // Store: on the writer thread when active (fresh DB — bundles applied // in the same file order this chain dispatches them), else on the main // thread (SQLite connections are per-thread). const language = detectLanguage(filePath, content, overrides); - if (storeWriter) { + if (grammarUnavailable) { + // Store nothing: a row from an earlier run keeps its data, and with no + // row (or an older hash) the next sync or index retries the file. + } else if (storeWriter) { if (result.kernelBuffers) { // Buffers go to the writer as-is; the worker decodes + finalizes. // The main thread's only per-file work stays O(1) + the content hash. @@ -2321,7 +2327,9 @@ export class ExtractionOrchestrator { errors.push(...result.errors); } - if (nodeCount > 0) { + if (grammarUnavailable) { + filesErrored++; + } else if (nodeCount > 0) { filesIndexed++; totalNodes += nodeCount; totalEdges += edgeCount; @@ -2735,7 +2743,9 @@ export class ExtractionOrchestrator { errors.push(...result.errors); } - if (result.nodes.length > 0) { + if (hasGrammarLoadFailure(result.errors)) { + filesErrored++; // nothing was stored (#2335) + } else if (result.nodes.length > 0) { filesIndexed++; totalNodes += result.nodes.length; totalEdges += result.edges.length; @@ -2918,6 +2928,12 @@ export class ExtractionOrchestrator { result: ExtractionResult, onYield?: MaybeYield ): Promise { + // The file was never parsed: its grammar failed to load (#2335). Storing + // this would replace the file's symbols with an empty row under the new + // content hash, which no hash-based sync revisits. Keep whatever the index + // has; the stale (or missing) row makes the next sync retry the file. + if (hasGrammarLoadFailure(result.errors)) return; + // A kernel result can arrive as an undecoded buffer transport (empty // node/edge arrays, tables riding in kernelBuffers). Decode it before // storing — persisting the transport as-is records the file as having no @@ -2944,7 +2960,10 @@ export class ExtractionOrchestrator { const existingIsMarker = existingFile.nodeCount === 0 && (existingFile.errors?.length ?? 0) > 0; const incomingHasContent = result.nodes.length > 0; - if (!existingIsMarker || !incomingHasContent) { + // A row an older engine stored while the grammar could not load + // (#2335) records no parse at all: any real result replaces it. + const existingNeverParsed = hasGrammarLoadFailure(existingFile.errors); + if (!existingNeverParsed && (!existingIsMarker || !incomingHasContent)) { return; // No changes } } @@ -3400,13 +3419,18 @@ export class ExtractionOrchestrator { } const fullPath = path.join(this.rootDir, filePath); const tracked = trackedMap.get(filePath); + // A row an older engine stored while the file's grammar could not load + // (#2335) holds no parse of these bytes: re-index it even though its + // size, mtime and hash all match. Rows with a real parse error are + // deterministic and are not retried. + const neverParsed = tracked !== undefined && hasGrammarLoadFailure(tracked.errors); // Cheap pre-filter: an already-indexed file whose size AND mtime both match // the DB is unchanged — skip it without reading or hashing. (A content // change that preserves both exactly is the blind spot every mtime-based // incremental tool accepts; `index --force` is the escape hatch. Git bumps // mtime on every file it writes during checkout/merge, so pulls are caught.) - if (tracked) { + if (tracked && !neverParsed) { try { const stat = fs.statSync(fullPath); if (stat.size === tracked.size && Math.floor(stat.mtimeMs) === Math.floor(tracked.modifiedAt)) { @@ -3442,7 +3466,7 @@ export class ExtractionOrchestrator { filesToIndex.push(filePath); changedFilePaths.push(filePath); filesAdded++; - } else if (tracked.contentHash !== contentHash) { + } else if (tracked.contentHash !== contentHash || neverParsed) { onFileChange?.(filePath, content); filesToIndex.push(filePath); changedFilePaths.push(filePath); @@ -3477,7 +3501,7 @@ export class ExtractionOrchestrator { const result = await this.indexFile(filePath); if (result.errors.some(e => e.severity === 'error')) failedFilePaths.push(filePath); - nodesUpdated += result.nodes.length; + if (!hasGrammarLoadFailure(result.errors)) nodesUpdated += result.nodes.length; // else nothing was stored (#2335) const pause = backpressure?.(); if (pause) await pause; diff --git a/src/index.ts b/src/index.ts index 47330503aa..c1e5d25e58 100644 --- a/src/index.ts +++ b/src/index.ts @@ -25,6 +25,7 @@ import { BuildContextOptions, FindRelevantContextOptions, UnresolvedReference, + IndexHealth, } from './types'; import { DatabaseConnection, getDatabasePath, removeDatabaseFiles } from './db'; import { WalCheckpointValve, resolveWalValveMb } from './db/wal-valve'; @@ -43,6 +44,7 @@ import { extractFromSource, initGrammars, } from './extraction'; +import { hasGrammarLoadFailure, isFileLevelOnlyLanguage } from './extraction/grammars'; import { ReferenceResolver, createResolver, @@ -2065,6 +2067,34 @@ export class CodeGraph { return this.queries.countGeneratedFiles(); } + /** + * Indexed files whose symbols are missing although their content is current + * — rows a content-hash comparison calls up to date (#2336). Reported by + * `status`; `sync` repairs the `needsReindex` group. + */ + getIndexHealth(): IndexHealth { + const needsReindex: string[] = []; + const parseErrors: string[] = []; + for (const file of this.queries.getFilesWithoutNodesOrWithErrors()) { + const errors = file.errors ?? []; + if (hasGrammarLoadFailure(errors)) { + // Stored without being parsed — its grammar could not load (#2335). + needsReindex.push(file.path); + } else if (file.nodeCount === 0) { + // Every parse stores at least the file node, so zero nodes means a + // wiped row (#1541) or a recorded failure — except file-level-only + // languages and files over the size limit, which are empty on purpose. + if (isFileLevelOnlyLanguage(file.language)) continue; + if (errors.length === 0) needsReindex.push(file.path); + else if (errors.some((e) => e.severity === 'error' || e.code === 'parse_error')) parseErrors.push(file.path); + } else if (errors.some((e) => e.code === 'parse_error')) { + // Parsed, but the tree had errors and no symbols survived. + parseErrors.push(file.path); + } + } + return { needsReindex, parseErrors }; + } + // =========================================================================== // Graph Query Methods // =========================================================================== diff --git a/src/mcp/daemon.ts b/src/mcp/daemon.ts index 73a4969598..db38f97f5e 100644 --- a/src/mcp/daemon.ts +++ b/src/mcp/daemon.ts @@ -54,7 +54,7 @@ import { getDaemonSocketCandidates, getDaemonSocketPath, } from './daemon-paths'; -import { CodeGraphPackageVersion } from './version'; +import { CodeGraphPackageJsonPath, CodeGraphPackageVersion } from './version'; import { releaseWriterLock, swapWriterLock, @@ -120,6 +120,38 @@ export function finalizeDaemonExit( /** How often the daemon sweeps connected clients for a dead peer process (#692). */ const DEFAULT_CLIENT_SWEEP_MS = 30_000; +/** How often the daemon checks that the install it runs from is still in place (#2335). */ +const DEFAULT_INSTALL_CHECK_MS = 30_000; + +/** + * Why the install this daemon was started from no longer matches it, or null + * while it still does. An upgrade replaces or deletes the package directory + * under a running daemon (npm reinstalling the package, the launcher pruning + * an old bundle), and everything the daemon loads lazily from there afterwards + * fails — grammars among them, so every file its watcher re-indexed came out + * empty (#2335). Such a daemon should make way for one started from the + * current install. Any other read failure, or a package.json caught mid-write, + * answers null: the next check looks again. Exported for testing. + */ +export function installChangedReason(packageJsonPath: string, runningVersion: string): string | null { + let raw: string; + try { + raw = fs.readFileSync(packageJsonPath, 'utf8'); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + return code === 'ENOENT' || code === 'ENOTDIR' ? `Install removed (${packageJsonPath} is gone)` : null; + } + let version: unknown; + try { + version = (JSON.parse(raw) as { version?: unknown } | null)?.version; + } catch { + return null; + } + return typeof version === 'string' && version !== runningVersion + ? `Install replaced by v${version} (this daemon is v${runningVersion})` + : null; +} + /** How long the daemon waits for the optional client-hello before proceeding without it. */ const CLIENT_HELLO_TIMEOUT_MS = 3_000; @@ -185,6 +217,7 @@ export class Daemon { private lastActivityAt = Date.now(); private maxIdleTimer: NodeJS.Timeout | null = null; private clientSweepTimer: NodeJS.Timeout | null = null; + private installCheckTimer: NodeJS.Timeout | null = null; private engine: MCPEngine; private stopping = false; private socketPath: string; @@ -372,6 +405,10 @@ export class Daemon { clearInterval(this.clientSweepTimer); this.clientSweepTimer = null; } + if (this.installCheckTimer) { + clearInterval(this.installCheckTimer); + this.installCheckTimer = null; + } process.stderr.write(`[CodeGraph daemon] Shutting down (${reason}; clients=${this.clients.size}).\n`); for (const session of [...this.clients]) { try { session.stop(); } catch { /* best-effort */ } @@ -491,6 +528,31 @@ export class Daemon { this.clientSweepTimer = setInterval(() => this.reapDeadClients(isProcessAlive), sweepMs); this.clientSweepTimer.unref?.(); } + // An install whose version could not be read at startup has nothing to + // compare against; leave that daemon alone. + const installMs = resolveInstallCheckMs(); + if (installMs > 0 && CodeGraphPackageVersion !== '0.0.0-unknown') { + this.installCheckTimer = setInterval(() => { this.checkInstall(); }, installMs); + this.installCheckTimer.unref?.(); + } + } + + /** + * Exit once the install this daemon runs from has been upgraded or removed + * (#2335; see {@link installChangedReason}). Its clients' proxies fall back + * and reconnect, and the next launch starts a daemon from the current + * install. Returns whether the daemon is exiting. The check is injected for + * tests; the timer passes the real one. + */ + checkInstall( + changed: () => string | null = () => installChangedReason(CodeGraphPackageJsonPath, CodeGraphPackageVersion), + ): boolean { + if (this.stopping) return false; + const reason = changed(); + if (!reason) return false; + process.stderr.write(`[CodeGraph daemon] ${reason}; exiting so the next session starts a daemon from the current install.\n`); + void this.stop('install changed'); + return true; } /** @@ -865,6 +927,14 @@ function resolveClientSweepMs(): number { return Math.floor(parsed); // 0 disables the sweep } +function resolveInstallCheckMs(): number { + const raw = process.env.CODEGRAPH_DAEMON_INSTALL_CHECK_MS; + if (raw === undefined || raw === '') return DEFAULT_INSTALL_CHECK_MS; + const parsed = Number(raw); + if (!Number.isFinite(parsed) || parsed < 0) return DEFAULT_INSTALL_CHECK_MS; + return Math.floor(parsed); // 0 disables the check +} + /** * Parse one client-hello line. Returns the peer pids if `line` is a well-formed * client-hello (carries the `codegraph_client` marker), or null otherwise — in diff --git a/src/mcp/index.ts b/src/mcp/index.ts index ac7aa1d0ad..e41bd43e05 100644 --- a/src/mcp/index.ts +++ b/src/mcp/index.ts @@ -44,12 +44,13 @@ import { MCPSession } from './session'; import { Daemon, clearStaleDaemonLock, + installChangedReason, isProcessAlive, tryAcquireDaemonLock, } from './daemon'; import { clearStaleDaemonArtifacts, stopOlderDaemon } from './daemon-registry'; import { connectWithHello, runLocalHandshakeProxy } from './proxy'; -import { CodeGraphPackageVersion } from './version'; +import { CodeGraphPackageJsonPath, CodeGraphPackageVersion } from './version'; import { readWriterLock, assertNoRebuild, @@ -150,6 +151,14 @@ function makeFallbackEngine(root: string): MCPEngine { if (existing && isProcessAlive(existing.pid)) { return readOnlyFallback(`live daemon PID ${existing.pid} holds the project lock`); } + // This session's own install was upgraded or removed underneath it (#2335): + // its code no longer loads whole, so as the project's writer it would only + // keep a daemon from the current install from starting. Its daemon exits + // for the same reason, which is how it got here. + if (CodeGraphPackageVersion !== '0.0.0-unknown' && + installChangedReason(CodeGraphPackageJsonPath, CodeGraphPackageVersion) !== null) { + return readOnlyFallback('this session\'s CodeGraph install was upgraded or removed; restart the session to use the current one'); + } return new MCPEngine({ writerLockRoot: root, queryPool: true, queryPoolDefaultMax: DIRECT_QUERY_POOL_MAX }); } @@ -295,6 +304,12 @@ function spawnDetachedDaemon(root: string, handover = false): void { env, }, ); + // An upgrade can delete the install this launcher runs from — its daemon + // then exits (#2335) and a respawn finds no executable. spawn reports that + // as an asynchronous 'error' event, which would crash this process if + // nobody listened. No daemon binds either way; the caller's poll gives up + // and the session is served in-process. + child.on('error', () => { /* no daemon — see above */ }); child.unref(); } finally { // The child holds its own dup of the log fd now; the launcher doesn't need it. diff --git a/src/mcp/version.ts b/src/mcp/version.ts index b6c919f0f0..e2ee11ea4d 100644 --- a/src/mcp/version.ts +++ b/src/mcp/version.ts @@ -20,10 +20,12 @@ import * as fs from 'fs'; import * as path from 'path'; +/** The `package.json` of the install this process runs from. */ +export const CodeGraphPackageJsonPath = path.join(__dirname, '..', '..', 'package.json'); + function readPackageVersion(): string { try { - const pkgPath = path.join(__dirname, '..', '..', 'package.json'); - const raw = fs.readFileSync(pkgPath, 'utf8'); + const raw = fs.readFileSync(CodeGraphPackageJsonPath, 'utf8'); const parsed = JSON.parse(raw); if (typeof parsed?.version === 'string' && parsed.version.length > 0) { return parsed.version; diff --git a/src/types.ts b/src/types.ts index 44ffaf4e43..f30080002f 100644 --- a/src/types.ts +++ b/src/types.ts @@ -265,6 +265,24 @@ export interface FileRecord { generated?: boolean; } +/** + * Indexed files whose symbols are missing even though their content hash is + * current — what a hash comparison alone calls "up to date" (#2336). Paths are + * project-relative and sorted. + */ +export interface IndexHealth { + /** + * Files `sync` re-indexes: stored while their grammar could not load (#2335) + * or stored with no nodes and no recorded reason (#1541). + */ + needsReindex: string[]; + /** + * Files with a recorded parse error and no symbols from it. Deterministic: + * they stay this way until the file (or the parser) changes. + */ + parseErrors: string[]; +} + // ============================================================================= // Extraction Types // ============================================================================= From 1846ec280b79f7c78f2b62c4ed499c6208a3e7e3 Mon Sep 17 00:00:00 2001 From: Aaron Queen Date: Mon, 5 Oct 2026 11:01:17 -0600 Subject: [PATCH 4/5] fix(mcp): coordinate writer handovers and preserve legacy owners --- .../codegraph-build-validation/SKILL.md | 2 +- .github/workflows/daemon-lifecycle.yml | 4 +- CHANGELOG.md | 4 + README.md | 6 +- __tests__/daemon-older-version.test.ts | 21 +- __tests__/fixtures/older-daemon.cjs | 9 +- __tests__/fixtures/writer-lock-ordering.cjs | 84 +++++++ __tests__/writer-handover-ordering.test.ts | 59 +++++ codegraph-kernel/Cargo.toml | 1 + codegraph-kernel/src/lib.rs | 1 + codegraph-kernel/src/writer_lock.rs | 33 +++ site/src/content/docs/reference/mcp-server.md | 9 +- src/extraction/kernel/loader.ts | 2 + src/mcp/daemon-paths.ts | 5 +- src/mcp/daemon-registry.ts | 7 +- src/mcp/daemon.ts | 2 + src/mcp/index.ts | 4 +- src/mcp/writer-lock.ts | 209 ++++++++++++------ 18 files changed, 373 insertions(+), 89 deletions(-) create mode 100644 __tests__/fixtures/writer-lock-ordering.cjs create mode 100644 __tests__/writer-handover-ordering.test.ts create mode 100644 codegraph-kernel/src/writer_lock.rs diff --git a/.claude/skills/codegraph-build-validation/SKILL.md b/.claude/skills/codegraph-build-validation/SKILL.md index 4a71704954..5c1f8494fd 100644 --- a/.claude/skills/codegraph-build-validation/SKILL.md +++ b/.claude/skills/codegraph-build-validation/SKILL.md @@ -29,7 +29,7 @@ Under Bun the suite runs green via `__tests__/bun-homedir.setup.ts` (an `os.home `copy-assets` (called from `build`) copies `src/db/schema.sql` into `dist/`. **Any new SQL asset must be copied or it won't ship.** -The native kernel (`codegraph-kernel/`, Rust) is the **only parser**: every grammar is compiled into it (crates.io pins plus the vendored C under `codegraph-kernel/grammars/`, provenance in `grammars/PROVENANCE.md`). There is no wasm fallback. A source checkout needs a prebuild at `codegraph-kernel/prebuilds/-/codegraph-kernel.node`; `npm test` builds it through `scripts/ensure-kernel.mjs` when it is missing or was built from other kernel sources (needs a Rust toolchain), release bundles ship it, and a platform without a prebuild is unsupported (the CLI says so at startup). Languages with a bespoke walker are extracted in Rust; the rest are parsed by the kernel and walked by the generic TypeScript extractor over the serialized tree (`src/extraction/parse-tree.ts`). The whole-graph golden dumps (`__tests__/kernel-golden-dumps.test.ts`) are the regression gate for any extraction change; see `docs/design/kernel-only-extraction-plan.md`. +The native kernel (`codegraph-kernel/`, Rust) is the **only parser**: every grammar is compiled into it (crates.io pins plus the vendored C under `codegraph-kernel/grammars/`, provenance in `grammars/PROVENANCE.md`). There is no wasm fallback. A source checkout needs a prebuild at `codegraph-kernel/prebuilds/-/codegraph-kernel.node`; `npm test` builds it through `scripts/ensure-kernel.mjs` when it is missing or was built from other kernel sources (needs Rust 1.89 or newer), release bundles ship it, and a platform without a prebuild is unsupported (the CLI says so at startup). Languages with a bespoke walker are extracted in Rust; the rest are parsed by the kernel and walked by the generic TypeScript extractor over the serialized tree (`src/extraction/parse-tree.ts`). The whole-graph golden dumps (`__tests__/kernel-golden-dumps.test.ts`) are the regression gate for any extraction change; see `docs/design/kernel-only-extraction-plan.md`. One other build step writes into `dist/` and is subject to the same rule: `build:ui` builds the browser viewer into `dist/viewer/` (never `dist/ui/` — that's the terminal ui). diff --git a/.github/workflows/daemon-lifecycle.yml b/.github/workflows/daemon-lifecycle.yml index dc59dff9f6..214a9669c1 100644 --- a/.github/workflows/daemon-lifecycle.yml +++ b/.github/workflows/daemon-lifecycle.yml @@ -10,6 +10,8 @@ on: - '__tests__/daemon-*.test.ts' - '__tests__/mcp-*.test.ts' - '__tests__/writer-lock.test.ts' + - '__tests__/writer-handover-ordering.test.ts' + - '__tests__/fixtures/writer-lock-ordering.cjs' - '__tests__/query-pool.test.ts' - '__tests__/status-index-health.test.ts' - '__tests__/fixtures/older-daemon.cjs' @@ -52,4 +54,4 @@ jobs: - run: npm ci --ignore-scripts - run: npm run build:kernel - run: npm run build - - run: npx vitest run __tests__/daemon-install-check.test.ts __tests__/daemon-older-version.test.ts __tests__/cli-version.test.ts __tests__/daemon-registry.test.ts __tests__/daemon-pid-reuse.test.ts __tests__/daemon-lock-refresh.test.ts __tests__/mcp-daemon.test.ts __tests__/daemon-client-liveness.test.ts __tests__/daemon-bind-failure.test.ts __tests__/daemon-socket-fallback.test.ts __tests__/daemon-rendezvous-case.test.ts __tests__/writer-lock.test.ts __tests__/mcp-writer-lock.test.ts __tests__/mcp-projectpath-lifecycle.test.ts __tests__/query-pool.test.ts __tests__/status-index-health.test.ts --maxWorkers=2 --minWorkers=1 + - run: npx vitest run __tests__/daemon-install-check.test.ts __tests__/daemon-older-version.test.ts __tests__/cli-version.test.ts __tests__/daemon-registry.test.ts __tests__/daemon-pid-reuse.test.ts __tests__/daemon-lock-refresh.test.ts __tests__/mcp-daemon.test.ts __tests__/daemon-client-liveness.test.ts __tests__/daemon-bind-failure.test.ts __tests__/daemon-socket-fallback.test.ts __tests__/daemon-rendezvous-case.test.ts __tests__/writer-lock.test.ts __tests__/writer-handover-ordering.test.ts __tests__/mcp-writer-lock.test.ts __tests__/mcp-projectpath-lifecycle.test.ts __tests__/query-pool.test.ts __tests__/status-index-health.test.ts --maxWorkers=2 --minWorkers=1 diff --git a/CHANGELOG.md b/CHANGELOG.md index ed0dc517c9..1759dd2257 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Breaking Changes +- Source builds require Rust 1.89 or newer for native writer coordination. + - **The native engine is now the only parser; the older WebAssembly parser is gone.** Every supported language is built into the native engine, so there is no fallback for a platform without a native build. Release bundles cover macOS (Intel and Apple silicon), Linux (x64 and arm64, glibc) and Windows (x64 and arm64); other platforms need a from-source build with a Rust toolchain, and CodeGraph now says so at startup instead of failing later. Installs are about 40% smaller and the MCP server starts one process lighter. `CODEGRAPH_KERNEL_LANGS` and `CODEGRAPH_NO_RELAUNCH` no longer do anything; `CODEGRAPH_KERNEL=0` now only switches the per-language native walkers off for debugging (every file is still parsed natively). - **Node.js 25 and newer are no longer refused.** The block existed for a V8 bug in compiling the old parser's grammars. Those versions are untested rather than unsupported. - **Node.js 22.13 or Bun 1.4.0 is now the minimum.** Both are the first releases where the built-in `node:sqlite` loads without a flag; older ones failed at the first database open. CodeGraph now says so at startup (`CODEGRAPH_ALLOW_UNSAFE_NODE=1` skips the check). `package.json` declares both in `engines`. @@ -87,6 +89,8 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixes +- Writer handovers, stale-lock cleanup, readiness updates and releases now share an OS file lock, preventing a competing launcher from overwriting a newer writer. The OS releases coordination locks on process exit. Contended retirement retries release while the owner stays alive; legacy daemons without coordinated handover stay running until their old sessions are restarted. + - Python member calls no longer keep a receiver's original type after it is reassigned on a later line. - Source builds now rebuild the native kernel after edits to new, untracked source files instead of using an older prebuild. diff --git a/README.md b/README.md index 5f0acdf72c..5c494744f5 100644 --- a/README.md +++ b/README.md @@ -58,7 +58,7 @@ The fork publishes no releases. The install scripts, npm package, badges and `co ### Install the fork from source -You need Node.js 22.13 or newer (or Bun 1.4.0 or newer), git, and a [Rust toolchain](https://rustup.rs/) for the native kernel. +You need Node.js 22.13 or newer (or Bun 1.4.0 or newer), git, and a [Rust toolchain](https://rustup.rs/) 1.89 or newer for the native kernel. ```bash git clone https://github.com/bompus/codegraph.git @@ -110,7 +110,7 @@ C# property accessors and expression-bodied properties contribute calls and refe `codegraph status` reports files that need re-indexing and files with recorded parse errors. `status --json` includes `index.filesNeedingReindex` and `index.filesWithParseErrors`; `files --json` includes each file's extraction errors. A transient parser failure preserves the previous graph and retries on the next sync. -The MCP launcher can replace a daemon from an older release. A daemon exits when its installation is deleted or its package version changes. Different managed builds of the same release retain the fork's version-identity checks. +The MCP launcher can replace a daemon from an older release when its hello confirms coordinated writer handover. Legacy daemons stay running while new sessions serve reads without auto-sync; stop the old MCP sessions and daemon, then reconnect with the current install. A daemon exits when its installation is deleted or its package version changes. Different managed builds of the same release retain the fork's version-identity checks. Dispatch and framework coverage the fork adds, by kind: @@ -1108,7 +1108,7 @@ Framework routing is validated the same way, on a canonical app per framework: E **MCP server not connecting** — Your agent starts the server itself, so you don't launch it by hand. Make sure the project is initialized and indexed (`codegraph status`) and that the path in your MCP config is correct. If it still won't connect, re-run `codegraph install` to rewrite the config. -**Two `codegraph serve --mcp` on one project fight over the index / auto-sync stops** — CodeGraph allows one live MCP *writer* per project (the shared background daemon, or a single direct-mode process). Extra clients should proxy to that daemon. If you set `CODEGRAPH_NO_DAEMON=1`, run only one `serve --mcp` for that project; a second instance exits with a clear writer-lock error (see `writer.pid` under `.codegraph/`). Prefer leaving the daemon enabled so multiple MCP hosts share one watcher. +**Two `codegraph serve --mcp` on one project fight over the index / auto-sync stops** — CodeGraph allows one live MCP *writer* per project (the shared background daemon, or a single direct-mode process). Extra clients should proxy to that daemon. If you set `CODEGRAPH_NO_DAEMON=1`, run only one `serve --mcp` for that project; a second instance exits with a clear writer-lock error (see `writer.pid` under `.codegraph/`; its persistent `writer.pid.mutation.lock` coordinates ownership changes and needs no stale-lock deletion). Prefer leaving the daemon enabled so multiple MCP hosts share one watcher. **MCP tool calls fail with `Transport closed` while `codegraph status`/`sync` are healthy** — almost always WSL2 with the project on a Windows drive (a `/mnt/c` or `/mnt/d` path), where the local socket CodeGraph uses to share one background server across sessions is unreliable. CodeGraph now falls back to serving the session in-process instead of dropping the connection, and keeps trying to reach the shared server in the background: first after 5 seconds, backing off to every 5 minutes. `CODEGRAPH_DAEMON_RETRY_MS` and `CODEGRAPH_DAEMON_RETRY_MAX_MS` set those two delays, and `CODEGRAPH_DAEMON_RETRY_MS=0` stops retrying. If you still hit it, set `CODEGRAPH_NO_DAEMON=1` in your MCP server's environment to skip the shared server entirely (each session runs in its own process). Moving the project onto the Linux-native filesystem (e.g. under `~/` instead of `/mnt/`) restores the shared server. diff --git a/__tests__/daemon-older-version.test.ts b/__tests__/daemon-older-version.test.ts index 4a9dcdb2bc..ead655dde0 100644 --- a/__tests__/daemon-older-version.test.ts +++ b/__tests__/daemon-older-version.test.ts @@ -150,7 +150,8 @@ describe('swapWriterLock', () => { fs.rmSync(file); expect(swapWriterLock(root, 333, record(444, 'daemon'))).toBe(false); expect(fs.existsSync(file)).toBe(false); - expect(fs.readdirSync(path.join(root, '.codegraph'))).toEqual([]); + // The permanent guard inode remains; no PID record or swap temporary does. + expect(fs.readdirSync(path.join(root, '.codegraph'))).toEqual(['writer.pid.mutation.lock']); }); }); @@ -198,11 +199,11 @@ describe('stopOlderDaemon', () => { }); /** A daemon of `version`: its lock, and a socket answering the hello as `helloPid`. */ - async function fakeDaemon(version: string, helloPid?: number): Promise<{ pid: number; lock: string }> { + async function fakeDaemon(version: string, helloPid?: number, writerProtocol: unknown = 1): Promise<{ pid: number; lock: string }> { const pid = startDetachedProcess(); pids.push(pid); server = net.createServer((socket) => { - socket.end(JSON.stringify({ protocol: 1, codegraph: version, pid: helloPid ?? pid, socketPath }) + '\n'); + socket.end(JSON.stringify({ protocol: 1, codegraph: version, pid: helloPid ?? pid, socketPath, ...(writerProtocol === null ? {} : { writerProtocol }) }) + '\n'); }); await listen(server, socketPath); const lock = encodeLockInfo({ pid, version, socketPath, startedAt: Date.now() }); @@ -220,6 +221,18 @@ describe('stopOlderDaemon', () => { try { return JSON.parse(fs.readFileSync(getWriterPidPath(root), 'utf8')); } catch { return null; } }; + it.each([null, 2])('preserves a legacy writer without verified coordination capability (%s)', async (writerProtocol) => { + const { pid, lock } = await fakeDaemon('1.6.1', undefined, writerProtocol); + const record = { pid, mode: 'daemon', startedAt: 1, ready: false }; + fs.writeFileSync(getWriterPidPath(root), JSON.stringify(record)); + const kill = vi.spyOn(process, 'kill'); + expect(await stopOlderDaemon(root, '1.6.2')).toMatchObject({ pid, outcome: 'legacy-writer' }); + expect(signalsTo(kill, pid)).toEqual([]); + expect(writerRecord()).toEqual(record); + expect(fs.readFileSync(pidPath, 'utf8')).toBe(lock); + expect(isProcessAlive(pid)).toBe(true); + }); + it('stops a verified daemon of an older release and clears its lock', async () => { const { pid } = await fakeDaemon('1.6.1'); const result = await stopOlderDaemon(root, '1.6.2'); @@ -692,7 +705,7 @@ describe('a launcher meeting a daemon of another version (#2335)', () => { const socketDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-sock-')); try { const socketPath = listenPath(socketDir, 'older'); - const older = spawn(process.execPath, [OLDER_DAEMON, realRoot, OLDER, socketPath], { detached: true, stdio: 'ignore' }); + const older = spawn(process.execPath, [OLDER_DAEMON, realRoot, OLDER, socketPath], { detached: true, stdio: 'ignore', env: { ...process.env, CODEGRAPH_TEST_WRITER_MODULE: path.resolve(__dirname, '../dist/mcp/writer-lock.js') } }); older.unref(); daemonPids.add(older.pid!); await waitFor(() => daemonLock()?.pid === older.pid, 10000, 'the older daemon to hold the project'); diff --git a/__tests__/fixtures/older-daemon.cjs b/__tests__/fixtures/older-daemon.cjs index a6535f4d9b..cd162c8e1c 100644 --- a/__tests__/fixtures/older-daemon.cjs +++ b/__tests__/fixtures/older-daemon.cjs @@ -13,20 +13,23 @@ const fs = require('fs'); const net = require('net'); const path = require('path'); +const writer = require(process.env.CODEGRAPH_TEST_WRITER_MODULE); const [root, version, socketPath] = process.argv.slice(2); const dir = path.join(root, '.codegraph'); const writeLock = (name, record) => fs.writeFileSync(path.join(dir, name), JSON.stringify(record) + '\n'); const server = net.createServer((socket) => { - socket.end(JSON.stringify({ codegraph: version, pid: process.pid, socketPath, protocol: 1 }) + '\n'); + socket.end(JSON.stringify({ codegraph: version, pid: process.pid, socketPath, protocol: 1, writerProtocol: 1 }) + '\n'); }); server.listen(socketPath, () => { - writeLock('writer.pid', { pid: process.pid, mode: 'daemon', startedAt: Date.now(), ready: true }); + writer.tryAcquireWriterLock(root, 'daemon'); + writer.markWriterReady(root); writeLock('daemon.pid', { pid: process.pid, version, socketPath, startedAt: Date.now() }); }); process.on('SIGTERM', () => { - for (const name of ['writer.pid', 'daemon.pid']) { + writer.releaseWriterLock(root); + for (const name of ['daemon.pid']) { const file = path.join(dir, name); try { if (JSON.parse(fs.readFileSync(file, 'utf8')).pid === process.pid) fs.unlinkSync(file); diff --git a/__tests__/fixtures/writer-lock-ordering.cjs b/__tests__/fixtures/writer-lock-ordering.cjs new file mode 100644 index 0000000000..0dd9479dda --- /dev/null +++ b/__tests__/fixtures/writer-lock-ordering.cjs @@ -0,0 +1,84 @@ +const fs = require('node:fs'); +const { spawn, spawnSync } = require('node:child_process'); +const writer = require(process.argv[2]); +const root = process.argv[3]; +const operation = process.argv[4]; +const record = (pid, mode = 'daemon') => ({ pid, mode, startedAt: 1, ready: false }); + +if (operation === 'hold') { + const path = require('node:path'); + const kernel = require(path.join(path.dirname(process.argv[2]), '../extraction/kernel/loader.js')).requireKernel(); + const held = kernel.tryWriterMutationLock(`${writer.getWriterPidPath(root)}.mutation.lock`); + if (!held) process.exit(2); + process.send('held'); + process.on('message', () => { held.release(); process.exit(0); }); +} else if (operation === 'retire') { + (async () => { + writer.tryAcquireWriterLock(root, 'direct'); + const holder = spawn(process.execPath, [__filename, process.argv[2], root, 'hold'], { stdio: ['ignore', 'ignore', 'inherit', 'ipc'] }); + try { + await new Promise((resolve, reject) => { + holder.once('message', resolve); + holder.once('error', reject); + holder.once('exit', () => reject(new Error('holder exited before readiness'))); + }); + writer.releaseWriterLock(root); + const exited = new Promise(resolve => holder.once('exit', resolve)); + holder.send('release'); + await exited; + const deadline = Date.now() + 2000; + while (writer.readWriterLock(root) && Date.now() < deadline) await new Promise(resolve => setTimeout(resolve, 20)); + const released = writer.readWriterLock(root) === null; + const contender = spawnSync(process.execPath, [__filename, process.argv[2], root, 'reacquire'], { encoding: 'utf8', timeout: 10000 }); + if (contender.status !== 0) throw new Error(contender.stderr); + process.stdout.write(JSON.stringify({ released, ...JSON.parse(contender.stdout) })); + } finally { + if (holder.exitCode === null) holder.kill('SIGKILL'); + } + })().catch(error => { console.error(error); process.exitCode = 1; }); +} else if (operation === 'crash') { + const path = require('node:path'); + const kernel = require(path.join(path.dirname(process.argv[2]), '../extraction/kernel/loader.js')).requireKernel(); + const held = kernel.tryWriterMutationLock(`${writer.getWriterPidPath(root)}.mutation.lock`); + if (!held) process.exit(2); + process.stdout.write(JSON.stringify({ held: true })); + process.exit(0); // Deliberately bypass release/finalizers. +} else if (operation === 'reacquire') { + const result = writer.tryAcquireWriterLock(root, 'direct'); + process.stdout.write(JSON.stringify({ acquired: result.kind === 'acquired' })); +} else if (operation === 'competitor') { + const from = Number(process.argv[5]); + const claimed = writer.swapWriterLock(root, from, record(process.pid, 'handover')); + const promoted = claimed && writer.swapWriterLock(root, process.pid, record(333)); + process.stdout.write(JSON.stringify({ claimed, promoted })); +} else { + const file = writer.getWriterPidPath(root); + const from = operation === 'acquire' ? 999999999 : process.pid; + fs.writeFileSync(file, JSON.stringify(record(from))); + const boundary = operation === 'release' || operation === 'acquire' ? 'unlinkSync' : 'renameSync'; + const original = fs[boundary]; + let paused = false; + let competitor; + let acquired; + fs[boundary] = (...args) => { + const target = boundary === 'renameSync' ? args[1] : args[0]; + if (target === file && !paused) { + paused = true; + if (operation === 'successor') { + writer.releaseWriterLock(root); + acquired = writer.tryAcquireWriterLock(root, 'fallback').kind; + } else { + const child = spawnSync(process.execPath, [__filename, process.argv[2], root, 'competitor', String(from)], { encoding: 'utf8', timeout: 10000 }); + if (child.status !== 0) throw new Error(child.stderr || `actor exited ${child.status}`); + competitor = JSON.parse(child.stdout); + } + } + return original(...args); + }; + let result; + if (operation === 'swap' || operation === 'successor') result = writer.swapWriterLock(root, from, record(operation === 'successor' ? 333 : 222)); + else if (operation === 'ready') writer.markWriterReady(root); + else if (operation === 'release') writer.releaseWriterLock(root); + else result = writer.tryAcquireWriterLock(root, 'fallback').kind; + process.stdout.write(JSON.stringify({ paused, competitor, acquired, result, info: writer.readWriterLock(root), pid: process.pid })); +} diff --git a/__tests__/writer-handover-ordering.test.ts b/__tests__/writer-handover-ordering.test.ts new file mode 100644 index 0000000000..a1a6e5fffd --- /dev/null +++ b/__tests__/writer-handover-ordering.test.ts @@ -0,0 +1,59 @@ +import { afterEach, describe, expect, it } from 'vitest'; +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +const MODULE = path.resolve(__dirname, '../dist/mcp/writer-lock.js'); +const ACTOR = path.resolve(__dirname, 'fixtures/writer-lock-ordering.cjs'); + +describe('writer handover ordering', () => { + let root: string; + afterEach(() => { + if (root) fs.rmSync(root, { recursive: true, force: true }); + }); + + function run(operation: string) { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-writer-order-')); + fs.mkdirSync(path.join(root, '.codegraph')); + const child = spawnSync(process.execPath, [ACTOR, MODULE, root, operation], { encoding: 'utf8', timeout: 15000 }); + expect(child.status, child.stderr).toBe(0); + return JSON.parse(child.stdout); + } + + it.each(['swap', 'ready', 'release', 'acquire'] as const)( + 'refuses a second handover while %s is between its ownership read and mutation', + (operation) => { + const outcome = run(operation); + expect(outcome.paused, `${operation} barrier was not reached`).toBe(true); + expect(outcome.competitor, `${operation} -> competing handover -> ${operation} mutation`).toEqual({ claimed: false, promoted: false }); + if (operation === 'swap') expect(outcome.info).toMatchObject({ pid: 222, mode: 'daemon' }); + else if (operation === 'ready') expect(outcome.info).toMatchObject({ pid: outcome.pid, ready: true }); + else if (operation === 'release') expect(outcome.info).toBeNull(); + else expect(outcome.info).toMatchObject({ pid: outcome.pid, mode: 'fallback' }); + }, + ); + + it('eventually releases a retired writer after coordination contention', () => { + const outcome = run('retire'); + expect(outcome.released).toBe(true); + expect(outcome.acquired).toBe(true); + }); + + it('releases the coordination lock when its process exits without cleanup', () => { + const outcome = run('crash'); + expect(outcome).toEqual({ held: true }); + const child = spawnSync(process.execPath, [ACTOR, MODULE, root, 'reacquire'], { encoding: 'utf8', timeout: 15000 }); + expect(child.status, child.stderr).toBe(0); + expect(JSON.parse(child.stdout)).toEqual({ acquired: true }); + expect(fs.existsSync(path.join(root, '.codegraph/writer.pid.mutation.lock'))).toBe(true); + }); + + it('keeps release and fallback acquisition from invalidating a pending successor', () => { + const outcome = run('successor'); + expect(outcome.result).toBe(true); + expect(outcome.paused).toBe(true); + expect(outcome.acquired, 'successor read -> launcher release -> fallback acquire -> successor rename').toBe('taken'); + expect(outcome.info).toMatchObject({ pid: 333, mode: 'daemon' }); + }); +}); diff --git a/codegraph-kernel/Cargo.toml b/codegraph-kernel/Cargo.toml index da7a9fdb88..e8d7472a10 100644 --- a/codegraph-kernel/Cargo.toml +++ b/codegraph-kernel/Cargo.toml @@ -2,6 +2,7 @@ name = "codegraph-kernel" version = "0.1.0" edition = "2021" +rust-version = "1.89" license = "MIT" publish = false description = "Native extraction kernel for CodeGraph — tree-sitter parse+extract with one JS boundary crossing per file" diff --git a/codegraph-kernel/src/lib.rs b/codegraph-kernel/src/lib.rs index a076d903b3..0d091be22c 100644 --- a/codegraph-kernel/src/lib.rs +++ b/codegraph-kernel/src/lib.rs @@ -395,6 +395,7 @@ mod ruby; mod rustlang; mod scala; mod contained_source; +mod writer_lock; mod stack; mod swift; mod textutil; diff --git a/codegraph-kernel/src/writer_lock.rs b/codegraph-kernel/src/writer_lock.rs new file mode 100644 index 0000000000..50f9c8dc7d --- /dev/null +++ b/codegraph-kernel/src/writer_lock.rs @@ -0,0 +1,33 @@ +use std::fs::{File, OpenOptions}; + +/// Keep this file in place: replacing its inode would split the lock domain. +#[napi_derive::napi] +pub struct WriterMutationLock { + file: Option, +} + +#[napi_derive::napi] +impl WriterMutationLock { + #[napi] + pub fn release(&mut self) { + self.file.take(); + } +} + +/// OS locks release on close or process exit, so a crashed writer cannot +/// leave a stale coordination lock that another process must delete. +#[napi_derive::napi] +pub fn try_writer_mutation_lock(path: String) -> napi::Result> { + let file = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .open(path) + .map_err(|e| napi::Error::from_reason(e.to_string()))?; + match file.try_lock() { + Ok(()) => Ok(Some(WriterMutationLock { file: Some(file) })), + Err(std::fs::TryLockError::WouldBlock) => Ok(None), + Err(std::fs::TryLockError::Error(e)) => Err(napi::Error::from_reason(e.to_string())), + } +} diff --git a/site/src/content/docs/reference/mcp-server.md b/site/src/content/docs/reference/mcp-server.md index 00a881589a..5cda110fd0 100644 --- a/site/src/content/docs/reference/mcp-server.md +++ b/site/src/content/docs/reference/mcp-server.md @@ -29,10 +29,15 @@ The scan checks its 300 ms budget between files and stops at sixteen matches or When you ask about tests, `codegraph_explore` includes source from the nearest test callers within three caller hops. These links describe static callers, not measured runtime coverage. Pinned JavaScript and TypeScript test files prioritize matching `it`/`test` callbacks. File and output budgets still apply. -## The other tools +## Daemon lifecycle + +Writer handovers, readiness updates and releases share an OS file lock. The persistent `.codegraph/writer.pid.mutation.lock` file stays in place; the OS releases its lock when the process exits. -The launcher can replace a daemon from an older release. A daemon exits when its installation is deleted or its package version changes. Managed builds of the same release still require matching build identities. +The launcher can replace a daemon from an older release when its hello confirms coordinated writer handover. Legacy daemons stay running; new sessions serve reads without auto-sync. Stop the old MCP sessions and daemon, then reconnect with the current install to migrate. A daemon exits when its installation is deleted or its package version changes. Managed builds of the same release still require matching build identities. +`CODEGRAPH_DAEMON_INSTALL_CHECK_MS` sets the installation-check interval in milliseconds. It defaults to `30000`; `0` disables the check. + +## The other tools Seven more tools exist and stay fully functional, but are **unlisted by default** — everything they return already arrives inline on a `codegraph_explore` response (its blast-radius section, the relationship map, a symbol's body and its callee list): diff --git a/src/extraction/kernel/loader.ts b/src/extraction/kernel/loader.ts index c0f54917d2..9b22a72bfe 100644 --- a/src/extraction/kernel/loader.ts +++ b/src/extraction/kernel/loader.ts @@ -290,6 +290,8 @@ export interface ContainedSourceReaderLike { } export interface KernelModule { + /** Required for writer mutations; older kernels remain usable for reads. */ + tryWriterMutationLock?(path: string): { release(): void } | null; /** Descriptor-verified source reads. Older released kernels omit this API; * scans then leave absence unproven. */ ContainedSourceReader?: new (root: string) => ContainedSourceReaderLike; diff --git a/src/mcp/daemon-paths.ts b/src/mcp/daemon-paths.ts index 8f78d5063e..494364a4fe 100644 --- a/src/mcp/daemon-paths.ts +++ b/src/mcp/daemon-paths.ts @@ -123,7 +123,7 @@ export function canProbeDaemonIdentity(info: DaemonLockInfo): boolean { * its socket. A bare PID liveness probe is insufficient because OSes reuse PIDs * after an OOM/SIGKILL (#1553). */ -export function probeDaemonIdentity(info: DaemonLockInfo, timeoutMs = 1_000): Promise { +export function probeDaemonIdentity(info: DaemonLockInfo, timeoutMs = 1_000, requireWriterProtocol = false): Promise { if (!canProbeDaemonIdentity(info)) return Promise.resolve(false); return new Promise((resolve) => { let socket: net.Socket; @@ -156,7 +156,8 @@ export function probeDaemonIdentity(info: DaemonLockInfo, timeoutMs = 1_000): Pr finish( hello.protocol === 1 && hello.pid === info.pid && - (info.version === 'unknown' || hello.codegraph === info.version) + (info.version === 'unknown' || hello.codegraph === info.version) && + (!requireWriterProtocol || hello.writerProtocol === 1) ); } catch { finish(false); diff --git a/src/mcp/daemon-registry.ts b/src/mcp/daemon-registry.ts index a9e5860116..e9f3ba681f 100644 --- a/src/mcp/daemon-registry.ts +++ b/src/mcp/daemon-registry.ts @@ -251,7 +251,7 @@ export interface StopResult { root: string; pid: number | null; /** 'term' graceful, 'kill' force, 'still-running' refused, 'not-running' stale, 'no-daemon' absent, 'unverified' preserved. */ - outcome: 'term' | 'kill' | 'still-running' | 'not-running' | 'no-daemon' | 'unverified'; + outcome: 'term' | 'kill' | 'still-running' | 'not-running' | 'no-daemon' | 'unverified' | 'legacy-writer'; /** The daemon's version as its lock recorded it, when the stop got that far. */ version?: string; } @@ -409,6 +409,11 @@ export async function stopOlderDaemon( if (!canProbeDaemonIdentity(identity) || !await probeDaemonIdentity(identity)) { return { root, pid, outcome: 'unverified', version: identity.version }; } + // A legacy daemon can still publish readiness or release after our claim; + // its unfenced mutations cannot be serialized by the new OS lock. + if (!await probeDaemonIdentity(identity, undefined, true)) { + return { root, pid, outcome: 'legacy-writer', version: identity.version }; + } const slot = takeWriterSlotFrom(root, pid); if (!slot) return null; const result = await stopVerifiedDaemon(root, identity, lockContents, options.shutdownGraceMs); diff --git a/src/mcp/daemon.ts b/src/mcp/daemon.ts index 4268ea3963..6618887907 100644 --- a/src/mcp/daemon.ts +++ b/src/mcp/daemon.ts @@ -172,6 +172,7 @@ export interface DaemonHello { pid: number; // daemon pid (informational; for `ps` debugging) socketPath: string; // echoed back so the proxy can log it protocol: 1; // bump if the hello shape changes + writerProtocol?: 1; // ownership mutations use the OS coordination lock } /** @@ -453,6 +454,7 @@ export class Daemon { pid: process.pid, socketPath: this.socketPath, protocol: 1, + writerProtocol: 1, }; socket.write(JSON.stringify(hello) + '\n'); diff --git a/src/mcp/index.ts b/src/mcp/index.ts index cffc4eb5be..1f0653c9e7 100644 --- a/src/mcp/index.ts +++ b/src/mcp/index.ts @@ -363,7 +363,9 @@ async function replaceOlderDaemon(root: string, answered: boolean): Promise<'sto ); return 'stopped'; } - if (result.outcome === 'still-running') { + if (result.outcome === 'legacy-writer') { + process.stderr.write(`[CodeGraph MCP] Left ${daemon} running because it lacks coordinated writer handover; serving reads without auto-sync. Stop its old MCP sessions and daemon, then reconnect with this install.\n`); + } else if (result.outcome === 'still-running') { process.stderr.write(`[CodeGraph MCP] Asked ${daemon} serving this project to stop, but it has not exited yet.\n`); } else if (answered) { process.stderr.write( diff --git a/src/mcp/writer-lock.ts b/src/mcp/writer-lock.ts index 6b8e0de25c..8678a2cdb2 100644 --- a/src/mcp/writer-lock.ts +++ b/src/mcp/writer-lock.ts @@ -16,6 +16,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { getCodeGraphDir } from '../directory'; +import { requireKernel } from '../extraction/kernel/loader'; /** Signal-0 liveness (EPERM ⇒ alive). Local copy to avoid a daemon↔writer cycle. */ function isProcessAlive(pid: number): boolean { try { @@ -58,6 +59,28 @@ export type WriterAcquireResult = | { kind: 'acquired'; pidPath: string; info: WriterLockInfo } | { kind: 'taken'; existing: WriterLockInfo | null; pidPath: string }; +const pendingReleases = new Map(); + +function cancelPendingRelease(pidPath: string): void { + const timer = pendingReleases.get(pidPath); + if (timer) clearTimeout(timer); + pendingReleases.delete(pidPath); +} + +/** Serialize every ownership comparison and mutation, including stale cleanup. + * Keep the guard file permanently; the OS releases its lock on process exit. */ +function lockWriterMutation(pidPath: string, waitMs = 1000): { release(): void } | null { + const acquire = requireKernel().tryWriterMutationLock; + if (!acquire) throw new Error('Native kernel lacks writer coordination; rebuild or reinstall CodeGraph.'); + const deadline = Date.now() + waitMs; + for (;;) { + const lock = acquire(`${pidPath}.mutation.lock`); + if (lock) return lock; + if (Date.now() >= deadline) return null; + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 5); + } +} + function encode(info: WriterLockInfo): string { return JSON.stringify(info) + '\n'; } @@ -89,87 +112,104 @@ export function tryAcquireWriterLock( const pidPath = getWriterPidPath(projectRoot, lockName); fs.mkdirSync(path.dirname(pidPath), { recursive: true }); - const info: WriterLockInfo = { - pid: process.pid, - mode, - startedAt: Date.now(), - ready: false, - }; + const mutation = lockWriterMutation(pidPath); + if (!mutation) return { kind: 'taken', existing: readWriterLock(projectRoot, lockName), pidPath }; + try { + const info: WriterLockInfo = { + pid: process.pid, + mode, + startedAt: Date.now(), + ready: false, + }; - const attempt = (): WriterAcquireResult => { - const tmp = `${pidPath}.${process.pid}.tmp`; - let acquired = false; - try { - fs.writeFileSync(tmp, encode(info), { mode: 0o600 }); + const attempt = (): WriterAcquireResult => { + const tmp = `${pidPath}.${process.pid}.tmp`; + let acquired = false; try { - fs.linkSync(tmp, pidPath); - acquired = true; - } catch (err: unknown) { - if ((err as NodeJS.ErrnoException).code === 'EEXIST') { - // taken - } else { - // No hard links — O_EXCL create. - try { - const fd = fs.openSync(pidPath, 'wx', 0o600); + fs.writeFileSync(tmp, encode(info), { mode: 0o600 }); + try { + fs.linkSync(tmp, pidPath); + acquired = true; + } catch (err: unknown) { + if ((err as NodeJS.ErrnoException).code === 'EEXIST') { + // taken + } else { + // No hard links — O_EXCL create. try { - fs.writeSync(fd, encode(info)); - acquired = true; - } finally { - fs.closeSync(fd); + const fd = fs.openSync(pidPath, 'wx', 0o600); + try { + fs.writeSync(fd, encode(info)); + acquired = true; + } finally { + fs.closeSync(fd); + } + } catch (e2: unknown) { + if ((e2 as NodeJS.ErrnoException).code !== 'EEXIST') throw e2; } - } catch (e2: unknown) { - if ((e2 as NodeJS.ErrnoException).code !== 'EEXIST') throw e2; } } + } finally { + try { fs.unlinkSync(tmp); } catch { /* ignore */ } } - } finally { - try { fs.unlinkSync(tmp); } catch { /* ignore */ } - } - - if (acquired) return { kind: 'acquired', pidPath, info }; - let existing: WriterLockInfo | null = null; - try { - existing = decodeWriterLockInfo(fs.readFileSync(pidPath, 'utf8')); - } catch { /* unreadable */ } - return { kind: 'taken', existing, pidPath }; - }; + if (acquired) { + cancelPendingRelease(pidPath); + return { kind: 'acquired', pidPath, info }; + } - let result = attempt(); - if (result.kind === 'taken' && result.existing && result.existing.pid === process.pid) { - // Same process already holds it (daemon acquired before engine watch). - return { kind: 'acquired', pidPath: result.pidPath, info: result.existing }; - } - if (result.kind === 'taken') { - const existing = result.existing; - if (!existing || existing.pid <= 0 || !isProcessAlive(existing.pid)) { - // Stale — clear (pid-verified) and retry once. + let existing: WriterLockInfo | null = null; try { - const raw = fs.readFileSync(pidPath, 'utf8'); - const cur = decodeWriterLockInfo(raw); - if (!cur || cur.pid === existing?.pid) { - if (!cur || cur.pid <= 0 || !isProcessAlive(cur.pid)) { - fs.unlinkSync(pidPath); + existing = decodeWriterLockInfo(fs.readFileSync(pidPath, 'utf8')); + } catch { /* unreadable */ } + return { kind: 'taken', existing, pidPath }; + }; + + let result = attempt(); + if (result.kind === 'taken' && result.existing && result.existing.pid === process.pid) { + // Same process already holds it (daemon acquired before engine watch). + cancelPendingRelease(pidPath); + return { kind: 'acquired', pidPath: result.pidPath, info: result.existing }; + } + if (result.kind === 'taken') { + const existing = result.existing; + if (!existing || existing.pid <= 0 || !isProcessAlive(existing.pid)) { + // Stale — clear (pid-verified) and retry once. + try { + const raw = fs.readFileSync(pidPath, 'utf8'); + const cur = decodeWriterLockInfo(raw); + if (!cur || cur.pid === existing?.pid) { + if (!cur || cur.pid <= 0 || !isProcessAlive(cur.pid)) { + fs.unlinkSync(pidPath); + } } - } - } catch { /* ENOENT ok */ } - result = attempt(); + } catch { /* ENOENT ok */ } + result = attempt(); + } } + return result; + } finally { + mutation.release(); } - return result; } /** Publish catch-up readiness without exposing a partially-written pidfile. */ export function markWriterReady(projectRoot: string): void { const pidPath = getWriterPidPath(projectRoot); - const info = readWriterLock(projectRoot); - if (!info || info.pid !== process.pid) return; - const tmp = `${pidPath}.${process.pid}.ready.tmp`; + if (!fs.existsSync(pidPath)) return; + const mutation = lockWriterMutation(pidPath); + if (!mutation) return; try { - fs.writeFileSync(tmp, encode({ ...info, ready: true }), { mode: 0o600 }); - fs.renameSync(tmp, pidPath); + const info = readWriterLock(projectRoot); + if (!info || info.pid !== process.pid) return; + const tmp = `${pidPath}.${process.pid}.ready.tmp`; + try { + fs.writeFileSync(tmp, encode({ ...info, ready: true }), { mode: 0o600 }); + fs.renameSync(tmp, pidPath); + } finally { + try { fs.unlinkSync(tmp); } catch { /* best-effort */ } + } } finally { - try { fs.unlinkSync(tmp); } catch { /* best-effort */ } + mutation.release(); } } @@ -181,18 +221,22 @@ const SWAP_RETRY_DELAYS_MS = [25, 50, 100, 200]; * the slot passes between two processes without ever falling free (#2335). A * stopped daemon's sessions serve themselves in-process the moment it goes, and * one that found the slot free or stale would claim it as their own writer. - * Compare and rename are separate steps, so callers check the result: whether - * the record was replaced. Never throws. + * The OS coordination lock keeps comparison and rename in one critical + * section. Callers check whether the record was replaced. Never throws. */ export function swapWriterLock(projectRoot: string, fromPid: number, next: WriterLockInfo): boolean { const pidPath = getWriterPidPath(projectRoot); const tmp = `${pidPath}.${process.pid}.swap.tmp`; + let mutation: { release(): void } | null = null; try { + mutation = lockWriterMutation(pidPath); + if (!mutation) return false; if (readWriterLock(projectRoot)?.pid !== fromPid) return false; fs.writeFileSync(tmp, encode(next), { mode: 0o600 }); for (let attempt = 0; ; attempt++) { try { fs.renameSync(tmp, pidPath); + if (next.pid === process.pid) cancelPendingRelease(pidPath); return true; } catch (err) { const code = (err as NodeJS.ErrnoException).code; @@ -207,20 +251,43 @@ export function swapWriterLock(projectRoot: string, fromPid: number, next: Write } catch { return false; } finally { - try { fs.unlinkSync(tmp); } catch { /* renamed, or never written */ } + if (mutation) { + try { fs.unlinkSync(tmp); } catch { /* renamed, or never written */ } + } + mutation?.release(); } } -/** Release if we still own the lock (pid match). */ +/** Release if we still own the lock. Contention keeps a retry obligation; + * callers may retire the engine while this process remains alive. */ export function releaseWriterLock(projectRoot: string, lockName: 'writer.pid' | 'rebuild.pid' = 'writer.pid'): void { const pidPath = getWriterPidPath(projectRoot, lockName); - try { - if (!fs.existsSync(pidPath)) return; - const info = decodeWriterLockInfo(fs.readFileSync(pidPath, 'utf8')); - if (info && info.pid === process.pid) { - fs.unlinkSync(pidPath); + const release = (waitMs: number): void => { + let mutation: { release(): void } | null = null; + try { + if (!fs.existsSync(path.dirname(pidPath))) return; + mutation = lockWriterMutation(pidPath, waitMs); + if (!mutation) { + // One unref'd retry per record. Reacquisition by this process cancels it, + // so an old retirement cannot release a newly started watcher. + if (!pendingReleases.has(pidPath)) { + const timer = setTimeout(() => { + pendingReleases.delete(pidPath); + release(0); + }, 100); + timer.unref(); + pendingReleases.set(pidPath, timer); + } + return; + } + cancelPendingRelease(pidPath); + const info = readWriterLock(projectRoot, lockName); + if (info?.pid === process.pid) fs.unlinkSync(pidPath); + } catch { /* best-effort for unavailable files or kernel */ } finally { + mutation?.release(); } - } catch { /* best-effort */ } + }; + release(1000); } /** Read current lock without acquiring. */ From e6404ad6edfe6883cd594124152318ea1f26864c Mon Sep 17 00:00:00 2001 From: Aaron Queen Date: Mon, 5 Oct 2026 11:26:35 -0600 Subject: [PATCH 5/5] fix(mcp): retain readiness publication through contention --- __tests__/fixtures/writer-lock-ordering.cjs | 33 +++++++++- __tests__/writer-handover-ordering.test.ts | 21 +++++++ src/mcp/writer-lock.ts | 69 ++++++++++++++++----- 3 files changed, 107 insertions(+), 16 deletions(-) diff --git a/__tests__/fixtures/writer-lock-ordering.cjs b/__tests__/fixtures/writer-lock-ordering.cjs index 0dd9479dda..df12d44378 100644 --- a/__tests__/fixtures/writer-lock-ordering.cjs +++ b/__tests__/fixtures/writer-lock-ordering.cjs @@ -12,7 +12,7 @@ if (operation === 'hold') { if (!held) process.exit(2); process.send('held'); process.on('message', () => { held.release(); process.exit(0); }); -} else if (operation === 'retire') { +} else if (operation === 'retire' || operation.startsWith('contended-ready')) { (async () => { writer.tryAcquireWriterLock(root, 'direct'); const holder = spawn(process.execPath, [__filename, process.argv[2], root, 'hold'], { stdio: ['ignore', 'ignore', 'inherit', 'ipc'] }); @@ -22,10 +22,39 @@ if (operation === 'hold') { holder.once('error', reject); holder.once('exit', () => reject(new Error('holder exited before readiness'))); }); - writer.releaseWriterLock(root); + let failedRenames = 0; + if (operation === 'contended-ready-io') { + const rename = fs.renameSync; + fs.renameSync = (...args) => { + if (String(args[0]).endsWith('.ready.tmp') && failedRenames++ === 0) { + const error = new Error('writer pid file temporarily held by a reader'); + error.code = 'EPERM'; + throw error; + } + return rename(...args); + }; + } + if (operation === 'retire') writer.releaseWriterLock(root); + else writer.markWriterReady(root); const exited = new Promise(resolve => holder.once('exit', resolve)); holder.send('release'); await exited; + if (operation.startsWith('contended-ready')) { + if (operation === 'contended-ready-retire') writer.releaseWriterLock(root); + else if (operation === 'contended-ready-renew') { + writer.releaseWriterLock(root); + writer.tryAcquireWriterLock(root, 'fallback'); + } else if (operation === 'contended-ready-transfer') { + writer.swapWriterLock(root, process.pid, record(333)); + } + // Drive queued readiness work without sleeping while the guard is held. + const deadline = Date.now() + 2000; + while (Date.now() < deadline && !writer.readWriterLock(root)?.ready) { + await new Promise(resolve => setTimeout(resolve, 20)); + } + process.stdout.write(JSON.stringify({ info: writer.readWriterLock(root), pid: process.pid, failedRenames })); + return; + } const deadline = Date.now() + 2000; while (writer.readWriterLock(root) && Date.now() < deadline) await new Promise(resolve => setTimeout(resolve, 20)); const released = writer.readWriterLock(root) === null; diff --git a/__tests__/writer-handover-ordering.test.ts b/__tests__/writer-handover-ordering.test.ts index a1a6e5fffd..7178888c32 100644 --- a/__tests__/writer-handover-ordering.test.ts +++ b/__tests__/writer-handover-ordering.test.ts @@ -34,6 +34,27 @@ describe('writer handover ordering', () => { }, ); + it('eventually publishes readiness after coordination contention', () => { + const outcome = run('contended-ready'); + expect(outcome.info).toMatchObject({ pid: outcome.pid, ready: true }); + }); + + it('survives a transient readiness rename error and retries publication', () => { + const outcome = run('contended-ready-io'); + expect(outcome.failedRenames).toBeGreaterThan(1); + expect(outcome.info).toMatchObject({ pid: outcome.pid, ready: true }); + }); + + it.each(['retire', 'renew', 'transfer'] as const)( + 'does not publish queued readiness after %s changes ownership', + (operation) => { + const outcome = run(`contended-ready-${operation}`); + if (operation === 'retire') expect(outcome.info).toBeNull(); + else if (operation === 'renew') expect(outcome.info).toMatchObject({ pid: outcome.pid, mode: 'fallback', ready: false }); + else expect(outcome.info).toMatchObject({ pid: 333, ready: false }); + }, + ); + it('eventually releases a retired writer after coordination contention', () => { const outcome = run('retire'); expect(outcome.released).toBe(true); diff --git a/src/mcp/writer-lock.ts b/src/mcp/writer-lock.ts index 8678a2cdb2..0e15880301 100644 --- a/src/mcp/writer-lock.ts +++ b/src/mcp/writer-lock.ts @@ -60,6 +60,13 @@ export type WriterAcquireResult = | { kind: 'taken'; existing: WriterLockInfo | null; pidPath: string }; const pendingReleases = new Map(); +const pendingReadiness = new Map(); + +function cancelPendingReadiness(pidPath: string): void { + const timer = pendingReadiness.get(pidPath); + if (timer) clearTimeout(timer); + pendingReadiness.delete(pidPath); +} function cancelPendingRelease(pidPath: string): void { const timer = pendingReleases.get(pidPath); @@ -153,6 +160,7 @@ export function tryAcquireWriterLock( } if (acquired) { + cancelPendingReadiness(pidPath); cancelPendingRelease(pidPath); return { kind: 'acquired', pidPath, info }; } @@ -192,25 +200,56 @@ export function tryAcquireWriterLock( } } -/** Publish catch-up readiness without exposing a partially-written pidfile. */ +/** Publish catch-up readiness without exposing a partially-written pidfile. + * Contention retains a retry until publication or an ownership change. */ export function markWriterReady(projectRoot: string): void { const pidPath = getWriterPidPath(projectRoot); - if (!fs.existsSync(pidPath)) return; - const mutation = lockWriterMutation(pidPath); - if (!mutation) return; - try { - const info = readWriterLock(projectRoot); - if (!info || info.pid !== process.pid) return; - const tmp = `${pidPath}.${process.pid}.ready.tmp`; + const retry = (): void => { + if (pendingReadiness.has(pidPath)) return; + const timer = setTimeout(() => { + pendingReadiness.delete(pidPath); + try { + publish(0); + } catch (error) { + // Permanent failures stay local to publication, like the lifecycle's + // synchronous catch boundary; an unguarded timer would exit the server. + process.stderr.write(`[CodeGraph MCP] Writer readiness publication failed: ${error instanceof Error ? error.message : String(error)}\n`); + } + }, 100); + timer.unref(); + pendingReadiness.set(pidPath, timer); + }; + const publish = (waitMs: number): void => { + let mutation: { release(): void } | null = null; try { - fs.writeFileSync(tmp, encode({ ...info, ready: true }), { mode: 0o600 }); - fs.renameSync(tmp, pidPath); + if (!fs.existsSync(pidPath)) { + cancelPendingReadiness(pidPath); + return; + } + mutation = lockWriterMutation(pidPath, waitMs); + if (!mutation) { + retry(); + return; + } + cancelPendingReadiness(pidPath); + const info = readWriterLock(projectRoot); + if (!info || info.pid !== process.pid) return; + const tmp = `${pidPath}.${process.pid}.ready.tmp`; + try { + fs.writeFileSync(tmp, encode({ ...info, ready: true }), { mode: 0o600 }); + fs.renameSync(tmp, pidPath); + } finally { + try { fs.unlinkSync(tmp); } catch { /* best-effort */ } + } + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (['EPERM', 'EACCES', 'EBUSY', 'EINTR', 'EAGAIN'].includes(code ?? '')) retry(); + else throw error; } finally { - try { fs.unlinkSync(tmp); } catch { /* best-effort */ } + mutation?.release(); } - } finally { - mutation.release(); - } + }; + publish(1000); } /** Waits between retries of a writer-record swap refused by Windows (another process has the file open). */ @@ -236,6 +275,7 @@ export function swapWriterLock(projectRoot: string, fromPid: number, next: Write for (let attempt = 0; ; attempt++) { try { fs.renameSync(tmp, pidPath); + cancelPendingReadiness(pidPath); if (next.pid === process.pid) cancelPendingRelease(pidPath); return true; } catch (err) { @@ -262,6 +302,7 @@ export function swapWriterLock(projectRoot: string, fromPid: number, next: Write * callers may retire the engine while this process remains alive. */ export function releaseWriterLock(projectRoot: string, lockName: 'writer.pid' | 'rebuild.pid' = 'writer.pid'): void { const pidPath = getWriterPidPath(projectRoot, lockName); + cancelPendingReadiness(pidPath); const release = (waitMs: number): void => { let mutation: { release(): void } | null = null; try {