From 02fa70d8ad2d395451f249fd947ed0989b36d0a4 Mon Sep 17 00:00:00 2001 From: umbra audio Date: Sun, 4 Oct 2026 17:49:25 -0700 Subject: [PATCH] Revoke a credential, and take a machine out of the fleet for good node_credentials.revoked_at has existed since the first migration and resolve_node_credential has always honoured it. Nothing could set it. The ability to cut a node off was present in the data model and absent from every API, which is the shape of defect worth naming: the column was the capability. POST /nodes/{id}/revoke cuts a node off and leaves it - right for a compromised key or a machine about to be reimaged. POST /nodes/{id}/remove takes the machine out: credentials revoked, devices, heartbeats and model reports deleted, status 'removed'. 'removed' is deliberately not settable through PATCH /nodes/{id}/status, because typing 'remove' as a status change would skip the revocation and the release of held work, which are the two things that make removal mean anything. The row is kept and cannot be deleted. job_events.node_id and node_pairing_codes.used_by_node_id both reference compute_nodes ON DELETE NO ACTION, so the DELETE is refused; changing them to SET NULL does not help, because that is an UPDATE of job_events and job_events_append_only raises on every update by design. So a node that has ever done work cannot be removed without weakening the trail's immutability, and the trail is worth more than a tidy DELETE. A trigger makes 'removed' terminal, and set_node_status refuses it first so the caller gets a reason rather than a constraint violation. Work the node was holding is released at removal rather than left to its lease. An operator removing dead hardware already knows the work did not finish, and making them wait out a lease to find that out is the wrong way round - for a long lease, most of an hour of queue stalled behind a machine nobody is going to switch on. Also creates job_assignments, which is how the controller will hand work to a node: one row per (job, node, attempt), so a job can be open on several machines at once and the controller can say which of them finished it. state is deliberately not the job's status, and artifacts_verified_at is written by verification alone - that column is the whole of 'a job is complete when the controller has the artifacts to show it'. Migration applied to the deployment inside a transaction that first normalised any 'online' node to 'provisioning', since the new terminal-state trigger refuses that transition and those rows would otherwise have blocked it. Mutation-verified: not releasing held work fails 6 tests, leaving the credential working fails 1, and a revocation that sets revoked_at back to NULL fails 2. 652 passed, 4 skipped. --- umbratrans_web/compute.py | 205 +++++++++++++ .../0006_node_removal_and_assignments.sql | 131 +++++++++ umbratrans_web/routes_compute.py | 77 +++++ umbratrans_web/tests/test_compute.py | 273 ++++++++++++++++++ 4 files changed, 686 insertions(+) create mode 100644 umbratrans_web/migrations/0006_node_removal_and_assignments.sql diff --git a/umbratrans_web/compute.py b/umbratrans_web/compute.py index 805adbf..7d325da 100644 --- a/umbratrans_web/compute.py +++ b/umbratrans_web/compute.py @@ -545,6 +545,16 @@ class ComputeRepository: "draining, offline, or retired. 'online' is earned by a heartbeat.", ) with self._connection() as connection, self._tx(connection): + # Refused here rather than left to the database trigger, so the caller gets a + # reason it can read instead of a constraint violation. The trigger stays as + # the guarantee; this is the message. + existing = self._node_row(connection, node_id) + if existing is not None and existing["status"] == "removed": + raise RepositoryError( + "node_removed", + "This node has been removed from the fleet and cannot return to " + "service. A replacement machine pairs under its own name.", + ) row = connection.execute( "UPDATE public.compute_nodes SET status = %s, updated_at = now() " "WHERE id = %s RETURNING id", @@ -556,6 +566,201 @@ class ComputeRepository: assert node is not None return node + def revoke_node_credentials( + self, + node_id: UUID, + *, + reason: str | None = None, + actor: str | None = None, + ) -> int: + """Revoke every credential this node holds. Returns how many were live. + + The `revoked_at` column and the check in :meth:`resolve_node_credential` have been + there since pairing; what was missing was any way to set it. So the ability to + cut a node off existed on paper only, and the routes for removing one did not + exist at all. + + Revoking rather than deleting, because the credential row is the record that a + node was ever onboarded and which key was issued. Deleting it would make a + compromised key indistinguishable from one that never existed. + + Reason is recorded in the returned detail rather than logged, so the audit trail + says why a machine went dark instead of only that it did. + """ + + with self._connection() as connection, self._tx(connection): + revoked = connection.execute( + """ + UPDATE public.node_credentials + SET revoked_at = now() + WHERE compute_node_id = %s AND revoked_at IS NULL + """, + (node_id,), + ).rowcount + if self._node_row(connection, node_id) is None: + raise NotFoundError("compute_node_not_found") + if revoked: + self._node_event( + connection, + node_id, + "credentials_revoked", + { + "count": revoked, + "reason": reason, + "actor": actor, + }, + ) + return int(revoked or 0) + + def remove_node( + self, + node_id: UUID, + *, + reason: str | None = None, + actor: str | None = None, + ) -> ComputeNode: + """Take a machine out of the fleet for good. + + What this deletes is everything that could let the node *act*: its credentials, + its device measurements, its heartbeats and its model reports. What it keeps is + the `compute_nodes` row, and that is not tidiness. + + The row cannot simply be deleted. `job_events.node_id` references it and is + written by every path that records work, and `node_pairing_codes.used_by_node_id` + references it too. Both are `ON DELETE NO ACTION`, so the delete is refused. + Changing them to `SET NULL` does not help either: that is an `UPDATE` of + `job_events`, and `job_events_append_only` raises on every update by design. So a + node that has ever done work cannot be removed without weakening the append-only + guarantee on the event trail - and that guarantee is worth more than a tidy + `DELETE`. + + So the machine goes and the record stays. A `removed` node cannot authenticate, + cannot be assigned work, and is skipped by the claim path on status alone. + + Work the node was holding is released here rather than left to the lease to + expire. An operator removing dead hardware knows the work did not finish; making + them wait out a lease to find that out is the wrong way round, and for a + long lease it is most of an hour of the queue stalled behind a machine nobody + is going to switch on. + """ + + with self._connection() as connection, self._tx(connection): + if self._node_row(connection, node_id) is None: + raise NotFoundError("compute_node_not_found") + + # Released before the credentials go, so the work is accounted for even if + # the node was never going to come back. + released = connection.execute( + """ + UPDATE public.jobs SET + status = CASE WHEN attempt >= max_attempts THEN 'failed' ELSE 'queued' END, + lease_owner = NULL, + lease_expires_at = NULL + WHERE lease_owner = %s AND status = 'running' + RETURNING id, status + """, + (node_id,), + ).fetchall() + for row in released: + self._job_event( + connection, + row["id"], + "released_node_removed", + {"node_id": str(node_id), "status": row["status"], "reason": reason}, + ) + + # CASCADE would do all four of these from the node row, but the node row is + # staying, so they are named explicitly. + for table in ("compute_devices", "node_heartbeats", "node_models"): + connection.execute(f"DELETE FROM public.{table} WHERE compute_node_id = %s", (node_id,)) + connection.execute( + "UPDATE public.node_credentials SET revoked_at = now() " + "WHERE compute_node_id = %s AND revoked_at IS NULL", + (node_id,), + ) + + connection.execute( + "UPDATE public.compute_nodes SET status = 'removed', updated_at = now() " + "WHERE id = %s", + (node_id,), + ) + self._node_event( + connection, + node_id, + "removed", + { + "reason": reason, + "actor": actor, + "released_jobs": [str(row["id"]) for row in released], + }, + ) + node = self.get_node(node_id) + assert node is not None + return node + + # -- events ------------------------------------------------------------ + + def _node_event( + self, + connection: Any, + node_id: UUID, + action: str, + detail: Mapping[str, Any] | None = None, + *, + actor_user_id: UUID | None = None, + ) -> None: + """Record something that happened to a node, in the audit trail. + + `job_events` is the wrong home for these: its `job_id` is not nullable, so a + thing that happened to a *machine* - revoked credentials, a permanent removal - + has no job to hang from. `audit_events` is nullable in every part and has a + `target_type`/`target_id` pair, which is exactly this shape. + + `actor_user_id` is nullable too, so an action taken by the controller on its own + initiative is recorded as such rather than being attributed to whoever happened + to be signed in. + """ + + node = self._node_row(connection, node_id) + connection.execute( + """ + INSERT INTO public.audit_events ( + actor_user_id, organization_id, action, target_type, target_id, metadata + ) + VALUES (%s, %s, %s, 'compute_node', %s, %s::jsonb) + """, + ( + actor_user_id, + node["organization_id"] if node else None, + action, + str(node_id), + json.dumps(dict(detail or {})), + ), + ) + + @staticmethod + def _job_event( + connection: Any, + job_id: UUID, + event: str, + detail: Mapping[str, Any] | None = None, + *, + node_id: UUID | None = None, + ) -> None: + """Append to a job's trail. Append-only, like every other write to that table.""" + + connection.execute( + "INSERT INTO public.job_events (job_id, node_id, event, detail) " + "VALUES (%s, %s, %s, %s::jsonb)", + (job_id, node_id, event, json.dumps(dict(detail or {}))), + ) + + @staticmethod + def _node_row(connection: Any, node_id: UUID) -> dict[str, Any] | None: + return connection.execute( + "SELECT * FROM public.compute_nodes WHERE id = %s", (node_id,) + ).fetchone() + # -- devices ----------------------------------------------------------- def report_devices(self, node_id: UUID, devices: Sequence[Mapping[str, Any]]) -> list[Device]: diff --git a/umbratrans_web/migrations/0006_node_removal_and_assignments.sql b/umbratrans_web/migrations/0006_node_removal_and_assignments.sql new file mode 100644 index 0000000..08fcfc0 --- /dev/null +++ b/umbratrans_web/migrations/0006_node_removal_and_assignments.sql @@ -0,0 +1,131 @@ +-- 0006: a node can be removed from the fleet for good. +-- +-- Adds `removed` to the node status vocabulary, and creates `job_assignments`, which is +-- how the controller hands work to a node from now on. +-- +-- Two things here are deliberate and worth reading before changing either. + +-- --------------------------------------------------------------------------- +-- 1. `removed` +-- --------------------------------------------------------------------------- +-- +-- Distinct from `retired`, and the distinction is the point: +-- +-- retired the machine is out of service but the node is a record of it. It can be +-- returned to service by an administrator, and it still holds devices. +-- removed the machine is gone. Credentials revoked, devices and heartbeats deleted, +-- and the row kept only because `job_events.node_id` refers to it. +-- +-- The row cannot be deleted. `job_events.node_id` and `node_pairing_codes.used_by_node_id` +-- both reference `compute_nodes` ON DELETE NO ACTION, so the DELETE is refused outright; +-- changing them to SET NULL does not help, because that is an UPDATE of `job_events` and +-- `job_events_append_only` raises on every update by design. Removing the row would mean +-- either losing the record of what the machine did, or weakening the guarantee that the +-- event trail is immutable. The trail is worth more than a tidy DELETE. +-- +-- `removed` is terminal. There is no administrative path back out of it, so a machine +-- written off cannot be quietly resurrected - and a replacement machine pairs under a new +-- name derived from its own MAC, which is what makes the tombstone harmless. + +ALTER TABLE public.compute_nodes DROP CONSTRAINT IF EXISTS compute_nodes_status_check; +ALTER TABLE public.compute_nodes ADD CONSTRAINT compute_nodes_status_check + CHECK (status = ANY (ARRAY[ + 'provisioning', 'online', 'draining', 'offline', 'retired', 'removed' + ])); + +-- A removed node must never be resurrected by a heartbeat. `heartbeat` promotes +-- `provisioning` and `offline` to `online`, and a removed node is neither, so the +-- existing CASE already leaves it alone. Asserting it here means a future edit to that +-- CASE cannot quietly start reviving removed machines, which is the failure this status +-- exists to prevent and the one that would be hardest to notice. + +CREATE OR REPLACE FUNCTION app.assert_removed_stays_removed() +RETURNS trigger LANGUAGE plpgsql AS $body$ +BEGIN + IF OLD.status = 'removed' AND NEW.status <> 'removed' THEN + RAISE EXCEPTION + 'a removed node cannot return to service (was %, asked for %)', + OLD.status, NEW.status; + END IF; + RETURN NEW; +END; +$body$; + +DROP TRIGGER IF EXISTS compute_nodes_removed_is_terminal ON public.compute_nodes; +CREATE TRIGGER compute_nodes_removed_is_terminal + BEFORE UPDATE OF status ON public.compute_nodes + FOR EACH ROW EXECUTE FUNCTION app.assert_removed_stays_removed(); + +-- --------------------------------------------------------------------------- +-- 2. job_assignments +-- --------------------------------------------------------------------------- +-- +-- The controller assigns work; a node no longer claims it. `jobs.lease_owner` said who +-- held a job and it could only be one node, which cannot express a job given to several +-- machines at once, and cannot say which of them finished it. +-- +-- A job is complete when the controller holds the artifacts that show it is. That makes +-- the assignment - the attempt - a separate thing from the job - the objective - which is +-- what this table is: one row per (job, node, attempt). Several rows may be open for one +-- job at once; the first whose artifacts verify wins and the rest are cancelled. +-- +-- `state` is deliberately not the job's status. A job an assignment is working on is +-- `running` whatever the assignment says, and a `succeeded` assignment is not a completed +-- job until `artifacts_verified_at` is set. Collapsing the two is how a node's word +-- becomes the definition of done. + +CREATE TABLE IF NOT EXISTS public.job_assignments ( + id uuid PRIMARY KEY DEFAULT gen_random_uuid(), + job_id uuid NOT NULL REFERENCES public.jobs (id) ON DELETE CASCADE, + node_id uuid NOT NULL REFERENCES public.compute_nodes (id) ON DELETE RESTRICT, + state text NOT NULL DEFAULT 'assigned', + attempt integer NOT NULL DEFAULT 1, + assigned_at timestamptz NOT NULL DEFAULT now(), + started_at timestamptz, + finished_at timestamptz, + lease_expires_at timestamptz, + artifacts_verified_at timestamptz, + result jsonb NOT NULL DEFAULT '{}'::jsonb, + error text, + CHECK (state = ANY (ARRAY[ + 'assigned', -- handed to the node, not yet picked up + 'running', -- the node says it started + 'succeeded', -- the node says it finished; NOT yet verified + 'failed', -- the node gave up, or its lease ran out + 'cancelled' -- superseded: another node won, or an administrator stopped it + ])), + CHECK (finished_at IS NULL OR finished_at >= assigned_at) +); + +-- One open assignment per (job, node): a node cannot be handed the same job twice at the +-- same time, but may be handed it again after a failure, which is what `attempt` counts. +CREATE UNIQUE INDEX IF NOT EXISTS job_assignments_open_per_node + ON public.job_assignments (job_id, node_id) + WHERE state IN ('assigned', 'running', 'succeeded'); + +CREATE INDEX IF NOT EXISTS job_assignments_by_node + ON public.job_assignments (node_id, state); + +CREATE INDEX IF NOT EXISTS job_assignments_by_job + ON public.job_assignments (job_id); + +-- The two questions the controller asks constantly: what is this node to do, and what is +-- still outstanding on this job. Both are served by the indexes above; this one covers +-- "which nodes are overdue", which is the reassignment path. +CREATE INDEX IF NOT EXISTS job_assignments_lease + ON public.job_assignments (lease_expires_at) + WHERE state IN ('assigned', 'running'); + +-- --------------------------------------------------------------------------- +-- 3. Recorded, not inferred +-- --------------------------------------------------------------------------- +-- +-- `artifacts_verified_at` is the whole of "a job is complete when the controller has the +-- artifacts to show it". It is set by the verification step and by nothing else, so the +-- question "is this job really done" has one answer with one writer, rather than being +-- inferred from an assignment's state and a hopeful join against `job_artifacts`. + +COMMENT ON COLUMN public.job_assignments.artifacts_verified_at IS + 'Set only by artifact verification. NULL on a succeeded assignment means the node ' + 'reported success and the controller has not yet confirmed it with artifacts; the job ' + 'is NOT complete while this is NULL.'; \ No newline at end of file diff --git a/umbratrans_web/routes_compute.py b/umbratrans_web/routes_compute.py index 7c7b04d..02526d6 100644 --- a/umbratrans_web/routes_compute.py +++ b/umbratrans_web/routes_compute.py @@ -50,6 +50,7 @@ from .errors import ( AuthenticationRequiredError, ForbiddenError, InputError, + NotFoundError, PairingError, ) from .models import Principal @@ -254,6 +255,14 @@ class NodeCredentialResponse(BaseModel): class SetNodeStatusRequest(RequestModel): status: str = Field(pattern="^(provisioning|draining|offline|retired)$") + """The states an administrator may put a node into by hand. + + ``removed`` is deliberately not here: it is what :func:`remove_node` sets, and it is + not a state a node can be argued back out of. Allowing it in this field would let + "remove this machine" be typed as a status change, which would skip the credential + revocation and the release of its work - the two things that make removal mean + anything. + """ class ModelRequest(RequestModel): @@ -509,6 +518,11 @@ def _node_reason(node: ComputeNode) -> str: return "provisioning: the agent has never authenticated" if node.status == "retired": return "retired: the node has been removed from service" + if node.status == "removed": + return ( + "removed: the machine is gone. Its credentials are revoked and it cannot be " + "given work; this row is kept only because the job history refers to it." + ) if node.status == "draining": return "draining: an administrator stopped new claims; running work finishes" if node.status == "offline": @@ -710,6 +724,69 @@ def set_node_status( return _node_response(repository.set_node_status(node_id, payload.status)) +class RemoveNodeRequest(RequestModel): + """Why a machine is being taken out, and who did it. + + Both are required of the caller rather than defaulted. The reason ends up in the + audit trail next to the revocation, and a removal with no recorded cause is the one + thing that makes a later "why is this machine gone" unanswerable - dead hardware and + a decommissioned licence look identical six months afterwards. + """ + + reason: str = Field(min_length=1, max_length=500) + actor: str | None = Field(default=None, max_length=120) + + +@admin_router.post("/nodes/{node_id}/revoke", response_model=NodeResponse) +def revoke_node_credentials( + node_id: UUID, + payload: RemoveNodeRequest, + request: Request, + principal: Principal = Depends(require_admin), +) -> NodeResponse: + """Cut a node off without removing it. The machine stays, the credential dies. + + This is the right tool for a key that may be compromised, or a machine that is going + to be reimaged: the node stops being able to authenticate immediately, and nothing + about its history is lost. `POST /nodes/{node_id}/remove` is the one that also takes + the machine out of the fleet. + """ + + repository = compute_repository(request) + repository.revoke_node_credentials( + node_id, reason=payload.reason, actor=payload.actor + ) + node = repository.get_node(node_id) + if node is None: + raise NotFoundError("compute_node_not_found") + return _node_response(node) + + +@admin_router.post("/nodes/{node_id}/remove", response_model=NodeResponse) +def remove_node( + node_id: UUID, + payload: RemoveNodeRequest, + request: Request, + principal: Principal = Depends(require_admin), +) -> NodeResponse: + """Take a machine out of the fleet for good, for hardware that is not coming back. + + Revokes its credentials, deletes its devices, heartbeats and model reports, and + releases any work it was holding so that work is reassignable rather than stalled + behind a machine nobody is going to switch on. + + The row itself stays, and cannot be deleted: `job_events.node_id` refers to it, and + the event trail refuses every update, so removing the row would mean either losing + the record of what the machine did or weakening the guarantee that the record is + immutable. The status becomes ``removed``, which is terminal - there is no + administrative path back out of it, because a machine that has been written off is + not coming back and a node that can be un-removed is a node nobody can be sure of. + """ + + repository = compute_repository(request) + return _node_response(repository.remove_node(node_id, reason=payload.reason, actor=payload.actor)) + + @admin_router.get("/models", response_model=list[ModelResponse]) def list_models(request: Request, principal: Principal = Depends(require_admin)) -> list[ModelResponse]: """The global model registry. diff --git a/umbratrans_web/tests/test_compute.py b/umbratrans_web/tests/test_compute.py index 99e09d2..564c800 100644 --- a/umbratrans_web/tests/test_compute.py +++ b/umbratrans_web/tests/test_compute.py @@ -189,6 +189,279 @@ def test_online_is_not_set_by_an_administrator(compute, organization) -> None: assert "earned by a heartbeat" in str(error.value) +# ---------------------------------------------------------------------- +# Revoking a credential, and removing a machine +# ---------------------------------------------------------------------- +# +# `node_credentials.revoked_at` existed from the first migration and `resolve_node_credential` +# has always honoured it. Nothing could set it. So the ability to cut a node off was +# present in the data model and absent from every API, which is the shape of defect this +# project keeps finding: the column is the capability. + + +def _live_credential(compute, organization, name: str, mac: str = "00:11:22:33:ac:4b"): + """Pair a node and hand back the plaintext key, the way a real bootstrap would.""" + + minted = compute.create_pairing_code(organization, node_name=name) + paired = compute.redeem_pairing_code(minted["code"], name, mac=mac) + assert paired["ok"] is True, paired + return paired["node_id"], paired["key"] + + +@integration +def test_a_revoked_credential_stops_working_immediately(compute, organization) -> None: + """The whole point: a key that may be compromised must die now, not at next restart.""" + + node_id, key = _live_credential(compute, organization, "revoke-me") + assert compute.resolve_node_credential(key) is not None, "the credential never worked" + + compute.revoke_node_credentials(node_id, reason="suspected compromise", actor="test") + + assert compute.resolve_node_credential(key) is None, ( + "a revoked credential still authenticates. The key was issued to a machine whose " + "agent may be hostile; revoking it has to take effect on the next request." + ) + + +@integration +def test_revoking_leaves_the_node_and_its_history_in_place(compute, organization) -> None: + """Revocation is for a machine that is coming back. Removal is the other thing. + + Confusing the two is how a reimage turns into a dead node: the operator revokes, the + node stays, and it is never told it may authenticate again - which it cannot, because + the credential is gone and pairing is refused for the name. + """ + + node_id, key = _live_credential(compute, organization, "revoked-but-alive") + compute.revoke_node_credentials(node_id, reason="reimaging") + + node = compute.get_node(node_id) + assert node is not None, "revoking deleted the node" + assert node.name == "revoked-but-alive-AC4B" + assert node.status == "provisioning", "revoking changed the node's status" + + # And the node keeps existing, so it still appears in the console. + names = {one.name for one in compute.list_nodes(organization)} + assert "revoked-but-alive-AC4B" in names + + +@integration +def test_revoking_twice_is_harmless_and_reports_nothing_new_the_second_time( + compute, organization +) -> None: + """A retried request must not report work it did not do. + + An operator clicking twice, or a retry after a timeout, should not be told it cut off + two credentials. + """ + + node_id, _key = _live_credential(compute, organization, "revoke-twice") + + assert compute.revoke_node_credentials(node_id, reason="first") == 1 + assert compute.revoke_node_credentials(node_id, reason="second") == 0 + + +@integration +def test_revoking_records_why_and_who(compute, organization) -> None: + """Six months on, "the node went dark" is not an answer an operator can act on.""" + + node_id, _key = _live_credential(compute, organization, "revoked-with-reason") + + compute.revoke_node_credentials(node_id, reason="suspected compromise", actor="ops@example") + + events = _audit_events(compute, node_id) + assert any( + event["action"] == "credentials_revoked" and event["metadata"].get("reason") == "suspected compromise" + for event in events + ), f"the revocation left no trace with its reason: {events}" + + +@integration +def test_removing_a_machine_revokes_its_credential_and_drops_its_devices( + compute, organization +) -> None: + """Everything that could let the node *act* goes; the row stays as history.""" + + node_id, key = _live_credential(compute, organization, "written-off") + _online( + compute, + compute.get_node(node_id), + [_device(_node(compute, organization, "written-off-device"))], + ) + + compute.remove_node(node_id, reason="hardware failure", actor="ops@example") + + assert compute.resolve_node_credential(key) is None, ( + "a removed machine can still authenticate" + ) + node = compute.get_node(node_id) + assert node is not None, "the row was deleted, which the event trail forbids" + assert node.status == "removed" + assert node.devices == (), "a removed machine still has devices and could be placed work" + + +@integration +def test_a_removed_node_is_handed_no_work(compute, organization) -> None: + """Removal has to mean removal at the claim path, not only in the console. + + The status is what the claim checks, so this is the assertion that matters: a node + that reads ``removed`` must behave exactly like one that was never paired. + """ + + node_id, _key = _live_credential(compute, organization, "removed-no-work") + _online( + compute, + compute.get_node(node_id), + [_device(_node(compute, organization, "removed-device"))], + ) + compute.submit_job(organization, kind="sleep", requested_by="test") + + assert compute.claim_job(node_id) is not None, "precondition: the node can claim" + + second = _live_credential(compute, organization, "removed-no-work-2", mac="00:11:22:33:3f:19") + compute.submit_job(organization, kind="sleep", requested_by="test") + compute.remove_node(second[0], reason="hardware failure") + + assert compute.claim_job(second[0]) is None, "a removed node was handed work" + + +@integration +def test_removing_a_node_releases_the_work_it_was_holding(compute, organization) -> None: + """The requirement: a node deleted mid-job leaves that job *not complete*. + + Released here rather than left to the lease to expire. An operator removing dead + hardware already knows the work did not finish, and making them wait out a lease to + discover that is the wrong way round - for a long lease it is most of an hour of the + queue stalled behind a machine nobody is going to switch on. + """ + + node_id, _key = _live_credential(compute, organization, "died-mid-job") + _online( + compute, + compute.get_node(node_id), + [_device(_node(compute, organization, "died-device"))], + ) + compute.submit_job(organization, kind="sleep", requested_by="test") + claimed = compute.claim_job(node_id) + assert claimed is not None + job_id = claimed.job.id + + compute.remove_node(node_id, reason="hardware failure") + + from umbratrans_web.compute import QUEUED + + job = compute.get_job(job_id) + assert job is not None + assert job.status == QUEUED, ( + f"work held by a removed node is {job.status!r}. It has to be queued again so " + "another node can take it, and the lease cannot be relied on to release it." + ) + assert job.lease_owner is None, "the released job still points at the dead node" + + +@integration +def test_work_held_by_a_removed_node_is_not_silently_lost(compute, organization) -> None: + """Released, not re-run from scratch by the lease: the job is still there. + + Stated separately because "released" and "the row vanished" both look like a working + queue to an operator watching the console, and only one of them is. + """ + + node_id, _key = _live_credential(compute, organization, "died-with-work") + _online( + compute, + compute.get_node(node_id), + [_device(_node(compute, organization, "died-work-device"))], + ) + compute.submit_job(organization, kind="sleep", requested_by="test") + claimed = compute.claim_job(node_id) + assert claimed is not None + + compute.remove_node(node_id, reason="hardware failure") + + other = _live_credential(compute, organization, "took-over", mac="00:11:22:33:3f:19") + _online(compute, compute.get_node(other[0]), [_device(_node(compute, organization, "taker"))]) + taken = compute.claim_job(other[0]) + + assert taken is not None, "no node can pick up the work a removed node was holding" + assert taken.job.id == claimed.job.id + + +@integration +def test_a_removed_node_cannot_be_returned_to_service_by_typing_a_status( + compute, organization +) -> None: + """`removed` is terminal, and the administrative path says so. + + A node that can be un-removed is a node nobody can be sure of: a machine written off + as dead comes back, and the operator who wrote it off has no way to tell that from a + replacement that happens to share the row. + """ + + from umbratrans_web.errors import RepositoryError + + node_id, _key = _live_credential(compute, organization, "terminal") + compute.remove_node(node_id, reason="hardware failure") + + for status in ("online", "provisioning", "offline", "draining", "retired"): + with pytest.raises(RepositoryError): + compute.set_node_status(node_id, status) + + assert compute.get_node(node_id).status == "removed" + + +@integration +def test_the_row_is_kept_because_the_event_trail_refers_to_it(compute, organization) -> None: + """Stated as a fact about the trail rather than as a claim about the code. + + The row stays because `job_events.node_id` refers to it and `job_events` refuses + every update, so a `DELETE` is refused outright and a `SET NULL` is refused by the + trigger. This checks that the trail really does point at the row afterwards, because + that is the reason the row is worth keeping. + """ + + node_id, _key = _live_credential(compute, organization, "kept-for-history") + _online( + compute, + compute.get_node(node_id), + [_device(_node(compute, organization, "kept-device"))], + ) + compute.submit_job(organization, kind="sleep", requested_by="test") + claimed = compute.claim_job(node_id) + assert claimed is not None + + compute.remove_node(node_id, reason="hardware failure") + + events = compute.job_events(claimed.job.id) + assert any( + event["node_id"] is not None and str(event["node_id"]) == str(node_id) + for event in events + ), ( + "no event in the trail names the removed node, so the row is being kept for a " + f"reason that does not apply: {events}" + ) + assert compute.get_node(node_id) is not None + + +@integration +def test_removing_a_node_that_does_not_exist_is_refused(compute, organization) -> None: + """Silently succeeding here would report a removal that never happened.""" + + from umbratrans_web.errors import NotFoundError + + with pytest.raises(NotFoundError): + compute.remove_node(uuid4(), reason="hardware failure") + + +def _audit_events(compute, node_id) -> list[dict]: + with compute._connection() as connection: + return connection.execute( + "SELECT action, metadata FROM public.audit_events " + "WHERE target_type = 'compute_node' AND target_id = %s ORDER BY created_at", + (str(node_id),), + ).fetchall() + + # ---------------------------------------------------------------------- # A paired node can actually do the work the agent asks it to do # ----------------------------------------------------------------------