Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the subscription delivery logic to handle out-of-order resequencing and late-committing transactions, ensuring that merged writes are correctly delivered to subscribers. It introduces helper functions for tracking merged entries and stale events, updates the audit-store notification batching to manage RocksDB iterators, and adds comprehensive tests for late-patch and late-commit scenarios. Feedback was provided regarding the need to explicitly close RocksDB iterators to prevent resource leaks and the addition of null-entry guards when iterating over audit references.
| if (auditStore.reusableIterable && !databaseSubscriptions.activeCount && !databaseSubscriptions.notifyScheduled) { | ||
| // with rocksdb-js iterator we can and should not specify a start time so we just start at the end of the txn log | ||
| // and still match older version numbers that may commit in the future. But we have to start | ||
| // immediately so we are at the right position. | ||
| databaseSubscriptions.auditLogIterator = auditStore.getRange({}); | ||
| } |
There was a problem hiding this comment.
When recreating the reusable RocksDB iterator (auditLogIterator), any previously opened iterator on databaseSubscriptions should be explicitly closed to prevent leaking native RocksDB iterators and snapshots. Open snapshots pin old versions of data in memory/SSTs, which can prevent compaction from reclaiming deleted space and lead to write stalls.
| if (auditStore.reusableIterable && !databaseSubscriptions.activeCount && !databaseSubscriptions.notifyScheduled) { | |
| // with rocksdb-js iterator we can and should not specify a start time so we just start at the end of the txn log | |
| // and still match older version numbers that may commit in the future. But we have to start | |
| // immediately so we are at the right position. | |
| databaseSubscriptions.auditLogIterator = auditStore.getRange({}); | |
| } | |
| if (auditStore.reusableIterable && !databaseSubscriptions.activeCount && !databaseSubscriptions.notifyScheduled) { | |
| if (databaseSubscriptions.auditLogIterator?.close) { | |
| databaseSubscriptions.auditLogIterator.close(); | |
| } | |
| databaseSubscriptions.auditLogIterator = auditStore.getRange({}); | |
| } |
References
- When opening multiple database handles, only track pre-loop handles (the leak fix targets) in a cleanup array, and ensure that any per-iteration handles are closed immediately within their own try/finally blocks to prevent resource exhaustion.
| entry.additionalAuditRefs?.some( | ||
| (ref: { version: number; nodeId?: number }) => | ||
| ref.version === txnLogKey && (ref.nodeId ?? 0) === (nodeId ?? 0) | ||
| ) |
There was a problem hiding this comment.
When iterating over array elements that could potentially be null or undefined, include a null-entry guard to prevent runtime TypeErrors.
entry.additionalAuditRefs?.some(
(ref: { version: number; nodeId?: number }) => {
if (!ref) return false;
return ref.version === txnLogKey && (ref.nodeId ?? 0) === (nodeId ?? 0);
}
)References
- When iterating over array elements that could potentially be null or undefined, include a null-entry guard (e.g.,
if (!entry) continue;) to prevent runtime TypeErrors.
TODO: set the @harperfast/rocksdb-js version in package.json and package-lock.json to the release containing HarperFast/rocksdb-js#897, #901 and #902 once it is published. The subscriber fixes this bump relies on land separately in kris/subscriber-late-commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e0cdb71 to
1e1915f
Compare
Prerequisite: Deliver subscription events for commits that finish out of timestamp order (harper#3029). Merge it first. With #902's concurrent commit threads, commits to one database routinely finish out of timestamp order, and #3029 fixes the subscription paths that dropped such commits. Those fixes have moved there from this PR, so this PR no longer closes #3024 or #2933.
⊙ Problem
Harper pins
@harperfast/rocksdb-js2.10.0. On it, a database's async commits run on one commit thread, which caps commit throughput at one core of work. In rocksdb-js #898's measurements, four workers committing 64-key transactions with transaction-log entries got 4.7k commits/s on that thread, against 9.7k/s with the concurrent commit threads in #902.💡 Solution
Bump the dependency once it is released. No Harper code change is needed for the bump itself.
✅ Verification
Pending the release. I ran Harper's resources suite locally against a build of the #902 branch, with the #3029 fixes, and it passed apart from three
sourceApplyConflictRetry.test.jsfailures that also fail on unmodifiedmainon this machine. CI on the bumped version is the remaining check.🤖 Generated with Claude Code (Claude Opus 5.5); posted via @kriszyp.
Related PRs: #3029 overlaps (prerequisite; holds the subscriber fixes split out of this PR)
Complexity: medium