Skip to content

fix(gateway): atomic JSONL transcript rewrite - #15085

Closed
simbam99 wants to merge 1 commit into
NousResearch:mainfrom
simbam99:fix/atomic-transcript-rewrite
Closed

fix(gateway): atomic JSONL transcript rewrite#15085
simbam99 wants to merge 1 commit into
NousResearch:mainfrom
simbam99:fix/atomic-transcript-rewrite

Conversation

@simbam99

Copy link
Copy Markdown
Contributor

Summary

SessionStore.rewrite_transcript overwrites the session's JSONL transcript in place:

with open(transcript_path, "w", encoding="utf-8") as f:
    for msg in messages:
        f.write(json.dumps(msg, ensure_ascii=False) + "\n")

A crash between the truncate and the final flush — SIGKILL, OOM, power loss, a failing disk, or an exception raised during JSON serialization — leaves the transcript empty or partially written. This is the exact failure mode already acknowledged on the read side by TestLoadTranscriptCorruptLines (GH-1193), which learned to skip corrupt lines after the fact. This PR closes the write-side hole so the corrupt state is never reachable in the first place.

Why this matters

rewrite_transcript sits on several hot gateway paths:

  • /retry — strips and replays the last assistant turn (gateway/run.py:5930)
  • /undo — rewrites history back to a prior user turn (gateway/run.py:5896)
  • /compress — persists the compressed history into a new session (gateway/run.py:7085)
  • Tool-result truncation (gateway/run.py:4425)

For sessions predating the SQLite layer, the JSONL is the only source of history — the existing comment at gateway/session.py:1207 explicitly says so: "legacy JSONL transcript (may contain more history than SQLite for sessions created before the DB layer was introduced)." A bad shutdown during any of the flows above silently wipes that session. Even on post-DB sessions, a corrupted JSONL defeats the replay fallback in load_transcript.

Changes

  1. utils.py — new atomic_jsonl_write(path, items, **dump_kwargs) helper. Mirrors the existing atomic_json_write / atomic_yaml_write pattern: tempfile + fsync + os.replace, with _preserve_file_mode / _restore_file_mode so Docker/NAS permission layouts from fix: preserve file permissions on atomic writes (Docker/NAS fix) #10618 are preserved, and BaseException cleanup so stray .tmp files are never left behind on KeyboardInterrupt / SystemExit.
  2. gateway/session.pySessionStore.rewrite_transcript now routes through atomic_jsonl_write. No behavior change on the happy path.

No API surface changes, no callers touched beyond the one in rewrite_transcript.

Alignment with recent merges

Same crash-safety pattern already shipped across the gateway:

  • fix: preserve file permissions on atomic writes (Docker/NAS fix) #10618 — preserve file permissions on atomic writes (Docker/NAS fix)
  • fix(gateway): make Telegram DM topic config writes atomic
  • fix(gateway/weixin): ensure atomic persistence for critical session state
  • fix(tui): atomic config persistence
  • ACP history persistence atomicity (acp_adapter/session.py + hermes_state.py)

rewrite_transcript was the obvious remaining gateway write that still truncated-in-place.

Reproduction

Before the fix:

from unittest.mock import patch

# Populate a transcript, then crash during rewrite.
store.rewrite_transcript(sid, [
    {"role": "user", "content": "hello"},
    {"role": "assistant", "content": "hi"},
])

with patch("utils.json.dumps", side_effect=IOError("disk full")):
    try:
        store.rewrite_transcript(sid, [{"role": "user", "content": "replace"}])
    except IOError:
        pass

# Without the fix: transcript is empty / truncated.
# With the fix:    original two messages are still intact.
assert store.load_transcript(sid) == [
    {"role": "user", "content": "hello"},
    {"role": "assistant", "content": "hi"},
]

Tests

  • Newtests/hermes_cli/test_atomic_jsonl_write.py: 14 cases covering crash safety (BaseException + mid-write IOError), tempfile cleanup on success and failure, unicode, generators, dump_kwargs forwarding, empty input, string path support.
  • Newtests/gateway/test_session.py::TestSessionStoreRewriteTranscriptAtomicity: 4 cases verifying that a crash during rewrite_transcript preserves the prior transcript, that no .tmp files are left behind on success or failure, and that unicode survives the round trip.
  • PreservedTestSessionStoreRewriteTranscript and TestLoadTranscriptCorruptLines remain green.

Local run:

py -3.11 -m pytest tests/gateway/test_session.py tests/hermes_cli/test_atomic_jsonl_write.py -q
→ 37 passed

Risk

Very low. atomic_jsonl_write is a drop-in for the old open("w") + write loop, and the helper matches the existing atomic_json_write / atomic_yaml_write style line-for-line. Behavior on the happy path is unchanged; only the failure window is closed.

Checklist

  • Bug fix with deterministic repro
  • No new dependencies
  • No API or config changes
  • Unit + regression tests covering both the helper and the caller
  • Matches existing atomic-write conventions in utils.py
  • Preserves Docker/NAS permission behavior from fix: preserve file permissions on atomic writes (Docker/NAS fix) #10618
  • No Windows-specific behavior; cross-platform by construction (uses the same primitives as atomic_json_write)

SessionStore.rewrite_transcript opened the transcript with mode="w"
and wrote JSON lines directly to the target path. A crash between
truncation and final flush left the transcript empty or partial,
losing conversation history for sessions whose SQLite DB layer is
absent (pre-DB sessions) and corrupting /retry, /undo, /compress,
and /reset flows during unclean shutdowns.

Add utils.atomic_jsonl_write — a tempfile + fsync + os.replace
helper mirroring atomic_json_write / atomic_yaml_write (including
_preserve_file_mode / _restore_file_mode for Docker/NAS parity) —
and route rewrite_transcript through it so the target file is
never observable in an intermediate state.

The existing read-side fallback for corrupt JSONL lines
(TestLoadTranscriptCorruptLines, NousResearchGH-1193) already confirmed this
failure mode; this closes the write-side hole.

Tests:
- tests/hermes_cli/test_atomic_jsonl_write.py covers the new helper
  (14 cases: crash-safety, cleanup, unicode, generators, dump kwargs)
- tests/gateway/test_session.py adds
  TestSessionStoreRewriteTranscriptAtomicity covering crash
  preservation and tempfile cleanup for the transcript rewrite path
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery labels Apr 24, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competing PRs for the same atomic transcript rewrite: #8065, #4985, #8077. Maintainers should pick one.

1 similar comment
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competing PRs for the same atomic transcript rewrite: #8065, #4985, #8077. Maintainers should pick one.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the careful crash-safety work. This is an automated hermes-sweeper review: current main has superseded the JSONL transcript path, so this PR's requested guarantee is already provided by the canonical SQLite implementation.

This behavior shipped in v2026.5.28.

@teknium1 teknium1 closed this Jul 12, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants