Make phase I's cancellation reliable, and cheap enough not to fight itself - #96
Merged
Conversation
Two defects in the cancellation path, both found by review of the last change. RequestCancel gave up on the first ElementNotEnabledException and then waited for the run to report itself finished - which it could not do for another ten seconds, because the run was still going. The click is now retried, each attempt reacquiring the button, and the wait ends on a click that was accepted, a status saying the run finished on its own, or the timeout. A control that refuses the first five clicks now passes and still interrupts the run; against the previous shape a single refusal failed the phase twice out of two - and failed it with the wrong explanation, reporting that the conversion had finished when it was still writing. The trigger asked RewrittenCount(...) >= 1, which opened all thousand files on every 50 ms probe while EC was rewriting those same files. It now asks AnyRewritten, which stops at the first one. The count is still taken once, after the run has stopped, where it is what the assertions compare. The margin this bought is visible in the phase's own line: cancellation used to land after 38-93 of the thousand files and now lands after 16-60. EC is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The refusal was caught inside the predicate and turned into "not yet". That retried just as often, but the shared loop clears its retained error whenever a probe answers cleanly, so every refusal erased the very thing a timeout would have reported - while the remarks claimed the wait retained it. ElementNotEnabledException derives from InvalidOperationException, which the loop already catches, so the refusal now simply propagates: it is retried and kept. Confirmed by compilation rather than assumed - catching it after InvalidOperationException is CS0160, "a previous catch clause already catches all exceptions of this or of a super type". The behaviour is unchanged where it was already right: a control refusing the first five clicks still passes and still interrupts the run. No control here shows the improved message, because staging one needs a conversion outliving the thirty-second wait, and this workload ends in about ten seconds. The first attempt at such a control was invalid - blinding the completion check made the predicate answer cleanly for the rest of the wait, which cleared the error in both shapes and would have shown no difference for the wrong reason. Also drops a stranded summary left above AnyRewritten by the previous commit. EC is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It keeps the last error only while the probe is still failing: a poll that answers cleanly clears it. Three comments said or implied otherwise, including the parameter's own documentation, which is where the rule belongs. RequestCancel now says the loop can report a refusal if refusals continue to the timeout, and that catching it here would turn every refusal into a clean "not ready" answer - which is precisely what makes the loop drop the cause. The duplicate claim higher in the same remarks is gone. Comments only; no behaviour changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Check builds its message whether or not the assertion fails, so each of these asked the window for the target twice on every run: once to test it, once to describe it. That drove the control on passing runs, and let a transient automation error inside a message fail a check whose condition had held - the defect the driver added Safely() to avoid. It could also report a value the assertion never tested, since the second read is a separate observation. The comparison was ordinal while SelectCombo and RequireDefaultTarget both use OrdinalIgnoreCase, so a control reporting "UTF-8" would have satisfied the driver and failed the phase. Both now read once into a local and compare case-insensitively, and the message reports that same reading. The assertions still catch what they exist for: with SetTargetEncoding reduced to a no-op, phase A fails twice out of two with "The target encoding did not change: utf-8". EC is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Delivering a click is not proof it took effect, and failing to deliver one is not proof it did not. RequestCancel tracked a cancelled flag and threw when it was false, so a click that landed and then failed - because the window hid the button in response, which is exactly what a successful cancel looks like - reported "The conversion finished before cancellation could be exercised" while the status bar read "Conversion stopped". EC writes that headline only for a run that was stopped, so the failure contradicted its own evidence. The flag is gone. RequestCancel asks and then waits for the run to report something; WaitForStoppedConversion reads what it reported. Conversion stopped means cancellation worked, Conversion complete means it was not exercised, and no final status at all is the timeout. The two ways a click can fail are no longer treated alike. A refusal happens before anything is delivered, so it is retried. An element that vanishes mid-call, or a COM failure, may follow a click that did land, so the attempt stops rather than pressing the button a second time. SelectedName absorbs a provider's null instead of handing it out, and every comparison of its result uses the static string.Equals overload. The instance call introduced a NullReferenceException where the previous == could not throw. Controls: a click that lands and then throws now passes twice out of two and still interrupts the run, where the previous shape failed twice with the false message. Five refusals followed by acceptance still cancel. A no-op target setter still fails phase A with "The target encoding did not change: utf-8". EC is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two gaps in how RequestCancel classified a failed press. A refusal was caught and answered "not ready", which is a clean answer, and the shared loop clears its retained error on one of those - so the diagnostic was erased again, the same way the earlier local catch erased it. The refusal is now left to that loop, which retries it and keeps it as the cause a timeout can name. Every other automation failure from the press is now treated as an uncertain outcome, not only ElementNotAvailableException and COMException. A plain InvalidOperationException escaped to the shared loop, which retried the whole predicate and pressed a button whose outcome was unknown: instrumented, that shape presses Cancel 35-38 times where one press is correct. It now presses once and waits for the status. Controls on the corrected shape: an uncertain press that never landed presses once and the phase fails with "Cancellation was not exercised", five refusals followed by acceptance still cancel and interrupt the run, and a press that lands and then throws still passes. EC is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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-ups from reviewing PR #95, which made phase I fail when it cannot prove it
interrupted a run. That is the right rule, but it left the phase depending on two
things that were not reliable. No production code changes;
MainFormwas readand not edited.
What was wrong
One refused click was read as "the run finished".
RequestCancelcaught thefirst
ElementNotEnabledExceptionand then waited for the run to report itselffinished - which it could not do for another ten seconds, because the run was
still going. The click is now retried, each attempt reacquiring the button, and
the wait ends on a click that was accepted, a status saying the run finished on
its own, or the timeout.
The refusal is no longer caught at the call site either.
ElementNotEnabledExceptionderives from
InvalidOperationException, which the shared retry loop alreadyhandles, so catching it locally only turned every refusal into a clean "not ready"
answer - which is exactly what makes that loop drop the cause it would otherwise
report.
The trigger opened a thousand files, twenty times a second.
writingHasBegunasked
RewrittenCount(...) >= 1, which counts the whole directory, while EC wasrewriting those same files. It now asks
AnyRewritten, which stops at the firstone. The count is still taken once, after the run has stopped, where the
assertions compare it.
Phase A read the target encoding twice per assertion.
Checkbuilds itsmessage whether or not it fails, so the interpolated message drove the control on
every passing run, could fail a check whose condition had held, and could report a
value the assertion never tested. Both assertions now read once into a local and
compare with
OrdinalIgnoreCase, matching whatSelectComboandRequireDefaultTargetalready use - an ordinal comparison would have let acontrol reporting
UTF-8satisfy the driver and fail the phase.Evidence
SetTargetEncodingreduced to a no-opEvery mutation required a successful build and a changed binary hash before it ran,
with the source restored byte-for-byte afterwards.
The cheaper probe shows up in the phase's own line: cancellation used to land after
38-93 of the thousand files and now lands after 16-66.
What is not shown
No control here demonstrates the improved timeout message. Staging one needs a
conversion that outlives the thirty-second wait, and this workload ends in about
ten seconds - the run finishes, the button hides, and the wait leaves through the
"finished first" branch instead of expiring. A first attempt at such a control was
invalid and is recorded as such: blinding the completion check made the predicate
answer cleanly for the rest of the wait, which cleared the retained error in both
shapes and would have shown no difference for the wrong reason. That part rests on
the compiler rejecting a redundant
catchwith CS0160, and on the five lines ofWaitForthat clear the error on a clean poll.🤖 Generated with Claude Code