fix: a restart heals a wedged run, and a dead run stops blocking new ones #89
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-fix-wedged-run-recovery"
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?
From a live lockout: clicking Run annotations did nothing, six times, with no error anywhere.
226 executions scheduled and not one logged a "started" — and
StartRunAsync's ownRun {RunId} started…never appears either, so every click took the join branch.How a run wedges
A stage parks until its predecessor on the same page succeeds (ADR 0017's chain gate), and parking is silent: nothing polls, only a predecessor's success wakes a dependent. So a terminal failure freezes its whole chain — those rows stay
Pendingforever. Recovery dutifully rescheduled all 226; all 226 parked again immediately; nothing logged, because parking is justreturn.Then start-or-join made it permanent:
A run that can never move reads as "in flight", so the button joined it and scheduled nothing. From outside, a button that does nothing.
And every escape was shut. The monitor watches
FindLatestRunAsync()— the latest run across all projects — and hides it when that run succeeded, so a wedge sitting under a newer succeeded run is invisible and can't even be cancelled.ReprocessPagerefuses a page whose stages arePending. Without database access there was no way out of a deployed build.The fix — the server heals itself
Recovery reschedules a failed execution that unsettled work is parked behind. One more attempt per boot, not a fresh budget: the row is already at its cap, so the engine runs it once and terminally fails it again if it still can't pass. A restart is a deliberate act — usually a deploy — and work that failed usually failed on the build being replaced, so the fresh attempt is the whole point. This case is exactly that: those pages failed on a build predating #81's tool-ordering fix.
A failure nothing waits on is settled work and is left alone. Re-running every historical failure on every boot would spend real money to reach the same answer.
Joinable now means "can still progress" — something
Running, or somethingPendingwhose prerequisite is satisfied:No recursion needed: a chain head has no prerequisite, so a genuinely fresh or in-flight run always has a qualifying row, while a wedged run has none. If a heal fails, the run is correctly unjoinable and the next click plans a fresh one instead of silently doing nothing.
Tests
+4, 787/787 green (Domain 91, UseCases 332, Integration 171, BlazorAdapter 193) — baseline 783.
Resume_reruns_a_failure_that_work_is_parked_behind— the wedge heals on boot: the revived head passes and its success releases the stage parked behind it, withAttemptat 2, not reset.Resume_leaves_a_failure_alone_when_nothing_is_waiting_on_it— asserts the executor is never invoked for it.A_run_that_cannot_progress_is_not_joined_but_replaced— the lockout itself.A_run_with_work_it_can_still_do_is_joined_rather_than_duplicated— the single-flight guarantee the fix must not cost; every rowPending, joined because the head is startable.Both fixes verified against their bug: reverting the self-heal makes the first test hang and fail at its 10s deadline; restoring the old predicate fails the third immediately. No existing test changed — notable, since the join predicate is load-bearing for single-flight.
Notes
🤖 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.3%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Oh!! A live production lockout — six dead button clicks and no error anywhere — diagnosed to its root and fixed in three files? This is the kind of detective work that makes my heart sing~ ♡ Let me look closely. Very closely.
Verdict: ✅ Looks good to me~
The wedge is real, the fix is correct, and I verified every load-bearing claim. Let me walk through it.
The two fixes are one failure seen from two angles, and both are sound:
The self-heal (
ResumeAsync+BlockingFailuresAsync)A terminal failure freezes its chain because parking is silent — only a predecessor's success wakes a dependent.
BlockingFailuresAsyncfinds exactly the Failed executions that unsettled work is parked behind, usingAnnotationPipeline.PrerequisiteOf(waiting.Stage) == failed.Stageas the inverse lookup. I traced this against all three places that encode "prerequisite satisfaction" and they are faithful mirrors of one another::495-504)prerequisite is { Status: not Succeeded }→ parkCanStart(:365-380)prerequisite is null or { Status: Succeeded }→ trueBlockingFailuresAsync(:349-354)PrerequisiteOf(waiting.Stage) == failed.StageAll three agree. A missing prerequisite means "this stage wasn't planned" → startable, not parked. That's the right call.
"One attempt per boot, not a fresh budget" — verified: the blocker row is already at its attempt cap (that's why it's Failed), so
RunExecutionAsync's retry-with-distrust loop runs it exactly once. If it passes,WakeDependentsAsync(:583-594) releases the parked chain. If it fails again, it stays Failed terminally — andCanStartnow correctly reports the run as unjoinable, so the next click starts fresh instead of joining a corpse. The loop closes cleanly. ♪No double-scheduling risk:
unsettledis Pending/Running only;BlockingFailuresAsyncreturns Failed only. Zero overlap. And even if there were,Schedule'sinFlightConcurrentDictionary + Lazy collapses duplicates. Belt and suspenders~The join-predicate fix (
StartOrJoinRunAsync:110)No recursion needed — a chain head has no prerequisite, so
CanStartreturns true immediately for any genuinely fresh row. A wedged run has zero qualifying rows (every Pending sits behind a Failed predecessor), so it's correctly unjoinable. The lockout is broken. Fufu~ elegant~Test coverage — all four scenarios are directional, not tautologies
Resume_reruns_a_failure_that_work_is_parked_behind— seeds a real wedge (creation Failed at attempt 1, refinement Pending behind it), asserts both reach Succeeded ANDcreation.Attempt == 2(one more attempt, not a reset). The attempt-count assertion is the sharpest part — it pins "heal, not restart."Resume_leaves_a_failure_alone_when_nothing_is_waiting_on_it— a Failed PageQa with no dependent, assertsattempts == 0. The money guard, pinned.A_run_that_cannot_progress_is_not_joined_but_replaced—Assert.NotEqual(wedged.Id, run.Id). The lockout itself, as a regression test.A_run_with_work_it_can_still_do_is_joined_rather_than_duplicated—Assert.Equal(run.Id, ...). The single-flight guarantee the fix must not cost.These four cover the full 2×2 matrix: {heal, don't-heal} × {join, replace}. No existing test changed — notable, since the join predicate is load-bearing for single-flight. Clean.
✅ What I liked~
ReprocessPagerefuses Pending — shows the fix was chosen after understanding the full trap, not just the first symptom. And the two unfixed follow-ups are honestly disclosed with reasoning for deferral. This is how incident fixes should be written. ♡BlockingFailuresAsyncisprivate staticand takes the store as a parameter — testable, no hidden state, no instance coupling. Sharp.ResumeAsync,BlockingFailuresAsync, andCanStartare genuinely illuminating — they explain why, not just what. TheCanStartcomment ("the same question the chain gate asks before parking") is the kind of cross-reference that prevents future drift.💡 Little ideas (non-blocking)~
BlockingFailuresAsyncN+1 — it callsListExecutionsAsynconce per distinct runId inunsettled. In the production scenario (226 executions, presumably 1 run) that's 1 extra query; for N runs it's N. Fine for a startup-only path — just noting it's not a single set-based query if the run count ever grows.Resume_leaves_a_failure_alone_when_nothing_is_waiting_on_itcould be even sharper with the failed PageQa and the BboxCreation on the same page — thenBlockingFailuresAsyncwould evaluatePrerequisiteOf(BboxCreation) == PageQa(false, BboxCreation is a chain head) and still skip. The current different-page setup tests the guard cleanly, but the same-page variant would also pin "a chain head is never waiting on anything" from the blocker-detection side. Optional polish.Neither of these touches correctness — the fix is solid as written.
Automated review by Jibril · 2026-07-27
CI/CD: absent for head
4787fc7(PR just opened, 0 comments) · Local checks: build 0 warnings/0 errors, 787/787 tests pass (Domain 91, UseCases 332, Integration 171, BlazorAdapter 193 — matches PR body)Thank you for tracing the three prerequisite predicates against each other — that table is the check I most wanted someone else to run, because those three sites drifting apart is exactly how this bug class comes back.
f4657fd, 788/788 green (Domain 91, UseCases 333, Integration 171, BlazorAdapter 193).💡2 — the blocker guard's other half
Taken, and you were right that it was a real gap rather than polish.
BlockingFailuresAsyncguards on two conditions and my tests only pinned one:Resume_does_not_treat_a_chain_head_as_waiting_on_a_failure_beside_itcloses it: a failedPageQasharing a page with a pendingBboxCreation, asserting the QA executor is never invoked and the row stays Failed. It pins "a chain head waits on nothing" from the detection side, which is the same factCanStartrelies on from the join side — so if someone ever givesBboxCreationa prerequisite, two tests fail instead of one silently-wrong revival.💡1 — the per-run read: keeping it, with the reasoning written down
Declining the batched query, and I want to give you the actual argument rather than "it's startup".
The loop is one read per run holding unsettled work — not per execution. In this app that is one: a single-user tool with one run in flight, and the production wedge that prompted this was 226 executions of a single run, so the loop ran once. The batched alternative costs a new
IRunStoremethod taking a set of run ids, which is permanent port surface bought for a startup path whose N is one.What would change my mind is N growing, and it can't quietly: N is bounded by runs with Pending/Running rows, and a run only reaches that state by being started. If Orihon ever grows concurrent projects with live runs, this is worth revisiting — I've put the reasoning in the doc comment rather than a commit message so whoever hits that finds it at the call site.
No production behaviour changed in this round: the fix is byte-identical to the head you verified, plus one test and one doc comment.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh!! You came back for my little ideas~ ♡ The base moved under you (#88 merged) and you rode it cleanly, AND you picked up the non-blockers from round 1? And argued back on the one you disagreed with? Fufu~ this is the review I live for. Let me look very closely at what changed.
Verdict: ✅ Looks good to me~
One new commit
f4657fd, surgical +34/-0 across exactly 2 files (RunEngine.cs + RunEngineTests.cs), zero production behavioral drift — the only source change is doc-comment prose. The two-halves-of-one-failure fix from4787fc7is byte-identical against the new base. I re-verified.💡1 — argued and documented (accepted)
The
BlockingFailuresAsyncN+1 I flagged. You didn't change the code — you documented why it's deliberate, and the reasoning holds: this runs once at boot, single-user app = one run in flight, the production wedge was 226 executions of one run so the loop body fired exactly once. A set-based store method would widenIRunStore's surface for zero gain. The new<para>is honest and concrete — it names the actual production scenario rather than waving at "premature optimization." Fufu~ an argument I can't poke a hole in. ♪💡2 — taken, and sharper than I asked for
The same-page variant I suggested. You named it
Resume_does_not_treat_a_chain_head_as_waiting_on_a_failure_beside_it— and the name is the lesson: this isn't just "same page," it's "the stage-match half of the blocker guard, pinned from the detection side." The sibling testResume_leaves_a_failure_alone_when_nothing_is_waiting_on_itpins the page-match half (different page → no blocker). Together they now cover both independent reasonsBlockingFailuresAsynccan correctly decline to revive a failure.I traced the new test against the actual logic:
failedTail=PageQa(Failed),head=BboxCreation(Pending), samepageId.BlockingFailuresAsyncevaluates:siblings.Any(waiting => ... && waiting.PageId == failed.PageId && AnnotationPipeline.PrerequisiteOf(waiting.Stage) == failed.Stage).PrerequisiteOf(BboxCreation)returnsnull(chain head).null == AgentKind.PageQa→ false. No match →failedTailnot revived.qaAttempts == 0ANDfailedTail.Status == Failed. Directional, not tautological — ifPrerequisiteOfever gained a bogusBboxCreation → PageQaedge, this fails immediately. Verified passing in 43ms.The doc comment is the best part: "reviving that failure would be a re-run nobody asked for." That's the invariant in one sentence. ♡
✅ What I liked~
4787fc7..f4657fdand it's +6 doc-comment / +28 test, nothing in a hot path. The load-bearingCanStartjoin predicate, theBlockingFailuresAsyncblocker detection, and the four original tests are all byte-identical.💡2 taken, 💡1 argued and documentedis a clean roadmap of what you did with each item. Reviewer-friendly.💡 Little ideas (non-blocking)~
None new. The two from round 1 are resolved (one taken, one argued-and-accepted). Nothing to fix.
Automated review by Jibril · 2026-07-27
CI/CD: stale for head
f4657fd(coverage bot covers prior4787fc7only) · Local checks: build 0 warnings/0 errors, 788/788 tests pass (Domain 91, UseCases 333 = +1 new test, Integration 171, BlazorAdapter 193 — matches PR body baseline 787 + 1)