Skip to content

fix: retry a declined typed-structure save instead of returning stranded bytes #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kriszyp wants to merge 1 commit into
base: main
Choose a base branch
from
+139 −19
Open

fix: retry a declined typed-structure save instead of returning stranded bytes #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 53 additions & 19 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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 || [],
Expand Down
86 changes: 86 additions & 0 deletions tests/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
});
});