fix: Expire zombie subagents blocking IDLE-DEFER after 20 minutes

[PureWeen]

Split from #508. Separate concern — handles zombie subagent entries that prevent session completion.

[PureWeen]

🔍 Multi-Model Code Review — PR #511

PR: fix: Expire zombie subagents blocking IDLE-DEFER after 20 minutes
CI Status: ⚠️ No CI checks on this branch. Pre-existing build errors in WsBridgeIntegrationTests.cs (confirmed same on main).
Existing Reviews: None.

---

🔴 CRITICAL — Stale SubagentDeferStartedAt Leaks Across Turns (Premature Zombie Expiry)

Flagged by: All 3 reviewers independently
Files: CopilotService.cs:3291, 3582, 3775, 3811, 3883, 3906, 4059, 4185, 4473 / CopilotService.Events.cs:890, 1995, 2602, 2701, 2757, 2857, 2895

SubagentDeferStartedAt is only cleared in CompleteResponse (Events.cs:1246), but its companion field HasDeferredIdle is cleared in 17 different paths — SendPromptAsync, AbortSessionAsync, watchdog timeout, reconnect, error handling, IDLE-DEFER-REARM, etc. None of those other 16 paths clear SubagentDeferStartedAt.

Concrete failure scenario:

1. Turn N enters IDLE-DEFER → SubagentDeferStartedAt = T0
2. Turn N ends via watchdog/abort/error (not CompleteResponse) → HasDeferredIdle = false, but SubagentDeferStartedAt is still T0
3. Turn N+1: user sends new prompt → SendPromptAsync resets HasDeferredIdle but NOT SubagentDeferStartedAt
4. Turn N+1: legitimate IDLE-DEFER fires → ??= preserves stale T0 (20+ min old)
5. HasActiveBackgroundTasks(idle, stale_T0) → immediately expires real, healthy subagents as zombies

Impact: Active subagents are killed on their first IDLE-DEFER of the new turn. Session completes prematurely with truncated/missing results. Especially dangerous after user abort — next orchestration is permanently broken.

Fix: Add state.SubagentDeferStartedAt = null; alongside every state.HasDeferredIdle = false; assignment. The two fields are an inseparable companion pair. Most critical locations: SendPromptAsync (line 3291) and AbortSessionAsync (line 4059).

---

🟡 MODERATE — Thread Safety: DateTime? is Not Atomic

Flagged by: All 3 reviewers
File: CopilotService.cs:706

public DateTime? SubagentDeferStartedAt;  // ⚠️ NOT volatile

Every other companion field in SessionState is declared volatile (HasUsedToolsThisTurn, HasDeferredIdle, WasUserAborted, etc.) or uses Interlocked operations. Nullable<DateTime> is a 12–16 byte struct — reads/writes are NOT atomic. The field is written on the SDK event thread (Events.cs:692) and cleared on the UI thread (CompleteResponse).

Fix: Use long ticks with Interlocked.CompareExchange/Interlocked.Read — matching the existing pattern for TurnEndReceivedAtTicks:

// 0 = not set; positive = UTC ticks
private long _subagentDeferStartedAtTicks;
// Set:  Interlocked.CompareExchange(ref _subagentDeferStartedAtTicks, DateTime.UtcNow.Ticks, 0)
// Clear: Interlocked.Exchange(ref _subagentDeferStartedAtTicks, 0)
// Read:  var ticks = Interlocked.Read(ref _subagentDeferStartedAtTicks);

---

🟡 MODERATE — Console.WriteLine Bypasses Diagnostic Log Pipeline

Flagged by: All 3 reviewers
File: CopilotService.Events.cs:2163-2166

Console.WriteLine(
    $"[IDLE-DEFER] Zombie expiry: {bt.Agents!.Length} background agent(s) still ...");

Every other event in the handler uses Debug() which routes to ~/.polypilot/event-diagnostics.log. Console.WriteLine goes to stdout — invisible on Mac Catalyst/iOS/Android and absent from the diagnostics log. Per the codebase convention, every code path that terminates session processing MUST have a diagnostic log entry with a tag.

Fix: Since HasActiveBackgroundTasks is static (no access to Debug()), either:

• (a) Move the expiry decision + logging to the call site where Debug() is available, or
• (b) Return the expiry state and let the caller log with [IDLE-DEFER-ZOMBIE] tag

---

🟢 MINOR — Missing Test for Cross-Turn Stale Timestamp Scenario

Flagged by: All 3 reviewers
File: ZombieSubagentExpiryTests.cs

The test suite thoroughly covers HasActiveBackgroundTasks behavior in isolation but does NOT test the critical lifecycle failure: a pre-populated SubagentDeferStartedAt from a previous turn surviving into the next turn. A test simulating "SubagentDeferStartedAt is 25 min old from a prior turn, current turn has fresh agents → should NOT expire" would directly expose the stale-timestamp bug.

---

🟢 MINOR — Boundary Test Is Time-Sensitive

Flagged by: 2/3 reviewers
File: ZombieSubagentExpiryTests.cs:117-119

ZombieThresholdExactlyMet_ReturnsFalse creates a timestamp at exactly the boundary, then HasActiveBackgroundTasks calls DateTime.UtcNow again internally. The test relies on nonzero execution time between the two clock reads. While stable at minute-level granularity, injecting a clock abstraction or using fixed timestamps would eliminate any edge-case fragility.

---

Discarded Findings

Finding	Flagged by	Reason discarded
??= set-before-check ordering (timestamp set even when no BG tasks)	1/3	Harmless — immediately cleared by CompleteResponse on fall-through. Other reviewers verified as non-issue.

---

Test Coverage Assessment

The new ZombieSubagentExpiryTests.cs (195 lines, 14 tests) provides excellent coverage of the pure HasActiveBackgroundTasks function. However, the critical risk of this PR is state lifecycle management, not the helper logic — and no lifecycle tests exist for the new field.

---

⚠️ Recommendation: Request Changes

The core logic is sound and well-designed. The zombie expiry concept is correct, the shell exclusion is proper, the ??= first-write semantics are right within a single turn, and the test suite for the helper function is thorough.

However, the stale SubagentDeferStartedAt across turns is a 🔴 CRITICAL regression risk that can silently kill legitimate subagents after any abort/watchdog/error. This follows the exact pattern this codebase has regressed on 13+ times (companion fields not cleared in all paths).

Specific asks:

1. Add state.SubagentDeferStartedAt = null; alongside every state.HasDeferredIdle = false; (16 locations)
2. Convert DateTime? to long ticks with Interlocked operations
3. Move Console.WriteLine to use Debug() / diagnostic log pipeline
4. Add at least one lifecycle test for cross-turn stale timestamp scenario

[PureWeen]

Thanks for the thorough review! All four findings addressed in 6d615e7:

🔴 CRITICAL — Stale SubagentDeferStartedAtTicks across turns
Added Interlocked.Exchange(ref state.SubagentDeferStartedAtTicks, 0L) alongside every HasDeferredIdle = false assignment — all 17 locations across SendPromptAsync, AbortSessionAsync, watchdog abort, reconnect, error handlers, sibling reconnect, and new-state reset paths. The field is now a tracked inseparable companion pair with HasDeferredIdle.

🟡 MODERATE — DateTime? → long ticks with Interlocked
Replaced SubagentDeferStartedAt with SubagentDeferStartedAtTicks (long), matching the TurnEndReceivedAtTicks pattern. Set via Interlocked.CompareExchange(0 → now) (preserves first-write timestamp); cleared via Interlocked.Exchange(0); read via Interlocked.Read.

🟡 MODERATE — Console.WriteLine → Debug() at call site
HasActiveBackgroundTasks is static so can't call Debug() directly. Moved the zombie expiry log to the IDLE-DEFER call site with tag [IDLE-DEFER-ZOMBIE], routing through ~/.polypilot/event-diagnostics.log consistently with all other state transitions.

🟢 MINOR — Cross-turn stale timestamp test
Added StaleDeferTimestamp_FromPriorTurn_NewTurnShouldNotExpireAgents which documents the exact failure scenario (25-min-old ticks passed to a fresh IDLE-DEFER immediately expires live agents) and verifies fresh ticks work correctly. All tests updated to use long ticks.

[PureWeen]

PR #511 Review (R3) — ✅ SHIP IT

3/3 reviewers verified all 18 companion-pair sites. No issues found. Merging.