diff --git a/server/config/uoEventActions.js b/server/config/uoEventActions.js index 103985e..c73c0a4 100644 --- a/server/config/uoEventActions.js +++ b/server/config/uoEventActions.js @@ -106,6 +106,31 @@ async function currentBootId() { } } +// Statuses that will never succeed however many times they are tried: a data +// refusal, a bad token, a switched-off write plane, a protocol mismatch. Named +// here rather than folded into `shardAnnounce.classify` because 403 is reachable +// only from the `/admin/*` verbs — the announce leg posts to the town crier, +// which the admin write plane does not gate — and widening a shared classifier +// for a case its own caller cannot produce is how a shared rule stops being one. +const PERMANENT_STATUSES = new Set([400, 401, 403, 404, 409]) + +/** + * What the shard actually said, in its own words. + * + * **The sidecar explains its refusals and `legError` drops the explanation**, and + * this was worth its own helper the moment an event started making these calls + * unattended. A `403` body reads `{"reason":"admin write plane disabled"}`; + * `legError` looks for `data.message`, finds nothing, and falls back to "sidecar + * responded 403". For a staff member clicking a button that is survivable — they + * know what they just switched off. For an event that ran at four in the morning, + * the run log is the only place anyone will ever learn why, and "403" is not an + * answer an operator can act on. + */ +function sidecarReason(result, what) { + const data = (result && result.data) || {} + return data.reason || data.message || (result && result.error) || `the shard refused the ${what}` +} + /** * The sidecar's answer, as an event outcome. * @@ -117,8 +142,13 @@ async function currentBootId() { * a leg's own `classify()`. */ function sidecarFailure(result, what) { - const { outcome, error } = classifySidecarWrite(result) - return { ok: false, retry: outcome === 'retry', error: error || `the shard refused the ${what}` } + const { outcome } = classifySidecarWrite(result) + const permanent = PERMANENT_STATUSES.has(result && result.status) + return { + ok: false, + retry: outcome === 'retry' && !permanent, + error: sidecarReason(result, what), + } } /** Split an authored text block into crier lines, and say why it is not one. */ @@ -228,11 +258,20 @@ const ACTIONS = [ // that retry away; that is the trade, taken knowingly, because the failure // this refuses to risk is announcing twice to everyone online. Phase 11 // puts an idempotency key on the wire and this line is what changes. - const { error } = classifySidecarWrite(result) + // + // **The clause is only added where a retry was genuinely given up**, and + // the rig is what made that distinction matter. A 403 — the shard's admin + // write plane switched off — will not succeed on any attempt, so telling an + // operator it was "not retried because a repeat would announce twice" points + // them at a policy decision when what they need is the sentence the shard + // already wrote: "admin write plane disabled". A reason that explains the + // wrong thing is worse than a bare status code. + const reason = sidecarReason(result, 'broadcast') + if (PERMANENT_STATUSES.has(result.status)) return { ok: false, retry: false, error: reason } return { ok: false, retry: false, - error: `${error || 'the shard refused the broadcast'} (not retried: a repeat would announce twice)`, + error: `${reason} (not retried: a repeat would announce twice)`, } }, }, @@ -527,6 +566,8 @@ module.exports = { MAX_NEWS_TITLE, MAX_NEWS_BODY, MAX_OPTIONS, + PERMANENT_STATUSES, + sidecarReason, resourceId, crierLines, reconcileByBootId, diff --git a/server/test/shardIngest.eventReconcile.test.js b/server/test/shardIngest.eventReconcile.test.js index 69a9e17..9f3475b 100644 --- a/server/test/shardIngest.eventReconcile.test.js +++ b/server/test/shardIngest.eventReconcile.test.js @@ -87,6 +87,35 @@ test('a hello with no bootId at all changes nothing', async () => { assert.ok(!deps.order.includes('reconcile')) }) +test('a backfill replay never reconciles, however many boots it walks through', async () => { + // **The defect the live rig found, and nothing else could.** A WS reconnect + // replays the last several `server.hello` frames in order — this rig saw three, + // each with a different `bootId` — so every replayed frame looks like a + // restart. Acting on the intermediate ones would compare a resource stamped + // with the CURRENT boot against a boot that ended hours ago and mark it + // `orphaned`: a live crier line core will never take down again, lost to + // nothing worse than the website reconnecting. + const deps = makeDeps() + await shardIngest.ingest(hello('boot-1'), deps) + for (const boot of ['boot-2', 'boot-3', 'boot-4']) { + await shardIngest.ingest(hello(boot), { ...deps, fromBackfill: true }) + } + assert.ok(!deps.order.includes('reconcile')) + // The replay still moves the tracked boot on, so the NEXT live hello is + // measured against where the replay left off rather than against boot-1. + assert.ok(deps.order.includes('recordStatus:boot-4')) +}) + +test('a live hello after a replay is still a restart', async () => { + // The gate is about the frame, not about the module going quiet: skipping the + // replay must not make the next genuine restart invisible. + const deps = makeDeps() + await shardIngest.ingest(hello('boot-1'), deps) + await shardIngest.ingest(hello('boot-2'), { ...deps, fromBackfill: true }) + await shardIngest.ingest(hello('boot-3'), deps) + assert.equal(deps.order.filter((s) => s === 'reconcile').length, 1) +}) + test('a reconcile that throws does not take the ingest down with it', async () => { // Fire-and-forget by the contract, and the feed must survive one bad module: // `ingest()` never throws, because a single event may not kill the socket. diff --git a/server/test/uoEventActions.test.js b/server/test/uoEventActions.test.js index 2241780..7fb22fe 100644 --- a/server/test/uoEventActions.test.js +++ b/server/test/uoEventActions.test.js @@ -142,15 +142,54 @@ test('a broadcast is never retried, whatever the sidecar says', async () => { // a shard that timed out. The last two are genuinely transient, and this is // the trade being taken knowingly — a lost announcement is cheaper than one // delivered twice to everyone online. - for (const status of [0, 400, 401, 409, 503, 504]) { + for (const status of [0, 400, 401, 403, 409, 503, 504]) { uoLinkClient.adminBroadcast = async () => ({ ok: false, status, error: `status ${status}` }) const result = await broadcast.perform({ runId: 7, params: { text: 'hear ye' }, verify: false }) assert.equal(result.ok, false) assert.equal(result.retry, false, `a ${status} must not be retried`) - assert.match(result.error, /announce twice/, 'the refusal must say why it is not retried') + // The clause belongs only where a retry was genuinely given up. On a + // permanent status it would explain the wrong thing. + if (!actions.PERMANENT_STATUSES.has(status)) { + assert.match(result.error, /announce twice/, 'a discarded retry must say why') + } else { + assert.doesNotMatch(result.error, /announce twice/, `a ${status} was never retryable`) + } } }) +test("the shard's own words reach the run log, not just a status code", async () => { + // **The rig found this.** The sidecar refuses a broadcast with + // `{"reason":"admin write plane disabled"}` and `legError` looks for + // `data.message`, so the run console read "sidecar responded 403" for a cause + // the shard had already explained in a sentence. A staff member clicking a + // button knows what they switched off; an event that ran at four in the morning + // leaves the run log as the only place anyone will learn why. + uoLinkClient.adminBroadcast = async () => ({ + ok: false, + status: 403, + data: { kind: 'admin.error', reason: 'admin write plane disabled' }, + error: 'sidecar responded 403', + }) + const result = await byId('uo.broadcast').perform({ runId: 1, params: { text: 'hear ye' }, verify: false }) + assert.match(result.error, /admin write plane disabled/) + // And NOT the double-announce clause: a 403 will not succeed on any attempt, so + // pointing an operator at a policy decision misdirects them away from the + // switch they actually have to flip. + assert.doesNotMatch(result.error, /announce twice/) + assert.equal(result.retry, false) +}) + +test('a permanent refusal of a keyed verb is not retried either', async () => { + // Same distinction on the other side: the keyed verbs DO retry a transient, and + // must not burn three attempts on a refusal that cannot change. + uoLinkClient.postTownCrier = async () => ({ ok: false, status: 403, data: { reason: 'admin write plane disabled' } }) + const result = await byId('uo.towncrier.post').perform({ + runId: 1, idempotencyKey: 'k'.repeat(40), params: { lines: 'hear ye' }, verify: false, + }) + assert.equal(result.retry, false) + assert.match(result.error, /admin write plane disabled/) +}) + test('a broadcast names its run in the shard audit, not a staff member', async () => { await byId('uo.broadcast').perform({ runId: 42, params: { text: 'hear ye', hue: 1153 }, verify: false }) assert.equal(calls.broadcast.length, 1) @@ -223,7 +262,7 @@ test('the keyed verbs DO retry, because a repeat replaces', async () => { const params = { lines: 'hear ye', title: 'The Fair', body: 'Merchants gather.' } // The announce leg's own classification of this transport, reused rather // than re-decided: a config or data problem is terminal, the rest transient. - for (const [status, retry] of [[400, false], [401, false], [409, false], [503, true], [504, true], [0, true]]) { + for (const [status, retry] of [[400, false], [401, false], [403, false], [409, false], [503, true], [504, true], [0, true]]) { uoLinkClient[stub] = async () => ({ ok: false, status, error: `status ${status}` }) const result = await byId(id).perform({ runId: 1, idempotencyKey: 'k'.repeat(40), params, verify: false }) assert.equal(result.ok, false) diff --git a/server/utils/shardIngest.js b/server/utils/shardIngest.js index e084588..7a040c1 100644 --- a/server/utils/shardIngest.js +++ b/server/utils/shardIngest.js @@ -103,7 +103,7 @@ async function resolveShardName(shard, deps) { // Apply the state-change side effect for a kind (if any). Returns a promise. async function applyStateChange(event, deps) { - const { shardState, uoLinkConfig, eventsReconcile, log } = deps + const { shardState, uoLinkConfig, eventsReconcile, fromBackfill, log } = deps switch (event.kind) { case 'server.hello': { const incoming = event.bootId || null @@ -117,7 +117,7 @@ async function applyStateChange(event, deps) { } if (incoming) state.bootId = incoming await uoLinkConfig.recordStatus({ pluginConnected: true, bootId: incoming, lastEventAt: event.t }) - if (restarted) { + if (restarted && !fromBackfill) { // EVENTS.md F: core has no concept of the game being up, so the module // says when a ledger of live shard resources has become a claim about a // world that no longer exists. This is that moment, and a changed @@ -130,8 +130,16 @@ async function applyStateChange(event, deps) { // row. Asking first would have every resource compared against the boot // that has just ended, and every one of them would look live. // - // Fire-and-forget by the contract: core logs what it orphaned, and there - // is nothing an ingest handler could correctly do with the answer. + // **And never on a backfill replay**, which is the same rule the + // engagement fan-out and the SSE broadcast state below and is far more + // expensive to break here. A reconnect replays the last several + // `server.hello` frames in order — this rig saw three, each with a + // different `bootId` — so every replayed frame looks like a restart, and + // the intermediate ones would compare a resource stamped with the CURRENT + // boot against a boot that ended hours ago. The row is then `orphaned`: + // a live crier line core will never take down again, lost to nothing + // worse than the website reconnecting. The website-was-down case is not + // missed by skipping these — core asks every module at its own boot. eventsReconcile() } return @@ -315,6 +323,10 @@ function resolveDeps(deps) { // reconcile must be able to see the call without a live event engine behind // it. eventsReconcile: deps.eventsReconcile || (() => coreEvents.reconcile()), + // Not injectable — it is the caller's statement about this frame rather than + // a dependency. It reaches `applyStateChange` because the reconcile below is + // the one state change that must not act on a replay; see the note there. + fromBackfill: Boolean(deps.fromBackfill), log: deps.log || defaultLog, } }