Skip to content

馃悰 fix(soft): break stale markers without emptying the lock path - #843

Merged
gaborbernat merged 22 commits into
mainfrom
fix/soft-marker-reuse
Oct 17, 2026
Merged

gaborbernat merged 22 commits into
mainfrom
fix/soft-marker-reuse

Conversation

@gaborbernat

@gaborbernat gaborbernat commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

The soft locks identify the marker (the file whose presence says a process holds the lock) by its inode number (the kernel's identity for a file, independent of its name) and modification time. They then acted on the lock path by name. Each of the steps below could remove, publish or displace the wrong file, so two processes held one SoftFileLock, MarkerSoftFileLock or SoftFileLease at once. 馃悰 After this PR a contender never leaves the lock path empty while it breaks a marker. Where it cannot avoid that, it does not move the marker at all, as stale lock detection now describes. This resolves GHSA-2g95-wg33-c5x2, GHSA-fq56-3cx3-fjxp and GHSA-crv4-6mhj-j5jh.

A restored marker followed a symlink

When a break found that the marker it had renamed aside was recreated, the restore branch from #786 checked the break name with lstat and then called os.link by name. On CPython 3.14 on Linux that call follows symlinks. A symlink placed under the break name between the two calls therefore got its target published at the lock path as a hard link (a second name for the same file). The restore now links without following, the behavior link(2) gives without AT_SYMLINK_FOLLOW. A swapped-in symlink is then at worst linked as itself and read as a malformed marker. With the exchange below, this path remains only on Windows.

sequenceDiagram
    participant C as contender
    participant P as path lock.break.*
    C->>P: lstat: regular file
    Note over P: replaced by a symlink to another file
    rect rgb(255, 214, 214)
    C->>P: os.link follows it: target linked at the lock path
    end
    rect rgb(216, 243, 220)
    C->>P: link with follow_symlinks=False: the symlink itself
    end
Loading

The judged marker's inode number was reused

A contender read the marker, closed it and decided its owner was gone. Only then did it compare the inode number and the modification time before breaking. Once the stale marker was unlinked, nothing kept its inode allocated. ext4 hands a freed number to the next file created, so the next holder's marker could carry it. A modification time no newer than the one read passed the second check, a time set in the future included. All three soft locks now hold the marker open through pin_inode, with O_PATH where open(2) has it, from before the read until the break ends. They give up when the file they read is not the pinned one, and they compare modification times for equality. Windows pins nothing, since it already refuses to rename a marker its holder keeps open.

sequenceDiagram
    participant C as contender
    participant S as stale marker (inode I)
    participant N as next holder
    C->>S: read record, judge owner gone
    S-->>S: unlinked by a peer, inode I freed
    N->>N: create marker, gets inode I
    rect rgb(255, 214, 214)
    C->>N: inode I matches: break the live marker
    end
    rect rgb(216, 243, 220)
    Note over C,S: now C holds I open, so N gets another number
    end
Loading

The malformed-marker watch inherited another file's age

A contender evicts a malformed marker once it has seen it unchanged for two seconds. _marker_unchanged_for told two looks apart by modification time and inode number, the key #796 introduced. Another file can repeat both, since its owner sets the time and ext4 reuses the number. A fresh, still-empty marker then inherited the time spent watching that file and was evicted milliseconds after creation. SoftFileLease uses the same watch for expiry. The key now includes st_ctime_ns, the change time, which inode(7) defines as set by the kernel on every change and which no process can set.

stateDiagram-v2
    [*] --> Watching: first look (mtime, ino, ctime)
    Watching --> Watching: same key, age grows
    Watching --> Restart: any key part differs
    Restart --> Watching
    Watching --> Evict: unchanged for 2 s
Loading

A failed acquire unlinked a successor's marker

When writing a new marker's record failed, the rollback from #614 closed the descriptor first. It then unlinked the lock path if lstat still reported the marker's (st_dev, st_ino). If a contender had already evicted the still-empty marker, the close freed its inode, the next holder's marker could reuse the number, and the rollback removed that live marker. The rollback now removes its own file while the descriptor is still open, through remove_own_file, the order _release already uses.

sequenceDiagram
    participant A as failing acquirer
    participant B as contender
    participant N as next holder
    B->>A: evict A's empty marker
    rect rgb(255, 214, 214)
    A->>A: close fd, inode freed
    N->>N: new marker reuses the number
    A->>N: identity matches: unlink
    end
    rect rgb(216, 243, 220)
    Note over A: now A removes its file before the close
    end
Loading

The ENOSYS fallback deleted a peer's marker

When flock fails with ENOSYS, the lock removes the empty file its own O_EXCL open created before it switches to SoftFileLock. Since #480, and still after #785, _switch_to_soft_lock closed the file, compared identities and unlinked the path by name. A waiting soft lock evicts an empty file after two seconds and writes its own marker there. A process stalled past those two seconds therefore removed the marker that peer had written. The placeholder is now removed with remove_own_file while its descriptor is open, so a peer's marker found at the path is left in place.

sequenceDiagram
    participant A as creator (stalled)
    participant B as soft peer
    A->>A: lstat: still my placeholder
    B->>B: evict empty placeholder, write own marker
    rect rgb(255, 214, 214)
    A->>B: unlink by name removes B's marker
    end
    rect rgb(216, 243, 220)
    Note over A: now A checks and removes only while its file is open
    end
Loading

The path stood empty while a break decided

A stale break renamed the marker aside and only then checked whether it was the one judged stale. Until a recreated live marker went back, the lock path stood empty. A third process could create its marker there beside the live holder, and where linking failed, the rename back could replace that third marker. On Linux and macOS the contender now writes a complete marker of its own under a private name and swaps it with the lock path in one step. The swap is RENAME_EXCHANGE of renameat2(2) or RENAME_SWAP of renamex_np(2), through exchange_names. On Linux the call goes through syscall(2) with the architecture's number, since bionic before API 30 and musl before 1.2.2 have no wrapper for it.

The contender holds the lock from the moment the exchange took the pinned, judged file, and it removes that file. A peer whose exchange moves the holder's marker aside for the length of that peer's check does not change that: the peer finds the file it took is not the one it judged and swaps the marker back. A holder that releases in that window leaves a record no lock parses, so the marker the peer puts back ages out. A SoftFileLease heartbeat that finds another file at the lock path once, in that window, waits for the next refresh before it reports a compromise. If the file taken is not the judged one, the contender swaps the live marker back. Any failure before that handoff takes its own marker off whichever name holds it and puts back what it took.

The marker swapped in is the record the lock class writes, so a SoftFileLease contender swaps in a lease record with its own token and duration. Windows keeps the rename aside: CreateFile without FILE_SHARE_DELETE makes a live holder's open marker refuse the rename. Elsewhere without an exchange, including NFS and SMB, which refuse it, break_lock_file no longer moves the marker.

sequenceDiagram
    participant C as contender
    participant L as lock path
    participant A as private name
    C->>A: write own complete marker
    C->>L: exchange A and L in one step
    Note over L: never empty: holds C's marker
    alt the file taken is the judged one
        C->>A: remove the stale marker
        rect rgb(216, 243, 220)
        Note over C,L: C holds the lock
        end
    else a peer recreated the marker
        C->>L: exchange back
        C->>A: remove own marker
        rect rgb(207, 228, 255)
        Note over C,L: live holder keeps the lock
        end
    end
Loading

A killed breaker stranded the marker it took

A breaker killed between its exchange and the put-back (SIGKILL, the out-of-memory killer) leaves two files behind. Its own marker sits at the lock path and names its now-dead PID. The live holder's marker sits under the breaker's private <lock>.break.<pid>.<token> name. The next contender reads the marker at the path as stale, takes it over, and holds the lock beside the first holder.

The private name carries the breaker's PID, so a contender that is about to take over a dead breaker's marker now looks for names with that prefix first. It exchanges back a marker whose owner is alive or cannot be proven dead and removes one whose owner is dead. A marker counts as the breaker's own only when it names the breaker's process on this host; the same process number on another host, as on a shared filesystem, is a holder and is exchanged back. Where the filesystem refuses the exchange, the lock counts as held until timeout or break_lock(), as it does wherever no exchange is possible. The placeholder a break writes while it works now records its PID too, so a contender that evicts a placeholder a killed process left finds the same names.

Two contenders can find the same stranded record. Exchanging it takes four separate steps, so one contender's exchange could be reversed by the other's, and the first one's final unlink then removed the live holder's marker. A recoverer now first renames the record to a name of its own, <lock>.restore.<pid>.<token>, with its own PID. The rename succeeds for one contender only, so only that one exchanges. A .restore. record whose PID is alive counts the lock as held for that poll. One whose PID is dead is claimable again. If the lock path changed since the check, the record goes back under a new break name, and an exchange that took a peer's marker is swapped straight back. A claim that a failed put-back left under the recoverer's own PID is retried on the next poll, unless another thread of the same process is still working on it.

sequenceDiagram
    participant B as breaker (killed)
    participant L as lock path
    participant A as private name
    participant H as live holder
    participant C as contender
    B->>L: exchange: B's marker at the path
    Note over A: H's marker now sits under the private name
    rect rgb(255, 214, 214)
    Note over B: killed before the put-back
    C->>L: marker names a dead PID: take it over
    Note over C,H: C and H both hold the lock
    end
    rect rgb(216, 243, 220)
    C->>A: look for names with the dead PID first
    C->>L: exchange H's marker back, remove B's
    Note over H: H keeps the lock, C waits
    end
Loading

The lifetime break judged a marker by a reused inode number

_try_break_expired_lock, the age-based break of #476, read the marker's age with lstat and then broke it by path, with nothing holding the judged inode. A successor's marker created on the freed number with the same modification time passed the break's check and was removed. The contender then acquired beside it. SoftFileLock now judges the age under the same pin and takes the marker over by the same exchange, so the lock is held in the same poll. The base implementation is a no-op, since only existence locks accept a lifetime.

sequenceDiagram
    participant C as contender (lifetime)
    participant E as expired marker (inode I)
    participant N as next holder
    C->>E: lstat: older than lifetime
    E-->>E: broken by a peer, inode I freed
    N->>N: create marker, gets inode I and the same mtime
    rect rgb(255, 214, 214)
    C->>N: inode I matches: remove the live marker
    end
    rect rgb(216, 243, 220)
    Note over C,E: now C holds I open through the exchange, N gets another number
    end
Loading

Behavior changes

On NFS, SMB and other filesystems or platforms without an atomic exchange, except Windows, a stale or expired marker is no longer removed. It waits for timeout or break_lock(). On Linux and macOS a contender that breaks a stale marker holds the lock in the same poll. Any change to a malformed marker restarts the two-second watch.

@gaborbernat gaborbernat changed the title fix(soft): act on the marker that was judged, not its name 馃悰 fix(soft): act on the judged marker, not its path Oct 16, 2026
@gaborbernat
gaborbernat force-pushed the fix/soft-owner-identity branch from 1172d46 to 8130fc6 Compare October 16, 2026 02:07
@gaborbernat
gaborbernat force-pushed the fix/soft-marker-reuse branch from a8eaffd to 5cfdf62 Compare October 16, 2026 02:07
@gaborbernat
gaborbernat force-pushed the fix/soft-owner-identity branch from 8130fc6 to d5e4069 Compare October 16, 2026 02:36
@gaborbernat
gaborbernat force-pushed the fix/soft-marker-reuse branch 2 times, most recently from 874f972 to db4696a Compare October 16, 2026 03:04
@gaborbernat
gaborbernat force-pushed the fix/soft-owner-identity branch from d5e4069 to faf4a11 Compare October 16, 2026 04:57
@gaborbernat
gaborbernat force-pushed the fix/soft-marker-reuse branch 2 times, most recently from fd6f986 to d551b66 Compare October 16, 2026 04:58
@gaborbernat gaborbernat changed the title 馃悰 fix(soft): act on the judged marker, not its path 馃悰 fix(soft): break stale markers without emptying the lock path Oct 16, 2026
@gaborbernat
gaborbernat force-pushed the fix/soft-marker-reuse branch 2 times, most recently from 706a2ee to 78961af Compare October 16, 2026 05:50
@gaborbernat gaborbernat added the security Fixes a published or draft security advisory label Oct 16, 2026
@gaborbernat
gaborbernat force-pushed the fix/soft-marker-reuse branch from 78961af to 2e78892 Compare October 16, 2026 06:09
@gaborbernat
gaborbernat force-pushed the fix/soft-owner-identity branch from d19532f to 97e5859 Compare October 16, 2026 06:29
@gaborbernat
gaborbernat force-pushed the fix/soft-marker-reuse branch 14 times, most recently from c7ab9e2 to f36cd65 Compare October 17, 2026 03:06
Base automatically changed from fix/soft-owner-identity to main October 17, 2026 09:21
gaborbernat and others added 22 commits October 17, 2026 02:39
When a stale break finds that the marker it renamed aside was recreated, it
hard-links the file back to the lock path. It checked the break name with
lstat and then called os.link by name, which follows a symlink on CPython 3.14
on Linux. A symlink that replaced the break name between the two calls made
the contender publish the symlink's target at the lock path as a second hard
link.

The restore now passes follow_symlinks=False, so a swapped-in symlink is at
worst linked as a symlink, which the next read treats as a malformed marker.
Where the runtime cannot honor the option, the marker stays under the break
name, as it already does without os.link.
A contender read a marker, closed it, decided its owner was gone and only then
called break_lock_file(), which recognised the judged marker by inode number
and a modification time no newer than the one read. Once the stale marker was
unlinked nothing kept its inode allocated, and ext4 hands a freed number to
the next file created, so the next holder's marker could pass the check and
the contender renamed and removed a live lock. A modification time set in the
future, which the file's owner may choose, made the second check pass every
time.

SoftFileLock, MarkerSoftFileLock and SoftFileLease now hold the marker open,
with O_PATH where it exists, from before the read until the break returns, and
give up when the node they read is not the one they pinned. break_lock_file()
compares modification times for equality. Windows already refuses to rename a
holder's open marker, so nothing is pinned there.
A waiting contender evicts a malformed marker once it has seen it unchanged
for two seconds, and it told two looks apart by modification time and inode
number alone. Another file can repeat both: its owner sets the mtime, and ext4
gives a freed inode number to the next file created. A fresh, still-empty
marker that matched an older file's pair then inherited the time spent
watching that file and was evicted milliseconds after its creation, which let
a second holder in. SoftFileLease uses the same watch for lease expiry.

The watch key now includes st_ctime_ns, which no process can set and which
every create and change moves.
When writing the record of a new marker failed, the rollback closed the
descriptor first and then removed the lock path if lstat still reported the
marker's (st_dev, st_ino). If a contender had already evicted the still-empty
marker, the close freed its inode, the next holder's marker could reuse the
number on ext4, and the rollback unlinked that live marker. The release path
already removes the marker before closing it for this reason.

The rollback now breaks its marker while the descriptor is still open, with
the rename and re-check a stale break uses, and closes it afterwards. Windows,
which cannot rename an open file, keeps unlinking after the close.
When flock fails with ENOSYS, the lock removes the empty file its own O_EXCL
open created before it switches to SoftFileLock. It closed that file, compared
the lock path's identity with it, and unlinked the path by name. A waiting
soft contender evicts an empty file after two seconds and writes its own
marker there, so a process stalled between the check and the unlink removed a
live peer's marker and acquired beside it. A stall before the check had the
same effect where the freed inode number went to the peer's marker.

The placeholder is now removed with the rename and re-check a stale break
uses while its descriptor is still open, so a peer's marker found at the path
is put back instead of deleted.
A stale break that renamed aside a marker a peer had just recreated put it
back with os.link. Without os.link (Termux), on a filesystem that refuses hard
links, or where the link cannot be told not to follow a symlink, the live
marker stayed under the break name, the lock path was left free, and the next
contender acquired beside the live holder.

Where linking back fails, the break now renames the marker back while the lock
path is still free. Rename never follows a symlink, so a symlink swapped in
under the break name is at worst published as a symlink, which the next read
treats as a malformed marker.
Windows pins nothing, so the check for a node replaced since the pin never
fires there, and Termux has no os.link, so linking a marker back never runs
there; mark both for coverage on those platforms.
A stale break renamed the marker aside and only then checked whether it was
the one judged stale. Until a recreated live marker was linked or renamed
back, the lock path stood empty and a third process could create its marker
beside the live holder; where linking failed, the rename back could replace
that third marker.

On Linux and macOS the contender now writes a complete marker of its own under
a private name and swaps it with the lock path in one renameat2 or
renamex_np exchange. If the file it took is the judged one, it removes that
file and holds the lock in the same poll; otherwise it swaps the live marker
back. Any failure before the handoff takes its own marker off the path and
puts back what it took. Windows keeps renaming aside, since it refuses to
rename a marker its live holder has open. Elsewhere without an exchange,
including NFS and SMB, the marker is no longer moved and counts as held. The
lifetime break and a failed acquire's cleanup use the same exchange.
The lifetime break read the marker's age with lstat and then broke it by
path, with nothing keeping the judged inode allocated. A successor's marker
created on the freed inode number with the same modification time passed the
break's check and was removed, and the contender acquired beside it.

SoftFileLock now judges the age under the same pin the stale break uses and
takes the marker over by the same exchange, so the judged inode cannot name
another file meanwhile and the lock is held in the same poll. The base
implementation becomes a no-op: only existence locks accept a lifetime.
Shorter sentences and paragraphs, the inode and the change time defined
where they first appear, and the two test helper modules folded into
conftest.py as fixtures, so the break's rules read without prior knowledge
of the filesystem and the tests share one place for their stand-ins.
remove_own_file sits above the break it calls, the inode-reuse fixture is
excluded from Windows coverage as a whole, and the stale-breaker table is
shared by every test that runs all three soft locks.
Two contenders that judged one stale marker both exchanged their own marker
onto the path. The second exchange moved the first contender's marker aside
before its samestat check, so the first gave up its hold, and the second put
that marker back. The marker named a live process, nobody held the lock, and
every contender waited on it.

A contender now holds the lock as soon as the file its exchange took is the
one it judged. A peer's later exchange moves the holder's marker aside only
while that peer checks what it took, and the peer puts it back.
A holder that released while a breaker's exchange had its marker under a break
name found another file at the lock path and skipped the unlink. The breaker
then put the marker back, and it still named the live PID of a lock nobody
held, so every contender waited until that process exited.

The release now overwrites the record through its open descriptor with one no
lock parses. The put-back lands a marker that contenders age out after the
usual grace period.
A peer's stale break exchanges a live marker aside for the length of its
check. The heartbeat treated a missing marker once as transient, but a
refresh that found the peer's marker at the path reported owner-changed at
once, ran on_compromise and ended the heartbeat, though the break put the
marker back. The claim then stopped refreshing and a contender took it after
lease_duration.

The first owner-changed is now tolerated like the first marker-missing.
A second change in a row, which a real takeover produces, is still reported.
A breaker killed between its exchange and the put-back left its own marker
at the lock path and a live holder's marker under its break name. The
marker at the path named a dead PID, so the next contender took it over and
held the lock beside the first holder.

A contender that finds a dead breaker's marker now searches the lock's
directory for names starting with that breaker's PID before it takes the
marker over. It exchanges back a marker whose owner is alive or cannot be
proven dead, removes one whose owner is dead, and waits where the exchange
is refused. The placeholder a break writes carries its PID so an evicted
placeholder leads to the same names.
A stranded marker was treated as the dead breaker's own whenever it
named the breaker's PID. The same number on another host, on a shared
filesystem, is a live holder, so the marker is put back instead of
removed.
Two contenders that found the same dead breaker's marker could both
exchange the record stranded under its break name. The second exchange
reversed the first, and the first one's unlink then removed the live
holder's marker.

A contender now renames the record to <lock>.restore.<pid>.<token>
first, so only one of them acts on it. A record whose recoverer died
is claimable again, and an exchange that took a peer's marker is
swapped back at once.
Replaying #843 over main dropped path handling from its merge with #836.
Restore the relative-path and marker cleanup behavior, and cover a failed
released-record write when another marker takes the path.
@gaborbernat
gaborbernat force-pushed the fix/soft-marker-reuse branch from 49a086e to 737f120 Compare October 17, 2026 09:48
@gaborbernat
gaborbernat merged commit 740ff66 into main Oct 17, 2026
46 checks passed
@gaborbernat
gaborbernat deleted the fix/soft-marker-reuse branch October 17, 2026 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug security Fixes a published or draft security advisory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant