Skip to content

fix(master): publish merged watermarks after release, one shard per put - #357

Merged
beinan merged 1 commit into
lance-format:mainfrom
beinan:fix/release-demand-detached
Oct 8, 2026
Merged

fix(master): publish merged watermarks after release, one shard per put#357
beinan merged 1 commit into
lance-format:mainfrom
beinan:fix/release-demand-detached

Conversation

@beinan

@beinan beinan commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator

Part of #333. Fixes review finding 2 [P1] (round 4) on #350 (credit: production review). Independent of #352.

Problems in #350

  1. Watermark read after the watchdog exited and before release, with no deadline → a stalled storage read holds a completed merge's ownership.
  2. Release txn compared every shard's demand key by mod_revision → a normal writer flush on any shard fails the whole release. Reproduced.
  3. 129+ shards → etcd default --max-txn-ops=128 exceeded.

Fix

  • release is the plain ownership txn again; release_with_demand / demand_release_changes removed.
  • After a successful Catchup release: demand_publish::spawn_merged_demand — detached, 15 s timeout, 4 slots, drop-and-count, one monotonic put per shard, bounded CAS retry on a lost race. Loss/lateness cannot misstate demand (per-dimension join; scan reconciles).
  • Both publish paths share join_events + retry, so the writer path no longer silently returns Ok(false) after losing a race either.
  • master_demand_merged_watermarks_total{result}.

Validation

  • New etcd test: writer sealed=25 and executor merged=18 published concurrently on one shard → both succeed, stored (25, Some(18)); lower merged no-op; unknown shard → fresh epoch-0 record.
  • Resident two-master merge test polls for the detached publish; both shards fold to zero pending.
  • 7 demand, 9 maintenance/resident, 9 sweeper tests green against etcd; clippy -D warnings, fmt, typos clean.

🤖 Generated with Claude Code

@beinan @claude
fix(master): publish merged watermarks after release, one shard per put
5ea483e 
Review of lance-format#350 found three defects in coupling demand to the release
transaction:

- The base-table watermark read ran after the watchdog had exited and
  before release, with no deadline of its own. A stalled storage read
  held a completed merge's ownership indefinitely.
- The release txn compared every shard's demand key by mod-revision. A
  normal writer flush on any shard between read and txn failed the whole
  release. Reproduced.
- A table with 129+ shards exceeded etcd's default 128-op txn limit.

Release is now the plain ownership transaction again (release_with_demand
and demand_release_changes are removed). After a successful Catchup
release, demand_publish::spawn_merged_demand reads the merged watermarks
and publishes one monotonic put per shard, detached (never awaited),
bounded (15 s timeout, 4 slots, drop-and-count), with a bounded CAS
retry against the fresh record when a writer flush wins the race. Loss
or lateness cannot misstate demand: each put is a per-dimension join and
the scan reconciles.

Both publish paths (writer and executor) now share join_events and the
same retry, so neither publisher's progress is lost to the other's -
the earlier writer publish could still lose a race and report Ok(false).

Metric master_demand_merged_watermarks_total gains a result label.

Validation: new etcd test publishes writer sealed=25 and executor
merged=18 concurrently on one shard and asserts both succeed and both
survive; lower merged is a no-op; unknown shard gets a fresh epoch-0
record. The resident two-master merge test polls for the detached
publish and still asserts both shards fold to zero pending. 7 demand
tests, 9 master maintenance/resident etcd tests, 9 sweeper tests pass.
Clippy -D warnings, fmt, typos clean.

Refs lance-format#333. Review finding 2 (round 4) on lance-format#350.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Oct 8, 2026
Merged
Merged
Open
@beinan
beinan merged commit d815896 into lance-format:main Oct 8, 2026
11 checks passed
beinan added a commit that referenced this pull request Oct 8, 2026
@beinan @claude
fix(merge): demand fold orders by generation, carries age, refuses ne…
2ef7a11 
…wer schema (#359)

Part of #333. **Supersedes #352** (which I will close). Fixes review
findings **1 [P1], 4, 5 [P2]** (round 4) (credit: production review).
Stacked on #357.

## What #352 got wrong

It ordered marks by `(epoch, value)` — treating a writer epoch change as
a numbering restart. Lance keeps generation numbering on a new writer
claim (`ShardManifest { writer_epoch: next, ..base }`). So "merged to
40, new writer flushes to 60" is **20 pending**; #352 said 60 in both
orders.

## Fix

- `Mark` orders by `(value, epoch)`; epoch is a fence/tiebreak. `pending
= sealed − merged`, plain.
- Stored record is the per-dimension join; `fold` applies the same join
→ **rebuild from etcd equals in-memory fold**, now including the age
clock, which the stored record carries (`sealed_times`, bounded to 64
lowest unmerged, min per generation). Finding 4: in-memory 10 vs rebuilt
20 before.
- `merged_epoch` keeps provenance when merged was reported under a
different epoch.
- Publishers **refuse to overwrite a stored record from a newer schema**
(finding 5: `v=999` was overwritten). Both publish paths share one
read-join-CAS with bounded retry.
- Spec §1.1 rewritten.

## Validation (15 tests, all green against real etcd)

- Named counterexamples: round-3 (both), round-4 (merged 40 / epoch-2
flush 60 ⇒ 20, in every order, and after rebuild).
- Writer never forgets merged; lower value from any source is a no-op;
marks order by generation with epoch tiebreak.
- Age clock: stable across newer flushes, moves correctly on merge,
**identical after rebuild**; beyond-cap still exact for counts.
- Schema: enforced on fold; stored `v=999` untouched by both publishers
(mod_revision unchanged).
- Concurrent writer + executor publishes both succeed and both survive.
- **Randomized**: 200 × 5-event streams (every permutation) and 50 ×
200-event (reverse, shuffle) over 2 shards / 3 epochs / 3 sources — fold
order-independent **and** `rebuild(events) == fold(events)`.
- Resident two-master merge and 9 sweeper tests pass; clippy `-D
warnings`, fmt, typos clean.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
beinan added a commit that referenced this pull request Oct 8, 2026
@beinan @claude
feat(master): stats scan publishes scan-sourced demand for every shard (
0312239 
#356)

Part of #333 (P1.4). Spec §1.3, source `scan`. **Stacked on #359**
(rebased from the superseded #352); merge after #357 and #359.

## What

- **core**: `all_shard_watermarks()` — every shard's `(shard_id,
writer_epoch, max flushed generation, pending)` from its latest
manifest. Does **not** enforce the read cap (the scheduler must see an
over-cap shard, not be refused).
- **master**: `demand_publish::spawn_scan_demand` after each successful
observation. Detached and bounded (15 s, 4 slots, drop-and-count), one
`scan`-sourced event per shard via `publish_demand_event`'s
per-dimension join, so a scan can only raise and never lowers a `merged`
it does not know. Metric `master_scan_demand_events_total{result}`.
Existing enqueue/retire behaviour untouched (review point 5 / P1 rule).

## Validation

- `scan_publishes_every_shard_and_never_lowers_a_watermark` (etcd): two
writer shards, no server flush path → one event per shard, fold = 6
pending; executor reports `merged=2` on a shard; a second scan leaves
`merged` intact and does not bump `mod_revision`.
- 15 scanner unit tests; clippy `-D warnings`, fmt, typos clean.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@beinan