Skip to content

Make HasUsedToolsThisTurn volatile for ARM memory model safety#344

Merged
PureWeen merged 1 commit intomainfrom
fix/hasusedtoolsthisturn-volatile-consistency
Mar 10, 2026
Merged

Make HasUsedToolsThisTurn volatile for ARM memory model safety#344
PureWeen merged 1 commit intomainfrom
fix/hasusedtoolsthisturn-volatile-consistency

Conversation

@PureWeen
Copy link
Copy Markdown
Owner

Problem

HasUsedToolsThisTurn on SessionState is read by the watchdog timer thread and SDK background threads while being written from the UI thread and SDK event handlers. It had inconsistent synchronization: 5 sites used Volatile.Read/Write while 16 used plain access.

On ARM (iOS/Android), plain writes from the UI thread may not be immediately visible to background thread reads. This is a theoretical data race that could cause the watchdog to use incorrect timeouts (120s vs 600s).

Identified during code review of PR #342 (INV-7 from the processing-state-safety skill).

Fix

  • Declare the field volatile bool — all reads/writes automatically have correct memory barriers
  • Remove the 5 now-redundant Volatile.Read/Write calls (would generate CS0420 warnings)
  • Remove a duplicate write in the reconnect path (line ~2613 was redundant with ~2639)
  • Replace the HasUsedToolsThisTurn_VolatileConsistency test with HasUsedToolsThisTurn_IsDeclaredVolatile that uses reflection to verify the volatile modifier

Testing

All 2401 tests pass. No CS0420 warnings.

The field is read by the watchdog timer thread and SDK background
threads while being written from the UI thread. Previously it had
inconsistent synchronization: 5 sites used Volatile.Read/Write
while 16 used plain access.

Fix: Declare the field volatile and remove the now-redundant
Volatile.Read/Write calls (which would generate CS0420 warnings).
Also remove a duplicate write in the reconnect path.

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

PR #344 Review — Make HasUsedToolsThisTurn volatile for ARM memory model safety

CI Status: No CI checks on this branch (all 2401 existing tests pass per PR description)
Existing reviews: None
Models: claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex (5/5 — 3 completed, 2 in-progress at synthesis time; all 3 completed: CLEAN)


✅ No consensus bugs found

All three completed models agree the PR is correct:

  • volatile bool vs Volatile.Read/Write — semantically identical (both emit acquire-fence on read, release-fence on write). The substitution is correct at all 5 sites. ✅
  • Deleted newState.HasUsedToolsThisTurn = falsenewState is a freshly allocated new SessionState { Session=..., Info=... }. The bool defaults to false, so the explicit reset was dead code. The actual behavioral reset at ~line 2662 (state.HasUsedToolsThisTurn = false before StartProcessingWatchdog) remains and is the real one. ✅
  • Reflection testGetRequiredCustomModifiers().Any(m => m == typeof(IsVolatile)) correctly verifies the C# volatile modifier (emitted as modreq(IsVolatile) in IL). ✅
  • CS0420 warnings — all ref state.HasUsedToolsThisTurn uses removed, so no warnings remain. ✅

One harmless nit (no action needed): the first assertion in the test — field.FieldType == typeof(bool) is always true, making the || (FieldAttributes.NotSerialized) arm unreachable. The important second assertion (IsVolatile check) is what matters and is correct.


✅ Approve

Clean mechanical fix. The volatile declaration is the right approach for this pattern (directional bool: SDK threads write true, UI thread writes false, watchdog reads). No bugs found across models.

@PureWeen PureWeen merged commit 8c9462f into main Mar 10, 2026
@PureWeen PureWeen deleted the fix/hasusedtoolsthisturn-volatile-consistency branch March 10, 2026 21:13
btessiau added a commit to btessiau/PolyPilot that referenced this pull request Mar 11, 2026
_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>
PureWeen pushed a commit that referenced this pull request Mar 11, 2026
## 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 #344 for
`HasUsedToolsThisTurn`)
- All existing tests pass

Closes #348

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
arisng pushed a commit to arisng/PolyPilot that referenced this pull request Apr 4, 2026
…een#344)

## Problem

`HasUsedToolsThisTurn` on `SessionState` is read by the watchdog timer
thread and SDK background threads while being written from the UI thread
and SDK event handlers. It had **inconsistent synchronization**: 5 sites
used `Volatile.Read/Write` while 16 used plain access.

On ARM (iOS/Android), plain writes from the UI thread may not be
immediately visible to background thread reads. This is a theoretical
data race that could cause the watchdog to use incorrect timeouts (120s
vs 600s).

Identified during code review of PR PureWeen#342 (INV-7 from the
processing-state-safety skill).

## Fix

- Declare the field `volatile bool` — all reads/writes automatically
have correct memory barriers
- Remove the 5 now-redundant `Volatile.Read/Write` calls (would generate
CS0420 warnings)
- Remove a duplicate write in the reconnect path (line ~2613 was
redundant with ~2639)
- Replace the `HasUsedToolsThisTurn_VolatileConsistency` test with
`HasUsedToolsThisTurn_IsDeclaredVolatile` that uses reflection to verify
the volatile modifier

## Testing

All 2401 tests pass. No CS0420 warnings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

1 participant