fix(cli): execute confirmed orphan cleanup plans by fallintoplace · Pull Request #1452 · apache/iceberg-go

fallintoplace · 2026-07-11T21:45:33Z

Summary

Split orphan cleanup into an explicit plan and execution phase so confirmation applies to the exact files shown to the user.

capture candidate paths, sizes, and the age cutoff in an immutable cleanup plan;
show the planned files before confirmation while keeping JSON output to one final document;
execute only the confirmed plan, without rescanning or adding newly appeared files;
return the actual deleted-file result and sizes in the final CLI output;
reject planning-only options when executing an existing plan.

The existing DeleteOrphanFiles path uses the same planning and execution flow.

Testing

go test ./table ./cmd/iceberg
coordinated plan/execute coverage proves files appearing after planning remain untouched
CLI coverage verifies preview and final deleted-file output
real modification-time coverage verifies the age filter

zeroshade

Reusing a scanned plan for the confirmed delete is a nice improvement. Two small non-blocking notes on the CLI output (inline).

zeroshade · 2026-07-12T17:12:07Z

+		OrphanFileLocations: plan.Files(),
+		TotalSizeBytes:      plan.TotalSizeBytes(),
+	}
+	cliResult := buildCleanOrphanFilesResult(tbl, result, true)


Non-blocking: this hardcodes dryRun=true for the pre-confirmation result, so a real (non-dry-run) run that finds zero orphans still reports "dry_run": true in JSON. Consider using cmd.DryRun for the final/no-op result.

zeroshade · 2026-07-12T17:12:07Z

 		return
 	}

+	output.CleanOrphanFilesResult(cliResult)


Non-blocking: with --output json, emitting this preview and then the post-deletion result later produces two top-level JSON objects on stdout for a single invocation (and an aborted confirmation leaves a success-looking dry-run object behind). Consider keeping the pre-confirmation preview out of the structured output, or emitting a single JSON result at the end.

laskoviymishka

The plan/execute split is the right shape here. Planning once and then deleting exactly that set closes the window between the confirm prompt and the delete, so files that show up after the user confirms can't get swept in — and capturing the cutoff at plan time is the correct instinct.

I'd hold this before merging though.

The blocker is the hand-built OrphanCleanupResult in the CLI: it never populates OrphanFiles, so buildCleanOrphanFilesResult intersects a nil slice and comes up empty. The result is that the text preview prints "No orphan files found." right before we prompt to delete real files, and the final summary reports 0 files deleted even on a successful cleanup (the deleteResult from ExecuteOrphanCleanup gets discarded). That defeats the "show the exact plan before confirmation" goal the PR is built around. The clean fix is a public OrphanFiles() accessor on the plan so the CLI stops reconstructing a lossy result by hand — details inline.

The other thing I'd want addressed is ExecuteOrphanCleanup accepting the full option set but silently honoring only deleteFunc/maxConcurrency/dryRun. Passing WithFilesOlderThan there does nothing, with no error — a real footgun when the plan is supposed to be immutable.

A few things I'd want before merge:

populate OrphanFiles and rebuild the CLI result from the delete outcome
narrow (or reject) the options ExecuteOrphanCleanup can't honor
a test for the text preview content, and a real-mtime case so the age filter is actually exercised
drop or document Cutoff(), which nothing enforces at execution

Once those are in, happy to take another pass and approve. I've stayed off the CLI output details since zeroshade's already looking there.

laskoviymishka · 2026-07-14T15:55:09Z

 		os.Exit(1)
 	}

+	result := table.OrphanCleanupResult{


This hand-built result never populates OrphanFiles, and that quietly breaks both the preview and the final output.

buildCleanOrphanFilesResult with dryRun=false intersects result.OrphanFiles against DeletedFiles — but OrphanFiles is nil here, so the intersection is empty. The text preview prints "No orphan files found." right before we prompt to delete real files, and the final output reuses this same cliResult (deleteResult from ExecuteOrphanCleanup is discarded), so the tool reports OrphanFileCount: 0 even after it deletes the full set.

The plan already carries the per-file entries privately. I'd add a public accessor — func (p OrphanCleanupPlan) OrphanFiles() []OrphanFile { return slices.Clone(p.orphanFiles) } — populate result.OrphanFiles from it for the preview, and rebuild cliResult from deleteResult after ExecuteOrphanCleanup before the final output. wdyt?

laskoviymishka · 2026-07-14T15:55:09Z

 	}

-	if len(result.OrphanFileLocations) == 0 {
+	if len(plan.Files()) == 0 {


plan.Files() clones the slice on every call, and we hit it three times in this function (this guard, the prompt, the result build). Minor, but I'd pull it into a local once — or add a Len() int on the plan for the count-only checks.

laskoviymishka · 2026-07-14T15:55:09Z


+func shouldPrintCleanOrphanPreview(output Output) bool {
+	switch output.(type) {
+	case jsonOutput, *jsonOutput:


jsonOutput is only ever constructed as a value, so the *jsonOutput arm is dead outside the test — I'd drop it. Separately, isMachineReadable reads a touch clearer than shouldPrintCleanOrphanPreview, since that's really the property we're keying on.

laskoviymishka · 2026-07-14T15:55:09Z

 	assert.Equal(t, int64(2048), result.OrphanFiles[0].SizeBytes)
 }

+func TestShouldPrintCleanOrphanPreview(t *testing.T) {


This covers the predicate, but the behavior the PR is actually selling — the text preview listing the exact files before we prompt — has no test. An end-to-end runCleanOrphanFiles case that captures text output and asserts the preview contains the orphan paths would have caught the empty-preview bug I flagged in clean_orphan_files.go; right now nothing does. Could we add one?

laskoviymishka · 2026-07-14T15:55:09Z

+}
+
+// Cutoff returns the age cutoff used to create the cleanup plan.
+func (p OrphanCleanupPlan) Cutoff() time.Time {


Cutoff() is only read by the test — no execution path uses it, and ExecuteOrphanCleanup doesn't re-check age against it. As a public accessor it reads like an execute-time guard that doesn't exist. I'd either drop it, or godoc it as informational: the plan-time filter, not enforced at execution.

laskoviymishka · 2026-07-14T15:55:09Z

+
+// ExecuteOrphanCleanup deletes exactly the files in plan. It does not perform
+// another orphan scan, so files appearing after planning are not included.
+func (t Table) ExecuteOrphanCleanup(ctx context.Context, plan OrphanCleanupPlan, opts ...OrphanCleanupOption) (OrphanCleanupResult, error) {


executeOrphanCleanup only reads deleteFunc and maxConcurrency off cfg, but ExecuteOrphanCleanup accepts the full OrphanCleanupOption set — so WithFilesOlderThan, WithLocation, WithPrefixMismatchMode etc. are silently dropped here. Someone calling ExecuteOrphanCleanup(ctx, plan, WithFilesOlderThan(24*time.Hour)) gets no error and no effect, which is a nasty footgun given the plan's whole point is that its set is already fixed.

I'd narrow this to a smaller option type (just WithDeleteFunc/WithMaxConcurrency/WithDryRun), so the scan-time options aren't even expressible on the execute path. wdyt?

laskoviymishka · 2026-07-14T15:55:09Z

+		nil,
+	)
+
+	plan, err := tbl.PlanOrphanFiles(ctx, WithFilesOlderThan(time.Hour))


The don't-expand-after-planning behavior this asserts is good and worth having. But WithFilesOlderThan(time.Hour) isn't exercising the age filter — MemFS reports a zero ModTime, so every file is "older than" any cutoff and the option is a no-op here. If we want real coverage of the age boundary, a separate case with actual mtimes that asserts a recent file gets excluded would do it.

I'd also assert result.OrphanFiles here, not just DeletedFiles — the path/size pairs are exactly what broke in the CLI.

zeroshade

(Medium) The pre-confirmation preview for a non-dry-run cleanup renders zero files.

In runCleanOrphanFiles (cmd/iceberg/clean_orphan_files.go:61), cliResult is built with dryRun = cmd.DryRun (i.e. false for a real run) from the plan result. The "exact plan before confirmation" preview (clean_orphan_files.go:77-79) then reuses that result, but buildCleanOrphanFilesResult with dryRun=false filters entries by result.DeletedFiles (:118-127) — which is empty before deletion has happened. So the preview always shows 0 orphan files even though planFiles is non-empty (guaranteed by the early return at :69).

Fix: build the pre-confirmation preview with dryRun=true semantics (from result.OrphanFiles), and keep the dryRun=false/DeletedFiles build only for the post-deletion output at :94.

zeroshade

Re-reviewed — my earlier concern is resolved. The pre-confirmation preview now builds with dry-run semantics (the plan's orphan set) instead of the empty post-deletion set, so it shows the real count, and JSON output emits a single document. Thanks @fallintoplace!

zeroshade · 2026-07-17T15:43:54Z

@laskoviymishka mind giving this another look when you have time? The plan/execute split is in place, and the pre-confirmation preview now uses dry-run semantics (it shows the real planned orphan set, and JSON output emits a single document), so the concern I'd raised is resolved. I've approved on my side — a review update from you would unblock the merge. Thanks!

fallintoplace requested a review from zeroshade as a code owner July 11, 2026 21:45

fallintoplace changed the title ~~fix(cli): execute only confirmed orphan cleanup plan~~ fix: execute only confirmed orphan cleanup plan Jul 11, 2026

zeroshade reviewed Jul 12, 2026

View reviewed changes

laskoviymishka requested changes Jul 14, 2026

View reviewed changes

fallintoplace changed the title ~~fix: execute only confirmed orphan cleanup plan~~ fix(cli): execute confirmed orphan cleanup plans Jul 14, 2026

zeroshade requested changes Jul 15, 2026

View reviewed changes

fallintoplace force-pushed the fix/orphan-cleanup-confirmed-plan branch from e905660 to 3bba143 Compare July 16, 2026 23:52

zeroshade approved these changes Jul 17, 2026

View reviewed changes

fix(cli): execute confirmed orphan cleanup plans

c68c90e

fallintoplace force-pushed the fix/orphan-cleanup-confirmed-plan branch from 3bba143 to c68c90e Compare July 17, 2026 23:20

Uh oh!

Conversation

fallintoplace commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

zeroshade commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fallintoplace commented Jul 11, 2026 •

edited

Loading