Rank a capture's packs by their first publish so a replayed pack cannot flip a pinned read by zaoxing · Pull Request #161 · ProjectDMX/DMI

zaoxing · 2026-09-28T22:42:53Z

Replaces #156, which the Claude GitHub app opened. Its two commits are re-landed on current main (8b7991d) and authored by Alan Liu. An earlier merge commit on #156's branch was dropped, and its conflict resolution redone and verified. Follow-up commits from an independent review are added on top.

Original description (#156)

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:

Pass A publishes P1 at v1, then dies before commit_packs.
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.
Pass C does not find P1 in the inventory. It re-indexes P1 and writes P1's rows at v3 before publishing anything.
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:

Pass A publishes P1 at v1, then crashes before commit_packs.
Pass B publishes P2 at v2, superseding P1 for X.
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.

Re-landed on main, plus review fixes

How it composes with main:

With Resolve a search page's argMax for its own keys, not every row past the cursor #139's two-phase page (reader.cpp search()): both the inner key query and the outer argMax query read FROM snapshot(), and both carry the caller's filters. The inner WHERE is omitted when there are no filters. With the inner query switched back to FROM capture_raw, Resolve a search page's argMax for its own keys, not every row past the cursor #139's test_a_page_walk_skips_unpublished_keys_without_ending_early fails, so the silent snapshot-drop trap is covered.
With Bound how long the publisher lease can go unrenewed: a deadline for every request under it #159's index() split: the conflict-path guard is in NativeIndexer::commit()'s publish-retry loop, the only place where commit_packs runs on the kPublishConflict path.

Added commits:

463a23a: a lease refusal (kHeld/kLease) raised by commit_packs on the conflict path keeps its kind; it is no longer relabelled as a publish conflict. This matters after Bound how long the publisher lease can go unrenewed: a deadline for every request under it #159, whose renewal hook can refuse there.
199c6c8: a C++ test for the conflict-path guard. The conformance driver can inject a foreign watermark row at publish time. Before, removing the guard left every native test passing.
9d6298b: a parity test for the trailing index_version tiebreak within one pack.
7c4b14b: measured performance in docs/benchmarks.md.
- At 20k packs, pages are 3–15% faster than main.
- At about 100k packs in the manifest, pages are 6–34% slower, with identical rows read. The join's hash table costs more than the old IN set, which is the price of pinned-read stability.

Evidence:

CPU: 2562 passed.
Live suites, all passing:
- reader parity (50, including Resolve a search page's argMax for its own keys, not every row past the cursor #139's walk test and the new tests);
- snapshot;
- capture storage;
- chain;
- catalog lease (unmodified).
Repeats: the replay tests and Resolve a search page's argMax for its own keys, not every row past the cursor #139's walk test passed 3 runs out of 3.
Mutations: max instead of min, or the inner query without the snapshot, each turns a test red.

A pass that re-indexes an already-published pack (crash before commit_packs, an outcome-unknown publish that landed, a conflict whose commit_packs failed, a rebuild beside the live indexer) writes that pack's descriptor rows at a fresh, higher version before publishing anything. The reader ranked a capture's candidate rows by the row's own index_version, and membership bounds packs rather than rows, so those rows outranked a newer pack describing the same capture inside snapshots that were already pinned. If the replay never published, that stayed true. Found by TLA+ model checking (VersionPublish Replay/ReplayCrash/ ReplayCrashPinned). The Python and C++ readers now join the snapshot's packs with the newest version their publish reached the watermark at (clickhouse_sql. member_versions, the same paired-manifest rows as membership_predicate) and resolve on (member_version, store_id, pack_id, index_version). The public view's DDL is unchanged. The C++ indexer also gets Python's guard on the conflict path: a commit_packs failure no longer replaces kPublishConflict. On top of the two-phase search page, both of its queries now read the snapshot join: the inner key query and the outer argMax query take the same FROM and the same caller filters, and the inner one adds WHERE only when there are filters. Membership left `clauses`, so an inner query kept on the raw table would either render "WHERE GROUP BY" when unfiltered or pick page keys from unpublished and replayed rows and end a walk early.

The reader ranked a capture's packs on the newest paired publish at or below the pin, max(index_version) over the manifest. A replay that does publish moves that rank: pass A publishes P1 at v1 and dies before commit_packs, pass B publishes P2 at v2 describing the same capture, and a later pass that re-indexes P1 publishes it again at v3. From v3 on every head resolved the capture back to P1, the older pack, on the normal crash-recovery path. Pins stayed correct; heads did not. Both readers now take min(index_version): a pack's rank is fixed once it is first published, so a replay, published or not, cannot move it. Pins are stable either way, since a later publish lands above the pin. A genuine re-capture is a new pack and a mirror is another store, so both still get a fresh first publish. The Python and C++ member_versions subqueries change together and stay identical. The live replay test and the native/Python parity test now publish the replay and require P2 at the new head, before and after a merge; both failed with max. EXPLAIN indexes=1 plans for the probe corpus (2M rows, 20k packs) are identical under min and max, as are rows read, and timings are within run-to-run noise.

NativeIndexer::commit() records a conflicted publish's packs in the inventory and rethrows kPublishConflict, and when that commit_packs fails the conflict still wins (catalog.py's `raise conflict from commit_failure`). Only the Python oracle's test covered it: the conformance driver had no way to produce a same-version conflict, so removing the guard passed every native suite. The `index` op now takes conflict_at_publish: a foreign watermark row lands at the pass's version after the pass's own and before its owners read-back, from the request hook RequestDeadline runs before each request, which is the hook the storage service's lease scope uses. after_conflict="transport" then fails the inventory INSERT that follows. The live test checks both: a plain conflict records the packs, and a failed record still reports SnapshotPublishConflictError with the failure in its message. With the guard reverted to main's shape the transport case reports ClickHouseError and fails.

…ts packs The conflict-path guard in NativeIndexer::commit() rethrew any commit_packs failure as kPublishConflict. Since #159 that INSERT runs under index_bounded's second LeaseScope, whose before-request hook renews the lease once a renewal is due. A rival holding the lease, which is the situation a second writer creates, refuses that renewal with kHeld, and the guard relabelled the refusal as a conflict. index_bounded rethrows only a lease refusal (is_lease_refusal), so it treated the lost lease as an ordinary failed batch. When a reconcile was due, run_cycle went on to it under no lease: it read a page's missing packs from the object store before commit() refused with kLease, and those packs inflated index_failures. If this happened on the last page of the start-time reconcile that had missing packs, reconcile_owed_ stayed false. The conflicted batch then stayed out of the inventory until something reconciled again, which with the default interval of 0 is the next start. Readers were unaffected, because the packs are members. A kHeld or kLease from commit_packs now keeps its kind and carries the conflict in its message. Any other failure still becomes kPublishConflict. is_lease_refusal moves from storage_service.cpp's anonymous namespace to lease_coordinator.h, so the service and the indexer share one definition. The Python oracle's commit_packs never renews, so it has no such case and does not change. The conformance `index` op's after_conflict="lease_refused" plants a rival claim at the lease's term and renews before the inventory INSERT, as keep_lease_in_pass does. Before this change the new live case got SnapshotPublishConflictError; it now gets PublisherLeaseHeldError.

The resolution order ends in index_version so that when one pack's rows exist at two versions, a read resolves to the row a merge will keep (ReplacingMergeTree(index_version)). No native test covered this. Replays write byte-identical rows, so the pick never showed, and dropping index_version from kResolutionOrder passed every live suite. The parity suite now publishes a pack at 7 and writes it again at 9 with payload_offset moved, then requires the v9 rows from native search and get_by_ids, and agreement with the Python reader, before and after OPTIMIZE FINAL. An undefined tie resolves by the order parts are read in, so the v9 rows go once into a part written after the v7 part and once into one written before it. With the component dropped, the second case returns the v7 rows (3 of 3 runs).

A pinned read now joins capture_raw to a members subquery that supplies each pack's member_version, in place of the old (store_id, pack_id) IN membership. Both queries of the two-phase page read the join, so a page builds the members set twice. docs/benchmarks.md now records the cost against main (8b7991d), measured on a 2.1M-row synthetic corpus with superseding packs and unpublished and republished replays. Each build's captured statement was re-run 9 times, interleaved, with use_query_condition_cache=0. - 20k packs: selective pages 19-28% slower (+11 to +14 ms), about two members builds (11 ms each, against 2 ms for main's IN set). Unfiltered pages unchanged. - 100k packs: 0.88x to 1.08x. An independent review on another corpus shape measured 1.06x to 1.34x here. - 1M packs: 2.7x to 3.7x faster, because main's primary-key condition carries a 2M-element pack set. Rows read and capture_raw granules are identical in every case. Page contents differ from main only where main mis-ranks a replayed pack. The design doc's two-phase section points to the numbers. Sharing one members build between the two queries has not been measured.

Copilot

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

zaoxing added 6 commits September 28, 2026 16:39

Copilot AI balanced review requested due to automatic review settings September 28, 2026 22:42

Copilot AI reviewed Sep 28, 2026

zaoxing merged commit d3fe7c2 into main Sep 28, 2026
3 checks passed

zaoxing mentioned this pull request Sep 28, 2026

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

Closed

zaoxing deleted the fix/pinned-read-first-publish branch September 28, 2026 22:54

Navigation Menu

Rank a capture's packs by their first publish so a replayed pack cannot flip a pinned read - #161

Rank a capture's packs by their first publish so a replayed pack cannot flip a pinned read#161
zaoxing merged 6 commits into
mainfrom
fix/pinned-read-first-publish

zaoxing commented Sep 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

zaoxing commented Sep 28, 2026

Original description (#156)

How

Test evidence

Re-landed on main, plus review fixes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants