Skip to content

fix(tui-gateway): serialize WS session transport ownership - #95709

Open
JoaoMarcos44 wants to merge 6 commits into
NousResearch:mainfrom
JoaoMarcos44:fix/ws-session-ownership-77127
Open

fix(tui-gateway): serialize WS session transport ownership#95709
JoaoMarcos44 wants to merge 6 commits into
NousResearch:mainfrom
JoaoMarcos44:fix/ws-session-ownership-77127

Conversation

@JoaoMarcos44

@JoaoMarcos44 JoaoMarcos44 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #77127.

This change closes the WebSocket disconnect/session resume ownership race without changing the orphan-reap grace policy or adding a second session implementation.

Validation of the latest issue report

The newest issue comment confirms the user-visible failure on macOS: a WebSocket drop can terminate an in-flight turn while the backend remains alive. The comment also describes the grace window expiring while a laptop is asleep. That sleep/awake-time mechanism is a separate concern tracked by #44183/#44190; this PR addresses the narrower race named by #77127: a reconnect that reaches session.resume while the old disconnect teardown is still acting on a stale snapshot.

The TOCTOU is confirmed on current main by a deterministic snapshot → reattach → claim test. The vulnerable implementation closes or parks the session even after its transport has moved to a live replacement.

Root cause

tui_gateway.ws.handle_ws() closes a WSTransport and offloads _close_sessions_for_transport() to the RPC worker pool. The disconnect helper snapshots sessions under _sessions_lock, releases that lock, and then processes the snapshot later.

At the same time, session.resume is also dispatched on the worker pool. Its warm-reuse path rebinds session["transport"] while holding _session_resume_lock. On the old code, the two operations did not share an ownership boundary:

  1. Disconnect snapshots session S while S["transport"] is the old socket.
  2. session.resume rebinds S to a replacement socket.
  3. Disconnect resumes from its stale snapshot.
  4. The close branch can tear down S; the detach branch can overwrite the replacement with _detached_ws_transport and schedule orphan reap.

There was a second reachable variant: a slow resume could finish after its request socket had already closed. _claim_or_reuse_live() then registered a session whose transport was a closed WSTransport. Because it was not the detached sentinel, the existing orphan reaper could not recognize it as orphaned.

The same unsafe transport assignment also existed in the unpersisted resume path, prompt.submit, the queued-prompt drain, and the shared live payload helper.

Implementation

  • Make _session_resume_lock re-entrant and use it as the common lifecycle boundary.
  • Revalidate both the exact session record and its current transport under _session_resume_lock_sessions_lock before a disconnect can claim it.
  • Claim close-on-disconnect sessions through _pop_session_by_id() so _closing is set before the record leaves _sessions.
  • Keep _teardown_popped_session() and orphan-timer scheduling outside the lifecycle locks.
  • Route live payload, unpersisted resume, prompt.submit, and queued-prompt transport changes through _bind_live_session_transport().
  • Reject a closed candidate transport; a late resume is parked on _detached_ws_transport and handed to the existing orphan-reap path.
  • Remove the old shallow regression that never entered the snapshot-to-claim window and replace it with deterministic coverage for both close branches and the late-commit path.

Non-duplicate analysis

Invariants and security posture

  • A disconnect can only close or detach a session that still points to that disconnecting transport.
  • A live reattach cannot be overwritten by a stale disconnect claim.
  • No registered session is published with a transport already marked closed.
  • A closed late-resume transport is converted to the existing detached sentinel, preserving the normal recoverable/reap behavior.
  • The canonical _closing lifecycle barrier and message/queue ordering remain intact.
  • No new endpoint, credential, privilege, persistence format, or external dependency is introduced.

Hardening record

Issue label: P1 (type/bug, comp/tui, area/sessions, sweeper:risk-session-state). Required intensity: three adversarial check-ins.

  1. Premise/root-cause pass: read the full issue thread, confirmed the stale snapshot on current main, traced handle_ws_close_sessions_for_transport and session.resume, and swept open/closed/merged related PRs.
  2. Failure-mode pass: checked both close/detach branches, exact-record replacement, lock ordering, slow teardown, timer cancellation, direct rebind callers, queued prompts, and a resume worker committing after its socket closed.
  3. Simplification/regression pass: kept one shared ownership helper, preserved teardown outside locks, avoided fan-out/grace-policy changes, removed the vacuous test, and proved the new tests fail on the old behavior.

Test plan

All commands below use the repository's canonical scripts/run_tests.sh wrapper.

  • scripts/run_tests.sh tests/test_tui_gateway_server.py tests/test_tui_gateway_queue_on_busy.py -q
    • Head ca656678edab738b14b89e2dd60b90a82deedff5: 647 passed in the two changed test files.
    • The same run reports one unrelated pre-existing failure: test_model_options_preserves_canonical_custom_row_after_agent_init. It reproduces on a clean origin/main checkout with the same assertion.
  • scripts/run_tests.sh tests/tui_gateway/ -q
    • Head: 613 passed, 4 failed.
    • The four failures are outside this diff (test_compute_host.py, test_entry_import_off_main_thread.py, test_bot_relay_methods.py, and test_compute_host_phase1.py) and reproduce with the same results on clean origin/main.
  • python scripts/check-windows-footguns.py --all
    • Clean on the repository-wide gate scope.
  • ruff on all five changed files and git diff --check
    • Clean.
  • Regression witnesses:
    • Before the fix, the deterministic close test failed with (reaped, detached) == (1, 0) instead of (0, 0).
    • Before the fix, the late-resume test registered a closed transport instead of the detached sentinel and scheduled no reap.
    • A sabotage run restoring the old live-payload assignment failed because the payload replaced a live owner with a closed transport.

Remote CI is intentionally reported by GitHub after this PR is opened; local results are not substituted for remote status.

Limitations

This validation ran on Windows 11. The deterministic tests use real imports and real lifecycle functions with isolated test state, but no authenticated production gateway or macOS sleep/wake E2E was available from this environment. The sleep-time grace behavior remains out of scope and is explicitly tracked separately by #44183/#44190.

Infographic

Session ownership after WebSocket disconnect


Review round 4 — 1b23b964fc

This round closes both blockers @andrexibiza left standing on 0efbb20a70.

Blocker 1 — the discarded ownership refusal

_bind_live_session_transport() proved registry authority and then answered with a bare bool. That collapsed two situations that are not interchangeable for the caller:

Refusal Meaning Correct continuation
TRANSPORT_DEAD the record is still authoritative, the request socket died finish already-admitted work; the response just cannot be delivered on that socket
STALE_RECORD the record lost registration authority to a concurrent teardown terminal — nothing may be queued, mutated, or started

Both mutation callers discarded that False:

  • prompt.submit resolves its session through the UNLOCKED _sess_nowait() read, so a close_on_disconnect teardown can pop that exact record before the bind. The handler then kept appending history, queueing prompts, and latching running on a record teardown already owned.
  • _drain_queued_prompt() had the same hole in the opposite order: it popped the queued envelope and set session["running"] = True before asking about ownership.

The bind now returns a typed SessionBindOutcome (BOUND / TRANSPORT_DEAD / STALE_RECORD), and registry authority is proved before transport liveness so the two reasons stay distinguishable even when both apply.

  • prompt.submit answers 4001 "session not found" on STALE_RECORD — the code the client already treats as "resume the stored session" — before touching history, the queue, or running. A dead request transport stays non-terminal: that turn is legitimate and its events reach whatever the session is bound to now.
  • _drain_queued_prompt() proves authority before the claim and, if the window closes mid-claim, restores the envelope and releases running, leaving the popped record exactly as teardown expects it.
%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#00f0ff', 'mainBkg': '#0a0a16', 'primaryTextColor': '#ffffff', 'primaryBorderColor': '#ff007f', 'lineColor': '#00f0ff'}}}%%
graph TD
    A["🔌 prompt.submit worker"] -->|"unlocked registry read"| B["📄 Session Record S"]
    T["⚡ WS Disconnect Teardown"] -->|"_pop_session_by_id sets _closing"| B
    B --> C{"🔒 Ownership Claim<br/>_session_resume_lock → _sessions_lock"}
    C -->|"authoritative + live socket"| D["✅ BOUND<br/>rebind transport, cancel orphan reap"]
    C -->|"authoritative + closed socket"| E["🔇 TRANSPORT_DEAD<br/>keep previous transport, continue turn"]
    C -->|"popped / _closing / _finalized"| F["🛑 STALE_RECORD"]
    D --> G["🚀 History + Queue + running mutation"]
    E --> G
    F --> H["🧯 Safe Abort<br/>4001 session not found · queue restored · running released"]
    H -.->|"client resumes stored session"| I["🔄 session.resume mints one authoritative runtime"]
Loading

New deterministic witnesses in tests/tui_gateway/test_session_ownership.py:

  • test_prompt_submit_refuses_a_record_teardown_claimed_mid_request — barriers a real prompt.submit between _sess_nowait() and the bind (on _load_dashboard_process_isolation_config(), the last call before the claim — no sleeps, no polling), lets a real _pop_session_by_id() claim that exact record, then proves 4001 and that running, history, queued_prompt, queued_prompts, and transport are all untouched on the popped object.
  • test_prompt_submit_refuses_a_claimed_record_with_no_request_transport — the same race when the request carries no socket at all.
  • test_prompt_submit_survives_a_dead_request_transport — proves the fix does not over-refuse: a dead socket still runs the turn.
  • test_queued_drain_refuses_a_popped_record_without_claiming_the_queue and test_queued_drain_restores_the_envelope_when_ownership_is_lost_mid_claim — the drain witnesses, before the claim and inside it.
  • Plus three outcome-typing tests, including test_stale_record_wins_over_a_dead_transport.

Regression proof: with only the prompt.submit gate reverted (module, enum, and tests intact), the two method-boundary tests fail with KeyError: 'error' — the RPC succeeded against the popped record. That is exactly the defect named in the review.

Blocker 2 — shard topology

Ownership and deferred-build fencing now live in a new focused module, tui_gateway/session_ownership.py (260 lines), instead of being added back to the decomposition-owned godfile:

  • Moved out of server.py: SessionBindOutcome, bind_live_session_transport, session_record_is_authoritative / session_is_live_for_commit, session_lifecycle_lock, run_live_build_effect, commit_agent_build, commit_resume_hydration, commit_resume_failure_state, the bounded retired-session-id registry, and the deferred_build_effect_session ContextVar.
  • server.py keeps only the import plus underscore aliases, so the handler modules' rebound-globals seam (method_ctx.py) and every existing monkeypatch target resolve unchanged.
  • Net effect on the godfile: tui_gateway/server.py −216 / +96 (≈120 net lines removed), and tests/test_tui_gateway_server.py −20 lines (the retired-session-id test moved into the focused module). This diff now shrinks both decomposition-owned surfaces instead of growing them.
  • All new regressions land in tests/tui_gateway/test_session_ownership.py (333 lines), not in tests/test_tui_gateway_server.py.

Test plan — round 4

Command Head 1b23b964fc Baseline 0efbb20a70
pytest tests/tui_gateway/ tests/test_tui_gateway_queue_on_busy.py -q 14 failed, 646 passed, 1 skipped 14 failed, 636 passed, 1 skipped
pytest tests/tui_gateway/ tests/test_tui_gateway_queue_on_busy.py tests/test_tui_gateway_server.py -q 30 failed, 1251 passed, 1 skipped 30 failed (14 + 16)
ruff check tui_gateway/ tests/ clean clean

The failure set is identical between head and baseline (verified by diffing the collected failure names). All of it is local Windows/env noise, not CI: ModuleNotFoundError: No module named 'ruamel' (16 config/YAML-save tests), AttributeError: module 'signal' has no attribute 'SIGPIPE', assert 'hermes.CMD' in ('hermes', 'hermes.exe'), a Windows tempfile-unlink case, and the test_protocol.py ordering artifacts that also fail on baseline. Head adds +10 net passing tests with zero new failures. Remote CI remains the authority on this head.

Two existing test harnesses were updated because they exercised the drain against a record that was never registered — an assumption production never satisfies, and the exact assumption this change now rejects:

  • tests/test_tui_gateway_queue_on_busy.py_session() registers into server._sessions, with an autouse fixture isolating the registry per test.
  • tests/tui_gateway/test_goal_command.py_turn_session() does the same, so a queued user prompt can actually preempt the goal continuation.

Interlocks

methods_session.py's live-lazy reattach also calls the bind and still ignores the outcome. That path is left as-is deliberately: it mutates only last_active before returning, and a STALE_RECORD there degrades to the pre-existing 4001 the client already recovers from on its next session-scoped RPC. Flagging it here rather than widening this diff; happy to fold it in if you would rather it be terminal too.

The #94697 / #86784 composition notes from the earlier review are unchanged — bind_live_session_transport() is now a single named entry point in its own module, which should make a fan-out replacement strictly easier than a helper buried in server.py.

Serialize disconnect claims and client transport rebinds through one lifecycle boundary. Quarantine deferred resumes whose request WebSocket closed before registration so they remain eligible for the existing orphan reap.

Refs NousResearch#77127
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tui Terminal UI (ui-tui/ + tui_gateway/) area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state duplicate This issue or pull request already exists labels Aug 26, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #95197: both patches claim the snapshot-owned session under the resume/session locks before teardown so a stale disconnect cannot overwrite a rebound live transport.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head ca656678edab738b14b89e2dd60b90a82deedff5 on live main@9aa7530f7b53699e2c6d648ded8f6300503b3dc7, including all five changed files, the session/disconnect ownership paths, focused regressions, current CI, #77127, the narrower #95197 candidate, merged #93430, the viewer-detach/fan-out neighbors, and the active TUI godfile decomposition graph.

The core locking repair is good work. In particular, the lock order is consistently _session_resume_lock -> _sessions_lock; disconnect now revalidates both the exact registered record and its current transport before acting; slow teardown stays outside those locks; a late resume whose request transport is already closed is parked on the existing detached sentinel; and the new close/detach tests finally enter the real snapshot -> reattach -> claim window rather than simulating a shallow assignment. That is materially stronger than the current-main #93430 salvage and the narrower #95197 shape. 🚀

I found one P1 ownership hole in the new abstraction, plus one hard repository invariant blocker.

P1 — _bind_live_session_transport() refuses stale ownership, but mutation callers ignore the refusal

The new helper correctly returns False when _sessions.get(sid) is not session or the candidate transport is dead. But every converted mutation caller treats that refusal as advisory and continues on the old session object.

The clearest path is tui_gateway/methods_prompt.py::prompt.submit:

session, err = _sess_nowait(params, rid)
...
if (t := current_transport()) is not None:
    _bind_live_session_transport(sid, session, t)  # return value discarded
while True:
    with session["history_lock"]:
        ...

_sess_nowait() itself is an unlocked _sessions.get(sid). Therefore this interleaving is still legal:

  1. a prompt worker resolves live record S;
  2. the old disconnect acquires the new lifecycle boundary and, for a close_on_disconnect session, claims/pops S and marks it closing;
  3. the prompt worker reaches _bind_live_session_transport(); the helper correctly sees that S is no longer the registered record and returns False;
  4. prompt.submit ignores that proof failure and keeps mutating/queueing/starting work against the popped S reference.

The queued-drain path has the same class in the opposite order: _drain_queued_prompt() pops a queued prompt and sets session["running"] = True before calling _bind_live_session_transport(), then ignores a False result. A stale/unregistered record can therefore still acquire new work after the ownership gate has told us it is no longer authoritative.

This is the same defect class the PR is trying to close: the exact-record proof is checked, then weakened to a boolean that the side-effect path discards. A dead request transport and a stale/unregistered session are not the same continuation case. It can be reasonable to finish already-admitted work when the request socket dies; it is not safe to start/queue a mutation on a record that lost registration authority.

Required repair: make the bind result strong enough for the caller to distinguish at least bound/current-but-request-dead/stale-record (typed result or equivalent). For prompt.submit and queued-drain mutation paths, a stale-record result must refuse/reacquire the current registered record before any history/queue/running mutation. Do not turn exact-record mismatch into a silent continue.

Please add a deterministic regression that barriers a prompt.submit after _sess_nowait() but before the bind, lets a close_on_disconnect teardown pop that exact record, then releases the prompt worker and proves: no prompt is queued/run against the popped record, no stale running mutation appears, and the RPC fails/reconciles against the authoritative current state. Add the analogous queued-drain witness if the shared repair does not make it structurally redundant.

Hard blocker — this PR adds behavior back into both active TUI godfiles

This exact diff adds/changes behavior in tui_gateway/server.py at hunks through ~line 9,695 and adds regressions to tests/test_tui_gateway_server.py through ~line 17,730. Both files have explicit live decomposition owners:

  • #78630 records tui_gateway/server.py as a godfile and the standing rule all godfiles are sharded, never restored; its shipped shard set is #79259#79263.
  • #78629 records the same rule for tests/test_tui_gateway_server.py; its shipped test-shard set is #80537#80541.
  • #78647 records the repository-wide campaign result and the continuing no-regression law.

So this is not a style nit. The submitted object grows two files that the repository has already declared decomposition-owned, and it collides with open extraction carriers touching those same paths. The local 647 passed result cannot waive the architecture gate.

Required repair: compose this behavior onto the accepted shard topology instead of adding another transport/lifecycle cluster to server.py. Route the ownership primitive and its exclusive state/helpers into the appropriate focused sub-2k module (or retarget onto the exact owning shard after a fresh FILE-LIST read), preserve server re-export/monkeypatch seams where required, and put the new regression set in a focused sub-2k TUI test module rather than extending test_tui_gateway_server.py. Re-run the current-main seam/golden/behavior gates after that composition.

Interlocks / provenance / merge order

  • #95197 / @konsisumer is not an exact duplicate of this head. It owns the narrower snapshot-to-claim repair and has green exact-head CI on 0a63e13e...; this PR adds the late-resume/dead-transport path plus a common rebind boundary. If this broader object lands, classify #95197 as superseded-by rather than silently duplicate, and preserve its contribution.
  • Merged #93430 is the current-main predecessor. It salvaged #93369/@kshitijk4poor, #92870/@pierrenode, #90466/@A2chitect, #65422/@halaprix, and #77977/@c-pompa, and explicitly carried the #77129 revalidation concept with @JoaoMarcos44 credit. This PR extends that lineage; it does not replace those contributors.
  • #94697 is complementary and load-bearing for the newest macOS/sleep report: it changes the policy after all viewers disappear so Bot-owned active work can survive zero viewers. #95709 fixes stale ownership after a reconnect; it does not by itself solve a grace window that expires while no client can reconnect. Both touch server.py / session methods and need semantic composition.
  • #86784 (porting @OmarB97's #40822 fan-out architecture) changes these same rebind sites from single-owner replacement to attach/detach fan-out; stacked #87371 adds wire origin/mirroring semantics. If fan-out lands first, _bind_live_session_transport() cannot simply overwrite the slot. If this lands first, #86784 must consume/replace this lifecycle gate rather than reintroducing a parallel ownership rule.
  • #44190 remains the separate generic sleep/wake timer semantics track, as the PR correctly states.

Preferred order: close the discarded-refusal hole, compose the change onto #78629/#78630's shard topology, then re-read #94697/#86784 at the landing edge so there is one transport/session authority model rather than three mutually plausible ones.

Exact-head hosted evidence at review time: CI 32989652755 and Docker 32989651873 are green on ca656678...; Nix 32989651995 is still in progress. I am not inheriting #95197's green Nix receipt onto this object.

The concurrency analysis and deterministic race tests here are strong. The remaining work is concentrated at the exact place the new abstraction says "this record no longer owns the mutation" and then lets callers continue anyway, plus the repository's already-established decomposition boundary. Fix those two and this becomes a much cleaner canonical owner for #77127.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Exact-head evidence update after the review above: Nix 32989651995 has now completed successfully on the same unchanged ca656678edab738b14b89e2dd60b90a82deedff5 head. CI 32989652755, Docker 32989651873, and Nix are therefore all green on the reviewed object. That closes the evidence-status gap; it does not change the discarded-refusal P1 or the godfile/shard-topology blocker.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Landing-edge topology update: main advanced after the review to 1a5547c5c5e3d46cfe5de621342b7acccc85f4e1. The reviewed head is unchanged and is now 1 commit ahead / 15 behind merge base 9aa7530f7b53699e2c6d648ded8f6300503b3dc7. The 15-commit main drift is path-disjoint from this PR's five TUI files (Desktop + contributor-map changes only), so it does not alter either review finding. GitHub's latest PR snapshot reports the branch non-mergeable; after the two blockers are repaired, recompute the landing merge object/restack and rerun exact-head evidence rather than carrying forward this head's green workflows.

Serialize deferred hydration and agent-build publication with runtime teardown. Discard agents that lose the runtime claim and prevent session events from falling back to stdio after the registry entry is gone.

Refs NousResearch#77127

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review of 2f1606dd6a0c833c1dd0b71ef59f7bc56a89771b against base 9aa7530f7b53699e2c6d648ded8f6300503b3dc7.

The follow-up lifecycle-lock change closes the prior post-commit agent leak. The new disconnect-after-commit regression test exercises that ownership handoff, and the exact-head Core Unit Tests and Regression Tests pass.

One correctness blocker remains:

P1 — prompt.submit still continues after transport-bind refusal.

methods_prompt.execute() still ignores the result of _bind_live_session_transport(session_id, transport). That helper returns False once the expected session is missing, replaced, closing, or closed, but the method then starts the build, drains pending_user_inputs, appends history, and can run or emit against the stale session. This is the prior blocker and is untouched by 2f1606dd.

Make bind refusal terminal before any queue/history mutation or event emission, and add a close/reconnect race test at the prompt.submit method boundary.

Blocking CI

The exact-head CI workflow for 2f1606dd6a0c833c1dd0b71ef59f7bc56a89771b is red: Ruff fails in run 33009027323. Docker Build, Nix flake check, Core Unit Tests, Regression Tests, Import Smoke Test, and CRLF Guard pass, but this commit is not merge-ready until Ruff is fixed and the exact head reruns fully green.

Status: not merge-ready on this head.

write_json's session lookup returned False outright when a sid had no
registered session, instead of falling through to the stdio path -
breaking every _block/_emit caller that writes to stdout without a
live session (tests, and any pre-registration prompt). Gate the
short-circuit on an active deferred-build context or a retired id
instead, and close orphaned agent builds that lose the reap race
rather than silently dropping the handle.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review on exact head 0efbb20a70131025ea2cd36af74791c154f20521.

The new lifecycle-lock/build-commit work closes an additional late-build/teardown class, but the previously identified prompt.submit ownership blocker is still present on this head.

tui_gateway/methods_prompt.py still does:

if (t := current_transport()) is not None:
    _bind_live_session_transport(sid, session, t)
while True:
    with session["history_lock"]:
        ...

The helper still returns False when the exact session record is no longer registered or the candidate transport is dead, and the caller still discards that result. A worker that resolved session before disconnect teardown can therefore receive an explicit stale-record refusal and then continue into history/queue/running mutation on the popped record. The same authority failure remains in _drain_queued_prompt(): it sets session["running"] = True and removes the queued prompt before calling _bind_live_session_transport(...), then ignores a false return.

This head therefore still needs the same fail-closed ownership contract: distinguish stale-record from request-transport-dead, and make stale-record refusal terminal/reconciliatory before any prompt history/queue/running mutation. The deterministic method-boundary race requested in the prior review is still missing.

The repository-structure blocker also remains mechanically present: this 7-file head still modifies tui_gateway/server.py and tests/test_tui_gateway_server.py, both existing decomposition-owned >2K surfaces. The newly added lifecycle ownership helpers/state are still being added back to server.py, rather than composed into the accepted bounded shard topology.

Exact-head hosted evidence is not fully green: Docker 33061564801 and Nix 33061564804 succeeded, while CI 33061565088 completed failure and currently exposes zero jobs through the workflow API. I am not assigning a test cause without a job-level receipt.

Status: stronger lifecycle hardening, but the discarded ownership-refusal path and shard-topology violation remain blocking on this exact head.

_bind_live_session_transport() proved registry authority and then
answered with a bare bool, collapsing two situations that are not
interchangeable for the caller: the record is still authoritative but
the request socket died, versus the record lost registration authority
to a concurrent disconnect teardown.

Every mutation caller discarded that False. prompt.submit resolves its
session through the UNLOCKED _sess_nowait() read, so a teardown can pop
that exact record before the bind; the handler then kept appending
history, queueing prompts and latching `running` on a record teardown
already owned. _drain_queued_prompt() had the same hole in the opposite
order - it claimed the queue and set `running` BEFORE asking about
ownership.

Return a typed SessionBindOutcome (BOUND / TRANSPORT_DEAD /
STALE_RECORD) and make STALE_RECORD terminal:

- prompt.submit answers 4001 "session not found", the code the client
  already treats as "resume the stored session", before it touches
  history, the queue or `running`. A dead request transport stays
  non-terminal - that turn is legitimate.
- _drain_queued_prompt() proves authority before the claim and, if the
  window closes mid-claim, restores the envelope and releases `running`
  so the popped record is left exactly as teardown expects it.

Ownership and deferred-build fencing move out of the decomposition-owned
server.py godfile (NousResearch#78630) into tui_gateway/session_ownership.py, which
server.py re-exports under the original underscore names so the
rebound-globals handler seam (method_ctx.py) and the existing
monkeypatch targets keep working. server.py loses ~120 net lines. The
new regressions live in tests/tui_gateway/test_session_ownership.py
rather than extending tests/test_tui_gateway_server.py (NousResearch#78629), and the
retired-session-id test moves there too.

The two prompt.submit method-boundary tests fail on the prior behavior
with KeyError: 'error' - the RPC succeeded against the popped record.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013XSgsNweukuU4n2WYX19JW
@JoaoMarcos44

Copy link
Copy Markdown
Contributor Author

@andrexibiza both blockers are closed on 1b23b964fc. Thanks for holding the line on the first one — you were right that the exact-record proof was being weakened to a boolean the side-effect path discarded.

P1 — discarded ownership refusal

_bind_live_session_transport() now returns a typed SessionBindOutcome instead of a bool:

Outcome Meaning Caller contract
BOUND claimed proceed
TRANSPORT_DEAD record still authoritative, request socket died non-terminal — already-admitted work may finish
STALE_RECORD record lost registration authority terminal before any history / queue / running mutation

Registry authority is proved before transport liveness, so the two reasons stay distinguishable even when both apply (test_stale_record_wins_over_a_dead_transport). STALE_RECORD also covers a record that is still in _sessions but already marked _closing/_finalized — the window _pop_session_by_id() opens when it sets _closing under the registry lock before the pop.

prompt.submit:

    t = current_transport()
    if t is not None:
        if _bind_live_session_transport(sid, session, t).is_stale_record:
            return _err(rid, 4001, "session not found")
    elif not _session_record_is_authoritative(sid, session):
        return _err(rid, 4001, "session not found")

The elif matters: a request with no socket at all still has to prove authority, otherwise the same worker walks past the gate untouched. 4001 is the reconciliation you asked for rather than a new error class — it is the exact code the client already treats as "resume the stored session".

_drain_queued_prompt() proves authority before the claim, and if the window closes mid-claim it restores the envelope and releases running, so the popped record is left exactly as teardown expects it. Losing ownership must not eat the user's queued turn.

Method-boundary race test, as requestedtest_prompt_submit_refuses_a_record_teardown_claimed_mid_request. It barriers a real prompt.submit after _sess_nowait() and before the bind (on _load_dashboard_process_isolation_config(), the last call before the claim — no sleeps, no polling), lets a real _pop_session_by_id() claim that exact record, then releases the worker and asserts:

  • the RPC answers 4001;
  • running is still False, history is still [], queued_prompt/queued_prompts are absent, and transport was never replaced on the popped object.

Plus the no-transport variant, the two drain witnesses, and test_prompt_submit_survives_a_dead_request_transport so the fix cannot over-refuse a legitimate turn whose socket merely died.

Falsification: reverting only the prompt.submit gate (module, enum and tests intact) makes both method-boundary tests fail with KeyError: 'error' — the RPC succeeded against the popped record. That is your defect, reproduced and then closed.

Hard blocker — shard topology

This head now shrinks both decomposition-owned surfaces instead of growing them.

New focused module tui_gateway/session_ownership.py (260 lines) owns the ownership primitive and the deferred-build fencing: SessionBindOutcome, bind_live_session_transport, session_record_is_authoritative/session_is_live_for_commit, session_lifecycle_lock, run_live_build_effect, commit_agent_build, commit_resume_hydration, commit_resume_failure_state, the bounded retired-session-id registry, and the deferred_build_effect_session ContextVar.

server.py keeps only the import plus underscore aliases, which preserves the method_ctx.py rebound-globals seam and every existing monkeypatch target.

 tests/test_tui_gateway_queue_on_busy.py     |  18 +-
 tests/test_tui_gateway_server.py            |  20 --
 tests/tui_gateway/test_goal_command.py      |  14 +-
 tests/tui_gateway/test_session_ownership.py | 333 ++++++++++++++++++
 tui_gateway/methods_prompt.py               |  16 +-
 tui_gateway/server.py                       | 216 +++++-------
 tui_gateway/session_ownership.py            | 260 ++++++++++++++

tui_gateway/server.py: −216/+96, ≈120 net lines removed. tests/test_tui_gateway_server.py: −20 (the retired-session-id test moved into the focused module). All new regressions live in tests/tui_gateway/test_session_ownership.py.

Local evidence

Run Head 1b23b964fc Baseline 0efbb20a70
tests/tui_gateway/ + test_tui_gateway_queue_on_busy.py 14 failed, 646 passed, 1 skipped 14 failed, 636 passed, 1 skipped
the above + tests/test_tui_gateway_server.py 30 failed, 1251 passed, 1 skipped 30 failed
ruff check tui_gateway/ tests/ clean clean

The failure set is identical between head and baseline (diffed by collected failure name); it is all local Windows/env noise — missing ruamel (16 config/YAML-save tests), signal.SIGPIPE absent on Windows, hermes.CMD, a Windows tempfile-unlink case, and test_protocol.py ordering artifacts that fail on baseline too. Net +10 passing tests, zero new failures. I am not substituting this for the hosted receipt — remote CI on this exact head is the authority, and I will chase any job-level failure it reports.

Two existing harnesses were updated because they drove the drain against a record that was never registered — an assumption production never satisfies and the one this change now rejects: _session() in test_tui_gateway_queue_on_busy.py and _turn_session() in test_goal_command.py now register into server._sessions, with registry isolation per test. The goal case is a real behavioral witness, not a cosmetic fix: without registration the queued user prompt silently loses to the goal continuation.

One deliberate omission

methods_session.py's live-lazy reattach still ignores the bind outcome. I left it: it mutates only last_active before returning, and a STALE_RECORD there degrades to the pre-existing 4001 the client already recovers from on its next session-scoped RPC. Say the word and I will make it terminal too, but it is not the same authority-failure class as the two mutation paths and I would rather not widen the diff without you asking.

On #94697 / #86784: bind_live_session_transport() is now a single named entry point in its own module, which should make a fan-out replacement strictly easier to compose than a helper buried in server.py. And on the #95197 provenance — agreed, superseded-by rather than duplicate; @konsisumer's snapshot-to-claim repair is the correct narrower shape and I have kept it credited in the non-duplicate analysis.

JoaoMarcos44 and others added 2 commits August 27, 2026 12:53
# Conflicts:
#	tests/test_tui_gateway_server.py
#	tui_gateway/server.py
…stone

Merging current main surfaced two problems on this branch.

1. The merge concatenated two copies of
   test_close_sessions_for_transport_skips_session_rebound_before_claim.
   The second copy re-captured `original_sessions_lock` AFTER the first
   monkeypatch had already replaced `_sessions_lock` with the interlock,
   so `__enter__` called `.acquire()` on the interlock itself and both
   parametrizations died with AttributeError. Keep one copy - the one
   that records teardowns.

2. The retired-runtime-id LRU is process-global and, unlike production,
   test code reaches it without the registration paths that clear it
   (`_init_session` / `_claim_or_reuse_live` call
   forget_retired_session_id). Many modules under tests/tui_gateway/
   share generic ids like "s1" and pop records straight out of
   `_sessions`, so one module's tombstone silenced `_emit` for that id
   in every later module - test_protocol.py failed only when it ran
   after such a module. Add tests/tui_gateway/conftest.py to clear the
   registry per test so module order cannot change outcomes.

Local gateway suites now match clean origin/main exactly: 23 failed on
both, identical failure set (all pre-existing Windows/env noise), with
+22 passing tests on this head. tests/tui_gateway/test_serve_exit_flush.py
is excluded from the comparison because it terminates the pytest process
(exit 15) on clean origin/main too.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013XSgsNweukuU4n2WYX19JW

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review on exact head 7c28eda02b1bdb9125b866d27466677951749986.

The two blockers from my prior review are materially repaired on this object. prompt.submit now treats STALE_RECORD as terminal before history/queue/running mutation; _drain_queued_prompt() proves authority before claiming the queue and restores the envelope/latch if teardown wins inside the claim window. The ownership policy is also extracted into the focused 260-line tui_gateway/session_ownership.py, with the new race witnesses in tests/tui_gateway/test_session_ownership.py; this no longer regrows the ownership implementation inside the decomposition-owned server godfile.

Exact-head hosted evidence is green: CI 33094965734, Docker 33094965046, Nix 33094965033 all succeeded.

One P1 ownership hole remains in the live-lazy session.resume path in tui_gateway/methods_session.py.

_find_live_unpersisted() and the following _sessions.get(live_sid) are unlocked. After that lookup, the handler immediately mutates live["last_active"], then calls _bind_live_session_transport(live_sid, live, transport) but discards the typed outcome, and finally returns a successful resume payload from that same live object. If a disconnect teardown pops/replaces the record between the unlocked lookup and the bind, the helper correctly returns STALE_RECORD — but this RPC still reports success against a record the registry has already declared non-authoritative. That is the same proof-loss class the new enum fixed in prompt.submit, just on the resume admission path.

The transport is None branch is weaker still: it calls _cancel_ws_orphan_reap(live_sid) without any exact-record authority proof. A stale lookup can therefore cancel the sid-keyed reap while no longer owning that runtime.

The comment in the PR body says this can degrade to 4001 on the next session-scoped RPC, but the current session.resume itself returns _ok(...); the stale success is observable before that later recovery happens.

Required closure: make the typed ownership result authoritative here too. Do not mutate last_active, cancel a reap, or return the live-lazy success payload unless the exact record is still authoritative. A STALE_RECORD result should retry/re-resolve the authoritative record or fail this resume with the existing recovery-compatible not-found outcome. For the no-transport path, perform the same exact-record claim before touching the timer.

Please add a deterministic method-boundary witness that barriers live-lazy resume after the unlocked registry lookup, lets teardown pop or replace that exact record, then proves the RPC cannot return stale success and cannot cancel the replacement runtime's reap/timer. A companion no-current-transport schedule should pin the timer case if it is not structurally covered by the same claim.

Landing note: current main is 0dfba37b11ff2ca908ae2df85b55f4f4c9b7fd8b, 24 commits beyond this PR's recorded base, and that interval does touch tui_gateway/server.py. GitHub currently reports the PR mergeable, but the final repaired head still needs a semantic re-read of that overlapping server surface rather than transferring today's green receipts after another head move.

Status: prior mutation-caller and shard-topology blockers closed; live-lazy resume still weakens STALE_RECORD into success.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review of exact head 7c28eda02b1bdb9125b866d27466677951749986 against current base/merge-base f3cbb262c1d7014f1a4d225242d06983122c5ddf.

The typed SessionBindOutcome and the focused tui_gateway/session_ownership.py owner are directionally right. Exact-head hosted CI is green. This object is still not merge-ready: two ownership blockers remain, and the current merged diff has reintroduced the decomposition regression.

P1 — prompt.submit proves authority only after it has already acquired and mutated ownership

In tui_gateway/methods_prompt.py, the order after the unlocked _sess_nowait() read is currently:

  1. _ensure_active_session_slot(sid, session)
  2. session["client_surface"] = ...
  3. _load_dashboard_process_isolation_config()
  4. _bind_live_session_transport(...) / _session_record_is_authoritative(...)

That means the claimed terminal rule — “nothing may be queued, mutated, or started on STALE_RECORD” — is still false before the typed gate runs. _ensure_active_session_slot() is not a read: it can acquire a process-global active-session lease and store it on the record.

A concrete interleaving remains:

  1. _sess_nowait() returns record S.
  2. disconnect teardown pops and fully finalizes S; _release_active_session_slot() sees no lease.
  3. the stale submit acquires a new lease onto S and writes client_surface.
  4. the bind finally reports STALE_RECORD, and the RPC returns 4001.

No registered owner remains to release that late lease, so a stale request can permanently consume one of the configured concurrent-session slots. If the cap is already full, the same stale request can return 4090 before it ever reaches the required 4001 ownership refusal.

The new regression does not cover this. _barrier_prompt_submit() blocks on _load_dashboard_process_isolation_config(), which is downstream of both _ensure_active_session_slot() and the client_surface write; its “nothing was touched” assertion omits exactly the two effects that already happened.

Required repair: make the generation/authority claim the first effectful operation after _sess_nowait(). No lease acquisition, surface mutation, config-dependent admission, history/queue mutation, or turn start may precede it. Add a deterministic method-boundary witness that lets a real pop and finalization complete before admission, then proves 4001, an unchanged client_surface, no lease on the stale record, and no entry left in the active-session registry.

P1 — the typed terminal outcome is still discarded by two live reattach surfaces

The closure note identifies one deliberate omission, but the current head has two:

  • tui_gateway/server.py::_live_session_payload() calls _bind_live_session_transport() and discards the outcome, then mutates cols / last_active, reads the record, and returns a payload. session.activate reaches this helper from an unlocked _sess_nowait() lookup without holding _session_resume_lock. A pop between lookup and bind therefore produces STALE_RECORD, yet session.activate still returns success for a runtime teardown owns.
  • The live-lazy/unpersisted branch in tui_gateway/methods_session.py::session.resume writes live["last_active"] before the bind, discards the bind outcome, and returns _ok(...) with the stale runtime id/history. Its no-request-transport branch cancels orphan reap without proving authority at all.

“the next session-scoped RPC will return 4001” is not graceful recovery. The current session.activate / session.resume RPC has already minted a false ownership receipt and attached the frontend to a dead runtime.

Required repair: propagate the typed outcome through _live_session_payload() and make STALE_RECORD terminal at both method boundaries before any touch, timer cancellation, history projection, or success response. Add deterministic lookup → pop → claim races for session.activate and the live-lazy resume path; assert no mutation, no reap cancellation, and no successful payload from the stale object.

Hard blocker — the exact merged diff still grows both decomposition-owned godfiles

The current base is also the merge base, so these are not main-only drift artifacts. The exact f3cbb262… → 7c28eda0… compare is:

  • tui_gateway/server.py: +287 / −94 — net +193
  • tests/test_tui_gateway_server.py: +332 / −4 — net +328

The 1b23b964… closure note’s server.py −216 / +96 and test-file −20 receipts are stale for the object now under review. The new focused ownership module and focused test file are useful, but the current deliverable still adds hundreds of PR-owned lines back into both decomposition surfaces, including new ownership/deferred-build regressions in tests/test_tui_gateway_server.py.

Required repair: finish the composition on current main. Move the PR-owned lifecycle/deferred-build behavior and its regressions into focused sub-2K owners so the exact current-base diff does not grow either protected surface. Re-run the exact-head suite after that move; old-head arithmetic is not a closure receipt.

Exact-head verification

All current check runs are completed with no failed, cancelled, or in-progress check; GitHub’s All required checks pass job succeeded. Hosted workflow receipts:

Green CI is necessary, but it does not discharge the three blockers above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/tui Terminal UI (ui-tui/ + tui_gateway/) duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WS disconnect teardown race can close or orphan a session reconnected via session.resume

3 participants