diff --git a/hosted/lib/redis.js b/hosted/lib/redis.js index ca4361ec..e8c27cbe 100644 --- a/hosted/lib/redis.js +++ b/hosted/lib/redis.js @@ -96,6 +96,12 @@ export function createMemoryRedis({ now = () => Date.now() } = {}) { else { drop(devices[index]); devices.splice(index, 1); } save(); return [1, 1]; } + if (typeof key === "string" && key.includes("np-last-play-v1") && Number(rest[0]) === 2) { + const [tokenKey, historyKey, recordText, ttl] = rest.slice(1); + if (!live(tokenKey)) return 0; + data.set(historyKey, { value: String(recordText), expiresAt: now() + Number(ttl) * 1000 }); + return 1; + } // Emulates the hosted ingest script as one synchronous Redis action. // No await may split its comparison from the card-state write. if (typeof key !== "string" || !key.includes("local previous = tonumber(redis.call('GET', KEYS[1])") || Number(rest[0]) !== 4) throw new Error("unsupported script"); diff --git a/hosted/lib/service.js b/hosted/lib/service.js index 248567f0..4bd2c68f 100644 --- a/hosted/lib/service.js +++ b/hosted/lib/service.js @@ -78,6 +78,14 @@ redis.call('SET', KEYS[1], ARGV[1], 'EX', tonumber(ARGV[2])) return {1, previous} `; +// History is best-effort, but its write must not follow token revocation. +export const LAST_PLAY_ATOMIC_SCRIPT = ` +-- np-last-play-v1 +if not redis.call('GET', KEYS[1]) then return 0 end +redis.call('SET', KEYS[2], ARGV[1], 'EX', tonumber(ARGV[2])) +return 1 +`; + // All device-list writes and associated token/state changes share this Redis atomic action. export const DEVICE_ATOMIC_SCRIPT = ` -- np-device-lifecycle-v1 @@ -301,7 +309,7 @@ export function createService({ redis, now = () => Date.now(), githubUser = crea if (update.state !== "idle" && update.title) { // Remember the last playback of each kind for the last-played badge. A // failing record must never fail an accepted ingest. - try { await recordLastPlay(device, update); } catch { /* ignore */ } + try { await recordLastPlay(device, update, hashToken(token)); } catch { /* ignore */ } } await markDeviceActive(device.deviceId); return { accepted: true, expiresIn: update.state === "idle" ? 0 : STATE_TTL_SECONDS }; @@ -474,10 +482,10 @@ export function createService({ redis, now = () => Date.now(), githubUser = crea return presence; } - async function recordLastPlay(device, update) { + async function recordLastPlay(device, update, tokenHash) { const scope = device.userId != null ? `np:last:user:${device.userId}` : device.cardId != null ? `np:last:card:${device.cardId}` : null; if (!scope) return; - await cmd("SET", `${scope}:${update.kind}`, JSON.stringify({ title: update.title, subtitle: update.subtitle, state: update.state, kind: update.kind, at: now() }), "EX", LAST_TTL_SECONDS); + await cmd("EVAL", LAST_PLAY_ATOMIC_SCRIPT, 2, `np:tok:${tokenHash}`, `${scope}:${update.kind}`, JSON.stringify({ title: update.title, subtitle: update.subtitle, state: update.state, kind: update.kind, at: now() }), LAST_TTL_SECONDS); } // Move an old per-PC card's last-played records to the user scope during a diff --git a/test/hosted-last-revoke-race.test.js b/test/hosted-last-revoke-race.test.js new file mode 100644 index 00000000..563f1209 --- /dev/null +++ b/test/hosted-last-revoke-race.test.js @@ -0,0 +1,47 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { createMemoryRedis } from "../hosted/lib/redis.js"; +import { createService } from "../hosted/lib/service.js"; + +function deferred() { + let resolve; + const promise = new Promise((r) => { resolve = r; }); + return { promise, resolve }; +} + +test("revoking a card while ingest is paused after its atomic commit cannot resurrect last-played history", async () => { + const now = () => 1_800_000_000_000; + const redis = createMemoryRedis({ now }); + const committed = deferred(); + const resume = deferred(); + const original = redis.command.bind(redis); + redis.command = async (args) => { + const result = await original(args); + if (args[0] === "EVAL" && String(args[1]).includes("local previous = tonumber")) { + committed.resolve(); + await resume.promise; + } + return result; + }; + const service = createService({ redis, now }); + const { cardId, token } = await service.register(); + const ingest = service.ingest({ token, payload: { v: 1, seq: 1, observedAt: now(), state: "playing", kind: "track", title: "Private Song" } }); + await committed.promise; + await service.revoke({ token }); + resume.resolve(); + await ingest; + assert.equal((await service.readBadgeState({ id: cardId, last: true })).title, null); + assert.equal(redis.data.has(`np:last:card:${cardId}:track`), false); +}); + +test("a history write completed before revoke is deleted and normal ingest still remembers playback", async () => { + const now = () => 1_800_000_000_000; + const redis = createMemoryRedis({ now }); + const service = createService({ redis, now }); + const { cardId, token } = await service.register(); + await service.ingest({ token, payload: { v: 1, seq: 1, observedAt: now(), state: "playing", kind: "track", title: "Song" } }); + assert.equal((await service.readBadgeState({ id: cardId, last: true })).title, "Song"); + await service.revoke({ token }); + assert.equal((await service.readBadgeState({ id: cardId, last: true })).title, null); + assert.equal(redis.data.has(`np:last:card:${cardId}:track`), false); +}); diff --git a/test/hosted-service.test.js b/test/hosted-service.test.js index 7f9093a9..462bd900 100644 --- a/test/hosted-service.test.js +++ b/test/hosted-service.test.js @@ -289,7 +289,7 @@ test("a failing last-played record never fails an accepted ingest", async () => const { cardId, token } = await service.register(); const original = redis.command.bind(redis); redis.command = async (args) => { - if (String(args[0]).toUpperCase() === "SET" && String(args[1]).startsWith("np:last:")) throw new Error("redis down"); + if (String(args[0]).toUpperCase() === "EVAL" && String(args[1]).includes("np-last-play-v1")) throw new Error("redis down"); return original(args); }; const result = await service.ingest({ token, payload: update(now(), { seq: 1, kind: "track", title: "Song" }) });