Skip to content

locking: deal with clock skew - #10077

Open
ThomasWaldmann wants to merge 7 commits into
borgbackup:masterfrom
ThomasWaldmann:clock-skew-9870
Open

locking: deal with clock skew#10077
ThomasWaldmann wants to merge 7 commits into
borgbackup:masterfrom
ThomasWaldmann:clock-skew-9870

Conversation

@ThomasWaldmann

@ThomasWaldmann ThomasWaldmann commented Aug 11, 2026

Copy link
Copy Markdown
Member

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 ObjectNotFound race, with-lock epilog); 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:

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 with LockTimeout. 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.rst updated 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:

  • torn-state safety: everything we know about our own lock object (store key, content timestamp, store-side mtime, monotonic anchor) lives in one LockAnchor namedtuple that is only ever replaced as a whole, so an unserialized LockRefresher thread (borg with-lock) can no longer cause KeyError/TypeError or silently pair one lock object's mtime with another's anchor.
  • no false alarms: skew warnings are always magnitude-gated; a veto caused by our own lagging store-time estimate (after a suspend, time.monotonic() stood still) no longer produces a bogus "clock skew of ~0s" warning.
  • efficiency: the store-clock anchor survives deletion of our transient lock object (it is a clock calibration, not a property of the object), so an acquirer blocked by a healthy-but-skewed lock vetoes the kill on the first listing of every retry (~2 store round-trips) instead of re-running the whole defer/create/veto/delete cycle (~7 round-trips plus lock-object churn). A wall-clock age cap in _store_now() refuses anchors older than the stale timeout (wall-clock age counts suspends; monotonic does not), bounding any suspend-frozen anchor.
  • documented residual risk: a store clock stepping BACK by more than the stale timeout during the anchor's lifetime defeats the veto — accepted (pre-borg2: clock skew issues #9870 there was no cross-check at all) and bounded by re-anchoring at each lock creation.
  • tests: helpers deduplicated — write_raw_lock reuses _create_lock via a new explicit content-timestamp parameter (the lock wire format lives in one place, ahead of AEAD locks), and free_pid is imported from fslocking_test like platform_test already does.

Out of scope, noted for a separate PR: serializing with-lock's LockRefresher against the main thread (pre-existing race class; fuse/hlfuse/webdav already pass a serialization lock).

Verification

  • lock testsuite: 23/23 (8 tests new in this PR: healthy-but-skewed lock survives incl. exclusive-acquire timeout without killing it; stale-in-both-domains lock killed; warning above / none below threshold; warning during exclusive acquire; no warning when only our store time lags after a suspend; no lock churn while blocked by a skewed lock; dead-process lock killed despite fresh store mtime; mtime==0 legacy fallback)
  • full local testsuite: green after the review-hardening commits (2500+ passed, 0 failed, against the released borgstore 0.6.1)
  • E2E smoke on a real file: repo with injected foreign locks: skewed-writer lock survives repo-list with Clock 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 block borg 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

@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.54839% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.06%. Comparing base (e15404a) to head (52a91e4).
⚠️ Report is 5 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/borg/storelocking.py 93.44% 2 Missing and 2 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

@ThomasWaldmann
ThomasWaldmann force-pushed the clock-skew-9870 branch 3 times, most recently from 3445c28 to 722b7e9 Compare August 13, 2026 18:54
@ThomasWaldmann ThomasWaldmann changed the title locking: make stale-lock detection immune to client clock skew locking: clock skew and fixes Aug 14, 2026
@ThomasWaldmann ThomasWaldmann changed the title locking: clock skew and fixes locking: deal with clock skew Aug 14, 2026
ThomasWaldmann and others added 7 commits August 14, 2026 13:01
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

borg2: clock skew issues

1 participant