fix(tmux): guard PTY triple with ptmxMu to eliminate ptmx data race - #377
Draft
tstapler wants to merge 2 commits into
Draft
fix(tmux): guard PTY triple with ptmxMu to eliminate ptmx data race#377tstapler wants to merge 2 commits into
tstapler wants to merge 2 commits into
Conversation
Requirements, research, implementation plan, and ADR for fixing the confirmed -race data race on TmuxSession.ptmx/attachCmd/attachCmdWaitOnce (backlog item c42de545-ee23-420f-950b-d7635ab6ae27). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TmuxSession.ptmx/attachCmd/attachCmdWaitOnce were read/written from 10+ call sites with no synchronization, causing a confirmed -race failure when CreateSession's async controller-start goroutine (GetPTY) raced DeleteSession's cleanup goroutine (closePTYAndAttachCmd). Adds a ptmxMu deadlock.Mutex plus three helper methods (lockedPTMX, setPTYTriple, clearPTYTriple) and converts every read/write site to use them. closePTYAndAttachCmd snapshots the triple under the lock then runs blocking cleanup (Close/Kill/Wait) outside it, so ptmxMu is never held across I/O -- the accepted side effect is that concurrent closePTYAndAttachCmd callers now serialize instead of each racing Close()/Kill() independently, covered by a dedicated test. Adds a grep-based `make ptmx-field-guard` CI target (wired into `ci:`) so a future direct field access outside the three helpers fails the build, plus a new TestSessionService_CreateThenImmediateDelete_NoDataRace integration test that reproduces the original interleave directly instead of relying on TestServer_should_WriteUnchangedHookURL_...'s incidental timing. Two real, unrelated issues surfaced by the -count=10 verification sweep were filed separately rather than folded in here: a genuine data race in session/instance_state.go's hibernation code (item c23a9b30), and a recurrence of the known load-triggered flakiness pattern documented in BUG-051. A known, deliberately out-of-scope check-then-act (TOCTOU) in AttachToExisting/RestoreWithWorkDir is documented inline and tracked separately (item 0f4b1300). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q4ieanW2uf3yeeszo5uifJ
7 tasks
Contributor
✅ Registry ValidationTest Coverage: 34/186 features have
|
Contributor
Go Benchmarks (Tier 1) |
Contributor
E2E RPC Latency |
Contributor
Frontend Terminal Throughput |
Contributor
📊 Feature E2E CoverageFeature coverage report unavailable
|
Contributor
🎬 E2E Feature Demos2 shard(s) recorded feature flows for this PR. recordings shard 1 Demo preview opens directly in browser (single-file HTML). Raw WebM recordings in ZIP. Expires after 30 days. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #375, which unintentionally included 6 unrelated commits from other backlog sessions sharing the same local workspace (branched from a point ahead of
origin/mainthat hadn't been pushed there yet). This branch is cherry-picked cleanly onto currentorigin/mainwith only the 2 commits relevant to this item.Summary
go test -raceintermittently failedTestServer_should_WriteUnchangedHookURL_When_StartedOnExplicitPortwith a real data race onTmuxSession.ptmx— read fromGetPTY()(viaCreateSession's async controller-start goroutine) while concurrently written/nilled byclosePTYAndAttachCmd()(viaDeleteSession's cleanup goroutine).ptmx,attachCmd, andattachCmdWaitOnce(the "PTY triple") were read/written from 10+ call sites insession/tmux/tmux.gowith no synchronization.Fixes backlog item
c42de545-ee23-420f-950b-d7635ab6ae27.What Changed
ptmxMu deadlock.Mutex+ three helper methods (lockedPTMX,setPTYTriple,clearPTYTriple) toTmuxSession, and converted every read/write site (AttachToExisting,RestoreWithWorkDir,closePTYAndAttachCmd,GetPTY,TapEnter/TapDAndEnter/SendKeys,updateWindowSize, bothAttach()goroutines) to go through them.closePTYAndAttachCmdsnapshots the triple under the lock, then runs blocking cleanup (Close/Kill/Wait) outside it —ptmxMuis never held across I/O. Accepted side effect: concurrentclosePTYAndAttachCmdcallers now serialize instead of each racingClose()/Kill()independently (covered by a dedicated test, documented as intentional).make ptmx-field-guardCI target (wired intoci:) that fails the build if a future change reads/writes the PTY triple outside the three helpers.TestSessionService_CreateThenImmediateDelete_NoDataRace(server/server_integration_test.go), which reproduces the originalCreateSession/DeleteSessioninterleave directly instead of relying on the original test's incidental timing.session/tmux/tmux_test.gocovering the helper methods, lock ordering (detachMutexouter →ptmxMuinner, never reversed), and regression coverage confirming every converted call site's pre-fix error strings/behavior are unchanged.Two real, unrelated issues surfaced by broad
-race -count=10verification sweeps were filed separately rather than folded into this diff:session/instance_state.go's hibernation code — backlog itemc23a9b30-62f3-4739-850f-6c1ae7025bd3.docs/bugs/fixed/BUG-051-session-tmux-package-flaky-under-parallel-quick-check.md(logged there, not re-filed).A known, deliberately out-of-scope check-then-act (TOCTOU) race in
AttachToExisting/RestoreWithWorkDir(distinct from the data race this PR fixes — it's a logic racego test -racecan't detect, requiring a larger behavioral change to close) is documented inline and tracked separately as backlog item0f4b1300-d667-437b-b51f-89d81a668693.Test plan
make ptmx-field-guard— passes on the fixed tree; verified it catches a planted violation (botht.-receiver and non-t-receiver forms) before revertinggo test -race ./session/tmux/...— passes (no failures, no races), re-verified on this rebased branchgo test -race ./session/... ./server/... -count=10— no data race reported onptmx/attachCmd/attachCmdWaitOncego test -race ./server/... -run TestServer_should_WriteUnchangedHookURL_When_StartedOnExplicitPort -count=20— 20/20 passgo test -race ./server/... -run TestSessionService_CreateThenImmediateDelete_NoDataRace -count=20— 20/20 passmake quick-check— build + test + lint all passsdd:6-verifypass (idiom, architecture, refactor-candidates review) — all findings resolved, zero BLOCKERs🤖 Generated with Claude Code