Fix: Atomic job deletion and database cleanup (#302)

[diksha78dev]

Linked issue

Closes #302

What this PR does

Fixes an issue where deleting a scan job (DELETE /jobs/{job_id}) would only remove the on-disk workspace, leaving orphaned data in the database. This PR introduces an atomic database transaction to clean up jobs, findings, and verify_outcomes rows, and ensures the workspace directory is only deleted if the database operation succeeds.

Type of change

• Bug fix
• New feature
• ML model / training pipeline
• Refactor (no behaviour change)
• Documentation
• Tests only

ML tier (if applicable)

• Tier 1 — Triage
• Tier 2 — Predictive
• Tier 3 — Autonomous
• Not ML-related

Stack affected

• Backend
• Frontend
• Both

---

Changes

Backend

• Updated delete_job in db.py to wrap row deletions in a single transaction with explicit rollback on error.
• Re-ordered endpoint logic in main.py so the on-disk workspace is deleted only after successful database cleanup.
• Removed the 404 block for missing directories, allowing orphaned DB rows to be successfully cleaned up even if the directory is already missing.

Frontend

• No frontend changes.

New dependencies

• None.

Database / schema changes

• None. (Transaction logic implemented using existing schema).

---

Testing

How did you test this?

• Added automated tests to mock the database transactions.
• Verified that DB failures roll back and do not delete the on-disk workspace.
• Verified that missing directories are gracefully handled while cleaning up the database.

Checklist

• Tested locally end-to-end (upload ZIP or GitHub URL → scan → findings returned correctly)
• New ML model falls back gracefully when model file is absent
• No new console.error or unhandled Python exceptions introduced
• Added or updated tests where applicable
• requirements.txt / package.json updated if new dependencies added
• New model files (.pkl, .pt, etc.) are gitignored, not committed

---

Anything reviewers should focus on

Please review the transaction wrapper in delete_job and the new endpoint tests in test_job_endpoints.py to ensure edge cases (e.g. missing directory, DB errors) are fully covered.

Screenshots (if UI changed)

N/A

[github-actions]

🎉 Thank you @diksha78dev for submitting a Pull Request!

We're excited to review your contribution.

Before Review

✅ Ensure all CI checks pass
✅ Complete the PR template
✅ Link the related issue

⚡ Want faster reviews and contributor support?

Join our Discord community:

🔗 https://discord.gg/FcXuyw2Rs

Maintainers and mentors are active there and can help resolve blockers quickly.

Happy Contributing! 🚀

[github-actions]

✅ PR template check passed!

@arpit2006 this PR is ready for your review. 🚀

[arpit2006]

@diksha78dev , Please Resolve CI and Quality issue.

[github-actions]

✅ PR template check passed!

@arpit2006 this PR is ready for your review. 🚀

[github-actions]

✅ PR template check passed!

@arpit2006 this PR is ready for your review. 🚀

[github-actions]

✅ PR template check passed!

@arpit2006 this PR is ready for your review. 🚀

[arpit2006]

PR #314 Review — fix/delete-orphaned-data-db

Verdict: Changes Requested ⚠️

---

Summary

PR #314 claims to fix a narrow, well-scoped bug: DELETE /jobs/{job_id} was leaving orphaned database rows when deleting a scan job. The actual fix is correct and well-implemented. However, the branch carries massive scope creep from absorbed upstream merges — 49 files and 3,200+ net line changes vs. the 3 files and ~89 lines that belong to this PR.

---

✅ What the Fix Actually Does (commit 3711d77)

The real work lives in exactly 3 files and is clean:

backend/app/db.py — delete_job() Transaction

Before:

async def delete_job(db: aiosqlite.Connection, job_id: str):
    await db.execute("DELETE FROM jobs WHERE job_id = ?", (job_id,))
    await db.execute("DELETE FROM findings WHERE job_id = ?", (job_id,))
    await db.execute("DELETE FROM verify_outcomes WHERE job_id = ?", (job_id,))
    await db.commit()

After:

async def delete_job(db: aiosqlite.Connection, job_id: str):
    try:
        await db.execute("BEGIN TRANSACTION")
        await db.execute("DELETE FROM findings WHERE job_id = ?", (job_id,))
        await db.execute("DELETE FROM verify_outcomes WHERE job_id = ?", (job_id,))
        await db.execute("DELETE FROM jobs WHERE job_id = ?", (job_id,))
        await db.commit()
    except Exception:
        await db.rollback()
        raise

✅ Correct ordering (child tables deleted before parent to respect FK semantics).
✅ BEGIN TRANSACTION + rollback() on failure — atomic.
✅ Exception is re-raised after rollback so the caller can handle it.

backend/app/main.py — Endpoint Re-ordering

Before (broken):

if not job_dir.exists(): raise 404   ← blocks orphaned-row cleanup
safe_rmtree(job_dir)                 ← filesystem deleted BEFORE DB cleanup
await delete_job(db, job_id)         ← orphaned rows if rmtree succeeds but this fails

After (correct):

await delete_job(db, job_id)         ← DB cleaned atomically first
if job_dir.exists():
    safe_rmtree(job_dir)             ← filesystem only deleted on DB success

✅ "DB first, filesystem second" — correct invariant for durability.
✅ 404 guard removed — orphaned rows in DB can now be cleaned even when directory is already gone.

backend/tests/test_job_endpoints.py — 3 New Test Cases

Test	Validates
test_delete_success	DB cleaned + rmtree called
test_delete_missing_directory	DB cleaned + rmtree skipped gracefully
test_delete_db_failure	rollback called + rmtree NOT called

---

🔴 Issues Found

1. BEGIN TRANSACTION is Redundant / Potentially Harmful

aiosqlite (backed by sqlite3) operates in auto-commit-off mode by default — every connection is already in an implicit transaction. Manually issuing BEGIN TRANSACTION when a transaction may already be open (e.g., if aiosqlite already began one implicitly) can raise:

sqlite3.OperationalError: cannot start a transaction within a transaction

Fix: Remove the explicit BEGIN TRANSACTION call and rely on the default isolation behavior:

async def delete_job(db: aiosqlite.Connection, job_id: str):
    try:
        await db.execute("DELETE FROM findings WHERE job_id = ?", (job_id,))
        await db.execute("DELETE FROM verify_outcomes WHERE job_id = ?", (job_id,))
        await db.execute("DELETE FROM jobs WHERE job_id = ?", (job_id,))
        await db.commit()
    except Exception:
        await db.rollback()
        raise

Or use async with db: as a context manager, which handles commit/rollback automatically.

2. safe_rmtree Exception is Swallowed

If safe_rmtree(job_dir) raises (e.g., a PermissionError), the endpoint silently returns {"deleted": True} — a false success response. This should either be caught and return a 500, or logged as a non-fatal warning.

# Current — silently succeeds even if filesystem cleanup fails
if job_dir.exists():
    safe_rmtree(job_dir)
return {"deleted": True}

Consider wrapping in a try/except and logging a warning if the directory can't be removed.

3. Test Mock Type Mismatch (AsyncMock for sync .exists())

In the original commit (3711d77), mock_job_dir is created as AsyncMock(), but job_dir.exists() is a synchronous call. The follow-up commit 1c74881 correctly fixes this by switching to MagicMock(). This is good — the fix is in the branch — but it means the original test was not actually validating exists() behavior correctly.

4. No 404 When Job Not Found in DB

After removing the directory-existence check, there is no check at all for whether the job exists in the database before attempting deletion. DELETE WHERE job_id = ? against a non-existent ID silently succeeds (0 rows deleted) and returns {"deleted": True} — a misleading response for clients.

Recommended: Query for job existence first and return 404 if not found:

job = await get_job(db, job_id)
if not job:
    raise HTTPException(status_code=404, detail=f"No job found with id '{job_id}'")

---

🔴 Scope Creep — Critical Issue

The PR description states "No frontend changes" and "Database / schema changes: None." The diff tells a very different story:

Area	Files Changed	Lines
PR's actual fix	3	~89
Other backend changes	10+	~600
Frontend changes	28	~2,974
Total diff	49	~4,700

The branch has absorbed many unrelated upstream merges: dashboard modularization, CSV export, fix confidence indicator, API key auth, path traversal fix, org scan threadpool, SQLite CRUD helpers, etc. These are not part of this bug fix and should not be reviewed or merged here.

This is the same pattern seen in PRs #316, #325, #327, #328, and #300 — contributors are opening PRs from branches that are far ahead of main instead of creating a clean branch off main.

---

✅ What to Approve

The logic in these 3 commits is correct and should be approved once the branch is cleaned up:

• 3711d77 — the core fix
• 2d012f1 — removes unused exception variable e
• 1c74881 — fixes mock type in tests

---

Required Changes Before Approval

1. 🔴 Rebase onto main — Strip all unrelated upstream commits. The PR branch must only contain commits that belong to this fix (the 3 commits above, at minimum).
2. 🟡 Remove explicit BEGIN TRANSACTION — Redundant and potentially error-prone with aiosqlite's implicit transaction model.
3. 🟡 Add 404 check — Return 404 when job_id doesn't exist in the database, rather than silently succeeding.
4. 🟡 Handle safe_rmtree failure — Don't return {"deleted": True} if the filesystem cleanup threw an exception.

---

Checklist

	Item
✅	Core bug fix is correct (transaction wrapping, ordering)
✅	Rollback on DB failure prevents filesystem deletion
✅	Missing directory gracefully handled
✅	Tests cover success, missing directory, and DB failure
❌	Branch scoped to PR description — 49 files changed vs. 3 expected
❌	BEGIN TRANSACTION may cause double-transaction error
❌	No 404 when job_id doesn't exist in DB
❌	safe_rmtree failure not surfaced to caller

[diksha78dev]

@arpit2006 i have resolved all the required fixes . You can review it and let me know if anything requires to fix i'd be happy to resolve them.
Thanks.