fix(core): release tasks.py DB connections on exception + retain fire-and-forget auto-close task (#650)

[frankbria]

Summary

Fixes two low-severity backend robustness issues flagged in the release-readiness audit (#650).

1. Connection leak-on-exception in tasks.py

Several DB paths opened get_db_connection and called conn.close() with no try/finally. An exception raised between open and close leaked a connection (holding a read lock). Now wrapped in try/finally (connection acquisition stays outside the try, mirroring the already-correct guarded paths at lines 122/208/254/370/576):

• get (read)
• list_tasks (read)
• count_by_status (read)
• update_depends_on (write)
• update_requirement_ids (write)
• delete (write)
• delete_all (write)

2. Fire-and-forget tasks without strong references

• _close_issue_background's async branch did loop.create_task(...) without retaining the task. asyncio keeps only a weak reference, so it could be garbage-collected mid-flight. Now retained in a module-level set() with a done-callback that discards it (and consumes any exception), mirroring WebhookNotificationService.send_event_background.
• Removed the unused, buggy send_blocker_notification_background (bare asyncio.create_task, no reference, no no-loop fallback) plus its test. Production blocker notifications already use the safe send_event_background.

Acceptance criteria

• All tasks.py DB reads release the connection on exception
• Fire-and-forget tasks are retained until completion

Testing

• New tests/core/test_tasks_connection_safety.py: parametrized tests force a cursor/commit error and assert every opened connection is closed; async test asserts the auto-close task is retained until completion then discarded. (RED→GREEN confirmed.)
• ruff check clean on all changed files.
• Regression suites pass: tests/core/test_tasks_edge_cases.py, test_task_dependencies.py, test_task_github_traceability.py, test_task_requirement_ids.py, tests/notifications/ (78 passed); tests/ui/test_v2_routers_integration.py + test_github_integrations_v2.py (107 passed).
• Cross-family review (codex review): no actionable issues.

Known limitations

• The sync branch of _close_issue_background (non-daemon thread) was already correct and is unchanged.
• send_blocker_notification (the underlying async method) is retained — it is not flagged by the audit and has its own test coverage; only the broken fire-and-forget wrapper was removed.

Closes #650

Summary by CodeRabbit

• Bug Fixes
  • Improved database connection cleanup to prevent leaked connections when operations fail mid-flight.
  • Strengthened lifecycle handling for background GitHub issue auto-closing to avoid premature cancellation during execution.
• Refactor
  • Updated webhook notification behavior by removing the fire-and-forget background wrapper.
• Tests
  • Added regression coverage for connection release on exceptions and for background task retention behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 23a6e8c0-b6d5-4e30-8076-e4ce6905a71a

📥 Commits

Reviewing files that changed from the base of the PR and between 30574b2 and 0fd0295.

📒 Files selected for processing (1)

• tests/core/test_tasks_connection_safety.py

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/core/test_tasks_connection_safety.py

---

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)

Layer / File(s)	Summary
try/finally DB connection safety in tasks.py codeframe/core/tasks.py	get, list_tasks, update_depends_on, update_requirement_ids, delete, delete_all, and count_by_status now close their DB connections in finally blocks, guaranteeing release on exception.
Strong references for fire-and-forget asyncio tasks codeframe/core/tasks.py, codeframe/notifications/webhook.py	Module-level _background_tasks set added; _close_issue_background stores each new task there and discards it via a done_callback. WebhookNotificationService.send_blocker_notification_background (unretained create_task with no reference) is removed.
Connection-safety and task-retention regression tests tests/core/test_tasks_connection_safety.py, tests/notifications/test_webhook_notifications.py	TestConnectionReleasedOnException parametrizes all affected DB entry points with a _TrackingConn wrapper to assert conn.close() fires on mid-operation exceptions. TestFireAndForgetRetention asserts the background task is in _background_tasks immediately after scheduling and is removed after completion. The now-deleted method's test is removed from the webhook test suite.

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! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title accurately captures both main changes: database connection leak fixes and fire-and-forget task retention, matching the substantial modifications across tasks.py and webhook.py.
Linked Issues check	✅ Passed	All acceptance criteria from issue #650 are met: tasks.py database reads release connections via try/finally blocks on exception, fire-and-forget tasks are retained in _background_tasks with done_callbacks until completion, and the buggy send_blocker_notification_background is removed.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to issue #650: connection safety wrapping in tasks.py, background task retention, removal of the unused send_blocker_notification_background method, and corresponding test updates. No unrelated modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/650-tasks-connection-leak-fire-and-forget

---

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

[claude]

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.

[frankbria]

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.

[claude]

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.