From 8def6e19f485d0b4a3a1d32435bea8ba95b155a4 Mon Sep 17 00:00:00 2001 From: wtclaude Date: Wed, 9 Sep 2026 08:27:50 -0500 Subject: [PATCH] fix(events): the atlas import, and a teardown that was a no-op (Phase 16a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects the acceptance walk found in shipped code, both invisible to the suites that were green on either side of them. **The spawn atlas cannot import on a stock ServUO tree.** `spawnAtlasSource.js` dedupes decoration types with a case-SENSITIVE `Map`, but `shard_decor_types.type` is a PRIMARY KEY under MariaDB's default `..._ai_ci` collation, which folds case. Stock 57.4's own `Data/Decoration/` names four types under two spellings each (CheckerBoard/Checkerboard, ChessBoard/Chessboard, MetalChest/Metalchest, SpinningWheelEastAddon/SpinningwheelEastAddon), and in every pair exactly one is a real class. The second row raised `1062 Duplicate entry` and took the WHOLE import transaction down. The blast radius is not decoration: with no atlas, EVERY option source answers empty and no Phase 12 world verb can be authored at all. The shard end already knew — `BridgeWorld.cs` resolves a decor type with `FindTypeByName(name, ignoreCase: true)` and its comment says the atlas and the decoration files disagree about casing. Folding here is the two ends agreeing. **Teardown of every world verb was a no-op that reported success.** `revertOwned` forwarded core's `idempotencyKey` as the despawn's OWN key — and core's key is the step's, the one `placeOwned` spawned under. `BridgeIdempotency` keys on the key alone, so the despawn was taken for a repeat and answered with the SPAWN's stored reply; `OnDespawn` never ran. Core read `ok` with no `refused` and marked every row `reverted` while the shard still held every object. Measured on the rig: ledger `world | reverted | 21`, shard `world.owned` 21 alive with `pruned: 0`, and the identical despawn re-sent with a fresh key removed all 21. It affected all five world verbs, so an invasion's creatures, boss, oracle, gate and decoration stayed in the world for ever while the console reported a clean teardown. `MODULE_API.md` says what that key is for and it is not this: it identifies a dispatch core never learned the outcome of, so the module can ask about it. No key is needed on a despawn — a repeat answers `gone`, which both ends already treat as success — and dropping it also makes the documented empty-`resources` case work, since no serials means "everything this run owns". The parameter is removed from `despawnWorld`'s signature rather than left optional. Both fixes are verified end to end against a real ServUO + sidecar + website rig: the import now yields 309 decor types (was failing at 313 with 4 collisions), 6,455 spawn points, 800 creatures, 558 landmarks; and a full four-phase run's teardown left the shard owning 0 objects. Each new test was confirmed to FAIL without its fix. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016wDDVXWMDz82WqE1i969r4 --- server/config/uoEventActions.js | 21 +++++++++++-- server/test/spawnAtlas.source.test.js | 36 +++++++++++++++++++++ server/test/uoEventActions.test.js | 45 +++++++++++++++++++++++++++ server/utils/spawnAtlasSource.js | 22 +++++++++++-- server/utils/uoLinkClient.js | 12 +++++-- 5 files changed, 130 insertions(+), 6 deletions(-) diff --git a/server/config/uoEventActions.js b/server/config/uoEventActions.js index 674cedb..02dd32b 100644 --- a/server/config/uoEventActions.js +++ b/server/config/uoEventActions.js @@ -481,12 +481,29 @@ async function placeOwned({ runId, idempotencyKey, what, body }) { * shard denies this run ever owned that serial — nothing will ever delete it * through this path, so the row must land unresolved with a reason rather than * be quietly marked reverted. + * + * **The despawn carries NO idempotency key, and that is the whole point.** This + * function used to forward core's `idempotencyKey` as the despawn's own — which + * is the step's key, the very key `placeOwned` spawned under. The shard's + * at-most-once store is keyed on the key ALONE (`BridgeIdempotency.Intercept` + * does `_byKey.TryGetValue(key, …)`, not a lookup by key AND command), so the + * despawn was recognised as a repeat and answered with the SPAWN's stored reply. + * `OnDespawn` never ran, core saw `ok` with no `refused`, and every row was + * marked `reverted` while the shard still held every object. Teardown of all + * five world verbs was a no-op that reported success. + * + * No key is needed here. A repeat despawn is already safe by the handler's own + * three-answer design: the second pass finds the serial gone and answers `gone`, + * which is a success on both ends. `MODULE_API.md` says what core's key is FOR, + * and it is not this — it identifies a dispatch core never learned the outcome + * of, so the module can ask about it. That case arrives here as an EMPTY + * `resources` list, and it is answered correctly by the same call: no serials + * means "everything this run owns", which is exactly the right sweep. */ -async function revertOwned({ runId, resources, idempotencyKey }) { +async function revertOwned({ runId, resources }) { const result = await uoLinkClient.despawnWorld({ runId: String(runId), serials: resources.map((resource) => resource.ref), - idempotencyKey, }) if (!result.ok) return { ok: false, error: sidecarReason(result, 'despawn') } diff --git a/server/test/spawnAtlas.source.test.js b/server/test/spawnAtlas.source.test.js index b22bdd3..efc5ffb 100644 --- a/server/test/spawnAtlas.source.test.js +++ b/server/test/spawnAtlas.source.test.js @@ -440,6 +440,42 @@ test('decoration is read recursively and rolled up per type', () => { } }) +test('two spellings of one decoration type fold into one row', () => { + // The Phase 16 acceptance walk's blocking finding. Stock ServUO 57.4's own + // `Data/Decoration/` names four types under two casings each — + // CheckerBoard/Checkerboard, ChessBoard/Chessboard, MetalChest/Metalchest, + // SpinningWheelEastAddon/SpinningwheelEastAddon — and in every pair exactly one + // is a real class; the other is a mis-cased line the shard's own loader resolves + // anyway. + // + // A case-SENSITIVE Map keeps both. `shard_decor_types.type` is a PRIMARY KEY + // under MariaDB's default `..._ai_ci` collation, which folds case, so the second + // row raised `1062 Duplicate entry` and took the WHOLE atlas import transaction + // down with it. The blast radius is not decoration: with no atlas, EVERY option + // source answers empty and no world verb can be authored at all. + // + // Asserted on the count as well as the row, because the failure mode was two + // rows that a database — not this function — would later refuse. + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'atlas-decorcase-')) + try { + writeTree(root) + fs.writeFileSync( + path.join(root, 'Data', 'Decoration', 'miscased.cfg'), + 'checkerboard 0x0FA6\n600 600 0\nCheckerBoard 0x0FA6\n700 700 0\n', + ) + const atlas = buildAtlas(root) + + const boards = atlas.decor.filter((d) => d.type.toLowerCase() === 'checkerboard') + assert.equal(boards.length, 1, 'two casings of one type must not be two rows') + // First spelling seen wins, exactly as the first item id does. Which one + // survives is cosmetic — the shard resolves either. + assert.equal(boards[0].type, 'checkerboard') + assert.equal(boards[0].uses, 2, 'both lines still count as uses of the one type') + } finally { + fs.rmSync(root, { recursive: true, force: true }) + } +}) + test('a tree with no decoration at all still builds', () => { // Optional, like the champion file. A shard that has stripped its decoration // has a perfectly good atlas; the decoration verb simply has nothing to offer. diff --git a/server/test/uoEventActions.test.js b/server/test/uoEventActions.test.js index 1067166..dabf238 100644 --- a/server/test/uoEventActions.test.js +++ b/server/test/uoEventActions.test.js @@ -766,6 +766,51 @@ test('teardown reports a refused serial as failed, and a killed creature as done assert.equal((await actions.revertOwned({ runId: 7, resources })).ok, false) }) +test('the despawn carries NO idempotency key, whatever core hands revert()', async () => { + // The Phase 16 acceptance walk's critical finding, as the test that would have + // caught it. `revertOwned` used to forward core's `idempotencyKey` onto the + // despawn — and core's key is the STEP's, the one `placeOwned` spawned under. + // The shard's at-most-once store is keyed on the key ALONE + // (`BridgeIdempotency.Intercept` does `_byKey.TryGetValue(key, …)`, with no + // reference to which command carried it), so the despawn was taken for a repeat + // and answered with the SPAWN's stored reply. `OnDespawn` never ran. Core read + // `ok` with no `refused` and marked every row `reverted` while the shard still + // held every object — teardown of all five world verbs was a no-op that + // reported success. + // + // Every other stub in this file ignores the body, which is why the suite was + // green throughout. This one asserts on the body, and it asserts ABSENCE — the + // property that matters — rather than pinning the rest of the shape. + let sent = null + uoLinkClient.despawnWorld = async (body) => { + sent = body + return { ok: true, status: 200, data: { removed: ['0x40000000'], gone: [], refused: [] } } + } + + await actions.revertOwned({ + runId: 7, + resources: [{ kind: 'world', ref: '0x40000000', payload: {} }], + // Core passes this on every call (MODULE_API.md), and it must not reach the wire. + idempotencyKey: 'the-step-key-the-spawn-went-out-under', + }) + + assert.ok(sent, 'despawnWorld was not called') + assert.equal( + Object.prototype.hasOwnProperty.call(sent, 'idempotencyKey'), + false, + 'the despawn must not carry an idempotency key — the shard would replay the spawn', + ) + + // MODULE_API.md: revert is sometimes called with the key and an EMPTY list, + // meaning "a command went out under this key and core never learned what it + // did". No serials is the shard's own idiom for "everything this run owns", + // which is the correct sweep for exactly that case. + sent = null + await actions.revertOwned({ runId: 7, resources: [], idempotencyKey: 'lost-dispatch' }) + assert.deepEqual(sent.serials, []) + assert.equal(Object.prototype.hasOwnProperty.call(sent, 'idempotencyKey'), false) +}) + test('reconcile ASKS the shard, because these resources survive a restart', async () => { // The one property that separates this from every other resource in the file. // A crier line lives in shard memory, so a changed `bootId` IS proof it is diff --git a/server/utils/spawnAtlasSource.js b/server/utils/spawnAtlasSource.js index 78ef0b2..5b9364e 100644 --- a/server/utils/spawnAtlasSource.js +++ b/server/utils/spawnAtlasSource.js @@ -336,11 +336,29 @@ function buildAtlas(root, options = {}) { // Decoration: what this shard already calls scenery, which is what makes the // authoring dropdown the operator's own vocabulary rather than our taste. + // + // **Keyed case-INSENSITIVELY, because the decoration files disagree with + // themselves about casing.** Stock 57.4 names four types under two spellings + // each — `CheckerBoard`/`Checkerboard`, `ChessBoard`/`Chessboard`, + // `MetalChest`/`Metalchest`, `SpinningWheelEastAddon`/`SpinningwheelEastAddon` + // — and in every pair exactly one is a real class, the other a mis-cased line + // the shard's own loader resolves anyway. A case-sensitive Map keeps both, and + // then `shard_decor_types.type` (a PRIMARY KEY under MariaDB's default + // `..._ai_ci` collation, which folds case) rejects the second row and takes the + // WHOLE import transaction down with it. That is not a decoration bug: with no + // atlas, every option source answers empty and no world verb can be authored at + // all. The shard end of this feature already knew — `BridgeWorld.cs` resolves a + // decor type with `FindTypeByName(name, ignoreCase: true)` and says why — so + // folding here is the two ends agreeing rather than a new rule. + // + // The first spelling seen wins, exactly as the first item id does. Either + // spelling resolves on the shard, so which one survives is cosmetic. const decorUses = new Map() for (const file of files) { if (!file.label.startsWith('Data/Decoration/')) continue for (const entry of parseDecoration(file.text)) { - const seen = decorUses.get(entry.type) + const key = entry.type.toLowerCase() + const seen = decorUses.get(key) if (seen) { seen.uses += 1 continue @@ -348,7 +366,7 @@ function buildAtlas(root, options = {}) { // The FIRST item id wins, and it is only a preview: a type appears under // as many ids as it has facings or variants, and picking one arbitrarily // is honest in a way that picking "the most used" would not be. - decorUses.set(entry.type, { type: entry.type, itemId: entry.itemId, uses: 1 }) + decorUses.set(key, { type: entry.type, itemId: entry.itemId, uses: 1 }) } } const decor = [...decorUses.values()].sort((a, b) => a.type.localeCompare(b.type)) diff --git a/server/utils/uoLinkClient.js b/server/utils/uoLinkClient.js index 16d4cd6..c9f5aed 100644 --- a/server/utils/uoLinkClient.js +++ b/server/utils/uoLinkClient.js @@ -330,10 +330,18 @@ const ownedWorld = ({ runId }) => call(`/world/${encodeURIComponent(runId)}`) // teardown makes. The reply splits three ways: `removed` was deleted, `gone` was // already absent (a player killed it — an ordinary success), and `refused` was never // this run's to delete. -const despawnWorld = ({ runId, serials, idempotencyKey }) => +// +// **It takes no idempotency key, and the parameter is gone rather than optional.** +// It used to accept one, and `revertOwned` passed the step's — the key the SPAWN +// went out under. The shard's at-most-once store is keyed on the key alone, so the +// despawn was answered with the spawn's stored reply and nothing was ever deleted. +// A repeat despawn needs no key: the second pass answers `gone`, which both ends +// already treat as a success. Removed from the signature so it cannot be handed +// one again by accident. +const despawnWorld = ({ runId, serials }) => call(`/world/${encodeURIComponent(runId)}/despawn`, { method: 'POST', - body: { serials, idempotencyKey }, + body: { serials }, }) // ── Help-page (support) queue commands (§6) ────────────────────────────────