fix(tui-gateway): WS disconnect/reconnect TOCTOU fix + real regression test (RAH-05 + RAH-06) - #77212
Closed
JoaoMarcos44 wants to merge 2 commits into
Closed
Conversation
…d session.resume reattach _close_sessions_for_transport() snapshotted sessions owned by the disconnecting transport under _sessions_lock, released the lock, then mutated each session (close or repoint to the detached sentinel) without re-checking ownership. session.resume's warm-reuse path (_reuse_live_payload -> _live_session_payload) rebinds session["transport"] under _session_resume_lock independently, so a reconnect landing in that window got silently undone: the old transport's teardown either force-closed the just-reattached session or stomped its new transport back to _detached_ws_transport, making a live reconnect look orphaned and eligible for grace-reap. Revalidate transport ownership under the same resume_lock -> sessions_lock ordering already used by the orphan-reap timer, immediately before claiming (close) or repointing (detach) each session. Slow teardown work still runs after releasing both locks. Fixes #HPA-01
…H-05) The existing regression test for the WS disconnect/reconnect TOCTOU fix started both sessions already on new_transport, so they never entered owned_sids (filtered by old_transport) and the revalidation-under-lock logic the fix added was never exercised. The test passed identically before and after the fix and proved nothing about it. Rewrite it to start the session on old_transport (so the snapshot captures it) and inject the reattach strictly between the snapshot and the per-sid claim under _session_resume_lock, via a thin wrapper around the real lock that performs the reattach on first acquire — modeling session.resume winning the lock race before teardown's revalidation runs. Confirmed this fails against the pre-fix implementation (reaped == 1, not 0) and passes against the fix in this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 3, 2026
Collaborator
4 tasks
Contributor
Author
|
Duplicate root cause with #77129 (opened independently for the same TOCTOU bug, tracked as #77127 there). Consolidated: pushed this PR's real regression-race test on top of #77129's branch (fix/ws-disconnect-reconnect-transport-race), replacing its shallow version. #77129 now carries the fix + the real interleaving test, and closes #77127, #77191, and #77192. Closing this one in favor of #77129. |
This was referenced Aug 3, 2026
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
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.
Summary
Closes #77192 (RAH-06), closes #77191 (RAH-05).
Combines the WS disconnect/reconnect TOCTOU fix (RAH-06) with its regression test fix (RAH-05) into one PR — RAH-05's test exercises exactly the revalidation logic RAH-06 introduces, so splitting them into two PRs meant the second one couldn't pass CI on its own without the first merged first. Together they tell one coherent story: a real race, a real fix, and a test that actually proves the fix works.
RAH-06 — WS disconnect/reconnect TOCTOU race
While validating a prior investigation's claim that this race was already fixed, found that the fix commit existed only on branch
fix/ws-disconnect-reconnect-transport-race— never merged intomain:main's_close_sessions_for_transport()had the original bug: a session snapshot taken under_sessions_lock, released, then each session mutated (closed or re-pointed to the detached sentinel) without re-validating transport ownership — racing againstsession.resume's warm-reuse rebind ofsession["transport"]under_session_resume_lock. A reconnect landing in that window could be silently undone: the disconnecting transport's teardown either force-closed the just-reattached session, or stomped its new transport back to the detached sentinel, making a live reconnect look orphaned and eligible for grace-reap.Fix: cherry-picked the existing, already-authored fix (
13241a6a3) — applies cleanly onto identical surrounding code. Revalidates transport ownership under the same_session_resume_lock→_sessions_lockordering already used by the orphan-reap timer, immediately before claiming (close) or repointing (detach) each session. Slow teardown work still runs after releasing both locks.RAH-05 — the existing regression test didn't exercise the race
The pre-existing regression test for this exact fix
(
test_close_sessions_for_transport_skips_session_reattached_mid_teardown)started both test sessions already pointing at
new_transport. Since_close_sessions_for_transport()filtersowned_sidsby the oldtransport, those sessions never entered the snapshot in the first place
— the revalidation-under-lock logic RAH-06 adds was never reached. The
test passed identically before and after RAH-06's fix and proved
nothing about it.
Fix: rewrote the test to start the session on
old_transport(sothe snapshot captures it) and inject the reattach — via a thin wrapper
around the real
_session_resume_lock— strictly between the snapshotand the per-sid claim, matching the actual race window.
%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#00f0ff', 'mainBkg': '#0a0a16', 'primaryTextColor': '#ffffff', 'primaryBorderColor': '#ff007f', 'lineColor': '#00f0ff'}}}%% graph TD A[🔌 WS disconnect] -->|snapshot owned sids| B[⚡ _sessions_lock] C[🔁 session.resume reattach] -->|rebind transport| D[⚡ _session_resume_lock] B --> D D -->|revalidate ownership per sid| E{Still old transport?} E -->|yes| F[🚀 Claim: close or detach] E -->|no: reattached| G[✅ Skip — live reconnect preserved] H[🧪 Old test: sessions start on new_transport] -->|never in owned_sids| I[🚫 Revalidation branch never runs] J[🧪 New test: session starts on old_transport] -->|_RaceLock reattaches mid-claim| DInfographic :
Test plan
tests/test_tui_gateway_server.py -k close_sessions_for_transport— 2 passedreaped == 1, not0)python -m py_compile tui_gateway/server.py tests/test_tui_gateway_server.pySupersedes #77205 and #77206 (closed — split into separate PRs originally, recombined here since one depended on the other).