Skip to content

Clean up storage probe files when validation fails#717

Open
M-Hassan-Raza wants to merge 1 commit intocodingjoe:mainfrom
M-Hassan-Raza:fix/storage-probe-cleanup
Open

Clean up storage probe files when validation fails#717
M-Hassan-Raza wants to merge 1 commit intocodingjoe:mainfrom
M-Hassan-Raza:fix/storage-probe-cleanup

Conversation

@M-Hassan-Raza
Copy link
Copy Markdown

@M-Hassan-Raza M-Hassan-Raza commented Apr 15, 2026

The storage health check writes a temp probe file, validates it, and then deletes it. Before this change, if the write succeeded but validation failed after that, the temp file could be left behind.

That is usually harmless on local disk, but it is not great behavior on object storage backends where failed probes can leave residue behind over time. This change makes the failed validation path do a best effort cleanup before returning the original error.

The normal success path stays the same. The check still verifies that the backend can actually delete the probe file when the check passes, so this does not loosen the contract, it just fixes the broken failure path.

I also added regression coverage for the cases where the saved file is missing after write, where the saved content does not match, and where cleanup itself fails. In the cleanup failure case we still keep the original validation error and only log the cleanup problem instead of masking it.

Tests run:
uv run pytest -q tests/test_checks.py
uv run pytest -q
uvx ruff check health_check/checks.py tests/test_checks.py

Results on my side:
uv run pytest -q tests/test_checks.py passed
uv run pytest -q -> 154 passed, 26 skipped
uvx ruff check health_check/checks.py tests/test_checks.py passed

Copilot AI review requested due to automatic review settings April 15, 2026 20:08
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes the Storage health check failure path so that probe files are best-effort deleted when validation fails after a successful write, avoiding residue on object storage backends.

Changes:

  • Refactors the storage probe flow to save → validate → (cleanup on validation failure) → delete.
  • Adds a cleanup_file() helper that logs (but does not mask) cleanup failures.
  • Adds regression tests covering missing file after save, content mismatch, and cleanup failure behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
health_check/checks.py Adds best-effort cleanup when post-write validation fails, while preserving the original validation error.
tests/test_checks.py Adds parameterized regression coverage ensuring saved probe files are deleted on validation failures and cleanup failures are only logged.

@codingjoe codingjoe enabled auto-merge (squash) April 15, 2026 21:55
@codingjoe codingjoe disabled auto-merge April 15, 2026 21:55
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.75%. Comparing base (c674d2f) to head (310821e).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #717   +/-   ##
=======================================
  Coverage   99.74%   99.75%           
=======================================
  Files          13       13           
  Lines         792      800    +8     
=======================================
+ Hits          790      798    +8     
  Misses          2        2           
Flag Coverage Δ
python-3.10-django-5.2 50.25% <100.00%> (+0.50%) ⬆️
python-3.10-django-5.2-celery 54.87% <100.00%> (+0.45%) ⬆️
python-3.10-django-5.2-kafka 53.12% <100.00%> (+0.47%) ⬆️
python-3.10-django-5.2-psutil 61.25% <100.00%> (+0.39%) ⬆️
python-3.10-django-5.2-rabbitmq 52.87% <100.00%> (+0.47%) ⬆️
python-3.10-django-5.2-redis 56.25% <100.00%> (+0.44%) ⬆️
python-3.10-django-5.2-rss 71.12% <100.00%> (+0.29%) ⬆️
python-3.11-django-5.2 50.25% <100.00%> (+0.50%) ⬆️
python-3.12-django-5.2 50.25% <100.00%> (+0.50%) ⬆️
python-3.12-django-6.0 50.25% <100.00%> (+0.50%) ⬆️
python-3.13-django-5.2 50.25% <100.00%> (+0.50%) ⬆️
python-3.13-django-6.0 50.25% <100.00%> (+0.50%) ⬆️
python-3.14-django-5.2 50.50% <100.00%> (+0.50%) ⬆️
python-3.14-django-5.2-celery 55.15% <100.00%> (+0.45%) ⬆️
python-3.14-django-5.2-psutil 61.55% <100.00%> (+0.39%) ⬆️
python-3.14-django-5.2-rabbitmq 53.01% <100.00%> (+0.47%) ⬆️
python-3.14-django-5.2-redis 56.53% <100.00%> (+0.44%) ⬆️
python-3.14-django-5.2-rss 72.11% <100.00%> (+0.28%) ⬆️
python-3.14-django-6.0 50.50% <100.00%> (+0.50%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants