Skip to content

fix(materialize): bound the full read a source falls back to when its watermark expires - #1663

Open
christophediprima wants to merge 6 commits into
fluree:mainfrom
christophediprima:fix/materialize-unbounded-full-read
Open

fix(materialize): bound the full read a source falls back to when its watermark expires#1663
christophediprima wants to merge 6 commits into
fluree:mainfrom
christophediprima:fix/materialize-unbounded-full-read

Conversation

@christophediprima

Copy link
Copy Markdown
Contributor

The bug

A materialize source whose stored watermark snapshot has been expired by the source table's snapshot retention cannot prove incremental safety, so it falls back to a full table read. That fallback is unbounded, and being unbounded makes it self-sustaining rather than merely slow:

  1. A full read is the most expensive read the table has, so it is the most likely to exhaust the target's novelty and defer.
  2. A deferral writes no watermark.
  3. So the next poll performs the same full read — and every poll after that, indefinitely. Nothing recovers it.

One deployment had 13 of 17 tables in exactly this state, each reading its whole self on every poll, with no watermark moving for hours.

The fix

A full read cannot be bounded by snapshot, because there is no from to take a prefix after. So bound it by rows and checkpoint at the snapshot that prefix corresponds to.

  • FileScanTask gains data_sequence_number. Iceberg assigns it per commit and never reuses or reorders it, so it is the only value on a task that says when the rows arrived — paths are arbitrary. Both planners already computed it and then dropped it; they now attach it. Option, not a sentinel: a task the planner cannot attribute is "unknown", and a consumer must not read that as "oldest".
  • Both plans are sorted by (data_sequence_number, file_path). Manifest traversal is sequential and manifest files are immutable, so for a pinned snapshot the order was already stable — but only incidentally, and a resumed scan against a different order either repeats files or skips them. Sequence first with path only as a tiebreak is the point: across a resume boundary, if a key's newer row is processed in one pass and its older row in the next, the older one lands last and wins.
  • A row-budgeted prefix of that sorted plan is therefore a prefix in commit order. snapshot_at_or_before_sequence names the snapshot that prefix leaves the target at, and reporting that as the scan's to makes the caller's existing watermark write land there. No new vocabulary, no schema change, and the crash-safety ordering is unchanged — the watermark still follows the data.
  • FLUREE_MATERIALIZE_MAX_ROWS_PER_FULL_PASS, default 250_000; 0 restores the previous read-it-all behaviour.

Two rules decide correctness, and both are asserted rather than assumed:

  • The cut extends to the end of its commit. Stopping inside one leaves the target holding part of a commit, which no snapshot names, and a state that cannot be named cannot be resumed from.
  • The checkpoint rounds DOWN, to the newest ancestor whose sequence is fully applied. Rounding up would write a watermark claiming rows that were never applied — silent data loss — and the next scan would start after them.

Every reason to decline returns "read it whole", which is today's behaviour: the bound is disabled, the plan already fits, the cut would land on the head anyway, or any task lacks an attributable sequence number. That last one matters — without a sequence there is no safe cut, and an expensive honest read beats a wrong checkpoint.

The third commit is the one that makes the other two work

snapshot_at_or_before_sequence was first written using snapshot_window(None, to_id), which walks the whole history to the root and errors on any missing ancestor.

The tables that need a checkpoint are precisely the ones whose old snapshots have expired — that is why they are full-reading. So the helper errored on every one of them, and the caller fell through to the unbounded read it was supposed to prevent. The bound could never fire on the only tables it was written for, and it failed silently: every unit test passed, because every fixture had an intact chain.

It now walks parents from to and stops at the first qualifying snapshot. The walk is short — the cut lands near the start of the backlog, so the answer is a few hops from to, nowhere near the expired region. Reaching the root, or a parent that is gone, means there is no nameable checkpoint: that is None, and the caller reads the whole thing, which is correct rather than merely safe.

The regression test uses a deliberately severed chain (5 ← 6 ← 7 retained, 4 expired) because that is the shape the original fixtures could not express.

Behaviour and compatibility

A bounded pass does not change what is read. A full read selects the files live at to; applying those at or below the cut is exactly the state at the checkpoint snapshot, and the next pass scans (checkpoint, to] for the rest.

The first commit is behaviour-neutral on its own: nothing consumes data_sequence_number yet, and the sort only makes the existing order explicit.

Tests

Three mutations, each killed: cutting inside a commit, rounding the checkpoint up, and letting an unattributed task through. Plus a regression test for the expired-ancestor walk, on a severed snapshot chain.

cargo clippy --all --all-targets -- -D warnings and the same with --all-features are both clean; fluree-db-iceberg 259 pass, fluree-db-api --lib 756 pass.

Groundwork for resumable materialization. A consumer that stops part-way
through a backlog and resumes needs two things this planner did not provide:
to know WHEN each file's rows entered the table, and for the file order to be
a contract rather than an accident.

`FileScanTask` gains `data_sequence_number`. Iceberg assigns it per commit and
never reuses or reorders it, so it is the only value on a task that says when
the rows arrived — paths are arbitrary. Both planners already compute it to
decide what to select and then dropped it; they now attach it. `Option`, not a
sentinel: a task the planner cannot attribute is "unknown", and a consumer must
not read that as "oldest".

Both plans are then sorted by `(data_sequence_number, file_path)`.

Manifest traversal is sequential and manifest files are immutable, so for a
pinned snapshot the order was already stable — but only incidentally. Anything
that later reads manifests concurrently would silently change it, and a resumed
scan against a different order either repeats files or skips them. Sorting makes
the guarantee explicit at the cost of a sort over file metadata.

Sequence number first, path only as a tiebreak within one commit, and that
direction is the point rather than a style choice. Materialization decides
latest-by-key from an ordering column in the data, not from scan order, so
within a single pass file order does not matter. ACROSS a resume boundary it
does: if a key's newer row is processed in one pass and its older row in the
next, the older one lands last and wins. Processing commits in order is what
prevents that, and sorting by path alone would not.

The full-scan path carries it too. That is the read a consumer falls back to
when its watermark has expired — precisely the read that most needs to become
resumable.

No behaviour change: nothing consumes the field yet, and the sort only makes
the existing order explicit.
…ndary

A full read is the path a source lands on once its watermark has expired, and
without a bound it is a trap rather than merely slow. It is the most expensive
read the table has, so it is the most likely to exhaust the target's novelty and
defer; a deferral writes no watermark; so the next poll performs the same full
read. Every later poll is then the same read, forever, and nothing recovers it.

Measured: one deployment had 13 of 17 tables in exactly this state, each reading
its whole self on every poll, with no watermark moving for hours.

A full read cannot be bounded by snapshot — there is no `from` to take a prefix
after — so bound it by rows and checkpoint at the snapshot that prefix
corresponds to. `FLUREE_MATERIALIZE_MAX_ROWS_PER_FULL_PASS`, default 250_000;
`0` restores the previous read-it-all behaviour.

The mechanism needs no new vocabulary and no schema change. Tasks now arrive
sorted by `(data_sequence_number, path)`, so a row-budgeted prefix is a prefix
in COMMIT order; `snapshot_at_or_before_sequence` names the snapshot that prefix
leaves the target at; and reporting THAT as the scan's `to` makes the caller's
existing watermark write land there. The crash-safety ordering is unchanged —
the watermark still follows the data.

Two rules decide correctness, and both are asserted rather than assumed:

- **The cut extends to the end of its commit.** Stopping inside one leaves the
  target holding part of a commit, which no snapshot names, and a state that
  cannot be named cannot be resumed from.
- **The checkpoint rounds DOWN**, to the newest ancestor whose sequence is fully
  applied. Rounding up would write a watermark claiming rows that were never
  applied — silent data loss, and the next scan would start after them.

Every reason to decline returns "read it whole", which is today's behaviour:
the bound is disabled, the plan already fits, the cut would land on the head
anyway, or any task lacks an attributable sequence number. That last one
matters — without a sequence there is no safe cut, and an expensive honest read
beats a wrong checkpoint.

This does not change what a bounded pass reads. A full read selects the files
live at `to`; applying those at or below the cut is exactly the state at the
checkpoint snapshot, and the next pass scans `(checkpoint, to]` for the rest.

Three mutations, each killed: cutting inside a commit, rounding the checkpoint
up, and letting an unattributed task through.
`snapshot_at_or_before_sequence` used `snapshot_window(None, to_id)`, which
walks the whole history to the ROOT and errors on any missing ancestor.

The tables that need a checkpoint are precisely the ones whose old snapshots
have expired — that is why they are full-reading in the first place. So the
helper errored on every one of them and the caller fell through to an unbounded
read. The bound could never fire on the only tables it was written for, and it
failed silently: every unit test passed, because every fixture had an intact
chain.

Walk parents from `to` and stop at the first qualifying snapshot instead. The
walk is short — the cut lands near the start of the backlog, so the answer is a
few hops from `to`, nowhere near the expired region. Reaching the root, or a
parent that is gone, means there is no nameable checkpoint: that is `None`, and
the caller reads the whole thing, which is correct rather than merely safe.

The regression test uses a deliberately severed chain (5 <- 6 <- 7 retained,
4 expired) because that is the shape the original fixtures could not express.
…ommit

The row bound added earlier could not fire on the deployment it was written for,
and when it declined it said nothing. Measured in production: its success line
appeared 0 times across 16 consecutive passes while the same 727k-row table was
re-read roughly once a minute, and finding out why needed a state-ledger query
rather than a log read. Three defects, one per change here.

1. The budget was denominated in the wrong currency. A bounded pass is only
   worth anything if it can COMMIT: `at_max_novelty` is
   `novelty.size >= reindex_max_bytes`, a window over that ceiling is DEFERRED,
   and a deferral discards the window's progress — so the next poll re-reads the
   same rows and stops at the same wall. A flat 250_000 rows is ~27 MB of flakes,
   which against a deployment that has pinned the ceiling to 8 MiB never commits;
   the bound therefore changed how much was read and nothing about whether any of
   it landed. The budget is now DERIVED from the ceiling, read from the same
   `FLUREE_REINDEX_MAX_BYTES` that configures it so the two cannot drift, via a
   tunable `FLUREE_MATERIALIZE_FLAKE_BYTES_PER_ROW` (default 108, measured).
   `FLUREE_MATERIALIZE_MAX_ROWS_PER_FULL_PASS` still overrides, `0` still
   disables. Floored at 1_000 rows so a tiny ceiling still makes progress instead
   of a zero-row pass that reads nothing and checkpoints nowhere.

2. Running out of budget inside the HEAD commit declined outright, which is how a
   COMPACTED table livelocks: a compaction rewrites every file with one sequence
   number, so from then on the budget always lands in the head commit and the read
   is never bounded. It now falls back to the newest boundary strictly below the
   head — a smaller pass than asked for, and still forward progress, which is the
   only property that matters. The test that asserted the old `None` here was
   encoding the bug, so its expectation is flipped.

3. Every decline was silent. `full_read_prefix` now returns a typed `FullReadCut`
   and the caller logs which reason fired, warning on the two that mean an
   unbounded read (`NoSequence`, `SingleCommit`) and each checkpoint-walk failure,
   while staying quiet on `PlanFits` and `Disabled` so the log does not fill with
   noise on every small read.

`rows_for_ceiling` is split out from the env plumbing so the arithmetic that has
to be right is testable without process-global state; its test asserts
`rows * bytes_per_row <= ceiling` directly. New tests cover the compaction shape
and that the healthy outcomes are NOT alarming, so logging stays proportionate.

Known limit, deliberately left: flake-bytes-per-row is a heuristic measured on one
table, so a much wider row still overshoots. Stopping on accumulated bytes rather
than a row count is the robust version and is a larger change.

Verified: fmt clean, clippy clean under BOTH default and all features,
`fluree-db-api --all-features --lib graph_source::r2rml` 83 pass / 0 fail.
…ursor

The durable fix for the livelock. A source whose stored watermark has fallen
outside the source table's snapshot retention full-reads the whole table on every
poll, forever, and no amount of bounding by SNAPSHOT can rescue it:
`snapshot_at_or_before_sequence` can only name a checkpoint that is still
retained, and on the only tables that need one there is nothing to name. So the
bound declines, the read stays unbounded, it exceeds the novelty ceiling, the
window is deferred, the deferral records no progress, and the next poll performs
the identical read. Measured in production: 16 identical 727k-row reads in 14
minutes with `from_snapshot_id` frozen, and two of four fan-out targets receiving
no rows from that table at all.

Bound it by COMMIT SEQUENCE instead. Iceberg assigns sequence numbers per commit
and never reuses or reorders them, and unlike a snapshot id a sequence stays
meaningful after the snapshot that carried it has been expired away — which is
precisely the state that forced the full read. `sequence_prefix_cut` therefore
needs nothing retained; note what is absent from its tests: any TableMetadata,
any snapshot lookup, any intact history.

Each pass takes whole commits above the cursor until the row budget (already
derived from the novelty ceiling) is spent, applies them, and records the highest
sequence it covered. The next pass resumes above that. A table too large to
materialize in one pass now drains in bounded steps instead of never.

THE SAFETY PROPERTY, and the one thing to check in review: a partial pass writes
the cursor and MUST NOT advance the snapshot watermark. Advancing it while the
target holds only a prefix claims rows that were never applied and makes the next
scan start after them — the applied-marker ratchet that silently discarded ~80 %
of a deployment's entities. `MaterializeScan::to_sequence` encodes it in one
field (`Some` => prefix => suppress the snapshot write), `watermark_node` writes
cursor XOR snapshot, and
`a_partial_pass_writes_the_cursor_and_never_advances_the_snapshot` asserts it
directly so it cannot be reintroduced quietly.

Additive on purpose. `appliedSequence` sits alongside `lastSnapshotId` rather
than replacing it: no state migration, the snapshot stays the fast path, and
rolling back to an image without this loses resumability but not watermarks.

A cursor left behind by a COMPLETE pass is deliberately not cleared, and that is
safe rather than sloppy: everything at or below it was applied when it was set,
and anything modified since carries a higher sequence — the same invariant the
incremental path already relies on. Clearing it would need an explicit retract
for no benefit.

`force_full` ignores the cursor: an explicit full refresh means start over, so
honouring a cursor would silently narrow it.

Verified: fmt clean, clippy clean under BOTH default and all features with
--all-targets, fluree-db-api --all-features --lib 1076 pass, graph_source::r2rml
87 pass, 0 failures.

Caught late and worth recording: six call sites lived in fluree-db-api/tests/,
so `--lib` passed at 1076 while `clippy --all-targets` failed on all six. A
--lib run hides integration targets exactly as it hides feature-gated modules.
…nd engages

Found by deploying the previous two commits to production and watching them do
nothing. The arithmetic was individually correct at every step, which is why no
unit test caught it.

Raising the ceiling 8 MiB -> 96 MiB to make the window committable also took the
derived row budget to ~932k rows, against a 733,608-row table. The budget
exceeded the table, so `full_read_prefix` answered `PlanFits`, no prefix was
taken and no cursor was ever written — while two of four fan-out targets still
could not absorb the window and kept deferring (8 deferrals and 6 `novelty at
max` in 4 minutes). The watermark stayed frozen at the same snapshot it had been
stuck on for hours, and zero `appliedSequence` rows existed in the state ledger.

Raising the ceiling had DISABLED the mechanism meant to drain it. A bound has to
ENGAGE to be worth anything, and a budget larger than the table cannot engage —
the pass then fails in the APPLY instead, which is where novelty is actually
spent and where nothing records progress.

So a pass now claims `ceiling / 4`. At 96 MiB that is 233,016 rows (24 MiB of
flakes), which cuts the 733,608-row plan into ~4 committable passes, each writing
a cursor the next resumes from. Chunks stay pinned at 2 MiB, so staging is
untouched.

A quarter rather than some other fraction for three reasons. It is the honest
share: the ceiling is per-ledger and 17 sources commit into it, so one table's
pass laying claim to all of it was never right. It matches the per-transaction
budget's existing `reindexMaxBytes / 4` derivation. And it leaves margin for
`flake_bytes_per_row` being an UNDER-estimate — 108 was measured per accumulated
item, and a source row can yield several, which is why 733k rows did not in fact
fit under 96 MiB.

`the_budget_still_engages_on_the_table_that_livelocked` pins the property that
actually matters, which is not "the budget fits the ceiling" but "the budget is
small enough to engage on the table that livelocked". It also asserts the budget
still tracks the ceiling upward, so this stays a fraction rather than becoming a
cap.

Verified: fmt clean, clippy clean under BOTH default and all features with
--all-targets, fluree-db-api --all-features --lib 1077 pass, graph_source::r2rml
88 pass, 0 failures.

@aaj3f aaj3f left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christophediprima this is clearly a real issue and I'm grateful for the PR and the engineering narrative established over the six commits. The test discipline also held up under adversarial checking (I ran six mutations, your claimed three included, and every one died by the named test).

But two wiring gaps undercut the headline before it can deliver on its value.

First, the cursor route can't bootstrap: to_sequence only becomes Some inside the branch that requires an existing cursor (r2rml.rs:1683/:1713), and the checkpoint route can't name a snapshot on expired-history tables (your own walk test's None case). The consequence is that on the "13 of 17 tables" shape this PR is named for, the first pass is still the unbounded read, and if it defers the livelock persists exactly as before.

Second, novelty_ceiling_bytes() reads FLUREE_REINDEX_MAX_BYTES from the environment while flag-, config-file-, and builder-configured deployments enforce a different ceiling. For them the budget is derived from a sysinfo default and the "bound that cannot commit" (or the PlanFits-forever variant you just fixed) comes straight back.

Both fixes are small and local: write the first cursor from the unnameable-checkpoint decline arm (v1-safe via SingleCommit), and thread self.fluree.index_config.reindex_max_bytes into the budget instead of the env re-read. Details and test shapes inline.

Adherence to repo commitments:

  • Patterns/abstractions: ✔ attaches data the planners already computed, reuses the existing watermark write by reporting a shorter to, keeps the cursor additive beside lastSnapshotId--no parallel constructs.
  • Performance (speed first, memory second): ✔ background materialize path; metadata-only sort; bounded passes cut peak memory and novelty pressure; pre-filter record_count budgeting errs safe. No query-hot-path risk — the blockers are availability, not speed.
  • Testing: ⚠️ excellent unit coverage, all 13 new tests verified running by name and six mutations verified killed — but the two blocking gaps are both composition seams no test crosses, and the PR's own history shows that's where this feature gets hurt.
  • Conventions: ⚠️ commit bodies are exemplary; fmt/clippy clean (verified package-scoped; CI all-features green); the PR body is stale against the branch (it seems to only cover the first few commits) and should be refreshed since it feeds release archaeology.

Verified locally at branch HEAD: fluree-db-iceberg 281/281, api graph_source::r2rml 88/88 under --features iceberg,native, both it_iceberg_* targets green, six mutation checks red-then-restored, delete-file/mor_guard and compaction convergence traced through plan_incremental.

The bones here are exactly right, just be sure the first-pass cursor bootstrap and the real-ceiling plumbing land before this merges (they're the difference between "fixed the livelock" and "fixed the livelock for env-var deployments whose history hasn't expired yet")

);
// `Some` => prefix only, so the caller persists the cursor and
// leaves the snapshot watermark alone.
to_sequence = keep_through;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — the cursor route cannot bootstrap: on the expired-history tables this PR names as its motivation, the first pass is still unbounded and the livelock survives.

Commit d674f0d0d calls the sequence cursor "the durable fix for the livelock" and argues — correctly — that "no amount of bounding by SNAPSHOT can rescue" a table whose history has expired, because snapshot_at_or_before_sequence can only name a retained snapshot. But trace who ever writes the first cursor: to_sequence = Some(...) is assigned in exactly one place (this line), inside the branch that only runs when from_sequence is already Some (cursor_cut = from_sequence.and_then(...), :1683). from_sequence comes from materialize_sequence_cursor, which is None on first run by design (and its doc-comment is right that absent must never read as 0 — on a v1 table every data sequence IS 0, so a zero cursor would exclude the entire table and advance the watermark over rows never applied).

So on a table whose watermark AND backlog base are expired — the "13 of 17 tables" shape the PR body leads with — the first pass goes: no cursor → checkpoint route → the cut lands in the ancient region, every retained snapshot's sequence is above it, the walk hits the expired parent → None (exactly what snapshot_at_or_before_sequence_survives_an_expired_ancestor's at(5) == None case pins) → warn + unbounded whole read. If that read defers — the motivating failure — nothing is recorded and the identical read repeats next poll. The mechanism built for these tables can't take its first step on them; it only ever engages once the checkpoint route has bootstrapped a cursor, which requires retained history.

The fix looks like one arm: in the unnameable-checkpoint decline (outcome match, :1726-1750), instead of falling through to plan.tasks, keep the full_read_prefix prefix and report to_sequence = Some(cut_seq)watermark_node already writes cursor XOR snapshot, so the next poll resumes above the cut via the cursor route, and the pass count is exactly what the cursor path would have produced. It's v1-safe for free: an all-zero-sequence table lands in SingleCommit inside full_read_prefix and still declines. And it's testable with the fixture shape you already built for the walk — severed chain + over-budget plan → first pass must yield a kept prefix and to_sequence: Some, not an unbounded read. Notably, sequence_prefix_cut_resumes_above_the_cursor_without_naming_a_snapshot already proves the machinery handles a cursor at the very start of history — the wiring just never supplies one.

/// `IndexConfig::reindex_max_bytes`, which `at_max_novelty` compares novelty
/// against (`novelty.size >= reindex_max_bytes`) — so the budget below and the
/// wall it has to fit under cannot drift apart.
fn novelty_ceiling_bytes() -> i64 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — novelty_ceiling_bytes() reads the env var, not the configured ceiling, so flag- and config-file-configured deployments derive the budget from the wrong wall — resurrecting the "bound that cannot commit."

The derivation's whole argument (commit 82317e709: "read from the same FLUREE_REINDEX_MAX_BYTES that configures it so the two cannot drift") only holds for deployments that configure the ceiling through the environment. --reindex-max-bytes is a clap flag with env = "FLUREE_REINDEX_MAX_BYTES" as fallback (fluree-db-server/src/config.rs:426), and there's also the [server.indexing] reindex_max_bytes config-file key and FlureeBuilder::with_indexing_thresholds for embedded callers — in all three of those, at_max_novelty enforces the operator's value while novelty_ceiling_bytes() sees no env var and silently falls back to default_reindex_max_bytes() (a sysinfo RAM-fraction, server_defaults.rs:48).

When the real ceiling is smaller than that default — the 8 MiB pin from your own production narrative is exactly this shape — the derived budget is again too big to commit, the pass defers, no watermark or cursor is written, and we're back to the livelock this PR exists to kill, now with a log line claiming the bound is derived "so the two cannot drift." The drift direction can also produce PlanFits-forever (the c5cbdd7bb failure) when the default exceeds the table.

The provider already holds &Fluree, and Fluree.index_config.reindex_max_bytes is the resolved value the process actually enforces — thread that in (make materialize_max_rows_per_full_pass take the ceiling as a parameter; keep FLUREE_MATERIALIZE_MAX_ROWS_PER_FULL_PASS as the explicit override). That also removes the OnceLock on the ceiling, which today freezes the first-seen env value for the process lifetime and makes the derivation untestable end-to-end — rows_for_ceiling being split out for testability was the right instinct; plumbing the real ceiling finishes the job.

// A full read carries it too. This is the read a consumer falls
// back to when its watermark expired, so it is exactly the read
// that most needs to be resumable.
.with_data_sequence_number(eff_seq);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accuracy nit. Commit 4b334e350 says "Both planners already computed it and then dropped it; they now attach it" — the non-Send ScanPlanner's plan_scan/plan_scan_for_snapshot (planner.rs:254/:264) still don't attach a sequence. Harmless (its consumers get None and every None declines the cut, and materialize uses SendScanPlanner exclusively — I checked call sites), but worth a word so nobody trusts ordering from the non-Send planner.

(Commenting here because planner.rs:254 is not in this diff.)

/// rows, and returns the sequence of the commit the budget ran out in. The whole
/// of that commit is kept: a cut INSIDE a commit leaves the target in a state no
/// snapshot names, and an unnameable state cannot be checkpointed.
fn full_read_prefix(tasks: &[fluree_db_iceberg::scan::FileScanTask], max_rows: i64) -> FullReadCut {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future-proofing, one comment. The cross-cut delete-file hazard (a delete file with sequence above the cut targeting data files at/below it) is structurally unreachable today because mor_guard fails closed on delete-bearing snapshots before any of this runs — but the day MoR delete support lands, a row-budgeted prefix silently becomes unsound. A one-line comment naming that dependency next to the cut logic would make the future work trip over it instead of past it.

// Full unpinned streaming read: all 5 rows.
let scan = provider
.scan_for_materialize_stream(gs, "silver.people", &[], None, None)
.scan_for_materialize_stream(gs, "silver.people", &[], None, None, None)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation, not a demand. The unit coverage here is genuinely strong (and I verified all six mutations die by name — the three you claimed plus three more), but both production surprises in this PR's own history (af8269e64's intact-chain fixtures, c5cbdd7bb's individually-correct arithmetic) were composition failures no unit test saw. One it_ test that drains an over-budget local-fs table in multiple passes through the real state ledger would pin the loop the way nothing else can — and the ceiling plumbing (parameterized instead of OnceLock env reads) is what makes such a test writable. It does depend on the ceiling plumbing landing first, so it's a real scope call — but given that both production surprises here were composition failures, I'd rather see this PR grow to include it than watch it wait in the backlog.

@@ -918,10 +998,25 @@ fn watermark_node(
table_name,
)),
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Praise. The cursor-XOR-snapshot encoding, with the ratchet documented in the field's own doc-comment and pinned by a_partial_pass_writes_the_cursor_and_never_advances_the_snapshot, is exactly how an invariant that once discarded 80% of a deployment's entities should be nailed down. Same for the round-down checkpoint walk and its severed-chain test — the "note what is absent from its tests" framing in d674f0d0d is the kind of commit-message reviewers dream about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants