From 145a0d9d803530831d9d18a4534c05d35c8c62e6 Mon Sep 17 00:00:00 2001 From: Baptiste Tessiau Date: Wed, 11 Mar 2026 09:01:57 +0100 Subject: [PATCH] fix: make _activeSessionName volatile for cross-thread visibility _activeSessionName is read by WsBridge background threads (SyncRemoteSessions), restore background threads, and SDK event handlers, while being written by the UI thread (SetActiveSession, CloseSession, RenameSession). Without volatile, writes may not be visible to other threads on ARM (iOS/Android) due to CPU memory reordering. Changes: - CopilotService: private string? -> private volatile string? - DemoService: same change for consistency - VolatileFieldGuardTests: 2 reflection-based regression tests verifying the volatile modifier is present (same pattern as PR #344) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/VolatileFieldGuardTests.cs | 37 ++++++++++++++++++++++ PolyPilot/Services/CopilotService.cs | 2 +- PolyPilot/Services/DemoService.cs | 2 +- 3 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 PolyPilot.Tests/VolatileFieldGuardTests.cs diff --git a/PolyPilot.Tests/VolatileFieldGuardTests.cs b/PolyPilot.Tests/VolatileFieldGuardTests.cs new file mode 100644 index 0000000000..9593de28db --- /dev/null +++ b/PolyPilot.Tests/VolatileFieldGuardTests.cs @@ -0,0 +1,37 @@ +using System.Reflection; +using System.Runtime.CompilerServices; +using PolyPilot.Services; + +namespace PolyPilot.Tests; + +/// +/// Guards that fields accessed from multiple threads are declared volatile. +/// Prevents regressions where someone removes the volatile modifier. +/// +public class VolatileFieldGuardTests +{ + [Fact] + public void ActiveSessionName_IsDeclaredVolatile_OnCopilotService() + { + // _activeSessionName is read by WsBridge background threads (SyncRemoteSessions), + // restore background threads, and written by UI thread (SetActiveSession, CloseSession). + // Must be volatile for cross-thread visibility on ARM (iOS/Android). + var field = typeof(CopilotService) + .GetField("_activeSessionName", BindingFlags.NonPublic | BindingFlags.Instance)!; + Assert.NotNull(field); + Assert.True( + field.GetRequiredCustomModifiers().Any(m => m == typeof(IsVolatile)), + "_activeSessionName must be declared volatile for cross-thread visibility"); + } + + [Fact] + public void ActiveSessionName_IsDeclaredVolatile_OnDemoService() + { + var field = typeof(DemoService) + .GetField("_activeSessionName", BindingFlags.NonPublic | BindingFlags.Instance)!; + Assert.NotNull(field); + Assert.True( + field.GetRequiredCustomModifiers().Any(m => m == typeof(IsVolatile)), + "DemoService._activeSessionName must be declared volatile for consistency"); + } +} diff --git a/PolyPilot/Services/CopilotService.cs b/PolyPilot/Services/CopilotService.cs index 9bdb631d78..ffaf8fc8b2 100644 --- a/PolyPilot/Services/CopilotService.cs +++ b/PolyPilot/Services/CopilotService.cs @@ -51,7 +51,7 @@ public partial class CopilotService : IAsyncDisposable // Cached dotfiles status — checked once when first SetupRequired state is encountered private CodespaceService.DotfilesStatus? _dotfilesStatus; private ConnectionSettings? _currentSettings; - private string? _activeSessionName; + private volatile string? _activeSessionName; private SynchronizationContext? _syncContext; // Serializes the IsConnectionError reconnect path so concurrent workers // don't destroy each other's freshly-created client (thundering herd fix). diff --git a/PolyPilot/Services/DemoService.cs b/PolyPilot/Services/DemoService.cs index 2e4fa81d33..7958f5b73e 100644 --- a/PolyPilot/Services/DemoService.cs +++ b/PolyPilot/Services/DemoService.cs @@ -10,7 +10,7 @@ namespace PolyPilot.Services; public class DemoService : IDemoService { private readonly ConcurrentDictionary _sessions = new(); - private string? _activeSessionName; + private volatile string? _activeSessionName; private int _sessionCounter; public event Action? OnStateChanged;