Skip to content

fix: keep one schema lease holder and answer a batch push race with 409 - #930

Merged
catinspace-au merged 4 commits into
mainfrom
fix/lease-reread-batch-409
Oct 20, 2026
Merged

catinspace-au merged 4 commits into
mainfrom
fix/lease-reread-batch-409

Conversation

@catinspace-au

@catinspace-au catinspace-au commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Two fixes on the schema lease and the gitops batch path.

The schema lease let two appliers both take it. The lease table keeps the latest write per lock name, and a claimer never checked whether anyone else had written after it. Four appliers started on a barrier against a real ClickHouse all got the lease on main.

  • A claimer now writes its claim, waits 0.5s, re-reads, and keeps the lease only if the latest claim is still its own. The later claim wins and the rest go back to waiting.
  • Reads use FINAL. Two claims in the same millisecond resolve to the row ClickHouse keeps when it merges them (the last insert), and a FINAL read sees that same row before the merge too, so every reader names one winner either side of it.
  • Ordering a tie any other way lets a merge change the answer after the claimers have read it. We forced that with OPTIMIZE FINAL against the holder-ordered read and both claims stood.
  • Every lock instance writes its own holder, the hostname plus a random suffix (pod-7/1a2b3c4d). Concurrent source deploys on one pod each build a lock, and with the bare hostname they had nothing to break a tie on. The status route reports the same name the lease row carries.
  • A release now replaces only the releaser's own claim. It used to write the release time as the row's version, so a holder that ran past its expiry and released after a takeover outranked the new holder, and a third applier then ran alongside it. That was live on main too.
  • With that, exactly one claimer holds the lease when every claim lands within the 0.5s settle of its peers AND each claimer reads its own insert back. The chart covers the second with wait_for_async_insert=1.
  • A read that doesn't show the claim yet is no sign of a later one, so the claim stands. The lost-claim path checks the deadline, so it can't spin.
  • FINAL costs nothing that matters here. The read uses the lock_name key and folds the parts in one step, and 50 reads each on 7 parts averaged 10.9ms with FINAL against 15.6ms for the old argMax read.
  • test_appliers_that_find_the_lease_free_together_leave_one_holder runs the four-applier race against ClickHouse. It fails on main (all four hold) and passes here (one holds). A racer that raises now fails the test instead of dropping out of the count.
  • test_claims_in_the_same_millisecond_keep_one_holder_through_a_merge writes two claims with one timestamp from two locks on one host, larger holder first. It checks one stands and both readers name it, then merges with OPTIMIZE FINAL and checks the same holder still stands alone.
  • test_a_release_after_the_lease_was_taken_over_leaves_the_takeover_held lets a one-second lease lapse, takes it over, then releases the stale holder. The takeover still holds and a third applier is refused. It fails on the old release (the stale holder's release row wins) and passes here.

What this does NOT cover:

  • Replicas whose reads lag inserts by more than the 0.5s settle can still both hold.
  • A ClickHouse whose profile sets wait_for_async_insert=0 loses read-your-own-insert, and with it the guarantee.
  • Both need a consistent read or a compare-and-swap lease.

A conditional write inside GitCrud.batch() whose push lost its race raised GitopsHeadMovedError from the batch exit, so it skipped the 409 mapping. The batch now records each conditional write and answers with ConcurrencyConflictError naming the first. Only the e2e seeder batches today, so no route hit it yet.

Checks: hyperi-ci check green (13684 passed). All 11 lease integration tests pass against ClickHouse. The four-applier race passed 5 runs out of 5 and the tie test 3 out of 3. The new batch test fails on main and passes here.

Two appliers that both found the schema lease free both wrote a claim and both applied DDL, because the lease table keeps the latest write and nothing re-read it. Four appliers started together all took the lease against a real ClickHouse. Each claimer now settles, re-reads, and keeps the lease only if no other claim landed at or after its own, so exactly one holds it.

A conditional write inside GitCrud.batch() whose push lost its race raised the deploy repo's GitopsHeadMovedError instead of ConcurrencyConflictError, so it escaped the 409 mapping. The batch now names the first conditional write in the conflict.
Lease claims now sort by acquired time, then holder, so every reader breaks a same-millisecond tie the same way instead of trusting argMax over equal times. A SchemaLock built without a holder writes the hostname plus a random suffix, so concurrent source deploys on one pod no longer share a name and still sort apart, and the bootstrap status reports that same name. The four-applier race test now fails on a racer that raises rather than dropping it from the count, and a new test writes two claims with one timestamp and checks that one stands and both readers name it.
The lease table keeps one row per lock name on merge, and for two claims in the same millisecond it keeps the last insert. Ordering the tie by holder could pick the other claim before the merge and leave two holders after it. Reads now use FINAL, which resolves a tie to the row a merge keeps whether or not the merge has run, and a claim stands only when that row is its own or an earlier claim. The tie test now merges with OPTIMIZE FINAL and checks the same single holder before and after.
A release wrote acquired_at as the time of release, so a holder that ran past its expiry and released after another applier took over outranked the takeover, and the lease read as free while the new holder was still applying. The release row now carries the holder's own claim time, so it replaces only that claim and a later takeover still outranks it. A new test lets a one-second lease lapse, takes it over, releases the stale holder and checks the takeover still holds and a third applier is refused.
@catinspace-au
catinspace-au merged commit 5954f0d into main Oct 20, 2026
22 checks passed
@catinspace-au
catinspace-au deleted the fix/lease-reread-batch-409 branch October 20, 2026 11:38
@github-actions

Copy link
Copy Markdown

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.

1 participant