apps/notify some notify fixes

2026-09-09 15:11:29 -07:00 · 2026-07-21 19:32:38 -04:00
parent 40c38d7a18
commit ec324558a0
3 changed files with 600 additions and 38 deletions
@@ -15,10 +15,14 @@ import type { Env } from './context'
 *      3 = completion, 6 = ping, 7 = close.
 *
 * Connection/subscription state lives in SQLite so it survives hibernation:
- *   - `subscriptions(connectionId, playerId)` — both the connection→players and
- *     (queried the other way) the player→connections maps.
+ *   - `connection_owner(connectionId, playerId)` — who each socket belongs to,
+ *     from the token the worker validated at connect. This is how a player's own
+ *     notifications reach them; the client never subscribes to itself.
+ *   - `subscriptions(connectionId, playerId)` — *other* players a connection asked
+ *     for updates about, both the connection→players and (queried the other way)
+ *     the player→connections maps.
 *   - `pending(id, playerId, payload)` — the per-player queue delivered once a
- *     player is subscribed.
+ *     player is reachable again.
 */

 /** SignalR record separator (0x1e) that terminates every protocol message. */
@@ -27,6 +31,8 @@ const RS = '\u001e'
 interface SocketState {
 	connectionId: string
 	handshakeDone: boolean
+	/** The player this socket belongs to, from the token validated at connect. */
+	playerId?: number
 }

 interface HubMessage {
@@ -36,6 +42,63 @@ interface HubMessage {
 	arguments?: unknown[]
 }

+/**
+ * How the worker tells the DO which player a connecting socket belongs to, having
+ * validated the connect request's token. The worker sets it on every connect it lets
+ * through and refuses the rest, so a client can't present its own and be believed.
+ */
+export const OWNER_HEADER = 'x-recflare-connection-owner'
+
+/**
+ * Read the player ids off a `SubscribeToPlayers` invocation. SignalR gives no schema, so
+ * all three plausible spellings are accepted — an options object
+ * (`[{playerIds:[1,2]}]`), a single array argument (`[[1,2]]`), and varargs
+ * (`[1,2]`) — rather than guessing which one the client uses.
+ *
+ * `null` means the argument didn't resolve to a list of ids at all, which the caller
+ * must not treat as an empty subscription; `[]` is a real "subscribe to nobody".
+ */
+function parsePlayerIds(args: unknown[] | undefined): number[] | null {
+	if (args === undefined || args.length === 0) return []
+
+	const first = args[0]
+	const candidate =
+		typeof first === 'object' && first !== null && !Array.isArray(first)
+			? (first as { playerIds?: unknown }).playerIds
+			: Array.isArray(first)
+				? first
+				: args
+
+	if (!Array.isArray(candidate)) return null
+	// Ids arriving as strings ("153") still count — the wire format is the client's
+	// choice, and the subscriptions table is what has to be numeric.
+	const ids = candidate
+		.map((value) =>
+			typeof value === 'number' ? value : typeof value === 'string' ? Number(value) : Number.NaN
+		)
+		.filter((value) => Number.isInteger(value))
+	return ids.length === 0 && candidate.length > 0 ? null : ids
+}
+
+/** What {@link NotificationsHub.inspect} reports — see it for what each field is for. */
+export interface HubState {
+	connections: Array<{
+		connectionId: string
+		/** Whether a socket for this connectionId is still held by the DO. */
+		live: boolean
+		handshakeDone: boolean
+		/**
+		 * The player this connection belongs to. Null only for a connection that predates
+		 * authenticated connects and hasn't closed yet — a new one always has an owner.
+		 */
+		playerId: number | null
+		/** Other players this connection subscribed to updates for. */
+		playerIds: number[]
+	}>
+	/** Queued-while-offline notifications, newest payload per player for identification. */
+	pending: Array<{ playerId: number; count: number; latest: string }>
+}
+
 /** The Coach system account — the `FromPlayerId` on a coach message (see coachMessageAll). */
 const COACH_PLAYER_ID = 1

@@ -59,6 +122,11 @@ export class NotificationsHub extends DurableObject<Env> {
 					payload TEXT NOT NULL
 				);
 				CREATE INDEX IF NOT EXISTS idx_pending_player ON pending(playerId);
+				CREATE TABLE IF NOT EXISTS connection_owner (
+					connectionId TEXT PRIMARY KEY,
+					playerId INTEGER NOT NULL
+				);
+				CREATE INDEX IF NOT EXISTS idx_owner_player ON connection_owner(playerId);
 			`)
 		})
 	}
@@ -73,11 +141,30 @@ export class NotificationsHub extends DurableObject<Env> {
 		// negotiate handed the client this id as `connectionToken`/`connectionId`.
 		const connectionId = url.searchParams.get('id') || crypto.randomUUID()

+		// Who this socket belongs to, established by the worker from the connect
+		// request's token (see OWNER_HEADER). The worker rejects a connect it can't
+		// identify and always sets the header itself, so an absent one means the DO was
+		// reached by some other path — not something to serve a socket to.
+		const playerId = Number.parseInt(request.headers.get(OWNER_HEADER) ?? '', 10)
+		if (!Number.isInteger(playerId)) {
+			return new Response('Unidentified connection', { status: 401 })
+		}
+
 		const pair = new WebSocketPair()
 		const server = pair[1]
 		// Tag by connectionId so we can find this socket via getWebSockets(id).
 		this.ctx.acceptWebSocket(server, [connectionId])
-		server.serializeAttachment({ connectionId, handshakeDone: false } satisfies SocketState)
+		server.serializeAttachment({
+			connectionId,
+			handshakeDone: false,
+			playerId,
+		} satisfies SocketState)
+
+		this.ctx.storage.sql.exec(
+			'INSERT OR REPLACE INTO connection_owner (connectionId, playerId) VALUES (?, ?)',
+			connectionId,
+			playerId
+		)

 		return new Response(null, { status: 101, webSocket: pair[0] })
 	}
@@ -115,6 +202,10 @@ export class NotificationsHub extends DurableObject<Env> {
 				'DELETE FROM subscriptions WHERE connectionId = ?',
 				state.connectionId
 			)
+			this.ctx.storage.sql.exec(
+				'DELETE FROM connection_owner WHERE connectionId = ?',
+				state.connectionId
+			)
 		}
 		try {
 			ws.close()
@@ -145,6 +236,25 @@ export class NotificationsHub extends DurableObject<Env> {

 		// Send "OnConnect" to the caller after connecting.
 		ws.send(this.invocation('OnConnect', []))
+
+		// Deliver whatever piled up while this player was away. Done here rather than at
+		// accept because SignalR won't read invocation frames sent before the handshake
+		// reply, and only here because the client never calls SubscribeToPlayers — with
+		// the flush living solely in there, a queued notification was stranded forever.
+		if (state.playerId !== undefined) this.flushPending(ws, state.playerId)
+	}
+
+	/** Send and clear a player's queued notifications on `ws`. */
+	private flushPending(ws: WebSocket, playerId: number): void {
+		const pending = this.ctx.storage.sql
+			.exec<{
+				payload: string
+			}>('SELECT payload FROM pending WHERE playerId = ? ORDER BY id', playerId)
+			.toArray()
+		if (pending.length === 0) return
+
+		for (const row of pending) ws.send(this.invocation('Notification', [row.payload]))
+		this.ctx.storage.sql.exec('DELETE FROM pending WHERE playerId = ?', playerId)
 	}

 	private handleMessage(ws: WebSocket, connectionId: string, msg: HubMessage): void {
@@ -166,9 +276,18 @@ export class NotificationsHub extends DurableObject<Env> {
 	private handleInvocation(ws: WebSocket, connectionId: string, msg: HubMessage): void {
 		switch (msg.target) {
 			case 'SubscribeToPlayers': {
-				const arg = msg.arguments?.[0] as { playerIds?: number[] } | undefined
-				const playerIds = (arg?.playerIds ?? []).filter((n) => typeof n === 'number')
-				this.subscribeToPlayers(ws, connectionId, playerIds)
+				const playerIds = parsePlayerIds(msg.arguments)
+				// An argument we can't read is not "subscribe to nobody": subscribing
+				// replaces the connection's whole set, so acting on a misread would wipe
+				// a working connection to zero and silently strand every push.
+				if (playerIds === null) {
+					console.warn('hub: unreadable SubscribeToPlayers argument', {
+						connectionId,
+						arguments: JSON.stringify(msg.arguments),
+					})
+				} else {
+					this.subscribeToPlayers(ws, connectionId, playerIds)
+				}
 				if (msg.invocationId) ws.send(this.completion(msg.invocationId, null))
 				break
 			}
@@ -178,6 +297,14 @@ export class NotificationsHub extends DurableObject<Env> {
 				break
 			}
 			default:
+				// Logged, not just answered with an error completion: a hub method we
+				// don't implement is a client expectation we've missed, and the client
+				// gives no sign of it.
+				console.warn('hub: unknown invocation target', {
+					connectionId,
+					target: msg.target,
+					arguments: JSON.stringify(msg.arguments),
+				})
 				if (msg.invocationId) {
 					ws.send(this.completionError(msg.invocationId, `Unknown method '${msg.target}'`))
 				}
@@ -199,18 +326,26 @@ export class NotificationsHub extends DurableObject<Env> {
 		}

 		// Flush any notifications queued while these players were offline.
-		for (const playerId of unique) {
-			const pending = this.ctx.storage.sql
-				.exec<{
-					payload: string
-				}>('SELECT payload FROM pending WHERE playerId = ? ORDER BY id', playerId)
-				.toArray()
-			if (pending.length === 0) continue
-			for (const row of pending) {
-				ws.send(this.invocation('Notification', [row.payload]))
-			}
-			this.ctx.storage.sql.exec('DELETE FROM pending WHERE playerId = ?', playerId)
-		}
+		for (const playerId of unique) this.flushPending(ws, playerId)
+	}
+
+	/**
+	 * Every connection that receives notifications for a player. Two ways to qualify: the
+	 * connection *is* that player (established from the token at connect), or it
+	 * subscribed to them. The client only ever uses the first — it never calls
+	 * SubscribeToPlayers — so a player's own notifications reach them through
+	 * connection_owner, and subscriptions carry other players' updates.
+	 */
+	private connectionIdsFor(playerId: number): string[] {
+		return this.ctx.storage.sql
+			.exec<{ connectionId: string }>(
+				`SELECT connectionId FROM connection_owner WHERE playerId = ?1
+				UNION
+				SELECT connectionId FROM subscriptions WHERE playerId = ?1`,
+				playerId
+			)
+			.toArray()
+			.map((r) => r.connectionId)
 	}

 	private getSubscribedPlayers(connectionId: string): number[] {
@@ -237,6 +372,17 @@ export class NotificationsHub extends DurableObject<Env> {
 		const payload = this.buildNotificationPayload(notificationType, data)
 		const delivered = this.deliverToPlayer(playerId, payload)

+		if (delivered === 0) {
+			// Distinguishes the two ways this fails: no connection is registered for the
+			// player at all, versus one is registered but has no live socket behind it.
+			console.warn('hub: notification queued, nobody to deliver to', {
+				playerId,
+				notificationType,
+				connectionIds: this.connectionIdsFor(playerId),
+				liveSockets: this.ctx.getWebSockets().length,
+			})
+		}
+
 		if (delivered === 0) {
 			this.ctx.storage.sql.exec(
 				'INSERT INTO pending (playerId, payload) VALUES (?, ?)',
@@ -270,7 +416,97 @@ export class NotificationsHub extends DurableObject<Env> {
 		notificationType: string | number,
 		data?: Record<string, unknown>
 	): Promise<{ delivered: number }> {
-		return { delivered: this.broadcastToConnected(this.buildNotificationPayload(notificationType, data)) }
+		return {
+			delivered: this.broadcastToConnected(this.buildNotificationPayload(notificationType, data)),
+		}
+	}
+
+	/**
+	 * Dump the hub's routing state for debugging delivery. A notification only reaches a
+	 * player through a `subscriptions` row (see {@link deliverToPlayer}), so when a push
+	 * doesn't arrive this answers the two questions that matter: is the player subscribed
+	 * on a live connection, and is the frame sitting in `pending` instead?
+	 *
+	 * Connections are listed even when one side is missing — a socket that has yet to
+	 * subscribe (`playerIds: []`) and a subscription set whose socket is gone
+	 * (`live: false`, a close we never saw) are both delivery failures worth seeing.
+	 */
+	async inspect(): Promise<HubState> {
+		const live = new Map<string, boolean>()
+		for (const ws of this.ctx.getWebSockets()) {
+			const state = ws.deserializeAttachment() as SocketState | null
+			if (state) live.set(state.connectionId, state.handshakeDone)
+		}
+
+		const subscribed = new Map<string, number[]>()
+		const rows = this.ctx.storage.sql
+			.exec<{
+				connectionId: string
+				playerId: number
+			}>('SELECT connectionId, playerId FROM subscriptions ORDER BY connectionId, playerId')
+			.toArray()
+		for (const row of rows) {
+			const players = subscribed.get(row.connectionId) ?? []
+			players.push(row.playerId)
+			subscribed.set(row.connectionId, players)
+		}
+
+		const owners = new Map(
+			this.ctx.storage.sql
+				.exec<{
+					connectionId: string
+					playerId: number
+				}>('SELECT connectionId, playerId FROM connection_owner')
+				.toArray()
+				.map((row) => [row.connectionId, row.playerId] as const)
+		)
+
+		const connections = [...new Set([...live.keys(), ...subscribed.keys(), ...owners.keys()])].map(
+			(connectionId) => ({
+				connectionId,
+				live: live.has(connectionId),
+				handshakeDone: live.get(connectionId) ?? false,
+				playerId: owners.get(connectionId) ?? null,
+				playerIds: subscribed.get(connectionId) ?? [],
+			})
+		)
+
+		const pending = this.ctx.storage.sql
+			.exec<{ playerId: number; count: number; latest: string }>(
+				`SELECT playerId, COUNT(*) AS count,
+					(SELECT payload FROM pending AS newest WHERE newest.playerId = pending.playerId
+						ORDER BY id DESC LIMIT 1) AS latest
+				FROM pending GROUP BY playerId ORDER BY playerId`
+			)
+			.toArray()
+
+		return { connections, pending }
+	}
+
+	/**
+	 * Drop queued notifications — for one player, or (`playerId` omitted) the whole
+	 * queue. Anything pending is delivered the moment that player next subscribes, so a
+	 * frame queued by a bug that has since been fixed would otherwise arrive, out of
+	 * context, at the next reconnect. Returns how many were discarded.
+	 */
+	async clearPending(playerId?: number): Promise<{ cleared: number }> {
+		// Counted first rather than read off the cursor: the delete's own row count isn't
+		// reported, and this is an admin-facing number we want to be exact.
+		const [counted] =
+			playerId === undefined
+				? this.ctx.storage.sql
+						.exec<{ count: number }>('SELECT COUNT(*) AS count FROM pending')
+						.toArray()
+				: this.ctx.storage.sql
+						.exec<{
+							count: number
+						}>('SELECT COUNT(*) AS count FROM pending WHERE playerId = ?', playerId)
+						.toArray()
+
+		if (playerId === undefined) this.ctx.storage.sql.exec('DELETE FROM pending')
+		else this.ctx.storage.sql.exec('DELETE FROM pending WHERE playerId = ?', playerId)
+
+		return { cleared: counted?.count ?? 0 }
 	}

 	// ---- Helpers -------------------------------------------------------------
@@ -281,13 +517,7 @@ export class NotificationsHub extends DurableObject<Env> {
 	 * shared send path for {@link notifyPlayer} and {@link coachMessageAll}.
 	 */
 	private deliverToPlayer(playerId: number, payload: string): number {
-		const connectionIds = this.ctx.storage.sql
-			.exec<{ connectionId: string }>(
-				'SELECT DISTINCT connectionId FROM subscriptions WHERE playerId = ?',
-				playerId
-			)
-			.toArray()
-			.map((r) => r.connectionId)
+		const connectionIds = this.connectionIdsFor(playerId)

 		let delivered = 0
 		for (const connectionId of connectionIds) {