From 49dbb22b010458d805196f78812fb0e58ef86955 Mon Sep 17 00:00:00 2001 From: S'Bussiso Dube <80188685+Sbussiso@users.noreply.github.com> Date: Sun, 13 Sep 2026 20:50:32 -0700 Subject: [PATCH] Retry a colliding node_id instead of 500ing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit node_id is the first 8 characters of a uuid4 — 32 bits — and the column is unique across EVERY org, not per-org. So a collision is not a per-tenant curiosity: it is one customer's new node landing on an id another customer already holds. The code drew once and committed, so the unique constraint surfaced as an unhandled IntegrityError, i.e. a 500 at the worst possible moment — someone adding their first node. Odds of a single creation colliding, by fleet size: 1k nodes 1 in 4,294,967 10k nodes 1 in 429,496 100k nodes 1 in 42,949 Rare per request, certain enough in aggregate, and free to handle. Retries rather than widening the id, because 8 hex characters is a deliberate UX choice: the operator types it into the installer. Three draws, then a clean 503 — three collisions in a row is not bad luck at any plausible fleet size, it means something else is wrong (a duplicated uuid source, or a constraint firing on a different column), and a 503 says "try again" where a 500 says "we crashed". Lets the unique constraint be the arbiter rather than a pre-check SELECT, which would race two concurrent creates. Two tests, both verified to fail without the retry: one forces a single collision and asserts the second draw succeeds with a different id, the other collides forever and asserts 503 rather than an unhandled error. The uuid shim in those tests replaces nodes.py's OWN module reference rather than mutating the real uuid module — request_context.py also calls uuid4() and wants a genuine UUID with .hex, which is how the first version of the test broke. 874 backend tests pass, ruff clean. Co-Authored-By: Claude Opus 5 --- backend/app/api/nodes.py | 62 +++++++++++++++++++++++++++++------ backend/tests/test_nodes.py | 65 +++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 10 deletions(-) diff --git a/backend/app/api/nodes.py b/backend/app/api/nodes.py index fa0d68a7..f30e90e3 100644 --- a/backend/app/api/nodes.py +++ b/backend/app/api/nodes.py @@ -4,6 +4,7 @@ from datetime import UTC, datetime from fastapi import APIRouter, Depends, HTTPException, Request +from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session from app.core.audit import audit_label, write_audit @@ -25,6 +26,9 @@ logger = logging.getLogger(__name__) +# Retries for the 32-bit node_id draw. See the comment at the retry loop. +_NODE_ID_ATTEMPTS = 3 + router = APIRouter(prefix="/api/nodes", tags=["nodes"]) @@ -835,19 +839,57 @@ async def create_node( detail=f"Node limit reached ({limits['max_nodes']} on {plan_name} plan). Upgrade your plan to add more nodes.", ) - node_id = str(uuid_mod.uuid4())[:8] + # node_id is the first 8 chars of a uuid4 — 32 bits — and the column is + # unique across EVERY org, not per-org. A collision therefore isn't a + # per-tenant curiosity: it's one customer's new node landing on an id + # another customer already holds. + # + # 8 hex characters is a deliberate UX choice (the operator types this + # into the installer), so the fix is to retry rather than to widen it. + # Without the retry a collision surfaced as an unhandled IntegrityError + # -> 500, at the worst possible moment: someone adding their first node. + # + # Odds of a single creation colliding, by fleet size: + # 1k nodes 1 in 4,294,967 + # 10k nodes 1 in 429,496 + # 100k nodes 1 in 42,949 + # Rare per request, certain enough in aggregate, and free to handle. api_key = str(uuid_mod.uuid4()) api_key_hash = hashlib.sha256(api_key.encode()).hexdigest() - node = CameraNode( - node_id=node_id, - org_id=user.org_id, - name=data.name or f"Node-{node_id}", - api_key_hash=api_key_hash, - status="pending", - ) - db.add(node) - db.commit() + node = None + for attempt in range(_NODE_ID_ATTEMPTS): + node_id = str(uuid_mod.uuid4())[:8] + node = CameraNode( + node_id=node_id, + org_id=user.org_id, + name=data.name or f"Node-{node_id}", + api_key_hash=api_key_hash, + status="pending", + ) + db.add(node) + try: + db.commit() + break + except IntegrityError: + # Let the unique constraint be the arbiter rather than a + # pre-check SELECT, which would race two concurrent creates. + db.rollback() + node = None + logger.warning( + "node_id collision on %s (attempt %d/%d) — regenerating", + node_id, attempt + 1, _NODE_ID_ATTEMPTS, + ) + + if node is None: + # Three collisions in a row is not bad luck at any plausible fleet + # size; it means something else is wrong (a duplicated uuid source, + # or a constraint firing on a different column). + logger.error("node creation failed after %d id attempts", _NODE_ID_ATTEMPTS) + raise HTTPException( + status_code=503, + detail="Could not allocate a node ID. Please try again.", + ) logger.info("Node created: node_id=%s, name=%s, org=%s", node_id, node.name, user.org_id) diff --git a/backend/tests/test_nodes.py b/backend/tests/test_nodes.py index a46054cf..d92f5597 100644 --- a/backend/tests/test_nodes.py +++ b/backend/tests/test_nodes.py @@ -851,3 +851,68 @@ def boom(*args, **kwargs): hb = _heartbeat_with_disk(admin_client, node_id, api_key, used_pct=99.0) assert hb.status_code == 200 + + +# --- node_id collision handling ------------------------------------------- +# +# node_id is the first 8 chars of a uuid4 (32 bits) and the column is unique +# across EVERY org, so a collision is one customer's new node landing on an id +# another customer already holds. Before the retry it surfaced as an +# unhandled IntegrityError -> 500 while someone was adding their first node. +# +# The shim replaces nodes.py's OWN reference to the uuid module rather than +# mutating the real one — request_context.py also calls uuid4() and wants a +# genuine UUID with a .hex attribute. + + +class _UuidShim: + """Stands in for the uuid module inside app.api.nodes only.""" + + def __init__(self, collide_with, *, forever): + self._collide_with = collide_with + self._forever = forever + self.calls = 0 + + def uuid4(self): + import uuid as _real + + self.calls += 1 + # call 1 is the api_key; node_id draws start at call 2. + if self.calls == 1: + return _real.uuid4() + if self._forever or self.calls == 2: + return _real.UUID(self._collide_with.ljust(8, "0") + "0" * 24) + return _real.uuid4() + + +def test_create_node_survives_a_node_id_collision(admin_client, monkeypatch): + """A colliding first draw must be retried, not 500.""" + import app.api.nodes as nodes_mod + + first = admin_client.post("/api/nodes", json={"name": "First"}) + assert first.status_code == 200 + taken = first.json()["node_id"] + + shim = _UuidShim(taken, forever=False) + monkeypatch.setattr(nodes_mod, "uuid_mod", shim) + + resp = admin_client.post("/api/nodes", json={"name": "Second"}) + assert resp.status_code == 200, resp.text + assert resp.json()["node_id"] != taken + assert shim.calls >= 3, "the collision should have forced a second draw" + + +def test_create_node_gives_up_cleanly_if_every_draw_collides( + admin_client, monkeypatch +): + """Exhausting the retries is a 503, never an unhandled 500.""" + import app.api.nodes as nodes_mod + + first = admin_client.post("/api/nodes", json={"name": "First"}) + assert first.status_code == 200 + taken = first.json()["node_id"] + + monkeypatch.setattr(nodes_mod, "uuid_mod", _UuidShim(taken, forever=True)) + + resp = admin_client.post("/api/nodes", json={"name": "Doomed"}) + assert resp.status_code == 503, resp.text