fix(operations): refuse __unset__ whenever there is no record to remove from

Delta review of the previous three commits found the insert refusal guarding the
wrong condition. It keyed on `requires_no_existing` — the insert flag — so the
same silent partial write was still reachable one branch over:
`insertUpdateValidate` requires a primary key only for `update`, so
`{operation:'upsert', records:[{name:'Penny', color:'black', __unset__:['color']}]}`
took the `id == undefined → Table.create` path and stored an auto-keyed record with
`color` stripped, returning 200. An `upsert` naming a primary key that isn't stored
had the same shape.

Now keyed on `!existingRecord`, which is the actual invariant: no stored record
means nothing to remove from, so the directive can only be a misunderstanding. That
covers insert, upsert-without-a-key, and upsert-of-an-absent-key together. `update`
never reaches it — a missing record is already skipped upstream.

This is the fix-one-of-N-surfaces mistake: the condition was written from the
operation that exhibited the bug rather than from the property being protected.

Also from the review:

- Managed timestamps are refused off `Table.createdTimeProperty`/
  `updatedTimeProperty` instead of the validator's static legacy list, so a
  schema-declared `createdAt: Float @createdTime` is caught too. Those properties
  resolve `assignCreatedTime` OR the legacy `__createdtime__` spelling
  (resources/Table.ts:504-505), so one check covers both and there is no second
  list to drift. The validator's list is removed rather than kept alongside it.
- The malformed-directive warning is latched and names the table, following
  `warnedNullSourcePut` in resources/Table.ts — it sits on the write and
  replication apply path, where one line per record buries the log under a single
  bad batch.
- Comment density trimmed. Both reviews flagged it; the removed lines narrated
  review history or addressed the reviewer rather than the next reader. Kept: why
  the malformed case warns instead of throwing, why the refusals live in this layer,
  and the falsy-vs-nullish reasoning.

Tests: 33 in the suite, all passing, and the new guards are mutation-checked —
reverting to the insert flag fails the two new upsert probes, and removing the
managed-timestamp loop fails both timestamp probes. 118 more across security,
terminology and auto-fields still pass.

One test correction worth noting: the first version of the upsert probe asserted
"no record with this name and null breed/color", which matched eight records other
probes had legitimately stripped attributes from. It now uses a name unique to the
probe.

Still open from the review, unfixed: `__unset__` remains a whole-record put under
contention (#2350), the directive refusals happen after dynamic attribute
expansion so a rejected write can still have registered new attributes on an open
table, and the bulk-load authorization fix has no test because the JSON bulk path
needs S3/URL input. The CSV entry point takes a different attribute list
(bulkLoad.ts:82, papaparse header fields) which this change does not touch, and
CSV cannot express the array anyway.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: dawsontoth

dataLayer/harperBridge/ResourceBridge.ts

@@ -39,6 +39,8 @@ const DELETE_PAUSE_MS = 10;
 /** Shared result for the overwhelmingly common case of a record carrying no `__unset__`, so the
  * default write path allocates nothing per record. Frozen because it is handed out repeatedly. */
 const NO_UNSET_ATTRIBUTES = Object.freeze([]) as unknown as string[];
+/** Latch for the malformed-directive warning; see takeUnsetAttributes. */
+let warnedMalformedUnset = false;
 
 export type SearchByConditionsRequest = Query &
 	Context & {
@@ -280,14 +282,28 @@ export class ResourceBridge extends BridgeMethods {
 						}
 					}
 				}
-				const unset = takeUnsetAttributes(record, Table.primaryKey, upsertObj.requires_no_existing);
+				// Keyed on whether there IS a record to remove from, not on which operation asked. Guarding
+				// the insert flag alone left `upsert` with no primary key reaching the same silent partial
+				// write one branch over: `insertUpdateValidate` requires a hash attribute only for
+				// `update`, so such a record takes the `id == undefined → Table.create` path and stored an
+				// auto-keyed record with the named attributes stripped, returning 200.
+				const unset = takeUnsetAttributes(record, Table, !existingRecord);
 				// `__unset__` removes named attributes while everything else still merges, which a patch
 				// cannot express — so it resolves the merge here, against the record this transaction
 				// already loaded, and writes the result as a full replace. Doing it server-side inside the
 				// write transaction is the point: a client emulating this with read-then-replace races
 				// every other writer, and `full_record` would make the caller resend attributes it never
 				// meant to touch. Under `full_record` the submitted record is already the whole intended
 				// state, so there is nothing to merge and the names are simply dropped from it.
+				//
+				// SINGLE-WRITER ONLY, and the surrounding wording should not be read as more than that.
+				// Resolving the merge here still writes a `put`, and a full put is last-writer-wins over
+				// the whole record: out-of-order reconciliation folds field-wise only for patches
+				// (`resources/Table.ts:2902-2914`) and the commit-retry path re-reads the stored record
+				// only when `!fullUpdate` (`resources/Table.ts:2530`). So a concurrent write to an
+				// attribute this request never named can be lost — the opposite of what "everything else
+				// merges" implies. Tracked in HarperFast/harper#2350; fixing it properly means resolving
+				// removals at the patch layer instead.
 				const toWrite = unset.length && existingRecord && !fullRecord ? { ...existingRecord, ...record } : record;
 				for (const attribute of unset) {
 					delete toWrite[attribute];
@@ -808,53 +824,53 @@ async function* groupRecordsInHistory(table, start?, end?, limit?) {
 /**
  * Read and remove a record's `__unset__` list, returning the attribute names to drop.
  *
- * Removed from the record because it is a directive, not data: leaving it in place would store it as
- * an attribute. Shape is already validated (`validation/insertValidator.ts`); the primary key is
- * refused here instead, since only this layer knows which attribute that is. Unsetting it would
- * otherwise be silently ignored — `Table._writeUpdate` re-asserts the primary key on a full update —
- * and a directive that quietly does nothing is worse than one that says no.
+ * The key is removed because it is a directive, not data — left in place it would be stored as an
+ * attribute. The refusals below live here rather than in the validator because only this layer knows
+ * the table: which attribute is the primary key, and what the managed timestamps are called. Each is
+ * refused rather than skipped, because `_writeUpdate` re-asserts all three on a full update, so
+ * accepting them would return 200 and change nothing.
  */
-function takeUnsetAttributes(
-	record: Record<string, unknown>,
-	primaryKey: string,
-	isCreateOnly: boolean | undefined
-): string[] {
+function takeUnsetAttributes(record: Record<string, unknown>, Table: any, hasNoExistingRecord: boolean): string[] {
 	if (!(OPERATIONS_UNSET_KEY in record)) return NO_UNSET_ATTRIBUTES;
 	const unset = record[OPERATIONS_UNSET_KEY];
-	// Removed before the shape is judged, and on every exit below: the key is a directive, so
-	// leaving it on a record that is about to be written stores it as data. `in` rather than an
-	// `undefined` check for the same reason — an explicitly-undefined key still has to come off.
+	// Before the shape is judged, and on every exit: `in` rather than a value check, so an
+	// explicitly-undefined key still comes off the record.
 	delete record[OPERATIONS_UNSET_KEY];
-	// `insert` has no existing record to remove anything from, so the directive can only mean the
-	// caller misunderstood it. Refused rather than applied: applying it deleted attributes the same
-	// request had just supplied, which is a silent partial write, and rather than ignored, because a
-	// directive that quietly does nothing is worse than one that says no — the same reason the primary
-	// key below is refused instead of skipped. Authorization contributes nothing for `insert` on the
-	// strength of this (`utility/operation_authorization.ts` getRecordAttributes).
-	if (isCreateOnly) {
+	if (hasNoExistingRecord) {
 		throw new ClientError(
-			`'${OPERATIONS_UNSET_KEY}' is not valid on an insert, which has no existing record to remove attributes from`
+			`'${OPERATIONS_UNSET_KEY}' needs an existing record to remove attributes from; nothing is stored under this primary key`
 		);
 	}
-	// Self-safeguarding rather than trusting the caller. `validation/insertValidator.ts` rejects a
-	// malformed value, and every current route here goes through it (`dataLayer/insert.ts`
-	// create/update/upsertRecords, including the replication catchup path) — but the bridge's own
-	// `insertUpdateValidate` does not check this key, so that safety lives in a different layer, and
-	// this is a module boundary an internal caller could reach directly. A non-iterable reaching the
-	// `for…of` in `upsertRecords` would throw and take the write down; a bare string would iterate
-	// per character and delete single-letter attributes.
-	//
 	// Ignored with a warning rather than thrown: this is the write and replication apply path, where
-	// throwing aborts the commit and can wedge a subscription. Skipping the directive degrades to a
-	// plain merge, which removes nothing — the safe direction. Same choice `resources/tracked.ts`
-	// makes for an unrecognized CRDT operation, and the reason it makes it.
+	// throwing aborts the commit and can wedge a subscription, and skipping the directive degrades to
+	// a plain merge, which removes nothing. `resources/tracked.ts` makes the same choice for an
+	// unrecognized CRDT operation. Checked at all because the shape is validated one layer up, in a
+	// module this one does not own: a non-iterable would throw in the caller's `for…of` and take the
+	// write down, and a bare string would iterate per character, deleting single-letter attributes.
 	if (!Array.isArray(unset) || unset.some((name) => typeof name !== 'string' || name.length === 0)) {
-		logger.warn(`Ignoring a malformed '${OPERATIONS_UNSET_KEY}' on a record; expected an array of attribute names`);
+		// Latched, following `warnedNullSourcePut` in resources/Table.ts: one line per record would
+		// bury the log under a single bad batch.
+		if (!warnedMalformedUnset) {
+			warnedMalformedUnset = true;
+			logger.warn(
+				`Ignoring a malformed '${OPERATIONS_UNSET_KEY}' on a record in ${Table.databaseName}.${Table.tableName}; expected an array of attribute names`
+			);
+		}
 		return NO_UNSET_ATTRIBUTES;
 	}
 	const names = unset as string[];
+	const primaryKey = Table.primaryKey;
 	if (primaryKey && names.includes(primaryKey)) {
 		throw new ClientError(`'${primaryKey}' is the primary key of this table and cannot be unset`);
 	}
+	// `createdTimeProperty`/`updatedTimeProperty` resolve `assignCreatedTime` OR the legacy
+	// `__createdtime__` spelling (resources/Table.ts), so this covers a schema-declared
+	// `createdAt: Float @createdTime` as well as the legacy names — which a static list in the
+	// validator, with no schema in scope, could not.
+	for (const managed of [Table.createdTimeProperty, Table.updatedTimeProperty]) {
+		if (managed?.name && names.includes(managed.name)) {
+			throw new ClientError(`'${managed.name}' is maintained by Harper and cannot be unset`);
+		}
+	}
 	return names;
 }

integrationTests/database/full-record-write.test.ts

@@ -530,15 +530,54 @@ suite('full_record: full replace over the operations API', { skip: skipSuite },
 		strictEqual((await read('Dog', 'us-6')).name, 'Penny', 'nothing should have changed');
 	});
 
-	test('__unset__ refuses a system timestamp', async () => {
+	// Both spellings, because they are refused by one check rather than two. `Table.createdTimeProperty`
+	// resolves `assignCreatedTime` OR the legacy `__createdtime__` name (`resources/Table.ts:504-505`),
+	// so a schema-declared `createdAt: Float @createdTime` — which this fixture has, and which a static
+	// name list in the validator could never catch — is refused by the same code as the legacy name.
+	// Without this the write path silently re-asserts the value: a 200 that changes nothing.
+	test('__unset__ refuses a schema-declared managed timestamp', async () => {
 		await insert('Dog', [{ id: 'us-7', name: 'Penny' }]);
+		const before = await read('Dog', 'us-7');
+
+		// This fixture declares `createdAt: Float @createdTime` / `updatedAt: Float @updatedTime`, so
+		// these are the table's managed timestamps and a static name list in the validator could never
+		// have caught them. Without the refusal the write path silently re-asserts the value.
+		for (const name of ['createdAt', 'updatedAt']) {
+			const r = await ops({ operation: 'update', table: 'Dog', records: [{ id: 'us-7', __unset__: [name] }] });
+			ok(r.status >= 400, `unsetting '${name}' should be refused; got ${r.status} ${JSON.stringify(r.body)}`);
+		}
 
-		const r = await ops({
+		const after = await read('Dog', 'us-7');
+		strictEqual(after.createdAt, before.createdAt, 'no refused request may have altered the record');
+		strictEqual(after.name, 'Penny');
+
+		// ...and on THIS table `__createdtime__` is not a managed attribute at all, just a name the
+		// record doesn't have — so it is an ordinary no-op, not a refusal. The legacy spelling is only
+		// managed where it is the table's actual timestamp attribute (next test).
+		const legacy = await ops({
 			operation: 'update',
 			table: 'Dog',
 			records: [{ id: 'us-7', __unset__: ['__createdtime__'] }],
 		});
-		ok(r.status >= 400, `unsetting a system timestamp should be refused; got ${r.status} ${JSON.stringify(r.body)}`);
+		strictEqual(legacy.status, 200, `an unknown attribute name is a no-op; got ${JSON.stringify(legacy.body)}`);
+	});
+
+	// The other half of "one check resolves both spellings": on an open table created through
+	// `create_table`, `__createdtime__` IS the table's createdTimeProperty — `Table.ts:504` assigns it
+	// by name when no `assignCreatedTime` flag exists — so the same code refuses it there.
+	test('__unset__ refuses the legacy timestamp names on an open table, where they are the managed ones', async () => {
+		strictEqual(
+			(await ops({ operation: 'create_table', database: 'data', table: 'OpenStamp', primary_key: 'id' })).status,
+			200
+		);
+		await insert('OpenStamp', [{ id: 'os-1', name: 'Penny' }]);
+
+		for (const name of ['__createdtime__', '__updatedtime__']) {
+			const r = await ops({ operation: 'update', table: 'OpenStamp', records: [{ id: 'os-1', __unset__: [name] }] });
+			ok(r.status >= 400, `unsetting '${name}' should be refused; got ${r.status} ${JSON.stringify(r.body)}`);
+		}
+
+		strictEqual((await read('OpenStamp', 'os-1')).name, 'Penny', 'no refused request may have altered the record');
 	});
 
 	test('a malformed __unset__ is rejected', async () => {
@@ -639,6 +678,42 @@ suite('full_record: full replace over the operations API', { skip: skipSuite },
 		ok(stored == null, `nothing should have been written; got ${JSON.stringify(stored)}`);
 	});
 
+	// The refusal keys on whether there is a record to remove from, not on which operation asked.
+	// Guarding the insert flag alone left this case reaching the same silent partial write one branch
+	// over: `insertUpdateValidate` requires a primary key only for `update`, so an `upsert` without one
+	// takes the auto-keyed `Table.create` path and stored the record with the named attributes stripped.
+	test('__unset__ is refused on an upsert that has no primary key to look up', async () => {
+		// A name no other probe in this suite uses, so the search below can only match the record this
+		// request would have auto-keyed. Querying on "name plus some null columns" matched records other
+		// tests had legitimately stripped attributes from.
+		const r = await ops({
+			operation: 'upsert',
+			table: 'Dog',
+			records: [{ name: 'UpsertNoPrimaryKey', color: 'black', __unset__: ['color'] }],
+		});
+		ok(r.status >= 400, `upsert with no primary key should be refused; got ${r.status} ${JSON.stringify(r.body)}`);
+
+		// Nothing auto-keyed should have been stored — a 200 here was the bug.
+		const all = await ops({
+			operation: 'sql',
+			sql: `SELECT id FROM data.Dog WHERE name = 'UpsertNoPrimaryKey'`,
+		});
+		strictEqual(all.status, 200, `sql should 200; got ${JSON.stringify(all.body)}`);
+		strictEqual((all.body ?? []).length, 0, `no auto-keyed record may exist; got ${JSON.stringify(all.body)}`);
+	});
+
+	// ...and the same for an `upsert` naming a primary key that isn't stored: there is still nothing to
+	// remove from, so it is a misunderstanding rather than a create.
+	test('__unset__ is refused on an upsert whose primary key is not stored', async () => {
+		const r = await ops({
+			operation: 'upsert',
+			table: 'Dog',
+			records: [{ id: 'ups-absent', name: 'Penny', color: 'black', __unset__: ['color'] }],
+		});
+		ok(r.status >= 400, `upsert of an absent record should be refused; got ${r.status} ${JSON.stringify(r.body)}`);
+		ok((await read('Dog', 'ups-absent')) == null, 'nothing should have been created');
+	});
+
 	// `hdbTable` is `Joi.alternatives(Joi.string(), Joi.number())`, so a table named `0` validates. A
 	// truthy `table` test dropped it from `schemaTableMap`, leaving `hasPermissions` nothing to
 	// iterate — the same vacuous-truth bypass the database resolution fixed, one guard over.

validation/insertValidator.ts

@@ -1,6 +1,6 @@
 import { hdbTable, hdbDatabase } from './common_validators.ts';
 import * as validator from './validationWrapper.ts';
-import { TIME_STAMP_NAMES, UNSET_ATTRIBUTES } from '../utility/hdbTerms.ts';
+import { UNSET_ATTRIBUTES } from '../utility/hdbTerms.ts';
 import Joi from 'joi';
 const INVALID_ATTRIBUTE_NAMES = {
 	undefined: 'undefined',
@@ -53,12 +53,12 @@ function unsetAttributesError(unset: unknown): string | undefined {
 		if (typeof name !== 'string' || name.length === 0) {
 			return `'${UNSET_ATTRIBUTES}' must contain only non-empty attribute names`;
 		}
-		// The server owns these; `checkAttributePerms` already refuses to let a role write them, and
-		// a full-record write retains __createdtime__ rather than dropping it, so removing them here
-		// would be the one way to lose them.
-		if ((TIME_STAMP_NAMES as readonly string[]).includes(name)) {
-			return `'${name}' is maintained by Harper and cannot be unset`;
-		}
+		// Managed timestamps are NOT refused here. This validator has no table schema, so a static
+		// list would catch only the legacy `__createdtime__`/`__updatedtime__` and miss a
+		// schema-declared `createdAt: Float @createdTime` — which the write path then silently
+		// re-asserts, a 200 that changes nothing. `ResourceBridge.takeUnsetAttributes` refuses them
+		// instead, off `Table.createdTimeProperty`/`updatedTimeProperty`, which resolve both spellings.
+		// One check in the layer that can do it completely, rather than two that disagree.
 		if (name === UNSET_ATTRIBUTES) {
 			return `'${UNSET_ATTRIBUTES}' is not an attribute and cannot be unset`;
 		}