From 41d330e3c69315a25cd35ef48e154bcdbcd547a6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 22:20:06 +0800 Subject: [PATCH] fix: take the analysis lock in devmap analyze devmap analyze now holds .devmap/analyze.lock, so a terminal analysis and an MCP refresh cannot write the snapshot at the same time. The lock code moves to cache/analysisLock.ts and is re-exported from mcp/refresh.ts. The lock is taken in analyzeCommand, not runAnalyze, because the MCP refresher already holds it when it calls runAnalyze. A missing target folder still fails without creating a .devmap folder. --- CHANGELOG.md | 6 ++ CLAUDE.md | 10 +-- docs/architecture.md | 4 +- docs/commands.md | 4 +- docs/generated-files.md | 6 +- docs/guide/commands/09-mcp.md | 7 +- docs/mcp.md | 4 +- packages/cli/src/cache/analysisLock.ts | 77 ++++++++++++++++++++ packages/cli/src/commands/analyze.ts | 27 ++++++- packages/cli/src/mcp/refresh.ts | 81 +-------------------- packages/cli/test/analyze-lock.test.ts | 99 ++++++++++++++++++++++++++ 11 files changed, 229 insertions(+), 96 deletions(-) create mode 100644 packages/cli/src/cache/analysisLock.ts create mode 100644 packages/cli/test/analyze-lock.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index eb9f19d..34a9bdc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to DevMap are documented in this file. ## [Unreleased] +### Fixed + +- `devmap analyze` takes `.devmap/analyze.lock`, like the MCP server does, so a terminal analysis + and an MCP refresh can no longer write the snapshot at the same time. A second analysis fails with + "Another DevMap analysis is already running" instead of racing + ### Planned - Go, Java, C# and PHP (PSR-4) import resolution, after JS/TS is mature diff --git a/CLAUDE.md b/CLAUDE.md index 9030e59..caf2346 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,7 +29,7 @@ devmap/ │ │ ├── mcp/ ← MCP server: tools, resources, prompts, snapshot provider, refresher + lock │ │ ├── onboarding/ ← Onboarding model and builder │ │ └── utils/ ← Config, errors, output, prompts, markdown renderer -│ ├── test/ ← 48 test files (`*.test.ts`, auto-discovered by `run-tests.ts`) + `mcp-smoke.mjs`, `package-e2e.mjs` +│ ├── test/ ← 49 test files (`*.test.ts`, auto-discovered by `run-tests.ts`) + `mcp-smoke.mjs`, `package-e2e.mjs` │ └── package.json ├── apps/web/ ← Astro marketing site (unrelated to CLI) ├── docs/ ← Public documentation @@ -76,7 +76,7 @@ Everything else (files, graph, routes, features, flows, critical files) is 100% | MCP server | `src/mcp/server.ts` | `createDevmapServer()` — tools (`devmap_overview/map/flow/routes/impact/find/analyze`, `devmap_explain` only with `--allow-ai`), resources, prompts | | MCP queries | `src/mcp/queries.ts` | Pure read-only queries over a `ProjectMap` (overview, routes, impact, find) | | MCP snapshot provider | `src/mcp/snapshotProvider.ts` | mtime-cached snapshot, TTL-cached staleness, opt-in auto-refresh | -| MCP refresher | `src/mcp/refresh.ts` | Single-flight analysis + `.devmap/analyze.lock` | +| MCP refresher | `src/mcp/refresh.ts` | Single-flight analysis; holds `.devmap/analyze.lock` (lock code in `src/cache/analysisLock.ts`) | ## Data Model @@ -129,12 +129,12 @@ Order for a non-relative specifier: `#subpath` (package.json `imports`) → tsco ## Testing -48 test files in `packages/cli/test/`. Key covered areas: +49 test files in `packages/cli/test/`. Key covered areas: - Analyzers, AI clients, agent navigation, entity extractors - Dependency graph: aliases, workspace packages, `#imports`, Nuxt auto-imports/aliases, entry points (`graph-resolution`, `dependency-graph-alias`, `js-ts-ecosystem`, `js-ts-frameworks`) - Feature similarity/merge, frontend features, framework routes (`framework-routes`; Remix/React Router/TanStack/Expo/Hono and bundler aliases in `js-ts-frameworks`; client-side routers, Hono factories, Next normalization and critical-file ranking in `js-ts-gaps`; SolidStart, Qwik City, Angular, Elysia, Koa, AdonisJS and constant-resolved paths in `js-ts-more-frameworks`), detector fixes - Heuristic language readers (`heuristic-languages`), scanner robustness, project scope/metadata -- Commands (init, analyze, map, flow, explain, onboarding, config, doctor) +- Commands (init, analyze, map, flow, explain, onboarding, config, doctor); `analyze-lock` covers `devmap analyze` holding `.devmap/analyze.lock` - MCP server (`core-api`, `mcp-server`, `mcp-freshness`, `mcp-explain`): tools, resources, prompts, stale/missing snapshots, auto-refresh and lock, offline-never-touches-AI, stdio process with a clean protocol stream. `test/mcp-smoke.mjs` runs the compiled `dist/` as a server (CI and publish workflow) - Utils (markdown terminal, JSON output, prompts, errors) @@ -174,6 +174,7 @@ MCP server (after `pnpm build:cli`): `node packages/cli/test/mcp-smoke.mjs`. - Domain inference is cached for idempotency (`.devmap/domain-cache.json`) - Snapshot is regenerated on every `devmap analyze` run - `AGENTS.md` is never overwritten by init (append-only) +- `analyzeCommand` (CLI) holds `.devmap/analyze.lock` around `runAnalyze`; `runAnalyze` itself never takes it, because the MCP refresher already holds the lock when it calls it - `.devmap/` is always gitignored - `devmap mcp` owns stdout: it is the JSON-RPC stream. Never `console.log` from code the MCP server can reach; use `output.*` inside `withJsonOutput()` (silent) or stderr - The MCP server is read-only and static by default. Only `devmap_analyze` / `--auto-refresh` write, and only inside `.devmap/`. AI is reachable only with `--allow-ai`; `devmap_explain` is registered only then @@ -189,7 +190,6 @@ MCP server (after `pnpm build:cli`): `node packages/cli/test/mcp-smoke.mjs`. - Elysia, Koa and Adonis detectors follow literal call chains and mounts; routers built inside factory functions, `.use()` of computed values and runtime-assembled paths are not followed. Angular reads literal `Routes` arrays and `loadChildren` imports - Feature similarity threshold (0.35) is tuned but may cause false merges - Domain inference is single-shot — no conversation context -- `.devmap/analyze.lock` is taken by MCP-driven analysis only; `devmap analyze` does not take or check it - The MCP staleness check re-scans project files (cached for 10 s), so it is not free on very large repositories ## Engineering Notes diff --git a/docs/architecture.md b/docs/architecture.md index f9b5683..a86af26 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -564,8 +564,8 @@ Consequences for the architecture: snapshot can be refreshed on request (`devmap_analyze`) or automatically (`--auto-refresh`). - **Concurrency now exists.** Two agents, or an agent and a terminal, can ask for - analysis at once. In-process calls are single-flight; MCP-driven analysis also - takes `.devmap/analyze.lock`. The CLI `analyze` does not take the lock yet. + analysis at once. In-process calls are single-flight; MCP-driven analysis and + the CLI `analyze` both take `.devmap/analyze.lock`. Deferred: a remote (HTTP) transport, serving several projects from one server, and watching files for changes instead of checking staleness per request. diff --git a/docs/commands.md b/docs/commands.md index 99b659f..1b026a5 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -482,8 +482,8 @@ Cursor, opencode and Codex. * Static by default: no AI provider is contacted, and the AI config is not read, unless `--allow-ai` is set * `--auto-refresh` is off by default: a question never writes to disk -* Analysis takes `.devmap/analyze.lock` so two servers do not write the snapshot - at once +* Analysis takes `.devmap/analyze.lock` (as does `devmap analyze`) so two analyses + do not write the snapshot at once * `devmap mcp` has no `--json` flag; its output is the protocol --- diff --git a/docs/generated-files.md b/docs/generated-files.md index bbdade9..d127476 100644 --- a/docs/generated-files.md +++ b/docs/generated-files.md @@ -240,8 +240,8 @@ Purpose: ## .devmap/analyze.lock -Created while `devmap mcp` runs an analysis (`devmap_analyze` or -`--auto-refresh`) and removed when it finishes. +Created while an analysis runs, either `devmap analyze` or the MCP server +(`devmap_analyze`, `--auto-refresh`), and removed when it finishes. ```json { "pid": 12345, "startedAt": "2026-10-30T02:11:09.000Z" } @@ -249,7 +249,7 @@ Created while `devmap mcp` runs an analysis (`devmap_analyze` or Purpose: -- Stop two MCP servers (or editor windows) from writing the snapshot at once +- Stop two analyses (a terminal `devmap analyze`, or MCP servers in different editor windows) from writing the snapshot at once A lock whose process is gone, or that is older than 15 minutes, is taken over automatically. If no analysis is running and the file is still there, it is safe diff --git a/docs/guide/commands/09-mcp.md b/docs/guide/commands/09-mcp.md index 3792f76..b97727c 100644 --- a/docs/guide/commands/09-mcp.md +++ b/docs/guide/commands/09-mcp.md @@ -98,8 +98,11 @@ Before taking the lock, `run()` checks staleness. When nothing changed it return `unchanged` without touching the lock or the analyzer, so a call that has nothing to do is also never blocked by another process's analysis. -`devmap analyze` in the terminal does **not** take this lock. The window for a -collision is small, but it exists; if it matters, `runAnalyze` is the place to add it. +`devmap analyze` in the terminal takes the same lock (`analyzeCommand` wraps +`runAnalyze`). `runAnalyze` itself does not, because the refresher already holds +the lock when it calls it. If a terminal analysis is running, `devmap_analyze` +returns the "already running" error, and `devmap analyze` fails the same way +while the server is refreshing. ### `aiDataDropped` diff --git a/docs/mcp.md b/docs/mcp.md index 18a313b..74d1614 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -330,8 +330,8 @@ There are three ways to refresh, from most to least hands-on: Without `--auto-refresh`, a question never writes to disk. -Analysis takes a lock (`.devmap/analyze.lock`) so two servers or editor windows -cannot write the snapshot at once. A lock left by a dead process, or older than +Analysis takes a lock (`.devmap/analyze.lock`) so two servers, editor windows or a +terminal `devmap analyze` cannot write the snapshot at once. A lock left by a dead process, or older than 15 minutes, is taken over. If another analysis is running, `devmap_analyze` returns an error telling the agent to retry. diff --git a/packages/cli/src/cache/analysisLock.ts b/packages/cli/src/cache/analysisLock.ts new file mode 100644 index 0000000..26d7803 --- /dev/null +++ b/packages/cli/src/cache/analysisLock.ts @@ -0,0 +1,77 @@ +import { mkdir, open, readFile, rm } from "node:fs/promises"; +import { join } from "node:path"; +import { DevmapError } from "../utils/errors.js"; + +/** A lock older than this is considered abandoned, even if its pid is still alive. */ +export const LOCK_STALE_MS = 15 * 60 * 1000; + +type LockInfo = { pid: number; startedAt: string }; + +function lockPath(projectRoot: string): string { + return join(projectRoot, ".devmap", "analyze.lock"); +} + +function isProcessAlive(pid: number): boolean { + if (!Number.isInteger(pid) || pid <= 0) return false; + try { + process.kill(pid, 0); + return true; + } catch (error) { + // EPERM: the process exists but belongs to someone else. + return (error as NodeJS.ErrnoException).code === "EPERM"; + } +} + +async function readLock(path: string): Promise { + try { + const parsed = JSON.parse(await readFile(path, "utf8")) as Partial; + if (typeof parsed.pid === "number" && typeof parsed.startedAt === "string") { + return { pid: parsed.pid, startedAt: parsed.startedAt }; + } + } catch { + // unreadable or half-written: treated as abandoned + } + return undefined; +} + +/** + * Take the analysis lock (`.devmap/analyze.lock`) so two analyses (`devmap analyze`, + * or the MCP server of two editor windows) never write the snapshot at the same time. A lock whose + * process is gone, or that is older than LOCK_STALE_MS, is taken over. + */ +export async function acquireLock(projectRoot: string): Promise<() => Promise> { + const path = lockPath(projectRoot); + await mkdir(join(projectRoot, ".devmap"), { recursive: true }); + + for (let attempt = 0; attempt < 2; attempt += 1) { + try { + const handle = await open(path, "wx"); + try { + const info: LockInfo = { pid: process.pid, startedAt: new Date().toISOString() }; + await handle.writeFile(JSON.stringify(info)); + } finally { + await handle.close(); + } + return async () => { + await rm(path, { force: true }); + }; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; + + const info = await readLock(path); + const age = info ? Date.now() - Date.parse(info.startedAt) : Number.POSITIVE_INFINITY; + if (info && isProcessAlive(info.pid) && age < LOCK_STALE_MS) { + throw new DevmapError( + `Another DevMap analysis is already running (pid ${info.pid}).`, + "Wait for it to finish, then retry." + ); + } + await rm(path, { force: true }); + } + } + + throw new DevmapError( + "Could not acquire the DevMap analysis lock.", + "Delete .devmap/analyze.lock if no analysis is running, then retry." + ); +} diff --git a/packages/cli/src/commands/analyze.ts b/packages/cli/src/commands/analyze.ts index 71a4fe2..fd4d135 100644 --- a/packages/cli/src/commands/analyze.ts +++ b/packages/cli/src/commands/analyze.ts @@ -1,3 +1,4 @@ +import { stat } from "node:fs/promises"; import { resolve } from "node:path"; import { completeWithOptionalStreaming } from "../ai/completion.js"; import { @@ -8,6 +9,7 @@ import { buildAnalyzeMessages } from "../ai/prompts.js"; import { enrichSnapshotWithAi } from "../ai/snapshotEnrichment.js"; import type { AiClient } from "../ai/types.js"; import { createProjectMap } from "../analyzers/pipeline/index.js"; +import { acquireLock } from "../cache/analysisLock.js"; import { inspectSnapshot, saveSnapshot } from "../cache/snapshot.js"; import { writeAgentNavigationFiles } from "../cache/agentNavigation.js"; import { resolveEffectiveConfig, type DevmapConfig } from "../utils/config.js"; @@ -38,13 +40,34 @@ export async function analyzeCommand( ): Promise { if (options.json) { await withJsonOutput(async () => { - const snapshot = await runAnalyze(target, options, dependencies); + const snapshot = await withAnalysisLock(target, () => runAnalyze(target, options, dependencies)); output.json(snapshot); }); return; } - await runAnalyze(target, options, dependencies); + await withAnalysisLock(target, () => runAnalyze(target, options, dependencies)); +} + +/** + * Hold `.devmap/analyze.lock` while the CLI analyzes, so it never writes the + * snapshot at the same time as another `devmap analyze` or an MCP server's + * refresh. Only `analyzeCommand` takes it: `runAnalyze` is also called by the MCP + * refresher, which holds the same lock itself. + */ +async function withAnalysisLock(target: string, run: () => Promise): Promise { + const projectRoot = resolve(target); + const isDirectory = await stat(projectRoot).then((info) => info.isDirectory(), () => false); + // A missing target keeps failing in the analyzer with its own message + // instead of getting a stray .devmap folder from the lock. + if (!isDirectory) return run(); + + const release = await acquireLock(projectRoot); + try { + return await run(); + } finally { + await release(); + } } /** diff --git a/packages/cli/src/mcp/refresh.ts b/packages/cli/src/mcp/refresh.ts index b2293d5..8e37594 100644 --- a/packages/cli/src/mcp/refresh.ts +++ b/packages/cli/src/mcp/refresh.ts @@ -1,13 +1,11 @@ -import { mkdir, open, readFile, rm } from "node:fs/promises"; -import { join } from "node:path"; import type { ProjectMap } from "../analyzers/pipeline/index.js"; +import { acquireLock, LOCK_STALE_MS } from "../cache/analysisLock.js"; import { inspectSnapshot, isSnapshotStale } from "../cache/snapshot.js"; import { runAnalyze } from "../commands/analyze.js"; -import { DevmapError } from "../utils/errors.js"; import { withJsonOutput } from "../utils/output.js"; -/** A lock older than this is considered abandoned, even if its pid is still alive. */ -export const LOCK_STALE_MS = 15 * 60 * 1000; +// The lock itself lives in cache/analysisLock.ts so `devmap analyze` can share it. +export { acquireLock, LOCK_STALE_MS }; export type AnalyzeFn = ( projectRoot: string, @@ -109,76 +107,3 @@ export function createRefresher(options: RefresherOptions): Refresher { } }; } - -// ── cross-process lock ───────────────────────────────────────────────────── - -type LockInfo = { pid: number; startedAt: string }; - -function lockPath(projectRoot: string): string { - return join(projectRoot, ".devmap", "analyze.lock"); -} - -function isProcessAlive(pid: number): boolean { - if (!Number.isInteger(pid) || pid <= 0) return false; - try { - process.kill(pid, 0); - return true; - } catch (error) { - // EPERM: the process exists but belongs to someone else. - return (error as NodeJS.ErrnoException).code === "EPERM"; - } -} - -async function readLock(path: string): Promise { - try { - const parsed = JSON.parse(await readFile(path, "utf8")) as Partial; - if (typeof parsed.pid === "number" && typeof parsed.startedAt === "string") { - return { pid: parsed.pid, startedAt: parsed.startedAt }; - } - } catch { - // unreadable or half-written: treated as abandoned - } - return undefined; -} - -/** - * Take the analysis lock (`.devmap/analyze.lock`) so two MCP servers (or two - * editor windows) never write the snapshot at the same time. A lock whose - * process is gone, or that is older than LOCK_STALE_MS, is taken over. - */ -export async function acquireLock(projectRoot: string): Promise<() => Promise> { - const path = lockPath(projectRoot); - await mkdir(join(projectRoot, ".devmap"), { recursive: true }); - - for (let attempt = 0; attempt < 2; attempt += 1) { - try { - const handle = await open(path, "wx"); - try { - const info: LockInfo = { pid: process.pid, startedAt: new Date().toISOString() }; - await handle.writeFile(JSON.stringify(info)); - } finally { - await handle.close(); - } - return async () => { - await rm(path, { force: true }); - }; - } catch (error) { - if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; - - const info = await readLock(path); - const age = info ? Date.now() - Date.parse(info.startedAt) : Number.POSITIVE_INFINITY; - if (info && isProcessAlive(info.pid) && age < LOCK_STALE_MS) { - throw new DevmapError( - `Another DevMap analysis is already running (pid ${info.pid}).`, - "Wait for it to finish, then retry." - ); - } - await rm(path, { force: true }); - } - } - - throw new DevmapError( - "Could not acquire the DevMap analysis lock.", - "Delete .devmap/analyze.lock if no analysis is running, then retry." - ); -} diff --git a/packages/cli/test/analyze-lock.test.ts b/packages/cli/test/analyze-lock.test.ts new file mode 100644 index 0000000..9201b5c --- /dev/null +++ b/packages/cli/test/analyze-lock.test.ts @@ -0,0 +1,99 @@ +import assert from "node:assert/strict"; +import { access, mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import test from "node:test"; +import { acquireLock } from "../src/cache/analysisLock.js"; +import { analyzeCommand } from "../src/commands/analyze.js"; +import { DevmapError } from "../src/utils/errors.js"; + +async function tinyProject(): Promise { + const projectRoot = await mkdtemp(join(tmpdir(), "devmap-analyze-lock-")); + await mkdir(join(projectRoot, "src"), { recursive: true }); + await writeFile(join(projectRoot, "package.json"), JSON.stringify({ name: "lock-app", version: "1.0.0" })); + await writeFile(join(projectRoot, "src", "index.ts"), "export const a = 1;\n"); + return projectRoot; +} + +async function exists(path: string): Promise { + return access(path).then(() => true, () => false); +} + +async function quietly(action: () => Promise): Promise { + const originalLog = console.log; + const originalError = console.error; + console.log = () => undefined; + console.error = () => undefined; + try { + await action(); + } finally { + console.log = originalLog; + console.error = originalError; + } +} + +const offline = { offline: true }; + +test("devmap analyze releases the analysis lock when it finishes", async () => { + const projectRoot = await tinyProject(); + try { + await quietly(() => analyzeCommand(projectRoot, offline, {})); + assert.equal(await exists(join(projectRoot, ".devmap", "snapshot.json")), true); + assert.equal(await exists(join(projectRoot, ".devmap", "analyze.lock")), false); + } finally { + await rm(projectRoot, { recursive: true, force: true }); + } +}); + +test("devmap analyze refuses to run while another analysis holds the lock", async () => { + const projectRoot = await tinyProject(); + const release = await acquireLock(projectRoot); + try { + await assert.rejects( + () => quietly(() => analyzeCommand(projectRoot, offline, {})), + (error: unknown) => error instanceof DevmapError && /already running/.test(error.message) + ); + // The refused run must not delete the other analysis's lock or write a snapshot. + assert.equal(await exists(join(projectRoot, ".devmap", "analyze.lock")), true); + assert.equal(await exists(join(projectRoot, ".devmap", "snapshot.json")), false); + } finally { + await release(); + await rm(projectRoot, { recursive: true, force: true }); + } +}); + +test("devmap analyze --json also respects the lock", async () => { + const projectRoot = await tinyProject(); + const release = await acquireLock(projectRoot); + try { + await assert.rejects( + () => quietly(() => analyzeCommand(projectRoot, { ...offline, json: true }, {})), + (error: unknown) => error instanceof DevmapError && /already running/.test(error.message) + ); + } finally { + await release(); + await rm(projectRoot, { recursive: true, force: true }); + } +}); + +test("devmap analyze takes the lock again after a previous run released it", async () => { + const projectRoot = await tinyProject(); + try { + await quietly(() => analyzeCommand(projectRoot, offline, {})); + await quietly(() => analyzeCommand(projectRoot, { ...offline, fresh: true }, {})); + assert.equal(await exists(join(projectRoot, ".devmap", "analyze.lock")), false); + } finally { + await rm(projectRoot, { recursive: true, force: true }); + } +}); + +test("devmap analyze on a missing folder fails without creating a .devmap folder", async () => { + const parent = await mkdtemp(join(tmpdir(), "devmap-analyze-missing-")); + const missing = join(parent, "does-not-exist"); + try { + await assert.rejects(() => quietly(() => analyzeCommand(missing, offline, {}))); + assert.equal(await exists(missing), false); + } finally { + await rm(parent, { recursive: true, force: true }); + } +});