Bug: AgentChat resume test intermittently throws NullReferenceException in fast suite

[JoshuaRowePhantom]

Unrelated test failure — observed during checkin for #1493

Failing test: Phantom.Workspaces.Llm.Tests.AgentChatResumeTests.AgentChat_Resume_RunningSubAgents_StillResolveToSucceededWithPreservedTimestamp

Failure:

System.NullReferenceException: Object reference not set to an instance of an object.
at AgentChatResumeTests.AgentChat_Resume_RunningSubAgents_StillResolveToSucceededWithPreservedTimestamp() line 474

The failure occurred once in the fast suite (1 failure among 7,364 tests). No dump was produced. The exact test then passed 10/10 isolated runs, and the complete AgentChatResumeTests class passed 10/10 runs.

Why this is unrelated to #1493

The #1493 diff does not modify AgentChatResumeTests, SubAgent, AgentChatFactory, or RunningAgentChatLease. Its only AgentChat change adds/invokes a modal-response test hook; this restore test never enters modal-response code. The CTS disposal race fixed by #1493 is in RemoteAgentSessionLease.AttachmentPublisher; this failure has no remote-session stack or ObjectDisposedException, so it is not a recurrence of that race.

Root-cause hypothesis

This is suite-order/parallelism-sensitive lifetime interference around restored sub-agent lease acquisition or disposal. The source line contains an await using acquisition, so the generated state machine can attribute a null during acquisition/disposal to line 474 without exposing the originating object. Isolation removes the trigger.

Proposed solution

1. Split acquisition and disposal in this regression test into explicit statements and assert stub and lease before use so the next occurrence identifies which value is unexpectedly null.
2. Add a stress test that runs restored child materialization and parent/factory disposal concurrently.
3. Audit AgentChatFactory.ReleaseAsync/DisposeAsync coordination so factory disposal cannot clear entries while a child lease acquisition or release is in flight.
4. If shared AgentChat lifecycle tests mutate process-wide state, place them in a non-parallel xUnit collection or remove that shared state.

[JoshuaRowePhantom]

Design

Root cause

The failure line is the await using lease statement, so it covers both asynchronous acquisition and generated asynchronous disposal; SubAgent, its factory, and the returned local chat are non-null by construction. Isolated stress passes, which rules out a stable null payload. The race is the restore contract: RestoreSubAgentsAsync publishes each stub through an unawaited foreground task and records add/terminal tasks in two mutable lists, while the test-only waiter snapshots those lists. There is no single authoritative completion for the restore transaction, so callers can observe a partially published child set and later lease/disposal work.

Chosen approach

Make restore a single lifecycle-owned operation. Build the complete stub snapshot, apply restored state, publish the snapshot on the already-authoritative foreground execution, and complete one task exactly once with success, the original exception, or cancellation. Creation will not return a partially restored AgentChat; consumers/tests await that same readiness contract rather than snapshots, polling, retries, delays, null-forgiving, or dispatcher pumping. Disposal will cancel/drain initialization before resource teardown, and no restore callback may publish after failure/disposal.

Tests will be written first and deterministically gate persistence/events to cover synchronous success, failure and original-exception preservation, cancellation/disposal during resume, repeated/replacement restore attempts and stale callbacks, independent concurrent chats/sessions, exactly-once readiness publication, and absence of leaked work. Existing completion-state/timestamp assertions remain strict.

Files

• Phantom.Workspaces.Llm.Core/AgentChat.cs
• Phantom.Workspaces.Llm.Core.Tests/AgentChatResumeTests.cs

[JoshuaRowePhantom]

Implementation notes

Commits: 696dca351c1ddb7a787e200bb0ed16f35d90a87e

Root cause confirmed

The source line covered generated await using acquisition/disposal, but the lease/chat were not intrinsically nullable. The actual race was the restore observation contract: child stubs were published by detached foreground tasks while the waiter snapshotted two mutable task lists. Creation could therefore return before one authoritative restore state existed. Isolated pre-fix stress passed 12 times and the pre-fix fast suite passed 7,395/7,395, confirming suite-order sensitivity rather than a stable null object.

Deviations from proposed design

None. The implementation uses one lifecycle completion task and foreground-atomic child publication. Existing lazy child materialization remains intact.

Scope changes

Added cleanup-on-initialization-failure so partially constructed resources cannot leak. The flaky assertion now splits acquisition, chat observation, assertion, and disposal so any future contract violation has an exact source operation.

Files changed

• Phantom.Workspaces.Llm.Core/AgentChat.cs
• Phantom.Workspaces.Llm.Core.Tests/AgentChatResumeTests.cs
• Phantom.Workspaces.Llm.Core.Tests/AgentChatFactoryTests.cs
• Phantom.Workspaces.Llm.Core.Tests/CopilotSdkEventPipelineTests.cs

Validation

• Build: passed, 0 errors
• Targeted lifecycle tests: 68/68 passed
• Stability: 10/10 runs, 68 tests/run
• Fast: 7,401/7,401 passed
• Full: 7,633/7,633 passed

An initial fast run encountered pre-existing unrelated test-host hangs/crash (0 assertion failures); the clean rerun passed completely. No push performed.

[JoshuaRowePhantom]

Local verification — VERIFIED ✅

Fresh independent verification of commit 696dca35 (branch fix/1577-agent-chat-resume-readiness) per skill/CODE-VERIFICATION. Implementation notes were not trusted; conclusions come from reading the issue/comments, the full commit diff, and the current production/test source, plus running the tests via scripts/run-tests.ps1 only.

Root cause & fix (real, not suppression/timing)

The NullReferenceException at the await using line was suite-order/parallelism sensitivity in the restore observation contract, not a nullable lease/chat. Pre-fix, RestoreSubAgentsAsync published each child stub through detached foreground Task.Factory.StartNew(subAgentItems.Add, …) tasks and tracked two mutable lists that a test-only waiter snapshotted, so creation could return with SubAgents partially published.

The fix makes restore a single lifecycle-owned, atomically-published operation:

• RestoreSubAgentsAsync runs on the foreground scheduler (via RunSessionInitAsync/RunOnForegroundAsync) and adds the complete prepared stub snapshot directly to subAgentItems synchronously — no detached Add tasks, no snapshot waiter. The only await is the store read at the top; after a single ThrowIfCancellationRequested, table-map population and item publication are fully synchronous → no half-published state.
• One TaskCompletionSource restoreCompletion (RunContinuationsAsynchronously) completed exactly once: TrySetResult on success, TrySetCanceled(token) on cancellation, TrySetException with the primary exception preserved via ExceptionDispatchInfo. CreateAsync awaits it before returning, so a returned AgentChat is always fully restored. RestoreCompleted exposes the same handshake to the construction hook.
• Cancellation/disposal: InitializeAsync uses a linked CTS (request token + cts). DisposeCoreAsync cancels cts (cancelling the in-flight restore read before publication), then drains restoreCompletion and processTask before tearing down owned resources/child chats. Disposal is guarded by an Interlocked disposeStarted flag shared with the CreateAsync failure path, so cleanup runs exactly once and never double-disposes or races an external DisposeAsync.
• Init-failure cleanup added so partially-constructed owned resources cannot leak.
• Existing semantics preserved: lazy child materialization, SetRestoredCompletionState(Succeeded), CompletionStateChanged, and Reloading a Copilot SDK session should mark all restored SDK sub-agents completed #1128/Reloading a session bumps completed sub-agents' last-updated time to the reload time (should preserve original) #1140/Regression: startup dialog hangs (stuck in front) when restoring sub-agents from default workspaces — 'Agent definition does not specify a model' #1186/Bug: flaky test — AgentChatFactoryTests.RestoreSubAgentsAsync_ChildTerminalTaskFaults_ParentInitializeCompletes (SubAgents.Count race) #1459 behaviors are intact. No lock/await deadlock, reentrancy, double-subscription, or event-loss issue found in the 176-line rewrite.

Test quality (deterministic; forces production path, not the old race)

• The old CapturingTaskScheduler with manual Drain()/RunPending() (fixed dispatcher-pass pumping) is removed and replaced with a ThreadPool-backed ObservingTaskScheduler + event-driven TaskCompletionSource synchronization anchored to observable state (CollectionChanged, ReadStarted).
• The historically flaky test now splits acquisition / chat observation / assertion / disposal for exact fault attribution while keeping strict Succeeded + preserved-timestamp asserts (no weakening).
• New tests cover exactly-once complete-snapshot publication, original-exception preservation + owned-resource disposal on failure, cancellation without partial state, disposal-during-read cancel/drain, replacement/stale-publication isolation, and independent concurrent sessions.
• No [Skip]/quarantine, no unresolved TODOs, no Task.Delay/Thread.Sleep/polling/fixed timeouts, no .Result/.Wait()/.GetAwaiter().GetResult() on the UI thread or inside a lock.

Test execution (scripts/run-tests.ps1 only)

Relevant classes: AgentChatResumeTests, AgentChatFactoryTests, CopilotSdkEventPipelineTests (in Phantom.Workspaces.Llm.Core.Tests).

• Single run: Executed 64 / Passed 64 / Failed 0.
• 10-run stability (-NoBuild): 10/10 runs, 64/64 each, 0 failed runs.

Result: VERIFIED — verified-locally.

[JoshuaRowePhantom]

Checked in

Commit: 696dca3
Branch fast-forwarded: features → 696dca3

[JoshuaRowePhantom]

Release-workflow recurrence

The corrected v0.0.21 workflow run 35067649486 failed CopilotSdkChatClientTests.RunStreamingTurn_WhenImmediateSendFaults_DoesNotRaiseUnobservedException. Its process-global GC handler captured an unrelated InvalidOperationException: Agent definition could not be resolved from request or persistence store from AgentChat.InitializeAsync/CreateAsync. This indicates the #1577 restore/readiness lifecycle leaves a faulted internal task unobserved on initialization failure even though the public create failure is observed. Reopened for exact task-ownership correction before release.

[JoshuaRowePhantom]

Retry design — exact AgentChat creation task ownership

Root cause

#1577 added a second faultable restoreCompletion TCS beside the public CreateAsync task. On initialization/restore failure, CreateAsync faulted that TCS, skipped awaiting it during owner cleanup, and rethrew the original exception through its own async-method task. A caller could correctly observe the public failure while the duplicate readiness task remained unobserved and later contaminated an unrelated process-global finalizer test. The construction callback is also outside the cleanup try.

Chosen approach

Make the lifecycle/readiness signal the exact Task<AgentChat> returned by AgentChat.CreateAsync. A non-faulting completion driver will run the callback and initialize/cleanup the chat, transferring exactly one success, original failure, or cancellation (with its token) to that public task. RestoreCompleted will reference the same task, eliminating the independently faulted readiness task. Concurrent disposal continues to claim cleanup exactly once and drain the shared task without circular waits.

Tests will cover unresolved definitions through the public factory, restore/initialize failures, cancellation identity, callback failure, synchronous completion, concurrent dispose/failure, and repeated failures while asserting task identity and resource cleanup. The steering test will stop subscribing to process-global UnobservedTaskException; it will instead assert the exact ForwardSteeringAsync tasks settle successfully after owning synchronous and returned-task faults.

Files

• Phantom.Workspaces.Llm.Core/AgentChat.cs
• Phantom.Workspaces.Llm.Core/AgentFactory.cs (creation-token propagation)
• Phantom.Workspaces.Llm.Core.Tests/AgentChatResumeTests.cs
• Phantom.Workspaces.Llm.Core.Tests/AgentFactoryTests.cs
• Phantom.Workspaces.Llm.Core.Tests/CopilotSdkChatClientTests.cs

[JoshuaRowePhantom]

Retry implementation notes

Commit: c3336da018825d3833a6bd12dbbc89eee0e156bf
Base: exact features commit 5384ddb9c9c47b5e8d9b04d7f1e782dd98f095d7

Root cause confirmed

#1577 introduced a second faultable restoreCompletion TCS. Initialization failures faulted it, cleanup explicitly skipped awaiting it, and CreateAsync rethrew through a different async-method task. Thus Assert.RecordExceptionAsync correctly observed the public create task while the duplicate readiness task remained unobserved until finalization.

Fix

• RestoreCompleted is now the exact public Task<AgentChat> returned by AgentChat.CreateAsync; there is no independently faultable readiness task.
• A non-faulting, lifecycle-owned creation driver transfers exactly one result/failure/cancellation to that task after exactly-once cleanup. Callback failures are inside the same ownership path.
• Concurrent disposal drains the same public task with SuppressThrowing; failures are owned by the create caller and are not double-reported by disposal.
• Request cancellation now flows through the public factory and all persistence initialization reads; cancellation preserves request-token identity.
• Failed construction settles/cancels HistoryPopulated, and tests explicitly inspect the creation driver, readiness, history, background initialization, and process-loop tasks.
• Replaced the process-global UnobservedTaskException/forced-GC steering test with direct assertions that the exact ForwardSteeringAsync tasks own synchronous and returned-task faults.

Tests

• Build: passed, 0 errors/warnings emitted by filter.
• Expanded targeted lifecycle classes: 241/241 passed.
• Stability: 10/10 runs, 241/241 per run.
• Fast: 7,418/7,418 passed. Initial fast run had one unrelated concurrent MXC integration build artifact race; exact test passed isolated, clean fast rerun passed.
• Full: 7,650/7,650 passed.

Files changed

• Phantom.Workspaces.Llm.Core/AgentChat.cs
• Phantom.Workspaces.Llm.Core/AgentFactory.cs
• Phantom.Workspaces.Llm.Core.Tests/AgentChatResumeTests.cs
• Phantom.Workspaces.Llm.Core.Tests/AgentFactoryTests.cs
• Phantom.Workspaces.Llm.Core.Tests/CopilotSdkChatClientTests.cs

No push, amend, or rebase performed.