From 5eadef54992c9d0f759274d2c75960a5be81dbee Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Thu, 10 Sep 2026 09:46:02 -0500 Subject: [PATCH] Correct what sharing a transaction gives the reachability check The docstring claimed that running the check and the rebuild in one transaction closes the window in which a concurrent write could strand a row. It does not: under READ COMMITTED, which is the default, every statement takes a fresh snapshot, so a reparent committed between the two is still missed. Sharing the transaction is still worth doing, for reasons the docstring now gives instead: a refusal rolls back cleanly, and populate_paths_sql() needs a transaction for its SET LOCAL regardless. Neither is an isolation guarantee. Say in the documentation that a rebuild assumes nothing else is changing the hierarchy, which is what the command actually requires, alongside the existing advice to run it in a maintenance window. --- docs/administration/management-commands.md | 2 ++ .../utilities/management/commands/rebuild_ltree_paths.py | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/administration/management-commands.md b/docs/administration/management-commands.md index 954d961e4..364d9490a 100644 --- a/docs/administration/management-commands.md +++ b/docs/administration/management-commands.md @@ -96,6 +96,8 @@ One of the listed objects is in a cycle, parented to itself, or pointing at an o !!! warning A rebuild rewrites every row of each named model in a single statement, locking those rows until it commits. On a large table this blocks concurrent writes for minutes, so run it during a maintenance window. Use `--check` first to limit the rebuild to the models which need it. + A rebuild also assumes nothing else is changing the hierarchy while it runs. An object reparented after the command has checked the model, but before it rewrites it, is not accounted for, and the check which refuses unreachable objects cannot see it either. This is another reason to run the command with writes paused rather than against a live system. + ## rebuild_prefixes Rebuild the IPAM prefix hierarchy, recalculating the depth and child counts for all prefixes. diff --git a/netbox/utilities/management/commands/rebuild_ltree_paths.py b/netbox/utilities/management/commands/rebuild_ltree_paths.py index 3de783c74..24349ca9c 100644 --- a/netbox/utilities/management/commands/rebuild_ltree_paths.py +++ b/netbox/utilities/management/commands/rebuild_ltree_paths.py @@ -72,9 +72,11 @@ class Command(BaseCommand): data is still wrong. Refuse instead, and leave correcting the parent relationships to the operator, since only they can say what the intended hierarchy was. - Takes the caller's cursor to keep it visible that this must run in the same - transaction as the rebuild it guards. Checking in a separate transaction would - leave a window in which a concurrent write could strand a row between the two. + Takes the caller's cursor so a refusal rolls back with the transaction the rebuild + would have run in. That does not make the pair atomic with respect to other + writers: under READ COMMITTED every statement takes a fresh snapshot, so a + reparent committed between the check and the rebuild is still missed. Pause writes + for the duration, as the documentation says to. """ cursor.execute(unreachable_rows_sql(model._meta.db_table, self.REPORTED_IDS)) unreachable, ids = cursor.fetchone()