From 8558432c1f87078d06da7c2c6b05ec8737a47af6 Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Wed, 26 Aug 2026 02:00:04 +0000 Subject: [PATCH 1/7] fix(session): fence sidecar writes by durable revision --- ARCHITECTURE.md | 55 + api/models.py | 1266 +++++++++-- api/routes.py | 215 +- api/session_discoverability.py | 148 +- api/session_recovery.py | 191 +- api/streaming.py | 151 +- .../webui-run-state-consistency-contract.md | 41 + tests/test_cancelled_turn_status.py | 24 +- ...test_compression_snapshot_runtime_clear.py | 16 +- tests/test_file_manager_external_session.py | 7 +- tests/test_gateway_sync.py | 29 +- tests/test_issue2057_worktree_lifecycle.py | 145 +- .../test_issue3987_imported_session_titles.py | 22 +- tests/test_issue765_streaming_persistence.py | 47 +- tests/test_metadata_save_wipe_1558.py | 28 +- .../test_pr1341_context_window_persistence.py | 31 +- tests/test_provider_mismatch.py | 12 + tests/test_regressions.py | 61 +- .../test_session_db_sidecar_reconciliation.py | 245 +- tests/test_session_discoverability_repair.py | 145 ++ tests/test_session_index.py | 6 + .../test_session_lost_response_regression.py | 61 + .../test_session_save_empty_pending_guard.py | 5 +- tests/test_session_sidecar_revision_fence.py | 2009 +++++++++++++++++ tests/test_sprint23.py | 23 +- tests/test_webui_state_db_reconciliation.py | 42 +- tests/test_workspace_stale_recovery.py | 321 ++- 27 files changed, 4868 insertions(+), 478 deletions(-) create mode 100644 tests/test_session_sidecar_revision_fence.py diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index ffd46b40386..ff341f72709 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -260,6 +260,61 @@ usable timestamp and rows at or after the sidecar tail also append normally. The fallback therefore preserves an accepted state-only row when exact ordering is ambiguous, while safely placeable recovery rows remain chronological. +#### Session sidecar publication authority + +Every compliant writer of `SESSION_DIR/.json` participates in the same +per-SID thread and cross-process authority. This includes normal `Session.save()` +writes, backup recovery, and discoverability repairs. Existing sidecars are +fenced by their generation plus exact digest; first publication is create-or-fail, +so a stale alias or repair cannot overwrite a sidecar that appeared concurrently. +Out-of-band replacements increment `_sidecar_generation_v1` and invalidate cached +aliases before later saves can proceed. + +When a State DB self-heal saves through a freshly loaded owner, the caller's +cached alias inherits the new revision only if it owned the exact pre-save +revision. Otherwise the self-heal returns and installs the freshly saved owner; +it never grants a stale alias authority over unrelated unsaved fields. + +State DB sidecar materialization treats its initial scan only as candidate +discovery and re-reads the complete authoritative row after acquiring the SID +authority. That targeted reread uses one explicit SQLite read transaction for +session metadata and ordered messages. Its private temporary file is flushed +before create-only publication. Hidden background-session cleanup follows agent +lock then SID authority, records a durable delete tombstone before unlinking, +invalidates cached aliases, removes recoverable backups, attempts State DB +cleanup, and fsyncs the session directory. + +Deleted-WebUI-session tombstone updates are serialized by a global cross-process +authority in a lock-path namespace that no accepted session SID can alias. It is +acquired only after any SID authority. Manual delete, hidden-background cleanup, +and empty-session cleanup share one artifact-removal helper. Empty-session cleanup +reads and validates the embedded SID, payload, and exact revision while holding +the SID authority, then rechecks that revision immediately before deletion. +Tombstone publication flushes the file and parent directory before sidecar +deletion can start; a failure leaves that candidate uncounted. Primary, backup, +or archive unlink failure also fails closed before State DB cleanup or cleanup +success. A successful delete verifies those files are absent and fsyncs the +session directory again. + +Sidecar, primary-backup, and incomparable-backup archive publications flush the +file before atomic publication and fsync the parent directory on POSIX. Native +Windows keeps atomic publication and file flushing, but Python does not expose an +equivalent directory-fsync guarantee here; the final directory entry is therefore +not guaranteed across sudden power loss on native Windows. POSIX ignores only +filesystem-declared unsupported directory-fsync errors (`EINVAL`/`ENOTSUP`); +permission and I/O failures propagate. + +Hidden ephemeral (`/btw`) sessions use the same deletion protocol on both +cancelled and normally completed turns. Callers keep the canonical lock order +(agent lock, then SID authority), validate the session ID, canonical sidecar +path, embedded payload, and exact owned revision, and then invoke the shared +artifact-removal helper. The durable tombstone is published before removing the +primary, `.json.bak`, incomparable-backup archives, or session-owned replay-v10 +backup/manifest/temporary artifacts. Cleanup remains best-effort for the SSE +response, but protocol failures are warning-logged and never fall back to a raw +sidecar unlink; a successful normal cleanup clears transient in-memory stream +fields so final recovery does not attempt to recreate the deleted sidecar. + #### Imported `state.db` sidebar projection `api.models.get_cli_sessions()` projects conversations from the active Hermes diff --git a/api/models.py b/api/models.py index 00f1931a4cb..8dcd4f2cf7a 100644 --- a/api/models.py +++ b/api/models.py @@ -3,6 +3,7 @@ import contextvars import copy import datetime +import errno import hashlib import inspect import json @@ -13,6 +14,7 @@ import threading import time import uuid +import weakref from contextlib import closing, contextmanager from dataclasses import dataclass from pathlib import Path @@ -253,10 +255,570 @@ def _safe_replace(src: Path, dst: Path) -> None: # Serializes index writers so concurrent Session.save() calls cannot race on # stale baselines while still allowing LOCK to be released before disk I/O. _INDEX_WRITE_LOCK = threading.RLock() +_SESSION_SAVE_AUTHORITIES_LOCK = threading.Lock() +_SESSION_SAVE_AUTHORITIES: "weakref.WeakValueDictionary[str, threading.RLock]" = weakref.WeakValueDictionary() _SESSION_INDEX_REBUILD_LOCK = threading.Lock() _SESSION_INDEX_REBUILD_THREAD = None _SESSION_INDEX_REBUILD_THREAD_TARGET: tuple[Path, Path] | None = None + +def _session_save_authority(session_id: str) -> threading.RLock: + """Return the process-wide reentrant save authority for one session ID.""" + with _SESSION_SAVE_AUTHORITIES_LOCK: + authority = _SESSION_SAVE_AUTHORITIES.get(session_id) + if authority is None: + authority = threading.RLock() + _SESSION_SAVE_AUTHORITIES[session_id] = authority + return authority + + +class StaleSessionGenerationError(RuntimeError): + """A sidecar changed after this Session observed its generation.""" + + +@dataclass(frozen=True) +class SidecarRevision: + sid: str + state: str + generation: int + digest_sha256: str | None + + @classmethod + def absent(cls, sid: str) -> "SidecarRevision": + return cls(sid=sid, state="ABSENT", generation=0, digest_sha256=None) + + +def _sidecar_revision_record(revision: SidecarRevision) -> dict: + """Return a JSON-compatible token for storage on Session.__dict__.""" + return { + "sid": revision.sid, + "state": revision.state, + "generation": revision.generation, + "digest_sha256": revision.digest_sha256, + } + + +def _coerce_sidecar_revision(value, sid: str) -> SidecarRevision | None: + if isinstance(value, SidecarRevision): + return value if value.sid == sid else None + if not isinstance(value, dict) or value.get("sid") != sid: + return None + state = value.get("state") + generation = value.get("generation") + digest = value.get("digest_sha256") + if ( + not isinstance(state, str) + or type(generation) is not int + or (digest is not None and not isinstance(digest, str)) + ): + return None + return SidecarRevision( + sid=sid, + state=state, + generation=generation, + digest_sha256=digest, + ) + + +def _sidecar_revision_from_bytes( + sid: str, + raw: bytes, + *, + parsed=None, +) -> SidecarRevision: + if parsed is None: + try: + parsed = json.loads(raw) + except (json.JSONDecodeError, UnicodeDecodeError, ValueError): + parsed = None + raw_generation = ( + parsed.get("_sidecar_generation_v1") + if isinstance(parsed, dict) + else None + ) + generation = ( + raw_generation + if type(raw_generation) is int and raw_generation >= 0 + else 0 + ) + return SidecarRevision( + sid=sid, + state="PRESENT", + generation=generation, + digest_sha256=hashlib.sha256(raw).hexdigest(), + ) + + +def _read_sidecar_revision(path: Path, sid: str | None = None) -> SidecarRevision: + resolved_sid = sid or path.stem + try: + raw = path.read_bytes() + except FileNotFoundError: + return SidecarRevision.absent(resolved_sid) + return _sidecar_revision_from_bytes(resolved_sid, raw) + + +def _read_sidecar_snapshot(path: Path, sid: str) -> tuple[SidecarRevision, dict]: + """Read one validated payload and its exact durable revision.""" + raw = path.read_bytes() + payload = json.loads(raw) + if not isinstance(payload, dict): + raise ValueError(f"Session sidecar for {sid!r} is not an object") + if payload.get("session_id") != sid: + raise ValueError(f"Session sidecar for {sid!r} has a foreign embedded SID") + return _sidecar_revision_from_bytes(sid, raw, parsed=payload), payload + + +def _fsync_sidecar_directory(directory: Path) -> None: + """Persist a sidecar directory entry where POSIX supports directory fsync. + + Native Windows Python does not expose an equivalent directory handle here; + file contents remain flushed and publication remains atomic, but power-loss + durability of the directory entry is a POSIX-only guarantee. + """ + if os.name == "nt": + return + flags = os.O_RDONLY | getattr(os, "O_DIRECTORY", 0) + fd = os.open(Path(directory), flags) + try: + try: + os.fsync(fd) + except OSError as exc: + if exc.errno not in {errno.EINVAL, errno.ENOTSUP}: + raise + finally: + os.close(fd) + + +def _publish_sidecar_no_replace(source: Path, destination: Path) -> None: + """Publish *source* only when *destination* is still absent. + + Hard links provide an atomic create-only primitive on POSIX. Native Windows + rename is also create-only. If neither primitive is available, fail closed: + opening the destination with ``O_EXCL`` before copying would expose a + partial sidecar to lock-free readers. + """ + try: + os.link(str(source), str(destination)) + _fsync_sidecar_directory(destination.parent) + return + except FileExistsError: + raise + except OSError: + if os.name != "nt": + raise + os.rename(source, destination) + _fsync_sidecar_directory(destination.parent) + + +_BACKUP_SNAPSHOT_DOMINANCE_MAX_FIELDS = 512 +_BACKUP_SNAPSHOT_DOMINANCE_MAX_LIST_ROWS = 200_000 +_BACKUP_SNAPSHOT_DOMINANCE_MAX_CANONICAL_BYTES = 64 * 1024 * 1024 +_BACKUP_SNAPSHOT_DOMINANCE_MAX_DEPTH = 64 +_BACKUP_SNAPSHOT_DOMINANCE_MAX_ITEMS = 1_000_000 + + +class _SnapshotDominanceBudgetExceeded(ValueError): + """Canonical snapshot comparison exceeded its fail-closed work budget.""" + + +class _SnapshotDominanceBudget: + def __init__(self, max_canonical_bytes: int): + self.remaining_bytes = max_canonical_bytes + self.remaining_rows = _BACKUP_SNAPSHOT_DOMINANCE_MAX_LIST_ROWS + self.remaining_items = _BACKUP_SNAPSHOT_DOMINANCE_MAX_ITEMS + self._active_containers = set() + + def consume_rows(self, count: int) -> None: + if count > self.remaining_rows: + raise _SnapshotDominanceBudgetExceeded + self.remaining_rows -= count + + def _consume_items(self, count: int) -> None: + if count > self.remaining_items: + raise _SnapshotDominanceBudgetExceeded + self.remaining_items -= count + + @staticmethod + def _add_size(total: int, amount: int, limit: int) -> int: + if amount > limit - total: + raise _SnapshotDominanceBudgetExceeded + return total + amount + + def _measure_string(self, value: str, limit: int) -> int: + size = len(value) + 2 # one UTF-8 byte minimum per codepoint, plus quotes + if size > limit: + raise _SnapshotDominanceBudgetExceeded + if value.isascii(): + for match in re.finditer(r'["\\\x00-\x1f]', value): + codepoint = ord(match.group()) + size = self._add_size( + size, + 1 if codepoint in {8, 9, 10, 12, 13, 34, 92} else 5, + limit, + ) + return size + + size = 2 + for char in value: + codepoint = ord(char) + if char in {'"', '\\'} or char in {'\b', '\f', '\n', '\r', '\t'}: + encoded_size = 2 + elif codepoint < 0x20: + encoded_size = 6 + elif codepoint < 0x80: + encoded_size = 1 + elif codepoint < 0x800: + encoded_size = 2 + elif 0xD800 <= codepoint <= 0xDFFF: + raise UnicodeError("surrogate is not valid UTF-8 JSON") + elif codepoint < 0x10000: + encoded_size = 3 + else: + encoded_size = 4 + size = self._add_size(size, encoded_size, limit) + return size + + def _measure_json(self, value, *, depth: int, limit: int) -> int: + if depth > _BACKUP_SNAPSHOT_DOMINANCE_MAX_DEPTH: + raise _SnapshotDominanceBudgetExceeded + + value_type = type(value) + if value_type is str: + return self._measure_string(value, limit) + if value is None: + if limit < 4: + raise _SnapshotDominanceBudgetExceeded + return 4 + if value_type is bool: + size = 4 if value else 5 + if size > limit: + raise _SnapshotDominanceBudgetExceeded + return size + if value_type is int: + bit_count = abs(value).bit_length() + digit_upper_bound = max(1, (bit_count * 30103 + 99_999) // 100_000) + size = digit_upper_bound + int(value < 0) + if size > limit: + raise _SnapshotDominanceBudgetExceeded + return size + if value_type is float: + # CPython's JSON float spellings are bounded by the longest finite + # repr (24 ASCII bytes); NaN and infinities are shorter. + if limit < 24: + raise _SnapshotDominanceBudgetExceeded + return 24 + if value_type not in {list, dict}: + raise TypeError("snapshot value is not canonical JSON") + + container_id = id(value) + if container_id in self._active_containers: + raise ValueError("circular snapshot value") + self._active_containers.add(container_id) + try: + self._consume_items(len(value)) + size = 2 # [] or {} + if size > limit: + raise _SnapshotDominanceBudgetExceeded + if value_type is list: + for index, item in enumerate(value): + if index: + size = self._add_size(size, 1, limit) + item_size = self._measure_json( + item, + depth=depth + 1, + limit=limit - size, + ) + size = self._add_size(size, item_size, limit) + return size + + for index, (key, item) in enumerate(value.items()): + if type(key) is not str: + raise TypeError("snapshot object key is not a string") + if index: + size = self._add_size(size, 1, limit) + key_size = self._measure_string(key, limit - size) + size = self._add_size(size, key_size, limit) + size = self._add_size(size, 1, limit) # colon + item_size = self._measure_json( + item, + depth=depth + 1, + limit=limit - size, + ) + size = self._add_size(size, item_size, limit) + return size + finally: + self._active_containers.remove(container_id) + + def consume_json(self, value) -> str: + encoded_size = self._measure_json( + value, + depth=0, + limit=self.remaining_bytes, + ) + self.remaining_bytes -= encoded_size + return json.dumps( + value, + ensure_ascii=False, + sort_keys=True, + separators=(",", ":"), + ) + + +def _ordered_json_rows_cover(candidate, baseline, *, _budget=None) -> bool: + """Return whether baseline is an ordered canonical-row subsequence.""" + if type(candidate) is not list or type(baseline) is not list: + return False + try: + budget = _budget or _SnapshotDominanceBudget( + _BACKUP_SNAPSHOT_DOMINANCE_MAX_CANONICAL_BYTES + ) + budget.consume_rows(len(candidate) + len(baseline)) + candidate_rows = iter(budget.consume_json(row) for row in candidate) + baseline_rows = (budget.consume_json(row) for row in baseline) + return all( + any(candidate_row == baseline_row for candidate_row in candidate_rows) + for baseline_row in baseline_rows + ) + except ( + MemoryError, + OverflowError, + RecursionError, + TypeError, + UnicodeError, + ValueError, + ): + return False + + +_BACKUP_SNAPSHOT_BOOKKEEPING_FIELDS = frozenset({ + "_sidecar_generation_v1", + "message_count", + "updated_at", +}) + + +def _session_snapshot_covers( + candidate, + baseline, + *, + max_canonical_bytes=_BACKUP_SNAPSHOT_DOMINANCE_MAX_CANONICAL_BYTES, +) -> bool: + """Return whether candidate covers the complete recoverable baseline. + + Top-level lists use ordered canonical-row coverage; every other durable + field must remain canonically equal. Storage generation, derived count, + and the superseded activity timestamp are bookkeeping rather than recovery + content. Missing fields in legacy baselines are therefore compatible, + while an unknown baseline field can never be silently discarded. + + Comparison work is capped. Oversized, deeply nested, or non-canonical + payloads are incomparable and therefore take the archive/keep path. + """ + if type(candidate) is not dict or type(baseline) is not dict: + return False + if ( + type(max_canonical_bytes) is not int + or max_canonical_bytes <= 0 + or len(candidate) > _BACKUP_SNAPSHOT_DOMINANCE_MAX_FIELDS + or len(baseline) > _BACKUP_SNAPSHOT_DOMINANCE_MAX_FIELDS + ): + return False + try: + if any(type(key) is not str for key in candidate) or any( + type(key) is not str for key in baseline + ): + return False + budget = _SnapshotDominanceBudget(max_canonical_bytes) + for key, baseline_value in baseline.items(): + if key in _BACKUP_SNAPSHOT_BOOKKEEPING_FIELDS: + continue + if key not in candidate: + return False + candidate_value = candidate[key] + if isinstance(baseline_value, list): + if not _ordered_json_rows_cover( + candidate_value, + baseline_value, + _budget=budget, + ): + return False + continue + if budget.consume_json(candidate_value) != budget.consume_json( + baseline_value + ): + return False + except ( + MemoryError, + OverflowError, + RecursionError, + TypeError, + UnicodeError, + ValueError, + ): + return False + return True + + +def _archive_incomparable_backup( + session_id: str, + backup_path: Path, + backup_receipt: SidecarRevision, +) -> Path: + """Preserve an incomparable primary backup before promoting a newer one.""" + if ( + not isinstance(backup_receipt, SidecarRevision) + or backup_receipt.state != "PRESENT" + or not backup_receipt.digest_sha256 + ): + raise RuntimeError( + f"Cannot archive unverified recoverable backup for {session_id!r}" + ) + archive_path = backup_path.with_name( + f"{backup_path.name}.archive-{backup_receipt.digest_sha256}" + ) + if not archive_path.exists(): + archive_tmp = backup_path.parent / ( + f".sidecar-archive-{uuid.uuid4().hex}.tmp" + ) + try: + digest = hashlib.sha256() + with open(backup_path, "rb") as source, open(archive_tmp, "xb") as target: + while chunk := source.read(1024 * 1024): + digest.update(chunk) + target.write(chunk) + target.flush() + os.fsync(target.fileno()) + if digest.hexdigest() != backup_receipt.digest_sha256: + raise RuntimeError( + f"Recoverable backup changed while archiving {session_id!r}" + ) + _safe_replace(archive_tmp, archive_path) + _fsync_sidecar_directory(archive_path.parent) + finally: + archive_tmp.unlink(missing_ok=True) + archived_receipt = _read_sidecar_revision(archive_path, session_id) + if archived_receipt != backup_receipt: + raise RuntimeError( + f"Archived recoverable backup failed verification for {session_id!r}" + ) + return archive_path + + +def _retire_backup_if_owned( + session_id: str, + backup_path: Path, + backup_receipt, + live_receipt, +) -> bool: + """Delete a backup only while both save receipts still own the generation.""" + if not isinstance(backup_receipt, SidecarRevision): + return False + if not isinstance(live_receipt, SidecarRevision): + return False + with _session_sidecar_authority( + session_id, + session_dir=backup_path.parent, + ): + live_path = backup_path.with_suffix("") + if _read_sidecar_revision(live_path, session_id) != live_receipt: + return False + if _read_sidecar_revision(backup_path, session_id) != backup_receipt: + return False + backup_path.unlink(missing_ok=True) + for archive_path in backup_path.parent.glob( + f"{backup_path.name}.archive-*" + ): + archive_path.unlink(missing_ok=True) + return not backup_path.exists() + + +def _invalidate_cached_session_generation(session_id: str) -> None: + """Evict and fence an alias after an out-of-band sidecar replacement.""" + with LOCK: + cached = SESSIONS.pop(session_id, None) + if cached is not None: + cached._sidecar_revisions[session_id] = _sidecar_revision_record( + SidecarRevision( + sid=session_id, + state="INVALIDATED", + generation=-1, + digest_sha256=None, + ) + ) + + +@contextmanager +def _cross_process_sidecar_file_authority( + lock_id: str, + *, + session_dir: Path | None = None, + lock_domain: str = "session", +): + """Serialize one sidecar-store mutation key across threads/processes.""" + if not is_safe_session_id(lock_id): + raise ValueError(f"Unsafe sidecar lock id {lock_id!r}") + if lock_domain == "session": + thread_lock_id = lock_id + lock_dir = (session_dir or SESSION_DIR) / ".sidecar-locks" + elif lock_domain == "deleted-session-tombstone": + # Domain separation is mandatory: ``lock_id`` is also a valid session + # SID, so sharing the session lock path would self-deadlock when that + # SID saves while clearing the global tombstone set. + thread_lock_id = f"deleted-session-tombstone:{lock_id}" + lock_dir = ( + (session_dir or SESSION_DIR) + / ".sidecar-locks" + / ".deleted-session-tombstone" + ) + else: + raise ValueError(f"Unknown sidecar lock domain {lock_domain!r}") + thread_authority = _session_save_authority(thread_lock_id) + with thread_authority: + lock_dir.mkdir(parents=True, exist_ok=True) + lock_path = lock_dir / f"{lock_id}.lock" + fd = os.open(lock_path, os.O_CREAT | os.O_RDWR, 0o600) + with os.fdopen(fd, "r+b", buffering=0) as lock_file: + if _fcntl is not None: + _fcntl.flock(lock_file.fileno(), _fcntl.LOCK_EX) + try: + yield + finally: + _fcntl.flock(lock_file.fileno(), _fcntl.LOCK_UN) + return + if _msvcrt is not None: # pragma: no cover - Windows only. + if os.fstat(lock_file.fileno()).st_size == 0: + lock_file.write(b"\0") + lock_file.seek(0) + _msvcrt.locking( # type: ignore[attr-defined] + lock_file.fileno(), _msvcrt.LK_LOCK, 1 # type: ignore[attr-defined] + ) + try: + yield + finally: + lock_file.seek(0) + _msvcrt.locking( # type: ignore[attr-defined] + lock_file.fileno(), _msvcrt.LK_UNLCK, 1 # type: ignore[attr-defined] + ) + return + raise RuntimeError("cross-process sidecar locking is unavailable") + + +@contextmanager +def _session_sidecar_authority( + session_id: str, + *, + session_dir: Path | None = None, +): + """Serialize compliant sidecar writers for one SID across processes.""" + if not is_safe_session_id(session_id): + raise ValueError(f"Unsafe session_id {session_id!r}") + with _cross_process_sidecar_file_authority( + session_id, + session_dir=session_dir, + ): + yield + # Serializes ``_record_webui_zero_message_orphan_tombstone`` / # ``_clear_webui_zero_message_orphan_tombstone`` so two concurrent sidebar # polls (or a poll racing ``Session.save`` / ``new_session`` / @@ -269,7 +831,22 @@ def _safe_replace(src: Path, dst: Path) -> None: # WebUI sidebar polling path is single-process) but must wrap the WHOLE # load-modify-write/unlink sequence in both helpers. _WEBUI_ZERO_MESSAGE_ORPHAN_TOMBSTONE_LOCK = threading.Lock() -_WEBUI_DELETED_SESSION_TOMBSTONE_LOCK = threading.Lock() +_WEBUI_DELETED_SESSION_TOMBSTONE_LOCK_SID = "_webui_deleted_sessions_global" + + +@contextmanager +def _webui_deleted_session_tombstone_authority(): + """Serialize the deleted-session tombstone RMW across processes. + + Lock order is an optional per-session sidecar authority first, then this + global tombstone authority. No path may acquire a SID authority while this + global authority is held. + """ + with _cross_process_sidecar_file_authority( + _WEBUI_DELETED_SESSION_TOMBSTONE_LOCK_SID, + lock_domain="deleted-session-tombstone", + ): + yield # Path-safety contract for session IDs. Accept alphanumerics, underscore, and # hyphen so API/gateway-issued ids (``api-*``, ``reachy-voice-*``) round-trip @@ -816,15 +1393,32 @@ def _load_webui_deleted_session_tombstone() -> frozenset[str]: ) -def _save_webui_deleted_session_tombstone(ids) -> None: - try: - sorted_ids = sorted(set( - str(sid).strip() for sid in (ids or []) if str(sid or "").strip() - )) - except TypeError: - return +def _save_webui_deleted_session_tombstone( + ids, + *, + required_sid: str | None = None, +) -> None: + sorted_ids = sorted(set( + str(sid).strip() for sid in (ids or []) if str(sid or "").strip() + )) + required_sid = str(required_sid or "").strip() + if required_sid and required_sid not in sorted_ids: + raise ValueError("required deleted-session tombstone SID is missing") if len(sorted_ids) > WEBUI_DELETED_SESSION_TOMBSTONE_CAP: - sorted_ids = sorted_ids[-WEBUI_DELETED_SESSION_TOMBSTONE_CAP:] + if required_sid: + if WEBUI_DELETED_SESSION_TOMBSTONE_CAP < 1: + raise RuntimeError("deleted-session tombstone cannot retain required SID") + other_ids = [candidate for candidate in sorted_ids if candidate != required_sid] + retained_other_count = WEBUI_DELETED_SESSION_TOMBSTONE_CAP - 1 + sorted_ids = ( + other_ids[-retained_other_count:] + if retained_other_count + else [] + ) + sorted_ids.append(required_sid) + sorted_ids.sort() + else: + sorted_ids = sorted_ids[-WEBUI_DELETED_SESSION_TOMBSTONE_CAP:] payload = { "version": WEBUI_DELETED_SESSION_TOMBSTONE_VERSION, "ids": sorted_ids, @@ -841,6 +1435,7 @@ def _save_webui_deleted_session_tombstone(ids) -> None: f.flush() os.fsync(f.fileno()) os.replace(_tmp, p) + _fsync_sidecar_directory(p.parent) except Exception: logger.debug("Failed to save webui deleted-session tombstone", exc_info=True) if _tmp is not None: @@ -848,25 +1443,31 @@ def _save_webui_deleted_session_tombstone(ids) -> None: _tmp.unlink(missing_ok=True) except Exception: pass + raise def _record_webui_deleted_session_tombstone(sid: str) -> None: sid = str(sid or "").strip() if not sid: return - with _WEBUI_DELETED_SESSION_TOMBSTONE_LOCK: + with _webui_deleted_session_tombstone_authority(): current = set(_load_webui_deleted_session_tombstone()) if sid in current: - return - current.add(sid) - _save_webui_deleted_session_tombstone(current) + _fsync_sidecar_directory(SESSION_DIR) + else: + current.add(sid) + _save_webui_deleted_session_tombstone(current, required_sid=sid) + if sid not in _load_webui_deleted_session_tombstone(): + raise RuntimeError( + f"Deleted-session tombstone did not retain current SID {sid!r}" + ) def _clear_webui_deleted_session_tombstone(sid: str) -> None: sid = str(sid or "").strip() if not sid: return - with _WEBUI_DELETED_SESSION_TOMBSTONE_LOCK: + with _webui_deleted_session_tombstone_authority(): current = set(_load_webui_deleted_session_tombstone()) if sid not in current: return @@ -880,6 +1481,93 @@ def _clear_webui_deleted_session_tombstone(sid: str) -> None: logger.debug("Failed to remove empty webui deleted-session tombstone", exc_info=True) +class SessionDeleteTombstoneError(RuntimeError): + """A sidecar delete could not durably fence stale writers.""" + + +def _offline_replay_artifact_paths(sidecar: Path) -> list[Path]: + """Return replay-v10 recovery artifacts owned by one session sidecar.""" + sidecar = Path(sidecar) + name = sidecar.name + + def is_owned(candidate_name: str) -> bool: + return bool( + ( + candidate_name.startswith(f"{name}.replay-v10.") + and candidate_name.endswith(".bak") + ) + or ( + candidate_name.startswith(f"_replay-v10.{name}.") + and candidate_name.endswith(".manifest.json") + ) + or candidate_name.startswith(f".{name}.replay-v10.tmp.") + or candidate_name.startswith(f".{name}.replay-v10.restore.") + or ( + candidate_name.startswith(f"._replay-v10.{name}.") + and ".manifest.json.tmp." in candidate_name + ) + ) + + return [candidate for candidate in sidecar.parent.iterdir() if is_owned(candidate.name)] + + +def _delete_session_sidecar_artifacts_locked( + sid: str, + *, + session_dir: Path | None = None, + expected_revision: SidecarRevision | None = None, + record_tombstone: bool = True, +) -> bool: + """Delete one SID's sidecar artifacts while its authority is held. + + Callers must hold ``_session_sidecar_authority(sid)``. When supplied, the + expected revision is rechecked immediately before the durable tombstone and + unlinks; a mismatch returns False without changing any state. + """ + if not is_safe_session_id(sid): + raise ValueError(f"Unsafe session_id {sid!r}") + directory = Path(session_dir or SESSION_DIR) + sidecar = directory / f"{sid}.json" + if expected_revision is not None: + if not isinstance(expected_revision, SidecarRevision): + return False + if _read_sidecar_revision(sidecar, sid) != expected_revision: + return False + if record_tombstone: + try: + _record_webui_deleted_session_tombstone(sid) + except Exception as exc: + raise SessionDeleteTombstoneError( + f"Failed to durably tombstone deleted WebUI session {sid!r}" + ) from exc + + _invalidate_cached_session_generation(sid) + backup = sidecar.with_suffix(".json.bak") + archived_backups = list(directory.glob(f"{sidecar.name}.bak.archive-*")) + replay_artifacts = _offline_replay_artifact_paths(sidecar) + failures = [] + for artifact in (sidecar, backup, *archived_backups, *replay_artifacts): + try: + artifact.unlink(missing_ok=True) + except Exception as exc: + failures.append(exc) + + remaining = [ + artifact + for artifact in (sidecar, backup, *archived_backups, *replay_artifacts) + if artifact.exists() + ] + remaining.extend(directory.glob(f"{sidecar.name}.bak.archive-*")) + remaining.extend(_offline_replay_artifact_paths(sidecar)) + _fsync_sidecar_directory(directory) + if failures or remaining: + cause = failures[0] if failures else None + raise RuntimeError( + f"Failed to delete required sidecar artifacts for {sid!r}" + ) from cause + return True + + def _content_has_reasoning_only_parts(content) -> bool: if not isinstance(content, list) or not content: return False @@ -1554,24 +2242,57 @@ def __init__(self, session_id: str=None, title: str='Untitled', except (TypeError, ValueError): parsed_message_count = None self._metadata_message_count = parsed_message_count if parsed_message_count is not None and parsed_message_count >= 0 else None + # Revisions are scoped by durable SID because compression can rotate a + # live Session object between parent and continuation identities. + self._sidecar_revisions = { + self.session_id: _sidecar_revision_record( + SidecarRevision.absent(self.session_id) + ) + } @property def path(self): return SESSION_DIR / f'{self.session_id}.json' - def save(self, touch_updated_at: bool = True, skip_index: bool = False) -> None: + def save(self, touch_updated_at: bool = True, skip_index: bool = False): + with _session_sidecar_authority(self.session_id): + return self._save_owned_generation( + touch_updated_at=touch_updated_at, + skip_index=skip_index, + ) + + def _save_if_sidecar_unchanged( + self, + expected_revision, + *, + touch_updated_at: bool = True, + skip_index: bool = False, + ) -> bool: + """Persist only when the exact durable sidecar revision still matches.""" + if not isinstance(expected_revision, SidecarRevision): + return False + with _session_sidecar_authority(self.session_id): + if _read_sidecar_revision(self.path, self.session_id) != expected_revision: + return False + self._sidecar_revisions[self.session_id] = _sidecar_revision_record( + expected_revision + ) + self._save_owned_generation( + touch_updated_at=touch_updated_at, + skip_index=skip_index, + ) + return True + + def _save_owned_generation( + self, + touch_updated_at: bool = True, + skip_index: bool = False, + ): if not is_safe_session_id(self.session_id): raise ValueError(f"Unsafe session_id {self.session_id!r}; refusing to write outside session store") - # ── #1558 P0 guard ────────────────────────────────────────────── - # Refuse to save a session that was loaded with metadata_only=True. - # Such sessions have messages=[] (it's the whole point of the partial - # load), and save() unconditionally writes self.messages to disk via - # an atomic os.replace(). Saving a metadata-only stub thus wipes the - # full conversation history — which is exactly the v0.50.279 - # _clear_stale_stream_state() regression that lost users 1000+ - # message conversations. Any caller that needs to mutate persisted - # fields on a metadata-only session must reload with - # metadata_only=False first. + # Reject intrinsically unsafe partial objects before checking ownership. + # A stale metadata-only alias must still report the stronger data-loss + # contract instead of looking like an ordinary retryable CAS conflict. if getattr(self, '_loaded_metadata_only', False): raise RuntimeError( f"Refusing to save metadata-only session {self.session_id!r}: " @@ -1579,6 +2300,34 @@ def save(self, touch_updated_at: bool = True, skip_index: bool = False) -> None: f"Reload with metadata_only=False before mutating state. " f"See #1558." ) + expected_record = self._sidecar_revisions.get(self.session_id) + expected_revision = ( + SidecarRevision.absent(self.session_id) + if expected_record is None + else _coerce_sidecar_revision(expected_record, self.session_id) + ) + if expected_revision is None: + raise StaleSessionGenerationError( + f"Invalid session generation token for {self.session_id!r}; reload before saving" + ) + current_revision = _read_sidecar_revision(self.path, self.session_id) + if current_revision != expected_revision: + with LOCK: + if SESSIONS.get(self.session_id) is self: + SESSIONS.pop(self.session_id, None) + self._sidecar_revisions[self.session_id] = _sidecar_revision_record( + SidecarRevision( + sid=self.session_id, + state="INVALIDATED", + generation=-1, + digest_sha256=None, + ) + ) + raise StaleSessionGenerationError( + f"Stale session generation for {self.session_id!r}; reload before saving" + ) + next_sidecar_generation = current_revision.generation + 1 + # ── #1558 P0 guard ────────────────────────────────────────────── if touch_updated_at: self.updated_at = time.time() self._webui_pending_user_timestamp_identity = ( @@ -1621,6 +2370,7 @@ def save(self, touch_updated_at: bool = True, skip_index: bool = False) -> None: 'gateway_run', ] meta = {k: getattr(self, k, None) for k in METADATA_FIELDS} + meta['_sidecar_generation_v1'] = next_sidecar_generation # #5854: message_count and a compact anchor-scene fingerprint go in the # metadata prefix (BEFORE messages) so load_metadata_only() and the # sidebar-poll freshness check never have to parse the full (250-480KB) @@ -1668,6 +2418,7 @@ def save(self, touch_updated_at: bool = True, skip_index: bool = False) -> None: # The recovery path is api/session_recovery.py — at server startup and # via /api/session/recover, sessions whose JSON has fewer messages than # their .bak get restored automatically. + backup_receipt = None try: if self.path.exists(): # The on-disk count, without reading the body. @@ -1700,7 +2451,7 @@ def save(self, touch_updated_at: bool = True, skip_index: bool = False) -> None: # array next to it), a corrupt or truncated prefix, a file with # no top-level `messages` key at all, or metadata alone that # overflows the budget. - # Fail-open is the contract -- never "assume no shrink". + # Unknown counts fall back to the full parse; never assume no shrink. existing_text = None existing_msg_count = _prefix_message_count(self.path) if existing_msg_count is None: @@ -1724,46 +2475,120 @@ def save(self, touch_updated_at: bool = True, skip_index: bool = False) -> None: incoming_msg_count, self.active_stream_id, ) - return + return None if existing_msg_count > incoming_msg_count: bak_path = self.path.with_suffix('.json.bak') if existing_text is None: - # The .bak body is the one thing that needs the full text, - # and a shrink is the one time it is needed. + # Only shrink paths need the full snapshot for dominance. existing_text = self.path.read_text(encoding='utf-8') - # 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_tmp = bak_path.with_suffix( - f'.bak.tmp.{os.getpid()}.{threading.current_thread().ident}' + existing = json.loads(existing_text) + except (json.JSONDecodeError, ValueError): + existing = None + replace_backup = not bak_path.exists() + if bak_path.exists(): + current_backup_receipt = _read_sidecar_revision( + bak_path, + self.session_id, ) - with open(bak_tmp, 'w', encoding='utf-8') as bf: - bf.write(existing_text) - bf.flush() - os.fsync(bf.fileno()) - _safe_replace(bak_tmp, bak_path) - except OSError: - # Backup is best-effort; main save proceeds regardless. try: - bak_tmp.unlink(missing_ok=True) + backup = json.loads(bak_path.read_text(encoding='utf-8')) + if not isinstance(backup, dict): + raise ValueError("backup payload is not a session snapshot") + if not isinstance(backup.get('messages'), list): + raise ValueError("backup payload is not a session snapshot") + if not isinstance(existing.get('messages'), list): + raise ValueError("live payload is not a session snapshot") + except (json.JSONDecodeError, UnicodeDecodeError, ValueError): + _archive_incomparable_backup( + self.session_id, + bak_path, + current_backup_receipt, + ) + replace_backup = True + else: + if backup.get('session_id') != self.session_id: + _archive_incomparable_backup( + self.session_id, + bak_path, + current_backup_receipt, + ) + replace_backup = True + else: + existing_covers_backup = _session_snapshot_covers( + existing, + backup, + ) + backup_covers_existing = _session_snapshot_covers( + backup, + existing, + ) + if backup_covers_existing: + backup_receipt = current_backup_receipt + elif existing_covers_backup: + replace_backup = True + else: + _archive_incomparable_backup( + self.session_id, + bak_path, + current_backup_receipt, + ) + replace_backup = True + bak_tmp = None + try: + if replace_backup: + 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()) + _safe_replace(bak_tmp, bak_path) + _fsync_sidecar_directory(bak_path.parent) + backup_receipt = _read_sidecar_revision( + bak_path, + self.session_id, + ) + except OSError as exc: + try: + if bak_tmp is not None: + bak_tmp.unlink(missing_ok=True) except Exception: pass - except OSError: - pass + raise RuntimeError( + f"Failed to publish recoverable backup for {self.session_id!r}" + ) from exc + except OSError as exc: + raise RuntimeError( + f"Failed to inspect recoverable backup state for {self.session_id!r}" + ) from exc tmp = self.path.with_suffix(f'.tmp.{os.getpid()}.{threading.current_thread().ident}') try: - with open(tmp, 'w', encoding='utf-8') as f: + with open(tmp, 'w', encoding='utf-8', newline='\n') as f: f.write(payload) f.flush() os.fsync(f.fileno()) - _safe_replace(tmp, self.path) + if expected_revision.state == "ABSENT": + try: + _publish_sidecar_no_replace(tmp, self.path) + except FileExistsError as exc: + raise StaleSessionGenerationError( + f"Stale session generation for {self.session_id!r}; " + "sidecar appeared during first save" + ) from exc + tmp.unlink(missing_ok=True) + else: + _safe_replace(tmp, self.path) + _fsync_sidecar_directory(self.path.parent) + self._sidecar_revisions[self.session_id] = _sidecar_revision_record( + _sidecar_revision_from_bytes( + self.session_id, + payload.encode('utf-8'), + parsed={"_sidecar_generation_v1": next_sidecar_generation}, + ) + ) except Exception: try: tmp.unlink(missing_ok=True) @@ -1796,6 +2621,7 @@ def save(self, touch_updated_at: bool = True, skip_index: bool = False) -> None: self.session_id, exc_info=True, ) + return backup_receipt @classmethod def load(cls, sid): @@ -1807,22 +2633,55 @@ def load(cls, sid): p = SESSION_DIR / f'{sid}.json' if not p.exists(): return None - # #5854: snapshot the stat signature BEFORE reading so a legacy-facts - # cache write is only committed if the file didn't change under us - # during the parse (TOCTOU guard against an atomic replace mid-read). - _pre_read_sig = _sidecar_stat_signature(p) - data = json.loads(p.read_text(encoding='utf-8')) - data['messages'], _collapsed_partials = _collapse_adjacent_duplicate_partials(data.get('messages')) - session = cls(**data) - if _collapsed_partials: - try: - # Self-heal bloated sessions on first full load without touching - # recency/index ordering; save() creates a .bak because this - # intentionally shrinks the transcript (#2592). - session.save(touch_updated_at=False, skip_index=True) - except Exception: - logger.debug("Failed to persist collapsed duplicate partials for %s", sid, exc_info=True) - else: + for attempt in range(2): + # #5854: snapshot the stat signature BEFORE reading so a legacy-facts + # cache write is only committed if the file didn't change under us + # during the parse (TOCTOU guard against an atomic replace mid-read). + _pre_read_sig = _sidecar_stat_signature(p) + raw = p.read_bytes() + data = json.loads(raw) + pre_read_revision = _sidecar_revision_from_bytes( + sid, + raw, + parsed=data, + ) + data['messages'], collapsed_partials = _collapse_adjacent_duplicate_partials( + data.get('messages') + ) + session = cls(**data) + session._sidecar_revisions[sid] = _sidecar_revision_record( + pre_read_revision + ) + if collapsed_partials: + try: + repaired = session._save_if_sidecar_unchanged( + pre_read_revision, + touch_updated_at=False, + skip_index=True, + ) + except Exception: + logger.debug( + "Failed to persist collapsed duplicate partials for %s", + sid, + exc_info=True, + ) + repaired = False + if repaired: + return session + if attempt == 0: + continue + raise RuntimeError( + f"Session {sid!r} changed during duplicate-partial repair" + ) + + post_read_revision = _read_sidecar_revision(p, sid) + if post_read_revision != pre_read_revision: + if attempt == 0: + continue + raise RuntimeError(f"Session {sid!r} changed while loading") + session._sidecar_revisions[sid] = _sidecar_revision_record( + post_read_revision + ) # #5854: for a LEGACY sidecar (no modern anchor_scene_index key), the # cheap metadata-prefix read cannot recover message_count/scenes when # scenes serialize before them, so cache the authoritative facts we @@ -1831,8 +2690,8 @@ def load(cls, sid): # Keyed by stat signature, so any edit invalidates it; the next # save() rewrites the modern layout and the fallback stops firing. # expected_sig guards against an atomic replace during the read. - # (When _collapsed_partials fired, save() above already rewrote the - # modern layout, so no legacy caching is needed.) + # (When collapsed_partials fired, the fenced save above already + # returned, so no legacy caching is needed.) # # WHY STILL GATED, and not "obviously" broadened to modern files: the # cache is only READ from the two sites that reach it when the cheap @@ -1855,7 +2714,8 @@ def load(cls, sid): ) except Exception: logger.debug("legacy sidecar facts cache populate failed for %s", sid, exc_info=True) - return session + return session + return None @classmethod def load_metadata_only(cls, sid, *, index_message_counts=None): @@ -4289,7 +5149,7 @@ def _repair_stale_pending(session) -> bool: return False -def _sync_sidecar_from_state_db_if_newer(session) -> bool: +def _sync_sidecar_from_state_db_if_newer(session) -> bool | Session: """Read-side self-heal when WebUI sidecar lags Hermes state.db. A WebUI stream can lose its terminal ``done``/``stream_end`` path while the @@ -4499,6 +5359,19 @@ def _sync_sidecar_from_state_db_if_newer(session) -> bool: state_messages=state_messages, ) + locked_pre_revision = _coerce_sidecar_revision( + locked._sidecar_revisions.get(sid), + sid, + ) + alias_pre_revision = _coerce_sidecar_revision( + session._sidecar_revisions.get(sid), + sid, + ) + alias_owned_pre_save_revision = ( + locked_pre_revision is not None + and alias_pre_revision == locked_pre_revision + ) + # Mutate + persist the freshly-loaded, locked object. Because we hold the # lock and reloaded under it, this save cannot clobber a concurrent # writer's newer record. @@ -4518,22 +5391,38 @@ def _sync_sidecar_from_state_db_if_newer(session) -> bool: ) return False - # Durable write succeeded — reflect the reconciled state on the caller's - # shared/cached object so the in-flight read returns the recovered data. - session.messages = merged_messages - session.context_messages = merged_context - session.active_stream_id = None - session.pending_user_message = None - session.pending_attachments = [] - session.pending_started_at = None - session.pending_user_source = None + locked_post_revision = _coerce_sidecar_revision( + locked._sidecar_revisions.get(sid), + sid, + ) + if locked_post_revision is None: + logger.error( + "Session %s: state.db sync saved without a durable revision owner", + sid, + ) + return False + + # Only an alias that owned the exact pre-save revision may inherit the + # post-save authority. Otherwise return the freshly-saved owner so the + # caller replaces the stale alias instead of blessing unsaved fields. + if alias_owned_pre_save_revision: + session.messages = merged_messages + session.context_messages = merged_context + session.active_stream_id = None + session.pending_user_message = None + session.pending_attachments = [] + session.pending_started_at = None + session.pending_user_source = None + session._sidecar_revisions[sid] = _sidecar_revision_record( + locked_post_revision + ) logger.info( "Session %s: synced sidecar from newer state.db transcript (%d -> %d messages)", sid, locked_count, len(merged_messages), ) - return True + return True if alias_owned_pre_save_revision else locked finally: lock.release() @@ -4917,6 +5806,44 @@ def _cached_session_lags_disk(cached) -> bool: sid = getattr(cached, 'session_id', None) if not sid: return False + expected_revision = _coerce_sidecar_revision( + getattr(cached, '_sidecar_revisions', {}).get(sid), + sid, + ) + if expected_revision is not None: + path = SESSION_DIR / f'{sid}.json' + if expected_revision.state == "ABSENT": + if not path.exists(): + return False + # A generation-less legacy/cache alias cannot prove ownership of a + # sidecar that appeared later. Preserve the historical directional + # freshness checks below: reload only when disk is demonstrably + # ahead, and let save() enforce the strict ABSENT-vs-PRESENT CAS. + elif expected_revision.state != "PRESENT": + return True + if expected_revision.state == "PRESENT": + disk_prefix = _persisted_session_meta_prefix(sid) + if disk_prefix is None: + if not path.exists(): + return True + else: + raw_generation = disk_prefix.get('_sidecar_generation_v1') + disk_generation = ( + raw_generation + if type(raw_generation) is int and raw_generation >= 0 + else 0 + ) + if disk_generation != expected_revision.generation: + return True + if expected_revision.generation > 0: + # Modern compliant writers advance the prefix generation for + # metadata and transcript changes alike. Parity therefore + # avoids hashing/parsing the potentially huge sidecar on every + # cache hit and preserves any newer unsaved in-memory tail. + return False + # Generation-zero legacy files can still be changed by older writers + # without a revision bump. Fall through to the directional count/scene + # checks below for compatibility. cached_count = len(getattr(cached, 'messages', None) or []) # Fast path: prefix read of just the metadata header. disk_count = _persisted_message_count(sid) @@ -5467,7 +6394,15 @@ def _resolve_session_once( ) if not metadata_only: try: - _sync_sidecar_from_state_db_if_newer(cached) + sync_result = _sync_sidecar_from_state_db_if_newer(cached) + if isinstance(sync_result, Session): + with LOCK: + current = SESSIONS.get(sid) + if current is cached or current is None: + SESSIONS[sid] = sync_result + cached = sync_result + else: + cached = current except Exception: logger.debug( "state.db newer-sidecar sync failed on cache hit for session %s", sid, exc_info=True, @@ -5490,7 +6425,17 @@ def _resolve_session_once( _evict_sessions_over_cap() # #4765: safe LRU eviction (never active/unsaved) if not metadata_only: try: - synced_from_state = _sync_sidecar_from_state_db_if_newer(s) + sync_result = _sync_sidecar_from_state_db_if_newer(s) + synced_from_state = bool(sync_result) + if isinstance(sync_result, Session): + stale_loaded = s + s = sync_result + if cache_on_miss: + with LOCK: + if SESSIONS.get(sid) is stale_loaded: + SESSIONS[sid] = s + if promote_cache: + SESSIONS.move_to_end(sid) repaired = False if synced_from_state else _repair_stale_pending(s) # If the stale-pending repair did not fire but the session # already carries a pending-journal-retry marker (e.g. set on @@ -5757,14 +6702,11 @@ def new_session(workspace=None, model=None, profile=None, model_provider=None, p worktree_created_at=wt.get('created_at') if wt else None, enabled_toolsets=enabled_toolsets, ) - # #4985: defensive — auto-generated uuids don't collide with the - # tombstone, but if a future caller ever passes an explicit id that - # was previously pruned, clear the entry so the new session isn't - # shadowed on the next poll. Wrapped because a tombstone failure - # must never block new-session creation. + # #4985: clear the zero-message orphan tombstone defensively. The deleted- + # session tombstone is cleared only by a successful ``Session.save`` while + # the per-SID authority is held, so it cannot race a verified delete. try: _clear_webui_zero_message_orphan_tombstone(s.session_id) - _clear_webui_deleted_session_tombstone(s.session_id) except Exception: logger.debug( "Failed to clear webui tombstone for %s", @@ -6314,63 +7256,121 @@ def persist_recovered_workspace_binding( path = SESSION_DIR / f"{sid}.json" lock = _get_session_agent_lock(sid) with lock: - if not path.exists(): - # Recovery only repairs an existing WebUI sidecar. Creating a new - # sidecar here can resurrect a session that was deleted after the - # recovery decision but before this lock was acquired. - raise WorkspaceBindingPersistenceError( - "Failed to persist recovered workspace: session sidecar is missing" - ) - - try: - payload = json.loads(path.read_text(encoding="utf-8")) - except Exception as exc: - raise WorkspaceBindingPersistenceError( - "Failed to persist recovered workspace: unreadable session sidecar" - ) from exc - if not isinstance(payload, dict): - raise WorkspaceBindingPersistenceError( - "Failed to persist recovered workspace: invalid session sidecar" - ) - current = str(payload.get("workspace") or "") - if current != resolved: - if current != expected: + with _session_sidecar_authority(sid): + revision = _read_sidecar_revision(path, sid) + if revision.state != "PRESENT": raise WorkspaceBindingPersistenceError( - "Failed to persist recovered workspace: session workspace changed" + "Failed to persist recovered workspace: session sidecar is missing" ) - payload["workspace"] = resolved - tmp = path.with_suffix( - f".tmp.{os.getpid()}.{threading.current_thread().ident}" - ) + revision_before = revision + try: - with open(tmp, "w", encoding="utf-8") as handle: - json.dump(payload, handle, ensure_ascii=False, indent=2) - handle.flush() - os.fsync(handle.fileno()) - _safe_replace(tmp, path) + payload = json.loads(path.read_text(encoding="utf-8")) except Exception as exc: - try: - tmp.unlink(missing_ok=True) - except Exception: - pass raise WorkspaceBindingPersistenceError( - "Failed to persist recovered workspace" + "Failed to persist recovered workspace: unreadable session sidecar" ) from exc + if not isinstance(payload, dict): + raise WorkspaceBindingPersistenceError( + "Failed to persist recovered workspace: invalid session sidecar" + ) + current = str(payload.get("workspace") or "") + if current != resolved: + if current != expected: + raise WorkspaceBindingPersistenceError( + "Failed to persist recovered workspace: session workspace changed" + ) + payload["workspace"] = resolved + payload["_sidecar_generation_v1"] = revision.generation + 1 + serialized = json.dumps(payload, ensure_ascii=False, indent=2) + tmp = path.with_suffix( + f".tmp.{os.getpid()}.{threading.current_thread().ident}" + ) + try: + with open(tmp, "w", encoding="utf-8", newline="\n") as handle: + handle.write(serialized) + handle.flush() + os.fsync(handle.fileno()) + if _read_sidecar_revision(path, sid) != revision: + raise StaleSessionGenerationError( + f"Session {sid!r} changed during workspace recovery" + ) + _safe_replace(tmp, path) + revision = _sidecar_revision_from_bytes( + sid, + serialized.encode("utf-8"), + parsed=payload, + ) + except Exception as exc: + try: + tmp.unlink(missing_ok=True) + except Exception: + pass + raise WorkspaceBindingPersistenceError( + "Failed to persist recovered workspace" + ) from exc - session.workspace = resolved + session_expected = _coerce_sidecar_revision( + getattr(session, "_sidecar_revisions", {}).get(sid), + sid, + ) + session_owned = session_expected == revision_before + if session_owned: + session.workspace = resolved + session._sidecar_revisions[sid] = _sidecar_revision_record(revision) + elif hasattr(session, "_sidecar_revisions"): + session._sidecar_revisions[sid] = _sidecar_revision_record( + SidecarRevision( + sid=sid, + state="INVALIDATED", + generation=-1, + digest_sha256=None, + ) + ) + cached_owned = False with LOCK: cached = SESSIONS.get(sid) if cached is not None: - cached.workspace = resolved + cached_expected = _coerce_sidecar_revision( + getattr(cached, "_sidecar_revisions", {}).get(sid), + sid, + ) + cached_owned = cached_expected == revision_before + if cached_owned: + cached.workspace = resolved + cached._sidecar_revisions[sid] = _sidecar_revision_record( + revision + ) + else: + SESSIONS.pop(sid, None) + if hasattr(cached, "_sidecar_revisions"): + cached._sidecar_revisions[sid] = _sidecar_revision_record( + SidecarRevision( + sid=sid, + state="INVALIDATED", + generation=-1, + digest_sha256=None, + ) + ) + result = cached if cached_owned else (session if session_owned else None) + if result is None: + result = Session.load(sid) + if result is None: + raise WorkspaceBindingPersistenceError( + "Failed to reload session after workspace recovery" + ) + with LOCK: + SESSIONS[sid] = result + SESSIONS.move_to_end(sid) try: - _write_session_index(updates=[cached or session]) + _write_session_index(updates=[result]) except Exception: logger.debug( "Failed to refresh session index after workspace recovery for %s", sid, exc_info=True, ) - return cached or session + return result def get_session_for_file_ops(sid: str): @@ -7500,13 +8500,11 @@ def import_cli_session( parent_session_id=parent_session_id, ) # #4985: import_cli_session uses an explicit sid (the CLI sidecar's id). - # If that sid was previously tombstoned as a webui zero-message orphan, - # clear the tombstone entry so the freshly-imported session is visible - # on the next poll. Wrapped because a tombstone failure must never block - # an import. + # Clear only the zero-message orphan tombstone here. The deleted-session + # tombstone is cleared by ``Session.save`` after durable publication while + # the per-SID authority is held, so it cannot race a verified delete. try: _clear_webui_zero_message_orphan_tombstone(s.session_id) - _clear_webui_deleted_session_tombstone(s.session_id) except Exception: logger.debug( "Failed to clear webui tombstone for %s", diff --git a/api/routes.py b/api/routes.py index b5f87fc7252..0b94890c921 100644 --- a/api/routes.py +++ b/api/routes.py @@ -10815,7 +10815,6 @@ def _keep_latest_messaging_session_per_source( _record_webui_zero_message_orphan_tombstone, _clear_webui_zero_message_orphan_tombstone, _load_webui_deleted_session_tombstone, - _record_webui_deleted_session_tombstone, ensure_cron_project, _profile_has_user_projects, is_cron_session, @@ -16602,34 +16601,47 @@ def _draft_mark(name): session_lock = _get_session_agent_lock(sid) if not session_lock.acquire(timeout=5): return bad(handler, "Session busy, try again", 503) + sidecar_authority = None try: - with LOCK: - SESSIONS.pop(sid, None) + from api.models import ( + SessionDeleteTombstoneError, + _delete_session_sidecar_artifacts_locked, + _session_sidecar_authority, + ) + + sidecar_authority = _session_sidecar_authority(sid) + sidecar_authority.__enter__() try: p = (SESSION_DIR / f"{sid}.json").resolve() p.relative_to(SESSION_DIR.resolve()) except Exception: return bad(handler, "Invalid session_id", 400) - sidecar_deleted = False try: - p.unlink(missing_ok=True) + _delete_session_sidecar_artifacts_locked( + sid, + record_tombstone=not is_messaging_session, + ) + except SessionDeleteTombstoneError: + logger.warning( + "Failed to durably tombstone deleted WebUI session %s", + sid, + exc_info=True, + ) + return bad(handler, "Failed to persist session deletion", 500) except Exception: - logger.debug("Failed to unlink session file %s", p) - sidecar_deleted = not p.exists() + logger.warning( + "Failed to delete required session file for %s", + sid, + exc_info=True, + ) + return bad(handler, "Failed to delete session files", 500) try: prune_session_from_index(sid) except Exception: logger.debug("Failed to prune deleted session from index: %s", sid, exc_info=True) - try: - p.with_suffix('.json.bak').unlink(missing_ok=True) - except Exception: - logger.debug("Failed to unlink session backup file %s", p.with_suffix('.json.bak')) - if sidecar_deleted and not is_messaging_session: - try: - _record_webui_deleted_session_tombstone(sid) - except Exception: - logger.debug("Failed to tombstone deleted WebUI session %s", sid, exc_info=True) finally: + if sidecar_authority is not None: + sidecar_authority.__exit__(None, None, None) session_lock.release() # Evict outside the mutation lock: lifecycle commit may perform provider # I/O and must not hold a per-session Session lock. @@ -16757,7 +16769,9 @@ def _draft_mark(name): # again (#3542 lifecycle gap). from api.session_ops import apply_session_title_rename apply_session_title_rename(s, "Untitled") - s.save() + backup_receipt = s.save() + from api.models import _read_sidecar_revision, _retire_backup_if_owned + committed_receipt = _read_sidecar_revision(s.path, sid) persisted_clear = False try: persisted = json.loads(s.path.read_text(encoding="utf-8")) @@ -16777,7 +16791,12 @@ def _draft_mark(name): logger.warning("session clear could not verify persisted empty state for %s", sid, exc_info=True) if had_sidecar_messages and persisted_clear: try: - s.path.with_suffix('.json.bak').unlink(missing_ok=True) + _retire_backup_if_owned( + sid, + s.path.with_suffix('.json.bak'), + backup_receipt, + committed_receipt, + ) except OSError: logger.warning("session clear could not remove stale backup for %s", sid, exc_info=True) # Evict cached agent outside the per-session lock. Eviction may run a @@ -23011,26 +23030,42 @@ def _handle_memory_read(handler, parsed=None): def _handle_sessions_cleanup(handler, body, zero_only=False): cleaned = 0 - phase1_removed_ids = set() + phase1_delete_candidate_ids = set() # Phase 1: Clean orphan session files (existing behavior). for p in SESSION_DIR.glob("*.json"): if p.name.startswith("_"): continue try: - s = Session.load(p.stem) - if zero_only: - should_delete = s and len(s.messages) == 0 - else: - should_delete = s and s.title == "Untitled" and len(s.messages) == 0 - if should_delete: - with LOCK: - SESSIONS.pop(p.stem, None) - p.unlink(missing_ok=True) + from api.models import ( + _delete_session_sidecar_artifacts_locked, + _read_sidecar_snapshot, + _session_sidecar_authority, + ) + + sid = p.stem + with _session_sidecar_authority(sid): + revision, payload = _read_sidecar_snapshot(p, sid) + messages = payload.get("messages") + if messages is None: + messages = [] + if not isinstance(messages, list): + continue + title = payload.get("title", "Untitled") + should_delete = not messages and ( + zero_only or title == "Untitled" + ) + if not should_delete: + continue + phase1_delete_candidate_ids.add(sid) + if not _delete_session_sidecar_artifacts_locked( + sid, + expected_revision=revision, + ): + continue cleaned += 1 - phase1_removed_ids.add(p.stem) except Exception: - logger.debug("Failed to clean up session file %s", p) + logger.debug("Failed to clean up session file %s", p, exc_info=True) phase1_touched = bool(cleaned) phase2_rewrote_index = False @@ -23069,10 +23104,10 @@ def _handle_sessions_cleanup(handler, body, zero_only=False): if not sid or sid in live_ids or sid in in_memory_ids: survivors.append(entry) continue - # Phase 1 already removed the backing file for this - # sid, so the index entry is stale too. Drop it - # from the index without double-counting. - if sid in phase1_removed_ids: + # Phase 1 already classified this sid for deletion. Drop + # its stale index row, but only a fully durable artifact + # removal contributes to the cleaned count. + if sid in phase1_delete_candidate_ids: continue # Index-only ghost — no backing file, not in memory. cleaned += 1 @@ -23175,6 +23210,26 @@ def _handle_btw(handler, body): return j(handler, {"stream_id": stream_id, "session_id": ephemeral.session_id, "parent_session_id": body["session_id"]}) +def _delete_hidden_background_session_sidecar(session_id: str) -> None: + """Delete a completed hidden session through the normal SID authorities.""" + if not is_safe_session_id(session_id): + raise ValueError(f"Unsafe hidden background session_id {session_id!r}") + with _get_session_agent_lock(session_id): + from api.models import ( + _delete_session_sidecar_artifacts_locked, + _session_sidecar_authority, + delete_cli_session, + ) + + with _session_sidecar_authority(session_id): + _delete_session_sidecar_artifacts_locked(session_id) + if not delete_cli_session(session_id): + logger.warning( + "Hidden background session %s remains in state.db; durable tombstone prevents recovery", + session_id, + ) + + def _handle_background(handler, body): """POST /api/background — run prompt in parallel background agent. @@ -23257,7 +23312,7 @@ def _run_bg_and_notify(): # clutter the sidebar or SESSION_DIR. The index is pruned on the # next rebuild via _index_entry_exists(). try: - (SESSION_DIR / f"{bg_sid}.json").unlink(missing_ok=True) + _delete_hidden_background_session_sidecar(bg_sid) except Exception: pass except Exception: @@ -24339,6 +24394,8 @@ def start_session_turn( try: workspace = _resolve_chat_workspace_with_recovery(s, None) + s = getattr(workspace, "session", s) + workspace = str(workspace) except WorkspaceBindingPersistenceError as e: return {"error": str(e), "_status": 500} except ValueError as e: @@ -24985,13 +25042,15 @@ def _handle_chat_start(handler, body, diag=None): return bad(handler, "invalid profile", 400) except ImportError: requested_profile = "" - session_profile = getattr(s, "profile", None) - has_persisted_turns = bool( - getattr(s, "messages", None) - or getattr(s, "context_messages", None) - or getattr(s, "pending_user_message", None) - ) - if not _session_visible_to_active_profile(session_profile, handler): + def _authorize_chat_start_session(candidate): + session_profile = getattr(candidate, "profile", None) + has_persisted_turns = bool( + getattr(candidate, "messages", None) + or getattr(candidate, "context_messages", None) + or getattr(candidate, "pending_user_message", None) + ) + if _session_visible_to_active_profile(session_profile, handler): + return True if ( requested_profile and _profiles_match(requested_profile, active_profile) @@ -24999,8 +25058,13 @@ def _handle_chat_start(handler, body, diag=None): ): # Empty placeholders can still be retagged when the # requested profile matches the active request profile. - s.profile = requested_profile - elif session_profile: + candidate.profile = requested_profile + return True + return False + + def _reject_chat_start_session(candidate): + session_profile = getattr(candidate, "profile", None) + if session_profile: # #7710: known other profile → 409 ``session_profile_mismatch`` # so the client can offer to switch to it (#5419). # 404 is preserved only for the None-profile @@ -25011,8 +25075,10 @@ def _handle_chat_start(handler, body, diag=None): "session_id": body.get("session_id", ""), "profile": session_profile, }, status=409) - else: - return bad(handler, "Session not found", 404) + return bad(handler, "Session not found", 404) + + if not _authorize_chat_start_session(s): + return _reject_chat_start_session(s) # Resolve durable rotations before any workspace/model/pending mutation. # GET navigation adopts the tip; POST never silently replays a user turn. from api.compression_continuation import durable_compression_continuation @@ -25048,29 +25114,46 @@ def _handle_chat_start(handler, body, diag=None): diag.stage("normalize_attachments") if diag else None if attachments is None: attachments = _normalize_chat_attachments(body.get("attachments") or [])[:20] - recovery = compression_recovery_payload_for_session(s) - if recovery and not attachments and is_generic_continuation_intent(msg): + + def _compression_recovery_required_response(candidate_recovery): return j( handler, { "error": "This session exhausted context compression. Start a focused continuation, then describe the next narrow task.", "type": "compression_recovery_required", - "recommended_recovery_action": recovery.get("recommended_action"), - "compression_recovery": recovery, + "recommended_recovery_action": candidate_recovery.get( + "recommended_action" + ), + "compression_recovery": candidate_recovery, "session_id": getattr(s, "session_id", body["session_id"]), }, status=409, ) + + candidate_recovery = compression_recovery_payload_for_session(s) diag.stage("resolve_workspace") if diag else None try: if regeneration is not None: workspace = _resolve_chat_workspace_for_regeneration(s, body.get("workspace")) else: workspace = _resolve_chat_workspace_with_recovery(s, body.get("workspace")) + s = getattr(workspace, "session", s) + workspace = str(workspace) except WorkspaceBindingPersistenceError as e: return bad(handler, str(e), 500) except ValueError as e: + if ( + candidate_recovery + and not attachments + and is_generic_continuation_intent(msg) + ): + return _compression_recovery_required_response(candidate_recovery) return bad(handler, str(e)) + if not _authorize_chat_start_session(s): + return _reject_chat_start_session(s) + recovery = compression_recovery_payload_for_session(s) + if recovery and not attachments and is_generic_continuation_intent(msg): + return _compression_recovery_required_response(recovery) requested_model = body.get("model") or s.model requested_provider = ( body.get("model_provider") @@ -25218,15 +25301,27 @@ def _restore_cleared_recovery(): -def _resolve_chat_workspace_with_recovery(s, requested_workspace) -> str: +class _ResolvedChatWorkspace(str): + """String-compatible workspace resolution carrying the durable SID owner.""" + + def __new__(cls, workspace, session): + resolved = super().__new__(cls, str(workspace)) + resolved.session = session + return resolved + + +def _resolve_chat_workspace_with_recovery(s, requested_workspace) -> _ResolvedChatWorkspace: """Recover stale implicit session workspaces without hiding explicit errors.""" _session_profile = getattr(s, "profile", None) explicit = requested_workspace not in (None, "") if explicit: try: - return str(resolve_trusted_workspace(requested_workspace, profile=_session_profile)) + return _ResolvedChatWorkspace( + resolve_trusted_workspace(requested_workspace, profile=_session_profile), + s, + ) except TypeError: - return str(resolve_trusted_workspace(requested_workspace)) + return _ResolvedChatWorkspace(resolve_trusted_workspace(requested_workspace), s) stored_workspace = getattr(s, "workspace", None) try: workspace, recovered = resolve_implicit_workspace_with_recovery( @@ -25240,13 +25335,13 @@ def _resolve_chat_workspace_with_recovery(s, requested_workspace) -> str: get_last_workspace, ) if not recovered: - return str(workspace) + return _ResolvedChatWorkspace(workspace, s) persisted = persist_recovered_workspace_binding( s, workspace, expected_workspace=stored_workspace, ) - return str(persisted.workspace) + return _ResolvedChatWorkspace(persisted.workspace, persisted) def _resolve_chat_workspace_for_regeneration(s, requested_workspace) -> str: @@ -28229,10 +28324,16 @@ def _summarize_manual_compression( s.truncation_boundary = compress_watermark s.compression_anchor_mode = "manual" s.last_prompt_tokens = new_tokens - s.save() + backup_receipt = s.save() # Drop stale backups that would undo an intentional manual compress. try: - s.path.with_suffix(".json.bak").unlink(missing_ok=True) + from api.models import _read_sidecar_revision, _retire_backup_if_owned + _retire_backup_if_owned( + s.session_id, + s.path.with_suffix(".json.bak"), + backup_receipt, + _read_sidecar_revision(s.path, s.session_id), + ) except OSError: pass diff --git a/api/session_discoverability.py b/api/session_discoverability.py index 671ad23ce38..37b02448cf6 100644 --- a/api/session_discoverability.py +++ b/api/session_discoverability.py @@ -449,17 +449,33 @@ def _plan_discoverability_repairs(report: dict) -> list[dict]: def _clear_sidecar_cli_flag(session_dir: Path, sid: str, backup_dir: Path, backed_up: dict[Path, str]) -> dict: path = session_dir / f"{sid}.json" - payload = _read_json(path) - if not isinstance(payload, dict): - return {"session_id": sid, "action": "clear_sidecar_cli_flag", "applied": False, "error": "sidecar_unreadable"} - if not _webui_origin(payload): - return {"session_id": sid, "action": "clear_sidecar_cli_flag", "applied": False, "skipped": "not_webui_origin"} - if payload.get("is_cli_session") is not True: - return {"session_id": sid, "action": "clear_sidecar_cli_flag", "applied": False, "skipped": "already_clear"} - backup = _backup_file(path, backup_dir, backed_up) - payload["is_cli_session"] = False - _atomic_write_json(path, payload) - return {"session_id": sid, "action": "clear_sidecar_cli_flag", "applied": True, "backup": backup} + from api.models import ( + _fsync_sidecar_directory, + _invalidate_cached_session_generation, + _read_sidecar_revision, + _session_sidecar_authority, + ) + + with _session_sidecar_authority(sid, session_dir=session_dir): + revision = _read_sidecar_revision(path, sid) + payload = _read_json(path) + if revision.state != "PRESENT" or not isinstance(payload, dict): + return {"session_id": sid, "action": "clear_sidecar_cli_flag", "applied": False, "error": "sidecar_unreadable"} + if payload.get("session_id") != sid: + return {"session_id": sid, "action": "clear_sidecar_cli_flag", "applied": False, "skipped": "foreign_sidecar"} + if not _webui_origin(payload): + return {"session_id": sid, "action": "clear_sidecar_cli_flag", "applied": False, "skipped": "not_webui_origin"} + if payload.get("is_cli_session") is not True: + return {"session_id": sid, "action": "clear_sidecar_cli_flag", "applied": False, "skipped": "already_clear"} + backup = _backup_file(path, backup_dir, backed_up) + payload["is_cli_session"] = False + payload["_sidecar_generation_v1"] = revision.generation + 1 + if _read_sidecar_revision(path, sid) != revision: + return {"session_id": sid, "action": "clear_sidecar_cli_flag", "applied": False, "skipped": "stale_generation"} + _atomic_write_json(path, payload) + _fsync_sidecar_directory(path.parent) + _invalidate_cached_session_generation(sid) + return {"session_id": sid, "action": "clear_sidecar_cli_flag", "applied": True, "backup": backup} def _clear_index_cli_flag(session_dir: Path, sid: str, backup_dir: Path, backed_up: dict[Path, str]) -> dict: @@ -489,49 +505,81 @@ def _materialize_sidecar_from_state_db(session_dir: Path, state_db_path: Path | if state_db_path is None: return {"session_id": sid, "action": "materialize_sidecar_from_state_db", "applied": False, "error": "state_db_required"} target = session_dir / f"{sid}.json" - if target.exists(): - return {"session_id": sid, "action": "materialize_sidecar_from_state_db", "applied": False, "skipped": "sidecar_exists"} try: - from api.session_recovery import _read_state_db_missing_sidecar_rows, _state_db_row_to_sidecar + from api.models import ( + _fsync_sidecar_directory, + _invalidate_cached_session_generation, + _publish_sidecar_no_replace, + _session_sidecar_authority, + ) + from api.session_recovery import ( + _durable_tombstone_marks_deleted_webui_session, + _read_state_db_missing_sidecar_rows, + _state_db_row_to_sidecar, + ) except Exception as exc: return {"session_id": sid, "action": "materialize_sidecar_from_state_db", "applied": False, "error": f"recovery_import_failed:{exc}"} - rows = {str(row.get("id") or ""): row for row in _read_state_db_missing_sidecar_rows(session_dir, state_db_path)} - row = rows.get(sid) - if not row: - return {"session_id": sid, "action": "materialize_sidecar_from_state_db", "applied": False, "skipped": "state_row_not_repairable"} - payload = _state_db_row_to_sidecar(row) - _backup_file(state_db_path, backup_dir, backed_up) - session_dir.mkdir(parents=True, exist_ok=True) - tmp = target.with_suffix(target.suffix + f".tmp.{os.getpid()}.{threading.get_ident()}") - tmp.write_text(json.dumps(payload, ensure_ascii=False, indent=2), encoding="utf-8") - try: - os.link(str(tmp), str(target)) - except FileExistsError: - return {"session_id": sid, "action": "materialize_sidecar_from_state_db", "applied": False, "skipped": "sidecar_appeared_during_repair"} - finally: + with _session_sidecar_authority(sid, session_dir=session_dir): + if target.exists(): + return {"session_id": sid, "action": "materialize_sidecar_from_state_db", "applied": False, "skipped": "sidecar_exists"} + if _durable_tombstone_marks_deleted_webui_session(session_dir, sid): + return {"session_id": sid, "action": "materialize_sidecar_from_state_db", "applied": False, "skipped": "deleted_tombstone"} + rows = { + str(row.get("id") or ""): row + for row in _read_state_db_missing_sidecar_rows( + session_dir, + state_db_path, + ) + } + row = rows.get(sid) + if not row: + return {"session_id": sid, "action": "materialize_sidecar_from_state_db", "applied": False, "skipped": "state_row_not_repairable"} + payload = _state_db_row_to_sidecar(row) + payload["_sidecar_generation_v1"] = 1 + backup = _backup_file(state_db_path, backup_dir, backed_up) + session_dir.mkdir(parents=True, exist_ok=True) + tmp = target.with_suffix( + target.suffix + f".tmp.{os.getpid()}.{threading.get_ident()}" + ) try: - tmp.unlink(missing_ok=True) - except OSError: - pass - index_updated = False - index_path = session_dir / "_index.json" - index_payload = _read_json(index_path) - if not isinstance(index_payload, list): - index_payload = [] - if not any(isinstance(entry, dict) and str(entry.get("session_id") or "") == sid for entry in index_payload): - _backup_file(index_path, backup_dir, backed_up) - index_entry = {key: value for key, value in payload.items() if key not in {"messages", "tool_calls"}} - index_payload.append(index_entry) - _atomic_write_json(index_path, index_payload) - index_updated = True - return { - "session_id": sid, - "action": "materialize_sidecar_from_state_db", - "applied": True, - "messages": len(payload.get("messages") or []), - "index_updated": index_updated, - "backup": str((backup_dir / state_db_path.name)) if (backup_dir / state_db_path.name).exists() else None, - } + with open(tmp, "x", encoding="utf-8") as handle: + handle.write(json.dumps(payload, ensure_ascii=False, indent=2)) + handle.flush() + os.fsync(handle.fileno()) + try: + _publish_sidecar_no_replace(tmp, target) + except FileExistsError: + return {"session_id": sid, "action": "materialize_sidecar_from_state_db", "applied": False, "skipped": "sidecar_appeared_during_repair"} + finally: + try: + tmp.unlink(missing_ok=True) + except OSError: + pass + index_updated = False + index_path = session_dir / "_index.json" + index_payload = _read_json(index_path) + if not isinstance(index_payload, list): + index_payload = [] + if not any(isinstance(entry, dict) and str(entry.get("session_id") or "") == sid for entry in index_payload): + _backup_file(index_path, backup_dir, backed_up) + index_entry = { + key: value + for key, value in payload.items() + if key not in {"messages", "tool_calls"} + } + index_payload.append(index_entry) + _atomic_write_json(index_path, index_payload) + _fsync_sidecar_directory(index_path.parent) + index_updated = True + _invalidate_cached_session_generation(sid) + return { + "session_id": sid, + "action": "materialize_sidecar_from_state_db", + "applied": True, + "messages": len(payload.get("messages") or []), + "index_updated": index_updated, + "backup": backup, + } def repair_session_discoverability( diff --git a/api/session_recovery.py b/api/session_recovery.py index d9b09479977..b0137cf31b3 100644 --- a/api/session_recovery.py +++ b/api/session_recovery.py @@ -30,9 +30,9 @@ import logging import os import re -import shutil import sqlite3 import threading +import uuid from contextlib import closing from pathlib import Path @@ -398,23 +398,104 @@ def inspect_session_recovery_status(session_path: Path) -> dict: } -def recover_session(session_path: Path) -> dict: - """Restore session_path from its .bak when the bak has more messages. +def recover_session( + session_path: Path, + *, + state_db_path: Path | None = None, +) -> dict: + """Restore from .bak without superseding a newer sidecar generation.""" + from api.models import _session_sidecar_authority + + with _session_sidecar_authority( + session_path.stem, + session_dir=session_path.parent, + ): + return _recover_session_owned( + session_path, + state_db_path=state_db_path, + ) - Returns a status dict identical to ``inspect_session_recovery_status`` - plus a "restored" boolean. - """ + +def _recover_session_owned( + session_path: Path, + *, + state_db_path: Path | None = None, +) -> dict: + """Run one recovery while holding the cross-process SID authority.""" + from api.models import ( + _fsync_sidecar_directory, + _invalidate_cached_session_generation, + _publish_sidecar_no_replace, + _read_sidecar_revision, + _safe_replace, + ) + + bak_path = session_path.with_suffix('.json.bak') + expected_live_revision = _read_sidecar_revision( + session_path, + session_path.stem, + ) + expected_backup_revision = _read_sidecar_revision( + bak_path, + session_path.stem, + ) status = inspect_session_recovery_status(session_path) if status["recommend"] != "restore": return {**status, "restored": False} - bak_path = session_path.with_suffix('.json.bak') - # Stage the recovery via a tmp copy + atomic replace so a crash mid-restore - # cannot leave a half-written session.json. - tmp_path = session_path.with_suffix('.json.recover.tmp') + if expected_live_revision.state == "ABSENT": + if _durable_tombstone_marks_deleted_webui_session( + session_path.parent, + session_path.stem, + ): + return {**status, "restored": False, "deleted": True} + if not _state_db_has_session(session_path.stem, state_db_path): + return { + **status, + "restored": False, + "deleted": True, + "missing_state_row": True, + } + if expected_backup_revision.state != "PRESENT": + return {**status, "restored": False, "stale_generation": True} + tmp_path = session_path.with_suffix( + f'.json.recover.tmp.{os.getpid()}.{threading.current_thread().ident}' + ) try: - shutil.copyfile(bak_path, tmp_path) - tmp_path.replace(session_path) - except OSError as exc: + backup = json.loads(bak_path.read_text(encoding='utf-8')) + if not isinstance(backup, dict): + raise ValueError("backup payload is not a session object") + if backup.get('session_id') != session_path.stem: + raise ValueError("backup session id does not match sidecar path") + base_generation = ( + expected_live_revision.generation + if expected_live_revision.state == "PRESENT" + else expected_backup_revision.generation + ) + backup['_sidecar_generation_v1'] = base_generation + 1 + with open(tmp_path, 'w', encoding='utf-8') as fh: + fh.write(json.dumps(backup, ensure_ascii=False, indent=2)) + fh.flush() + os.fsync(fh.fileno()) + if ( + _read_sidecar_revision(session_path, session_path.stem) + != expected_live_revision + or _read_sidecar_revision(bak_path, session_path.stem) + != expected_backup_revision + ): + tmp_path.unlink(missing_ok=True) + return {**status, "restored": False, "stale_generation": True} + if expected_live_revision.state == "ABSENT": + try: + _publish_sidecar_no_replace(tmp_path, session_path) + except FileExistsError: + tmp_path.unlink(missing_ok=True) + return {**status, "restored": False, "stale_generation": True} + tmp_path.unlink(missing_ok=True) + else: + _safe_replace(tmp_path, session_path) + _fsync_sidecar_directory(session_path.parent) + _invalidate_cached_session_generation(session_path.stem) + except (OSError, json.JSONDecodeError, ValueError) as exc: logger.warning("recover_session: copy failed for %s: %s", session_path, exc) try: tmp_path.unlink(missing_ok=True) @@ -499,6 +580,7 @@ def _read_state_db_missing_sidecar_rows( state_db_path: Path | None, *, include_empty: bool = False, + session_id: str | None = None, ) -> list[dict]: """Return WebUI-origin state.db rows whose JSON sidecar is missing.""" if state_db_path is None or not state_db_path.exists(): @@ -506,6 +588,9 @@ def _read_state_db_missing_sidecar_rows( try: with closing(sqlite3.connect(f"file:{state_db_path}?mode=ro", uri=True)) as conn: conn.row_factory = sqlite3.Row + # A read-only connection does not begin a transaction for SELECTs + # automatically. Pin metadata and messages to one SQLite snapshot. + conn.execute("BEGIN") session_cols = {row[1] for row in conn.execute("PRAGMA table_info(sessions)").fetchall()} message_cols = {row[1] for row in conn.execute("PRAGMA table_info(messages)").fetchall()} if not {'id', 'source'}.issubset(session_cols): @@ -521,6 +606,11 @@ def _read_state_db_missing_sidecar_rows( worktree_repo_root_expr = _sql_optional_col('worktree_repo_root', session_cols) worktree_created_at_expr = _sql_optional_col('worktree_created_at', session_cols) rows = [] + where_clause = "source = 'webui'" + query_params: tuple[str, ...] = () + if session_id is not None: + where_clause += " AND id = ?" + query_params = (session_id,) for row in conn.execute( f""" SELECT id, source, {title_expr}, {model_expr}, {started_expr}, @@ -528,9 +618,10 @@ def _read_state_db_missing_sidecar_rows( {worktree_path_expr}, {worktree_branch_expr}, {worktree_repo_root_expr}, {worktree_created_at_expr} FROM sessions - WHERE source = 'webui' + WHERE {where_clause} ORDER BY COALESCE(started_at, 0) DESC - """ + """, + query_params, ).fetchall(): data = dict(row) sid = str(data.get('id') or '').strip() @@ -656,6 +747,8 @@ def _state_db_row_to_sidecar(row: dict) -> dict: def recover_missing_sidecars_from_state_db(session_dir: Path, state_db_path: Path | None) -> dict: """Materialize missing WebUI JSON sidecars from canonical state.db rows.""" + from api.models import _publish_sidecar_no_replace, _session_sidecar_authority + rows = _read_state_db_missing_sidecar_rows(session_dir, state_db_path) materialized = 0 details: list[dict] = [] @@ -667,30 +760,62 @@ def recover_missing_sidecars_from_state_db(session_dir: Path, state_db_path: Pat target = session_dir / f"{sid}.json" if target.exists(): continue - payload = _state_db_row_to_sidecar(row) # Per-process/per-thread tmp suffix to avoid corruption under # concurrent reconciliation calls (matches api/models.py:484 # Session.save() convention). - tmp_suffix = f".json.reconcile.tmp.{os.getpid()}.{threading.current_thread().ident}" + tmp_suffix = ( + f".json.reconcile.tmp.{os.getpid()}." + f"{threading.current_thread().ident}.{uuid.uuid4().hex}" + ) tmp = target.with_suffix(tmp_suffix) detail_recorded = False - try: - tmp.write_text(json.dumps(payload, ensure_ascii=False, indent=2), encoding='utf-8') - except OSError as exc: - try: - tmp.unlink(missing_ok=True) - except OSError: - pass - details.append({'session_id': sid, 'materialized': False, 'error': str(exc)}) - continue + payload = None # Atomic create-or-fail: os.link() refuses to overwrite an existing # target. Closes the TOCTOU window between the target.exists() check # above and the rename — a concurrent Session.save() for the same SID # will win and we silently skip rather than overwrite a live sidecar. materialized_now = False + skipped_deleted = False try: - os.link(str(tmp), str(target)) - materialized_now = True + with _session_sidecar_authority(sid, session_dir=session_dir): + if _durable_tombstone_marks_deleted_webui_session(session_dir, sid): + skipped_deleted = True + else: + current_row = next( + ( + candidate + for candidate in _read_state_db_missing_sidecar_rows( + session_dir, + state_db_path, + session_id=sid, + ) + if str(candidate.get('id') or '').strip() == sid + ), + None, + ) + if current_row is None: + skipped_deleted = True + else: + payload = _state_db_row_to_sidecar(current_row) + payload['_sidecar_generation_v1'] = 1 + fd = os.open( + tmp, + os.O_WRONLY | os.O_CREAT | os.O_EXCL, + 0o600, + ) + with os.fdopen( + fd, + 'w', + encoding='utf-8', + newline='\n', + ) as handle: + handle.write( + json.dumps(payload, ensure_ascii=False, indent=2) + ) + handle.flush() + os.fsync(handle.fileno()) + _publish_sidecar_no_replace(tmp, target) + materialized_now = True except FileExistsError: # Live sidecar appeared between the check and the link — keep it. pass @@ -704,7 +829,13 @@ def recover_missing_sidecars_from_state_db(session_dir: Path, state_db_path: Pat pass if materialized_now: materialized += 1 - details.append({'session_id': sid, 'materialized': True, 'messages': len(payload.get('messages') or [])}) + details.append({ + 'session_id': sid, + 'materialized': True, + 'messages': len((payload or {}).get('messages') or []), + }) + elif skipped_deleted: + details.append({'session_id': sid, 'materialized': False, 'skipped': 'deleted_during_reconcile'}) elif not detail_recorded: details.append({'session_id': sid, 'materialized': False, 'skipped': 'sidecar_appeared_during_reconcile'}) return {'scanned': len(rows), 'materialized': materialized, 'details': details} @@ -1063,7 +1194,7 @@ def recover_all_sessions_on_startup( scanned = len(live_paths) + len(orphan_paths) for path in [*recovery_paths, *orphan_paths]: try: - result = recover_session(path) + result = recover_session(path, state_db_path=state_db_path) except Exception as exc: # Defensive: a malformed session file shouldn't break recovery # for the rest. Log and continue. diff --git a/api/streaming.py b/api/streaming.py index 489c91a8721..eae0551dfb9 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -2774,18 +2774,80 @@ def _persist_cancelled_turn(session, *, message: str = 'Task cancelled.') -> Non }) -def _cleanup_ephemeral_cancelled_turn(session) -> None: - """Remove transient /btw session state after a cancel without saving it.""" +def _clear_ephemeral_turn_state(session) -> None: + """Clear transient stream fields on an in-memory hidden session.""" session.active_stream_id = None session.pending_user_message = None session.pending_attachments = [] session.pending_started_at = None session.pending_user_source = None + + +def _cleanup_ephemeral_cancelled_turn(session) -> None: + """Remove transient /btw session state after a cancel without saving it.""" + _clear_ephemeral_turn_state(session) + _cleanup_ephemeral_session_sidecar_locked(session, outcome="cancelled") + + +def _cleanup_ephemeral_session_sidecar_locked(session, *, outcome: str) -> bool: + """Best-effort hidden-session delete while the caller holds its agent lock. + + Lock order stays agent lock -> SID sidecar authority, matching manual and + hidden-background deletion. Any validation or durable-delete failure leaves + the HTTP/SSE outcome best-effort, but it is logged and never falls back to a + direct unlink outside the tombstone protocol. + """ + sid = str(getattr(session, "session_id", "") or "").strip() try: - import pathlib - pathlib.Path(session.path).unlink(missing_ok=True) + from api.models import ( + _coerce_sidecar_revision, + _delete_session_sidecar_artifacts_locked, + _read_sidecar_snapshot, + _session_sidecar_authority, + is_safe_session_id, + ) + + if not is_safe_session_id(sid): + raise ValueError(f"Unsafe ephemeral session_id {sid!r}") + directory = Path(SESSION_DIR).resolve() + expected_path = (directory / f"{sid}.json").resolve() + session_path = Path(getattr(session, "path", "")).resolve() + if session_path != expected_path: + raise ValueError( + f"Ephemeral session path {session_path} does not match SID {sid!r}" + ) + expected_revision = _coerce_sidecar_revision( + getattr(session, "_sidecar_revisions", {}).get(sid), + sid, + ) + if expected_revision is None: + raise RuntimeError( + f"Ephemeral session {sid!r} has no owned sidecar revision" + ) + with _session_sidecar_authority(sid, session_dir=directory): + current_revision, _payload = _read_sidecar_snapshot(expected_path, sid) + if current_revision != expected_revision: + raise RuntimeError( + f"Ephemeral session {sid!r} changed before {outcome} cleanup" + ) + deleted = _delete_session_sidecar_artifacts_locked( + sid, + session_dir=directory, + expected_revision=current_revision, + ) + if not deleted: + raise RuntimeError( + f"Ephemeral session {sid!r} lost revision authority during cleanup" + ) + return True except Exception: - logger.debug("Failed to clean up ephemeral cancelled session", exc_info=True) + logger.warning( + "Failed to clean up %s ephemeral session %s through durable sidecar protocol", + outcome, + sid or "", + exc_info=True, + ) + return False def _resolve_current_session_for_write(session): @@ -5613,46 +5675,39 @@ def _preserve_pre_compression_snapshot(s, old_sid: str) -> None: # In-memory messages are newer than the file; save the full old # snapshot from the current session object while preserving its # pre-existing parent_session_id lineage. - saved_sid = s.session_id - saved_snapshot = bool(getattr(s, 'pre_compression_snapshot', False)) - saved_pinned = bool(getattr(s, 'pinned', False)) - s.session_id = old_sid - s.pre_compression_snapshot = True - s.pinned = False + from api.models import Session + + owned_old = Session.load(old_sid) + if owned_old is None: + return + snapshot = copy.copy(s) + snapshot._sidecar_revisions = dict(owned_old._sidecar_revisions) + snapshot.session_id = old_sid + snapshot.parent_session_id = existing.get( + 'parent_session_id', + getattr(s, 'parent_session_id', None), + ) + snapshot.pre_compression_snapshot = True + snapshot.pinned = False # Stage-359 / PR #2295: clear runtime stream-state fields on the # archived snapshot so the sidebar does not reopen the parent as # a permanently-running session while the child already holds the - # completed answer. The continuation session's live state is - # restored from saved_* locals in the finally block. - saved_active_stream_id = getattr(s, 'active_stream_id', None) - saved_pending_user_message = getattr(s, 'pending_user_message', None) - saved_pending_attachments = list(getattr(s, 'pending_attachments', []) or []) - saved_pending_started_at = getattr(s, 'pending_started_at', None) - saved_pending_user_source = getattr(s, 'pending_user_source', None) - s.active_stream_id = None - s.pending_user_message = None - s.pending_attachments = [] - s.pending_started_at = None - s.pending_user_source = None - try: - # skip_index=False so the snapshot appears in _index.json with - # the pre_compression_snapshot marker. The sidebar projection - # (#2285) reads that marker to hide the snapshot from active - # rows while keeping the JSON discoverable for lineage traversal. - s.save(touch_updated_at=False, skip_index=False) - logger.info( - "Preserved pre-compression session %s (%d messages) to disk", - old_sid, len(s.messages), - ) - finally: - s.session_id = saved_sid - s.pre_compression_snapshot = saved_snapshot - s.pinned = saved_pinned - s.active_stream_id = saved_active_stream_id - s.pending_user_message = saved_pending_user_message - s.pending_attachments = saved_pending_attachments - s.pending_started_at = saved_pending_started_at - s.pending_user_source = saved_pending_user_source + # completed answer. The continuation object remains untouched; + # only the revision-owning snapshot copy is persisted. + snapshot.active_stream_id = None + snapshot.pending_user_message = None + snapshot.pending_attachments = [] + snapshot.pending_started_at = None + snapshot.pending_user_source = None + # skip_index=False so the snapshot appears in _index.json with the + # pre_compression_snapshot marker. The sidebar projection (#2285) + # reads that marker to hide the snapshot from active rows while + # keeping the JSON discoverable for lineage traversal. + snapshot.save(touch_updated_at=False, skip_index=False) + logger.info( + "Preserved pre-compression session %s (%d messages) to disk", + old_sid, len(snapshot.messages), + ) return # Existing file is already at least as complete as memory; stamp only # the snapshot marker so index/sidebar projection can hide it without @@ -12062,11 +12117,13 @@ def _periodic_checkpoint(): }) if _checkpoint_stop is not None: _checkpoint_stop.set() - try: - import pathlib - pathlib.Path(s.path).unlink(missing_ok=True) - except Exception: - pass + with _agent_lock: + _ephemeral_deleted = _cleanup_ephemeral_session_sidecar_locked( + s, + outcome="completed", + ) + if _ephemeral_deleted: + _clear_ephemeral_turn_state(s) return # skip all normal persistence for ephemeral sessions if _checkpoint_stop is not None: _checkpoint_stop.set() diff --git a/docs/rfcs/webui-run-state-consistency-contract.md b/docs/rfcs/webui-run-state-consistency-contract.md index ca3c4e35a4d..d7b27c96faf 100644 --- a/docs/rfcs/webui-run-state-consistency-contract.md +++ b/docs/rfcs/webui-run-state-consistency-contract.md @@ -267,6 +267,39 @@ and 5; it does not mark every run-state boundary implemented. still owns a live channel. Staleness is measured from the cancellation timestamp (falling back to run start), so a long-running turn cancelled moments ago is never mistaken for an orphan. +10. **Sidecar writes are generation-fenced.** A writer may replace a session JSON + sidecar only while the exact durable revision it observed is still current. + Text publication must use canonical LF bytes so the stored digest matches the + file on native Windows. First creation must use an atomic create-only primitive: + hard-link on POSIX, create-only rename on Windows, or fail closed when neither + is available. SID rotation must start from an absent revision for the new ID. + A raw metadata patch may advance only aliases that owned the exact pre-write + revision; stale aliases are invalidated and reloaded instead, and callers must + adopt the returned revision owner before model resolution or turn start. + Recovery must recheck durable deletes under the SID authority, validate the + embedded SID, and invalidate stale in-memory aliases. A shrinking write must + not proceed unless its recoverable history is durable: row count or message + coverage alone does not prove dominance. Dominance covers the complete + canonical recovery snapshot, including ordered context/tool sequences and + all unknown metadata; only derived generation/count and superseded activity + time are excluded. The comparison accepts legacy snapshots that omit newer + bookkeeping fields, has a fixed work budget, and treats any overflow or + unverifiable field as incomparable. Malformed, non-object, foreign-SID, and + incomparable primary backups are archived byte-for-byte and + content-addressedly before the valid live snapshot is promoted. Archive + publication streams into a complete + temporary file, verifies the expected digest, and atomically renames it, so + archive preservation does not depend on hard-link support; the stricter + create-only primitive remains mandatory for live sidecars. Backup retirement + requires matching receipts for both the backup and the committed live + generation. Any deletion path must decide and act while holding the same SID + authority, revalidate its payload and exact revision at the deletion point, + publish a durable tombstone before unlinking, and remove the primary sidecar, + backup, backup archives, and session-owned replay-v10 recovery artifacts + before reporting success. Hidden ephemeral deletion follows this rule after + both cancellation and normal completion, with the agent lock acquired before + the SID authority. Its response-level cleanup may remain best-effort, but a + protocol failure must be logged and must not fall back to a raw unlink. ## Client-side unread persistence (sidebar layer) @@ -382,6 +415,14 @@ context reconstruction, or session metadata: (`hermes-session-viewed-counts`, `hermes-session-completion-unread`, `hermes-session-completion-unread-cleared`), and does it keep the merge and tombstone rules in the client-side unread persistence section? +- Can a stale save, repair, recovery, or metadata patch replace a newer sidecar? + Does recovery revalidate delete tombstones and state-db ownership while holding + the same SID authority used for publication? +- If the write shrinks history, is backup publication fail-closed and monotone by + complete canonical snapshot coverage rather than message rows or count? Does + the bounded comparison include every durable top-level field, archive + unverifiable/incomparable generations before promotion, and does cleanup prove + both backup and live receipts still match before unlinking? - What test or manual evidence proves the invariant? ## Existing Issue Map diff --git a/tests/test_cancelled_turn_status.py b/tests/test_cancelled_turn_status.py index df9554cb3ef..18df8245fcc 100644 --- a/tests/test_cancelled_turn_status.py +++ b/tests/test_cancelled_turn_status.py @@ -8,6 +8,7 @@ import pathlib +import api.streaming as streaming from api.streaming import ( _CANCEL_MARKER_PATTERNS, _cancelled_turn_content, @@ -24,6 +25,7 @@ def _read(rel_path: str) -> str: class _DummySession: def __init__(self, path: str = ''): + self.session_id = 'ephemeral-cancel-probe' self.path = path self.messages = [] self.active_stream_id = 'stream-1' @@ -90,10 +92,20 @@ def test_persistent_cancel_finalizer_clears_pending_and_saves_cancel_marker(self assert session.messages[-1]['provider_details_label'] == 'Cancellation details' assert session.messages[-1]['_error'] is True - def test_ephemeral_cancel_finalizer_unlinks_temp_session_without_saving_error_marker(self, tmp_path): + def test_ephemeral_cancel_finalizer_delegates_without_saving_error_marker( + self, + tmp_path, + monkeypatch, + ): temp_session = tmp_path / 'btw-session.json' temp_session.write_text('{}', encoding='utf-8') session = _DummySession(str(temp_session)) + cleanup_calls = [] + monkeypatch.setattr( + streaming, + "_cleanup_ephemeral_session_sidecar_locked", + lambda candidate, *, outcome: cleanup_calls.append((candidate, outcome)), + ) _finalize_cancelled_turn(session, ephemeral=True) @@ -103,7 +115,8 @@ def test_ephemeral_cancel_finalizer_unlinks_temp_session_without_saving_error_ma assert session.pending_started_at is None assert session.saved == 0 assert session.messages == [] - assert not temp_session.exists() + assert cleanup_calls == [(session, "cancelled")] + assert temp_session.exists() def test_message_renderer_allows_non_provider_details_label(self): @@ -121,6 +134,13 @@ def test_cancel_error_carrier_is_not_folded_into_worklog(self): class TestCancelledTurnPersistenceGuards: + def test_ephemeral_cleanup_never_directly_unlinks_a_sidecar(self): + src = _read("api/streaming.py") + assert ".unlink(" not in src + assert "_session_sidecar_authority" in src + assert "_delete_session_sidecar_artifacts_locked" in src + assert 'outcome="completed"' in src + def test_cancel_marker_patterns_are_centralized_for_dedupe(self): assert _CANCEL_MARKER_PATTERNS == ('task cancelled', 'task canceled', 'response interrupted') src = _read("api/streaming.py") diff --git a/tests/test_compression_snapshot_runtime_clear.py b/tests/test_compression_snapshot_runtime_clear.py index 69e79d252b2..4a235c8970b 100644 --- a/tests/test_compression_snapshot_runtime_clear.py +++ b/tests/test_compression_snapshot_runtime_clear.py @@ -36,12 +36,17 @@ def save(self, *, touch_updated_at=True, skip_index=False): def test_preserve_pre_compression_snapshot_clears_runtime_fields_while_restoring_continuation_state(tmp_path, monkeypatch): monkeypatch.setattr(streaming, "SESSION_DIR", tmp_path) - (tmp_path / "old_session.json").write_text(json.dumps({"messages": []}), encoding="utf-8") + monkeypatch.setattr(models, "SESSION_DIR", tmp_path) + (tmp_path / "old_session.json").write_text( + json.dumps({"session_id": "old_session", "messages": []}), + encoding="utf-8", + ) session = FakeSession() streaming._preserve_pre_compression_snapshot(session, "old_session") - assert session.saved_payload == { + saved = json.loads((tmp_path / "old_session.json").read_text(encoding="utf-8")) + assert saved == { "session_id": "old_session", "parent_session_id": "original_parent", "pre_compression_snapshot": True, @@ -61,7 +66,6 @@ def test_preserve_pre_compression_snapshot_clears_runtime_fields_while_restoring assert session.pending_attachments == [{"name": "file.txt"}] assert session.pending_started_at == 123.0 - saved = json.loads((tmp_path / "old_session.json").read_text(encoding="utf-8")) assert saved["pre_compression_snapshot"] is True assert saved["pinned"] is False assert saved["active_stream_id"] is None @@ -107,7 +111,11 @@ def test_preserve_pre_compression_snapshot_load_and_mark_branch_clears_runtime_f def test_preserve_pre_compression_snapshot_does_not_leave_continuation_marked_as_snapshot(tmp_path, monkeypatch): """A continuation loaded from an old snapshot must not remain hidden.""" monkeypatch.setattr(streaming, "SESSION_DIR", tmp_path) - (tmp_path / "old_session.json").write_text(json.dumps({"messages": []}), encoding="utf-8") + monkeypatch.setattr(models, "SESSION_DIR", tmp_path) + (tmp_path / "old_session.json").write_text( + json.dumps({"session_id": "old_session", "messages": []}), + encoding="utf-8", + ) session = FakeSession() session.pre_compression_snapshot = True diff --git a/tests/test_file_manager_external_session.py b/tests/test_file_manager_external_session.py index 59648804678..ae3da73218d 100644 --- a/tests/test_file_manager_external_session.py +++ b/tests/test_file_manager_external_session.py @@ -194,9 +194,10 @@ def get_session(_sid, metadata_only=False): recovered = models_module.get_session_for_file_ops(metadata_session.session_id) - assert recovered is metadata_session + assert recovered is not metadata_session assert recovered.session_id == metadata_session.session_id assert Path(recovered.workspace) == fallback.resolve() + assert metadata_session.workspace == str(stale) persisted = json.loads(sidecar.read_text(encoding="utf-8")) assert persisted["workspace"] == str(fallback.resolve()) assert persisted["messages"] == [{"role": "user", "content": "preserve me"}] @@ -452,7 +453,7 @@ def paused_replace(source, target): ) monkeypatch.setattr(routes_module, "prune_session_from_index", lambda _sid: None) monkeypatch.setattr( - routes_module, "_record_webui_deleted_session_tombstone", lambda _sid: None + models_module, "_record_webui_deleted_session_tombstone", lambda _sid: None ) monkeypatch.setattr( routes_module, "_publish_session_list_changed", lambda *_args, **_kwargs: None @@ -564,7 +565,7 @@ def __exit__(self, *_args): lambda _sid: observed["mutations"].append("index"), ) monkeypatch.setattr( - routes_module, + models_module, "_record_webui_deleted_session_tombstone", lambda _sid: observed["mutations"].append("tombstone"), ) diff --git a/tests/test_gateway_sync.py b/tests/test_gateway_sync.py index cb89f1c0c85..f4c2e39a6e1 100644 --- a/tests/test_gateway_sync.py +++ b/tests/test_gateway_sync.py @@ -2662,21 +2662,20 @@ def test_importing_older_gateway_session_preserves_original_timestamps_and_order assert rename_status == 200, rename from api.models import Session from tests.conftest import TEST_WORKSPACE - newer_webui_session = Session( - session_id=newer_webui_sid, - title='Newer WebUI Session', - workspace=str(TEST_WORKSPACE), - model='openai/gpt-5', - created_at=newer_webui['session']['created_at'], - updated_at=time.time(), - profile='default', - messages=[{ - 'role': 'user', - 'content': 'newer visible row', - 'timestamp': time.time(), - }], - tool_calls=[], - ) + newer_webui_session = Session.load(newer_webui_sid) + assert newer_webui_session is not None + newer_webui_session.title = 'Newer WebUI Session' + newer_webui_session.workspace = str(TEST_WORKSPACE) + newer_webui_session.model = 'openai/gpt-5' + newer_webui_session.created_at = newer_webui['session']['created_at'] + newer_webui_session.updated_at = time.time() + newer_webui_session.profile = 'default' + newer_webui_session.messages = [{ + 'role': 'user', + 'content': 'newer visible row', + 'timestamp': time.time(), + }] + newer_webui_session.tool_calls = [] newer_webui_session.save(touch_updated_at=False) rename_refresh, rename_refresh_status = post( '/api/session/rename', diff --git a/tests/test_issue2057_worktree_lifecycle.py b/tests/test_issue2057_worktree_lifecycle.py index 638089c56a2..f852eba194f 100644 --- a/tests/test_issue2057_worktree_lifecycle.py +++ b/tests/test_issue2057_worktree_lifecycle.py @@ -2,6 +2,8 @@ from pathlib import Path from types import SimpleNamespace +import pytest + import api.models as models import api.routes as routes from api.models import SESSIONS, Session @@ -20,6 +22,15 @@ def _capture_post(monkeypatch, body): ) or True, ) + monkeypatch.setattr( + routes, + "bad", + lambda handler, message, status=400: captured.update( + payload={"error": message}, + status=status, + ) + or True, + ) return captured @@ -126,15 +137,7 @@ def test_delete_session_records_tombstone_when_state_db_delete_fails(tmp_path, m def fail_delete(value): raise RuntimeError("state.db locked") - real_unlink = Path.unlink - - def fail_backup_unlink(path, *args, **kwargs): - if path.name == f"{sid}.json.bak": - raise PermissionError("backup locked") - return real_unlink(path, *args, **kwargs) - monkeypatch.setattr(models, "delete_cli_session", fail_delete) - monkeypatch.setattr(Path, "unlink", fail_backup_unlink) assert routes.handle_post(object(), SimpleNamespace(path="/api/session/delete")) is True @@ -142,6 +145,132 @@ def fail_backup_unlink(path, *args, **kwargs): assert captured["payload"]["ok"] is True assert captured["payload"]["state_db_cleanup_failed"] is True assert not (session_dir / f"{sid}.json").exists() + assert not (session_dir / f"{sid}.json.bak").exists() + assert sid in models._load_webui_deleted_session_tombstone() + + +def test_delete_session_fails_closed_when_tombstone_publication_fails( + tmp_path, + monkeypatch, +): + session_dir = _isolate_session_store(tmp_path, monkeypatch) + sid = "tombstoneeio1" + session = Session( + session_id=sid, + title="Deletion must fail closed", + messages=[{"role": "user", "content": "retain on failure"}], + ) + session.save() + captured = _capture_post(monkeypatch, {"session_id": sid}) + monkeypatch.setattr(routes, "_lookup_cli_session_metadata", lambda value: {}) + monkeypatch.setattr(routes, "_is_messaging_session_id", lambda value: False) + real_replace = models.os.replace + tombstone_path = models._webui_deleted_session_tombstone_file() + + def fail_tombstone_replace(source, destination): + if Path(destination) == tombstone_path: + raise OSError("injected tombstone durability failure") + return real_replace(source, destination) + + monkeypatch.setattr(models.os, "replace", fail_tombstone_replace) + + assert routes.handle_post( + object(), + SimpleNamespace(path="/api/session/delete"), + ) is True + + assert captured["status"] == 500 + assert captured["payload"]["error"] == "Failed to persist session deletion" + assert (session_dir / f"{sid}.json").exists() + assert sid not in models._load_webui_deleted_session_tombstone() + + +@pytest.mark.parametrize("target_kind", ["primary", "backup", "archive"]) +def test_delete_session_fails_closed_when_required_unlink_fails( + tmp_path, + monkeypatch, + target_kind, +): + session_dir = _isolate_session_store(tmp_path, monkeypatch) + sid = f"{target_kind}unlinkfail1" + session = Session( + session_id=sid, + title="Required unlink must fail closed", + messages=[{"role": "user", "content": "retain on unlink failure"}], + ) + session.save() + primary = session_dir / f"{sid}.json" + backup = session_dir / f"{sid}.json.bak" + archive = session_dir / f"{sid}.json.bak.archive-deadbeef" + backup.write_text("backup", encoding="utf-8") + archive.write_text("archive", encoding="utf-8") + target = { + "primary": primary, + "backup": backup, + "archive": archive, + }[target_kind] + captured = _capture_post(monkeypatch, {"session_id": sid}) + monkeypatch.setattr(routes, "_lookup_cli_session_metadata", lambda value: {}) + monkeypatch.setattr(routes, "_is_messaging_session_id", lambda value: False) + state_db_deletes = [] + monkeypatch.setattr( + models, + "delete_cli_session", + lambda value: state_db_deletes.append(value) or True, + ) + real_unlink = Path.unlink + + def fail_required_unlink(path, *args, **kwargs): + if path == target: + raise PermissionError(f"{target_kind} session file locked") + return real_unlink(path, *args, **kwargs) + + monkeypatch.setattr(Path, "unlink", fail_required_unlink) + + assert routes.handle_post( + object(), + SimpleNamespace(path="/api/session/delete"), + ) is True + + assert captured["status"] == 500 + assert captured["payload"]["error"] == "Failed to delete session files" + assert target.exists() + if target_kind == "primary": + assert Session.load(sid) is not None + assert state_db_deletes == [] + + +def test_delete_session_fsyncs_tombstone_and_file_unlinks( + tmp_path, + monkeypatch, +): + session_dir = _isolate_session_store(tmp_path, monkeypatch) + sid = "durabledelete1" + session = Session( + session_id=sid, + title="Durable deletion", + messages=[{"role": "user", "content": "delete durably"}], + ) + session.save() + captured = _capture_post(monkeypatch, {"session_id": sid}) + monkeypatch.setattr(routes, "_lookup_cli_session_metadata", lambda value: {}) + monkeypatch.setattr(routes, "_is_messaging_session_id", lambda value: False) + monkeypatch.setattr(models, "delete_cli_session", lambda value: True) + fsynced = [] + monkeypatch.setattr( + models, + "_fsync_sidecar_directory", + lambda directory: fsynced.append(Path(directory)), + ) + + assert routes.handle_post( + object(), + SimpleNamespace(path="/api/session/delete"), + ) is True + + assert captured["status"] == 200 + assert fsynced == [session_dir, session_dir] + assert not (session_dir / f"{sid}.json").exists() assert sid in models._load_webui_deleted_session_tombstone() diff --git a/tests/test_issue3987_imported_session_titles.py b/tests/test_issue3987_imported_session_titles.py index 77ff5b26986..369cd24a87a 100644 --- a/tests/test_issue3987_imported_session_titles.py +++ b/tests/test_issue3987_imported_session_titles.py @@ -174,21 +174,13 @@ def test_generated_title_persist_reloads_latest_session_before_saving(tmp_path, stale.save(skip_index=True) stale_snapshot = models.Session.load(stale.session_id) - latest = models.Session( - session_id=stale.session_id, - title="CLI Session", - workspace=".", - model="test-model", - messages=[ - {"role": "user", "content": "first"}, - {"role": "assistant", "content": "reply"}, - {"role": "user", "content": "second"}, - ], - source_tag="cli", - raw_source="cli", - session_source="external_agent", - source_label="CLI", - ) + latest = models.Session.load(stale.session_id) + assert latest is not None + latest.messages = [ + {"role": "user", "content": "first"}, + {"role": "assistant", "content": "reply"}, + {"role": "user", "content": "second"}, + ] latest.save(skip_index=True) cache[latest.session_id] = latest diff --git a/tests/test_issue765_streaming_persistence.py b/tests/test_issue765_streaming_persistence.py index c3689b6976c..2eb30864701 100644 --- a/tests/test_issue765_streaming_persistence.py +++ b/tests/test_issue765_streaming_persistence.py @@ -277,28 +277,42 @@ class TestIssue765FollowupHardening: an exception fires before the checkpoint thread is created. """ - def test_same_session_concurrent_saves_use_distinct_temp_files(self, monkeypatch): - """Two concurrent saves of the same session must not collide on one tmp path. - - The key regression guard here is that each save call should reach os.replace() - with a distinct source tmp path. With the old shared `.tmp` scheme, both - threads would target the same path and the second replace would deterministically - fail once the first consume/remove happened. + def test_same_session_concurrent_saves_are_serialized_with_distinct_temp_files( + self, + monkeypatch, + ): + """Same-session saves serialize and never collide on one tmp path. + + Each call must reach os.replace() with a distinct source tmp path, but the + sidecar authority must prevent simultaneous publication for the same SID. """ s = _make_session("same_sid") s.save(skip_index=True) # seed the file on disk original_replace = models.os.replace - barrier = threading.Barrier(2) replace_sources = [] errors = [] + publication_lock = threading.Lock() + active_publications = 0 + max_active_publications = 0 + + def _tracked_replace(src, dst): + nonlocal active_publications, max_active_publications + with publication_lock: + replace_sources.append(str(src)) + active_publications += 1 + max_active_publications = max( + max_active_publications, + active_publications, + ) + try: + time.sleep(0.05) + return original_replace(src, dst) + finally: + with publication_lock: + active_publications -= 1 - def _replace_with_barrier(src, dst): - replace_sources.append(str(src)) - barrier.wait(timeout=5) - return original_replace(src, dst) - - monkeypatch.setattr(models.os, "replace", _replace_with_barrier) + monkeypatch.setattr(models.os, "replace", _tracked_replace) def _save_worker(): try: @@ -316,9 +330,10 @@ def _save_worker(): assert not errors, f"Concurrent same-session saves should not fail: {errors}" assert len(replace_sources) >= 2, f"Expected replace calls, got {replace_sources}" assert len(set(replace_sources)) == 2, ( - "Concurrent same-session saves must use distinct temp files even if Windows-safe " - f"replace retries one of them; got {replace_sources}" + "Concurrent same-session saves must use distinct temp files; " + f"got {replace_sources}" ) + assert max_active_publications == 1 data = json.loads(s.path.read_text(encoding="utf-8")) assert data["session_id"] == "same_sid" diff --git a/tests/test_metadata_save_wipe_1558.py b/tests/test_metadata_save_wipe_1558.py index 4228b6684ef..e1778c83522 100644 --- a/tests/test_metadata_save_wipe_1558.py +++ b/tests/test_metadata_save_wipe_1558.py @@ -183,17 +183,13 @@ def test_save_writes_bak_when_messages_shrink(temp_session_dir): from api.models import Session sid = _make_session_on_disk(temp_session_dir, n_msgs=1000, with_active_stream=False) - # Build a fresh in-memory Session with a smaller messages array, then save — + # Load the durable owner, apply a smaller messages array, then save — # this models the precise failure shape of #1558 (a caller mutates messages - # downward and saves). We construct the Session directly rather than going - # through get_session() so we don't trigger _repair_stale_pending side-effects. - s = Session( - session_id=sid, - title="t", - workspace="", - model="m", - messages=[{"role": "user", "content": f"m{i}"} for i in range(500)], - ) + # downward and saves). Session.load() avoids get_session() repair side-effects + # while retaining the revision token required by the sidecar CAS contract. + s = Session.load(sid) + assert s is not None + s.messages = s.messages[:500] s.save() bak_path = temp_session_dir / f"{sid}.json.bak" @@ -213,14 +209,10 @@ def test_save_does_not_write_bak_when_messages_grow(temp_session_dir): from api.models import Session sid = _make_session_on_disk(temp_session_dir, n_msgs=1000, with_active_stream=False) - # Build a session with MORE messages than on disk — the normal grow path. - s = Session( - session_id=sid, - title="t", - workspace="", - model="m", - messages=[{"role": "user", "content": f"m{i}"} for i in range(1001)], - ) + # Load the current owner and append one message — the normal grow path. + s = Session.load(sid) + assert s is not None + s.messages.append({"role": "user", "content": "m1000"}) s.save() bak_path = temp_session_dir / f"{sid}.json.bak" diff --git a/tests/test_pr1341_context_window_persistence.py b/tests/test_pr1341_context_window_persistence.py index 8fa9a4fa573..145eb98c6fa 100644 --- a/tests/test_pr1341_context_window_persistence.py +++ b/tests/test_pr1341_context_window_persistence.py @@ -16,6 +16,7 @@ Implementation reference: api/streaming.py around line 2188 (the per-turn post-merge save) writes from getattr(agent, 'context_compressor', None). """ +import ast import re from pathlib import Path @@ -74,13 +75,29 @@ def test_streaming_persists_context_fields_on_session_before_save(): def test_session_init_accepts_context_fields(): """Session.__init__ must accept the three fields as named kwargs.""" src = MODELS.read_text(encoding="utf-8") - # The init signature spans many lines — read the full def block - init_match = re.search(r"def __init__\(self,(.*?)\):", src, re.DOTALL) - assert init_match, "Session.__init__ signature not found" - sig = init_match.group(1) - assert "context_length" in sig, "Session.__init__ must accept context_length" - assert "threshold_tokens" in sig, "Session.__init__ must accept threshold_tokens" - assert "last_prompt_tokens" in sig, "Session.__init__ must accept last_prompt_tokens" + module = ast.parse(src) + session_class = next( + ( + node + for node in module.body + if isinstance(node, ast.ClassDef) and node.name == "Session" + ), + None, + ) + assert session_class is not None, "Session class not found" + init = next( + ( + node + for node in session_class.body + if isinstance(node, ast.FunctionDef) and node.name == "__init__" + ), + None, + ) + assert init is not None, "Session.__init__ signature not found" + parameters = {arg.arg for arg in (*init.args.args, *init.args.kwonlyargs)} + assert "context_length" in parameters, "Session.__init__ must accept context_length" + assert "threshold_tokens" in parameters, "Session.__init__ must accept threshold_tokens" + assert "last_prompt_tokens" in parameters, "Session.__init__ must accept last_prompt_tokens" def test_session_metadata_fields_includes_context_fields(): diff --git a/tests/test_provider_mismatch.py b/tests/test_provider_mismatch.py index d22be3b9687..45513434a92 100644 --- a/tests/test_provider_mismatch.py +++ b/tests/test_provider_mismatch.py @@ -1081,6 +1081,18 @@ def end_headers(self): assert captured_thread["kwargs"]["model_provider"] == "openai-codex" assert save_calls[-1]["model_provider"] == "openai-codex" + # FakeThread intentionally does not execute the real worker, so reproduce + # its teardown rather than leaking an active-stream marker into later + # sidebar-cache tests. + stream_id = payload["stream_id"] + with routes.STREAMS_LOCK: + routes.STREAMS.pop(stream_id, None) + routes.unregister_stream_owner(stream_id) + from api.config import clear_session_writeback_owner_if_owned + + clear_session_writeback_owner_if_owned(session.session_id, stream_id) + session.active_stream_id = None + def test_stale_at_provider_model_falls_back_when_family_mismatches(monkeypatch): """Unroutable @provider:model should not invent a bare model for another family.""" diff --git a/tests/test_regressions.py b/tests/test_regressions.py index caf171d54c3..9c2b2a822ba 100644 --- a/tests/test_regressions.py +++ b/tests/test_regressions.py @@ -46,17 +46,17 @@ def _make_session_visible(sid): from api.models import Session from tests.conftest import TEST_WORKSPACE - session = Session( - session_id=sid, - title="regression-test-delete-R8", - workspace=str(TEST_WORKSPACE), - model="test", - created_at=time.time(), - updated_at=time.time(), - profile="default", - messages=[{"role": "user", "content": "visible row", "timestamp": time.time()}], - tool_calls=[], - ) + session = Session.load(sid) + if session is None: + session = Session(session_id=sid) + session.title = "regression-test-delete-R8" + session.workspace = str(TEST_WORKSPACE) + session.model = "test" + session.profile = "default" + session.messages = [ + {"role": "user", "content": "visible row", "timestamp": time.time()} + ] + session.tool_calls = [] session.save(touch_updated_at=False) @@ -354,7 +354,16 @@ def test_server_delete_prunes_session_index(cleanup_test_sessions): text.find('if parsed.path == "/api/session/delete":'), ) if delete_idx >= 0: - delete_block = text[delete_idx:delete_idx+2400] + clear_indices = [ + idx + for idx in ( + text.find("if parsed.path == '/api/session/clear':", delete_idx), + text.find('if parsed.path == "/api/session/clear":', delete_idx), + ) + if idx > delete_idx + ] + delete_end = min(clear_indices) if clear_indices else len(text) + delete_block = text[delete_idx:delete_end] assert "prune_session_from_index(sid)" in delete_block, \ f"{label} session/delete must prune SESSION_INDEX_FILE" return @@ -369,9 +378,31 @@ def test_server_delete_removes_session_bak_snapshot(cleanup_test_sessions): routes_src.find('if parsed.path == "/api/session/delete":'), ) assert delete_idx >= 0, "session/delete handler not found in api/routes.py" - delete_block = routes_src[delete_idx:delete_idx+2400] - assert "with_suffix('.json.bak').unlink" in delete_block or 'with_suffix(".json.bak").unlink' in delete_block, \ - "session/delete must unlink .json.bak to avoid later orphan-backup recovery" + clear_indices = [ + idx + for idx in ( + routes_src.find("if parsed.path == '/api/session/clear':", delete_idx), + routes_src.find('if parsed.path == "/api/session/clear":', delete_idx), + ) + if idx > delete_idx + ] + delete_end = min(clear_indices) if clear_indices else len(routes_src) + delete_block = routes_src[delete_idx:delete_end] + assert "_delete_session_sidecar_artifacts_locked(" in delete_block, ( + "session/delete must route all sidecar removal through the canonical helper" + ) + + import inspect + from api.models import _delete_session_sidecar_artifacts_locked + + helper_src = inspect.getsource(_delete_session_sidecar_artifacts_locked) + assert 'backup = sidecar.with_suffix(".json.bak")' in helper_src + assert "artifact.unlink(missing_ok=True)" in helper_src, ( + "the canonical delete helper must unlink .json.bak" + ) + assert "bak.archive-*" in helper_src, ( + "the canonical delete helper must unlink versioned backup archives" + ) # ── R9: Token/tool SSE events write to wrong session after switch ───────────── diff --git a/tests/test_session_db_sidecar_reconciliation.py b/tests/test_session_db_sidecar_reconciliation.py index ba27e574f36..2b85d1a5422 100644 --- a/tests/test_session_db_sidecar_reconciliation.py +++ b/tests/test_session_db_sidecar_reconciliation.py @@ -1,5 +1,6 @@ import json import sqlite3 +from contextlib import contextmanager from api.session_recovery import recover_missing_sidecars_from_state_db, audit_session_recovery @@ -69,30 +70,242 @@ def test_recovered_sidecar_derives_message_count_from_materialized_rows(tmp_path monkeypatch.setattr(models, "SESSION_DIR", tmp_path) monkeypatch.setattr(models, "SESSION_INDEX_FILE", tmp_path / "_index.json") - active_empty = models.Session( - session_id=sid, - title="Recovered from DB", - workspace=str(tmp_path), - model="openai/gpt-5", - messages=[], - active_stream_id="a" * 32, - pending_user_message="still typing", - ) + active_empty = models.Session.load(sid) + assert active_empty is not None + active_empty.messages = [] + active_empty.active_stream_id = "a" * 32 + active_empty.pending_user_message = "still typing" active_empty.save() assert len(json.loads(sidecar.read_text(encoding="utf-8"))["messages"]) == 5 - shrinking = models.Session( - session_id=sid, - title="Recovered from DB", - workspace=str(tmp_path), - model="openai/gpt-5", - messages=[{"role": "user", "content": f"replacement {i}"} for i in range(3)], - ) + shrinking = models.Session.load(sid) + assert shrinking is not None + shrinking.messages = [{"role": "user", "content": f"replacement {i}"} for i in range(3)] shrinking.save() backup = sidecar.with_suffix(".json.bak") assert len(json.loads(backup.read_text(encoding="utf-8"))["messages"]) == 5 +def test_recover_missing_sidecar_rereads_state_db_under_sid_authority( + tmp_path, + monkeypatch, +): + import api.models as models + + state_db = tmp_path / "state.db" + sid = _make_state_db(state_db, sid="state_changes_before_lock", messages=1) + real_authority = models._session_sidecar_authority + updated = False + + @contextmanager + def update_before_recovery_enters_authority(session_id, *, session_dir=None): + nonlocal updated + with real_authority(session_id, session_dir=session_dir): + if not updated: + with sqlite3.connect(state_db) as conn: + conn.execute( + "INSERT INTO messages " + "(session_id, role, content, timestamp) VALUES (?, ?, ?, ?)", + (sid, "assistant", "committed while recovery waited", 1235.0), + ) + conn.execute( + "UPDATE sessions SET message_count = 2 WHERE id = ?", + (sid,), + ) + updated = True + yield + + monkeypatch.setattr( + models, + "_session_sidecar_authority", + update_before_recovery_enters_authority, + ) + + result = recover_missing_sidecars_from_state_db(tmp_path, state_db) + + assert result["materialized"] == 1 + data = json.loads((tmp_path / f"{sid}.json").read_text(encoding="utf-8")) + assert [message["content"] for message in data["messages"]] == [ + "message 1", + "committed while recovery waited", + ] + + +def test_recover_reread_uses_one_sqlite_snapshot_for_metadata_and_messages( + tmp_path, + monkeypatch, +): + from api import session_recovery + + state_db = tmp_path / "state.db" + sid = _make_state_db(state_db, sid="coherent_snapshot", messages=1) + real_connect = sqlite3.connect + with real_connect(state_db) as conn: + conn.execute("PRAGMA journal_mode=WAL") + + scans = 0 + + class _CursorProxy: + def __init__(self, cursor, mutate_after_fetch=False): + self._cursor = cursor + self._mutate_after_fetch = mutate_after_fetch + + def fetchall(self): + rows = self._cursor.fetchall() + if self._mutate_after_fetch: + with real_connect(state_db) as writer: + writer.execute( + "UPDATE sessions SET title = ?, message_count = 2 WHERE id = ?", + ("Committed replacement", sid), + ) + writer.execute( + "INSERT INTO messages " + "(session_id, role, content, timestamp) VALUES (?, ?, ?, ?)", + (sid, "assistant", "committed replacement", 1235.0), + ) + return rows + + class _ConnectionProxy: + def __init__(self, connection): + self._connection = connection + + @property + def row_factory(self): + return self._connection.row_factory + + @row_factory.setter + def row_factory(self, value): + self._connection.row_factory = value + + def execute(self, sql, parameters=()): + nonlocal scans + cursor = self._connection.execute(sql, parameters) + normalized = " ".join(sql.split()).lower() + mutate = False + if " from sessions " in f" {normalized} " and "source = 'webui'" in normalized: + scans += 1 + mutate = scans == 2 + return _CursorProxy(cursor, mutate_after_fetch=mutate) + + def close(self): + self._connection.close() + + def proxied_connect(*args, **kwargs): + return _ConnectionProxy(real_connect(*args, **kwargs)) + + monkeypatch.setattr(session_recovery.sqlite3, "connect", proxied_connect) + + result = recover_missing_sidecars_from_state_db(tmp_path, state_db) + + assert result["materialized"] == 1 + data = json.loads((tmp_path / f"{sid}.json").read_text(encoding="utf-8")) + observed = ( + data["title"], + data["message_count"], + tuple(message["content"] for message in data["messages"]), + ) + assert observed in { + ("Recovered from DB", 1, ("message 1",)), + ("Committed replacement", 2, ("message 1", "committed replacement")), + } + + +def test_recovered_sidecar_fsyncs_temp_before_create_only_publication( + tmp_path, + monkeypatch, +): + from api import models, session_recovery + + state_db = tmp_path / "state.db" + sid = _make_state_db(state_db, sid="durable_materialization", messages=1) + events = [] + monkeypatch.setattr( + session_recovery.os, + "fsync", + lambda _fd: events.append("file"), + ) + monkeypatch.setattr( + models, + "_fsync_sidecar_directory", + lambda _directory: events.append("directory"), + ) + + result = recover_missing_sidecars_from_state_db(tmp_path, state_db) + + assert result["materialized"] == 1 + assert events == ["file", "directory"] + assert (tmp_path / f"{sid}.json").exists() + + +def test_hidden_background_cleanup_cannot_be_recreated_from_state_db( + tmp_path, + monkeypatch, +): + from api import models, routes + + state_db = tmp_path / "state.db" + sid = _make_state_db(state_db, sid="hidden_background_lifecycle", messages=1) + monkeypatch.setattr(models, "SESSION_DIR", tmp_path) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", tmp_path / "_index.json") + monkeypatch.setattr(routes, "SESSION_DIR", tmp_path) + monkeypatch.setattr(models, "delete_cli_session", lambda _sid: False) + session = models.Session( + session_id=sid, + title="bg: hidden result", + messages=[{"role": "assistant", "content": "hidden result"}], + ) + session.save(skip_index=True) + + routes._delete_hidden_background_session_sidecar(sid) + result = recover_missing_sidecars_from_state_db(tmp_path, state_db) + + assert result["materialized"] == 0 + assert not (tmp_path / f"{sid}.json").exists() + assert sid in models._load_webui_deleted_session_tombstone() + + +def test_delete_at_tombstone_cap_retains_current_sid_and_blocks_state_db_recovery( + tmp_path, + monkeypatch, +): + from api import models + + state_db = tmp_path / "state.db" + sid = _make_state_db( + state_db, + sid="a-target-deleted-session", + messages=1, + ) + monkeypatch.setattr(models, "SESSION_DIR", tmp_path) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", tmp_path / "_index.json") + models._save_webui_deleted_session_tombstone( + {f"z-{index:04d}" for index in range(models.WEBUI_DELETED_SESSION_TOMBSTONE_CAP)} + ) + session = models.Session( + session_id=sid, + messages=[{"role": "user", "content": "deleted but retained in state.db"}], + ) + session.save(skip_index=True) + + with models._session_sidecar_authority(sid): + deleted = models._delete_session_sidecar_artifacts_locked(sid) + + retained = models._load_webui_deleted_session_tombstone() + expected_retained = { + sid, + *(f"z-{index:04d}" for index in range(1, models.WEBUI_DELETED_SESSION_TOMBSTONE_CAP)), + } + assert deleted is True + assert retained == frozenset(expected_retained) + assert not (tmp_path / f"{sid}.json").exists() + + result = recover_missing_sidecars_from_state_db(tmp_path, state_db) + + assert result["materialized"] == 0 + assert not (tmp_path / f"{sid}.json").exists() + + + def test_recover_missing_sidecars_from_state_db_skips_deleted_webui_tombstone(tmp_path, monkeypatch): import api.models as _m monkeypatch.setattr(_m, "SESSION_DIR", tmp_path) diff --git a/tests/test_session_discoverability_repair.py b/tests/test_session_discoverability_repair.py index c43c9fc11b3..fe609c17af2 100644 --- a/tests/test_session_discoverability_repair.py +++ b/tests/test_session_discoverability_repair.py @@ -2,6 +2,7 @@ import sqlite3 import subprocess import sys +import threading from pathlib import Path from api.session_discoverability import repair_session_discoverability @@ -147,6 +148,150 @@ def test_repair_discoverability_apply_backs_up_and_repairs_safe_findings(tmp_pat assert "state.db" in backed_up +def test_clear_sidecar_cli_flag_never_silently_overwrites_successful_save( + tmp_path, + monkeypatch, +): + from api import models + from api import session_discoverability as discoverability + + sid = "repair-save-race" + session_path = _write_sidecar( + tmp_path, + sid, + messages=1, + title="repair snapshot", + source_tag="webui", + session_source="webui", + is_cli_session=True, + ) + monkeypatch.setattr(models, "SESSION_DIR", tmp_path) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", tmp_path / "_index.json") + writer = models.Session.load(sid) + assert writer is not None + writer.title = "concurrent writer" + repair_paused = threading.Event() + release_repair = threading.Event() + writer_done = threading.Event() + real_backup = discoverability._backup_file + + def blocking_backup(path, backup_dir, backed_up): + if Path(path) == session_path: + repair_paused.set() + assert release_repair.wait(timeout=10) + return real_backup(path, backup_dir, backed_up) + + monkeypatch.setattr(discoverability, "_backup_file", blocking_backup) + repair_result = {} + writer_result = {} + + def run_repair(): + repair_result.update( + discoverability._clear_sidecar_cli_flag( + tmp_path, + sid, + tmp_path / "backup", + {}, + ) + ) + + def run_writer(): + try: + writer.save(skip_index=True) + except models.StaleSessionGenerationError: + writer_result["status"] = "stale" + else: + writer_result["status"] = "saved" + finally: + writer_done.set() + + repair_thread = threading.Thread(target=run_repair) + writer_thread = threading.Thread(target=run_writer) + repair_thread.start() + assert repair_paused.wait(timeout=10) + writer_thread.start() + writer_finished_before_repair = writer_done.wait(timeout=0.25) + release_repair.set() + repair_thread.join(timeout=10) + writer_thread.join(timeout=10) + assert not repair_thread.is_alive() + assert not writer_thread.is_alive() + + persisted = json.loads(session_path.read_text(encoding="utf-8")) + assert repair_result["applied"] is True + assert persisted["is_cli_session"] is False + assert not ( + writer_result["status"] == "saved" + and persisted["title"] != "concurrent writer" + ) + assert writer_finished_before_repair is False + assert writer_result["status"] == "stale" + + +def test_materialize_sidecar_does_not_resurrect_concurrent_tombstone( + tmp_path, + monkeypatch, +): + from api import models + from api import session_discoverability as discoverability + + sid = "repair-delete-race" + db = _state_db( + tmp_path, + [{"id": sid, "source": "webui", "message_count": 1}], + {sid: 1}, + ) + target = tmp_path / f"{sid}.json" + monkeypatch.setattr(models, "SESSION_DIR", tmp_path) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", tmp_path / "_index.json") + repair_paused = threading.Event() + release_repair = threading.Event() + delete_done = threading.Event() + real_backup = discoverability._backup_file + + def blocking_backup(path, backup_dir, backed_up): + if Path(path) == db: + repair_paused.set() + assert release_repair.wait(timeout=10) + return real_backup(path, backup_dir, backed_up) + + monkeypatch.setattr(discoverability, "_backup_file", blocking_backup) + repair_result = {} + + def run_repair(): + repair_result.update( + discoverability._materialize_sidecar_from_state_db( + tmp_path, + db, + sid, + tmp_path / "backup", + {}, + ) + ) + + def run_delete(): + with models._session_sidecar_authority(sid, session_dir=tmp_path): + models._record_webui_deleted_session_tombstone(sid) + target.unlink(missing_ok=True) + delete_done.set() + + repair_thread = threading.Thread(target=run_repair) + delete_thread = threading.Thread(target=run_delete) + repair_thread.start() + assert repair_paused.wait(timeout=10) + delete_thread.start() + delete_finished_before_repair = delete_done.wait(timeout=0.25) + release_repair.set() + repair_thread.join(timeout=10) + delete_thread.join(timeout=10) + assert not repair_thread.is_alive() + assert not delete_thread.is_alive() + + assert repair_result["applied"] is True + assert delete_finished_before_repair is False + assert not target.exists() + + def test_repair_discoverability_cli_defaults_to_dry_run(tmp_path): sid = "webui-stale-cli-flag" _write_sidecar(tmp_path, sid, messages=1, source_tag="webui", session_source="webui", is_cli_session=True) diff --git a/tests/test_session_index.py b/tests/test_session_index.py index 0f66ff846aa..db9a4115126 100644 --- a/tests/test_session_index.py +++ b/tests/test_session_index.py @@ -1412,6 +1412,12 @@ def test_concurrent_saves_dont_lose_data(): # Build initial index _write_session_index(updates=None) + # Raw fixture writes bypass Session.save(), so reload the durable revisions + # before exercising concurrent mutations under the sidecar CAS contract. + sA = models.Session.load("sess_a") + sB = models.Session.load("sess_b") + assert sA is not None and sB is not None + # Now update both sessions concurrently barrier = threading.Event() errors = [] diff --git a/tests/test_session_lost_response_regression.py b/tests/test_session_lost_response_regression.py index df8e91fa7f6..ca9112a2b54 100644 --- a/tests/test_session_lost_response_regression.py +++ b/tests/test_session_lost_response_regression.py @@ -1147,9 +1147,70 @@ def test_sync_persists_recovered_state_db_tail_when_stream_dead(monkeypatch): assert s.pending_user_message is None assert [m["content"] for m in s.messages] == [m["content"] for m in state_messages] + s.title = "save after recovered tail" + s.save(skip_index=True) + reloaded = Session.load(sid) assert reloaded.active_stream_id is None assert reloaded.messages[-1]["content"] == "recovered tail" + assert reloaded.title == "save after recovered tail" + + +def test_sync_returns_fresh_owner_when_alias_did_not_own_pre_save_revision( + monkeypatch, +): + sid = "state_sync_stale_alias_sid" + stream_id = "stream_dead_stale_alias" + durable = Session( + session_id=sid, + title="Durable owner", + messages=[ + {"role": "user", "content": "old question", "timestamp": 100.0}, + {"role": "assistant", "content": "old answer", "timestamp": 101.0}, + ], + active_stream_id=stream_id, + pending_user_message="new request", + pending_started_at=0, + ) + durable.save() + stale_alias = Session( + session_id=sid, + title="Stale alias", + messages=list(durable.messages), + active_stream_id=stream_id, + pending_user_message="new request", + pending_started_at=0, + ) + state_messages = [ + *durable.messages, + {"role": "user", "content": "new request", "timestamp": 102.0}, + {"role": "assistant", "content": "recovered tail", "timestamp": 103.0}, + ] + monkeypatch.setattr( + models, + "get_state_db_session_summary", + lambda sid_arg, profile=None: { + "message_count": len(state_messages), + "last_message_at": 103.0, + }, + ) + monkeypatch.setattr( + models, + "get_state_db_session_messages", + lambda sid_arg, **kwargs: list(state_messages), + ) + + recovered_owner = models._sync_sidecar_from_state_db_if_newer(stale_alias) + + assert isinstance(recovered_owner, Session) + assert recovered_owner is not stale_alias + recovered_owner.title = "Fresh owner remains saveable" + recovered_owner.save(skip_index=True) + reloaded = Session.load(sid) + assert reloaded.title == "Fresh owner remains saveable" + assert reloaded.messages[-1]["content"] == "recovered tail" + with pytest.raises(models.StaleSessionGenerationError): + stale_alias.save(skip_index=True) def test_sync_skips_during_registration_window_recent_pending(monkeypatch): diff --git a/tests/test_session_save_empty_pending_guard.py b/tests/test_session_save_empty_pending_guard.py index 3753b6d7d59..7bccc21c0ae 100644 --- a/tests/test_session_save_empty_pending_guard.py +++ b/tests/test_session_save_empty_pending_guard.py @@ -2,6 +2,8 @@ import json +import pytest + import api.config as config import api.models as models from api.models import Session @@ -33,7 +35,8 @@ def test_empty_active_pending_save_cannot_overwrite_existing_messages(tmp_path, pending_user_message="prompt", pending_started_at=123.0, ) - stale.save() + with pytest.raises(models.StaleSessionGenerationError): + stale.save() persisted = json.loads((session_dir / f"{sid}.json").read_text(encoding="utf-8")) assert [m["content"] for m in persisted["messages"]] == ["prompt", "answer"] diff --git a/tests/test_session_sidecar_revision_fence.py b/tests/test_session_sidecar_revision_fence.py new file mode 100644 index 00000000000..f46afaa0c03 --- /dev/null +++ b/tests/test_session_sidecar_revision_fence.py @@ -0,0 +1,2009 @@ +import builtins +import errno +import hashlib +import json +import logging +import multiprocessing +import os +import queue +import sqlite3 +import subprocess +import sys +import threading +import time +import types +from contextlib import contextmanager +from pathlib import Path +from unittest import mock + +import pytest + + +def _patch_store(monkeypatch, models, session_dir: Path) -> None: + session_dir.mkdir(parents=True, exist_ok=True) + monkeypatch.setattr(models, "SESSION_DIR", session_dir) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", session_dir / "_index.json") + with models.LOCK: + models.SESSIONS.clear() + + +def _seed_sidecar_delete_artifacts(sidecar: Path) -> tuple[list[Path], Path]: + artifacts = [ + sidecar, + sidecar.with_suffix(".json.bak"), + sidecar.with_name(f"{sidecar.name}.bak.archive-deadbeef"), + sidecar.with_name(f"{sidecar.name}.replay-v10.deadbeef.bak"), + sidecar.with_name(f"_replay-v10.{sidecar.name}.deadbeef.manifest.json"), + sidecar.with_name(f".{sidecar.name}.replay-v10.tmp.probe"), + sidecar.with_name(f".{sidecar.name}.replay-v10.restore.probe"), + sidecar.with_name( + f"._replay-v10.{sidecar.name}.deadbeef.manifest.json.tmp.probe" + ), + ] + for artifact in artifacts[1:]: + artifact.write_text("recoverable hidden transcript", encoding="utf-8") + unrelated = sidecar.with_name("unrelated.json.replay-v10.deadbeef.bak") + unrelated.write_text("must survive", encoding="utf-8") + return artifacts, unrelated + + +def _process_writer( + session_dir, sid, marker, start_event, ready_queue, result_queue +): + from api import models + + models.SESSION_DIR = Path(session_dir) + models.SESSION_INDEX_FILE = Path(session_dir) / "_index.json" + session = models.Session.load(sid) + assert session is not None + session.messages.append({"role": "assistant", "content": marker}) + ready_queue.put(marker) + start_event.wait(timeout=15) + try: + session.save(skip_index=True) + except models.StaleSessionGenerationError: + result_queue.put((marker, "stale")) + else: + result_queue.put((marker, "saved")) + + +def _process_cleanup_race_writer( + session_dir, + sid, + marker, + start_event, + ready_event, + done_event, + result_queue, +): + from api import models + + models.SESSION_DIR = Path(session_dir) + models.SESSION_INDEX_FILE = Path(session_dir) / "_index.json" + session = models.Session.load(sid) + assert session is not None + session.messages.append({"role": "assistant", "content": marker}) + ready_event.set() + start_event.wait(timeout=15) + try: + session.save(skip_index=True) + except models.StaleSessionGenerationError: + result_queue.put("stale") + except Exception as exc: + result_queue.put(f"{type(exc).__name__}:{exc}") + else: + result_queue.put("saved") + finally: + done_event.set() + + +def _process_record_deleted_tombstone( + session_dir, + sid, + peer_sid, + result_queue, +): + from api import models + + models.SESSION_DIR = Path(session_dir) + models.SESSION_INDEX_FILE = Path(session_dir) / "_index.json" + real_load = models._load_webui_deleted_session_tombstone + + def load_then_wait_for_peer(): + current = real_load() + marker_dir = Path(session_dir) / "tombstone-read-markers" + marker_dir.mkdir(parents=True, exist_ok=True) + (marker_dir / sid).write_text("read", encoding="utf-8") + deadline = time.monotonic() + 3 + while not (marker_dir / peer_sid).exists() and time.monotonic() < deadline: + time.sleep(0.01) + return current + + models._load_webui_deleted_session_tombstone = load_then_wait_for_peer + try: + models._record_webui_deleted_session_tombstone(sid) + except Exception as exc: + result_queue.put((sid, type(exc).__name__, str(exc))) + else: + result_queue.put((sid, "ok", "")) + + +def _process_save_session_for_lock_namespace(session_dir, sid, result_queue): + from api import models + + models.SESSION_DIR = Path(session_dir) + models.SESSION_INDEX_FILE = Path(session_dir) / "_index.json" + try: + session = models.Session( + session_id=sid, + workspace=str(session_dir), + messages=[{"role": "user", "content": "lock namespace probe"}], + ) + session.save(skip_index=True) + except Exception as exc: + result_queue.put((sid, type(exc).__name__, str(exc))) + else: + result_queue.put((sid, "ok", "")) + + +def test_deleted_session_tombstone_rmw_is_cross_process_serialized( + tmp_path, + monkeypatch, +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + context = multiprocessing.get_context("spawn" if os.name == "nt" else "fork") + result_queue = context.Queue() + session_ids = ("deleted-a", "deleted-b") + processes = [ + context.Process( + target=_process_record_deleted_tombstone, + args=(session_dir, sid, session_ids[1 - index], result_queue), + ) + for index, sid in enumerate(session_ids) + ] + try: + for process in processes: + process.start() + results = [result_queue.get(timeout=15) for _ in processes] + for process in processes: + process.join(timeout=15) + assert all(not process.is_alive() for process in processes) + assert all(process.exitcode == 0 for process in processes) + assert {result[:2] for result in results} == { + ("deleted-a", "ok"), + ("deleted-b", "ok"), + } + assert models._load_webui_deleted_session_tombstone() == frozenset( + session_ids + ) + finally: + for process in processes: + if process.is_alive(): + process.terminate() + process.join(timeout=5) + + +def test_global_tombstone_lock_namespace_cannot_alias_session_sid( + tmp_path, + monkeypatch, +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + context = multiprocessing.get_context("spawn" if os.name == "nt" else "fork") + for sid in ( + "ordinary-lock-namespace", + models._WEBUI_DELETED_SESSION_TOMBSTONE_LOCK_SID, + ): + result_queue = context.Queue() + process = context.Process( + target=_process_save_session_for_lock_namespace, + args=(session_dir, sid, result_queue), + ) + try: + process.start() + process.join(timeout=5) + assert not process.is_alive(), f"save deadlocked for accepted SID {sid!r}" + assert process.exitcode == 0 + assert result_queue.get(timeout=2)[:2] == (sid, "ok") + finally: + if process.is_alive(): + process.terminate() + process.join(timeout=2) + + +def test_existing_deleted_session_tombstone_retries_directory_fsync( + tmp_path, + monkeypatch, +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + fsynced = [] + monkeypatch.setattr( + models, + "_fsync_sidecar_directory", + lambda directory: fsynced.append(Path(directory)), + ) + models._record_webui_deleted_session_tombstone("durable-retry") + fsynced.clear() + + models._record_webui_deleted_session_tombstone("durable-retry") + + assert fsynced == [session_dir] + + +def test_sidecar_delete_fails_closed_when_current_tombstone_cannot_be_verified( + tmp_path, + monkeypatch, +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "unverifiable-current-tombstone" + session = models.Session( + session_id=sid, + messages=[{"role": "user", "content": "must remain durable"}], + ) + session.save(skip_index=True) + artifacts, _unrelated = _seed_sidecar_delete_artifacts(session.path) + real_load = models._load_webui_deleted_session_tombstone + load_count = 0 + + def hide_current_sid_on_verification(): + nonlocal load_count + load_count += 1 + retained = real_load() + if load_count > 1: + return frozenset(candidate for candidate in retained if candidate != sid) + return retained + + monkeypatch.setattr( + models, + "_load_webui_deleted_session_tombstone", + hide_current_sid_on_verification, + ) + + with models._session_sidecar_authority(sid): + with pytest.raises(models.SessionDeleteTombstoneError): + models._delete_session_sidecar_artifacts_locked(sid) + + assert load_count == 2 + assert all(artifact.exists() for artifact in artifacts) + + +def test_import_clear_cannot_race_tombstone_verification_and_delete( + tmp_path, + monkeypatch, +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "import-delete-race" + seed = models.Session( + session_id=sid, + messages=[{"role": "user", "content": "before delete"}], + ) + seed.save(skip_index=True) + + recorded = threading.Event() + allow_delete = threading.Event() + import_ready_to_save = threading.Event() + allow_import_save = threading.Event() + errors: queue.Queue[BaseException] = queue.Queue() + delete_result: list[bool] = [] + imported: list[models.Session] = [] + + real_record = models._record_webui_deleted_session_tombstone + real_save = models.Session.save + + def record_then_pause(target_sid): + real_record(target_sid) + recorded.set() + assert allow_delete.wait(timeout=5) + + def save_after_delete(self, *args, **kwargs): + import_ready_to_save.set() + assert allow_import_save.wait(timeout=5) + return real_save(self, *args, **kwargs) + + monkeypatch.setattr( + models, + "_record_webui_deleted_session_tombstone", + record_then_pause, + ) + monkeypatch.setattr(models.Session, "save", save_after_delete) + + def delete_worker(): + try: + with models._session_sidecar_authority(sid): + delete_result.append( + models._delete_session_sidecar_artifacts_locked(sid) + ) + except BaseException as exc: # pragma: no cover - surfaced below + errors.put(exc) + + def import_worker(): + try: + imported.append( + models.import_cli_session( + sid, + "Imported", + [{"role": "user", "content": "after delete"}], + ) + ) + except BaseException as exc: # pragma: no cover - surfaced below + errors.put(exc) + + delete_thread = threading.Thread(target=delete_worker, name="delete-worker") + import_thread = threading.Thread(target=import_worker, name="import-worker") + delete_thread.start() + assert recorded.wait(timeout=5) + import_thread.start() + assert import_ready_to_save.wait(timeout=5) + + allow_delete.set() + delete_thread.join(timeout=5) + try: + assert not delete_thread.is_alive() + assert errors.empty(), list(errors.queue) + assert delete_result == [True] + assert sid in models._load_webui_deleted_session_tombstone() + assert not (session_dir / f"{sid}.json").exists() + finally: + allow_import_save.set() + import_thread.join(timeout=5) + + assert not import_thread.is_alive() + assert errors.empty(), list(errors.queue) + assert len(imported) == 1 + assert (session_dir / f"{sid}.json").exists() + assert sid not in models._load_webui_deleted_session_tombstone() + + +def test_hidden_background_cleanup_uses_agent_and_sidecar_authorities( + tmp_path, + monkeypatch, +): + from api import models, routes + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + monkeypatch.setattr(routes, "SESSION_DIR", session_dir) + monkeypatch.setattr(models, "delete_cli_session", lambda _sid: True) + sid = "hidden-background-cleanup" + seed = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[{"role": "assistant", "content": "background result"}], + ) + seed.save(skip_index=True) + stale_alias = models.Session.load(sid) + assert stale_alias is not None + with models.LOCK: + models.SESSIONS[sid] = stale_alias + backup = session_dir / f"{sid}.json.bak" + archive = session_dir / f"{sid}.json.bak.archive-deadbeef" + backup.write_text("backup", encoding="utf-8") + archive.write_text("archive", encoding="utf-8") + real_authority = models._session_sidecar_authority + authority_entered = threading.Event() + + @contextmanager + def observed_authority(session_id, *, session_dir=None): + authority_entered.set() + with real_authority(session_id, session_dir=session_dir): + yield + + monkeypatch.setattr(models, "_session_sidecar_authority", observed_authority) + fsynced = [] + monkeypatch.setattr( + models, + "_fsync_sidecar_directory", + lambda directory: fsynced.append(Path(directory)), + ) + agent_lock = routes._get_session_agent_lock(sid) + assert agent_lock.acquire(timeout=1) + started = threading.Event() + failures = [] + + def cleanup(): + started.set() + try: + routes._delete_hidden_background_session_sidecar(sid) + except Exception as exc: + failures.append(exc) + + thread = threading.Thread(target=cleanup) + try: + thread.start() + assert started.wait(timeout=1) + assert not authority_entered.wait(timeout=0.2) + assert (session_dir / f"{sid}.json").exists() + finally: + agent_lock.release() + assert authority_entered.wait(timeout=2) + thread.join(timeout=5) + + assert not thread.is_alive() + assert failures == [] + assert not (session_dir / f"{sid}.json").exists() + assert not backup.exists() + assert not archive.exists() + assert fsynced == [session_dir, session_dir] + with models.LOCK: + assert sid not in models.SESSIONS + with pytest.raises(models.StaleSessionGenerationError): + stale_alias.save(skip_index=True) + + +def test_hidden_ephemeral_cancel_cleanup_uses_durable_sidecar_delete_protocol( + tmp_path, + monkeypatch, +): + from api import models, streaming + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + monkeypatch.setattr(streaming, "SESSION_DIR", session_dir) + sid = "hidden-ephemeral-cancel" + session = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[{"role": "user", "content": "private side question"}], + ) + session.active_stream_id = "ephemeral-stream" + session.pending_user_message = "private side question" + session.pending_attachments = ["secret.txt"] + session.pending_started_at = time.time() + session.pending_user_source = "webui" + session.save(skip_index=True) + with models.LOCK: + models.SESSIONS[sid] = session + artifacts, unrelated = _seed_sidecar_delete_artifacts(session.path) + + with streaming._get_session_agent_lock(sid): + streaming._finalize_cancelled_turn(session, ephemeral=True) + + assert session.active_stream_id is None + assert session.pending_user_message is None + assert session.pending_attachments == [] + assert session.pending_started_at is None + assert session.pending_user_source is None + assert all(not artifact.exists() for artifact in artifacts) + assert unrelated.read_text(encoding="utf-8") == "must survive" + assert sid in models._load_webui_deleted_session_tombstone() + with models.LOCK: + assert sid not in models.SESSIONS + with pytest.raises(models.StaleSessionGenerationError): + session.save(skip_index=True) + + +def test_hidden_ephemeral_cleanup_logs_tombstone_failure_without_unlinking( + tmp_path, + monkeypatch, + caplog, +): + from api import models, streaming + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + monkeypatch.setattr(streaming, "SESSION_DIR", session_dir) + sid = "hidden-ephemeral-tombstone-failure" + session = models.Session(session_id=sid, workspace=str(tmp_path)) + session.active_stream_id = "ephemeral-stream" + session.save(skip_index=True) + with models.LOCK: + models.SESSIONS[sid] = session + artifacts, _unrelated = _seed_sidecar_delete_artifacts(session.path) + + def reject_tombstone(_sid): + raise OSError("synthetic tombstone persistence failure") + + monkeypatch.setattr( + models, + "_record_webui_deleted_session_tombstone", + reject_tombstone, + ) + with caplog.at_level(logging.WARNING, logger="api.streaming"): + with streaming._get_session_agent_lock(sid): + streaming._finalize_cancelled_turn(session, ephemeral=True) + + assert all(artifact.exists() for artifact in artifacts) + assert sid not in models._load_webui_deleted_session_tombstone() + with models.LOCK: + assert models.SESSIONS.get(sid) is session + assert f"cancelled ephemeral session {sid}" in caplog.text + assert "synthetic tombstone persistence failure" in caplog.text + + +def test_hidden_ephemeral_cleanup_rejects_noncanonical_session_path( + tmp_path, + monkeypatch, + caplog, +): + from api import models, streaming + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + monkeypatch.setattr(streaming, "SESSION_DIR", session_dir) + sid = "hidden-ephemeral-path-check" + session = models.Session(session_id=sid, workspace=str(tmp_path)) + session.save(skip_index=True) + outside = tmp_path / "outside.json" + outside.write_text("do not unlink", encoding="utf-8") + noncanonical = types.SimpleNamespace( + session_id=sid, + path=outside, + _sidecar_revisions=session._sidecar_revisions, + ) + + with caplog.at_level(logging.WARNING, logger="api.streaming"): + with streaming._get_session_agent_lock(sid): + deleted = streaming._cleanup_ephemeral_session_sidecar_locked( + noncanonical, + outcome="cancelled", + ) + + assert deleted is False + assert session.path.exists() + assert outside.read_text(encoding="utf-8") == "do not unlink" + assert sid not in models._load_webui_deleted_session_tombstone() + assert "does not match SID" in caplog.text + + +def test_hidden_ephemeral_normal_completion_uses_durable_sidecar_delete_protocol( + tmp_path, + monkeypatch, + caplog, +): + from api import config, models, streaming + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + monkeypatch.setattr(config, "SESSION_DIR", session_dir) + monkeypatch.setattr(streaming, "SESSION_DIR", session_dir) + sid = "hidden-ephemeral-complete" + stream_id = "hidden-ephemeral-stream" + session = models.Session( + session_id=sid, + workspace=str(tmp_path), + title="btw: private side question", + ) + session.active_stream_id = stream_id + session.pending_user_message = "private side question" + session.pending_started_at = time.time() + session.pending_user_source = "webui" + session.save(skip_index=True) + with models.LOCK: + models.SESSIONS[sid] = session + artifacts, unrelated = _seed_sidecar_delete_artifacts(session.path) + event_queue = queue.Queue() + streaming.STREAMS[stream_id] = event_queue + + class SuccessfulEphemeralAgent: + def __init__(self, **kwargs): + self.session_id = kwargs.get("session_id") + self.context_compressor = None + self.session_prompt_tokens = 3 + self.session_completion_tokens = 2 + self.session_estimated_cost_usd = 0.001 + self.session_cache_read_tokens = 0 + self.session_cache_write_tokens = 0 + self.ephemeral_system_prompt = None + self._last_error = None + + def run_conversation(self, **kwargs): + history = list(kwargs.get("conversation_history") or []) + return { + "messages": history + + [ + {"role": "user", "content": kwargs["persist_user_message"]}, + {"role": "assistant", "content": "private answer"}, + ] + } + + def interrupt(self, _message): + pass + + fake_runtime_module = types.ModuleType("hermes_cli.runtime_provider") + fake_runtime_module.resolve_runtime_provider = mock.Mock( + return_value={ + "provider": "openai", + "api_key": "synthetic-key", + "base_url": None, + } + ) + fake_hermes_cli = types.ModuleType("hermes_cli") + fake_hermes_cli.runtime_provider = fake_runtime_module + fake_hermes_state = types.ModuleType("hermes_state") + fake_hermes_state.SessionDB = mock.Mock(return_value=None) + monkeypatch.setitem(sys.modules, "hermes_cli", fake_hermes_cli) + monkeypatch.setitem( + sys.modules, + "hermes_cli.runtime_provider", + fake_runtime_module, + ) + monkeypatch.setitem(sys.modules, "hermes_state", fake_hermes_state) + monkeypatch.setattr(streaming, "get_session", lambda _sid: session) + monkeypatch.setattr( + streaming, + "_get_ai_agent", + lambda: SuccessfulEphemeralAgent, + ) + monkeypatch.setattr( + streaming, + "resolve_model_provider", + lambda *_args, **_kwargs: ("test-model", "openai", None), + ) + monkeypatch.setattr(config, "get_config", lambda: {}) + monkeypatch.setattr(config, "_resolve_cli_toolsets", lambda _cfg: []) + + with caplog.at_level(logging.ERROR, logger="api.streaming"): + streaming._run_agent_streaming( + session_id=sid, + msg_text="private side question", + model="test-model", + workspace=str(tmp_path), + stream_id=stream_id, + ephemeral=True, + ) + + assert any( + event == "done" and payload.get("answer") == "private answer" + for event, payload in list(event_queue.queue) + ) + assert all(not artifact.exists() for artifact in artifacts) + assert unrelated.read_text(encoding="utf-8") == "must survive" + assert sid in models._load_webui_deleted_session_tombstone() + assert "_last_resort_sync_from_core failed" not in caplog.text + with models.LOCK: + assert sid not in models.SESSIONS + + +@pytest.mark.parametrize( + ("zero_only", "title"), + [(False, "Untitled"), (True, "Named empty session")], +) +def test_empty_session_cleanup_fences_writer_between_check_and_delete( + tmp_path, + monkeypatch, + zero_only, + title, +): + from api import models, routes + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + monkeypatch.setattr(routes, "SESSION_DIR", session_dir) + monkeypatch.setattr(routes, "SESSION_INDEX_FILE", session_dir / "_index.json") + monkeypatch.setattr(routes, "SESSIONS", models.SESSIONS) + monkeypatch.setattr(routes, "LOCK", models.LOCK) + sid = f"cleanup-race-{'zero' if zero_only else 'normal'}" + marker = "UNIQUE DURABLE TURN" + models.Session( + session_id=sid, + title=title, + workspace=str(tmp_path), + messages=[], + ).save(skip_index=True) + sidecar = session_dir / f"{sid}.json" + backup = sidecar.with_suffix(".json.bak") + archive = backup.with_name(f"{backup.name}.archive-review-probe") + backup.write_text("offline backup", encoding="utf-8") + archive.write_text("offline archive", encoding="utf-8") + + context = multiprocessing.get_context("spawn" if os.name == "nt" else "fork") + start_event = context.Event() + ready_event = context.Event() + done_event = context.Event() + result_queue = context.Queue() + writer = context.Process( + target=_process_cleanup_race_writer, + args=( + session_dir, + sid, + marker, + start_event, + ready_event, + done_event, + result_queue, + ), + ) + writer.start() + assert ready_event.wait(timeout=15) + + original_load = models.Session.load.__func__ + + def load_then_commit_writer(cls, candidate_sid): + loaded = original_load(cls, candidate_sid) + if candidate_sid == sid: + start_event.set() + assert done_event.wait(timeout=15), "writer blocked after cleanup check" + return loaded + + monkeypatch.setattr(models.Session, "load", classmethod(load_then_commit_writer)) + monkeypatch.setattr(routes, "j", lambda _handler, payload: payload) + try: + result = routes._handle_sessions_cleanup( + object(), + {}, + zero_only=zero_only, + ) + # The fixed cleanup reads directly while holding SID authority, so the + # legacy Session.load interposition is not reached. Release the writer + # after deletion to prove its stale generation remains fenced. + start_event.set() + writer.join(timeout=20) + assert not writer.is_alive() + assert writer.exitcode == 0 + writer_result = result_queue.get(timeout=5) + finally: + if writer.is_alive(): + writer.terminate() + writer.join(timeout=5) + + assert result == {"ok": True, "cleaned": 1} + assert writer_result == "stale" + assert not sidecar.exists() + assert not backup.exists() + assert not archive.exists() + assert sid in models._load_webui_deleted_session_tombstone() + + +def test_empty_session_cleanup_does_not_count_partial_delete_as_index_ghost( + tmp_path, + monkeypatch, +): + from api import models, routes + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + index_path = session_dir / "_index.json" + monkeypatch.setattr(routes, "SESSION_DIR", session_dir) + monkeypatch.setattr(routes, "SESSION_INDEX_FILE", index_path) + monkeypatch.setattr(routes, "SESSIONS", models.SESSIONS) + monkeypatch.setattr(routes, "LOCK", models.LOCK) + failed_sid = "cleanup-partial-delete" + healthy_sid = "cleanup-best-effort-peer" + for sid in (failed_sid, healthy_sid): + models.Session( + session_id=sid, + title="Untitled", + workspace=str(tmp_path), + messages=[], + ).save(skip_index=True) + failed_backup = session_dir / f"{failed_sid}.json.bak" + failed_backup.write_text("required offline backup", encoding="utf-8") + index_path.write_text( + json.dumps( + [ + {"session_id": failed_sid, "title": "Untitled", "message_count": 0}, + {"session_id": healthy_sid, "title": "Untitled", "message_count": 0}, + ] + ), + encoding="utf-8", + ) + real_unlink = Path.unlink + + def fail_required_backup_unlink(path, *args, **kwargs): + if path == failed_backup: + raise PermissionError("required cleanup backup is locked") + return real_unlink(path, *args, **kwargs) + + monkeypatch.setattr(Path, "unlink", fail_required_backup_unlink) + monkeypatch.setattr(routes, "j", lambda _handler, payload: payload) + + result = routes._handle_sessions_cleanup(object(), {}) + + assert result == {"ok": True, "cleaned": 1} + assert not (session_dir / f"{failed_sid}.json").exists() + assert failed_backup.exists() + assert not (session_dir / f"{healthy_sid}.json").exists() + assert json.loads(index_path.read_text(encoding="utf-8")) == [] + assert failed_sid in models._load_webui_deleted_session_tombstone() + + +def test_stale_loaded_instance_cannot_overwrite_newer_sidecar( + tmp_path, monkeypatch +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "revision-cas" + seed = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[{"role": "user", "content": "seed"}], + ) + seed.save(skip_index=True) + first = models.Session.load(sid) + with models.LOCK: + models.SESSIONS.pop(sid, None) + second = models.Session.load(sid) + assert first is not None and second is not None + + second.messages.append({"role": "assistant", "content": "newer"}) + second.save(skip_index=True) + first.title = "stale mutation" + with pytest.raises(models.StaleSessionGenerationError): + first.save(skip_index=True) + + persisted = json.loads( + (session_dir / f"{sid}.json").read_text(encoding="utf-8") + ) + assert persisted["messages"][-1]["content"] == "newer" + + +def test_native_windows_newlines_do_not_invalidate_second_save( + tmp_path, monkeypatch +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "windows-newline-revision" + real_open = builtins.open + + def windows_text_open(file, mode="r", *args, **kwargs): + if mode == "w" and ".tmp." in str(file) and kwargs.get("newline") is None: + kwargs["newline"] = "\r\n" + return real_open(file, mode, *args, **kwargs) + + monkeypatch.setattr(builtins, "open", windows_text_open) + session = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[{"role": "user", "content": "first"}], + ) + session.save(skip_index=True) + session.messages.append({"role": "assistant", "content": "second"}) + + session.save(skip_index=True) + + persisted = json.loads( + (session_dir / f"{sid}.json").read_text(encoding="utf-8") + ) + assert persisted["_sidecar_generation_v1"] == 2 + assert persisted["messages"][-1]["content"] == "second" + + +def test_new_instance_expected_absent_never_overwrites_existing_sid( + tmp_path, monkeypatch +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "create-only" + first = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[{"role": "user", "content": "first"}], + ) + stale = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[{"role": "user", "content": "stale"}], + ) + + first.save(skip_index=True) + with pytest.raises(models.StaleSessionGenerationError): + stale.save(skip_index=True) + persisted = json.loads( + (session_dir / f"{sid}.json").read_text(encoding="utf-8") + ) + assert persisted["messages"] == first.messages + + +def test_create_only_publish_fails_closed_without_atomic_primitive( + tmp_path, monkeypatch +): + from api import models + + source = tmp_path / "source.tmp" + destination = tmp_path / "destination.json" + source.write_bytes(b'{"complete": true}') + + def unsupported_link(*_args, **_kwargs): + raise OSError(errno.EXDEV, "hard links unsupported") + + monkeypatch.setattr(models.os, "link", unsupported_link) + + with pytest.raises(OSError, match="hard links unsupported"): + models._publish_sidecar_no_replace(source, destination) + + assert not destination.exists() + assert source.read_bytes() == b'{"complete": true}' + + +def test_generation_is_scoped_per_sid_across_rotation(tmp_path, monkeypatch): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + old_sid = "parent" + new_sid = "continuation" + session = models.Session( + session_id=old_sid, + workspace=str(tmp_path), + messages=[{"role": "user", "content": "parent"}], + ) + session.save(skip_index=True) + + session.session_id = new_sid + session.parent_session_id = old_sid + session.messages.append({"role": "assistant", "content": "continuation"}) + session.save(skip_index=True) + session.session_id = old_sid + session.title = "archived parent" + session.save(skip_index=True) + + parent = json.loads( + (session_dir / f"{old_sid}.json").read_text(encoding="utf-8") + ) + continuation = json.loads( + (session_dir / f"{new_sid}.json").read_text(encoding="utf-8") + ) + assert parent["_sidecar_generation_v1"] == 2 + assert continuation["_sidecar_generation_v1"] == 1 + + +@pytest.mark.skipif(os.name == "nt", reason="fork-based multiprocess CAS probe") +def test_two_process_writers_have_exactly_one_cas_winner(tmp_path, monkeypatch): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "multiprocess-cas" + models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[{"role": "user", "content": "base"}], + ).save(skip_index=True) + + context = multiprocessing.get_context("fork") + start_event = context.Event() + ready_queue = context.Queue() + result_queue = context.Queue() + processes = [ + context.Process( + target=_process_writer, + args=( + session_dir, + sid, + marker, + start_event, + ready_queue, + result_queue, + ), + ) + for marker in ("writer-a", "writer-b") + ] + for process in processes: + process.start() + assert {ready_queue.get(timeout=15), ready_queue.get(timeout=15)} == { + "writer-a", + "writer-b", + } + start_event.set() + for process in processes: + process.join(timeout=20) + assert process.exitcode == 0 + + results = dict(result_queue.get(timeout=5) for _ in processes) + assert sorted(results.values()) == ["saved", "stale"] + persisted = json.loads( + (session_dir / f"{sid}.json").read_text(encoding="utf-8") + ) + committed = { + message.get("content") + for message in persisted["messages"] + if message.get("content") in {"writer-a", "writer-b"} + } + assert len(committed) == 1 + assert persisted["_sidecar_generation_v1"] == 2 + + +def test_recovery_expected_absent_uses_create_or_fail(tmp_path, monkeypatch): + from api import session_recovery + + session_path = tmp_path / "absent.json" + backup_path = session_path.with_suffix(".json.bak") + backup_path.write_text( + json.dumps( + { + "session_id": "absent", + "messages": [{"role": "user", "content": "backup"}], + } + ), + encoding="utf-8", + ) + competing = { + "messages": [{"role": "user", "content": "competing"}] + } + real_link = session_recovery.os.link + + def competing_link(src, dst): + Path(dst).write_text(json.dumps(competing), encoding="utf-8") + return real_link(src, dst) + + monkeypatch.setattr(session_recovery.os, "link", competing_link) + result = session_recovery.recover_session(session_path) + assert result["restored"] is False + assert result["stale_generation"] is True + assert json.loads(session_path.read_text(encoding="utf-8")) == competing + + +def test_recovery_invalidates_cached_alias_and_publishes_generation( + tmp_path, monkeypatch +): + from api import models, session_recovery + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "recovery-alias" + session_path = session_dir / f"{sid}.json" + backup_path = session_path.with_suffix(".json.bak") + live = { + "session_id": sid, + "workspace": str(tmp_path), + "messages": [{"role": "user", "content": "live"}], + } + backup = { + "session_id": sid, + "workspace": str(tmp_path), + "messages": [ + {"role": "user", "content": "one"}, + {"role": "assistant", "content": "two"}, + {"role": "user", "content": "three"}, + ], + } + session_path.write_text(json.dumps(live), encoding="utf-8") + backup_path.write_text(json.dumps(backup), encoding="utf-8") + alias = models.Session.load(sid) + assert alias is not None + with models.LOCK: + models.SESSIONS[sid] = alias + fsynced = [] + monkeypatch.setattr( + models, + "_fsync_sidecar_directory", + lambda directory: fsynced.append(Path(directory)), + ) + + result = session_recovery.recover_session(session_path) + assert result["restored"] is True + assert fsynced == [session_dir] + with models.LOCK: + assert sid not in models.SESSIONS + restored = json.loads(session_path.read_text(encoding="utf-8")) + assert restored["_sidecar_generation_v1"] == 1 + alias.title = "stale alias" + with pytest.raises(RuntimeError, match="stale|generation"): + alias.save(skip_index=True) + + +def test_backup_is_monotone_across_later_poorer_shrinks(tmp_path, monkeypatch): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "monotone-backup" + session = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[ + {"role": "user", "content": str(index)} for index in range(10) + ], + ) + session.save(skip_index=True) + session.messages = session.messages[:2] + session.save(skip_index=True) + session.messages = [ + {"role": "user", "content": str(index)} for index in range(5) + ] + session.save(skip_index=True) + session.messages = session.messages[:4] + session.save(skip_index=True) + backup = json.loads( + (session_dir / f"{sid}.json.bak").read_text(encoding="utf-8") + ) + assert len(backup["messages"]) == 10 + + +def test_backup_retirement_requires_matching_receipt(tmp_path, monkeypatch): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "cleanup" + backup_path = session_dir / f"{sid}.json.bak" + live_path = session_dir / f"{sid}.json" + live_path.write_text("committed live", encoding="utf-8") + live_receipt = models._read_sidecar_revision(live_path, sid) + backup_path.write_text("first backup", encoding="utf-8") + + first_receipt = models._read_sidecar_revision(backup_path, sid) + backup_path.write_text("newer foreign backup", encoding="utf-8") + + assert models._retire_backup_if_owned( + sid, backup_path, None, live_receipt + ) is False + assert ( + models._retire_backup_if_owned( + sid, backup_path, first_receipt, live_receipt + ) + is False + ) + assert backup_path.read_text(encoding="utf-8") == "newer foreign backup" + + current_receipt = models._read_sidecar_revision(backup_path, sid) + archive_path = backup_path.with_name(f"{backup_path.name}.archive-test") + archive_path.write_text('{"archived": true}', encoding="utf-8") + assert ( + models._retire_backup_if_owned( + sid, backup_path, current_receipt, live_receipt + ) + is True + ) + assert not backup_path.exists() + assert not archive_path.exists() + + +def test_shrinking_save_fails_closed_when_backup_publish_fails( + tmp_path, monkeypatch +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "backup-fail-closed" + session = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[ + {"role": "user", "content": "one"}, + {"role": "assistant", "content": "two"}, + {"role": "user", "content": "three"}, + ], + ) + session.save(skip_index=True) + original = json.loads(session.path.read_text(encoding="utf-8")) + real_replace = models._safe_replace + + def fail_backup_publish(source, destination): + if Path(destination).suffix == ".bak": + raise OSError("simulated backup publish failure") + return real_replace(source, destination) + + monkeypatch.setattr(models, "_safe_replace", fail_backup_publish) + session.messages = session.messages[:1] + with pytest.raises(RuntimeError, match="backup"): + session.save(skip_index=True) + persisted = json.loads(session.path.read_text(encoding="utf-8")) + assert persisted["messages"] == original["messages"] + assert ( + persisted["_sidecar_generation_v1"] + == original["_sidecar_generation_v1"] + ) + + +def test_malformed_backup_is_archived_before_live_snapshot_promotion( + tmp_path, monkeypatch +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "malformed-backup-recovery" + session = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[ + {"role": "user", "content": "one"}, + {"role": "assistant", "content": "two"}, + {"role": "user", "content": "three"}, + ], + ) + session.save(skip_index=True) + before_shrink = session.path.read_bytes() + malformed = b'{"broken":' + backup_path = session.path.with_suffix(".json.bak") + backup_path.write_bytes(malformed) + session.messages = session.messages[:1] + + session.save(skip_index=True) + + archive = backup_path.with_name( + f"{backup_path.name}.archive-{hashlib.sha256(malformed).hexdigest()}" + ) + assert archive.read_bytes() == malformed + assert backup_path.read_bytes() == before_shrink + assert len(json.loads(session.path.read_text(encoding="utf-8"))["messages"]) == 1 + + +def test_malformed_backup_archive_does_not_require_hard_links( + tmp_path, monkeypatch +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "malformed-backup-no-hardlinks" + session = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[ + {"role": "user", "content": "one"}, + {"role": "assistant", "content": "two"}, + {"role": "user", "content": "three"}, + ], + ) + session.save(skip_index=True) + before_shrink = session.path.read_bytes() + malformed = b'{"broken":' + backup_path = session.path.with_suffix(".json.bak") + backup_path.write_bytes(malformed) + session.messages = session.messages[:1] + + def unsupported_link(*_args, **_kwargs): + raise OSError(errno.EOPNOTSUPP, "hard links unsupported") + + monkeypatch.setattr(models.os, "link", unsupported_link) + + session.save(skip_index=True) + + archive = backup_path.with_name( + f"{backup_path.name}.archive-{hashlib.sha256(malformed).hexdigest()}" + ) + assert archive.read_bytes() == malformed + assert backup_path.read_bytes() == before_shrink + assert len(json.loads(session.path.read_text(encoding="utf-8"))["messages"]) == 1 + + +def test_archive_temporary_name_handles_maximum_valid_session_id(tmp_path): + from api import models + + sid = "s" * 150 + backup_path = tmp_path / f"{sid}.json.bak" + backup_path.write_bytes(b'{"broken":') + receipt = models._read_sidecar_revision(backup_path, sid) + + archive_path = models._archive_incomparable_backup( + sid, + backup_path, + receipt, + ) + + assert archive_path.read_bytes() == b'{"broken":' + assert len(os.fsencode(archive_path.name)) <= 255 + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX directory fsync contract") +def test_first_sidecar_publication_fsyncs_parent_directory(tmp_path, monkeypatch): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + fsynced = [] + monkeypatch.setattr( + models, + "_fsync_sidecar_directory", + lambda directory: fsynced.append(Path(directory)), + raising=False, + ) + session = models.Session( + session_id="durable-first-publication", + workspace=str(tmp_path), + messages=[{"role": "user", "content": "first"}], + ) + + session.save(skip_index=True) + + assert fsynced == [session_dir] + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX directory fsync contract") +def test_sidecar_directory_fsync_tolerates_only_unsupported_filesystems( + tmp_path, + monkeypatch, +): + from api import models + + def unsupported(_fd): + raise OSError(errno.EINVAL, "directory fsync unsupported") + + monkeypatch.setattr(models.os, "fsync", unsupported) + models._fsync_sidecar_directory(tmp_path) + + def real_failure(_fd): + raise OSError(errno.EIO, "durability failure") + + monkeypatch.setattr(models.os, "fsync", real_failure) + with pytest.raises(OSError, match="durability failure"): + models._fsync_sidecar_directory(tmp_path) + + def permission_failure(*_args, **_kwargs): + raise PermissionError(errno.EACCES, "directory open denied") + + monkeypatch.setattr(models.os, "open", permission_failure) + with pytest.raises(PermissionError, match="directory open denied"): + models._fsync_sidecar_directory(tmp_path) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX directory fsync contract") +def test_shrink_publications_fsync_archive_backup_then_live( + tmp_path, + monkeypatch, +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + session = models.Session( + session_id="durable-shrink-publication", + workspace=str(tmp_path), + messages=[ + {"role": "user", "content": "one"}, + {"role": "assistant", "content": "two"}, + {"role": "user", "content": "three"}, + ], + ) + session.save(skip_index=True) + backup_path = session.path.with_suffix(".json.bak") + malformed = b'{"broken":' + backup_path.write_bytes(malformed) + session.messages = session.messages[:1] + events = [] + real_replace = models._safe_replace + + def record_replace(source, destination): + events.append(("replace", Path(destination).name)) + return real_replace(source, destination) + + monkeypatch.setattr(models, "_safe_replace", record_replace) + monkeypatch.setattr( + models, + "_fsync_sidecar_directory", + lambda directory: events.append(("fsync", Path(directory).name)), + raising=False, + ) + + session.save(skip_index=True) + + archive_name = ( + f"{backup_path.name}.archive-{hashlib.sha256(malformed).hexdigest()}" + ) + assert events == [ + ("replace", archive_name), + ("fsync", session_dir.name), + ("replace", backup_path.name), + ("fsync", session_dir.name), + ("replace", session.path.name), + ("fsync", session_dir.name), + ] + + +def test_non_object_backup_is_archived_before_live_snapshot_promotion( + tmp_path, monkeypatch +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "non-object-backup-recovery" + session = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[ + {"role": "user", "content": "one"}, + {"role": "assistant", "content": "two"}, + ], + ) + session.save(skip_index=True) + before_shrink = session.path.read_bytes() + unusable = b"[]" + backup_path = session.path.with_suffix(".json.bak") + backup_path.write_bytes(unusable) + session.messages = session.messages[:1] + + session.save(skip_index=True) + + archive = backup_path.with_name( + f"{backup_path.name}.archive-{hashlib.sha256(unusable).hexdigest()}" + ) + assert archive.read_bytes() == unusable + assert backup_path.read_bytes() == before_shrink + + +def test_foreign_sid_backup_is_archived_and_replaced_before_shrink( + tmp_path, monkeypatch +): + from api import models, session_recovery + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "foreign-backup-owner" + messages = [ + {"role": "user", "content": "one"}, + {"role": "assistant", "content": "two"}, + {"role": "user", "content": "three"}, + ] + session = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=messages, + ) + session.save(skip_index=True) + foreign_bytes = json.dumps( + {"session_id": "foreign-owner", "messages": messages}, + ensure_ascii=False, + ).encode("utf-8") + backup_path = session.path.with_suffix(".json.bak") + backup_path.write_bytes(foreign_bytes) + session.messages = messages[:1] + + session.save(skip_index=True) + + archive = backup_path.with_name( + f"{backup_path.name}.archive-{hashlib.sha256(foreign_bytes).hexdigest()}" + ) + assert archive.read_bytes() == foreign_bytes + primary_backup = json.loads(backup_path.read_text(encoding="utf-8")) + assert primary_backup["session_id"] == sid + result = session_recovery.recover_session(session.path) + assert result["restored"] is True + restored = json.loads(session.path.read_text(encoding="utf-8")) + assert restored["messages"] == messages + + +def test_workspace_patch_does_not_grant_stale_alias_new_revision( + tmp_path, monkeypatch +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "workspace-stale-alias" + original_workspace = tmp_path / "before" + recovered_workspace = tmp_path / "after" + seed = models.Session( + session_id=sid, + workspace=str(original_workspace), + messages=[{"role": "user", "content": "seed"}], + ) + seed.save(skip_index=True) + stale = models.Session.load(sid) + newer = models.Session.load(sid) + assert stale is not None and newer is not None + newer.messages.append({"role": "assistant", "content": "newer"}) + newer.save(skip_index=True) + with models.LOCK: + models.SESSIONS[sid] = stale + + current = models.persist_recovered_workspace_binding( + stale, + recovered_workspace, + expected_workspace=str(original_workspace.resolve()), + ) + + assert current is not stale + assert current.messages[-1]["content"] == "newer" + assert current.workspace == str(recovered_workspace.resolve()) + stale.title = "must not own current revision" + with pytest.raises(models.StaleSessionGenerationError): + stale.save(skip_index=True) + persisted = json.loads(seed.path.read_text(encoding="utf-8")) + assert persisted["messages"][-1]["content"] == "newer" + + +def test_incomparable_backup_is_archived_before_latest_snapshot_promotion( + tmp_path, monkeypatch +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "incomparable-backup" + a = {"role": "user", "content": "A"} + unique = {"role": "assistant", "content": "UNIQUE-U"} + session = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[a, unique], + ) + session.save(skip_index=True) + session.messages = [a] + session.save(skip_index=True) + session.messages = [ + a, + {"role": "assistant", "content": "D1"}, + {"role": "user", "content": "D2"}, + ] + session.save(skip_index=True) + + session.messages = [a] + session.save(skip_index=True) + + live = json.loads(session.path.read_text(encoding="utf-8")) + backup_path = session.path.with_suffix(".json.bak") + backup = json.loads(backup_path.read_text(encoding="utf-8")) + archives = list(session_dir.glob(f"{backup_path.name}.archive-*")) + assert [row["content"] for row in live["messages"]] == ["A"] + assert [row["content"] for row in backup["messages"]] == ["A", "D1", "D2"] + assert len(archives) == 1 + archived = json.loads(archives[0].read_text(encoding="utf-8")) + assert [row["content"] for row in archived["messages"]] == ["A", "UNIQUE-U"] + + +def test_backup_dominance_preserves_complete_legacy_snapshot_payload( + tmp_path, monkeypatch +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "backup-complete-snapshot" + first = {"role": "user", "content": "first"} + second = {"role": "assistant", "content": "second"} + third = {"role": "user", "content": "third"} + unique_tool_call = {"id": "unique-old-tool-call", "name": "must-survive"} + unique_context = {"role": "system", "content": "unique old model context"} + unique_draft = {"text": "unique old draft", "attachments": ["draft.txt"]} + unique_future_metadata = {"opaque": ["future", "must-survive"]} + session = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[first, second], + context_messages=[first, unique_context], + tool_calls=[unique_tool_call], + composer_draft=unique_draft, + ) + session.save(touch_updated_at=False, skip_index=True) + + session.messages = [first] + session.context_messages = [first] + session.tool_calls = [] + session.composer_draft = {} + session.save(touch_updated_at=False, skip_index=True) + backup_path = session.path.with_suffix(".json.bak") + + # Legacy backups predate the generation/count/fingerprint metadata. Their + # remaining durable payload must participate in exactly the same dominance + # decision as a current sidecar. + legacy_backup = json.loads(backup_path.read_text(encoding="utf-8")) + for derived_key in ( + "_sidecar_generation_v1", + "message_count", + "anchor_scene_index", + ): + legacy_backup.pop(derived_key, None) + legacy_backup["future_recovery_metadata"] = unique_future_metadata + backup_path.write_text( + json.dumps(legacy_backup, ensure_ascii=False, indent=2), + encoding="utf-8", + ) + + # Message-only dominance considers this live snapshot richer, although it + # lacks all three uniquely recoverable non-transcript artifacts. + session.messages = [first, second, third] + session.context_messages = [first, third] + session.save(touch_updated_at=False, skip_index=True) + session.messages = [first] + session.context_messages = [first] + session.save(touch_updated_at=False, skip_index=True) + + recoverable_payloads = [ + json.loads(path.read_text(encoding="utf-8")) + for path in [backup_path, *session_dir.glob(f"{backup_path.name}.archive-*")] + ] + assert any( + unique_tool_call in (payload.get("tool_calls") or []) + and unique_context in (payload.get("context_messages") or []) + and payload.get("composer_draft") == unique_draft + and payload.get("future_recovery_metadata") == unique_future_metadata + for payload in recoverable_payloads + ) + + +def test_backup_dominance_preserves_message_order(): + from api.models import _ordered_json_rows_cover + + first = {"role": "user", "content": "first"} + second = {"role": "assistant", "content": "second"} + + assert _ordered_json_rows_cover([first, second], [first]) is True + assert _ordered_json_rows_cover([second, first], [first, second]) is False + + +def test_backup_snapshot_dominance_budget_exhaustion_fails_closed(monkeypatch): + from api import models + + baseline = { + "session_id": "bounded-backup", + "messages": [{"role": "user", "content": "x" * 128}], + "future_metadata": {"opaque": "y" * 128}, + } + candidate = { + **baseline, + "messages": [ + *baseline["messages"], + {"role": "assistant", "content": "new"}, + ], + } + + assert models._session_snapshot_covers( + candidate, + baseline, + max_canonical_bytes=64, + ) is False + + boundary = {"future_metadata": "x" * 7} + assert models._session_snapshot_covers( + boundary, + boundary, + max_canonical_bytes=18, + ) is True + + real_dumps = models.json.dumps + dumps_calls = [] + + def counted_dumps(value, *args, **kwargs): + dumps_calls.append(value) + return real_dumps(value, *args, **kwargs) + + monkeypatch.setattr(models.json, "dumps", counted_dumps) + assert models._session_snapshot_covers( + boundary, + boundary, + max_canonical_bytes=17, + ) is False + assert len(dumps_calls) == 1 + + oversized = "z" * (4 * 1024 * 1024) + deeply_nested = None + # Top-level list-valued fields compare their rows independently, so one + # layer is consumed by the ordered-subsequence traversal before preflight. + for _ in range(66): + deeply_nested = [deeply_nested] + cyclic = [] + cyclic.append(cyclic) + too_many_items = {"one": 1, "two": 2, "three": 3} + lone_surrogate = "\ud800" + + def reject_unproven_serialization(value, *args, **kwargs): + if ( + value is oversized + or value is deeply_nested + or value is cyclic + or value is too_many_items + or value is lone_surrogate + or value == b"opaque" + ): + pytest.fail("out-of-budget or non-canonical value reached json.dumps") + return real_dumps(value, *args, **kwargs) + + monkeypatch.setattr(models.json, "dumps", reject_unproven_serialization) + assert models._session_snapshot_covers( + {"future_metadata": oversized}, + {"future_metadata": oversized}, + max_canonical_bytes=1024 * 1024, + ) is False + assert models._session_snapshot_covers( + {"future_metadata": deeply_nested}, + {"future_metadata": deeply_nested}, + ) is False + assert models._session_snapshot_covers( + {"future_metadata": b"opaque"}, + {"future_metadata": b"opaque"}, + ) is False + assert models._session_snapshot_covers( + {"future_metadata": {"cycle": cyclic}}, + {"future_metadata": {"cycle": cyclic}}, + ) is False + assert models._session_snapshot_covers( + {"future_metadata": lone_surrogate}, + {"future_metadata": lone_surrogate}, + ) is False + + monkeypatch.setattr(models, "_BACKUP_SNAPSHOT_DOMINANCE_MAX_ITEMS", 2) + assert models._session_snapshot_covers( + {"future_metadata": too_many_items}, + {"future_metadata": too_many_items}, + ) is False + + def memory_error(*_args, **_kwargs): + raise MemoryError + + monkeypatch.setattr(models.json, "dumps", memory_error) + assert models._session_snapshot_covers( + {"future_metadata": "small"}, + {"future_metadata": "small"}, + ) is False + + +@pytest.mark.skipif(sys.platform != "linux", reason="requires Linux RSS and RLIMIT_AS") +def test_backup_snapshot_dominance_oversize_preflight_bounds_memory(): + repo_root = Path(__file__).resolve().parents[1] + probe = r'''import json +import resource + +from api.models import _session_snapshot_covers + +payload = "x" * (80 * 1024 * 1024) +snapshot = {"future_unknown_metadata": payload} +rss_before_kib = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss +with open("/proc/self/status", encoding="utf-8") as status_file: + vm_size_kib = next( + int(line.split()[1]) + for line in status_file + if line.startswith("VmSize:") + ) +headroom = 96 * 1024 * 1024 +_, hard_limit = resource.getrlimit(resource.RLIMIT_AS) +soft_limit = vm_size_kib * 1024 + headroom +if hard_limit != resource.RLIM_INFINITY: + soft_limit = min(soft_limit, hard_limit) +resource.setrlimit( + resource.RLIMIT_AS, + (soft_limit, hard_limit), +) +try: + result = _session_snapshot_covers(snapshot, snapshot) +except BaseException as exc: + outcome = type(exc).__name__ +else: + outcome = repr(result) +rss_after_kib = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss +print(json.dumps({ + "outcome": outcome, + "rss_peak_delta_mib": (rss_after_kib - rss_before_kib) / 1024, +})) +''' + completed = subprocess.run( + [sys.executable, "-c", probe], + cwd=repo_root, + env={**os.environ, "PYTHONDONTWRITEBYTECODE": "1"}, + capture_output=True, + text=True, + timeout=30, + check=False, + ) + + assert completed.returncode == 0, completed.stderr + report = json.loads(completed.stdout) + assert report["outcome"] == "False" + assert report["rss_peak_delta_mib"] < 16 + + +def test_backup_retirement_requires_live_revision_to_still_match( + tmp_path, monkeypatch +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "cleanup-live-race" + session = models.Session( + session_id=sid, + workspace=str(tmp_path), + messages=[ + {"role": "user", "content": "one"}, + {"role": "assistant", "content": "two"}, + ], + ) + session.save(skip_index=True) + session.messages = session.messages[:1] + backup_receipt = session.save(skip_index=True) + committed_receipt = models._read_sidecar_revision(session.path, sid) + session.messages.append({"role": "assistant", "content": "new generation"}) + session.save(skip_index=True) + + assert models._retire_backup_if_owned( + sid, + session.path.with_suffix(".json.bak"), + backup_receipt, + committed_receipt, + ) is False + assert session.path.with_suffix(".json.bak").exists() + + +def test_same_count_external_metadata_update_reloads_cached_owner( + tmp_path, monkeypatch +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "same-count-metadata" + seed = models.Session( + session_id=sid, + title="before", + workspace=str(tmp_path), + messages=[{"role": "user", "content": "same transcript"}], + ) + seed.save(skip_index=True) + cached = models.Session.load(sid) + external = models.Session.load(sid) + assert cached is not None and external is not None + with models.LOCK: + models.SESSIONS[sid] = cached + external.title = "after" + external.save(skip_index=True) + + loaded = models.get_session(sid) + + assert loaded is not cached + assert loaded.title == "after" + assert loaded.messages == cached.messages + + +def test_modern_cache_freshness_uses_prefix_generation_not_full_digest( + tmp_path, monkeypatch +): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + session = models.Session( + session_id="prefix-generation", + workspace=str(tmp_path), + messages=[{"role": "user", "content": "large sidecar proxy"}], + ) + session.save(skip_index=True) + cached = models.Session.load(session.session_id) + assert cached is not None + + monkeypatch.setattr( + models, + "_read_sidecar_revision", + lambda *_args, **_kwargs: pytest.fail("cache hit hashed the full sidecar"), + ) + + assert models._cached_session_lags_disk(cached) is False + + +def test_recovery_rejects_backup_with_foreign_embedded_sid(tmp_path, monkeypatch): + from api import models, session_recovery + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "expected-sid" + session_path = session_dir / f"{sid}.json" + live = { + "session_id": sid, + "messages": [{"role": "user", "content": "live"}], + } + foreign = { + "session_id": "foreign-sid", + "messages": [ + {"role": "user", "content": "foreign one"}, + {"role": "assistant", "content": "foreign two"}, + ], + } + session_path.write_text(json.dumps(live), encoding="utf-8") + session_path.with_suffix(".json.bak").write_text( + json.dumps(foreign), encoding="utf-8" + ) + + result = session_recovery.recover_session(session_path) + + assert result["restored"] is False + assert "session id" in result["error"].lower() + assert json.loads(session_path.read_text(encoding="utf-8")) == live + + +def test_create_only_publish_uses_native_windows_rename_without_hardlinks( + tmp_path, monkeypatch +): + from api import models + + source = tmp_path / "source.tmp" + destination = tmp_path / "destination.json" + source.write_bytes(b'{"complete": true}') + + def unsupported_link(_source, _destination): + raise OSError(errno.EOPNOTSUPP, "hard links unsupported") + + real_rename = models.os.rename + rename_calls = [] + + def create_only_rename(rename_source, rename_destination): + rename_calls.append((rename_source, rename_destination)) + assert not destination.exists() + real_rename(rename_source, rename_destination) + + monkeypatch.setattr(models.os, "link", unsupported_link) + monkeypatch.setattr(models.os, "rename", create_only_rename) + monkeypatch.setattr(models.os, "name", "nt") + + models._publish_sidecar_no_replace(source, destination) + + assert rename_calls == [(source, destination)] + assert destination.read_bytes() == b'{"complete": true}' + assert not source.exists() + + +def test_orphan_recovery_rechecks_delete_tombstone_under_authority( + tmp_path, monkeypatch +): + from api import models, session_recovery + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "deleted-before-recovery" + session_path = session_dir / f"{sid}.json" + session_path.with_suffix(".json.bak").write_text( + json.dumps( + { + "session_id": sid, + "messages": [{"role": "user", "content": "deleted"}], + } + ), + encoding="utf-8", + ) + models._record_webui_deleted_session_tombstone(sid) + + result = session_recovery.recover_session(session_path) + + assert result["restored"] is False + assert result.get("deleted") is True + assert not session_path.exists() + + +def test_state_db_materialization_rechecks_delete_inside_authority( + tmp_path, monkeypatch +): + from api import models, session_recovery + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "deleted-during-reconcile" + db_path = tmp_path / "state.db" + with sqlite3.connect(db_path) as conn: + conn.execute( + "CREATE TABLE sessions (id TEXT PRIMARY KEY, source TEXT, title TEXT, " + "model TEXT, started_at REAL, message_count INTEGER)" + ) + conn.execute( + "CREATE TABLE messages (id INTEGER PRIMARY KEY, session_id TEXT, " + "role TEXT, content TEXT, timestamp REAL)" + ) + conn.execute( + "INSERT INTO sessions VALUES (?, 'webui', 'deleted', 'model', 1, 1)", + (sid,), + ) + conn.execute( + "INSERT INTO messages VALUES (1, ?, 'user', 'deleted', 1)", + (sid,), + ) + + real_authority = models._session_sidecar_authority + + @contextmanager + def delete_before_authority_yields(session_id, *, session_dir=None): + with sqlite3.connect(db_path) as conn: + conn.execute("DELETE FROM sessions WHERE id = ?", (session_id,)) + models._record_webui_deleted_session_tombstone(session_id) + with real_authority(session_id, session_dir=session_dir): + yield + + monkeypatch.setattr( + models, "_session_sidecar_authority", delete_before_authority_yields + ) + + result = session_recovery.recover_missing_sidecars_from_state_db( + session_dir, db_path + ) + + assert result["materialized"] == 0 + assert not (session_dir / f"{sid}.json").exists() diff --git a/tests/test_sprint23.py b/tests/test_sprint23.py index d0f088be382..5523bcdf091 100644 --- a/tests/test_sprint23.py +++ b/tests/test_sprint23.py @@ -34,17 +34,18 @@ def _make_session_visible(sid): from api.models import Session from tests.conftest import TEST_WORKSPACE - session = Session( - session_id=sid, - title="Compact Usage", - workspace=str(TEST_WORKSPACE), - model="test", - created_at=1.0, - updated_at=1.0, - profile="default", - messages=[{"role": "user", "content": "visible row", "timestamp": 1.0}], - tool_calls=[], - ) + session = Session.load(sid) + assert session is not None + session.title = "Compact Usage" + session.workspace = str(TEST_WORKSPACE) + session.model = "test" + session.created_at = 1.0 + session.updated_at = 1.0 + session.profile = "default" + session.messages = [ + {"role": "user", "content": "visible row", "timestamp": 1.0} + ] + session.tool_calls = [] session.save(touch_updated_at=False) diff --git a/tests/test_webui_state_db_reconciliation.py b/tests/test_webui_state_db_reconciliation.py index 8077bf157fa..962b5887797 100644 --- a/tests/test_webui_state_db_reconciliation.py +++ b/tests/test_webui_state_db_reconciliation.py @@ -1934,18 +1934,13 @@ def test_get_session_keeps_equal_count_newer_cached_user_tail(monkeypatch, tmp_p ], ) - cached = models.Session( - session_id=sid, - title="Reconcile", - workspace=str(tmp_path), - model="test-model", - messages=[ - {"role": "user", "content": "old prompt", "timestamp": 1000.0}, - {"role": "user", "content": "new prompt before stream id", "timestamp": 1002.0}, - ], - created_at=1000.0, - updated_at=1002.0, - ) + cached = models.Session.load(sid) + assert cached is not None + cached.messages = [ + {"role": "user", "content": "old prompt", "timestamp": 1000.0}, + {"role": "user", "content": "new prompt before stream id", "timestamp": 1002.0}, + ] + cached.updated_at = 1002.0 models.SESSIONS[sid] = cached loaded = models.get_session(sid) @@ -2014,20 +2009,15 @@ def test_get_session_reloads_when_cached_session_lags_disk(monkeypatch, tmp_path cached.pending_user_message = "next prompt" models.SESSIONS[sid] = cached - newer = models.Session( - session_id=sid, - title="Reconcile", - workspace=str(tmp_path), - model="test-model", - messages=old_messages + [ - {"role": "user", "content": "new user", "timestamp": 1002.0}, - {"role": "assistant", "content": "new final answer", "timestamp": 1003.0}, - ], - created_at=1000.0, - updated_at=1003.0, - active_stream_id="stream-cache-lags-disk", - pending_user_message="next prompt", - ) + newer = models.Session.load(sid) + assert newer is not None + newer.messages = old_messages + [ + {"role": "user", "content": "new user", "timestamp": 1002.0}, + {"role": "assistant", "content": "new final answer", "timestamp": 1003.0}, + ] + newer.updated_at = 1003.0 + newer.active_stream_id = "stream-cache-lags-disk" + newer.pending_user_message = "next prompt" newer.save(touch_updated_at=False) loaded = models.get_session(sid) diff --git a/tests/test_workspace_stale_recovery.py b/tests/test_workspace_stale_recovery.py index 7eb8ea71cc4..c43f9705037 100644 --- a/tests/test_workspace_stale_recovery.py +++ b/tests/test_workspace_stale_recovery.py @@ -68,10 +68,325 @@ def test_resolve_chat_workspace_with_recovery_repairs_missing_implicit_workspace resolved = routes._resolve_chat_workspace_with_recovery(session, None) assert resolved == str(fallback.resolve()) - assert session.workspace == str(fallback.resolve()) + assert session.workspace == str(stale) assert str(fallback.resolve()) in sidecar.read_text(encoding="utf-8") +def test_chat_start_adopts_reloaded_owner_after_implicit_workspace_recovery( + monkeypatch, tmp_path +): + fallback = tmp_path / "fallback" + fallback.mkdir() + stale_workspace = tmp_path / "deleted-workspace" + sid = "chat-recovery-owner" + session_dir = tmp_path / "sessions" + session_dir.mkdir() + monkeypatch.setattr(models, "SESSION_DIR", session_dir) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", session_dir / "_index.json") + monkeypatch.setattr(models, "_write_session_index", lambda **_kwargs: None) + seed = models.Session( + session_id=sid, + workspace=str(stale_workspace), + model="test-model", + model_provider="test-provider", + profile="default", + messages=[{"role": "user", "content": "preserve me"}], + ) + seed.save(skip_index=True) + stale_alias = models.Session.load(sid) + concurrent_owner = models.Session.load(sid) + assert stale_alias is not None and concurrent_owner is not None + concurrent_owner.title = "concurrent mutation" + concurrent_owner.save(skip_index=True) + with models.LOCK: + models.SESSIONS[sid] = stale_alias + + captured = {} + + def start_run(session, **kwargs): + captured["session"] = session + session.workspace = kwargs["workspace"] + session.save(skip_index=True) + return {"stream_id": "recovered-owner-stream"} + + monkeypatch.setattr(workspace, "_home_path", lambda: tmp_path) + monkeypatch.setattr(workspace, "load_workspaces", lambda: []) + monkeypatch.setattr(routes, "get_last_workspace", lambda: str(fallback)) + monkeypatch.setattr(routes, "_get_or_materialize_session", lambda *_a, **_k: stale_alias) + monkeypatch.setattr( + routes, + "_read_profile_model_config", + lambda *_a, **_k: (None, None, {}), + ) + monkeypatch.setattr( + routes, + "_resolve_compatible_session_model_state", + lambda *_a, **_k: ("test-model", "test-provider", "test-model"), + ) + monkeypatch.setattr( + routes, + "_repair_foreign_session_model_provider", + lambda _session, **_kwargs: "test-provider", + ) + monkeypatch.setattr(routes, "_start_run", start_run) + monkeypatch.setattr(routes, "j", lambda _handler, payload, status=200: payload) + + response = routes._handle_chat_start( + None, + {"session_id": sid, "message": "continue"}, + ) + + assert response["stream_id"] == "recovered-owner-stream" + assert captured["session"] is not stale_alias + assert captured["session"].workspace == str(fallback.resolve()) + + +def _patch_chat_start_after_workspace_adoption( + monkeypatch, + tmp_path, + stale_alias, + recovered_owner, + start_run, +): + monkeypatch.setattr(routes, "_agent_runtime_barrier_response", lambda **_kwargs: None) + monkeypatch.setattr( + routes, + "_get_or_materialize_session", + lambda *_args, **_kwargs: stale_alias, + ) + monkeypatch.setattr(routes, "_get_active_profile_name", lambda: "default") + monkeypatch.setattr( + routes, + "_resolve_chat_workspace_with_recovery", + lambda *_args, **_kwargs: routes._ResolvedChatWorkspace( + tmp_path, + recovered_owner, + ), + ) + monkeypatch.setattr( + routes, + "_read_profile_model_config", + lambda *_args, **_kwargs: (None, None, {}), + ) + monkeypatch.setattr( + routes, + "_resolve_compatible_session_model_state", + lambda *_args, **_kwargs: ("test-model", "test-provider", "test-model"), + ) + monkeypatch.setattr( + routes, + "_repair_foreign_session_model_provider", + lambda _session, **_kwargs: "test-provider", + ) + monkeypatch.setattr(routes, "get_config_snapshot", lambda: {}) + monkeypatch.setattr(routes, "webui_gateway_chat_enabled", lambda _config: False) + monkeypatch.setattr(routes, "_start_run", start_run) + monkeypatch.setattr( + routes, + "bad", + lambda _handler, message, status=400: { + "error": message, + "_status": status, + }, + ) + monkeypatch.setattr( + routes, + "j", + lambda _handler, payload, status=200: {**payload, "_status": status}, + ) + + +def test_chat_start_rechecks_profile_after_workspace_owner_adoption( + monkeypatch, tmp_path +): + stale_alias = SimpleNamespace( + session_id="profile-owner-race", + workspace=str(tmp_path), + model="test-model", + model_provider="test-provider", + profile="default", + messages=[{"role": "user", "content": "default profile"}], + context_messages=[], + pending_user_message=None, + compression_recovery={}, + recommended_recovery_action=None, + ) + recovered_owner = SimpleNamespace( + session_id=stale_alias.session_id, + workspace=str(tmp_path), + model="test-model", + model_provider="test-provider", + profile="other", + messages=[{"role": "user", "content": "other profile"}], + context_messages=[], + pending_user_message=None, + compression_recovery={}, + recommended_recovery_action=None, + ) + started = {"value": False} + + def start_run(_session, **_kwargs): + started["value"] = True + return {"stream_id": "must-not-start"} + + _patch_chat_start_after_workspace_adoption( + monkeypatch, + tmp_path, + stale_alias, + recovered_owner, + start_run, + ) + + response = routes._handle_chat_start( + object(), + {"session_id": stale_alias.session_id, "message": "continue safely"}, + ) + + # The adopted durable owner belongs to a known other profile, so the + # post-adoption recheck uses the same #7710 contract as the first check. + assert response["_status"] == 409 + assert response["code"] == "session_profile_mismatch" + assert response["profile"] == "other" + assert started["value"] is False + + +def test_chat_start_recomputes_recovery_after_workspace_owner_adoption( + monkeypatch, tmp_path +): + stale_recovery = { + "terminal_state": "compression_exhausted", + "recommended_action": "start_focused_continuation", + } + stale_alias = SimpleNamespace( + session_id="recovery-owner-race", + workspace=str(tmp_path), + model="test-model", + model_provider="test-provider", + profile="default", + messages=[{"role": "user", "content": "stale recovery"}], + context_messages=[], + pending_user_message=None, + compression_recovery=stale_recovery, + recommended_recovery_action="start_focused_continuation", + ) + saves = {"count": 0} + recovered_owner = SimpleNamespace( + session_id=stale_alias.session_id, + workspace=str(tmp_path), + model="test-model", + model_provider="test-provider", + profile="default", + messages=[{"role": "user", "content": "recovery already cleared"}], + context_messages=[], + pending_user_message=None, + compression_recovery={}, + recommended_recovery_action=None, + save=lambda: saves.__setitem__("count", saves["count"] + 1), + ) + + _patch_chat_start_after_workspace_adoption( + monkeypatch, + tmp_path, + stale_alias, + recovered_owner, + lambda _session, **_kwargs: {"error": "busy", "_status": 409}, + ) + adoptions = {"count": 0} + + def resolve_workspace(_session, _requested): + adoptions["count"] += 1 + return routes._ResolvedChatWorkspace(tmp_path, recovered_owner) + + monkeypatch.setattr( + routes, + "_resolve_chat_workspace_with_recovery", + resolve_workspace, + ) + + response = routes._handle_chat_start( + object(), + { + "session_id": stale_alias.session_id, + "message": "continue", + }, + ) + + assert response["_status"] == 409 + assert response["error"] == "busy" + assert adoptions["count"] == 1 + assert recovered_owner.compression_recovery == {} + assert recovered_owner.recommended_recovery_action is None + assert saves["count"] == 0 + + +def test_server_turn_adopts_recovered_owner_before_model_resolution( + monkeypatch, tmp_path +): + stale_alias = SimpleNamespace( + session_id="server-turn-owner", + model="stale-model", + model_provider="stale-provider", + profile="default", + ) + recovered_owner = SimpleNamespace( + session_id=stale_alias.session_id, + model="owner-model", + model_provider="owner-provider", + profile="default", + ) + sessions = iter((stale_alias, recovered_owner)) + captured = {} + + monkeypatch.setattr(routes, "_agent_runtime_barrier_response", lambda **_k: None) + monkeypatch.setattr(routes, "get_session", lambda _sid: next(sessions)) + monkeypatch.setattr( + routes, + "_resolve_chat_workspace_with_recovery", + lambda _session, _requested: routes._ResolvedChatWorkspace( + tmp_path, + recovered_owner, + ), + ) + + def read_profile(session, provider): + captured["profile_session"] = session + captured["profile_provider"] = provider + return None, None, {} + + monkeypatch.setattr(routes, "_read_profile_model_config", read_profile) + monkeypatch.setattr( + routes, + "_resolve_compatible_session_model_state", + lambda model, provider, **_kwargs: (model, provider, model), + ) + monkeypatch.setattr( + routes, + "clear_process_wakeup_pause_if_model_changed", + lambda *_a, **_k: False, + ) + + def start_run(session, **kwargs): + captured["run_session"] = session + captured["run_model"] = kwargs["model"] + captured["run_workspace"] = kwargs["workspace"] + return {"_status": 200} + + monkeypatch.setattr(routes, "_start_run", start_run) + + response = routes.start_session_turn( + stale_alias.session_id, + "wake up", + source="manual", + ) + + assert response["_status"] == 200 + assert captured["profile_session"] is recovered_owner + assert captured["profile_provider"] == "owner-provider" + assert captured["run_session"] is recovered_owner + assert captured["run_model"] == "owner-model" + assert captured["run_workspace"] == str(tmp_path) + + def test_chat_recovery_persistence_failure_fails_closed(monkeypatch, tmp_path): fallback = tmp_path / "fallback" fallback.mkdir() @@ -384,7 +699,7 @@ def fake_list_dir(workspace_path, rel_path): ) assert captured == {"workspace": fallback.resolve(), "rel_path": "."} - assert session.workspace == str(fallback.resolve()) + assert session.workspace == str(stale) assert str(fallback.resolve()) in sidecar.read_text(encoding="utf-8") assert payload == { "entries": [], @@ -433,7 +748,7 @@ def capture_list(workspace_path, _rel): selected["workspace"] = str(fallback_b) assert payload["workspace"] == str(fallback_a.resolve()) - assert session.workspace == str(fallback_a.resolve()) + assert session.workspace == str(stale) assert captured["listed"] == fallback_a.resolve() assert str(fallback_a.resolve()) in sidecar.read_text(encoding="utf-8") From 83ab144a094bd7b9a0d4104685d604b87700c4b0 Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Wed, 23 Sep 2026 10:39:25 +0000 Subject: [PATCH 2/7] fix(sidecar): reconcile owners after create-only fsync failure Track exact visible revision on failed first-save durability so the same owner can retry; fence aliases on uncertain publication and invalidate recovery caches without reporting success. --- api/models.py | 42 ++++- api/session_recovery.py | 21 ++- tests/test_session_sidecar_revision_fence.py | 171 +++++++++++++++++++ 3 files changed, 229 insertions(+), 5 deletions(-) diff --git a/api/models.py b/api/models.py index 8dcd4f2cf7a..347e641d9e2 100644 --- a/api/models.py +++ b/api/models.py @@ -390,6 +390,10 @@ def _fsync_sidecar_directory(directory: Path) -> None: os.close(fd) +class SidecarPublicationDurabilityError(OSError): + """The create-only publication happened, but directory durability failed.""" + + def _publish_sidecar_no_replace(source: Path, destination: Path) -> None: """Publish *source* only when *destination* is still absent. @@ -400,15 +404,20 @@ def _publish_sidecar_no_replace(source: Path, destination: Path) -> None: """ try: os.link(str(source), str(destination)) - _fsync_sidecar_directory(destination.parent) - return except FileExistsError: raise except OSError: if os.name != "nt": raise os.rename(source, destination) + # Do not confuse a failed durability step with a failed create (or retry + # the already-created destination through the Windows fallback). + try: _fsync_sidecar_directory(destination.parent) + except OSError as exc: + raise SidecarPublicationDurabilityError( + exc.errno, exc.strerror or str(exc) + ) from exc _BACKUP_SNAPSHOT_DOMINANCE_MAX_FIELDS = 512 @@ -2589,7 +2598,34 @@ def _save_owned_generation( parsed={"_sidecar_generation_v1": next_sidecar_generation}, ) ) - except Exception: + except Exception as exc: + if isinstance(exc, SidecarPublicationDurabilityError): + # The link/rename completed, but the directory entry was not + # confirmed durable. Track only the *visible* exact payload so + # this owner may retry; never report this save as successful. + intended = _sidecar_revision_from_bytes( + self.session_id, + payload.encode('utf-8'), + parsed={"_sidecar_generation_v1": next_sidecar_generation}, + ) + try: + published = _read_sidecar_revision(self.path, self.session_id) + except OSError: + published = None + if published == intended: + self._sidecar_revisions[self.session_id] = _sidecar_revision_record( + intended + ) + else: + _invalidate_cached_session_generation(self.session_id) + self._sidecar_revisions[self.session_id] = _sidecar_revision_record( + SidecarRevision( + sid=self.session_id, + state="INVALIDATED", + generation=-1, + digest_sha256=None, + ) + ) try: tmp.unlink(missing_ok=True) except Exception: diff --git a/api/session_recovery.py b/api/session_recovery.py index b0137cf31b3..3fe9c6671e1 100644 --- a/api/session_recovery.py +++ b/api/session_recovery.py @@ -423,6 +423,7 @@ def _recover_session_owned( ) -> dict: """Run one recovery while holding the cross-process SID authority.""" from api.models import ( + SidecarPublicationDurabilityError, _fsync_sidecar_directory, _invalidate_cached_session_generation, _publish_sidecar_no_replace, @@ -496,6 +497,11 @@ def _recover_session_owned( _fsync_sidecar_directory(session_path.parent) _invalidate_cached_session_generation(session_path.stem) except (OSError, json.JSONDecodeError, ValueError) as exc: + if isinstance(exc, SidecarPublicationDurabilityError): + # The live entry may already be visible despite the failed fsync. + # Do not leave an absent-generation cache owner behind, and do not + # call this restoration durable until a later successful write. + _invalidate_cached_session_generation(session_path.stem) logger.warning("recover_session: copy failed for %s: %s", session_path, exc) try: tmp_path.unlink(missing_ok=True) @@ -747,7 +753,12 @@ def _state_db_row_to_sidecar(row: dict) -> dict: def recover_missing_sidecars_from_state_db(session_dir: Path, state_db_path: Path | None) -> dict: """Materialize missing WebUI JSON sidecars from canonical state.db rows.""" - from api.models import _publish_sidecar_no_replace, _session_sidecar_authority + from api.models import ( + SidecarPublicationDurabilityError, + _invalidate_cached_session_generation, + _publish_sidecar_no_replace, + _session_sidecar_authority, + ) rows = _read_state_db_missing_sidecar_rows(session_dir, state_db_path) materialized = 0 @@ -814,7 +825,13 @@ def recover_missing_sidecars_from_state_db(session_dir: Path, state_db_path: Pat ) handle.flush() os.fsync(handle.fileno()) - _publish_sidecar_no_replace(tmp, target) + try: + _publish_sidecar_no_replace(tmp, target) + except SidecarPublicationDurabilityError: + # The entry exists, but no durable success was + # confirmed. Fence the absent-generation owner. + _invalidate_cached_session_generation(sid) + raise materialized_now = True except FileExistsError: # Live sidecar appeared between the check and the link — keep it. diff --git a/tests/test_session_sidecar_revision_fence.py b/tests/test_session_sidecar_revision_fence.py index f46afaa0c03..357c2a82483 100644 --- a/tests/test_session_sidecar_revision_fence.py +++ b/tests/test_session_sidecar_revision_fence.py @@ -904,6 +904,80 @@ def test_new_instance_expected_absent_never_overwrites_existing_sid( assert persisted["messages"] == first.messages +@pytest.mark.skipif(os.name == "nt", reason="POSIX hard-link publication") +def test_first_save_directory_fsync_failure_keeps_owner_retryable(tmp_path, monkeypatch): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + session = models.Session( + session_id="published-without-fsync", + workspace=str(tmp_path), + messages=[{"role": "user", "content": "keep this turn"}], + ) + with models.LOCK: + models.SESSIONS[session.session_id] = session + original_fsync = models._fsync_sidecar_directory + attempts = 0 + + def fail_once(directory): + nonlocal attempts + attempts += 1 + if attempts == 1: + raise OSError(errno.EIO, "directory fsync failed after link") + original_fsync(directory) + + monkeypatch.setattr(models, "_fsync_sidecar_directory", fail_once) + with pytest.raises(OSError, match="directory fsync failed after link"): + session.save(skip_index=True) + + first = models._read_sidecar_revision(session.path, session.session_id) + assert first.state == "PRESENT" and first.generation == 1 + assert json.loads(session.path.read_text(encoding="utf-8"))["messages"] == session.messages + with models.LOCK: + assert models.SESSIONS[session.session_id] is session + assert models._coerce_sidecar_revision( + session._sidecar_revisions[session.session_id], session.session_id + ) == first + + session.messages.append({"role": "assistant", "content": "retry kept the turn"}) + session.save(skip_index=True) + persisted = json.loads(session.path.read_text(encoding="utf-8")) + assert persisted["_sidecar_generation_v1"] == 2 + assert persisted["messages"] == session.messages + assert attempts == 2 + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX hard-link publication") +def test_first_save_fsync_failure_with_mismatched_payload_fences_owner(tmp_path, monkeypatch): + from api import models + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + session = models.Session( + session_id="published-but-replaced", + workspace=str(tmp_path), + messages=[{"role": "user", "content": "local turn"}], + ) + with models.LOCK: + models.SESSIONS[session.session_id] = session + + def replace_before_error(_directory): + payload = json.loads(session.path.read_text(encoding="utf-8")) + payload["messages"] = [{"role": "user", "content": "foreign turn"}] + session.path.write_text(json.dumps(payload), encoding="utf-8") + raise OSError(errno.EIO, "fsync failed after foreign replace") + + monkeypatch.setattr(models, "_fsync_sidecar_directory", replace_before_error) + with pytest.raises(OSError, match="fsync failed after foreign replace"): + session.save(skip_index=True) + with models.LOCK: + assert session.session_id not in models.SESSIONS + with pytest.raises(models.StaleSessionGenerationError): + session.save(skip_index=True) + assert json.loads(session.path.read_text(encoding="utf-8"))["messages"][0]["content"] == "foreign turn" + + def test_create_only_publish_fails_closed_without_atomic_primitive( tmp_path, monkeypatch ): @@ -1043,6 +1117,48 @@ def competing_link(src, dst): assert json.loads(session_path.read_text(encoding="utf-8")) == competing +@pytest.mark.skipif(os.name == "nt", reason="POSIX hard-link publication") +def test_missing_backup_recovery_fsync_failure_eviction_and_reload(tmp_path, monkeypatch): + from api import models, session_recovery + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "missing-backup-fsync" + alias = models.Session(session_id=sid, workspace=str(tmp_path)) + with models.LOCK: + models.SESSIONS[sid] = alias + alias.path.with_suffix(".json.bak").write_text( + json.dumps({"session_id": sid, "workspace": str(tmp_path), + "messages": [{"role": "user", "content": "recover me"}]}), + encoding="utf-8", + ) + monkeypatch.setattr(session_recovery, "_state_db_has_session", lambda *_: True) + original_fsync = models._fsync_sidecar_directory + attempts = 0 + + def fail_once(directory): + nonlocal attempts + attempts += 1 + if attempts == 1: + raise OSError(errno.EIO, "directory fsync failed after link") + original_fsync(directory) + + monkeypatch.setattr(models, "_fsync_sidecar_directory", fail_once) + result = session_recovery.recover_session(alias.path) + assert result["restored"] is False and "error" in result + assert attempts == 1 + assert json.loads(alias.path.read_text(encoding="utf-8"))["messages"][0]["content"] == "recover me" + with models.LOCK: + assert sid not in models.SESSIONS + with pytest.raises(models.StaleSessionGenerationError): + alias.save(skip_index=True) + reloaded = models.Session.load(sid) + assert reloaded is not None and reloaded.messages[0]["content"] == "recover me" + reloaded.messages.append({"role": "assistant", "content": "safe retry"}) + reloaded.save(skip_index=True) + assert json.loads(alias.path.read_text(encoding="utf-8"))["_sidecar_generation_v1"] == 2 + + def test_recovery_invalidates_cached_alias_and_publishes_generation( tmp_path, monkeypatch ): @@ -1960,6 +2076,61 @@ def test_orphan_recovery_rechecks_delete_tombstone_under_authority( assert not session_path.exists() +@pytest.mark.skipif(os.name == "nt", reason="POSIX hard-link publication") +def test_state_db_materialization_fsync_failure_invalidates_absent_owner(tmp_path, monkeypatch): + from api import models, session_recovery + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "state-db-publish-fsync" + db_path = tmp_path / "state.db" + with sqlite3.connect(db_path) as conn: + conn.execute( + "CREATE TABLE sessions (id TEXT PRIMARY KEY, source TEXT, title TEXT, " + "model TEXT, started_at REAL, message_count INTEGER)" + ) + conn.execute( + "CREATE TABLE messages (id INTEGER PRIMARY KEY, session_id TEXT, " + "role TEXT, content TEXT, timestamp REAL)" + ) + conn.execute( + "INSERT INTO sessions VALUES (?, 'webui', 'recovered', 'model', 1, 1)", + (sid,), + ) + conn.execute( + "INSERT INTO messages VALUES (1, ?, 'user', 'canonical turn', 1)", + (sid,), + ) + alias = models.Session(session_id=sid, workspace=str(tmp_path)) + with models.LOCK: + models.SESSIONS[sid] = alias + original_fsync = models._fsync_sidecar_directory + attempts = 0 + + def fail_once(directory): + nonlocal attempts + attempts += 1 + if attempts == 1: + raise OSError(errno.EIO, "directory fsync failed after link") + original_fsync(directory) + + monkeypatch.setattr(models, "_fsync_sidecar_directory", fail_once) + result = session_recovery.recover_missing_sidecars_from_state_db(session_dir, db_path) + assert result["materialized"] == 0 + assert result["details"][0]["materialized"] is False + assert "error" in result["details"][0] + assert attempts == 1 + assert "canonical turn" in (session_dir / f"{sid}.json").read_text(encoding="utf-8") + with models.LOCK: + assert sid not in models.SESSIONS + with pytest.raises(models.StaleSessionGenerationError): + alias.save(skip_index=True) + reloaded = models.Session.load(sid) + assert reloaded is not None + reloaded.save(skip_index=True) + assert json.loads(reloaded.path.read_text(encoding="utf-8"))["_sidecar_generation_v1"] == 2 + + def test_state_db_materialization_rechecks_delete_inside_authority( tmp_path, monkeypatch ): From 3225dde0087fd57df47b2d2999756e4802f1bae6 Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Wed, 23 Sep 2026 23:30:03 +0000 Subject: [PATCH 3/7] fix(recovery): evict absent owners on create conflicts --- api/session_recovery.py | 8 ++- tests/test_session_sidecar_revision_fence.py | 76 +++++++++++++++++++- 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/api/session_recovery.py b/api/session_recovery.py index 3fe9c6671e1..95457edadc9 100644 --- a/api/session_recovery.py +++ b/api/session_recovery.py @@ -489,6 +489,10 @@ def _recover_session_owned( try: _publish_sidecar_no_replace(tmp_path, session_path) except FileExistsError: + # A competing create made every cached ABSENT owner stale. + # Evict it before reporting the failed recovery so a later + # read/retry must adopt the visible generation. + _invalidate_cached_session_generation(session_path.stem) tmp_path.unlink(missing_ok=True) return {**status, "restored": False, "stale_generation": True} tmp_path.unlink(missing_ok=True) @@ -835,7 +839,9 @@ def recover_missing_sidecars_from_state_db(session_dir: Path, state_db_path: Pat materialized_now = True except FileExistsError: # Live sidecar appeared between the check and the link — keep it. - pass + # The cached owner (if any) still claims ABSENT and must not survive + # this failed materialization result. + _invalidate_cached_session_generation(sid) except OSError as exc: details.append({'session_id': sid, 'materialized': False, 'error': str(exc)}) detail_recorded = True diff --git a/tests/test_session_sidecar_revision_fence.py b/tests/test_session_sidecar_revision_fence.py index 357c2a82483..a3208b78f3f 100644 --- a/tests/test_session_sidecar_revision_fence.py +++ b/tests/test_session_sidecar_revision_fence.py @@ -1088,9 +1088,11 @@ def test_two_process_writers_have_exactly_one_cas_winner(tmp_path, monkeypatch): def test_recovery_expected_absent_uses_create_or_fail(tmp_path, monkeypatch): - from api import session_recovery + from api import models, session_recovery - session_path = tmp_path / "absent.json" + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + session_path = session_dir / "absent.json" backup_path = session_path.with_suffix(".json.bak") backup_path.write_text( json.dumps( @@ -1102,8 +1104,13 @@ def test_recovery_expected_absent_uses_create_or_fail(tmp_path, monkeypatch): encoding="utf-8", ) competing = { + "session_id": "absent", + "_sidecar_generation_v1": 1, "messages": [{"role": "user", "content": "competing"}] } + alias = models.Session(session_id="absent", workspace=str(tmp_path)) + with models.LOCK: + models.SESSIONS[alias.session_id] = alias real_link = session_recovery.os.link def competing_link(src, dst): @@ -1115,6 +1122,10 @@ def competing_link(src, dst): assert result["restored"] is False assert result["stale_generation"] is True assert json.loads(session_path.read_text(encoding="utf-8")) == competing + with models.LOCK: + assert alias.session_id not in models.SESSIONS + with pytest.raises(models.StaleSessionGenerationError): + alias.save(skip_index=True) @pytest.mark.skipif(os.name == "nt", reason="POSIX hard-link publication") @@ -2131,6 +2142,67 @@ def fail_once(directory): assert json.loads(reloaded.path.read_text(encoding="utf-8"))["_sidecar_generation_v1"] == 2 +def test_state_db_materialization_create_conflict_invalidates_absent_owner( + tmp_path, monkeypatch +): + from api import models, session_recovery + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + sid = "state-db-create-conflict" + db_path = tmp_path / "state.db" + with sqlite3.connect(db_path) as conn: + conn.execute( + "CREATE TABLE sessions (id TEXT PRIMARY KEY, source TEXT, title TEXT, " + "model TEXT, started_at REAL, message_count INTEGER)" + ) + conn.execute( + "CREATE TABLE messages (id INTEGER PRIMARY KEY, session_id TEXT, " + "role TEXT, content TEXT, timestamp REAL)" + ) + conn.execute( + "INSERT INTO sessions VALUES (?, 'webui', 'recovered', 'model', 1, 1)", + (sid,), + ) + conn.execute( + "INSERT INTO messages VALUES (1, ?, 'user', 'canonical turn', 1)", + (sid,), + ) + alias = models.Session(session_id=sid, workspace=str(tmp_path)) + with models.LOCK: + models.SESSIONS[sid] = alias + competing = { + "session_id": sid, + "_sidecar_generation_v1": 1, + "messages": [{"role": "user", "content": "competing turn"}], + } + target = session_dir / f"{sid}.json" + real_link = models.os.link + + def competing_link(src, dst): + target.write_text(json.dumps(competing), encoding="utf-8") + return real_link(src, dst) + + monkeypatch.setattr(models.os, "link", competing_link) + result = session_recovery.recover_missing_sidecars_from_state_db( + session_dir, db_path + ) + + assert result["materialized"] == 0 + assert result["details"] == [ + { + "session_id": sid, + "materialized": False, + "skipped": "sidecar_appeared_during_reconcile", + } + ] + assert json.loads(target.read_text(encoding="utf-8")) == competing + with models.LOCK: + assert sid not in models.SESSIONS + with pytest.raises(models.StaleSessionGenerationError): + alias.save(skip_index=True) + + def test_state_db_materialization_rechecks_delete_inside_authority( tmp_path, monkeypatch ): From a26b4284684c86ec34883f8d2d61bcca677bda64 Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Wed, 23 Sep 2026 23:41:40 +0000 Subject: [PATCH 4/7] fix(recovery): preserve newly adopted revision owners --- api/models.py | 47 +++++++++++++++----- api/session_recovery.py | 21 +++++++-- tests/test_session_sidecar_revision_fence.py | 46 ++++++++++++++++--- 3 files changed, 93 insertions(+), 21 deletions(-) diff --git a/api/models.py b/api/models.py index 347e641d9e2..1f282976096 100644 --- a/api/models.py +++ b/api/models.py @@ -742,19 +742,39 @@ def _retire_backup_if_owned( return not backup_path.exists() -def _invalidate_cached_session_generation(session_id: str) -> None: - """Evict and fence an alias after an out-of-band sidecar replacement.""" +def _invalidate_cached_session_generation( + session_id: str, + *, + expected_revision: SidecarRevision | None = None, +) -> bool: + """Evict and fence an alias after an out-of-band sidecar replacement. + + When ``expected_revision`` is provided, preserve a cache entry that has + already adopted another revision while the caller was publishing. + """ with LOCK: - cached = SESSIONS.pop(session_id, None) - if cached is not None: - cached._sidecar_revisions[session_id] = _sidecar_revision_record( - SidecarRevision( - sid=session_id, - state="INVALIDATED", - generation=-1, - digest_sha256=None, - ) + cached = SESSIONS.get(session_id) + if cached is None: + return False + if expected_revision is not None: + cached_record = cached._sidecar_revisions.get(session_id) + cached_revision = ( + SidecarRevision.absent(session_id) + if cached_record is None + else _coerce_sidecar_revision(cached_record, session_id) + ) + if cached_revision != expected_revision: + return False + SESSIONS.pop(session_id, None) + cached._sidecar_revisions[session_id] = _sidecar_revision_record( + SidecarRevision( + sid=session_id, + state="INVALIDATED", + generation=-1, + digest_sha256=None, ) + ) + return True @contextmanager @@ -2617,7 +2637,10 @@ def _save_owned_generation( intended ) else: - _invalidate_cached_session_generation(self.session_id) + _invalidate_cached_session_generation( + self.session_id, + expected_revision=expected_revision, + ) self._sidecar_revisions[self.session_id] = _sidecar_revision_record( SidecarRevision( sid=self.session_id, diff --git a/api/session_recovery.py b/api/session_recovery.py index 95457edadc9..c84113043ba 100644 --- a/api/session_recovery.py +++ b/api/session_recovery.py @@ -492,7 +492,10 @@ def _recover_session_owned( # A competing create made every cached ABSENT owner stale. # Evict it before reporting the failed recovery so a later # read/retry must adopt the visible generation. - _invalidate_cached_session_generation(session_path.stem) + _invalidate_cached_session_generation( + session_path.stem, + expected_revision=expected_live_revision, + ) tmp_path.unlink(missing_ok=True) return {**status, "restored": False, "stale_generation": True} tmp_path.unlink(missing_ok=True) @@ -505,7 +508,10 @@ def _recover_session_owned( # The live entry may already be visible despite the failed fsync. # Do not leave an absent-generation cache owner behind, and do not # call this restoration durable until a later successful write. - _invalidate_cached_session_generation(session_path.stem) + _invalidate_cached_session_generation( + session_path.stem, + expected_revision=expected_live_revision, + ) logger.warning("recover_session: copy failed for %s: %s", session_path, exc) try: tmp_path.unlink(missing_ok=True) @@ -759,6 +765,7 @@ def recover_missing_sidecars_from_state_db(session_dir: Path, state_db_path: Pat """Materialize missing WebUI JSON sidecars from canonical state.db rows.""" from api.models import ( SidecarPublicationDurabilityError, + SidecarRevision, _invalidate_cached_session_generation, _publish_sidecar_no_replace, _session_sidecar_authority, @@ -834,14 +841,20 @@ def recover_missing_sidecars_from_state_db(session_dir: Path, state_db_path: Pat except SidecarPublicationDurabilityError: # The entry exists, but no durable success was # confirmed. Fence the absent-generation owner. - _invalidate_cached_session_generation(sid) + _invalidate_cached_session_generation( + sid, + expected_revision=SidecarRevision.absent(sid), + ) raise materialized_now = True except FileExistsError: # Live sidecar appeared between the check and the link — keep it. # The cached owner (if any) still claims ABSENT and must not survive # this failed materialization result. - _invalidate_cached_session_generation(sid) + _invalidate_cached_session_generation( + sid, + expected_revision=SidecarRevision.absent(sid), + ) except OSError as exc: details.append({'session_id': sid, 'materialized': False, 'error': str(exc)}) detail_recorded = True diff --git a/tests/test_session_sidecar_revision_fence.py b/tests/test_session_sidecar_revision_fence.py index a3208b78f3f..63cb02a2b9e 100644 --- a/tests/test_session_sidecar_revision_fence.py +++ b/tests/test_session_sidecar_revision_fence.py @@ -1087,7 +1087,10 @@ def test_two_process_writers_have_exactly_one_cas_winner(tmp_path, monkeypatch): assert persisted["_sidecar_generation_v1"] == 2 -def test_recovery_expected_absent_uses_create_or_fail(tmp_path, monkeypatch): +@pytest.mark.parametrize("cache_visible_owner", [False, True]) +def test_recovery_expected_absent_uses_create_or_fail( + tmp_path, monkeypatch, cache_visible_owner +): from api import models, session_recovery session_dir = tmp_path / "sessions" @@ -1112,9 +1115,16 @@ def test_recovery_expected_absent_uses_create_or_fail(tmp_path, monkeypatch): with models.LOCK: models.SESSIONS[alias.session_id] = alias real_link = session_recovery.os.link + winner = {} def competing_link(src, dst): Path(dst).write_text(json.dumps(competing), encoding="utf-8") + if cache_visible_owner: + loaded = models.Session.load(alias.session_id) + assert loaded is not None + with models.LOCK: + models.SESSIONS[alias.session_id] = loaded + winner["session"] = loaded return real_link(src, dst) monkeypatch.setattr(session_recovery.os, "link", competing_link) @@ -1123,9 +1133,18 @@ def competing_link(src, dst): assert result["stale_generation"] is True assert json.loads(session_path.read_text(encoding="utf-8")) == competing with models.LOCK: - assert alias.session_id not in models.SESSIONS + if cache_visible_owner: + assert models.SESSIONS[alias.session_id] is winner["session"] + else: + assert alias.session_id not in models.SESSIONS with pytest.raises(models.StaleSessionGenerationError): alias.save(skip_index=True) + if cache_visible_owner: + winner["session"].title = "valid owner survives" + winner["session"].save(skip_index=True) + persisted = json.loads(session_path.read_text(encoding="utf-8")) + assert persisted["title"] == "valid owner survives" + assert persisted["_sidecar_generation_v1"] == 2 @pytest.mark.skipif(os.name == "nt", reason="POSIX hard-link publication") @@ -2142,8 +2161,9 @@ def fail_once(directory): assert json.loads(reloaded.path.read_text(encoding="utf-8"))["_sidecar_generation_v1"] == 2 -def test_state_db_materialization_create_conflict_invalidates_absent_owner( - tmp_path, monkeypatch +@pytest.mark.parametrize("cache_visible_owner", [False, True]) +def test_state_db_materialization_create_conflict_reconciles_cached_owner( + tmp_path, monkeypatch, cache_visible_owner ): from api import models, session_recovery @@ -2178,9 +2198,16 @@ def test_state_db_materialization_create_conflict_invalidates_absent_owner( } target = session_dir / f"{sid}.json" real_link = models.os.link + winner = {} def competing_link(src, dst): target.write_text(json.dumps(competing), encoding="utf-8") + if cache_visible_owner: + loaded = models.Session.load(sid) + assert loaded is not None + with models.LOCK: + models.SESSIONS[sid] = loaded + winner["session"] = loaded return real_link(src, dst) monkeypatch.setattr(models.os, "link", competing_link) @@ -2198,9 +2225,18 @@ def competing_link(src, dst): ] assert json.loads(target.read_text(encoding="utf-8")) == competing with models.LOCK: - assert sid not in models.SESSIONS + if cache_visible_owner: + assert models.SESSIONS[sid] is winner["session"] + else: + assert sid not in models.SESSIONS with pytest.raises(models.StaleSessionGenerationError): alias.save(skip_index=True) + if cache_visible_owner: + winner["session"].title = "valid owner survives" + winner["session"].save(skip_index=True) + persisted = json.loads(target.read_text(encoding="utf-8")) + assert persisted["title"] == "valid owner survives" + assert persisted["_sidecar_generation_v1"] == 2 def test_state_db_materialization_rechecks_delete_inside_authority( From 57d06a69b976d07cf82ed4a54fad241379ed3a4e Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Thu, 24 Sep 2026 11:58:23 +0000 Subject: [PATCH 5/7] fix(sidecar): recover malformed compression parents and harden publication --- ARCHITECTURE.md | 5 + api/models.py | 68 ++++++++- api/session_discoverability.py | 23 ++- api/session_recovery.py | 10 +- api/streaming.py | 43 ++++-- tests/test_pr7036_review_regressions.py | 156 ++++++++++++++++++++ tests/test_save_count_without_full_parse.py | 47 +++++- 7 files changed, 320 insertions(+), 32 deletions(-) create mode 100644 tests/test_pr7036_review_regressions.py diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index ff341f72709..6aee45ced5c 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -267,6 +267,11 @@ per-SID thread and cross-process authority. This includes normal `Session.save() writes, backup recovery, and discoverability repairs. Existing sidecars are fenced by their generation plus exact digest; first publication is create-or-fail, so a stale alias or repair cannot overwrite a sidecar that appeared concurrently. +The digest is recomputed in chunks on each save, while only the bounded JSON +metadata prefix is parsed for the generation. This avoids decoding a large +transcript on every save but intentionally keeps a linear file scan: a cached +prefix or file stat alone cannot detect an external same-generation body rewrite. +Legacy layouts with metadata after messages fall back to a full parse. Out-of-band replacements increment `_sidecar_generation_v1` and invalidate cached aliases before later saves can proceed. diff --git a/api/models.py b/api/models.py index 1f282976096..fa92394985a 100644 --- a/api/models.py +++ b/api/models.py @@ -350,12 +350,63 @@ def _sidecar_revision_from_bytes( def _read_sidecar_revision(path: Path, sid: str | None = None) -> SidecarRevision: + """Hash the exact file in bounded memory; parse only its metadata prefix. + + A prefix-only CAS cannot detect an out-of-band body rewrite at the same + generation. Never substitute inode/size/mtime or a cached count for the + digest: even a same-length in-place rewrite must fence the stale owner. + """ resolved_sid = sid or path.stem try: - raw = path.read_bytes() + with open(path, "rb") as source: + digest = hashlib.sha256() + prefix = b"" + stop = None + stage = min(_METADATA_PREFIX_FIRST_STAGE_BYTES, _METADATA_PREFIX_MAX_BYTES) + while len(prefix) < _METADATA_PREFIX_MAX_BYTES: + chunk = source.read(min(stage, _METADATA_PREFIX_MAX_BYTES - len(prefix))) + if not chunk: + break + digest.update(chunk) + prefix += chunk + decoded = prefix.decode("utf-8", errors="ignore") + stop = _find_top_level_json_key(decoded, "messages") + scenes_stop = _find_top_level_json_key(decoded, "anchor_activity_scenes") + if scenes_stop is not None and (stop is None or scenes_stop < stop): + stop = scenes_stop + if stop is not None: + break + stage *= 2 + for chunk in iter(lambda: source.read(1024 * 1024), b""): + digest.update(chunk) except FileNotFoundError: return SidecarRevision.absent(resolved_sid) - return _sidecar_revision_from_bytes(resolved_sid, raw) + + if stop is None: + # Legacy layouts without a bounded metadata stop still need their exact + # generation. This is the exceptional compatibility path, not a hot save. + return _sidecar_revision_from_bytes(resolved_sid, path.read_bytes()) + metadata = decoded[:stop].rstrip() + if metadata.endswith(","): + metadata = metadata[:-1].rstrip() + try: + parsed = json.loads(f"{metadata}\n}}") + except (json.JSONDecodeError, ValueError): + parsed = None + raw_generation = ( + parsed.get("_sidecar_generation_v1") if isinstance(parsed, dict) else None + ) + if raw_generation is None: + # Recovery/legacy writers may append the generation after messages. + # Those files must be parsed fully once; a normal Session.save rewrites + # the generation into the bounded metadata prefix. + return _sidecar_revision_from_bytes(resolved_sid, path.read_bytes()) + return SidecarRevision( + sid=resolved_sid, + state="PRESENT", + generation=raw_generation if type(raw_generation) is int and raw_generation >= 0 else 0, + digest_sha256=digest.hexdigest(), + ) def _read_sidecar_snapshot(path: Path, sid: str) -> tuple[SidecarRevision, dict]: @@ -673,6 +724,8 @@ def _archive_incomparable_backup( session_id: str, backup_path: Path, backup_receipt: SidecarRevision, + *, + archive_name: str | None = None, ) -> Path: """Preserve an incomparable primary backup before promoting a newer one.""" if ( @@ -684,7 +737,7 @@ def _archive_incomparable_backup( f"Cannot archive unverified recoverable backup for {session_id!r}" ) archive_path = backup_path.with_name( - f"{backup_path.name}.archive-{backup_receipt.digest_sha256}" + f"{archive_name or backup_path.name}.archive-{backup_receipt.digest_sha256}" ) if not archive_path.exists(): archive_tmp = backup_path.parent / ( @@ -2594,6 +2647,7 @@ def _save_owned_generation( ) from exc tmp = self.path.with_suffix(f'.tmp.{os.getpid()}.{threading.current_thread().ident}') + replace_started = False try: with open(tmp, 'w', encoding='utf-8', newline='\n') as f: f.write(payload) @@ -2609,6 +2663,7 @@ def _save_owned_generation( ) from exc tmp.unlink(missing_ok=True) else: + replace_started = True _safe_replace(tmp, self.path) _fsync_sidecar_directory(self.path.parent) self._sidecar_revisions[self.session_id] = _sidecar_revision_record( @@ -2619,10 +2674,9 @@ def _save_owned_generation( ) ) except Exception as exc: - if isinstance(exc, SidecarPublicationDurabilityError): - # The link/rename completed, but the directory entry was not - # confirmed durable. Track only the *visible* exact payload so - # this owner may retry; never report this save as successful. + if isinstance(exc, SidecarPublicationDurabilityError) or replace_started: + # Link or replacement may have landed before a durability error. + # Adopt only the exact visible bytes; never call this save durable. intended = _sidecar_revision_from_bytes( self.session_id, payload.encode('utf-8'), diff --git a/api/session_discoverability.py b/api/session_discoverability.py index 37b02448cf6..2638b0a61ee 100644 --- a/api/session_discoverability.py +++ b/api/session_discoverability.py @@ -454,6 +454,7 @@ def _clear_sidecar_cli_flag(session_dir: Path, sid: str, backup_dir: Path, backe _invalidate_cached_session_generation, _read_sidecar_revision, _session_sidecar_authority, + _sidecar_revision_from_bytes, ) with _session_sidecar_authority(sid, session_dir=session_dir): @@ -472,8 +473,26 @@ def _clear_sidecar_cli_flag(session_dir: Path, sid: str, backup_dir: Path, backe payload["_sidecar_generation_v1"] = revision.generation + 1 if _read_sidecar_revision(path, sid) != revision: return {"session_id": sid, "action": "clear_sidecar_cli_flag", "applied": False, "skipped": "stale_generation"} - _atomic_write_json(path, payload) - _fsync_sidecar_directory(path.parent) + intended = _sidecar_revision_from_bytes( + sid, json.dumps(payload, ensure_ascii=False, indent=2).encode("utf-8"), + parsed=payload, + ) + try: + _atomic_write_json(path, payload) + _fsync_sidecar_directory(path.parent) + except OSError: + # The rename can have completed before the directory fsync fails. + # A cached alias still claiming the old revision may not write again. + try: + visible = _read_sidecar_revision(path, sid) + except OSError: + visible = None + if visible == intended: + _invalidate_cached_session_generation(sid, expected_revision=revision) + elif visible != revision: + # Foreign or unreadable publication: do not leave an old owner. + _invalidate_cached_session_generation(sid, expected_revision=revision) + raise _invalidate_cached_session_generation(sid) return {"session_id": sid, "action": "clear_sidecar_cli_flag", "applied": True, "backup": backup} diff --git a/api/session_recovery.py b/api/session_recovery.py index c84113043ba..2d535be2439 100644 --- a/api/session_recovery.py +++ b/api/session_recovery.py @@ -461,6 +461,7 @@ def _recover_session_owned( tmp_path = session_path.with_suffix( f'.json.recover.tmp.{os.getpid()}.{threading.current_thread().ident}' ) + replace_started = False try: backup = json.loads(bak_path.read_text(encoding='utf-8')) if not isinstance(backup, dict): @@ -500,14 +501,15 @@ def _recover_session_owned( return {**status, "restored": False, "stale_generation": True} tmp_path.unlink(missing_ok=True) else: + replace_started = True _safe_replace(tmp_path, session_path) _fsync_sidecar_directory(session_path.parent) _invalidate_cached_session_generation(session_path.stem) except (OSError, json.JSONDecodeError, ValueError) as exc: - if isinstance(exc, SidecarPublicationDurabilityError): - # The live entry may already be visible despite the failed fsync. - # Do not leave an absent-generation cache owner behind, and do not - # call this restoration durable until a later successful write. + if isinstance(exc, SidecarPublicationDurabilityError) or replace_started: + # A create or replace may be visible despite failed directory fsync. + # Recovery has no live owner to adopt: evict only the prior alias, + # then require a fresh load before any subsequent write. _invalidate_cached_session_generation( session_path.stem, expected_revision=expected_live_revision, diff --git a/api/streaming.py b/api/streaming.py index eae0551dfb9..74c01f3aa10 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -5660,33 +5660,50 @@ def _preserve_pre_compression_snapshot(s, old_sid: str) -> None: if not old_path.exists(): return try: - existing_text = old_path.read_text(encoding='utf-8') + from api.models import ( + Session, _archive_incomparable_backup, _sidecar_revision_from_bytes, + _sidecar_revision_record, + ) + existing_raw = old_path.read_bytes() + malformed_revision = None try: - existing = json.loads(existing_text) + existing = json.loads(existing_raw) + if not isinstance(existing, dict): + raise ValueError("parent sidecar is not an object") existing_msgs = len(existing.get('messages') or []) - existing_snapshot = bool(existing.get('pre_compression_snapshot')) - except (json.JSONDecodeError, ValueError): + except (json.JSONDecodeError, UnicodeDecodeError, ValueError): # Treat corrupt/malformed old JSON as missing history and rewrite it - # from the in-memory pre-compression messages below. That is safer - # than leaving an unreadable recovery snapshot behind. + # from the in-memory pre-compression messages, but preserve the raw + # generation before replacing it. Session.load cannot parse it. + existing = {} existing_msgs = -1 - existing_snapshot = False + malformed_revision = _sidecar_revision_from_bytes(old_sid, existing_raw) if len(s.messages) > existing_msgs: # In-memory messages are newer than the file; save the full old # snapshot from the current session object while preserving its # pre-existing parent_session_id lineage. - from api.models import Session - - owned_old = Session.load(old_sid) - if owned_old is None: - return + if malformed_revision is None: + owned_old = Session.load(old_sid) + if owned_old is None: + return + revisions = dict(owned_old._sidecar_revisions) + else: + # Archive with the same artifact family as backup generations; + # do not destroy malformed bytes if this repair cannot commit. + _archive_incomparable_backup( + old_sid, old_path, malformed_revision, + archive_name=f"{old_path.name}.bak", + ) + revisions = {old_sid: _sidecar_revision_record(malformed_revision)} snapshot = copy.copy(s) - snapshot._sidecar_revisions = dict(owned_old._sidecar_revisions) + snapshot._sidecar_revisions = revisions snapshot.session_id = old_sid snapshot.parent_session_id = existing.get( 'parent_session_id', getattr(s, 'parent_session_id', None), ) + if snapshot.parent_session_id == old_sid: + snapshot.parent_session_id = None snapshot.pre_compression_snapshot = True snapshot.pinned = False # Stage-359 / PR #2295: clear runtime stream-state fields on the diff --git a/tests/test_pr7036_review_regressions.py b/tests/test_pr7036_review_regressions.py new file mode 100644 index 00000000000..a7f50eb8cc7 --- /dev/null +++ b/tests/test_pr7036_review_regressions.py @@ -0,0 +1,156 @@ +"""Maintainer re-gate: malformed compression parents and post-replace failures.""" +import json +from pathlib import Path + +import pytest + +from api import models, session_discoverability, session_recovery, streaming + + +@pytest.fixture +def store(tmp_path, monkeypatch): + directory = tmp_path / "sessions" + directory.mkdir() + monkeypatch.setattr(models, "SESSION_DIR", directory) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", directory / "_index.json") + monkeypatch.setattr(streaming, "SESSION_DIR", directory) + monkeypatch.setattr(models, "SESSIONS", {}) + return directory + + +def test_malformed_compression_parent_is_archived_and_restored(store): + parent = models.Session(session_id="parent", messages=[{"role": "user", "content": "old"}]) + parent.save(skip_index=True) + malformed = b'{"session_id": "parent", broken' + parent.path.write_bytes(malformed) + child = models.Session( + session_id="child", parent_session_id="parent", + messages=[{"role": "user", "content": "recoverable history"}], + ) + + streaming._preserve_pre_compression_snapshot(child, "parent") + + loaded = models.Session.load("parent") + assert loaded is not None + assert loaded.pre_compression_snapshot is True + assert loaded.messages == child.messages + assert loaded.parent_session_id is None # no self-referential lineage + archives = list(store.glob("parent.json.bak.archive-*")) + assert len(archives) == 1 + assert archives[0].read_bytes() == malformed + assert child.session_id == "child" + + +def test_replace_fsync_failure_adopts_exact_visible_revision(store, monkeypatch): + session = models.Session(session_id="owner", messages=[{"role": "user", "content": "old"}]) + session.save(skip_index=True) + session.messages.append({"role": "assistant", "content": "new"}) + real_fsync = models._fsync_sidecar_directory + fail = True + + def fail_once(directory): + nonlocal fail + if fail: + fail = False + raise OSError("injected post-replace fsync failure") + return real_fsync(directory) + + monkeypatch.setattr(models, "_fsync_sidecar_directory", fail_once) + with pytest.raises(OSError, match="post-replace"): + session.save(skip_index=True) + assert [m["content"] for m in models.Session.load("owner").messages] == ["old", "new"] + session.save(skip_index=True) + assert [m["content"] for m in models.Session.load("owner").messages] == ["old", "new"] + + +def test_replace_fsync_failure_foreign_visible_revision_invalidates(store, monkeypatch): + session = models.Session(session_id="owner", messages=[{"role": "user", "content": "old"}]) + session.save(skip_index=True) + models.SESSIONS["owner"] = session + + def foreign_after_replace(directory): + raw = json.loads(session.path.read_text(encoding="utf-8")) + raw["messages"].append({"role": "assistant", "content": "foreign"}) + session.path.write_text(json.dumps(raw), encoding="utf-8") + raise OSError("injected foreign publication") + + monkeypatch.setattr(models, "_fsync_sidecar_directory", foreign_after_replace) + session.messages.append({"role": "assistant", "content": "ours"}) + with pytest.raises(OSError, match="foreign publication"): + session.save(skip_index=True) + assert "owner" not in models.SESSIONS + with pytest.raises(models.StaleSessionGenerationError): + session.save(skip_index=True) + assert models.Session.load("owner").messages[-1]["content"] == "foreign" + + +def test_flag_repair_post_replace_failure_evicts_cached_owner(store, tmp_path, monkeypatch): + session = models.Session(session_id="repair", messages=[{"role": "user", "content": "kept"}]) + session.is_cli_session = True + session.source_tag = "webui" + session.save(skip_index=True) + models.SESSIONS["repair"] = session + monkeypatch.setattr(models, "_fsync_sidecar_directory", lambda _: (_ for _ in ()).throw(OSError("fsync repair"))) + with pytest.raises(OSError, match="fsync repair"): + session_discoverability._clear_sidecar_cli_flag(store, "repair", tmp_path / "backup", {}) + assert json.loads(session.path.read_text(encoding="utf-8"))["is_cli_session"] is False + assert "repair" not in models.SESSIONS + with pytest.raises(models.StaleSessionGenerationError): + session.save(skip_index=True) + + +def test_backup_recovery_post_replace_failure_evicts_cached_owner(store, monkeypatch): + session = models.Session(session_id="recover", messages=[{"role": "user", "content": "old"}]) + session.save(skip_index=True) + session.messages.append({"role": "assistant", "content": "lost if shrunk"}) + session.save(skip_index=True) + session.messages = [{"role": "user", "content": "old"}] + session.save(skip_index=True) + models.SESSIONS["recover"] = session + monkeypatch.setattr(models, "_fsync_sidecar_directory", lambda _: (_ for _ in ()).throw(OSError("fsync recovery"))) + result = session_recovery.recover_session(session.path) + assert result["restored"] is False + assert "recover" not in models.SESSIONS + assert len(json.loads(session.path.read_text(encoding="utf-8"))["messages"]) == 2 + with pytest.raises(models.StaleSessionGenerationError): + session.save(skip_index=True) + + +def test_large_growing_save_streams_exact_revision_without_full_json_parse(store, monkeypatch): + session = models.Session( + session_id="large", messages=[{"role": "user", "content": "x" * (8 * 1024 * 1024)}], + ) + session.save(skip_index=True) + real_loads = models.json.loads + real_read_bytes = Path.read_bytes + parse_sizes = [] + full_reads = [] + + def tracking_loads(data, *args, **kwargs): + parse_sizes.append(len(data)) + return real_loads(data, *args, **kwargs) + + def tracking_bytes(path, *args, **kwargs): + if path == session.path: + full_reads.append(path) + return real_read_bytes(path, *args, **kwargs) + + monkeypatch.setattr(models.json, "loads", tracking_loads) + monkeypatch.setattr(Path, "read_bytes", tracking_bytes) + session.messages.append({"role": "assistant", "content": "next"}) + session.save(skip_index=True) + assert full_reads == [] + assert parse_sizes and max(parse_sizes) < 1024 * 1024 + assert session.path.with_suffix(".json.bak").exists() is False + + +def test_same_generation_body_rewrite_is_still_fenced(store): + session = models.Session( + session_id="body_rewrite", messages=[{"role": "user", "content": "aaa"}], + ) + session.save(skip_index=True) + before = session.path.read_bytes() + session.path.write_bytes(before.replace(b"aaa", b"bbb")) + with pytest.raises(models.StaleSessionGenerationError): + session.save(skip_index=True) + assert b"bbb" in session.path.read_bytes() diff --git a/tests/test_save_count_without_full_parse.py b/tests/test_save_count_without_full_parse.py index 543ace4111c..c2f722462b6 100644 --- a/tests/test_save_count_without_full_parse.py +++ b/tests/test_save_count_without_full_parse.py @@ -156,9 +156,11 @@ def test_empty_active_snapshot_is_still_refused(session_store, monkeypatch): no signature to trust and refusing may cost today's single full read -- never more, and it must still refuse.""" _make(session_store, "r1", 5) - empty = M.Session(session_id="r1", title="T", workspace=str(session_store.parent), - model="glm", messages=[], active_stream_id="a" * 32, - pending_user_message="still typing") + empty = M.Session.load("r1") + assert empty is not None + empty.messages = [] + empty.active_stream_id = "a" * 32 + empty.pending_user_message = "still typing" calls = _spy_full_reads(monkeypatch, empty.path) empty.save() @@ -185,8 +187,9 @@ def test_legacy_sidecar_without_message_count_still_backs_up_on_shrink(session_s parse and keep the safeguard. Silently treating "no count" as "no shrink" would reopen #1558.""" _write_legacy_without_count(session_store, "l1", 5) - s = M.Session(session_id="l1", title="Legacy", workspace=str(session_store.parent), - model="glm", messages=_msgs(3)) + s = M.Session.load("l1") + assert s is not None + s.messages = _msgs(3) s.save() @@ -207,6 +210,12 @@ def test_file_changed_on_disk_since_last_save_falls_back_and_still_backs_up(sess "message_count": 6, "messages": _msgs(6)} s.path.write_text(json.dumps(external, indent=2), encoding="utf-8") + s.messages = _msgs(4) + with pytest.raises(M.StaleSessionGenerationError): + s.save() + assert json.loads(s.path.read_text(encoding="utf-8"))["messages"] == _msgs(6) + s = M.Session.load("x1") + assert s is not None s.messages = _msgs(4) s.save() @@ -225,6 +234,12 @@ def test_file_changed_on_disk_without_a_count_still_backs_up_on_shrink(session_s "messages": _msgs(6)} s.path.write_text(json.dumps(external, indent=2), encoding="utf-8") + s.messages = _msgs(4) + with pytest.raises(M.StaleSessionGenerationError): + s.save() + assert json.loads(s.path.read_text(encoding="utf-8"))["messages"] == _msgs(6) + s = M.Session.load("x2") + assert s is not None s.messages = _msgs(4) s.save() @@ -252,6 +267,12 @@ def test_unmarked_stale_count_from_a_foreign_writer_still_backs_up_on_shrink(ses "message_count": 2, "messages": _msgs(6)} # stale count, NO _mc_v s.path.write_text(json.dumps(foreign, indent=2), encoding="utf-8") + s.messages = _msgs(4) + with pytest.raises(M.StaleSessionGenerationError): + s.save() + assert json.loads(s.path.read_text(encoding="utf-8"))["messages"] == _msgs(6) + s = M.Session.load("x3") + assert s is not None s.messages = _msgs(4) s.save() @@ -270,6 +291,12 @@ def test_wrong_marker_version_still_backs_up_on_shrink(session_store): "message_count": 2, "messages": _msgs(6)} s.path.write_text(json.dumps(foreign, indent=2), encoding="utf-8") + s.messages = _msgs(4) + with pytest.raises(M.StaleSessionGenerationError): + s.save() + assert json.loads(s.path.read_text(encoding="utf-8"))["messages"] == _msgs(6) + s = M.Session.load("x4") + assert s is not None s.messages = _msgs(4) s.save() @@ -288,7 +315,10 @@ def test_first_save_remarks_the_file_and_the_fast_path_resumes(session_store, mo s.path.write_text(json.dumps(foreign, indent=2), encoding="utf-8") s.messages = _msgs(6) - s.save() # equal-count save; unmarked prefix count ignored, full parse, no shrink + with pytest.raises(M.StaleSessionGenerationError): + s.save() + s = M.Session.load("x5") + s.save() # authorized legacy file becomes marked; no shrink assert json.loads(s.path.read_text(encoding="utf-8"))["_mc_v"] == M._MESSAGE_COUNT_MARKER @@ -377,6 +407,11 @@ def _render(n): "premise: the rewrite must be invisible to (inode, size, mtime_ns)" assert len(loaded.messages) == 2, "premise: this object remembers 2 messages" + loaded.messages = _msgs(3) + with pytest.raises(M.StaleSessionGenerationError): + loaded.save() + assert json.loads(p.read_text(encoding="utf-8"))["messages"] == _msgs(5) + loaded = M.Session.load("i1") loaded.messages = _msgs(3) loaded.save() From 032724cd5b13220099edb08b32a3d2358aa23a12 Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Thu, 24 Sep 2026 22:52:06 +0000 Subject: [PATCH 6/7] fix(session): preserve live cleanup and retire stale compression backups --- api/routes.py | 69 +++++++++----- ...t_issue4836_manual_compression_recovery.py | 37 ++++++++ tests/test_session_sidecar_revision_fence.py | 93 +++++++++++++++++++ 3 files changed, 176 insertions(+), 23 deletions(-) diff --git a/api/routes.py b/api/routes.py index 0b94890c921..cffa940b688 100644 --- a/api/routes.py +++ b/api/routes.py @@ -23044,26 +23044,41 @@ def _handle_sessions_cleanup(handler, body, zero_only=False): ) sid = p.stem - with _session_sidecar_authority(sid): - revision, payload = _read_sidecar_snapshot(p, sid) - messages = payload.get("messages") - if messages is None: - messages = [] - if not isinstance(messages, list): - continue - title = payload.get("title", "Untitled") - should_delete = not messages and ( - zero_only or title == "Untitled" - ) - if not should_delete: - continue - phase1_delete_candidate_ids.add(sid) - if not _delete_session_sidecar_artifacts_locked( - sid, - expected_revision=revision, - ): - continue - cleaned += 1 + # Chat-start and worker persistence use this lock before sidecar + # authority. Keep the same order so an empty durable transcript + # cannot be deleted between pending-turn publication and reply save. + with _get_session_agent_lock(sid): + with _session_sidecar_authority(sid): + revision, payload = _read_sidecar_snapshot(p, sid) + if payload.get("active_stream_id") or payload.get( + "pending_user_message" + ): + continue + with LOCK: + cached = SESSIONS.get(sid) + if cached is not None and ( + getattr(cached, "active_stream_id", None) + or getattr(cached, "pending_user_message", None) + ): + continue + messages = payload.get("messages") + if messages is None: + messages = [] + if not isinstance(messages, list): + continue + title = payload.get("title", "Untitled") + should_delete = not messages and ( + zero_only or title == "Untitled" + ) + if not should_delete: + continue + phase1_delete_candidate_ids.add(sid) + if not _delete_session_sidecar_artifacts_locked( + sid, + expected_revision=revision, + ): + continue + cleaned += 1 except Exception: logger.debug("Failed to clean up session file %s", p, exc_info=True) @@ -28324,14 +28339,22 @@ def _summarize_manual_compression( s.truncation_boundary = compress_watermark s.compression_anchor_mode = "manual" s.last_prompt_tokens = new_tokens + from api.models import _read_sidecar_revision, _retire_backup_if_owned + + backup_path = s.path.with_suffix(".json.bak") + existing_backup_receipt = _read_sidecar_revision( + backup_path, + s.session_id, + ) + if existing_backup_receipt.state != "PRESENT": + existing_backup_receipt = None backup_receipt = s.save() # Drop stale backups that would undo an intentional manual compress. try: - from api.models import _read_sidecar_revision, _retire_backup_if_owned _retire_backup_if_owned( s.session_id, - s.path.with_suffix(".json.bak"), - backup_receipt, + backup_path, + backup_receipt or existing_backup_receipt, _read_sidecar_revision(s.path, s.session_id), ) except OSError: diff --git a/tests/test_issue4836_manual_compression_recovery.py b/tests/test_issue4836_manual_compression_recovery.py index 3ddc0a94093..0dce2014661 100644 --- a/tests/test_issue4836_manual_compression_recovery.py +++ b/tests/test_issue4836_manual_compression_recovery.py @@ -132,6 +132,43 @@ def test_manual_compress_persists_truncation_boundary(monkeypatch, cleanup_test_ assert loaded.messages == original_messages +def test_manual_compress_retires_existing_backup_when_visible_count_is_unchanged( + monkeypatch, + cleanup_test_sessions, + tmp_path, +): + session_dir = tmp_path / "sessions" + session_dir.mkdir(parents=True) + monkeypatch.setattr(models, "SESSION_DIR", session_dir) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", session_dir / "_index.json") + models.SESSIONS.clear() + + sid = f"issue4836_retire_stale_backup_{time.time_ns()}" + cleanup_test_sessions.append(sid) + session = Session( + session_id=sid, + title="Untitled", + workspace=str(tmp_path), + model="openai/gpt-5.4-mini", + messages=[ + _msg("user", "one", 1.0), + _msg("assistant", "two", 2.0), + _msg("user", "three", 3.0), + _msg("assistant", "four", 4.0), + ], + ) + session.save(touch_updated_at=False) + backup_path = session.path.with_suffix(".json.bak") + backup_path.write_bytes(session.path.read_bytes()) + + _install_fake_compression_runtime(monkeypatch, _FakeAgent) + handler = _FakeHandler() + _handle_session_compress(handler, {"session_id": sid}) + + assert handler.status == 200 + assert not backup_path.exists() + + def test_manual_compress_blocks_state_db_replay(monkeypatch, cleanup_test_sessions, tmp_path): session_dir = tmp_path / "sessions" session_dir.mkdir(parents=True) diff --git a/tests/test_session_sidecar_revision_fence.py b/tests/test_session_sidecar_revision_fence.py index 63cb02a2b9e..91fb1dc59d6 100644 --- a/tests/test_session_sidecar_revision_fence.py +++ b/tests/test_session_sidecar_revision_fence.py @@ -758,6 +758,99 @@ def load_then_commit_writer(cls, candidate_sid): assert sid in models._load_webui_deleted_session_tombstone() +def test_empty_session_cleanup_waits_for_live_turn_and_preserves_reply( + tmp_path, + monkeypatch, +): + from api import models, routes + + session_dir = tmp_path / "sessions" + _patch_store(monkeypatch, models, session_dir) + monkeypatch.setattr(routes, "SESSION_DIR", session_dir) + monkeypatch.setattr(routes, "SESSION_INDEX_FILE", session_dir / "_index.json") + monkeypatch.setattr(routes, "SESSIONS", models.SESSIONS) + monkeypatch.setattr(routes, "LOCK", models.LOCK) + monkeypatch.setattr(routes, "j", lambda _handler, payload: payload) + sid = "cleanup-live-pending-turn" + session = models.Session( + session_id=sid, + title="Untitled", + workspace=str(tmp_path), + messages=[], + ) + session.save(skip_index=True) + + lock_attempted = threading.Event() + authority_entered = threading.Event() + real_get_agent_lock = routes._get_session_agent_lock + real_authority = models._session_sidecar_authority + + def observed_get_agent_lock(candidate_sid): + if ( + candidate_sid == sid + and threading.current_thread().name == "cleanup-live-session" + ): + lock_attempted.set() + return real_get_agent_lock(candidate_sid) + + @contextmanager + def observed_authority(candidate_sid, *args, **kwargs): + if ( + candidate_sid == sid + and threading.current_thread().name == "cleanup-live-session" + ): + authority_entered.set() + with real_authority(candidate_sid, *args, **kwargs): + yield + + monkeypatch.setattr(routes, "_get_session_agent_lock", observed_get_agent_lock) + monkeypatch.setattr(models, "_session_sidecar_authority", observed_authority) + result = {} + + def cleanup(): + result["payload"] = routes._handle_sessions_cleanup(object(), {}) + + agent_lock = routes._get_session_agent_lock(sid) + assert agent_lock.acquire(timeout=1) + cleanup_thread = threading.Thread( + target=cleanup, + name="cleanup-live-session", + daemon=True, + ) + try: + cleanup_thread.start() + assert lock_attempted.wait(timeout=1), ( + "cleanup did not use the live turn's agent lock" + ) + assert not authority_entered.is_set(), ( + "cleanup reached durable deletion while the live turn owned the agent lock" + ) + + session.active_stream_id = "stream-cleanup-race" + session.pending_user_message = "keep this live request" + session.pending_started_at = time.time() + session.save(skip_index=True) + session.messages.extend( + [ + {"role": "user", "content": "keep this live request"}, + {"role": "assistant", "content": "reply survived cleanup"}, + ] + ) + session.active_stream_id = None # type: ignore[assignment] + session.pending_user_message = None # type: ignore[assignment] + session.pending_started_at = None + session.save(skip_index=True) + finally: + agent_lock.release() + + cleanup_thread.join(timeout=5) + assert not cleanup_thread.is_alive() + assert result["payload"] == {"ok": True, "cleaned": 0} + loaded = models.Session.load(sid) + assert loaded is not None + assert loaded.messages[-1]["content"] == "reply survived cleanup" + + def test_empty_session_cleanup_does_not_count_partial_delete_as_index_ghost( tmp_path, monkeypatch, From dc2877b23ba7b2525df7b2dd1252758a714c7beb Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Thu, 24 Sep 2026 23:09:33 +0000 Subject: [PATCH 7/7] fix(repair): invalidate owner after partial publication --- api/session_discoverability.py | 29 +++++++++- tests/test_session_discoverability_repair.py | 56 ++++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/api/session_discoverability.py b/api/session_discoverability.py index 2638b0a61ee..b25a0af9828 100644 --- a/api/session_discoverability.py +++ b/api/session_discoverability.py @@ -529,6 +529,7 @@ def _materialize_sidecar_from_state_db(session_dir: Path, state_db_path: Path | _fsync_sidecar_directory, _invalidate_cached_session_generation, _publish_sidecar_no_replace, + _read_sidecar_revision, _session_sidecar_authority, ) from api.session_recovery import ( @@ -541,6 +542,9 @@ def _materialize_sidecar_from_state_db(session_dir: Path, state_db_path: Path | with _session_sidecar_authority(sid, session_dir=session_dir): if target.exists(): return {"session_id": sid, "action": "materialize_sidecar_from_state_db", "applied": False, "skipped": "sidecar_exists"} + expected_revision = _read_sidecar_revision(target, sid) + if expected_revision.state != "ABSENT": + return {"session_id": sid, "action": "materialize_sidecar_from_state_db", "applied": False, "skipped": "sidecar_exists"} if _durable_tombstone_marks_deleted_webui_session(session_dir, sid): return {"session_id": sid, "action": "materialize_sidecar_from_state_db", "applied": False, "skipped": "deleted_tombstone"} rows = { @@ -568,12 +572,36 @@ def _materialize_sidecar_from_state_db(session_dir: Path, state_db_path: Path | try: _publish_sidecar_no_replace(tmp, target) except FileExistsError: + _invalidate_cached_session_generation( + sid, + expected_revision=expected_revision, + ) return {"session_id": sid, "action": "materialize_sidecar_from_state_db", "applied": False, "skipped": "sidecar_appeared_during_repair"} + except OSError: + # The create-only link/rename may be visible even when its + # directory fsync fails. Fence only the cached ABSENT owner; + # preserve any alias that already adopted the visible revision. + try: + visible_revision = _read_sidecar_revision(target, sid) + except OSError: + visible_revision = None + if visible_revision != expected_revision: + _invalidate_cached_session_generation( + sid, + expected_revision=expected_revision, + ) + raise finally: try: tmp.unlink(missing_ok=True) except OSError: pass + # Invalidate before index maintenance so a later index/fsync failure + # cannot leave an ABSENT cached owner behind a visible sidecar. + _invalidate_cached_session_generation( + sid, + expected_revision=expected_revision, + ) index_updated = False index_path = session_dir / "_index.json" index_payload = _read_json(index_path) @@ -590,7 +618,6 @@ def _materialize_sidecar_from_state_db(session_dir: Path, state_db_path: Path | _atomic_write_json(index_path, index_payload) _fsync_sidecar_directory(index_path.parent) index_updated = True - _invalidate_cached_session_generation(sid) return { "session_id": sid, "action": "materialize_sidecar_from_state_db", diff --git a/tests/test_session_discoverability_repair.py b/tests/test_session_discoverability_repair.py index fe609c17af2..55e431315ec 100644 --- a/tests/test_session_discoverability_repair.py +++ b/tests/test_session_discoverability_repair.py @@ -1,3 +1,4 @@ +import errno import json import sqlite3 import subprocess @@ -148,6 +149,61 @@ def test_repair_discoverability_apply_backs_up_and_repairs_safe_findings(tmp_pat assert "state.db" in backed_up +def test_materialize_sidecar_fsync_failure_invalidates_absent_cached_owner( + tmp_path, + monkeypatch, +): + from api import models + + sid = "state-only-partially-published" + db = _state_db( + tmp_path, + [{"id": sid, "source": "webui", "message_count": 1}], + {sid: 1}, + ) + monkeypatch.setattr(models, "SESSION_DIR", tmp_path) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", tmp_path / "_index.json") + cached = models.Session(session_id=sid, workspace=str(tmp_path), messages=[]) + with models.LOCK: + models.SESSIONS.clear() + models.SESSIONS[sid] = cached + + real_fsync = models._fsync_sidecar_directory + attempts = 0 + + def fail_first_directory_fsync(directory): + nonlocal attempts + attempts += 1 + if attempts == 1: + raise OSError(errno.EIO, "directory fsync failed after link") + real_fsync(directory) + + monkeypatch.setattr( + models, + "_fsync_sidecar_directory", + fail_first_directory_fsync, + ) + result = repair_session_discoverability( + tmp_path, + state_db_path=db, + dry_run=False, + backup_dir=tmp_path / "backup", + ) + + target = tmp_path / f"{sid}.json" + assert result["ok"] is False + assert target.exists() + assert json.loads(target.read_text(encoding="utf-8"))["_sidecar_generation_v1"] == 1 + with models.LOCK: + assert sid not in models.SESSIONS + + reloaded = models.Session.load(sid) + assert reloaded is not None + reloaded.messages.append({"role": "assistant", "content": "retry works"}) + reloaded.save(skip_index=True) + assert json.loads(target.read_text(encoding="utf-8"))["_sidecar_generation_v1"] == 2 + + def test_clear_sidecar_cli_flag_never_silently_overwrites_successful_save( tmp_path, monkeypatch,