🐛 fix(mobile): preserve authenticated legacy unregister cleanup (#15723)

Follow-up to #15719 addressing a Codex P2 review note.

After #15719, legacy v1.0.7 clients that only send `deviceId` were
silent-OKed unconditionally. But `publicProcedure` still receives
`ctx.userId` from `createLambdaContext` — and in the *active*
sign-out path (the user is still authenticated when logout fires)
that userId is valid. Skipping the delete in that case orphans the
existing `(userId, deviceId)` row, so `PushChannel.deliver` keeps
fanning notifications out to a signed-out device. Expo's
`DeviceNotRegistered` receipt only fires on uninstall, not on
logout, so the cron worker doesn't catch this either.

Fix: add a Path B fallback — when `ctx.userId` is available, run
the original `(userId, deviceId)` delete. Path A (expoToken pair)
still wins when present; Path C (silent OK) is now reserved for
the case the original PR was actually targeting: a v1.0.7 client
whose session is already gone, which is the source of the 401
storm.

Path matrix:
  expoToken present                  → Path A: precise delete by (expoToken, deviceId)
  no expoToken, ctx.userId present   → Path B: legacy (userId, deviceId) delete
  no expoToken, no session           → Path C: silent OK, cron cleans up

Tests added:
- legacy + valid session → falls back to (userId, deviceId)
- legacy + no session    → silent OK
- expoToken always takes precedence over userId fallback

apps/server/src/routers/lambda/__tests__/pushToken.test.ts

@@ -112,20 +112,36 @@ describe('pushTokenRouter', () => {
       expect(mockUnregister).not.toHaveBeenCalled();
     });
 
-    it('should silently succeed without expoToken (1.0.7 legacy clients)', async () => {
+    it('should fall back to (userId, deviceId) for legacy clients with a session', async () => {
+      // Path B — v1.0.7 only sends deviceId; if the request still carries a
+      // valid session we MUST delete the row, otherwise PushChannel keeps
+      // notifying a signed-out device (Expo DeviceNotRegistered only fires on
+      // uninstall, not logout).
+      mockUnregister.mockResolvedValueOnce(undefined);
       const caller = createCaller();
 
       const result = await caller.unregister({ deviceId: 'device-1' });
 
-      // Cleanup happens via process-push-receipts cron — no DB delete here
+      expect(mockUnregister).toHaveBeenCalledWith('device-1');
+      expect(mockDeleteByExpoTokenAndDevice).not.toHaveBeenCalled();
+      expect(result).toEqual({ success: true });
+    });
+
+    it('should silently succeed without expoToken AND without session', async () => {
+      // Path C — v1.0.7 + dead session: the only safe move is silent OK.
+      // Orphan row will be cleaned up by the process-push-receipts worker via
+      // Expo DeviceNotRegistered receipts. Returning 200 here stops the storm.
+      const caller = createCaller({ userId: undefined });
+
+      const result = await caller.unregister({ deviceId: 'device-1' });
+
       expect(mockDeleteByExpoTokenAndDevice).not.toHaveBeenCalled();
       expect(mockUnregister).not.toHaveBeenCalled();
       expect(result).toEqual({ success: true });
     });
 
-    it('should succeed for an unauthenticated caller (no userId)', async () => {
-      // The whole point of making this public: clients call it during sign-out
-      // when their session may already be gone. Must not 401.
+    it('should succeed for an unauthenticated caller carrying expoToken', async () => {
+      // New clients (>=1.0.8) hit Path A regardless of session.
       const caller = createCaller({ userId: undefined });
 
       const result = await caller.unregister({
@@ -135,6 +151,21 @@ describe('pushTokenRouter', () => {
 
       expect(result).toEqual({ success: true });
       expect(mockDeleteByExpoTokenAndDevice).toHaveBeenCalled();
+      expect(mockUnregister).not.toHaveBeenCalled();
+    });
+
+    it('should prefer expoToken precision over the legacy userId fallback', async () => {
+      // If both are available, always take Path A — the (expoToken, deviceId)
+      // pair is more precise and doesn't risk deleting a wrong row.
+      const caller = createCaller();
+
+      await caller.unregister({
+        deviceId: 'device-1',
+        expoToken: 'ExponentPushToken[abc]',
+      });
+
+      expect(mockDeleteByExpoTokenAndDevice).toHaveBeenCalled();
+      expect(mockUnregister).not.toHaveBeenCalled();
     });
 
     it('should reject empty deviceId', async () => {

apps/server/src/routers/lambda/pushToken.ts

@@ -31,20 +31,25 @@ export const pushTokenRouter = router({
     }),
 
   /**
-   * Public on purpose: clients call this during sign-out, when their session
-   * may already be invalid (expired token / cleared cookie). Authenticating by
-   * session here causes a 401 storm on every logout in the wild — the original
-   * intent was "clean up before clearing auth", but in practice the auth has
-   * already been cleared on the server long before logout fires.
+   * Public on purpose: clients call this during sign-out, and in the wild many
+   * of those calls arrive after the session is already gone (expired OIDC
+   * token / cleared cookie). Authenticating by session here causes a 401
+   * storm on every such logout.
    *
-   * Authorization model: the caller presents the (deviceId, expoToken) pair it
-   * received at registration. Holding both = proof of ownership of the row,
-   * same trust model as APNs/FCM unregister.
+   * Authorization model (Path A — new clients ≥ 1.0.8): the caller presents the
+   * (deviceId, expoToken) pair it received at registration. Holding both = proof
+   * of ownership of the row, same trust model as APNs/FCM unregister.
    *
-   * Backwards compat: older clients (≤ 1.0.7) only send `deviceId`. We silently
-   * succeed in that case and let the `process-push-receipts` worker clean up
-   * stale rows via `DeviceNotRegistered` receipts from Expo. Returning 200 here
-   * is what actually stops the 401 storm in production.
+   * Backwards compat for v1.0.7 (only sends `deviceId`):
+   *  - Path B — when the request still carries a valid session, fall back to
+   *    the original (userId, deviceId) delete. This covers the *active*
+   *    sign-out path so PushChannel doesn't keep notifying a signed-out device
+   *    until the user uninstalls (Expo's DeviceNotRegistered receipt only
+   *    fires on uninstall, not on logout).
+   *  - Path C — when there's no session either, silently succeed. The orphan
+   *    row will be cleaned up by the existing `process-push-receipts` worker
+   *    via Expo's DeviceNotRegistered receipts. Returning 200 here is what
+   *    actually stops the 401 storm in production.
    */
   unregister: publicProcedure
     .use(serverDatabase)
@@ -57,10 +62,20 @@ export const pushTokenRouter = router({
     .mutation(async ({ ctx, input }) => {
       const { deviceId, expoToken } = input;
 
+      // Path A: new clients — precise delete by (expoToken, deviceId), no session needed
       if (expoToken) {
         await deletePushTokenByExpoTokenAndDevice(ctx.serverDB, { deviceId, expoToken });
+        return { success: true };
       }
 
+      // Path B: legacy v1.0.7 + valid session — fall back to (userId, deviceId)
+      if (ctx.userId) {
+        const pushTokenModel = new PushTokenModel(ctx.serverDB, ctx.userId);
+        await pushTokenModel.unregister(deviceId);
+        return { success: true };
+      }
+
+      // Path C: legacy v1.0.7 with no session — silent OK, cron worker cleans up
       return { success: true };
     }),
 });