fix(webhooks): bound merge polls and read merge workflows from the merge

Merged
BN
12d ago
+897-99

Fixes the two review findings on #409 (fix(webhooks): resolve fresh pull request merge commits, merged as 26e6fee). Both were verified against main (86f4977) before any change.

Finding 1: merge-state polls can park every webhook worker

Verified in code. drain_webhook_queue_loop runs one delivery per worker and process_one_delivery waits for that delivery to finish. resolve_fresh_merge probed GitHub every 2 s for up to the 30 s budget before building the engine's merge, with no bound on concurrent pollers. A superseded head polls to the full budget by construction, because the API reports a newer head that no probe can match. Repeated pushes, fork pull-request pushes included, could hold all PRELOOP_WEBHOOK_WORKERS (default 8) for 30 s each and stall every other webhook. The superagent P2 on #409 was dismissed against an earlier head-ref design; the 30 s poll is what main ships, so the finding stands.

Fix:

  • AppState::fresh_merge_poll_slots, a semaphore of fresh_merge_poll_capacity() = max(1, PRELOOP_WEBHOOK_WORKERS / 4) permits (2 at the default 8 workers).
  • A delivery takes a slot with try_acquire_owned only when it has to wait. The first probe runs without a slot, so a merge GitHub already offers is still used.
  • With no slot free, the delivery stops polling and builds the engine's merge at once, the existing budget-expiry fallback.
  • The slot is an RAII permit held across the poll and dropped on every exit, including the processing task being aborted when the delivery's lease is lost.

Deviation from the suggested fix: the reviewer's wording could be read as skipping even the first probe. I kept that probe. A saturated node still uses GitHub's merge when it is already there, at the cost of one probe per delivery.

Finding 2: a self-built merge ran the pull request head's workflows

Verified in code. inventory_sha was the PR head whenever the engine built the merge. The run executed the head's copy of a workflow file that the base had since changed, while it checked out the merge, which carries the base's copy. GitHub's own pull_request runs read workflows from the merge.

Fix:

  • For a pull-request run on an engine-built merge, the workflow inventory is read from the merge commit in the engine's mirror (PrebuiltMerge::mirror_repository under state_dir) with git rev-parse, ls-tree, and show <merge>:<path>. The helper workflows_at_git_ref now serves both this read and the local-workspace branch of fetch_workflows_at.
  • Only a failed mirror read falls back to the head's inventory, with a warning.
  • Check runs still attach to the head.

#409's description says "workflow discovery remains pinned to the payload head". That no longer holds for self-built merges. docs/github-app-webhook.md §4 and the CHANGELOG (Unreleased, Fixed) are updated to match.

Checked and unchanged: the dedup (already_published, covers_echo, published_runs) keys on the echo head sha, the workflow path, and tested_merge. Those describe what was actually tested, so they stay consistent with a merge-pinned inventory. Push-back keys on the CLI submission's sha, not on webhook inventory.

Tests

New, in github::tests (7, all pass on af00405):

  • saturated_poll_slots_build_the_merge_without_waiting_for_the_budget: with every slot held, the delivery finishes well inside the 10 s budget, polls at most once, and runs the engine's merge.
  • poll_slot_is_released_once_github_offers_the_merge
  • poll_slot_is_released_when_the_probe_turns_fatal
  • poll_slot_is_released_when_the_budget_runs_out
  • poll_slot_is_released_when_the_waiting_delivery_is_cancelled: aborts a waiting poll and checks the slot comes back.
  • self_built_merge_runs_the_workflow_revision_at_the_merge: the base advanced ci.yml, the head did not touch it, and the run executes the merge's revision.
  • unreadable_merge_mirror_falls_back_to_the_head_workflows

Fail-before, observed: I temporarily reverted both behaviors (head-pinned inventory, and no slot acquisition) in a scratch build. self_built_merge_runs_the_workflow_revision_at_the_merge failed on the head's ci.yml, and saturated_poll_slots_build_the_merge_without_waiting_for_the_budget failed on "must not wait out the poll budget". The committed file was then restored and both pass.

Verification (af00405, built in a private target clone)

  • cargo fmt --all -- --check: clean.
  • cargo check --locked -p preloop-runner-server --tests: clean.
  • cargo clippy --locked -p preloop-runner-server --all-targets: no warnings from changed code. One pre-existing chunks_exact lint in live_log_segments.rs, which this change does not touch.
  • cargo test --locked -p preloop-runner-server --features test-support --test broker --test merge --test concurrency --test webhooks: broker 62/62, merge 15/15, concurrency 48/48, webhooks 44/44. These match the stated baselines.
  • cargo test --locked -p preloop-runner-server --lib: 1073 passed, 3 ignored, 1 failed. The failure rotated between two runs: snapshots::snapshot_sweep_tests::sweep_rearms_fresh_terminal_snapshot once, and timeline_logs::tests::successful_step_does_not_mask_the_jobs_real_completion once. Both fail in an AppState::new fixture with ENOENT under parallel load and pass when run alone. I did not run the lib suite on main to confirm the flake predates this branch.

Not run: the full just test-ci gate (zizmor and conform replay). I ran the components listed above.

Devin Review

Summary by cubic

Bounds merge-state polling during fresh pull-request merge resolution so a burst of pushes can't park every webhook worker, and makes pull-request runs read their workflow files from the merge commit they check out instead of the head.

Bug Fixes

  • A delivery takes a poll slot (a quarter of the webhook workers, at least one) only when it has to wait; with no slot free it builds the engine's merge at once. Slots are released on success, failure, and cancellation, and a merge GitHub already offers is still used without taking a slot.
  • Each merge-state probe takes one of the same capacity's probe permits and waits at most two seconds for one; without a permit the delivery builds its own merge without asking GitHub.
  • Pull-request runs on an engine-built merge now read workflow files from that merge commit in the engine's mirror; only a failed mirror read falls back to the head, with a warning. Check runs still attach to the head.

Summary by CodeRabbit

  • Bug Fixes
    • Pull-request workflows now use files from the merge commit being tested. If those files can’t be read, the system falls back to the pull request’s head version and issues a warning.
    • Check runs continue to report on the pull request’s head.
  • Reliability
    • Merge checks now limit concurrent GitHub probes and waiting deliveries. Probes wait up to two seconds for capacity; when capacity is unavailable, the system builds a merge for testing.
  • Documentation
    • Clarified which revision supplies workflow files and when the system falls back to the pull request’s head.