[ybai08]

What

snapshot_before_upgrade no longer lets the cleanup in its error handler replace the error it is reporting. Removing the .partial file is now wrapped in contextlib.suppress(OSError), so the caller always gets RuntimeError: could not write a database snapshot to ... before migrating: <reason>.

Why

One of the three integration failures that #2186 uncovered once the unit step passed (the integration step had not run on a PR since 2026-10-05 18:47 UTC):

tests/integration/test_migration_snapshot.py::test_a_copy_that_cannot_be_written_stops_the_upgrade
NotADirectoryError: [Errno 20] Not a directory: '.../backups/0025-0026.db.partial'

The test puts a file where the backups folder should be. mkdir raises FileExistsError, and the handler then calls partial.unlink(missing_ok=True) on a path beneath that file. macOS and Linux answer that with ENOTDIR, which missing_ok does not cover, so the NotADirectoryError escapes instead of the RuntimeError. Windows reports the path as not found, which missing_ok swallows, so the test passes there.

The upgrade is still not run in either case, so no database migrates without a copy. What is lost on macOS and Linux is the message: start fails with a bare NotADirectoryError on the .partial path rather than the sentence that says the copy could not be written and why.

How to test

• cd surfsense_local/backend && uv run pytest -m integration tests/integration/test_migration_snapshot.py
  • macOS (arm64): 5 passed. Without the fix: 2 failed, 3 passed.
  • Linux (ghcr.io/astral-sh/uv:python3.12-bookworm-slim, arm64): 5 passed. dev fails the existing test there with the runner's message.
• New test test_a_cleanup_that_fails_too_still_reports_the_copy makes the removal of the partial file raise through a patched Path.unlink, so the handler is exercised on Windows as well, where the existing test cannot reach it. It fails without the fix.
• ruff check and ruff format --check on both files.

Not run: Windows. The new test's claim about Windows (that mkdir on an existing file raises FileExistsError there) is from the standard library's code, not from a run.

This PR's Backend job will still be red until #2186 and the other two integration failures are fixed; the snapshot test should be gone from its list.

High-level PR Summary

This PR fixes an error handling issue in the database migration snapshot logic where cleanup failures during error recovery could mask the original error. Specifically, when the system attempts to create a backup before upgrading the database but that backup fails (e.g., when the backup directory is blocked by a file), removing the partial backup file could also fail on macOS and Linux, causing a NotADirectoryError to replace the more informative RuntimeError about the snapshot failure. The fix wraps the cleanup operation in contextlib.suppress(OSError) to ensure the original error message always reaches the user.

⏱️ Estimated Review Time: 5-15 minutes

💡 Review Order Suggestion

Order	File Path
1	surfsense_local/backend/shared/migration_snapshot.py
2	surfsense_local/backend/tests/integration/test_migration_snapshot.py

Summary by CodeRabbit

• Bug Fixes
  • Database upgrades now report the original snapshot error even if cleanup of a partial snapshot also fails.
  • When snapshot creation fails, the database remains at its previous revision.

[vercel]

@ybai08 is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel.

A member of the Team first needs to authorize it.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: MODSetter/SurfSense/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 8e1f7601-fc46-4402-8458-10d1ccc6626e

📥 Commits

Reviewing files that changed from the base of the PR and between 2fdc57d and 98c5b0f.

📒 Files selected for processing (2)

• surfsense_local/backend/shared/migration_snapshot.py
• surfsense_local/backend/tests/integration/test_migration_snapshot.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

---

📝 Walkthrough

Walkthrough

When snapshot creation fails, cleanup now suppresses OSError so it does not replace the original error. An integration test checks the original error cause and confirms that the database remains at its previous revision.

Changes

Snapshot error handling

Layer / File(s)	Summary
Preserve snapshot errors during cleanup surfsense_local/backend/shared/migration_snapshot.py, surfsense_local/backend/tests/integration/test_migration_snapshot.py	Cleanup suppresses OSError when removal of a partial snapshot fails. The integration test verifies that the snapshot-related RuntimeError retains the original FileExistsError as its cause and that the database stays at its previous revision.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 98c5b

When a pre-upgrade snapshot fails, users now see the real snapshot error instead of a confusing cleanup error. The migration still does not run after a snapshot failure. The change is small and covered by an integration test, so merge risk is minimal.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title describes the main change: preserve the database snapshot failure when removing the partial file also fails. It is specific and relevant, though somewhat wordy.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.}