WS reconnect loses session state on reload: is_editor/is_cutting and live-show leadership (on_close finalises before REFRESH_CLIENT can reclaim)

Summary

REFRESH_CLIENT (sent by every client on reconnect/page-reload when it already has a persisted internalUUID) does not actually preserve is_editor/is_cutting across a full page reload. This was found by a new Playwright test (client-v3/e2e/tests/15-collaborative-script-editing.spec.ts, "reloading during a draft rejoins it") that is the first test in the suite to reload the page while holding an edit lock — no existing spec does this, so the path has zero coverage. Reproduced 4/4 times locally (initial attempt + 3 retries).

Root cause (traced in code, not yet fixed)

server/controllers/ws_controller.py:

open() (~L99-103) generates a fresh internal_id for every new WS connection and immediately calls self.update_session(user_id=...), which creates a Session row for that fresh id with is_editor=False (the default).
on_message's dispatcher (~L340) computes entry = session.get(Session, self.internal_id) once, using the connection's current (fresh) internal_id — before branching on the op.
In the REFRESH_CLIENT branch (~L369-390), is_editor = entry.is_editor reads that fresh, just-created row — which is never an editor — not the row for new_uuid (the old, persisted id the client is asking to resume). So the value copied forward is always False.
Separately, on_close (~L111 onward) unconditionally deletes the Session row for the disconnecting internal_id as soon as the old socket closes (no grace period). On an ordinary page reload the old socket closes at or before the new one opens, so even if step 3's lookup targeted the right id, the row it needs may already be gone by the time REFRESH_CLIENT runs.
on_close also actively tears down an in-progress collaborative script room if the disconnecting client was its only editor (room.remove_client → close_active_room() when not room.has_editors), on every disconnect — including a same-tab reload, not just an intentional "Stop Editing".

Net effect: reloading the page while editing the script silently drops is_editor, so the UI shows "Edit" again instead of "Stop Editing" after a reload. No draft data is lost (the room rebuilds from the checkpointed draft on the next join), but the user's "I am editing" state is lost without any explanation, and this appears to affect the legacy single-editor lock and live-session leader election equally, not just the new collaborative editor — those flows have simply never had a reload-mid-session test either.

Evidence

Zero references to REFRESH_CLIENT anywhere in server/test/ — this path has never been unit-tested.
Playwright trace/screenshot for the failing E2E attempt (available on request / in CI artifacts) shows a normal "Edit" button after reload where "Stop Editing" was present before.

Scope note

This is orthogonal to the collaborative-editing rebuild (issue #889) — it's shared WS session-lifecycle code used by script editing, cutting, and live-session leader election alike. Please investigate and, if the fix is small and safe, propose it; if it touches broader session semantics, scope a follow-up.

Suggested starting points (not verified as sufficient — the on_close race needs checking too)

In the REFRESH_CLIENT branch, look up the row by new_uuid (the id being resumed), not by the connection's fresh self.internal_id.
Investigate whether on_close needs to leave a resumable trace (e.g. a short grace period, or not deleting a row whose is_editor/is_cutting a same-tab reload is likely to reclaim) rather than deleting unconditionally, given the two events race.

Investigated — root cause confirmed, but not what the write-up says. No production fix implemented (needs a design decision).

I branched investigate/refresh-client-session-bug from dev (local only, not pushed) and added two empirical integration tests to server/test/controllers/test_ws_controller.py, using real WebSocket connections through the existing Tornado test harness (matches this file's existing _connect_and_auth pattern). Committed locally as 4069c74 (not pushed).

What I verified empirically (via the two new tests, not just reading)

I confirmed by removing @unittest.expectedFailure and running each test in isolation (pytest -k refresh_client -n0) that both fail exactly where intended, not via timeout or an earlier assertion:

test_refresh_client_after_stale_close_loses_is_editor fails at:
AssertionError: False is not true : is_editor did not survive a real reload -- on_close's unconditional delete raced ahead of REFRESH_CLIENT
test_refresh_client_before_stale_close_loses_is_editor fails at:
AssertionError: unexpectedly None : stale ws1's on_close deleted the live, just-resumed uuid1 row instead of leaving it alone

Both are now left in the branch with @unittest.expectedFailure (xfail), so they currently "pass" (as documented, known-broken behavior) and will flip to a loud unexpected pass the moment a real fix lands — rather than silently continuing to assert the bug. Full backend suite: 934 passed, 2 xfailed, 0 failures (no production code was changed, only tests added, so this is a clean baseline, not a regression check).

Two independent mechanisms cause the loss, one per possible ordering between the old socket's on_close and the new socket's REFRESH_CLIENT:

Mechanism A — on_close runs first (the scenario the issue describes): on_close synchronously deletes the Session row for the disconnecting internal_id, with no grace period, before the new connection's REFRESH_CLIENT ever runs. By the time REFRESH_CLIENT looks for the row to resume, it's already gone — there is nothing to recover, regardless of which key you look it up by.
Mechanism B — REFRESH_CLIENT runs first (not previously identified): the new connection (H2) successfully resumes the old row via REFRESH_CLIENT, reassigning its own internal_id attribute to the old uuid. But the old, now-stale connection object (H1) never updates its own internal_id attribute — it's still holding the old uuid. When H1.on_close eventually fires (the old socket takes a moment to actually tear down after navigation), it looks up Session by its own self.internal_id — unchanged, and now identical to H2's — and deletes it unconditionally. This destroys H2's freshly-resumed, live session purely because the two handler objects collided on the same internal_id string.

I did not determine which ordering a real browser reload produces — neither my tests nor the E2E repro establish this; both orderings were reproduced by explicit sequencing in the test, and both independently cause data loss, so it doesn't actually matter which one wins in practice: either way, the bug reproduces.

Correction to the issue's stated root cause

Item 3 in the trace (REFRESH_CLIENT reads is_editor from entry, which is keyed by the connection's own fresh internal_id from open(), rather than from the row keyed by new_uuid) is real code as described, but I verified it is currently behaviorally inert — it has zero effect on the outcome in either ordering I tested. That's because update_session() (server/controllers/ws_controller.py:64-89) only sets is_editor/is_cutting when it creates a new Session row (the else branch); when a row for that internal_id already exists (the if entry: branch), it silently ignores the is_editor/is_cutting kwargs entirely and only touches last_ping/last_pong/user_id. So:

When the target row still exists at REFRESH_CLIENT time, whatever is_editor was already on it survives untouched — regardless of what (wrong) value REFRESH_CLIENT's local variable computed, because update_session never writes it.
When the target row is already gone, update_session takes the create-path and would use whatever the REFRESH_CLIENT branch's local variable holds — which is currently always False (since entry is the fresh, just-opened row) — but this only matters if the row doesn't already exist, which per mechanism A/B above is the dominant case anyway.

These two bugs are coupled, not independent: fixing only the "wrong row" lookup, or only update_session's asymmetry, without the other, doesn't help (and fixing update_session alone would make things worse in the row-still-exists case, overwriting a surviving True with the fresh row's False). I actually implemented and tested the "look up by new_uuid" lookup fix in isolation to confirm this empirically — it did not make either xfail test pass; the real target row is gone (mechanism A) or gets deleted immediately after by the stale handler (mechanism B) regardless of which row REFRESH_CLIENT reads from. I reverted that change; it's not in the branch.

On the "affects leader election equally" claim

Based on reading (not tested): this appears to be only partly right. The live-session leader-election path (ShowSession.last_client_internal_id / client_internal_id, set in on_close and reclaimed in the REFRESH_CLIENT branch when new_uuid matches) already has a resumable-trace mechanism that survives the Session row's deletion, since it lives on the separate ShowSession row. That makes leader election resilient to mechanism A specifically. But under mechanism B, the stale handler's on_close still finds entry.live_session truthy on the now-H2-owned row, deletes it, and runs leader election anyway — which for a single-tab reload would likely elect NO_LEADER (no other session for that user). So leader election is not fully immune either; it just has a partial mitigation that editor/cutting flags entirely lack. I have not written a test for this — flagging as inference from reading only.

Why I didn't implement a production fix

Per the task scope: this needs a real design decision, not a small/safe patch. Concretely:

Mechanism A requires on_close to stop unconditionally deleting a Session row that a same-tab reload may be about to reclaim — e.g. a grace period, or a persisted resumable trace analogous to ShowSession.last_client_internal_id, but for is_editor/is_cutting. This needs decisions about: how long to wait, where the trace lives, how/when it's cleaned up if never reclaimed, and interaction with RBAC (should a resumed edit lock re-validate write access?) and the room-teardown logic that also currently runs unconditionally on every on_close.
Mechanism B has a narrower, more mechanical candidate guard: on_close could check whether another WebSocketHandler in self.application.clients currently holds the same internal_id before deleting the Session row / firing notifications, and skip cleanup entirely if so (a live connection now owns that identity). I have not implemented or tested this — it's a candidate, not a verified fix — but it looks narrower than mechanism A's fix and might be worth doing first/independently.

Recommended direction

Address mechanism B first as a narrow, mostly-mechanical fix (skip on_close cleanup when another live client already holds the same internal_id) — but it should still get its own test before landing, and needs a look at whether the room-teardown/broadcast logic in on_close has the same issue.
Treat mechanism A as a separate, deliberate design task: a grace period or persisted resumable trace for is_editor/is_cutting, following the existing ShowSession.last_client_internal_id precedent.
Once a fix lands for either/both, flip the corresponding expectedFailure test(s) in my branch (or equivalents) to plain passing assertions — that's exactly what they're for.
E2E verification (client-v3/e2e/tests/15-collaborative-script-editing.spec.ts, "reloading during a draft rejoins it") is still pending — I skipped running Playwright entirely per the task's guidance (another session may be using port 8888), so this is unverified at the E2E layer beyond the original report. Note: this spec file doesn't exist on dev yet — it's on an in-progress feature branch.

Where things are

Branch investigate/refresh-client-session-bug, local only, not pushed, commit 4069c74 on top of dev at 549e847.
Only file touched: server/test/controllers/test_ws_controller.py (+149 lines, two new tests). No production code changed.
Full backend suite: 934 passed, 2 xfailed, 0 failures.

Happy to paste the full test diff here on request, or push the branch for review if that's preferred over leaving it local-only.

Consolidating #1424 into this issue: same root cause

#1424 ("Live show leader re-election can race when the same user has two open tabs") is being closed as a duplicate of this issue. Both bugs come from the same WS session-lifecycle flaw in server/controllers/ws_controller.py:

on_close treats every disconnect as final and immediately destroys state tied to the departing internal_id (it deletes the Session row, promotes a new live-session leader and tears down the collab room). REFRESH_CLIENT can only reclaim whatever state survives that. On a reload the old socket's close and the new socket's REFRESH_CLIENT race each other, so resumable state is lost.

A fix for either bug has to change the same rule: when on_close finalises a departed uuid, and how REFRESH_CLIENT reclaims it. Fixing them separately would produce two conflicting reclaim designs in the same functions.

Context carried over from #1424 (corrected mechanism, per its final comment)

Two tabs, same user, leader L and follower F, both reloading (e.g. RELOAD_CLIENT broadcast by "Jump To Page" / "Reload Clients"):

L's on_close runs election: select(Session).where(Session.user_id == live_session.user_id).first() (no ORDER BY) finds F's still-open row and promotes F: client_internal_id = F, last_client_internal_id = None.
When F's own socket closes, F is now the leader, so F's on_close runs election again. It sets last_client_internal_id = F, finds no other same-user session, and emits NO_LEADER.
F's REFRESH_CLIENT matches last_client_internal_id == F and reclaims leadership. That reclaim never checks who currently holds client_internal_id, and NEW_CLIENT never clears last_client_internal_id.
L's REFRESH_CLIENT has nothing to match, so L stays a follower.

Evidence: A/B-testing the #1425 E2E fix (reverting the follower to the admin login) failed 3 of 4 full Firefox runs, with the leader's own header showing "Following" after reload.

This lines up with Mechanism B in the investigation above: a stale handler's on_close acts on a uuid that a live handler has already resumed. The investigation already noted this affects leader election as well as editor flags.

Acceptance criteria for the fix

Each of these needs a backend WS integration test, written before the production change:

1. A single-tab reload keeps is_editor / is_cutting (Mechanism A: on_close runs first).
2. A stale handler's on_close doesn't delete or finalise a row that a live handler has resumed via REFRESH_CLIENT (Mechanism B).
3. A single-tab reload by the live-session leader keeps leadership.
4. Two same-user tabs both reloading: the original leader ends up leader, in both close orderings.
5. Regression guard: a genuine disconnect with no reconnect still releases the edit/cut lock (and notifies clients) and still re-elects a leader or emits NO_LEADER.
The two expectedFailure tests on investigate/refresh-client-session-bug (4069c74) become plain passing tests.

A fix is being picked up on a fix/ branch targeting dev.

For whoever picks this up: client-v3/e2e/tests/15-collaborative-script-editing.spec.ts has a test named "reloading loses editor status (known gap, issue #1419) but not the draft" that currently asserts the buggy behaviour (the Edit button reappears after a reload) as a tripwire. Once this is fixed server-side, that test will start failing — at that point, invert the assertion to check that stopEditing(page) is visible instead of editButton(page). CollabScriptEditor.vue's reload-rejoin code path (onMounted's if (isEditor.value) retryJoin();) needs no client-side change to pick up the fix.

Address PR #1421 review: Stop Editing lifecycle bug, stuck-spinner fa…

Hold WS session state through a reconnect grace window (fixes #1419, #…

Fixed by #1427, squash-merged into dev as d9718b8. Closing manually, because the PR targeted dev rather than the default branch, so GitHub's "Fixes #1419" auto-close didn't trigger.

Summary of the fix

Reconnect grace window: a disconnect is no longer final straight away. The Session row, its edit/cut lock, live-show leadership and the collab room close are held for a short window (5s by default). The original deadline is never re-armed.
Ownership rule: a client uuid identifies a session but is never a credential. Edit, cut and leader privileges can only be used by a connection authenticated as the row's owner (and, for leader ops, as the show session's user), checked at every use. REFRESH_CLIENT restores identity only.
No inheritance: a different user presenting someone else's uuid gets a fresh one (REASSIGN_UUID, handled by both clients). show/sessions/start binds to the requester.
Leader election is deterministic, never replaces a live holder, and skips tabs that are about to be logged out. The original leader tab wins the two-tab reload race from Live show leader re-election can race when the same user has two open tabs #1424.
Per-tab identity in both clients: the client uuid is stored in sessionStorage, and the old localStorage copy is removed on first load.
Pre-existing bugs fixed along the way:
- broadcast loops skipping clients;
- on_message exceptions killing the read loop;
- ApiRoute binding classes to None;
- a SQLite lock stall on admin password reset;
- force-logout leaving locks held.

The acceptance checklist in this issue is covered by backend tests in server/test/controllers/test_ws_controller.py (written test-first and mutation-checked) plus E2E coverage in specs 02, 03 and 10. Full suites were green at merge: pytest 1043, and Playwright 225/225 on Chromium and Firefox.

Follow-up: #1430, LIVE_SHOW_JUMP_TO_PAGE isn't leader-gated. It's still open.

Merge remote-tracking branch 'origin/dev' into feature/collab-editing…

Collaborative script editing: read-only lifecycle and browse path (Ph…

Navigation Menu

Repository navigation

WS reconnect loses session state on reload: is_editor/is_cutting and live-show leadership (on_close finalises before REFRESH_CLIENT can reclaim) #1419

Summary

Root cause (traced in code, not yet fixed)

Evidence

Scope note

Suggested starting points (not verified as sufficient — the on_close race needs checking too)

Investigated — root cause confirmed, but not what the write-up says. No production fix implemented (needs a design decision).

What I verified empirically (via the two new tests, not just reading)

Correction to the issue's stated root cause

On the "affects leader election equally" claim

Why I didn't implement a production fix

Recommended direction

Where things are

Consolidating #1424 into this issue: same root cause

Context carried over from #1424 (corrected mechanism, per its final comment)

Acceptance criteria for the fix

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

WS reconnect loses session state on reload: is_editor/is_cutting and live-show leadership (on_close finalises before REFRESH_CLIENT can reclaim) #1419

Description

Summary

Root cause (traced in code, not yet fixed)

Evidence

Scope note

Suggested starting points (not verified as sufficient — the on_close race needs checking too)

Activity

Tim020 commented on Sep 22, 2026

Investigated — root cause confirmed, but not what the write-up says. No production fix implemented (needs a design decision).

What I verified empirically (via the two new tests, not just reading)

Correction to the issue's stated root cause

On the "affects leader election equally" claim

Why I didn't implement a production fix

Recommended direction

Where things are

Tim020 commented on Sep 24, 2026

Consolidating #1424 into this issue: same root cause

Context carried over from #1424 (corrected mechanism, per its final comment)

Acceptance criteria for the fix

Tim020 commented on Sep 24, 2026

Tim020 commented on Oct 19, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions