diff --git a/index.js b/index.js index 4536b63..eb3febd 100644 --- a/index.js +++ b/index.js @@ -69,26 +69,55 @@ export function createStructon(BaseClass) { _structonEncode(value, encodeOptions, superEncode) { if (value && typeof value === 'object' && value.constructor === Object) { - const prevLen = this.typedStructs.length; - let structuresUpdated = false; - this._onStructureAdded = () => { structuresUpdated = true; }; - try { - const encoded = writeStruct(value, v => this.encode(v), this); - if (encoded !== null) { - if (structuresUpdated || this.typedStructs.length !== prevLen) { - this._saveTypedStructures(); + // A struct encoding is a bare reference to a typed structure id, so it is only valid + // once that structure is durably saved. When saveStructures declines (a concurrent + // writer advanced the shared structures), reload and re-mint against the durable + // dictionary rather than returning bytes that point at an id nobody persisted — the + // same contract msgpackr's own pack call site enforces by re-packing on a declined + // save. One retry: the reload realigns us with durable, so a second decline means + // sustained contention, which we surface rather than paper over. + for (let attempt = 0; ; attempt++) { + const prevLen = this.typedStructs.length; + let structuresUpdated = false; + this._onStructureAdded = () => { structuresUpdated = true; }; + try { + const encoded = writeStruct(value, v => this.encode(v), this); + if (encoded !== null) { + // On a retry, always re-attempt the save: the declined attempt means durable may + // not hold our dictionary at all, and the reload may not have changed it (an empty + // durable leaves our unpersisted mint in place), so "no new structure this pass" + // does not imply the referenced id is persisted. + if (attempt > 0 || structuresUpdated || this.typedStructs.length !== prevLen) { + if (this._saveTypedStructures() === false) { + if (attempt > 0) { + throw new Error( + 'Unable to save typed structures: saveStructures declined twice, ' + + 'the encoded structure id would not be persisted'); + } + this._loadStructures(); + continue; + } + } + return encoded; } - return encoded; + // Capped miss: fall back to plain base encoding. The base may persist its own + // named structures via saveStructures, overwriting our combined {named, typed} + // payload and stranding previously written struct data. Re-save afterward so the + // typed structures survive (this.structures now also holds any base record added). + const result = superEncode(value, encodeOptions); + if (this.typedStructs && this.typedStructs.length > 0) { + // Best-effort: unlike the struct path above, `result` carries no typed-structure + // reference, so a declined re-save cannot strand this record — only previously + // written struct data. Reload and try once more, then return the (valid) bytes. + if (this._saveTypedStructures() === false && attempt === 0) { + this._loadStructures(); + this._saveTypedStructures(); + } + } + return result; + } finally { + this._onStructureAdded = null; } - // Capped miss: fall back to plain base encoding. The base may persist its own - // named structures via saveStructures, overwriting our combined {named, typed} - // payload and stranding previously written struct data. Re-save afterward so the - // typed structures survive (this.structures now also holds any base record added). - const result = superEncode(value, encodeOptions); - if (this.typedStructs && this.typedStructs.length > 0) this._saveTypedStructures(); - return result; - } finally { - this._onStructureAdded = null; } } return superEncode(value, encodeOptions); @@ -159,6 +188,11 @@ export function createStructon(BaseClass) { if (sharedData) onLoadedStructures.call(this, sharedData); } + /** + * Persist the combined {named, typed} structures. Returns `false` when saveStructures + * declined the save (CAS conflict or a non-durable commit); callers must not return an + * encoding that references a structure id from a declined save. + */ _saveTypedStructures() { if (typeof this.saveStructures === 'function') { const structures = prepareStructures(this.structures || [], this); @@ -168,7 +202,7 @@ export function createStructon(BaseClass) { // that runs an optimistic CAS on the parameter (e.g. Harper's RocksDB override) sees // `undefined` and a concurrent same-length save silently clobbers the previously persisted // struct. See HarperFast/harper#1441. - this.saveStructures(structures, structures.isCompatible); + return this.saveStructures(structures, structures.isCompatible); } else if (typeof this.saveShared === 'function') { this.saveShared({ structures: this.structures || [], diff --git a/struct.js b/struct.js index 57a71ba..8a13887 100644 --- a/struct.js +++ b/struct.js @@ -1185,10 +1185,21 @@ export function onLoadedStructures(sharedData) { named = sharedData; typed = this.typedStructs || []; } else if (typeof sharedData === 'object') { - // Plain object form (e.g. a Map round-tripped through msgpackr with - // mapsAsObjects: true, which is the default). - named = sharedData.named || []; - typed = sharedData.typed || []; + if (!sharedData.named && !sharedData.typed && Array.isArray(sharedData.structures)) { + // cbor-x's SharedData ({structures, packedValues, version}). Its own updateSharedData + // writes this shape through the same consumer-supplied saveStructures we use, so a + // durable store can be left holding it (e.g. a capped plain-fallback whose base encode + // saved, then our re-save was declined). It carries named structures only — keep ours, + // same as the legacy bare-array form. Without this, the object branch below reads no + // `named`/`typed` and silently clears the whole dictionary. + named = sharedData.structures; + typed = this.typedStructs || []; + } else { + // Plain object form (e.g. a Map round-tripped through msgpackr with + // mapsAsObjects: true, which is the default). + named = sharedData.named || []; + typed = sharedData.typed || []; + } } else { return this.structures; } diff --git a/tests/test-cbor-x.js b/tests/test-cbor-x.js index c71e81b..3aec4e7 100644 --- a/tests/test-cbor-x.js +++ b/tests/test-cbor-x.js @@ -435,3 +435,54 @@ suite('structon (cbor-x base) – maxOwnStructures cap', function () { assert.ok(enc.typedStructs.length <= 1, 'same-encoder getter mints must not exceed the cap, got ' + enc.typedStructs.length); }); }); + +// cbor-x writes shared structures through the SAME consumer-supplied saveStructures callback that +// structon uses, but in its own shape: SharedData {structures, packedValues, version} (encode.js +// updateSharedData), versus structon's Map {named, typed}. A durable store can therefore be left +// holding cbor-x's form — e.g. a capped plain-fallback where the base encode saved and structon's +// re-save was then declined. onLoadedStructures must recognize it; reading it as the generic plain +// object form finds no `named`/`typed` and silently clears the entire dictionary. + +suite('structon (cbor-x base) – reload from cbor-x SharedData form', function () { + // Capture a genuine SharedData instance from cbor-x's own save path rather than hand-building + // the shape, so the test tracks whatever cbor-x actually writes. + function captureSharedData() { + let captured = null; + const enc = new Structon({ + structures: [], + maxOwnStructures: 1, + saveStructures(s) { if (s && !(s instanceof Map) && Array.isArray(s.structures)) captured = s; return true; }, + }); + enc.encode({ a: 1, b: 2 }); // mints a typed struct (cap = 1) + enc.encode({ p: 'x', q: 'y', r: 'z' }); // capped miss → base encode → cbor-x saves SharedData + assert.ok(captured, 'cbor-x should have written its SharedData form via saveStructures'); + return captured; + } + + test('cbor-x SharedData is recognized on reload and does not clear the dictionary', function () { + const sharedData = captureSharedData(); + assert.ok(Array.isArray(sharedData.structures), 'captured payload should carry a structures array'); + assert.strictEqual(sharedData.named, undefined, 'cbor-x form has no `named` key'); + + const enc = new Structon({ structures: [], getStructures() { return sharedData; } }); + enc.encode({ a: 1, b: 2 }); + const typedBefore = enc.typedStructs.length; + assert.ok(typedBefore > 0, 'encoder should hold a typed structure before the reload'); + + enc._loadStructures(); + + assert.strictEqual(enc.typedStructs.length, typedBefore, + 'reloading cbor-x\'s named-only form must preserve typed structures, not clear them'); + assert.deepStrictEqual(enc.structures.slice(), sharedData.structures, + 'the named structures from the cbor-x form should be adopted'); + }); + + test('records still decode after a reload from the cbor-x SharedData form', function () { + const sharedData = captureSharedData(); + const enc = new Structon({ structures: [], getStructures() { return sharedData; } }); + const buf = enc.encode({ a: 1, b: 2 }); + enc._loadStructures(); + const out = enc.decode(buf); + assert.deepStrictEqual(out.toJSON ? out.toJSON() : out, { a: 1, b: 2 }); + }); +}); diff --git a/tests/test.js b/tests/test.js index 9f47fa9..e1d8996 100644 --- a/tests/test.js +++ b/tests/test.js @@ -837,3 +837,89 @@ suite('structon – typed structure reload on miss (standalone, harper#1163)', f assert.strictEqual(store.loads, loads, 'a known structure id should not cause a reload'); }); }); + +// A struct encoding is a bare reference to a typed structure id, so it is only meaningful once that +// structure is durably saved. When a CAS-ing saveStructures declines the save (a concurrent writer +// advanced the shared structures), returning the encoded bytes as-is strands the record: it points at +// an id that was never persisted, or — worse — at an id the winning writer assigned to a different +// shape, so it decodes as another record's fields. msgpackr's own pack call site handles this by +// re-packing on a declined save; the standalone path must do the same. + +suite('structon – declined structure save is retried (standalone)', function () { + const StandaloneStructon = createStructon(LegacyPackr); // v1.11: no struct hooks → standalone path + + // A durable store with the optimistic CAS a real backing store runs (e.g. Harper's RocksDB + // RecordEncoder override): the save commits only if the caller's view of the existing structures + // is still current. + function casStore() { + const meta = new Packr(); + let buf = null; + let saves = 0; + let declines = 0; + return { + saveStructures(s, isCompatible) { + const existing = buf ? meta.decode(buf) : undefined; + if (typeof isCompatible === 'function' && !isCompatible(existing)) { + declines++; + return false; + } + buf = meta.encode(s); + saves++; + return true; + }, + getStructures() { return buf ? meta.decode(buf) : undefined; }, + get saves() { return saves; }, + get declines() { return declines; }, + }; + } + + test('a record encoded against a declined save is re-minted against durable', function () { + const store = casStore(); + const options = { structures: [], saveStructures: store.saveStructures, getStructures: store.getStructures }; + const writerA = new StandaloneStructon(options); + const writerB = new StandaloneStructon(options); + + // A mints and persists its shape first. + const bufA = writerA.encode({ a: 1, b: 2 }); + + // B still has an empty view, so its save is CAS-declined. It must reload, re-mint against the + // durable dictionary, and re-save — otherwise its bytes reference an id durable assigned to A's + // shape and decode as A's fields. + const bufB = writerB.encode({ p: 'x', q: 'y', r: 'z' }); + assert.ok(store.declines >= 1, 'the stale writer\'s first save should have been CAS-declined'); + + const reader = new StandaloneStructon({ structures: [], getStructures: store.getStructures }); + assert.deepStrictEqual(materialize(reader.decode(bufB)), { p: 'x', q: 'y', r: 'z' }); + // The winning writer's record must survive the re-mint too. + assert.deepStrictEqual(materialize(reader.decode(bufA)), { a: 1, b: 2 }); + }); + + test('a save that keeps being declined throws rather than returning stranded bytes', function () { + let calls = 0; + const enc = new StandaloneStructon({ + structures: [], + saveStructures() { calls++; return false; }, + getStructures() { return undefined; }, + }); + + assert.throws( + () => enc.encode({ a: 1, b: 2 }), + /declined twice/, + 'a persistently declined save must surface, not return a reference to an unpersisted structure' + ); + assert.strictEqual(calls, 2, 'exactly one retry after the reload'); + }); + + test('a committed save is unaffected (no reload, no retry)', function () { + const store = casStore(); + const enc = new StandaloneStructon({ + structures: [], + saveStructures: store.saveStructures, + getStructures: store.getStructures, + }); + const buf = enc.encode({ a: 1, b: 2 }); + assert.strictEqual(store.declines, 0); + assert.strictEqual(store.saves, 1); + assert.deepStrictEqual(materialize(enc.decode(buf)), { a: 1, b: 2 }); + }); +});