t4947: add cross-machine dispatch dedup via assignee check + pulse jitter#4948
t4947: add cross-machine dispatch dedup via assignee check + pulse jitter#4948marcusquinn merged 2 commits intomainfrom
Conversation
…itter Root cause: multiple pulse runners evaluating the same issue simultaneously create duplicate PRs. Process-based dedup (has_worker_for_repo_issue, is-duplicate) only sees local processes — invisible across machines. Fix 1: Add is-assigned command to dispatch-dedup-helper.sh that queries GitHub assignees before dispatch. If another runner already self-assigned, skip the issue. This is the primary cross-machine dedup guard. Fix 2: Add 0-30s random startup jitter to pulse-wrapper.sh so concurrent launchd-triggered pulses don't evaluate issues at the same instant. Configurable via PULSE_JITTER_MAX (set to 0 to disable). Fix 3: Update pulse.md dispatch instructions to enforce the assignee check as a mandatory step alongside existing local process dedup. Observed: PR #4940 duplicated PR #4938 for issue #4937 because alex-solovyev's pulse dispatched 2 min after marcusquinn self-assigned, interpreting the in-progress worker as 'failed'.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical race condition that led to duplicate work dispatches across multiple runners. By implementing a GitHub-based assignee check and introducing a random startup delay, the system can now effectively prevent different machines from simultaneously processing the same issue, ensuring that work is assigned and executed uniquely. This significantly improves the robustness and efficiency of the dispatch mechanism. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a cross-machine dedup guard ( Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Runner Instance
participant PulseWrapper as pulse-wrapper
participant GitHub as GitHub API
participant Dispatch as Dispatch Logic
participant Helper as dispatch-dedup-helper
Runner->>PulseWrapper: start pulse-wrapper.sh
Note over PulseWrapper: apply startup jitter (random 0-30s)
PulseWrapper->>GitHub: gh api user
GitHub-->>PulseWrapper: RUNNER_USER (login)
PulseWrapper->>Dispatch: provide RUNNER_USER
Dispatch->>Dispatch: iterate open issues
Dispatch->>Helper: is-assigned ISSUE_NUM SLUG "$RUNNER_USER"
Helper->>GitHub: query issue assignees
GitHub-->>Helper: assignees
alt assigned to other runner
Helper-->>Dispatch: exit 0 (skip)
Dispatch->>Dispatch: skip issue
else unassigned or self
Helper-->>Dispatch: exit 1 (proceed)
Dispatch->>GitHub: gh issue edit ... assign to RUNNER_USER
Dispatch->>Dispatch: dispatch worker
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Sun Mar 15 18:20:17 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
marcusquinn
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a solid strategy to prevent duplicate work across runners by adding an assignee check and startup jitter. The changes are well-documented and address the race condition described. My feedback includes a couple of suggestions to enhance the robustness of the new shell scripts by improving input parsing and validation, ensuring they handle unexpected edge cases gracefully.
| PULSE_JITTER_MAX="${PULSE_JITTER_MAX:-30}" | ||
| if [[ "$PULSE_JITTER_MAX" -gt 0 ]]; then | ||
| # $RANDOM is 0-32767; modulo gives 0 to PULSE_JITTER_MAX | ||
| jitter_seconds=$((RANDOM % (PULSE_JITTER_MAX + 1))) | ||
| if [[ "$jitter_seconds" -gt 0 ]]; then | ||
| sleep "$jitter_seconds" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
The script assumes PULSE_JITTER_MAX is an integer. If a non-integer value is provided (e.g., from an environment variable), the script could fail on the arithmetic comparison or calculation, especially with set -e. It's safer to validate that the variable is numeric before using it in an arithmetic context.
| PULSE_JITTER_MAX="${PULSE_JITTER_MAX:-30}" | |
| if [[ "$PULSE_JITTER_MAX" -gt 0 ]]; then | |
| # $RANDOM is 0-32767; modulo gives 0 to PULSE_JITTER_MAX | |
| jitter_seconds=$((RANDOM % (PULSE_JITTER_MAX + 1))) | |
| if [[ "$jitter_seconds" -gt 0 ]]; then | |
| sleep "$jitter_seconds" | |
| fi | |
| fi | |
| PULSE_JITTER_MAX="${PULSE_JITTER_MAX:-30}" | |
| if [[ "$PULSE_JITTER_MAX" =~ ^[0-9]+$ && "$PULSE_JITTER_MAX" -gt 0 ]]; then | |
| # $RANDOM is 0-32767; modulo gives 0 to PULSE_JITTER_MAX | |
| jitter_seconds=$((RANDOM % (PULSE_JITTER_MAX + 1))) | |
| if [[ "$jitter_seconds" -gt 0 ]]; then | |
| sleep "$jitter_seconds" | |
| fi | |
| fi |
References
- When parsing delimited data from an untrusted source in a shell script, validate numeric fields before using them in calculations. This provides defense-in-depth against injection attacks that could result from delimiter shifting caused by malicious data. This applies to environment variables as untrusted input.
There was a problem hiding this comment.
Fixed in 540ac93 — added numeric validation with [[ "$PULSE_JITTER_MAX" =~ ^[0-9]+$ ]] before the arithmetic comparison.
| local IFS=',' | ||
| for assignee in $assignees; do | ||
| if [[ "$assignee" != "$self_login" ]]; then | ||
| dominated_by_self=false | ||
| break | ||
| fi | ||
| done | ||
| unset IFS |
There was a problem hiding this comment.
While using local IFS=',' with a for ... in loop is a common idiom for iterating over comma-separated values, it can be brittle if values contain whitespace. A more robust method in bash is to use read -ra to parse the values into an array. This avoids unintended word splitting and handles various edge cases more gracefully, aligning with best practices for parsing delimited data in shell scripts.
| local IFS=',' | |
| for assignee in $assignees; do | |
| if [[ "$assignee" != "$self_login" ]]; then | |
| dominated_by_self=false | |
| break | |
| fi | |
| done | |
| unset IFS | |
| local IFS=',' | |
| read -ra assignee_array <<< "$assignees" | |
| unset IFS | |
| for assignee in "${assignee_array[@]}"; do | |
| if [[ "$assignee" != "$self_login" ]]; then | |
| dominated_by_self=false | |
| break | |
| fi | |
| done |
References
- In shell scripts, the
readcommand's delimiter (set viaIFS) should always match the delimiter of the input data to ensure correct parsing. This suggestion introducesreadfor more robust parsing.
There was a problem hiding this comment.
Fixed in 540ac93 — switched to IFS=',' read -ra assignee_array for robust parsing.
|
Gemini Code Assist found 2 actionable findings. Dispatching worker to address before merge. High: Both are genuine improvements, not noise. |
…parsing Address Gemini review feedback: - Validate PULSE_JITTER_MAX is numeric before arithmetic (prevents set -e failures from non-integer env var values) - Use read -ra for comma-separated assignee parsing instead of IFS word splitting (more robust against whitespace edge cases)
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Sun Mar 15 18:24:05 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Summary
dispatch-dedup-helper.sh is-assignedto check GitHub assignees before dispatch, so multi-machine runners share a single source of truthpulse-wrapper.sh(PULSE_JITTER_MAX, default 30s) to desynchronize simultaneous pulse startspulse.mddedup sequence to include assignee guard with existing local process checksWhy
Local process dedup only sees workers on the current machine. When multiple runners evaluate the same issue at nearly the same time, both can dispatch before assignment propagates. GitHub assignee state + startup jitter prevent this race.
Verification
shellcheck .agents/scripts/dispatch-dedup-helper.sh .agents/scripts/pulse-wrapper.shdispatch-dedup-helper.sh is-assignedbehavior verified for assigned-to-other (skip), assigned-to-self (allow), and unassigned (allow)Closes #4947
Summary by CodeRabbit
New Features
Chores