diff --git a/apps/api/app/routes/session.ts b/apps/api/app/routes/session.ts index e8fb696d..afb1065f 100644 --- a/apps/api/app/routes/session.ts +++ b/apps/api/app/routes/session.ts @@ -5,6 +5,7 @@ import { Examples } from '@nestri/core/examples'; import { Game } from '@nestri/core/game/index'; import { Identifier } from '@nestri/core/id'; import { Session } from '@nestri/core/session/index'; +import { Library } from '@nestri/core/user/library'; import { LinkedAccount } from '@nestri/core/user/linked-account'; import { Hono } from 'hono'; import { describeRoute } from 'hono-openapi'; @@ -136,6 +137,27 @@ export namespace SessionApi { ); } + // A run launches as a Steam account that has to own the game, so + // a game outside the caller's library is a box that starts, tries + // to launch, and fails minutes later with nothing to point at. + // Refusing here is the same answer sooner. + // + // Told apart from a game that does not exist rather than hidden: + // the catalog is public, so there is nothing to hide, and "you do + // not own this" is the sentence a person can act on. + // + // The library is a synced copy, so this refuses a game bought + // since the last sync. That is a staleness bug in the sync and + // not a reason to launch runs that cannot work. + const owned = await Library.findByUserAndGame({ userId, gameId: game.id }); + if (!owned) { + throw new VisibleError( + 'forbidden', + ErrorCodes.Permission.FORBIDDEN, + 'That game is not in your library' + ); + } + const actor = Actor.use(); const linkedAccountId = body.linkedAccountId || @@ -274,7 +296,7 @@ export namespace SessionApi { tags: ['Session'], summary: 'Publish the address a client should connect to', description: - 'For the host the run’s box is placed on, and no other. Republish freely: a later ticket is a better address for the same run, not a second run, and the address changes as more of them are discovered. A run that has stopped has no address, so that is 409.', + 'For the host the run’s box is placed on, and no other. Republish freely: a later ticket is a better address for the same run, not a second run, and the address changes as more of them are discovered. Only a run being brought up has an address: claim it by reporting `starting` first, and expect 409 both before that and once it has stopped.', responses: { 200: { content: { 'application/json': { schema: Result(Session.Info) } }, @@ -307,6 +329,8 @@ export namespace SessionApi { switch (result.outcome) { case 'forbidden': notYours(); + case 'unclaimed': + conflict('Claim this run by reporting `starting` before publishing an address'); case 'closed': conflict('That run has stopped, so it has no address to publish'); default: diff --git a/apps/api/test/session.test.ts b/apps/api/test/session.test.ts index de58463c..b151e32d 100644 --- a/apps/api/test/session.test.ts +++ b/apps/api/test/session.test.ts @@ -8,6 +8,7 @@ import { Game } from '@nestri/core/game/index'; import { Identifier } from '@nestri/core/id'; import { Machine } from '@nestri/core/machine/index'; import { Session } from '@nestri/core/session/index'; +import { Library } from '@nestri/core/user/library'; import { app } from '../app/index'; import './setup'; @@ -64,11 +65,24 @@ async function scene(label: string, steamAppId: number) { name: label }); + const gameId = await newGame(steamAppId); + // A run launches as a Steam account that owns the game, so the endpoint + // refuses one outside the caller's library. Every scene here is about + // something else, so the game is stocked. + await Library.upsert({ + id: Identifier.ascending('userLibrary'), + userId: owner.userId, + gameId, + playtime2w: null, + playtimeForever: null, + lastPlayed: null + }); + return { owner, box, machineId: registered.id, - gameId: await newGame(steamAppId), + gameId, user: { authorization: `Bearer ${pat.token}`, 'content-type': 'application/json' @@ -230,6 +244,27 @@ describe('POST /session', () => { expect(res.status).toBe(403); }); + test('you can only run a game you own', async () => { + const s = await scene('route-unowned', 5560); + // A real game in the catalog, simply not in this person's library. + const unowned = await newGame(5561); + + const res = await app.request('/session', { + method: 'POST', + headers: s.user, + body: JSON.stringify({ + boxId: s.box.id, + gameId: unowned, + linkedAccountId: s.owner.linkedAccountId + }) + }); + // Told apart from a game that does not exist, deliberately: the catalog + // is public, so there is nothing to hide, and a box that starts and + // then cannot launch is a worse answer minutes later. + expect(res.status).toBe(403); + expect(await Session.listByBox(s.box.id)).toHaveLength(0); + }); + test('an unknown game is a 404 and not a foreign key crash', async () => { const s = await scene('route-nogame', 5507); const res = await app.request('/session', { @@ -526,6 +561,33 @@ describe('POST /session/:id/ticket', () => { expect(await Session.listByBox(s.box.id)).toHaveLength(1); }); + test('a run nobody has claimed has no address to publish', async () => { + const s = await scene('route-ticket-early', 5546); + const { body } = await requestSession(s); + + const early = await app.request(`/session/${body.data.id}/ticket`, { + method: 'POST', + headers: s.host, + body: JSON.stringify({ ticket: 'nodeaaa-too-soon' }) + }); + // Publishing before reporting `starting` means the agent skipped the + // claim, which is the only mutual exclusion in the design. + expect(early.status).toBe(409); + expect((await Session.fromID(body.data.id))?.ticket).toBeNull(); + + await app.request(`/session/${body.data.id}/state`, { + method: 'POST', + headers: s.host, + body: JSON.stringify({ state: 'starting' }) + }); + const now = await app.request(`/session/${body.data.id}/ticket`, { + method: 'POST', + headers: s.host, + body: JSON.stringify({ ticket: 'nodeaaa-in-time' }) + }); + expect(now.status).toBe(200); + }); + test('a different host cannot publish an address for someone else’s run', async () => { const mine = await scene('route-ticket-mine', 5541); const theirs = await scene('route-ticket-theirs', 5542); @@ -603,6 +665,33 @@ describe('POST /session/:id/ticket', () => { }); }); +describe('The box a run happens on', () => { + test('the endpoints move the box, not just the run', async () => { + const s = await scene('route-box-state', 5550); + const { body } = await requestSession(s); + const report = (state: string, errorMessage?: string) => + app.request(`/session/${body.data.id}/state`, { + method: 'POST', + headers: s.host, + body: JSON.stringify({ state, errorMessage }) + }); + + expect((await Box.fromID(s.box.id))?.state).toBe('created'); + + await report('starting'); + await report('live'); + // The screens that tell a person what their hardware is doing read the + // box, so a live run has to be visible there and not only on the run. + expect((await Box.fromID(s.box.id))?.state).toBe('running'); + + await report('failed', 'the guest never came up'); + const stopped = await Box.fromID(s.box.id); + expect(stopped?.state).toBe('stopped'); + expect(stopped?.stopClean).toBe(false); + expect(stopped?.stopReason).toBe('the guest never came up'); + }); +}); + describe('Session routes in the spec', () => { test('every path a caller needs is documented', async () => { const res = await app.request('/doc'); diff --git a/packages/core/migrations/0008_session_one_active_run_per_box.sql b/packages/core/migrations/0008_session_one_active_run_per_box.sql index fc391067..da2b4aa4 100644 --- a/packages/core/migrations/0008_session_one_active_run_per_box.sql +++ b/packages/core/migrations/0008_session_one_active_run_per_box.sql @@ -15,8 +15,14 @@ -- -- `ended` and not `failed`: nothing about these runs failed. They were work -- nobody picked up, and `failed` carries a reason there is none of. +-- +-- The ticket goes with the state, exactly as the terminal transitions in +-- `Session` do it. A duplicate that reached `starting` or `live` may have +-- published an address, and a stopped run that still answers with one is an +-- address a polling client would dial — with publishing a replacement already +-- refused, it would also be the last word. UPDATE "session" s -SET "state" = 'ended', "time_stopped" = now() +SET "state" = 'ended', "time_stopped" = now(), "ticket" = NULL WHERE s."time_stopped" IS NULL AND s."time_deleted" IS NULL AND EXISTS ( diff --git a/packages/core/src/session/index.ts b/packages/core/src/session/index.ts index 893da4f1..82f94d4c 100644 --- a/packages/core/src/session/index.ts +++ b/packages/core/src/session/index.ts @@ -1,7 +1,8 @@ -import { and, desc, eq, inArray, isNull, notInArray, sql } from 'drizzle-orm'; +import { and, desc, eq, inArray, isNull, sql } from 'drizzle-orm'; import z from 'zod'; import { BoxTable, BoxTier } from '../box/box.sql.js'; +import { Box } from '../box/index.js'; import { Database } from '../db/index.js'; import { ErrorCodes, VisibleError } from '../error.js'; import { Examples } from '../examples.js'; @@ -497,6 +498,39 @@ export namespace Session { session: Info | null; } + /** + * What a run reaching a state means for the box underneath it. + * + * The box has its own three states and nothing was writing them, so a box + * read `created` while a run on it was `live` — the screens that show a + * person what their hardware is doing would all have been wrong. The two + * state machines are not the same shape and should not be: a box has no + * `starting`, deliberately, because that transition is synchronous from + * the agent's side and a state nobody sets is a state that lies. So only + * the states that mean something to the box are mapped, and `requested` + * and `starting` map to nothing at all. + * + * `failed` is a `stopped` box that did not stop cleanly, which is the + * distinction `stopClean` exists for: "it is not running" and "it faulted" + * are different facts and the difference lives in the reason. + */ + function boxStateFor( + run: Info + ): { state: 'running' | 'stopped'; stopReason: string | null; stopClean: boolean | null } | null { + switch (run.state) { + case 'live': + return { state: 'running', stopReason: null, stopClean: null }; + case 'ended': + return { state: 'stopped', stopReason: null, stopClean: true }; + case 'failed': + // The run's own reason, so a box explains its stop in the words + // the agent used rather than in a second wording of one event. + return { state: 'stopped', stopReason: run.errorMessage ?? null, stopClean: false }; + default: + return null; + } + } + export const transition = fn( z.object({ id: Info.shape.id, @@ -505,40 +539,66 @@ export namespace Session { errorMessage: Info.shape.errorMessage }), async (input): Promise => { - const current = await forMachine({ id: input.id, machineId: input.machineId }); - if (!current) return { outcome: 'forbidden', session: null }; - if (current.state === input.state) return { outcome: 'unchanged', session: current }; - if (!NEXT_STATES[current.state].includes(input.state)) { - return { outcome: 'illegal', session: current }; - } + // One transaction, because "this run is live" and "the box under it + // is running" are one fact written to two tables. Committing the + // first without the second is how a box gets stuck `running` with + // nothing running on it, and nothing here would ever correct it. + return Database.transaction(async (): Promise => { + const current = await forMachine({ id: input.id, machineId: input.machineId }); + if (!current) return { outcome: 'forbidden', session: null }; + if (current.state === input.state) return { outcome: 'unchanged', session: current }; + if (!NEXT_STATES[current.state].includes(input.state)) { + return { outcome: 'illegal', session: current }; + } - const moved = await compareAndSetState({ - id: input.id, - machineId: input.machineId, - from: current.state, - to: input.state, - errorMessage: input.errorMessage + const moved = await compareAndSetState({ + id: input.id, + machineId: input.machineId, + from: current.state, + to: input.state, + errorMessage: input.errorMessage + }); + // The state read above is not the state written below, and the + // gap is where two agents race. Nothing moved means somebody + // else did — and then the box is that caller's to update, not + // this one's. + if (!moved) return { outcome: 'lost', session: current }; + + const box = boxStateFor(moved); + if (box) { + await Box.setState({ id: moved.boxId, ...box }); + } + + return { outcome: 'moved', session: moved }; }); - // The state read above is not the state written below, and the gap - // is where two agents race. Nothing moved means somebody else did. - if (!moved) return { outcome: 'lost', session: current }; - return { outcome: 'moved', session: moved }; } ); export interface TicketResult { - outcome: 'forbidden' | 'closed' | 'published'; + outcome: 'forbidden' | 'unclaimed' | 'closed' | 'published'; session: Info | null; } + /** + * The states a run can have an address in. + * + * `starting` is in and `requested` is out, which is the whole distinction: + * a ticket is the address of something being brought up, so publishing one + * means the host has taken the work. It cannot have an address for a run it + * has not claimed, and the terminal states are out because a run that is + * not there has no address at all. + */ + const ADDRESSABLE = ['starting', 'live'] as const; + /** * Publish a ticket for a run, on behalf of the host it is placed on. * * A ticket may appear while the state is still `starting` — it is * republished as addresses are discovered, so the client polls and re-reads - * rather than keeping the first one. A run that has stopped is refused: an - * address for something that is not there can only mislead whoever is - * still polling. + * rather than keeping the first one. Outside {@link ADDRESSABLE} it is + * refused, and the two refusals are separate answers because they are + * different mistakes: a run not yet claimed is an agent that skipped a + * step, and a run that stopped is one that has nothing left to reach. */ export const publishTicket = fn( z.object({ @@ -549,6 +609,7 @@ export namespace Session { async (input): Promise => { const current = await forMachine({ id: input.id, machineId: input.machineId }); if (!current) return { outcome: 'forbidden', session: null }; + if (current.state === 'requested') return { outcome: 'unclaimed', session: current }; return Database.use(async (tx) => { return tx @@ -557,7 +618,10 @@ export namespace Session { .where( and( eq(SessionTable.id, input.id), - notInArray(SessionTable.state, ['ended', 'failed']), + // The state is in the write and not only in the check + // above it, so a run that stops underneath this call + // does not acquire an address on the way out. + inArray(SessionTable.state, [...ADDRESSABLE]), isNull(SessionTable.timeDeleted), inArray(SessionTable.boxId, boxesOn(tx, input.machineId)) ) diff --git a/packages/core/src/session/session.test.ts b/packages/core/src/session/session.test.ts index 4da9c9d5..84b16347 100644 --- a/packages/core/src/session/session.test.ts +++ b/packages/core/src/session/session.test.ts @@ -530,3 +530,111 @@ describe('Session tickets and the end of a run', () => { expect(ended?.ticket).toBeNull(); }); }); + +describe('Session and the box underneath it', () => { + test('a live run is what makes its box running', async () => { + const { machineId, box, session } = await requestedRun('ses-box-live', 5460); + // A box starts out `created` and nothing had ever moved it, so it read + // `created` while a run on it was `live`. + expect((await Box.fromID(box.id))?.state).toBe('created'); + + await Session.transition({ id: session.id, machineId, state: 'starting', errorMessage: null }); + // `starting` is deliberately not a box state: that transition is + // synchronous from the agent's side, so nothing would ever write it. + expect((await Box.fromID(box.id))?.state).toBe('created'); + + await Session.transition({ id: session.id, machineId, state: 'live', errorMessage: null }); + const running = await Box.fromID(box.id); + expect(running?.state).toBe('running'); + expect(running?.stopReason).toBeNull(); + expect(running?.stopClean).toBeNull(); + }); + + test('a run that ends stops its box, cleanly', async () => { + const { machineId, box, session } = await requestedRun('ses-box-ended', 5461); + await Session.transition({ id: session.id, machineId, state: 'starting', errorMessage: null }); + await Session.transition({ id: session.id, machineId, state: 'live', errorMessage: null }); + await Session.transition({ id: session.id, machineId, state: 'ended', errorMessage: null }); + + const stopped = await Box.fromID(box.id); + expect(stopped?.state).toBe('stopped'); + expect(stopped?.stopClean).toBe(true); + expect(stopped?.stopReason).toBeNull(); + }); + + test('a run that fails stops its box in the words the agent used', async () => { + const { machineId, box, session } = await requestedRun('ses-box-failed', 5462); + await Session.transition({ id: session.id, machineId, state: 'starting', errorMessage: null }); + await Session.transition({ + id: session.id, + machineId, + state: 'failed', + errorMessage: 'the guest never came up' + }); + + const stopped = await Box.fromID(box.id); + expect(stopped?.state).toBe('stopped'); + // "It is not running" and "it faulted" are different facts, and the + // difference lives in the reason rather than in a fourth state. + expect(stopped?.stopClean).toBe(false); + expect(stopped?.stopReason).toBe('the guest never came up'); + }); + + test('a refused report leaves the box alone', async () => { + const { machineId, box, session } = await requestedRun('ses-box-untouched', 5463); + const other = await scene('ses-box-otherhost', 5464); + + const refused = await Session.transition({ + id: session.id, + machineId: other.machineId, + state: 'starting', + errorMessage: null + }); + expect(refused.outcome).toBe('forbidden'); + + // An illegal transition does not move the run, so it must not move the + // box either — otherwise the box records a run that never happened. + const illegal = await Session.transition({ + id: session.id, + machineId, + state: 'live', + errorMessage: null + }); + expect(illegal.outcome).toBe('illegal'); + expect((await Box.fromID(box.id))?.state).toBe('created'); + }); +}); + +describe('Session tickets need a claim first', () => { + test('a run nobody has claimed has no address to publish', async () => { + const { machineId, session } = await requestedRun('ses-ticket-unclaimed', 5465); + + // A ticket is the address of something being brought up, so publishing + // one for a `requested` run means the agent skipped the claim — the + // step that is the only mutual exclusion in the design. + const early = await Session.publishTicket({ id: session.id, machineId, ticket: 'too-soon' }); + expect(early.outcome).toBe('unclaimed'); + expect((await Session.fromID(session.id))?.ticket).toBeNull(); + + await Session.transition({ id: session.id, machineId, state: 'starting', errorMessage: null }); + const now = await Session.publishTicket({ id: session.id, machineId, ticket: 'in-time' }); + expect(now.outcome).toBe('published'); + expect(now.session?.ticket).toBe('in-time'); + }); + + test('the two refusals are different answers, because they are different mistakes', async () => { + const { machineId, session } = await requestedRun('ses-ticket-refusals', 5466); + const unclaimed = await Session.publishTicket({ id: session.id, machineId, ticket: 'a' }); + + await Session.transition({ id: session.id, machineId, state: 'starting', errorMessage: null }); + await Session.transition({ id: session.id, machineId, state: 'live', errorMessage: null }); + await Session.transition({ id: session.id, machineId, state: 'ended', errorMessage: null }); + const closed = await Session.publishTicket({ id: session.id, machineId, ticket: 'b' }); + + // One is an agent that has not claimed the work; the other is a run + // with nothing left to reach. Collapsing them would tell an agent + // retrying the wrong thing. + expect(unclaimed.outcome).toBe('unclaimed'); + expect(closed.outcome).toBe('closed'); + }); +});