[ragnorc]

Amends §7 of the accepted storage outcomes and restartable ownership RFC. Found while reviewing the S3 lock lease (#1097). Two defects let a restarted server take over the cluster lock from a server that was still serving and heartbeating, leaving two serving owners. The schema-apply, branch-control and cleanup boundaries all assume one writer process, so this broke them. A third defect let an owner that could not read its claim keep beating.

What went wrong

1. Silence counted on the clock alone. watch_and_claim stopped watching once a lease and a half had passed on the successor's clock. It did not check when the last listing was sent. A listing shows the beats as storage read them, which is some time after the request was sent. So an answer that stalled, or that the successor read late after a pause, counted silence that storage never showed. Example with the default 30 s lease: the successor's second listing is sent at 5 s and its answer arrives at 65 s. The successor then saw 65 s of "silence" while the owner had beaten six times.
2. The takeover deleted the claim. After its if-match, take_over ran delete_prefix on the old lease directory, superseded.json included. The owner checked only that claim and never re-read lock.json. Its next claim read came up to a third of a lease later, almost always after the delete. So it kept beating into the recreated directory and kept serving. The same delete also defeated the claim for an owner whose monotonic clock had stopped (a suspended VM), the case the claim exists for.
3. A failed claim read counted as "no claim", so an owner that could not read its claim kept beating.

The fix

• Silence is counted only up to the send of a listing that found no newer beat. The last listing is sent when the silence would end, so a takeover still happens a lease and a half after the last beat seen. A late answer only leads to another listing.
• The owner reads lock.json with the claim before every beat. lock.json is the record whose if-match decides the owner, and it never names a lock id again. The owner fences when a claim exists or the record no longer holds its lock id. That covers a takeover, a release, and a cluster force-unlock of a live server's ID. It works whatever happened to the lease directory, including after a takeover by an earlier binary that deleted it.
• A takeover no longer deletes the previous owner's lease directory. The claim stays, so an owner running 0.14.0, which reads only the claim, also stops at its next beat. Each takeover leaves at most the claim and two beats, and nothing lists __cluster/lease/.
• A check that gets no answer sends no beat. The beat is retried like an unanswered one, so a transient failure does not fence a live owner, and reads that keep failing end at the owner's deadline.
• A beat's deadline now counts from the start of its first attempt, before the reads. So a pause between a read and the send also counts against the owner.

Docs: the lease sections of docs/dev/control-plane.md, docs/user/operations/storage-failures.md, and an amendment to the storage-outcomes RFC (§7 body plus a Decision-log entry naming the sentences it replaces).

Tests

Every new or extended case below fails on main:

• lease::tests::a_late_listing_cannot_unseat_a_beating_owner_issue_1067 (new): one of the successor's first four listings is answered two leases late. The successor must refuse and write no claim, and the owner must keep its lease. On main the second listing yields Watch::Silent, a claim, and the owner fenced Superseded.
• lease::tests::a_claim_or_a_lost_record_fences_the_owner_issue_1067 (was a_claim_fences_the_owner_issue_1067): adds the record replaced, released or removed. Each fences at the next beat.
• lease::tests::an_owner_fences_itself_when_its_beats_lapse_issue_1067: adds a claim the owner cannot read, which must end in Lapsed, not endless beats.
• lease::tests::one_failed_heartbeat_does_not_fence_a_live_owner_issue_1067: adds three unanswered reads, which must not fence. This case passes on main too; it guards the retry.
• state_lock::tests::a_superseded_owner_whose_clock_stopped_fences_at_its_next_beat_issue_1067 (new): the owner and the successor each run on their own paused runtime. The owner's runtime is not driven while the successor watches and takes over, so its clock stops. When it resumes, it must fence Superseded at its next beat, with the lease directory kept and with it removed.
• state_lock::tests::a_restarted_server_takes_over_a_silent_owners_lock_issue_1067: now asserts that the claim survives the takeover.
• The DST lease scenario writes the lock records it needs and replaces the owner's record by if-match, as a takeover does.

crates/omnigraph-cluster/tests/failpoints.rs is not extended. Its fixtures are local roots, which carry no lease, and the cluster's seams are fail-only. The timing defects need control of time, which paused-time tests give exactly.

Limits that remain

• An owner whose clock stopped can still admit work until its next beat, up to a third of a lease. The RFC now says so.
• After an ordinary process pause, the lease task's deadline fences the storage scope when that task runs. Other tasks that resume first could still send a request before that, because the scope does not check the lease deadline itself.

[ragnorc]

Recommendation: revise. Do not merge af82fd4e until the mutable-key inventory matches this code. The lease fix is the right design.

Why

A restarted server could take the S3 cluster lock from a server that was still serving and still writing heartbeats. Schema apply, branch control, and cleanup each assume one writer process. Two serving owners break that contract.

What

The lease from #1097 had three defects.

1. The successor ended its watch on its own clock. A listing whose answer arrived late counted as silence that storage had not shown.
2. Takeover deleted the old lease directory, claim included. An owner that read only the claim kept beating. A suspended VM is that owner. Its monotonic clock does not advance during the suspend, so its timer is still ahead when it resumes.
3. A failed claim read counted as no claim. The owner kept beating.

How

Silence now lasts only until the send of a listing that still shows the same newest beat. The successor sends that last listing when the silence would end. A late answer leads to another listing.

Before each beat the owner reads the claim and lock.json. It sends no beat when either read has no answer. It fences when a claim exists, or when the record is missing, released, or names another lock id. Takeover leaves the old lease directory in place, so an owner that reads only the claim also stops. The if-match on lock.json remains the only election. The claim still elects nobody.

Impact

A server that is still writing heartbeats keeps the lock. A server whose lock was replaced, released, or removed stops at its next beat. An owner that cannot read the claim reaches its deadline and fences.

One gap remains, and the RFC states it. An owner whose monotonic clock stopped can still admit work until its next beat. That wait is at most one third of the lease. At the default of 30 seconds the wait is 10 seconds. At the maximum of 600 seconds the wait is 200 seconds. The storage scope does not read the lease deadline. Other tasks can send a request before the lease task runs.

Blocking

mutable_key_requests_are_registered_issue_1067 fails on this head.

crates/omnigraph/tests/forbidden_apis.rs still registers one .delete_prefix( call in omnigraph-cluster/state_lock.rs. This change removed that call from take_over. The registered reason says the delete is safe because the fence latched first and the lock id is never reused. That delete is the defect. Remove the registration.

CI on this commit fails the same assertion in Test Workspace and in Storage Upgrade Compatibility. The CI Gate fails because those two jobs fail.

Inline, crates/omnigraph-cluster/src/state_lock.rs:405: Leaving the old lease directory is the right fix. take_over must keep the claim. crates/omnigraph/tests/forbidden_apis.rs still registers one .delete_prefix( call in this file. The registered reason says the delete is safe because the fence latched first. That delete is the defect this change removes. Delete the registration. mutable_key_requests_are_registered_issue_1067 fails on this head with registered 1, observed 0. CI Test Workspace and Storage Upgrade Compatibility fail on that assertion.

Optional

1. The control-plane page says the owner fences at once on a claim or on a record that no longer holds its lock id. The same section later says a stopped clock stops at its next beat. The user page docs/user/operations/storage-failures.md also says the owner stops every storage write at once. The check runs at the next beat. Make the three sentences state that bound.

Inline, docs/dev/control-plane.md:331: "At once" disagrees with the stopped-clock sentence later in this section. The owner reads the claim and lock.json at its next beat. That beat can be one third of a lease later. docs/user/operations/storage-failures.md also says the owner stops every storage write at once. State the next-beat bound in all three sentences.

2. LOCK_RECORD_MAX_BYTES in lease.rs repeats the literal 64 * 1024 used for lock.json reads in state_lock.rs. One constant should own the bound. A smaller owner bound makes holds return a limit error, so a live server stops at its deadline.

Inline, crates/omnigraph-cluster/src/lease.rs:138: This bound repeats the literal 64 * 1024 on the lock.json reads in state_lock.rs. One constant should own it. A smaller bound here makes holds return a limit error, and a live server then stops at its deadline.

Tradeoffs and liability

This removes an unbounded period with two serving owners. The change keeps the one if-match election.

The new cost is one read of lock.json before each beat, on the lease client, outside the storage scope. The beat interval is one third of the lease. That read is the right place to sample ownership. A read on every graph request would close the stopped-clock window and would add a GET to the serving path. Leave the window as the RFC states it.

Takeover now leaves the claim and at most two beats under the old lock id. Nothing lists __cluster/lease/. Clean release still deletes the releaser's own directory. The leftover objects are the evidence an older binary needs. Deleting them let a live owner keep serving. The number of old lock ids grows with each takeover. Each id holds a few small objects.

Beat::holds is the one owner-side check. watch_and_claim answers a different question, which is whether beats went silent. The claim and lock.json both have to stay. holds already reads both. Put any later ownership signal in holds. The if-match stays the one election.

Five more fixes of this kind should extend holds and watch_and_claim. They should keep a single object as the election.

#1097 added the lease and made the claim elect nobody. This change keeps that rule and closes the two ways a live owner still lost the lock. Open #1176 re-issues a lost conditional write and reads the object back. Success there is equal bytes, so another successor's record stays a conflict. This takeover can keep treating Some as "this record applied". Open #1214 would send maintenance through the serving owner. That is safe only while one process serves. This change is the lease condition that work needs. The remaining third of a lease is the limit that work would still have to name.

The lease stays S3-only. Lance's object-store guide requires the store to honor conditional puts, and OmniGraph does not check that for the caller. Local and Azure locks stay unleased, as the RFC requires. This change adds no second engine path for another cloud.

Validation

Merge base: 7b3a1c9190730d31391c225a63c7b6171146bc74 (current origin/main).
Reviewed head: af82fd4e3ccb89c08d31821caebca1439fa0b85b.

GQT cannot express a late listing, a stopped clock, or a failed claim read. The owner is the in-source cluster tests. No .gqt case is required.

Local, at the head:

CARGO_TARGET_DIR=/workspace/target cargo test -p omnigraph-cluster --lib --offline -- issue_1067

Result: 16 passed, 0 failed, in 6.01s. The run includes a_late_listing_cannot_unseat_a_beating_owner_issue_1067, a_claim_or_a_lost_record_fences_the_owner_issue_1067, an_owner_fences_itself_when_its_beats_lapse_issue_1067, one_failed_heartbeat_does_not_fence_a_live_owner_issue_1067, a_superseded_owner_whose_clock_stopped_fences_at_its_next_beat_issue_1067, and a_restarted_server_takes_over_a_silent_owners_lock_issue_1067.

Local, merge-base behavior. Temporary tests were added in an isolated checkout and then removed. Nothing was pushed.

cargo test -p omnigraph-cluster --lib --offline -- a_late_listing_cannot_unseat_a_beating_owner_issue_1067 an_unreadable_claim_must_not_count_as_absence_issue_1067 a_superseded_owner_whose_clock_stopped_fences_at_its_next_beat_issue_1067

Result: 0 passed, 3 failed.

• Late listing: the successor returned Watch::Silent. The test required Watch::Alive. A listing answered two leases late counted as silence while the owner was still beating.
• Unreadable claim: the fence was None. The test required Some(Lapsed). The owner did not fence within one lease.
• Stopped clock: the fence was None. The test required Some(Superseded). After takeover the resumed owner did not fence within one lease.

Local inventory, at the head:

cargo test -p omnigraph-engine --test forbidden_apis --offline -- mutable_key_requests_are_registered_issue_1067 -- --exact

Result: failed in 1.64s. The panic text is omnigraph-cluster/state_lock.rs .delete_prefix(: registered 1, observed 0.

The same test, with only merge-base state_lock.rs in place and then restored, passed in 1.61s. The head failure is that one removed call.

CI on this exact head, workflow run 41245761322:

• Test Workspace and Storage Upgrade Compatibility failed on the same assertion.
• The CI Gate failed because those two jobs failed.
• Clippy, rustfmt, GQT, the DST pinned suite, RustFS, Azurite, and the server aws job passed.
• Test Workspace cancelled the remaining tests after this failure. CI did not report the new lease tests. The local run did.

Limitations:

• No live S3. The tests use the in-memory store. list_dir calls list_with_delimiter, and OmniGraph does not cache that result. object_store 0.14.2 clamps a ranged get that runs past a short object, so the 64 KiB lock read returns a normal lock record. AWS S3 returns a short object when the range starts inside it. A store that ignores conditional puts is already outside this lease. Lance's object-store guide states that requirement.
• The DST lease scenario was not re-run here. The DST pinned suite on this head passed in CI.
• The extra GET was not timed.

[ragnorc]

Recommendation: ready.

Why

The review of af82fd4e asked for two edits. This commit makes both. The lease design is unchanged. A takeover still leaves the old lease directory in place. Before each beat the owner still reads the claim and lock.json.

What this commit changes

The mutable-key inventory drops its registration of .delete_prefix( in omnigraph-cluster/state_lock.rs. take_over does not make that call.

state_lock::LOCK_RECORD_MAX_BYTES is 64 KiB. It is the limit on each read of lock.json that fences the owner or that returns a version for if-match. lease.rs no longer has its own copy of that number.

The control-plane page and the storage-failure page say the owner stops on a claim, or on a record that no longer holds its lock id, at its next beat. That wait is at most one third of a lease. Only a missed heartbeat uses a timer of its own. That timer is two thirds of a lease after the start of the last acknowledged beat.

Status of the earlier findings

The blocking finding is fixed. On af82fd4e, mutable_key_requests_are_registered_issue_1067 fails because the inventory still registers one .delete_prefix( in state_lock.rs and the code has zero. This commit deletes that registration. The same test passes on this head.

The first optional finding is fixed. The control-plane page said the owner fences at once on a claim or a lost record. It now says the reads before the next beat do that, at most one third of a lease later. The storage-failure page said the owner stops every storage write at once in that case. It now says the owner stops at its next heartbeat, with the same bound. The stopped-clock sentence still says the claim or the replaced record stops the owner at its next beat. The three sentences agree.

The second optional finding is fixed for every read that can fence a live S3 server or that decides a conditional update. Those reads are Beat::holds, the acquire refresh, release_settled_lock, read_bootstrap_lock, and ClusterStore::read_versioned_opt when the URI is the lock file. Each passes LOCK_RECORD_MAX_BYTES. The new optional note covers the two drop-time reads that stay outside the constant.

The gap the RFC states is still open. The storage scope does not read the lease deadline. This commit does not say that it does. PR 1221 is the change that closes that gap. This review treats that PR as context only.

Blocking

None.

Optional

1. The comment on LOCK_RECORD_MAX_BYTES says the constant bounds every read of lock.json. Two drop-time reads do not use it. A local guard reads the file with std::fs::read_to_string. release_remote_lock_if_owned calls read_text_if_exists, which has no byte limit. Both run only when release_on_drop is set. Acquisition sets that flag only when the storage kind is not S3, so these reads belong to unleased local and Azure locks. They apply no smaller limit. They do not stop a live server at its deadline. Narrow the comment to the reads that decide the lock, or pass this constant into those two reads.

Inline, crates/omnigraph-cluster/src/state_lock.rs:24: The comment says this constant bounds every read of lock.json. Two drop-time reads do not use it. A local guard reads the file with std::fs::read_to_string (the local arm of Drop). release_remote_lock_if_owned calls read_text_if_exists, which has no byte limit. Both run only when release_on_drop is set. Acquisition sets that flag only when the storage kind is not S3. These reads belong to unleased local and Azure locks. They apply no smaller limit. They do not stop a live server at its deadline. Narrow the comment to the reads that decide the lock. Or pass LOCK_RECORD_MAX_BYTES into those two reads.

Tradeoffs and liability

This commit removes a registration that described a delete the lease fix already removed. It also removes a second copy of the 64 KiB limit. A smaller limit on the owner read makes holds return a limit error. The owner then sends no beat and fences when its deadline fires. The record's module, state_lock, is the right owner of that number. lease and store import it.

A later read that can fence the owner has to pass the same constant. A helper that reads lock.json would make a missed limit a compile error. This commit already has five call sites on the constant. A helper would be a second surface for the same reads. Add that helper when more reads appear. Until then the constant is the one source of the limit.

The if-match on lock.json stays the one election. The read before each beat stays on the lease client, outside the storage scope. The stopped-clock window stays the window the RFC states. The owner can admit work until its next beat. That wait is at most one third of a lease. At the default of 30 seconds the wait is 10 seconds. At the maximum of 600 seconds the wait is 200 seconds.

Five more fixes of this kind should extend holds and watch_and_claim, pass this constant, and keep one object as the election.

Open PR 1221 stacks on this head and moves the deadline into the storage scope. Open PR 1176 re-issues a lost conditional write and reads the object back. Success there is equal bytes, so another successor's record stays a conflict. Open PR 1214 would send maintenance through the serving owner. That work stays safe only while one process serves. This commit does not change those contracts.

The lease stays on S3 only. Lance's object-store guide says a safe concurrent s3:// write needs the store to enforce conditional puts. This commit adds no second path for another cloud. Local locks and Azure locks stay unleased, as the RFC requires.

Validation

Merge base with current origin/main (f728f01e1543c442eff77f8b0d10c223e38da36d): 7b3a1c9190730d31391c225a63c7b6171146bc74.

Reviewed head: a7533b6a4881e7532b1553828d0a7a47a6a93594.

Parent: af82fd4e3ccb89c08d31821caebca1439fa0b85b.

GQT cannot express an inventory count or a lock-record byte limit. No .gqt case is required. tests/forbidden_apis.rs owns the inventory.

The inventory defect is absent on the merge base. There the delete exists and the registration matches it. The defect starts at af82fd4e, which removes the call and leaves the registration. This review runs the test on that parent and on the head.

Local, isolated worktree, CARGO_TARGET_DIR=/workspace/target.

Parent af82fd4e:

cargo test -p omnigraph-engine --test forbidden_apis --locked -- mutable_key_requests_are_registered_issue_1067 -- --exact

Result: failed in 1.70s. The panic text is omnigraph-cluster/state_lock.rs .delete_prefix(: registered 1, observed 0.

Head a7533b6a, same command:

Result: passed. 1 passed, 0 failed, in 1.61s.

Head, cluster lease tests:

cargo test -p omnigraph-cluster --lib --locked -- issue_1067

Result: 16 passed, 0 failed, in 6.01s. The run includes a_late_listing_cannot_unseat_a_beating_owner_issue_1067, a_claim_or_a_lost_record_fences_the_owner_issue_1067, an_owner_fences_itself_when_its_beats_lapse_issue_1067, one_failed_heartbeat_does_not_fence_a_live_owner_issue_1067, a_superseded_owner_whose_clock_stopped_fences_at_its_next_beat_issue_1067, and a_restarted_server_takes_over_a_silent_owners_lock_issue_1067.

This commit does not change those lease tests. The earlier review showed the new cases fail on the merge base. This review does not run them again there.

CI on this exact head. Every completed check passed. The skipped checks are the tag-publish jobs.

• CI run 41262959283. Test Workspace passed. Its log shows mutable_key_requests_are_registered_issue_1067 PASS in 2.175s, and the cluster issue_1067 lease tests PASS. Storage Upgrade Compatibility passed. Test omnigraph-server --features aws passed. CI Gate passed. Clippy and rustfmt passed.
• GQ Logic Tests run 41262959278. Ordinary, served, dst, and dst-clippy passed.
• DST run 41262959361. The DST pinned suite passed.

Limitations:

• No live S3. The cluster tests use the in-memory store. holds reads with get_range of the limit plus one byte. In object_store 0.14.2, GetRange::as_range returns the rest of the object when the requested range ends past it, so a normal lock record comes back whole. The conditional reads use a full GET and refuse when meta.size is above the same limit.
• The DST lease scenario was not re-run in this worktree. The DST pinned suite on this head passed in CI.
• This commit does not change the 64 KiB value. The extra GET before each beat was already in af82fd4e. This review did not time it.

[ragnorc]

Addressed in the new commit on this branch:

• Blocking: removed the omnigraph-cluster/state_lock.rs .delete_prefix( registration from forbidden_apis.rs. mutable_key_requests_are_registered_issue_1067 passes locally.
• Optional 1: docs/dev/control-plane.md and docs/user/operations/storage-failures.md now say the owner stops on a claim or a lost record at its next beat, at most a third of a lease later. Only the lapse is a timer of its own.
• Optional 2: state_lock::LOCK_RECORD_MAX_BYTES is the one bound on every lock.json read: the lease's ownership check, acquisition, release, the bootstrap lock, and ClusterStore::read_versioned_opt.

The remaining limit, where another task could send before the lease task fenced the scope after a process pause, is closed separately in #1221, which is stacked on this PR.