fix(stage-284): absorb Opus advisor SHOULD-FIX items (5+6 LOC)
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.
This commit is contained in:
1 parent
029a349304
commit
da3932a7ef
2 files changed
+51
-2
No files matched your search
+20
-2
@@ -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
|
||||
|
||||
|
||||
@@ -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 ────────────────────────────────────
|
||||
|
||||
Reference in new issue
Block a user