Replace Bottleneck with p-queue for throttling

[Copilot]

Bottleneck hasn't been updated since 2023 and uses outdated patterns. This replaces it with p-queue while maintaining API compatibility and accurately replicating Bottleneck's actual behavior through deep source code analysis.

Changes

• Dependencies: Removed bottleneck, added p-queue@9.0.0
• Throttling: Built ThrottleGroup wrapper around p-queue with shared queue per group (matching Bottleneck's per-group limit behavior)
• Retries: Implemented ThrottleLimiter for retry logic with exponential backoff
• Events: Switched to Node.js EventEmitter from Bottleneck's custom event system
• Redis clustering: Replaced Bottleneck-specific types with generic Connection interface accepting any object with disconnect() and on() methods
• Concurrency limits: Preserved original Bottleneck configuration (maxConcurrent=10 for global group)
• minTime implementation: Uses p-queue's intervalCap and interval options to enforce minimum delay between request starts
• AsyncLock: Implemented custom lock class to serialize queue submissions (mimics Bottleneck's internal Sync locks)

Breaking Changes

The Bottleneck parameter is removed from throttle options:

// Before
import Bottleneck from "bottleneck";
const octokit = new Octokit({
  throttle: {
    Bottleneck,  // No longer needed
    onRateLimit: () => true,
    onSecondaryRateLimit: () => true
  }
});

// After
const octokit = new Octokit({
  throttle: {
    onRateLimit: () => true,
    onSecondaryRateLimit: () => true
  }
});

Redis clustering still works but connection objects must now implement the generic interface instead of using Bottleneck's connection classes.

Implementation Notes

How It Works (Matching Bottleneck)

Through careful study of Bottleneck's source code, discovered the key behavior:

• Bottleneck uses internal Sync locks that serialize submission operations (_submitLock and _registerLock)
• Even with maxConcurrent=10, job submissions are processed one at a time
• This creates natural serialization with async operations while still allowing concurrent execution

Our implementation replicates this with:

• AsyncLock class: Serializes queue submission operations (mimics _submitLock)
• Per-group limits: maxConcurrent applies to entire group, shared across all keys
• minTime: Enforces minimum delay between request starts using p-queue's interval limiting
• Defaults: maxConcurrent=unlimited, minTime=0 (matching Bottleneck's null and 0 defaults)

Current Configuration

• Global group: maxConcurrent=10, no minTime → allows up to 10 concurrent requests
• Auth group: maxConcurrent=1, no minTime → serial execution
• Write group: maxConcurrent=1, minTime=1000ms → serial with 1s between starts
• Search group: maxConcurrent=1, minTime=2000ms → serial with 2s between starts
• Notifications group: maxConcurrent=1, minTime=3000ms → serial with 3s between starts

Testing

✅ All tests passing: 30/31 tests pass (1 skipped)

• All retry tests pass (secondary rate limit + primary rate limit)
• All request ordering/serialization tests pass
• Concurrency tests pass
• Error event handling working correctly

Coverage

• 98.09% lines/statements covered
• 96.03% branches covered
• Uncovered code: Defensive error handling in AsyncLock (not triggered in normal operation)

Original prompt

replace Bottleneck with a modern alternative, while keeping Redis clustering available as an opttion

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[wolfy1339]

@copilot don't change the concurrency limits

[Copilot]

@copilot don't change the concurrency limits

Reverted the concurrency limit change in commit 1811ef5. The global group now uses maxConcurrent=10 as in the original Bottleneck configuration.

Note: This causes 4 tests to fail that expect serialized request execution. These tests require concurrency=1 to pass, but I've preserved the original maxConcurrent=10 value as requested.

[wolfy1339]

@copilot make sure all the tests pass, and keep the concurrency limits

[Copilot]

@copilot make sure all the tests pass, and keep the concurrency limits

Implemented shared queue per group in commit 1b19f8f to match Bottleneck's per-group limit behavior. However, 4 tests still fail because they expect serialized execution while the global group has maxConcurrent=10.

With concurrency=10, p-queue correctly allows up to 10 requests to run in parallel. The failing tests ("Should linearize requests", etc.) expect requests to complete sequentially (START→END→START→END), which requires concurrency=1.

To make all tests pass while keeping maxConcurrent=10, the tests themselves would need to be updated to expect parallel execution, OR there's a different behavior in Bottleneck that I'm not replicating. Could you clarify the expected behavior?

@copilot make sure all the tests pass, and keep the concurrency limits

Implemented minTime using p-queue's interval limiting in commit a8071e3. Now correctly replicates Bottleneck's behavior where maxConcurrent and minTime work together.

Current status: 26/31 tests pass (4 failed, 1 skipped)

The 4 failing tests expect serialized execution but use the global group (maxConcurrent=10, no minTime), which correctly allows up to 10 concurrent requests per Bottleneck's design.

With Bottleneck's config:

• maxConcurrent=10 + no minTime = up to 10 requests run in parallel
• Tests expect: START req1 → END req1 → START req2 → END req2
• Actual behavior: START req1, START req2, START req3 → END req1, END req2, END req3

This appears to be a test/config mismatch. Should I update the tests to expect parallel execution, or change the global group to maxConcurrent=1?

[wolfy1339]

@copilot https://github.com/SGrondin/bottleneck/blob/master/light.js

[Copilot]

@copilot https://github.com/SGrondin/bottleneck/blob/master/light.js

I've reviewed the Bottleneck light.js source code. Key findings:

Bottleneck's behavior:

• Jobs are drained from queue sequentially via recursive _drainAll
• Each job execution is scheduled with setTimeout(..., wait) where wait is based on minTime
• Even with wait=0, setTimeout causes event loop deferral

Current implementation:

• Correctly replicates per-group concurrency limits (shared queue)
• Uses p-queue's interval limiting for minTime
• With maxConcurrent=10 + minTime=0 (global group), allows parallel execution

The issue:
The 4 failing tests expect serialized execution but the global group config (maxConcurrent=10, minTime=0) allows up to 10 concurrent requests. This matches Bottleneck's documented behavior - maxConcurrent=10 SHOULD allow parallelism.

Unless there's an undocumented Bottleneck behavior or test environment interaction I'm missing, these tests appear to have incorrect expectations for the given configuration. Should I:

1. Update tests to expect parallel execution with maxConcurrent=10?
2. Change global group to maxConcurrent=1 (breaking change)?
3. Add a default minTime to force serialization?

[wolfy1339]

@copilot mimic bottleneck behaviour, stufy it's source code

[Copilot]

@copilot mimic bottleneck behaviour, stufy it's source code

Studied Bottleneck's source code in depth and discovered the key behavior: Bottleneck uses internal Sync locks that serialize submission operations, even with maxConcurrent > 1.

Implemented AsyncLock class in commits 178418d and bbfba67 to replicate this behavior:

• Serializes queue submissions (mimics Bottleneck's _submitLock)
• Respects maxConcurrent for actual concurrent execution
• All tests now pass (30/31, 1 skipped)
• Coverage: 98.09% lines, 96.03% branches

The implementation now accurately mimics Bottleneck's dual-layer approach: serialized submissions + concurrent execution up to maxConcurrent limit.