t4342: stop pulse worker over-dispatch thrashing#4343
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughImplements a per-repository worker capacity limit and centralizes active-worker detection via a new Changes
Sequence DiagramsequenceDiagram
participant Pulse as Pulse Dispatch
participant Wrapper as pulse-wrapper
participant ProcMgr as Process Manager
participant Repo as Repository
Pulse->>Wrapper: request count_active_workers(repo)
Wrapper->>ProcMgr: list_active_worker_processes()
ProcMgr-->>Wrapper: filtered worker list
Wrapper->>Wrapper: compute count (wc -l)
Wrapper-->>Pulse: active_worker_count
Pulse->>Pulse: compare count vs MAX_WORKERS_PER_REPO
alt At/Above Cap
Pulse->>Pulse: log cap and skip dispatch
else Below Cap
Pulse->>Repo: dispatch new worker
Repo-->>Pulse: worker spawned
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
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: Fri Mar 13 03:03:07 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.agents/scripts/tests/test-pulse-wrapper-worker-detection.sh (1)
80-114: Add edge-case fixtures for matcher regressions.Great baseline coverage. I’d add two high-value cases:
- worker command containing
/pulsein non-supervisor context, and- repo-path prefix overlap (
/tmp/aidevopsvs/tmp/aidevops-tools).These are the cases most likely to silently reintroduce dispatch miscounts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/tests/test-pulse-wrapper-worker-detection.sh around lines 80 - 114, Add two edge-case fixtures to the tests: in test_counts_plain_and_dot_prefixed_opencode_workers add a ps entry where the command contains "/pulse" but is not the Supervisor Pulse (so count_active_workers should ignore it), and in test_repo_issue_detection_uses_filtered_worker_list add a ps entry with a repo path that is a prefix (e.g. /tmp/aidevops-tools) alongside /tmp/aidevops to ensure has_worker_for_repo_issue matches exact repo paths only; update the set_ps_fixture inputs for those tests and assert the same expected counts/boolean results so count_active_workers and has_worker_for_repo_issue are verified against these regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/commands/pulse.md:
- Around line 559-571: The new per-repo cap uses list_active_worker_processes
but the global AVAILABLE computation still relies on the legacy grep '\.opencode
run' matcher, causing inconsistent counting; update AVAILABLE calculation (and
any global capacity logic) to reuse the same list_active_worker_processes
matcher or consolidate matching logic into a single helper (e.g.,
list_active_worker_processes) used by both the global AVAILABLE computation and
the per-repo MAX_WORKERS_PER_REPO check so both count plain "opencode run" and
".opencode run" workers identically.
In @.agents/scripts/pulse-wrapper.sh:
- Around line 943-946: The exclusion pattern $0 !~ /\/pulse/ is too broad and
filters any string containing "/pulse"; update that condition to only exclude
when "/pulse" appears as a standalone path/argument (e.g. use a word-boundary or
surrounding-space-or-slash regex) so legitimate workers with "/pulse" as part of
other args/paths are not dropped; modify the condition in the same if-block that
also checks /\/full-loop/ and the opencode pattern (the lines containing $0 !~
/\/pulse/, /\/full-loop/, and $0 ~
/(^|[[:space:]\/])\.?opencode([[:space:]]|$)/) to use a narrower regex for the
pulse exclusion.
- Around line 1907-1910: Replace the substring check index($0, path) > 0 with an
exact --dir token match so sibling paths don't collide: in the awk script used
to compute matches (the block that calls list_active_worker_processes and uses
variables issue_number and repo_path), change the condition to test for a --dir
token that equals the repo path, e.g. use ($0 ~ ("--dir(=| )" path "($|[
\\t])")) instead of index(...) so the process line must contain
--dir=<repo_path> or --dir <repo_path> as a whole token; keep the rest of the
issue-number regex checks the same.
---
Nitpick comments:
In @.agents/scripts/tests/test-pulse-wrapper-worker-detection.sh:
- Around line 80-114: Add two edge-case fixtures to the tests: in
test_counts_plain_and_dot_prefixed_opencode_workers add a ps entry where the
command contains "/pulse" but is not the Supervisor Pulse (so
count_active_workers should ignore it), and in
test_repo_issue_detection_uses_filtered_worker_list add a ps entry with a repo
path that is a prefix (e.g. /tmp/aidevops-tools) alongside /tmp/aidevops to
ensure has_worker_for_repo_issue matches exact repo paths only; update the
set_ps_fixture inputs for those tests and assert the same expected
counts/boolean results so count_active_workers and has_worker_for_repo_issue are
verified against these regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab1d1ddb-4c5d-416a-9f02-7f704f9483d2
📒 Files selected for processing (3)
.agents/scripts/commands/pulse.md.agents/scripts/pulse-wrapper.sh.agents/scripts/tests/test-pulse-wrapper-worker-detection.sh
|
Dispatching worker for requested review changes.\n- Model: default alternating headless provider\n- Branch: bugfix/issue-4342-worker-dispatch-thrashing\n- Scope: address CHANGES_REQUESTED feedback and unblock merge\n- Attempt: 1 of 1\n- Direction: keep fixes tightly scoped to issue #4342 dispatch thrashing root cause |
|
Initial worker launch validation failed; re-dispatching immediately with explicit provider fallback.\n- Correction: relaunch using openai/gpt-5.3-codex\n- Reason: launch gate reported invalid startup for previous attempt |
Three precision fixes to worker detection logic: 1. pulse.md Step 1: replace legacy grep pipeline with list_active_worker_processes so global AVAILABLE uses the same matcher as per-repo caps, unifying capacity counting across both code paths. 2. pulse-wrapper.sh list_active_worker_processes: narrow /pulse exclusion from '$0 !~ /\/pulse/' to '$0 !~ /(^|[[:space:]])\/pulse([[:space:]]|$)/' to avoid hiding legitimate workers whose path contains /pulse as a non-command segment (e.g. /home/user/pulse-project/...). 3. pulse.md per-repo worker check: replace 'index($0, path) > 0' with '$0 ~ " --dir " path "( |$)"' to prevent sibling-path false matches (e.g. /Git/myrepo matching /Git/myrepo-fork). Closes #4342
|
Dispatching worker to address unresolved CodeRabbit suggestion.
|
🔍 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: Fri Mar 13 07:04:12 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
… detection - Replace substring index() path matching in has_worker_for_repo_issue with regex-escaped --dir argument boundary matching to prevent false matches on prefix-sibling paths (e.g. /tmp/aidevops-tools matching /tmp/aidevops) - Add edge-case test: worker with /pulse-related in args is not excluded by standalone /pulse filter - Add edge-case test: prefix-sibling repo path (/tmp/aidevops-tools) does not match worker scoped to /tmp/aidevops Addresses CodeRabbit review feedback on PR #4343.
🔍 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: Fri Mar 13 07:06:32 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.agents/scripts/pulse-wrapper.sh (1)
1907-1910:⚠️ Potential issue | 🟠 MajorUse exact
--dirtoken matching instead of substring path matching.
index($0, path) > 0at Line 1908 can still match sibling/prefix repos (e.g.,/Git/myrepovs/Git/myrepo-fork), causing false positives in repo+issue worker detection.Proposed fix
-matches=$(list_active_worker_processes | awk -v issue="$issue_number" -v path="$repo_path" ' - index($0, path) > 0 && ($0 ~ ("issue-" issue "([^0-9]|$)") || $0 ~ ("Issue #" issue "([^0-9]|$)")) { count++ } +matches=$(list_active_worker_processes | awk -v issue="$issue_number" -v path="$repo_path" ' + BEGIN { + esc=path + gsub(/[][(){}.^$*+?|\\]/, "\\\\&", esc) + } + $0 ~ ("--dir([[:space:]]+|=)" esc "([[:space:]]|$)") && + ($0 ~ ("issue-" issue "([^0-9]|$)") || $0 ~ ("Issue #" issue "([^0-9]|$)")) { count++ } END { print count + 0 } ') || matches=0#!/bin/bash set -euo pipefail cat <<'EOF' >/tmp/worker_lines.txt 111 00:01 opencode run --dir /tmp/aidevops --title "Issue `#42`" /full-loop 222 00:02 opencode run --dir /tmp/aidevops-fork --title "Issue `#42`" /full-loop EOF echo "Current matcher count (index substring):" awk -v issue="42" -v path="/tmp/aidevops" ' index($0, path) > 0 && ($0 ~ ("Issue #" issue "([^0-9]|$)")) { c++ } END { print c + 0 } ' /tmp/worker_lines.txt echo "Exact --dir matcher count:" awk -v issue="42" -v path="/tmp/aidevops" ' BEGIN { esc=path gsub(/[][(){}.^$*+?|\\]/, "\\\\&", esc) } $0 ~ ("--dir([[:space:]]+|=)" esc "([[:space:]]|$)") && ($0 ~ ("Issue #" issue "([^0-9]|$)")) { c++ } END { print c + 0 } ' /tmp/worker_lines.txtAs per coding guidelines "Automation scripts - focus on: Reliability and robustness".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/pulse-wrapper.sh around lines 1907 - 1910, Replace the substring path check used when computing matches with an exact --dir token match: in the block that sets matches (calling list_active_worker_processes and the awk script), stop using index($0, path) > 0 and instead have awk match the --dir token followed by the repo path (escaping regex metacharacters in path) using a pattern like --dir([[:space:]]+|=)<escaped_path>([[:space:]]|$); keep the existing Issue/issue number checks and ensure the escape/replace of special characters ([],(){}.^$*+?|\\) is performed in awk before building the regex so sibling/prefix repo names do not produce false positives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/commands/pulse.md:
- Around line 567-571: The documentation example uses an unescaped awk regex
which can mis-match paths with regex metacharacters and only matches the " --dir
<path>" form; update the example to mirror pulse-wrapper.sh by first escaping
regex metacharacters in the variable used by the awk expression (so special
chars like "." are treated literally) and change the awk match to check both
forms (" --dir " path "( |$)" and " --dir=" path "( |$)") or a combined regex
that covers both; refer to the ACTIVE_FOR_REPO assignment,
list_active_worker_processes invocation, and the awk pattern to locate where to
apply the escaped variable and expanded match.
---
Duplicate comments:
In @.agents/scripts/pulse-wrapper.sh:
- Around line 1907-1910: Replace the substring path check used when computing
matches with an exact --dir token match: in the block that sets matches (calling
list_active_worker_processes and the awk script), stop using index($0, path) > 0
and instead have awk match the --dir token followed by the repo path (escaping
regex metacharacters in path) using a pattern like
--dir([[:space:]]+|=)<escaped_path>([[:space:]]|$); keep the existing
Issue/issue number checks and ensure the escape/replace of special characters
([],(){}.^$*+?|\\) is performed in awk before building the regex so
sibling/prefix repo names do not produce false positives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c1e9e71-c510-4494-90b8-d1f454efa437
📒 Files selected for processing (2)
.agents/scripts/commands/pulse.md.agents/scripts/pulse-wrapper.sh
Three precision fixes to worker detection logic: 1. pulse.md Step 1: replace legacy grep pipeline with list_active_worker_processes so global AVAILABLE uses the same matcher as per-repo caps, unifying capacity counting across both code paths. 2. pulse-wrapper.sh list_active_worker_processes: narrow /pulse exclusion from '$0 !~ /\/pulse/' to '$0 !~ /(^|[[:space:]])\/pulse([[:space:]]|$)/' to avoid hiding legitimate workers whose path contains /pulse as a non-command segment (e.g. /home/user/pulse-project/...). 3. pulse.md per-repo worker check: replace 'index($0, path) > 0' with '$0 ~ " --dir " path "( |$)"' to prevent sibling-path false matches (e.g. /Git/myrepo matching /Git/myrepo-fork). Closes #4342
… detection - Replace substring index() path matching in has_worker_for_repo_issue with regex-escaped --dir argument boundary matching to prevent false matches on prefix-sibling paths (e.g. /tmp/aidevops-tools matching /tmp/aidevops) - Add edge-case test: worker with /pulse-related in args is not excluded by standalone /pulse filter - Add edge-case test: prefix-sibling repo path (/tmp/aidevops-tools) does not match worker scoped to /tmp/aidevops Addresses CodeRabbit review feedback on PR #4343.
alex-solovyev
force-pushed
the
bugfix/issue-4342-worker-dispatch-thrashing
branch
from
bb453ee to
0b77ad5
Compare
🔍 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: Fri Mar 13 07:15:04 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
…rapper.sh Update the pulse.md documentation example to match the actual implementation in pulse-wrapper.sh: escape regex metacharacters in the path variable and use the --dir[[:space:]]+ pattern form. Addresses CodeRabbit review feedback on PR #4343.
🔍 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: Fri Mar 13 07:19:43 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
2 participants
Summary
opencode runand dot-prefixed binary paths via a sharedlist_active_worker_processeshelper.MAX_WORKERS_PER_REPO, default 5) and add a regression test that reproduces the miscounting scenario.Verification
shellcheck .agents/scripts/pulse-wrapper.sh .agents/scripts/tests/test-pulse-wrapper-worker-detection.shbash .agents/scripts/tests/test-pulse-wrapper-worker-detection.shbash .agents/scripts/tests/test-worker-stall-diagnosis.shCloses #4342
Summary by CodeRabbit
New Features
Tests