Skip to content

fix: make _activeSessionName volatile for cross-thread visibility#349

Merged
PureWeen merged 1 commit intoPureWeen:mainfrom
btessiau:fix/active-session-name-thread-safety
Mar 11, 2026
Merged

fix: make _activeSessionName volatile for cross-thread visibility#349
PureWeen merged 1 commit intoPureWeen:mainfrom
btessiau:fix/active-session-name-thread-safety

Conversation

@btessiau
Copy link
Copy Markdown
Contributor

Problem

_activeSessionName is a plain string? accessed from multiple threads without memory barriers:

Thread Operation Location
UI thread Write (SetActiveSession, CloseSession, RenameSession) CopilotService.cs
WsBridge background Read + conditional write (SyncRemoteSessions) Bridge.cs:532
Restore background Conditional write (??=) Persistence.cs:380
SDK event handler Read comparison Events.cs:876
SaveActiveSessionsToDisk Read Persistence.cs:457

On ARM (iOS/Android), without volatile, writes may sit in a CPU store buffer and never become visible to reads on other cores — causing stale active session name in bridge sync, persistence, and event handling.

Fix

- private string? _activeSessionName;
+ private volatile string? _activeSessionName;

Applied to both CopilotService and DemoService.

All compound patterns (??=, compare-then-swap in Close/Rename) are either on the UI thread or benign (first-write-wins on restore/bridge sync).

Testing

Closes #348

_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 PureWeen#344)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

@btessiau btessiau left a comment

Choose a reason for hiding this comment

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

🔍 PR #349 Review -- _activeSessionName volatile for cross-thread visibility

CI Status: ⚠️ No checks configured
Prior reviews: None
Diff: 3 files, +39 -2 (minimal surgical change)


Review Summary

The change is correct, minimal, and well-justified. No blocking issues found.


Correctness Analysis

volatile string? is valid and effective here. In C#, volatile on a reference type provides acquire/release memory barrier semantics — reads are acquire reads, writes are release writes. This prevents CPU store-buffer reordering on ARM (iOS/Android) where writes from the UI thread may not be visible to background threads without a barrier.

Compound patterns are safe:

  • ??= (CopilotService.cs:1536, Persistence.cs:374, DemoService.cs:37) — These are first-write-wins races. If both UI thread and restore/bridge thread race to set the active session name from null, the last writer wins, but both values are valid session names (not corrupted data). volatile reduces the race window significantly by ensuring readers see fresh writes. This is the correct analysis in the PR description.

  • if (_activeSessionName == null && ...) _activeSessionName = remoteActive; (Bridge.cs:532) — Runs on the WsBridge background thread, once on first sync. With volatile, the null check correctly sees any UI-thread write, making the race window negligible. If it fires anyway, first-write-wins is acceptable.

  • if (_activeSessionName == oldName) _activeSessionName = newName; (CopilotService.cs:2975, 3015, 3118) — These run on the UI thread exclusively (CloseSession, RenameSession). The background thread never mutates _activeSessionName in the rename/close paths. volatile ensures background readers see the rename/close immediately.

  • Events.cs:797 if (state.Info.Name == _activeSessionName) — Read-only use for marking messages as read. A stale read here simply means "read" badge lags one cycle. Benign.

No deadlock risk. volatile uses no locks.


Test Quality

VolatileFieldGuardTests uses field.GetRequiredCustomModifiers().Any(m => m == typeof(IsVolatile)) — this is the correct and idiomatic way to verify volatile via reflection in .NET. The tests serve as a reliable regression guard against accidental removal of the modifier. Both CopilotService and DemoService are compiled into the test project via <Compile Include> in the csproj, so typeof(CopilotService) resolves correctly.


Minor Observation (not blocking)

IsRestoring is a property { get; private set; } read by the SaveActiveSessionsToDisk timer callback (Persistence.cs:15, 54) on a background thread without a memory barrier. This is a pre-existing issue outside this PR's scope — the consequence is benign (at worst the timer fires once during restore and writes a partial snapshot, which is overwritten when restore completes). Worth a follow-up but not a blocker here.


Verdict: ✅ Approve

Three-line change, correct analysis, reflection-based regression tests. Ready to merge.

@btessiau btessiau marked this pull request as ready for review March 11, 2026 08:08
@PureWeen PureWeen merged commit 5d54b70 into PureWeen:main Mar 11, 2026
arisng pushed a commit to arisng/PolyPilot that referenced this pull request Apr 4, 2026
…reWeen#349)

## Problem

`_activeSessionName` is a plain `string?` accessed from multiple threads
without memory barriers:

| Thread | Operation | Location |
|--------|-----------|----------|
| UI thread | Write (SetActiveSession, CloseSession, RenameSession) |
CopilotService.cs |
| WsBridge background | Read + conditional write (SyncRemoteSessions) |
Bridge.cs:532 |
| Restore background | Conditional write (`??=`) | Persistence.cs:380 |
| SDK event handler | Read comparison | Events.cs:876 |
| SaveActiveSessionsToDisk | Read | Persistence.cs:457 |

On ARM (iOS/Android), without `volatile`, writes may sit in a CPU store
buffer and never become visible to reads on other cores — causing stale
active session name in bridge sync, persistence, and event handling.

## Fix

```diff
- private string? _activeSessionName;
+ private volatile string? _activeSessionName;
```

Applied to both `CopilotService` and `DemoService`.

All compound patterns (`??=`, compare-then-swap in Close/Rename) are
either on the UI thread or benign (first-write-wins on restore/bridge
sync).

## Testing

- 2 new reflection-based guard tests in `VolatileFieldGuardTests.cs`
verify the `volatile` modifier is present (same pattern as PR PureWeen#344 for
`HasUsedToolsThisTurn`)
- All existing tests pass

Closes PureWeen#348

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

_activeSessionName lacks volatile — stale reads on ARM from bridge/restore threads

2 participants