Skip to content

Decide cancellation in one place, and keep the error that explains it #97

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
amrali-eg merged 3 commits into from
Sep 9, 2026
Merged

Decide cancellation in one place, and keep the error that explains it #97

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9fa708
Decide cancellation in one place, keeping the error that explains it
amrali-eg Sep 9, 2026
bccb0e6
Name a refused press, not just an uncertain one
amrali-eg Sep 9, 2026
9719eea
Record the phase A timeout nobody can explain yet
amrali-eg Sep 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 45 additions & 2 deletions docs/DEFECT-BACKLOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ This is the current ledger for defects and review findings in EncodingChecker.
It is organised by status, not discovery date, so the open work is visible in
one place. Longer evidence and history follow the ledger.

<!-- backlog-counts total=66 fixed=55 open=7 not-reproduced=1 withdrawn=1 intentional-behavior=1 decision=1 -->
<!-- backlog-counts total=67 fixed=55 open=8 not-reproduced=1 withdrawn=1 intentional-behavior=1 decision=1 -->

**Derived count: 66 findings — 55 fixed, 7 open, 1 not reproduced, 1 withdrawn,
**Derived count: 67 findings — 55 fixed, 8 open, 1 not reproduced, 1 withdrawn,
1 intentional behavior, and 1 design decision.** Recompute and check these
figures with:

Expand Down Expand Up @@ -52,6 +52,7 @@ the 2026-09-08 reformat to findings that previously had only a sentence.
|---|---|---|---|---|---|
| EC-17 | A comment describes the text check backwards | Open | Low | Common | [EC-17](#ec-17) |
| EC-20 | A file can change while EC is detecting its encoding | Open | Low | Rare | [EC-20](#ec-20) |
| EC-28 | Phase A once timed out waiting for the window to go idle | Open | Medium | Rare | [EC-28](#ec-28) |
| CX-06 | EC checks for random-looking data before checking for a BOM | Open | Medium | Theoretical | [CX-06](#cx-06) |
| BL-05 | Force-closing during a conversion can produce an error on exit | Open | Low | Rare | [BL-05](#bl-05) |
| BL-19 | ASCII with many NUL bytes can be reported as UTF-16 | Open | Medium | Rare | [BL-19](#bl-19) |
Expand Down Expand Up @@ -1049,6 +1050,48 @@ early. EC-26 was later fixed by removing the remaining enabled-button checks fro
readiness decisions. The driver now waits for evidence produced by the operation
itself.

### EC-28

**Phase A failed once with `TimeoutException: EncodingChecker did not return to
its idle state`, and the cause is not known.** It happened on 2026-09-10 in one
full-suite run out of thirteen, while validating the cancellation state machine.

`WaitForMainReady` waits for the review window to be gone and for the status to
show a final conversion result. The diagnostic it printed listed only the window's
chrome - `File Encoding Checker | System Menu Bar | System | Minimize | Maximize |
Close` - with no status bar text and no result rows among it, so at that moment
the driver could see the window frame but nothing inside it.

**It did not reproduce.** Phase A alone passed twelve times out of twelve on the
build that failed, and twelve out of twelve on `master`. The full suite passed
eight times out of eight afterwards. Isolated runs may simply be the wrong shape
to catch it: the failure appeared in a sequence where nine other phases had
already driven the same window.

**It is not attributable to the change being validated.** Phase A cancels a review
and never enters the cancellation path that change rewrote, and the patch was
checked to have removed only the two methods it intended to remove. `master`
carries the same `WaitForMainReady`, so the flake most likely predates it.

Where to look, in the order that would settle it fastest:

- `WaitForMainReady` waits for *any* final conversion status rather than evidence
of the action just performed. That is [EC-26](#ec-26)'s shape reappearing in a
helper the fix did not reach, and it is recorded there as still open.
- The timeout reports what the window showed but not whether the process is still
alive, whether the main-window handle is still valid, whether the review is
genuinely gone, or whether `statusBar` can be found at all. A failure that
cannot distinguish those is hard to diagnose from CI alone.
- The driver holds the `AutomationElement` for the main window from startup. If
that element goes stale, every later read fails while the window is perfectly
healthy; reacquiring it on failure would tell the two apart.
- Reproduction should run the whole suite rather than the phase alone.

Until then this is one unexplained red on a required check. It is recorded rather
than waited out because a gate that fails for reasons nobody can name is the
problem this project keeps returning to - [EC-24](#ec-24), [EC-26](#ec-26) and
[EC-27](#ec-27) are all the same story, and each of them looked like noise first.

## Decisions and mistakes that must remain visible

### A known defect shipped after being reported closed
Expand Down
132 changes: 73 additions & 59 deletions sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,103 +245,117 @@ internal void ProceedThenCancel(AutomationElement review, Func<bool> writingHasB
() => writingHasBegun() || ConversionHasFinished(),
"The conversion neither began writing nor reported that it had finished.");

RequestCancel();

WaitForStoppedConversion();
CancelAndConfirmStopped();
}

/// <summary>
/// Requests cancellation, or fails if the run finishes before it can be cancelled.
/// Cancels the run and requires the window to report that it was stopped.
/// </summary>
/// <remarks>
/// The window hides the button when a run ends - <c>MainForm</c> sets
/// <c>btnCancel.Visible</c> - so an absent button has two meanings: the run beat the
/// phase to it, or automation failed to see a button that is on screen. Only the
/// window's own final status separates them. Either meaning prevents this phase from
/// proving cancellation, so neither is accepted as a successful test.
///
/// The button and final status are raced so a fast completion produces a clear failure
/// instead of a misleading timeout.
/// One wait decides everything, because the three things that could be asked
/// separately - has a button appeared, did the press land, what did the run report -
/// are only meaningful together. Pressing is not proof of cancelling: the window
/// hides the button when a run ends, so a press can fail precisely because it
/// worked, and an absent button says only that some run is over. The status is the
/// single fact that separates a run that was stopped from one that finished on its
/// own, and EC writes "Conversion stopped" for the first and "Conversion complete"
/// for the second.
///
/// Whether cancellation happened is not decided here. Delivering a click is not
/// proof it took effect, and failing to deliver one is not proof it did not: the
/// window's own final status is the only thing that separates a run that was stopped
/// from one that finished on its own, and <see cref="WaitForStoppedConversion"/>
/// reads it. This method's job is to ask, and to wait until the run has reported
/// something.
/// At most one press with an uncertain outcome is attempted. A press that was
/// definitely refused may be retried: a refusal - the control reporting itself
/// not-enabled - is the one failure that certainly delivered nothing, so it is left
/// to the retry loop, which repeats it and keeps it as a cause. Any other automation
/// failure might have followed a press that did land, so pressing stops there and
/// the exception is kept.
///
/// The two ways a click can fail mean different things. A refusal - the control
/// reporting itself not-enabled - happens before anything is delivered, so trying
/// again is safe and right, and it is left to the shared retry loop, which repeats it
/// and keeps it as the cause a timeout would name.
///
/// Any other automation failure might have followed a click that did land: an element
/// disappearing mid-call is what a successful cancel looks like when the window hides
/// the button in response. Clicking again there would be a fresh action rather than a
/// retry, so the attempt stops and the status is left to say what happened.
/// Either way the reason is named in the failure. A run that finishes uncancelled
/// after a press that was refused, or one whose outcome was unknown, is otherwise
/// indistinguishable from a machine too fast to interrupt.
/// </remarks>
private void RequestCancel()
private void CancelAndConfirmStopped()
{
bool clickMayHaveLanded = false;
bool pressed = false;
Exception? uncertainPress = null;

WaitUntil(
string? finalStatus = WaitFor(
() =>
{
// Once a click may be in flight, stop pressing the button and just watch.
if (!clickMayHaveLanded)
if (!pressed)
{
AutomationElement? cancel = FindById(MainWindow, "btnCancel");

// Gone means the window hid it, which it does only when a run ends -
// but that has to come from the status, not the button's absence.
if (cancel is not null)
{
try
{
Invoke(cancel);
clickMayHaveLanded = true;
pressed = true;
}
// A refusal is the one failure that certainly delivered nothing,
// and it is deliberately not caught: the shared loop retries it
// and keeps it, so a wait that expires on repeated refusals can
// name them. Answering "not ready" here would clear that cause.
//
// Every other automation failure might have followed a click that
// landed, so the attempt stops and the status is left to say.
catch (Exception ex) when (
ex is not ElementNotEnabledException &&
ex is ElementNotAvailableException
or COMException
or InvalidOperationException)
{
clickMayHaveLanded = true;
uncertainPress = ex;
pressed = true;
}
}
}

return ConversionHasFinished();
return StatusLine() is string status && IsFinalConversionStatus(status)
? status
: null;
},
"The run never reported a final status after cancellation was requested.");
Timeout,
out Exception? lastError);

if (finalStatus is null)
{
// Only the uncertain press is added here: Expired already names whatever
// the wait was still retrying, which is where a refusal shows up.
throw Expired(
(pressed
? "Cancel was pressed but the run never reported a final status."
: "No Cancel button appeared and the run never reported a final status.")
+ Blame(uncertainPress, null),
lastError);
}

if (!finalStatus.Contains("Conversion stopped", StringComparison.Ordinal))
{
throw new GuiDriverException(
"Cancellation was not exercised. EC instead reported: " + finalStatus
+ Blame(uncertainPress, lastError));
}
}

/// <summary>Requires the cancellation request to produce an interrupted run.</summary>
private void WaitForStoppedConversion()
/// <summary>
/// Names why the press did not stop the run, so a run that finished uncancelled is
/// not reported as a machine that was simply too fast.
/// </summary>
/// <remarks>
/// The two are different evidence and read differently. 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, and arrives as
/// <paramref name="lastRefusal"/> - the error the wait was still holding, which it
/// keeps only when the refusal was the most recent thing to happen.
/// </remarks>
private static string Blame(Exception? uncertainPress, Exception? lastRefusal)
{
string? finalStatus = null;

WaitForOperationOutcome(
() =>
{
finalStatus = StatusLine();
return finalStatus is not null && IsFinalConversionStatus(finalStatus);
},
"The conversion did not report a final result after cancellation.");
if (uncertainPress is not null)
{
return " The Cancel press failed with an unknown outcome and was not repeated: "
+ $"{uncertainPress.GetType().Name}: {uncertainPress.Message}";
}

if (!finalStatus!.Contains("Conversion stopped", StringComparison.Ordinal))
if (lastRefusal is ElementNotEnabledException)
{
throw new GuiDriverException(
"Cancellation was not exercised. EC instead reported: " + finalStatus);
return " The last attempt to press Cancel was refused: "
+ $"{lastRefusal.GetType().Name}: {lastRefusal.Message}";
}

return string.Empty;
}

/// <summary>Waits until the status line contains <paramref name="fragment"/>.</summary>
Expand Down
Loading