HarperFast / HarperFast/harper

Over-time transaction abort excludes sourceApply/isReplay, so those paths still force-commit partial write sets and can orphan secondary-index entries (silently) — #1407 gap

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

#1407 ("Force-committing an over-time transaction leaves orphaned secondary-index entries") was
closed by making the transaction monitor **abort** an over-time transaction that has pending
writes, instead of force-committing a partial write set. That fix is correct, but it is guarded by
two exclusions — `sourceApply` and `isReplay` — and those paths still take the original
force-commit branch. The atomicity violation #1407 describes therefore still occurs on
replication-apply and crash-replay writes, and it is silent.

## The code

`core/resources/DatabaseTransaction.ts`, in the monitor's expiry handler:

```ts
} else if (txn.hasPendingWrites() && !txn.sourceApply && !txn.isReplay) {
// Abort and surface an error rather than force-committing a partial write set: silently
// committing on the application's behalf breaks atomicity and can leave orphaned
// secondary-index entries that only a full index rebuild repairs (issue #1407). ...
harperLogger.error(`Transaction was open too long and has been aborted ...`);
txn.abortDueToTimeout();
} else {
// Read-only long transaction (no atomicity/index risk ...), or a canonical-source
// apply/replay that must never drop a write: preserve the prior behavior of committing
// to close out the snapshot without poisoning the transaction.
const result = txn.commit();
...
}
```

The `else` arm is reached by two distinct cases with very different risk:

1. a **read-only** long transaction — genuinely no atomicity or index risk, as the comment says;
2. a **`sourceApply` / `isReplay` transaction with pending writes** — which is exactly the
force-commit-a-partial-write-set operation the first arm exists to prevent.

The comment justifies the exclusion on the grounds that aborting a canonical-source write would
drop it while the resume cursor advanced past it (harper-pro#348) — a permanent divergence. That
trade-off is real. The problem is that the alternative chosen is the one #1407 documented as
corrupting the index, and nothing marks or repairs the result.

## Why this matters

An orphaned secondary-index entry does not self-heal. Per #1407 it needs a full index rebuild, and
`updateIndices` cannot recover it during normal operation:

```ts
const existingValue = existingRecord && (resolver ? resolver(existingRecord) : existingRecord[key]);
if (value === existingValue && !isIndexing) continue; // an unchanged value does no index work
...
let valuesToRemove = getIndexedValues(existingValue, indexNulls);
if (valuesToRemove?.length > 0) { ... index.remove(...) } // removal driven ONLY by existingRecord
```

So a subsequent `put` of the same value is a no-op for the index, and a removal that was missed
stays missed. That is the mechanism behind #2211's report that "rewriting the record does not
repair it".

Two independent index divergences have now been reported in the wild on 5.2.x:

- #2211 — indexed reads returning a strict subset of the rows carrying a value, persistent, not
repairable by rewriting.
- A separate 4-node cluster where a `Long @indexed` attribute had entries left at a record's
**old** value with none at its current value. Three rows in ~1.3M, on two nodes independently,
arising on two different application write paths. Consequence there was severe: the stale entry
sorted to position 0 of an ascending index walk, and a consumer that stops at the first
future-dated row therefore processed nothing, silently, for ~18 days.

Neither is proven to originate from this branch — but this is a documented, in-tree path that
produces exactly that state, and it is the one path the #1407 fix deliberately left open.

## It is silent

The abort arm logs at `error` with the table name. The force-commit arm logs nothing on success,
and only `debug` on failure:

```ts
(result as any).catch((error) => {
harperLogger.debug?.(`Error committing timed out transaction: ${error.message}`);
});
```

So a partial write set committed on a replication-apply path leaves no operator-visible trace at
`info` or above, and no marker on the affected table. An operator cannot tell that an index may
now be inconsistent, nor which table to rebuild.

## Asks

1. Should `sourceApply` / `isReplay` transactions with pending writes really take the
force-commit path, given #1407 concluded that operation corrupts secondary indexes? If the
harper-pro#348 constraint makes aborting unacceptable, is there a third option — extending the
deadline for canonical-source applies, committing without the index short-circuit, or splitting
the apply — rather than choosing between dropping a write and corrupting an index?
2. If the force-commit must stay, can it at minimum be **observable**? A `warn` naming the table,
and ideally a durable marker that an index may be divergent, would turn a silent corruption
into something an operator can act on.
3. Is there a supported index rebuild/repair operation? #1407 says a full rebuild is the remedy
and #2211 asks the same question; we could not find one.
4. Is the read-only case worth separating from the write-bearing canonical-source case in that
`else`? They are grouped today, and only one of them is actually risk-free.

## Version

Observed on `harper-pro` 5.2.2/5.2.3 with `@harperfast/rocksdb-js` 2.7.x. Related: #1407 (closed),
#2211 (open), harper-pro#348 (closed).

Contributor guide

Open the contributing guide

Research direction

Start in core/resources/DatabaseTransaction.ts at the monitor expiry handler and compare the read-only timeout path with sourceApply and isReplay transactions that have pending writes. Trace the index behavior described in the issue and review related issues #1407, #2211, and harper-pro#348; the work is complete only after the pending-write behavior and operator-visible handling are explicitly decided.

Written by the indexing model from the issue text.

Assessment

Tech stack
node.js, typescript
Domain
databases
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.