Merge pull request #2046 from Open-Source-Legal/claude/wonderful-keller-anbgrj

Address PR #2030 review nits on bulk_soft_delete_documents (#2036)

Author: JSv4

changelog.d/2036-bulk-soft-delete-review-polish.changed.md

@@ -0,0 +1,11 @@
+- **Hardened the bulk-trash audit log (#2036, follow-up to #1951).**
+  `DocumentLifecycleService.bulk_soft_delete_documents`
+  (`opencontractserver/corpuses/services/lifecycle.py`) now emits its
+  "Bulk soft-deleted N document(s)" log via `transaction.on_commit`, so the line
+  is written only for durably committed trashing (no false success on a
+  rolled-back or failed-`COMMIT` block) — matching the rollback-safety contract
+  the primitive's dispatched `post_save` signals already use. Also promoted the
+  in-body PERF/MEMORY note to a "Scaling caveat" in the method docstring: the
+  primitive is O(1) in queries but still materializes the full document set in
+  memory, so it carries no built-in document-count ceiling for the
+  `empty_corpus` / folder cascade-delete callers.

opencontractserver/corpuses/services/lifecycle.py

@@ -462,6 +462,13 @@ def bulk_soft_delete_documents(
         ``Corpus.remove_document`` produces, so trashed documents stay in the
         trash listing and remain restorable.
 
+        Scaling caveat — O(1) in *queries*, not in *memory*: this primitive
+        still materializes both ``document_ids`` and the locked ``active_paths``
+        fully into Python lists, so there is **no built-in document-count
+        ceiling**. That is fine for realistic corpus sizes, but chunk/iterate
+        the doc set here before exposing an unbounded ``empty_corpus`` / folder
+        cascade-delete to arbitrarily large corpora (memory follow-up to #1951).
+
         NOTE: This is an internal primitive that performs **no permission
         check** — callers (``empty_corpus``, folder cascade-delete) must already
         have verified corpus DELETE permission. It does not touch the folder
@@ -505,12 +512,10 @@ def bulk_soft_delete_documents(
             # absent here (skipped, never double-trashed) and one added after it
             # is left alone — the same semantics as the prior per-document loop.
             #
-            # PERF/MEMORY: both ``document_ids`` and ``active_paths`` are fully
-            # materialized in memory, bounded by the in-scope document count —
-            # which the current callers (empty_corpus, folder cascade-delete)
-            # leave unbounded. Fine for realistic corpus sizes; this is the
-            # allocation point to revisit (chunk/iterate the doc set) before
-            # raising any hard document-count ceiling on these paths (#1951).
+            # PERF/MEMORY: this ``list()`` (plus the caller's ``document_ids``
+            # snapshot) is the unbounded allocation point behind the docstring
+            # "Scaling caveat" — chunk/iterate here to add a document-count
+            # ceiling for the empty_corpus / cascade-delete callers (#1951).
             active_paths = list(
                 DocumentPath.objects.select_for_update(of=("self",))
                 .select_related("document")
@@ -600,10 +605,17 @@ def bulk_soft_delete_documents(
                         is_public=False
                     )
 
-        logger.info(
-            "Bulk soft-deleted %s document(s) in corpus %s by user %s",
-            len(trashed_doc_ids),
-            corpus.id,
-            user.id,
-        )
-        return len(trashed_doc_ids)
+            # Defer the audit log to on_commit so it reports only durably
+            # committed trashing (never a false "soft-deleted N" on a rolled-back
+            # block) — the rollback-safety contract the signals above rely on.
+            trashed_count = len(trashed_doc_ids)
+            corpus_id, user_id = corpus.id, user.id
+            transaction.on_commit(
+                lambda: logger.info(
+                    "Bulk soft-deleted %s document(s) in corpus %s by user %s",
+                    trashed_count,
+                    corpus_id,
+                    user_id,
+                )
+            )
+            return trashed_count