fix(release): decide lock takeover on owner liveness, not lockfile age - #1055
fix(release): decide lock takeover on owner liveness, not lockfile age#1055BryanFRD wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
The design is the right call, including the "block rather than steal" trade-off and leaving the mtime refresh out. Two blocking findings, both about the liveness check being wrong at the host boundary: the env-var hostname makes the same-host branch dead code on Linux (and, I think, red CI on this commit), and the Windows path reads an access-denied process as dead. One nit on pid parsing.
There was a problem hiding this comment.
Liveness over mtime is the right call, and the tests are the kind that fail for the right reason (a_dead_pid asserting its own premise, the foreign-host test using a live local PID). One blocking issue on the Windows path, plus two nits.
One more thing not covered by the host check: equal hostnames do not imply the same PID namespace. Two containers with a pinned hostname sharing a bind-mounted repo compare PIDs from different namespaces, where low PIDs collide readily, and that can steal a live lock as well as block on a dead one. Stamping something namespace-specific alongside the host (the /proc/self/ns/pid inode on Linux, absent elsewhere) and requiring it to match would close it, and an unrecognised or missing marker falls back to the TTL exactly like a foreign host. Not needed for this PR, but the deliberate trade-off in the description reads as if the same-host case is fully sound, and this is the case where it is not.
Skipping the mtime refresh is fine as argued, given the TTL is no longer the mechanism on the host that matters.
SonarQube — aucune nouvelle issueComparaison entre le projet bac à sable de cette PR et la branche par défaut : SonarQube Community n'analyse pas les PR, ce delta est calculé côté CI. Détail |
144ecfd to
03442d4
Compare
|
All four applied. Access denied on Windows (03442d4): The test uses pid 1 on unix and pid 4 on Windows, per your suggestion. Process-group pids (03442d4): Recursion on a failed removal (03442d4): both branches return Docs: the takeover paragraph is rewritten in Hostname: as you noted, already fixed in 91f5859 before this round. |
03442d4 to
5d526a1
Compare
5d526a1 to
3e0aa6f
Compare
…failed removal recursing
3e0aa6f to
a3b67ec
Compare
Closes #792.
ReleaseLockwrote a PID and a hostname into the lockfile and then never read either.take_over_if_stalelooked only at the mtime, which is written once at acquire and so records the start time rather than any liveness signal. Two consequences, both reproduced as failing tests before the fix:Takeover is now decided by asking whether the owner is still alive:
STALE_LOCK_TTLdecides, as before.unknown. Treated as another host. It is the fallback when neitherHOSTNAMEnorCOMPUTERNAMEis set, so two machines can both claim it and their PIDs would be compared meaninglessly.STALE_LOCK_TTLgoes from 30 minutes to 6 hours. It is now only the fallback for an owner on a machine we cannot query, where hours is the honest number.Liveness uses
kill(pid, 0)on unix (treatingEPERMas alive, since the process exists but belongs to another user) andOpenProcess+WaitForSingleObjecton Windows.WaitForSingleObjectavoids theGetExitCodeProcessambiguity where a process that legitimately exits with 259 is indistinguishable fromSTILL_ACTIVE. Bothlibcandwindows-syswere already in the dependency tree, so this adds no new compilation.One thing the issue asked for that this does not do
The issue also asked to refresh the lockfile mtime through the run so the TTL measures staleness rather than duration. That is unnecessary under this design and I left it out: on the same host the TTL no longer applies at all, and the same-host case is the one both reported bugs are in. Adding a refresh would mean threading a touch through the release pipeline (the long stretch is inside a single phase, so the existing phase boundaries are the wrong seam anyway) to buy nothing. Happy to add it if you disagree.
There is one deliberate trade-off. If a dead owner's PID has been reused by an unrelated live process, the lock blocks until
--force-unlockrather than expiring on the TTL. That direction is chosen on purpose: blocking wrongly costs one documented manual step, while stealing a live lock corrupts a release. It is also the direction the issue asks for, since "a live release past 30 minutes loses its lock" is the bug being fixed.Verification
Against the previous mtime-only rule:
a_dead_pid()spawns a real process, reaps it, and assertsprocess_is_alivereports it dead before the test proceeds, so a test that would otherwise pass vacuously fails loudly instead.another_hosts_lock_is_still_judged_on_the_ttl_aloneuses this process's own live PID under a foreign hostname, so it fails if the host check is ever dropped.an_unnamed_host_is_never_trusted_for_livenessandan_unreadable_lockfile_falls_back_to_the_ttlcover the two fallback paths.These ran on Windows, so the
OpenProcesspath is exercised rather than only compiled.Top of a stack of four. Based on #1054.