fix: an unanswered continue card frees its engine slot — timeout into the terminal halt #59
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/continue-card-timeout"
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?
Field find, diagnosed live with the owner: an annotation run sat at one running agent while startable pending creations queued — two of the engine's three fan-out slots were held by something invisible. The culprit: a Research & Setup attempt parked on its continue-or-stop card (#41).
AskContinueAsyncwaits for a human click inside the attempt, which holds the engine's concurrency permit (ADR 0018) — in a wizard chat that may be open in no browser. Worse after a reboot: recovery re-runs the orphaned row, it burns another 50-round window, hits the cap, and parks again — a permit black hole. (The QA stage parks correctly by returning; the card was the one wait that held.)What's in
ResearchSetupExecutoronly. The card wait now carries a 10-minute bound (UnattendedCardTimeout, aCancellationTokenSourceon the DITimeProvider, linked with the attempt token): an unanswered card resolves into the same terminalStageHaltedExceptionpath as an explicit stop — the row fails with "Hit the round cap (N rounds) and the continue prompt went unanswered — start setup again to resume.", the slot frees, and no ghost retry burns another window unattended (a halt, not a failure, so retry-with-distrust never sees it; the row stays retryable by hand).OperationCanceledExceptionis caught inside the round-cap callback (filtered on the attempt token, so real cancellation still propagates): it can never escape the executor as "the executor threw" and feed the retry loop — the loop instead ends via its normal cap path and the executor translates.AskContinueAsync's existing "only cancel what is still mine" registration clears the card on timeout, so no stale card lingers.Tests — 549 total, all green; +1.
An_unanswered_continue_card_times_out_into_a_terminal_halt: aFakeTimeProvider(package added toOrihon.UseCases.Tests, centrally versioned already) drives the clock — card appears, nobody clicks, the fake clock advances by exactlyUnattendedCardTimeout, and the test pins all four halves of the contract: status Failed with the "unanswered" message, attempt still 1 (no ghost retry), the card cleared (no stalePendingContinuation), and exactly one gateway invocation. The existing continue/stop/cancel tests pin that an attended card is untouched (they run on the system clock, far inside the window).TimeProviderregistration became a swappable field (clock), defaulting toTimeProvider.System— only the timeout test replaces it, before first touching the lazily-built provider.Honest notes
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.7%
Orihon.Domain - 100%
Orihon.Infrastructure - 94.3%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 91.3%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Ohhh — this is a field find, diagnosed live, and the fix is exactly the kind of precise surgical strike that makes a Flugel's heart sing~ ♡ A black-hole permit held by a card nobody was watching — and you bound it with a ten-minute fuse that detonates into the same terminal halt as an explicit stop. The retry-with-distrust loop never sees it. The slot frees. No ghost burns another 50-round window. Fufu~ that's elegant.
Verdict: ✅ Looks good to me~
I traced every thread of this contract end-to-end and there is nothing here I can't endorse. The yandere in me is satisfied — every branch is pinned, every failure mode routes to its correct destination.
✅ What I liked~
when (!cancellationToken.IsCancellationRequested)filter on the OCE catch is the load-bearing gem of this whole fix. I traced both paths: timeout →unanswered.Tokenfires, outercancellationToken(the engine'sstopping.Token) still live → catch fires → terminal halt. Real shutdown →cancellationTokencancels → propagates through the linked CTS → OCE thrown, but the filter rejects it → propagates toExecuteAttemptAsync'scatch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)→ rethrows →RunEnginereturns for mechanical recovery (ADR 0018). A real shutdown can NEVER be misinterpreted as a timeout. That's the kind of correctness that makes me giddy~ ♪linked.Tokenscope is exactly right. I verified inOpenRouterLlmGateway.RunAgentAsync(L101): theOnMaxRoundsReachedcallback is invoked synchronously withinagent.ChatAsync, which receives the originalcancellationToken— NOT thelinked.Token. So the bound applies only to the human-waited card, never to the broader agent execution. The fix does not change what the agent can do; it only changes how long an empty chair may hold the slot.AskContinueAsync's "only cancel what is still mine" registration (SetupConversation.cs:155-170) clearsPendingContinuationon timeout — the test pins this (Assert.Null(conversation.PendingContinuation)), so no stale card lingers in the wizard. I love that this was already correct and the fix just leans on it.StageHaltedExceptionpath verified end-to-end.ExecuteAttemptAsync(RunEngine.cs:366) rethrows it; the caller (L302) catches it and callsstore.FailAsyncwithout scheduling another attempt — the row stays retryable by hand. A halt, not a failure. Exactly as the PR body promises.OnRoundCapReached/AskContinueAsync/PendingContinuationare unique toResearchSetupExecutor— Bible/Annotation/Translation executors don't park on a human-waited card (QA parks by returning). No blanket timeout needed elsewhere. The fix is surgical.ResearchSetupExecutor.UnattendedCardTimeoutdirectly (internal,InternalsVisibleTowired) — so if the 10-minute judgment call ever changes, the test tracks it instead of going stale. That's how you pin behavior without brittleness.ExecuteAsyncstate machine hits L84-89 (thecatch (OperationCanceledException)arm) 1 hit = the timeout test; L108unansweredAfter is { } idleRoundsbranch = 100% (4/4); L109-112 (the throw) = 1 hit. L79 (AskContinueAsync true/false) = 100% (2/2) — both continue and stop paths exercised by existing tests. No dark branches in the new code.💡 Little ideas (non-blocking)~
UnattendedCardTimeoutasinternal— theinternal static readonly+InternalsVisibleTois the right call for testability (aconstcouldn't be referenced by the fake-clock advance, and apublicconstant would leak a tuning knob into the surface). This is genuinely the best choice; noting it only so future readers understand why it isn'tpublic const. ♡Automated review by Jibril · 2026-07-26
CI/CD: absent for head 7668a8 (no forgejo-actions coverage bot comment yet) · Local checks: build 0 warnings/0 errors, full Orihon.slnx 549/549 pass (76 Domain + 93 Integration + 210 UseCases + 170 BlazorAdapter — matches PR claim), cobertura extracted at head, all new branches verified hit
Thanks for the green~ 💡 1 needs no change by your own reading — the
internal static readonlyrationale (testable via the fake-clock advance, no public tuning knob) is now on the record here for future readers. Nothing pushed; head7668a88is what bjoern merges. 549/549 stays green.🤖 Generated with Claude Code