fix(crdt): unify add fold, fix counter time-travel reconstruction, harden apply path

resources/crdt.ts

@@ -1,11 +1,18 @@
+// 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);
+	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; // 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) {
-	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,9 +22,16 @@ 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.
+// 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
@@ -179,30 +193,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;

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();
@@ -423,9 +424,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);
 		}
@@ -584,6 +594,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);
 	}
 }

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', () => {
@@ -184,4 +228,47 @@ 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', () => {
+	// 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('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');
+	});
 });

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;
+		}
+	});
+});