fix: replace fcntl.flock spawn-mutex with a cross-platform primitive (Windows no-op today) - #2096
Open
KeilerHirsch wants to merge 1 commit into
Open
Conversation
start_daemon()'s spawn mutual-exclusion guard was built entirely on fcntl.flock. fcntl is POSIX-only, so `_fcntl` is None on Windows and the whole `if _fcntl is not None` block was skipped there: no mutual exclusion of any kind. Two near-simultaneous auto-start callers (two hooks firing back-to-back, or a hook plus a manual `daemon start`) both observed no running daemon and both called subprocess.Popen. Both wrote endpoint.json and pid.json; whichever process did not win that race became a permanently orphaned daemon, since stop_daemon() only ever targets the pid recorded in endpoint.json. Add _try_lock_start_file() / _wait_lock_start_file(), which use fcntl.flock on POSIX and msvcrt.locking on Windows, and rewire start_daemon() to them. Both primitives are advisory locks tied to the open file handle and release automatically when it closes, including on process crash. msvcrt has no indefinite-blocking mode (LK_LOCK gives up after ~10s), so the Windows wait polls the non-blocking primitive up to a 30s ceiling. Both helpers raise DaemonError rather than a raw OSError: cli.py's cmd_daemon and _submit_daemon_cli_job catch only DaemonError, so an OSError escaping here would surface as an unhandled traceback instead of the usual "mempalace: daemon error: ..." exit-1 path. Also close the lock file handle on the two paths that leave start_daemon before the readiness-probe try/finally that owns it -- the lock-wait failure and the "reuse the winner's daemon" return. The latter leaked an open handle to start.lock on every contended start before this change, on POSIX too. Tests: assert the primitive actually contends and that a blocked waiter wakes on release; drive two concurrent start_daemon() callers through the real on-disk lock (not mocked) and assert exactly one spawn; assert the handle is released on both early-exit paths; assert both platforms translate a lock failure into DaemonError. The POSIX translation test fakes the fcntl module so it runs on every CI platform, not just Linux/macOS.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
start_daemon()'s spawn mutual-exclusion guard is built entirely onfcntl.flock. The comment above it states why it exists:But
fcntlis POSIX-only, so_fcntlisNoneon Windows and the wholeif _fcntl is not Noneblock is skipped there. On Windows there is no mutual exclusion of any kind — the exact race the comment describes is completely unguarded.When it fires, two near-simultaneous auto-start callers (two hooks firing back-to-back, or a hook plus a manual
daemon start) both callsubprocess.Popen. Both children writeendpoint.jsonandpid.json; whichever process is not referenced by the finalendpoint.jsonbecomes a permanently orphaned daemon, sincestop_daemon()only ever targets the pid recorded there. It holds a Python process, an HTTP port, and a live sqlite connection indefinitely, and there is no supported way to stop it.Fix
Add
_try_lock_start_file()/_wait_lock_start_file(), which usefcntl.flockon POSIX andmsvcrt.lockingon Windows, and rewirestart_daemon()to them.Both are advisory locks tied to the open file handle and release automatically when it closes, including on abnormal process termination. I measured this on Windows before relying on it:
msvcrt.locking(LK_NBLCK, 1)succeeds on a handle opened in"w"modeOSErrorwhile the first holds the lockclose()alone releases it, without an explicitLK_UNLCKmsvcrthas no indefinite-blocking mode (LK_LOCKgives up after ~10s), so the Windows wait polls the non-blocking primitive up to a 30s ceiling instead of blocking forever likeflock(LOCK_EX).Error contract
Both helpers raise
DaemonErrorrather than a rawOSError.cli.py'scmd_daemonand_submit_daemon_cli_jobcatch onlyDaemonError, so anOSErrorescaping from here would surface as an unhandled traceback instead of the usualmempalace: daemon error: .../ exit 1 path.Handle lifetime
This also closes the lock file handle on the two paths that leave
start_daemonbefore the readiness-probetry/finallythat owns it: the lock-wait failure, and the "reuse the winner's daemon"return existing. The latter leaked an open handle tostart.lockon every contended start before this change, on POSIX too.Tests
_try_lock_start_fileactually contends (second handle is refused) and_wait_lock_start_filewakes once the holder releases — exercising whichever primitive the running platform provides, not a mock.start_daemon()callers race through the real on-disk lock, with onlysubprocess.Popenand the readiness probe faked; asserts exactly one spawn. Athreading.Barrierforces both callers to observe "no daemon running" at the same instant, so the race window is deterministic rather than scheduler-dependent. This test fails on the current code withgot 2spawns.DaemonError. The POSIX test fakes thefcntlmodule so it runs on every CI platform rather than only Linux/macOS.Notes for reviewers
Two adjacent issues are deliberately not included here, to keep this change focused:
_detached_kwargs()/subprocess.Popen()sit outside anytrythat closeslock_fh. IfPopenraises, the handle leaks. This is unchanged fromdevelopand affects the uncontended first-spawn path too, so it is orthogonal to this fix._LOCK_WAIT_TIMEOUT(the Windows-only poll ceiling) is decoupled fromstart_daemon's caller-suppliedtimeout. Under contention on Windows, a caller passing a shorttimeoutcan still wait up to 30s in the lock phase. Now that this surfaces as a cleanDaemonErrorit is a latency surprise rather than a correctness problem, but plumbingtimeoutthrough would be a reasonable follow-up.Happy to fold either in if you would prefer them handled together.