also looking at this
fix(dashboard): fail closed when the dashboard lookup errors
#1452
2 files
+39
-4
Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.
| @@ -3685,8 +3685,21 @@ export class Buddy { | ||
| 3685 | 3685 | return null |
| 3686 | 3686 | } |
| 3687 | 3687 | catch (error) { |
| 3688 | this.logger.warn(`Failed to search for existing dashboard: ${error}`) | |
| 3689 | return null | |
| 3688 | // "Found nothing" and "the lookup failed" must not collapse to the same | |
| 3689 | // value. The caller treats null as "no dashboard exists" and creates one, | |
| 3690 | // so a transient 5xx, timeout or secondary rate-limit on getIssues() would | |
| 3691 | // open a duplicate dashboard and still exit 0 - permanent damage from a | |
| 3692 | // momentary failure. | |
| 3693 | // | |
| 3694 | // Both call sites reach this: the initial lookup and the pre-create race | |
| 3695 | // check. The race check in particular was no protection at all while it | |
| 3696 | // failed open, because whatever broke the first lookup is usually still | |
| 3697 | // broken a moment later. | |
| 3698 | // | |
| 3699 | // A failed run is recoverable - the next scheduled run retries. A duplicate | |
| 3700 | // dashboard is not: someone has to notice it and close it by hand. | |
| 3701 | this.logger.error(`Failed to search for existing dashboard: ${error}`) | |
| 3702 | throw error | |
| 3690 | 3703 | } |
| 3691 | 3704 | } |
| 3692 | 3705 | } |
| @@ -670,10 +670,32 @@ describe('buddy orchestration', () => { | ||
| 670 | 670 | expect(await findDashboard(provider)).toBeNull() |
| 671 | 671 | }) |
| 672 | 672 | |
| 673 | it('failure case - a getIssues failure degrades to null instead of throwing', async () => { | |
| 673 | // Absence and failure must stay distinguishable. The caller reads null as | |
| 674 | // "no dashboard exists" and creates one, so degrading a transient failure to | |
| 675 | // null turned a momentary 5xx into a permanent duplicate dashboard while the | |
| 676 | // run still exited 0. Failing the run is recoverable - the next scheduled run | |
| 677 | // retries; a duplicate dashboard has to be closed by hand. | |
| 678 | it('failure case - a getIssues failure rejects instead of reporting absence', async () => { | |
| 674 | 679 | const provider = { getIssues: mock(() => Promise.reject(new Error('api down'))) } |
| 675 | 680 | |
| 676 | expect(await findDashboard(provider)).toBeNull() | |
| 681 | await expect(findDashboard(provider)).rejects.toThrow('api down') | |
| 682 | }) | |
| 683 | ||
| 684 | it('failure case - the pre-create race check rejects on the same failure', async () => { | |
| 685 | // The race check calls this same helper, so while it failed open it was no | |
| 686 | // protection at all: whatever broke the first lookup is usually still | |
| 687 | // broken a moment later. | |
| 688 | const provider = { getIssues: mock(() => Promise.reject(new Error('secondary rate limit'))) } | |
| 689 | ||
| 690 | await expect(findDashboard(provider)).rejects.toThrow('secondary rate limit') | |
| 691 | }) | |
| 692 | ||
| 693 | it('failure case - a configured issueNumber lookup rejects rather than reporting it missing', async () => { | |
| 694 | // This path calls getIssues too, so it failed open in the same way - and | |
| 695 | // "configured dashboard is missing" is exactly what triggers a create. | |
| 696 | const provider = { getIssues: mock(() => Promise.reject(new Error('api down'))) } | |
| 697 | ||
| 698 | await expect(findDashboard(provider, 1911)).rejects.toThrow('api down') | |
| 677 | 699 | }) |
| 678 | 700 | |
| 679 | 701 | it('success case - honors an explicit issueNumber even without marker or labels', async () => { |