-
Notifications
You must be signed in to change notification settings - Fork 351
Add retry with jitter to create_issue safe-output handler #26056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2b214ad
bd9f50a
733aae4
99a7bfd
4df60c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -452,7 +452,7 @@ describe("create_issue", () => { | |
| const result = await handler({ title: "Test" }); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.error).toBe("API Error"); | ||
| expect(result.error).toContain("API Error"); | ||
| }); | ||
| }); | ||
|
|
||
|
|
@@ -667,4 +667,95 @@ describe("create_issue", () => { | |
| expect(mockGithub.rest.issues.create).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("retry on rate limit errors", () => { | ||
| beforeEach(() => { | ||
| vi.useFakeTimers(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.useRealTimers(); | ||
| }); | ||
|
|
||
| it("should retry issue creation on transient rate limit error and succeed", async () => { | ||
| mockGithub.rest.issues.create = vi | ||
| .fn() | ||
| .mockRejectedValueOnce(new Error("Secondary rate limit hit")) | ||
| .mockResolvedValue({ | ||
| data: { | ||
| number: 456, | ||
| html_url: "https://github.com/owner/repo/issues/456", | ||
| title: "Retried Issue", | ||
| }, | ||
| }); | ||
|
|
||
| const handler = await main({}); | ||
| const resultPromise = handler({ | ||
| title: "Retried Issue", | ||
| body: "Test body", | ||
| }); | ||
|
|
||
| await vi.runAllTimersAsync(); | ||
| const result = await resultPromise; | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.number).toBe(456); | ||
| expect(mockGithub.rest.issues.create).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it("should fail after exhausting retries on persistent rate limit error", async () => { | ||
| mockGithub.rest.issues.create = vi.fn().mockRejectedValue(new Error("Secondary rate limit hit")); | ||
|
|
||
| const handler = await main({}); | ||
| const resultPromise = handler({ | ||
| title: "Failing Issue", | ||
| body: "Test body", | ||
| }); | ||
|
|
||
| await vi.runAllTimersAsync(); | ||
| const result = await resultPromise; | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.error).toBeDefined(); | ||
| // 1 initial + 3 retries = 4 calls | ||
| expect(mockGithub.rest.issues.create).toHaveBeenCalledTimes(4); | ||
| }); | ||
|
Comment on lines
+680
to
+722
|
||
|
|
||
| it("should have retry delays that never exceed maxDelayMs + jitterMs", async () => { | ||
| const setTimeoutSpy = vi.spyOn(globalThis, "setTimeout"); | ||
|
|
||
| mockGithub.rest.issues.create = vi | ||
| .fn() | ||
| .mockRejectedValueOnce(new Error("Secondary rate limit hit")) | ||
| .mockRejectedValueOnce(new Error("Secondary rate limit hit")) | ||
| .mockResolvedValue({ | ||
| data: { | ||
| number: 789, | ||
| html_url: "https://github.com/owner/repo/issues/789", | ||
| title: "Bounded Delay Issue", | ||
| }, | ||
| }); | ||
|
|
||
| const handler = await main({}); | ||
| const resultPromise = handler({ | ||
| title: "Bounded Delay Issue", | ||
| body: "Test body", | ||
| }); | ||
|
|
||
| await vi.runAllTimersAsync(); | ||
| await resultPromise; | ||
|
|
||
| // create_issue uses { initialDelayMs: 15000, maxDelayMs: 45000, jitterMs: 10000 } | ||
| // Maximum possible delay per retry = maxDelayMs + jitterMs = 55000ms | ||
| const maxBound = 55000; | ||
| // Filter out short setTimeout calls (e.g. from test infrastructure) to isolate retry delays | ||
| const sleepDelays = setTimeoutSpy.mock.calls.filter(([, ms]) => ms > 1000).map(([, ms]) => ms); | ||
|
|
||
| for (const delay of sleepDelays) { | ||
| expect(delay).toBeLessThanOrEqual(maxBound); | ||
| } | ||
|
|
||
| setTimeoutSpy.mockRestore(); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same non-idempotency concern as the main issue creation: retrying
issues.createfor the parent issue on network/timeout errors can create multiple parent issues for the same group. Consider narrowingshouldRetryfor this call or adding an idempotency guard (e.g., re-search for an existing parent marker before retrying).This issue also appears on line 597 of the same file.