Writer cap 3 by default, ceiling 8, --max-writers; refusals visible under --detach - #4
Conversation
…з при --detach Лимит писателей был константой 2 в четырёх несогласных местах: таблица PARALLEL разрешала 2–3 слайса, строка Limits — 2 писателя, README обещал «larger fleets» без механизма, а env-переопределение в agent-run нигде не документировалось. - agent-run: cap 3 по умолчанию, ceiling 8; workers = writers + 3 (раньше константа 4 упиралась в panel/led при живых писателях); --max-writers N, DELEGATE_KIT_MAX_WRITERS, DELEGATE_KIT_MAX_WORKERS не ниже writers + 1; нативные lock'и репозитория входят в счёт. - agent-run: проверки потолка и lock'а worktree до fork супервизора — при --detach отказ печатается родителю; отказ внутри супервизора пишет meta со статусом failed вместо тихого исчезновения. - agent-wt lock: тот же потолок по lock'ам репозитория, --max-writers N. - SKILL.md, README, external.md, hosts.md: одно правило вместо четырёх; SKILL.md ужат до 7962 байт — CI на main падал на 8406. - tests/caps.sh + шаг CI. Closes #3 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c1340daab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| local held; held=$(locked_worktrees | tr '\n' ' '); held=${held% } | ||
| local n=0; [ -n "$held" ] && n=$(wc -w <<<"$held" | tr -d ' ') | ||
| if [ "$n" -ge "$cap" ]; then |
There was a problem hiding this comment.
When multiple native writers are launched concurrently, each agent-wt lock can observe the same pre-lock count here and then create its own lock at line 139, allowing all of them past the cap. For example, eight simultaneous agent-wt lock <name> --max-writers 1 calls all succeeded and left eight locked worktrees. agent-run has the same check-then-acquire window, so the advertised ceiling can also be exceeded by concurrent external/native starts; serialize the count-and-reservation through a repository-wide atomic lock.
Useful? React with 👍 / 👎.
| const running = activeRuns(); | ||
| if (running.length >= caps.workers) die(`max ${caps.workers} active workers reached (${running.map((r) => r.id).join(", ")}). The worker cap is writers (${caps.writers}) + ${WORKERS_HEADROOM}; raise --max-writers or DELEGATE_KIT_MAX_WORKERS.`); | ||
| if (!write) return; |
There was a problem hiding this comment.
When native writer locks already exist and read-only external workers are launched, this total-worker check counts only activeRuns(), which contains external runs. Because checkCaps returns immediately for non-writers at line 262, known native writers are never included in the writers + 3 total: with three native writers and the default cap, six external reviewers can run, yielding nine workers instead of six. Include native locks in the total before applying the read-only early return.
Useful? React with 👍 / 👎.
| checkCaps(caps, write, cwd); | ||
| if (write) { const conflict = writeLockConflict(cwd, null); if (conflict) die(conflict); } | ||
|
|
||
| if (opts.detach && !opts._supervise) { |
There was a problem hiding this comment.
When a --detach request is rejected by a cap or existing worktree lock, these preflight checks call die before the detached branch allocates an ID or starts a supervised run. Consequently no failed metadata is recorded and agent-run list/status cannot report the refusal, contrary to the newly documented promise that refused detached runs are recorded as failed (the new test even asserts that the run count stays unchanged). Allocate the run identity and persist the failure before exiting, or remove the advertised recording contract.
Useful? React with 👍 / 👎.
…k'и в общем счёте По находкам ревью PR #4 (Codex P1/P2, Macroscope): - окно между чтением чужих meta/lock'ов и записью своего пропускало параллельные старты мимо потолка — воспроизведено: восемь `agent-wt lock --max-writers 1` давали восемь lock'ов. Теперь agent-run держит файл с флагом wx в state dir от проверки до появления meta.json, agent-wt — атомарный mkdir в общем .git; труп по мёртвому pid снимается; - нативные lock'и входят и в общий счёт воркеров для read-only прогонов; - документация: отказ при --detach родитель печатает, как failed записывается только отказ внутри супервизора. tests/caps.sh: гонка 8 lock'ов, трупы мьютексов, общий счёт — 43 проверки. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| const holder = Number(fs.readFileSync(CAPS_MUTEX, "utf8").trim() || 0); | ||
| if (!holder || !alive(holder)) { try { fs.rmSync(CAPS_MUTEX, { force: true }); } catch {} continue; } |
There was a problem hiding this comment.
🟠 High scripts/agent-run:276
Contended agent-run invocations terminate with ENOENT instead of retrying when the current mutex holder releases CAPS_MUTEX between the EEXIST failure and readFileSync. Handle a missing mutex file as a retry condition so the invocation can acquire the lock normally.
try { fs.writeFileSync(CAPS_MUTEX, String(process.pid), { flag: "wx" }); capsMutexHeld = true; return; } catch (e) { if (e.code !== "EEXIST") throw e; }
- const holder = Number(fs.readFileSync(CAPS_MUTEX, "utf8").trim() || 0);
+ let holder;
+ try { holder = Number(fs.readFileSync(CAPS_MUTEX, "utf8").trim() || 0); } catch (e) { if (e.code === "ENOENT") continue; throw e; }
if (!holder || !alive(holder)) { try { fs.rmSync(CAPS_MUTEX, { force: true }); } catch {} continue; }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @skills/delegate-kit/scripts/agent-run around lines 276-277:
Contended `agent-run` invocations terminate with `ENOENT` instead of retrying when the current mutex holder releases `CAPS_MUTEX` between the `EEXIST` failure and `readFileSync`. Handle a missing mutex file as a retry condition so the invocation can acquire the lock normally.
| * pid. Держится от проверки до появления meta.json — для отсоединённого | ||
| * прогона его пишет супервизор, поэтому родитель ждёт файл, не отпуская. | ||
| */ | ||
| const CAPS_MUTEX = path.join(STATE_DIR, "caps.lock"); |
There was a problem hiding this comment.
🟠 High scripts/agent-run:270
--max-writers 1 can be exceeded when an agent-run writer races with agent-wt lock: both can observe one free writer slot and create an external and native lock, so two writers run despite the advertised combined cap. CAPS_MUTEX only serializes agent-run processes, while agent-wt lock uses the repository-local .git/delegate-kit.caps.lock; both entry points must use a shared mutex for the combined count-and-create operation.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @skills/delegate-kit/scripts/agent-run around line 270:
`--max-writers 1` can be exceeded when an `agent-run` writer races with `agent-wt lock`: both can observe one free writer slot and create an external and native lock, so two writers run despite the advertised combined cap. `CAPS_MUTEX` only serializes `agent-run` processes, while `agent-wt lock` uses the repository-local `.git/delegate-kit.caps.lock`; both entry points must use a shared mutex for the combined count-and-create operation.
| done | ||
| die "another agent-wt (pid $pid) has held $d for 15s; if it is stuck, kill it or remove the directory" | ||
| } | ||
| locked_worktrees() { |
There was a problem hiding this comment.
🟠 High scripts/agent-wt:127
agent-wt lock can exceed the combined repository writer cap when writers occupy ordinary linked worktrees, because locked_worktrees scans only wt_base_dir() and omits locks recorded under the common Git directory by agent-run. Make the count include those external worktree locks before allowing a new lock.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @skills/delegate-kit/scripts/agent-wt around line 127:
`agent-wt lock` can exceed the combined repository writer cap when writers occupy ordinary linked worktrees, because `locked_worktrees` scans only `wt_base_dir()` and omits locks recorded under the common Git directory by `agent-run`. Make the count include those external worktree locks before allowing a new lock.
| const now = nowIso(); | ||
| const result = { status: "failed", summary: `agent-run refused to start the worker: ${msg}`, changes: [], checks_run: [], not_verified: [], findings: [], plan: [], questions: [], sources: [], next_steps: [] }; | ||
| fs.mkdirSync(runDir(SUPERVISED.id), { recursive: true }); | ||
| saveMeta({ ...SUPERVISED, dispatch: "external", status: "failed", pid: process.pid, started: now, finished: now, error: msg, result }); |
There was a problem hiding this comment.
🟡 Medium scripts/agent-run:129
The detached parent can return the stale {"status":"starting"} response instead of the recorded failed refusal. saveMeta writes meta.json non-atomically, so the parent can observe the file after truncation but before the JSON is complete; metaOf(id) then returns null and the polling path never retries. Write metadata atomically (for example, to a temporary file followed by a rename) or retry parsing when metaOf(id) returns null.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @skills/delegate-kit/scripts/agent-run around line 129:
The detached parent can return the stale `{"status":"starting"}` response instead of the recorded `failed` refusal. `saveMeta` writes `meta.json` non-atomically, so the parent can observe the file after truncation but before the JSON is complete; `metaOf(id)` then returns `null` and the polling path never retries. Write metadata atomically (for example, to a temporary file followed by a rename) or retry parsing when `metaOf(id)` returns `null`.
…бход lock'ов По второму раунду ревью PR #4 (Macroscope High ×3): - внешний и нативный писатель одного репозитория считали слоты под разными замками: agent-run теперь берёт и репозиторный mkdir-мьютекс, общий с agent-wt lock; порядок машинный → репозиторный, agent-wt берёт только второй — цикла нет; - ENOENT между EEXIST и чтением pid (держатель отпустил) — повтор, не падение; - agent-wt считает lock'и всех linked worktree из общего .git, а не только каталога <repo>.worktrees: писатель `agent-run --cwd` может сидеть где угодно; - writeJson через tmp + rename: читатель никогда не видит обрезанный meta.json. tests/caps.sh: трупы обоих мьютексов, process-lock в чужом worktree — 47 проверок. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
Closes #3.
Problem
The writer cap was a constant (
2) stated in four places that disagreed: the PARALLEL row allowed 2–3 slices, the limits line said 2 writers, the README promised "larger fleets" with no mechanism, and the env override inagent-runwas undocumented. Under--detacha cap or lock refusal happened inside the supervisor withstdio: "ignore"and the run silently never existed. Native writers were not counted at all.Change
agent-run: writer cap 3 by default, ceiling 8; workers = writers + 3 (the old constant 4 collided withpanel/ledbeside live writers).--max-writers Nper run,DELEGATE_KIT_MAX_WRITERSper session,DELEGATE_KIT_MAX_WORKERSfloored at writers + 1. Native locks (agent-wt lock) of the target repository count toward the cap.agent-run: cap and worktree-lock checks run before the detach fork, so the parent prints the refusal. A refusal inside the supervisor (race) writesmeta.jsonwithstatus: failedand the reason;status/listshow it.wxfile in the state dir foragent-run, an atomicmkdirin the common.gitforagent-wt; stale holders are detected by dead pid. Reproduced before the fix: eight parallelagent-wt lock --max-writers 1left eight locks. Native locks now count toward the total worker cap for read-only runs too.agent-wt lock: same cap over the repository's locked worktrees,--max-writers N.external.mdand--help. SKILL.md trimmed to 7954 bytes — CI onmainhas been failing the 8000-byte check since 33b73ed (8406 bytes). Wording only, every rule kept.tests/caps.sh(36 checks, synthetic runs, no CLI spawned) plus a CI step.Checks
bash skills/delegate-kit/tests/caps.sh— 43/43 (incl. the 8-way lock race and stale-mutex recovery)bash skills/delegate-kit/tests/delivery.sh— 25/25node --check,bash -n,shellcheck -S warning, schema JSON, SKILL.md size — all green locallyNot verified
No real Claude/Codex worker was launched; the pass-through path (cap satisfied, lock free, CLI spawned) is exercised only up to the point where the old code already ran unchanged.
🤖 Generated with Claude Code
Note
Add writer cap 3 to
agent-runandagent-wtvia--max-writerswriters + 3, overridable via--max-writersorDELEGATE_KIT_MAX_WRITERS--detachrefusals now write failedmeta.jsonandresult.jsonso detached parents see the failure reasonagent-runandagent-wtrefuse starts and locks past the new caps, replacing the old fixedMAX_WORKERS=4andMAX_WRITERS=2limitsMacroscope summarized 658d3e6.