Skip to content

Check whether a failed commit landed before cleaning up its files - #4023

Open
dancsi wants to merge 3 commits into
apache:mainfrom
dancsi:fix/commit-landed-check
Open

dancsi wants to merge 3 commits into
apache:mainfrom
dancsi:fix/commit-landed-check

Conversation

@dancsi

@dancsi dancsi commented Sep 25, 2026 •

Copy link
Copy Markdown

Closes #4021
Closes #4022

Rationale for this change

When commit_table raises CommitFailedException after the catalog has already applied the commit (a lost response), Transaction.commit_transaction can delete the landed snapshot's manifest list and manifests, which leaves the table unreadable:

We ran into both bugs in Polars' sink_iceberg, which commits through pyiceberg, and work around them in pola-rs/polars#29511. The workaround wraps the catalog's commit_table, reloads the table after a failed commit, and returns a success response when a snapshot it sent is present, so pyiceberg never reaches the cleanup. This PR moves that check into commit_transaction, and Polars can drop the wrapper once it's released.

This change:

  • bases the landed check on the AddSnapshotUpdate snapshot ids that were actually sent to the catalog. Each attempt is atomic, so finding any of them proves an attempt landed;
  • runs the check after every CommitFailedException, including on the last attempt, before any cleanup;
  • raises CommitStateUnknownException (chained to the original error) if the refresh for the check fails. That exception skips the cleanup, so files a landed snapshot may reference are never deleted when the outcome can't be verified;
  • still refreshes before rebuilding a retry that sent no snapshots, so its validation sees current metadata;
  • runs the post-commit cleanup of superseded attempts' manifests once, after the loop.

Are these changes tested?

Yes, in tests/table/test_commit_retry.py, parametrized over the existing catalog fixture:

  • a lost response on the last attempt, with no retries and with retries exhausted;
  • a lost response for an overwrite of an empty table;
  • a lost response whose landed check can't refresh the table.

Each asserts that exactly one snapshot exists, that its manifest list is still on disk and that the table scans. All of them fail on main.

Are there any user-facing changes?

A lost response on the final attempt now returns successfully when the commit landed, instead of raising CommitFailedException. If the landed check itself can't reach the catalog, CommitStateUnknownException is raised instead of CommitFailedException.

Generated-by: Claude Code (Claude Opus 5.5)

dancsi and others added 3 commits September 25, 2026 12:05
A transaction whose producers staged no snapshot skipped the refresh, so the
rebuild validated against stale metadata and missed concurrent conflicts.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Run the post-commit cleanup once after the loop, flatten the landed check,
drop a redundant local import, and trim test setup and docstrings.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant