Skip to content

fix: clear stale saved session on 404 + structured api() errors - #1304

Closed
nesquena-hermes wants to merge 1 commit into
masterfrom
fix/stale-session-404-structured-errors
Closed

nesquena-hermes wants to merge 1 commit into
masterfrom
fix/stale-session-404-structured-errors

Conversation

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

fix: clear stale saved session on 404 + structured api() errors

Two coupled fixes for a real user-facing regression: when a saved session ID returns 404 (e.g. the session was deleted from another browser, or a state DB rotation removed it), the prior behavior was to show "Session not available in web UI." and stick there forever — every reload reproduced the broken state because the saved localStorage entry never got cleared.

Salvaged from PR #1084

The contributor PR #1084 (@GeoffBao) included this fix mixed with multiple unrelated changes. Lifted out as a focused PR.

What ships

1. static/workspace.js — structured api() errors

Previously api() threw a new Error(message) and discarded the HTTP context. Callers that wanted to branch on status (404 stale-session cleanup, 401 redirect, 503 retry) had to re-parse the message string.

Now it attaches .status, .statusText, and .body to the thrown error:

const err = new Error(message);
err.status = res.status;
err.statusText = res.statusText;
err.body = text;
throw err;

The 401 → /login redirect path is unchanged and still short-circuits before the throw.

2. static/sessions.js — stale-session 404 cleanup in loadSession()

When /api/session?id=X returns 404 for the currently-saved active session ID:

  1. Remove hermes-webui-session from localStorage
  2. Clear _loadingSessionId so the next session can load
  3. Rethrow the 404 so boot can fall through to the empty state

The cleanup is gated on !currentSid — if the user clicked into a session that subsequently 404s (a different code path), we don't wipe their saved active-session key.

if (e.status === 404) {
  _msgInner.innerHTML = '<div...>Session not available in web UI.</div>';
  if (!currentSid && localStorage.getItem('hermes-webui-session') === sid) {
    localStorage.removeItem('hermes-webui-session');
    if (_loadingSessionId === sid) _loadingSessionId = null;
    throw e;
  }
}

Tests

tests/test_stale_empty_session_restore.py — 3 new assertions:

Test Asserts
test_api_http_errors_preserve_response_status api() attaches .status/.statusText/.body to thrown errors
test_load_session_clears_saved_stale_404_and_rethrows_to_boot loadSession 404 branch clears localStorage, clears _loadingSessionId, rethrows
test_click_into_404_does_not_clear_saved_session The !currentSid gate prevents user-initiated clicks-into-missing sessions from wiping the saved key

tests/test_1038_pwa_auth_redirect.py::test_workspace_js_401_before_throw updated to accept either the old throw new Error(...) or the new throw err; pattern. The check that the 401 redirect comes before any throw is preserved.

Full suite: 3255 passed, 2 skipped, 3 xpassed, 0 failures.

What's intentionally NOT included from #1084

The other changes in #1084 ship as separate sibling PRs:

  • Sienna skin (the warm-palette part)
  • Session title quality improvements
  • Cmd/Ctrl+K works while busy

Each is independently scoped and reviewable.

Risk

Low. Adds context to thrown errors (additive — existing callers using e.message keep working). The localStorage cleanup is gated narrowly on !currentSid && saved === sid so it can't wipe state for active interactions.

Diff stats

 static/sessions.js                          |   8 +++
 static/workspace.js                         |  11 +++-
 tests/test_1038_pwa_auth_redirect.py        |  10 +++-
 tests/test_stale_empty_session_restore.py   |  79 ++++++++++++++++++++++++++++++++++
 4 files changed, 108 insertions(+), 4 deletions(-)

Closes part of #1084 once merged.

Two coupled fixes for the stale-empty-session regression:

1. api() in static/workspace.js now attaches HTTP context (.status,
   .statusText, .body) to thrown errors. Callers can branch on status
   without re-parsing the message string.

2. loadSession() in static/sessions.js: when a 404 comes back for the
   currently-saved active session ID, wipe the localStorage entry, clear
   the in-flight load marker, and rethrow so boot can fall through to
   the empty state. Previously the UI would stick on "Session not
   available in web UI." across reloads because the saved key never got
   removed. Click-into-404 (where the user clicked an existing list item
   that vanished) is unaffected — the cleanup is gated on !currentSid.

Tests: tests/test_stale_empty_session_restore.py — 3 new assertions:
  * api() attaches status/statusText/body to errors
  * loadSession clears saved-stale-404 + rethrows
  * Click-into-404 does NOT clear the saved session (gate on !currentSid)

Full suite: 3255 passed.

Salvaged from contributor work in PR #1084.

Co-authored-by: Hermes Agent <hermes@get-hermes.ai>

@nesquena nesquena left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Review — end-to-end ✅ (clean approve)

Two coupled, scope-tight UX fixes salvaged from external PR #1084 (@GeoffBao). When a user has a saved active-session ID in localStorage and that session 404s on the server (e.g. it was deleted from another browser, or a state-DB rotation removed it), the WebUI used to stick on "Session not available in web UI." across reloads because the saved key was never cleared. This PR makes the boot path self-heal.

What this ships

  • static/workspace.js api() — attaches .status / .statusText / .body to thrown errors so callers can branch on HTTP status without re-parsing the message string (static/workspace.js:14-26)
  • static/sessions.js loadSession() 404 handler — when the request was for the saved active-session ID (!currentSid && localStorage.getItem('hermes-webui-session') === sid), wipe the stale key, clear the in-flight load marker, and rethrow so boot can fall through to the fresh empty-state (static/sessions.js:336-355)
  • 3 new regression tests in tests/test_stale_empty_session_restore.py covering: status preservation on errors, 404-cleanup-and-rethrow, click-into-404 NOT clearing saved key
  • Update to tests/test_1038_pwa_auth_redirect.py — accepts both throw new Error(...) and throw err; patterns; the 401-before-throw invariant is preserved

End-to-end trace

Pre-fix path (the bug):

  1. Boot at static/boot.js:911 reads saved sid from localStorage
  2. await loadSession(saved) (static/boot.js:914)
  3. api('/api/session?...') 404s
  4. Old loadSession() swallowed the 404 — set the "Session not available" innerHTML and return-ed without throwing
  5. Boot's try block resolved successfully (no exception), S.session is null, localStorage.removeItem('hermes-webui-session') at static/boot.js:945 never runs (it's only in the catch)
  6. Stale key persists → next reload reproduces the bug

Post-fix path:

1-3. Same.
4. New api() throws err with err.status === 404
5. New loadSession() 404 handler:

  • Sets innerHTML to "Session not available"
  • Checks !currentSid && localStorage.getItem('hermes-webui-session') === sid (static/sessions.js:344)
  • Wipes localStorage, clears _loadingSessionId, rethrows e (static/sessions.js:347)
  1. Boot's catch(e){localStorage.removeItem(...)} at static/boot.js:945 runs (defensively redundant but harmless)
  2. Boot falls through to the no-saved-session empty-state at static/boot.js:947-959
  3. On the next reload there's no saved sid, so the empty state renders cleanly

Verified the gating logic

currentSid is captured at static/sessions.js:314 as S.session ? S.session.session_id : null. So:

  • Boot path (the bug): S.session is null → currentSid === null!currentSid is true → cleanup runs ✅
  • Click-into-404 path: user clicks a different session while another is active → S.session set → currentSid non-null → !currentSid is false → cleanup is skipped, saved active-session preserved ✅
  • Reload-same-stale-session path: same as boot path — gated condition holds ✅

The second guard localStorage.getItem('hermes-webui-session') === sid is belt-and-suspenders: even on a wonky boot where currentSid races to null but the in-flight sid wasn't the saved one, we don't wipe. ✅

Security audit

  • err.body = text — attaches raw response body to a JS Error object. The body is server-controlled and the error object is in-process; no PII exfil channel. Callers can introspect for branching but no logging change.
  • No new endpoints / no new routes / no auth changes — the 401 redirect short-circuit at static/workspace.js:15 still runs before the new throw block. Confirmed by the test_workspace_js_401_before_throw test still passing with the new throw err; pattern.
  • No XSS / no innerHTML user-data interpolation — the "Session not available" innerHTML is a static literal.
  • No retry behavior change — the catch-block check if(e.message&&/401/.test(e.message)) throw e; at static/workspace.js:36 still works because the new err.message is still set from the JSON error body. Network errors (TypeError) still retry.

Other audit — things that are correct

  • Existing callers using e.message keep working — the new throw still sets message exactly as before
  • _loadingSessionId cleared before throw (static/sessions.js:346) — prevents the in-flight marker from blocking subsequent loads after the rethrow
  • The post-cleanup throw lands in boot's already-existing catch — no caller changes required at static/boot.js:945
  • No agent / cross-tool surface touched — pure WebUI client-side fix; CLI never reads localStorage; agent never sees the saved-session key

Edge-case matrix

Scenario Expected Verified
Fresh boot, saved sid 404s Wipe localStorage, fall through to empty state ✅ trace
User clicks list item that 404s (active session present) Show "not available", DON'T wipe saved key !currentSid gate
User reloads same stale sid Wipe + clean empty state on next reload ✅ trace
Saved sid resolves successfully Normal flow ✅ unchanged
Network error (5xx, timeout) "Failed to load" toast, no wipe ✅ unchanged (else branch)
401 during api() Redirect to /login, no throw ✅ short-circuit preserved
Generic error from api() e.message, e.status, e.body all populated ✅ test
Caller does if (e.status === 404) Works without re-parsing message string ✅ contract test

Tests

  • tests/test_stale_empty_session_restore.py — 3/3 pass (new file)
  • tests/test_1038_pwa_auth_redirect.py — 12/12 pass (1 updated test)
  • Full suite: 3203 passed, 54 skipped, 3 xpassed, 0 failed in 15.22s (matches PR body's 3255-on-PR-runner once skip-set differences are accounted for)

Minor observations (non-blocking)

  • The "Session not available in web UI." DOM message remains briefly visible during the rethrow before boot's catch transitions to the empty state. This is fine — on the next reload the empty state renders cleanly because the localStorage key is now gone. A future polish could explicitly clear msgInner in boot's catch handler, but it's out of scope.
  • test_click_into_404_does_not_clear_saved_session asserts "!currentSid" in block — it verifies the gate token is present but not that it's the negation gating the clear. Acceptable for a regex-style assertion; behavioural confirmation is in the trace above.
  • api() retry logic still inspects e.message for /401/ (static/workspace.js:36) — could now use e.status === 401 directly, but that's a separate cleanup PR.

Recommendation

Approved. End-to-end trace verified the self-heal path; gating logic correctly distinguishes saved-session-load from click-into-load; no security regression; 401 path preserved; full suite green. Parked at approval — ready for the release agent's merge/tag pipeline.

nesquena-hermes added a commit that referenced this pull request Apr 30, 2026
release: v0.50.244

Batch release of 4 PRs:

- #1303 (@fecolinhares) — TTS playback of agent responses via Web Speech API.
  Per-message speaker button + auto-read toggle + voice/rate/pitch in
  Settings. localStorage-only state. Closes #499.

- #1304 — Stale saved session 404 cleanup + structured api() errors.
  Salvaged from #1084. Independently approved on 358275e.

- #1306 — Cmd/Ctrl+K works while a conversation is busy.
  Salvaged from #1084. Independently approved on 2e8a239.

- #1307 — Sienna skin (warm clay & sand earth palette).
  Salvaged from #1084. Independently approved on 5cd79c8.

Tests: 3290 passed, 2 skipped, 3 xpassed, 0 failures (was 3254; +36 tests).

Independently reviewed and approved by nesquena (commit 47f0e0d). End-to-end
trace verified the TTS flow; security audit confirmed SpeechSynthesisUtterance
is plain-text-only with no XSS surface; behavioural harness confirmed
_stripForTTS handles all 12 markdown-stripping cases; bounds clamping on
rate/pitch verified; opt-in behavior verified.
@nesquena-hermes

Copy link
Copy Markdown
Collaborator Author

Shipped in v0.50.244 🎉

Thanks @nesquena-hermes! Your work landed in the v0.50.244 batch release.

Closing — the change is on master and live in production. Appreciate the contribution!

@nesquena-hermes nesquena-hermes mentioned this pull request Apr 30, 2026
@nesquena-hermes
nesquena-hermes deleted the fix/stale-session-404-structured-errors branch April 30, 2026 04:37
SysAdminDoc pushed a commit to SysAdminDoc/hermes-webui that referenced this pull request Jun 26, 2026
release: v0.50.244

Batch release of 4 PRs:

- nesquena#1303 (@fecolinhares) — TTS playback of agent responses via Web Speech API.
  Per-message speaker button + auto-read toggle + voice/rate/pitch in
  Settings. localStorage-only state. Closes nesquena#499.

- nesquena#1304 — Stale saved session 404 cleanup + structured api() errors.
  Salvaged from nesquena#1084. Independently approved on 358275e.

- nesquena#1306 — Cmd/Ctrl+K works while a conversation is busy.
  Salvaged from nesquena#1084. Independently approved on 2e8a239.

- nesquena#1307 — Sienna skin (warm clay & sand earth palette).
  Salvaged from nesquena#1084. Independently approved on 5cd79c8.

Tests: 3290 passed, 2 skipped, 3 xpassed, 0 failures (was 3254; +36 tests).

Independently reviewed and approved by nesquena (commit 47f0e0d). End-to-end
trace verified the TTS flow; security audit confirmed SpeechSynthesisUtterance
is plain-text-only with no XSS surface; behavioural harness confirmed
_stripForTTS handles all 12 markdown-stripping cases; bounds clamping on
rate/pitch verified; opt-in behavior verified.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants