Fixes #755: Evict deleted Custom Object Types' models from other workers

[bctiemann]

Fixes: #755

Deleting a Custom Object Type unregistered its generated model only in the worker process that handled the delete. Every other worker kept it registered, so it stayed in other models' _meta.related_objects. A later delete of a tag, an owner, or anything the type's fields pointed at then made core's handle_deleted_object query the dropped table: relation "custom_objects_N" does not exist. This persisted until NetBox was restarted.

Changed types don't have this problem, because get_model() notices a new cache_timestamp the next time it's called. A deleted type's model is never looked up again, though, so nothing evicted it.

Change

This uses NetBox's shared cache, the same way the live GraphQL schema signature already propagates type changes across workers (graphql/live.py).

• Signal: CustomObjectType.delete(), outside a branch, marks the type's ID as deleted in the shared cache and writes a fresh random token there. It does this in transaction.on_commit, so other workers only act once the delete is visible to them. A token rather than a counter, so a cache flush or restart can't make a later delete look already seen.
• Check: evict_deleted_custom_object_types() (new stale_models.py) runs on Django's request_started and core's job_start (both available back to NetBox 4.5.2).
  • It compares the token with the last one this process saw. When it changed, it reads the deleted marks of the registered table<id>models (one get_many). It unregisters each marked model, along with its through models, then clears the registry caches.
  • Only marked types are evicted. Evicting every registered type without a database row would also evict a type another thread in the same worker has created but not yet committed, since this thread can't see its row.
  • Marked types that still exist in the database are kept (one query, only when something is marked). This covers a mark that outlives a database restore after which the ID is reused.
  • Steady-state cost is one cache read per request or job, with no database queries.
  • If the cache is unavailable, the check does nothing (today's behavior).
• Branches:
  • request_started fires before any middleware, so no netbox-branching branch is active and the existence check reads main, where the models registered in apps.all_models belong.
  • A type deleted inside a branch doesn't signal, since main's model is still valid. Merging that delete into main runs delete() in main and signals then.
• Refactor: the unregister step moves out of CustomObjectType.delete() into a reusable CustomObjectType.unregister_model(), with the same behavior: under _global_lock, then apps.clear_cache().

Testing

New tests/test_stale_models.py:

• test_tag_delete_succeeds_after_type_deleted_in_another_worker: recreates the bug in one process. It deletes a type, re-registers its stale model as a worker that didn't handle the delete would still have it, signals, then deletes a tag through the API.
  • Without the fix this fails with ProgrammingError: relation "custom_objects_<id>" does not exist.
  • With the fix it returns 204 and the stale model is gone.
• test_eviction_keeps_existing_types: only the deleted type's model is evicted, even if an existing type's ID is marked.
• test_eviction_keeps_unmarked_types_missing_from_the_database: a type that isn't marked stays registered when its row isn't visible, as with an uncommitted create in another thread.
• test_deleting_a_type_signals_other_workers: a committed delete marks the type and writes a new token.
• test_check_is_free_when_nothing_was_deleted: no queries when the token is unchanged.

End to end, against NetBox 4.7.1 under gunicorn with 4 workers:

Scenario	Before	After
Create tag, create type, delete type, delete tag (×N)	failed at iteration 20/20	50/50 OK
Process A registers a type, process B deletes it, A deletes a tag via the API	500 every time	204

The full plugin suite passes: 1384 tests without netbox-branching and 1455 with it, 8 skipped in each. The end-to-end results above predate the change to per-type marks. ruff check is clean.

Release note

Fixed deletes (e.g. of tags) failing with relation "custom_objects_N" does not exist on multi-worker deployments after a Custom Object Type was deleted (#755).

🤖 Generated with Claude Code

[bctiemann]

@claude /review and report only on actionable issues.

[github-actions]

Claude finished @bctiemann's task in 2m 56s —— View job

---

Code Review

• Read changed files
• Analyze code for issues
• Post review

---

The approach is sound: a single shared-cache token triggers a per-worker eviction check on every request/job start, costing one cache read in the steady state. Two actionable issues and one minor note below.

---

1. Test isolation — _seen_token not reset between tests

stale_models._seen_token is module-level and persists across test methods. If tests run in certain orders, _seen_token may already equal the current cache token, causing evict_deleted_custom_object_types() to skip the DB query and silently pass assertions that should exercise eviction. test_check_is_free_when_nothing_was_deleted is the most exposed: if a previous test left _seen_token matching the token it writes, the first evict_deleted_custom_object_types() call returns early without updating _seen_token, and the assertNumQueries(0) assertion is vacuously correct.

Fix: reset stale_models._seen_token = None in setUp.

def setUp(self):
    super().setUp()
    stale_models._seen_token = None   # ← add this
    self.user.is_superuser = True
    ...

Fix this →

---

2. apps.clear_cache() called N times when N models are stale

evict_deleted_custom_object_types calls CustomObjectType.unregister_model(model) inside the loop, and unregister_model always calls apps.clear_cache() — so clearing the app registry cache happens once per stale model. For a single deletion this doesn't matter, but if many types are deleted simultaneously (e.g. bulk teardown), it's unnecessary work.

The natural fix is to factor the clearing out of unregister_model or add an _evict_batch helper, but given unregister_model is already a public method called from delete() which expects the cache to be cleared on return, the simplest surgical fix is to call apps.clear_cache() once after the loop in the eviction function:

# stale_models.py — after the for-loop
evicted_any = False
for model_name, model in list(apps.all_models.get(APP_LABEL, {}).items()):
    cot_id = extract_cot_id_from_model_name(model_name)
    if cot_id is None or int(cot_id) in existing:
        continue
    CustomObjectType.clear_model_cache(int(cot_id), all_branches=True)
    CustomObjectType.unregister_model(model)  # still calls clear_cache per call
    evicted_any = True
    logger.debug(...)

This is a minor efficiency note — the operation is rare and apps.clear_cache() is cheap — so it's low priority, but worth knowing about.

---

3. (Minor) Thread safety of _seen_token read-then-update

In a threaded WSGI worker, two concurrent requests can both read _seen_token != token, both run the DB query, and both run eviction. Correctness is maintained (both produce the same result), and the existing codebase already accepts this pattern for get_model() regeneration (see the comment at models.py:1833). Calling it out for completeness — not a blocker.

---

The rest of the design is solid: the transaction.on_commit placement (so other workers only act after the delete is visible), the in_branch guard with correct merge-time fallback, the None timeout so cache flushes/restarts safely fall back to today's behavior, and the list() snapshot before loop mutation are all correct. Test coverage for the primary bug scenario, the eviction selectivity, the commit-time signal, and the zero-query steady state is comprehensive.

[bctiemann]

Checked all three; no changes needed:

1. _seen_token between tests: each test that exercises eviction first calls signal_custom_object_type_deleted(), which writes a fresh random uuid4 token. A token left over from an earlier test can never match it, so the first eviction check always runs.
   • test_check_is_free_when_nothing_was_deleted signals a new token and evicts, which acts on it and records it.
   • Its assertNumQueries(0) then covers the second call, which is exactly the steady state.
   • Resetting _seen_token in setUp wouldn't change any outcome.
2. apps.clear_cache() per stale model: accurate, but it only matters when several types were deleted between two checks in one worker, as a one-off cost per worker. The suggested snippet still calls unregister_model(), and so clear_cache(), once per model. Batching would need a clear_cache=False parameter on the public unregister_model(), which isn't worth the extra API for this.
3. Concurrent checks: agreed it's harmless. Eviction is idempotent (pop under _global_lock), so a duplicate check costs one extra query at most.

🤖 Generated with Claude Code

[jnovinger]

With the shipped gunicorn config (threads = 3), can a request in one thread evict a type another thread in the same worker just created but hasn't committed yet, since the id snapshot at stale_models.py:66 won't see that row?

Separately, CustomObjectTypeField.delete() still deregisters a dropped multi-object through model only in its own process, so is that worth a follow-up issue for the same cross-worker case?

[bctiemann]

@jnovinger Thanks, both good catches.

Threads evicting an uncommitted type: yes, that could happen. Fixed in fb30e88: a delete now marks its type's ID as deleted in the shared cache, and eviction unregisters only marked types. A type another thread has created but not committed isn't marked, so it stays registered even though this thread can't see its row (test_eviction_keeps_unmarked_types_missing_from_the_database). Marked types that still exist in the database are also kept, in case a mark outlives a database restore that reuses the ID.

(Before the fix, the impact was limited: the evicted type ended up in the same "not yet registered" state it's in on every other worker after a create, and the next get_model() regenerated it. But there's no reason to leave the race in.)

Multi-object field deletes: agreed, that's the same bug for through models. Workers regenerate the type's model when its cache_timestamp changes, but regeneration doesn't remove the dropped field's through model, so it stays in the target model's related objects. Filed as #840, to extend this PR's mechanism to fields.