Closes #21326: Defer global search cache updates to a background job

[jnovinger]

Closes: #21326

Updates to the global search cache (CachedValue) currently happen synchronously inside the post_save/post_delete signal handlers, adding latency to every write. This defers that work to a background job that runs after the response is sent.

• Search-cache updates are coalesced per transaction and dispatched on commit via a new SearchCacheJob, rather than running inline in the signal handlers.
• When no RQ worker is available, indexing falls back to running synchronously, so installs without a worker behave as before.
• Bulk operations enqueue a single job for the whole transaction instead of one update per object.
• The originating database alias is captured from the save/delete and replayed on the deferred write, keeping cache entries correctly schema-routed under netbox-branching without core depending on the plugin.
• docs/features/search.md is updated to note that search results are now eventually consistent (a newly created or changed object may not appear for a brief moment).

I'll follow up with a comment covering the design details (the coalescing buffer, the alias capture/replay, and the worker-gated fallback).

[jnovinger]

Some additional design context for reviewers (and the bot). Collapsed since it's deeper than most reviews will need.

Design details

Coalescing buffer

netbox/netbox/search/deferred.py buffers dirty objects per (database alias, transaction). The buffer dict lives inside the closure of the transaction.on_commit callback that will flush it, and the callback is tagged so subsequent mark_dirty() calls in the same transaction find and extend the existing batch rather than scheduling another flush. The result is one flush per alias per transaction: a 5,000-row bulk import enqueues a single job, not 5,000.

Coalescing collapses repeated operations on the same object, and a deletion supersedes a pending create/update (the object ends up removed from the index).

Rollback safety

Django clears a connection's run_on_commit list on both commit and rollback. Because the batch only exists inside a scheduled callback's closure, a rolled-back transaction's buffered objects are discarded with the callback: there is no thread- or module-level state that can leak a rolled-back object into a later transaction's flush. A commit on the same connection after a rollback indexes normally.

In autocommit (no open transaction), on_commit runs its callback immediately at registration, so mark_dirty() indexes inline rather than scheduling a flush that would fire before the batch is populated.

Schema routing without a plugin dependency

netbox-branching routes CachedValue writes to a per-branch Postgres schema via a database router that reads a context variable at query time. Deferred work runs after that context is gone (and, on a worker, in a separate process), so the deferral would otherwise write to the main schema.

Rather than have core know about branching, the database alias is captured from the signal's using kwarg (the alias Django resolved for the write) and replayed on the deferred read and write via .using(alias). QuerySet.using() overrides the router, so the entries land in the originating schema regardless of routing context. Only primitives cross the job boundary: the alias string and {object_type_id: [pk]} maps. The worker re-resolves the model and re-fetches live instances on that alias. Cross-schema correctness is covered by netbox-branching's own routing test suite; this PR's tests assert only that the alias is captured and forwarded.

Worker-gated fallback

On flush, if a default-queue worker is registered the work is dispatched as a SearchCacheJob; otherwise it runs inline. Installs without a running worker keep today's synchronous behavior, so this is not a hard dependency on having a worker.

Why a JobRunner rather than a bare queue.enqueue

The deferred work is dispatched as a SearchCacheJob(JobRunner) rather than a raw queue.enqueue('module.func', ...). The raw-enqueue form exists in core only for webhooks (extras/events.py), and predates the JobRunner framework; the documented mechanism for decoupling work from the request/response cycle (including housekeeping and synchronization jobs) is JobRunner. Coalescing keeps the volume reasonable (one job per alias per transaction), so this doesn't create a Job row per object.

[jnovinger]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

[github-actions]

Claude finished @jnovinger's task in 2m 49s —— View job

---

Review

• Read changed files
• Analyze for issues of concern
• Post findings

---

1. Redis outage turns a successful save into a 500

_flush() calls any_workers_for_queue() (which opens a Redis connection) inside the on_commit callback. The callback is registered without robust=True, so any ConnectionError propagates back to the HTTP handler — causing a 500 even though the database write already committed. Before this PR, search-cache updates had no Redis dependency in the signal path.

netbox/netbox/search/deferred.py:63

transaction.on_commit(flush, using=alias)
# Should be:
transaction.on_commit(flush, using=alias, robust=True)

robust=True (Django 4.1+) logs the exception rather than propagating it, which matches the intent of graceful degradation already expressed in the worker-fallback logic. Fix this →

---

2. removal_handler is missing ProgrammingError handling

caching_handler wraps ObjectType.objects.get_for_model() in a ProgrammingError guard (for incomplete schemas during migrations), but removal_handler does not. Deletes can fire during migrations too.

netbox/netbox/search/backends.py:98

object_type = ObjectType.objects.get_for_model(indexer.model)  # unguarded

Fix this →

---

3. connection.run_on_commit internal structure

_pending_batch() unpacks each entry as (_sids, func, _robust) — a 3-tuple that is Django's internal representation, not a documented API. This is the only viable mechanism for finding an existing callback without introducing module-level state, so there may not be a better option, but it's worth flagging as a fragile dependency that could silently break on a Django upgrade.

netbox/netbox/search/deferred.py:83

for _sids, func, _robust in connection.run_on_commit:

---

4. SearchCacheJob creates a Job DB row per committed transaction

Job.enqueue() calls job.save(), so every transaction that touches a searchable model (when a worker is running) creates a persistent Job row. The design notes acknowledge this is one row per transaction rather than one per object, but on a busy instance this could produce a large number of short-lived Job records. It's worth considering whether a bare queue.enqueue() (bypassing the Job ORM record) would be more appropriate here, given that these housekeeping jobs have no meaningful object association or user-visible state — unlike the rest of the Job framework users.

[jnovinger]

1. Redis outage turns a successful save into a 500

Fixed. The on_commit flush is now registered with robust=True, so a Redis failure while checking for an available worker is logged instead of propagating. A committed save will never 500 on a search-cache update.

2. removal_handler is missing ProgrammingError handling

Fixed. removal_handler now wraps the get_for_model() lookup in the same ProgrammingError guard as caching_handler.

3. connection.run_on_commit internal structure

Agreed it's a Django internal with no public alternative (short of module-level state, which we avoid for rollback safety). Added a test that asserts the (savepoint_ids, func, robust) shape directly, so a change in a future Django release fails with a message pointing at _pending_batch() rather than an opaque unpack error.

4. SearchCacheJob creates a Job DB row per committed transaction

Staying as-is. JobRunner is the documented mechanism for decoupling work from the request/response cycle; the bare queue.enqueue() form exists in core only for webhooks and predates the JobRunner framework. Coalescing holds this to one job per transaction (not per object), and expired Job records are already pruned by the housekeeping job. Our view is that consistency with the job framework is worth the row; if row volume proves to be a problem on large instances, narrowing it is a reasonable follow-up.

[jnovinger]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration. See previous comments for review response.

[github-actions]

Claude finished @jnovinger's task in 3m 18s —— View job

---

Review

• Read changed files
• Analyze for remaining issues of concern
• Post findings

---

Previous issues #1 and #2 are correctly fixed. Issue #3 is acknowledged with a test. Issue #4 is staying as-is (noted).

One remaining concern:

---

Autocommit path lacks the same Redis-error protection as the transactional path

mark_dirty() has two branches:

• Transactional (connection.in_atomic_block is True): registers an on_commit callback with robust=True, so a Redis ConnectionError inside _flush() (e.g. from any_workers_for_queue()) is logged and swallowed — the committed write is never turned into a 500.
• Autocommit (connection.in_atomic_block is False): calls _flush() directly with no error handling. A Redis ConnectionError from the same any_workers_for_queue() call propagates through mark_dirty() → the signal handler → Django's signal.send() → the save/delete caller.

any_workers_for_queue opens a real Redis connection (get_connection(queue_name) + Worker.all_keys()); if Redis is unreachable it raises. This path was not present before this PR, so this introduces a new Redis dependency in the autocommit post_save/post_delete path.

netbox/netbox/search/deferred.py:46–48

if not connection.in_atomic_block:
    _flush({(object_type_id, pk): op}, alias)
    return

Suggested fix — wrap the autocommit flush to match the transactional path's intent:

if not connection.in_atomic_block:
    try:
        _flush({(object_type_id, pk): op}, alias)
    except Exception:
        logger.exception("Error flushing search cache (autocommit)")
    return

Fix this →

[jnovinger]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

Since the last review, the autocommit-path Redis gap you flagged is fixed: _flush is now the single guarded dispatcher, wrapping the worker probe and the enqueue together in a try/except RedisError with an inline fallback, and the autocommit branch of mark_dirty() is guarded as well. The transactional path keeps robust=True as defense in depth. See the latest commit message ("Harden deferred search cache error handling") and the collapsed design-details comment above for the full rationale, including why the removal loop isn't wrapped in a transaction and the recover-via-reindex (no retry) failure model.

[github-actions]

Claude finished @jnovinger's task in 4m 5s —— View job

---

Review

• Read changed files
• Analyze latest changes for remaining issues
• Post findings

---

All previous issues are resolved. No remaining concerns.

The three structural fixes hold up:

• Single guarded dispatcher: _flush() wraps the worker probe and enqueue together in try/except RedisError, and falls through to inline indexing on any Redis failure. Both callers add a second layer (robust=True on the transactional path, except Exception on the autocommit path), so a non-Redis exception from enqueue() — e.g. a DatabaseError saving the Job row — is logged and swallowed rather than propagated. The object won't be indexed in that case, but that's consistent with the stated recover-via-reindex failure model, and such a failure would indicate broader DB trouble.
• removal_handler ProgrammingError guard: present and symmetric with caching_handler.
• run_on_commit tuple shape test: correctly asserts the (savepoint_ids, func, robust) structure with a pointer back to _pending_batch().

The psycopg sqlstate attribute in _is_missing_schema is correct for psycopg3, which is the only supported driver (psycopg[c,pool]==3.3.4).

[jnovinger]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

Since the last review, a few follow-up items identified during local review have been addressed:

• Reduced the deferral's nested imports to a single, genuinely cycle-bound one (_flush's import of netbox.search.tasks); everything else is now top-level. The remaining nested import is commented with the cycle and a pointer to a tracked follow-up.
• Renamed mark_dirty() to mark_for_deferred_indexing() for clarity about what it does.
• Expanded the robust=True comment on the on_commit flush to record why it's required (an exception escaping the post-COMMIT callback would otherwise become a 500 on an already-committed write).

Full detail is in the commit messages and the collapsed design-details comment above; please consult them as needed.

[github-actions]

Claude finished @jnovinger's task in 2m 18s —— View job

---

Review

• Read changed files
• Analyze latest changes for remaining issues
• Post findings

All previous issues have been resolved. The three changes described in the trigger comment are all correct:

• Import reduction: _flush now has exactly one nested import — the genuine cycle (netbox.search.tasks → netbox.search.backends → netbox.search.deferred → netbox.search.tasks) — and everything else is top-level. The remaining nested import is commented with the cycle explanation and a pointer to a follow-up.
• Rename: mark_dirty → mark_for_deferred_indexing is complete. No stale references remain anywhere in the codebase.
• robust=True comment: The expanded comment correctly records both that robust=True is required (not just defensive) and why — an exception escaping a post-COMMIT callback propagates out of the atomic block and becomes a 500 on an already-committed write.

No remaining concerns.

[pheus]

Thanks! The direction looks good to me.

I do think this needs a bit more adjustment before merge, mainly around transaction correctness and code organization.

Nested atomic() rollback behavior

I think the current batching logic can leak rolled-back work from a nested transaction into an outer batch.

_pending_batch() appears to find an existing pending callback by database alias only. That means an operation performed inside an inner atomic() block can be added to a batch that belongs to an outer on_commit() callback.

For example:

with transaction.atomic():
    site_a.save()  # schedules an outer search-cache batch

    try:
        with transaction.atomic():
            site_b.delete()  # added to the existing outer batch
            raise RuntimeError()
    except RuntimeError:
        pass

# outer transaction commits

At the database level, site_b still exists because the inner savepoint rolled back. But because the delete was appended to the outer batch, the search cache update can still remove site_b after the outer transaction commits.

Could we make the pending batch lookup savepoint-aware, or otherwise avoid reusing a batch registered outside the current savepoint scope?

I think this should have a regression test covering:

1. schedule a search-cache update in an outer transaction;
2. delete or update another searchable object inside an inner atomic();
3. roll back the inner savepoint;
4. commit the outer transaction;
5. confirm the rolled-back inner operation did not affect the search cache.

Code organization / ownership

I also think the new deferred search-cache path would be easier to maintain with clearer ownership boundaries.

Right now the import/dependency shape feels a bit circular:

search.backends -> search.deferred -> search.tasks -> search.backends

The local import avoids the runtime circular import, but I think it is a sign that the responsibilities are not quite separated yet. deferred.py is handling transaction batching, callback coalescing, enqueue-vs-inline behavior, and dispatching. Then the task/job code reaches back into backend-specific cache internals.

I would prefer something closer to:

netbox/search/
├── backends.py   # search backend implementations
├── deferred.py   # transaction/on_commit batching and coalescing
└── jobs.py       # SearchCacheJob(JobRunner)

In particular, I think SearchCacheJob(JobRunner) should live in netbox/search/jobs.py, matching the usual NetBox job organization. The job itself can stay very thin and delegate the actual cache mutation to the backend.

For example:

# netbox/search/jobs.py

class SearchCacheJob(JobRunner):
    class Meta:
        name = "Search cache update"

    def run(self, using=None, cache_groups=None, remove_groups=None, **kwargs):
        from netbox.search.backends import search_backend

        search_backend.apply_deferred_updates(
            using=using,
            cache_groups=cache_groups,
            remove_groups=remove_groups,
            log=self.logger,
        )

Then the CachedValue-specific behavior can remain owned by CachedValueSearchBackend:

class CachedValueSearchBackend(SearchBackend):
    def apply_deferred_updates(self, using=None, cache_groups=None, remove_groups=None, log=None):
        # owns CachedValue lookups, removals, cache(..., using=...), etc.
        ...

That would also make the custom backend contract clearer. Before this change, custom SEARCH_BACKEND implementations mostly needed to provide the public cache() / remove() behavior used by the signal handlers. With the deferred path, the job now appears to assume CachedValueSearchBackend internals such as _remove_by_id() and a cache(..., using=...) signature. Keeping the deferred implementation on CachedValueSearchBackend would avoid making those assumptions globally.

Comments

Small style note: I think some of the comments in this path could be trimmed once the responsibilities are split up.

[jnovinger]

Both addressed, in two commits now pushed.

Nested atomic() rollback leak (19ba073c9). Confirmed with a failing regression test first, and the root cause is what you described. The mechanism: Django scopes on_commit callbacks to savepoints by the savepoint-id set captured when the callback is registered, and savepoint_rollback prunes callbacks by that set. Our single callback was registered at the outer level and only mutated (not re-registered) from the inner savepoint, so the inner rollback never pruned it and the op flushed on the outer commit.

Fix: scope each flush callback to the savepoint stack active when it is registered (tuple(connection.savepoint_ids)). _pending_batch now matches on alias and scope, so each savepoint scope gets its own callback and batch, and Django's existing savepoint_rollback pruning discards a rolled-back scope's callback for free, no manual bookkeeping. Coalescing still holds within a scope; cross-scope operations produce one job per scope, which is correct because the worker re-fetches live rows at flush time. Added your requested regression test plus coverage for committed nested scopes, cross-scope save-then-delete, sibling savepoints, and deep nesting with a middle rollback.

Code organization (4b87d7383). Done as you sketched. SearchCacheJob moved to netbox/search/jobs.py and is thin; it delegates to CachedValueSearchBackend.apply_deferred_updates(), which now owns the deferred-update logic and the missing-schema handling (_is_missing_schema and the Postgres SQLSTATE set are private to that class). SearchBackend gains an abstract apply_deferred_updates() raising NotImplementedError like its siblings, so the contract is explicit for custom SEARCH_BACKEND classes rather than leaking _remove_by_id / cache(..., using=...) assumptions. Old tasks.py is gone, and I trimmed the comments while splitting things up.

One thing this does not fully resolve: it renames and relocates the backends -> deferred -> jobs -> backends cycle but does not break it. deferred._flush still carries a single function-local import, because deferred.py can't import the search_backend singleton at module level (it's bound at the bottom of backends.py, after backends.py imports deferred). Breaking the cycle properly means extracting the singleton and moving signal wiring into an AppConfig.ready(), which is broad enough that I've kept it separate as #22485.

[pheus]

Thanks for the updates! This looks very close.

I have two small follow-ups.

For the delete/removal tests, could we make sure the object PK is captured before calling delete() wherever we assert against CachedValue.object_id? Assertions using site.pk after site.delete() can accidentally query for object_id=None instead of the deleted object’s original ID.

It would also be useful for at least one delete/removal test to seed existing CachedValue rows before the delete and then assert that rows for the captured PK are removed.

One other API question: do we want deferred indexing to become part of the base SearchBackend contract? apply_deferred_updates() makes sense for CachedValueSearchBackend, but it is a new expectation for custom SEARCH_BACKEND implementations that may previously have only implemented cache(), remove(), search(), and clear().

Other than that, this looks good to me. Thanks again for iterating on it.