fix(iceberg): fail closed on merge-on-read delete files (silent stale-row risk) - #1520
Conversation
Iceberg merge-on-read (position/equality) delete files are recognized but never applied on the virtual read path: delete manifests are dropped and only the live data files are read. A query over a MoR-maintained table silently returns deleted rows as if live, and the manifest COUNT/row-total sums over-count by the delete cardinality — a silent wrong answer, the worst failure class for a database. Copy-on-write deletes are unaffected (status=DELETED entries are already dropped). Until delete application lands, fail closed: refuse any Iceberg read whose snapshot carries delete files. Two independent signals are checked so a snapshot cannot slip through — the snapshot summary counters (zero extra I/O) and the manifest list (backstop for summaries that omit/under-count). The refusal gates both scan planners (runtime-agnostic + Send) and the stats/COUNT preview path, and is disabled by FLUREE_ICEBERG_ALLOW_MOR_DELETES (default off; warns once per table and restores the prior skip-and-proceed behavior). Adds Snapshot::total_delete_files / total_position_deletes / total_equality_deletes accessors, an IcebergError::MergeOnReadDeletes variant, real content=1 delete fixtures driving the planner + stats guards through to a refusal, and switch docs. Closes audit finding F-AUD-1.
Review follow-ups to the merge-on-read guard: - Extract the duplicated guard block from ScanPlanner and SendScanPlanner into two shared mor_guard helpers (ensure_summary_scannable + ensure_manifests_scannable) so the fail-closed check cannot drift between the two planners. Behavior is unchanged: same checks, same order (zero-I/O summary check before the manifest-list read, belt-and-suspenders after), same override. - Add an end-to-end refusal test on the PRODUCTION SendScanPlanner — the path a COUNT / table_row_count query takes. A delete-bearing snapshot fails closed with MergeOnReadDeletes, proving the guard is not merely transitive. The runtime-agnostic ScanPlanner was already covered; the Send variant shares the same helpers. - Unit-test both new helpers directly. Gates: fmt clean; clippy -p fluree-db-iceberg -D warnings clean; iceberg 203 tests pass; api (all bins) 0 failures.
…s [audit item 14] C2's manifest-backed /info stats route was gated on an empty native shell (`t == 0`), so `get_data_model` lost the virtual model the moment a virtual-dataset shell received any native write (`t > 0`). Item 14 makes routing per-member: a graph-source-registered ledger serves its manifest-backed virtual stats regardless of the native `t`. An empty shell (t == 0) serves virtual-only (unchanged); a HYBRID (t > 0 + a graph source) builds BOTH members and merges — native ledger/commit/index metadata stays authoritative and native-only classes/properties are preserved, while the graph-source member's classes are overlaid by IRI (the graph source wins a shared IRI; a UNION, never a SUM, so no double-count) and its `source` block (snapshot + counts) is attached. Bypasses the response cache (hybrids are rare); a deserialize failure falls back to the virtual model (fail-safe). New default-on switch `FLUREE_R2RML_INFO_MEMBER_ROUTING` (off = the strict t==0 reroute). Keeps the existing `FLUREE_ICEBERG_INFO_COUNT_BUDGET_MS` time-budget semantics. MoR rider (#1520 F-AUD-1): `fetch_virtual_table_row_counts` derives per-table counts from the snapshot summary `total-records`, which OVER-COUNTS a merge-on-read table (it does not subtract position/equality deletes — recognized but not yet applied). It now flags such a table via `mor_guard::summary_indicates_deletes` (zero I/O) into a new `mor-approximate-tables` list on the `source` block, so the count is reported as an honest upper bound rather than silently authoritative — matching the existing `StatsCompleteness.has_delete_files` convention. Consumers that ignore the new key (the /data-model + MCP readers, all null-safe per C-adversarial O4) are unaffected. Hermetic tests: routing default-on parse, the hybrid class-union merge (graph source wins, no double-count, native metadata kept), and the MoR upper-bound flag surfacing. Gate record: docs/audit-impl/cov-scan-gates.md (fold-side).
…s [audit item 14] C2's manifest-backed /info stats route was gated on an empty native shell (`t == 0`), so `get_data_model` lost the virtual model the moment a virtual-dataset shell received any native write (`t > 0`). Item 14 makes routing per-member: a graph-source-registered ledger serves its manifest-backed virtual stats regardless of the native `t`. An empty shell (t == 0) serves virtual-only (unchanged); a HYBRID (t > 0 + a graph source) builds BOTH members and merges — native ledger/commit/index metadata stays authoritative and native-only classes/properties are preserved, while the graph-source member's classes are overlaid by IRI (the graph source wins a shared IRI; a UNION, never a SUM, so no double-count) and its `source` block (snapshot + counts) is attached. Bypasses the response cache (hybrids are rare); a deserialize failure falls back to the virtual model (fail-safe). New default-on switch `FLUREE_R2RML_INFO_MEMBER_ROUTING` (off = the strict t==0 reroute). Keeps the existing `FLUREE_ICEBERG_INFO_COUNT_BUDGET_MS` time-budget semantics. MoR rider (#1520 F-AUD-1): `fetch_virtual_table_row_counts` derives per-table counts from the snapshot summary `total-records`, which OVER-COUNTS a merge-on-read table (it does not subtract position/equality deletes — recognized but not yet applied). It now flags such a table via `mor_guard::summary_indicates_deletes` (zero I/O) into a new `mor-approximate-tables` list on the `source` block, so the count is reported as an honest upper bound rather than silently authoritative — matching the existing `StatsCompleteness.has_delete_files` convention. Consumers that ignore the new key (the /data-model + MCP readers, all null-safe per C-adversarial O4) are unaffected. Hermetic tests: routing default-on parse, the hybrid class-union merge (graph source wins, no double-count, native metadata kept), and the MoR upper-bound flag surfacing. Gate record: docs/audit-impl/cov-scan-gates.md (fold-side).
bplatz
left a comment
bplatz
left a comment
There was a problem hiding this comment.
Approving — the design is right, and I verified the substantive claims locally rather than taking the gate table on faith: cargo test -p fluree-db-iceberg --all-features gives 203 passed / 0 failed, and cargo clippy -p fluree-db-iceberg --all-features --all-targets is clean.
Three things I checked independently and can confirm:
- No bypass on the scan path. Every production entry point (
r2rml.rs:1074and:1117,iceberg_sample.rs:143,ephemeral.rs:179) routes throughSendScanPlanner::plan_scan, so all of them inherit the guard. - The scan-files cache-hit arm is genuinely safe on
main, as claimed. It rebuilds tasks without re-enteringplan_scan, but the key ismetadata_location, which Iceberg versions per commit, and entries are only ever written from the guarded miss arm — so a cached entry can only correspond to a snapshot that already passed the guard. send_read_snapshot_data_fileshas exactly one production consumer (iceberg_catalog.rs:651), and it is guarded.
One definite issue and three worth a look items inline. The definite one is the metadata-preview path implementing only one of the two signals — it's a small fix, and I'd take it before merge since it leaves open the same silent over-count this PR exists to eliminate.
Two non-blocking notes:
- The
/infotable_row_countsdeferral is fine to defer, but worth flagging for #1528 that it is structurally unable to detect deletes rather than just missing a call:fetch_virtual_table_row_countsis metadata-only by design (loadTablesummary, never reads the manifest list), so closing it needs a different mechanism than this guard, not a guard call. - This converts silently-wrong into hard-fail for anyone already querying Athena/Flink MoR tables. Agree that's the correct call and the escape hatch covers it, but it probably deserves a release note — the failure is immediate and total rather than gradual.
| // merge-on-read deletes, so they over-count. Refuse unless | ||
| // the operator opted out via FLUREE_ICEBERG_ALLOW_MOR_DELETES. | ||
| let allow_mor = fluree_db_iceberg::mor_deletes_allowed(); | ||
| fluree_db_iceberg::ensure_no_delete_manifests( |
There was a problem hiding this comment.
Definite issue — this path implements only one of the guard's two signals.
Both planners check the summary counters and the manifest list. Here only the manifest-list signal (has_delete_files) is consulted; ensure_no_summary_deletes is never called, even though snapshot is in scope at :635.
That matters because the manifest-list signal fails open:
// manifest_list.rs:249-252
let content = match get_field("content") {
Some(AvroValue::Int(i)) => ManifestContent::from_avro(*i),
_ => ManifestContent::Data,
};Any encoding other than a bare Avro Int — a ["null","int"] union, a long — silently reclassifies a delete manifest as a data manifest. The "default": 0 in MANIFEST_LIST_SCHEMA_V2 doesn't rescue it either: that const is only used by tests (manifest_list.rs:390), while production parses with Reader::new, i.e. writer schema only, so the declared default never applies at read time.
On the planners the summary check backstops exactly this case. Here there is no backstop, so a delete-bearing table returns over-counted stats with no error and no warning.
The v2 spec makes content a required int, so this is defense-in-depth rather than something I'd expect to fire on today's data — but it is the one place where the two-signal design isn't actually two signals.
Fix is to add the summary check outside the if has_delete_files block (it needs to run precisely when has_delete_files is wrongly false):
let allow_mor = fluree_db_iceberg::mor_deletes_allowed();
fluree_db_iceberg::ensure_no_summary_deletes(snapshot, &table.qualified(), allow_mor)
.map_err(|e| crate::ApiError::config(e.to_string()))?;One knock-on: the warnings.push(...) caveat below is currently reachable only under the override, which is correct — but if the summary arm fires while has_delete_files is false, you'd want that warning on the override path too.
There was a problem hiding this comment.
Confirmed — and it extended one tier further than flagged: the Schema tier surfaces total_records with no delete detection at all (iceberg_catalog.rs:540-543 → returned at :619-628 guard-free), so a summary check added only to the Stats arm would have left the same over-count reachable. Fixed in a3068f8 by hoisting a shared preview_summary_guard that runs before BOTH tiers (planner pattern — env read once, override flag reused by the existing manifest check inside if has_delete_files), with the upper-bound caveat now pushed from either signal on the override path. Regression tests cover Stats + Schema tiers for summary-reported deletes, manifest-only deletes with a silent summary, and override-with-warning. One nit on the write-up for the record: the call the preview was making is ensure_no_delete_manifests (the manifest-signal guard) — the substance of the comment was exactly right, the named function just differs from the one at the site.
| /// summary (`total-delete-files`), if present. A value `> 0` means the | ||
| /// snapshot carries position/equality delete files that Fluree does not yet | ||
| /// apply — see [`crate::mor_guard`]. | ||
| pub fn total_delete_files(&self) -> Option<i64> { |
There was a problem hiding this comment.
Worth a look — these accessors fail open on malformed input.
.parse().ok() turns a present-but-unparseable counter into None, which ensure_no_summary_deletes then reads as 0 via unwrap_or(0) — i.e. "no deletes." The .max(0) there also clamps a negative to 0. Both resolve toward proceed inside a guard whose entire premise is to resolve toward refuse.
The manifest-list check backstops this on the planners, so it's low-stakes there. It's only genuinely unbackstopped on the preview path (see the iceberg_catalog.rs comment). If you'd rather the summary arm carry its own weight, having a present-but-unparseable value refuse — rather than read as zero — would make it fail closed independently.
There was a problem hiding this comment.
Confirmed and fixed in 83c13d9, with one deliberate placement choice: the fail-closed flip lives in the guard (ensure_no_summary_deletes now reads the raw summary map; a PRESENT key that fails integer parse — or parses negative — refuses with the malformed key/value named in the evidence text; ABSENT keys still proceed, since that arm is what the manifest-list check backstops), rather than in the shared accessors. Reason: the accessors' Option<i64> shape is consumed for display on the audit line (/info), where "unparseable" must surface as deletes-present rather than a refusal — the guard-level flip gives both consumers the right behavior, and the audit-line copy is reconciled to the same semantics in 8effa91. Unit tests: "garbage", "-1", absent-key-proceeds. Spec check for the false-positive question: snapshot summary is map<string,string> with string-encoded integer counters, so no conforming writer emits a non-integer — refusal has effectively zero false-positive risk.
| // only live data files and never applies deletes, so a MoR snapshot | ||
| // would silently return deleted rows. Cheap zero-I/O check first. | ||
| let allow_mor = | ||
| crate::mor_guard::ensure_summary_scannable(snapshot, &self.metadata.location)?; |
There was a problem hiding this comment.
Worth a look — the guard names the table's storage location, not the table.
Both planners pass &self.metadata.location, so the operator-facing error reads Refusing to read Iceberg table `s3://bucket/dw/fact_orders` . The preview path passes table.qualified() and produces dw.fact_orders, so the same refusal reads differently depending on which path tripped it.
Worth noting that error_message_is_actionable doesn't catch the difference — it calls the guard directly with a literal "dw.dim_customer", so it asserts against a string that neither planner actually produces. Not wrong, just not covering the real call sites' output.
The location is arguably the more actionable identifier of the two; the main thing is that they agree, so an operator grepping logs finds both.
There was a problem hiding this comment.
Fixed in 83c13d9: the preview now renders qualified (location) so both identifiers appear in one refusal, planners keep the location they have in scope, and an operator grepping logs finds either form. error_message_is_actionable no longer asserts a hand literal — it goes through the real call-site shapes for both the planner and preview renderings.
| &table.qualified(), | ||
| allow_mor, | ||
| ) | ||
| .map_err(|e| crate::ApiError::config(e.to_string()))?; |
There was a problem hiding this comment.
Worth a look — ApiError::config for what isn't a configuration problem.
Nothing is misconfigured when this fires: the config is correct and the table simply has delete files. Flattening it into a config error likely surfaces to the caller as a 4xx configuration complaint, which points the operator at the wrong thing — and it drops the typed MergeOnReadDeletes discriminant that the rest of the PR went to the trouble of introducing.
There was a problem hiding this comment.
Fixed in a3068f8: new feature-gated ApiError::Iceberg(#[from] IcebergError) preserves the typed discriminant end-to-end; MergeOnReadDeletes maps to 409 (nothing is misconfigured — the table simply has delete files), Display carries the guard message verbatim so every substring consumer survives. Downstream check as part of this fix: solo's only consumer of the preview error wraps it as its own fixed 400 and discards the db status code, so the 400→409 change is invisible there while the preserved message keeps any text-based handling working; surfacing the 409 in solo would be a solo-side follow-up. Tests assert 409, both identifiers, and that the message is no longer an "Invalid configuration".
…er (review W1/W2) W1 (reviewer, snapshot.rs:94): the summary MoR guard read the delete counters via the typed accessors (`.parse().ok()` + `unwrap_or(0)` + `.max(0)`), so a present-but-unparseable or negative counter resolved toward *proceed* inside a fail-closed guard. Read the raw `summary` map directly in `ensure_no_summary_deletes`: a present counter that fails to parse, is negative, or is non-zero refuses; only an ABSENT counter proceeds (the manifest-list check remains the backstop for omitted counters). Factored into `summary_delete_evidence` + a new `summary_indicates_deletes` helper (exported) so the preview path can caveat an override read that mis-parses its delete manifest. Tests: "garbage", "-1", absent-key proceeds, and the classifier. W2 (reviewer, scan/planner.rs:206): the refusal named `metadata.location` on the planners but `table.qualified()` on the preview, and `error_message_is_actionable` asserted a bare `"dw.dim_customer"` literal neither site produces. Retarget the test to assert BOTH real call-site shapes — the planner's location-only string and the preview's "qualified (location)" string (wired in the api crate). The preview identifier change lands with the D1/W3 preview edits.
…review D1/W3) D1 (reviewer, iceberg_catalog.rs:685, "Definite issue"): the preview implemented only ONE of the guard's two signals — the manifest-list count (`has_delete_files`), which fails OPEN when a delete manifest mis-parses (`content` union/long silently reclassified as a data manifest). The summary-counter check was never called even though `snapshot` was in scope, so a delete-bearing table returned over-counted stats with no error. The gap also reaches the Schema tier, which surfaces `row_count` from `total-records` with no delete detection at all. Hoist a shared `preview_summary_guard` (mirrors the two scan planners) so the zero-I/O summary check runs BEFORE any count is surfaced, for BOTH tiers; keep the manifest-list backstop inside `if has_delete_files`, reusing the override flag; and push the upper-bound caveat on the override path from EITHER signal (`preview_counts_are_upper_bounds`). W3 (reviewer, iceberg_catalog.rs:690): the preview flattened the typed `MergeOnReadDeletes` discriminant into `ApiError::config` → 400 "Invalid configuration", mislabeling a correctly-configured table as misconfigured. Add a feature-gated `ApiError::Iceberg(#[from] IcebergError)` variant (mirrors Vector/Credential), status_code → 409 for MoR (400 otherwise, preserving prior behavior), Display verbatim so the "merge-on-read" substring the CLI/solo classifiers match on survives. Replace the guard-site `.map_err(config)` calls with the typed variant via `?`. W2 wiring: the preview now passes "qualified (location)" to the guard so its refusal names both identifiers (test retargeted in the guard crate, prior commit). Tests: preview refusal covers both tiers via the pre-split guard, asserts 409 + both identifiers + verbatim message; clean-table proceed; the upper-bound caveat truth table.
aaj3f
added
the
breaking-change
…ty + stable disk-cache hasher (B2 Option A, W3) B2 (Option A, guard-independent): PersistedScanFiles/CachedScanFiles gain has_delete_manifests and CACHE_FORMAT_VERSION bumps 2->3 (struct + version + conversions + tests ported verbatim from the #1528 rung). The scan-files miss arm stamps the flag from a manifest-list-only delete probe (send_snapshot_has_delete_manifests, mirroring the COUNT(*) path's detection without re-reading data manifests); both cache-hit arms route the fetched entry through servable_scan_files, treating a delete-bearing entry as a MISS (re-plan). No merge-on-read guard exists at this rung, so this is a plain fall-through, never an override re-check — the planner is left untouched to avoid colliding with #1520 when it lands on main first. The version bump alone invalidates every v2 scanfiles entry. W3: replace std DefaultHasher (unstable across toolchains) with a fixed xxh64 stable_key_hash (seed 0) + CACHE_SCOPE filename segment + a golden pin test, so a toolchain bump no longer silently orphans the on-disk cache. Ported verbatim from the #1528 rung; disk_catalog_cache.rs is byte-identical to it.
…formed-counter test (#1520 W1/W2) Absorb #1520's W1/W2 fail-closed guard into family-c's re-applied mor_guard.rs. `summary_delete_evidence` reads the raw summary map: a present counter that is non-zero, negative, or unparseable refuses (naming key+value); an absent one proceeds (manifest-list backstop). `ensure_no_summary_deletes` and the reconciled `summary_indicates_deletes` route through it — one fail-closed definition serving the guard, the planner's ScanPlan.has_delete_manifests stamp, the /info row-count path, and override-read caveats. W1 tests (garbage / -1 / absent / classifier) and the W2 `error_message_is_actionable` retarget (planner location + preview "qualified (location)" shapes) ported. /info side: extract `metadata_indicates_mor_approximate_count` (the exact predicate `fetch_virtual_table_row_counts` uses) and pin the 1528-W1 outcome — a malformed or negative delete counter lists the table in mor-approximate-tables (row count reported as an upper bound), never counted as exact; a clean append snapshot is not flagged.
|
All four review items are addressed at this head (83c13d9 + a3068f8): the preview guard now covers both tiers including the Schema-tier gap that had no detection at all, summary counters fail closed on malformed input at the guard level, the refusal identifier is unified across planners and preview with the test retargeted to real call-site output, and the preview returns a typed 409 instead of a 400 config error with the message text preserved for downstream substring consumers (solo verified unaffected). Per the release-note suggestion: no CHANGELOG file exists in-repo — release notes are generated from PR titles via |
Reconcile graph-source secret redaction + CLI /info display refactor: - ledger_info.rs: union redact_json_secrets — keep lambda's ConfigValue::SecretRef preservation (is_secret_ref_object, checked first) AND main's #1534 fail-closed container handling (redact_secret_subtree / SECRET_REF_SAFE_KEYS). - cli iceberg.rs & info.rs: adopt main's unified graph_source_display module; drop the now-orphaned inline print/redact helpers and their tests (coverage moved to graph_source_display.rs).
…keep-min Adjudicated follow-up to bplatz's inline id=3690216248 (dupkey-adjudication.md, ruling C2). The last-wins pick on a duplicate parent join key was run-to-run NONDETERMINISTIC — the parent index consumes scan_table's buffer_unordered stream, so when a key's colliding rows span data files an I/O-completion race decided the winner, and baking one draw violated the twin's mapping-hash / stamp reproducibility contract. - Deterministic keep-min tie-break shared by BOTH parent-index builders: fluree-db-r2rml `ParentIndexSet::index_batch` (twin) and fluree-db-query `build_parent_lookup` (virtual path), via the shared `parent_key_insert_keep_min` — the lexicographically smallest parent subject wins, so twin and virtual agree AND both are reproducible regardless of scan order. Detection is free (HashMap occupancy); ambiguous keys are counted as DISTINCT keys (order-independent, unlike a raw collision tally). Shared both-feed-orders unit test. - Decline gate (C2): the twin builder refuses a source with duplicate parent keys by default, with a typed `DuplicateParentKeys` error naming the table(s), join column(s), and count; overridable via `--allow-duplicate-parent-keys` (CLI flag -> import builder option -> drive_virtual_import), mirroring the #1520 MoR-guard posture. Checked after the full index build, before the stamp, so a declined source never publishes a stamped twin. - `MaterializeStats.dup_parent_keys` + the completion stamp record the anomaly (a new OPTIONAL `dupParentKeys` stamp predicate, present only for an override-built twin, so a clean twin's stamp stays byte-identical) — the overridden twin self-documents what it baked. - Tests: decline fires; the override builds a deterministic twin (FK -> the keep-min winner in both batch orders) with the stat + stamp field set. Deferred to a tracked follow-up: the true R2RML RefObjectMap fan-out (one edge per matching parent) — a SHARED virtual+materialize bug; once it lands, re-materialization heals existing twins and the decline gate can relax to warn.
2 participants
fix(iceberg): fail closed on merge-on-read delete files (silent stale-row risk)
This is one of five surviving pull requests in the consolidated virtual-dataset review program, which collapses the #1475–#1529 lineage into five reviewable units at the maintainers' request. #1520 is the standalone correctness fix and the only member based on
main, so it receives full CI on its own branch; it is meant to land first, ahead of the four stacked survivors. Reading order across the program: #1520 (this PR) → #1507 (R2RML/Iceberg virtual-dataset performance program) → #1514 (serverless Lambda-usability + BYO-IAM) → #1528 (big-Iceberg-audit implementation) → #1529 (native-twin materialize builder). The durable, outward-facing audit record these PRs draw on is published on thedocs/virtual-dataset-audit-2026-07branch — for this PR start with 00-MASTER-AUDIT.md §2 (finding F-AUD-1) and the independent verification in V1-mor-verification.md.Original PR body
Forest map
This is PR-SAFE-MOR, the first (and only main-based) implementation PR of the big Iceberg audit. It closes audit finding F-AUD-1 (silent merge-on-read staleness).
main(portable, gets real CI). The other Tier-0/1/2 items are branch-coupled and stack on theperf/audit-tier012integration branch (see #1528).Audit references: 00-MASTER-AUDIT.md (§2 F-AUD-1, §6 Tier-0) · V1-mor-verification.md (the verified gap + fix sites).
The bug
Fluree's virtual Iceberg read path recognizes merge-on-read (MoR) position/equality delete files (
content=1manifests) but never applies them: delete manifests are dropped with adebug!log and only the live data files are read (manifest_list.rs,planner.rs, and no delete reader anywhere inio/). Consequences, both silent:COUNT(*)/ row-total shortcut over-counts by exactly the delete cardinality (stats.rssumsrecord_count, never subtracting deletes).Copy-on-write deletes are not affected — status=
DELETEDmanifest entries are already dropped correctly, so CoW scans exactly ADDED+EXISTING files. The gap is strictly MoR delete files.Exposure (why this matters now)
fl-svl-iceberg-smoke-use2tables as format-v2, append-only,total-delete-files/position/equality = 0— Snowflake's current copy-on-write default. The bug is not triggered by today's data.DELETEwrites position delete files by default; Flink/CDC upserts write equality deletes by design; Spark is one table-property away. The BYO-IAM segment (feat(iceberg): BYO-IAM on operator-owned S3 — inline metadata, fail-closed credentials, SecretRef rotation fix, typed storage errors, verify probe (#1500, #1497, #1498) #1505) is the highest-exposure surface.ICEBERG_MERGE_ON_READ_BEHAVIOR=AUTOalready means MoR for v3 (deletion vectors) and for all externally-managed tables. A pending Snowflake BCR bundle (2026_03/ bcr-2279) would flip SF-managed v2 to MoR positional deletes by default. The2026_03date is WEB-sourced and unverified — verify the BCR before scheduling against it; the gap is V1-verified, the date is not.Severity: latent-but-certain — no delete files in today's smoke data, but MoR arrives with certainty down at least four independent paths. The failure mode (silent wrong answers, no error/log/metric) is the worst class for a database, which is why this ships fail-closed regardless of the trigger date.
The guard (fail-closed, with an escape hatch)
Refuse — never silently mis-answer — any Iceberg read whose snapshot carries delete files, until MoR application is implemented. Two independent signals so a snapshot cannot slip through:
total-delete-files/total-position-deletes/total-equality-deletes) — a zero extra-I/O check; the summary is already in memory. NewSnapshotaccessors mirrordeleted_records().content=1delete manifest present even when the summary omits/under-counts it. The scan planners now parse the manifest list with deletes and count them (the list is read anyway).Both scan planners (
ScanPlanner+ the Send-safeSendScanPlannerused by the server) and the stats/COUNT preview consumer (iceberg_catalog.rs) inherit the guard, so scan results and row/COUNT totals are covered. On refusal the error is typed (IcebergError::MergeOnReadDeletes) and actionable — it names the table, the counters/evidence, why (deletes not applied → deleted rows / over-count), and the override switch.Switch | test | behavior ledger
FLUREE_ICEBERG_ALLOW_MOR_DELETES1/true/yes/on) disables the guard: delete files ignored, read proceeds, results may include deleted rows / over-count, warns once per table.mor_guardunit tests (both arms) + planner + stats fixture testsField revert is per-mechanism via the switch (no redeploy needed). Docs: added to
docs/graph-sources/iceberg.md(Limitations + a new "Environment switches" table). There is noSWITCHES.mdonmain; the harness PR (PR-HARNESS) folds this row into the regenerated registry.Tests (the fixture that did not exist before)
V1 flagged that no MoR delete fixtures existed, so CI proved nothing about this class. This PR adds real
content=1delete fixtures (in-code Avro) that drive the guard end-to-end:snapshot.rs).mor_guard.rs) — position>0 refuses; equality>0 refuses; zero/absent proceeds; override proceeds; error message names the table + switch; pure truthy-parsing.planner.rs) — a manifest list with acontent=1entry drivesplan_scan()toMergeOnReadDeletes; a summary-counter snapshot refuses before any I/O; a delete-free snapshot plans normally.stats.rs) —read_snapshot_data_filesdetects deletes, then the guard refuses (default) / proceeds (override), mirroring theiceberg_catalog.rsconsumer exactly.The override "skipped under override" arm is proven at the guard-function level rather than through the planner, to avoid mutating the shared process env in a parallel test run (documented in-code).
Verification record
All run locally in this branch's worktree (base =
main@7581f0ac8). CI runs live on this PR (base=main firesfmt/clippy/test/testsuite-sparql, even while draft).cargo fmt -p fluree-db-iceberg -p fluree-db-api -- --check-D warnings)cargo clippy -p fluree-db-iceberg --all-features --all-targets --no-deps -- -D warningscargo test -p fluree-db-iceberg --all-featurescargo test -p fluree-db-api --features icebergcargo build -p fluree-db-iceberg --no-default-features+ defaultcargo clippy -p fluree-db-api --features iceberg --no-depsiceberg_catalog.rsPerf arm: not applicable (this is a correctness guard, not an optimization) — no vbench claim.
Update — review follow-ups (commit
c5f7feaff)Two review follow-ups, behavior-preserving:
ScanPlannerandSendScanPlanneris extracted into twomor_guardhelpers —ensure_summary_scannable(zero-I/O summary check before the manifest-list read, returns the override flag) andensure_manifests_scannable(the post-read belt-and-suspenders) — so the fail-closed check cannot drift between the two planners. Same checks, same order, same override.table_row_countquery routes through the productionSendScanPlanner; fix(iceberg): fail closed on merge-on-read delete files (silent stale-row risk) #1520 originally tested only the runtime-agnosticScanPlanner. Added a hermetic test provingSendScanPlanner::plan_scan()fails closed on a delete-bearing snapshot (MergeOnReadDeletes), so the guard is not merely transitive. Both new helpers are also unit-tested directly.Gates re-run at this head: fmt clean;
clippy -p fluree-db-iceberg -D warningsclean; iceberg 203 tests pass;cargo test -p fluree-db-api --features iceberg0 failures. CI re-fires on this push (base=main).Note (residual, out of scope for this PR): the
/infotable_row_countsdisplay (ledger_info.rs::fetch_virtual_table_row_counts) derives counts from the snapshot summarytotal-records, which over-counts a merge-on-read table. It is a metadata display, not a query answer, and is not guarded here — flagged for the audit follow-up if a guarded/infocount is wanted.Consolidated scope
#1520 folds in no other pull requests — it is the standalone Tier-0 correctness fix and stands exactly as scoped. Its two commits are the guard (
771f6e88b) and the review-follow-up test plus shared planner helpers (c5f7feaff). The pre-consolidation tip is preserved at tagarchive/pre-refactor-2026-07-21/fix_iceberg-mor-delete-guard; nothing was rewritten.Verification of record
CI fires on this PR because its base is
main, sofmt/clippy/test/testsuite-sparqlrun against the change directly — it is the one survivor with real CI on its own diff, and the in-body gate table records the local reproduction.The independent adversarial diff review is recorded under R-1520 in pr-reviews-impl.md: verdict SHIP on main — the guard is complete there (both live planners plus the stats/preview consumer guarded, the in-memory scan-files cache safe by construction, and the branch-only disk cache does not exist on
main). The same review flagged that the guard was incomplete on the stacked perf line, where a cached scan-files hit-arm rebuilds scan tasks aroundplan_scan; that gap was closed on the audit line and is recorded under R-CACHE-ARM in the same file (verdict CLOSED — delete-flag on the cached entry, re-check on both hit arms, and aCACHE_FORMAT_VERSIONbump so pre-guard entries miss and recompute through the guard). That fix travels into the program via #1528, so the guard is complete on both the main-based and stacked paths.The finding itself and its fix sites are verified independently in V1-mor-verification.md with live AWS metadata measurement, and the exposure timeline is summarized in 00-MASTER-AUDIT.md §2.
Native-impact appendix
Zero native surface. Every edit is in
fluree-db-iceberg(planner.rs,send_planner.rs,stats.rs,mor_guard.rs,snapshot.rs) plusfluree-db-api/iceberg_catalog.rs, and each is reached only through the Iceberg scan / stats path — never through a native SPARQL query. The one switch,FLUREE_ICEBERG_ALLOW_MOR_DELETES, defaults off (guard active) and is an inverted-polarity escape hatch. Coverage is 203 iceberg tests including realcontent=1Avro delete fixtures. There is nothing on this PR for a native-pathway reviewer to check.Landing and hand-off notes
Land #1520 first. It shares its fail-closed guard content with #1528's line (the same guard was re-applied on the audit integration branch). The two guard commits carry different git patch-ids only because they were cut against different base trees, but their added and removed lines are byte-identical, so once #1520 merges to
main, #1528's guard portion resolves as an identical-content merge and the audit line's cache-arm guard (CACHE_FORMAT_VERSION2→3) rides on top cleanly.The
/infotable_row_countsover-count noted above as out of scope is picked up downstream: #1528 carries the rider that flags delete-bearing tables into anmor-approximate-tableslist (upper-bound counts), so this PR's deferral is closed there rather than left open.