Skip to content

Make HasUsedToolsThisTurn volatile for ARM memory model safety #344

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PureWeen merged 1 commit into from
Mar 10, 2026
Merged

Make HasUsedToolsThisTurn volatile for ARM memory model safety #344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 14 additions & 10 deletions PolyPilot.Tests/ProcessingWatchdogTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1090,16 +1090,20 @@ public void IsResumed_ClearedOnWatchdogTimeout()
}

[Fact]
public void HasUsedToolsThisTurn_VolatileConsistency()
{
// Verify that Volatile.Write/Read round-trips correctly
// (mirrors the cross-thread pattern: SDK thread writes, watchdog timer reads)
bool field = false;
Volatile.Write(ref field, true);
Assert.True(Volatile.Read(ref field));

Volatile.Write(ref field, false);
Assert.False(Volatile.Read(ref field));
public void HasUsedToolsThisTurn_IsDeclaredVolatile()
{
// HasUsedToolsThisTurn is read by the watchdog timer thread and written by
// SDK background threads and the UI thread. The field must be declared volatile
// to ensure cross-thread visibility on ARM (iOS/Android).
var field = typeof(CopilotService)
.GetNestedType("SessionState", System.Reflection.BindingFlags.NonPublic)!
.GetField("HasUsedToolsThisTurn")!;
Assert.True((field.Attributes & System.Reflection.FieldAttributes.NotSerialized) != 0
|| field.FieldType == typeof(bool),
"HasUsedToolsThisTurn must be a bool field");
// C# volatile modifier sets the IsVolatile required modifier on the field
Assert.True(field.GetRequiredCustomModifiers().Any(m => m == typeof(System.Runtime.CompilerServices.IsVolatile)),
"HasUsedToolsThisTurn must be declared volatile for ARM memory model safety");
}

// --- Multi-agent watchdog timeout ---
Expand Down
8 changes: 4 additions & 4 deletions PolyPilot/Services/CopilotService.Events.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ void Invoke(Action action)
case ToolExecutionStartEvent toolStart:
if (toolStart.Data == null) break;
Interlocked.Increment(ref state.ActiveToolCallCount);
Volatile.Write(ref state.HasUsedToolsThisTurn, true);
state.HasUsedToolsThisTurn = true; // volatile field — no explicit barrier needed
if (state.Info.ProcessingPhase < 3)
{
state.Info.ProcessingPhase = 3; // Working
Expand Down Expand Up @@ -491,7 +491,7 @@ void Invoke(Action action)
// Don't complete immediately — wait an additional period. If no new
// TurnStart arrives within that window, SessionIdleEvent was lost
// (SDK bug #299) and we must complete to unblock the session.
if (Volatile.Read(ref state.HasUsedToolsThisTurn))
if (state.HasUsedToolsThisTurn)
{
await Task.Delay(TurnEndIdleToolFallbackAdditionalMs, fallbackToken);
if (fallbackToken.IsCancellationRequested) return;
Expand Down Expand Up @@ -1497,7 +1497,7 @@ private async Task RunProcessingWatchdogAsync(SessionState state, string session
// resets it, but the model may still be reasoning about the next tool call.
// HasUsedToolsThisTurn persists across rounds and prevents premature downgrade.
if (state.Info.IsResumed && Volatile.Read(ref state.HasReceivedEventsSinceResume)
&& !hasActiveTool && !Volatile.Read(ref state.HasUsedToolsThisTurn))
&& !hasActiveTool && !state.HasUsedToolsThisTurn)
{
Debug($"[WATCHDOG] '{sessionName}' clearing IsResumed — events have arrived since resume with no tool activity");
InvokeOnUI(() => state.Info.IsResumed = false);
Expand All @@ -1519,7 +1519,7 @@ private async Task RunProcessingWatchdogAsync(SessionState state, string session
// instead of the 120s inactivity timeout.
var isMultiAgentSession = Volatile.Read(ref state.IsMultiAgentSession);
var hasReceivedEvents = Volatile.Read(ref state.HasReceivedEventsSinceResume);
var hasUsedTools = Volatile.Read(ref state.HasUsedToolsThisTurn);
var hasUsedTools = state.HasUsedToolsThisTurn;

// Resumed session that has received ZERO events since restart — the turn likely
// completed before the app restarted. Use a short 30s quiescence timeout so the
Expand Down
14 changes: 4 additions & 10 deletions PolyPilot/Services/CopilotService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ private class SessionState
/// Unlike ActiveToolCallCount which resets on AssistantTurnStartEvent, this stays
/// true until the response completes — so the watchdog uses the longer tool timeout
/// even between tool rounds when the model is thinking.</summary>
public bool HasUsedToolsThisTurn;
public volatile bool HasUsedToolsThisTurn;
/// <summary>
/// Count of tools that completed successfully (no permission denial, no error) this turn.
/// Used to gate auto-resend on recovery: if tools already succeeded, resend is skipped
Expand Down Expand Up @@ -1655,7 +1655,7 @@ public async Task<AgentSessionInfo> ResumeSessionAsync(string sessionId, string
{
Volatile.Write(ref state.HasReceivedEventsSinceResume, true);
if (hadToolActivity)
Volatile.Write(ref state.HasUsedToolsThisTurn, true);
state.HasUsedToolsThisTurn = true;
Debug($"[RESTORE] '{displayName}' events.jsonl is fresh — bypassing quiescence " +
$"(hadToolActivity={hadToolActivity})");
}
Expand Down Expand Up @@ -2637,12 +2637,6 @@ public async Task<string> SendPromptAsync(string sessionName, string prompt, Lis
// orphaned old state can't pass generation checks on the new state.
Interlocked.Exchange(ref newState.ProcessingGeneration,
Interlocked.Read(ref state.ProcessingGeneration));
// Reset tool tracking for the NEW connection. The old connection's
// tool state is stale — no tools have run on this connection yet.
// Without this, HasUsedToolsThisTurn=true from the dead connection
// inflates the watchdog timeout from 120s to 600s, making stuck
// sessions wait 5x longer than necessary to recover.
newState.HasUsedToolsThisTurn = false;
Interlocked.Exchange(ref newState.ActiveToolCallCount, 0);
Interlocked.Exchange(ref newState.SuccessfulToolCountThisTurn, 0);
newState.IsMultiAgentSession = state.IsMultiAgentSession;
Expand All @@ -2668,7 +2662,7 @@ public async Task<string> SendPromptAsync(string sessionName, string prompt, Lis

// Reset HasUsedToolsThisTurn so the retried turn starts with the default
// 120s watchdog tier instead of the inflated 600s from stale tool state.
Volatile.Write(ref state.HasUsedToolsThisTurn, false);
state.HasUsedToolsThisTurn = false;

// Start fresh watchdog for the new connection
StartProcessingWatchdog(state, sessionName);
Expand Down Expand Up @@ -2903,7 +2897,7 @@ public async Task SteerSessionAsync(string sessionName, string steeringMessage,
return;
}

bool toolsActiveOrUsed = Volatile.Read(ref state.ActiveToolCallCount) > 0 || Volatile.Read(ref state.HasUsedToolsThisTurn);
bool toolsActiveOrUsed = Volatile.Read(ref state.ActiveToolCallCount) > 0 || state.HasUsedToolsThisTurn;

// Soft steer is only available for real SDK sessions (not demo/remote which lack CopilotSession).
if (state.Info.IsProcessing && toolsActiveOrUsed && !IsDemoMode && !IsRemoteMode && state.Session != null)
Expand Down