fix(audit): track deleteHistory removals with bounded concurrency - #2051
Draft
kriszyp wants to merge 8 commits into
Draft
fix(audit): track deleteHistory removals with bounded concurrency#2051kriszyp wants to merge 8 commits into
kriszyp wants to merge 8 commits into
Conversation
…he last Table.deleteHistory() (the LMDB path behind delete_transaction_logs_before) stashed each removeAuditEntry()/removeEntry() promise in a single `completion` variable, overwriting it every iteration and awaiting only the last one. A rejection from any non-last entry — e.g. a decode failure over a corrupt or misaligned audit entry — was silently dropped and surfaced later as an unhandled rejection instead of being caught here. On Bun this killed the process a few seconds after the operation had already reported COMPLETE, with no shutdown log line (harper#F-264, 4th independent reproduction). Mirrors the same fix already applied to the sibling scheduleAuditCleanup loop in auditStore.ts (d6c64ea, 4ad7fdc, 1f7ddbe) — deleteHistory had the identical pattern, unpatched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…unting failed removals From cross-model review (Codex, Gemini, Grok): - removeAuditEntry() still let the primary-store tombstone removal (the addDeleteRemovalCallback callback, which calls primaryStore.remove()) run detached — the exact class of bug this change set out to fix, just one level deeper. Return the callback's promise and combine it with the audit-store removal via Promise.all so a caller that awaits removeAuditEntry() actually covers both writes. - deleteHistory() counted entriesDeleted even when the removal it just caught had failed, so a purge that hit a systemic failure (e.g. LMDB map full) could report entries_deleted equal to the full backlog and status COMPLETE. Only count on success now. - Regression test rewritten without sinon (AGENTS.md bans new sinon/rewire usage in unitTests/resources/*) using a plain property swap instead, and keyed on the target entry's actual audit-store key rather than a global call counter so an unrelated background scheduleAuditCleanup pass can't perturb it. Added an isolated unit test for removeAuditEntry's delete-callback fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t-entry removal From cross-model review round 2 (Codex, Gemini, domain adjudication): - Promise.all failed fast on a rejecting delete-callback even when the audit- store removal itself succeeded, mis-logging it as "Error removing audit entry" and under-counting a removal that actually happened. The delete- callback's rejection is now caught and logged separately as an orphaned- tombstone warning; only the audit-store removal's own outcome determines removeAuditEntry's result and deleteHistory's count, matching the fallback cleanupDeletedRecords sweep that already exists for exactly this case. - Rewrote the isolated removeAuditEntry unit test for that contract, and dropped a "real callback wiring" integration test I'd added — it turned out the auditStore/primaryStore instance `AuditedTable.primaryStore` exposes in this test file isn't reference-equal to the one Table.ts's addDeleteRemoval callback actually closes over, so the test was silently asserting nothing. Filed as a Finding rather than shipping a misleading green check. - Moved the first test's orphan-cleanup call into its finally block so a failed assertion doesn't leak state into a later test. - Trimmed DESIGN.md's note (dropped commit-hash/ticket-ID provenance and an overclaimed "kills the process on Node" framing) and extended it to cover the nested delete-callback hazard. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…an throw From cross-model review round 3 (Codex, Gemini, domain adjudication) — narrowing residuals only, no new behavior change: - The delete-callback's promise was created and its .catch() attached across two separate statements with auditStore.remove() in between. A synchronous throw from that remove() call (store closing mid-pass) could unwind past the .catch() attachment, leaving the callback's promise without a handler. Wrapped the whole capture-and-attach sequence in one try/catch so nothing can run between creating the promise and handling it. - DESIGN.md/comment overstated the recovery story: the orphaned tombstone left by a failed callback is only swept by an operator-triggered cleanup_deleted_records run, not by the automatic scheduleAuditCleanup pass (which has no way to find it once the audit entry is gone). Qualified the claim. - Switched the LMDB-only regression test's rocksdb guard to this.skip() instead of a bare return, so a RocksDB CI leg reports "skipped" rather than a false "passed" that asserted nothing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses a promise handling issue where audit-entry and tombstone removal loops did not properly await each iteration's promise, leading to unhandled rejections and missed cleanups. The changes ensure that each removal is awaited inline and errors are caught and logged. Additionally, regression tests were added to verify this behavior. The reviewer suggested simplifying the promise wrapping in removeAuditEntry by using new Promise to catch both synchronous throws and asynchronous rejections in a single block, eliminating the redundant try/catch block.
Contributor
|
Reviewed; no blockers found. |
…executor From gemini-code-assist's review comment on PR #2051: use new Promise((resolve) => resolve(callback())) instead of a try/catch around Promise.resolve(callback()) — a synchronous throw from the executor rejects the promise the same way a rejection would, so one .catch() now covers both paths instead of two separate warn() call sites. Extended the isolated removeAuditEntry unit test to cover both a rejecting and a synchronously-throwing delete-callback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kriszyp
marked this pull request as ready for review
August 2, 2026 17:42
kriszyp
marked this pull request as draft
August 2, 2026 17:42
Keep up to ten audit or tombstone removals in flight while attaching error handling to every write and draining each phase before returning. Add an LMDB regression test that proves the loop is neither serial nor unbounded. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Keep the tracked removal promise fulfilled if its warning handler throws, and pin that secondary rejection path in the LMDB regression test. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Restore the process-wide logger immediately after the removal call and assert the original storage failure was passed to the warning handler. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
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.
What
Table.deleteHistory()— the LMDB path behinddelete_transaction_logs_before— overwrote each iteration's removal promise and awaited only the last one. A non-final rejection therefore surfaced later as an unhandled rejection.The fix now tracks every audit-entry or tombstone removal immediately, attaches failure handling at creation, permits at most 10 removals in flight, and drains each phase before returning. This keeps storage writes overlapped without restoring the original unbounded pending set.
removeAuditEntry()also joins the delete callback that removes the corresponding primary-store tombstone, instead of firing that promise and forgetting it.Why
Under Bun, an unhandled rejection here killed Harper a few seconds after the cleanup job had already reported
COMPLETE. The delayed failure looked like a native storage crash, but the root cause was detached JavaScript promises in the prune loop.This mirrors the fix already merged for the sibling
scheduleAuditCleanuploop in #1963.Changes
resources/Table.tsdeleteHistory()removal in a local set.primaryStore.remove(id, version)promise from the delete callback.resources/auditStore.tsunitTests/resources/auditLog.test.jsDESIGN.mdVerification
npm run build— passed.HARPER_STORAGE_ENGINE=lmdb npx mocha unitTests/resources/auditLog.test.js— 21 passing, 5 pending.npm run test:unit:resources— 1,354 passing, 17 pending.HARPER_STORAGE_ENGINE=lmdb npm run test:unit:resources— 1,202 passing, 79 pending.npm run test:unit:main— 4,211 passing; 10 pre-existingglobalIsolation.test.jsfailures caused by this nested worktree resolvingmqttthrough the parent checkout.npx harper-integration-test-run integrationTests/apiTests/transaction-logs.test.mjswith an isolated loopback pool — 7/7 passing.npm run test:integration:allwith an isolated loopback pool — current-version suites completed cleanly; two legacy 5.1 cross-version suites failed before their test bodies because the nested worktree's generated Unix-domain socket path exceeded the kernel limit (listen EINVAL).Risks & open questions
deleteHistorystill cannot report partial or total removal failures throughDeleteTransactionLogsBeforeResults. A systemic failure can returnentries_deleted: 0with a successful operation response and may emit one warning per failed record. Changing the result schema or abort policy is a separate design decision.addDeleteRemovalcallback wiring is covered indirectly. Dropping the callback's newreturnspecifically would not fail the current tests.deleteHistoryregression tests are LMDB-only, matching the code path, and the optionalcleanupDeletedRecordsphase does not have dedicated concurrency coverage.cleanup_deleted_records: truesweep later.Refs #1963
Co-Authored-By: GPT-5 Codex noreply@openai.com
🤖 Generated with Claude Code