authorgravatar for julius0216@outlook.comJulius Marminge <julius0216@outlook.com> 2026-09-18 10:54:46-07:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-09-18 10:54:46-07:00
logeadeaf228267be215a2e9486f16b876d703dcccc
tree2700f9b658dd6e54fbbca9e24e72437c212e0335
parentbd5dc58c65b87bfd2222e4697c4742b48e639ea4
signature Signed by PGP key B5690EEEBB952194

fix(bitbucket): preserve rate limits from optional PR reads (#12486)

4 files changed, 142 insertions(+), 10 deletions(-)

apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts+80-1
...@@ -1,11 +1,90 @@...@@ -1,11 +1,90 @@
1import { describe, expect, it } from "vite-plus/test";1import { describe, expect, it } from "@effect/vitest";
2import * as Effect from "effect/Effect";
3import * as Layer from "effect/Layer";
4import * as Result from "effect/Result";
25
3import * as BitbucketApi from "../sourceControl/BitbucketApi.ts";6import * as BitbucketApi from "../sourceControl/BitbucketApi.ts";
7import * as BitbucketPullRequestApi from "./BitbucketPullRequestApi.ts";
8import { decodePullRequestJson } from "./bitbucketPullRequestJson.ts";
4import {9import {
5 bitbucketProviderFailure,10 bitbucketProviderFailure,
6 bitbucketViewerPermissions,11 bitbucketViewerPermissions,
12 make,
7} from "./BitbucketPullRequestProvider.ts";13} from "./BitbucketPullRequestProvider.ts";
814
15for (const operation of [
16 "getMergeability",
17 "listChecks",
18 "getRepositoryPermission",
19 "listComments",
20 "listCommits",
21] as const) {
22 it.effect.each(["response", "body read"])(
23 `preserves rate limits from ${operation} on %s errors while recovering other optional-read failures`,
24 (variant) =>
25 Effect.gen(function* () {
26 const pullRequest = Result.getOrThrow(
27 decodePullRequestJson(`{
28 "id": 1, "title": "Check polling", "state": "OPEN",
29 "source": { "branch": { "name": "feature" } },
30 "destination": { "branch": { "name": "main" } },
31 "created_on": "2026-09-16T00:00:00Z",
32 "updated_on": "2026-09-16T00:00:00Z",
33 "links": { "html": { "href": "https://bitbucket.org/acme/web/pull-requests/1" } }
34 }`),
35 );
36 for (const status of [429, 403]) {
37 const provider = yield* make.pipe(
38 Effect.provide(
39 Layer.mock(BitbucketPullRequestApi.BitbucketPullRequestApi)({
40 getPullRequest: () => Effect.succeed(pullRequest),
41 getDiffStat: () => Effect.succeed({ additions: 0, deletions: 0, changedFiles: 0 }),
42 getMergeability: () => Effect.succeed("unknown" as const),
43 listChecks: () => Effect.succeed([]),
44 getRepositoryPermission: () => Effect.succeed(true),
45 listComments: () => Effect.succeed({ comments: [], threads: [], truncated: false }),
46 listCommits: () => Effect.succeed([]),
47 [operation]: () =>
48 Effect.fail(
49 variant === "response"
50 ? new BitbucketApi.BitbucketResponseError({
51 operation: "request",
52 status,
53 responseBodyLength: 0,
54 retryAt: 120_000,
55 })
56 : new BitbucketApi.BitbucketResponseBodyReadError({
57 operation: "request",
58 status,
59 cause: new Error("response stream failed"),
60 retryAt: 120_000,
61 }),
62 ),
63 }),
64 ),
65 );
66 const reference = {
67 cwd: "/repo",
68 repository: "acme/web",
69 number: 1,
70 host: "bitbucket.org",
71 };
72 const result = yield* operation === "listComments" || operation === "listCommits"
73 ? Effect.result(provider.getChangeRequestActivity(reference))
74 : Effect.result(provider.getChangeRequest(reference));
75 if (status === 429) {
76 expect(result).toMatchObject({
77 _tag: "Failure",
78 failure: { reason: "rate-limited", retryAt: 120_000 },
79 });
80 } else {
81 expect(result._tag).toBe("Success");
82 }
83 }
84 }),
85 );
86}
87
9describe("bitbucketProviderFailure", () => {88describe("bitbucketProviderFailure", () => {
10 it("treats only an HTTP 401 as unusable credentials", () => {89 it("treats only an HTTP 401 as unusable credentials", () => {
11 const responseError = (status: number) =>90 const responseError = (status: number) =>
apps/server/src/pullRequest/BitbucketPullRequestProvider.ts+34-8
...@@ -73,7 +73,10 @@ export function bitbucketProviderFailure(...@@ -73,7 +73,10 @@ export function bitbucketProviderFailure(
73 if (error._tag === "BitbucketResponseError" && error.status === 401) {73 if (error._tag === "BitbucketResponseError" && error.status === 401) {
74 return { reason: "unauthenticated" };74 return { reason: "unauthenticated" };
75 }75 }
76 if (error._tag === "BitbucketResponseError" && error.status === 429) {76 if (
77 (error._tag === "BitbucketResponseError" || error._tag === "BitbucketResponseBodyReadError") &&
78 error.status === 429
79 ) {
77 return {80 return {
78 reason: "rate-limited",81 reason: "rate-limited",
79 ...(error.retryAt === undefined ? {} : { retryAt: error.retryAt }),82 ...(error.retryAt === undefined ? {} : { retryAt: error.retryAt }),
...@@ -122,6 +125,31 @@ export const make = Effect.gen(function* () {...@@ -122,6 +125,31 @@ export const make = Effect.gen(function* () {
122 cause: error,125 cause: error,
123 });126 });
124127
128 const recoverRead = <A>(
129 read: Effect.Effect<A, BitbucketPullRequestApi.BitbucketPullRequestApiError>,
130 fallback: A,
131 ) => {
132 const recover = () => Effect.succeed(fallback);
133 return Effect.catchTags(read, {
134 BitbucketResponseError: (error) => (error.status === 429 ? Effect.fail(error) : recover()),
135 BitbucketUntrustedUrlError: recover,
136 BitbucketRepositoryLocatorError: recover,
137 BitbucketRequestError: recover,
138 BitbucketResponseBodyReadError: (error) =>
139 error.status === 429 ? Effect.fail(error) : recover(),
140 BitbucketResponseDecodeError: recover,
141 BitbucketRepositoryVcsResolveError: recover,
142 BitbucketRepositoryRemotesListError: recover,
143 BitbucketRepositoryRemoteNotFoundError: recover,
144 BitbucketPullRequestBodyReadError: recover,
145 BitbucketCheckoutError: recover,
146 BitbucketPullRequestReadError: recover,
147 BitbucketViewerUnavailableError: recover,
148 BitbucketRepositoryUnsupportedError: recover,
149 BitbucketDiffCommitError: recover,
150 });
151 };
152
125 const provider: PullRequestProviderApi = {153 const provider: PullRequestProviderApi = {
126 kind: "bitbucket",154 kind: "bitbucket",
127 capabilities: CAPABILITIES,155 capabilities: CAPABILITIES,
...@@ -156,12 +184,12 @@ export const make = Effect.gen(function* () {...@@ -156,12 +184,12 @@ export const make = Effect.gen(function* () {
156 [184 [
157 api.getPullRequest(target),185 api.getPullRequest(target),
158 api.getDiffStat(target),186 api.getDiffStat(target),
159 api.getMergeability(target).pipe(Effect.orElseSucceed(() => "unknown" as const)),187 recoverRead(api.getMergeability(target), "unknown" as const),
160 api.listChecks(target).pipe(Effect.orElseSucceed(() => [])),188 recoverRead(api.listChecks(target), []),
161 // A permission that could not be read is an unknown one, which is granted: a hidden189 // A permission that could not be read is an unknown one, which is granted: a hidden
162 // Merge leaves someone entitled to it with no way through, and one Bitbucket refuses190 // Merge leaves someone entitled to it with no way through, and one Bitbucket refuses
163 // at least says why.191 // at least says why.
164 api.getRepositoryPermission(target).pipe(Effect.orElseSucceed(() => true)),192 recoverRead(api.getRepositoryPermission(target), true),
165 ],193 ],
166 { concurrency: 5 },194 { concurrency: 5 },
167 ).pipe(195 ).pipe(
...@@ -200,10 +228,8 @@ export const make = Effect.gen(function* () {...@@ -200,10 +228,8 @@ export const make = Effect.gen(function* () {
200 // Reviews ride on the pull request itself, so this inexpensive core read is repeated228 // Reviews ride on the pull request itself, so this inexpensive core read is repeated
201 // here rather than making the core response wait for the conversation endpoints.229 // here rather than making the core response wait for the conversation endpoints.
202 api.getPullRequest(target),230 api.getPullRequest(target),
203 api231 recoverRead(api.listComments(target), { comments: [], threads: [], truncated: true }),
204 .listComments(target)232 recoverRead(api.listCommits(target), []),
205 .pipe(Effect.orElseSucceed(() => ({ comments: [], threads: [], truncated: true }))),
206 api.listCommits(target).pipe(Effect.orElseSucceed(() => [])),
207 ],233 ],
208 { concurrency: 3 },234 { concurrency: 3 },
209 ).pipe(235 ).pipe(
apps/server/src/sourceControl/BitbucketApi.test.ts+24
...@@ -618,6 +618,30 @@ it.effect("preserves Bitbucket response body read failures as their immediate ca...@@ -618,6 +618,30 @@ it.effect("preserves Bitbucket response body read failures as their immediate ca
618 }).pipe(Effect.provide(layer));618 }).pipe(Effect.provide(layer));
619});619});
620620
621it.effect("keeps the 429 retry time when the response body cannot be read", () => {
622 const { layer } = makeLayer({
623 response: () =>
624 new Response(
625 new ReadableStream<Uint8Array>({
626 start: (controller) => controller.error(new Error("response stream failed")),
627 }),
628 { status: 429, headers: { "Retry-After": "120" } },
629 ),
630 });
631
632 return Effect.gen(function* () {
633 yield* TestClock.setTime(1_000);
634 const bitbucket = yield* BitbucketApi.BitbucketApi;
635 const error = yield* bitbucket
636 .request({ method: "GET", url: "/repositories/acme/web" })
637 .pipe(Effect.flip);
638
639 assert.instanceOf(error, BitbucketApi.BitbucketResponseBodyReadError);
640 assert.strictEqual(error.status, 429);
641 assert.strictEqual(error.retryAt, 121_000);
642 }).pipe(Effect.provide(layer));
643});
644
621it.effect("checks out same-repository pull requests with the existing Bitbucket remote", () => {645it.effect("checks out same-repository pull requests with the existing Bitbucket remote", () => {
622 const { git, layer } = makeLayer({646 const { git, layer } = makeLayer({
623 response: () =>647 response: () =>
apps/server/src/sourceControl/BitbucketApi.ts+4-1
...@@ -117,6 +117,7 @@ export class BitbucketResponseBodyReadError extends Schema.TaggedError<Bitbucket...@@ -117,6 +117,7 @@ export class BitbucketResponseBodyReadError extends Schema.TaggedError<Bitbucket
117 {117 {
118 operation: BitbucketApiOperation,118 operation: BitbucketApiOperation,
119 status: Schema.Int,119 status: Schema.Int,
120 retryAt: Schema.optional(Schema.Number),
120 cause: Schema.Defect(),121 cause: Schema.Defect(),
121 },122 },
122) {123) {
...@@ -584,6 +585,7 @@ function responseError(...@@ -584,6 +585,7 @@ function responseError(
584 // only its length is reported anyway.585 // only its length is reported anyway.
585 return Effect.gen(function* () {586 return Effect.gen(function* () {
586 const now = yield* Clock.currentTimeMillis;587 const now = yield* Clock.currentTimeMillis;
588 const retryAt = retryAtFromHeader(response.headers["retry-after"], now);
587 const collected = yield* collectUint8StreamText({589 const collected = yield* collectUint8StreamText({
588 stream: response.stream,590 stream: response.stream,
589 maxBytes: DEFAULT_MAX_RESPONSE_BYTES,591 maxBytes: DEFAULT_MAX_RESPONSE_BYTES,
...@@ -593,6 +595,7 @@ function responseError(...@@ -593,6 +595,7 @@ function responseError(
593 new BitbucketResponseBodyReadError({595 new BitbucketResponseBodyReadError({
594 operation,596 operation,
595 status: response.status,597 status: response.status,
598 retryAt,
596 cause,599 cause,
597 }),600 }),
598 ),601 ),
...@@ -601,7 +604,7 @@ function responseError(...@@ -601,7 +604,7 @@ function responseError(
601 operation,604 operation,
602 status: response.status,605 status: response.status,
603 responseBodyLength: collected.text.length,606 responseBodyLength: collected.text.length,
604 retryAt: retryAtFromHeader(response.headers["retry-after"], now),607 retryAt,
605 });608 });
606 });609 });
607}610}