fix(events): three defects the live rig found, two of them data loss
All checks were successful
PR Checks / client-build (pull_request) Successful in 20s
PR Checks / frozen-manifest (pull_request) Successful in 41s
PR Checks / server-tests (pull_request) Successful in 8m33s

The whole-rig walk (ServUO + sidecar + website) against a real two-phase event.

- **A WS reconnect would have orphaned every live resource.** The backfill
  replays the last several `server.hello` frames in order — this rig saw three,
  each with a different `bootId` — so every replayed frame reads as a restart,
  and the intermediate ones 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. Gated on `!fromBackfill`, the rule the engagement fan-out and
  the SSE broadcast beside it already state. The website-was-down case is not
  missed — core asks every module at its own boot.
- **The shard explains its refusals and the run log dropped the explanation.**
  A 403 body reads `{"reason":"admin write plane disabled"}`; `legError` looks
  for `data.message`, finds nothing, and reports "sidecar responded 403". For a
  staff member clicking a button that is survivable. For an event that ran at
  four in the morning the run log is the only place anyone will learn why.
- **The "not retried" clause explained the wrong thing on a permanent status.**
  A 403 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 instead of at the switch they have to flip. The clause is now added
  only where a retry was genuinely given up, and 403/404 join the statuses the
  keyed verbs treat as terminal.

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
2026-09-04 07:36:03 -05:00
parent 57419111e6
commit 021f191f65
4 changed files with 132 additions and 11 deletions

View File

@@ -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. * The sidecar's answer, as an event outcome.
* *
@@ -117,8 +142,13 @@ async function currentBootId() {
* a leg's own `classify()`. * a leg's own `classify()`.
*/ */
function sidecarFailure(result, what) { function sidecarFailure(result, what) {
const { outcome, error } = classifySidecarWrite(result) const { outcome } = classifySidecarWrite(result)
return { ok: false, retry: outcome === 'retry', error: error || `the shard refused the ${what}` } 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. */ /** 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 // that retry away; that is the trade, taken knowingly, because the failure
// this refuses to risk is announcing twice to everyone online. Phase 11 // 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. // 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 { return {
ok: false, ok: false,
retry: 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_TITLE,
MAX_NEWS_BODY, MAX_NEWS_BODY,
MAX_OPTIONS, MAX_OPTIONS,
PERMANENT_STATUSES,
sidecarReason,
resourceId, resourceId,
crierLines, crierLines,
reconcileByBootId, reconcileByBootId,

View File

@@ -87,6 +87,35 @@ test('a hello with no bootId at all changes nothing', async () => {
assert.ok(!deps.order.includes('reconcile')) 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 () => { 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: // 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. // `ingest()` never throws, because a single event may not kill the socket.

View File

@@ -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 // 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 // the trade being taken knowingly — a lost announcement is cheaper than one
// delivered twice to everyone online. // 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}` }) uoLinkClient.adminBroadcast = async () => ({ ok: false, status, error: `status ${status}` })
const result = await broadcast.perform({ runId: 7, params: { text: 'hear ye' }, verify: false }) const result = await broadcast.perform({ runId: 7, params: { text: 'hear ye' }, verify: false })
assert.equal(result.ok, false) assert.equal(result.ok, false)
assert.equal(result.retry, false, `a ${status} must not be retried`) 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 () => { 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 }) await byId('uo.broadcast').perform({ runId: 42, params: { text: 'hear ye', hue: 1153 }, verify: false })
assert.equal(calls.broadcast.length, 1) 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.' } const params = { lines: 'hear ye', title: 'The Fair', body: 'Merchants gather.' }
// The announce leg's own classification of this transport, reused rather // The announce leg's own classification of this transport, reused rather
// than re-decided: a config or data problem is terminal, the rest transient. // 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}` }) uoLinkClient[stub] = async () => ({ ok: false, status, error: `status ${status}` })
const result = await byId(id).perform({ runId: 1, idempotencyKey: 'k'.repeat(40), params, verify: false }) const result = await byId(id).perform({ runId: 1, idempotencyKey: 'k'.repeat(40), params, verify: false })
assert.equal(result.ok, false) assert.equal(result.ok, false)

View File

@@ -103,7 +103,7 @@ async function resolveShardName(shard, deps) {
// Apply the state-change side effect for a kind (if any). Returns a promise. // Apply the state-change side effect for a kind (if any). Returns a promise.
async function applyStateChange(event, deps) { async function applyStateChange(event, deps) {
const { shardState, uoLinkConfig, eventsReconcile, log } = deps const { shardState, uoLinkConfig, eventsReconcile, fromBackfill, log } = deps
switch (event.kind) { switch (event.kind) {
case 'server.hello': { case 'server.hello': {
const incoming = event.bootId || null const incoming = event.bootId || null
@@ -117,7 +117,7 @@ async function applyStateChange(event, deps) {
} }
if (incoming) state.bootId = incoming if (incoming) state.bootId = incoming
await uoLinkConfig.recordStatus({ pluginConnected: true, bootId: incoming, lastEventAt: event.t }) 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 // 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 // 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 // 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 // row. Asking first would have every resource compared against the boot
// that has just ended, and every one of them would look live. // 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 // **And never on a backfill replay**, which is the same rule the
// is nothing an ingest handler could correctly do with the answer. // 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() eventsReconcile()
} }
return return
@@ -315,6 +323,10 @@ function resolveDeps(deps) {
// reconcile must be able to see the call without a live event engine behind // reconcile must be able to see the call without a live event engine behind
// it. // it.
eventsReconcile: deps.eventsReconcile || (() => coreEvents.reconcile()), 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, log: deps.log || defaultLog,
} }
} }