Fix: Connection leak in db_update_cluster() to prevent crashes in transactions

[yugalkaushik]

Fixes #824
Changes:

• Modified db_update_cluster() to only close connections it created (own_connection flag)
• Prevents premature closure of externally-managed connections
• Follows the same pattern used throughout the codebase

Summary by CodeRabbit

• Bug Fixes
  • Improved database connection lifecycle: the system now only commits and closes connections it creates itself, avoiding unintended closure of externally provided connections and reducing integration errors with shared database resources.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The db_update_cluster() function now only commits and closes database connections it created (own_connection=True); when an external connection is provided, commit/close are skipped to avoid prematurely closing externally-managed connections (preserving caller-managed transactions).

Changes

Cohort / File(s)	Summary
Connection Lifecycle Management backend/app/database/face_clusters.py	Modified db_update_cluster() to commit/close the DB connection only when own_connection is True; when an external connection is supplied, the function no longer commits or closes it in the finally block.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant db_update_cluster as Updater
  participant DB

  rect rgb(240,248,255)
  Note over Caller,Updater: New flow (conditional close)
  Caller->>Updater: call db_update_cluster(conn=external_conn, own_connection=False)
  Updater->>DB: perform update using external_conn
  Updater-->>Caller: return result (no commit/close)
  Note over Caller: Caller retains responsibility for commit/close
  end

  rect rgb(245,255,240)
  Note over Caller,Updater: Internal-connection flow (own_connection=True)
  Caller->>Updater: call db_update_cluster(conn=None, own_connection=True)
  Updater->>DB: open connection, perform update
  Updater->>DB: commit
  Updater->>DB: close connection
  Updater-->>Caller: return result
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: fixing a connection leak in db_update_cluster() to prevent crashes in transactions, which aligns with the changeset.
Linked Issues check	✅ Passed	The code changes implement the core requirement from issue #824 by preventing db_update_cluster() from closing connections it did not create, using an own_connection flag.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the connection management issue in db_update_cluster(), with no out-of-scope modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eb3037b and 3c79d2b.

📒 Files selected for processing (1)

• backend/app/database/face_clusters.py

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

• backend/app/database/face_clusters.py

---

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 and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

backend/app/database/face_clusters.py (1)

209-230: Missing exception handling with rollback logic.

The try block lacks an except clause to handle errors and rollback transactions when exceptions occur. This is inconsistent with similar functions in this file (db_delete_all_clusters lines 63-67 and db_insert_clusters_batch lines 117-120), which explicitly rollback when own_connection is True.

Without this, externally-managed connections may be left in an inconsistent state if an exception occurs after the UPDATE but before returning.

🔎 Proposed fix

     try:
         # Build the update query dynamically based on provided parameters
         update_fields = []
         update_values = []

         if cluster_name is not None:
             update_fields.append("cluster_name = ?")
             update_values.append(cluster_name)

         if not update_fields:
             return False

         update_values.append(cluster_id)

         cursor.execute(
             f"UPDATE face_clusters SET {', '.join(update_fields)} WHERE cluster_id = ?",
             update_values,
         )

         updated = cursor.rowcount > 0
-        conn.commit()
+        if own_connection:
+            conn.commit()
         return updated
+    except Exception:
+        if own_connection:
+            conn.rollback()
+        raise
     finally:
         if own_connection:
             conn.close()

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d07d817 and eb3037b.

📒 Files selected for processing (1)

• backend/app/database/face_clusters.py