Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions PolyPilot.Tests/ChatExperienceSafetyTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,73 @@ public async Task CompleteResponse_GenerationMatch_Executes()
"Normal completion should allow one late TurnStart to recover from premature idle");
}

/// <summary>
/// ClearProcessingState must increment ProcessingGeneration so that any InvokeOnUI
/// callback captured before completion sees a generation mismatch and bails out.
/// Without this, the generation guard passes and only the !IsProcessing check
/// prevents resurrection of completed turns — a fragile single-point-of-failure.
/// </summary>
[Fact]
public async Task ClearProcessingState_IncrementsGeneration()
{
var svc = CreateService();
await svc.ReconnectAsync(new ConnectionSettings { Mode = ConnectionMode.Demo });
var session = await svc.CreateSessionAsync("gen-increment-test");

var state = GetSessionState(svc, "gen-increment-test");
session.IsProcessing = true;
SetField(state, "ProcessingGeneration", 5L);
SetField(state, "SendingFlag", 1);
SetResponseCompletion(state, new TaskCompletionSource<string>(TaskCreationOptions.RunContinuationsAsynchronously));

var genBefore = GetField<long>(state, "ProcessingGeneration");

// Act: CompleteResponse calls ClearProcessingState internally
InvokeCompleteResponse(svc, state, 5L);

var genAfter = GetField<long>(state, "ProcessingGeneration");

Assert.False(session.IsProcessing);
Assert.True(genAfter > genBefore,
$"ClearProcessingState must increment ProcessingGeneration (was {genBefore}, now {genAfter}). " +
"This prevents resurrection of completed turns by stale InvokeOnUI callbacks.");
}

/// <summary>
/// Demonstrates the resurrection scenario: a stale callback that captured the
/// generation before CompleteResponse ran must see a mismatch and bail out,
/// rather than re-setting IsProcessing=true on a completed session.
/// </summary>
[Fact]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 MINOR — Test verifies arithmetic, not behavior (Flagged by: 2/3 reviewers)

This test checks that 6 ≠ 5 after incrementing — tautologically true regardless of whether the production guard works. A stronger test would create a closure simulating a stale callback, complete the session, execute the closure, and assert IsProcessing was NOT changed.

Also: only the ClearProcessingState path (via CompleteResponse) is tested. The other 3 modified paths have zero generation increment verification.

Not blocking — the first test (ClearProcessingState_IncrementsGeneration) is solid.

public async Task GenerationGuard_PreventsResurrection_AfterCompletion()
{
var svc = CreateService();
await svc.ReconnectAsync(new ConnectionSettings { Mode = ConnectionMode.Demo });
var session = await svc.CreateSessionAsync("resurrection-test");

var state = GetSessionState(svc, "resurrection-test");
session.IsProcessing = true;
SetField(state, "ProcessingGeneration", 5L);
SetField(state, "SendingFlag", 1);
SetResponseCompletion(state, new TaskCompletionSource<string>(TaskCreationOptions.RunContinuationsAsynchronously));

// Simulate: stale callback captures generation BEFORE completion
var capturedGen = GetField<long>(state, "ProcessingGeneration");
Assert.Equal(5L, capturedGen);

// Turn completes — ClearProcessingState increments generation
InvokeCompleteResponse(svc, state, 5L);
Assert.False(session.IsProcessing);

// Stale callback fires and checks its captured generation
var currentGen = GetField<long>(state, "ProcessingGeneration");
var guardPasses = currentGen == capturedGen;

Assert.False(guardPasses,
$"Stale callback with captured gen={capturedGen} must see mismatch (current={currentGen}). " +
"Without the increment, this would pass and allow resurrection.");
}

/// <summary>
/// Late TurnStart events should only revive sessions after speculative auto-completion.
/// Explicit aborts, watchdog kills, and force-complete recovery paths must not be re-armed
Expand Down
5 changes: 5 additions & 0 deletions PolyPilot/Services/CopilotService.Events.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3114,6 +3114,11 @@ private void ClearProcessingStateForRecoveryFailure(SessionState state, string s
state.PendingReasoningMessages.Clear();
if (state.Info.ProcessingStartedAt is { } started)
state.Info.TotalApiTimeSeconds += (DateTime.UtcNow - started).TotalSeconds;
// Increment generation to invalidate stale InvokeOnUI callbacks —
// mirrors ClearProcessingState (see PR #612).
// Skip if orphaned (long.MaxValue) to avoid overflow.
if (Interlocked.Read(ref state.ProcessingGeneration) != long.MaxValue)
Interlocked.Increment(ref state.ProcessingGeneration);
state.Info.IsProcessing = false;
state.Info.IsResumed = false;
state.HasUsedToolsThisTurn = false;
Expand Down
12 changes: 12 additions & 0 deletions PolyPilot/Services/CopilotService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,14 @@ private void ClearProcessingState(SessionState state, bool accumulateApiTime = t
ClearFlushedReplayDedup(state);
state.PendingReasoningMessages.Clear();

// Increment generation so any InvokeOnUI callback that captured the old
// generation before this ClearProcessingState call will see a mismatch and
// bail out — preventing resurrection of a completed turn. Without this,
// the generation guard passes and only the !IsProcessing check saves us.
// Skip if orphaned (long.MaxValue) — incrementing would overflow to long.MinValue.
if (Interlocked.Read(ref state.ProcessingGeneration) != long.MaxValue)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 MINOR — TOCTOU window in read-then-increment (Flagged by: 2/3 reviewers)

Theoretical race between Interlocked.Read and Interlocked.Increment: a background thread could Exchange(..., long.MaxValue) between them. Some ClearProcessingState callers (SendPromptAsync catch blocks, AbortSessionAsync) may run on thread-pool threads, making the window wider than "UI thread only" implies.

Practically low-risk. A CompareExchange loop would close it definitively. Not blocking — acceptable as follow-up.

Interlocked.Increment(ref state.ProcessingGeneration);

state.Info.IsProcessing = false;
state.Info.IsResumed = false;
state.Info.ProcessingStartedAt = null;
Expand Down Expand Up @@ -4475,6 +4483,8 @@ await InvokeOnUIAsync(() =>
state.IsReconnectedSend = false; // INV-1 item 8: prevent stale 35s timeout on next watchdog start
Interlocked.Exchange(ref state.SendingFlag, 0);
// Clear IsProcessing BEFORE completing TCS (INV-O3)
// No MaxValue guard needed: STEER-ERROR only reaches non-orphaned sessions
Interlocked.Increment(ref state.ProcessingGeneration); // Invalidate stale callbacks
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MODERATE — Missing long.MaxValue overflow guard (Flagged by: 3/3 reviewers)

This bare Interlocked.Increment lacks the long.MaxValue sentinel check that ClearProcessingState (line 794) and ClearProcessingStateForRecoveryFailure (line 3120) both have. If a concurrent reconnect orphans this session (Exchange(..., long.MaxValue)) while the STEER-ERROR catch is in-flight, the increment wraps to long.MinValue, breaking the monotonicity invariant for all future generation comparisons.

Suggested fix:

if (Interlocked.Read(ref state.ProcessingGeneration) != long.MaxValue)
    Interlocked.Increment(ref state.ProcessingGeneration); // Invalidate stale callbacks

state.Info.IsProcessing = false;
if (state.Info.ProcessingStartedAt is { } steerStarted)
state.Info.TotalApiTimeSeconds += (DateTime.UtcNow - steerStarted).TotalSeconds;
Expand Down Expand Up @@ -4754,6 +4764,8 @@ public async Task ReloadMcpServersAsync(string sessionName)
await InvokeOnUIAsync(() =>
{
FlushCurrentResponse(state);
// No MaxValue guard needed: MCP-reload only reaches non-orphaned sessions
Interlocked.Increment(ref state.ProcessingGeneration); // Invalidate stale callbacks
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MODERATE — Missing long.MaxValue overflow guard (Flagged by: 3/3 reviewers)

Same issue as the STEER-ERROR path. This unconditional Interlocked.Increment can wrap to long.MinValue if a concurrent reconnect has already set the sentinel. The session is orphaned at line 4789, after this block runs — so a concurrent reconnect at lines 3966/4000/4011 can set long.MaxValue between the IsProcessing check (line 4760) and this increment.

Suggested fix:

if (Interlocked.Read(ref state.ProcessingGeneration) != long.MaxValue)
    Interlocked.Increment(ref state.ProcessingGeneration); // Invalidate stale callbacks

state.Info.IsProcessing = false;
state.Info.IsResumed = false;
state.HasUsedToolsThisTurn = false;
Expand Down