mirror of
https://github.com/nestriness/nestri.git
synced 2026-09-19 09:15:19 +03:00
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
This commit is contained in:
@@ -239,7 +239,8 @@ export namespace MachineApi {
|
||||
},
|
||||
400: ErrorResponses[400],
|
||||
403: ErrorResponses[403],
|
||||
404: ErrorResponses[404]
|
||||
404: ErrorResponses[404],
|
||||
409: ErrorResponses[409]
|
||||
}
|
||||
}),
|
||||
validator(
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
}
|
||||
);
|
||||
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
Reference in New Issue
Block a user