# Log Backup Delete Cleanup Fixes — Design Spec

**Date**: 2026-05-15  
**Scope**: `dataflow-service` — log backup delete DAG cleanup bugs  
**Status**: Design approved, pending implementation

---

## Problem Statement

When a log backup (PiTR) is deleted, its S3 storage is cleaned by the `CleanLogBackupStorageAction` in the delete DAG. Investigation of residual S3 files in CRRF (Cross-Region Replication Failover) clusters uncovered several bugs in this cleanup flow.

### Confirmed Issues

| # | File | Issue | Severity |
|---|------|-------|----------|
| 1 | `clean_storage.go` | Compact backup data written to DR bucket is **never cleaned** — only the SR bucket path (`logBackup.Storage`) is deleted. `compact_backups` may reference a different `bucket_id` (DR bucket). | Known bug — **TODO comment only, not fixed in this PR** |
| 2 | `clean_storage.go` | S3 `DeleteSubDir` errors are **swallowed**: logged as `Warn` but not returned, so `cleaned_up=1` is set even when deletion fails | Fix in this PR |
| 3 | `delete_log_backup_workload.go` | Running compact K8s CRs are **not cancelled** before log backup storage is deleted — compact jobs can continue writing to an already-cleaned path | Fix in this PR (best-effort) |
| 4 | `clean_storage.go` | Empty `Storage` field silently marks `cleaned_up=1` with no deletion | **No change** — intentional backward-compatible behavior for historical data |

---

## Design

### Issue 2: Return delete error instead of swallowing it

**File**: `app/workflow/action/backup/pit/del/clean_storage.go`

**Current behavior** (lines 129–133):
```go
if deleteErr != nil && !s3.IsNotFoundError(deleteErr) && !blob.IsNotFoundError(deleteErr) {
    logger.Warn("force delete log backup objects failed", ...)
}
return nil  // always returns nil — bug
```

**Fix**: Return `deleteErr` so the DAG task fails and retries on transient S3 errors:
```go
if deleteErr != nil && !s3.IsNotFoundError(deleteErr) && !blob.IsNotFoundError(deleteErr) {
    logger.Error("force delete log backup objects failed", ...)
    return deleteErr  // fail the task
}
return nil
```

`cleaned_up=1` is only set in `Run()` after `cleanCloudStorage()` succeeds, so this change prevents premature `cleaned_up` marking.

**Why this is safe**: The DAG task is idempotent — `DeleteSubDir` is safe to retry. The existing guard at the top of `Run()` (`if logBackup.CleanedUp`) prevents re-running on an already-cleaned backup.

---

### Issue 3: Best-effort cancel running compact CRs before log backup deletion

**File**: `app/workflow/action/backup/pit/del/delete_log_backup_workload.go`

**Problem**: `DeleteLogBackupWorkloadAction` cancels the log backup K8s CR but does not cancel any in-progress compact backup CRs that write to the same storage path. After log backup deletion, compact CRs may continue running and writing data to paths that will then be cleaned — or worse, paths that have already been cleaned.

**Fix**: Before deleting the log backup K8s CR, enumerate and best-effort-cancel any running/pending compact backup CRs.

**New dependencies** injected into `DeleteLogBackupWorkloadAction`:
- `compactBackupRepo backup_repo.CompactBackupRepo` — for `ListByLogBackup`
- `compactFacade infraapi.CompactFacade` — for `DeleteCompactBackupIgnoreNotFound`

Note: `cloudProvider` and `region` are already derived from `cluster` in existing `Run()` code before this call. `tenantID` and `projectID` come from `logBackup` directly (same pattern as the existing `DeleteLogBackupIgnoreNotFound` call).

**New cancellation logic** added to `Run()` before the existing `DeleteLogBackupIgnoreNotFound` call:
```go
// Best-effort: cancel any running compact CRs before deleting log backup workload.
// Errors are logged as warnings but do not block deletion.
a.cancelRunningCompacts(ctx.Context(), logBackup, logger, cloudProvider, region)
```

```go
func (a *DeleteLogBackupWorkloadAction) cancelRunningCompacts(
    ctx context.Context,
    logBackup *entity.LogBackup,
    logger *zap.Logger,
    cloudProvider, region string,
) {
    compacts, err := a.compactBackupRepo.ListByLogBackup(ctx, logBackup.ID.Uint64())
    if err != nil {
        logger.Warn("failed to list compact backups for cancellation", zap.Error(err))
        return
    }
    for _, c := range compacts {
        if c.Status != entity.CompactBackupStatusRunning && c.Status != entity.CompactBackupStatusPending {
            continue
        }
        if c.DownstreamCompactID == nil {
            continue
        }
        if err := a.compactFacade.DeleteCompactBackupIgnoreNotFound(
            ctx, cloudProvider, region,
            logBackup.TenantID.Uint64(), logBackup.ProjectID.Uint64(), logBackup.ClusterID.Uint64(),
            *c.DownstreamCompactID,
        ); err != nil {
            logger.Warn("failed to cancel compact backup CR (best-effort)",
                zap.String("downstream_id", *c.DownstreamCompactID),
                zap.Uint64("compact_id", c.ID.Uint64()),
                zap.Error(err))
        } else {
            logger.Info("cancelled compact backup CR",
                zap.String("downstream_id", *c.DownstreamCompactID),
                zap.Uint64("compact_id", c.ID.Uint64()))
        }
    }
}
```

**Why best-effort**: Compact CR cancellation is a safety measure against a race, not a hard prerequisite. The log backup CR deletion must proceed regardless; otherwise the cluster deletion flow could block indefinitely.

---

### Issue 1: TODO comment (known bug — not fixed in this PR)

**File**: `app/workflow/action/backup/pit/del/clean_storage.go`

Add a TODO comment in `cleanCloudStorage()` after the existing `DeleteSubDir` call:

```go
// TODO(known-bug): For CRRF clusters, compact_backups may reference a different
// bucket_id (DR bucket) than logBackup.BucketID (SR bucket). This cleanup only
// deletes the SR bucket path. Compact backup data written to the DR bucket via
// bucket.URIInBucket(logBackup.Storage) is never cleaned and will persist
// indefinitely after log backup deletion.
// See: log-backup-lifecycle.md §10.4 Issue 1
```

---

### Issue 4: No change (backward-compatible behavior)

The empty `Storage` guard (lines 63–71 of `clean_storage.go`) silently marks `cleaned_up=1` for records where `Storage == ""`. This is **intentional** for historical data that was cleaned by a previous program. No behavior change is made.

---

## Affected Files

| File | Change type |
|------|-------------|
| `app/workflow/action/backup/pit/del/clean_storage.go` | Fix Issue 2: return `deleteErr`; add Issue 1 TODO comment |
| `app/workflow/action/backup/pit/del/delete_log_backup_workload.go` | Fix Issue 3: add `cancelRunningCompacts` best-effort logic |
| `app/workflow/dag/backup/pit/delete.go` | Wire new dependencies to `DeleteLogBackupWorkloadAction` |
| `app/workflow/action/backup/pit/del/clean_storage_test.go` | Add test: `cleanCloudStorage` returns error when DeleteSubDir fails |
| `app/workflow/action/backup/pit/del/delete_log_backup_workload_test.go` | Add test: running compact CRs are cancelled best-effort |

---

## Testing

### Issue 2 test
- Mock `blob.Storage.DeleteSubDir` returning a non-NotFound error
- Assert `cleanCloudStorage()` returns that error
- Assert `cleaned_up` is NOT set to true in the `LogBackupRepo`

### Issue 3 test
- Setup: 2 compact backups for the log backup — one `running` with a `DownstreamCompactID`, one `success`
- Assert: `compactFacade.DeleteCompactBackupIgnoreNotFound` is called once (for the running compact)
- Assert: the success compact's CR is not touched
- Assert: a `DeleteCompactBackupIgnoreNotFound` error does not block the main deletion

---

## Out of Scope

- **Issue 1 fix**: Cleaning DR bucket compact data requires querying `compact_backups` by `log_backup_id`, detecting distinct `bucket_id`s, and calling `URIInBucket` to derive DR paths. This is a larger change with its own test surface. Tracked as TODO in code and §10.4 of `log-backup-lifecycle.md`.
- No schema changes
- No DAG step additions (Issue 3 is handled inside the existing step)