HarperFast / HarperFast/harper

sourcedFrom auto-store version precondition is disarmed on any refill of an existing entry

Open
#2,383 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

## Summary

The `sourcedFrom` auto-store carries a version precondition, but it only has effect on a **true miss**. On a refill of a key that still had an entry (INVALIDATED, expired, tombstoned, stale-while-revalidate), the precondition compares the fill's own pre-fetch snapshot against itself, so it can never fail — and the delayed auto-store overwrites any value published while the source call was in flight.

Measured on `harper@5.2.7`; the same code shape is present in 5.0.10, 5.0.22, 5.1.15, 5.1.23, and 5.2.7.

## Where

`resources/Table.ts`, `getFromSource()` stages the auto-store with the entry read **before** the source call:

```js
const sourceWrite: any = {
key: id,
store: primaryStore,
entry: existingEntry, // <- captured before throttledCallToSource()
nodeName: 'source',
commit: (txnTime, existingEntry, _retry, transaction) => {
sourceWrite.skipped = false;
if (existingEntry?.version !== existingVersion) {
// don't do anything if the version has changed
sourceWrite.skipped = true;
return;
}
...
```

`resources/DatabaseTransaction.ts`, `save()` re-reads that entry inside the commit transaction only when the seed is absent:

```js
if (reloadEntry || operation.entry === undefined) {
operation.entry = operation.store.getEntry(operation.key, { transaction });
}
operation.commit(txnTime, operation.entry, this.retries > 0, transaction);
```

So:

- `existingEntry === undefined` (true miss) -> the entry is re-read at commit, the precondition sees the concurrent publication, and the write is correctly skipped.
- `existingEntry` present -> no reload; `existingEntry.version` and `existingVersion` are the same captured value, the precondition always passes, and the stale source result is committed over the newer publication.

## Reproduction

Minimal component: a `Probe` table `sourcedFrom` a source whose `get()` blocks on a gate; a direct publication commits while the source is blocked; the gate is released.

| Key state when the source call began | Result after the auto-store |
| --- | --- |
| deleted (no entry) | newer publication survives |
| `Probe.invalidate(id)` (entry present) | **newer publication overwritten by the older source result** |

```
{"harperVersion":"5.2.7","results":[
{"keyState":"invalidated","guarded":false,"observed":"SOURCE-OLD","STALE_OVERWRITE":true},
{"keyState":"deleted","guarded":false,"observed":"NEWER-DIRECT-PUBLICATION","STALE_OVERWRITE":false}]}
```

## Impact

Any caching table that combines source-on-miss fills with direct publications loses data on the refill path. It is not a narrow window: a component that forces refills by deleting and re-getting, or that relies on expiry, is in the unprotected case on essentially every refill. HarperFast/harper-woo-cache#252 hit this in production shape.

## Suggested smallest fix

Arm the precondition on every path by giving the source write the same reload the miss path already gets — stage it so `save()` re-reads the entry inside the commit transaction (e.g. leave `operation.entry` unset for the source write, or pass `reloadEntry` for it). Then `existingEntry.version !== existingVersion` compares current durable state against the version captured before the source call, which is what the comment already claims it does.

## Workaround in use

The component sets `noCacheStore` from inside the source's `get()` and performs its own publication under a precondition evaluated inside an isolated transaction. Two notes for anyone doing the same:

- `noCacheStore` aborts the source context's transaction, so a publication staged on that inherited transaction is rolled back with it — the publication must run in its own explicit transaction.
- A direct `put`/`delete` on a table that has a source is written through to that source unless the calling context carries `source`; a source implementing only `get()` answers `405 ... does not have a put method implemented`.

Contributor guide

Open the contributing guide

Research direction

Start in resources/Table.ts at getFromSource() and in resources/DatabaseTransaction.ts at save(), then reproduce the blocked Probe source call with a concurrent direct publication. Done means a refill re-reads the entry at commit, skips the stale source result when the version changed, and preserves the existing true-miss behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, nodejs
Domain
database
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.