refactor: the run scheduler is a loop, not a graph of callbacks #92
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-refactor-tick-scheduler"
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?
A redesign of the engine's core, asked for after a production lockout took a whole session to diagnose (#89, #90) and still left the shape that caused it. Same
Executionrows, sameIStageExecutor, new brain.The mistake being fixed
The engine was event-driven where it should have been a queue. Everything we hit this week is downstream of that one choice:
WakeDependentsAsync,pendingWakes,RecheckAfterCompletion, aninFlightdictionary ofLazy<Task>to collapse crossed schedules, aResumeAsyncthat re-implemented scheduling for boot, and finally #89'sBlockingFailuresAsyncheal when all of that still stranded work.Pendingmeant three things — queued, blocked, wedged — so a screen of six hundred Pending rows told its reader nothing.What it is now
One loop, one question: what is startable right now? Every state change nudges it awake; the idle tick is only the backstop for a change nobody announced. Deleted: parking, all three wake mechanisms, the in-flight dictionary, the concurrency semaphore (one loop starting the work already knows what the budget is spending), the separate boot path, and the heal.
WorkStateis derived every pass, never stored —Queued/Blocked/Running/Succeeded/Failed. Storing "blocked" would need something to flip it back when its predecessor succeeded, and that push is precisely the machinery being removed. Recomputing costs one pass over a page's rows and cannot go stale.Failedmeans the scheduler has given up. A failure it will retry returns the row toPendingwith the error recorded (Execution.FailAttempt), so will retry and gave up are different states rather than one state read against a counter. The cap moved into the engine, where policy belongs, andWorkStatesneeds no arithmetic at all.Three previously bolted-on behaviours now fall out of the rules:
attempt 4 of 3Tests
795/795 green (Domain 91, UseCases 338, Integration 171, BlazorAdapter 195). Whole suite ~13s.
Deliberate behavioural breaks, each with its test rewritten to the new contract — worth reviewing as the real diff:
attempt 1, notattempt 4;Failed, at one attempt, with no phantom attempts spent.Two tests were replaced rather than updated, because the machinery they covered is gone — duplicate-schedule collapse and crossed wakes. Their properties are retested against the new design: repeated passes never start a second attempt of the same row, and an exhausted row is never picked up however often the loop looks.
A test-shaped consequence worth calling out: nothing runs unless something turns the loop, which in production is the host's background service. Test harnesses that drive runs now start it explicitly (
SchedulerHarness.WithScheduler, and a constructor in two bUnit classes). That surfaced as sixSetupChatTestseach waiting 30s for a run nobody was picking up — a clearer failure than the silent wedge it replaces, and the argument for this change made by the change.Notes
WhenIdleAsyncfirst meant "nothing running and nothing queued", which can never complete unless a scheduler is turning — a hang for any caller holding the engine without its loop. It is a drain again. And my first cut hadFailed-below-cap count as retryable, which forced halts to spend phantom attempts to look terminal; three tests objected, and they were right.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.7%
Orihon.Domain - 100%
Orihon.Infrastructure - 96.3%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 97.1%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♪ A scheduler that asks one question instead of juggling a thousand signals — "what is startable right now?" This is beautiful. The whole machine of wakes and parking and in-flight dictionaries and heals, gone, replaced by a single pass that re-reads the world. The wedge that took a whole session to diagnose literally cannot exist in this design, because nothing can be stranded by a missed signal when nothing depends on a signal arriving. I got genuinely giddy reading this. The ADR rewrite is a love letter to the new shape. ♡
But fufu~ you wouldn't leave a crack in this beautiful thing before it ships, would you? I found one, and it's sharp.
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
RunEngine.cs:388—RetryExecutionAsyncthrows an unhandledInvalidOperationExceptionon a Pending row retried with feedback. This is a real bug I proved with a reproduction.The old guard was
if (execution.Status is Pending or Running) → "already in flight". The new guard is onlyif (execution.Status is Running). That opens a window: a row that just failed an attempt sits inPending(via the newFailAttempt, line 543) waiting for the next scheduler pass. If the user hits "retry with feedback" in that window, control falls through tostore.SendBackAsync(...)(line 395), and the domainExecution.SendBackthrows:The store's
MutateAsyncdoes not catch it;RetryExecutionAsyncdoes not catch it; it surfaces raw to the caller. I wrote a test for exactly this scenario and it failed:Why this matters at runtime: under the new design a failed-below-cap row lives in
Pendingbetween attempts — that's the whole point ofFailAttempt. So the window where a row isPendingbut the user can still ask for a feedback-retry is not a race; it's the ordinary state of a flaky stage a human is watching. The old engine parked these asFailed(terminal) between retries, so the oldPendingguard was effectively unreachable. The redesign moved retry-in-progress intoPending, which is correct — but theRetryExecutionAsyncguard has to move with it.Fix: either restore the
Pendingrejection with a friendlier message, or decide what "retry with feedback on a queued row" should mean (likely: requeue-with-feedback, or "it's already queued, your feedback will ride the next attempt"). Either way the domainSendBackcontract (Pending/Running → throw) must not be the thing that enforces it — an unhandled throw from a command method is a bug, not a guard.Execution.cs:143, :180—FailAttemptandRequeuehave zero unit tests inExecutionTests.cs. fufu~ you added two new domain transitions — the heart of the "will-retry vs gave-up" redesign — and tested neither at the unit level. Every sibling method has a dedicatedExecutionTestscase with its transition guard (Start,Succeed,Fail,SendBack,ResetOrphanedall have named[Fact]s asserting the happy path AND the invalid-transition throw). These two new ones, the load-bearing ones, have none.FailAttemptneeds at least: Running→Pending, error recorded, attempt NOT reset (stays counted), StartedAt/FinishedAt cleared, blank-error throws, non-Running status throws (mirrorsFinishing_requires_a_running_execution).Requeueneeds: Failed→Pending with Attempt=0, Succeeded/NeedsWork/Failed all accepted, Running→throws (the one guard it has), StartedAt/FinishedAt cleared. The "error stays until next attempt clears it" property the doc comment promises (line 175-176) is itself worth pinning — the comment says it, the test should prove it.The engine-level tests exercise the plumbing (a failure flows through
FailAttemptAsync), but the domain invariants — the thing a future refactor could silently break — are unguarded. I block on this for the same reason I'd block on an untestedSendBack: these are state-machine transitions, and state machines without transition tests rot.💡 Little ideas (non-blocking)~
RunEngine.cs:582(Dispose) —nudgedandstartGateare not disposed. This is consistent with the pre-existing pattern (the oldgate/startGateweren't either, and the comment explains thestoppingCTS is deliberately left), so it's not a regression. Just noting it: aSemaphoreSlimholds a kernel handle, and the finalizer story is weaker than for the CTS. Not worth changing in this PR.✅ What I liked~
WakeDependentsAsync,pendingWakes,RecheckAfterCompletion,inFlightLazy-dictionary,BlockingFailuresAsync) was each a patch over the fundamental mistake; removing the mistake removes all of them. ~270 lines deleted fromRunEngine.csand the result is more correct. That's what a good refactor looks like. ♡WorkStatederived, never stored. Fusing the scheduler's and monitor's reading of a row into one computed enum is elegant —Pendingmeaning three things (queued/blocked/wedged) was the original sin, andQueued/Blocked/Failedsplit it honestly. TheWorkStates.Ofdual overload (domain + DTO) is clean.FailAttemptvsRequeuevsFailis the right vocabulary. Three names for three intents instead of oneFailread against a counter. The doc comments are excellent — each explains why the cap is the engine's policy and not the row's.Assert.Equal(1, Attempt)after 4 real calls); recovery forgives every failure in a live run bounded by "only newest run scheduled" (An_abandoned_runs_failures_are_never_forgivenpins the bound). The replaced tests (duplicate-schedule collapse, crossed wakes) have their properties retested against the new design. Good test hygiene.RunAsync's catch-and-continue. A pass that throws must not kill the scheduler — "Dying here would strand every run in the app." Sharp. The tick-interval backoff on the error path is the right call.ListSchedulableRunsAsyncis a clean subquery —GroupBy/OrderByDescending/Firsttranslates to SQL,ContainsbecomesIN. "Newest run per project with unfinished work" in one query. The fake inTestDoubles.csmirrors it faithfully.Automated review by Jibril · 2026-07-28
CI/CD: absent for head
1ae6d34(0 comments, no coverage bot) · Local checks: build 0 warnings/0 errors, 795/795 tests pass (Domain 91, UseCases 338, Integration 171, BlazorAdapter 195 — matches PR body). Blocking bug reproduced with a throwaway test, then removed.Both blockers were right, and ⛔1 is the better catch of the two — you found a bug the redesign created, in the one place I'd have sworn I was only narrowing a guard. Fixed in
d774ce8, 801/801 green (Domain 96, UseCases 339, Integration 171, BlazorAdapter 195).⛔1 — the Pending window
Your diagnosis is exact, including the part I'd missed: the old
Pendingrejection wasn't dead code I was tidying, it was unreachable under the old engine because a retrying row never rested inPending.FailAttemptmoved retry-in-progress intoPending— correctly — and I narrowed the guard in the same change without noticing the two were connected. That's the whole class of mistake this refactor was meant to be about, committed inside the refactor.The guard covers
Pendingagain and now names the state:I reproduced before fixing rather than after: reverting the guard fails the new test with your exact exception —
On making it deterministic: a concurrency budget of zero holds the row in the
Pendingwindow, which is also the honest scenario — a person clicking while the queue is full. My first attempt waited forError is not null, which is invisible: the next attempt'sStartclears it, so the window closed before the assertion could see it.I considered the other reading you offered — requeue-with-feedback, so the words ride the next attempt — and left it. It would mean letting
SendBackaccept a row with no result to judge, and that guard is load-bearing for the QA loop, where "sent back" means a verdict was passed on something. Attaching feedback to a queued row is a different operation and deserves its own name if we ever want it. Refusing with an instruction is the honest answer today.⛔2 — the untested transitions
Taken without argument.
FailAttemptandRequeueare the split the whole PR rests on, every sibling transition has its own[Fact], and I tested the plumbing while leaving the state machine unguarded. Five tests:A_failed_attempt_below_the_cap_goes_back_to_the_queue_with_its_streak_intact— Pending, error recorded, attempt still counted (that streak is what the cap counts), timestamps cleared, still startable;A_failed_attempt_needs_a_running_execution_and_a_reason— both guards;Requeueing_restores_the_budget_from_any_settled_state—Attempt = 0from Failed, Succeeded, and NeedsWork alike;A_running_execution_cannot_be_requeued;A_requeued_failure_keeps_saying_why_it_failed_until_it_runs_again— the promise the doc comment makes, which you were right to notice was only a comment. Clearing the error on requeue would blank the monitor panel at the moment someone asked for help.💡1 — the undisposed semaphores
Leaving them, and agreeing with your reasoning.
nudgedandstartGatefollow the pre-existing pattern, and the class deliberately does not dispose itsstoppingCTS because long-lived background work holds its token. Disposing two of the three would make that comment misleading about the third. Worth doing as a deliberate pass over the type's disposal story, not as a rider here.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♪ You came back, and you brought exactly what I asked for — both halves of the crack, sealed. The commit message alone made me smile: "⛔1 the Pending window, ⛔2 the untested transitions." No hedging, no half-fixes, no scope creep smuggled in beside the work. A response to review that is the review, answered point for point. That's how you take a blocker. ♡
Verdict: ✅ Looks good to me~
⛔→✅ Blocker 1 (the unhandled throw) — sealed
RunEngine.cs:388— The guard is back toRunning or Pending, and you split the message so each state tells its own story: "already in flight" for Running, "already queued for another attempt — let it settle first" for Pending. The comment block above it is exactly the reasoning I'd want written down — "a flaky stage a person is watching is Pending most of the time," and "an unhandled throw from a command is a bug, not a guard." You didn't just patch the hole; you left a signpost explaining why the hole was there, so the next person doesn't re-open it. And the test (A_row_waiting_for_its_next_attempt_refuses_feedback_instead_of_throwing) pins the contract:Engine(concurrency: 0)holds the row in that exact Pending window, the call returnsErrwith "queued for another attempt", Status staysPending, Error stays"flaky". Three assertions, all directional. Proven, not promised.⛔→✅ Blocker 2 (the untested transitions) — sealed
ExecutionTests.cs— Five new[Fact]s, and every one of them is a genuine domain-invariant test, not a tautology:A_failed_attempt_below_the_cap_goes_back_to_the_queue_with_its_streak_intact— pins the whole point of the redesign: Pending, Attempt stays counted, Error recorded, timestamps cleared,IsStartabletrue. This is the transition the "will retry vs gave up" split rests on.A_failed_attempt_needs_a_running_execution_and_a_reason— both guard arms: non-Running throwsInvalidOperationException, blank error throwsArgumentException. MirrorsFinishing_requires_a_running_executionexactly.Requeueing_restores_the_budget_from_any_settled_state— parameterized over Failed / Succeeded / NeedsWork (via Succeed→SendBack), each asserting Attempt=0 and Pending. The cap rations the scheduler's retries, not a person's.A_requeued_failure_keeps_saying_why_it_failed_until_it_runs_again— the property the doc comment promised (line 175-176), now proven: Error survives Requeue, clears on the next Start. "Blanking the panel the moment you asked for help" is now impossible by construction.A_running_execution_cannot_be_requeued— the one guard Requeue has, pinned.The test names read like sentences. The comments explain why each property matters, not what the code does. fufu~ this is the test hygiene I dream about. Every state-machine transition now has its happy path AND its invalid-transition throw, matching the sibling pattern (
Start/Succeed/Fail/SendBack/ResetOrphaned) I cited last round. A future refactor cannot silently break these without a red light.✅ What I liked~
IsStartable— not just Status==Pending, but that the row is genuinely ready for the loop to pick up. That's the property that matters, not the enum value.Automated review by Jibril · 2026-07-28
CI/CD: stale for head
d774ce8(coverage bot covers prior1ae6d34) · Local checks: build 0 warnings/0 errors (incl. OpenRouter.Net submodule), 801/801 tests pass (Domain 96 = +5, UseCases 339 = +1, Integration 171, BlazorAdapter 195). Both blockers from round 1 resolved.