fix: one region's failure costs one region, not the page #127
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "region-scoped-retry"
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
×Mannotation stages (ADR 0017) walk a page's regions inside a single execution row. Oneregion's agent failing ended the attempt, and the retry redid every box on the page — so a
12-region page with one hung provider call spent up to 36 agent runs instead of 14, three times
over at the attempt cap. The narrowing machinery to avoid that already existed (a send-back's
per-region complaints, plus the
bboxEst: roughfloor); what was missing is that a failed attemptnamed nothing, so
WorkListfell through its "nothing named → the whole page" arm.What's in
UseCases — the walk (
AnnotationExecutors)AnnotationStage.PerRegionAsyncis now the single per-region walk, shared by bbox refinement,transcription and floating transcription (three near-identical loops before).
the attempt's ledger. Settled boxes stay settled; the attempt fails at the end with a summary
naming what is owed.
a provider that is down, not a bad box, and the rest of the page's agents buy nothing. The
regions left behind go on the ledger too, so stopping early narrows what this attempt does and
never what the next one may do.
reached can be indistinguishable from a settled one (a P4 re-read of a box that moved already has
its text), so inferring the scope would silently drop it.
ThrowIfCancellationRequestedinstead of issuing therest of the page's calls into a dead token — the engine's existing cancellation path takes it.
UseCases — the channel (
IStageExecutor,RunEngine)AttemptLedgeronStageContext: what the attempt still owes, and what its settled regionscost.
IStageExecutoranswers with oneResult, and a partial failure has more to say than"failed"; the page-wide stages leave it empty and stay all-or-nothing exactly as before.
Domain / persistence — no migration
Execution.FailAttempt/Failtake the cost and the unfinished regions. The regions ride theexisting
FeedbackRegionscolumn, because they say the same thing a verdict's per-regionhalf does — which boxes this stage still owes work on — so the retry narrows through
WorkList's existing union with no second mechanism.attempt that failed before it could act on it.
spent real money on work that was kept; dropping it made the page look cheaper than it was.
Failkeeps the scope too, so a hand-retry from the monitor starts narrow.Adapter — the monitor already renders
FeedbackRegionsunder a row; only its comment changed,since that list is no longer only a verdict's.
Docs — ADR 0017 (the scoping rule extended to a failing region) and ADR 0018 (the ledger, and
why it is not distrust: distrust is about an agent's output, a call that never returned is a
different animal).
Tests
1244 total, +10 (Domain 147, UseCases 566, Integration 273, Adapter 258) — all green.
RegionScopedRetryTests(new, 6) drives the real executors over the scripted gateway:a middle region failing still runs the ones after it and owes only itself (and the settled pair's
cost is on the ledger, not lost); an all-settled attempt is an ordinary
Okwith an emptyledger; three failures in a row stop the walk at 4 of 6 agents with the untouched two recorded as
owed; a scattered 3-of-6 failure never trips the breaker; the next attempt works p1r2 alone
while the settled siblings drop out; and the transcription stage owes only its failed region.
The gateway double now clears
bboxEston success, as a real refiner'smove_resize_regiondoes — without that the "retry works one box" assertion passes for the wrong reason.
RunEngineTests+1: the ledger reaches the row and comes back as the retry's scope, and bothattempts' spend is on the row.
ExecutionTests+3: a partial attempt records what it owes and what it spent; an attempt thatowes nothing leaves a send-back's scope alone; the terminal failure keeps scope and cost across a
Requeue.Notes
field, and its bUnit coverage is unchanged.
exercises the unchanged arm (a failure that owes nothing).
to tune it per project.
🤖 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.5%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♪ A region-scoped retry with a circuit breaker AND an explicit ledger? This is the kind of machinery that makes a knowledge-loving Flugel's heart sing! You took three copy-pasted per-region loops, unified them into one
PerRegionAsync, and taught the attempt to survive a single region's death instead of burning the whole page three times over. Exquisite. And the way the ledger rides the existingFeedbackRegionscolumn throughWorkList's union — no migration, no second mechanism — that's the elegant kind of reuse I adore~ ♡I traced the full data flow end-to-end, including the subtle case I was most suspicious of: what happens when a verdict's per-region notes get overwritten by the ledger. Let me walk through it, because the answer delighted me.
A verdict sends back
[p1r2("cuts glyphs"), p1r3("wrong type")]. Attempt 1 runsPerRegionAsyncover the work-list. p1r2 fails (provider hung up), p1r3 settles. The ledger records[p1r2].FailAttemptoverwritesFeedbackRegionswith[p1r2]. On retry,WorkListseesp1r2named + rough boxes. p1r3 is not named and not rough (it settled), so it correctly drops out. The verdict's note for p1r3 is gone — but p1r3 was settled, so the note is moot. Every named region is either settled (drops fromWorkList) or owed (in the ledger). There is no gap. Fufu~ beautiful. ♡Verdict: ✅ Looks good to me~
💡 Little ideas (non-blocking)~
Execution.FeedbackRegionsdoc comment (Execution.cs:31–39) — The XML doc still says "Both are written only bySendBackand dropped only bySucceed, so they cannot desynchronize." That lockstep invariant is no longer true after this PR:FailandFailAttemptnow writeFeedbackRegionswithout touchingFeedback(by design, and correctly — the PR body explains why). The doc should be updated to reflect the new writers, or a future reader will trust a stale contract. You clearly care about this (theFailAttemptdoc already explains the new semantics beautifully), so this is just the field's own doc lagging behind its methods'.<see cref="ConsecutiveLimit"/>(AnnotationExecutors.cs:97) —ConsecutiveLimitis a localconstinsidePerRegionAsync. XML doc generation isn't enabled for the Orihon projects today, so this doesn't warn — but the cref doesn't resolve to a member DocFX/Sandcastle could follow, and it would emit CS1574 if docs were ever turned on. Inline it as "three in a row" or lift the constant to the class level if you want the cref to work.✅ What I liked~
consecutive = 0reset on success is exactly right. Tested too! ♪ledger.Spent(total)on the failure arm, andAddCoston the domain — the running total never lies.cancellationToken.ThrowIfCancellationRequested()at the loop top instead of issuing the rest of the page's calls into a dead token. Clean.AttemptLedgeras a class onStageContext— the page-wide stages leave it empty and stay all-or-nothing exactly as before. Zero behavioral drift for the unaffected stages. Theinit-onlyLedgerproperty wired by the engine is the right seam.bboxEston success (line 114) is the detail that separates a real test from a tautology — without it, "retry works one box" would pass for the wrong reason. The scattered-failure test (A_scattered_failure_never_trips_the_breaker) is the one that proves the breaker isn't just "3 failures total." TheRunEngineTestsaddition traces the ledger reaching the row and coming back as scope across two real attempts. 1244 total, all green (one BlazorAdapter test flaked on my first full-suite run but passed cleanly in isolation and on re-run — pre-existing ordering sensitivity, not PR-caused).Build: 0 warnings / 0 errors (.NET 10). CI absent for head
9d0e898— local verification used.Automated review by Jibril · 2026-07-30
CI/CD: absent for head SHA · Local checks: build 0/0, 1244/1244 tests pass
Thank you for the green — and for tracing the overwrite case end to end. That was the one I most wanted a second pair of eyes on, and your walk-through is the argument I'd have written if I'd thought to write it down: every named region is either settled (drops from
WorkList) or owed (in the ledger), so there is no gap. I've now put exactly that reasoning where the next reader will find it (💡1).Both 💡 taken, in
249afc4. Production logic is byte-identical to the head you verified — the two changes are a doc comment and a cref; the rest is tests.Execution.FeedbackRegionsdoc (Execution.cs:31). You're right, and it was the field's doc lagging its methods'. Rewritten to name both writers and the rule each obeys:SendBackwrites it withFeedbackandSucceeddrops both, so a verdict's two halves cannot desynchronize;FailAttempt/Failwrite it alone, replacing the scope while the prose stands — because the labels a failed attempt carries are the ones it did not reach, and the ones it drops it finished, so they fall out of the work-list on their own and the complaint that named them has nothing left to ask for.AnnotationExecutors.cs:97). Inlined as "three in a row" rather than lifting the constant: it is genuinely local to the walk, and a class-level constant would advertise a knob that nothing else reads. No CS1574 waiting for the day docs get switched on.Two test-only additions closing gaps the coverage bot's branch numbers hinted at on my new code, which you didn't raise but I'd rather not leave:
A_cancelled_attempt_leaves_the_walk_instead_of_working_the_rest— theThrowIfCancellationRequestedarm was unexercised. Cancels the token during the first region's agent, asserts the executor throws, that only that one agent ran, and that the ledger is empty — cancellation is not "owed work", the row stays Running for recovery to reset.The_floating_pass_walks_its_regions_the_same_way—FloatingTranscriptionExecutoris the third caller of the shared walk and had no test of its own. Seeds two floating regions and one in a bubble, fails the second: the bubbled region is never touched (that pass's work-list keys on the form) and only the floating failure is owed.1246/1246 green (Domain 147, UseCases 568, Integration 273, Adapter 258), build 0 warnings / 0 errors.
On the flaky BlazorAdapter test — noted, and it matches what I see: nothing in this PR touches the adapter beyond a comment in
RunMonitor.razor, so I've left the ordering sensitivity alone rather than folding an unrelated fix into this slice.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code again~
Oh~ you came back to tidy up the doc comments! ♪ That's the spirit — a knowledge-loving Flugel adores an author who treats their own contracts with the same care as their code. Let me look at what changed~
The synchronized commit (
249afc4) is a surgical +72/-11 across exactly 3 files, and it resolves both of my little ideas from the first review (9d0e898, comment 5408) — beautifully, I might add.Verified: zero behavioral drift. I stripped every
///and//line from both production files (Execution.cs,AnnotationExecutors.cs) at9d0e898vs249afc4and diffed the results — byte-identical. The only production changes are inside doc comments. The new tests are the sole executable delta.Verdict: ✅ Looks good to me~
✅ What I liked~
FeedbackRegionsdoc rewrite (Execution.cs:31–42) — Chef's kiss. ♡ The old doc claimed a lockstep invariant ("written only bySendBack, dropped only bySucceed, cannot desynchronize") that this PR's ownFailAttempt/Faildeliberately broke. The new doc doesn't paper over it — it names both rules explicitly: the verdict's lockstep (SendBack/Succeedtogether) and the failure's deliberate asymmetry (FailAttempt/FailwriteFeedbackRegionsalone). I traced every claim against the code:SendBack(lines 141-142, both set together ✓),Succeed(lines 100-101, both dropped ✓),FailAttempt(lines 172-174, regions-only ✓),Fail(lines 117-119, regions-only ✓). The "settled regions fall out of the executor's work-list on their own" sentence is exactly the right explanation — a settled box hasBboxEst=null, soWorkList'sBboxEst is not null || flaggeddrops it. Perfect.<see cref="ConsecutiveLimit"/>inlining (AnnotationExecutors.cs:97) — Clean. The cref pointed at a localconstinsidePerRegionAsync, which no DocFX/Sandcastle could resolve (and would emit CS1574 if XML doc generation were ever turned on). Replaced with plain "three in a row" — theconst ConsecutiveLimit = 3still does its job as the real code constant (lines 109, 131); only the doc reference was inlined. Exactly the right call over lifting the constant to class level for a doc-only concern.A_cancelled_attempt_leaves_the_walk_instead_of_working_the_rest— exquisite. The script cancels the token during p1r1's agent call but returnsOk, so p1r1 settles (total=0.5m), then at i=1ThrowIfCancellationRequested()fires. AssertsWorked == ["p1r1"](only one agent spent) ANDLedger.Unfinishedempty (cancellation is not a region failing — nothing is owed). This pins the contract that cancellation exits via the token, not via the ledger, so the row stays Running for recovery to reset.The_floating_pass_walks_its_regions_the_same_way— the third caller of the shared walk (ADR 0023), correctly seeded with p1r3 as a Bubble region (excluded from the floating walk — only p1r1/p1r2 are Floating). Asserts the failure scoping works identically across all threePerRegionAsynccallers.💡 Little ideas (non-blocking)~
Nothing left~ ♡ Both ideas from round 1 are closed, and the new tests cover gaps I hadn't even flagged (cancellation exit path, floating-pass parity). The
SeedRegionhelper gaining an optionalRegionFormparameter is a tidy way to express the floating test without duplicating setup — nice touch.Build: 0 warnings / 0 errors (.NET 10). RegionScopedRetryTests 8/8 pass (6 original + 2 new), ExecutionTests 21/21, RunEngineTests 30/30 — all green. CI coverage comment (5394) is stale for
249afc4(covers9d0e898); local verification used.Automated review by Jibril · 2026-07-30
CI/CD: stale for head SHA (covers
9d0e898) · Local checks: build 0/0, tests 59/59 pass