GithubWebhooksService.handlePullRequest processes every linked issue in one unguarded loop — one bounty's failure aborts and mismarks all other linked bounties in the same merged PR

[chonilius]

Overview

A single merged PR can reference multiple issues via closing keywords ("Fixes #12, Closes #34, Resolves #56"), and GithubWebhooksService.handlePullRequest processes every one of them in a single loop with no per-issue error isolation:

// src/github/github-webhooks.service.ts:107-149
private async handlePullRequest(payload: GithubPullRequestPayload): Promise<void> {
  if (payload.action !== 'closed' || !payload.pull_request.merged) return;

  const issueNumbers = this.extractLinkedIssueNumbers(payload.pull_request.body ?? '');
  if (issueNumbers.length === 0) { /* ... */ return; }

  for (const number of issueNumbers) {
    const issue = await this.issueRepo.findOne({ where: { number, repository: { githubRepoId: ... } }, relations: { repository: true, bounty: true } });
    if (!issue?.bounty) continue;

    const bounty = await this.bountyRepo.findOne({ where: { id: issue.bounty.id } });
    if (!bounty) continue;

    if (bounty.status === BountyStatus.CLAIMED) {
      await this.bountiesService.markInReview(bounty.id, payload.pull_request.html_url, payload.pull_request.number);
    }
    await this.bountiesService.markMergedAndRelease(bounty.id);   // <- can throw, nothing catches it per-iteration
  }
}

If markMergedAndRelease throws for the bounty tied to the first linked issue — for any of the many reasons it can (an invalid state transition, an escrow release failure per the companion "stuck MERGED bounty" issue, a Soroban error) — the exception propagates straight out of the for loop, and every subsequent linked issue in the same PR is never processed at all. The only place this is caught is handleEvent's outer try/catch:

// src/github/github-webhooks.service.ts:84-104
try {
  if (eventType === 'pull_request') {
    await this.handlePullRequest(payload as unknown as GithubPullRequestPayload);
  }
  // ...
  event.status = WebhookEventStatus.PROCESSED;
} catch (err) {
  event.status = WebhookEventStatus.FAILED;
  event.error = (err as Error).message;
}

Which marks the entire webhook delivery FAILED, with a single error message that only describes whichever issue happened to fail first — even if the second and third linked bounties in that same PR were perfectly fine and would have processed successfully if the loop had kept going. Since GitHub doesn't retry a webhook delivery based on the response body (only on HTTP status, and this controller always returns 202 regardless of processing outcome — see github-webhooks.controller.ts:16,36), a FAILED status here isn't just cosmetic: it's the only durable record that something in this PR's processing didn't fully complete, and today that record can't distinguish "issue #12 failed, #34 and #56 were never attempted" from "all three failed" from "all three succeeded but the event row update itself raced" — an operator looking at a FAILED WebhookEvent has no way to tell which of the PR's several linked bounties actually need attention without manually re-deriving it from the PR body and cross-referencing bounty statuses by hand.

There's also a more mundane trigger for exactly this failure mode, not just genuine errors: extractLinkedIssueNumbers uses matchAll with no de-duplication, so a PR body like "Fixes #12. This also resolves #12 as discussed in review." produces [12, 12]. The first iteration processes bounty #12 successfully (MERGED → PAID). The second iteration calls markMergedAndRelease again on the now-PAID bounty, which throws InvalidBountyTransitionError (since PAID's transition list is empty) — a completely benign, duplicate-reference PR body is enough to mark the webhook FAILED, even though the actual payout succeeded correctly and nothing needs fixing. (Confirmed safe from double-payment either way — assertTransition correctly blocks the second call — but the false-FAILED status is still a real operational cost: it looks exactly like a genuine failure in monitoring/alerting with no way to tell the difference without reading logs.)

Requirements

• Wrap each iteration of the for (const number of issueNumbers) loop in its own try/catch, so one linked issue's failure doesn't prevent the others from being processed.
• Track and surface a per-issue outcome (succeeded / skipped-no-bounty / failed-with-reason) rather than collapsing the whole PR's processing into one pass/fail on the WebhookEvent row — at minimum, aggregate all per-issue errors into event.error instead of only capturing whichever one happened to throw first and abort the loop, so the aggregate reflects everything that actually happened.
• De-duplicate issueNumbers before the loop ([...new Set(issueNumbers)]) so a PR body referencing the same issue twice doesn't attempt to process the same bounty twice in the same event, eliminating the spurious FAILED case described above without relying on assertTransition to silently absorb it.
• Decide and document what event.status should be when some but not all linked issues succeed — fully PROCESSED (optimistic, since payouts that succeeded did succeed), a new partial-success status, or FAILED with the successes still recorded per-issue for operator visibility. Whichever is chosen, it should be a deliberate decision, not an accident of which iteration happened to throw.

Acceptance Criteria

• A PR linking three issues where the middle one's bounty processing throws still successfully processes the first and third.
• Duplicate issue-number references in a single PR body no longer produce a spurious FAILED WebhookEvent for an otherwise-fully-successful merge.
• WebhookEvent.error (or an equivalent structured field) can represent multiple independent per-issue outcomes for a single PR event, not just one string from whichever failure occurred first.
• A test reproduces both scenarios (genuine partial failure among multiple linked issues; benign duplicate issue reference) and asserts the improved behavior.

Additional Notes

Precise references: src/github/github-webhooks.service.ts:107-149 (handlePullRequest, the unguarded loop), :84-104 (handleEvent, the only current error boundary, one level too coarse), :183-186 (extractLinkedIssueNumbers, no de-duplication), src/bounties/bounty-state-machine.ts:33 (PAID's empty transition list — confirms the duplicate-reference case is safe from double-payment, purely a false-alarm/observability problem, not a fund-safety one).

Test/reproduction plan:

// three issues, three bounties, all CLAIMED and ready to merge
const prBody = 'Fixes #12. Also resolves #34. See also #34 again above.';
jest.spyOn(bountiesService, 'markMergedAndRelease')
  .mockImplementationOnce(async (id) => { throw new Error('escrow release failed'); }) // #12
  .mockImplementation(async (id) => ({ id, status: BountyStatus.PAID } as Bounty));    // #34 (both refs)

const event = await service.handleEvent('pull_request', 'delivery-1', mergedPrPayload(prBody), true);
// pre-fix: event.status === FAILED, bounty #34 never attempted at all
// post-fix: bounty #34 is processed exactly once (de-duped) and succeeds regardless of #12's failure;
//           event captures both the #12 failure and the #34 success distinctly

Cross-references: compounds with the companion "stuck MERGED bounty" issue — a bounty stuck in MERGED due to a release failure is exactly the kind of per-issue failure this loop currently lets abort processing of every other linked bounty in the same PR, which is a second, independent reason to isolate iterations even after that issue's own fix lands (transient failures will still happen even with a retry path available).

[boluwacodes]

GithubWebhooksService.handlePullRequest has a failure-isolation problem when one merged PR closes multiple bounty-linked issues.

A single PR can reference multiple issues through GitHub closing keywords, and the webhook handler processes those issue numbers sequentially in one for loop. However, there is no per-issue try/catch around the processing of each bounty.

If markMergedAndRelease() throws for the first linked issue—for example because of an invalid state transition, an escrow release failure, or a Soroban error—the exception exits the loop immediately. The outer webhook handler catches the failure, but by then every subsequent issue in the same PR has been skipped.

This means one broken bounty can prevent unrelated bounties from the same merged PR from transitioning to MERGED and entering their payout flow. The failure of one resource should not determine whether other independently valid resources in the same webhook event are processed.

I have experience designing event-driven processing where batch items require failure isolation. Each linked issue should be processed independently, with its own error boundary and structured logging/monitoring. A failure should record the affected issue and continue processing the remaining linked issues, while preserving retry or reconciliation mechanisms for the failed bounty.

[grantfox-oss]

🦊 GrantFox — @boluwacodes has been assigned to this issue as part of the Third Campaign campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #47)
2. Your PR will be reviewed by the MergeFi maintainers

Good luck! Track your progress on GrantFox.