Heap-based expiry for unauthorized cooldown and IndexedDB rapid user-switch race.#51
Merged
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a min-heap to optimize the cleanup of expired unauthorized access cooldowns in the realtime server and refactors the IndexedDB service in the web client to safely handle rapid user switches. Feedback indicates that triggering heap rebuilds during the connection handshake could introduce latency spikes and potential DoS vulnerabilities; it is recommended to move this logic to the background cleanup process.
Collaborator
Author
|
The DoS argument from above comments doesn't seem to hold up but the suggestion to remove the calls is still correct because they're redundant! |
Previously, cleanupExpiredUnauthorizedCooldown scanned the entire unauthorizedAccessCooldown map on every run. This was O(n) even if nothing had expired. Maintain a min-heap (unauthorizedCooldownExpirations) sorted by denyUntil. Cleanup now peeks at the heap root and stops if the earliest expiry has not passed, skipping the full scan. Process at most 512 entries per run to avoid blocking. When a cooldown is renewed (same key, new denyUntil), a second entry is pushed to the heap. Stale entries surface later; compare existingState.denyUntil with the expired entry and skip mismatches to avoid incorrect eviction. maybeRebuildUnauthorizedCooldownExpirationHeap handles heap bloat from stale entries by rebuilding from the source map when the heap exceeds 4x the map size and is above 1024 entries.
Previously, switching users rapidly could cause a race: setUserId only closed the DB if no close was in progress. Rapid switches (A → B → C → B) skipped closures, and a subsequent getDB could open the wrong database. Replace dbClosingPromise with a chained dbSwitchPromise. Each setUserId appends closeCurrentConnection to the chain so switches are serialized and none are skipped. awaitStableUserSwitch loops until the promise reference stabilizes, handling switches queued while awaiting.
e367d93 to
e8dcac6
Compare
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.
realtime/src/server.ts— Heap-based expiry for unauthorized cooldown:Previously,
cleanupExpiredUnauthorizedCooldowndid a full scan of the entireunauthorizedAccessCooldownmap on every run. This was O(n) even if nothing had expired.The new code maintains a min-heap (
unauthorizedCooldownExpirations) sorted bydenyUntil. Cleanup now peeks at the heap root and stops immediately if the earliest expiry hasn't passed yet — skipping the full scan. The heap processes at most 512 entries per cleanup run to avoid blocking.The subtle correctness issue it handles: when a cooldown is renewed (same key, new
denyUntil), a second entry is pushed onto the heap rather than updating the existing one. The stale entry from before the renewal will eventually surface during cleanup. The checkexistingState.denyUntil !== expiredEntry.denyUntildetects and skips those stale entries, preventing a renewed cooldown from being incorrectly evicted.maybeRebuildUnauthorizedCooldownExpirationHeaphandles heap bloat from accumulated stale entries by doing a full rebuild from the source-of-truth map when the heap is more than 4× the map size (and above 1024 entries).web/services/indexed-db.service.ts— Fix race condition in user switches:Previously, switching users rapidly could cause a race:
setUserIdonly closed the DB if no close was already in progress (!this.dbClosingPromise). Rapid switches (A → B → C → B) would skip closure steps, and a subsequentgetDBcall might await a staledbClosingPromiseand then open the wrong database.The fix replaces the single
dbClosingPromisewith a chained promise (dbSwitchPromise). Each call tosetUserIdappends acloseCurrentConnectionstep onto the chain — so no switch is ever skipped, they're always serialized.awaitStableUserSwitchloops until the promise reference stabilises, handling the case where another switch was queued while you were awaiting.