HarperFast / HarperFast/harper

Add `__unset__`: field-scoped attribute removal that is safe under concurrent writes

Open
#2,350 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
89
Forks
10
Avg merge
2d 6h
Merged PRs (30d)
200

Description

## What

A per-record directive on `update`/`upsert` naming attributes to remove, so a caller can drop one field while merging everything else — without resending the rest of the record:

```json
{ "operation": "update", "database": "dev", "table": "dog",
"records": [{ "id": 1, "__unset__": ["age"] }] }
```

`age` is removed; every attribute the request didn't mention keeps its value, and stays safe against a concurrent writer touching a different attribute.

## Why this isn't just `put`

`put` (added in #2347) covers full replacement, which is what removing an attribute needs when the caller has the whole record — Studio's row editor, for instance. What it can't do is remove one field *without* the caller resending everything else:

- The caller must read the record first, so it carries a read-modify-write race.
- Anything it fails to resend is dropped too.
- It replaces at whole-record granularity, so a concurrent write to an unrelated attribute is lost.

A field-scoped removal has none of those. It's the removal counterpart to a patch, and it's the shape a caller wants when it knows one attribute should go and nothing about the rest.

## The hard part, and why the first attempt was withdrawn

#2347 originally shipped this, implemented in `ResourceBridge.upsertRecords` by snapshotting the stored record, deleting the named attributes in JS, and writing the result with `Table.put`. That is a read-then-replace wearing a patch's clothing, and it loses concurrent writes:

- The audit/replication record type is chosen by `fullUpdate` (`resources/Table.ts:2554`), so the write replicates as a whole-record put.
- Out-of-order reconciliation folds field-wise **only** for patches: a newer full put overwrites wholesale, an older one is dropped entirely (`resources/Table.ts:2902-2914`).
- The commit-retry path re-reads the existing record only when `!fullUpdate` (`resources/Table.ts:2530`), so when the optimistic-lock retry fires, the pre-computed merged record is re-applied verbatim — this is reachable **single-node**, not only across replicas.
- The audit record carries only the resolved put, so replication cannot recover the *deletion intent*. That's what rules out fixing it by retrying harder.

Failure scenario: node A takes `update {id:1, __unset__:['nickname']}` while node B concurrently takes `update {id:1, phone:'555'}`. A's write carries the later timestamp, so after convergence `phone` is gone on both nodes. The same two requests without the directive converge to both changes.

So the directive was removed from #2347 rather than shipped with a contract it couldn't honor.

## What doing it properly looks like

The removal has to survive as *intent* through the write path, not be resolved to a value before it. `resources/tracked.ts` `updateAndFreeze` already dispatches on `__op__`, and `resources/crdt.ts` holds the operation registry — currently just `add` — so the apply path exists. The work is the contract around it:

- **Reversibility.** `applyReverse` calls `operations[op].reverse(record, key, { value: value.value })` — the reverse gets only the op's own value. `add` inverts by subtracting from the current value; a removal cannot reconstruct what it removed, so the op has to carry the prior value for audit reconstruction (`getRecordAtTime`) to work.
- **Conflict resolution.** `rebuildUpdateBefore` throws when merging updates with different operations, so removal-racing-a-set needs a defined rule rather than an emergent one.
- **Convergence.** Removal isn't commutative the way `add` is. Two nodes, one removing and one setting the same attribute, need a decision — probably last-writer-wins per attribute, but it should be a decision, not an accident.
- **Attribute permissions.** Named removals can be checked precisely, unlike a full replace: contribute the removed names to `getRecordAttributes` so removing an attribute requires the same `update` permission as writing it. (#2347 did this and it worked; worth keeping.)
- **Namespace.** A per-record `__unset__` key collides with a legitimate user attribute of that name, and #2347 had no migration for tables carrying one. Either reserve it with a creation-time rejection plus a migration path, or put the directive outside the record — a request-level field, which removes the collision class entirely rather than guarding it.

## Test coverage this needs

None of it exists today, on this path or nearby:

- concurrent removal versus a disjoint patch, converging to both
- the same under the optimistic-lock retry, single-node
- audit reconstruction across a removal (`getRecordAtTime` before/after)
- both storage engines (rocksdb and lmdb)
- the attribute-permission check on named removals, including through the bulk-load path

## Notes

- Not a regression: no released version has ever had this. `put` is what shipped in its place.
- Found by cross-model review on #2347; the mechanism analysis above is largely kriszyp's.

Contributor guide

Open the contributing guide

Research direction

Read resources/tracked.ts around updateAndFreeze and resources/crdt.ts for the operation registry, then inspect resources/Table.ts around the cited retry and reconciliation paths. Define the removal contract, conflict behavior, namespace handling, and reverse-audit data before implementing. Done means coverage for concurrent and retry cases, audit reconstruction, both RocksDB and LMDB engines, and named-removal permissions including bulk loading.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js, typescript
Domain
backend, databases, distributed-systems
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.