refactor: extract endSession teardown helper - #134
Merged
Merged
Conversation
Five sites hand-rolled the same transport teardown (closeStream, stopKeepalive, polling=false, sid=null, plus assorted extras): the session-error auto-reconnect, auth_failed, alive===false, the transport-gave-up fallback, and the vault no-key disconnect. Fold the core into endSession(p, o) with the genuinely-different bits opt-in (o.disconnect / o.save / o.badge). The deferred-save timer is now cleared (and nulled) at every site, not just the vault one. Two fixes that review of the extraction surfaced: - connectPane's success path re-arms scheduleSaveCommit when a pendingSave is still outstanding. Before, the auto-reconnect path relied on the OLD timer firing against the reassigned sid to commit the save on an idle SSE session; clearing the timer in endSession exposed that, and reconnects outside the 2.6s window had always lost the idle-save. New test pins the scenario (fails without the re-arm). - transportFatal now bails when p.polling is already false: a stale long-poll rejection / SSE error from a transport endSession tore down must not re-banner the pane or reset p.connecting (which would defuse connectPane's in-flight duplicate guard mid-reconnect). 626 frontend tests OK.
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
2 participants
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.
What
Five sites hand-rolled the same transport teardown (closeStream, stopKeepalive,
polling=false,sid=null, plus assorted extras): the session-error auto-reconnect,auth_failed,alive===false, the transport-gave-up fallback, and the vault no-key disconnect. This folds the core intoendSession(p, o)with the genuinely-different bits opt-in:o.disconnect— POST/api/disconnectfor the pre-null sid (fire-and-forget)o.save—saveSessions()o.badge—updatePaneBadge(p)The deferred-save timer is now cleared and nulled at every site, not just the vault one (clearing drops the closure's pane ref; nulling makes
p.saveCommitTimertruthiness meaningful).Two behavior fixes the extraction surfaced (adversarial review)
connectPanesuccess now re-armsscheduleSaveCommitwhen apendingSaveis outstanding. The auto-reconnect path silently relied on the OLD deferred-save timer firing against the reassigned sid to commit the save on an idle SSE session. Clearing the timer inendSessionexposed that — and reconnects outside the 2.6s window had always lost the idle-save (pre-existing gap, also fixed by the re-arm). New test pins the scenario and was verified to fail without the re-arm.transportFatalbails whenp.pollingis already false. A stale long-poll rejection / SSE error from a transport thatendSessionalready tore down must not re-banner the pane — and must not resetp.connecting, which would defuseconnectPane's in-flight duplicate guard during an auto-reconnect (double/api/connect+ leaked server PTY).Ordering preserved
Per-site operation order verified against pre-image (badge-before-bar, save-after-bar, banner-before-teardown); the auth_failed badge/recentOutput swap is unobservable (badge reads neither). Sites that legitimately pair closeStream/stopKeepalive for transport restarts (visibility resume, SSE→long-poll fallback) are deliberately untouched.
Tests
626 frontend tests OK (+1 new: pendingSave survives a session-error auto-reconnect; verified red without the fix).