From 9cebfadf6bfd0e3feba3a132f7629dc0042f104f Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 6 Jul 2026 08:54:43 -0600 Subject: [PATCH 1/2] fix(crdt): unify add fold, fix counter time-travel reconstruction, harden apply path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes to CRDT handling found during a correctness review, plus two hardenings surfaced by the cross-model review: - Unify the numeric `add` fold behind a single `addValues()` shared by the storage path (crdt.add via updateAndFreeze) and the read/serialize path (Addition.update). Previously they diverged: a numeric string concatenated on one and coerced on the other, and a bigint folded on one but threw `+bigint` on the other. - getRecordAtTime: reconstruct "unknown" fields (a newer plain set with no inverse) by forward-replaying from the nearest base (reconstructForward) instead of copying the nearest audit entry's raw field, which returned an unresolved `{__op__}` object (or a single delta) for a counter later overwritten by a plain set. Partially addresses the reverse-reconstruction family in #1413 (a)/(b); pruned-history and out-of-order-branch residuals remain there. Use Object.hasOwn so an unknown field named like a prototype member isn't filled from the prototype chain. Pruned-history "keep live value" behavior preserved. - updateAndFreeze: an unrecognized CRDT op (corruption / newer-node op type) now warns and skips on the write/replication apply path instead of throwing, so it can't abort a commit and wedge a subscription. Resolve ops against the explicit `operations` registry rather than the crdt.ts export namespace, so a crafted `__op__` naming another export (addValues, getRecordAtTime, …) can't be invoked with the wrong arguments. Read-path reconstruction still throws loudly by design. Co-Authored-By: Claude Opus 4.8 (1M context) --- resources/crdt.ts | 61 ++++++++++++------------ resources/tracked.ts | 19 ++++++-- unitTests/resources/crdt.test.js | 74 ++++++++++++++++++++++++++++- unitTests/resources/tracked.test.js | 51 ++++++++++++++++++++ 4 files changed, 170 insertions(+), 35 deletions(-) diff --git a/resources/crdt.ts b/resources/crdt.ts index dd934db8fe..f20f74e3ec 100644 --- a/resources/crdt.ts +++ b/resources/crdt.ts @@ -1,11 +1,15 @@ +// Single definition of the numeric `add` fold, shared by the storage-time merge (crdt.add, via +// updateAndFreeze) and the read/serialize path (tracked.ts Addition.update). One implementation +// keeps the two from diverging: a numeric string was concatenated on the storage path but coerced +// on the read path, and a bigint folded on the storage path but threw (`+bigint`) on the read path. +export function addValues(previousValue: any, value: any) { + if (typeof previousValue === 'bigint') return previousValue + BigInt(value); + const previous = +previousValue; // coerce so a numeric string adds instead of concatenating + if (isNaN(previous)) return value; // no prior numeric value: the add establishes it + return previous + value; +} export function add(record, property, action) { - const previousValue = record[property]; - if (typeof previousValue === 'bigint') { - record[property] = previousValue + BigInt(action.value); - } else if (isNaN(record[property])) record[property] = action.value; - else { - record[property] = previousValue + action.value; - } + record[property] = addValues(record[property], action.value); } add.reverse = function (record, property, action) { const previousValue = record[property]; @@ -15,7 +19,12 @@ add.reverse = function (record, property, action) { record[property] = previousValue - action.value; } }; -const operations = { +// The CRDT operation registry. Exported so the storage/apply path (tracked.ts updateAndFreeze) +// resolves ops against this explicit set rather than the module's export namespace — otherwise a +// crafted/corrupt `__op__` naming any exported function (addValues, getRecordAtTime, …) would +// resolve truthy and be invoked with the wrong arguments (throwing and wedging the apply path, or +// silently corrupting the field). applyReverse/applyForward below already resolve through this. +export const operations = { add, }; @@ -179,30 +188,22 @@ export function getRecordAtTime(currentEntry, timestamp, store, tableId: number, const boundaryEntry = auditStore.get(auditTime, tableId, recordId); if (boundaryEntry?.type === 'delete') return null; } - // some patches may leave properties in an unknown state, so we need to fill in the blanks - // first we determine if there any unknown properties - // then continue to iterate back through the audit history, filling in the blanks - while (unknowns.size > 0 && auditTime > 0) { - const auditEntry = auditStore.get(auditTime, tableId, recordId); - // The history needed to resolve the remaining unknowns may have been pruned away; stop - // rather than dereferencing a missing entry (mirrors the reverse-walk guard above). - if (!auditEntry) break; - let priorRecord: any; - switch (auditEntry.type) { - case 'put': - priorRecord = auditEntry.getValue(store); - break; - case 'patch': - priorRecord = auditEntry.getValue(store); - break; - } - for (const key in priorRecord) { - if (unknowns.has(key)) { - record[key] = priorRecord[key]; - unknowns.delete(key); + // A reversed patch that set a field to a plain value can't be undone (a plain set has no inverse), + // so those fields are left "unknown": their value at `timestamp` must come from older history. + // Reconstruct that state forward from the nearest base so CRDT ops accumulate correctly — e.g. a + // counter built up by `add` patches and later overwritten by a plain set. The prior code copied + // the nearest audit entry's raw field, which for a patch is an unresolved `{ __op__ }` object or a + // single delta rather than the folded value at `timestamp`. If the history needed to reconstruct a + // key is unavailable (pruned), the key keeps its live value (best effort, as before). + if (unknowns.size > 0 && auditTime > 0) { + const priorRecord = reconstructForward(auditStore, store, tableId, recordId, auditTime, timestamp); + if (priorRecord) { + for (const key of unknowns) { + // Object.hasOwn, not `in`: an unknown field named like a prototype member (toString, + // constructor, …) must not be filled from priorRecord's prototype chain. + if (Object.hasOwn(priorRecord, key)) record[key] = priorRecord[key]; } } - auditTime = auditEntry.previousVersion; } // finally return the record in the state it was at the requested timestamp return record; diff --git a/resources/tracked.ts b/resources/tracked.ts index 846a087d05..3da0dc7dbc 100644 --- a/resources/tracked.ts +++ b/resources/tracked.ts @@ -1,6 +1,7 @@ import { ClientError } from '../utility/errors/hdbError.ts'; import * as crdtOperations from './crdt.ts'; import { Blob } from './blob.ts'; +import * as harperLogger from '../utility/logging/harper_logger.ts'; function getChanges(target) { let changes = target.getChanges(); @@ -427,9 +428,18 @@ export function updateAndFreeze(target, changes = target.getChanges?.()) { let value = changes[key]; if (value && typeof value === 'object') { if (value.__op__) { - const operation = crdtOperations[value?.__op__]; - if (!operation) throw new Error('Invalid CRDT operation ' + value.__op__); - else operation(mergedUpdatedObject, key, value); + // Resolve against the explicit operation registry, not the crdt.ts export namespace, + // so a crafted/corrupt `__op__` naming another export can't be invoked as an operation. + const operation = crdtOperations.operations[value.__op__]; + if (operation) operation(mergedUpdatedObject, key, value); + else { + // An unrecognized CRDT operation — corruption, or an op type from a newer node this + // version can't apply. This runs on the write/replication apply path, where throwing + // would abort the commit and can wedge a subscription, so skip the op instead: the + // field keeps its base value and a full-copy re-converges the record. Read-path + // reconstruction (crdt.applyForward/applyReverse) still throws loudly by design. + harperLogger.warn(`Skipping unrecognized CRDT operation "${value.__op__}" on property "${key}"`); + } continue; } else value = updateAndFreeze(value); } @@ -588,6 +598,7 @@ export class Addition { this.value = value; } update(previousValue) { - return (+previousValue || 0) + this.value; + // Shared with crdt.add so the read path folds identically to the storage path. + return crdtOperations.addValues(previousValue, this.value); } } diff --git a/unitTests/resources/crdt.test.js b/unitTests/resources/crdt.test.js index 3d33b8a3d7..5fada6786a 100644 --- a/unitTests/resources/crdt.test.js +++ b/unitTests/resources/crdt.test.js @@ -1,5 +1,5 @@ const assert = require('assert'); -const { getRecordAtTime, applyForward } = require('#src/resources/crdt'); +const { getRecordAtTime, applyForward, addValues } = require('#src/resources/crdt'); // Build a mock audit history and the `store` shape getRecordAtTime expects. Each event is // { version, type: 'put'|'patch'|'delete', value, previousVersion }. The audit store resolves an @@ -172,6 +172,50 @@ describe('crdt getRecordAtTime', () => { assert.deepStrictEqual(getRecordAtTime(current, 10, store, 1, 'N'), { id: 'N', v: 1 }); }); }); + + it('reconstructs a counter later overwritten by a plain set without leaking the op object', () => { + // put(count:5) -> patch(+3) [count=8] -> patch(count:100, plain overwrite) -> current. + // The reverse walk marks `count` unknown at the plain set (a plain set has no inverse). Filling + // it must forward-replay the add to 8; the prior code copied the nearest audit entry's raw + // field and returned the op object { __op__: 'add', value: 3 } instead of the value. + const events = [ + { version: 10, type: 'put', value: { id: 'X', count: 5 }, previousVersion: 0 }, + { version: 20, type: 'patch', value: { count: { __op__: 'add', value: 3 } }, previousVersion: 10 }, + { version: 30, type: 'patch', value: { count: 100 }, previousVersion: 20 }, + ]; + const store = makeStore(events); + const current = currentEntry({ id: 'X', count: 100 }, 30); + assert.deepStrictEqual(getRecordAtTime(current, 25, store, 1, 'X'), { id: 'X', count: 8 }); + }); + + it('does not fill an unknown field from a prototype member (own-property check)', () => { + // put(v:1, no `toString`) -> patch(v:2) -> patch(toString:'b') -> current. `toString` is + // unknown at t=25 and is NOT an own key of the reconstructed record, so it must keep its live + // value rather than being assigned Object.prototype.toString (which `key in priorRecord` did). + const events = [ + { version: 10, type: 'put', value: { id: 'P', v: 1 }, previousVersion: 0 }, + { version: 20, type: 'patch', value: { v: 2 }, previousVersion: 10 }, + { version: 30, type: 'patch', value: { toString: 'b' }, previousVersion: 20 }, + ]; + const store = makeStore(events); + const current = currentEntry({ id: 'P', v: 2, toString: 'b' }, 30); + const result = getRecordAtTime(current, 25, store, 1, 'P'); + assert.strictEqual(result.toString, 'b'); + assert.strictEqual(typeof result.toString, 'string'); + }); + + it('fills an unknown field from an older plain put when reconstructing forward', () => { + // put(status:'a', v:1) -> patch(v:2) -> patch(status:'b') -> current. At t=25 status is 'a' + // (the plain set at 30 is unknown to the reverse walk and resolved from the put at 10). + const events = [ + { version: 10, type: 'put', value: { id: 'S', status: 'a', v: 1 }, previousVersion: 0 }, + { version: 20, type: 'patch', value: { v: 2 }, previousVersion: 10 }, + { version: 30, type: 'patch', value: { status: 'b' }, previousVersion: 20 }, + ]; + const store = makeStore(events); + const current = currentEntry({ id: 'S', status: 'b', v: 2 }, 30); + assert.deepStrictEqual(getRecordAtTime(current, 25, store, 1, 'S'), { id: 'S', status: 'a', v: 2 }); + }); }); describe('crdt applyForward', () => { @@ -185,3 +229,31 @@ describe('crdt applyForward', () => { assert.throws(() => applyForward({}, { x: { __op__: 'multiply', value: 2 } }), /Unsupported operation multiply/); }); }); + +describe('crdt addValues', () => { + // Single fold shared by the storage path (crdt.add) and the read/serialize path + // (tracked.ts Addition.update), so the two can no longer diverge. + it('adds two numbers', () => { + assert.strictEqual(addValues(5, 3), 8); + }); + + it('establishes the value when there is no prior numeric value', () => { + assert.strictEqual(addValues(undefined, 3), 3); + assert.strictEqual(addValues(NaN, 3), 3); + assert.strictEqual(addValues('not a number', 3), 3); + }); + + it('coerces a numeric-string prior value instead of concatenating', () => { + // Regression: crdt.add did "5" + 3 = "53" while the read path coerced to 8. + assert.strictEqual(addValues('5', 3), 8); + }); + + it('folds bigint prior values without throwing', () => { + // Regression: the read path (Addition.update) did +bigint and threw a TypeError. + assert.strictEqual(addValues(5n, 3), 8n); + }); + + it('treats a null prior value as zero', () => { + assert.strictEqual(addValues(null, 3), 3); + }); +}); diff --git a/unitTests/resources/tracked.test.js b/unitTests/resources/tracked.test.js index 137cabb907..5c26205de5 100644 --- a/unitTests/resources/tracked.test.js +++ b/unitTests/resources/tracked.test.js @@ -6,6 +6,7 @@ const { collapseData, GenericTrackedObject, } = require('#src/resources/tracked'); +const harperLogger = require('#src/utility/logging/harper_logger'); describe('Tracked Object', () => { let source = { str: 'string', @@ -89,3 +90,53 @@ describe('Tracked Object', () => { assert.equal(collapseData(instance).arrayOfStrings[2], 'another string'); }); }); + +describe('updateAndFreeze CRDT operations', () => { + it('applies a recognized add operation', () => { + const result = updateAndFreeze({ count: 5 }, { count: { __op__: 'add', value: 3 } }); + assert.strictEqual(result.count, 8); + }); + + it('skips an unrecognized operation instead of throwing (apply path must not wedge)', () => { + // On the write/replication apply path a throw would abort the commit and can wedge a + // subscription, so an op this version can't apply is warned + skipped; the field keeps its + // base value and a full-copy re-converges the record. + const original = harperLogger.warn; + let warned = ''; + harperLogger.warn = (message) => { + warned = message; + }; + try { + let result; + assert.doesNotThrow(() => { + result = updateAndFreeze({ count: 5 }, { count: { __op__: 'multiply', value: 3 } }); + }); + assert.strictEqual(result.count, 5); // unchanged base value + assert.match(warned, /unrecognized CRDT operation "multiply"/); + } finally { + harperLogger.warn = original; + } + }); + + it('does not invoke a non-operation crdt export named by a crafted __op__', () => { + // Ops resolve against the explicit registry, not the crdt.ts export namespace. Before that, + // `__op__: 'getRecordAtTime'` resolved to the exported function and was invoked with the + // wrong arguments — throwing and wedging the apply path. It must now warn + skip like any + // other unrecognized op. + const original = harperLogger.warn; + let warned = ''; + harperLogger.warn = (message) => { + warned = message; + }; + try { + let result; + assert.doesNotThrow(() => { + result = updateAndFreeze({ count: 5 }, { count: { __op__: 'getRecordAtTime', value: 3 } }); + }); + assert.strictEqual(result.count, 5); + assert.match(warned, /unrecognized CRDT operation "getRecordAtTime"/); + } finally { + harperLogger.warn = original; + } + }); +}); From 2e6924196ab6c58d643c7b5b21000cd0f76212ed Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 6 Jul 2026 09:25:55 -0600 Subject: [PATCH 2/2] fix(crdt): null-prototype op registry + robust addValues mixed-type folding Address cross-model review comments on #1615: - operations registry is now a null-prototype object, so a crafted `__op__` naming an Object.prototype member (toString, valueOf, constructor) resolves to undefined rather than an inherited function in applyForward/applyReverse and updateAndFreeze. - addValues: explicitly establish from a null/undefined prior (avoids `0 + 3n` TypeError), and keep a number-typed field a number when a stray bigint delta arrives instead of throwing a number+bigint TypeError on the apply path. Co-Authored-By: Claude Opus 4.8 (1M context) --- resources/crdt.ts | 13 +++++++++---- unitTests/resources/crdt.test.js | 17 ++++++++++++++++- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/resources/crdt.ts b/resources/crdt.ts index f20f74e3ec..0df776b35e 100644 --- a/resources/crdt.ts +++ b/resources/crdt.ts @@ -4,9 +4,12 @@ // on the read path, and a bigint folded on the storage path but threw (`+bigint`) on the read path. export function addValues(previousValue: any, value: any) { if (typeof previousValue === 'bigint') return previousValue + BigInt(value); + if (previousValue == null) return value; // no prior value: the add establishes it const previous = +previousValue; // coerce so a numeric string adds instead of concatenating - if (isNaN(previous)) return value; // no prior numeric value: the add establishes it - return previous + value; + if (isNaN(previous)) return value; // non-numeric prior: the add establishes it + // A number-typed field stays a number even if a stray bigint delta arrives, rather than throwing + // a number+bigint TypeError on the apply path. + return previous + (typeof value === 'bigint' ? Number(value) : value); } export function add(record, property, action) { record[property] = addValues(record[property], action.value); @@ -24,9 +27,11 @@ add.reverse = function (record, property, action) { // crafted/corrupt `__op__` naming any exported function (addValues, getRecordAtTime, …) would // resolve truthy and be invoked with the wrong arguments (throwing and wedging the apply path, or // silently corrupting the field). applyReverse/applyForward below already resolve through this. -export const operations = { +// Null-prototype so a crafted `__op__` naming an Object.prototype member (toString, valueOf, +// constructor, …) resolves to undefined rather than an inherited function. +export const operations = Object.assign(Object.create(null), { add, -}; +}); /** * Rebuild a record update that has a timestamp before the provided newer update diff --git a/unitTests/resources/crdt.test.js b/unitTests/resources/crdt.test.js index 5fada6786a..5067c368ad 100644 --- a/unitTests/resources/crdt.test.js +++ b/unitTests/resources/crdt.test.js @@ -228,6 +228,13 @@ describe('crdt applyForward', () => { it('throws on an unsupported operation', () => { assert.throws(() => applyForward({}, { x: { __op__: 'multiply', value: 2 } }), /Unsupported operation multiply/); }); + + it('does not resolve an Object.prototype member as an operation', () => { + // The registry is null-prototype, so `__op__: 'toString'` (a prototype member on a plain + // object) resolves to undefined and is rejected like any other unsupported op, rather than + // invoking Object.prototype.toString. + assert.throws(() => applyForward({}, { x: { __op__: 'toString', value: 2 } }), /Unsupported operation toString/); + }); }); describe('crdt addValues', () => { @@ -253,7 +260,15 @@ describe('crdt addValues', () => { assert.strictEqual(addValues(5n, 3), 8n); }); - it('treats a null prior value as zero', () => { + it('establishes the value from a null/undefined prior, including a bigint delta', () => { assert.strictEqual(addValues(null, 3), 3); + assert.strictEqual(addValues(undefined, 3), 3); + assert.strictEqual(addValues(null, 3n), 3n); // would throw `0 + 3n` without the null guard + }); + + it('keeps a number field a number when a stray bigint delta arrives', () => { + // number + bigint would otherwise throw a TypeError on the apply path. + assert.strictEqual(addValues(5, 3n), 8); + assert.strictEqual(typeof addValues(5, 3n), 'number'); }); });