docs(resources): the audit floor detects pruning, not a restored rollback - #666
Open
dawsontoth wants to merge 13 commits into
Open
docs(resources): the audit floor detects pruning, not a restored rollback#666dawsontoth wants to merge 13 commits into
dawsontoth wants to merge 13 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the API documentation in resource-api.md to introduce and explain the oldestRetainedAuditTime() method, along with updating the startTime property description. The reviewer suggested formatting the critical caveat regarding database rollbacks as a blockquote admonition to ensure it is prominently displayed and consistent with the document's existing style.
cb1kenobi
reviewed
Sep 3, 2026
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
Kris Zyp on HarperFast/harper#2458: the "does not report a cursor as safe when history it needed is gone" bullet is stronger than what the floor can see. Restoring a backup, or opening a RocksDB checkpoint, replaces a database's state with a copy of an earlier state and reinstalls that copy's floor, so a cursor saved after the copy point compares as safe against a floor that predates it. Scope the one-direction guarantee to retention pruning and add the rollback limit as its own bullet, so nobody reads this method as the only gate on resuming across a restore.
dawsontoth
force-pushed
the
docs/audit-staleness-floor
branch
from
September 3, 2026 14:11
aa657ab to
5129059
Compare
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
cb1kenobi
reviewed
Sep 3, 2026
…implying safety Two P1s from Chris Barber on #666, both correct. The database-scoped bullet reversed the time direction: `cursor >= floor` was said to mean no entry was pruned "below the cursor", which is backwards and describes the one thing the floor does NOT promise. Entries below the cursor are older than the floor and may well be gone — that is the floor's whole purpose. What the comparison guarantees is that nothing was pruned *after* the cursor. The sample also did exactly what the prose two paragraphs below it warns against: `cursor >= floor` then subscribe, which after a restore silently resumes from a cursor newer than the restored state. There is no generation check to call yet (harper#2451), so the sample now leads with the definite conclusion — `cursor < floor` means resync — and the permissive branch carries the caveat that this is a pruning check only. Also formats the rollback limitation as a blockquote callout per gemini-code-assist, matching this file's existing `> **…**` callouts rather than the `:::caution` used elsewhere in the repo.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
cb1kenobi
reviewed
Sep 3, 2026
…itely lost history" Chris Barber on #666. The wording I added last round overclaimed in the mirror image of the direction bug it was fixing: an `Infinity` floor puts every cursor below it with nothing necessarily pruned, and the "errs in one direction only" bullet on the same page says the floor can ask for a resync that was not strictly necessary — which is exactly a `cursor < floor` that lost nothing. The certainty belongs to the action. Summary line and the sample's resync branch now say the history *may* have been pruned and the floor cannot certify otherwise. Same correction applied to the engine-side contract it came from, HarperFast/harper#2458.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
cb1kenobi
reviewed
Sep 3, 2026
…ed claim Two mediums from Chris Barber on #666, both introduced by my previous fix. Inverting the branch to `cursor < floor` changed what an unset or non-numeric cursor does. `undefined < floor` is false, as is any NaN comparison, so a consumer on its first run — or one whose persisted cursor failed to load — fell into the resume branch and called `subscribe({ startTime: undefined })`: live, no catch-up, no resync. The old `cursor >= floor` form failed the other way. Measured across undefined/null/NaN/"abc": only `null` resynced under the inverted form. Now `!(cursor >= floor)`, which keeps the definite conclusion first and sends everything non-comparable to resync. Carries a comment explaining the negation, so it does not get "simplified" back to the trap. Also scopes "everything above it is still retained" to "nothing above it has been pruned as of this reading", matching the two bullets below that already qualify it for restore/checkpoint rollback and for retention advancing after the call.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
Found by diffing this page against the engine-side contract, which Chris Barber's note about wording travelling between the two prompted. The bullet named the two most common causes of `Infinity` as a database's first open by a floor-recording version, and a migration between storage engines. Both are wrong, and stale from before the branch decided to stamp a starting epoch: a store with no floor record is now given `max(Date.now(), newest retained key)` the first time it is opened, including the audit-store-less result of an engine migration, so those are precisely the cases that report a real value. Verified against the engine test that asserts a fresh database's floor is finite. The real causes are a read-only database, a failed or unavailable metadata write, metadata that does not decode, and a prune that ran on a database with no floor recorded yet. Says so, and says explicitly that first open is not one of them — since "consumers resync once and then get real values" invited exactly the wrong mental model.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
cb1kenobi
reviewed
Sep 3, 2026
…pgrade note Chris Barber on #666, both points against my previous commit. The cause list contradicted itself: it named "history pruned on a database that had no floor recorded yet" as a peer cause while the next sentence said first open always stamps a floor. Both cannot hold — a prune finds no floor only when the stamping did not land, which the read-only and failed-write cases already cover. Folded in as a consequence rather than a cause. The same edit also dropped what made the old wording actionable, and he is right that this matters more than the contradiction. The starting floor is stamped at open time, so it sits ABOVE entries the database still retains: on a node upgraded with a week of history, every pre-upgrade cursor falls below it and resyncs once with nothing pruned. Against the page's own definition of the floor as "the oldest point retention has not pruned away", that resync reads as proof the upgrade lost history — the false "audit history lost" alarm from an earlier round arriving by a different route. Split into its own bullet rather than folded into the Infinity one, because it describes a FINITE floor and has nothing to do with the unknown sentinel. That also resolves the tension with the summary definition, by saying outright that the starting floor is not derived from history.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
cb1kenobi
reviewed
Sep 3, 2026
"a floor that only real prunes move" implied the floor is monotonically non-decreasing and prune-caused. A restore or checkpoint reinstalls the copy's floor, which can be LOWER — the one direction that sentence ruled out — so a consumer that caches the floor and re-reads it only to see whether it rose never registers a rollback, and resumes from a cursor newer than the restored state. That is the exact silent gap the rollback caution two bullets down exists to warn about. Names the two raisers (retention, `deleteHistory()`), calls the restore an exception to the direction rather than another mover, and states the consequence outright: re-read the floor on each resume. Tied the raisers to pruning rather than listing `deleteHistory()` as an independent mover, because it only prunes on LMDB — on RocksDB `remove()` is a no-op, so it moves nothing (Table.ts:5562). "Rises when history is actually pruned" holds on both engines; a flat list would not.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
cb1kenobi
reviewed
Sep 3, 2026
3037a0f put "re-read the floor on each resume rather than caching it and watching for it to rise" directly after the restore sentence, so it read as the mitigation for a rollback. It is not one. A restore reinstalls a LOWER floor, so a fresh read certifies at least the cursors a stale one did: cursor 600 against a floor restored 500 -> 100 passes either way. A consumer author could implement the re-read and conclude the restore hazard was handled — the overclaim shape this PR exists to remove. Scopes the advice to the reason it actually holds (retention advances between reads, so a stale floor certifies cursors the current one rejects) and rules out the rollback reading outright.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
cb1kenobi
reviewed
Sep 3, 2026
"It does not report a cursor as safe when a prune removed history that cursor needed" was stated absolutely, and the validity bullet four lines above documents a counterexample inside its own scope: a persisted `getHistory().localTime` is an origin version, so a record written with origin 900 can sit at audit key 400. Persist 900 against a floor of 500 and the check passes while the consumer's real position (400) is below the floor. No restore involved — this is pruning, which is exactly what the "Across retention pruning" qualifier admits. Conditions the claim on cursor validity and names the excluded case in the load-bearing sentence itself, so a reader lifting that sentence to restate the contract carries the condition with it. That is how the two previous overclaims travelled. Predates this branch (#660), so the fix is a correction rather than a regression, but the failure shape is the same.
dawsontoth
added a commit
to HarperFast/harper
that referenced
this pull request
Sep 3, 2026
The docstring opens by ruling out `getHistory`'s `localTime` as a cursor, then 13 lines later claims the floor will "never certify a cursor a prune truncated" — stated absolutely, so the sentence quoted on its own drops the condition that makes it true. That is how the claim reached the public docs unconditioned (HarperFast/documentation#666), where a reviewer caught the same conflict against the same counterexample. A `getHistory` cursor is an origin version, so a record written with origin 900 can sit at audit key 400: persist 900 against a floor of 500 and the check passes while the consumer's real position is below the floor. Names that as outside the guarantee rather than leaving it to collide with the validity note above. Comment-only; the contract does not change.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
cb1kenobi
reviewed
Sep 3, 2026
dawsontoth
added a commit
to HarperFast/harper
that referenced
this pull request
Sep 3, 2026
The summary says a consumer whose cursor is `>=` the floor can resume incrementally; the `getHistory` exclusion that makes it true is two paragraphs down. Same shape as the public docs summary a reviewer just caught (HarperFast/documentation#666), and the summary is the sentence that gets lifted to restate the contract. One word: "audit-log cursor", so the domain travels with the claim. The paragraph below still defines what that domain is. Left the DESIGN.md cheat-sheet row alone — it already hedges with "only that", says "audit cursor" in its own question, and points at the full contract rather than standing in for it.
Two fixes from review of 3902e7a. The summary said "a cursor at or above it has not been pruned" unconditionally, which 3902e7a had just conditioned four lines down. It is also the sentence a reader lifts to restate the contract — by this branch's own account, how the earlier overclaims reached the engine docstring. Now carries the same condition and says what is actually guaranteed: no history pruned AFTER a valid cursor. The sample's guard comment claimed an unset or non-numeric cursor "compares false either way". Measured: `null < 500` and `'' < 500` are both TRUE, since both coerce to 0 — so the claim is wrong for two of the five cases, not just null. That matters because the comment exists to stop the guard being simplified back to `cursor < floor`: a reader who checks it against a JSON-decoded null finds it false and discounts the whole comment. Restated as the fact that is actually load-bearing and holds for all five — neither is ever `>=` the floor.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
dawsontoth
added a commit
to HarperFast/harper
that referenced
this pull request
Sep 3, 2026
Last of the unconditioned `cursor >= floor` restatements found by a sweep of every guarantee-shaped sentence in this contract, prompted by the third instance turning up in review (HarperFast/documentation#666). The opening summary needed no change here: it claims only that every entry at or after the floor is retained, and never the converse — which is the direction the public docs had backwards.
Found by sweeping every guarantee-shaped sentence in this section for whether it survives being quoted alone — the property behind the last three review findings. The section's opening definition called the floor "the oldest point in the audit log that retention has not pruned away". Two of this page's own bullets contradict that: the starting floor is stamped at open time and so sits ABOVE entries the database still retains, and on RocksDB entries below the floor are routinely still on disk. The floor bounds what is retained; it does not measure it. This is the section's most liftable sentence, and unlike the previous three the engine had it right (getAuditFloor claims only that entries at or after the floor are retained, never the converse). Also conditions the database-scoped restatement of `cursor >= floor` on cursor validity, matching the summary and the one-direction bullet. Engine docstring mirrored in harper f885affef.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
cb1kenobi
reviewed
Sep 3, 2026
…nism Three fixes, one from review of 378fb1f and two my own sweep missed. Reviewed: the "exactly at the floor" gloss stated it flat. Conditioned. My keyword sweep missed it because it matched on has not/cannot/never/ guarantee/safe/only, and that sentence says "passes" and "already been handled" — a guarantee with none of those words. Re-ran the pass by reading every sentence instead of grepping, which found two more: - The sample's else-branch comment was the sole remaining unconditioned restatement, in the artifact readers copy. I left this deliberately last round, judging the qualifier noise three lines under the guard. Wrong call now that every other restatement carries the condition — the holdout is what looks authoritative. - The validity bullet still said the pruned messages sit "between" the cursor and the floor. That is the mechanism I diagnosed as backwards two rounds ago and fixed only in the one-direction bullet: they sit BELOW the floor, and the defect is the cursor overstating the consumer's position. The two bullets disagreed until now.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
cb1kenobi
reviewed
Sep 3, 2026
dawsontoth
added a commit
to HarperFast/harper
that referenced
this pull request
Sep 3, 2026
All three statements of the contract said the floor promises nothing below the CURSOR, and gave as the reason that such entries "sit below the floor". That reason only holds when cursor === floor. For any cursor strictly above it, `[floor, cursor)` is below the cursor and at or above the floor — and the floor's own definition guarantees nothing after it was pruned, so that range is covered. Consequence of getting it backwards: a consumer holding cursor 900 against a floor of 500 that wants `[500, 900)` — replaying events it processed but never committed, backfilling a derived store, an operator audit — was told the range may well be gone, so it falls back to a full re-read and discards per-event history the floor guarantees is intact. An operator reads the same sentence as a prune that removed data still on disk. Found in the public docs by a reviewer (HarperFast/documentation#666); all three engine-side copies had it identically. Comment-only. Build clean, audit suites pass.
…aimer Both from review of a2bc25e. The validity bullet said a getHistory cursor passes "while ... the entries that position still needed are already gone". Sitting below the floor is not the same as pruned — line 256, added in this same PR, says the floor is a lower bound and the log routinely holds older entries, and line 283 says a stamped starting floor sits ABOVE retained entries. So on an upgraded node this asserted confirmed history loss where nothing had been pruned: the same false alarm already removed from the summary line and the resync branch. My regression from a2bc25e, introduced while fixing the mechanism in that same sentence. The database-scoped bullet said the floor promises nothing below the CURSOR because those entries "are older than the floor". True only when cursor === floor. For cursor 900 against floor 500, `[500, 900)` is below the cursor and guaranteed unpruned — a consumer wanting that range for an uncommitted replay or a backfill was told it may well be gone. Scoped the disclaimer to the floor. All three engine-side copies of the second one had it identically; fixed in harper 1761543c6.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-666 This preview will update automatically when you push new commits. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #660, which shipped the
oldestRetainedAuditTime()docs with a guarantee stronger than the implementation.Kris Zyp, reviewing HarperFast/harper#2458:
He is right. Restoring a backup, or opening a RocksDB checkpoint, replaces a database's state with a copy of an earlier state — and reinstalls that copy's retention floor along with it. A cursor saved after the copy point then compares as safe against a floor that predates it, even though the database no longer holds a change stream describing the rollback. The floor detects retention pruning; it is not a database-generation check.
Changes:
The engine-side contract is corrected in the same terms in harper#2458 (JSDoc,
resources/DESIGN.md), and the underlying fix — a database generation stamped at every state-copy path — is harper#2451.