#2413

HTTP transport: thread-local writer-lease credit self-conflicts under ThreadingHTTPServer — second mutating call always fails

OpenBeowulfn4756opened 1mo ago

Summary

In --transport http mode, the writer-lease re-entrance credit is thread-local (_palace_lock_holders = threading.local(), palace.py), but ThreadingHTTPServer serves every request on a fresh thread. The first mutating tool call acquires the per-palace mine lock and holds it for process lifetime (_acquire_mcp_writer_lock); any subsequent mutating call lands on a different handler thread, is not credited as the holder, re-opens the lock file, and hits the process's own byte-0 lock.

Reproduction (3.5.0, Windows, Python 3.13)

Same-process second-handle conflict at the primitive level:

import msvcrt, os, tempfile
p = os.path.join(tempfile.gettempdir(), 'locktest.bin')
f1 = open(p, 'a+b'); f1.seek(0)
msvcrt.locking(f1.fileno(), msvcrt.LK_NBLCK, 1)
f2 = open(p, 'r+b'); f2.seek(0)
msvcrt.locking(f2.fileno(), msvcrt.LK_NBLCK, 1)   # -> OSError [Errno 13] Permission denied

End-to-end: start mempalace-mcp --transport http, issue two sequential mempalace_diary_write tool calls → the second fails with MineAlreadyRunning, naming the server's own PID as the blocker.

Related lifecycle effects (same root: lease scoped to thread/process-lifetime)

  • Once any mutating call ran, the lease is held until process exit — a scheduled external miner gets MineAlreadyRunning for the server's entire lifetime.
  • In the reverse ordering, read-only mode is sticky for process lifetime (mcp_server.py "stays read-only for its lifetime") and mempalace_reconnect does not reset it.

Suggested direction

Scope the lease credit to the process (e.g., record owner PID + a process-wide flag) rather than the thread, so any handler thread of the holding process passes re-entrance; or serialize mutating tool execution onto a single dedicated writer thread in HTTP mode.

Context: we run 3.5.0 as a long-lived loopback HTTP service (Windows/NSSM) and currently route all mutating tools through short-lived CLI invocations as a workaround. Related: #2411 (startup quick_check gate). Happy to test a patch.

Activity

2
Beowulfn47561mo ago

Addendum from further live testing of 3.5.0 HTTP mode: with the palace chroma.sqlite3 in journal_mode=delete, a heavy concurrent mine exclusively locks readers, and because every HTTP request serializes under the single _HTTP_REQUEST_LOCK, one sqlite-blocked request wedges the whole listener (health endpoint stays green — it bypasses the lock). Suggest, alongside the lease fix: (a) default/migrate palaces to WAL, (b) bound sqlite busy waits inside request handlers, (c) consider per-request timeout or dropping the global lock for read-only tools. Happy to test.

Beowulfn47561mo ago

One more from live testing: when the lazy SQLite integrity check happens to run while a miner holds the database (journal_mode=delete), the resulting database is locked error is recorded as an integrity FAILURE and sticks for process lifetime — mutating tools then refuse with "Palace SQLite integrity check failed; refusing tool call until the palace is repaired" even though the palace is healthy (reads fine, quick_check clean after restart). Suggest distinguishing SQLITE_BUSY/locked from real corruption in sqlite_integrity_errors and retrying rather than caching a corruption verdict.