locking: deal with clock skew - #10077
Open
ThomasWaldmann wants to merge 7 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10077 +/- ##
==========================================
+ Coverage 87.01% 87.06% +0.05%
==========================================
Files 99 99
Lines 17615 17669 +54
Branches 2664 2678 +14
==========================================
+ Hits 15327 15383 +56
+ Misses 1593 1591 -2
Partials 695 695 ☔ View full report in Codecov by Harness. |
ThomasWaldmann
force-pushed
the
clock-skew-9870
branch
3 times, most recently
from
August 13, 2026 18:54
3445c28 to
722b7e9
Compare
ThomasWaldmann
force-pushed
the
clock-skew-9870
branch
from
August 14, 2026 10:27
c2f1a0c to
3031f0d
Compare
…orgbackup#9870 Lock staleness was judged by comparing a lock's content timestamp (stamped by its writer's clock) against the reader's local clock. A client whose clock runs >30 min ahead would thus kill another client's healthy lock and could then e.g. run compact deleting chunks the victim still references - a finished archive referencing deleted chunks. Fix: a lock may only be considered stale by age if it looks stale in BOTH clock domains: - writer/local clock domain: local now vs. lock content timestamp (the pre-existing rule), AND - store clock domain: store "now" vs. the lock object's store-side mtime (new, using borgstore's ItemInfo.mtime). Store "now" is derived from our own lock object's mtime plus elapsed monotonic time, so all store-domain comparisons happen within the store's own clock domain: neither the clients' nor the store's absolute clock error matters. A client without an own lock object defers the kill until acquire() has created one (a listing made right afterwards confirms or vetoes the candidates). Store-side mtimes are advisory only: they can veto a kill, but they can never cause one on their own, so a hostile or broken store gains no new capabilities (it can already delete locks or serve fabricated fresh ones - lock objects are unauthenticated). For the same reason, the process_alive() check now runs *first* and is never vetoed by store timestamps: if the lock owner is a process on our own machine and it is dead, we know that locally, and a store serving bogus, always-fresh mtimes must not be able to keep an abandoned lock alive forever and block us. Backends without store-side mtimes (e.g. rclone: mtime == 0) keep the previous behavior. Additionally, since each lock object carries two timestamps of the same write instant (content time = writer clock, mtime = store clock), the writers' per-store clock offsets are comparable: on acquire, borg now warns (once) if another active client's clock is skewed by more than MAX_MUTUAL_CLOCK_SKEW (5 min) against ours - diagnosis only, never an abort, so spoofed store timestamps cannot block backups. The manifest-timestamp behavior is intentionally unchanged. ItemInfo.mtime requires borgstore 0.6.1, so the borgstore requirement is bumped accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ackup#9870 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The skew check only ran on acquire success, but the listing that satisfies an exclusive acquire can only contain our own lock, so exclusive commands could never warn about a moderately skewed peer. Checking each listing in _get_locks also warns when acquire times out on a skewed peer's lock, and makes the last_seen_locks replay machinery unnecessary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…orgbackup#9870 The stale-veto path warned unconditionally, but a veto is not by itself evidence of skew: after a suspend, our store "now" estimate lags (monotonic clock stood still), so a genuinely stale foreign lock of a perfectly synced client gets vetoed and produced a bogus "clock skew of ~0s" warning. The per-listing skew check in _get_locks already covers the vetoed lock with a proper magnitude gate, so the veto-path warning (and with it the optional-skew calling convention of _warn_clock_skew) can just go away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, see borgbackup#9870 borg with-lock runs its LockRefresher thread without serialization against the main thread, and terminate()'s bounded join can leave a wedged refresh() running while the main thread releases. The anchor state (store key, content timestamp, store mtime, monotonic) was spread over separate attributes, so such an interleaving could raise KeyError (my_lock_key rebound between the harvest's membership test and subscript), raise TypeError (fields nulled between _store_now's guard and use), or silently pair one lock object's mtime with another's monotonic/content timestamp, skewing _store_now and _mutual_skew by up to the refresh interval. A LockAnchor namedtuple replaced as a whole plus single-read locals makes every observed state internally consistent; the harvest only updates an anchor still describing the same lock object. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ckup#9870 The anchor is a store-clock calibration, not a property of the lock object, so deleting our transient lock does not invalidate it. Keeping it lets an acquire that is blocked by a healthy-but-skewed lock veto the kill on the first listing of every retry (2 store round-trips) instead of re-running the defer/create/veto/delete cycle (7 round-trips plus lock churn) each time. Safe: store times stay veto-only, so a kept anchor can never cause a kill. To bound the mis-veto window of an anchor frozen by a suspend, _store_now now refuses anchors older than the stale timeout, with age measured by our wall clock (which, unlike time.monotonic(), keeps counting while suspended); this also defuses the leftover-anchor hazard of the break_lock and refresh-abort paths. Also document the accepted residual risk of a store clock stepping backwards by more than the stale timeout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… see borgbackup#9870 write_raw_lock duplicated _create_lock's wire format (field layout, timestamp format, sha256 key, store path), so a future format change (e.g. AEAD lock objects) would have made the skew tests silently keep writing the old format. _create_lock gained an explicit content timestamp parameter instead; the helper keeps only the store-side mtime override. The free_pid fixture was a verbatim copy of the one in fslocking_test.py - import it like platform_test.py does (incl. the same per-file F811 ignore). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ThomasWaldmann
force-pushed
the
clock-skew-9870
branch
from
August 14, 2026 11:04
3031f0d to
52a91e4
Compare
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 #9870 (the locking part; the manifest-timestamp part was declared out of scope there — the borg2 manifest is a borg1 remnant and its timestamp check intentionally stays as a whole-repo rollback tripwire).
Two pre-existing fixes found during this PR's review were split out into #10096 (vanished-lock
ObjectNotFoundrace,with-lockepilog); this PR is rebased on top of that merge.Problem
Lock staleness was judged by comparing a lock's content timestamp (stamped by its writer's clock) against the reader's local clock. A client whose clock runs >30 min ahead stale-kills another client's healthy lock and can then e.g. run
compact, deleting chunks the victim still dedup-references for up to ~15 min — a finished archive referencing deleted chunks.Fix
AND-rule staleness — a lock may only be killed by age if it looks stale in BOTH clock domains:
ItemInfo.mtime, see ItemInfo: add mtime field (storage-side modification timestamp) borgstore#212).Store "now" is derived from our own lock object's mtime + elapsed monotonic time, so store-domain comparisons stay entirely within the store's clock domain: neither the clients' nor the store's absolute clock error matters (a third-party-hosted store may have a wrong clock — it only serves as a common reference). A client without an own lock object defers the kill decision until
acquire()has created one; the listing right afterwards confirms or vetoes the candidates.Threat model: store-side mtimes are advisory only — they can veto a kill, never cause one — so a hostile or broken store gains no new capabilities (lock objects are unauthenticated, so it can already delete them or serve fabricated fresh ones). For the same reason the
process_alive()check runs first and is never vetoed by store timestamps: if the lock owner is a process on our own machine and it is dead, we know that locally, and a store serving bogus always-fresh mtimes must not be able to keep an abandoned lock alive forever and block us. Backends without store-side mtimes (rclone, old rest servers) keep the previous behavior. This also composes with future AEAD locks: authentication makes the content timestamp trustworthy, but doesn't fix a skewed writer clock, so the advisory mtime cross-check stays the skew-immunity mechanism; all lock parsing remains centralized in_get_locks().Skew warning — each lock object carries two timestamps of the same write instant (content time = writer clock, mtime = store clock), so writers' clock offsets are comparable with the store's absolute error cancelled. borg checks this on every lock listing and warns (once per lock instance) when another active client's clock differs by more than
MAX_MUTUAL_CLOCK_SKEW(5 min) — including for exclusive acquirers, which only ever see the skewed peer in intermediate listings, and before giving up withLockTimeout. Diagnosis only, never an abort, so spoofed store timestamps cannot block backups.Plus docs (FAQ note recommending synchronized clocks for shared repos; the internals lock description in
data-structures.rstupdated for the new algorithm) and the borgstore pin bump to~= 0.6.1.Hardening after review
An adversarial multi-agent review of this PR produced 15 verified findings; 12 are fixed in the follow-up commits here (kept separate for reviewability), 2 pre-existing ones moved to #10096, one maintainability nit was deliberately left:
LockAnchornamedtuple that is only ever replaced as a whole, so an unserializedLockRefresherthread (borg with-lock) can no longer cause KeyError/TypeError or silently pair one lock object's mtime with another's anchor.time.monotonic()stood still) no longer produces a bogus "clock skew of ~0s" warning._store_now()refuses anchors older than the stale timeout (wall-clock age counts suspends; monotonic does not), bounding any suspend-frozen anchor.write_raw_lockreuses_create_lockvia a new explicit content-timestamp parameter (the lock wire format lives in one place, ahead of AEAD locks), andfree_pidis imported fromfslocking_testlikeplatform_testalready does.Out of scope, noted for a separate PR: serializing
with-lock'sLockRefresheragainst the main thread (pre-existing race class; fuse/hlfuse/webdav already pass a serialization lock).Verification
file:repo with injected foreign locks: skewed-writer lock survivesrepo-listwithClock skew of ~2400s detected ... 'slowclockhost'logged; genuinely stale lock removed; operations succeed. Also: an abandoned exclusive lock of a dead local PID carrying a fresh store mtime does not blockborg create(it is cleaned via process_alive).borgstore 0.6.1 (with
ItemInfo.mtime) is released on PyPI, so the pin bump included here is satisfiable.History: the feature itself is the first (squashed, rebased onto master) commit; the review hardening follows as separate commits.
🤖 Generated with Claude Code