Record history no longer shows a field before the patch that first wrote it

resources/DESIGN.md

@@ -197,6 +197,7 @@ A RocksDB transaction's log key is minted when its native handle is created, or
 
 ## Historical record reconstruction (`getRecordAtTime`)
 
+- **A field a newer patch wrote keeps its reversed value only when the reversal is exact.** A plain set has no inverse; an `add` that established its field from absent or `null` reverses to `0`, and float or unsafe-integer arithmetic can leave a rounding residue. So only an `add`-only field reversed in safe-integer arithmetic to a nonzero result keeps it (a coerced numeric string or boolean comes back as its number), which spares a counter a walk to its base. Every other written field takes its value from the forward rebuild at the timestamp, and a field that rebuild lacks did not exist then. Over incomplete history (a pruned link, a corrupt entry, an entry with no body, an `invalidate`, whose stub lacks the fields it did not index or project, or an out-of-order write merged on an `additionalAuditRefs` branch the single-link walks do not follow) a field the rebuild cannot settle keeps its reversed value. A newer patch with no body, or a newer `invalidate` (whose stub replaced the record), cannot be reversed; it is crossed like a put.
 - **Only a patch can be reversed.** The backward walk from the live record undoes patches; a put or delete newer than the requested timestamp replaced the whole record, so nothing recorded about it recovers what came before. After crossing one the walk stops reversing, keeps following links to the entry at or before the timestamp, and rebuilds forward from there (`reconstructForward`). Carrying a newer put's image back across it returns a future state, or throws when a later patch fills unknown fields into the frozen decoded put (harper#1413).
 - **An unlinked put ends the walk early, and that is not an empty past.** A put written with no predecessor link (a re-insert after eviction) cuts the chain although older entries are retained. The walk then falls to the timestamp-keyed rebuild, as it does for any broken chain. Rebuilding from a crossed put's own predecessor link would report an empty past for every state older than such a re-insert.
 

resources/crdt.ts

@@ -19,7 +19,8 @@ add.reverse = function (record, property, action) {
 	if (typeof previousValue === 'bigint') {
 		record[property] = previousValue - BigInt(action.value);
 	} else if (!isNaN(record[property])) {
-		record[property] = previousValue - action.value;
+		// addValues keeps a number field a number under a bigint delta
+		record[property] = previousValue - Number(action.value);
 	}
 };
 // The CRDT operation registry. Exported so the storage/apply path (tracked.ts updateAndFreeze)
@@ -130,19 +131,38 @@ function holdsField(record: any, key: string) {
 	return false;
 }
 
-export function applyReverse(record, update, unknowns: Set<string>) {
+// A plain set has no inverse, so its key is left to older history (`unknowns`), and an older add
+// under it is not reversed: subtracting from the set's value would invent one. An add reverses
+// exactly in safe-integer arithmetic, except one that established its field from absent or null,
+// which reverses to 0; such keys wait in `reversedAdds` for getRecordAtTime to keep only a nonzero
+// result (a coerced numeric string or boolean comes back as its number).
+export function applyReverse(
+	record,
+	update,
+	unknowns: Set<string>,
+	reversedAdds: Set<string>,
+	overwritten: Set<string>
+) {
 	for (const key in update) {
 		const value = update[key];
 		if (value?.__op__) {
+			if (overwritten.has(key)) continue;
 			const reverse = operations[value.__op__]?.reverse;
-			if (reverse) reverse(record, key, { value: value.value });
-			else throw new Error(`Unsupported operation ${value.__op__}`);
+			if (!reverse) throw new Error(`Unsupported operation ${value.__op__}`);
+			const exact = value.__op__ === 'add' && isExactInteger(record[key]) && isExactInteger(value.value);
+			reverse(record, key, { value: value.value });
+			(exact ? reversedAdds : unknowns).add(key);
 		} else {
 			unknowns.add(key);
+			overwritten.add(key);
 		}
 	}
 }
 
+function isExactInteger(value: unknown) {
+	return typeof value === 'bigint' || Number.isSafeInteger(value);
+}
+
 // Apply an audit update onto a record moving forward in time (the inverse of applyReverse). When
 // `skipUnseededOps` is true (a partial replay whose base was pruned), CRDT operations on keys not
 // yet present on `record` are skipped rather than folded onto undefined.
@@ -170,9 +190,9 @@ export function applyForward(record, update, skipUnseededOps = false) {
  * Entries newer than `timestamp` are skipped (the cutoff may fall between entries). Returns `null`
  * if the record did not exist at `timestamp` (the nearest in-range history boundary is a delete
  * with no surviving writes, or there is no in-range history). When a prior link leads to a pruned
- * entry, returns `undefined` if `allowPartial` is false (history is incomplete for a whole-record
- * caller) or replays the surviving plain-set writes when `allowPartial` is true (for `unknowns`
- * fill on a live record).
+ * entry, returns `undefined` without `partial` (history is incomplete for a whole-record caller);
+ * with `partial` (the `unknowns` fill on a live record) it replays the surviving writes and sets
+ * `partial.truncated` when the history it read was incomplete.
  */
 function reconstructForward(
 	auditStore,
@@ -181,25 +201,30 @@ function reconstructForward(
 	recordId: any,
 	fromPosition,
 	timestamp,
-	allowPartial = false
+	partial?: { truncated: boolean }
 ) {
 	// Collect the in-range entries (at or before `timestamp`) back to a base boundary, newest-first.
 	// The boundary is a full `put` (snapshot) or a `delete` (everything older is erased); a record
 	// whose first write was a `patch` has no put and bottoms out at the start of history. Only
-	// `put`/`patch` contribute to the value, matching the reverse walk's switch (other partial types
-	// such as `invalidate` are ignored).
+	// `put`/`patch` contribute to the value; other partial types such as `invalidate` are ignored by a
+	// whole-record caller and end a partial one's history.
 	const entries = [];
 	let truncated = false;
 	let { txnLogKey: auditTime, nodeId: auditNodeId } = fromPosition;
 	while (auditTime > 0) {
 		const auditEntry = auditStore.get(auditTime, tableId, recordId, auditNodeId);
 		if (!auditEntry) {
-			if (allowPartial) {
+			if (partial) {
 				truncated = true;
 				break;
 			}
 			return entries.length === 0 ? null : undefined;
 		}
+		// a corrupt entry reads with no type and no previous link, which must not pass for the start of history
+		if (partial && auditEntry.type === undefined) {
+			truncated = true;
+			break;
+		}
 		if (auditEntry.type === 'delete') {
 			// A delete at or before `timestamp` bounds the history; the record is rebuilt from the
 			// writes collected after it (an empty base). A delete newer than `timestamp` (a later
@@ -211,19 +236,34 @@ function reconstructForward(
 				break; // full snapshot reached; nothing older is needed
 			} else if (auditEntry.type === 'patch') {
 				entries.push(auditEntry);
+			} else if (partial && auditEntry.type === 'invalidate') {
+				// an invalidate replaced the record with a stub (projection and indexed fields): older writes
+				// no longer describe it, and a field the stub lacks is unknown, not absent
+				partial.truncated = truncated = true;
+				break;
 			}
 		}
 		const previousPosition = previousAuditPosition(auditStore, tableId, recordId, auditEntry);
+		if (partial && mergesBranchBy(auditEntry.previousAdditionalAuditRefs, previousPosition, timestamp))
+			partial.truncated = true;
 		auditTime = previousPosition.txnLogKey;
 		auditNodeId = previousPosition.nodeId;
 	}
+	if (partial && truncated) partial.truncated = true;
 	if (entries.length === 0) return null; // record did not exist at `timestamp`
 	// Replay oldest-first. The base is the put if the chain reached one; otherwise an empty record
 	// (the first in-range write was a patch, or a delete bounds the history) onto which patches apply.
 	let record = null;
 	for (let i = entries.length - 1; i >= 0; i--) {
 		const auditEntry = entries[i];
 		const value = auditEntry.getValue(store);
+		if (value == null && partial) {
+			// an entry with no readable body (minted without one, or undecodable) hides what it wrote, so
+			// nothing before it can settle a key either: restart as after a pruned link
+			partial.truncated = truncated = true;
+			record = {};
+			continue;
+		}
 		// Copy the base put: applyForward mutates `record`, and getValue may return a cached
 		// object shared with the audit entry.
 		if (auditEntry.type === 'put') record = { ...value };
@@ -256,6 +296,15 @@ function resolveAuditPosition(auditStore, tableId: number, recordId: any, versio
 	return { txnLogKey: version, nodeId };
 }
 
+// An out-of-order write merged into the record sits on a branch the walks never visit (they follow
+// one predecessor), so history that crosses one merged by `timestamp` cannot prove a key absent.
+function mergesBranchBy(refs, followed, timestamp) {
+	return refs?.some(
+		(ref) =>
+			ref.version <= timestamp && (ref.version !== followed.txnLogKey || (ref.nodeId ?? 0) !== (followed.nodeId ?? 0))
+	);
+}
+
 function previousAuditPosition(auditStore, tableId: number, recordId: any, auditEntry) {
 	return resolveAuditPosition(
 		auditStore,
@@ -295,22 +344,32 @@ export function getRecordAtTime(currentEntry, timestamp, store, tableId: number,
 		: { txnLogKey: currentEntry.localTime ?? currentEntry.version, nodeId: currentEntry.nodeId };
 	let auditTime = initialPosition.txnLogKey;
 	let auditNodeId = initialPosition.nodeId;
+	let branched = mergesBranchBy(currentEntry.additionalAuditRefs, initialPosition, timestamp);
 	// Iterate in reverse through the record history, trying to reverse all changes
 	const unknowns = new Set<string>();
+	const reversedAdds = new Set<string>();
+	const overwritten = new Set<string>();
 	let replaced = false;
 	while (auditTime > timestamp) {
 		const auditEntry = auditStore.get(auditTime, tableId, recordId, auditNodeId);
 		if (!auditEntry) break;
 		switch (auditEntry.type) {
 			case 'put':
 			case 'delete':
+			case 'invalidate': // the newer stub lacks unindexed fields, so it cannot be reversed from
 				replaced = true;
 				break;
 			case 'patch':
-				if (!replaced) applyReverse(record, auditEntry.getValue(store), unknowns);
+				if (!replaced) {
+					const update = auditEntry.getValue(store);
+					// a patch with no readable body cannot be reversed, but the state before it does not need it
+					if (update == null) replaced = true;
+					else applyReverse(record, update, unknowns, reversedAdds, overwritten);
+				}
 				break;
 		}
 		const previousPosition = previousAuditPosition(auditStore, tableId, recordId, auditEntry);
+		branched ||= mergesBranchBy(auditEntry.previousAdditionalAuditRefs, previousPosition, timestamp);
 		auditTime = previousPosition.txnLogKey;
 		auditNodeId = previousPosition.nodeId;
 	}
@@ -334,29 +393,28 @@ export function getRecordAtTime(currentEntry, timestamp, store, tableId: number,
 		const boundaryEntry = auditStore.get(auditTime, tableId, recordId, auditNodeId);
 		if (boundaryEntry?.type === 'delete') return null;
 	}
-	// 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).
+	for (const key of reversedAdds) {
+		const value = record[key];
+		if (!isExactInteger(value) || value === 0 || value === 0n) unknowns.add(key);
+	}
 	if (unknowns.size > 0 && auditTime > 0) {
-		const priorRecord = reconstructForward(
-			auditStore,
-			store,
-			tableId,
-			recordId,
-			{ txnLogKey: auditTime, nodeId: auditNodeId },
-			timestamp,
-			true
-		);
-		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];
-			}
+		const partial = { truncated: branched };
+		// null: no put or patch survives at or before `timestamp` (e.g. only a message after a delete)
+		const priorRecord =
+			reconstructForward(
+				auditStore,
+				store,
+				tableId,
+				recordId,
+				{ txnLogKey: auditTime, nodeId: auditNodeId },
+				timestamp,
+				partial
+			) ?? {};
+		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];
+			else if (!partial.truncated) delete record[key]; // complete history: it did not exist then
 		}
 	}
 	// finally return the record in the state it was at the requested timestamp

unitTests/resources/auditLog.test.js

@@ -1,6 +1,7 @@
 const assert = require('assert');
 const { setupTestDBPath } = require('../testUtils');
-const { table } = require('#src/resources/databases');
+const { table, tables } = require('#src/resources/databases');
+const { loadGQLSchema } = require('#src/resources/graphql');
 const { open } = require('lmdb');
 const {
 	setAuditRetention,
@@ -1007,6 +1008,116 @@ describe('Audit log', () => {
 			['v1', 'v2', 'v3', 'v4', 'v5']
 		);
 	});
+	describe('history before a patch that first wrote a field', () => {
+		const add = (value) => ({ __op__: 'add', value });
+		const fieldsAt = (value) => ({
+			name: value.name,
+			hasExtra: Object.hasOwn(value, 'extra'),
+			hasCount: Object.hasOwn(value, 'count'),
+			count: value.count,
+			total: value.total,
+		});
+
+		it('leaves out a later plain field and a later add-established counter', async () => {
+			const id = 'absent-before-later-patch';
+			await AuditedTable.put(id, { name: 'v1' });
+			await AuditedTable.patch(id, { name: 'v2' });
+			await AuditedTable.patch(id, { extra: 'later', count: add(5) });
+			const history = await AuditedTable.getHistoryOfRecord(id);
+			assert.deepStrictEqual(
+				history.map((entry) => entry.type),
+				['put', 'patch', 'patch']
+			);
+			assert.deepStrictEqual(fieldsAt(history[1].value), {
+				name: 'v2',
+				hasExtra: false,
+				hasCount: false,
+				count: undefined,
+				total: undefined,
+			});
+			assert.strictEqual(history[2].value.extra, 'later');
+			assert.strictEqual(history[2].value.count, 5);
+			const byHash = await new ResourceBridge().readAuditLog({
+				schema: AuditedTable.databaseName,
+				table: 'AuditedTable',
+				search_type: 'hash_value',
+				search_values: [id],
+			});
+			assert.strictEqual(fieldsAt(byHash[id][1].records[0]).hasExtra, false);
+			assert.strictEqual(fieldsAt(byHash[id][1].records[0]).hasCount, false);
+			const tableWide = [];
+			for await (const entry of AuditedTable.getHistory()) if (entry.id === id) tableWide.push(entry);
+			assert.deepStrictEqual(fieldsAt(tableWide[1].value), fieldsAt(history[1].value));
+			// a later put switches this time to the forward rebuild; the snapshot must not change with it
+			await AuditedTable.put(id, { name: 'v3' });
+			const afterPut = await AuditedTable.getHistoryOfRecord(id);
+			assert.deepStrictEqual(fieldsAt(afterPut[1].value), fieldsAt(history[1].value));
+			assert.strictEqual(afterPut[1].value.id, history[1].value.id);
+		});
+
+		it('keeps the fields a random-access struct record held then', async () => {
+			await loadGQLSchema(`type AuditedStructHistory @table(randomAccessFields: true, audit: true) {
+				id: ID @primaryKey
+				name: String
+				count: Int
+				extra: String
+			}`);
+			const StructTable = tables.AuditedStructHistory;
+			assert.strictEqual(StructTable.primaryStore.encoder.randomAccessStructure, true);
+			await StructTable.put('s', { name: 'v1', count: 1 });
+			await StructTable.patch('s', { name: 'v2' });
+			await StructTable.patch('s', { extra: 'later', count: 7 });
+			const history = await StructTable.getHistoryOfRecord('s');
+			assert.deepStrictEqual({ ...history[1].value }, { id: 's', name: 'v2', count: 1 });
+		});
+
+		it('restores the null a later add established its counter from', async () => {
+			const id = 'null-before-later-add';
+			await AuditedTable.put(id, { name: 'v1', count: null });
+			await AuditedTable.patch(id, { name: 'v2' });
+			await AuditedTable.patch(id, { count: add(5) });
+			const history = await AuditedTable.getHistoryOfRecord(id);
+			assert.strictEqual(history[1].value.name, 'v2');
+			assert.strictEqual(history[1].value.count, null);
+			assert.strictEqual(history[2].value.count, 5);
+		});
+
+		it('keeps the best-effort value of a field the history cannot settle once the base put is pruned', async () => {
+			const id = 'pruned-before-later-add';
+			await AuditedTable.put(id, { name: 'v1', total: 10, count: 3 });
+			await AuditedTable.patch(id, { name: 'v2', count: null });
+			await AuditedTable.patch(id, { total: add(2.5), count: add(4) });
+			const auditStore = AuditedTable.primaryStore.rootStore.auditStore;
+			let baseKey;
+			for (const entry of auditStore.getRange({ start: 1 })) {
+				if (entry.tableId === AuditedTable.tableId && entry.recordId === id && entry.type === 'put')
+					baseKey = entry.txnLogKey;
+			}
+			assert(baseKey, 'the base put is in the log');
+			// stand in for audit retention having removed the base put
+			const get = auditStore.get;
+			auditStore.get = function (key, tableId, recordId, nodeId) {
+				return key === baseKey ? undefined : get.call(this, key, tableId, recordId, nodeId);
+			};
+			let history;
+			try {
+				history = await AuditedTable.getHistoryOfRecord(id);
+			} finally {
+				auditStore.get = get;
+			}
+			assert.deepStrictEqual(
+				history.map((entry) => entry.type),
+				['put', 'patch', 'patch']
+			);
+			assert.deepStrictEqual(fieldsAt(history[1].value), {
+				name: 'v2',
+				hasExtra: false,
+				hasCount: true,
+				count: null,
+				total: 10,
+			});
+		});
+	});
 	// LMDB assigns each record's own localTime via lmdb-js's native monotonic clock, independent of
 	// the application-supplied `version` (e.g. a replicated/out-of-order write's origin timestamp,
 	// which can be backdated relative to this node's clock). previousVersion must point at the prior