From da3932a7efce2a739800768609803e6a20d1dd11 Mon Sep 17 00:00:00 2001 From: Hermes Bot Date: Sun, 3 May 2026 20:41:00 +0000 Subject: [PATCH] fix(stage-284): absorb Opus advisor SHOULD-FIX items (5+6 LOC) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both flagged by pre-release Opus advisor; both clearly defensive and small enough to absorb in-release per the reviewer-flagged-fix-in-release-not-followup policy. SHOULD-FIX #1 (api/routes.py:_clear_stale_stream_state, ~25 LOC): After the metadata-only reload (#1559 Layer 2), the local 'session' variable is reassigned to the full-load object but the caller still holds the original metadata-only stub. /api/session then returns the stale active_stream_id at routes.py:1791, causing the frontend to attempt one ghost SSE reconnect before recovering. Fix: capture original_stub at function entry, then patch its in-memory active_stream_id and pending_* fields to None after both the early-return (full-load already cleared) path AND the successful-mutation path. Now the caller's read returns fresh state, no ghost reconnect. SHOULD-FIX #2 (api/models.py:Session.save, ~20 LOC): The .bak write at api/models.py:436 used write_text() which truncates- then-writes — a crash mid-write or concurrent backup-producing save could leave a torn .bak. Recovery defends correctly (JSONDecodeError → returns -1 → 'no_action'), so the failure mode was 'backup lost' not 'spurious restore'. Fix: tmp + os.replace pattern matching the main file write at line 446-453. Now backup either lands cleanly or doesn't land at all. 4026/4026 tests pass post-absorb. --- api/models.py | 22 ++++++++++++++++++++-- api/routes.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/api/models.py b/api/models.py index 1e33ad30..66253bbd 100644 --- a/api/models.py +++ b/api/models.py @@ -432,11 +432,29 @@ class Session: incoming_msg_count = len(self.messages or []) if existing_msg_count > incoming_msg_count: bak_path = self.path.with_suffix('.json.bak') + # SHOULD-FIX #2 (Opus): atomic write via tmp+replace, + # mirroring the main save() pattern below. Prevents a + # torn .bak from a crash mid-write or a concurrent + # backup-producing save. Recovery defends against a + # torn .bak (JSONDecodeError → no_action), so the + # failure mode pre-fix was "backup is lost"; with + # this fix the backup either lands cleanly or doesn't + # land at all. try: - bak_path.write_text(existing_text, encoding='utf-8') + bak_tmp = bak_path.with_suffix( + f'.bak.tmp.{os.getpid()}.{threading.current_thread().ident}' + ) + with open(bak_tmp, 'w', encoding='utf-8') as bf: + bf.write(existing_text) + bf.flush() + os.fsync(bf.fileno()) + os.replace(bak_tmp, bak_path) except OSError: # Backup is best-effort; main save proceeds regardless. - pass + try: + bak_tmp.unlink(missing_ok=True) + except Exception: + pass except OSError: pass diff --git a/api/routes.py b/api/routes.py index 1b813411..0d1ab915 100644 --- a/api/routes.py +++ b/api/routes.py @@ -346,6 +346,10 @@ def _clear_stale_stream_state(session) -> bool: # full session before touching persisted state. The original # metadata-only object is left untouched so the caller's read path is # unaffected. + original_stub = session # SHOULD-FIX #1 (Opus): keep reference so we can + # patch the caller's in-memory copy after a + # successful clear, avoiding one ghost SSE + # reconnect on the very next /api/session GET. if getattr(session, "_loaded_metadata_only", False): try: from api.models import get_session as _get_session @@ -366,6 +370,20 @@ def _clear_stale_stream_state(session) -> bool: # The full-load path may have already repaired stale pending fields # via _repair_stale_pending(); only re-assert if still set. if not getattr(session, "active_stream_id", None): + # Patch the caller's stub so its read path also sees the cleared + # field (matches the Opus SHOULD-FIX #1 — without this, /api/session + # would briefly return the stale active_stream_id and the frontend + # would attempt one ghost SSE reconnect before recovering). + try: + original_stub.active_stream_id = None + if hasattr(original_stub, "pending_user_message"): + original_stub.pending_user_message = None + if hasattr(original_stub, "pending_attachments"): + original_stub.pending_attachments = [] + if hasattr(original_stub, "pending_started_at"): + original_stub.pending_started_at = None + except Exception: + pass return False # ── #1533 race fix: acquire the per-session lock and re-read @@ -389,6 +407,19 @@ def _clear_stale_stream_state(session) -> bool: "_clear_stale_stream_state: save() failed for session %s", getattr(session, "session_id", "?"), ) + # Patch the caller's stub (if different from the full-load object) so + # its in-memory active_stream_id matches what just got persisted. + if original_stub is not session: + try: + original_stub.active_stream_id = None + if hasattr(original_stub, "pending_user_message"): + original_stub.pending_user_message = None + if hasattr(original_stub, "pending_attachments"): + original_stub.pending_attachments = [] + if hasattr(original_stub, "pending_started_at"): + original_stub.pending_started_at = None + except Exception: + pass return True # ── CSRF: validate Origin/Referer on POST ────────────────────────────────────