Skip to content

Fix unbounded Queue growth when the queue never drains - #1235

Open
Evil0ctal wants to merge 2 commits into
porsager:masterfrom
Evil0ctal:fix/queue-unbounded-growth
Open

Evil0ctal wants to merge 2 commits into
porsager:masterfrom
Evil0ctal:fix/queue-unbounded-growth

Conversation

@Evil0ctal

Copy link
Copy Markdown

Fixes #747

Queue.shift() only resets its backing array when the queue drains completely. The pool's open queue never drains while two or more connections sit idle in it: a connection is taken with shift() and pushed back when its query finishes, so every query leaves one dead slot behind, and remove() scanned all of them from index 0. Connection hand-off therefore gets slower with every query served since open last drained — the "gets slower until restart" in #747, and why it only shows up once a burst has opened more than one connection. The same pattern grows busy under load, the pending queries backlog, and each connection's sent queue.

The fix is in src/queue.js only:

  • shift() compacts once the dead prefix is at least as long as the live entries (and longer than 64, so small queues aren't reallocated on every call). Each compaction copies no more elements than were shifted since the last one, so shift() stays O(1) amortized.
  • remove() searches from the cursor. Slots below it are always undefined, so for any defined value the result is unchanged; only the scan gets shorter.

Keeping c.queue in sync in index.js would stop remove() from missing, but not the growth — sent is never remove()d, and the backlog grows the same way — so the callers are untouched.

  • The first commit adds a test that fails on 3.4.9 with '3,,b,a,1,c,0,d,e,true' != '3,,b,a,1,c,0,d,e,10003'. The backing array has no public observable and the slowdown only becomes measurable past ~10^5 queries, so it's a Queue unit test rather than a database test: it passes an Array subclass as initial, which .slice() keeps via Symbol.species (as Result already relies on), so its push sees the backing array's size. It also checks a remove() miss, a non-head hit, and a hit at the cursor after a drain. No timing assertions; it runs in a few ms.
  • The second commit is the fix.
  • The full suite passes on ESM, CJS and Deno (265/265 each) against PostgreSQL 17 in a container set up like the CI workflow, including the second cluster on 5433. The new test also passes on Node 12, 14, 16, 18, 20, 21, 22, 23 and 24 (ESM and CJS). eslint src tests is clean.
  • The patched Queue was differentially fuzzed against 3.4.9 over ~124M operations, biased toward the compaction boundary, with no divergence in any push, shift, remove or length result.

End to end, max: 10, 500k select 1 after a warm-up burst:

open backing array µs per query, first → last 50k
3.4.9 500,010 30 → 123
this PR ≤ 75 25 → 24

🤖 Generated with Claude Code

https://claude.ai/code/session_01BMNuLV7ceCupEDQmQFNADT

Evil0ctal and others added 2 commits September 29, 2026 16:49
Queue.shift() only resets its backing array when the queue drains
completely at that instant. A queue that is shifted and pushed without
ever emptying keeps one dead slot per cycle, so the array grows without
bound and remove() scans all of it. The pool's `open` queue is in that
state whenever more than one connection is idle.

The test observes the backing array through an Array subclass passed as
`initial`: Queue copies it with .slice(), which keeps the subclass via
Symbol.species. It rotates three entries so compaction has to preserve
order across several live entries, then checks a remove() miss, a hit
on a non-head entry, and a hit at the cursor after a full drain. No
timing, no database.

Fails on 3.4.9 with
'3,,b,a,1,c,0,d,e,true' != '3,,b,a,1,c,0,d,e,10003'.

Refs porsager#747

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BMNuLV7ceCupEDQmQFNADT
Queue.shift() advances a read cursor and only resets the backing array
when the cursor reaches the end, i.e. when the queue drains completely.
The pool's `open` queue never drains while two or more connections sit
idle in it, which is the steady state with max > 1 once any burst has
opened a second connection (the porsager#747 repro: -c 10, then -c 5). A
connection is taken with shift() and later pushed back, so every query
leaves one dead slot behind. The same happens to `busy` under load, the
pending `queries` backlog, and each connection's `sent` queue.

remove() then scanned the whole array from index 0, so connection
hand-off cost grew linearly with the number of queries served since
`open` last drained. In production this reached 23.8M slots in three
weeks and pinned a CPU core.

- shift() compacts once the dead prefix is at least as long as the live
  entries (and longer than 64, so small queues are not reallocated on
  every call). Each compaction copies no more elements than were shifted
  since the last one, so shift() stays O(1) amortized.
- remove() searches from the cursor. Slots below it are always
  undefined, so for any x other than undefined -- the pool only removes
  connections and queries -- the result is unchanged, and the scan covers
  only live entries.

The fix belongs in Queue rather than the callers: keeping c.queue in
sync would stop remove() from missing, but not the dead-slot growth,
which also hits `sent` (never remove()d) and the `queries` backlog.

Fixes porsager#747

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BMNuLV7ceCupEDQmQFNADT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A weird issue: speed dropped

1 participant