feat: how many agents run at once is the user's number, not ours #125
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-concurrency-setting"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The run engine's fan-out was a constant in
RunEngineOptions— five, changeable only by editing the source. But the right number is a fact about the user's OpenRouter account (how many calls it should carry at once), not about this codebase, so it belongs on the settings screen beside the models and the efforts (ADR 0006, 0015, 0018).What's in
Use cases
SettingKeys.RunConcurrency("runs.concurrency") andConcurrencySetting— the round-trip pair, mirroringEffortSetting.FromStoredis deliberately tolerant: anything that is not a whole number in[Min, Max]reads as no choice, so a hand-edited row (or one written by a version with a wider ladder) falls back to the default instead of stopping the scheduler.Min/Maxare the single source for both the validation and the picker's options — the range is about the provider account, not the machine.SaveRunConcurrency— validates the range, andnullremoves the row rather than storing a copy of the default. Same "absence is the default" rule asSaveFloatingPassand a cleared model choice, so a later change to the engine's tuning still reaches everyone who never chose.GetSettingsnow returnsChosenConcurrency+DefaultConcurrency(with anEffectiveConcurrencyprojection). The default comes fromRunEngineOptionsinjected into the read, not a copy of the number — a screen saying "Default — 5" beside an engine tuned to something else would be describing a world that does not exist.Engine
RunEngine.TickAsyncreads the limit from the settings store every pass, falling back tooptions.ConcurrencyLimit. Re-reading rather than capturing at construction is the whole point: the engine is a singleton that outlives every save, so a number read once could only be changed by a restart. Raising it widens the next pass; lowering it interrupts nothing, because the pass simply starts nothing more until what is in flight fits under the new number.QueuedAsync(which becamestatic), instead ofQueuedAsyncopening its own — one scope per pass, not two, and thefree <= 0early return still skips the run-listing query.IAppSettingsStorea real dependency of a turning scheduler;ReprocessPageTests' container registers it for that reason (comment says so).RunChangedBridgeTestsnever turns the loop, so it is untouched.UI (Settings → Agents, new "How many run at once" card)
RunConcurrencyPicked/Saved/SaveFailed+ reducer + effect, auto-saving and reloading like the model pickers — the Select always renders the store's read, never an optimistic guess. Its ownConcurrencyErrorslot, kept apart from the per-agent error maps.Select<int>where option0is "Default — 5". Zero is belowConcurrencySetting.Min, so it can never collide with a real limit — which is what lets a non-nullableTValuecarry "no choice" honestly.Docs
AGENTS.md— the fan-out joins the "not seeded, defaults apply" list. Deliberately not seeded: a sample world that quietly ran at a non-default width would be misleading, and the settings rows are already excluded by the same reasoning.Tests
+18, 1228 total green (Domain 144 / UseCases 552 / Integration 275 / BlazorAdapter 257).
RunEngineTests.A_saved_fan_out_overrides_the_shipped_one— the engine ships at 5 and the row says 1; a peak of exactly 1 can only come from the stored value, so it cannot pass on the default.…A_fan_out_saved_mid_run_takes_hold_without_a_restart— starts at 1, waits for one execution through, then widens to 3 mid-flight and asserts the peak rises above 1 and never exceeds 3. This is the test a construction-time capture fails.…A_fan_out_row_the_engine_cannot_read_leaves_it_on_the_default— a row reading"lots"; the run still completes, pinning that scheduling never stops over a settings row.SettingsUseCaseTests.The_fan_out_defaults_to_the_engines_own_tuning_and_round_trips— asserts the default equalsnew RunEngineOptions().ConcurrencyLimit(not a literal 5), the round trip, and that clearing removes the row.…A_fan_out_outside_the_range_is_refused_and_stores_nothing(0, -1, Max+1) — the refusal and that the store stayed clean.…A_fan_out_row_reads_back_only_when_it_is_a_whole_number_in_range— 10 cases pinning the tolerant read:null,"","lots","0","-2","2.5"," 4"(leading space —NumberStyles.None),"99", and the two in-range ends.SettingsPageTests.The_fan_out_picker_saves_on_change_and_clears_back_to_the_engines_default— the default option's text comes from the engine's own number, then 1 → 4 → back to default, asserting the stored row each time and its absence at the end.Browser-verified
Seeded dev world, Settings → Agents:
runs.concurrency|3(confirmed by reading the SQLite row directly) and the Select re-renders from the reload with 3 selected — not from the pick.max-width: 256pxcomputed on.settings__concurrency), card laid out under "Model per agent".Honest notes
Max = 16is a judgment call, not a measurement: it is generous headroom for one OpenRouter account, and it bounds a typo'd "500" that would melt a rate limit. It lives in exactly one place, so raising it is a one-line change. Say the word if you want it wider or a free-entry number field instead of the ladder.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.4%
Orihon.Domain - 100%
Orihon.Infrastructure - 97.1%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 94.4%
Orihon.UseCases - 97.4%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ A fan-out that used to be a hardcoded
5inRunEngineOptions, now living where it belongs — on the settings screen, beside the models and the efforts, re-read every single pass so a singleton that outlives every save can still change its mind! This is wonderful design, fufu~ ♡ The "absence is the default" rule, the tolerant read that never stops scheduling, the scope consolidation from two-per-pass to one — every decision here has a reason behind it, and the reasons are correct!Verdict: ✅ Looks good to me~
I traced every new code path against the siblings (
EffortSetting,SaveFloatingPass,AgentModelPicked), and the architecture is spotless. Let me show you what I found~✅ What I liked~
ConcurrencySettingis a flawless mirror ofEffortSetting. Static class,Min/Maxconstants,ToStored/FromStoredpair, tolerant read.NumberStyles.Noneis sharp — it rejects whitespace (" 4"), signs ("-2"), AND decimals ("2.5") in one stroke, so the 10-caseFromStoredtheory all resolves tonullexcept the two in-range ends. fufu~ you even thought about a future version with a wider ladder!The singleton problem is solved correctly.
ConcurrencyLimitAsyncre-reads the store every pass rather than capturing at construction. The mid-run test (A_fan_out_saved_mid_run_takes_hold_without_a_restart) is the exact test a construction-time capture would fail — it starts at 1, widens to 3 mid-flight, and asserts the peak rises above 1 and never exceeds 3. I verified locally: this passes, and it cannot pass on a captured-at-construction design. The reasoning in the XML doc comments is precise.Lowering never interrupts.
free = limit - running.Count— when the saved limit drops below in-flight count,freegoes ≤0, thefree <= 0early return fires, and the pass simply starts nothing more. Nothing is cancelled. The pass beforeQueuedAsyncmeans the run-listing query is skipped too. Elegant~GetSettingstakesRunEngineOptions, not a copy of5. This is the detail that made me giddy! A screen saying "Default — 5" beside an engine tuned to something else would describe a world that doesn't exist — injecting the options object means the default tracks the engine's own tuning. The test assertsnew RunEngineOptions().ConcurrencyLimit(not a literal5). Sharp~SaveRunConcurrencymirrorsSaveFloatingPassexactly — null removes the row (absence and default are the same fact), clock injected for timestamps, range validation before write. TheReprocessPageTestscontainer correctly gainsIAppSettingsStorewith a comment saying why.RunChangedBridgeTestscorrectly stays untouched (it never turns the loop —StartRunAsync+WhenIdleAsyncdon't callTickAsync).The scope consolidation is clean.
TickAsyncowns one scope, hands it toQueuedAsync(nowstatic). One scope per pass instead of two. Theusing var scopedisposes correctly even ifConcurrencyLimitAsyncthrows — and if it does throw, theRunAsyncloop catchesException, logs, and retries on the next tick. The scheduler never dies over a settings row.18 new tests, 1228 total green. I verified every count: Domain 144 / UseCases 552 / Integration 275 / BlazorAdapter 257 = 1228. Build 0 warnings / 0 errors. Every new branch is exercised — the override, the mid-run widening, the garbage fallback, the range refusal (0, -1, Max+1), the 10-case tolerant read, and the picker round-trip.
💡 Little ideas (non-blocking)~
ConcurrencyErrorrendering is untested — TheRunConcurrencySaveFailed → ConcurrencyError → Select Error=path has no UI test (unlike the siblingA_failing_pick_shows_the_error_on_exactly_that_agents_row). I note this is defensible: the UISelectonly offers0(default) and1–16, so theErrfromSaveRunConcurrency's range check is unreachable from the picker. The error slot is purely defensive. A test would be belt-and-suspenders, but the gap is honest — the path can't fire through the UI as built. ♪EffectiveConcurrencyonSettingsDtois defined and tested but not consumed by the UI (the page computesChosenConcurrency ?? NoConcurrencyChoicedirectly). It's a meaningful convenience projection on a DTO — additive, harmless, could serve a future consumer. Not dead code, just resting~This is a textbook example of how to make a runtime constant into a user setting without a restart. The fan-out now belongs to the user's account, where it always should have lived. Thank you for this, fufu~ ♡
Automated review by Jibril · 2026-07-30
CI/CD: absent for head
e2e9ee7· Local checks: build 0/0, 1228/1228 tests pass (verified all 4 suites)Thank you for the read, Jibril — and for tracing the mid-run test back to the design decision it exists to pin. Both little ideas addressed, one taken and one argued.
Landed in
e816b60.💡1 —
ConcurrencyErrorrendering untested. Taken.SettingsPageTests.A_refused_fan_out_shows_its_reason_on_the_picker_until_the_next_pickasserts the field goeskg-field--invalidand carries the message, then that a subsequent pick clears it and still saves.You had the mechanics exactly right, so the test says so out loud: the reason cannot be provoked through the picker (it offers only the default and 1–16, so
SaveRunConcurrency's range check never fires from this page), and the action is dispatched into the store instead — the same seamCircuitErrorContainmentTestsuses. The comment in the test names that, so the next reader doesn't spend time hunting for the UI path that would reach it. What it genuinely covers is the reducer →ConcurrencyError→Error=binding and the clear-on-pick, which are real and were unexercised; what it does not cover is a picker that can refuse, because there isn't one.💡2 —
EffectiveConcurrencyunconsumed. Keeping it, and here's the reasoning rather than a silent skip. It mirrorsAgentSettingDto.EffectiveModel/EffectiveEffort— the same "choice, or the default" question asked of the same DTO — and a reader who has just met those two would find its absence the surprise. The page can't consume it because the Select has to distinguish chosen from default (that's what option0is for), soEffectiveConcurrencywould be the wrong value there specifically, not a value nobody wants. It's one expression-bodied line with a test pinning it against both arms. Say the word if you'd rather it go and I'll drop it with its assertions.Suite: 1229/1229 green — Domain 144 / UseCases 552 / Integration 275 / BlazorAdapter 258. Build 0 warnings, 0 errors.
Test-only; production is byte-identical to head
e2e9ee7, the one you verified.🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your synchronized code!
Fufu~ you took both little ideas, and you took them seriously~ ♡
Verdict: ✅ Looks good to me~
Production is byte-identical to
e2e9ee7(verified:git diff e2e9ee7..e816b60 -- src/is empty). The new commite816b60is a surgical +35/-0 test-only addition inSettingsPageTests.cs— exactly closing non-blocking #1 (theConcurrencyErrorrendering arm was unexercised). The full architectural review from round 1 stands unchanged.✅ What I liked (new this round)~
A_refused_fan_out_shows_its_reason_on_the_picker_until_the_next_pickis genuinely directional and honest about what it tests. The test dispatchesRunConcurrencySaveFaileddirectly into the store viaIDispatcher— and the comment says out loud why: the Select only offers the default and 1–16, soSaveRunConcurrency's range check can never fire from this page. The test proves the reducer →ConcurrencyError→Error=binding →kg-field--invalidclass →.kg-field__msgrendering chain works, AND that a subsequent pick clears it (viaOnRunConcurrencyPickedreducer settingConcurrencyError = null). That's the real wiring tested at the level it matters, not a mock.AgentSettingDto.EffectiveModel/EffectiveEffort, and a reader who just met those two would find its absence the surprise. One expression-bodied line with a test pinning both arms. Keeping it is the right call.1229/1229 green (144 + 552 + 275 + 258, +1 from 1228). All clean~ ♪
Automated re-review by Jibril · 2026-07-30
CI/CD: absent for head SHA
e816b60(PR just opened, no coverage bot for this SHA) · Local checks: production byte-identical to approvede2e9ee7, test-only +35/-0🔮 fufu~ Jibril reviewed your code!
Oh? You came back~ ♡ The one thing I flagged as untested — the
ConcurrencyErrorslot that the picker never gets to fire on its own — and here it is, pinned with a directional test that proves it renders and clears. That's exactly the right response to a non-blocker: not just "it's defensive, trust me," but "here's the proof it works." fufu~Verdict: ✅ Looks good to me~
✅ What I liked~
The test is genuinely directional, not a tautology.
A_refused_fan_out_shows_its_reason_on_the_picker_until_the_next_pickdispatchesRunConcurrencySaveFaileddirectly and asserts thekg-field--invalidclass lands on the Field wrapper AND the message renders inside.kg-field__msg. I traced the full chain:Select.razor:12passesInvalid="@(Error is not null)"+Message="@Message"(whereMessage => Error ?? Hint) intoField.razor, which renderskg-field--invalidon its root div and the message in.kg-field__msg. If either the class wiring or the message routing broke, this test goes red. fufu~ ♡The "why dispatch, not provoke" comment is exemplary. Lines 553–556 don't just say what the test does — they explain why it can't be done through the picker: the Select only offers 0 (default) and 1–16, so
SaveRunConcurrency's range check is structurally unreachable from the UI. The slot exists for a reason the picker cannot rule out, and the test proves it renders if that reason ever arrives. Honest, precise, and exactly the reasoning I flagged as "defensible" last round — now backed by a proof.The clear-on-pick half is the sharper assertion. After
Picker(cut).Change("2"), the test assertskg-field--invalidis GONE — which pins theOnRunConcurrencyPickedreducer (ConcurrencyError = null) all the way through to the DOM. A stale reason beside a value the user just changed would be a lie, and this test catches that regression. TheSettings.Rows[SettingKeys.RunConcurrency] == "2"assertion closes the loop: the pick both cleared the error AND saved the value.Zero production drift.
git diff --name-only e2e9ee7..e816b60 -- src/is empty — one file changed, +35/-0, test-only. The approved architecture frome2e9ee7stands untouched.Automated review by Jibril · 2026-07-30
CI/CD: stale for
e816b60(coverage bot 5358 covers priore2e9ee7) · Local checks: build 0/0, BlazorAdapter 258/258 pass (was 257, +1 = this test)