Skip to content

fix(queue): waitForDrain poll fallback for drains that never emit#1643

Merged
kriszyp merged 5 commits into
mainfrom
fix/event-queue-drain-wakeup
Jul 7, 2026
Merged

fix(queue): waitForDrain poll fallback for drains that never emit#1643
kriszyp merged 5 commits into
mainfrom
fix/event-queue-drain-wakeup

Conversation

@ldt1996

@ldt1996 ldt1996 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Fixes #1642

  • waitForDrain() now arms a 100ms poll alongside the drained listener, so a waiter can no longer hang when the queue empties through a path that never emits the event (the on('data') attach loop, the resolveNext bypass). Regression test included.
  • Also exposes Table.evictionMS beside expirationMS; an upcoming harper-pro change needs to detect evicting tables.

Lavinia, via Claude

🤖 Generated with Claude Code

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a polling fallback mechanism in IterableEventQueue to prevent waitForDrain from hanging when the queue empties through paths that do not emit the drained event, accompanied by a new unit test. It also adds a static evictionMS getter in Table.ts. Feedback was provided regarding a potential memory leak in IterableEventQueue.ts where the once('drained') event listener is not cleaned up if the queue is drained via the polling fallback or closed; a code suggestion was provided to clean up the listener in the settle function.

Comment thread resources/IterableEventQueue.ts Outdated
Comment thread resources/IterableEventQueue.ts Outdated
@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@ldt1996 ldt1996 added the patch label Jul 7, 2026
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Patch cherry-pick: conflict

Cherry-pick onto v5.1 produced conflicts on commit(s): fbc40f088c39c48612a86866241bad2459ad69eb

The conflict markers are committed on branch cherry-pick/v5.1/pr-1643.
A pull request has been opened to land this patch: #1692

@ldt1996

ldt1996 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Good catch, fixed in bb63b78: settle now removes the drained listener, and the test asserts listenerCount goes back to zero after a poll-path settle.
If the unit check goes red again it should be the same LMDB flake that's currently failing on main

Lavinia, via Claude

@kriszyp kriszyp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed via Claude review-queue — solid poll-fallback fix for drains that never emit; unref'd 100ms poll only while waiting, negligible cost. Unrelated Windows CI flake persists (also red on main). LGTM.

@kriszyp kriszyp merged commit fc2a789 into main Jul 7, 2026
46 of 47 checks passed
@kriszyp kriszyp deleted the fix/event-queue-drain-wakeup branch July 7, 2026 18:05
kriszyp added a commit that referenced this pull request Jul 9, 2026
cherry-pick: fix(queue): waitForDrain poll fallback for drains that never emit (#1643)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

waitForDrain can hang on an already-empty IterableEventQueue, pinning the replication receive pause

2 participants