diff --git a/backend/app/api/nodes.py b/backend/app/api/nodes.py index fa0d68a..f30e90e 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 a46054c..d92f559 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