Skip to content

Archive deletion task acknowledges failed storage cleanup as success #44059

Closed

Description

@CryoThrust

Self Checks

  • I have read the Contributing Guide and Language Policy.
  • This is a bug report.
  • I have searched existing issues, including closed ones.
  • This report is in English.

Dify version

Current main e421dcc1f53291e79dd62ae63f10e1d92d283ba3, after #44052.

Cloud or Self Hosted

Self Hosted (Source), local method-boundary reproduction with controlled storage/catalog/lease collaborators; not a deployed DB/Redis/S3 environment.

Steps to reproduce

  1. Request permanent deletion of an archived month so its catalog rows have a durable deletion status.
  2. Make an archive or prepared-download object deletion raise ArchiveStorageError.
  3. Execute the actual delete_workflow_run_archive_task through Celery's eager task path, while recording its retry calls.

WorkflowRunArchiveDeleter.delete() catches the exception, persists FAILED, and returns a WorkflowRunArchiveDeletionTask(status=FAILED). The Celery entry point discards that result and returns normally:

with patch.object(task, "retry", wraps=task.retry) as retry:
    result = task.apply(args=(tenant_id, 2025, 3))
    assert result.successful()
    assert result.result is None
    assert retry.call_count == 0
    assert catalog_record.deletion_status == WorkflowRunArchiveDeletionStatus.FAILED

The snippet describes assertions from an executed local probe. It runs unchanged deletion/storage/task methods, the real Celery 5.6.3 autoretry decorator and Pydantic 2.12.5, with controlled collaborators. A month-busy control reaches task.retry; clearing the storage fault and explicitly invoking deletion again completes cleanup. These controls separate the task-boundary problem from catalog persistence and idempotent manual retry.

I reran the eight boundary controls on the main revision above and checked that the relevant production files match the reviewed implementation. The original finding is here.

✔️ Expected Behavior

Retryable cleanup failure reaches the configured Celery retry/backoff policy, while keeping durable deletion requests and the failed status available for recovery. Completed/no-request work returns normally. Exhausting the retry budget must not silently report success.

❌ Actual Behavior

Celery reports SUCCESS with None, retry is called zero times, and the requested catalog rows/files remain. The declared autoretry_for=(Exception,), backoff and five-retry budget do not apply to these failures because no exception escapes. acks_late also does not retry a normally returning task.

The existing service tests demonstrate that a caller can manually invoke deletion again; they do not demonstrate that the worker automatically retries a returned failed result.

A narrow fix could propagate a retryable failure at the task boundary when the returned deletion status is FAILED, with real task-level controls for transient failure then success, repeated failure/exhaustion, and completed/no-request results. I can work on that follow-up if this matches the intended worker contract. The separate late-upload compensation finding is outside this issue's scope.

Activity

  1. dosubot commented on Oct 14, 2026

    @dosubot

    This matches the failure path in the current implementation: delete() persists FAILED and returns a failed result for exceptions other than WorkflowRunArchiveMonthBusyError. The task ignores that result despite declaring exception-based autoretry, so those failures never reach its retry policy. The month-busy exception is explicitly re-raised, which explains your control reaching task.retry. [1] [2]

    The intended resumability is supported by the deleter’s contract: it describes partial storage failures as retryable, selects records with durable deletion requests, and removes catalog records only after archive and prepared-download cleanup succeeds. That supports your proposed task-boundary follow-up, although a maintainer still needs to confirm the worker contract. [1]

    One distinction to retain in that follow-up: FAILED also covers validation and manifest-identity failures, not just transient storage errors. Raising on every failed result would therefore retry those failures too. If the policy should distinguish retryable cleanup errors, the service/task boundary needs to preserve that classification rather than relying only on the status. [1]

    To reply, just mention @dosu.


    Share context across your team and agents. Try Dosu.

    Leave Feedback Learn about dify with Dosu

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    review: mediumghfind author score; see https://ghfind.com

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions