Skip to content

fix(QuickFiler): stop the queue closing on a deadline, terminate the undo consumer, release rejected hooks - #625

Merged
drmoisan merged 14 commits into
epic/quickfiler-bug-family-integrationfrom
bug/quickfiler-bug-family-446
Aug 26, 2026
Merged

fix(QuickFiler): stop the queue closing on a deadline, terminate the undo consumer, release rejected hooks#625
drmoisan merged 14 commits into
epic/quickfiler-bug-family-integrationfrom
bug/quickfiler-bug-family-446

Conversation

@drmoisan

Copy link
Copy Markdown
Owner

fix(QuickFiler): stop the queue closing on a deadline, terminate the undo consumer, release rejected hooks

Summary

  • Bug: iteratequeueasync-deadline-closes-queue-early #446 (High — silent data loss): QfcHomeController.IterateQueueAsync no longer closes the UI queue when an empty dequeue was caused by an expired deadline. CompleteAddingAsync is now reachable only from a batch that reports QfcDequeueStop.SourceExhausted.
  • Bug: quickfiler-undoconsumer-nonterminating-loop #448 (High — hang and CPU burn): QfcFormController.UndoConsumer() now terminates. The idle path always awaits or breaks, the idle timer resets after every successful take, and _undoConsumerTask is cleared in a finally block so a later UndoDialog() starts a fresh consumer.
  • Bug: emailmovemonitor-rejected-item-hook-retention #426 (Medium — COM leak): items rejected by the high-confidence gate are unhooked from EmailMoveMonitor exactly once, releasing the retained MailItem reference and its BeforeItemMove subscription.
  • Scope 427-A (producer side only): the scorer widens to (long Score, string TopFolder) and the accepted candidate's top folder now reaches the datamodel boundary as QfcDequeueBatch.PreScored. The consumer side is not delivered here, so Bug: quickfiler-post-show-duplicate-scoring #427 remains open.
  • One cross-module contract change on IQfcDatamodel serves all four items, so the gate signature churns once instead of four times.
  • Six production files and seven test files changed. Feature review recorded zero blocking findings; 27 of 28 acceptance criteria PASS with AC28 PARTIAL and non-blocking.

Why

All four defects sit on or beside a single code path:

QfcStreamingDequeueConfidenceGate.DequeueAsync
  -> QfcDatamodel.DequeueWithHighConfidenceGateAsync
  -> IQfcDatamodel
  -> QfcHomeController.IterateQueueAsync

Each of them independently changes the same gate signature and therefore independently invalidates
the same reflective test helper. Repairing them separately would have paid that cost four times.

The root cause of #446 is that the gate returned only a batch, with no reason for the batch being
empty. The caller had no way to distinguish "the deadline expired with nothing accepted yet" from
"the mail source is genuinely drained", and treated both as exhaustion. Closing the queue is
irreversible, so a single expired deadline silently dropped every remaining queued item for the rest
of the session. The fix makes the stop reason explicit rather than inferred.

What Changed

Core behaviour (6 production files)

File Change
QuickFiler/Interfaces/IQfcDatamodel.cs (+75/-1) Additive only: new QfcDequeueStop enum, new QfcDequeueBatch carrier, one new overload. The three pre-existing DequeueNextItemGroupAsync / DequeueNextItemGroup declarations are unaltered.
QuickFiler/Controllers/QfcStreamingDequeueConfidenceGate.cs (+81/-13) All four exits now return an explicit stop reason (QuantitySatisfied, DeadlineExpired, SourceExhausted). Adds the _onRejected sink, invoked once per rejected candidate and wrapped so a throwing sink does not abort the scan. Accepted candidates carry TopFolder.
QuickFiler/Controllers/QfcDatamodel.QueueProcessing.cs (+115/-4) Projects the gate batch to the IQfcDatamodel boundary, supplies TryReleaseRejectedHook as the rejection sink, and carries PreScored forward.
QuickFiler/Controllers/QfcDatamodel.cs (+1/-17) ScoreRemainingQueueMailItemAsync widens from Task<long> to Task<(long Score, string TopFolder)>.
QuickFiler/Controllers/QfcHomeController.Iteration.cs (+13/-4) CompleteAddingAsync is called only inside the Stop == SourceExhausted branch, with a why-comment naming the irreversibility.
QuickFiler/Controllers/QfcFormController.Actions.cs (+89/-31) UndoConsumer loop rewritten against a TimeProvider seam: every idle iteration awaits TimeProvider.Delay, a successful take resets the idle deadline, and the task handle resets in finally.

Tests (7 files, failing-first)

Ten regression tests were landed red and recorded as red before any production change, each with a
fail-before and pass-after TRX. The red index is at
docs/features/active/quickfiler-bug-family-446/evidence/regression-testing/p1-t20-red-index.2026-08-26T09-50.md.

The reflective gate-constructor helper in QfcStreamingDequeueConfidenceGateTests.cs was also made
fail-closed: the descending GetConstructor fallback chain is replaced by a single exact lookup
guarded by Should().NotBeNull(), so a signature change can no longer silently select a different
constructor.

Docs and evidence (155 files)

Lifecycle documents plus per-task evidence artifacts under the feature folder. The insertion count
is dominated by two committed Cobertura reports and the vstest TRX files; all TRX host identifiers
were scrubbed before commit to satisfy the repository artifact-hygiene rule, with test names,
outcomes and counters preserved.

Architecture / How It Fits Together

The gate previously returned IList<QfcPreScoredItem> and swallowed both the reason it stopped and
the folder it had already computed. Two internal carriers replace that:

  • QfcGateBatch (internal to the gate) — Accepted, Stop, Scanned.
  • QfcDequeueBatch (on IQfcDatamodel) — the public projection, carrying PreScored.

Both are readonly struct rather than record, because the target framework has no
IsExternalInit and therefore no init accessors.

Control flow after the change: the gate decides why it stopped and says so; the datamodel projects
that decision across the interface boundary without reinterpreting it; the controller acts on the
stated reason and closes the queue only for genuine exhaustion. The rejection sink is injected into
the gate by the datamodel, so the gate never touches EmailMoveMonitor directly.

Verification

Completed

Four-gate toolchain, re-run against the rebased tree at the current integration tip
(37709d22), because ci.yml triggers pull_request only on main and development and this PR
therefore receives no CI checks:

Gate Command Result
Format dotnet tool run csharpier check . exit 0
Analyze msbuild TaskMaster.sln /t:Rebuild /m /p:Configuration=Debug "/p:Platform=Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true exit 0
Type-check msbuild TaskMaster.sln /t:Rebuild /m /p:Configuration=Debug "/p:Platform=Any CPU" /p:TreatWarningsAsErrors=true exit 0
Test vstest.console.exe <test assemblies> /InIsolation /EnableCodeCoverage exit 0 — 6522 total, 6522 passed, 0 failed

CoreCompile skip count across both msbuild invocations: 0, so the analyzer and nullable gates
actually compiled.

Evidence: docs/features/active/quickfiler-bug-family-446/evidence/qa-gates/post-rebase-verification.2026-08-26T11-42.md.

Coverage (re-derived from the committed Cobertura reports; every measured scope moved upward):

Scope Baseline Post-change
QfcStreamingDequeueConfidenceGate.cs 97.39%
QfcHomeController.Iteration.cs 100.00%
QfcFormController.Actions.cs 35.78% 47.89%
Repository-wide (unfiltered Cobertura root) 84.7782% 84.8402%

Recommended

dotnet tool restore
dotnet tool run csharpier check .
msbuild TaskMaster.sln /t:Rebuild /m /p:Configuration=Debug "/p:Platform=Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true
msbuild TaskMaster.sln /t:Rebuild /m /p:Configuration=Debug "/p:Platform=Any CPU" /p:TreatWarningsAsErrors=true
vstest.console.exe QuickFiler.Test\bin\Debug\QuickFiler.Test.dll /InIsolation /EnableCodeCoverage

Backward Compatibility / Migration Notes

  • IQfcDatamodel gains an enum, a struct and one overload. Existing declarations are untouched, so
    existing callers continue to compile and bind to the same overloads.
  • QfcDatamodel.ScoreRemainingQueueMailItemAsync changes its return type from Task<long> to
    Task<(long Score, string TopFolder)>. This is the one breaking signature change; all in-repo
    callers are updated in this change set.
  • No project file, .props, .targets or packages.config is modified. No file is added under
    QuickFiler/ or QuickFiler.Test/.
  • No non-owned production file is modified. The sibling partial declarations of QfcFormController
    and QfcHomeController, owned by other children of this epic, are untouched.

Risks and Mitigations

Risk Mitigation
The queue-close guard could over-correct and leave the queue open when the source really is drained. IterateQueueAsync_EmptyBatchWithSourceExhausted_CompletesAddingOnce pins Times.Once for the exhaustion case; its negative twin pins Times.Never for the deadline case.
The rewritten UndoConsumer could exit early and drop queued undo work. UndoConsumer_SuccessfulTake_ResetsIdleTimer advances a FakeTimeProvider 18 s in aggregate with no single idle gap past the threshold and asserts the loop kept draining, then exits only on an 11 s idle advance.
The rejected-hook release could unhook an item that is still in use. Release happens only on the rejection path, is invoked once per rejected item, and is guarded so a COM failure logs rather than aborting the scan.
This PR receives no CI checks (ci.yml does not trigger on this base). The full four-gate toolchain was re-run locally against the rebased tree; see Verification.
Rollback. The change is confined to six production files on one branch; reverting the merge commit restores the prior gate signature and caller behaviour.

Review Guide

Suggested order:

  1. QuickFiler/Interfaces/IQfcDatamodel.cs — the contract the rest of the change is written against.
  2. QuickFiler/Controllers/QfcStreamingDequeueConfidenceGate.cs — where the stop reason is decided.
  3. QuickFiler/Controllers/QfcHomeController.Iteration.cs — the 13-line change that actually closes Bug: iteratequeueasync-deadline-closes-queue-early #446.
  4. QuickFiler/Controllers/QfcDatamodel.QueueProcessing.cs and QfcDatamodel.cs — projection and scorer widening.
  5. QuickFiler/Controllers/QfcFormController.Actions.cs — the UndoConsumer rewrite (Bug: quickfiler-undoconsumer-nonterminating-loop #448), independent of the other three.
  6. Test files, then docs.

Noise notes:

  • QfcHomeControllerIterationTests.cs shows +228/-195 largely because of an ArrangeIterate helper
    de-duplication, not new assertions.
  • QfcFormControllerSeamTests.cs was compacted to stay under the 500-line file cap after the four
    new tests landed. No assertion was removed or weakened; the scoped run records 16/16 passed.
  • IterateQueueAsync_QueueEmpty had its arrangement changed (to stop: SourceExhausted), not its
    assertions: the base test encoded the defect under repair. Its three Verify calls and their
    Times arguments are byte-identical to the base version.
  • The 452,878-line insertion count is evidence artifacts (two Cobertura reports and TRX files), not
    source.

Follow-ups

Filed during this work:

Open items carried to the epic close-out:

  • AC28 requires a maintainer decision. Its checkbox demands >= 90% line coverage on the whole
    types QfcStreamingDequeueConfidenceGate, QfcFormController and QfcHomeController. Measured:
    97.39% / 55.37% / 71.05%. The latter two are partial types whose remaining declarations are owned
    by sibling children of this epic, and AC18 forbids modifying any non-owned production file. The two
    criteria cannot both hold for this change set. The blocking changed-file gate did pass
    (97.39 / 47.89 / 100.00). Feature review classified AC28 as PARTIAL and non-blocking, and
    recommends restating the criterion over the changed-file scope or deferring it to epic close.
  • Repository-wide line coverage reads 84.84% against the 85% uniform floor. This is the unfiltered,
    vendor-inclusive Cobertura denominator; the rate improved by 0.062 points against the same-session
    baseline and there is zero changed-line regression.
  • Dead using System.Diagnostics; in QfcFormController.Actions.cs — remove on the next authorized
    touch of that file (no plan task authorized it here).

Issue #427 is advanced but not completed by this PR: only the 427-A producer side is delivered.
It must remain open.

GitHub Auto-close

drmoisan and others added 14 commits August 26, 2026 11:36
The Phase 1 TRX artifacts are raw vstest output and embedded the absolute
worktree path, the bare account name and the machine name in 533 places
across 17 files. Replace them with REDACTED-REPO-ROOT, REDACTED-USER-PROFILE,
REDACTED-USER and REDACTED-HOST.

Plain tokens are used rather than the usual angle-bracket placeholders
because '<' is illegal in an XML attribute value and is not an entity inside
CDATA, so an angle-bracket form would leave the TRX unparseable. Every file
was re-parsed as well-formed XML after the edit, and all test names,
outcomes and counters are unchanged.

Also lands the [P1-T20] check-off and the p1-t20 red index, which that
task's own ordering (commit, then record the resulting HEAD sha) leaves
uncommitted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mic58ikwEhpXsTnhz9FShE
…17, AC20, AC21, AC23 and AC26

Phase 4 verification sweep: 21 acceptance criteria verified against evidence
on disk and checked off in spec.md, with one artifact per criterion under
evidence/qa-gates/.

Also files the four follow-up issues the sweep identified, via the MCP
promotion path, and mirrors each under evidence/issue-updates/:
- #620 three independent EmailMoveMonitor instances
- #621 QfcFormController.Cleanup() disposal ordering
- #622 dead scoreLoader parameter on QfcRemainingQueueAdmission
- #623 pre-existing 500-line cap violations

AC18, AC19, AC22, AC24, AC25, AC27 and AC28 remain unchecked by design; they
can only be judged after the Phase 5 formatting and toolchain pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mic58ikwEhpXsTnhz9FShE
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mic58ikwEhpXsTnhz9FShE
Adds the three review artifacts (policy-audit, code-review, feature-audit)
for the 2026-08-26T11-29 review. Verdict: zero blocking findings, 27 of 28
acceptance criteria PASS, branch fit to merge into the integration branch.

Also corrects issue.md, which listed #427 under "Also closes". Only the
427-A producer side is delivered here, so #427 must remain open. The line
now records #427 as advanced rather than closed, and points at the
closing-keyword constraint note so the PR author cannot transcribe the
old wording by accident.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mic58ikwEhpXsTnhz9FShE
The feature review accepted the QfcFormController.Actions.cs coverage
carve-out but recorded that no promoted document routed the resulting seam
debt. Prose in an active feature folder is lost when the folder is archived
at epic close, so the debt is now a real issue.

The issue also carries a correction the review established: the original
carve-out named only the MessageBox.Show calls, which understates the
problem. A dialog-only seam reaches roughly 67%, not 90%, because the
COM-bound LoadItems overloads at 29-160 dominate the uncovered set.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mic58ikwEhpXsTnhz9FShE
Phase 5 evidence was captured against merge base 61edc19. The integration
branch has since advanced twice, most recently by the merge of sibling epic
child 484 (PR #619). Because ci.yml triggers pull_request only on main and
development, a PR based on the integration branch gets zero CI checks, so
pre-rebase evidence would not describe the tree that actually merges.

All four gates re-run green against the rebased tree: csharpier check 0,
both /t:Rebuild gates 0 errors with a zero "Skipping target CoreCompile"
count proving they were not vacuous, and 6522 of 6522 tests passing. The
21-test increase over Phase 5 is sibling 484's added tests.

No collision: the sibling touched only QfcItemController.* files, and the
three types introduced here do not exist on the integration branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mic58ikwEhpXsTnhz9FShE
@drmoisan
drmoisan merged commit 902e5ce into epic/quickfiler-bug-family-integration Aug 26, 2026
drmoisan added a commit that referenced this pull request Aug 26, 2026
Projects the epic checkpoint after the fan-in-only run: 484 (PR #619), 446
(PR #625) and 498 (PR #626) are merged into the integration branch, whose tip
is now 8c8f769.

Records that 468 is halted rather than merged. Its stale-base merge is pushed
and its CI run is green, but its atomic plan is 120 of 180 tasks complete
(P13 stops at T3; P14, P15 and P16 never ran) and 14 of 29 acceptance criteria
in spec.md are unchecked, so no pull request was opened for it.

Also carries forward the child-PR CI trigger gap, the missing feature-review
artifacts, and the worktree-removal gate defect.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
drmoisan added a commit that referenced this pull request Aug 26, 2026
Projects the epic checkpoint after feature 468 merged as PR #636. Four of
twelve features are now on the integration branch: 484 (#619), 446 (#625),
498 (#626) and 468 (#636), whose merge is the current tip 808bf46.

Records that 468 landed on a second pass. It was halted earlier in this
session at 120 of 180 plan tasks with 14 of 29 acceptance criteria unchecked
and no feature review; it was re-delegated to resume at P13-T4 and is now
180 of 180 tasks and 28 of 29 criteria with three audit artifacts carrying
zero blocking findings. AC-28 remains unchecked by design, because an
integration-branch merge cannot close the seven referenced issues.

Also records the repository-wide line-coverage shortfall against the
rules-file floor and the absence of feature-review artifacts for 498.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@drmoisan
drmoisan deleted the bug/quickfiler-bug-family-446 branch August 28, 2026 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant