apache / apache/polaris

Batch file cleanup reports success and drops tasks after deletion failures

Open
#5,420 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
2.1k
Forks
522
Avg merge
1d 22h
Merged PRs (30d)
137

Description

### Describe the bug

Batch metadata cleanup can report success and delete its persisted task even when a file deletion fails. This leaves the file orphaned and removes the task record that identifies the unfinished work.

The iterable `FileCleanupTaskHandler.tryDelete(...)` overload calls Iceberg's `CatalogUtil.deleteFiles(...)` inside a `CompletableFuture`. That Iceberg helper logs and suppresses deletion exceptions: its non-bulk path uses `suppressFailureWhenFinished()`, and its bulk path catches deletion failures. Consequently, Polaris's exceptional-completion retry branch never sees those failures. `BatchFileCleanupTaskHandler.handleTask(...)` returns normally, and `TaskExecutorImpl` sets `TASK_SUCCESS=true` and drops the task.

This affects the files assigned to batch cleanup after `DROP TABLE PURGE`: previous metadata files, manifest lists, statistics files, and partition statistics files. It does not require a large table or a process crash; a storage delete error after a successful existence check is sufficient.

### To Reproduce

On current `main`, add the following focused test to `runtime/service/src/test/java/org/apache/polaris/service/task/BatchFileCleanupTaskHandlerTest.java`, with `import static org.assertj.core.api.Assertions.assertThatThrownBy;`:

```java
@Test
public void testBatchDeleteFailurePropagates() {
FileIO fileIO = Mockito.mock(FileIO.class);
String path = "memory://test/previous.metadata.json";
Mockito.doThrow(new IllegalStateException("simulated delete failure"))
.when(fileIO).deleteFile(path);
BatchFileCleanupTaskHandler handler = newBatchFileCleanupTaskHandler(fileIO);

assertThatThrownBy(
() -> handler.tryDelete(
TableIdentifier.of("ns", "table"), fileIO, List.of(path),
"table_metadata", true, null, 1)
.join())
.hasRootCauseInstanceOf(IllegalStateException.class);
}
```

Run from the repository root with Java 21:

```sh
./gradlew :polaris-runtime-service:test \
--tests 'org.apache.polaris.service.task.BatchFileCleanupTaskHandlerTest.testBatchDeleteFailurePropagates'
```

The assertion fails because no exception is propagated. The mock delete is called once despite failing.

I also reproduced the complete handler/executor path using an in-memory file and metastore, a persisted batch task, and the existing event collector. In two independent runs the failing-delete case produced:

```text
deleteCalls=1
handlerThrew=false
filePresent=true
taskPresent=false
taskSuccess=true
```

The otherwise identical successful-delete control removed both the file and task. The existing six batch-cleanup tests also passed; the existing retry test overrides `tryDelete` rather than injecting a real deletion exception.

### Actual Behavior

A deletion exception is logged by Iceberg, but the batch future completes normally. Polaris does not retry that deletion, marks the task successful, and deletes its persisted record while the file remains.

### Expected Behavior

Deletion errors should reach Polaris's bounded retry logic. After retries are exhausted, the handler should throw, record a failed attempt, and leave the task persisted. Already-missing files should remain an idempotent success. Successful batches should continue to remove their task.

This does not require changing the asynchronous REST response or adding startup/periodic task recovery.

### Additional context

Code references:

- [Polaris batch retry delegation](https://github.com/apache/polaris/blob/14fb296dea/runtime/service/src/main/java/org/apache/polaris/service/task/FileCleanupTaskHandler.java#L164-L177)
- [Polaris success and task deletion](https://github.com/apache/polaris/blob/14fb296dea/runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java#L241-L253)
- [Affected metadata file categories](https://github.com/apache/polaris/blob/14fb296dea/runtime/service/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java#L294-L304)
- [Iceberg 1.11.0 best-effort deletion helper](https://github.com/apache/iceberg/blob/apache-iceberg-1.11.0/core/src/main/java/org/apache/iceberg/CatalogUtil.java#L222-L244)

I searched issues and PRs across all states, including the related cleanup work. #774 concerns recovery of unfinished tasks that remain persisted; this bug incorrectly removes the task. #5378 preserves bulk-operation capabilities and #5398 bounds hung deletions; neither changes this failure suppression. #4914 and #4962 establish the intended throw-to-retry handler contract, but the delegated batch helper still returns normally after deletion errors.

A focused fix can call `SupportsBulkOperations.deleteFiles(...)` directly for bulk-capable IO and use error-propagating per-file deletion otherwise. The existing batch retry loop can retain ownership of the retry budget, without another retry layer or public API changes.

### System information

- OS: macOS / arm64
- Polaris: reproduced at `9053be2d96f51b96e9d3e9242c25b70f493ea888`; cleanup source reconfirmed unchanged at `14fb296dea` on 2026-08-31. The same delegation is present in Polaris 1.7.0.
- Java: OpenJDK 21.0.12.1; Gradle wrapper 9.7.1; Iceberg 1.11.0
- Storage: deterministic in-memory FileIO with an injected delete failure; no cloud credentials or live object store required

Contributor guide

Open the contributing guide

Research direction

Start with FileCleanupTaskHandler.tryDelete(...) and BatchFileCleanupTaskHandler.handleTask(...), then read TaskExecutorImpl's success and task-deletion path. Run the focused BatchFileCleanupTaskHandlerTest.testBatchDeleteFailurePropagates test with the provided Gradle command. Done means deletion failures reach the existing bounded retry logic, failed tasks remain persisted, missing files stay idempotent, and successful cleanup still removes its task.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.