fix(iceberg): reclaim the manifest files expired snapshots leave behind (#835)

[xe-nvdk]

Fixes #835 (first half; the second half is #1106).

The problem

expireSnapshots runs WithPostCommit(false) deliberately — #632: iceberg-go's post-commit hook would also delete Arc's data files, which the exporter must never be allowed to do. Nothing else deleted the manifest lists and manifests an expired snapshot leaves behind, and a dropped transaction leaves the same residue because iceberg-go writes its manifests before Commit. So a table's metadata/ directory grew with every commit for the life of the deployment and the whole pile was copied into every backup. A rig with one live snapshot held fifteen .avro files.

The fix

The reconciler sweeps them after the pass has committed, expired and published its discovery files.

Reachability is computed from every metadata.json on disk, not from the current snapshot: Arc deliberately keeps retain+1 v<N>.metadata.json copies and iceberg-go keeps its own NNNNN-* log as entry points a directory reader can resolve. That set only ever shrinks, and both things that shrink it have already run by the time the sweep looks — so a manifest the sweep spares stays spared, and one it deletes can never become referenced again.

Three rails bound what it can touch:

Rail	Effect
Only .avro directly in the table's metadata directory	Data files (.parquet, and outside that directory anyway), version-hint.text, the metadata files and Puffin statistics are not deletable by this path whatever reachability says
Age grace via storage.ObjectLister — 1 h, or 2× iceberg.reconcile_interval if longer	Covers a commit that wrote its manifests and then died before its metadata.json landed. A backend with no ObjectLister reports no ages, so the sweep does not run at all
Fail closed on every reachability failure	Listing error, unreadable/unparsable metadata.json, or an unopenable manifest list ⇒ incomplete set ⇒ zero deletions. A deleted manifest is the only record of which data files a snapshot held, and nothing regenerates it

This bounds the directory rather than emptying it — the retained metadata versions keep their manifests alive on purpose, so the steady state is on the order of retain_snapshots commits' worth of .avro instead of unbounded growth.

iceberg.orphan_sweep_enabled (default true) turns it off, with a startup warning when it is. It is the one deleter in the exporter whose work nothing regenerates, so it gets a switch.

What the grace does not do: it is keyed to a file's age, not to how long the file has been unreachable, so it does not widen the window for a reader that just resolved an older metadata version — that race stays bounded exactly as before, by pruneOldVersionFiles keeping retain+1 copies. An earlier draft of the release notes claimed otherwise; the deep review caught it and the claim is gone from the notes, the code comment and the design doc.

Configuration matrix

Configuration	Reaches new code?	Preconditions
OSS, iceberg.enabled=false	No — no Exporter exists (main.go:3520)	n/a
OSS, iceberg on, table not yet created	No — CreateTable branch	n/a
OSS, iceberg on, pass with adds/removes	Yes — exporter.go:583, after expiry + pruneOldVersionFiles	committed non-nil; e.backend nil-checked at :906; e.logger set in NewExporter
OSS, iceberg on, converged pass	Yes — :555	tbl non-nil from EnsureTable; writeVersionHint already derefs it two lines later
Quiet table, passes 2…N	No — fingerprint gate at scheduler.go:252-256 returns first; the first post-restart pass is the reclaim opportunity	n/a
orphan_sweep_enabled=false (non-default)	Returns at :906	verified live: 15 .avro untouched over 6 passes + startup warning
retain_snapshots 1 and 10 (default)	Yes	both tested; at 10 the rig commits fewer than 10 snapshots so nothing expires — the test says that rather than pretending to measure the bound
iceberg.warehouse = subdirectory (non-default, the #534 shape)	Yes	dirKey comes from parseVersionAndMetaDir, the helper pruneOldVersionFiles uses; matching is by basename, so no warehouse-vs-root arithmetic exists here. Tested, with a sibling wh-other/ decoy for the mid-segment match that broke #534's first fix
Metadata directly under the warehouse root (dirKey ""/".")	No — returns at :921 (#743)	same guard as pruneOldVersionFiles
Backend without ObjectLister	No — assertion fails at :911	tested
Cluster, not the active compactor	No — runPass returns at the writer gate	n/a
Cluster, active compactor	as OSS	single-writer; the sweep runs after this goroutine's own commit
Two compaction-capable nodes, no lease (main.go:3573-3578)	Yes, on both	pre-existing hazard, unchanged in kind. Each node sweeps only its own warehouse directory, so node B can compute reachability without node A's catalog pointer. I reproduced the existing form of this race by accident during testing (cross-pointed catalogs already delete each other's metadata.json), which is why that "configure only ONE compaction-capable node" warning is load-bearing
Restore writing the warehouse back	Yes, possibly mid-restore	fails closed; a warehouse whose .avro set lags its metadata.json set stays unswept until reconciled, which the code comment and release notes both say

Review

One adversarial pass on the plan and one deep pass on the implementation. The adversarial pass earned its keep: it killed the original design, which used "unreachable on two consecutive sweeps" instead of a clock on the false premise that storage.Backend has no mtime (ObjectLister.LastModified exists on all three backends) — and that scheme could never have deleted anything on a quiet table, because the fingerprint gate means such a table reaches the exporter once per process lifetime and the confirming sweep never arrives. It also argued the config key back in: #534 was about path arithmetic that coincides at the default, which a bool cannot do, and the real argument is blast radius.

The deep pass found no blockers on the mechanism (it probed the non-default warehouse and repeated aged sweeps empirically) and two real problems: the false reader-grace claim above, and that five of the nine sweep tests passed against a dead sweep. Both fixed.

Test plan

• Every one of the nine sweep tests fails with the sweep made a no-op — including the five guard tests, which now carry inverted controls (remove the condition, same orphan goes) so "the file survived" cannot pass against a dead sweep
• gofmt -l ./internal ./cmd empty, go vet clean, go build ./cmd/... ./internal/...
• go test and go test -race on ./internal/iceberg and ./internal/config
• New tests alone under -race -count=20 (one of them was order-dependent at first — snapshot ids are random, so "the first snap-*.avro" varied; fixed to pick a reachable one)
• Live single node, defaults: 15 .avro → 10 (deleted:5 failed:0 reachable:10 within_grace:0), exactly one sweep across six passes, metadata.json count and version-hint.text unchanged, every on-disk metadata version still resolving, DuckDB's iceberg_scan reading the table after
• Live, second aged run: deletes nothing — the set had converged, which is the "spared stays spared" invariant
• Live with the new key at its non-default false: fifteen files untouched over six passes plus the startup warning

Owed separately

The docs site needs iceberg.orphan_sweep_enabled in the Iceberg configuration table (docs.basekick.net, arc/integrations/apache-iceberg), plus a line about the sweep in the Limitations section. Not in this repo.