Repository navigation
fix(repair): confirm rollback absence before reporting unrecovered rows, and move the full-row dump off stderr - #974
Merged
Conversation
…ws, and move the full-row dump off stderr The repair rollback write shares the replacement add's commit-then-reject hazard: it can persist and still surface an error. repairLegacyScopes treated any rollback error as data loss and pushed the row into unrecovered, falsely reporting a row that was actually restored. The rollback catch now re-reads the row by id and reports unrecovered only when the table confirms absence; a failed confirmation read stays fail-closed and still reports. The repair-scopes CLI dumped every unrecovered row as full JSON (text, metadata, vectors) to stderr. Full rows now go to a recovery file next to the db path and stderr carries a bounded id/scope/preview summary plus the file path; when the recovery file cannot be written, the original full stderr dump remains as the fallback so the data is never lost.
gorkem2020
marked this pull request as ready for review
July 28, 2026 16:59
rwmjhb
approved these changes
Jul 29, 2026
rwmjhb
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed head fe3f225. The targeted tests, full suite, and repository CI pass, and the change improves both ambiguous rollback reporting and the prior unbounded full-row stderr dump. I found no demonstrated HIGH/CRITICAL defect on the production same-handle rollback path. Approving.
Please consider follow-up hardening for the recovery artifact: create it with 0600 and exclusive/collision-resistant semantics, avoid including raw memory text even in the bounded stderr summary, and verify the confirmed row is the restored legacy state rather than merely any row sharing the ID. A two-handle/latest-version regression case would also strengthen the rollback guarantee.
This was referenced Jul 30, 2026
Merged
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
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.
Problem
Two follow-ups the reviewer named explicitly on #923 round 4 ("the rollback commit-then-reject false
unrecoveredreport and full-row stderr output are useful follow-ups, but they are not the blocker"):repairLegacyScopestreated any rollback error as data loss and pushed the row intounrecovered, falsely reporting a row that was actually restored to the table.repair-scopesCLI dumped every unrecovered row as full JSON (memory text, metadata, embedding vectors) to stderr, flooding terminals and shared logs with row content.Change
src/store.ts: the rollback catch now re-reads the row by id and reportsunrecoveredonly when the table confirms the row is truly absent. A failed confirmation read stays fail-closed and still reports, so genuine loss is never hidden.cli.ts: full unrecovered rows are written to a recovery file next to the db path (<dbPath>-unrecovered-<timestamp>.json), and stderr carries a bounded id/scope/preview summary plus the file path. When the recovery file cannot be written (or no db path is available), the original full stderr dump remains as the fallback, so the data-preservation guarantee is unchanged.test/scope-owner-leak-hardening.test.mjs, three new cells in the file that owns this subsystem: a rollback commit-then-reject regression (rollback persists then errors, must not be reported unrecovered, row must remain discoverable as legacy), the recovery-file path (full row with vector preserved in the file, no raw JSON on stderr), and the fallback path (unwritable recovery file keeps the complete stderr dump).Validation
npm run buildclean,tsc --noEmitclean, fullnpm testchain green,cli-smokeCI group green (the CLI output change gate),core-regressiongroup green, dist rebuilt in the same commit. No registration changes needed (the test file was already registered).memory-pro repair-scopesreport-only run executed the changed command against the live store without issues.