Skip to content

fix: scope index-process cleanup to the running harness - #709

Open
DrTrippel wants to merge 1 commit into
zilliztech:mainfrom
DrTrippel:fix/per-harness-index-pidfile
Open

fix: scope index-process cleanup to the running harness#709
DrTrippel wants to merge 1 commit into
zilliztech:mainfrom
DrTrippel:fix/per-harness-index-pidfile

Conversation

@DrTrippel

Copy link
Copy Markdown
Contributor

Problem

Claude Code and Codex can share a single MEMSEARCH_DIR, and so can third-party clients that load these Claude-compatible plugin hooks. When two harnesses share one, either can reap an index the other still has in flight, because all three kill steps in kill_orphaned_index are harness-blind:

  1. INDEX_PIDFILE is one shared path, .index.pid — whoever starts last overwrites it.
  2. pgrep -f "memsearch index $MEMORY_DIR" matches every harness. They all index the same MEMORY_DIR, so the pattern cannot tell them apart.
  3. pgrep -f "milvus_lite/lib/milvus" is unscoped and matches every milvus_lite process on the machine.

The visible symptom is a killed index mid-run, then stale search results until the next session start re-indexes.

Worth noting that a per-harness pidfile alone does not fix this — steps 2 and 3 would keep cross-killing on their own. All three steps have to become ownership-aware.

Fix

Tag the pidfile with the harness — .index.<harness>.pid, defaulting to the plugin's own name and overridable via MEMSEARCH_HARNESS — then have both pattern sweeps skip PIDs owned by another harness's live index.

The protected set includes descendants: milvus_lite is spawned by memsearch index and outlives it, so sparing only the recorded root would still let step 3 kill the child.

Applied to claude-code and codex, which carry byte-identical cleanup logic. Fixing only one would leave the Claude Code + Codex pairing broken.

Two deliberate choices:

  • The pre-tagging .index.pid is left unowned and reaped by whichever cleanup runs first, so an in-place upgrade does not strand it.
  • WATCH_PIDFILE stays shared. One watcher per memory dir is the intended behavior, not a collision.

MEMSEARCH_HARNESS also gives harnesses outside this repo a supported way to identify themselves — I hit this running Grok Build, which loads the claude-code plugin hooks verbatim.

Tests

tests/test_hook_index_pidfile.py, 10 cases parametrized over both plugins, following the existing pytest-drives-bash pattern in test_claude_hooks.py:

  • pidfile is tagged with the harness
  • MEMSEARCH_HARNESS overrides the default tag
  • cleanup spares an index owned by another harness, and reaps its own
  • cleanup spares descendants of another harness's index
  • cleanup still reaps the untagged legacy pidfile

8 of the 10 fail against unpatched code. The 2 that pass are the legacy-pidfile cases, which the current code satisfies by definition — they are there to guard that path against regression.

Full suite: 360 passed, 7 skipped. ruff check and ruff format --check clean.

One thing I did not fix

The tests need a pgrep shim, and the reason is worth surfacing on its own.

Step 3's milvus_lite/lib/milvus pattern has no path, project, or user scoping, so calling kill_orphaned_index in-process kills every milvus_lite on the machine — including instances belonging to other tests in this suite, which is what the shim works around. The same is true in real use for unrelated projects and non-memsearch milvus work.

That felt like a distinct problem with a wider blast radius than this PR, so I left it alone rather than widening the diff. Happy to open a separate issue, or fold a fix in here if you would prefer that.

Claude Code and Codex (and third-party clients that load these
Claude-compatible plugin hooks) can share a single MEMSEARCH_DIR. When
they do, one harness's session start reaps an index another harness
still has in flight, because all three kill steps in
kill_orphaned_index are harness-blind:

  1. INDEX_PIDFILE is a single shared path, `.index.pid`.
  2. `pgrep -f "memsearch index $MEMORY_DIR"` matches every harness --
     they all index the same MEMORY_DIR.
  3. `pgrep -f "milvus_lite/lib/milvus"` is unscoped and matches every
     milvus_lite process on the machine.

The result is a killed index mid-run and, until the next session start
re-indexes, stale search results.

Tag the pidfile with the harness (`.index.<harness>.pid`, defaulting to
the plugin's own name and overridable via MEMSEARCH_HARNESS), then have
both pattern sweeps skip PIDs that another harness's live index owns.
The protected set includes descendants: milvus_lite is spawned by
`memsearch index` and outlives it, so sparing only the recorded root
would still let step 3 kill the child.

The pre-tagging `.index.pid` is left unowned and reaped by whichever
cleanup runs first, so an in-place upgrade does not strand it.

WATCH_PIDFILE is deliberately left shared -- one watcher per memory dir
is the intended behavior.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013WESEpT69xA2zPFErXnjYL

@zc277584121 zc277584121 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for tackling the cross-harness ownership problem. Per-harness pidfiles are a useful part of the solution, and the new shared-directory tests pass, but the cleanup boundary is not safe enough to merge yet.

The blocking issue is that step 3 still runs a host-global pgrep -f "milvus_lite/lib/milvus" sweep and signals every match that is not protected by a currently live root pidfile in the same MEMSEARCH_DIR. A controlled no-signal lifecycle matrix shows that this still targets both an unregistered Milvus Lite process from another project and a non-MemSearch Milvus Lite process. The test suite currently hides that behavior with a pgrep shim, so its passing result does not establish project-scoped cleanup.

There are several related ownership gaps that need to be addressed together:

  1. If another harness's recorded parent exits while its Milvus Lite child remains alive, _other_harness_pids drops the ownership record because kill -0 fails for the parent. The global sweep then targets the surviving child.
  2. A stale pidfile whose PID has been reused can cause an unrelated live process to be signaled because the pidfile path and kill -0 are treated as sufficient identity.
  3. ${MEMSEARCH_HARNESS:-default} already gives unset or empty values the plugin default; please preserve and document that compatibility behavior. Non-empty overrides are interpolated directly into a pathname, so they need validation or normalization that rejects slashes, traversal components, glob characters, whitespace, and other values that would create unsafe or ambiguous pidfile paths.
  4. Cleanup and pidfile publication are not atomic. Concurrent starts for the same harness can both observe an empty state and then overwrite the same pidfile, leaving one live owned process unregistered; start/stop has a similar publication race.
  5. MEMORY_DIR is inserted into a pgrep -f regular expression without literal escaping, so valid paths containing regular-expression metacharacters do not have literal matching semantics.

Please revise the lifecycle design so cleanup signals only processes whose ownership and identity are verified for the current harness/project, without a host-global fallback sweep. Please also preserve both affected plugins' existing default behavior and define a safe legacy-pidfile migration path that does not trust a reused PID by itself.

Regression coverage should exercise both plugin implementations with controlled process stubs and temporary directories for:

  • two harnesses sharing one MEMSEARCH_DIR;
  • different projects;
  • same-harness restart;
  • the legacy pidfile;
  • PID reuse;
  • a parent exiting while its child remains alive;
  • unset/empty harness values falling back to the plugin default, invalid non-empty harness identifiers, and paths containing regular-expression metacharacters; and
  • concurrent start/start and start/stop interleavings.

Those tests should expose the global sweep behavior rather than suppress it. No test should enumerate or signal unrelated host processes.

For reference, the immutable head reviewed was e0a3f19e96ee471adc1a0998fe18ee4aff92f54f. Its contributed focused tests passed (10 passed), the affected hook suites passed (40 passed), two direct full Python suite runs each passed (360 passed, 7 skipped), and complete non-overlapping shards independently produced the same total. The blocker is the ownership behavior above, not a conventional test or CI failure.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants