Conversation
…ck (volcengine#4210) The PID-reuse guard added in volcengine#1088 reads /proc/<pid>/cmdline, which exists only on Linux. On macOS `_is_pid_alive()` fell back to a bare os.kill(pid, 0), so after a reboot left a stale .openviking.pid behind and the OS handed that PID to an unrelated process, every start raised DataDirectoryLocked and the server never recovered — the reporter measured ~50,000 launchd retries over seven days against a PID held by Spotlight's mdwrite. Move the identity lookup into `_process_cmdline()` and add a Darwin branch that asks `ps`. When the command line cannot be read the function returns None and the caller keeps trusting the liveness probe, so an unreadable process never causes a lock to be stolen. `test_live_pid_blocks_acquisition` and its sibling pinned PID 1, whose verdict now depends on the identity check on macOS as well as Linux (and which already failed on Windows, where os.kill(1, 0) does not succeed). Both now stub the liveness check so they test the lock semantics they are named for.
Collaborator
|
Thanks for the patch and the reproduction work. For #4210, we have decided to keep recovery from leftover PID locks after an unclean shutdown as an operator responsibility. When startup cannot establish that a lock is stale, retaining the lock and refusing startup is acceptable. The operator must verify that no OpenViking instance is using the data directory before clearing the leftover lock. We are closing this PR without merging because we are not expanding automatic recovery with platform-specific command-line identity checks. This is a scope decision; we acknowledge the startup failure described in the issue. |
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.
Fixes #4210.
The gap
The PID-reuse guard from #1088 reads
/proc/<pid>/cmdline, and that branch issys.platform.startswith("linux"). macOS has no/proc, so_is_pid_alive()degraded to a bareos.kill(pid, 0)— which answers "is a process alive", not "is OpenViking alive".The reported consequence is not a warning, it is a permanent outage: a reboot leaves
.openviking.pidbehind, the OS hands that PID to something else (Spotlight'smdwritein the incident), and every start raisesDataDirectoryLocked. The impostor never exits, so launchd retried for seven days — ~50,000 crashes, ~330 MB of logs, no path to self-healing.The change
The identity lookup moves into
_process_cmdline(pid), with the Linux/procread unchanged and a Darwin branch that asksps:_is_pid_alive()then applies the same rule it already applied on Linux: a live PID whose command line does not mentionopenvikingis a recycled PID, not a lock holder.The fail-safe direction is deliberate and tested: when the command line cannot be read —
psmissing, timing out, exiting non-zero, or answering nothing —_process_cmdline()returnsNoneand the caller keeps trusting the liveness probe. Not being able to identify a process never causes its lock to be stolen.I dropped the redundant
and "openviking-server" not in cmdlinefrom the comparison:openviking-servercontainsopenviking, so the second test could never change the outcome.Two existing tests changed, and why
test_live_pid_blocks_acquisitionandtest_error_message_includes_remediationwrite PID 1 to the lock file and expect it to block. Since #1088 that outcome has depended on the platform, because PID 1 is init/launchd, not OpenViking:mainbefore this PR —os.kill(1, 0)raises,_is_pid_alivereturns False, no exception is raised (1 failed, 4 passed)./proc/1/cmdlineisinit/systemd, which the Stale PID lock file causes cascading Gateway failure: unhandled rejections + session deadlock #1088 check rejects — so it can only pass where/proc/1/cmdlineis unreadable and theexcept OSError: passfallback runs.Both tests now stub
_is_pid_alive, so they test the lock semantics they are named for instead of the host's process table. That is a change to existing tests, so I am flagging it rather than burying it — if you would rather I leave them as they are, the alternative is askipif(sys.platform != "linux")on both, and I can switch.Tests
tests/misc/test_process_lock_pid_reuse.py— 10 tests. They monkeypatchsys.platformandsubprocess.run, so the macOS path is covered from Linux and Windows CI as well; there is no Darwin-only skip.Coverage: the recycled-PID case (a live non-OpenViking process no longer holds the lock), the real-instance case (still blocks), the exact
psargv and timeout, four "cannot tell" outcomes that must keep the lock, the end-to-end path throughacquire_data_dir_lock(), and a guard that no other platform shells out.Against
mainwith onlyopenviking/utils/process_lock.pyreverted: 10 failed. With the change: 15 passed across both lock test files (5 pre-existing + 10 new).ruff checkandruff format --checkclean on the three touched files. (ruff check tests/misc/also reports an unrelated pre-existing I001 intest_retrieval_enable_intent.py, untouched here.)