Skip to content

Merge-readiness poll gate write can be clobbered by a concurrent hydration wave's whole-map republish #84

Description

@jaylann

Summary

publishChecks (gbar/Sources/Store/AppStoreHydration.swift) republishes the hydration maps as whole-map assignments (prGates = pending.gates), where pending.gates was snapshotted from prGates at drain start. If startMergeReadinessPoll's refreshPRState writes a freshly-unblocked gate under the same checksGeneration between that snapshot and the publish, the wave's wholesale reassign overwrites it — re-hiding the Merge button until the next full refetch.

Pre-existing (the old code already did prGates = pendingGates from a snapshot); noting it now because the .checksOnly path added in #82 widens the window — a skipped PR's gate in the snapshot is stale by construction rather than freshly fetched.

Repro (timing-dependent)

  1. Viewer approves PR refactor(design): deeper, richer state colors #7; mergeable_state still async-recomputing, so startMergeReadinessPoll begins polling (captures generation = G).
  2. A regular 60s poll fires and starts a hydration wave. If that wave reads checksGeneration == G (i.e. it didn't bump generation before the merge poll captured it, or they interleave under the same G), drainChecks snapshots pending.gates = prGates — with refactor(design): deeper, richer state colors #7's old "blocked" gate.
  3. The merge poll's refreshPRState writes prGates[#7] = the newly-unblocked gate (Merge should show).
  4. The wave completes and publishChecks does prGates = pending.gates, clobbering refactor(design): deeper, richer state colors #7 back to the stale "blocked" gate → Merge disappears until the next full refetch re-derives it.

Both run on @MainActor but interleave across await suspension points. The checksGeneration guard prevents cross-generation clobbers; the gap is a same-generation interleave.

Options

  • Merge refreshPRState's single-key gate write into the wave's pending map rather than writing prGates directly (or have the wave re-read per-key gates at publish time instead of a snapshot).
  • Have refreshPRState bump/own a generation so a concurrent wave's publish is dropped for keys it touched.
  • Per-key gate publishing for the merge-poll path instead of whole-map reassign.

Low severity / narrow timing window and self-heals on the next full refetch, but it can briefly hide a valid Merge button right after an approval — the exact UX the merge-readiness poll exists to fix.

Surfaced in the #82 code review.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions