Repository navigation
Conversation
A claim sends its INSERT only once its head read found no live holder, so whoever refused the service before had left by then. ensure_publisher_lease reset the refusal clock for a refusal by the service's own late claim row, but not for a claim whose INSERT timed out: a rival that left, such a claim, and a second rival refusing the claims after the quarantine added up to 2 x TTL and latched the service over a catalog nobody held in between. The unknown-outcome branch now restarts the clock whenever the writer quarantined, which in that branch is exactly when the INSERT may have been sent.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Quarantine expiry can bypass the reset, and the regression test does not reliably establish the initial rival refusal.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Updates the publisher lease latch so a timed-out claim INSERT ends the preceding run of rival refusals.
Changes:
- Resets the refusal clock when a failed claim quarantines the writer.
- Adds a live regression test covering two rivals separated by a free lease.
| File | Description |
|---|---|
| tests/test_native_capture_storage_live.py | Adds the timed-out claim handover regression test. |
| native/csrc/catalog/storage_service.cpp | Resets refusal tracking after a quarantined claim failure. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| exc.what()); | ||
| note_lease_failure(exc); | ||
| if (!writer_.quarantined()) { | ||
| if (writer_.quarantined()) { |
Comment on lines
+3443
to
+3444
| _wait_for(lambda: first.snapshot()["lease_state"] == "reacquiring", | ||
| timeout_s=10.0) |
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.

A small fix to the publisher lease's
2 x TTLlatch inCaptureStorageService: a claim whose INSERT was sent now ends a run of refusals, so the service no longer latches over a catalog that nobody held for part of the run.The defect
ensure_publisher_leaselatches the service ("held by another publisher for over 2 x TTL") once lease refusals have lasted two TTLs. #159 made a refusal by the service's own late claim row restart that clock. The comment there gives the reason: a claim sends its INSERT only once its head read found no live holder, so any rival's earlier refusals ended there.The same reasoning applies to a claim whose INSERT was sent and then timed out, but that path (the
std::exceptionbranch) never reset the clock. So refusals by two different rivals, separated by a stretch where nobody held the lease, added up:The fix
In that branch,
writer_.quarantined()is exactly the case where the INSERT may have been sent:CatalogWriter::acquire_leasequarantines a non-renewing claim only whenclaim_insert_sent(). The branch already used it to tell a claim that wrote nothing from one that may have. When it is set, the service now resetsheld_elsewhere_since_ns_, as it does for a refusal by its own row. A real takeover still latches: the clock restarts at the second publisher's first refusal, and a holder that keeps refusing for 2 x TTL from there is latched as before.native/csrc/catalog/storage_service.cpponly; 10 lines.Tests
tests/test_native_capture_storage_live.py::test_a_claim_sent_while_nobody_held_the_lease_restarts_the_refusal_clockreproduces steps 1–5 against a real ClickHouse. A_Switchholds the claim INSERT open and never forwards it, so no late row of the service's own is involved. It fails on main withindexing stopped: publisher lease held by another publisher for over 2 x TTL ... held by 'second-publisher', and passes with the fix (3 of 3 runs).test_native_capture_storage_live.py -k 'lease or latch or rival or claim or start': 30 passed. This includes the existing latch tests: a rival that takes over still latches, a rival that stops within two TTLs does not, and our own late claim does not.test_native_lease_request_bound.py,test_native_catalog_lease_live.pyandtest_native_capture_storage_wiring.py: all passed.How it was found
The
LeaseLifecycleTLA+ model on thespecs/formal-models-archivebranch, re-checked against main @ 5b3b632. ConfigLeaseLifecycle_O3_unkrivalviolatesNoFalsePositiveLatchwith this trace. No spec files are part of this PR.