feat: retry broke after #1623 started deleting staged temp files

[Wallgau]

upload retry broke after #1623 started deleting staged temp files
immediately after processing. The enhanced task API still marks those
failures as RETRYABLE, but retry then fails with source_file_missing
because the temp path no longer exists on disk.
Clean up upload temps per file only when they are not retryable:

• keep temps for failed local uploads classified as RETRYABLE
• delete temps for successes and non-retryable failures
• on cancel, force-delete all temps (no retry expected)
• if unlink fails, keep the path in temp_file_paths so cleanup can be
  retried later (safe_unlink swallows errors, so verify with exists)
  Add unit tests for retryable retention, mixed success/failure cleanup,
  failed unlink tracking, and ensuring retryable temps are never unlinked.

Summary by CodeRabbit

• Bug Fixes

  • Retryable local upload failures now retain staged temp files for retry; non-retryable temps are removed. Cancelling tasks, evicting old tasks, or shutting down now force-remove staged temps to prevent leftovers. Temp path normalization now warns on mismatches to avoid broken retry retention.

• Tests

  • Added unit tests covering retry vs non-retry temp retention, unlink behavior, and forced cleanup on eviction/shutdown.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6f0e3e9b-33e4-4f05-8057-db5233fbc132

📥 Commits

Reviewing files that changed from the base of the PR and between c638312 and 3e52141.

📒 Files selected for processing (2)

• src/services/task_service.py
• tests/unit/test_task_service_retry_failed_files.py

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/unit/test_task_service_retry_failed_files.py

---

Walkthrough

TaskService centralizes staged upload-temp cleanup with _cleanup_upload_temp_files, classifies retryable local-upload temps, and forces cleanup on cancel, eviction, and shutdown; tests exercise classification, retention, unlink failure handling, and forced-cleanup calls.

Changes

Temp File Cleanup Refactoring

Layer / File(s)	Summary
create_custom_task temp-path normalization src/services/task_service.py	create_custom_task string-normalizes and appends temp_file_paths, warning when normalized paths don't match upload_task.file_tasks keys.
Background processor cleanup integration src/services/task_service.py	background_custom_processor delegates staged-temp cleanup to _cleanup_upload_temp_files instead of unconditionally unlinking paths.
Eviction: cleanup_old_tasks forced cleanup src/services/task_service.py	cleanup_old_tasks calls _cleanup_upload_temp_files(task, force=True) before removing completed/failed tasks.
Cleanup helpers and cancel_task src/services/task_service.py	Adds _file_task_for_temp_path, _is_retryable_local_upload_temp, _should_retain_upload_temp, and _cleanup_upload_temp_files; cancel_task calls cleanup with force=True.
Shutdown forced cleanup src/services/task_service.py	shutdown iterates task_store and force-cleans staged temps for each UploadTask.
Unit tests: lifecycle forced cleanup tests/unit/test_task_service.py	Adds tests asserting cleanup_old_tasks and shutdown call _cleanup_upload_temp_files(..., force=True) and imports patch for mocking.
Unit tests: retryable local temp handling tests/unit/test_task_service_retry_failed_files.py	Adds tests for _is_retryable_local_upload_temp (local vs connector), and _cleanup_upload_temp_files behavior (retain retryable, remove non-retryable, handle unlink failure, retain unmapped absolute temps).

Sequence Diagram(s)

sequenceDiagram
  participant TaskService
  participant UploadTask
  participant FileSystem
  TaskService->>UploadTask: iterate upload_task.temp_file_paths
  TaskService->>TaskService: _file_task_for_temp_path(upload_task, temp_path)
  TaskService->>TaskService: _is_retryable_local_upload_temp(upload_task, temp_path)
  alt retryable and not forced
    TaskService-->>UploadTask: retain temp_path
  else non-retryable or forced
    TaskService->>FileSystem: safe_unlink(temp_path)
    FileSystem-->>TaskService: unlink result / os.path.exists check
    TaskService->>UploadTask: update temp_file_paths based on unlink outcome
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

bug

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely identifies the main fix: retry functionality broke due to temp file deletion introduced in PR #1623, and this PR restores it by conditionally retaining staged temp files.
Docstring Coverage	✅ Passed	Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch task-not-retryable-bug

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/services/task_service.py`:
- Around line 1374-1400: When evicting aged TaskStatus.COMPLETED/FAILED tasks in
TaskService.cleanup_old_tasks (and during shutdown), ensure you call the
existing _cleanup_upload_temp_files(upload_task, force=True) for each upload
task being removed so retained temp files in upload_task.temp_file_paths are
reclaimed; locate where tasks are removed from task_store (and shutdown logic)
and invoke _cleanup_upload_temp_files before deleting/evicting the task, passing
force=True to guarantee deletion of RETRYABLE temp files and updating task_store
accordingly.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f8eeeead-d723-44be-8914-aa2717c66666

📥 Commits

Reviewing files that changed from the base of the PR and between cdc4ef7 and f091a1e.

📒 Files selected for processing (2)

• src/services/task_service.py
• tests/unit/test_task_service_retry_failed_files.py

[mpawlow]

@Wallgau

Code Review 1

• ✅ Approved / LGTM 🚀
• See: PR comments:
  • (1a) Normal severity
  • (1b) Minor severity

[lucaseduoli]

Working perfectly. I think Mike's feedbacks are good