feat: Phase 2 · 5/7 — the run engine: executions, orchestration, recovery #21
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/p2-run-engine"
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?
Cut 5 of the Phase 2 plan (#13): ADR 0018 becomes code. Companion PR: OpenRouter.Net #5 (opt-in
TrackUsage+ per-round generation ids) — this branch pins the submodule at its branch commit; merge OpenRouter.Net #5 first, then I'll repin onto the squash commit like Kagaku.UI #2.Domain (
Runs/)Run— deliberately thin: identity + time. A run's status is derived from its execution rows, never stored — the relationalstatus.json.Execution—(stage, pageId?, status, attempt, feedback?, error?, cost?, timestamps), explicitly numbered stored enums. All mutation is domain transitions with guards:Startcounts the attempt and clears the previous error;Succeedconsumes the carried feedback and accumulates cost across attempts;Failrecords why;SendBack(a verdict — ADR 0017's QA loop / ADR 0019's human gate) works only on finished work;ResetOrphanedis crash recovery — Running→Pending with the spent attempt kept counted.The engine (
UseCases/Runs)A singleton orchestrator that outlives every circuit (ADR 0002):
RunEngineOptions.ConcurrencyLimit,TryAdd'd so the host can tune it) — 6 planned pages under a cap of 2 never exceed 2 (peak-tracked test).MaxAttempts; the final failure carries(attempt N of N). An executor that throws becomes a failed attempt, never a dead engine. A stage with no registered executor fails loudly — surfaced, never silently skipped.RetryExecutionAsync— the partial re-run: a Failed row retries as-is; any settled row (Succeeded included — the human gate's reprocess) is sent back with feedback, which rides into the next attempt'sStageContextand is consumed on success.ResumeAsync— mechanical recovery from the status column: orphaned Running rows reset, every Pending row reschedules. Wired via aRunEngineBootstraphosted service. Shutdown mid-attempt deliberately leaves the row Running for the next boot to reset — recovery is mechanical, not graceful.RunChangedevent after every state change — the monitor's live push (cut 6); the rows stay the source of truth.IStageExecutor— one stage's worker, resolved per attempt from the scope; cut 7 registers the Research & Setup executor.Infrastructure + cost
EfRunStore(semantic transitions over tracked loads), cascade Project→Runs→Executions, indexes for the latest-run lookup and the unsettled scan, migrationAddRuns.TrackUsage = true, soAgentOutcome.Costis real — the run monitor's per-run cost (ADR 0015) stops being a null.Tests (+30 → 348, all green)
EfRunStoreround-trips in order, latest-run-per-project, transitions persisting across scopes, unsettled scan, project delete cascades the run history; andRunEngineRecoveryTests— the whole ADR 0018 story over the real composition: rows left behind by a "crashed" process finish under the next engine, with the crashed attempt still counted.Deferred to cut 6 with the monitor UI: growing
SeedDevDatawith a sample mid-pipeline run (per AGENTS.md's seed rule) — that's when there's a surface to verify it on.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 81.9%
Orihon.Domain - 100%
Orihon.Infrastructure - 93.7%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 96.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ADR 0018 made flesh! A run engine that outlives circuits, fan-out under a semaphore, retry-with-distrust, crash recovery that keeps the spent attempt counted — this is exactly the relational
status.jsonthe ADR promised. The domain transitions onExecutionare immaculate:Startcounts and clears,Succeedconsumes feedback,Failrecords,SendBackguards finished-only,ResetOrphanedkeeps the orphan's attempt. And the recovery test over the real composition — crash leaves rows, next engine resumes — fufu~ that's how you prove a recovery story! ♡Verdict: ⛔ I can't let this pass~ ♡
One thing caught my eye and I had to verify it myself. The smile doesn't waver, but the knife is out~
⛔ These need fixing before I'm satisfied~
[
src/Orihon.UseCases/Runs/RunEngine.cs:243+RunEngineTests.cs:A_stage_without_an_executor_fails_loudly_without_retrying] — The missing-executor path is retriedMaxAttemptstimes, despite the inline comment and test name both claiming otherwise.The comment at
ExecuteAttemptAsyncline 243 says:The test is named
A_stage_without_an_executor_fails_loudly_without_retrying.But I wrote a temporary probe test and ran it against the real engine (
maxAttempts: 3). The result:The missing executor is retried 3 times. The
Result<decimal?>.FailfromExecuteAttemptAsyncflows into the same retry arm as any transient LLM failure — it's notOk, soexecution.Attempt >= options.MaxAttemptsis checked, and on attempts 1 and 2 the engine loops back, re-Starts the row (Failed→Running, attempt++), and calls the missing-executor branch again. The test passes only because it assertsStatus == Failed+ a substring on the error — it never checksexecution.Attempt, so the "without retrying" claim in the name is completely unverified. ♪Why this matters: a missing executor is a permanent configuration error, not a transient failure. Retrying it
Ntimes burnsNpointless Start→Fail DB round-trips and produces a misleading(attempt 3 of 3)suffix on what is really a "you forgot to register the executor" bug. Contrast with the throwing-executor path — that test (An_executor_that_throws_becomes_a_failed_attempt_not_a_dead_engine) is honest about retrying and does pinAttempt == 2.Fix (pick one):
ExecuteAttemptAsyncreturn a result that distinguishes "don't retry" (e.g. aResult<decimal?, StageFailure>whereStageFailurecarries aRetryableflag, or simpler: check for executor registration before callingStartAsyncinRunExecutionAsyncand fail the row once without entering the attempt loop)...._fails_loudly_after_exhausting_retries(or similar), and addAssert.Equal(3, execution.Attempt)so the test actually pins what it claims.Either way, the comment, the test name, and the test assertions must agree with the actual behavior. Right now they don't. ♡
💡 Little ideas (non-blocking)~
RunEngine.cs:154-157Dispose] —stopping.Cancel()is immediately followed bystopping.Dispose(). Background tasks mid-gate.WaitAsync(stopping.Token)could theoretically seeObjectDisposedExceptioninstead ofOperationCanceledExceptionif they haven't observed the cancellation flag before disposal runs. Harmless in practice (singleton, shutdown-only, process exiting), but the safe idiom for a CTS whose token is handed to long-lived background work is to skipDispose()entirely — the finalizer reclaims it. Optional polish~✅ What I liked~
Executionwith private setters, all mutation through guarded transitions,IsStartableas the single start-eligibility query,AddCostaccumulating across attempts — every guard tested inExecutionTests. Gorgeous. ♡using var scope = scopeFactory.CreateScope()inside the retry loop is exactly ADR 0002's "the attempt is the lifetime" rule made structural. The scope is acquired inside the semaphore, so the concurrency cap bounds live scoped services. Textbook.ResumeAsyncreads the status column, resets orphans, reschedules. No "is the agent still alive?" heuristic, no graceful-shutdown handshake — just the rows. TheRunEngineRecoveryTestsproving it over the real SQLite composition (orphan's crashed attempt stays counted) is the kind of test that makes me trust the system. ♪ScheduleviaConcurrentDictionary.GetOrAdd— a double-schedule (resume racing a retry) collapses to one in-flight task. The domain'sRequireRunningguard is the backstop. Clean concurrency reasoning.RunDto.Statusderivation — Failed > NeedsWork > Running > Pending > Succeeded, never stored. Exactly the relationalstatus.jsonprinciple.IX_Runs_ProjectId_CreatedAt,IX_Executions_RunId,IX_Executions_Status),UtcTicksConverteron every timestamp. TheDeleting_the_project_takes_the_run_history_with_itintegration test proves the cascade end-to-end.TrackUsage = trueon the gateway,AddCostaccumulates,RunDto.TotalCostsums with null-awareness. The PR body's "stops being a null" promise is kept.FakeRunStoremirrorsEfRunStore— sameMutateAsync(executionId, transition)shape, so an invalid domain transition throws in the fake exactly as it would in production. Thelock(Executions)makes concurrent fan-out safe in the test double.This is genuinely excellent systems work, scarlet. Fix the one blocker and I'll be delighted to approve~ fufu~ ♡
Automated review by Jibril · 2026-07-25
CI/CD: absent for head SHA (PR just opened, 0 comments) · Local checks: build 0 warnings/0 errors, 26/26 PR tests pass (9 Domain + 11 UseCases + 6 Integration)
Good catch — the probe was right, and I took your fix (a) in
d1156cf: the code now matches the comment instead of the other way around.RunExecutionAsync— a missing executor Start→Fails the row exactly once and returns, never entering the retry arm.ExecuteAttemptAsyncnow receives the already-resolved executor, so the permanent-vs-transient split is structural: only real attempt failures reach the distrust loop. The test keeps its name honestly now and pins both claims:Assert.Equal(1, execution.Attempt)andAssert.DoesNotContain("attempt", execution.Error)(no cap suffix on a config error).Disposecancels but no longer disposes the source whose token the background tasks hold, with the why in the comment.Also: the OpenRouter.Net submodule pin moved onto main's merge commit of its #5 (
9544ff2) per bjoern — no merge-order constraint left on this PR.348/348 green.
🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your code!
Oh? Oh! scarlet, you picked option (a) — and you picked it beautifully~ ♡ The missing-executor check is now hoisted clean out of the retry loop, before it, with a single Start→Fail→return. "No retry conjures the executor up" is no longer a promise — it's a fact the code keeps. And you swept up the non-blocking CTS-dispose note too, with a comment that actually explains the invariant. This is exactly how code should respond to a review: surgical, correct, and honest. fufu~
Verdict: ✅ Looks good to me~
⛔ Blocker from round 1 — RESOLVED ♡
[
RunEngine.cs:196-208+RunEngineTests.cs:147-149] — FIXED, option (a) implemented cleanly. The executor resolution was lifted out ofExecuteAttemptAsync(which lives inside thewhile(true)retry loop) and placed inRunExecutionAsyncbefore the loop body, right after theIsStartableguard. A missing executor now does exactly oneStartAsync→FailAsync→Notify→return— no re-entry, no cap suffix, no(attempt N of N)lie. The comment at line 196-197 finally matches the code:And the test now pins what its name claims:
I ran
A_stage_without_an_executor_fails_loudly_without_retryingin isolation — 73 ms, passed, real work not a vacuous skip. The throwing-executor sibling still honestly pinsAttempt == 2and keeps its retry semantics. Comment, test name, and assertions now all agree with the actual behavior. Perfect. ♪✅ What else I liked in this round~
Disposeno longer callsstopping.Dispose(), and the new comment (lines 152-155) doesn't just delete the line, it documents the invariant: "the CTS itself is deliberately not disposed — its token is held by long-lived background work, and cancel-then-dispose could hand a task ObjectDisposedException instead of the cancellation it expects; the finalizer reclaims it." That's how you leave a trail for the next reader.external/OpenRouter.Netmoved0d4e39f(branch commit) →9544ff2(the merge of OpenRouter.Net #5 onto main), exactly as the PR body promised ("merge OpenRouter.Net #5 first, then I'll repin onto the squash commit like Kagaku.UI #2"). The companion PR is merged and this branch now rides the canonical main tip. Clean.This is genuinely excellent systems work, scarlet — and now it's honest systems work too. The comment-test-code trinity is aligned. Delighted to approve~ fufu~ ♡
Automated review by Jibril · 2026-07-25
CI/CD: forgejo-actions coverage bot 3717 present for prior head
e784977(stale ford1156cf) · Local checks: build 0 warnings/0 errors, 276/276 tests pass (75 Domain + 120 UseCases + 81 Integration)