Resubmit #4381 (#4395)

* Add new rate limiter

* Fix for service injections

* Apply suggestions from code review

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Sam Cutler <itsibitzi@github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Author: 3 people

src/platform/github/common/githubApiFetcherService.ts

@@ -8,7 +8,7 @@ import { CallTracker } from '../../../util/common/telemetryCorrelationId';
 import { raceCancellationError } from '../../../util/vs/base/common/async';
 import { CancellationToken } from '../../../util/vs/base/common/cancellation';
 import { CancellationError, isCancellationError } from '../../../util/vs/base/common/errors';
-import { Disposable, dispose, IDisposable } from '../../../util/vs/base/common/lifecycle';
+import { Disposable, IDisposable } from '../../../util/vs/base/common/lifecycle';
 import { IEnvService } from '../../env/common/envService';
 import { ILogService } from '../../log/common/logService';
 import { ITelemetryService } from '../../telemetry/common/telemetry';
@@ -37,6 +37,7 @@ export interface GithubRequestOptions {
 export const githubHeaders = Object.freeze({
 	requestId: 'x-github-request-id',
 	totalQuotaUsed: 'x-github-total-quota-used',
+	quotaBucketName: 'x-github-quota-bucket-name',
 });
 
 /**
@@ -53,24 +54,16 @@ export interface IGithubApiFetcherService extends IDisposable {
  * If inserts are infrequent, the minimum-entry guarantee ensures there is always
  * some history to work with; when inserts are frequent the time window dominates.
  */
-class SlidingTimeAndNWindow implements IDisposable {
+class SlidingTimeAndNWindow {
 	private values: number[] = [];
 	private times: number[] = [];
 	private sumValues = 0;
 	private readonly numEntries: number;
 	private readonly windowDurationMs: number;
-	private cleanupInterval: ReturnType<typeof setInterval> | undefined;
 
 	constructor(numEntries: number, windowDurationMs: number) {
 		this.numEntries = numEntries;
 		this.windowDurationMs = windowDurationMs;
-		this.startPeriodicCleanup();
-	}
-
-	dispose(): void {
-		if (typeof this.cleanupInterval !== 'undefined') {
-			clearInterval(this.cleanupInterval);
-		}
 	}
 
 	increment(n: number): void {
@@ -107,22 +100,25 @@ class SlidingTimeAndNWindow implements IDisposable {
 		this.sumValues = 0;
 	}
 
-	private startPeriodicCleanup(): void {
-		this.cleanupInterval = setInterval(() => {
-			const tooOldTime = Date.now() - this.windowDurationMs;
-			while (
-				this.times.length > this.numEntries &&
-				this.times[0] < tooOldTime
-			) {
-				this.sumValues -= this.values[0];
-				this.values.shift();
-				this.times.shift();
-			}
-		}, 100);
+	/**
+	 * Removes entries that are both outside the time window and exceed the
+	 * minimum entry count. Called explicitly before throttle decisions so
+	 * that the window reflects the current state.
+	 */
+	cleanUpOldValues(now: number): void {
+		const tooOldTime = now - this.windowDurationMs;
+		while (
+			this.times.length > this.numEntries &&
+			this.times[0] < tooOldTime
+		) {
+			this.sumValues -= this.values[0];
+			this.values.shift();
+			this.times.shift();
+		}
 	}
 }
 
-class Throttler implements IDisposable {
+class Throttler {
 	private readonly target: number;
 	private lastSendTime: number;
 	private totalQuotaUsedWindow: SlidingTimeAndNWindow;
@@ -139,8 +135,6 @@ class Throttler implements IDisposable {
 	reset(): void {
 		if (this.numOutstandingRequests === 0) {
 			this.lastSendTime = Date.now();
-			this.totalQuotaUsedWindow.dispose();
-			this.sendPeriodWindow.dispose();
 			this.totalQuotaUsedWindow = new SlidingTimeAndNWindow(5, 2000);
 			this.sendPeriodWindow = new SlidingTimeAndNWindow(5, 2000);
 		}
@@ -163,9 +157,7 @@ class Throttler implements IDisposable {
 	 * sent right now or deferred. It uses sliding windows of recent quota
 	 * usage and send periods to compute proportional, integral, and
 	 * differential terms, which in turn determine a dynamic delay before
-	 * sending the next request. The ramp-up logic at the end ensures we
-	 * start slowly and calibrate based on server feedback before allowing
-	 * higher concurrency.
+	 * sending the next request.
 	 */
 	shouldSendRequest(): boolean {
 		const now = Date.now();
@@ -175,13 +167,24 @@ class Throttler implements IDisposable {
 			this.reset();
 		}
 
-		let shouldSend = false;
+		this.totalQuotaUsedWindow.cleanUpOldValues(now);
+		this.sendPeriodWindow.cleanUpOldValues(now);
 
-		if (this.totalQuotaUsedWindow.get() === 0) {
-			shouldSend = true;
+		// Ramp up slowly at start so the throttler can calibrate based on
+		// server feedback before allowing concurrent requests.
+		if (
+			this.totalQuotaUsedWindow.size() < 5 &&
+			this.numOutstandingRequests > 0
+		) {
+			return false;
 		}
 
-		if (this.sendPeriodWindow.average() > 0) {
+		let shouldSend = false;
+
+		// If there have been no requests, send one.
+		if (this.totalQuotaUsedWindow.get() === 0 || this.sendPeriodWindow.size() === 0) {
+			shouldSend = true;
+		} else if (this.sendPeriodWindow.average() > 0) {
 			const integral =
 				(this.totalQuotaUsedWindow.average() - this.target) / 100;
 			const differential = this.totalQuotaUsedWindow.delta();
@@ -193,35 +196,28 @@ class Throttler implements IDisposable {
 			}
 		}
 
-		// Ramp up slowly at start so the throttler can calibrate based on
-		// server feedback before allowing concurrent requests.
-		if (
-			this.totalQuotaUsedWindow.size() < 5 &&
-			this.numOutstandingRequests > 0
-		) {
-			shouldSend = false;
-		}
-
 		if (shouldSend) {
 			this.sendPeriodWindow.increment(now - this.lastSendTime);
 			this.lastSendTime = now;
 		}
 		return shouldSend;
 	}
-
-	dispose(): void {
-		this.totalQuotaUsedWindow.dispose();
-		this.sendPeriodWindow.dispose();
-	}
 }
 
 export class GithubApiFetcherService extends Disposable implements IGithubApiFetcherService {
 	declare readonly _serviceBrand: undefined;
 
+	/**
+	 * The target percentage usage of each throttler. Higher is faster but too close to 100 and you
+	 * can have queries rejected
+	 */
+	private readonly throttlerTarget = 80;
+	/** Quota-bucket name → {@link Throttler} that governs requests in that bucket. */
 	private readonly throttlers = new Map<string, Throttler>();
+	/** `"METHOD url"` → quota-bucket name, learned from response headers. */
+	private readonly endpointBuckets = new Map<string, string>();
 
 	constructor(
-		private readonly throttlerTarget: number = 80,
 		@IEnvService private readonly envService: IEnvService,
 		@ILogService private readonly logService: ILogService,
 		@ITelemetryService private readonly telemetryService: ITelemetryService,
@@ -230,9 +226,50 @@ export class GithubApiFetcherService extends Disposable implements IGithubApiFet
 	}
 
 	override dispose(): void {
-		super.dispose();
-		dispose(this.throttlers.values());
 		this.throttlers.clear();
+		this.endpointBuckets.clear();
+		super.dispose();
+	}
+
+	/**
+	 * Computes a normalized key for an endpoint, combining HTTP method and URL
+	 * pathname. This avoids fragmenting throttling state across different query
+	 * strings for the same logical endpoint.
+	 */
+	private getEndpointKey(method: string, url: string): string {
+		try {
+			const parsed = new URL(url);
+			return `${method} ${parsed.pathname}`;
+		} catch {
+			// Fall back to the raw URL if it cannot be parsed (e.g. relative URL),
+			// preserving existing behavior in those cases.
+			return `${method} ${url}`;
+		}
+	}
+
+	/**
+	 * Returns the throttler for a given endpoint (method + pathname) by looking
+	 * up its quota bucket. Returns `undefined` for endpoints whose bucket is not
+	 * yet known (i.e. no prior response has provided the bucket header).
+	 */
+	private getThrottlerForEndpoint(method: string, url: string): Throttler | undefined {
+		const endpointKey = this.getEndpointKey(method, url);
+		const bucket = this.endpointBuckets.get(endpointKey);
+		return bucket ? this.throttlers.get(bucket) : undefined;
+	}
+
+	/**
+	 * Updates the endpoint → quota-bucket and bucket → throttler mappings from
+	 * response headers. Creates a new throttler on demand when a bucket is seen
+	 * for the first time.
+	 */
+	private updateThrottlers(method: string, url: string, bucket: string, quotaUsed: number): void {
+		if (!this.throttlers.has(bucket)) {
+			this.throttlers.set(bucket, new Throttler(this.throttlerTarget));
+		}
+		this.throttlers.get(bucket)!.recordQuotaUsed(quotaUsed);
+		const endpointKey = this.getEndpointKey(method, url);
+		this.endpointBuckets.set(endpointKey, bucket);
 	}
 
 	async makeRequest(options: GithubRequestOptions, token: CancellationToken): Promise<Response> {
@@ -245,17 +282,18 @@ export class GithubApiFetcherService extends Disposable implements IGithubApiFet
 		retriesOn500Remaining: number,
 		retryOnRateLimitedRemaining: number,
 	): Promise<Response> {
-		const throttler = this.getThrottler(options.url);
-
-		// Throttle
-		while (!throttler.shouldSendRequest()) {
-			await raceCancellationError(sleep(5), token);
-		}
-		if (token.isCancellationRequested) {
-			throw new CancellationError();
+		// Throttle based on the URL's quota bucket (if known from prior responses)
+		const throttler = this.getThrottlerForEndpoint(options.method, options.url);
+		if (throttler) {
+			while (!throttler.shouldSendRequest()) {
+				await raceCancellationError(sleep(5), token);
+			}
+			if (token.isCancellationRequested) {
+				throw new CancellationError();
+			}
 		}
 
-		throttler.requestStarted();
+		throttler?.requestStarted();
 		try {
 			const res = await fetch(options.url, {
 				method: options.method,
@@ -268,10 +306,23 @@ export class GithubApiFetcherService extends Disposable implements IGithubApiFet
 			});
 
 			// Record quota usage for throttle calibration
+			// Record quota usage for throttle calibration, keyed by bucket. If the bucket name is not in the headers use a
+			// fake __global__ bucket.
+			const bucketNameHeader = res.headers.get(githubHeaders.quotaBucketName);
+			const bucketName = bucketNameHeader || '__global__';
 			const quotaUsedHeader = res.headers.get(githubHeaders.totalQuotaUsed);
-			const quotaUsed = quotaUsedHeader ? parseFloat(quotaUsedHeader) : 0;
-			if (quotaUsed > 0) {
-				throttler.recordQuotaUsed(quotaUsed);
+
+			// Learn the endpoint → bucket mapping whenever we have a bucket header, even if quota-used is missing.
+			if (bucketNameHeader && quotaUsedHeader === null) {
+				this.updateThrottlers(options.method, options.url, bucketName, 0);
+			}
+
+			// Only record quota usage when the parsed value is finite and greater than zero.
+			if (quotaUsedHeader !== null) {
+				const quotaUsed = parseFloat(quotaUsedHeader);
+				if (Number.isFinite(quotaUsed) && quotaUsed > 0) {
+					this.updateThrottlers(options.method, options.url, bucketName, quotaUsed);
+				}
 			}
 
 			if (!res.ok) {
@@ -333,20 +384,9 @@ export class GithubApiFetcherService extends Disposable implements IGithubApiFet
 			}
 			throw e;
 		} finally {
-			throttler.requestFinished();
+			throttler?.requestFinished();
 		}
 	}
-
-	private getThrottler(urlId: string): Throttler {
-		const existingThrottler = this.throttlers.get(urlId);
-		if (existingThrottler) {
-			return existingThrottler;
-		}
-
-		const throttler = new Throttler(this.throttlerTarget);
-		this.throttlers.set(urlId, throttler);
-		return throttler;
-	}
 }
 
 async function sleep(ms: number): Promise<void> {