Rank a capture's packs by publish version so a replayed pack cannot flip a pinned read

[claude]

Requested by Alan Liu · Slack thread

Before: Sometimes an indexing pass re-indexes a pack that is already published. That happens after a crash between publishing and commit_packs, or after an outcome-unknown publish that actually landed, or on the C++ conflict path when commit_packs fails. A replay like that could silently change which pack a pinned reader resolves a capture to. The pinned read went back to an older pack that a newer one had superseded. If the replaying pass never finished, the flip was permanent, and a merge made sure of it. This is the opposite of what the reader promises: "a pin still resolves to the pack it was taken over".

After: A capture's packs are ranked by when each pack's publish reached the watermark, at or below the pin. That comes from the manifest paired with the watermark log. The version a descriptor row happened to be written at no longer counts. A replay now changes nothing a pinned reader sees, before or after a merge. A pack's rank is fixed by its first publish at or below the pin. A replay that goes on to publish the same pack again does not re-promote it over a pack that superseded it. The C++ indexer also no longer buries kPublishConflict when commit_packs fails after a conflict.

How

Found by TLA+ model checking of the publish protocol (formal/tla/VersionPublish; configs Replay, ReplayCrash, ReplayCrashPinned). The scenario:

1. Pass A publishes P1 at v1, then dies before commit_packs.
2. Pass B publishes P2 at v2. P2 is a second pack describing the same capture. A reader pins W=2 and resolves the capture to P2.
3. Pass C does not find P1 in the inventory. It re-indexes P1 and writes P1's rows at v3 before publishing anything.
4. The W=2 reader now resolves to P1. P1 is still a member at ≤2, and the old argMax(..., (index_version, store_id, pack_id)) ranks (3,P1) above (2,P2).

Fix chosen: reader-side ranking (candidate (a)).

• clickhouse_sql.member_versions returns each member pack with min(index_version) over its paired manifest rows ≤ W, i.e. its first paired publish. It is built from the same manifest-rows fragment as membership_predicate, so the two cannot drift. The public view's DDL is byte-identical to before.
• The Python and C++ readers replace the membership IN with an INNER JOIN ... USING (store_id, pack_id) against it.
• Both readers now resolve on (member_version, store_id, pack_id, index_version).
• The trailing index_version only orders one pack's own rows. It picks the row a ReplacingMergeTree(index_version) merge keeps, so pre-merge and post-merge reads agree. An existing live test (test_hydration_rejects_a_re_described_catalog_row) depends on this.

Why not the writer-side skip (b):

• I modelled it too (SKIP_MEMBER_PACKS). It fixes ReplayCrash/ReplayCrashPinned but still fails Replay (ResolvesNewest, 31-state trace). That is the rebuild-beside-the-live-indexer route: the second pass checks membership before the first pass's publish lands. No check before re-indexing can close that window.
• It also would not repair rows that past replays already wrote.
• So (a) is the only candidate correct for every route. Its diff is also small: one SQL fragment plus a changed FROM clause in each reader.

Design note (changed after review, 59c28ce): member_version is the pack's first paired publish, not its newest. With the newest publish, this sequence resolves X back to the older pack at every head ≥ v3, on the normal crash-recovery path:

1. Pass A publishes P1 at v1, then crashes before commit_packs.
2. Pass B publishes P2 at v2, superseding P1 for X.
3. A later pass or reconcile re-indexes P1 at v3.

With the first publish, a pack's rank is fixed once it is first published. A real re-capture is a new pack, and a mirror is a different store, so both still get a fresh version. Pinned reads are stable either way.

C++ conflict guard: indexer.cpp now catches a commit_packs failure inside the kPublishConflict handler and rethrows it as kPublishConflict, with the commit error appended. This matches catalog.py's raise conflict from commit_failure.

Docs updated:

• clickhouse_reader.py module docstring (the pinned-resolution paragraph), _snapshot, _projection.
• The _publish docstring in catalog.py. The retry's descriptor rewrite is no longer what supersession relies on.
• capture-storage-design.md (Phase 5 ordering).
• A dated correction under M3 in catalog-differential-review-2026-09-01.md, next to "Reader-visible corruption: none".

Model re-check (measured before the switch to first-publish ranking; the scratch model was not committed and has not been re-run under min): a scratch copy of VersionPublish.tla with a RANK_BY_MEMBERSHIP flag, plus an optional MERGES action that collapses a pack's rows to the highest version. The spec was not committed. TLC results:

Config	Without the fix	With RANK_BY_MEMBERSHIP	With the fix and MERGES
Replay	ResolvesNewest violated	pass, 23,767 / 16,724 states, depth 60	pass, 78,183 / 38,746
ReplayCrash	ResolvesNewest violated	pass, 5,821 / 4,119	pass, 9,093 / 5,377
ReplayCrashPinned	PinnedStable violated	pass, 5,821 / 4,119	pass, 9,093 / 5,377
Faithful (all safety, NeverConflict, MonotonicLanding, Termination)	pass	pass, 5,239,843 / 2,110,944, depth 76 (same counts as unfixed)	pass, 8,523,437 / 3,057,864
ReplayRest	pass	pass	pass

SkipRewrite also passes under the fix, which confirms the retry rewrite is no longer load-bearing. The candidate (b) results are in the previous section.

Test evidence

New tests:

• CPU: tests/test_clickhouse_capture_reader.py::test_a_replayed_pack_ranks_by_its_publish_not_by_its_rewritten_rows. It pins the join and the ordering at both query sites. It fails on a987dfe and passes with the fix.
• Live: tests/test_clickhouse_snapshot_live.py::test_a_replayed_pack_does_not_flip_a_pinned_read[×2]. It runs the full scenario: pinned get_by_ids and search, a cursor walk, a forced merge, then publishing the replay.
• Live: tests/test_native_reader_parity_live.py::test_a_replayed_pack_does_not_flip_a_pinned_read_on_either_side. The same scenario against the native reader and the Python reader.

Existing CPU tests were adjusted for the new SQL shape. The fake client now identifies the head read by SELECT max(index_version) FROM.

Commands and results:

• python -m pytest -m cpu -q → 1944 passed, 324 skipped. The skips are native conformance drivers that are not built here.
• Since verified on real ClickHouse 25.12 (independent review, then again after the min change): the four live suites (snapshot, native reader parity, native capture storage, native catalog lease) gave 143 passed, and pytest -m cpu gave 2301 passed. Before the fix, the replay tests fail on both the native and the Python reader. EXPLAIN indexes=1 shows the same pruning for min and max, with timings within noise on a 2M-row corpus.
• Original run, before that: no real ClickHouse server was available. I ran the Python live suites against embedded ClickHouse 26.7 (chdb) through a scratch clickhouse_driver.Client shim that is not committed: pytest tests/*_live.py -m "clickhouse and manual and not garage" → 64 passed, 1 failed. The failure is test_a_role_that_cannot_see_one_object_is_told_to_grant_it_not_to_rebuild, which also fails on main under the shim because chdb has no roles. On a987dfe the new live test fails with "the replay flipped the pinned read"; with the fix it passes.
• The native reader was compiled from source (curl headers from the curl repo). A scratch harness drove it against the same embedded engine through a minimal HTTP emulator. Reading the replay scenario at W=2 before the replay, after it, and after OPTIMIZE FINAL: a987dfe resolves P2 → P1 → P1, and this branch resolves P2 → P2 → P2, for both get_by_ids and search.
• g++ -std=c++20 -fsyntax-only -Wall -Wextra is clean on reader.cpp and indexer.cpp. The full native build and the conformance driver could not be built here; this relies on CI's native-backend-compile and clickhouse-live jobs. test_native_reader_parity_live.py could not run here.
• The C++ conflict-path guard has no automated test. The conformance driver has no hook to inject a conflict together with a commit_packs failure. This is a follow-up.

Cost of the hot query: I measured an A/B of the IN form against the join form on the same data, interleaved, 9 trials, median, on chdb 26.7. The machine was shared, so timings are noisy.

Corpus	Change with the join
100k rows / 10 packs	−4% to −11%
1M rows / 10k packs	−11% to −35%
1M rows / 100k packs	−39% to +9% (the +9% is a 100-id lookup)

No regression stood out above the noise. benchmarks/bench_capture_search.py also ran on both trees through the shim, three runs each: medians overlap and the noise was too large to resolve a difference. Primary-key pruning survives the join: test_selection_resolve_prunes_to_the_tenant_range passes under the shim.

---

[Copilot]

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

[claude]

Re-checked the VersionPublish TLA+ model with 59c28ce's first-publish ranking (min over paired publishes <= W). All pass, with and without background merges:

Config	States (distinct)	Depth
Faithful	2,110,944	76
Faithful + merges	3,057,864	78
Replay / ReplayRest	16,724	60
Replay / ReplayRest + merges	38,746	62
ReplayCrash / ReplayCrashPinned	4,119	60
ReplayCrash / ReplayCrashPinned + merges	5,377	62

The model's old ResolvesNewest assumed newest-publish (max) ranking, so I replaced it with two new invariants. ResolvesFirstPublished: every head resolves to the member published first most recently. NoSupersededComeback: a pack that lost at one head never wins a later head. Both pass under min. Both fail under max in 37 steps, in exactly the case your commit describes: P1 published at v1, the pass crashes before commit_packs, P2 published at v2, a replay publishes P1 at v3, and head 3 resolves to P1.

---

Generated by Claude Code

[zaoxing]

Superseded by #161: the same change, re-landed on current main (conflict with #139 re-resolved and verified; guard placed in #159's commit()) and authored by Alan Liu, plus review fixes. Merged as d3fe7c2.