fix: a cancel that arrives before its attempt starts no longer waits forever (#67) #71
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-fix+usecases-test-hang"
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?
Closes #67 — and the defect behind it is not test-only: the same window makes the app's cancel-run button never come back.
The bug
RunEngine.Schedulequeued an attempt's task, but registered that attempt'sCancellationTokenSourceinside the task body. BetweenSchedulereturning and the task actually getting a thread, the execution is ininFlightbut absent fromattemptCancellations. A cancel landing in that window:inFlightand adds it todraining;await Task.WhenAll(draining)— waits on an attempt nobody cancelled.When the work only ends on cancellation (a hung provider call — exactly what
Cancelling_a_run_aborts_its_agents_and_deletes_every_tracescripts), that wait never ends.CancelRunAsyncnever returns; in the app that is a cancel click that hangs the circuit, with the run's agents still burning tokens.The queueing delay is ordinary under CPU contention, which is why #67 read as "hangs only under load" and why the hung test class was always
RunEngineTests.The fix
Schedule, before the work is queued. A cancel can now never arrive "too early".gate.WaitAsync'sOperationCanceledException— thrown outside the try until now — is swallowed like the mid-attempt one. Otherwise the drain would see a faulted task and handCancelRunAsync's caller an exception instead of itsOk. Nothing was acquired, so nothing is released: the permit accounting is unchanged.inFlightheld bareTasks behindGetOrAdd, whose factory may run more than once per key under contention. The losing factory had already started a second attempt of the same execution, and its cleanup removed the entry by key — evicting the live attempt, hiding running work fromWhenIdleAsyncand from the drain. The value is now aLazy<Task>(ExecutionAndPublication, so only the stored one is ever forced — a loser's task never comes into being), and both dictionaries remove by key and value.How it was found
Previous attempts failed because an incomplete
awaitholds no thread:--blame-hanganddotnet-stackon the hung host showed only VSTest/xunit runner frames, no Orihon frames at all. So I traced every test's start/finish to a file with an assembly-levelBeforeAfterTestAttributeand caught a hang with exactly one unmatchedSTART:That test plans two executions and cancels once one is Running — leaving the second still queued, which is the window. The tracer was diagnostic only and is not in this diff.
Tests
630 green (78 Domain / 267 UseCases / 184 BlazorAdapter / 101 Integration), each project run separately.
An_attempt_is_cancellable_from_the_moment_it_is_scheduled— seeds the rows, callsSchedule, and assertsIsCancellablesynchronously, then thatCancelRunAsynccompletes within the deadline with an executor that only ends on cancellation. 4 of 5 runs fail on the old ordering. Not 5/5, and that is the race itself: on an idle machine the task sometimes registers before the assertion runs. After the fix it is deterministic, because registration no longer depends on scheduling.A_burst_of_schedules_for_one_execution_still_runs_a_single_attempt— 64 threads schedule one execution whose key is absent (rows seeded without scheduling; my first draft scheduled first, so every caller read the existing entry, the factory never re-ran, and the test passed on unfixed code — it proved nothing). 8/8 fail before, 8/8 pass after, onAssert.False(idle.IsCompleted):WhenIdleAsyncreported idle while an attempt was parked in the executor.internalseams, each commented with why:ScheduleandIsCancellable. The races they expose cannot be scheduled from outside the engine.Measurement
Full assembly, Release, 16 CPU hogs on 8 cores, 60 s timeout per run:
maind0c1eefFor completeness: the
Lazy<Task>change alone did not move the needle (1/30) — it fixes early-idle, not the stall. The cancellation-registration change is what closes the hang. And 0/40 bounds the rate rather than proving absence; with a named mechanism and a directional test behind it I'd treat #67 as closed, but CI is the real jury.Baseline note for the record: pre-#66 main was 0/15 and post-#66 main 2-3/15 under the same conditions — #66 added no engine code, only tests, so it raised the assembly's concurrency and made a pre-existing race likely enough to notice. Nothing in #66 or #69 introduced the defect.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.9%
Orihon.Domain - 100%
Orihon.Infrastructure - 95.1%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 92.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my, a real race condition. The kind that only surfaces under load, that laughs at
--blame-hangbecause an incompleteawaitholds no thread at all, that had to be cornered with a hand-rolledBeforeAfterTestAttributetracer to even see. fufu~ This is the good stuff. The kind of bug that makes my collection flutter with joy. ♡Verdict: ✅ Looks good to me~
You traced the window precisely, you fixed the mechanism (not a symptom), and the two fixes that look like they're about different things are actually the same defect wearing two hats — which you noticed while reading the same method. That's exactly how a careful pass should go.
✅ What I liked~
The synchronous registration is the whole fix, and you know it. Moving
attemptCancellations[executionId] = attemptout of theTask.Runbody and intoScheduleproper closes the window where a cancel arrives "too early." Theinternal bool IsCancellableseam and its test (An_attempt_is_cancellable_from_the_moment_it_is_scheduled) assert the ordering directly —Assert.True(engine.IsCancellable(execution.Id))right afterSchedulereturns, no awaiting dispatch. That is directional. Pre-fix, 4/5 fail; post-fix, deterministic. The "not 5/5" honesty in the PR body is chef's kiss — you named the race instead of pretending the test was flaky.The
Lazy<Task>migration is the correct second fix, and you proved it's independent. The measurement table in the PR body (Lazy<Task>alone: 1/30, registration-change alone: 0/40) is exactly the kind of decomposition that separates "I changed things and the bug went away" from "I changed the thing that caused the bug."ExecutionAndPublicationmode guarantees only the stored Lazy is ever forced — a losing factory's task never comes into being. The pre-fixTask? created = null+ bareTask.Runpattern did have a duplicate-attempt hazard underGetOrAddcontention, and the loser's by-key-onlyTryRemovewould have evicted the live winner. Real bug, correctly described.The
gate.WaitAsyncOCE swallow is the load-bearing companion change. Once an attempt can be cancelled before it holds a permit, theOperationCanceledExceptionfromWaitAsync(which used to be thrown outside any try) would surface as a faulted task indraining, andCancelRunAsyncwould hand the caller an exception instead ofOk. You caught this in the same diff. The new catch (line 384)returns before the inner try/finally — sogate.Release()is correctly not called on the pre-permit cancel path. Nothing acquired, nothing released. Permit accounting unchanged. I traced this very carefully because it's the kind of subtle thing that could leak a semaphore permit, and it does not.Both removals are now key-AND-value (
ICollection<KeyValuePair<...>>.Remove), so a stale duplicate's cleanup can never evict a live attempt. Thecreated!nullable suppression is safe:GetOrAddinvokes the factory synchronously and assignscreatedbefore returning the storedentry, so by the timeentry.Valueis forced (and the closure capturescreated), they're the same reference. The loser'sLazyis dropped unforced — its closure never executes, no NRE.Finally-block ordering is race-safe against
CancelRunAsync.attemptCancellations.Remove(key, attempt)runs beforeattempt.Dispose(), so there's no window where the dict hands out an already-disposed CTS. And even ifCancelRunAsync'sTryGetValuegrabs the ref before step 1 and callsCancel()after step 2'sDispose(), the existingcatch (ObjectDisposedException)swallows it cleanly. I checked both interleavings.The PR body's measurement discipline is exemplary — 0/40 bounds the rate rather than claiming absence, the baseline note about
#66raising concurrency (not introducing the defect) pre-empts a misdiagnosis, and the explicit "CI is the real jury" closes honestly. This is how a concurrency fix should be narrated.Automated review by Jibril · 2026-07-26
CI/CD: absent for head SHA (PR just opened, no coverage-bot comment) · Local checks: build 0 warnings/0 errors, 267/267 UseCases.Tests pass (incl. both new tests + the originally-hanging
Cancelling_a_run_aborts_its_agents_and_deletes_every_trace), 5s