Skip to content

fix(iceberg): fail closed on merge-on-read delete files (silent stale-row risk)#1520

Draft
aaj3f wants to merge 2 commits into
mainfrom
fix/iceberg-mor-delete-guard
Draft

fix(iceberg): fail closed on merge-on-read delete files (silent stale-row risk)#1520
aaj3f wants to merge 2 commits into
mainfrom
fix/iceberg-mor-delete-guard

Conversation

@aaj3f

@aaj3f aaj3f commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

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).

This PR Finding Tier Where it sits in the program
Fail-closed MoR delete-file guard F-AUD-1 Tier-0 (ship regardless) Base = main (portable, gets real CI). The other Tier-0/1/2 items are branch-coupled and stack on the perf/audit-tier012 integration branch per DEC-001.

Audit references: 00-MASTER-AUDIT.md (§2 F-AUD-1, §6 Tier-0) · DEC-001-pr-bundling.md (this PR's charter) · 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=1 manifests) but never applies them: delete manifests are dropped with a debug! log and only the live data files are read (manifest_list.rs, planner.rs, and no delete reader anywhere in io/). Consequences, both silent:

  • A query over a MoR-maintained table returns deleted rows as if live.
  • The manifest COUNT(*) / row-total shortcut over-counts by exactly the delete cardinality (stats.rs sums record_count, never subtracting deletes).

Copy-on-write deletes are not affected — status=DELETED manifest entries are already dropped correctly, so CoW scans exactly ADDED+EXISTING files. The gap is strictly MoR delete files.

Exposure (why this matters now)

  • Snowflake-managed v2 (what the smoke datasets are today): SAFE. V1 measured the deployed fl-svl-iceberg-smoke-use2 tables 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.
  • External writers: MoR now. Athena DELETE writes 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.
  • Snowflake, imminent. ICEBERG_MERGE_ON_READ_BEHAVIOR=AUTO already 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. The 2026_03 date 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:

  1. Snapshot summary counters (total-delete-files / total-position-deletes / total-equality-deletes) — a zero extra-I/O check; the summary is already in memory. New Snapshot accessors mirror deleted_records().
  2. Manifest list — a content=1 delete 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-safe SendScanPlanner used 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

Switch Default Effect Coverage
FLUREE_ICEBERG_ALLOW_MOR_DELETES off (guard active) Truthy (1/true/yes/on) disables the guard: delete files ignored, read proceeds, results may include deleted rows / over-count, warns once per table. mor_guard unit tests (both arms) + planner + stats fixture tests

Field 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 no SWITCHES.md on main; the harness PR (PR-HARNESS) folds this row into the regenerated registry.

Tests (the fixture that did not exist before)

V1/DEC-001 flagged that no MoR delete fixtures existed, so CI proved nothing about this class. This PR adds real content=1 delete fixtures (in-code Avro) that drive the guard end-to-end:

  • Snapshot accessors — present/absent counters (snapshot.rs).
  • Guard logic (mor_guard.rs) — position>0 refuses; equality>0 refuses; zero/absent proceeds; override proceeds; error message names the table + switch; pure truthy-parsing.
  • Planner, real Avro (planner.rs) — a manifest list with a content=1 entry drives plan_scan() to MergeOnReadDeletes; a summary-counter snapshot refuses before any I/O; a delete-free snapshot plans normally.
  • Stats/COUNT path (stats.rs) — read_snapshot_data_files detects deletes, then the guard refuses (default) / proceeds (override), mirroring the iceberg_catalog.rs consumer 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 fires fmt/clippy/test/testsuite-sparql, even while draft).

Gate Command Result
fmt cargo fmt -p fluree-db-iceberg -p fluree-db-api -- --check clean
clippy (scoped, -D warnings) cargo clippy -p fluree-db-iceberg --all-features --all-targets --no-deps -- -D warnings clean (no pre-existing lints in this crate)
iceberg tests cargo test -p fluree-db-iceberg --all-features 200 passed, 0 failed (was 188; +12 new)
api tests (compile surface) cargo test -p fluree-db-api --features iceberg all bins pass, 0 failed (102 ignored = live-Snowflake)
featureless / default build cargo build -p fluree-db-iceberg --no-default-features + default both clean
api clippy (edited file) cargo clippy -p fluree-db-api --features iceberg --no-deps no findings in iceberg_catalog.rs

Perf 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:

  • Shared planner guard helpers. The identical guard block in ScanPlanner and SendScanPlanner is extracted into two mor_guard helpers — ensure_summary_scannable (zero-I/O summary check before the manifest-list read, returns the override flag) and ensure_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.
  • Production COUNT-path refusal test. A COUNT / table_row_count query routes through the production SendScanPlanner; fix(iceberg): fail closed on merge-on-read delete files (silent stale-row risk) #1520 originally tested only the runtime-agnostic ScanPlanner. Added a hermetic test proving SendScanPlanner::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 warnings clean; iceberg 203 tests pass; cargo test -p fluree-db-api --features iceberg 0 failures. CI re-fires on this push (base=main).

Note (residual, out of scope for this PR): the /info table_row_counts display (ledger_info.rs::fetch_virtual_table_row_counts) derives counts from the snapshot summary total-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 /info count is wanted.

aaj3f added 2 commits July 18, 2026 18:05
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.
aaj3f added a commit that referenced this pull request Jul 19, 2026
…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).
aaj3f added a commit that referenced this pull request Jul 19, 2026
…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).
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.

1 participant