Fix FetcherService retrying with multiple fetchers on 429 and other application errors (#282100)

* Initial plan

* Fix FetcherService to not retry with other fetchers on 429 status code

Co-authored-by: TylerLeonhardt <2644648+TylerLeonhardt@users.noreply.github.com>

* Improve test robustness based on code review feedback

Co-authored-by: TylerLeonhardt <2644648+TylerLeonhardt@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: TylerLeonhardt <2644648+TylerLeonhardt@users.noreply.github.com>

extensions/github-authentication/src/node/fetch.ts

@@ -76,6 +76,16 @@ export function createFetch(): Fetch {
 	};
 }
 
+function shouldNotRetry(status: number): boolean {
+	// Don't retry with other fetchers for these HTTP status codes:
+	// - 429 Too Many Requests (rate limiting)
+	// - 401 Unauthorized (authentication issue)
+	// - 403 Forbidden (authorization issue)
+	// - 404 Not Found (resource doesn't exist)
+	// These are application-level errors where retrying with a different fetcher won't help
+	return status === 429 || status === 401 || status === 403 || status === 404;
+}
+
 async function fetchWithFallbacks(availableFetchers: readonly Fetcher[], url: string, options: FetchOptions, logService: Log): Promise<{ response: FetchResponse; updatedFetchers?: Fetcher[] }> {
 	if (options.retryFallbacks && availableFetchers.length > 1) {
 		let firstResult: { ok: boolean; response: FetchResponse } | { ok: false; err: any } | undefined;
@@ -85,6 +95,11 @@ async function fetchWithFallbacks(availableFetchers: readonly Fetcher[], url: st
 				firstResult = result;
 			}
 			if (!result.ok) {
+				// For certain HTTP status codes, don't retry with other fetchers
+				// These are application-level errors, not network-level errors
+				if ('response' in result && shouldNotRetry(result.response.status)) {
+					return { response: result.response };
+				}
 				continue;
 			}
 			if (fetcher !== availableFetchers[0]) {

extensions/github-authentication/src/test/node/fetch.test.ts

@@ -144,4 +144,46 @@ suite('fetching', () => {
 		assert.strictEqual(res.status, 200);
 		assert.deepStrictEqual(await res.text(), 'Hello, world!');
 	});
+
+	test('should not retry with other fetchers on 429 status', async () => {
+		// Set up server to return 429 for the first request
+		let requestCount = 0;
+		const oldListener = server.listeners('request')[0] as (req: http.IncomingMessage, res: http.ServerResponse) => void;
+		if (!oldListener) {
+			throw new Error('No request listener found on server');
+		}
+		
+		server.removeAllListeners('request');
+		server.on('request', (req, res) => {
+			requestCount++;
+			if (req.url === '/rate-limited') {
+				res.writeHead(429, {
+					'Content-Type': 'text/plain',
+					'X-Client-User-Agent': String(req.headers['user-agent'] ?? '').toLowerCase(),
+				});
+				res.end('Too Many Requests');
+			} else {
+				oldListener(req, res);
+			}
+		});
+
+		try {
+			const res = await createFetch()(`http://localhost:${port}/rate-limited`, {
+				logger,
+				retryFallbacks: true,
+				expectJSON: false,
+			});
+
+			// Verify only one request was made (no fallback attempts)
+			assert.strictEqual(requestCount, 1, 'Should only make one request for 429 status');
+			assert.strictEqual(res.status, 429);
+			// Note: We only check that we got a response, not which fetcher was used,
+			// as the fetcher order may vary by configuration
+			assert.strictEqual(await res.text(), 'Too Many Requests');
+		} finally {
+			// Restore original listener
+			server.removeAllListeners('request');
+			server.on('request', oldListener);
+		}
+	});
 });