From 51d25f3e8ce48d4eadef868736334d5dfbd5c05c Mon Sep 17 00:00:00 2001 From: Wanjohi Date: Mon, 7 Sep 2026 00:19:34 +0300 Subject: [PATCH] fix(machine): a taken endpoint id is a conflict, not a server fault A host reporting an endpoint id another machine already holds hit the unique index, and the raw refusal reached the global handler as a 500 -- telling a host its beat broke the server rather than that the id is taken. It is now the 409 every other conflict here gives, and the route documents it. Checked-then-written would be worse rather than better: two hosts reporting the same id in the same instant both read "nobody holds it" and both write, which is precisely what the index is for. The read would add a query and remove nothing. Before: expect(res.status).toBe(409) Received: 500 --- apps/api/app/routes/machine.ts | 3 +- apps/api/test/heartbeat.test.ts | 25 ++++++++++ packages/core/src/machine/index.ts | 59 +++++++++++++++++------ packages/core/src/machine/machine.test.ts | 8 ++- 4 files changed, 78 insertions(+), 17 deletions(-) diff --git a/apps/api/app/routes/machine.ts b/apps/api/app/routes/machine.ts index 0698b9ce..08054944 100644 --- a/apps/api/app/routes/machine.ts +++ b/apps/api/app/routes/machine.ts @@ -239,7 +239,8 @@ export namespace MachineApi { }, 400: ErrorResponses[400], 403: ErrorResponses[403], - 404: ErrorResponses[404] + 404: ErrorResponses[404], + 409: ErrorResponses[409] } }), validator( diff --git a/apps/api/test/heartbeat.test.ts b/apps/api/test/heartbeat.test.ts index 59119c3d..0c85e1b0 100644 --- a/apps/api/test/heartbeat.test.ts +++ b/apps/api/test/heartbeat.test.ts @@ -157,6 +157,31 @@ describe('POST /machine/heartbeat', () => { expect((await Machine.fromID(host.id))?.lastSeen).not.toBeNull(); }); + test('claiming another host’s endpoint id is a conflict, not a fault', async () => { + const first = await registeredHost('beat-endpoint-taken-a'); + const second = await registeredHost('beat-endpoint-taken-b'); + const endpointId = 'f'.repeat(64); + + await app.request('/machine/heartbeat', { + method: 'POST', + headers: { ...first.headers, 'content-type': 'application/json' }, + body: JSON.stringify({ endpointId }) + }); + + const res = await app.request('/machine/heartbeat', { + method: 'POST', + headers: { ...second.headers, 'content-type': 'application/json' }, + body: JSON.stringify({ endpointId }) + }); + + // The unique index is the invariant, so the database refusing is the + // expected way to find out — and an expected refusal reaching a host as + // a 500 tells it the server broke rather than that the id is taken. + expect(res.status).toBe(409); + expect((await res.json()) as any).toMatchObject({ type: 'already_exists' }); + expect((await Machine.fromID(second.id))?.endpointId).toBeNull(); + }); + test('a user session cannot beat on a host’s behalf', async () => { // A box holds credentials but is not its owner, and the reverse holds // too: `machineOnly` exists so a route written for a host cannot be diff --git a/packages/core/src/machine/index.ts b/packages/core/src/machine/index.ts index 15019ba1..465c22fd 100644 --- a/packages/core/src/machine/index.ts +++ b/packages/core/src/machine/index.ts @@ -4,6 +4,7 @@ import { and, eq, isNull, sql } from 'drizzle-orm'; import z from 'zod'; import { Database } from '../db/index.js'; +import { ErrorCodes, VisibleError } from '../error.js'; import { Examples } from '../examples.js'; import { fn } from '../fn.js'; import { Member } from '../team/member.js'; @@ -83,6 +84,12 @@ export namespace Machine { .join(''); } + /** Postgres refusing a second row for the same key. */ + function isUniqueViolation(err: unknown): boolean { + const e = err as { code?: string; cause?: { code?: string } }; + return e?.code === '23505' || e?.cause?.code === '23505'; + } + /** Length-independent, content-constant comparison of two hex digests. */ function secureEquals(a: string, b: string): boolean { if (a.length !== b.length) { @@ -224,21 +231,43 @@ export namespace Machine { export const touchLastSeen = fn( Info.pick({ id: true }).extend({ endpointId: EndpointId.optional() }), async (input) => { - return Database.use(async (tx) => { - return tx - .update(MachineTable) - .set({ - lastSeen: sql`now()`, - // Omitted rather than nulled when it is absent: a caller - // that does not mention where it is has not moved, and - // clearing the column would deregister a working host - // from every route that reads it. - ...(input.endpointId ? { endpointId: input.endpointId } : {}) - }) - .where(eq(MachineTable.id, input.id)) - .returning({ lastSeen: MachineTable.lastSeen }) - .then((rows) => rows.at(0)?.lastSeen ?? null); - }); + try { + return await Database.use(async (tx) => { + return tx + .update(MachineTable) + .set({ + lastSeen: sql`now()`, + // Omitted rather than nulled when it is absent: a caller + // that does not mention where it is has not moved, and + // clearing the column would deregister a working host + // from every route that reads it. + ...(input.endpointId ? { endpointId: input.endpointId } : {}) + }) + .where(eq(MachineTable.id, input.id)) + .returning({ lastSeen: MachineTable.lastSeen }) + .then((rows) => rows.at(0)?.lastSeen ?? null); + }); + } catch (err) { + // Another host already holds this endpoint id. That is a + // conflict rather than a fault: the unique index is the + // invariant, so the database refusing is the expected way to + // find out, and letting it surface as a 500 would tell a host + // its beat broke the server. + // + // Checked-then-written would be worse rather than better. Two + // hosts reporting the same id in the same instant both read + // "nobody holds it" and both write, which is precisely what the + // index is for — so the read would add a query and remove + // nothing. + if (isUniqueViolation(err)) { + throw new VisibleError( + 'already_exists', + ErrorCodes.Validation.ALREADY_EXISTS, + 'Another machine is already reachable at that endpoint id' + ); + } + throw err; + } } ); diff --git a/packages/core/src/machine/machine.test.ts b/packages/core/src/machine/machine.test.ts index 09a8a67f..97cfcf5f 100644 --- a/packages/core/src/machine/machine.test.ts +++ b/packages/core/src/machine/machine.test.ts @@ -142,7 +142,13 @@ describe('Machine heartbeat', () => { // Two rows claiming one endpoint id would send a request addressed to // one machine to another machine's agent, and the authorisation in // front of it cannot catch that. - expect(Machine.touchLastSeen({ id: second, endpointId })).rejects.toThrow(); + // + // A conflict rather than a fault, and that distinction is the test: the + // database refusing is the *expected* way to find out, so it must not + // reach a host as "your beat broke the server". + await expect(Machine.touchLastSeen({ id: second, endpointId })).rejects.toMatchObject({ + type: 'already_exists' + }); }); test('online is derived from the last beat, not stored', async () => {