Skip to content

crow:auto creates duplicate review sessions during in-flight kickoff (regression from #405) #406

@dgershman

Description

@dgershman

Summary

After PR #405 (the beachball fix) shipped, the crow:auto auto-review watcher creates duplicate review sessions for the same PR when it ticks while a prior kickoff for that PR is still in flight.

Why this regressed

Before #405, createReviewSession() held the main actor for the entire ~10s of git/clone work (via the synchronous Process.waitUntilExit() inside shell()). That inadvertently serialized everything — the IssueTracker's refresh tick couldn't interleave, so the watcher's guard could never observe a stale reviewSessionID mid-kickoff.

After #405, the git work runs in Task.detached, so the main actor stays responsive and multiple watcher ticks can fire during the window where the session row hasn't yet been appended to appState.sessions.

Concrete race (auto-review path)

  1. IssueTracker tick T0: watcher sees request.reviewSessionID == nil for PR #X, calls enqueueReviewKickoff([url]). Task A starts; createReviewSession enters Task.detached for the git clone.
  2. IssueTracker tick T1 (~N seconds later, while clone is still in flight): tracker rebuilds requests. reviewSessionID is still nil because it's populated lazily inside the tracker's refresh closure (IssueTracker.swift:385-389) by matching appState.reviewSessions against PR URLs — the session row hasn't been appended yet.
  3. The autoReviewedFingerprints set at AppDelegate.swift:608 catches exact id@headRefOid repeats — but if headRefOid changes between ticks (force-push, intermediate commits), the fingerprint differs and slips through.
  4. Watcher enqueues a SECOND kickoff for the same PR. The serial kickoff queue runs it. createReviewSession has no dedup check — it does another full clone/setup and appends a second session.

UI symptom (same root cause)

ReviewBoardView.swift:309-310 hides the Start Review button on request.reviewSessionID != nil. Because reviewSessionID is populated lazily by the next tracker refresh, the button stays clickable for several seconds after a session has been created. Even though the user isn't double-clicking, the UI state is wrong during the window.

Suggested fix — three layered changes sharing one helper

  1. New AppState.existingReviewSession(forPRURL:) helper — single source of truth, checks appState.sessions by the canonical name pattern \"review-<repo>-<num>\" (already used by createReviewSession at SessionService.swift:~1322).

  2. Tighten the auto-review watcher guard (AppDelegate.swift:618-624) — augment the existing guard to also skip when appState.existingReviewSession(forPRURL: request.url) != nil. This stops relying on the lagging request.reviewSessionID and uses the authoritative appState.sessions instead.

  3. Universal backstop in createReviewSession — add an early-return dedup at the top of the function that calls the helper. Catches any future caller (RPC, manual, watcher) that re-enters during a kickoff. Safe because enqueueReviewKickoff already serializes — by the time Task B runs, Task A's appState.sessions.append has completed.

  4. UI — single-PR Start Review button (ReviewBoardView.swift:309-310) also uses the helper, so it hides instantly when the session appears in appState. Batch button (ReviewBoardView.swift:138-158) pre-filters URLs that already have sessions in appState.

Also extract the PR-URL parsing currently inlined at the top of createReviewSession (line ~1212) into a Session.parseReviewPR(url:) helper in CrowCore so all four sites share one parser.

Acceptance

  • crow:auto-labeled PRs trigger exactly one review session per (repo, prNumber) even when the watcher ticks multiple times during the clone window.
  • A second tick during an in-flight kickoff is silently skipped at the watcher level.
  • createReviewSession itself is idempotent per (repo, prNumber) — calling it again returns the existing session's ID without creating a duplicate.
  • Start Review button (single + batch) hides/filters as soon as the session exists in appState, not after the next tracker refresh.
  • Existing 145 tests still pass; add a focused unit test for existingReviewSession(forPRURL:).

Out of scope

  • Refactoring IssueTracker to populate reviewSessionID synchronously when the session is appended (the new authoritative-appState checks make this unnecessary).
  • Tracking explicit "in-flight" URL state on AppDelegate/SessionService (serial kickoff queue + appState.sessions check is sufficient).
  • Disabling the batch button entirely when all selected PRs already have sessions (pre-filtering is enough).

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions