Bound queue check cooldowns - #23
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| this.connectionStartedAt = performance.now(); | ||
| void this.enqueueStartup(epoch); | ||
| } else { | ||
| queueCheckQueue.cancelCooldown(); |
There was a problem hiding this comment.
🟡 One database losing its connection removes the pacing delay for all other databases' queue checks
The shared pacing delay that keeps queue checks from running back-to-back is wiped and woken up for every database whenever any single database drops its connection (queueCheckQueue.cancelCooldown() at src/firelease.ts:705), so unrelated healthy queues immediately run their next check with no throttling.
Impact: During connection churn, expensive queue size probes and listener reloads for other, still-healthy queues can run continuously instead of at the intended 50% duty cycle, adding load exactly when the database is already struggling.
Mechanism: a single global QueueCheckQueue is shared by all sources
queueCheckQueue is a module-level singleton (src/firelease.ts:579) used by every QueueSource. cancelCooldown() (src/firelease.ts:512-520) zeroes previousDuration/previousFinishedAt and resolves the in-flight cooldown promise unconditionally, without checking which source the currently-awaited job belongs to. The awakened job in drain() (src/firelease.ts:536-543) proceeds straight to job.run() even if its source never disconnected, and the following jobs also see a zeroed minimumStart. A flapping source therefore repeatedly erases the duty-cycle debt accumulated by other sources.
A targeted fix would be to only skip/shorten the cooldown when the awaited job belongs to the disconnected source (or when its epoch is no longer current), rather than cancelling the shared cooldown outright.
Prompt for agents
QueueCheckQueue is a process-wide singleton shared by all QueueSources (src/firelease.ts:579). The new disconnect handling calls queueCheckQueue.cancelCooldown() from QueueSource.onConnection whenever any source disconnects, which zeroes previousDuration/previousFinishedAt and resolves the pending cooldown promise regardless of which source's job is actually being delayed. The intent is only to stop a disconnected source from being held back by pre-disconnect cooldown debt, but the effect is that a single flapping database removes duty-cycle throttling for every other healthy source, letting expensive childrenKeys probes and listener reloads run back-to-back. Consider making the cancellation conditional: only wake the cooldown when the job currently being delayed (or all queued jobs) is no longer current / belongs to the disconnected source, or track cooldown debt per source instead of globally.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch. I replaced disconnect-driven cancellation with a global 30-second cooldown cap. A flapping source now leaves the shared pacing debt intact, while an unexpectedly long check can delay the queue by at most 30 seconds. The regression test also verifies that disconnect/reconnect does not erase an existing cooldown.
| if (job.source.isCurrent(job.epoch)) { | ||
| this.previousDuration = finishedAt - start; | ||
| this.previousFinishedAt = finishedAt; | ||
| } |
There was a problem hiding this comment.
🔍 Suppressing cooldown debt for invalidated jobs also suppresses it for crashing sources
The new guard only records duty-cycle debt when job.source.isCurrent(job.epoch) holds. isCurrent (src/firelease.ts:757) returns false not only when the epoch changed but also when the source is disconnected or crashing. A long-running check that actually consumed real database work (e.g. a slow childrenKeys probe that ends with a crash or a mid-job disconnect) therefore contributes zero cooldown for the next job of a different, healthy source, which starts immediately. This matches the stated intent for reconnects, but the crashing-source case is a side effect worth confirming.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed that this side effect was undesirable as well. The guard is removed, so stale or crashing jobs once again contribute their actual runtime to pacing for the next source; the new 30-second cap bounds the resulting cooldown.
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
@pkaminski resolved 1 discussion.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion.
pkaminski
changed the title
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
@pkaminski+CODX made 2 comments and resolved 1 discussion.
Reviewable status: 0 of 3 files reviewed, all discussions resolved.
| if (job.source.isCurrent(job.epoch)) { | ||
| this.previousDuration = finishedAt - start; | ||
| this.previousFinishedAt = finishedAt; | ||
| } |
There was a problem hiding this comment.
Agreed that this side effect was undesirable as well. The guard is removed, so stale or crashing jobs once again contribute their actual runtime to pacing for the next source; the new 30-second cap bounds the resulting cooldown.
| this.connectionStartedAt = performance.now(); | ||
| void this.enqueueStartup(epoch); | ||
| } else { | ||
| queueCheckQueue.cancelCooldown(); |
There was a problem hiding this comment.
Good catch. I replaced disconnect-driven cancellation with a global 30-second cooldown cap. A flapping source now leaves the shared pacing debt intact, while an unexpectedly long check can delay the queue by at most 30 seconds. The regression test also verifies that disconnect/reconnect does not erase an existing cooldown.
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
+r:@snoack
@pkaminski made 1 comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on snoack).
snoack
left a comment
snoack
left a comment
There was a problem hiding this comment.
@snoack reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pkaminski).
2 participants
Summary
Why
The shared check queue enforces a maximum 50% duty cycle by delaying the next job for the duration of the previous one. An unexpectedly slow probe or listener load could therefore leave every queue source waiting for an unbounded period during reconnect startup.
Canceling that cooldown on disconnect was too broad because the check queue is process-global: one flapping Firebase database would erase pacing for unrelated healthy sources. Bounding the enforced delay instead preserves global throttling while ensuring that a single abnormal job cannot stall subsequent queue initialization for more than 30 seconds.
Impact
Normal checks continue to receive an equal-duration cooldown. Checks longer than 30 seconds receive a 30-second cooldown, and disconnects do not allow unrelated sources to bypass it. Queue load logs expose the complete connection duration, including scheduler wait, size probe, and listener loading time.
Validation
yarn testyarn lintyarn check-typesyarn buildyarn pack --dry-rungit diff --checkThis change is