From eadeaf228267be215a2e9486f16b876d703dcccc Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Fri, 18 Sep 2026 10:54:46 -0700 Subject: [PATCH] fix(bitbucket): preserve rate limits from optional PR reads (#12486) --- .../BitbucketPullRequestProvider.test.ts | 81 ++++++++++++++++++- .../BitbucketPullRequestProvider.ts | 42 ++++++++-- .../src/sourceControl/BitbucketApi.test.ts | 24 ++++++ apps/server/src/sourceControl/BitbucketApi.ts | 5 +- 4 files changed, 142 insertions(+), 10 deletions(-) diff --git a/apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts b/apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts index db06a360a55c7fb440535ac72a8fc76200fac28b..f233880fd95d5722aaa00c26fb3a549e798afb53 100644 --- a/apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts @@ -1,11 +1,90 @@ -import { describe, expect, it } from "vite-plus/test"; +import { describe, expect, it } from "@effect/vitest"; +import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; +import * as Result from "effect/Result"; import * as BitbucketApi from "../sourceControl/BitbucketApi.ts"; +import * as BitbucketPullRequestApi from "./BitbucketPullRequestApi.ts"; +import { decodePullRequestJson } from "./bitbucketPullRequestJson.ts"; import { bitbucketProviderFailure, bitbucketViewerPermissions, + make, } from "./BitbucketPullRequestProvider.ts"; +for (const operation of [ + "getMergeability", + "listChecks", + "getRepositoryPermission", + "listComments", + "listCommits", +] as const) { + it.effect.each(["response", "body read"])( + `preserves rate limits from ${operation} on %s errors while recovering other optional-read failures`, + (variant) => + Effect.gen(function* () { + const pullRequest = Result.getOrThrow( + decodePullRequestJson(`{ + "id": 1, "title": "Check polling", "state": "OPEN", + "source": { "branch": { "name": "feature" } }, + "destination": { "branch": { "name": "main" } }, + "created_on": "2026-09-16T00:00:00Z", + "updated_on": "2026-09-16T00:00:00Z", + "links": { "html": { "href": "https://bitbucket.org/acme/web/pull-requests/1" } } + }`), + ); + for (const status of [429, 403]) { + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(BitbucketPullRequestApi.BitbucketPullRequestApi)({ + getPullRequest: () => Effect.succeed(pullRequest), + getDiffStat: () => Effect.succeed({ additions: 0, deletions: 0, changedFiles: 0 }), + getMergeability: () => Effect.succeed("unknown" as const), + listChecks: () => Effect.succeed([]), + getRepositoryPermission: () => Effect.succeed(true), + listComments: () => Effect.succeed({ comments: [], threads: [], truncated: false }), + listCommits: () => Effect.succeed([]), + [operation]: () => + Effect.fail( + variant === "response" + ? new BitbucketApi.BitbucketResponseError({ + operation: "request", + status, + responseBodyLength: 0, + retryAt: 120_000, + }) + : new BitbucketApi.BitbucketResponseBodyReadError({ + operation: "request", + status, + cause: new Error("response stream failed"), + retryAt: 120_000, + }), + ), + }), + ), + ); + const reference = { + cwd: "/repo", + repository: "acme/web", + number: 1, + host: "bitbucket.org", + }; + const result = yield* operation === "listComments" || operation === "listCommits" + ? Effect.result(provider.getChangeRequestActivity(reference)) + : Effect.result(provider.getChangeRequest(reference)); + if (status === 429) { + expect(result).toMatchObject({ + _tag: "Failure", + failure: { reason: "rate-limited", retryAt: 120_000 }, + }); + } else { + expect(result._tag).toBe("Success"); + } + } + }), + ); +} + describe("bitbucketProviderFailure", () => { it("treats only an HTTP 401 as unusable credentials", () => { const responseError = (status: number) => diff --git a/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts b/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts index b9fe9d669f4aec1f15438cc534f58af8f90cabd1..634519d467b9c1495986be9bfbba77532681179c 100644 --- a/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts +++ b/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts @@ -73,7 +73,10 @@ export function bitbucketProviderFailure( if (error._tag === "BitbucketResponseError" && error.status === 401) { return { reason: "unauthenticated" }; } - if (error._tag === "BitbucketResponseError" && error.status === 429) { + if ( + (error._tag === "BitbucketResponseError" || error._tag === "BitbucketResponseBodyReadError") && + error.status === 429 + ) { return { reason: "rate-limited", ...(error.retryAt === undefined ? {} : { retryAt: error.retryAt }), @@ -122,6 +125,31 @@ export const make = Effect.gen(function* () { cause: error, }); + const recoverRead = ( + read: Effect.Effect, + fallback: A, + ) => { + const recover = () => Effect.succeed(fallback); + return Effect.catchTags(read, { + BitbucketResponseError: (error) => (error.status === 429 ? Effect.fail(error) : recover()), + BitbucketUntrustedUrlError: recover, + BitbucketRepositoryLocatorError: recover, + BitbucketRequestError: recover, + BitbucketResponseBodyReadError: (error) => + error.status === 429 ? Effect.fail(error) : recover(), + BitbucketResponseDecodeError: recover, + BitbucketRepositoryVcsResolveError: recover, + BitbucketRepositoryRemotesListError: recover, + BitbucketRepositoryRemoteNotFoundError: recover, + BitbucketPullRequestBodyReadError: recover, + BitbucketCheckoutError: recover, + BitbucketPullRequestReadError: recover, + BitbucketViewerUnavailableError: recover, + BitbucketRepositoryUnsupportedError: recover, + BitbucketDiffCommitError: recover, + }); + }; + const provider: PullRequestProviderApi = { kind: "bitbucket", capabilities: CAPABILITIES, @@ -156,12 +184,12 @@ export const make = Effect.gen(function* () { [ api.getPullRequest(target), api.getDiffStat(target), - api.getMergeability(target).pipe(Effect.orElseSucceed(() => "unknown" as const)), - api.listChecks(target).pipe(Effect.orElseSucceed(() => [])), + recoverRead(api.getMergeability(target), "unknown" as const), + recoverRead(api.listChecks(target), []), // A permission that could not be read is an unknown one, which is granted: a hidden // Merge leaves someone entitled to it with no way through, and one Bitbucket refuses // at least says why. - api.getRepositoryPermission(target).pipe(Effect.orElseSucceed(() => true)), + recoverRead(api.getRepositoryPermission(target), true), ], { concurrency: 5 }, ).pipe( @@ -200,10 +228,8 @@ export const make = Effect.gen(function* () { // Reviews ride on the pull request itself, so this inexpensive core read is repeated // here rather than making the core response wait for the conversation endpoints. api.getPullRequest(target), - api - .listComments(target) - .pipe(Effect.orElseSucceed(() => ({ comments: [], threads: [], truncated: true }))), - api.listCommits(target).pipe(Effect.orElseSucceed(() => [])), + recoverRead(api.listComments(target), { comments: [], threads: [], truncated: true }), + recoverRead(api.listCommits(target), []), ], { concurrency: 3 }, ).pipe( diff --git a/apps/server/src/sourceControl/BitbucketApi.test.ts b/apps/server/src/sourceControl/BitbucketApi.test.ts index 856d043a7fce009af63d5a9b8c5fe6458aa146ed..b27d56bef3996feb572810aae88cb7fcf6efaa86 100644 --- a/apps/server/src/sourceControl/BitbucketApi.test.ts +++ b/apps/server/src/sourceControl/BitbucketApi.test.ts @@ -618,6 +618,30 @@ it.effect("preserves Bitbucket response body read failures as their immediate ca }).pipe(Effect.provide(layer)); }); +it.effect("keeps the 429 retry time when the response body cannot be read", () => { + const { layer } = makeLayer({ + response: () => + new Response( + new ReadableStream({ + start: (controller) => controller.error(new Error("response stream failed")), + }), + { status: 429, headers: { "Retry-After": "120" } }, + ), + }); + + return Effect.gen(function* () { + yield* TestClock.setTime(1_000); + const bitbucket = yield* BitbucketApi.BitbucketApi; + const error = yield* bitbucket + .request({ method: "GET", url: "/repositories/acme/web" }) + .pipe(Effect.flip); + + assert.instanceOf(error, BitbucketApi.BitbucketResponseBodyReadError); + assert.strictEqual(error.status, 429); + assert.strictEqual(error.retryAt, 121_000); + }).pipe(Effect.provide(layer)); +}); + it.effect("checks out same-repository pull requests with the existing Bitbucket remote", () => { const { git, layer } = makeLayer({ response: () => diff --git a/apps/server/src/sourceControl/BitbucketApi.ts b/apps/server/src/sourceControl/BitbucketApi.ts index 7bac33a30be16c45a317a7896def508914fa98a3..202740680be8252f797e2745d415a69615c8c5b8 100644 --- a/apps/server/src/sourceControl/BitbucketApi.ts +++ b/apps/server/src/sourceControl/BitbucketApi.ts @@ -117,6 +117,7 @@ export class BitbucketResponseBodyReadError extends Schema.TaggedError