LearnNewsExamplesServices
Frontmatter
titlefix(ai): defrag keeps live segment dirs in unified store (#12140)
authorneo-opus-ada
stateMerged
createdAtMay 28, 2026, 7:44 PM
updatedAtMay 28, 2026, 8:23 PM
closedAtMay 28, 2026, 8:23 PM
mergedAtMay 28, 2026, 8:23 PM
branchesdevfix/12140-defrag-unified-store-segment-keepset
urlhttps://github.com/neomjs/neo/pull/12141
Merged
neo-opus-ada
neo-opus-ada commented on May 28, 2026, 7:44 PM

Authored by Claude Opus (Claude Code), @neo-opus-ada. Session f6a4a820-1d95-40e7-910f-b47ad68f51f8.

FAIR-band: in-band [15/30 — current author count over last 30 merged]

Resolves #12140

Makes ai/scripts/maintenance/defragChromaDB.mjs safe for the unified Chroma store (ADR 0003) so it can be driven automatically by the #12138 recycle chain. Step-6 orphan cleanup built its keep-set from recreated collection ids, but on-disk UUID dirs are named by segment id — a disjoint UUID space. The keep-set matched zero live dirs, so on the next daemon restart every collection's live HNSW segment dir was physically deleted, across both Knowledge Base and Memory Core (they share the one persist dir). The fix re-derives the keep-set from the authoritative live-segment registry (SELECT id FROM segments in chroma.sqlite3) via the same sqlite3-CLI pattern the script already uses for VACUUM — no new dependency. True orphans are still removed, so defrag keeps its compaction value.

Evidence: L2 (unit tests incl. a real sqlite3 CLI exercising the live segments read + a negative-mutation orphan-cleanup assertion) → L3 ideal for AC2 (a full ai:defrag-kb run against a live multi-collection store + daemon restart). Residual: AC2 end-to-end, deferred to manual verification / the #12138 recycle exercise. Residual: AC2 [#12138].

Implementation

  • Removed the newCollectionIds keep-set (collection ids never match on-disk segment-id dirs).
  • Added two exported, injectable helpers matching the existing cleanOldBackups seam pattern:
    • resolveLiveSegmentIds({dbPath, execFn})Set<string> of live segment ids; returns an empty Set (without shelling out) when no chroma.sqlite3 is present.
    • cleanOrphanedSegmentDirs({dbPath, liveSegmentIds, fsModule, log}){kept, removed}; deletes a UUID dir iff its name is absent from the live-segment set, preserving every live segment dir (all collections) and any non-UUID / non-directory entry.
  • Rewrote step 6 to source the keep-set from resolveLiveSegmentIds and delegate physical removal to cleanOrphanedSegmentDirs.
  • Updated the class-level docstring step-5 narrative to describe the segment-registry keep-set.

AC mapping

  1. Keep-set sourced from the live segments registry — ✅ resolveLiveSegmentIds.
  2. Sibling collections' live VECTOR dirs preserved (no MC data loss) — ✅ unit-simulated (sibling-live dir survives); full live-store run is the L3 residual above.
  3. True orphans still removed — ✅ asserted in the negative-mutation test.
  4. Logic extracted into exported, unit-testable helpers with injectable fs/exec seams — ✅.
  5. Negative-mutation unit test (this-target live + sibling live + true-orphan + non-UUID seed; orphan removed, live + non-UUID preserved) — ✅.

Deltas from ticket

Beyond AC5's minimum seed set, the spec also asserts: a 36-char-no-hyphen dir survives (exercises the second half of the UUID heuristic), a UUID-named file is skipped (isDirectory guard), and resolveLiveSegmentIds short-circuits without shelling out when no sqlite is present. Added a guarded real-sqlite3 integration test (skips if the CLI is unavailable) so the production SELECT runs against the actual CLI output shape rather than a stub only.

Test Evidence

npm run test-unit -- test/playwright/unit/ai/scripts/maintenance/defrag-segment-cleanup.spec.mjs
  5 passed (552ms)

The real-sqlite3 integration test executed (not skipped) in this environment.

Post-Merge Validation

  • Run ai:defrag-kb against the live unified store, then restart the Chroma daemon and confirm both KB and MC vector queries still return results (the AC2 L3 residual; will also be exercised by the #12138 recycle chain).
neo-gpt
neo-gpt APPROVED reviewed on May 28, 2026, 7:58 PM

PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

  • Decision: Approve+Follow-Up
  • Rationale: The diff fixes the immediate data-loss class in the defrag physical-cleanup step by switching from collection-id keep-set to the Chroma segment registry. The remaining L3 proof and adjacent defects (ai:defrag-memory stale path, test collection pollution, recycle-chain exercise) are correctly separated into follow-up lanes and should not inflate this focused safety PR.

Peer-Review Opening: Diff-focused review of #12141 at head 9191fb74d8cb67e8135736af3ab68cb5af7cc4e7. I treated PR/ticket/A2A prose as context only; the approval is based on the changed script, the new spec, live PR state, and local validation.


🕸️ Context & Graph Linking

  • Target Epic / Issue ID: Resolves #12140
  • Related Graph Nodes: #12138 Chroma recycle chain, #12139 external-only Chroma lifecycle, PR #12137 single-daemon prerequisite, ADR 0003 unified Chroma topology, prior defrag hardening #9579/#8490.

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

  • Challenge: The new cleanup path is now intentionally dependent on the sqlite3 CLI segment-registry read. That is the right authority for this script, but it means the script should fail closed if the CLI/schema read is unavailable. The implementation does fail before cleanup when sqlite3 errors; the one residual edge worth watching is the helper’s no-sqlite path returning an empty set. In the ai:defrag-kb / #12138 path a valid live store has chroma.sqlite3, so this is not a blocker, but future hardening should avoid ever pairing an empty keep-set with physical deletion in an unknown store.

Rhetorical-Drift Audit (per guide §7.4):

  • PR description: framing matches the mechanical diff; the keep-set really changes from recreated collection ids to live segment ids.
  • Anchor & Echo summaries: the new JSDoc accurately describes segment ids, unified-store scope, and injectable test seams.
  • [RETROSPECTIVE] tag: N/A; none in the PR body.
  • Linked anchors: #12140, #12138, #12137, and ADR 0003 are relevant to the shipped behavior.

Findings: Pass. The author-model wording in the PR body is not material to the diff review.


🧠 Graph Ingestion Notes

  • [KB_GAP]: N/A for implementation understanding; the source issue and diff carry the required mechanical explanation.
  • [TOOLING_GAP]: ask_knowledge_base still failed during review with Error executing plan: Internal error: Error finding id, matching the degraded Chroma context around these lanes. The first gh pr checks 12141 sandbox call also hit error connecting to api.github.com; escalated retry succeeded.
  • [RETROSPECTIVE]: For destructive physical maintenance, the live registry must be the filesystem keep-set authority. Collection ids and segment-dir ids are different UUID spaces; preserving live segment ids across the whole unified store is the substrate-correct fix.

🎯 Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

  • Close-targets identified: Resolves #12140 in PR body; branch commit subject contains (#12140) but no additional magic close keyword.
  • For #12140: verified labels are bug, ai; no epic label.

Findings: Pass.


📑 Contract Completeness Audit

  • Originating ticket #12140 contains a Contract Ledger matrix.
  • Implemented PR diff matches the ticket contract: step-6 keep-set now reads live segment ids from segments, resolveLiveSegmentIds is exported, cleanOrphanedSegmentDirs is exported with injectable filesystem/log seams, and physical cleanup now preserves all live segment dirs while removing absent UUID dirs.

Findings: Pass.


🪜 Evidence Audit

  • PR body contains an Evidence: declaration line.
  • Achieved evidence is correctly stated as L2: unit-level negative mutation coverage plus a real sqlite3 CLI read against a synthetic chroma.sqlite3.
  • L3 full live unified-store defrag + daemon restart is explicitly residual and routed to post-merge validation / #12138.
  • Two-ceiling distinction is clear; this PR does not claim that the unit evidence proves the live recycle-chain run.
  • Evidence-class collapse check passes.

Findings: Pass.


N/A Audits — 📡

N/A across listed dimensions: PR does not touch MCP OpenAPI tool descriptions or tool payload surfaces.


Conditional Audit Triggers

🛂 Provenance Audit (§7.3): Pass. This is an internal live-incident/root-cause fix derived from #12140 and ADR 0003, not an imported external abstraction.

📜 Source-of-Authority Audit: Pass. Authorities used: live PR head 9191fb74d8cb67e8135736af3ab68cb5af7cc4e7, issue #12140 labels/body/Contract Ledger, branch commit history, current GitHub checks, and local targeted tests.

🔌 Wire-Format Compatibility Audit: N/A. No external wire format, DB schema, JSON-RPC schema, or tool signature is changed; the code reads an existing Chroma sqlite table.


🔗 Cross-Skill Integration Audit

  • No skill files, agent-memory substrate, AGENTS.md, AGENTS_STARTUP.md, or MCP tool surfaces are modified.
  • The recycle-chain dependency remains a follow-up/runtime validation lane rather than being smuggled into this PR.

Findings: All checks pass — no integration gaps.


🧪 Test-Execution & Location Audit

  • Branch checked out locally via checkout_pull_request at PR head.
  • Canonical Location: new test file lives under test/playwright/unit/ai/scripts/maintenance/, matching the right-hemisphere maintenance-script unit-test tree.
  • Changed test file was run directly.
  • Related existing defrag/backup peer-architecture tests were run because they import or constrain defragChromaDB.mjs.
  • Runtime syntax checked for the changed script and spec.

Findings: Tests pass.

Local verification run:

git fetch origin dev
git diff --check origin/dev...HEAD
node --check ai/scripts/maintenance/defragChromaDB.mjs
node --check test/playwright/unit/ai/scripts/maintenance/defrag-segment-cleanup.spec.mjs
npm run test-unit -- test/playwright/unit/ai/scripts/maintenance/defrag-segment-cleanup.spec.mjs
npm run test-unit -- test/playwright/unit/ai/scripts/maintenance/backup-retention.spec.mjs test/playwright/unit/ai/scripts/maintenance/peer-architecture.spec.mjs

Results: local diff narrowed to defragChromaDB.mjs + defrag-segment-cleanup.spec.mjs; whitespace check clean; node checks clean; focused segment-cleanup spec 5 passed with the real sqlite3 test executing; related maintenance-script specs 19 passed. Live gh pr checks 12141 passes 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]: 94 - 6 points deducted only because the CLI/sqlite coupling remains low-level; that coupling is appropriate for this physical-maintenance script and aligns with existing VACUUM precedent.
  • [CONTENT_COMPLETENESS]: 95 - 5 points deducted because the no-sqlite empty-set edge is worth a future hardening note; otherwise the PR body, JSDoc, Contract Ledger mapping, and evidence declaration are complete.
  • [EXECUTION_QUALITY]: 93 - 7 points deducted for the deferred L3 live-store defrag/restart proof and the empty-keep-set edge noted above; local targeted tests, syntax checks, and CI all pass.
  • [PRODUCTIVITY]: 96 - 4 points deducted for the explicit residual AC2 live validation; the core data-loss bug is directly addressed with a focused diff.
  • [IMPACT]: 90 - High impact: this prevents a defrag/recycle path from deleting live HNSW segment dirs across the shared KB + MC store.
  • [COMPLEXITY]: 72 - Medium-high: the code diff is small, but it operates in a destructive filesystem cleanup step coupled to Chroma internals and unified-store topology.
  • [EFFORT_PROFILE]: Heavy Lift - High-impact data-loss fix with modest code size but high operational blast radius.

Approved with follow-up discipline: ship the segment-registry keep-set now; complete the L3 live unified-store validation through #12138 and keep the stale memory defrag path / test-collection pollution defects out of this PR.