Decide cancellation in one place, and keep the error that explains it - #97
Merged
Conversation
RequestCancel and WaitForStoppedConversion split one decision across two methods that only worked in one order, and the second waited for a condition the first had already established - so its timeout message could never appear. They are now one wait: find the button or a final status, press once, and judge from what the window reports. The press failure is no longer thrown away. A refusal certainly delivered nothing, so it still escapes to the retry loop. Any other automation failure might have followed a press that landed, so pressing stops - but the exception is now kept and named in the failure, because it is the likeliest explanation of a run that then finishes uncancelled. Without it that outcome was indistinguishable from a machine too fast to interrupt. Simulating an RPC failure on the press, the previous code reported only "Cancellation was not exercised. EC instead reported: Conversion complete: 1000 converted"; it now adds "The press to Cancel failed with an unknown outcome and was not repeated: COMException: simulated RPC failure". Controls: that simulation names the cause where master does not, five refusals followed by acceptance still cancel and interrupt the run, and a press that lands and then throws still passes. Phase I keeps its own "Conversion stopped" assertion as an independent check. EC is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A definite refusal is retried, so it never reaches uncertainPress - it ends up in the wait's retained error instead. When refusals ran until the conversion finished on its own, the failure appended nothing and read as a machine that was merely too fast, which is the outcome this evidence exists to rule out. Blame now takes both and words them differently, because they are different evidence: an uncertain press may have landed and was deliberately not repeated; a refusal certainly delivered nothing and was retried for as long as the run lasted. The timeout path passes only the uncertain press, since Expired already names whatever the wait was still retrying. Simulating a button that refuses every press, the failure now ends "The last attempt to press Cancel was refused: ElementNotEnabledException: The operation is not allowed on a nonenabled element."; before it stopped at EC's tally. The remarks no longer claim the press is attempted once. At most one press with an uncertain outcome is attempted; a press that was definitely refused may be retried. EC is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One full-suite run in thirteen failed phase A with "EncodingChecker did not return to its idle state", and the diagnostic showed only the window's chrome - no status bar text, no result rows. It did not reproduce: phase A alone passed twelve of twelve on that build and twelve of twelve on master, and the suite passed eight of eight afterwards. It is not attributable to the cancellation work being validated at the time. Phase A cancels a review and never enters that path, and master carries the same WaitForMainReady, so the flake most likely predates it. EC-28 records where to look: WaitForMainReady still waits for any final status rather than evidence of the action just performed, which is EC-26's shape in a helper that fix did not reach; the timeout cannot say whether the process is alive, the window handle valid, the review gone, or the status bar findable; and the driver holds one AutomationElement for the main window from startup, which would fail every later read if it went stale while the window was healthy. Reproduction should run the whole suite, since the phase alone does not show it. Recorded rather than waited out: a gate that fails for reasons nobody can name is the problem EC-24, EC-26 and EC-27 all turned out to be. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to PR #96, from reviewing what it merged. No production code changes;
MainFormwas read and not edited.Do not merge yet - an unexplained phase A failure seen during validation is
being recorded as its own smoke-driver finding first. It is unrelated to the code
here: phase A does not use the cancellation path.
What was wrong
RequestCancelandWaitForStoppedConversionsplit one decision across twomethods that only worked in one order, and the second waited for a condition the
first had already established - so its 30-second wait always succeeded on the
first poll and its timeout message could never appear.
Worse, the press failure was discarded. An uncertain press was caught, its
exception bound and never used, and the predicate then answered cleanly - which
makes the shared loop clear its retained error too. A run that then finished
uncancelled reported only "Cancellation was not exercised. EC instead reported:
Conversion complete: 1000 converted", which reads as "this machine was too fast"
when the real cause was an automation failure the driver saw and threw away.
A refusal had the same problem by a different route: it is retried, so it never
reaches the uncertain slot and ended up only in the wait's retained error, which
the completed-run message did not consult.
Now
One wait does all of it: find the button or a final status, press once, retry only
a refusal, save any uncertain failure, and judge from what the window reports -
Conversion stoppedpasses,Conversion completefails, no final status is thetimeout. Both kinds of evidence are named, and worded differently because they are
different: an uncertain press may have landed and was deliberately not repeated; a
refusal certainly delivered nothing and was retried for as long as the run lasted.
Evidence
COMException: simulated RPC failure; master names nothingThe last attempt to press Cancel was refused: ElementNotEnabledException; previous commit named nothingMutations required a successful build before running, with the source restored
byte-for-byte afterwards.
Recorded, not hidden
One full-suite run out of thirteen failed in phase A with
TimeoutException: EncodingChecker did not return to its idle state. It did notreproduce: phase A alone passed 12/12 on this build and 12/12 on master, and the
suite passed 8/8 afterwards. Phase A does not touch the code in this PR, and the
patch was checked to have removed only the two intended methods. Cause
unidentified; tracked separately rather than merged over.
🤖 Generated with Claude Code