Skip to content

Commit a825e4b

Browse files
authored
Merge pull request #5 from ProofOfTechOrg/m3-qa-hardening
fix(backend): fail fast on disconnected sessions + honest error taxonomy
2 parents ed65f5a + b1b778c commit a825e4b

9 files changed

Lines changed: 471 additions & 73 deletions

File tree

‎apps/backend/README.md‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,25 @@ from the `SessionAgent` class name); the Hono app claims everything else, includ
2323
Command flow: `POST /v1/sessions/:sessionId/commands` → `authenticate` (bearer
2424
caller token → `{actor, tenantId}`, 401 on failure) → `scopeSession` (verifies the
2525
sessionId's embedded tenant matches the caller's, 404 on mismatch) → `safeParseCommand`
26-
(400 on schema failure) → `stub.dispatch()` or, for `fill_secret`, `stub.fillSecret()`
26+
(400 on an unparseable body or schema failure) → `stub.dispatch()` or, for `fill_secret`, `stub.fillSecret()`
2727
→ the correlated Event is returned as JSON. This order is load-bearing: an
2828
unauthenticated or cross-tenant request never reaches parsing or dispatch.
2929

30+
Dispatch failures map to status codes by error-message prefix (the only signal
31+
that survives the DO RPC boundary; the constants live in `src/coordinator.ts`):
32+
**503** `{error: "extension not connected"}` when the session has no live,
33+
onConnect-authorized extension socket — the gate consults that delivery
34+
predicate directly (not the persisted `status` scalar, which a late close from
35+
a replaced socket can leave stale), fails fast instead of burning the 30s
36+
timeout, and answers non-2xx deliberately: a 200 `ok:false` Event would be
37+
cached by a consumer's idempotency store and replayed after reconnect;
38+
**504** `{error: "command timed out"}` when a connected extension never
39+
answered; anything else is a uniform JSON **500** via `app.onError`. A real
40+
`fill_secret` checks the same predicate *before* touching the vault, so no
41+
plaintext is ever resolved for a command that cannot dispatch. Exception:
42+
a ref-less dryRun write (e.g. navigate) still short-circuits to simulated
43+
`ok:true` without touching the wire — it was never a liveness signal.
44+
3045
Inside the DO, `SessionAgent` holds a `CfSessionCoordinator` (the Cloudflare
3146
implementation of the portable `SessionCoordinator` interface). `send(cmd)` writes
3247
the command to the extension WebSocket, parks a `{resolve, reject, timer}` in an
@@ -143,8 +158,10 @@ the connection set during that window. Four things close this gap:
143158

144159
## Invariants
145160

146-
- Every `Command` produces exactly one `Event` bearing its `commandId`; the
147-
command route returns exactly that Event.
161+
- Every *successfully dispatched* `Command` produces exactly one `Event`
162+
bearing its `commandId`, and the command route returns exactly that Event;
163+
a command that cannot dispatch or never resolves maps to a non-2xx JSON
164+
error instead (503/504/500 — see the failure mapping above).
148165
- `fill_secret` plaintext never enters `setState`, logs, the Event response, or an
149166
error string; the coordinator logs only `{commandId, type}`.
150167
- One Durable Object per `sessionId`; a sessionId whose embedded tenant disagrees

‎apps/backend/src/coordinator-cf.ts‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
*/
1010

1111
import type { Command, Event } from "@understudy/protocol";
12+
import { COMMAND_TIMED_OUT, SESSION_NOT_CONNECTED } from "./coordinator";
1213
import type { PendingCommand, PendingMap, SessionCoordinator } from "./coordinator";
1314
import type { SessionStatus } from "./types";
1415

@@ -22,6 +23,14 @@ const DEFAULT_TIMEOUT_MS = 30_000;
2223
export interface CoordinatorHost {
2324
/** Writes a JSON frame to the live extension WebSocket. */
2425
sendToExtension(payload: string): void;
26+
/**
27+
* The delivery predicate: does a live, onConnect-authorized extension
28+
* socket exist right now? This is the exact precondition sendToExtension
29+
* relies on - NOT the persisted SessionState.status scalar, which is a
30+
* lossy last-writer-wins echo of it (a late onClose from a replaced
31+
* socket can stamp "detached" over a healthy reconnected session).
32+
*/
33+
hasAuthorizedConnection(): boolean;
2534
/** Reads the persisted awaiting-commandId marker (SessionState.awaitingCommandIds). */
2635
getAwaitingCommandIds(): string[];
2736
/** Persists the awaiting-commandId marker via the DO's setState. */
@@ -64,10 +73,21 @@ export class CfSessionCoordinator implements SessionCoordinator {
6473
* always settles even if the WebSocket is gone.
6574
*/
6675
send(cmd: Command): Promise<Event> {
76+
// Fail fast instead of parking a promise that can only time out: with no
77+
// deliverable socket, sendToExtension writes to nobody, so the 30s
78+
// timeout (surfacing as an opaque 500) would be the guaranteed outcome -
79+
// the M3 e2e's QA finding. The Worker maps this prefix to a 503. A
80+
// socket that dies between this check and the write still ends in the
81+
// timeout: fail-fast is best-effort, the timer stays the backstop.
82+
if (!this.host.hasAuthorizedConnection()) {
83+
return Promise.reject(
84+
new Error(`${SESSION_NOT_CONNECTED}: no authorized extension connection`),
85+
);
86+
}
6787
return new Promise<Event>((resolve, reject) => {
6888
const timer = setTimeout(() => {
6989
reject(
70-
new Error(`command ${cmd.commandId} (${cmd.type}) timed out after ${this.timeoutMs}ms`),
90+
new Error(`${COMMAND_TIMED_OUT}: ${cmd.commandId} (${cmd.type}) after ${this.timeoutMs}ms`),
7191
);
7292
this.pending.delete(cmd.commandId);
7393
this.dropAwaiting(cmd.commandId);

‎apps/backend/src/coordinator.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,16 @@
1212
import type { Command, Event } from "@understudy/protocol";
1313
import type { SessionStatus } from "./types";
1414

15+
/**
16+
* send()'s delivery-failure vocabulary. A message prefix is the only failure
17+
* signal that survives an RPC boundary (custom Error subclasses do not), so
18+
* every throw site and every host mapping failures onto its transport (the
19+
* Cloudflare Worker maps these to 503/504 in index.ts) shares these
20+
* constants instead of free-typing the strings.
21+
*/
22+
export const SESSION_NOT_CONNECTED = "session not connected";
23+
export const COMMAND_TIMED_OUT = "command timed out";
24+
1525
/** One outstanding `send(cmd)` call, awaiting its correlated Event. */
1626
export interface PendingCommand {
1727
resolve: (ev: Event) => void;

‎apps/backend/src/index.ts‎

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { Hono } from "hono";
1414
import { getAgentByName, routeAgentRequest } from "agents";
1515
import { safeParseCommand } from "@understudy/protocol";
1616
import { authenticate, mintSessionId, scopeSession } from "./auth";
17+
import { COMMAND_TIMED_OUT, SESSION_NOT_CONNECTED } from "./coordinator";
1718
import type { Env } from "./types";
1819
import type { SessionAgent } from "./session";
1920

@@ -60,7 +61,14 @@ app.post("/v1/sessions/:sessionId/commands", async (c) => {
6061
const scope = await scopeSession(sessionId, actor.tenantId, c.env);
6162
if (scope === "not-found") return c.json({ error: "not found" }, 404);
6263

63-
const body = await c.req.json();
64+
// Unparseable JSON is the client's fault: 400, not a rethrow into the
65+
// uniform 500 (which would dress a client error as a server error).
66+
let body: { command?: unknown; dryRun?: unknown };
67+
try {
68+
body = await c.req.json();
69+
} catch {
70+
return c.json({ error: "invalid body" }, 400);
71+
}
6472
const parsed = safeParseCommand(body?.command);
6573
if (!parsed.success) return c.json({ error: "invalid command" }, 400);
6674

@@ -70,12 +78,36 @@ app.post("/v1/sessions/:sessionId/commands", async (c) => {
7078
// carrying a secret through this route (DL-004).
7179
const dryRun = body?.dryRun === true;
7280
const stub = await getSessionStub(c.env, sessionId);
73-
const event =
74-
parsed.data.type === "fill_secret"
75-
? await stub.fillSecret(parsed.data, dryRun)
76-
: await stub.dispatch(parsed.data, dryRun);
81+
try {
82+
const event =
83+
parsed.data.type === "fill_secret"
84+
? await stub.fillSecret(parsed.data, dryRun)
85+
: await stub.dispatch(parsed.data, dryRun);
86+
return c.json(event);
87+
} catch (err) {
88+
// The DO's throw crosses the RPC boundary as a plain Error; its message
89+
// prefix is the contract (coordinator.ts). "Not connected" is 503 - a
90+
// retryable infrastructure state - deliberately NOT a 200 ok:false
91+
// Event, which a consumer's idempotency store would cache and replay
92+
// even after the extension reconnects.
93+
const message = err instanceof Error ? err.message : String(err);
94+
if (message.startsWith(SESSION_NOT_CONNECTED)) {
95+
return c.json({ error: "extension not connected" }, 503);
96+
}
97+
if (message.startsWith(COMMAND_TIMED_OUT)) {
98+
return c.json({ error: "command timed out" }, 504);
99+
}
100+
throw err;
101+
}
102+
});
77103

78-
return c.json(event);
104+
// Anything a route throws (or rethrows above) becomes a uniform JSON 500
105+
// instead of workerd's opaque non-JSON error page. Command payloads never
106+
// enter error messages by construction (DL-004), so logging the message
107+
// leaks nothing; the response body stays generic regardless.
108+
app.onError((err, c) => {
109+
console.error("unhandled route error", err.message);
110+
return c.json({ error: "internal error" }, 500);
79111
});
80112

81113
export default {

‎apps/backend/src/session.ts‎

Lines changed: 55 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type { AgentContext, Connection, ConnectionContext, WSMessage } from "age
33
import { isWriteCommand, safeParseEvent } from "@understudy/protocol";
44
import type { Command, Event } from "@understudy/protocol";
55
import { scopeSession, verifyExtensionToken } from "./auth";
6+
import { SESSION_NOT_CONNECTED } from "./coordinator";
67
import { CfSessionCoordinator } from "./coordinator-cf";
78
import { resolveSecret } from "./secrets";
89
import type { Env, SessionState, SessionStatus } from "./types";
@@ -35,6 +36,7 @@ export class SessionAgent extends Agent<Env, SessionState> {
3536
if (this.isAuthorizedConnection(connection)) connection.send(payload);
3637
}
3738
},
39+
hasAuthorizedConnection: () => this.hasAuthorizedConnection(),
3840
getAwaitingCommandIds: () => this.state.awaitingCommandIds,
3941
persistAwaitingCommandIds: (ids) => this.setState({ ...this.state, awaitingCommandIds: ids }),
4042
persistStatus: (status) => this.setState({ ...this.state, status }),
@@ -125,7 +127,14 @@ export class SessionAgent extends Agent<Env, SessionState> {
125127
}
126128

127129
async onClose(connection: Connection, code: number, reason: string, wasClean: boolean): Promise<void> {
128-
this.setState({ ...this.state, status: "detached" });
130+
// Only the LAST authorized socket's close detaches the session: a late
131+
// close event from a replaced socket must not stamp "detached" over a
132+
// healthy reconnect (the closing connection is excluded explicitly, in
133+
// case the SDK has not yet reaped it from the connection set).
134+
const stillLive = [...this.getConnections()].some(
135+
(c) => c !== connection && this.isAuthorizedConnection(c),
136+
);
137+
if (!stillLive) this.setState({ ...this.state, status: "detached" });
129138
}
130139

131140
async dispatch(command: Command, dryRun?: boolean): Promise<Event> {
@@ -136,26 +145,20 @@ export class SessionAgent extends Agent<Env, SessionState> {
136145
return this.coordinator.send(command);
137146
}
138147

139-
const ok = await this.checkRefResolves(this.commandRef(command));
140-
return {
141-
type: "action_result",
142-
commandId: command.commandId,
143-
ok,
144-
...(ok ? {} : { error: "dry-run: ref did not resolve" }),
145-
simulated: true,
146-
};
148+
const probe = await this.checkRefResolves(this.commandRef(command));
149+
return this.simulatedResult(command.commandId, probe);
147150
}
148151

149152
async fillSecret(cmd: FillSecretCommand, dryRun?: boolean): Promise<Event> {
150153
if (dryRun) {
151-
const ok = await this.checkRefResolves(cmd.ref);
152-
return {
153-
type: "action_result",
154-
commandId: cmd.commandId,
155-
ok,
156-
...(ok ? {} : { error: "dry-run: ref did not resolve" }),
157-
simulated: true,
158-
};
154+
return this.simulatedResult(cmd.commandId, await this.checkRefResolves(cmd.ref));
155+
}
156+
157+
// Gate BEFORE the vault: resolving a secret for a command that cannot
158+
// dispatch would materialize plaintext (and emit a vault access) for
159+
// nothing - fail-fast matters most exactly here (DL-004).
160+
if (!this.hasAuthorizedConnection()) {
161+
throw new Error(`${SESSION_NOT_CONNECTED}: no authorized extension connection`);
159162
}
160163

161164
let secret: string;
@@ -198,15 +201,37 @@ export class SessionAgent extends Agent<Env, SessionState> {
198201
// (generation bump), so it can never contain the consumer's ref AND it
199202
// invalidates the consumer's outstanding refs, breaking the approved
200203
// command that follows the dry-run.
201-
private async checkRefResolves(ref: string | undefined): Promise<boolean> {
202-
if (ref === undefined) return true;
204+
private async checkRefResolves(
205+
ref: string | undefined,
206+
): Promise<{ ok: true } | { ok: false; reason: string }> {
207+
if (ref === undefined) return { ok: true };
203208

204209
const ev = await this.coordinator.send({
205210
type: "resolve_ref",
206211
commandId: crypto.randomUUID(),
207212
ref,
208213
});
209-
return ev.type === "action_result" && ev.ok;
214+
if (ev.type !== "action_result") {
215+
return { ok: false, reason: `unexpected probe response '${ev.type}'` };
216+
}
217+
if (ev.ok) return { ok: true };
218+
// Surface the extension's own reason (e.g. "stale or unknown ref: s1e2")
219+
// instead of collapsing every probe failure into one generic string.
220+
return { ok: false, reason: ev.error ?? "ref did not resolve" };
221+
}
222+
223+
/** The simulated action_result a dry-run returns in place of dispatching. */
224+
private simulatedResult(
225+
commandId: string,
226+
probe: { ok: true } | { ok: false; reason: string },
227+
): Event {
228+
return {
229+
type: "action_result",
230+
commandId,
231+
ok: probe.ok,
232+
...(probe.ok ? {} : { error: `dry-run: ${probe.reason}` }),
233+
simulated: true,
234+
};
210235
}
211236

212237
private commandRef(command: Command): string | undefined {
@@ -224,4 +249,14 @@ export class SessionAgent extends Agent<Env, SessionState> {
224249
private isAuthorizedConnection(connection: Connection): boolean {
225250
return (connection.state as { authorized?: boolean } | null)?.authorized === true;
226251
}
252+
253+
// The delivery predicate the coordinator's fail-fast gate consults: the
254+
// same precondition sendToExtension relies on, NOT the persisted status
255+
// scalar (which a late onClose from a replaced socket can leave stale).
256+
private hasAuthorizedConnection(): boolean {
257+
for (const connection of this.getConnections()) {
258+
if (this.isAuthorizedConnection(connection)) return true;
259+
}
260+
return false;
261+
}
227262
}

‎apps/backend/src/types.ts‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,16 @@ export interface Env {
3838
VAULT: VaultBinding;
3939
/** Signs/verifies server-minted sessionIds so scopeSession can verify tenant ownership statelessly (M-006, DL-008). */
4040
AUTH_HMAC_SECRET: string;
41-
/** Static caller-token -> tenantId map for the dev auth verifier (M-006). */
42-
CALLER_TOKENS?: string;
43-
/** Extension per-user token(s), verified independently of caller auth. */
44-
EXTENSION_TOKENS?: string;
41+
/**
42+
* Static caller-token -> tenantId map (JSON) for the dev auth verifier
43+
* (M-006). Required, not optional: wrangler.jsonc lists it in
44+
* `secrets.required`, which is also the .dev.vars allowlist, so a
45+
* deployment without it cannot start. auth.ts still guards the empty
46+
* string at runtime.
47+
*/
48+
CALLER_TOKENS: string;
49+
/** Extension per-user token(s) (JSON), verified independently of caller auth. Required via `secrets.required`, like CALLER_TOKENS. */
50+
EXTENSION_TOKENS: string;
4551
}
4652

4753
/**

‎apps/backend/test/coordinator.test.ts‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,14 @@ import { describe, it, expect, vi } from "vitest";
22
import { CfSessionCoordinator, type CoordinatorHost } from "../src/coordinator-cf";
33
import type { Command, Event } from "@understudy/protocol";
44

5-
function createFakeHost(): CoordinatorHost & { sent: string[] } {
5+
function createFakeHost(connected = true): CoordinatorHost & { sent: string[] } {
66
let awaiting: string[] = [];
77
const sent: string[] = [];
88
return {
99
sendToExtension: (payload: string) => {
1010
sent.push(payload);
1111
},
12+
hasAuthorizedConnection: () => connected,
1213
getAwaitingCommandIds: () => awaiting,
1314
persistAwaitingCommandIds: (ids: string[]) => {
1415
awaiting = ids;
@@ -53,14 +54,32 @@ describe("CfSessionCoordinator", () => {
5354

5455
// #then it rejects with a payload-free error and the marker is cleared
5556
expect(err).toBeInstanceOf(Error);
56-
expect((err as Error).message).toBe("command c2 (click) timed out after 1000ms");
57+
expect((err as Error).message).toBe("command timed out: c2 (click) after 1000ms");
5758
expect((err as Error).message).not.toContain("r1");
5859
expect(host.getAwaitingCommandIds()).toEqual([]);
5960
} finally {
6061
vi.useRealTimers();
6162
}
6263
});
6364

65+
it("rejects immediately when no authorized socket exists - no marker, no frame, no timer burn", async () => {
66+
// #given a host whose session has no live authorized extension socket
67+
const host = createFakeHost(false);
68+
const coordinator = new CfSessionCoordinator(host);
69+
const cmd: Command = { type: "get_tabs", commandId: "c-fast" };
70+
71+
// #when send() is called
72+
const err = await coordinator.send(cmd).catch((e: unknown) => e);
73+
74+
// #then it rejects with the route-mappable prefix before parking anything
75+
expect(err).toBeInstanceOf(Error);
76+
expect((err as Error).message).toBe(
77+
"session not connected: no authorized extension connection",
78+
);
79+
expect(host.getAwaitingCommandIds()).toEqual([]);
80+
expect(host.sent).toEqual([]);
81+
});
82+
6483
it("no-ops on an unknown commandId without throwing or touching the marker", () => {
6584
// #given a coordinator with nothing pending and an empty marker
6685
const host = createFakeHost();

0 commit comments

Comments
 (0)