Skip to content

fix(mine): detect concurrent palace holder, exit non-zero with clear error (#1264) - #1349

Closed
adv3nt3 wants to merge 1 commit into
MemPalace:developfrom
adv3nt3:fix/1264-mine-detect-concurrent-writer
Closed

fix(mine): detect concurrent palace holder, exit non-zero with clear error (#1264)#1349
adv3nt3 wants to merge 1 commit into
MemPalace:developfrom
adv3nt3:fix/1264-mine-detect-concurrent-writer

Conversation

@adv3nt3

@adv3nt3 adv3nt3 commented May 4, 2026

Copy link
Copy Markdown
Contributor

Fixes #1264.

What does this PR do?

Closes the operator-visible silent-exit gap when mempalace mine is run while another writer (typically mempalace.mcp_server) holds the palace's chroma.sqlite3 open.

Today, mine would:

  1. Print the auto-defaults stderr warning from load_config.
  2. Hit chromadb 1.5.x's concurrent-writer path on the first write attempt.
  3. Exit silently — either via a Rust-binding SIGSEGV or with a swallowed lock contention. stdout's banner was lost to block-buffering on signal exit; only the stderr warning survived.

The result was the 200-byte log described in #1264.

This PR adds a pre-flight detection that runs before any other output and bails with a clear message + non-zero exit when a holder is found. It's complementary to #1162's structural fix at the ChromaCollection lock layer — once that lands, the lock-layer raise reaches the same loud MineAlreadyRunning handler instead of disappearing.

Changes

mempalace/palace.py — new helper

  • detect_palace_holder(palace_path) -> Optional[dict]
  • Uses lsof -Fpcn to find the first non-self process holding <palace>/chroma.sqlite3
  • _classify_palace_holder(pid, command) inspects full argv via ps to classify the holder as mempalace.mcp_server, mempalace mine, or the raw command name as fallback
  • POSIX-only: returns None on Windows or when lsof is unavailable, missing, or times out — graceful degrade preserves prior behavior on platforms we can't probe
  • Never raises, never blocks more than ~3s (1s lsof + 1s ps with caps)

mempalace/miner.py — wire pre-flight into mine()

  • Pre-flight runs before load_config, the banner, and get_collection. The auto-defaults warning can no longer leak through as the only line of an operator's log.
  • On detected holder: prints a one-line stderr message with palace path, holder PID, process kind, and a suggested next step, then sys.exit(1).
  • On MineAlreadyRunning: was return (clean exit 0 with "exiting cleanly" wording, which the issue called out as misleading); now sys.exit(1) with a clear cannot start message.
  • Dry-run skips the pre-flight (it doesn't open chroma).

tests/test_detect_palace_holder.py — new test file

13 tests covering:

  • Missing palace dir / missing chroma.sqlite3 → no detection attempt
  • lsof unavailable / timeout / non-zero exit → graceful None
  • Self-PID is filtered out
  • MCP server and concurrent-mine classification via mocked ps
  • Windows path returns None (monkeypatch.setattr(sys, "platform", "win32"))
  • mine() exits 1 with clear stderr when a holder is detected, without the auto-defaults warning leaking
  • mine() exits 1 with clear stderr on MineAlreadyRunning
  • mine() proceeds normally when no holder is reported
  • Dry-run skips the pre-flight

How to test

# unit + integration tests
pytest tests/test_detect_palace_holder.py tests/test_palace_locks.py tests/test_miner.py -v

# end-to-end smoke (POSIX): hold the chroma file open in one shell, mine in another
mkdir -p /tmp/sm/proj /tmp/sm/palace && touch /tmp/sm/palace/chroma.sqlite3
echo 'def main(): pass' > /tmp/sm/proj/main.py
python3 -c "open('/tmp/sm/palace/chroma.sqlite3'); import time; time.sleep(60)" &
sleep 0.5
mempalace --palace /tmp/sm/palace mine /tmp/sm/proj
# Expect:
#   mempalace mine: cannot start — palace at /tmp/sm/palace is held by Python (PID <pid>). Wait for it to finish, or stop the holder before mining.
# Exit code: 1

Verified locally on macOS 26.4 (Apple Silicon, Python 3.14): lsof correctly identifies the holding PID, the message reaches stderr before any other output, and exit code is 1.

Scope

Behavior change to flag

The MineAlreadyRunning handler now exits 1 instead of 0. The previous behavior was documented as "exiting cleanly", but per the issue's recommendation a non-zero exit makes nohup / shell wrappers / hooks see a useful signal. mempalace.hooks_cli._spawn_mine does not check the subprocess exit code, so the hook path is unaffected. If any caller relied on exit 0 from "another mine running", they should be updated to expect 1.

Checklist

  • Tests pass (pytest tests/ -v — 1502 passed, 1 skipped)
  • No hardcoded paths
  • Linter passes (ruff check . — All checks passed)
  • No new dependencies (uses only subprocess and sys from stdlib)

Refs:

@adv3nt3

adv3nt3 commented May 4, 2026

Copy link
Copy Markdown
Contributor Author

Heads up: the macOS CI failure is unrelated to this PR.

tests/test_cli.py::test_cmd_compress_output_readable_via_get_closets_collection failed with httpx.ConnectError: [Errno 8] nodename nor servname provided, or not known — a transient DNS resolution failure on the macOS runner when ChromaDB tried to fetch the embedding model from Hugging Face. The same test passed on develop's prior CI run (25272450918).

This PR adds no network calls — detect_palace_holder only shells out to local lsof / ps. All Linux runs (3.9 / 3.11 / 3.13) pass; lint passes; only the macOS runner hit this flake.

Could a maintainer re-run the failed macOS job? I can't trigger it from my fork without admin rights. If you'd prefer I push a no-op commit to retrigger CI, happy to do that — let me know.

@igorls igorls added bug Something isn't working area/mining File and conversation mining labels May 6, 2026
…error (MemPalace#1264)

When `mempalace.mcp_server` (or any other writer) holds chroma.sqlite3
open, `mempalace mine` would print only the auto-defaults stderr warning
and exit with no diagnostic visible — the chroma open hit lock contention
or a Rust-binding SIGSEGV under chromadb 1.5.x's concurrent-writer path,
and stdout buffering ate the banner that would have explained things.

Adds a pre-flight `detect_palace_holder()` to `palace.py` that uses
`lsof` on POSIX to find a non-self process holding the palace's
chroma.sqlite3, with a coarse `ps`-based classification (mcp_server vs.
mine vs. raw command name). `mine()` checks before any other output and
exits 1 with a one-line stderr error including palace path, holder PID,
process kind, and a suggested next step. The existing
`MineAlreadyRunning` catch now also exits 1 (was: clean exit 0 with
"exiting cleanly" wording, which was misleading per the issue).

Windows and hosts without `lsof` degrade silently to None — pre-flight
becomes a no-op and the existing behavior is preserved. Dry-run skips
the pre-flight (no chroma open). Complementary to PR MemPalace#1162's structural
fix at the ChromaCollection lock layer.
@adv3nt3
adv3nt3 force-pushed the fix/1264-mine-detect-concurrent-writer branch from 90bd2f4 to c48f25a Compare May 6, 2026 11:02
@adv3nt3

adv3nt3 commented May 6, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto develop to resolve the merge conflict (single-line import collision in palace.py from #1377's threading addition). Full test suite + ruff still pass.

@adv3nt3

adv3nt3 commented May 8, 2026

Copy link
Copy Markdown
Contributor Author

Closing — superseded by #1413 (merged), which solves #1264 with a cleaner approach:

  • Records holder PID + argv inside the existing lock file via a+ open mode (no external lsof/ps calls)
  • Works on all platforms uniformly (this PR's pre-flight degrades silently on Windows / hosts without lsof)
  • No fork+exec overhead on every mine startup
  • Combined with fix: serialize ChromaCollection writes through palace lock #1162 (also merged), MCP and direct backend writers now participate in mine_palace_lock, so the lock file is the source of truth for "who holds the palace"

Thanks @igorls for the better fix. Leaving this PR closed; the test ideas from tests/test_detect_palace_holder.py are not worth porting since the lock-file approach exercises the same surface from tests/test_palace_locks.py.

@adv3nt3 adv3nt3 closed this May 8, 2026
@adv3nt3
adv3nt3 deleted the fix/1264-mine-detect-concurrent-writer branch May 8, 2026 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/mining File and conversation mining bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mine: silent exit when concurrent writer holds chroma lock; should detect live mcp_server and back off with clear error

2 participants