fix(#4671): close 2 profile-switch render races via skeleton-flag + ws generation token
Reworked from the Codex re-gate (rebased onto fresh master so #4730's gateway-poll work is preserved, not reverted). Two CORE races + a test-window break: (1) Skeleton-clobber (sessions.js): renderSessionListFromCache() now bails while _sessionListSkeletonActive — _allSessions still holds the PREVIOUS profile until /api/sessions resolves, so a gateway-poll/SSE/timer/cache render mid-switch would repaint the wrong profile's rows over the skeleton. _applySessionListPayload clears the flag right before painting (fresh data in hand; superseded responses already discarded by the generation guard); the fetch-failure path and switch-failure catch clear it too so it can't strand. NO signature change (kept renderSessionListFromCache() and _applySessionListPayload(sessData,projData) so the ~10 static tests that anchor on those markers stay green). (2) Workspace wrong-profile tree (workspace.js + panels.js): an empty-session switch reuses the same session_id, so loadDir()'s id guard can't reject a stale pre-switch /api/list. Added _wsTreeGen generation token + bumpWorkspaceTreeGen(), bumped UNCONDITIONALLY at switch start (even panel-closed, since loadDir('.') still runs), captured in loadDir() and re-checked after both awaited /api/list points. (3) test_issue1116 proximity window 5000->6500 (applyBotName now ~char 5618, still fires). + regression tests: skeleton-flag bail/clear lifecycle, ws generation token, failure restore clears flag; updated #1785 stale-session guard test for the compound condition. Co-authored-by: rodboev <rodboev@users.noreply.github.com>
This commit is contained in:
1 parent
a8805bb658
commit
66786143a8
6 files changed
+155
-8
No files matched your search
+11
-3
@@ -5938,6 +5938,12 @@ async function switchToProfile(name) {
|
||||
if (typeof closeSessionActionMenu === 'function') closeSessionActionMenu();
|
||||
if (typeof showSessionListSkeleton === 'function') showSessionListSkeleton();
|
||||
const _workspaceVisibleAtStart = typeof _workspacePanelMode !== 'undefined' && _workspacePanelMode !== 'closed';
|
||||
// #4671 CORE: invalidate any in-flight workspace-tree load UNCONDITIONALLY at switch
|
||||
// start — even when the panel is closed, loadDir('.') still runs later in the switch,
|
||||
// and an empty-session switch reuses the same session_id so loadDir's id guard alone
|
||||
// can't reject a stale previous-workspace /api/list. Bumping the generation here (not
|
||||
// only inside showWorkspaceTreeSkeleton, which is panel-gated) closes the closed-panel race.
|
||||
if (typeof bumpWorkspaceTreeGen === 'function') bumpWorkspaceTreeGen();
|
||||
if (_workspaceVisibleAtStart && typeof showWorkspaceTreeSkeleton === 'function') showWorkspaceTreeSkeleton();
|
||||
|
||||
// Determine whether the current session has any messages.
|
||||
@@ -6119,9 +6125,11 @@ async function switchToProfile(name) {
|
||||
// are intact — restore the real list/tree so the loading skeletons we showed
|
||||
// up front don't strand. (#4662)
|
||||
if (_switchGen === _profileSwitchGeneration) {
|
||||
if (typeof _sessionListSkeletonActive === 'undefined' || _sessionListSkeletonActive) {
|
||||
if (typeof renderSessionListFromCache === 'function') renderSessionListFromCache();
|
||||
}
|
||||
// The switch failed; _allSessions still holds the (still-current) previous
|
||||
// profile, so clear the skeleton flag and re-render to restore the real list
|
||||
// rather than strand the up-front skeleton (#4671).
|
||||
_sessionListSkeletonActive = false;
|
||||
if (typeof renderSessionListFromCache === 'function') renderSessionListFromCache();
|
||||
if (_workspaceVisibleAtStart && S.session && S.session.workspace && typeof loadDir === 'function') {
|
||||
loadDir('.');
|
||||
} else if (_workspaceVisibleAtStart && typeof clearWorkspaceTreeSkeleton === 'function') {
|
||||
|
||||
@@ -3755,6 +3755,12 @@ function _applySessionListPayload(sessData, projData){
|
||||
_sessionListFirstRenderAnimated=true;
|
||||
}
|
||||
ensureSessionEventsSSE();
|
||||
// #4671: this payload is the freshly-resolved /api/sessions response (and a superseded
|
||||
// response was already discarded by the generation guard upstream), so _allSessions now
|
||||
// holds the CURRENT profile's rows. Clear the skeleton flag right before painting so this
|
||||
// authoritative render replaces the profile-switch skeleton — while unrelated renders that
|
||||
// fire before this point stay blocked by the guard in renderSessionListFromCache().
|
||||
_sessionListSkeletonActive = false;
|
||||
renderSessionListFromCache(); // no-ops if rename is in progress
|
||||
}
|
||||
|
||||
@@ -3829,6 +3835,10 @@ async function _runRenderSessionListRefresh(opts, _gen){
|
||||
const _scopeMatches = _allSessionsScope
|
||||
&& _allSessionsScope.profile === _curScope.profile
|
||||
&& _allSessionsScope.allProfiles === _curScope.allProfiles;
|
||||
// #4671: the /api/sessions fetch failed — clear the skeleton flag so this error
|
||||
// render (matched cache, or empty rows for a mismatched scope) replaces the
|
||||
// up-front profile-switch skeleton instead of stranding it.
|
||||
_sessionListSkeletonActive = false;
|
||||
if (_scopeMatches) {
|
||||
renderSessionListFromCache();
|
||||
} else {
|
||||
@@ -5429,6 +5439,14 @@ function _renderSidebarRowsFromRawSessions(sessionsRaw, referenceSessionsRaw){
|
||||
|
||||
|
||||
function renderSessionListFromCache(){
|
||||
// #4671: while a profile-switch skeleton is up, bail — _allSessions still holds the
|
||||
// PREVIOUS profile's rows until /api/sessions resolves, so any unrelated caller
|
||||
// (sidebar SSE syncs, stream/unread updates, gateway-poll timers, panel-resync
|
||||
// repairs) hitting this mid-switch would repaint the wrong profile's rows over the
|
||||
// skeleton. The authoritative switch render clears the flag from inside
|
||||
// _applySessionListPayload — once _allSessions is fresh — so only a render backed by
|
||||
// up-to-date data replaces the skeleton. The failure-restore path clears it too.
|
||||
if(_sessionListSkeletonActive) return;
|
||||
// Don't re-render while user is actively renaming a session (would destroy the input)
|
||||
if(_renamingSid) return;
|
||||
// Keep the per-conversation actions menu stable while the user is trying to
|
||||
|
||||
+22
-2
@@ -445,6 +445,21 @@ const _WS_SKELETON_ROWS = [
|
||||
{w: 39, indent: 1},
|
||||
];
|
||||
|
||||
// Workspace-tree render generation. loadDir() captures this at call time and
|
||||
// discards its render/cache writes if a newer generation started meanwhile.
|
||||
// #4671 CORE: an empty-session profile switch REUSES the same session_id, so
|
||||
// loadDir()'s session_id guard alone can't reject a pre-switch /api/list response
|
||||
// that resolves after the new profile's loadDir('.') — it would paint the previous
|
||||
// workspace's files over the switched-to profile. switchToProfile() bumps this
|
||||
// UNCONDITIONALLY at switch start (even when the workspace panel is closed, since
|
||||
// loadDir('.') still runs then), so the stale response is rejected.
|
||||
let _wsTreeGen = 0;
|
||||
function bumpWorkspaceTreeGen(){
|
||||
_wsTreeGen = (typeof _wsTreeGen === 'number' ? _wsTreeGen : 0) + 1;
|
||||
return _wsTreeGen;
|
||||
}
|
||||
if(typeof window!=='undefined') window.bumpWorkspaceTreeGen = bumpWorkspaceTreeGen;
|
||||
|
||||
function showWorkspaceTreeSkeleton(){
|
||||
const tree = $('fileTree');
|
||||
if(!tree) return;
|
||||
@@ -492,6 +507,11 @@ async function loadDir(path, opts={}){
|
||||
const refreshExpanded=!!(opts&&opts.refreshExpanded);
|
||||
if(!S.session)return;
|
||||
const sessionId=S.session.session_id;
|
||||
const treeGen=_wsTreeGen; // #4671: capture the workspace-tree generation. A profile
|
||||
// switch bumps it (bumpWorkspaceTreeGen), so a stale response
|
||||
// from the previous workspace — which would pass the session_id
|
||||
// guard because an empty-session switch reuses the same id — is
|
||||
// rejected here instead of painting the wrong profile's files.
|
||||
try{
|
||||
if(!path||path==='.'||refreshExpanded){
|
||||
S._dirCache={};
|
||||
@@ -499,7 +519,7 @@ async function loadDir(path, opts={}){
|
||||
}
|
||||
S.currentDir=path||'.';
|
||||
const data=await api(`/api/list?session_id=${encodeURIComponent(sessionId)}&path=${encodeURIComponent(path)}`);
|
||||
if(!S.session||S.session.session_id!==sessionId)return;
|
||||
if(!S.session||S.session.session_id!==sessionId||treeGen!==_wsTreeGen)return;
|
||||
S.entries=data.entries||[];renderBreadcrumb();renderFileTree();
|
||||
// #2673 — refresh Artifacts tab when its source data (the file tree) updates.
|
||||
if(typeof renderSessionArtifacts==='function') renderSessionArtifacts();
|
||||
@@ -514,7 +534,7 @@ async function loadDir(path, opts={}){
|
||||
.then(dc=>({dirPath,entries:dc.entries||[]}))
|
||||
.catch(()=>({dirPath,entries:[]}))
|
||||
));
|
||||
if(!S.session||S.session.session_id!==sessionId)return;
|
||||
if(!S.session||S.session.session_id!==sessionId||treeGen!==_wsTreeGen)return;
|
||||
for(const {dirPath,entries} of results) S._dirCache[dirPath]=entries;
|
||||
}
|
||||
if(expanded.size>0)renderFileTree();
|
||||
|
||||
@@ -81,8 +81,12 @@ class TestComposerPlaceholderProfile:
|
||||
# Find the function block (starts with 'async function switchToProfile')
|
||||
m = re.search(r'async function switchToProfile\s*\(', src)
|
||||
assert m, "switchToProfile must be an async function"
|
||||
# Get everything after the function declaration (enough context)
|
||||
after = src[m.start():m.start()+5000]
|
||||
# Get everything after the function declaration (enough context).
|
||||
# Window widened 5000 -> 6500 (#4671): the profile-switch loading-skeleton
|
||||
# logic grew switchToProfile, pushing the applyBotName() call to ~char 5618.
|
||||
# The call still fires on every switch; this is a proximity-window adjustment,
|
||||
# not a behavior change (same brittle-window class as the #4720 rotate-url test).
|
||||
after = src[m.start():m.start()+6500]
|
||||
assert "applyBotName" in after, \
|
||||
"switchToProfile must call applyBotName after profile switch"
|
||||
|
||||
|
||||
@@ -54,7 +54,16 @@ def test_load_dir_ignores_stale_session_results():
|
||||
block = _function_block(WORKSPACE_JS, "loadDir")
|
||||
assert "const sessionId=S.session.session_id" in block
|
||||
assert "encodeURIComponent(sessionId)" in block
|
||||
assert "if(!S.session||S.session.session_id!==sessionId)return;" in block
|
||||
# #4671 extended this guard with a workspace-tree generation check (an empty-session
|
||||
# profile switch reuses the same session_id, so the id check alone isn't enough), so
|
||||
# the rejection is now a compound condition. The session_id rejection must remain.
|
||||
assert "S.session.session_id!==sessionId" in block, (
|
||||
"loadDir must still reject results whose session_id no longer matches"
|
||||
)
|
||||
assert "treeGen!==_wsTreeGen" in block, (
|
||||
"loadDir must also reject results from a superseded workspace-tree generation "
|
||||
"(profile-switch stale-load race, #4671)"
|
||||
)
|
||||
|
||||
|
||||
def test_file_preview_breadcrumb_uses_directory_navigation_for_root():
|
||||
|
||||
@@ -56,6 +56,8 @@ class TestSwitchWiring:
|
||||
def test_restores_real_content_on_failure(self):
|
||||
body = _switch_body()
|
||||
catch = body[body.index("} catch (e) {"):]
|
||||
# #4671: failure-restore clears the skeleton flag (asserted in
|
||||
# test_switch_failure_restore_clears_skeleton) then re-renders the real list.
|
||||
assert "renderSessionListFromCache()" in catch, "failed switch must restore real list"
|
||||
|
||||
def test_failure_path_clears_workspace_skeleton_when_no_workspace(self):
|
||||
@@ -215,3 +217,89 @@ class TestSkeletonCss:
|
||||
assert "--skeleton-base:" in compact
|
||||
assert "--skeleton-sheen:" in compact
|
||||
assert ":root.dark{--skeleton-base:" in compact
|
||||
|
||||
|
||||
def _iter_indices(haystack: str, needle: str):
|
||||
"""Yield every start index of needle in haystack."""
|
||||
i = haystack.find(needle)
|
||||
while i != -1:
|
||||
yield i
|
||||
i = haystack.find(needle, i + 1)
|
||||
|
||||
|
||||
class TestSwitchRaceGuards:
|
||||
"""#4671: the two render races the Codex gate caught on the rebased stage."""
|
||||
|
||||
def test_session_skeleton_guard_blocks_non_switch_renders(self):
|
||||
# Race-2: while the profile-switch skeleton is up, renderSessionListFromCache()
|
||||
# must early-return, so a gateway-poll/SSE/timer/cache render can't repaint the
|
||||
# PREVIOUS profile's cached _allSessions over the skeleton in the window before
|
||||
# /api/sessions resolves.
|
||||
idx = SESSIONS.index("function renderSessionListFromCache(")
|
||||
head = SESSIONS[idx: idx + 800]
|
||||
# signature unchanged (many static tests anchor on the empty-paren marker)
|
||||
assert "function renderSessionListFromCache(){" in SESSIONS, (
|
||||
"renderSessionListFromCache signature must stay () — tests anchor on it"
|
||||
)
|
||||
assert "if(_sessionListSkeletonActive) return;" in head, (
|
||||
"renderSessionListFromCache must bail while the profile-switch skeleton is up"
|
||||
)
|
||||
|
||||
def test_skeleton_flag_cleared_by_authoritative_render_when_data_is_fresh(self):
|
||||
# Race-2 (the other half): the skeleton flag must be cleared ONLY when fresh data
|
||||
# is in hand, so the bail above can't strand the skeleton. _applySessionListPayload
|
||||
# runs on the resolved /api/sessions payload (superseded responses already discarded
|
||||
# by the generation guard), so it clears the flag right before painting.
|
||||
apply_idx = SESSIONS.index("function _applySessionListPayload(")
|
||||
apply_body = SESSIONS[apply_idx: SESSIONS.index("\nfunction _mergeRenderSessionListOptions(", apply_idx)]
|
||||
clear_pos = apply_body.find("_sessionListSkeletonActive = false;")
|
||||
paint_pos = apply_body.find("renderSessionListFromCache();")
|
||||
assert clear_pos != -1, "_applySessionListPayload must clear the skeleton flag"
|
||||
assert paint_pos != -1 and clear_pos < paint_pos, (
|
||||
"the skeleton flag must be cleared right before the authoritative paint"
|
||||
)
|
||||
# And the fetch-failure path must clear it too (else a failed switch strands it):
|
||||
refresh = SESSIONS[SESSIONS.index("async function _runRenderSessionListRefresh("):]
|
||||
refresh = refresh[: refresh.index("\nasync function _drainRenderSessionListQueue(")]
|
||||
assert refresh.count("_sessionListSkeletonActive = false;") >= 1, (
|
||||
"the /api/sessions failure path must clear the skeleton flag so it can't strand"
|
||||
)
|
||||
|
||||
def test_switch_failure_restore_clears_skeleton(self):
|
||||
# The switchToProfile catch (failure on the still-current previous profile) must
|
||||
# clear the skeleton flag before restoring the real list, or the skeleton strands.
|
||||
body = _switch_body()
|
||||
catch = body[body.index("} catch (e) {"):]
|
||||
clear_pos = catch.find("_sessionListSkeletonActive = false;")
|
||||
restore_pos = catch.find("renderSessionListFromCache()")
|
||||
assert clear_pos != -1, "switch failure path must clear the skeleton flag"
|
||||
assert restore_pos != -1 and clear_pos < restore_pos, (
|
||||
"skeleton flag must be cleared before the failure-restore render"
|
||||
)
|
||||
|
||||
def test_workspace_tree_generation_token_guards_loaddir(self):
|
||||
# Race-1 (CORE): an empty-session profile switch reuses the same session_id, so
|
||||
# loadDir()'s session_id guard alone can't reject a stale pre-switch /api/list.
|
||||
# A _wsTreeGen generation token, bumped UNCONDITIONALLY at switch start (even when
|
||||
# the workspace panel is closed, since loadDir('.') still runs), gates loadDir().
|
||||
assert "_wsTreeGen" in WORKSPACE, "missing workspace-tree generation token"
|
||||
assert "function bumpWorkspaceTreeGen(" in WORKSPACE, "missing bumpWorkspaceTreeGen helper"
|
||||
# switchToProfile bumps it unconditionally (NOT only inside the panel-gated skeleton call)
|
||||
body = _switch_body()
|
||||
assert "bumpWorkspaceTreeGen()" in body, (
|
||||
"switchToProfile must bump the workspace-tree generation at switch start"
|
||||
)
|
||||
bump_idx = body.index("bumpWorkspaceTreeGen()")
|
||||
wsskel_idx = body.index("showWorkspaceTreeSkeleton()")
|
||||
assert bump_idx < wsskel_idx, (
|
||||
"the unconditional bump must precede the panel-gated showWorkspaceTreeSkeleton"
|
||||
)
|
||||
# loadDir captures + re-checks the generation after BOTH awaited /api/list points
|
||||
ld = WORKSPACE[WORKSPACE.index("async function loadDir("):]
|
||||
ld = ld[: ld.index("\nfunction refreshWorkspacePanel(")]
|
||||
assert "const treeGen=_wsTreeGen" in ld, "loadDir must capture the tree generation at call time"
|
||||
assert ld.count("treeGen!==_wsTreeGen") >= 2, (
|
||||
"loadDir must re-check the tree generation after BOTH awaited /api/list points "
|
||||
"(root render + expanded-dirs prefetch) and discard stale renders"
|
||||
)
|
||||
|
||||
Reference in new issue
Block a user