Skip to content

fix(queue): guard ISSUE_CLOSURE solvedByPr write with state = CLOSED#221

Open
rsnetworkinginc wants to merge 1 commit into
entrius:testfrom
rsnetworkinginc:fix-closure-state-guard-199
Open

fix(queue): guard ISSUE_CLOSURE solvedByPr write with state = CLOSED#221
rsnetworkinginc wants to merge 1 commit into
entrius:testfrom
rsnetworkinginc:fix-closure-state-guard-199

Conversation

@rsnetworkinginc

Copy link
Copy Markdown

Summary

The async ISSUE_CLOSURE fetch job can write issues.solved_by_pr onto an issue
that has already been reopened. handleIssueClosure reads issue state once, does a
slow GraphQL timeline fetch (fetchIssueClosingPr), and then writes solvedByPr
unconditionally. If an issues.reopened webhook lands during that fetch, its
handler upserts state = OPEN and clears solved_by_pr, but the in-flight job then
clobbers that clear and leaves an OPEN issue carrying a solver-PR link.

This corrupts issue-discovery scoring inputs: an OPEN issue can be counted as solved
(or skew credibility ratios) until a later webhook/backfill happens to correct it.

Root cause

packages/das/src/queue/fetch.processor.tshandleIssueClosure() reads
issue.state once, then performs the slow GraphQL fetch, then writes:

await this.issueRepo.update({ repoFullName, issueNumber }, { solvedByPr });

with no re-check of state — a classic read-then-write race against the synchronous
reopen path in IssueHandler.

Fix

Guard the final write with state = 'CLOSED' in the update criteria, so the write
becomes a no-op if the issue was reopened while the GraphQL fetch was in flight:

await this.issueRepo.update(
  { repoFullName, issueNumber, state: "CLOSED" },
  { solvedByPr },
);

This is the UPDATE … WHERE state = 'CLOSED' direction suggested in the issue. It is
a single-statement conditional update (no extra round-trip, no schema change) and does
not touch the reopen webhook path. Distinct from #188/#189 (frozen closed_at
attribution) and #24 (the reopen clear itself) — this closes the concurrency window
that lets attribution reappear after that clear.

Tests

Added regression coverage in fetch.processor.spec.ts, driven through the public
process() entry point with an in-memory Issue repository whose update honours the
criteria like SQL WHERE:

  • reopen race — the closing-PR fetch mutates the row to state = OPEN mid-flight;
    asserts the issue stays OPEN with solved_by_pr = null and that the final write is
    guarded by state: "CLOSED". This test fails on the pre-fix code (the OPEN issue
    gets solved_by_pr = 123) and passes with the guard.
  • happy path — an issue that stays CLOSED still gets solved_by_pr written.

Verification

  • npm run lint — clean
  • npm run format:check — clean
  • npm test — 27/27 pass (RED→GREEN on the new race test)
  • npm run build — clean

Closes #199

@xiao-xiao-mao xiao-xiao-mao Bot added the bug Something isn't working label Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] ISSUE_CLOSURE job can write solved_by_pr onto a reopened (OPEN) issue after async GraphQL fetch

1 participant