Skip to content

Bug: qfc-collectioncontroller-removespecificcontrolgroup-counter-leak #286

Description

@drmoisan
  • Work Mode: full-bug

Summary

QfcCollectionController.RemoveSpecificControlGroupAsync (QuickFiler/Controllers/QfcCollectionController.cs:1142-1233) increments a static reentrancy counter at method entry but only decrements it at the normal exit path, with no try/finally. Any exception thrown mid-method leaves the counter permanently incremented, which then produces a false-positive "race condition" error log on every subsequent call for the remaining life of the process.

Environment

  • OS/version: n/a (logic defect, reproducible on any platform running QuickFiler)
  • Python version: n/a
  • Command/flags used: n/a
  • Data source or fixture: QuickFiler/Controllers/QfcCollectionController.cs

Steps to Reproduce

  1. Inspect QfcCollectionController.cs:1142: private static int removespecificcontrolgroupcounter = 0;
  2. Inspect RemoveSpecificControlGroupAsync (line 1144): Interlocked.Increment(ref removespecificcontrolgroupcounter); at line 1146 (method entry), followed by ~80 lines of method body (navigation unregister/register, active-item handling), then Interlocked.Decrement(ref removespecificcontrolgroupcounter); at line 1232 (method's final statement).
  3. Note there is no try/finally wrapping lines 1146-1232.
  4. Trigger any exception between the increment (line 1146) and the decrement (line 1232) - e.g. an unhandled exception from UnregisterNavigation(), RegisterNavigation(), or any of the active-item/theme handling in between.
  5. The decrement at line 1232 never executes; removespecificcontrolgroupcounter remains permanently incremented above its pre-call value.
  6. Every subsequent call to RemoveSpecificControlGroupAsync now sees removespecificcontrolgroupcounter > 1 at line 1222 and logs logger.Error("RemoveSpecificControlGroupAsync: Counter is greater than 1. Race Condition Exists") - a false positive, since no concurrent reentrancy is actually occurring.

Expected Behavior

The counter should always return to its pre-call value once the method returns or throws, so the race-condition check at line 1222 only fires on genuine concurrent reentrancy, not on a leaked count from an earlier unrelated exception.

Actual Behavior

The decrement is skipped whenever the method body throws, permanently inflating the counter and causing the race-condition detector to report a false positive on every future call, masking the detector's ability to catch a real race condition later (since the counter is already above 1 regardless of actual concurrency).

Logs / Screenshots

  • Attached minimal logs or screenshot
  • Snippet: Confirmed directly against source at QuickFiler/Controllers/QfcCollectionController.cs:1142 (counter field), :1146 (increment), :1222-1227 (race-condition check and logger.Error call), :1232 (decrement) - no try/finally present. Originally identified in docs/features/archive/2026-07-03-quickfiler-navigation-key-collision-232/evidence/other/follow-up-candidates.md (item 3, "removespecificcontrolgroupcounter reentrancy-counter hygiene"), explicitly deferred as out of scope for issue Bug: quickfiler-navigation-key-collision #232. No open GitHub issue currently references this (verified via gh issue list --search "RemoveSpecificControlGroup").

Impact / Severity

  • Blocker
  • High
  • Medium
  • Low

The counter is diagnostic/observability-only (it drives a log statement, not control flow), so this does not directly corrupt user-facing behavior. However it permanently degrades the reliability of the race-condition detector after the first unrelated exception, producing misleading error-log noise that could mask or be confused with a genuine future race condition.

Source

From: docs/features/potential/2026-07-09-qfc-collectioncontroller-removespecificcontrolgroup-counter-leak.md

Activity

  1. drmoisan commented on Aug 8, 2026

    @drmoisan
    OwnerAuthor

    Two additional findings (from issue #454 preparation research)

    Preparation research for issue #454 (epic #136, child F11) re-confirmed this defect and found two
    aspects the original report does not cover. Note that the line numbers have shifted since this issue
    was filed; on the current epic/quickfiler-per-file-coverage-integration tip the relevant lines in
    QuickFiler/Controllers/QfcCollectionController.cs are :1157 (field), :1161 (increment),
    :1237-1241 (check and error log), and :1247 (decrement).

    1. The counter is process-global across controller instances, so the check produces false positives without any exception

    removespecificcontrolgroupcounter is declared private static at :1157. It is therefore shared by
    every QfcCollectionController instance in the process, not scoped to one controller.

    Two independent controllers running RemoveSpecificControlGroupAsync concurrently — a normal
    condition, not an error — will trip the > 1 check at :1237 and log
    "RemoveSpecificControlGroupAsync: Counter is greater than 1. Race Condition Exists" at :1239-1241
    even though no reentrancy has occurred on either instance.

    This means the detector reports false positives on the very first concurrent use, independently of
    the exception-leak path already described in this issue. If the intent is to detect reentrancy on a
    single controller, the counter should be an instance field.

    2. The read at the check site is a plain non-volatile read of an Interlocked-mutated field

    Line 1237 reads the field directly while :1161 and :1247 mutate it via Interlocked.Increment /
    Interlocked.Decrement. Mixing an unsynchronized read with interlocked writes does not guarantee the
    reading thread observes the latest value. The read should use Interlocked.CompareExchange(ref counter, 0, 0), Volatile.Read, or the value returned directly by the Interlocked.Increment at
    :1161.

    Scope note

    Issue #454 does not fix this. Under epic #136's no-behavior-change NFR it will only characterize
    the current behavior. The three changes needed here — instance-scoped field, try/finally around
    the decrement, and a synchronized read — are all behavior changes and belong to this issue.

    Full analysis: docs/features/active/2026-08-07-quickfiler-collection-controller-coverage-454/research/qfc-collection-controller.md
    section E7.

  2. drmoisan commented on Aug 31, 2026

    @drmoisan
    OwnerAuthor

    Closing as completed. Delivered on main under the sibling qfc-collection-controller-defects-468 feature (spec AC-1, checked [x]).

    RemoveSpecificControlGroupAsync now wraps its full body in try/finally, with Interlocked.Decrement(ref removespecificcontrolgroupcounter) moved into the finally block. The counter now returns to its pre-call value on both the normal and the exceptional exit path, so an exception mid-method can no longer leave it permanently inflated. The fix carries an inline comment explicitly citing this issue:

    // Issue #286: the decrement must also run when the body throws. Before this change it was the method's last statement, so any exception left the process-wide counter permanently one higher and eventually tripped the race-condition error branch above on a later, legitimate call.

    Verified during an admission check against the bugs-638-644-647 parallel run (2026-09-01) by reading the method directly from origin/main:QuickFiler/Controllers/QfcCollectionController.cs. No branch or PR remains to ship; this was left open only as bookkeeping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions