fix: don't write last-played history after the device token is revoked

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");

hosted/lib/service.js

@@ -78,6 +78,14 @@
 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
@@ -154,32 +162,32 @@
 
 export function isCardId(value) { return typeof value === "string" && ID_PATTERN.test(value); }
 
 export function validateIngest(payload, { now = Date.now() } = {}) {
   if (payload === null || typeof payload !== "object" || Array.isArray(payload)) throw new ServiceError(400, "invalid_payload");
   for (const key of Object.keys(payload)) if (!INGEST_KEYS.has(key)) throw new ServiceError(400, "unknown_field");
   if (payload.v !== SCHEMA_VERSION) throw new ServiceError(400, "unsupported_version");
   if (!Number.isSafeInteger(payload.seq) || payload.seq < 0) throw new ServiceError(400, "invalid_seq");
   if (!Number.isSafeInteger(payload.observedAt)) throw new ServiceError(400, "invalid_observed_at");
   if (Math.abs(payload.observedAt - now) > MAX_CLOCK_SKEW_MS) throw new ServiceError(400, "clock_skew");
   if (!STATES.has(payload.state)) throw new ServiceError(400, "invalid_state");
   const kind = payload.kind ?? "unknown";
   if (!KINDS.has(kind)) throw new ServiceError(400, "invalid_kind");
   const text = (value) => {
     if (value == null) return null;
     if (typeof value !== "string" || value.length > TEXT_LIMIT) throw new ServiceError(400, "invalid_text");
     return value;
   };
   const ms = (value) => {
     if (value == null) return null;
     if (!Number.isSafeInteger(value) || value < 0) throw new ServiceError(400, "invalid_time");
     return value;
   };
   const positionMs = ms(payload.positionMs);
   const durationMs = ms(payload.durationMs);
   if (positionMs !== null && durationMs !== null && positionMs > durationMs) throw new ServiceError(400, "invalid_time");
   if (payload.artworkDataUri !== undefined && !isHostedArtwork(payload.artworkDataUri)) throw new ServiceError(400, "invalid_artwork");
   return { ...(payload.state !== "idle" && payload.artworkDataUri ? { artworkDataUri: payload.artworkDataUri } : {}), seq: payload.seq, observedAt: payload.observedAt, state: payload.state, kind, title: text(payload.title), subtitle: text(payload.subtitle), positionMs, durationMs };
 }
 
 export function isLogin(value) { return typeof value === "string" && LOGIN_PATTERN.test(value); }
 
@@ -301,7 +309,7 @@
     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 };
@@ -341,54 +349,54 @@
   const deviceAction = (userId, action, authHash = "", targetId = "", name = "", newId = "", newHash = "", previousHash = "") =>
     cmd("EVAL", DEVICE_ATOMIC_SCRIPT, 1, `np:udevs:${userId}`, String(userId), action, authHash, targetId, name, newId, newHash, String(now()), previousHash, String(MAX_DEVICES_PER_USER));
 
   async function signInWithGitHub({ githubToken, deviceName, legacyToken, previousToken, clientKey = "unknown" } = {}) {
     const bucket = `np:rl:signin:${hashToken(String(clientKey)).slice(0, 32)}`;
     await cmd("SET", bucket, "0", "EX", 3600, "NX");
     if (await cmd("INCR", bucket) > SIGNINS_PER_HOUR) throw new ServiceError(429, "rate_limited");
     if (typeof githubToken !== "string" || !GITHUB_TOKEN_PATTERN.test(githubToken)) throw new ServiceError(400, "invalid_github_token");
     const { id, login } = await githubUser(githubToken);
     const userId = String(id);
 
     const rawUser = await cmd("GET", `np:user:${userId}`);
     const user = rawUser ? JSON.parse(rawUser) : null;
     if (!user) { await bump("np:stats:users"); await countToday("users"); }
     if (user?.login && user.login.toLowerCase() !== login.toLowerCase()) {
       const oldKey = `np:login:${user.login.toLowerCase()}`;
       if (await cmd("GET", oldKey) === userId) await cmd("DEL", oldKey);
     }
     await cmd("SET", `np:login:${login.toLowerCase()}`, userId);
     await cmd("SET", `np:user:${userId}`, JSON.stringify({ login, createdAt: user?.createdAt ?? now() }));
 
     if (previousToken !== undefined && (typeof previousToken !== "string" || previousToken.length < 20 || previousToken.length > 128)) throw new ServiceError(400, "invalid_previous_token");
     const deviceId = randomId(16);
     const token = randomId(32);
     const result = await deviceAction(userId, "sign-in", "", "", cleanDeviceName(deviceName), deviceId, hashToken(token), previousToken ? hashToken(previousToken) : "");
     if (Number(result[0]) < 0) {
       const errors = { "-2": [409, "previous_device_missing"], "-3": [403, "previous_device_mismatch"], "-4": [409, "previous_device_mismatch"] };
       const [status, code] = errors[String(result[0])];
       throw new ServiceError(status, code);
     }
 
     // A PC moving from its old per-PC card: that link keeps working and now
     // shows this user's card; the old key and state go.
     let aliased = false;
     if (typeof legacyToken === "string" && legacyToken.length >= 20 && legacyToken.length <= 128) {
       const legacyHash = hashToken(legacyToken);
       const raw = await cmd("GET", `np:tok:${legacyHash}`);
       const legacy = raw ? JSON.parse(raw) : null;
       if (legacy?.cardId && !legacy.userId) {
         await cmd("DEL", `np:tok:${legacyHash}`);
         await cmd("SET", `np:alias:${legacy.cardId}`, userId);
         await cmd("DEL", `np:state:${legacy.cardId}`);
         await cmd("DEL", `np:seq:${legacy.deviceId}`);
         // The old link's last-played history moves to the user's card so the
         // badge keeps telling one story. Best effort, like the stats writes.
         try { await migrateLastPlays(`np:last:card:${legacy.cardId}`, `np:last:user:${userId}`); } catch { /* ignore */ }
         aliased = true;
       }
     }
     return { login, deviceId, token, cardPath: `/u/${login}.svg`, aliasedOldCard: aliased };
   }
 
   async function authenticateUser(token) {
     const device = await authenticate(token);
@@ -474,10 +482,10 @@
     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

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);
+});

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" }) });