Scope a terminal outcome's send-gate invalidation to its own attempt

[realtonyyoung]

Closes #698 — AI-2355

What & why

TerminalTabViewModel holds two attempt identities and takes them at different points in a reattach: BeginAttempt() claims the opening token before await prevClient.DisposeAsync() (it must, so an in-flight composer delivery stops writing to the client about to die), and the generation bump lands after. In that window the retired attempt is still current by generation while the replacement already owns the token — and Publish invalidated the token on every terminal state, so the retired attempt's own outcome retired its replacement. Symptoms: the reattach aborts at its post-dispose token check (no second client at all), or its Connecting and later Attached are discarded on a stale token and the tab renders Exited over a live, attached client with the send gate shut. Not a fake-only path — AgentAttachClient.RunAsync returns the claimed cause, so a daemon exit frame that beats the reattach's cancel comes back as Exited(code), not an OperationCanceledException.

The fix scopes that invalidation to the attempt that owns it, as a single compare-and-advance. Review then surfaced three more instances of the same shape — a check on one thread and the write that satisfies it on another — so the gate and the in-flight flag are now derived from token ownership rather than flags anyone may set, and send acceptance claims its slot atomically. An advance now settles all of it, with no path left needing to remember to close anything.

Where to look

TerminalSendGateTests asserted the defect in this exact scenario (Created.Count == 1, "the drained outcome retired the attempt"). Its sibling covering the case that must abort a reattach — an external invalidation, an agent removal, which publishes with a null owner and so still invalidates unconditionally — is unchanged and green. The two now distinguish cases they were conflating.

Verification

New An_outcome_landing_inside_the_reattach_dispose_window_leaves_the_reattach_alone parks the reattach on its dispose via DisposeGate, so the ordering is deterministic rather than raced. On the unpatched view model it fails Expected to be 2 but found 1.

The derived gate and derived in-flight flag were mutation-tested rather than assumed: breaking each fails existing tests (2 and 1 respectively), so both are load-bearing.

Capacitor.App.Tests.Unit      1266/1266 passed
--treenode-filter Terminal*      45/45 passed

Race stress on macOS arm64: 100+ consecutive clean runs of the Terminal set. A ~1-in-30 SIGSEGV of the Avalonia headless host reproduces identically on origin/main, so it is pre-existing and unrelated.

The CAS and derived-ownership windows are single interleavings across two threads with no seam to park between them; they are closed by construction, not by coverage, and no test claims otherwise.

Known follow-up, pre-existing and untouched here: the reattach path awaits prevClient.DisposeAsync() unbounded, so a hanging dispose leaves the last terminal state up until it completes. This branch strictly improves that case (the reattach now survives it) but does not bound it.

[linear-code]

AI-2355

[qodo-code-review]

PR Summary by Qodo

Scope terminal send-gate invalidation to the owning attach attempt

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Preserve replacement attaches when retired terminal outcomes arrive during client disposal.
• Carry attempt tokens through outcome publishing to scope send-gate invalidation.
• Add deterministic and stressed coverage for cancel/dispose race orderings.

Diagram

graph TD
  R["Reattach"] --> C["Claim token"] --> D["Dispose client"] --> N["Replacement attach"]
  O["Old outcome"] --> P["Publish terminal"] --> Q{"Owner current?"}
  Q -->|Yes| I["Invalidate gate"]
  Q -->|No| N
  C -. "New owner" .-> Q

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Claim replacement ownership after disposal

• ➕ Old outcomes would finish before the replacement token exists.
• ➕ Terminal publishing could retain unconditional invalidation semantics.
• ➖ The send gate must still close before disposal to reject input during reattach.
• ➖ A separate closure mechanism would add state and create new external-invalidation races.

2. Track explicit attempt objects

• ➕ Makes lifecycle and send-gate ownership share one strongly identified object.
• ➕ Could consolidate generation and token comparisons over time.
• ➖ Requires a broader lifecycle refactor for a narrowly scoped race.
• ➖ Touches more callbacks and teardown paths, increasing regression risk.

Recommendation: Keep the captured-token approach. It minimally extends the existing send-gate ownership model, preserves unconditional invalidation for removal and resolve verdicts, and complements the generation guard: token ownership covers outcomes before retirement increments the generation, while generation checks suppress outcomes afterward.

Files changed (3) +67 / -18

Bug fix (1) +22 / -11

TerminalTabViewModel.csScope terminal invalidation to captured attempt ownership +22/-11

Scope terminal invalidation to captured attempt ownership

• Carries each attempt's opening token through run completion and terminal outcome publishing. Terminal states still render, but only invalidate the send gate when their attempt remains the current owner; ownerless removal and resolve verdicts remain unconditional.

src/Capacitor.App/ViewModels/TerminalTabViewModel.cs

Tests (2) +45 / -7

TerminalSendGateTests.csExpect reattach to survive a drained retired outcome +4/-1

Expect reattach to survive a drained retired outcome

• Updates the disposal-window send-gate scenario to assert that the replacement client is created and remains Connecting after the old attempt's outcome lands.

test/Capacitor.App.Tests.Unit/TerminalSendGateTests.cs

TerminalTabViewModelTests.csCover terminal outcome and reattach disposal races +41/-6

Cover terminal outcome and reattach disposal races

• Clarifies the complementary generation and token guards, broadens scheduler ordering coverage, and adds a deterministic regression test that parks reattach inside old-client disposal.

test/Capacitor.App.Tests.Unit/TerminalTabViewModelTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Doc comment narrates prior behavior ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The new test documentation explains the failure mode of an unconditional invalidation and locates
itself relative to another test, rather than concisely stating the enduring behavior being pinned.
This embeds implementation-history context that will become stale as the implementation changes.

Code

test/Capacitor.App.Tests.Unit/TerminalTabViewModelTests.cs[R424-427]

+    /// The deterministic half of Cancel_dispose_orderings above: the retired attempt's own
+    /// outcome rendering while the reattach that retired it is parked on its dispose. Reattach
+    /// takes its opening token BEFORE that dispose, so if the outcome invalidates unconditionally
+    /// it retires the very attempt that is mid-flight -- either aborting it at the post-dispose

Evidence

Rule 28 requires test documentation to state the enduring behavior being pinned and prohibits
descriptions of prior implementations. The added doc comment calls itself the deterministic half
of a test above and explains what happens if the outcome invalidates unconditionally,
documenting source-relative and prior-implementation context instead of only the contract.

CLAUDE.md: Test Documentation Comments Must State the Behavior Being Pinned
test/Capacitor.App.Tests.Unit/TerminalTabViewModelTests.cs[424-429]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Rewrite the test documentation so it states the enduring race behavior being pinned, without referring to the test `above`, the prior unconditional-invalidation behavior, or implementation-specific failure paths.

## Issue Context
PR Compliance ID 28 requires test documentation comments to describe the pinned behavior and necessary preconditions rather than prior implementations or review-history context.

## Fix Focus Areas
- test/Capacitor.App.Tests.Unit/TerminalTabViewModelTests.cs[424-429]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Gate ownership check races ✓ Resolved 🐞 Bug ≡ Correctness

Description

Publish checks that the terminal outcome still owns _openingToken and then calls Invalidate,
which increments the token separately; if BeginAttempt claims the token between those operations,
the old outcome increments past the replacement token and aborts the replacement at its post-dispose
check. This recreates the dispose-window failure the PR is intended to fix under a supported
concurrent ReattachCommand.Execute ordering.

Code

src/Capacitor.App/ViewModels/TerminalTabViewModel.cs[177]

+        if (ownerToken is null || ownerToken == Volatile.Read(ref _openingToken)) Invalidate();

Evidence

BeginAttempt atomically increments _openingToken before awaiting old-client disposal, while the
new line first performs a volatile equality read and only afterward invokes another independent
increment in Invalidate. The PR's own deterministic race test executes reattach from Task.Run,
so BeginAttempt can run concurrently with the UI-dispatched terminal publish; an interleaving of
read-old-token, BeginAttempt, then Invalidate advances beyond the replacement's captured token,
which is rejected by the explicit check after disposal.

src/Capacitor.App/ViewModels/TerminalTabViewModel.cs[137-153]
src/Capacitor.App/ViewModels/TerminalTabViewModel.cs[166-178]
src/Capacitor.App/ViewModels/TerminalTabViewModel.cs[382-405]
test/Capacitor.App.Tests.Unit/TerminalTabViewModelTests.cs[424-450]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The terminal owner-token comparison and token increment are separate operations, allowing `BeginAttempt` to claim the gate between them and then be retired by the prior attempt's outcome.

## Issue Context
Concurrent command execution is exercised by the tests, and `TryStartAttemptAsync` calls `BeginAttempt` before awaiting disposal. Make the conditional invalidation a single atomic compare-and-advance operation; preserve unconditional invalidation for `null` owners and update gate/send projections consistently for both CAS outcomes.

## Fix Focus Areas
- src/Capacitor.App/ViewModels/TerminalTabViewModel.cs[137-178]
- test/Capacitor.App.Tests.Unit/TerminalTabViewModelTests.cs[424-452]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

Review mode: ⚖️ Balanced: This changes terminal-attempt lifecycle and send-gate invalidation across production code with race-sensitive concurrency behavior; it is materially risky but localized enough for one careful review pass.

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[realtonyyoung]

Independent review (codex, 4 rounds)

Ran an adversarial review over four rounds, asked specifically to construct an interleaving that defeats each guard. Summary of what it found and what happened.

Confirmed sound, no findings: the owner-scoped invalidation itself ("I could not construct an interleaving that defeats InvalidateOwner"), the decision to still publish a terminal state on a stale token (explicit detach invalidates before the Detached it asked for arrives, so suppressing would leave the tab reading Attached forever), the ownerToken + 1 vs Interlocked.Increment overflow equivalence, comment/convention compliance, and the judgement not to write tests for the instruction-level windows.

Fixed in response — all four are pre-existing races of the same shape, a check on one thread and the write that satisfies it on another:

#	Severity	Issue	Commit
1	high	TrySendText read _client before the token, pairing a retired client with its replacement's token — which DeliverAsync then reads as current and follows with the submit CR	f6c4f78
2	medium	An acceptance racing a claim could set an in-flight flag only its own delivery would clear, and a delivery declines to clear one it no longer owns — wedging the composer for the tab's whole life	079c311
3	medium	Generation compared by plain read while writers use Interlocked; five sites now route through Retired()	f6c4f78
4	low	A completion racing TeardownAsync's generation bump could still assign State; Retired() covers ResolveDisposed too	f6c4f78

Plus the send-gate itself (ae43450) and the atomic send claim (8a8c313).

Accepted as won't-fix, with the reviewer agreeing: the unbounded prevClient.DisposeAsync() await on the reattach path. Pre-existing, untouched, and this branch strictly improves the hang case rather than worsening it — before, the same hang left the same stale state up and aborted the reattach. Bounding it is a new timeout policy with its own behaviour and tests.

Left open deliberately: the paste in TrySendText is handed to the client before any post-claim token re-verification, so a reattach landing in that window sends it to a client being retired. This is the existing documented design — DeliverAsync's own comment notes "the paste already went to a client" and guards only the trailing CR, which is the half that submits. Closing it needs real mutual exclusion between send acceptance and attempt swap; each narrowing just moves the window rather than removing it.