No actionable comments were generated in the recent review. 🎉

Walkthrough

Seven tasks.py DB functions (get, list_tasks, update_depends_on, update_requirement_ids, delete, delete_all, count_by_status) gained try/finally blocks to guarantee conn.close() on exception. A module-level _background_tasks set was added to hold strong references to in-flight _safe_close_issue asyncio tasks, with a done-callback for removal. WebhookNotificationService.send_blocker_notification_background was deleted. New tests cover both fixes.

Changes

DB connection safety & asyncio task retention (issue #650)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 Hop, hop, finally-block delight,
No leaked connections left in sight!
My tasks stay referenced, safe and sound,
No garbage-collected mid-rebound.
The rabbit's code runs clean and bright! ✨

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

Code Review

This is a clean, focused robustness PR. Both fixes are correct and address real bugs. A few observations below.

Connection leak fix (tasks.py)

Pattern is correct. Placing conn = get_db_connection(...) outside the try and conn.close() in finally is the right idiom — if the connection cannot be acquired, there is nothing to close. This mirrors the existing guarded paths the PR description references.

Implicit rollback on write paths. For update_depends_on, update_requirement_ids, delete, and delete_all, a failure after the SQL execute but before conn.commit() closes the connection without an explicit rollback(). SQLite auto-rolls back on close, so this is functionally correct — worth noting for consistency if other paths use an explicit rollback in a handler.

Fire-and-forget retention (_background_tasks set)

Correct and thread-safe. Since loop.create_task and add_done_callback both execute in the event loop thread, set operations are confined to that thread — no lock needed. The discard done-callback prevents unbounded set growth. This correctly mirrors WebhookNotificationService.send_event_background.

Removal of send_blocker_notification_background

Correct removal. The method called asyncio.create_task with no strong reference and no loop-missing fallback — the exact bug this PR fixes in the tasks path. Production blocker notifications already go through send_event_background, so nothing is lost.

Tests (test_tasks_connection_safety.py)

The _TrackingConn wrapper is a clean approach — intercepts at the right level without heavier mocking. A few minor gaps:

1. delete and delete_all are not tested for commit errors. Both call conn.commit(), so a commit failure was also previously a leak vector. The fix covers it via the same try/finally, but the test suite does not exercise that path. Adding them to test_write_paths_release_on_commit_error would close the gap.

2. list_tasks status-filter branch. The parametrized test calls list_tasks with default args (no status filter). Both SQL branches share the same try/finally, so the risk is low — but adding a filtered call to the parametrize list would give full branch coverage.

3. Test isolation for _background_tasks. The test clears tasks._background_tasks at the start but does not restore it afterward. If the async task raises before its done-callback fires, the set may be dirty for subsequent tests. A finally cleanup or autouse fixture teardown would make the isolation airtight.

Summary

Both fixes are correct and well-reasoned, the test approach is sound, and the PR is well-scoped. The main improvement opportunity is the delete/delete_all commit-error gap in the test parametrization. No blocking issues — ready to merge once that gap is addressed or accepted as a known omission.

Thanks for the review. Addressed the test gaps in 0fd0295:

• delete/delete_all commit-error gap → both now included in test_write_paths_release_on_commit_error.
• list_tasks status-filter branch → faults now injected at execute() (reaching the function body), and a list_tasks(status=READY) variant added so both SQL branches are exercised.
• _background_tasks test isolation → snapshot + try/finally restore so the set is never left dirty for subsequent tests.

The implicit-rollback-on-close note is acknowledged as correct (SQLite auto-rolls back on close); no explicit rollback handler exists on these paths, so leaving it implicit keeps the diff consistent.

Follow-up Review (commit 0fd0295)

All three gaps flagged in the initial review are addressed — and the implementation in commit 0fd0295 is cleaner than the minimum required.

What changed

Fault injection moved from cursor() to execute() via _TrackingCursor.
The new approach threads a cursor proxy through _TrackingConn.cursor(), so each function's body (including list_tasks's if status: branch) is reached before the exception fires. This is strictly better than faulting at cursor acquisition — it proves the try/finally guard works at the point closest to the actual SQL call.

delete and delete_all added to test_write_paths_release_on_commit_error.
Both paths commit and were previously missing from that parametrize list. Covered now.

_background_tasks isolation hardened with snapshot/restore try/finally.
The saved-set + finally restore means the module-level set is always left clean, even if an assertion fails before the task drains. Correct.

One minor note (non-blocking)

In test_write_paths_release_on_commit_error, the delete and delete_all variants execute the real SQL (no execute fault, only a commit fault), so the DELETE runs on the real cursor but is silently rolled back by SQLite on close. Each test gets a fresh workspace fixture, so there is no cross-test contamination — just worth being aware of if test logs ever show unexpected row counts during a future debug session.

Summary

PR is clean. Implementation correct, tests thorough, scope tight. Ready to merge.