Skip to content

[MEDIUM] Report cleanup deletes files before the DB commit, risking orphaned rows on partial failure #117

Description

@jsogarro

[MEDIUM] cleanup_old_reports deletes files inside the loop but commits only once at the end

Summary
ReportRepository.cleanup_old_reports deletes each report's files from disk (shutil.rmtree) and marks the DB row deleted inside the same loop iteration, but only calls session.commit() once after the entire loop completes. A failure partway through (e.g. in self.delete() or the session itself) leaves already-deleted files with their corresponding DB rows never committed as deleted — on rollback, the DB still references files that no longer exist on disk.

Evidence
src/repositories/report_repository.py:376-399

        for report in old_reports:
            if not dry_run:
                # Delete associated files
                if report.storage_path and os.path.exists(report.storage_path):
                    try:
                        import shutil

                        shutil.rmtree(report.storage_path)
                        logger.info(f"Deleted files for report {report.id}")
                    except Exception as e:
                        logger.error(
                            f"Failed to delete files for report {report.id}: {e}"
                        )

                # Delete database record
                await self.delete(report.id)

            deleted_ids.append(str(report.id))
            deleted_count += 1

        if not dry_run:
            await self.session.commit()

Failure scenario: processing N reports, the filesystem delete succeeds for reports 1 through k, then self.delete() or the surrounding session raises on report k+1 before the trailing commit() is reached — the exception propagates, the session's pending soft-deletes for 1..k are never committed (or are rolled back by the caller), but their files are already permanently gone from disk. Any code that later reads report.storage_path for those rows will 404/error against a file that no longer exists, with no indication in the database that this happened.

Impact
Data-consistency gap between the filesystem and the database on partial failure during a housekeeping job — silent orphaned rows pointing at deleted files, discovered only when someone tries to read them.

Remediation
Either commit per-report (accepting more round trips) so a mid-loop failure only loses the reports already fully processed, or defer all filesystem deletes until after a successful commit of the DB-side soft-deletes, retrying/logging any file that fails to delete post-commit rather than deleting first. Add a test that simulates a mid-loop failure and asserts no DB row survives pointing at a deleted file.

Acceptance

uv run pytest tests/test_report_generator_service.py -v

Activity

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

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions