Skip to content

Out-of-band iceberg export compaction (icebird 0.8.10) + fix broken export smoke fixtures - #96

Merged
philcunliffe merged 4 commits into
masterfrom
iceberg-export-compaction-and-smoke-fixes
Jun 12, 2026
Merged

Out-of-band iceberg export compaction (icebird 0.8.10) + fix broken export smoke fixtures#96
philcunliffe merged 4 commits into
masterfrom
iceberg-export-compaction-and-smoke-fixes

Conversation

@philcunliffe

Copy link
Copy Markdown
Contributor

What

Two non-graph pieces carved out of the context-graph work, bundled because the compaction smoke can't run without the fixture fix:

1. Export-table compaction, out-of-band per LLP 0022

  • icebird 0.8.9 shipped icebergRewrite; 0.8.10 makes it safe for our formatVersion-3 tables (row lineage preserved across rewrites). Pin moves 0.8.9 -> 0.8.10.
  • The compactionSupported: false stub in format-iceberg/src/maintenance.js becomes a real compactExportTable, but it honors LLP 0022 §Compaction's constraint: rewrites are out-of-band only. hyp sink maintain --compact is the single path that runs one — default maintain, the daemon loop, and the sink tick never compact (the in-daemon read-rewrite memory landmine stays fenced off).
  • Gated on a compact_file_count threshold (default 32, configurable via the sink's maintenance config). Single commit attempt on conflict: a rewrite only saw the rows it read, so a blind retry could drop concurrently appended rows; the next manual run starts from fresh metadata.
  • LLP 0022 updated in the same change.

2. Fix the two broken iceberg export smokes

iceberg_export_local_fs and iceberg_export_s3_fixture were failing on master before this change (export: bytesWritten > 0, value=0): their inline fixture plugins pointed partition discovery at the legacy datasets/<ds>/all path, but the storage spool routes flushed rows to datasets/<ds>/source=<client>/table/ — so exportBatch exported an empty partition and nothing ever landed in the destination. The fixtures now discover via discoverCachePartitions, and the flows force-settle the spool (flushAll({ force: true })) instead of racing the fire-and-forget size-threshold flush. (iceberg_export_partitioned_local_fs is unaffected — it deliberately writes past the spool.)

Testing

  • npm test: 834/834 passing, including new test/plugins/iceberg-maintenance.test.js (config normalization, threshold gating, dry-run, and a real v3 rewrite consolidating two data files into one with format-version preserved)
  • npm run typecheck / npm run lint clean
  • Smokes: iceberg_export_local_fs (now also exercises a real icebergRewrite via compact_file_count: 2 and asserts the default run does not compact), iceberg_export_s3_fixture, iceberg_export_partitioned_local_fs — all ok

🤖 Generated with Claude Code

…smoke fixtures
Two non-graph pieces, bundled because the compaction smoke needs the
fixture fix to run at all:
1. Export-table compaction (LLP 0022#compaction). icebird 0.8.9 shipped
icebergRewrite and 0.8.10 made it safe for formatVersion-3 tables
(row lineage preserved across rewrites), so the
"compactionSupported: false" stub in format-iceberg maintenance
becomes a real rewrite — but strictly out-of-band per LLP 0022:
`hyp sink maintain --compact` is the only path that runs it; the
default maintain, the daemon loop, and the sink tick never compact.
Gated on a `compact_file_count` threshold (default 32), single
commit attempt on conflict (a blind retry could drop concurrently
appended rows). Pin bumped 0.8.9 -> 0.8.10. LLP 0022 updated in the
same change.
2. Fix the iceberg export smoke fixtures. Both iceberg_export_local_fs
and iceberg_export_s3_fixture were failing before this change: their
inline fixture plugins pointed partition discovery at the legacy
`datasets/<ds>/all` path, but the storage spool routes flushed rows
to `datasets/<ds>/source=<client>/table/`, so exportBatch exported an
empty partition (bytesWritten=0). The fixtures now discover via
discoverCachePartitions, and the flows force-settle the spool
(flushAll) instead of racing the fire-and-forget size-threshold
flush. iceberg_export_partitioned_local_fs is unaffected (it
deliberately writes past the spool).
New unit coverage in test/plugins/iceberg-maintenance.test.js: config
normalization, threshold gating, dry-run, and a real v3 rewrite
consolidating two data files into one.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@philcunliffe

philcunliffe commented Jun 11, 2026

Copy link
Copy Markdown
ContributorAuthor

Dual-agent review — request_changes

  • Verdict:request_changes
  • Risk class:medium
  • Auto-merge advisory: 👎 thumbs down — verdict is request_changes; needs human-gated follow-up

Advisory only: no merge was attempted.

Risk capstone

Cross-reference: reviewer findings vs high-risk surfaces

SourceFinding (severity, evidence)Intersects
CodexError Handling & Resilience: CLI reports any rewrite failure as below-threshold skip (major, maintenance.js:172-176, core_commands.js:2341-2342)Unobservable failure path; Concurrency surface (conflict path)
ClaudeFailed rewrite misreported as "compaction_skipped (below compact_file_count)" (major, maintenance.js:172-194, core_commands.js:2342)Unobservable failure path
ClaudeConflict silently leaks orphaned table copy, no reclamation pass (major, maintenance.js:92-94)Conflict-orphan leak; Lifecycle gap
ClaudeRewrite re-introduces unbounded-memory compaction class from #82/#90 (major, maintenance.js:90)Unbounded rewrite memory
ClaudeRewrite data preservation never precisely asserted (major, iceberg-maintenance.test.js:77-83, iceberg_export_local_fs.js:320-326)Coverage gap
ClaudeLLP 0022 References section stale pointer (minor, llp/0022:278)(doc hygiene only — no risk surface)
Codex review

Fix Validations

Broken export smoke fixtures

  • Status: correct
  • Evidence:hypaware-core/smoke/flows/iceberg_export_local_fs.js:269, hypaware-core/smoke/flows/iceberg_export_local_fs.js:464, hypaware-core/smoke/flows/iceberg_export_s3_fixture.js:269, hypaware-core/smoke/flows/iceberg_export_s3_fixture.js:424, src/core/cache/partition.js:161
  • Assessment: The fixtures now force-flush the spool before discovery and use the existing partition discovery helper instead of hard-coding datasets/<ds>/all. Existing storage handling supports this because discovered p.path is a logical table path, and storage resolves source-table layouts internally.

Out-of-band compaction fence

  • Status: correct
  • Evidence:hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:217, hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:248, src/core/cli/core_commands.js:2273, src/core/cli/core_commands.js:2330
  • Assessment:maintainExportTables defaults compact to false and only calls compactExportTable when the CLI passes --compact. The traced production caller is hyp sink maintain; the smoke calls are explicit.

Findings

5) Error Handling & Resilience

  • Severity: major
  • Confidence: high
  • Evidence:hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:172, hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:176, src/core/cli/core_commands.js:2341, src/core/cli/core_commands.js:2342
  • Why it matters: Any rewrite failure, including the intentional concurrent-commit conflict path or an IO/auth error, is reported by the CLI as compaction_skipped (below compact_file_count), which gives operators the wrong remediation signal.
  • Suggested fix: Return a skip/failure reason from compactExportTable such as below_threshold, commit_conflict, or rewrite_error; only print below compact_file_count when the threshold check actually failed, and consider a nonzero exit for unexpected rewrite errors.

No Finding

    1. Behavioral Correctness
    1. Contract & Interface Fidelity
    1. Change Impact / Blast Radius
    1. Concurrency, Ordering & State Safety
    1. Security Surface
    1. Resource Lifecycle & Cleanup
    1. Release Safety
    1. Test Evidence Quality
    1. Architectural Consistency
    1. Debuggability & Operability

Evidence Bundle

  • Changed hot paths:compactExportTable, maintainExportTables, hyp sink maintain --compact, iceberg export smoke fixture discovery, icebird pin to 0.8.10.
  • Impacted callers:src/core/cli/core_commands.js:2330, hypaware-core/smoke/flows/iceberg_export_local_fs.js:271, hypaware-core/smoke/flows/iceberg_export_local_fs.js:295, test/plugins/iceberg-maintenance.test.js:39.
  • Impacted tests:test/plugins/iceberg-maintenance.test.js:22, test/plugins/iceberg-maintenance.test.js:49, hypaware-core/smoke/flows/iceberg_export_local_fs.js:295, hypaware-core/smoke/flows/iceberg_export_s3_fixture.js:269.
  • Unresolved uncertainty: I did not run the suite; this is a static diff review. No explicit test covers the rewrite failure/conflict reporting path.
Claude review

Claude review

Failed rewrite misreported as "compaction_skipped (below compact_file_count)"

  • Severity: major
  • Confidence: 85
  • Evidence: hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:172-194 and src/core/cli/core_commands.js:2342
  • Why it matters:compactExportTable swallows every rewrite failure (including the concurrent-commit conflict the PR explicitly designs for, and e.g. S3 permission errors) into the same compacted: false shape as the below-threshold skip, so runSinkMaintain prints the affirmatively false reason compaction_skipped (below compact_file_count) with exit code 0 — the one manual compaction tool misdiagnoses its own failures, contrary to CLAUDE.md's log-driven-development requirement that failures identify the broken step. (Flagged independently by 4 of 5 review agents.)
  • Suggested fix: Add a discriminant to the compactExportTable result (e.g. reason: 'below-threshold' | 'conflict' | 'error' plus error_kind), propagate it through ExportMaintenanceDatasetReport, print the real reason in the per-dataset CLI line, and emit a structured log/span around the rewrite per LLP 0022's Observability section.

Rewrite commit conflict silently leaks a full table copy of orphaned data files, with no reclamation pass

  • Severity: major
  • Confidence: 85
  • Evidence: hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:92-94 (bare catchcompacted: false)
  • Why it matters: icebird's icebergStageRewrite writes all consolidated data files and the manifest to the blob store before the single-attempt commit, and its own docs say orphan cleanup "is left to a separate maintenance pass" — which maintainExportTables does not have (icebergExpireSnapshots only deletes files referenced by expired snapshots); since the design point of out-of-band compaction is that the daemon keeps appending concurrently, the deliberately no-retry rewrite is expected to lose races, and each lost race leaves a full rewritten copy of the table orphaned in S3 forever — the same leak class commit 085af71 (fix: make cache compaction memory-safe and reclaim orphaned generations #82) documented (9.5 GB) and fixed for the local cache.
  • Suggested fix: On commit failure, best-effort delete the staged writtenFiles, or add an orphan-file sweep to export maintenance mirroring fix: make cache compaction memory-safe and reclaim orphaned generations #82's orphan-generation cleanup; at minimum surface the conflict so the operator knows a leak occurred.

Out-of-band rewrite re-introduces the unbounded-memory compaction class fixed in #82/#90

  • Severity: major
  • Confidence: 85
  • Evidence: hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:90 (the icebergRewrite call); node_modules/icebird/src/write/rewrite.js:91-92
  • Why it matters: icebird's rewrite materializes the entire table's live rows in memory (await icebergRead(...) then full-array sort and per-column copies) with no targetFileRows and no byte gate — the exact failure mode fix: make cache compaction memory-safe and reclaim orphaned generations #82 fixed for local-cache compaction with a 32 MB compact_batch_bytes budget and Stop parquet exports bloating & OOMing on wide repeated columns #90 fixed for parquet exports; the 32-file default threshold fires precisely on large long-lived tables (the historically documented ai_gateway export was ~1.3 GB), so the first production hyp sink maintain --compact plausibly OOM-crashes under a default heap — running out-of-band protects the daemon, not the rewrite.
  • Suggested fix: Read total-files-size from the current snapshot summary (already available next to total-data-files) and skip-with-warning above a configurable byte cap, pass targetFileRows; longer-term request a streamed/per-partition rewrite from icebird mirroring compact_batch_bytes.

Rewrite data preservation is never precisely asserted

  • Severity: major
  • Confidence: 85
  • Evidence: test/plugins/iceberg-maintenance.test.js:77-83 and hypaware-core/smoke/flows/iceberg_export_local_fs.js:320-326
  • Why it matters: The PR's own JSDoc and LLP 0022 name row loss as the rewrite's central risk, yet the unit test asserts only file count and format-version after compaction (never reads rows back), and the smoke's post-compaction check is rows.length >= ROW_COUNT (>= 5) against a table holding 10 rows — a rewrite that silently dropped half the data would pass every tier.
  • Suggested fix: In the unit test, icebergRead after the successful compactExportTable and assert the appended records survive (sorted deep-equal on id/value); tighten the smoke's post-compaction assertion to the exact expected row count.

LLP 0022 References section still points at stale maintenance.js:120

  • Severity: minor
  • Confidence: 82
  • Evidence: llp/0022-iceberg-export-partitioning.spec.md:278
  • Why it matters: The PR fixed the identical stale maintenance.js:120 pointer in the Compaction section body but missed the References list, which still cites "compaction framing" at a line that is now the tail of discoverExportDatasets; CLAUDE.md requires LLP docs not to carry stale guidance.
  • Suggested fix: Update the References entry to drop the line number (matching the body's fix) or point it at compactExportTable / maintainExportTables.

Reports: /Users/phil/workspace/hypaware/.git/worktrees/dual-review-pr-96/dual-review/pr-96

…eanup, byte cap, row-preservation tests
- compactExportTable now discriminates every non-compaction outcome
(below-threshold / above-byte-cap / no-table / conflict / error) instead
of folding rewrite failures into the threshold skip; hyp sink maintain
prints the real reason per dataset and exits nonzero on rewrite errors.
- Stage + commit explicitly (icebergStageRewrite + fileCatalogCommit, the
same single-attempt sequence icebergRewrite performs) so a failed commit
deletes the staged writtenFiles best-effort instead of orphaning a full
rewritten table copy in the blob store on every lost race (#82's leak
class).
- New compact_max_bytes guard (default 128 MB, configurable via the sink's
maintenance config): skip the rewrite when the current snapshot's
total-files-size exceeds the cap, since icebird's rewrite materializes
all live rows in memory (#82/#90's OOM class).
- Tests now assert exact row preservation: unit test reads rows back after
the rewrite and deep-equals them; the local-fs smoke asserts the exact
post-compaction row count and per-id multiplicity. New unit coverage for
the byte cap, the conflict path (412 on the conditional metadata write,
staged-file cleanup verified on disk), and the error path.
- LLP 0022 Compaction section updated in the same change; stale
maintenance.js:120 pointer in References fixed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Dual-agent review — request_changes

  • Verdict:request_changes
  • Risk class:medium
  • Auto-merge advisory: 👎 thumbs down — verdict is request_changes; needs human-gated follow-up

Advisory only: no merge was attempted.

Risk capstone

Cross-reference: reviewer findings vs high-risk surfaces

SourceFinding (severity, evidence)Intersects
ClaudeThird smoke flow still broken (major, iceberg_export_s3_roundtrip.js:301)Risks bullet 4; Direct callers (fixture discovery)
ClaudeCLI --compact zero test coverage (major, core_commands.js:2270-2390)Risks bullet 3; Direct callers (runSinkMaintain)
ClaudeCommit-failure cleanup can delete landed-commit files (major, maintenance.js:231-241)Risks bullet 2; Concurrency surface (failure window)
ClaudeLLP 0022 self-contradiction (minor, llp/0022:134,256-267)Targets (LLP 0022)
Codex5) Error Handling: load failures reported as no-table, exit 0 (major, maintenance.js:177-180)Concurrency surface (failure window); Targets (compactExportTable)
Codex7) Resource Lifecycle: stage-phase failure leaks written files (major, maintenance.js:209-212)Concurrency surface (crash/stage orphans)
Codex review

Fix Validations

Broken export smoke fixtures used the legacy partition path

  • Status: correct
  • Evidence: hypaware-core/smoke/flows/iceberg_export_local_fs.js:191, hypaware-core/smoke/flows/iceberg_export_local_fs.js:483, hypaware-core/smoke/flows/iceberg_export_s3_fixture.js:272, hypaware-core/smoke/flows/iceberg_export_s3_fixture.js:421, hypaware-core/plugins-workspace/format-iceberg/src/table-format.js:223
  • Assessment: The fixtures now force-settle the storage spool before discovery and use discoverCachePartitions, so they hand the exporter the real routed source=.../table path. Existing export code already flushed the partitions it was handed, but it could not fix a fixture that discovered the wrong path.

Iceberg compaction stays out-of-band

  • Status: correct
  • Evidence: hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:321, hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:323, hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:352, src/core/cli/core_commands.js:2270, src/core/cli/core_commands.js:2278, src/core/cli/core_commands.js:2332
  • Assessment:compact defaults false and only the CLI --compact flag passes it through. The default maintenance path reports no compaction and the smoke asserts that boundary.

Single-attempt commit conflict handling

  • Status: incomplete
  • Evidence: hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:220, hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:228, hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:231, hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:234
  • Assessment: The commit itself is single-attempt and conflict is classified without retry, which matches LLP 0022. Cleanup is only covered after icebergStageRewrite returns a staged update; see the resource lifecycle finding below.

Findings

5) Error Handling & Resilience

  • Severity: major
  • Confidence: medium
  • Evidence: hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:177, hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:180, src/core/cli/core_commands.js:2347, src/core/cli/core_commands.js:2384, src/core/cli/core_commands.js:2367
  • Why it matters:--compact reports any metadata load failure as no-table, prints a skip, and exits 0, so auth errors, corrupt metadata, or transient read failures can be mistaken for an idle/missing table.
  • Suggested fix: Preserve the caught error; only return reason: 'no-table' for a known not-found/missing-metadata case, and return reason: 'error' with the message for other load failures so the CLI exits nonzero.

7) Resource Lifecycle & Cleanup

  • Severity: major
  • Confidence: high
  • Evidence: hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:209, hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:212, node_modules/icebird/src/write/rewrite.js:134, node_modules/icebird/src/write/rewrite.js:159, node_modules/icebird/src/write/snapshot.js:73
  • Why it matters: If staging fails after writing any data, manifest, or manifest-list file, compactExportTable returns reason: 'error' without deleting those files because cleanup only runs after a staged update exists.
  • Suggested fix: Wrap the resolver during icebergStageRewrite to track successfully finished writes and delete them in the stage-error catch; add a regression test that injects a failure after the first staged write.

No Finding

  1. Behavioral Correctness; 2) Contract & Interface Fidelity; 3) Change Impact / Blast Radius; 4) Concurrency, Ordering & State Safety; 6) Security Surface; 8) Release Safety; 9) Test Evidence Quality; 10) Architectural Consistency; 11) Debuggability & Operability.

Evidence Bundle

  • Changed hot paths:compactExportTable; maintainExportTables; hyp sink maintain --compact; smoke fixture partition discovery; icebird 0.8.10 rewrite path.
  • Impacted callers: src/core/cli/core_commands.js:2332; hypaware-core/smoke/flows/iceberg_export_local_fs.js:271; hypaware-core/smoke/flows/iceberg_export_local_fs.js:295.
  • Impacted tests: test/plugins/iceberg-maintenance.test.js:98; test/plugins/iceberg-maintenance.test.js:143; test/plugins/iceberg-maintenance.test.js:190; hypaware-core/smoke/flows/iceberg_export_local_fs.js:286; hypaware-core/smoke/flows/iceberg_export_local_fs.js:302.
  • Unresolved uncertainty: I did not run the test suite; review is based on the supplied diff plus targeted caller/API tracing.
Claude review

Claude review

Third smoke flow (iceberg_export_s3_roundtrip) still carries the broken fixture pattern this PR diagnoses

  • Severity: major
  • Confidence: 90
  • Evidence: hypaware-core/smoke/flows/iceberg_export_s3_roundtrip.js:301 (static datasets/<ds>/all discovery), :335 (appendRows with no flushAll)
  • Why it matters: The PR's own root cause ("spool routes flushed rows to datasets/<ds>/source=<client>/table/, so the legacy all path exports an empty partition and bytesWritten=0") applies byte-for-byte to this third, env-gated (HYP_SMOKE_REAL_S3=1) flow, so the next real-S3 acceptance run fails with the exact symptom this PR claims to fix.
  • Suggested fix: Apply the same two-part fix: switch the inline fixture's discoverPartitions to discoverCachePartitions(cacheDir, { datasets: [DATASET] }) and add await kernel.storage.flushAll({ force: true }) before discovery.

CLI --compact wiring has zero test coverage at any tier

  • Severity: major
  • Confidence: 85
  • Evidence: src/core/cli/core_commands.js:2270-2390; no hits for sink maintain/runSinkMaintain under test/; smokes call maintainExportTables directly
  • Why it matters:hyp sink maintain --compact is the only sanctioned rewrite path and LLP 0022 promises "unexpected rewrite errors exit nonzero", yet flag parsing, the config plumb-through, skip-reason rendering, and the exit-code contract (error→1, conflict→0) are all unverified, so a regression in the single manual entry point ships silently.
  • Suggested fix: Add a dispatch-style test (pattern in test/core/command-dispatch.test.js) covering: --compact compacts and exits 0, default maintain does not rewrite, reason: 'error' exits 1, conflict exits 0.

Commit-failure cleanup can delete data files of a commit that actually landed

  • Severity: major
  • Confidence: 80
  • Evidence: hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:231-241 (catch around fileCatalogCommit deletes staged.writtenFiles on any error before classifying)
  • Why it matters: Two ambiguous-success error shapes — a timeout after the conditional metadata PUT durably landed, and the S3 conditional-write hazard where an SDK retry of its own successful PUT surfaces a 412 — leave v<N+1>.metadata.json committed and referencing the staged files this code then deletes, silently corrupting the export table; icebird's own icebergRewrite deliberately defers cleanup to a maintenance pass for this reason (node_modules/icebird/src/write/write.js:531-535).
  • Suggested fix: Before deleting, re-load latest metadata: if the current snapshot is the staged rewrite, report compacted: true; only delete when a different writer's commit is confirmed current; on an unverifiable outcome leave the bounded orphans and report error.

LLP 0022 edits leave the doc contradicting itself

  • Severity: minor
  • Confidence: 90
  • Evidence: llp/0022-iceberg-export-partitioning.spec.md:134 ("not run in V1") and :256-267 ("npm tops out at 0.8.5 pinned / 0.8.8 latest", "pin moves from 0.8.5") vs the PR-rewritten §Compaction (:239, "pin sits at 0.8.10", tool now exists)
  • Why it matters: CLAUDE.md mandates living docs with no stale guidance; after this PR the spec tells two different stories about whether compaction runs and what icebird version is pinned.
  • Suggested fix: Update line 134 to point at the now-existing hyp sink maintain --compact path and refresh the §icebird dependency paragraph to the published 0.8.10 pin (or mark it historical).

Reports: /Users/phil/workspace/hypaware/.git/worktrees/smoke-fixture-fix/dual-review/pr-96

…stage tracking, load-error reporting, third smoke fixture, CLI tests
- compactExportTable now re-loads metadata after a failed commit before any
cleanup: a landed-but-thrown commit (timeout after the PUT applied, SDK
retry of its own conditional write surfacing 412) reports success instead
of deleting the data files the live snapshot references. Staged files are
only reclaimed on a confirmed-lost conflict; unverifiable outcomes leave
the bounded orphans and say so in the error.
- A tracking resolver records every stage-phase write that finished, so a
rewrite that dies between its first data file and the manifest list
reclaims the partial output (icebird only reports writtenFiles on a
completed StagedUpdate).
- Metadata load failures are no longer folded into the no-table skip:
'no-table' is reserved for a verifiably absent table (ENOENT / empty
listing); auth/IO/corrupt-metadata failures report reason 'error' and
exit the CLI nonzero.
- iceberg_export_s3_roundtrip gets the same two-part fixture fix as the
other flows: flushAll before discovery + discoverCachePartitions instead
of the legacy static datasets/<ds>/all path.
- New test/core/sink-maintain-command.test.js covers the hyp sink maintain
CLI wiring: default maintain never rewrites, --compact rewrites and
exits 0, conflict exits 0, rewrite error exits 1, unknown flag exits 2.
- LLP 0022: line 134 no longer claims compaction is "not run in V1"; the
icebird dependency section reflects the published 0.8.10 pin with the
pre-publication story marked historical; Compaction section documents
verify-then-delete and the no-table vs error split.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

🧭 Decision map — where to spend your attention

Companion to the dual-review verdict. This casts no verdict — it points at the 6 forks where the author made a real choice, so you can skim the rest.

Scanned: 38 hunks across 10 files. Most is mechanical: ~560 lines of new tests, three near-identical smoke-fixture fixes, report-field plumbing through types.d.ts/CLI output, and LLP 0022 prose. The decisions worth your eyes, in order:

1. Shared-engine pin bump: icebird 0.8.90.8.10 · magic value / dependency

package.json:41

"icebird": "0.8.10",
  • Decision: bump the shared engine because 0.8.10 preserves v3 row lineage across rewrites — per LLP 0022, 0.8.9's rewrite was not safe for the formatVersion: 3 export tables this PR starts rewriting.
  • Alternative not taken: keep 0.8.9 and hold compaction until the lineage fix ships, or gate the rewrite on a runtime icebird-version check.
  • Check: the "never regress below 0.8.10 while compaction exists" constraint lives only in LLP prose — nothing in code enforces it. And the cache (retention, compaction, store) rides the same engine: a regression there would not be attributable to any changed line in this diff. One character of diff, the widest blast radius in the PR.

2. Verify-then-delete commit cleanup; unverifiable outcome leaves orphans · unhappy-path policy

maintenance.js:271-318

if(outcome==='lost'&&isCommitConflict(err)){awaitdeleteFilesBestEffort(resolver,staged.writtenFiles)
  • Decision: on a failed commit, re-load metadata before any cleanup; delete staged files only when the loss is confirmed and the error is a 412/409 conflict; any other shape (network error, reload failure, even reload-confirmed-absent with a non-conflict error) leaves the staged files in place and reports error.
  • Alternative not taken: delete-on-any-commit-error — simpler, no orphans, but an ambiguous success (timeout after a durable PUT, SDK self-retry surfacing 412) would delete data files a landed snapshot references, corrupting the table. An earlier revision of this PR did exactly that.
  • Check: the deliberate residue — each unverifiable failure strands ≤128 MB of staged files with no sweep, accumulating across runs — and the landed probe, which matches the staged snapshot-id against the reloaded snapshot list; confirm an in-flight commit landing after the reload can't be misread as lost on the conflict branch.

3. compact_file_count: 32 and compact_max_bytes: 128 MB · magic value

maintenance.js:21-34

compact_file_count: 32,compact_max_bytes: 128*1024*1024,
  • Decision: trigger a rewrite at 32 live data files; refuse it once total-files-size exceeds 128 MB, sized off "compressed parquet expands ~10x into JS objects ≈ 1.3 GB heap" (icebird's rewrite materializes every live row in memory).
  • Alternative not taken: no byte cap at all (trust the operator's heap — the prior repo stance for manual exports), or deriving the cap from the actual configured heap at runtime.
  • Check: the ~10x expansion factor is a heuristic carried over from the parquet-OOM incidents (fix: make cache compaction memory-safe and reclaim orphaned generations #82, Stop parquet exports bloating & OOMing on wide repeated columns #90); sanity-check it against real export row shapes. Note the interplay: a table that grows past 128 MB before anyone runs --compact becomes permanently uncompactable at defaults.

4. Lost race exits 0; only reason: 'error' exits 1 · contract

core_commands.js:2347-2366 · pinned by sink-maintain-command.test.js:216

if(d.compactionReason==='error')rewriteErrors+=1
  • Decision: a concurrent-commit conflict is an expected outcome — exit 0, no retry, "re-run to retry from fresh metadata". Only error (including the unverified-commit case, which may have actually succeeded) exits 1.
  • Alternative not taken: auto-retry from fresh metadata (rejected: a blind retry could drop rows another writer appended), or exiting nonzero on conflict so automation notices nothing was compacted.
  • Check: a scripted nightly --compact that loses the race every night reports success forever; decide whether silent exit-0 is acceptable for the only sanctioned rewrite entry point.

5. no-table vs error classified by matching icebird's message string · unhappy-path policy

maintenance.js:350-355

if(record.code==='ENOENT'||record.code==='NoSuchKey')returntruereturnerrinstanceofError&&err.message.includes('no metadata files found')
  • Decision: distinguish "table verifiably absent" (exit 0) from "load failed" (exit 1) via error codes plus a literal match on icebird's 'no metadata files found' message.
  • Alternative not taken: a structured error kind upstreamed to icebird (a first-party, exactly-pinned dep), or an explicit empty-listing probe before the load.
  • Check: the exact pin makes drift bump-time-only, but the failure is silent and inverted: if a future icebird rewords the message, every genuinely missing table starts reporting error and failing the CLI run.

6. Deep-import icebird internals and re-implement icebergRewrite's sequence · dependency / contract

maintenance.js:8-9

import{fileCatalogCommit}from'icebird/src/write/commit.js'import{icebergStageRewrite}from'icebird/src/write/rewrite.js'
  • Decision: bypass the public icebergRewrite and hand-roll its load → stage → single-attempt-commit sequence, to hold the StagedUpdate so a failed commit can reclaim writtenFiles.
  • Alternative not taken: call public icebergRewrite and accept a full rewritten table copy orphaned in the blob store on every lost race (and lost races are expected — the daemon appends concurrently).
  • Check: the duplicated sequence can silently drift from upstream's (a future icebird could add a commit step this copy lacks), and src/write/* paths sit outside semver protection even with the exact pin.

Honorable mentions (real but lower-stakes): types.d.ts:137compactionSupported literal flips falsetrue, breaking any consumer that asserted false (the local_fs smoke's deleted assertion was the only one found); maintenance.js:232-244 — stage-phase cleanup via a resolver wrapper that intercepts writer.finish, assuming one finish per write; smoke fixtures now codegen an absolute pathToFileURL import of src/core/cache/partition.js into the generated plugin source, coupling fixtures to repo layout; compactExportTable reports compacted: true under dryRun without rewriting.

Generated by /decision-map. Advisory — directs attention, casts no verdict.

…memory BlobStore mock
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@philcunliffe
philcunliffe merged commit dade52a into masterJun 12, 2026
6 checks passed
@philcunliffe
philcunliffe deleted the iceberg-export-compaction-and-smoke-fixes branch June 12, 2026 19:02
Sign up for freeto 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

@philcunliffe