Say what the lease comments actually guarantee

[claude]

Requested by Alan Liu · Slack thread

Before: Four comments on the publisher lease said more than the code does, or said it loosely. One implied that renewal failures get retries. One still read as the strong spool-sweep guarantee that 2b74d14 had already weakened. One gave the start-wait caveat only roughly. One left the reader to work out why the claim read-back can take rows[0].

After: Each of the four comments now says what the code does and matches what the formal models checked. No code changes.

How.

• storage_service.cpp, renew_lease_if_due(): four lease-thread wakes do fit between the renewal falling due and the row expiring. They cover a renewal that runs late, though, not one that fails. Every failed renewal costs the lease at once. A refusal drops it in the coordinator (reject_live, or the failed read-back). A transport error or timeout goes through renew_for_publish()'s catch, which quarantines the writer on the first failure.
• storage_service.h, sweep_spool_on_start: now matches the .cpp. A start refused the lease never touches the spool. A quarantined holder, though, lets its row lapse, so a second process can take the lease and sweep while the first is still writing. The spool is not locked.
• native_capture.py, start_lease_wait_s: the default wait outlasts a crashed predecessor exactly when that predecessor's TTL is at most lease_ttl_s + publish_timeout_s, and clock_skew_s cancels out. On the shipped defaults that limit is 20 s, so a predecessor on the native 30 s default outlasts the default wait by 10 s.
• lease_coordinator.cpp, the claim read-back: a comment explaining why rows[0][2] is safe even though the rule is the minimum expires_at_ns. The only other row that could share (term, lease_id) is a release's tombstone. Releases write at the holder's own term and claims always go to head + 1, so a claim's term never already holds its own tombstone.

To avoid conflicting with the fix PRs, this leaves docs/catalog-descriptor-key.md and the lines at storage_service.cpp:435-437 and :723-739, storage_service.h:216-218 and catalog_writer.cpp:273 alone. python -m py_compile passes on the Python file. The C++ was not built (no nvcc in this environment), but every changed line in the diff is a comment line.

---

After independent review (592a90e)

The reviewer checked every original change against the code. The new comments were more accurate than the old ones, and six were tightened further. It is still comments only.

• Renewal slack: the renewal fires about a tick after it falls due, or later if an index pass holds the lease mutex. That leaves roughly half the TTL, which covers a late renewal, not a failed one.
• Spool sweep caveat: now covers any holder that stops renewing for a TTL, a process on another (database, table_prefix), and two processes then uploading one spool.
• What quarantines: any ClickHouse error, not only transport errors and timeouts.
• Start-wait wording: "only when" instead of "exactly when", with the replica-offset assumption stated.
• Where the quarantine happens: it is in CatalogWriter, and the client never re-sends an INSERT.
• "The first error" now means the first one that survives the client's read retries.

Rebased onto main after #150 merged (ad129a1, 68e6aee)

#150 was squash-merged, so this PR's two original commits were replayed onto main (71be2af) without conflicts, and the PR now targets main. It is still comments only: a script checks that every added or removed line is a comment.

Each comment was re-checked against main's current code, and these were corrected:

• The client does re-send an INSERT in one case. Since Connect to a secured ClickHouse catalog: header credentials, verified TLS, bounded retries #151, execute() repeats any statement whose connection was never made. The coordinator comment and the "first error that survives the client's retries" wording now say that a write is repeated only when it never connected.
• A holder that has lost its lease can still upload. run_cycle() checks the lease once, at the start of the cycle. An upload batch already running continues after the lease is lost, so the sweep caveat now describes that window instead of claiming a quarantined holder uploads nothing.
• The renewal slack can be smaller. "Roughly half the TTL" holds only when no index() is in the way. index() holds lease_mutex_, and index_bounded() restamps last_renew_ns_ when it returns.

Found, not fixed here (this PR is comments only): index_bounded() sets last_renew_ns_ = steady_ns() whenever index() returns with skipped packs, even when every pack was already committed and nothing was published or renewed. That restarts the renewal clock without renewing the lease row. It needs a separate code fix: stamp only when the index published, or take the writer's last successful renewal time.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.