Fix O(N) query regression in bulk folder operations (#1199)

[JSv4]

Summary

Optimize move_documents_to_folder() and delete_folder() in DocumentFolderService to reduce database queries from ~3 per document to 2 total queries per batch operation. This fixes a performance regression where bulk operations issued hundreds of queries instead of leveraging batch operations.

Key Changes

• Batch path disambiguation: Replace per-document _disambiguate_path() queries with a single pre-fetch of occupied paths in the target directory, shared across all documents in the batch. Within-batch conflicts are resolved in-memory by mutating the shared set as each path is claimed.

• Batch database writes: Replace individual save(update_fields=["is_current"]) and DocumentPath.objects.create() calls with two bulk operations:

  • Single DocumentPath.objects.filter().update(is_current=False) for all superseded paths
  • Single DocumentPath.objects.bulk_create() for all new successor paths

• Signal dispatch for bulk_create: Since bulk_create() bypasses per-row post_save signals, manually dispatch them via new _dispatch_document_path_created_signals() to preserve the document-text embedding side effect.

• Query optimization helpers:

  • _fetch_occupied_paths_in_directory(): Single-query fetch of active paths in a directory (with regex filtering to avoid pulling entire subtrees)
  • _target_directory_string(): Normalize folder paths to the format expected by the fetch helper
  • Updated _disambiguate_path() to accept occupied_override parameter for pre-fetched sets

• Row-level locking refinement: Add of=("self",) to select_for_update() calls to scope locks to DocumentPath rows only, avoiding accidental locks on Document rows for the transaction duration.

Implementation Details

• The batch path planning phase pre-computes all target paths and detects within-batch conflicts before any database writes occur
• Occupied paths are tracked in a mutable set that grows as each document claims its disambiguated path, ensuring siblings with identical filenames receive unique suffixes
• All changes occur within existing transaction.atomic() blocks, preserving rollback semantics
• Test updates reflect the shift from sequential to batch processing (e.g., extra_occupied → occupied_override parameter)

https://claude.ai/code/session_01Wp8voAR22aHiENmipX9jjL

[claude]

Code Review

The optimization is well-conceived and correctly implemented. The shift from ~2N round-trips to 4 total queries for a bulk operation is the right approach. A few things worth looking at before merge:

---

1. _fetch_occupied_paths_in_directory — regex vs. index-friendly prefix scan

path__regex=r"^/[^/]+$" and path__regex=rf"^{re.escape(directory)}[^/]+$" are correct but PostgreSQL can't use a btree index for these patterns. A prefix scan would be index-eligible:

# root case
qs = qs.filter(path__startswith="/").exclude(path__contains="/", path__regex=r"^/.+/.+$")
# OR simply:
qs = qs.filter(path__regex=r"^/[^/]+$")  # current — correct but seq-scan on path

A more index-friendly alternative for the non-root case:

# Uses the index for the prefix, then filters the suffix in-memory
qs = qs.filter(path__startswith=directory).extra(
    where=["path NOT LIKE %s"],
    params=[directory + "%/%"],
)

Or, since path always starts with /, you can count slashes:

from django.db.models.functions import Length, Replace
qs = qs.filter(path__startswith=directory).annotate(
    slash_count=Length("path") - Length(Replace("path", Value("/"), Value("")))
).filter(slash_count=directory.count("/"))

For typical corpus sizes this is unlikely to matter, but if a corpus has thousands of files at root the regex forces a sequential scan on the path column. Worth at least adding a comment acknowledging this, or adding a GinIndex / pg_trgm index if the project doesn't have one already.

---

2. Test name vs. new semantics — test_delete_folder_rolls_back_successful_relocations_on_later_failure

Before this PR, the failure occurred mid-write (after the first document was already persisted), so the test genuinely verified rollback of partial writes. With the new two-phase approach, _disambiguate_path is called during the planning phase — before any DB writes — so the ValueError on call 2 propagates before filter().update() or bulk_create() ever runs.

The test still passes and the observable end-state is the same (no rows changed), but the test name and docstring now describe behavior that no longer occurs. Suggesting a rename/docstring update:

test_delete_folder_rolls_back_on_planning_failure

and update the docstring to reflect that the failure now happens in the path-planning phase, before any writes.

---

3. _dispatch_document_path_created_signals — implicit update_fields omission

post_save.send() is called without update_fields. For created=True this defaults to None, which matches what a regular create() would send. The process_doc_on_document_path_create handler doesn't inspect update_fields, so this is fine in practice. It's a minor doc/convention gap but not a bug.

---

4. occupied_override mutation contract — worth a comment

In _disambiguate_path the parameter is used as a read-only snapshot:

occupied = occupied_override  # direct alias, not a copy

The callers then mutate occupied_paths after the call returns. If a future change were to accidentally mutate occupied inside the function (e.g. during the suffix loop), callers would see unexpected side-effects. A short contract comment would help:

# ``occupied_override`` is treated as read-only inside this method;
# callers are responsible for appending the returned path to the set.
occupied = occupied_override

---

Confirmed correct

• from __future__ import annotations (line 36) defers evaluation of the list[DocumentPath] type hint in _dispatch_document_path_created_signals, so the nested import correctly avoids the circular dependency without a NameError at class definition time. ✓

• Signal handler N+1: DocumentPath(document=current.document, ...) in the list comprehension populates _document_cache from the pre-fetched current.document (which came via select_related). instance.document in the signal handler resolves from cache, not a new query. ✓

• transaction.on_commit inside atomic block: Embedding tasks are queued via on_commit in the handler, so they fire only when the outermost transaction commits — same semantics as the old per-row create(). ✓

• bulk_create PK population on PostgreSQL: Django uses RETURNING id on PostgreSQL, so the instances in created_paths have PKs set and instance=path in the signal dispatch is fully populated. ✓

• Row-lock scoping of=("self",): Correctly limits SELECT FOR UPDATE to the DocumentPath table, preventing accidental locks on joined Document rows. ✓

• IntegrityError tests updated: Mocking bulk_create instead of create is the correct change given the new write path. ✓

---

Overall this is a clean, well-documented optimization. The three items above are minor — the regex index efficiency note is the only one I'd consider before shipping on a large-dataset instance.

[codecov]

Codecov Report

❌ Patch coverage is 98.46154% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
opencontractserver/corpuses/folder_service.py	98.46%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[claude]

PR Review: Fix O(N) query regression in bulk folder operations

This is a well-executed performance fix. The architecture is sound, the comments are thorough, and the batch approach correctly eliminates the O(N) round-trip regression. A few things worth addressing before merge:

---

Medium — move_single_document_to_folder not updated to of=("self",)

The bulk methods received select_for_update(of=("self",)) to scope row locks to DocumentPath only, but move_single_document_to_folder at line 998 still uses the unscoped select_for_update(), which would accidentally hold a lock on the joined Document row for the duration of its savepoint. Should be consistent:

# folder_service.py ~line 997-998
current = (
    DocumentPath.objects.select_for_update(of=("self",))  # add of=("self",)
    ...
)

---

Medium — _dispatch_document_path_created_signals omits raw and using kwargs

Django's Model.save() dispatches post_save with these kwargs:

post_save.send(sender=origin, instance=self, created=..., update_fields=..., raw=raw, using=using)

The manual dispatch omits raw and using. The current handler absorbs them via **kwargs, so nothing breaks today, but any future handler that inspects raw (e.g. a data-import guard) or using (multi-DB routing) would receive wrong values silently. Consider:

for path in paths:
    post_save.send(
        sender=DocumentPath,
        instance=path,
        created=True,
        raw=False,
        using=path._state.db,
    )

---

Low — _fetch_occupied_paths_in_directory empty-string fallback loads all corpus paths

When directory == "" (i.e., base_path has no /), the method queries is_current=True, is_deleted=False with no path filter — pulling the entire active path set for the corpus into memory. The docstring acknowledges this as a "rare fallback," but paths without a leading / appear to be structurally invalid in this codebase (every stored path starts with /). A defensive guard or assertion would make the invariant explicit and prevent a silent full-table scan if a malformed path ever reaches this code:

if not directory:
    # base_path had no leading slash — structurally unexpected.
    # Fall through to the unfiltered query rather than crashing,
    # but log a warning so we can catch regressions early.
    logger.warning("_fetch_occupied_paths_in_directory called with empty directory for corpus %s", corpus.id)

---

Low — Test coverage gap: signal dispatch after bulk_create

The test suite verifies rollback scenarios and path disambiguation for the new batch path, but I don't see a test that asserts _dispatch_document_path_created_signals (or the downstream calculate_embedding_for_doc_text.delay call) is actually invoked after a successful bulk move. The old code got this for free via Django's native post_save; the new manual dispatch is a new code path that could silently regress. A test that mocks post_save.send (or the Celery task) and asserts it was called N times would be good insurance.

---

Nit — _target_directory_string docstring example

- Folder ``Legal/Contracts`` → ``"/Legal/Contracts/"``

Should mention that the input comes from get_path() which returns a path already stripped of leading/trailing slashes (or link to the get_path() contract), since the .strip("/") call is load-bearing here and it's not obvious why it's needed.

---

Positive callouts

• The re.escape(directory) in the regex filter is the right call — prevents injection from user-controlled folder names.
• The select_related("document") + of=("self",) pairing is clean; it avoids the N+1 on current.document in the build loop while keeping the lock footprint minimal.
• The in-memory occupied_paths.add(new_path) mutation for within-batch sibling disambiguation is well-explained and correctly mirrors the old batch_claimed approach.
• Test updates accurately reflect the semantic shift from sequential→batch (the fail_on_second mock using **kwargs pass-through is clean).
• Deleting the TODO(perf) comments that this PR resolves is the right move per the "no dead code" rule in CLAUDE.md.

[claude]

Code Review

This is a well-executed optimization. The core approach — pre-fetching occupied paths once, planning all moves in-memory, then executing two bulk queries — is correct and the implementation is clean. A few items worth addressing before merge:

---

Bug / Correctness

1. _target_directory_string edge case with empty get_path() return

# _target_directory_string
folder_path = target_folder.get_path().strip("/")
return f"/{folder_path}/"

If CorpusFolder.get_path() ever returns an empty string (e.g. a root-level folder that isn't None), strip("/") produces "" and the result is "//". _fetch_occupied_paths_in_directory would then fall into the elif directory: branch and apply regex ^//[^/]+$, matching no real paths — the batch move would silently succeed with no conflict detection. Worth adding an assertion or guard:

if not folder_path:
    raise ValueError(f"CorpusFolder {target_folder.pk} returned empty path from get_path()")

---

Test Coverage Gap

2. No test for delete_folder signal dispatch

test_bulk_move_dispatches_post_save_for_each_created_path only covers move_documents_to_folder. The delete_folder method also calls _dispatch_document_path_created_signals (line 132 of the diff), but there's no corresponding test. Given that the signal dispatch is the primary side-effect safety net for document-text embedding, this path deserves the same coverage.

---

Minor Issues

3. directory == "" fallback loads ALL active paths

In _fetch_occupied_paths_in_directory, the empty-directory fallback:

else:
    logger.warning(...)
    # falls through with no filter — loads all active paths in corpus

The warning is good, but a corpus with tens of thousands of documents could hit an OOM or very slow query if this path is ever triggered. Consider raising instead of warning, or at minimum adding a hard filter like qs = qs.none() since this state is documented as "structurally unexpected":

else:
    logger.error(...)
    raise ValueError(f"_fetch_occupied_paths_in_directory: empty directory for corpus {corpus.id}")

4. occupied_override mutation contract is implicit

The docstring says occupied_override is "treated as read-only inside this method", but callers immediately mutate it with occupied_paths.add(new_path) after each call. This shared-mutable-set contract is unusual. It works correctly, but a comment at the call site clarifying the invariant (rather than just in the method docstring) would help future readers:

new_path = cls._disambiguate_path(new_path, corpus, occupied_override=occupied_paths)
occupied_paths.add(new_path)  # Claim path so next sibling in batch resolves to a different suffix

(The comment at line 208 already does this — just flagging it's worth keeping/not losing in future refactors.)

---

Positive Notes

• select_for_update(of=("self",)) is an excellent improvement — the original lock on joined rows could deadlock with concurrent operations on the Document table.
• The select_related("document") avoidance of N+1 in the planning loop is clean.
• Renaming test_delete_folder_rolls_back_successful_relocations_on_later_failure → test_delete_folder_rolls_back_on_planning_failure is accurate — the semantics genuinely changed (no writes happen before the rollback now).
• The regex injection safety via re.escape(directory) is correctly handled.
• Manual post_save dispatch arguments (raw=False, using=path._state.db, omitted update_fields) correctly replicate Django's native Model.save() dispatch semantics.

---

Summary

Blocking: item 1 (silent correctness failure on empty get_path()) and item 2 (missing test for delete_folder signal dispatch). Items 3 and 4 are suggestions rather than blockers, but 3 in particular would prevent a hard-to-diagnose OOM in production.

[claude]

Review: Fix O(N) query regression in bulk folder operations (#1199)

This is a well-executed optimization. The batch architecture is correct, the concurrency documentation is thorough, and the test expansion is meaningful. A few items worth addressing.

---

Bug: missing update_fields in post_save.send()

File: opencontractserver/corpuses/folder_service.py, lines 1387-1394

Django's Model.save() always dispatches post_save with update_fields set explicitly (None for creates). The manual dispatch in _dispatch_document_path_created_signals omits it:

# current
post_save.send(
    sender=DocumentPath,
    instance=path,
    created=True,
    raw=False,
    using=path._state.db,
)

The inline comment says "update_fields is intentionally omitted (defaults to None)", but omitting the key is not the same as passing update_fields=None. Any signal handler that declares update_fields as an explicit keyword argument without a default will raise TypeError. The current process_doc_on_document_path_create handler uses **kwargs so it survives, but this creates a silent compatibility trap for future handlers.

Fix — add the missing kwarg:

post_save.send(
    sender=DocumentPath,
    instance=path,
    created=True,
    update_fields=None,   # matches Django Model.save() dispatch for creates
    raw=False,
    using=path._state.db,
)

The two signal dispatch tests should also assert kwargs.get("update_fields") is None to lock this in.

---

Missing test: delete_folder + IntegrityError during bulk_create

TestCoverageGap_BulkMoveIntegrityErrorRollback covers the move_documents_to_folder path. delete_folder uses the identical bulk_create -> signal dispatch pattern (lines 918-919) but has no equivalent test. The planning-phase rollback test (test_delete_folder_rolls_back_on_planning_failure) covers failures before any DB writes; a complementary test that patches DocumentPath.objects.bulk_create to raise IntegrityError after the filter().update(is_current=False) call would verify that the outer transaction.atomic() correctly undoes the deactivation update too.

---

Minor: _target_directory_string error path has no test

The ValueError raised when a non-root CorpusFolder.get_path() returns an empty string (lines 1353-1356) is not exercised anywhere. A short unit test that mocks get_path to return "" would complete the contract.

---

Observations (no action required)

Regex index note is accurate but worth a follow-up ticket: The comment in _fetch_occupied_paths_in_directory correctly flags that path__regex can't use a btree index on path. For typical corpus sizes this is fine; opening a tracking issue for the GIN/pg_trgm path would keep it from being forgotten.

In-memory sync after filter().update(): The loop that sets current.is_current = False in memory (lines 1221-1222) is a good defensive pattern. It only syncs the objects in planned_paths; the original current_paths list (which includes already-in-target skipped entries) is not touched — but that's fine since the method returns immediately after.

Concurrency documentation: The _disambiguate_path docstring accurately describes the TOCTOU window and the unique constraint as the true safety net. This is exactly the right way to document this class of race condition.

target_folder_path pre-resolution: Hoisting folder.get_path() out of the inner loop (line 1188) correctly avoids O(N) recursive CTE queries. Good catch.

[claude]

PR Review: Fix O(N) query regression in bulk folder operations (#1199)

This is a solid performance fix with well-thought-out batching semantics. Here are my observations:

---

Strengths

Query reduction is well-targeted. The two-query batch pattern (filter().update() + bulk_create()) is the right approach for this use case. The move from ~3 queries/document to ~4 queries total for any batch size is a genuine O(N)→O(1) reduction.

Signal dispatch is correctly preserved. Manually firing post_save via _dispatch_document_path_created_signals() correctly compensates for bulk_create's signal bypass. The kwargs (created=True, raw=False, update_fields=None, using=path._state.db) match what Django's Model.save() would emit for a new row.

In-memory occupancy tracking is sound. Using a shared mutable set that grows as each path is claimed (within the planning loop) correctly handles same-batch filename conflicts without extra DB round-trips.

of=("self",) on select_for_update is a good defensive fix—scoping the row lock to DocumentPath prevents accidental locks on joined Document rows.

Test coverage is thorough. New tests cover: signal dispatch count, rollback on IntegrityError during bulk_create, ValueError on empty get_path(), and the behavior change for slashless paths.

---

Issues & Concerns

1. Behavior break: slashless paths now raise instead of silently succeeding

In _disambiguate_path, when occupied_override is None and the path has no /, the old code set directory = "" and matched all active paths (a wide but non-crashing fallback). The new code delegates to _fetch_occupied_paths_in_directory which explicitly raise ValueError for directory == "".

The test class TestCoverageGap_DisambiguateNoSlashPath has been updated to assert the new ValueError behavior—but this is a semantic change that the PR description does not highlight. If any caller can reach _disambiguate_path with a slashless path at runtime (e.g. a document with a legacy stored path missing the leading /), that caller now gets an unhandled exception rather than a graceful degradation.

Recommendation: Add a guard in callers or at the top of _disambiguate_path that asserts/normalizes the leading slash, and document this invariant clearly (it's partially noted in the docstring already).

2. Two redundant static methods doing the same thing

_target_directory_string(folder: CorpusFolder | None) and _target_directory_string_from_path(folder_path: str | None) are nearly identical:

# _target_directory_string
if target_folder is None:
    return "/"
folder_path = target_folder.get_path().strip("/")
...
return f"/{folder_path}/"

# _target_directory_string_from_path
if folder_path is None:
    return "/"
stripped = folder_path.strip("/")
...
return f"/{stripped}/"

The only difference is whether the caller hands in a CorpusFolder or a pre-resolved path string. This duplicates logic and two error messages. Per the project's DRY principle, a single method with an optional pre-resolved path parameter would be cleaner—or simply inline _target_directory_string_from_path at its one call site in move_documents_to_folder (it's a 3-line transformation).

3. _fetch_occupied_paths_in_directory uses unindexed regex scans

The docstring calls this out, but it's worth underscoring: path__regex=r"^/[^/]+" and path__regex=rf"^{re.escape(directory)}[^/]+$" cannot use a btree index on path. For any corpus with thousands of files, every call to this helper does a full table scan of DocumentPath filtered to (corpus, is_current=True, is_deleted=False).

The comment suggests a GIN/pg_trgm index as a future improvement. Given this is a performance fix, it's worth leaving a concrete TODO(index) comment or opening a follow-up issue so this doesn't silently become the new bottleneck for large corpora.

4. Signal dispatch test is fragile—it patches post_save at the module level

@patch("opencontractserver.corpuses.folder_service.post_save")
def test_delete_folder_dispatches_post_save_for_each_created_path(self, mock_signal):

Patching post_save at the module reference level is correct, but if _dispatch_document_path_created_signals is ever moved to a utility module or the import path changes, these tests will silently stop verifying the real dispatch (they'd patch a different name). This is low-risk today, but worth noting.

Also, the call.kwargs extraction logic inside the assertion loop is duplicated verbatim between the two new signal-dispatch tests:

kwargs = call.kwargs if call.kwargs else {}
if not kwargs and len(call) > 1:
    kwargs = call[1]

This would be worth extracting to a helper in the test class.

5. moved_count = len(created_paths) changes semantics slightly

Previously moved_count was incremented inside the per-document loop (i.e. it counted successful individual moves). Now it equals len(created_paths). Since bulk_create returns all inserted rows (including any that might have been filtered by update_conflicts—though that flag isn't used here), this is functionally equivalent. Just noting this in case bulk_create behavior changes or update_conflicts gets added later.

6. Minor: in-memory sync after .update() iterates twice over planned_paths

DocumentPath.objects.filter(pk__in=old_path_pks).update(is_current=False)
for current, _ in planned_paths:
    current.is_current = False

This double-iteration (once to build old_path_pks, once to sync in-memory) is fine for correctness, but the comment says "Keep in-memory instances in sync… so that callers relying on current.is_current after this method returns see the correct value without a refetch." In practice, current instances aren't returned to callers—only moved_count and an error string are. This sync is defensive but documented reasoning could note it's for signal handlers that might inspect the instance during _dispatch_document_path_created_signals.

---

Minor Nits

• The _dispatch_document_path_created_signals docstring says "Nested import to avoid circular dependency during app initialization"—this import is inside the method body, which is fine, but the comment would be more accurate if it noted this is a runtime circular import guard (not just initialization).
• planned_paths: list[tuple] = [] # (current, new_path) — worth typing this as list[tuple[DocumentPath, str]] for clarity.

---

Summary

The core optimization is correct, well-tested, and clearly explained. The main issues worth addressing before merge are:

1. The slashless-path behavior change (now raises instead of degrading gracefully) should be called out explicitly and any legacy path data validated.
2. The two near-identical directory-string helpers violate DRY and should be consolidated.
3. A follow-up issue for the regex index gap should be filed or the TODO made more actionable.

Everything else is minor. Good work on the batching logic and thorough signal-dispatch testing.

[JSv4]

Review feedback addressed

Merged main into the PR branch and assessed the four review items:

1. Slashless paths raising from wrong location (FIXED)

Valid concern, addressed. Added an early startswith("/") guard at the top of _disambiguate_path so callers that pass slashless paths get a clear, localized error message rather than a confusing ValueError from _fetch_occupied_paths_in_directory. Also removed the now-unreachable directory = "" dead code branch.

In practice, all current callers pass paths from _compute_moved_path, which always returns paths starting with /, so this is defense-in-depth for future callers.

2. Two redundant static methods (FIXED)

Valid concern, addressed. Consolidated _target_directory_string to delegate to _target_directory_string_from_path instead of duplicating the path-normalization logic. The _from_path variant is the canonical implementation; the folder-accepting variant is now a thin wrapper that calls get_path() then delegates.

3. move_single_document_to_folder not updated to of=("self",) (ALREADY ADDRESSED)

Not a real issue. The single-document method move_document_to_folder at line 998 already uses select_for_update(of=("self",)). This was present before the review.

4. Missing update_fields in post_save.send() (ALREADY ADDRESSED)

Not a real issue. The signal dispatch in _dispatch_document_path_created_signals already passes update_fields=None, which is correct for created=True signals — this matches Django's own Model.save() behavior for newly created instances.

[claude]

Review: Fix O(N) query regression in bulk folder operations (#1237)

The core optimization is sound. Replacing ~3N DB round-trips with ~4 total queries using bulk_create + filter().update() is the right approach, the batch path-planning phase correctly resolves within-batch filename conflicts in memory, and the select_for_update(of=("self",)) scoping is a good tightening of the lock strategy. Here are a few things worth addressing before merge:

---

Bug / Dead Code

_target_directory_string is unreachable production code (folder_service.py:1363–1378)

The new _target_directory_string(target_folder) wrapper is never called by production code. move_documents_to_folder calls _target_directory_string_from_path(target_folder_path) directly (after resolving get_path() once), and delete_folder hardcodes "/" without going through either helper. The method is only exercised by two test assertions in TestCoverageGap_TargetDirectoryStringEmptyPath. Per the "No dead code" rule in CLAUDE.md, this should either be removed or actively used in the call sites that comment about it.

Note: there's also a stale comment at folder_service.py:1172 that says "We reuse the same value for both _target_directory_string and _compute_moved_path" — but _target_directory_string is not actually called there.

---

Correctness Concern

TOCTOU blast radius is now larger with bulk_create

The docstring correctly notes the TOCTOU race: a concurrent insert can claim a path between the occupied-paths pre-fetch and the bulk_create. Previously, per-row DocumentPath.objects.create() calls meant a race only caused the one conflicting document to raise IntegrityError, which could be handled with a savepoint and retry. Now, a single constraint violation in bulk_create rolls back the entire batch. For most workloads this is fine — the caller retries — but it's a behavioral change worth calling out explicitly in the docstring, since the old code's partial-success semantics are gone.

---

Minor Issues

Imprecise type annotation (folder_service.py:881, 1190)

planned_paths: list[tuple] = []  # (current, new_path)

This could be list[tuple[DocumentPath, str]]. The comment explains the tuple shape, but the type is not enforced or communicated to tooling.

---

Fragile call_args inspection in signal dispatch tests (test_document_folder_service.py:2601–2608)

kwargs = call.kwargs if call.kwargs else {}
if not kwargs and len(call) > 1:
    kwargs = call[1]

unittest.mock.call objects expose .args and .kwargs. The len(call) > 1 branch is accessing mock internals and will silently produce an empty dict if the call structure ever changes. A simpler and more robust pattern:

for call in send_calls:
    _, call_kwargs = call
    self.assertEqual(call_kwargs["sender"], DocumentPath)
    self.assertTrue(call_kwargs["created"])

Or just call.kwargs if all arguments are passed as keyword args (they are — post_save.send is called with all kwargs explicitly).

---

delete_folder inconsistency with _target_directory_string_from_path (folder_service.py:879)

occupied_paths = cls._fetch_occupied_paths_in_directory(corpus, "/")

Using _target_directory_string_from_path(None) here would mirror the move_documents_to_folder pattern and make the intent explicit that "/" means corpus root. Minor, but the helper exists for exactly this purpose.

---

What's Good

• Signal dispatch fires inside transaction.atomic(), so transaction.on_commit callbacks in process_doc_on_document_path_create are correctly deferred until the outer transaction commits. ✓
• re.escape(directory) in _fetch_occupied_paths_in_directory prevents regex injection from adversarial folder names. ✓
• The performance regression test (test_bulk_move_calls_get_path_once) that asserts get_path() is called exactly once is a great pattern for locking in this invariant. ✓
• In-memory is_current = False sync after the batch update is a clean defensive touch. ✓
• The btree-index caveat in _fetch_occupied_paths_in_directory docstring is a good proactive note for future maintainers. ✓