Skip to content

Harden cancellation source construction and disposal - #22

Merged
harder merged 2 commits into
mainfrom
fix/cancellation-source-lifecycle
Sep 2, 2026
Merged

harder merged 2 commits into
mainfrom
fix/cancellation-source-lifecycle

Conversation

@harder

@harder harder commented Sep 2, 2026 •

Copy link
Copy Markdown
Owner

Summary

Closes residual lifecycle and resource-retention gaps in the guarded cancellation owner introduced by PR #21. Disposal now has one consistent admission contract, constructor failures use callback-safe deferred cleanup, and already-canceled parents do not leave abandoned owners rooted by long deadline timers.

Why this batch

The explicit SkillView hardening backlog and all lower-risk checklist items are complete. A post-PR #21 residual-risk pass revisited the new cancellation primitive's construction and disposal boundaries and found three related concerns:

  • TryGetActiveToken rejected work as soon as disposal was requested, while cancellation admission remained open until physical resources were disposed. Repeated cancellation in that narrow window was an already-canceled CTS no-op, but the two admission boundaries disagreed with each other and the documented contract.
  • timeout validation rejected non-positive values but left Timer to reject values above its supported uint.MaxValue - 1 millisecond range after parent registrations were already installed;
  • an already-canceled parent could still be followed by a long-lived deadline timer, retaining an otherwise abandoned cancellation owner in the timer queue for up to ten minutes in current production callers.

The primitive protects root cancellation, command deadlines, subprocess termination, shared flights, request gates, modal ownership, workspace lifetimes, cleanup, and removal.

Changes

  • Rename the internal cancellation method to TryCancel; its Boolean reports whether disposal admission was open, not whether it was the first cancellation request.
  • Reject cancellation as soon as _disposeRequested is published, matching TryGetActiveToken and the documented contract.
  • Remove the now-redundant _resourcesDisposed flag. Closing admission plus the active-cancellation count makes the transition that reaches zero the unique physical disposer.
  • Validate the complete conservative System.Threading.Timer due-time range before creating the underlying source or registering parents.
  • Validate injected Discover metadata-preview deadlines at SearchAgentMetadataLoader construction rather than faulting a whole search from inside a worker.
  • Record parent registrations only after successful registration.
  • Assign all callback-visible parent state before creating a deadline timer, create that timer disabled, then arm it. Timer creation/arming failures use normal deferred disposal so active callbacks quiesce before resources are released.
  • Stop registering additional parents after one parent has canceled the owner and avoid creating a deadline timer for an already-canceled owner.
  • Remove the cache's void TryCancel wrapper and call the owner method directly.
  • Update AGENTS.md, the UI lifecycle guide, and the adversarial audit with the durable construction/disposal rules and independent-review disposition.
  • Correct stale audit status text now that PRs Harden removal, async lifecycles, cache, and logging #12 through Contain cancellation callback faults across owned lifetimes #21 and Finding 13's replay/attach remediation are complete.

User impact / release notes

  • A request whose parent lifetime is already canceled no longer leaves its owner rooted by a deadline timer for the remainder of a five- or ten-minute timeout.
  • Invalid oversized injected deadlines fail at component construction rather than aborting an active Discover search.
  • Rare deadline construction failures cannot dispose the underlying source while a parent or timer callback is still active.
  • Cancellation admission is internally consistent after disposal begins.

There are no CLI flags, JSON schema, TUI behavior, or public API changes.

Regression coverage

  • A serialized WeakReference test proves an abandoned owner with an already-canceled parent and five-minute deadline is immediately collectible. This fails if the deadline timer is recreated.
  • A dedicated long-running callback test disposes the owner during active cancellation and proves a second request is rejected.
  • Oversized owner and metadata-preview deadlines fail at their constructor boundaries with the correct parameter names.
  • Existing callback-fault, parent, deadline, self-disposal, execution-context, contention, search-timeout, cache, modal, and workspace coverage remains intact.

Independent Claude Code review

Claude Code Opus independently ran approximately 1,900 barrier-synchronized construction, parent-cancellation, deadline, TryCancel, disposal, and throwing-callback races without an escaping failure. Seven recommendations were accepted:

  1. add the canceled-parent timer-retention regression;
  2. describe admission as contract consistency rather than a user-visible shutdown fix;
  3. remove redundant _resourcesDisposed state;
  4. make constructor rollback use the owner's normal callback-deferral protocol;
  5. validate oversized injected metadata deadlines at their entry boundary;
  6. correct the range-test name; and
  7. distinguish owner admission with TryCancel and remove the misleading cache wrapper.

One optional suggestion was not adopted: SkillView keeps the conservative TimeSpan upper bound rather than depending on Timer truncating a fractional millisecond at the approximately 49.7-day maximum. Production deadlines are at most ten minutes, so accepting that fractional edge adds no practical capability and couples validation to a runtime conversion detail.

Verification

  • dotnet format --no-restore --verify-no-changes
  • git diff --check
  • Debug build: zero warnings and zero errors
  • Debug full suite: 803 passed
  • Release build: zero warnings and zero errors
  • Release full suite: 803 passed
  • Focused threading/resource regressions: 10 passed
  • Focused metadata-loader regressions: 7 passed
  • Standalone macOS ARM64 Native AOT publish with IL2026, IL3050, and IL3053 promoted to errors
  • gh-extension macOS ARM64 Native AOT publish

CI repeats tests, CodeQL analysis, and Native AOT smoke publishes on Linux, macOS, and Windows.

Compatibility and risk

CancellationSource is internal. Existing lifecycle behavior remains synchronous for admitted calls, callback failures remain contained and reported, and physical disposal remains deferred until every already-admitted callback finishes. The disabled-then-armed timer construction closes the only interval where a callback could observe partially assigned constructor state.

@harder
harder merged commit 9328374 into main Sep 2, 2026
8 checks passed
@harder
harder deleted the fix/cancellation-source-lifecycle branch September 2, 2026 04:20
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