notify: only "the app is gone" deletes a device row

reported from the field on the first real push: it failed with BadDeviceToken
and the server deleted the registration. the build was production-signed and
the row said sandbox — a configuration mismatch, with the device and the app
both perfectly fine.

pruning there destroys the evidence. the phone re-registers on next launch,
the row comes back looking healthy, and the next send fails identically; you
can lose a day to that loop without ever seeing what is actually wrong.

so the one set becomes three. Unregistered/UNREGISTERED/NOT_FOUND prune —
apple and google are saying the app is uninstalled. BadDeviceToken,
DeviceTokenNotForTopic, TopicDisallowed, INVALID_ARGUMENT and
SENDER_ID_MISMATCH are OURS: the row is kept and the log says what to check.
everything else is transient and still counts toward three strikes.

INVALID_ARGUMENT was the sharpest one. it is how fcm reports a malformed
MESSAGE — including a non-string in `data`, which is the mistake this codebase
is most likely to make and already has a test for. deleting on it meant one
bug in the payload builder would wipe every android device from the registry
on a single send, presenting as "android push stopped working" with nothing
left to explain why.

misconfigured is also not counted as a strike. not deleting is not enough on
its own — a persistent mismatch would reach three failures and delete the row
anyway, which is the same bug with a slower fuse.

the apns log now names the mismatch rather than the reason code, since the
code alone sends people to look at the device: it prints the row's environment
and topic and says to check the SHIPPED artefact rather than the intermediate
build, which is exactly where this one hid.
This commit is contained in:
2026-08-19 21:25:20 +00:00
parent c6ef0b34db
commit 5cba1b65a6
19 changed files with 391 additions and 34 deletions
+21 -2
View File
@@ -227,6 +227,14 @@ type PersistentSession = {
closeInput: () => void;
abort: AbortController;
emit: (event: ChatEvent) => void;
/**
* A lightweight, non-durable ping for notification delivery — deliberately separate from `emit`.
* `ChatEvent`/`TurnMessage` is the durable, replayed transcript; this is not part of it, so it fires
* even for events (`plan:proposed` parking, an `AskUserQuestion` gate) that have no event of their own
* to piggyback on. Officer decides whether to actually notify (preference lookup + fan-out) — this
* only says "something notify-worthy just happened for this session."
*/
notify: (kind: 'turn-ended' | 'input-needed') => void;
isGenerating: boolean;
pendingTasks: Set<string>; // background tasks started but not yet notified; suppress idle-GC while non-empty
/**
@@ -390,6 +398,7 @@ function dropDeadSession(session: PersistentSession, detail: string): void {
if (!wasGenerating) return;
session.emit({ type: 'error', message: diagnosis.message });
session.notify('turn-ended');
}
/** Wrap a spawn so the session self-heals when its child goes away. */
@@ -502,7 +511,11 @@ function armIdle(session: PersistentSession): void {
}, IDLE_TIMEOUT_MS);
}
function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: ChatEvent) => void): PersistentSession {
function createSession(
params: ClaudeSpawnStreamingParams,
onEvent: (event: ChatEvent) => void,
onNotify: (kind: 'turn-ended' | 'input-needed') => void,
): PersistentSession {
const { sessionKey } = params;
const input = makeInputQueue();
const abort = new AbortController();
@@ -515,6 +528,7 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat
closeInput: () => input.close(),
abort,
emit: onEvent,
notify: onNotify,
isGenerating: false,
pendingTasks: new Set<string>(),
planMode: params.planMode === true,
@@ -767,6 +781,7 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat
// Whatever ended the turn, the interrupt is spent — a later genuine error must not wear it.
session.interrupted = false;
if (session.pendingTasks.size === 0) armIdle(session);
session.notify('turn-ended');
}
// Any event at all is a sign of life, so push the stall deadline back. Placed after the branch
// above so a terminal event disarms rather than re-arms it.
@@ -804,6 +819,7 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat
export async function spawnClaudeStreaming(
params: ClaudeSpawnStreamingParams,
onEvent: (event: ChatEvent) => void,
onNotify: (kind: 'turn-ended' | 'input-needed') => void,
): Promise<void> {
let session = sessions.get(params.sessionKey);
if (session && session.userId !== params.userId) {
@@ -817,10 +833,11 @@ export async function spawnClaudeStreaming(
}
if (session) {
session.emit = onEvent; // adopt the latest emitter (equivalent across turns; keeps events flowing)
session.notify = onNotify;
await applyModelChange(session, params.model);
await applyPlanModeChange(session, params.planMode);
} else {
session = createSession(params, onEvent);
session = createSession(params, onEvent, onNotify);
}
session.pushTurn(params.prompt, params.images);
}
@@ -903,6 +920,7 @@ async function planGate(
session.pendingPlans.set(requestId, { resolve, timer });
session.emit({ type: 'plan:proposed', requestId, plan });
session.notify('input-needed');
});
session.emit({ type: 'plan:decided', requestId, approved: decision.approve, feedback: decision.feedback });
@@ -958,6 +976,7 @@ async function questionGate(session: PersistentSession, toolCallId: string): Pro
}, QUESTION_ANSWER_TIMEOUT_MS);
session.pendingQuestions.set(toolCallId, { resolve, timer });
session.notify('input-needed');
});
}
+10 -1
View File
@@ -242,6 +242,11 @@ const sessionLog = createSessionLogStore((d) =>
connection.send({ type: 'claude:message', sessionKey: d.sessionId, msg: d.msg, seq: d.seq }),
);
// Notification delivery is a separate, non-durable ping — see `PersistentSession.notify` in
// claude-manager.ts for why it isn't folded into the message above.
const notifySignal = (sessionKey: string, userId: number, kind: 'turn-ended' | 'input-needed') =>
connection.send({ type: 'claude:notify', sessionKey, userId, kind });
// ── Command handlers ──
type ReplyFn = (msg: SidecarEvent) => void;
@@ -269,7 +274,11 @@ async function handleCommand(cmd: SidecarCommand, reply: ReplyFn) {
const { sessionKey, durable = true } = cmd.params;
claudeManager
.spawnClaudeStreaming(cmd.params, (event) => sessionLog.push(sessionKey, event, durable))
.spawnClaudeStreaming(
cmd.params,
(event) => sessionLog.push(sessionKey, event, durable),
(kind) => notifySignal(sessionKey, cmd.params.userId, kind),
)
.catch((err) => {
// Through the log like any other output, so a failure to start is durable and replayable too.
const message = err instanceof Error ? err.message : String(err);
+30
View File
@@ -2,6 +2,7 @@ import { describe, test, expect } from 'bun:test';
import { generateKeyPairSync, createVerify } from 'node:crypto';
import { ApnsAuthToken } from './apns-jwt';
import { buildApnsPayload, collapseId } from './apns-payload';
import { __testing } from './apns';
import { renderNotification, FALLBACK_TITLE } from './render';
import type { Notification } from './types';
import type { Catalogue } from '../../notifications/catalogue';
@@ -127,3 +128,32 @@ describe('APNs payload', () => {
expect(renderNotification({ code: 'nope/nope:x', userId: 1 }, catalogue).title).toBe(FALLBACK_TITLE);
});
});
describe('what a failure does to the device row', () => {
// Reported from the field 2026-08-19: a first real push failed with BadDeviceToken and the server
// deleted the registration. The build was production-signed and the row said sandbox — a configuration
// mismatch, with the device and the app both perfectly fine.
//
// Pruning there destroys the evidence: the phone re-registers on next launch, the row returns looking
// healthy, and the next send fails identically. You can lose a day to that loop.
test('only Unregistered means the app is gone', () => {
expect(__testing.dispositionFor('Unregistered')).toBe('gone');
});
test('an environment mismatch keeps the row so it can be diagnosed', () => {
expect(__testing.dispositionFor('BadDeviceToken')).toBe('misconfigured');
expect(__testing.dispositionFor('DeviceTokenNotForTopic')).toBe('misconfigured');
});
// Not merely "not deleted" — misconfigured must not count as a strike either, or a persistent mismatch
// deletes the row on the third send. Same bug, slower fuse.
test('misconfigured is neither pruned nor a strike', () => {
expect(__testing.dispositionFor('BadDeviceToken')).not.toBe('gone');
expect(__testing.dispositionFor('BadDeviceToken')).not.toBe('transient');
});
test('anything unrecognised is transient, so it counts toward three strikes', () => {
expect(__testing.dispositionFor('ServiceUnavailable')).toBe('transient');
expect(__testing.dispositionFor('timeout')).toBe('transient');
});
});
+56 -12
View File
@@ -20,8 +20,36 @@ const HOSTS = {
sandbox: 'https://api.sandbox.push.apple.com',
} as const;
/** Apple's reasons for "this token is dead" — the row goes immediately rather than after three strikes. */
const DEAD_TOKEN_REASONS = new Set(['BadDeviceToken', 'Unregistered', 'DeviceTokenNotForTopic']);
// ── Telling "the app is gone" apart from "we are misconfigured" ──
//
// These used to be one set, and every one of them deleted the row. That is right for exactly one reason
// and actively harmful for the others.
//
// `Unregistered` (410) means Apple knows the app was uninstalled. The token will never work again, so
// pruning is correct and a registry full of those is how delivery quietly degrades.
//
// `BadDeviceToken` and `DeviceTokenNotForTopic` are CONFIGURATION problems, and almost always the same
// one: a token minted under `aps-environment: development` sent to the production host, or the reverse.
// The device is fine, the app is installed, the registration is good — the server dialled the wrong
// Apple. Deleting the row there destroys the evidence: the phone re-registers on next launch, the row
// comes back looking healthy, and the next send fails identically. You can lose a day to that loop
// without ever seeing the thing that is actually wrong.
//
// So: only `Unregistered` prunes.
const GONE_REASONS = new Set(['Unregistered']);
/**
* Wrong on OUR side, not the device's. Never pruned, and deliberately not counted toward the
* three-strikes rule either — a persistent environment mismatch would otherwise delete the row on the
* third send, which is the same bug with a slower fuse.
*/
const MISCONFIGURED_REASONS = new Set(['BadDeviceToken', 'DeviceTokenNotForTopic', 'TopicDisallowed']);
/** What to do about one failed send. */
type Disposition = 'gone' | 'misconfigured' | 'transient';
const dispositionFor = (reason: string): Disposition =>
GONE_REASONS.has(reason) ? 'gone' : MISCONFIGURED_REASONS.has(reason) ? 'misconfigured' : 'transient';
// Secret store first, env second.
//
@@ -73,7 +101,7 @@ function session(host: string): http2.ClientHttp2Session {
return s;
}
type SendOutcome = { ok: boolean; dead: boolean; reason?: string };
type SendOutcome = { ok: boolean; disposition?: Disposition; reason?: string };
function sendOne(
host: string,
@@ -109,7 +137,7 @@ function sendOne(
raw += chunk;
});
req.on('end', () => {
if (status === 200) return resolve({ ok: true, dead: false });
if (status === 200) return resolve({ ok: true });
// Apple answers failures with { "reason": "BadDeviceToken" } and nothing else useful.
let reason = `status ${status}`;
try {
@@ -117,13 +145,13 @@ function sendOne(
} catch {
/* keep the status */
}
resolve({ ok: false, dead: DEAD_TOKEN_REASONS.has(reason), reason });
resolve({ ok: false, disposition: dispositionFor(reason), reason });
});
req.on('error', (err) => resolve({ ok: false, dead: false, reason: err.message }));
req.on('error', (err) => resolve({ ok: false, disposition: 'transient', reason: err.message }));
req.setTimeout(10_000, () => {
req.close();
resolve({ ok: false, dead: false, reason: 'timeout' });
resolve({ ok: false, disposition: 'transient', reason: 'timeout' });
});
req.end(body);
@@ -159,14 +187,27 @@ export const apnsChannel: Channel = {
failed++;
console.error(`[notify:apns] ${d.bundleId} ${d.environment}: ${outcome.reason}`);
if (outcome.dead) {
// An uninstalled app or a rotated token. Delete now — retrying cannot help, and a registry
// full of dead tokens is how delivery quietly degrades.
if (outcome.disposition === 'gone') {
// Apple says the app is uninstalled. Retrying cannot help.
dead.push(d.token);
await deletePushDevice(d.token);
} else {
await recordPushFailure(d.token);
return;
}
if (outcome.disposition === 'misconfigured') {
// Left ALONE — not deleted, and not counted as a strike. Say what is actually wrong instead,
// because the reason code alone sends people to look at the device.
const other = d.environment === 'production' ? 'sandbox' : 'production';
console.error(
`[notify:apns] '${outcome.reason}' is a configuration mismatch, not a dead device. This row ` +
`says environment='${d.environment}' and topic='${d.bundleId}'. If the installed build is ` +
`signed for '${other}', the registration is wrong and the device row is kept so it can be ` +
`corrected — check aps-environment in the SHIPPED artefact, not the intermediate build.`,
);
return;
}
await recordPushFailure(d.token);
}),
);
@@ -185,3 +226,6 @@ export function closeApnsSessions(): void {
}
sessions.clear();
}
/** Exposed for tests: classifying a failure is the whole of the pruning policy. */
export const __testing = { dispositionFor };
+37 -9
View File
@@ -15,8 +15,27 @@ import { FcmAccessToken, parseServiceAccount } from './fcm-auth';
const ENDPOINT = (projectId: string) => `https://fcm.googleapis.com/v1/projects/${projectId}/messages:send`;
/** Google's ways of saying "this token is dead" — the row goes immediately. */
const DEAD_TOKEN_CODES = new Set(['UNREGISTERED', 'INVALID_ARGUMENT', 'NOT_FOUND']);
// ── "The app is gone" versus "we sent something wrong" ──
//
// `UNREGISTERED` and `NOT_FOUND` mean the registration is dead: uninstalled, or a token that has been
// replaced. Pruning is correct.
//
// `INVALID_ARGUMENT` is NOT that, and it used to be in the same set. It is how FCM reports a malformed
// MESSAGE — including the one this codebase is most likely to produce, a non-string value in `data`,
// which has its own test for exactly that reason. Deleting on it means a single bug in the payload
// builder wipes every Android device from the registry on one send, and the symptom is "Android push
// stopped working" with no trace of why.
//
// `SENDER_ID_MISMATCH` is a wrong service account, i.e. also ours.
const GONE_CODES = new Set(['UNREGISTERED', 'NOT_FOUND']);
/** Wrong on OUR side. Never pruned, and not counted toward three strikes either. */
const MISCONFIGURED_CODES = new Set(['INVALID_ARGUMENT', 'SENDER_ID_MISMATCH', 'THIRD_PARTY_AUTH_ERROR']);
type Disposition = 'gone' | 'misconfigured' | 'transient';
const dispositionFor = (code: string): Disposition =>
GONE_CODES.has(code) ? 'gone' : MISCONFIGURED_CODES.has(code) ? 'misconfigured' : 'transient';
// Store first, env second — same reasoning as apns.ts.
const raw = (() => {
@@ -60,7 +79,7 @@ function buildMessage(token: string, n: Notification, ctx: SendContext): FcmMess
return { message: { token, data, android: { priority: 'high' } } };
}
type SendOutcome = { ok: boolean; dead: boolean; reason?: string };
type SendOutcome = { ok: boolean; disposition?: Disposition; reason?: string };
async function sendOne(
accessToken: string,
@@ -77,7 +96,7 @@ async function sendOne(
signal: AbortSignal.timeout(10_000),
});
if (res.ok) return { ok: true, dead: false };
if (res.ok) return { ok: true };
// FCM errors look like { error: { status: 'UNREGISTERED', message: '…' } }. The message can echo the
// token, so only the status is logged.
@@ -88,9 +107,9 @@ async function sendOne(
} catch {
/* keep the http status */
}
return { ok: false, dead: DEAD_TOKEN_CODES.has(status), reason: status };
return { ok: false, disposition: dispositionFor(status), reason: status };
} catch (err) {
return { ok: false, dead: false, reason: err instanceof Error ? err.message : 'send failed' };
return { ok: false, disposition: 'transient', reason: err instanceof Error ? err.message : 'send failed' };
}
}
@@ -131,12 +150,21 @@ export const fcmChannel: Channel = {
failed++;
console.error(`[notify:fcm] ${d.bundleId}: ${outcome.reason}`);
if (outcome.dead) {
if (outcome.disposition === 'gone') {
dead.push(d.token);
await deletePushDevice(d.token);
} else {
await recordPushFailure(d.token);
return;
}
if (outcome.disposition === 'misconfigured') {
console.error(
`[notify:fcm] '${outcome.reason}' is a problem with what WE sent, not with this device. ` +
`The row is kept so it can be diagnosed — check the message shape and the service account.`,
);
return;
}
await recordPushFailure(d.token);
}),
);
+4
View File
@@ -138,6 +138,10 @@ export type SidecarEvent =
// and `seq` is its cursor id there; officer relays it verbatim. No `seq` means it is not durable —
// an `assistant:delta` (superseded by the text that follows) or a message whose write failed.
| { type: 'claude:message'; sessionKey: string; msg: TurnMessage; seq?: number }
// A lightweight, non-durable ping for notification delivery — deliberately NOT part of
// chat_session_events/TurnMessage. Officer decides whether to actually notify (preference lookup +
// fan-out to the browser); this only says "something notify-worthy just happened for this session."
| { type: 'claude:notify'; sessionKey: string; userId: number; kind: 'turn-ended' | 'input-needed' }
| { type: 'claude:result'; id: string; result: ClaudeCodeResult }
| { type: 'claude:error'; id: string; error: string }
| { type: 'claude:killed'; id: string }