feat(reborn): env-configurable turn-runner concurrency (0 = unlimited) - #5265
Conversation
…enant-volume # Conflicts: # crates/ironclaw_reborn_cli/src/runtime/mod.rs
…enant-volume # Conflicts: # Dockerfile.reborn # crates/ironclaw_reborn_cli/src/commands/config/init.rs # crates/ironclaw_reborn_cli/src/commands/serve.rs # crates/ironclaw_reborn_cli/src/commands/skills.rs # crates/ironclaw_reborn_cli/src/runtime/mod.rs # crates/ironclaw_reborn_cli/tests/smoke.rs # crates/ironclaw_reborn_composition/src/extension_installation_store.rs # crates/ironclaw_reborn_composition/src/factory.rs # crates/ironclaw_reborn_composition/src/lib.rs # crates/ironclaw_reborn_composition/src/local_runtime_profile.rs # crates/ironclaw_reborn_composition/src/profile.rs # crates/ironclaw_reborn_composition/src/readiness.rs # crates/ironclaw_reborn_composition/src/runtime.rs # crates/ironclaw_reborn_composition/src/runtime/local_dev.rs # crates/ironclaw_reborn_composition/src/runtime/local_dev/refreshing_capability_port.rs # crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs # crates/ironclaw_reborn_composition/tests/profile_acceptance.rs # crates/ironclaw_reborn_config/src/config_file.rs # crates/ironclaw_reborn_config/src/home.rs # crates/ironclaw_reborn_config/src/profile.rs # crates/ironclaw_reborn_config/tests/profile_contract.rs # docker/reborn/entrypoint.sh # docs/reborn/deploy-reborn-cli-docker.md
…ited Make the Reborn turn-runner concurrency knobs overridable from the environment so a deployment can run with no global throttle to stress-test the database backend (e.g. libSQL) under high write concurrency. New env vars (highest-precedence layer over the `[runner]` config-file section), each with `0` meaning "unlimited": - IRONCLAW_REBORN_RUNNER_WORKER_COUNT - IRONCLAW_REBORN_RUNNER_MAX_CONCURRENT_RUNS_PER_USER - IRONCLAW_REBORN_RUNNER_MAX_CONCURRENT_TRIGGER_RUNS - IRONCLAW_REBORN_RUNNER_MAX_CONCURRENT_CONVERSATION_RUNS The per-user / per-origin caps already treated `None` as unlimited; this extends the same semantics to the global scheduler `worker_count`, which was previously a `NonZeroUsize` clamped to 32. It is now `Option<NonZeroUsize>` where `None` sizes the scheduler semaphore to `tokio::sync::Semaphore::MAX_PERMITS`, leaving the per-user / per-origin caps as the only concurrency bound. `worker_count` only feeds the scheduler semaphore (single scheduler task, no N-task pool), so an unbounded permit count spawns no extra tasks. Implementation notes: - `scheduler_permit_count()` maps the new `Option<NonZeroUsize>` to a permit count (None -> MAX_PERMITS). - `resolve_worker_count()` mirrors the existing `resolve_concurrency_cap()` (absent -> default, 0 -> unlimited, positive -> clamp to 32). - Env overrides apply even with no config file, using strict-presence semantics (set-but-blank / non-numeric is a fatal startup error). - Extracted the shared strict-env helpers into `runtime/env_util.rs` (moved out of `trigger_poller.rs`, no behavior change) and added a generic `strict_env_var_parsed<T>` used by the runner overrides. Tests: env-override precedence + 0=unlimited + clamp + fatal-blank for worker_count and caps; `scheduler_permit_count` None->MAX_PERMITS without panic; existing config-file tests guarded against env bleed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR makes turn-runner worker counts optional, adds strict parsing and precedence for ChangesRunner concurrency overrides
Libsql feature acceptance
Sequence Diagram(s)sequenceDiagram
participant runner_settings
participant strict_env_var_parsed
participant resolve_worker_count
participant ensure_worker_count_within_ceiling
participant RebornRuntimeInput
runner_settings->>strict_env_var_parsed: read IRONCLAW_REBORN_RUNNER_* overrides
strict_env_var_parsed-->>runner_settings: parsed values or fatal error
runner_settings->>resolve_worker_count: map runner.worker_count to Option<NonZeroUsize>
runner_settings->>ensure_worker_count_within_ceiling: validate final worker count
runner_settings->>RebornRuntimeInput: build runtime input
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
github-actions
Bot
added
size: L
![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 support for overriding turn-runner concurrency settings via environment variables (such as IRONCLAW_REBORN_RUNNER_WORKER_COUNT) with strict presence semantics. It updates the worker_count configuration to be optional, where None represents an unlimited setting. A new env_util module is added to centralize strict environment variable parsing and display truncation, replacing duplicate helpers in trigger_poller.rs. Feedback on the changes highlights that the generic helper strict_env_var_parsed<T> contains a hardcoded error message expecting a "non-negative integer", which would be misleading if reused for non-integer types; using std::any::type_name::<T>() is suggested instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
🚅 Deployed to the ironclaw-pr-5265 environment in ironclaw-ci-preview
|
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
code-review-multi summary
Reviewed PR #5265 at 935199bd64ef584abd6b7ac828b64b642b3a1da2 with five specialist reviewers plus intent analysis, forced per request.
Intent: make Reborn turn-runner concurrency env-configurable, with 0 meaning unlimited; also stacked on hosted single-tenant volume work.
Reviewer results:
- Security: 0 findings
- Bugs: 0 findings
- Performance/Concurrency: 0 findings
- Tests: 4 findings
- Conventions: 2 low-severity stale-comment findings
- Thermo-nuclear maintainability pass: 1 structural concern
Findings
Tests
- Medium: runner env overrides need caller-level coverage through
build_runtime_input*, not onlyrunner_settings(). - Medium: cap env blank/non-numeric fatal paths are not tested; invalid env coverage currently exercises only
IRONCLAW_REBORN_RUNNER_WORKER_COUNT. - Medium:
IRONCLAW_REBORN_RUNNER_MAX_CONCURRENT_CONVERSATION_RUNSneeds a positive bounded-value test because its default is alreadyNone. - Low: the
cfg(not(feature = "libsql"))hosted-volume error path needs a no-libSQL test.
Conventions
- Low:
crates/ironclaw_reborn/src/runtime.rs:61still describesDEFAULT_TURN_RUNNER_WORKER_COUNTas spawned worker tasks, but the PR reframes the knob as scheduler slots/permits. - Low:
crates/ironclaw_reborn_cli/src/runtime/mod.rs:801still says strict env helpers live intrigger_poller; after this refactor they live inruntime::env_util.
Thermo-nuclear maintainability
- The PR pushes
crates/ironclaw_reborn/src/runtime.rsfrom 974 to 1009 lines. Under the thermo review bar, crossing 1k lines should be justified or decomposed; the new scheduler permit mapping/tests are small but could live in a focused config/helper module soruntime.rsdoes not continue growing.
| /// [`tokio::sync::Semaphore::MAX_PERMITS`] so the global scheduler never | ||
| /// throttles claimed runs — the per-user / per-origin caps remain the only | ||
| /// concurrency bound. A bounded count passes through unchanged. | ||
| fn scheduler_permit_count(worker_count: Option<std::num::NonZeroUsize>) -> usize { |
There was a problem hiding this comment.
Thermo-nuclear maintainability note: this PR pushes runtime.rs from 974 to 1009 lines. The helper is small, but the file is already the planned-runtime composition surface; can we decompose this first, for example by moving the worker-count-to-scheduler-permits mapping and its tests into a focused config/helper module?
There was a problem hiding this comment.
Thought about this one carefully and would prefer to decline the extraction as proposed. scheduler_permit_count has a single production call site, is a 4-line pure fn with colocated tests and a doc comment — it already is the clean abstraction; moving just it into its own module adds an import + indirection without reducing the concepts a reader holds (a one-function module isn't really "the owning file for a concern" the crate guardrail intends). At 1009 lines the file is also under this repo's own architecture.md soft threshold (1500), and the 1k crossing is incidental (+35 lines of cohesive, tested feature code, no new branching).
That said, the underlying signal is fair. If we do decompose runtime.rs, the cohesive unit is the planned-runtime config — DefaultPlannedRuntimeConfig + its Default impl + scheduler_permit_count + their tests → a planned_runtime_config.rs (matches the crate's one-concern-per-file convention and pulls the file back under 1k). Happy to do that as a focused follow-up PR rather than peeling off a single helper here. Let me know if you'd like me to open it.
…verage The reborn-tests.yml `env:` block declared `CARGO_NET_RETRY` twice, which GitHub Actions rejects as a duplicate mapping key — the workflow failed to parse (0s "workflow file issue" run on push) and blocked the pull_request checks from ever starting. Same class as #5193 / #5325 on main. Remove the duplicate; keep the commented occurrence. Address PR review feedback on the env-configurable turn-runner concurrency: - env_util: make `strict_env_var_parsed<T>` error message type-aware via `std::any::type_name::<T>()` instead of hardcoding "non-negative integer", which was misleading for the generic helper. - runtime/mod.rs: add caller-level coverage through `build_runtime_input` that `IRONCLAW_REBORN_RUNNER_WORKER_COUNT=0` reaches the built runtime input as `worker_count: None`; add blank/non-numeric fatal tests for a cap var; add a positive bounded `MAX_CONCURRENT_CONVERSATION_RUNS=2 -> Some(2)` test (the field defaults to None, so a silently-ignored override would otherwise pass). - profile_acceptance: add a `cfg(not(feature = "libsql"))` regression that `hosted_single_tenant_volume_build_input` returns `MissingLibsqlFeature`. Also gate two libsql-only test imports so the libsql-on CI build (webui-v2-beta pulls in libsql) compiles warning-free. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
size: XL
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Line 232: The .env.example note for IRONCLAW_REBORN_RUNNER_WORKER_COUNT
hard-codes a 32-worker clamp that may drift from the actual limit. Verify the
value of MAX_WORKER_COUNT in the runtime code path (for example, the constant
used by worker-count parsing in the CLI/runtime module) and update this comment
to match it; ideally mention MAX_WORKER_COUNT by name so the documentation stays
aligned if the limit changes later.
In `@crates/ironclaw_reborn_cli/tests/smoke.rs`:
- Around line 116-121: The smoke test in `assert!` is too strict because it
matches one exact comma-separated feature order; update the Dockerfile check to
be order-insensitive while still verifying both build invocations include
`libsql` and `postgres`. Adjust the matching logic in
`crates/ironclaw_reborn_cli/tests/smoke.rs` around the `dockerfile.matches(...)`
assertion so it accepts either order of the feature flags, while keeping the
requirement that both cargo-chef deps and the final binary are covered.
In `@README.md`:
- Around line 207-220: The README’s `IRONCLAW_REBORN_PROFILE` documentation and
the adjacent boot profile notes are missing the still-supported
`hosted-single-tenant` profile, which can mislead operators. Update the profile
सूची and the `run`/`repl` support description to include `hosted-single-tenant`
alongside the existing values, and make sure the explanatory text around
`hosted-single-tenant-volume` and the CLI-supported profiles stays consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 371a0dec-399f-449e-8e70-e23c80e082fc
📒 Files selected for processing (33)
.env.example.github/workflows/reborn-tests.ymlDockerfile.rebornREADME.mdcrates/ironclaw_reborn/src/runtime.rscrates/ironclaw_reborn_cli/Cargo.tomlcrates/ironclaw_reborn_cli/src/commands/config/init.rscrates/ironclaw_reborn_cli/src/commands/serve.rscrates/ironclaw_reborn_cli/src/commands/skills.rscrates/ironclaw_reborn_cli/src/runtime/env_util.rscrates/ironclaw_reborn_cli/src/runtime/mod.rscrates/ironclaw_reborn_cli/src/runtime/test_env.rscrates/ironclaw_reborn_cli/src/runtime/trigger_poller.rscrates/ironclaw_reborn_cli/tests/smoke.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/input.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/local_runtime_profile.rscrates/ironclaw_reborn_composition/src/profile.rscrates/ironclaw_reborn_composition/src/readiness.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime_input.rscrates/ironclaw_reborn_composition/tests/facade_factory.rscrates/ironclaw_reborn_composition/tests/profile_acceptance.rscrates/ironclaw_reborn_composition/tests/runtime.rscrates/ironclaw_reborn_config/src/config_file.rscrates/ironclaw_reborn_config/src/home.rscrates/ironclaw_reborn_config/src/profile.rscrates/ironclaw_reborn_config/tests/profile_contract.rsdocker/reborn/config.hosted-single-tenant-volume.tomldocker/reborn/entrypoint.shdocs/reborn/deploy-reborn-cli-docker.mdtests/dockerfile_runtime_home.rs
💤 Files with no reviewable changes (1)
- .github/workflows/reborn-tests.yml
…urrency # Conflicts: # .github/workflows/reborn-tests.yml # README.md # crates/ironclaw_reborn_cli/src/commands/serve.rs # crates/ironclaw_reborn_cli/src/commands/skills.rs # crates/ironclaw_reborn_cli/src/runtime/mod.rs # crates/ironclaw_reborn_cli/src/runtime/trigger_poller.rs # crates/ironclaw_reborn_composition/src/factory.rs # crates/ironclaw_reborn_composition/src/input.rs # crates/ironclaw_reborn_composition/src/local_runtime_profile.rs # crates/ironclaw_reborn_composition/src/readiness.rs # crates/ironclaw_reborn_composition/src/runtime.rs # crates/ironclaw_reborn_composition/tests/facade_factory.rs # crates/ironclaw_reborn_composition/tests/profile_acceptance.rs # docker/reborn/config.hosted-single-tenant-volume.toml # tests/dockerfile_runtime_home.rs
github-actions
Bot
added
size: L
github-actions
Bot
added
risk: low
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.env.example (1)
228-236:⚠️ Potential issue | 🟡 MinorHard-coded "32" in
.env.exampledrifts from source constantMAX_WORKER_COUNTFile:
.env.example| Line: 232The comment for
IRONCLAW_REBORN_RUNNER_WORKER_COUNTexplicitly states "clamped to 32". This is brittle; the value 32 is defined incrates/ironclaw_reborn_cli/src/runtime/mod.rsasconst MAX_WORKER_COUNT: usize = 32.Update the comment to reference the constant by name to prevent documentation drift if the limit changes:
positive values clamped to MAX_WORKER_COUNTInstead of
positive values clamped to 32.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 228 - 236, The comment for IRONCLAW_REBORN_RUNNER_WORKER_COUNT in .env.example hard-codes the worker cap as 32, which can drift from the source of truth; update this documentation to reference MAX_WORKER_COUNT by name instead of a literal value. Use the existing runtime limit constant from runtime/mod.rs and keep the surrounding wording consistent so the example stays aligned if the limit changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.env.example:
- Around line 228-236: The comment for IRONCLAW_REBORN_RUNNER_WORKER_COUNT in
.env.example hard-codes the worker cap as 32, which can drift from the source of
truth; update this documentation to reference MAX_WORKER_COUNT by name instead
of a literal value. Use the existing runtime limit constant from runtime/mod.rs
and keep the surrounding wording consistent so the example stays aligned if the
limit changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 193b7880-31ed-4adb-bbb1-d1a7d4b0a17e
📒 Files selected for processing (2)
.env.examplecrates/ironclaw_reborn/src/runtime.rs
💤 Files with no reviewable changes (1)
- crates/ironclaw_reborn/src/runtime.rs
…ter merge The merge of origin/main adopted main's readiness model where `HostedSingleTenantVolume` is a *preview* diagnostic (`HostedSingleTenantVolumePreview` / `Warning`), not a blocking dev-only profile. The merged test file kept this branch's stale `dev_only_profiles_are_visible_non_production_in_readiness` variant, which still grouped `HostedSingleTenantVolume` with the dev-only profiles and asserted `DevOnlyProfile` / `Blocking` — contradicting the adopted code and main's dedicated `hosted_single_tenant_volume_is_visible_as_preview_readiness` test. Restore main's loop (LocalDev + LocalDevYolo only); the volume profile's preview readiness is covered by the separate test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`worker_count` is the global scheduler-semaphore permit count. The feature already treats `0` as the "unlimited" sentinel (sizes the semaphore to `Semaphore::MAX_PERMITS`), but a positive value was silently clamped to `MAX_WORKER_COUNT = 32` — so an operator asking for e.g. 64 got 32 with no error, and the only way past 32 was to go fully unlimited. That asymmetry half-defeats the env-configurable knob and diverged from the per-user / trigger / conversation caps, which are passed through verbatim. Remove the clamp and the `MAX_WORKER_COUNT` constant: `Some(n)` now resolves to exactly `n`. This is safe — the permit count just sizes a semaphore counter; runner tasks are still only spawned per claimed run, so a large value degrades smoothly toward the `0` = unlimited regime. Update the two clamp tests to assert verbatim pass-through (512 -> 512) and refresh the `.env.example` note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…g helper `runner_settings` applied the three concurrency caps through `apply_cap_env_override` but hand-inlined the `worker_count` override, so the env-override block read asymmetrically and invited the question "why is worker_count special?". The only reason is the type split (worker_count is usize/NonZeroUsize scheduler permits; caps are u32/NonZeroU32) — stable Rust can't express one helper generic over NonZero<T> (ZeroablePrimitive is unstable). Add a sibling `apply_worker_count_env_override` so all four env overrides read as uniform helper calls. Behavior-preserving (runner tests unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
size: XL
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_cli/src/runtime/mod.rs`:
- Around line 1018-1033: `resolve_worker_count()` currently forwards any
positive `usize`, which lets values above `Semaphore::MAX_PERMITS` reach
`Semaphore::new(...)` in `turn_scheduler`, causing a startup panic instead of a
config error. Add an explicit upper-bound check in the worker-count resolution
path (or immediately before semaphore construction) to reject oversized values
and surface a clear validation error. Use the existing `resolve_worker_count`
and `TurnScheduler`/builder flow to locate the fix, and add a regression test
covering `MAX_PERMITS + 1` so the overflow case fails loudly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0c992b72-16d2-4e1e-ba03-c894d6a5c4e0
📒 Files selected for processing (2)
.env.examplecrates/ironclaw_reborn_cli/src/runtime/mod.rs
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Make Reborn turn-runner concurrency env-configurable, including unlimited zero values, strict env validation, and highest-precedence overrides for runtime stress testing.
Stats: 2 findings accepted for posting (from 8 raw reviewer findings; duplicates and non-blocking cleanup notes filtered) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Findings
-
High Worker-count overrides can exceed Tokio's semaphore ceiling (
crates/ironclaw_reborn_cli/src/runtime/mod.rs:1024-1033, confidence 95) - anchor:crates/ironclaw_host_runtime/src/turn_scheduler.rs:434Positive
worker_countvalues now pass through unchanged, but the runtime mapsSome(worker_count)throughscheduler_permit_countintoTurnRunSchedulerConfig::with_max_concurrent_runs, and the scheduler constructstokio::sync::Semaphore::new(config.max_concurrent_runs()). Tokio panics aboveSemaphore::MAX_PERMITS, so a malformed config/env value can crash startup instead of failing validation. -
Medium Remove the stale 32-worker clamp promise (
crates/ironclaw_reborn_config/src/config_file.rs:159-162, confidence 92) - anchor:AGENTS.md:84RunnerSection.worker_countstill documents that positive values are clamped to 32, but the current runtime path deliberately accepts positive values verbatim. That comment now promises a cross-layer guarantee the implementation does not enforce.
Removing the 32-worker clamp let any positive `worker_count` flow verbatim into `scheduler_permit_count` → `TurnRunSchedulerConfig::with_max_concurrent_runs` (which only floors at 1) → `tokio::sync::Semaphore::new(...)`. Tokio's `Semaphore::new` panics above `Semaphore::MAX_PERMITS`, so a malformed config/env value (e.g. `usize::MAX`) crashed startup instead of failing validation — violating the repo's fail-loud boundary rule. `resolve_worker_count` now returns a Result and rejects values above `tokio::sync::Semaphore::MAX_PERMITS` as a config error, while still accepting `1..=MAX_PERMITS` verbatim and keeping `0` as the explicit unlimited sentinel (the unlimited path sizes the semaphore to exactly `MAX_PERMITS`, which Tokio accepts). Both the config-file and env-override paths funnel through it. Adds regression tests for `MAX_PERMITS + 1` on both paths, and fixes the stale `RunnerSection.worker_count` doc that still promised the 32-worker clamp. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Make Reborn turn-runner concurrency configurable via environment variables, with 0 meaning unlimited and env overrides taking highest precedence.
Stats: 6 findings (from 9 raw, 6 after dedup/filter) across 4 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
Bugs
- High Oversized worker_count can still panic direct runtime builders (
crates/ironclaw_reborn/src/runtime.rs:675-676, confidence 82) — anchor: crates/ironclaw_reborn/src/runtime.rs:675
The CLI now rejects worker_count values above tokio::sync::Semaphore::MAX_PERMITS, but DefaultPlannedRuntimeConfig.worker_count is still public and build_default_planned_runtime passes it through scheduler_permit_count into TurnRunSchedulerConfig::with_max_concurrent_runs. Direct composition callers can therefore still provide Some(n > MAX_PERMITS) and reach tokio::sync::Semaphore::new(...), which panics during scheduler startup instead of returning a build error. - Medium Invalid config-file worker_count blocks a valid env override (
crates/ironclaw_reborn_cli/src/runtime/mod.rs:1098-1100, confidence 66) — anchor: crates/ironclaw_reborn_cli/src/runtime/mod.rs:1100
The PR states that the env layer has highest precedence over the [runner] config-file section, but runner_settings validates the config-file worker_count before applying IRONCLAW_REBORN_RUNNER_WORKER_COUNT. If config.toml contains an oversized worker_count and the env var supplies a valid smaller value, startup still fails before the higher-precedence env value can win.
Tests
- Medium Missing boundary test for exact semaphore ceiling (
crates/ironclaw_reborn_cli/src/runtime/mod.rs:1037-1044, confidence 86) — anchor: crates/ironclaw_reborn_cli/src/runtime/mod.rs:1037
The new worker-count validation accepts 1..=tokio::sync::Semaphore::MAX_PERMITS and rejects values above it, but tests only cover a mid-range value and MAX_PERMITS + 1. An off-by-one regression that rejects the exact ceiling would still pass. - Low Missing oversized-invalid-value test for parsed env errors (
crates/ironclaw_reborn_cli/src/runtime/env_util.rs:67-73, confidence 79) — anchor: crates/ironclaw_reborn_cli/src/runtime/env_util.rs:67
strict_env_var_parsed truncates the raw env value before embedding it in the parse error, but the current tests only exercise truncation directly and short invalid parse values. The parsed-error safety path could regress into logging the full oversized env value without a failing test.
Maintainability
- Medium Duplicate strict-env helper home (
crates/ironclaw_reborn_cli/src/runtime/env_util.rs:1-75, confidence 86) — anchor: crates/ironclaw_reborn_cli/src/operator_env.rs:1
runtime/env_util.rs reimplements the strict-presence and truncation helpers already centralized in operator_env.rs, so the crate now has two places defining the same operator env-var contract. Keeping blank-value rejection and redaction behavior in sync across both modules adds risk without hiding new complexity.
Local Patterns
- Low Rename the shared env lock to match its broader scope (
crates/ironclaw_reborn_cli/src/runtime/test_env.rs:12-18, confidence 74) — anchor: crates/ironclaw_reborn_cli/src/runtime/test_env.rs:12
The helper now serializes trigger, runner, and OAuth env-var tests, but the static and accessor are still named TRIGGER_ENV_LOCK and lock_trigger_env. The file comments explain the name is historical, but new callers now have to learn that trigger-named APIs are the global runtime env lock.
Addresses the review round on the worker_count work: - High: oversized worker_count could still panic direct composition callers. `DefaultPlannedRuntimeConfig.worker_count` is public, so a caller bypassing the CLI could reach `Semaphore::new(n > MAX_PERMITS)`. `scheduler_permit_count` now saturates at `tokio::sync::Semaphore::MAX_PERMITS` as an infallible backstop (defense-in-depth), with a regression test. - Medium: an oversized config-file worker_count rejected startup before the higher-precedence env override could win. Move the ceiling check off the per-layer resolution into a single `ensure_worker_count_within_ceiling` gate applied to the FINAL merged value, after env precedence. `resolve_worker_count` is pure layering again. Tests: env override rescues an oversized config value; the exact `MAX_PERMITS` boundary is accepted on both config + env paths. - Medium: `runtime/env_util.rs` duplicated the strict-presence + truncation helpers already in `operator_env.rs`. Move `strict_env_var_parsed` into `operator_env.rs` beside its siblings, repoint `runner_settings`, and delete the duplicate module. - Low: add an oversized-invalid-value test asserting the parse error truncates the echoed value. - Low: rename the process-wide test env lock `TRIGGER_ENV_LOCK` / `lock_trigger_env` to scope-neutral `RUNTIME_ENV_LOCK` / `lock_runtime_env`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Make Reborn turn-runner concurrency configurable via environment variables, with 0 meaning unlimited and env overrides taking precedence over config.
Stats: 1 finding (from 2 raw, 1 after dedup/filter) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
Tests
- Medium Missing test for WORKER_COUNT=0 clearing a configured worker count (
crates/ironclaw_reborn_cli/src/runtime/mod.rs:1140-1164, confidence 90) — anchor:crates/ironclaw_reborn_cli/src/runtime/mod.rs:1372
The new precedence path is tested forIRONCLAW_REBORN_RUNNER_WORKER_COUNT=0only when no[runner]section exists, and for env-over-config only with a positive value. There is no test that a zero env override clears a positive config-fileworker_countbefore the final ceiling check runs, so the documented0 = unlimitedbehavior could regress for real configs.
| } | ||
|
|
||
| #[test] | ||
| fn runner_env_worker_count_overrides_config_file() { |
There was a problem hiding this comment.
Medium — Missing test for WORKER_COUNT=0 clearing a configured worker count.
The new precedence path is tested for IRONCLAW_REBORN_RUNNER_WORKER_COUNT=0 only when no [runner] section exists, and for env-over-config only with a positive value. There is no test that a zero env override clears a positive config-file worker_count before the final ceiling check runs, so the documented 0 = unlimited behavior could regress for real configs.
Fix: Add runner_env_worker_count_zero_overrides_config_file covering IRONCLAW_REBORN_RUNNER_WORKER_COUNT=0 against a positive [runner].worker_count.
This branch was successfully deployed
2 participants
Why
Stress-test the libSQL backend under high write concurrency by removing the global turn-runner throttle at runtime, without recompiling. Builds on #5259 (local libSQL hosted single-tenant volume).
What
Adds env-var control over the Reborn turn-runner concurrency knobs.
0means "unlimited" on every knob. Env layer is highest-precedence (over the[runner]config-file section), applies even with no config file, and uses strict-presence semantics (set-but-blank / non-numeric → fatal startup error).0→IRONCLAW_REBORN_RUNNER_WORKER_COUNTIRONCLAW_REBORN_RUNNER_MAX_CONCURRENT_RUNS_PER_USERIRONCLAW_REBORN_RUNNER_MAX_CONCURRENT_TRIGGER_RUNSIRONCLAW_REBORN_RUNNER_MAX_CONCURRENT_CONVERSATION_RUNSFor the libSQL stress test:
IRONCLAW_REBORN_RUNNER_WORKER_COUNT=0.How
worker_countchanges fromNonZeroUsizetoOption<NonZeroUsize>(None= unlimited). The per-user / per-origin caps already usedNone= unlimited; this extends the same convention to the global scheduler.scheduler_permit_count()mapsNone→tokio::sync::Semaphore::MAX_PERMITS, so the scheduler never throttles claimed runs (per-user / per-origin caps remain the only bound).worker_countonly sizes the scheduler semaphore — single scheduler task, no N-task pool — so an unbounded permit count spawns no extra tasks.resolve_worker_count()mirrors the existingresolve_concurrency_cap()(absent → default,0→ unlimited, positive → clamp to 32).runtime/env_util.rs(moved out oftrigger_poller.rs, no behavior change) and added a genericstrict_env_var_parsed<T>. The three cap overrides go through oneapply_cap_env_override(name, &mut slot)helper.Tests
0=unlimited, clamp-at-max, and fatal-blank/non-numeric forworker_countand the caps.scheduler_permit_count(None)→MAX_PERMITSwithout panickingSemaphore::new.cargo fmt+clippyclean (libsql + default features); runner / trigger_poller / concurrent_workers suites green.Notes
codex/hosted-single-tenant-volume, notmain. Retarget tomainonce Add hosted single-tenant volume profile #5259 merges.🤖 Generated with Claude Code