Repository navigation
fix(nfslock): a taker that dies holding lock.borrow blocks the lock for ever - #93
Merged
dhilst merged 11 commits intoOct 23, 2026
Merged
Conversation
… holding lock.borrow Assumption 5 of the protocol says a process cannot die while it holds lock.borrow. Under the CI it does: Jenkins sends SIGTERM and the OOM-killer sends SIGKILL. Step 3 is mkdir(lock.borrow), so a borrow left by a killed take-over skips steps 4 to 7 for every later acquire. The lock then names a dead owner that is never evicted, and acquire ends by telling the operator to remove the lock by hand. A dead OWNER recovered in milliseconds while a dead TAKER never did, and no test covered the second case. These checks stage a lock whose owner is dead beside a borrow whose taker is dead, and assert the lock is recovered. Three more hold the boundary: a borrow whose taker is alive is left alone, a borrow with no metadata is not assumed stale, and the borrow a take-over creates carries metadata that names it. A second group covers sweep_stranded against the lock names this project uses, including a live owner and another machine that must survive the sweep. A third asserts that a guard releases when it goes out of scope. Two red proofs are the narrow forms, not a missing subroutine: with sweep_stranded present and its matcher reading '.lock' alone, the checks for .lock.d, .lock.d.borrow and .nfslock fail; with DESTROY removed, the two scope checks fail while their control passes. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
…for ever Step 3 of the NFS lock protocol is mkdir(lock.borrow) and nothing recovers that directory. A take-over killed between step 3 and step 7 leaves it behind, so every later acquire fails step 3, skips steps 4 to 7, never evicts the dead owner, and ends by telling the operator to remove the lock by hand. Assumption 5 made this impossible by fiat; Jenkins SIGTERM and the OOM-killer make it routine. The borrow now carries the same metadata as the lock, written before the death check, which is where a signal lands. A borrow whose metadata proves its taker dead is removed and the acquire retries; one that names a live process, or that cannot be validated, is left alone. So the rule that recovers a dead owner now recovers a dead taker, and staleness is still never inferred from metadata that cannot be validated. release takes the same two steps at step 9, where a killed taker blocked the release instead. A guard also never released at scope exit, because the module had no DESTROY. The lock was then held to the end of the process and no take-over could recover it either, its owner being alive. Every caller here keeps its guards in an array for that reason, which hid the leak. release now returns early unless this acquisition holds the lock, because acquire blesses the object before it holds anything and an acquire that dies would otherwise enter the wait loop in release and never leave it. sweep_stranded moves here from the xcat-core-ci-cd copy, which had it alone, so the hole it covers stayed open in this copy and in xcat-core's. Its matcher read '.lock' and '.lock.borrow', which covered mockbuild-all.pl's cell locks and missed cluster-test.pl's .build.lock.d and sbuild-all.pl's .sbuild-all.<arch>.nfslock. It now matches all three spellings. A borrow the earlier protocol left carries no metadata, so the sweeper remains the only way to clear those. The header records that three repos vendor this file and that the copies must stay identical. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
…aimed t/nfslock_borrow_recovery.t asserts that a borrow with no metadata is NOT reclaimed, and t/nfslock.t stages one to prove a lock under another borrower is left alone. Both describe the permanent strand: the earlier protocol wrote no metadata into the borrow, so a taker killed there made the lock unacquirable for ever and no check objected. The protocol holds a metadata-less borrow for the time of two file reads, so one that two attempts find unchanged is held by nobody. The checks now assert that it is reclaimed, that a borrow touched between the attempts is not, and that a borrow whose metadata file exists but does not validate is not either - a half-finished write is still a partial read. A lock.d with no metadata is a different case and stays refused. Checks for it name the one condition under which the sweeper removes it, the caller's exclusive claim on the root. t/nfslock.t's live-borrower fixture now gives the borrow a live taker, which is what the lock must not be taken from. release returns early unless this process took the lock, so a forked child cannot free its parent's. No check covered that, and one now does. The workflow ran t/nfslock.t and not t/nfslock_borrow_recovery.t, so the new file was never run by the CI at all. It is on the list now. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
The borrow now carries metadata, so a taker that dies holding one is recovered. A borrow with NO metadata is not: _reap_borrow returns "invalid", acquire skips steps 4 to 7 for as long as the borrow exists, and the dead owner is never evicted. That is the state the earlier protocol left, because it wrote nothing into the borrow, and the state a death between mkdir and the first write leaves today. Every later acquire waits out its retries and tells the operator to remove the lock by hand. No participant holds a metadata-less borrow for longer than two file reads, and two attempts of one acquire are at least T seconds apart, so a metadata-less borrow that both attempts find unchanged is held by nobody. _reap_borrow removes that one. A borrow whose metadata file exists but does not validate may be a half-finished write and is still left alone. A lock.d with no metadata is a separate case. The build lock in xcat-core wrote none and held lock.d for the whole build, so no fact on disk tells that holder from a dead one, and acquire must keep refusing it. sweep_stranded removes it, and only with exclusive => 1, where the caller has already claimed the root and no build can be running. The refusal said "Trying to unlock <path> failed", which is the reverse of what it was doing, and gave a caller nothing to distinguish an exhausted retry budget from a mkdir that failed outright. It now reads "Cannot lock <path>: still held after N retries by <owner>". The Copies header called the xcat-dep path "this file, the original", which is false in the two copies that vendor it, and it did not say where sweep_stranded is called from. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
…s that way The refusal said "Trying to unlock <path> failed after N retries", which is the reverse of what acquire was doing, and the previous commit changed it to "Cannot lock <path>: still held after N retries by <owner>". Seven checks in t/genesis_openembedded_consumer.t and native/openeuler-srpm.t match the old text, so the CI failed on five of them; the two in native/openeuler-srpm.t are in a block the earlier run did not reach. One of the seven is an unlike(). It passed for the wrong reason, because the pattern it refuses to see cannot appear any more, so it also names the lock it must not report. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
_reap_borrow validates a borrow and then removes it by path. rmdir names a path, not the inode that was validated, so a contender stopped between those two steps removes whatever is at the path when it resumes. That can be a live taker's borrow, and two processes then take over one lock. No check covered it, because the existing ones drive one process at a time. The new checks fork Perl children and stop them at named points in the protocol, through an _at hook this commit adds to the module. The interleaving is a pipe barrier and not a delay, so a fast machine cannot hide the defect, and every child is reaped with its stdio detached. A contender that validated the borrow is released only after another one holds it, which is the order that reaches the defect: released together, the two race one mkdir, which is exclusive either way. A second check covers a live taker that has not written its metadata yet. An earlier revision reclaimed a borrow that two attempts found unchanged, and that rule cannot cover this case, because a live participant can pause for any length of time between two writes. Two more cover the destructor. An acquire whose commit write fails must return that write error and leave nothing for DESTROY to clean up, and release must stop after R retries rather than wait for metadata that cannot be validated. The sleep stub is capped, so a retry loop that lost its bound fails a named check instead of hanging the suite. Against the unfixed protocol 17 of the 56 checks fail, the ordered pair reporting that both contenders took the lock. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
…e lock _reap_borrow validated a borrow's metadata and then removed the borrow by path. rmdir names a path and not the inode that was validated, so a contender stopped between the two steps removes whatever is at the path when it resumes. Measured: it validated inode 157 and removed inode 33554970, a borrow a live taker held. Both contenders then took the lock, and the ordered pair of children in the test reports 11 where it must report 01. The borrow is now numbered. lock.borrow is generation 0 and lock.borrow.N is generation N, the taker is whoever holds the highest generation, and a transition whose taker is provably dead is ended by creating the NEXT generation rather than by removing the one that was validated. mkdir is exclusive, so one contender wins a generation, and a process removes only a generation it created and still holds. No process can delete another process's claim, which is the property the old shape could not have. The rule that reclaimed a borrow with no metadata, when two attempts found it unchanged, is withdrawn. Its argument covered a healthy participant, not one that pauses: a live taker can wait any length of time between its mkdir and its first write, and superseding it puts two processes in the transition. A generation with no valid metadata now blocks, and sweep_stranded removes it under an exclusive claim on the root, which is the disposition a lock.d with no metadata already has. acquire set identity before writing the replacement lock metadata. A write that left partial metadata and threw therefore gave DESTROY a lock to release, and release retried for ever because lock.d existed and its metadata could not be validated. identity is now set only after that write succeeds, so a failed acquire leaves the destructor nothing to act on, and release stops after R retries. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
…the lock The ownership identity is one process. What a lock protects outlives it: a build wrapper starts each command in its OWN process group, so SIGKILL on the wrapper leaves that group running. An owner proven dead therefore says nothing about the work it started, and recovery admits a second writer. No check covered it, because every holder the existing checks kill has no worker under it. attach, detach and live_workers arrive here with the checks that drive them. A caller records each process group it starts, and a group is gone only when kill(0, -pgid) answers ESRCH, which is the test the build wrapper's own cancellation path already uses before it releases. BOTH halves are asserted, and the second is the one that keeps the first honest: a lock that recovers nothing at all satisfies "the second build stays blocked" perfectly. So the worker exits and the same lock must then be recovered. Three more checks pin what "the group" means. A group outlives its leader, because children inherit it, so a check reaps the leader and asserts the group still counts as live. detach must refuse while any process is in the group, which makes the retention a property of what is on disk rather than of which call site remembered to run. And an exclusive claim on the root does not override a live group: the claim asserts no build is running, and a live group is evidence one is. Against the unfixed predicate 8 of the 75 checks fail. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
The ownership identity is one process, and what a lock protects outlives it. A build wrapper starts each command in its own process group, so SIGKILL on the wrapper leaves that group running. acquire proved the owner dead and took the lock, and two builds then wrote one checkout. The build wrapper's own cancellation path keeps the lock when a group survives cleanup, and this recovery undid that retention: it read a dead owner and took the lock the retention had kept. A holder now ATTACHES each process group it starts and DETACHES it once the group is gone. attach records lock.d/workers/<pgid>.<leader pstart>. An owner is recoverable only when it is provably dead AND every attached group is gone, so the retention is a fact recovery reads rather than a directory it has to guess about. A group is gone only when kill(0, -pgid) answers ESRCH, which is the test the cancellation path already uses before it releases. The two now agree. Reaping the LEADER is not enough, because its children inherit the group, so the test is on the group and not on the leader's pid. detach applies the same test and refuses while any process is in the group. Reuse windows. The entry name carries the leader's start time, so removing a record by name cannot remove that of a later group which reused the pgid. The group test itself cannot tell a recycled pgid from the original group and reads live for both, which costs a wait rather than admitting a second writer. Neither can tell a worker that left its group by calling setpgid itself; the cancellation path has the same limit, and the two agree. An exclusive claim on the root does not override a live group. The claim asserts no build is running, and a live group is evidence that one is. release drops the records of groups that are gone and keeps the rest, so a worker killed before its reap cannot make a lock unreleasable, and a live one stops the release because rmdir refuses a non-empty directory. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
…whole release deletes the owner metadata and marks the guard released before it knows the removal can finish. With a live attached worker the rmdir fails, and the lock is left holding workers and no owner. acquire then refuses it for ever, correctly, because there is nothing there to validate. The sweep has the same ordering: _remove deletes the metadata and then fails on the workers directory, reports "kept", and leaves the same wreck. A take-over recovers that very lock by itself, so the sweep was leaving it worse than doing nothing. A held lock is better than a half-released one. It is refused with a reason and it recovers when its owner dies; the half-released one is permanent. Four checks cover it, and each pair carries the half that keeps it honest. release with a live worker: the lock and its metadata survive, and once the worker exits the SAME guard finishes the release and a later acquire succeeds. An owner that exits without releasing: the metadata it left is intact, a later acquire is refused while the worker group exists and names that group, and once both are gone the lock is recovered. The sweep with completed worker records: the lock is removed, and a later acquire succeeds. And a lock the sweep keeps must still carry its metadata, or refusing to remove it is the same damage by another route. Without the second assertion in each pair, a release that frees nothing and a sweep that removes nothing would both pass. Against 2fa0e8b, the revision this was reviewed at, 11 of the 98 checks fail. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
…reeing release deleted the owner metadata and marked the guard released before it knew the removal could finish. With a live attached worker the rmdir failed on the non-empty directory, and the lock was left holding workers and no owner. acquire refuses that for ever, correctly, because there is nothing to validate, and the guard was already marked released, so nothing retried. A lock still HELD is better: it is refused with a reason, and it recovers when its owner dies. _remove had the same ordering. It deleted the metadata, then failed on the workers directory, reported "kept", and left the same wreck. A take-over recovers a dead owner's lock with completed worker records by itself, so the sweep was leaving that lock in a worse state than doing nothing. Both now clear the records of completed workers first, then ask whether the directory holds anything besides its metadata, and delete the metadata only when it does not. _only_metadata asks that question, because the unlink cannot be undone. release retries while something still blocks the removal, and marks the guard released only when nothing is left on disk. DESTROY and the signal handler that calls release_build_locks can therefore finish the job once the workers are gone. sweep_stranded's signature and its report fields are unchanged. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
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.
XCAT::NFSLockrecovers a lock whose owner died: a probe that kills the owner sees the next acquirer take over in 0 seconds. It does not recover from a taker that died during the take-over itself. Alock.borrowdirectory left behind makes every lateracquireexhaust its retries and exit telling the operator to remove the lock by hand, which is the permanent strand the protocol exists to prevent.acquirenow applies the same proof to a stranded borrow that it already applies to a stranded lock: read the recorded identity, prove the recorded process dead, and only then reclaim. Unreadable metadata still refuses to infer staleness.A second defect is fixed with it. The module had no
DESTROY, so a guard that went out of scope was never released and its lock was held to the end of the process, where no take-over could recover it either, because the owner was alive. Every caller in both repos happens to keep its guards in an array, which is why it was never seen.releasenow returns early unless this acquisition still holds the lock, becauseacquireblesses the object before it holds anything.Both are red-proved: 23 checks, with the borrow and
DESTROYcases failing against the unfixed module