fix: address review findings on durable repo-state (#311)

_clone_into_place: drop ignore_errors on the canonical-dir cleanup so a failed removal surfaces (ensure_clone wraps it into a logged RepoCloneError) instead of being swallowed and causing a confusing rename failure later.

update_repository_status / try_set_indexing_status: gate the missing-column migration fallback on the error actually naming indexing_started_at, so an unrelated failure (network, constraint) re-raises instead of being masked by the retry. CodeRabbit's 'fallback never runs' reading was incorrect (the not-in check does execute the retry), but narrowing the catch to the migration case is the right hardening.

Add an injection-safety note above the PostgREST .or_ filter: cutoff is server-controlled (utcnow + STUCK_INDEXING_THRESHOLD_MINUTES); any future user-supplied filter value must be SDK-parameterized. Tests: unrelated errors now re-raise rather than hit the fallback.

Author: DevanshuNEU

backend/services/repo_manager.py

@@ -209,9 +209,12 @@ def _clone_into_place(self, repo_id: str, git_url: str, branch: str, canonical:
         tmp = self.repos_dir / f".{repo_id}.tmp.{uuid.uuid4().hex}"
         try:
             git.Repo.clone_from(git_url, tmp, branch=branch, depth=1)
-            # Clear any leftover partial dir before the atomic swap.
+            # Clear any leftover partial dir before the atomic swap. Do NOT ignore errors here:
+            # a failed removal must surface (the outer except re-raises it, and ensure_clone wraps
+            # it into a logged RepoCloneError) rather than letting us rename onto a dir we could
+            # not clean, which would fail later with a more confusing error.
             if canonical.exists():
-                shutil.rmtree(canonical, ignore_errors=True)
+                shutil.rmtree(canonical)
             os.rename(tmp, canonical)  # atomic on the same filesystem
         except Exception:
             if tmp.exists():

backend/services/supabase_service.py

@@ -100,12 +100,13 @@ def update_repository_status(self, repo_id: str, status: str) -> None:
         try:
             self.client.table("repositories").update(updates).eq("id", repo_id).execute()
         except Exception as e:
-            # Degrade gracefully if migration 003 (indexing_started_at) hasn't been applied yet:
-            # retry the status update without the new column instead of failing the index.
-            if "indexing_started_at" not in updates:
+            # Only swallow the specific case where migration 003 (indexing_started_at) has not been
+            # applied yet -- detected by the column name appearing in the DB error. Any other error
+            # (network, constraint, etc.) must re-raise so a real failure isn't masked by the retry.
+            if "indexing_started_at" not in updates or "indexing_started_at" not in str(e):
                 raise
             logger.warning(
-                "Status update with indexing_started_at failed; retrying without it (apply migration 003)",
+                "indexing_started_at column missing; retrying status update without it (apply migration 003)",
                 repo_id=repo_id, error=str(e),
             )
             self.client.table("repositories").update({"status": status}).eq("id", repo_id).execute()
@@ -120,6 +121,11 @@ def try_set_indexing_status(self, repo_id: str) -> bool:
         -- or with no start stamp at all (legacy/pre-migration) -- is treated as orphaned and
         re-claimed, so a crashed job never permanently blocks retry.
         """
+        # cutoff is the staleness threshold for stealing an orphaned 'indexing' lock, derived from
+        # STUCK_INDEXING_THRESHOLD_MINUTES and a SERVER-CONTROLLED datetime.utcnow(). It is never
+        # user input, so interpolating it into the PostgREST .or_ filter string is safe. If any
+        # user-supplied value is ever introduced into this filter, parameterize it via the SDK
+        # (do not f-string it) to avoid filter-injection.
         cutoff = (datetime.utcnow() - timedelta(minutes=STUCK_INDEXING_THRESHOLD_MINUTES)).isoformat()
         try:
             result = self.client.table("repositories").update(
@@ -128,10 +134,12 @@ def try_set_indexing_status(self, repo_id: str) -> bool:
                 f"status.neq.indexing,indexing_started_at.is.null,indexing_started_at.lt.{cutoff}"
             ).execute()
         except Exception as e:
-            # Degrade gracefully if migration 003 hasn't been applied yet: fall back to the
-            # original atomic compare-and-set (no steal-on-stale) so indexing still works.
+            # Only fall back when migration 003 (indexing_started_at) is missing -- detected by the
+            # column name in the DB error. Re-raise anything else so a real failure isn't masked.
+            if "indexing_started_at" not in str(e):
+                raise
             logger.warning(
-                "try_set_indexing steal path failed; falling back to basic CAS (apply migration 003)",
+                "indexing_started_at column missing; falling back to basic CAS (apply migration 003)",
                 repo_id=repo_id, error=str(e),
             )
             result = self.client.table("repositories").update(

backend/tests/test_durable_repo_state.py

@@ -169,14 +169,32 @@ def test_try_set_indexing_falls_back_to_basic_cas_when_column_missing(self):
         """Steal path errors without the column -> fall back to the original atomic CAS."""
         svc = self._service()
         eq_chain = svc.client.table.return_value.update.return_value.eq.return_value
-        eq_chain.or_.return_value.execute.side_effect = Exception("PGRST204 column missing")
+        eq_chain.or_.return_value.execute.side_effect = Exception(
+            "PGRST204 Could not find the 'indexing_started_at' column in schema cache"
+        )
         fallback = MagicMock()
         fallback.data = [{"id": "r1"}]
         eq_chain.neq.return_value.execute.return_value = fallback
 
         assert svc.try_set_indexing_status("r1") is True
         eq_chain.neq.assert_called_once_with("status", "indexing")
 
+    def test_update_status_reraises_unrelated_error(self):
+        """A non-migration error must NOT be masked by the missing-column fallback."""
+        svc = self._service()
+        chain = svc.client.table.return_value.update.return_value.eq.return_value
+        chain.execute.side_effect = Exception("connection reset by peer")
+        with pytest.raises(Exception, match="connection reset"):
+            svc.update_repository_status("r1", "indexing")
+
+    def test_try_set_indexing_reraises_unrelated_error(self):
+        """A non-migration error must NOT trigger the basic-CAS fallback."""
+        svc = self._service()
+        eq_chain = svc.client.table.return_value.update.return_value.eq.return_value
+        eq_chain.or_.return_value.execute.side_effect = Exception("connection reset by peer")
+        with pytest.raises(Exception, match="connection reset"):
+            svc.try_set_indexing_status("r1")
+
 
 class TestRouteWiring:
     """Integration: a route actually invokes ensure_clone (guards against the guard being removed)."""