fix(fileio2): report write failure instead of success on the final retry (#647) - #712
Merged
drmoisan merged 8 commits intoSep 1, 2026
Conversation
Preparation-mode output for issue #647, where FileIO2.WriteTextFileAsync sets its success flag to true after exhausting its 100-attempt retry budget, so a caller cannot distinguish a completed write from one that never happened. The retry delay also ignores the caller's CancellationToken. Adds the active feature folder: issue.md (full-bug), spec.md with 21 acceptance criteria, the research findings, and the atomic plan at 9 phases and 89 tasks. The plan cleared three preflight rounds against atomic-executor and passes the MCP plan validator gate with no G1-G9 findings. Round 1 reported 12 defects over 192 signals, round 2 reported 2 blocking defects over roughly 160 signals, and round 3 returned ALL CLEAR with zero defects. Scope of this commit is preparation only. No production source is touched: atomic execution, PR authoring and CI monitoring are performed later by parallel-orchestrator. Part of parallel run bugs-638-644-647. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
FileIO2.WriteTextFileAsync conflated "stop retrying" with "the write succeeded", so two distinct failures were reported to callers as success. Defect 1 — retry exhaustion. After 100 failed open attempts the method logged and then set its success flag, returning normally. The return type is now Task<bool> and the exhaustion path returns false. Defect 2 — mid-write failure. The success flag was assigned inside the writer's using block before any line was written, so an IOException from WriteLineAsync or from the disposing flush exited the loop reporting success after one pointless delay. The flag is replaced by a per-attempt `opened` local; a failure raised after the writer opened is terminal and returns false immediately without consuming retry budget, because the file is opened in append mode and a retry after a partial flush would duplicate lines. Also: the catch clause now binds the exception and passes it to the two-argument logger.Error overload (it was previously discarded), and the retry delay receives the caller's token. Throwing was rejected: the AppOlObjects call site is an async void timer lambda, so a thrown exception would terminate the Outlook host process. An internal static seam overload takes a writer factory and a delay delegate as parameters, not static state, because UtilitiesCS.Test runs class-level parallel. All three call sites are updated to observe the new failure signal rather than discard it through the reference conversion. Tests: the ~10-second locked-fixture test is replaced by six deterministic seam-driven tests covering exhaustion, mid-write failure, transient recovery, both cancellation entry points and token propagation. They run in 51 ms with no filesystem access and no wall-clock wait. WriteTextFileAsync line coverage rises from 0.79 to 0.95. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Adds the policy-audit, code-review and feature-audit produced by the feature-review pass over branch head 8e773f3. Blocking findings: 0. Non-blocking observations: 18. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Promotes the three deferred non-goals fixed in the plan's P8-T3 and the two residuals the feature review identified, through the drm-copilot MCP promotion surface. Opens five tracking issues and retains each promoted record. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Adds two atomic-executor notes on coverage-document shape and on plan authoring-time token counts, and two feature-review notes on measuring every changed file and on the residuals this review left open. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Adds a note that the orchestration checkpoint is tracked in git despite .gitignore, corrects the analyzer version-skew bootstrap item as resolved upstream, and records that PR context collection resolved to the agent worktree in a parallel-run child. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
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
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.
Summary
FileIO2.WriteTextFileAsyncreported success on the final failure of its write-retry path. The method returnedvoid-shapedTask, and it set its internal success flag inside the writer'susingblock before any line was written. Two distinct defects followed from that:IOException, the flag was already set. The loop took the retry branch once, awaited one delay, and exited — reporting success for a file that had been partially written.Callers therefore silently discarded a real failure. In
TaskMaster/AppGlobals/AppOlObjects.csthat failure was a lost moved-mail log; inQuickFiler/Controllers/QfcHomeController.Metrics.csit was a lost metrics flush.Closes #647.
What changed
WriteTextFileAsyncnow returnsTask<bool>, andtrueis produced only after every line has been written and the writer has been disposed without error.UtilitiesCS/To Depricate/FileIO2.cs(+73 / −12). The public overload becomes a non-async forwarder to a newinternal staticseam overload that takes a writer factory (Func<string, TextWriter>?) and a delay delegate (Func<int, CancellationToken, Task>?), both defaulting to the production behavior. The loop is restructured around a per-attemptopenedflag: a failure raised after the writer opened is terminal and returnsfalseimmediately without consuming the retry budget, because the file is opened in append mode and retrying after a partial flush would duplicate lines. A failure raised while opening keeps the existing 100-attempt, 100-millisecond budget. The catch clause now binds the exception and passes it to the two-argumentlogger.Erroroverload; previously the clause bound nothing and the cause was discarded. The retry delay now receives the caller's token.QuickFiler/Controllers/QfcHomeController.Metrics.cs(+14 / −2).MetricsFileWriteris retyped to carry the boolean, and the flush assigns the awaited result to a local and logs on failure instead of discarding it. The fourth argument staysCancellationToken.Noneand the comment explaining why the session token must not be used is retained.TaskMaster/AppGlobals/AppOlObjects.cs(+33 / −6). The disk-writer lambda becomes block-bodied so it can capture and check the result, and its body is wrapped in a try/catch. The broad catch is deliberate: this is anasync voidlambda on aSystem.Timers.Timerelapsed callback, and an exception escaping it is re-raised on the thread pool and terminates the Outlook host process.UtilitiesCS.Test/HelperClasses/FileIO2_Tests.cs(+232 / −13) andQuickFiler.Test/Controllers/QfcHomeControllerMetricsTests.cs(+10 / −9). Six seam-driven regression tests replace one filesystem-dependent locked-fixture test.Throwing was considered and rejected for the same
async voidreason. The seam takes delegates as parameters rather than static mutable state becauseUtilitiesCS.Testruns class-level parallel, and it is typedTextWriterrather thanStreamWriterso aStringWriterfits.Test coverage
Six new deterministic tests, none of which creates a file or directory, uses a temporary path, or calls
Thread.Sleepor a realTask.Delay:WhenEveryOpenAttemptFails_ShouldReturnFalseAfterBudgetfalse; 100 factory calls; 99 delaysWhenWriteFailsAfterOpen_ShouldReturnFalseWithoutRetryingfalse; 1 factory call; 0 delaysWhenTransientOpenFailureThenSucceeds_ShouldReturnTrueAndWriteAllLinestrue; 3 delays; exact written contentWhenTokenAlreadyCancelled_ShouldThrowBeforeOpeningOperationCanceledException; 0 factory callsWhenCancelledDuringRetryWindow_ShouldThrowPromptlyOperationCanceledException; 1 factory callWhenRetrying_ShouldPassCallerTokenToDelayThe mid-write defect carries a genuine fail-before run: against pre-fix source the delay-count assertion fails with an observed value of 1, and passes with 0 after the fix. The retry-exhaustion defect cannot fail before the fix, because asserting a
falsereturn requires the new signature and the new signature is the fix; that case carries a fail-before exception dossier plus a pre-fix characterization run showing 100 factory invocations and 99 delays returning with no observable failure signal.Verification
Full toolchain, in the order CLAUDE.md fixes, with both msbuild gates using
/t:Rebuildso the analyzer and nullable passes cannot be skipped by an incremental up-to-date check:csharpier check .TreatWarningsAsErrors(errors / warnings)FileIO2.cscovered linesWriteTextFileAsyncline rateThe five warnings at both ends originate in
System.Reactive.PackagesConfigCheck.targets, carry no diagnostic ID, and are outside the change footprint. The repository line rate moves by −0.000377, which is numerator nondeterminism across two runs of a class-level-parallel suite rather than a coverage loss;lines-validrose from 64245 to 64291 and every one of the 15 additional covered lines is inFileIO2.cs.One remediation event occurred during the toolchain loop and is recorded rather than hidden: the analyzer gate raised
CS0104oncatch (Exception ex)inAppOlObjects.cs, because that file importsMicrosoft.Office.Interop.Outlook, which declares its ownException. It was resolved with a file-scopedusing Exception = System.Exception;alias following the repository's existing precedent, and the loop was restarted from formatting.Review
Feature review reports 0 blocking findings and recommends GO. Eighteen non-blocking observations are recorded in full. The two most substantive:
QfcHomeController.Metrics.csline coverage moves from about 80.18% to 77.05%, because the new failure branch's only observable effect is a call on a static log4net field, so a covering test could enter the branch but assert nothing. Tracked as Feature: injectable-logging-seam-for-qfchomecontroller-metrics #710.AppOlObjects.csis now 494 of the 500-line limit, leaving 6 lines of headroom.Audit artifacts are committed under the feature folder:
policy-audit.2026-08-31T19-44.md,code-review.2026-08-31T19-44.md,feature-audit.2026-08-31T19-44.md.Acceptance criteria
21 of 21 checked off in
spec.md, each verified individually against the tree and the recorded evidence rather than accepted from the executor's check-off. AC20 was the only criterion requiring judgment; it is graded PASS with both literal sub-clause deviations recorded in the feature audit.Follow-ups
Five tracking issues were opened for deferred non-goals and review residuals; none is a prerequisite for this change: #707, #708, #709, #710, #711.