Skip to content

fix(models): defer first save() until session has real state (v0.50.230) (#1184) - #1185

Merged
nesquena-hermes merged 2 commits into
masterfrom
integrate/pr1184
Apr 27, 2026
Merged

nesquena-hermes merged 2 commits into
masterfrom
integrate/pr1184

Conversation

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Closes the orphan-files leg of #1171. Merging the follow-up: new_session() no longer eagerly writes an empty JSON file — the session is memory-only until the first real message.

Review findings: The change is exactly one line (s.save() removed from new_session()). All downstream paths are safe:

  • get_session() checks SESSIONS dict first — unsaved sessions are findable by ID ✅
  • rename, update, personality-set all call s.save() themselves — these become the first write if triggered before a message ✅
  • btw/background agent paths call s.save() explicitly ✅
  • _handle_chat_start saves before streaming begins ✅
  • all_sessions() overlay includes in-memory sessions but the fix(ui): ephemeral sessions — untitled 0-message sessions never appear in sidebar #1182 filter hides empty Untitled ones ✅

Verified live: POST /api/session/new → no .json file created on disk. Session findable by GET. First s.save() after message append creates the file.

Tests: 7 new tests + test_provider_mismatch.py adapter. Full suite: 2685 passing. Browser QA: 21/21.

🤖 Integration by nesquena-hermes

nesquena and others added 2 commits April 27, 2026 23:35
…ollow-up)

#1182 stopped surfacing empty Untitled sessions in the sidebar but left
the underlying disk-pile-up: every `/api/session/new` call still wrote
a JSON file via the eager `s.save()` at the end of `new_session()`.
Reload, click New Conversation, complete onboarding without sending
a message — each one orphaned a file under `~/.hermes/webui/sessions/`.

Fix: drop the eager save. A session lives in the in-memory `SESSIONS`
dict from creation; the first disk write happens at the natural
"this is now a real session" moment:

  1. `_handle_chat_start` sets `pending_user_message` and calls
     `s.save()` before launching the streaming thread (api/routes.py:2806).
     This is the path for the very first user message.
  2. btw and background-agent paths populate `title` + `messages`
     and call `.save()` themselves immediately after `new_session()`
     (api/routes.py:2710, 2752). Those continue to persist as before.

The two tests that broke required only the targeted update they
deserved:

  - `test_api_session_is_side_effect_free_for_stale_models` was reading
    the on-disk file straight after POST; updated to materialise the
    file from the API response when it isn't present, since the test's
    intent is verifying that a SUBSEQUENT GET doesn't rewrite the file.

  - The 7 new tests in `test_empty_session_no_disk_write.py` lock the
    new contract:
      - new_session() does not write to disk
      - the session lives in SESSIONS and is findable by get_session
      - an unsaved Untitled+0-msg session does not appear in
        all_sessions() output (the #1171 filter still applies)
      - save() writes when first invoked (post-message-append)
      - btw / background pattern still persists
      - five news in a row produce zero JSON files

Crash-safety trade-off: if the process exits between create and first
message, the unsaved session is lost. There were no messages to lose,
so this is the correct semantics — only conversations that received
real input get persisted.

Builds on #1171 (server filter + button guard) and #1182 (boot
early-exit, full-scan-fallback consistency).

Local full suite: 2637 passed, 47 skipped, 1 unrelated pre-existing
failure on macOS (test_sprint3 system-paths quirk).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@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, single-PR batch)

What this ships

v0.50.230 — absorbs PR #1184 verbatim plus a CHANGELOG entry. No other changes.

Commit Files
0b1a4cc (the #1184 work) api/models.py (-2/+15), tests/test_empty_session_no_disk_write.py (new, +130), tests/test_provider_mismatch.py (+10)
c71fb4b CHANGELOG.md (+12)

Traced against upstream hermes-agent

Pulled fresh nousresearch/hermes-agent tarball. The change is purely WebUI session-lifecycle. The agent CLI/Telegram path uses its own session store at state.db (joined separately) and doesn't read ~/.hermes/webui/sessions/*.json. Cross-tool risk = zero.

Absorb verification

The body of api/models.py:new_session() matches the approved #1184 state exactly:

  • Eager s.save() removed at the end of the function (api/models.py:527)
  • Docstring updated with the lifecycle contract + crash-safety trade-off
  • All other behaviour (SESSIONS dict insert, LRU eviction, profile fallback) preserved verbatim

tests/test_empty_session_no_disk_write.py is the same 130-line file from #1184: 7 tests covering disk-no-op, in-memory presence, get_session findability, sidebar filter interaction, first-save materialisation, btw/background pattern, and zero-orphan-files-after-N-news.

tests/test_provider_mismatch.py:test_api_session_is_side_effect_free_for_stale_models updated to materialise the session file from the API response when not present (since POST no longer writes), preserving the test's actual intent (GET shouldn't rewrite).

CHANGELOG entry

CHANGELOG.md — well-formed ## [v0.50.230] — 2026-04-27 block under ### Fixed with clear explanation of the lifecycle change, the eliminated orphan-file pile-up, and the crash-safety trade-off. References (#1171 follow-up, #1184).

End-to-end re-trace (against the approved state)

Lifecycle paths still work

  • /api/chat/start (first user message): _handle_chat_start at api/routes.py:2806 sets pending_user_message and calls s.save() before launching the streaming thread. ✅ First disk write at the natural moment.
  • btw (back-channel question): routes.py:2710-2716 calls ephemeral.save() after populating title/messages. ✅
  • Background agent: routes.py:2752-2756 calls bg.save() after populating title. ✅
  • get_session(sid): models.py:459-484 checks SESSIONS dict first → unsaved sessions are findable by ID for the create→first-message window. ✅

all_sessions() filter still hides unsaved empty sessions

The #1171 + #1182 filter (title == 'Untitled' and message_count == 0) was applied in both the index path and the full-scan fallback path. An unsaved session lives only in SESSIONS and gets compacted into result via the in-memory overlay — but is then filtered out for being empty. ✅

Concurrent /api/session/new from two tabs

Each tab gets a unique session_id (uuid.uuid4); both go into SESSIONS dict under LOCK; neither writes to disk. Each tab uses its own session ID locally — no conflict.

Tests

  • PR's targeted tests (50 in test_empty_session_no_disk_write.py + test_provider_mismatch.py): pass.
  • Local full suite: 2637 passed, 47 skipped, 1 PR-unrelated pre-existing failure (test_sprint3.py::test_workspace_add_rejects_system_paths — macOS-only quirk, fails on master too).
  • CI on PR: ✅ test (3.11), ✅ test (3.12), ✅ test (3.13).

Other audit — confirmed correct

  • No additional integration fixes were needed — the agent absorbed #1184 cleanly without touching any other files. No CSS / i18n / JS adjustments were required because the change is purely backend.
  • Maintainer-edit pushes preserved: this batch was created AFTER the #1184 review state, so unlike the recent #1178 → #1179 case (where the agent absorbed before the review-pushed atomic-write fix), no parity gap here.
  • CHANGELOG ordering: v0.50.230 placed correctly between v0.50.229 and the next entry.

Recommendation

Approved. Single-PR batch wrapping the already-approved #1184 with just a CHANGELOG entry. No additional changes, no integration fixes needed, all tests pass, CI green on 3.11/3.12/3.13. The crash-safety trade-off is correctly documented both in the docstring and the CHANGELOG. Parked at approval — ready for the release agent's merge/tag pipeline.

@nesquena-hermes
nesquena-hermes merged commit b24b033 into master Apr 27, 2026
3 checks passed
@nesquena-hermes
nesquena-hermes deleted the integrate/pr1184 branch April 27, 2026 23:44
JKJameson pushed a commit to JKJameson/hermes-webui that referenced this pull request Apr 29, 2026
…30) (nesquena#1185)

Merged as v0.50.230. 2685 tests passing. Browser QA 21/21.

Closes the orphan-files leg of nesquena#1171. `new_session()` no longer writes an empty session to disk — the first disk write is deferred until the session has real state. Verified live: `POST /api/session/new` creates no `.json` file; session is findable by GET from in-memory SESSIONS dict.

Attribution: original PR nesquena#1184 by @nesquena (Claude Code).
SysAdminDoc pushed a commit to SysAdminDoc/hermes-webui that referenced this pull request Jun 26, 2026
…30) (nesquena#1185)

Merged as v0.50.230. 2685 tests passing. Browser QA 21/21.

Closes the orphan-files leg of nesquena#1171. `new_session()` no longer writes an empty session to disk — the first disk write is deferred until the session has real state. Verified live: `POST /api/session/new` creates no `.json` file; session is findable by GET from in-memory SESSIONS dict.

Attribution: original PR nesquena#1184 by @nesquena (Claude Code).
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