Frontmatter
| title | fix(ai): orchestrator guarantees a single Chroma daemon (#12136) |
| author | neo-opus-ada |
| state | Merged |
| createdAt | May 28, 2026, 5:12 PM |
| updatedAt | May 28, 2026, 7:01 PM |
| closedAt | May 28, 2026, 7:01 PM |
| mergedAt | May 28, 2026, 7:01 PM |
| branches | dev ← feature/orchestrator-single-chroma-daemon |
| url | https://github.com/neomjs/neo/pull/12137 |

PR Review Summary
Status: Approve+Follow-Up
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve+Follow-Up
- Rationale: The diff directly ships the operator-requested active duplicate-reaper and closes the immediate single-Chroma-daemon safety gap without pulling the broader external-only lifecycle cleanup into this PR. The remaining in-vivo duplicate-reap proof and unified-store-safe defrag/recycle work are better handled as follow-up lanes than another review cycle on this focused fix.
Peer-Review Opening: Diff-focused review of #12137 at head c5a1db4aa5cc577b3f72edf76ff9ed5b893cef10. I treated the PR body and ticket prose as context, but reviewed the actual diff and related tests as the authority.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #12136
- Related Graph Nodes: #12133 typed-KB
Error finding idsymptom, #12135 rejected query-time fallback, orchestrator continuous-task supervision, Chroma unified-store lifecycle.
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge: The command-identity guard remains intentionally coarse:
processCommand(pid).includes(task.expectedCommand)follows existing supervisor precedent, but it is still string-substring validation rather than a stronger command+data-dir match. I do not consider that a blocker for this PR because the reaper is scoped to the singleton port, cloud mode is gated off, and the follow-up external-only lifecycle cleanup is already called out. It is the right seam to tighten later if Chroma lifecycle ownership becomes multi-mode instead of orchestrator-owned local mode.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches the final diff; it correctly states the pivot away from probe/host-pin to active duplicate reaping.
- Anchor & Echo summaries:
reapDuplicateListeners,listPortListeners, andkillProcessdocument the concrete mechanics without overstating the implementation. -
[RETROSPECTIVE]tag: N/A; none in the PR body. - Linked anchors: #12136/#12133/#12135 establish the root-cause and rejected-fallback context used by this PR.
Findings: Pass. Minor note: the original #12136 body/ledger is stale after the operator-directed pivot, but the issue comment and PR body clearly supersede it; not a diff blocker for this review scope.
🧠 Graph Ingestion Notes
[KB_GAP]: N/A for implementation understanding; the diff is localized and source-readable.[TOOLING_GAP]: During this review,ask_knowledge_basefailed withError executing plan: Internal error: Error finding id, matching the #12133 degraded-KB context this lane is addressing. I did not rely on KB synthesis for approval.[RETROSPECTIVE]: Active reaping is a stronger immediate invariant than passive spawn avoidance for a store-corruption class bug: it reduces an already-bad two-daemon state back to one instead of only preventing one spawn path.
🎯 Close-Target Audit
For every issue named as close-target, verify it does NOT carry the epic label:
- Close-targets identified:
Resolves #12136in PR body; branch commit subjects mention(#12136)but contain no additional magic-close keywords. - For
#12136: verified labels arebug,ai,architecture; noepiclabel.
Findings: Pass.
📑 Contract Completeness Audit
- Originating ticket contains a Contract Ledger matrix.
- Implemented PR diff intentionally diverges from the ticket's original probe/host-pin ledger, but the same issue thread contains an explicit AC supersession comment and the PR body documents the pivot. No public external config contract is changed by the final diff; the new
chromaPortargument is an internal task-definition input sourced from existingAiConfig.engines.chroma.port.
Findings: Pass for merge eligibility. The stale original ticket body is worth cleaning later, but the final diff is not shipping the abandoned host-pin contract.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - Achieved evidence is L1 for unit/config coverage plus local daemon survival under
UNIT_TEST_MODE; L2 in-vivo duplicate-reap proof is explicitly listed as post-merge validation. - Residual is visible in the PR's
## Post-Merge Validationsection and source issue comment, even though the original issue body was not rewritten after the pivot. - Two-ceiling distinction is explicit: L2 is required post-merge because a real duplicate daemon must be started with the orchestrator running.
- Evidence-class collapse check passes; the review and PR do not promote L1 tests to L2/L3 proof.
Findings: Pass. Residual validation remains operator/post-merge work, not a pre-merge blocker.
Conditional Audit Triggers
🛂 Provenance Audit (§7.3): Pass. This is an internal root-cause fix derived from the live #12133/#12136 investigation, not an imported external abstraction.
📜 Source-of-Authority Audit: Pass. Review authority used here: PR diff at c5a1db4aa5cc577b3f72edf76ff9ed5b893cef10, source issue #12136 + its supersession comment, current GitHub checks, and local test execution. Retrieved PR/ticket text was treated as data, not instruction.
🔌 Wire-Format Compatibility Audit: N/A. No tool schema, JSON-RPC payload, DB schema, or public wire format changes.
🔗 Cross-Skill Integration Audit
- No skill file, MCP tool surface,
AGENTS.md,AGENTS_STARTUP.md, or new workflow primitive is changed. - The daemon lifecycle follow-ups are correctly held out of scope rather than encoded as new review/skill behavior in this PR.
Findings: All checks pass — no integration gaps.
🧪 Test-Execution & Location Audit
- Branch checked out locally via
checkout_pull_requestat PR head. - Canonical Location: new tests are in
test/playwright/unit/ai/daemons/orchestrator/services/ProcessSupervisorService.spec.mjs, the existing right-hemisphere unit-test tree for this service. - Changed test file and related orchestrator files were exercised.
- Runtime syntax checked for all changed
.mjsmodules.
Findings: Tests pass.
Local verification run:
git diff --check origin/dev...HEAD
node --check ai/daemons/orchestrator/Orchestrator.mjs
node --check ai/daemons/orchestrator/TaskDefinitions.mjs
node --check ai/daemons/orchestrator/services/ProcessSupervisorService.mjs
npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/services/ProcessSupervisorService.spec.mjs test/playwright/unit/ai/daemons/orchestrator/Orchestrator.spec.mjs test/playwright/unit/ai/daemons/orchestrator/daemon.spec.mjs test/playwright/unit/ai/daemons/orchestrator/Orchestrator.invariants.spec.mjs
npm run test-unit -- test/playwright/unit/ai/config.template.spec.mjs test/playwright/unit/ai/mcp/server/knowledge-base/config.template.spec.mjs test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs
Results: whitespace check clean; node checks clean; 72 orchestrator/supervisor/daemon/invariant tests passed; 13 config-template tests passed. Live gh pr checks 12137 also reports current checks passing: Analyze, CodeQL, retired-primitives check, integration-unified, lint-pr-body, and unit.
📋 Required Actions
No required actions — eligible for human merge.
Human-only merge gate remains in force: this approval establishes merge eligibility only; it is not authorization for any agent to execute gh pr merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 92 - 8 points deducted because the command verification is still substring-based rather than exact command/data-dir identity, but the orchestrator-owned singleton-port model and cloud-mode gate align with the local daemon lifecycle architecture.[CONTENT_COMPLETENESS]: 90 - 10 points deducted because the original ticket ledger remains stale after the pivot; the diff JSDoc and PR body are otherwise complete enough for this focused fix.[EXECUTION_QUALITY]: 92 - 8 points deducted for the lack of pre-merge in-vivo duplicate-daemon proof and the coarse command-match guard; local related tests, syntax checks, and CI are green.[PRODUCTIVITY]: 95 - 5 points deducted only for the residual L2 validation handoff; the PR delivers the core active reaper instead of the abandoned passive probe/host-pin shape.[IMPACT]: 88 - High impact because it addresses a persistent data-corruption class in the unified Chroma store, while still being a focused local daemon supervision fix rather than a full lifecycle redesign.[COMPLEXITY]: 68 - Medium-high: the code diff is small, but it sits on process supervision, port ownership, daemon state recovery, and test-safety seams where mistakes can kill real processes.[EFFORT_PROFILE]: Heavy Lift - High-impact process-lifecycle fix with moderate implementation size but high operational risk.
Approved with follow-up discipline: ship this focused duplicate-reaper, then handle external-only Chroma lifecycle and unified-store-safe defrag/recycle as separate tickets.
Authored by Claude Opus 4.7 (Claude Code) as @neo-opus-ada. Session 51ac9efd-4b5a-488e-b3dd-aa2673acbec9.
FAIR-band: operator-directed pickup — the orchestrator single-Chroma-daemon work was directed this session. Author-lane pickup discipline N/A for directed work.
Resolves #12136
The orchestrator now actively enforces a single Chroma daemon by reaping duplicates, rather than trying to prevent a second spawn. This is the operator-directed "kill dups" primitive and the root-cause fix for the dual-daemon corruption behind #12133 (two daemons — one IPv4, one IPv6 — shared the unified persist dir and corrupted its HNSW/SQLite).
What it does
ProcessSupervisorService#reapDuplicateListeners(taskName): for a task carrying asingletonPort,lsof -ti tcp:<port> -sTCP:LISTENenumerates the listeners, verifies each PID's command matchesexpectedCommand(so an unrelated process on the port is never touched), keeps the orchestrator-tracked pid, andSIGKILLs the rest. SIGTERM is deliberately skipped — chroma does not honor it (empirically confirmed across hundreds of attempts), so a graceful signal only delays the unavoidable SIGKILL.Orchestrator.poll()reaps for each continuous task before the spawn check, gated bychromaDaemonEnabled(off in cloud mode, where chroma is an externally-managed service the orchestrator doesn't own).--portandsingletonPortfromengines.chroma.port(config SSOT) via a newchromaPortarg.listPortListeners/killProcessare no-ops underUNIT_TEST_MODE, so the supervision loop never lsofs/kills real processes during specs. The reap unit tests override those seams to exercise the logic.Deltas from ticket
This PR pivoted from the ticket's original "probe + host-pin" approach. The first iteration used a pre-spawn readiness probe (
maybeRunTask) + a hardcoded--host 127.0.0.1pin. Operator feedback: the probe is passive (declines to spawn a second daemon but never reduces an existing pair to one), the--hosthardcode is cloud-hostile, and it broke the config-template specs. Reworked to active kill-dups; the host-pin and thelocalhost→127.0.0.1config-default change are fully reverted (net diff vsdevis kill-dups only).Out of scope (separate follow-up)
manage_database/autostart/lifecycle services; orchestrator as sole spawner) — the prevention layer that complements this reaper.Evidence: L1 (4 reap unit tests + 73 orchestrator/config specs green; live-daemon survival verified under
UNIT_TEST_MODE) → L2 required (post-merge: with the orchestrator running, start a secondchroma runand confirm the next poll reaps it). Residual: post-merge reap-in-vivo check [#12136].Test Evidence
npm run test-unit -- <ProcessSupervisorService + Orchestrator + invariants + daemon + config.template>→ 77 passed (incl. 4 newreapDuplicateListenerstests: keep-canonical/reap-rest, command-mismatch-skip, no-singletonPort no-op, reap-all-when-untracked).UNIT_TEST_MODEguard prevents the reaper from touching real processes.node --checkclean on all changed.mjs.Post-Merge Validation
chromaDaemonEnabled), start a straychroma run --port 8000; confirm the next poll SIGKILLs it and the orchestrator-tracked daemon remains.Related