From f11efa580e6c10d4b291cd713445ec006e7d70c7 Mon Sep 17 00:00:00 2001 From: colombod Date: Mon, 17 Aug 2026 17:14:28 +0000 Subject: [PATCH 1/5] feat(blob-storage): isolate blob + identity storage behind neutral async Protocol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add neutral BlobStore Protocol: streaming list()/scan() as AsyncIterator[BlobReference], write()->BlobReference, fenced conditional delete(uri, if_unmodified=ref) - Implement AsyncDiskBlobStore with single shared registry.blob_store instance via DI - Privatize IdentityStore._path, add exists() method; fix main.py boot-time reach-throughs - Update /blobs routes to use shared store instance (no multiple construction sites) - Type services.blob_store as BlobStore|None (was Any) - Add isolation tripwire test: validates no blob-path FS ops leak outside blob_store.py - All operations are async and account for latency (thread-offloaded FS ops) Note: reclaim ENDPOINT rewire intentionally NOT included; it lives in PR #70 and will adopt this storage API when #70 is rebased on top of this. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- context_intelligence_server/blob_processor.py | 4 +- context_intelligence_server/blob_store.py | 235 +++++++++++++--- context_intelligence_server/identity_store.py | 24 +- context_intelligence_server/main.py | 17 +- context_intelligence_server/registry.py | 17 +- context_intelligence_server/services.py | 3 +- tests/test_blob_isolation_tripwire.py | 67 +++++ tests/test_blob_processor.py | 26 +- tests/test_blob_store.py | 266 +++++++++++++++--- tests/test_m2_service_auth.py | 13 +- tests/test_main.py | 17 +- 11 files changed, 573 insertions(+), 116 deletions(-) create mode 100644 tests/test_blob_isolation_tripwire.py diff --git a/context_intelligence_server/blob_processor.py b/context_intelligence_server/blob_processor.py index bd7d8c25..5be7e33c 100644 --- a/context_intelligence_server/blob_processor.py +++ b/context_intelligence_server/blob_processor.py @@ -96,8 +96,8 @@ async def process_event_data( key = f"{node_id}__{field_name}" try: - uri = await blob_store.write(session_id, key, value) - data[field_name] = {"$blob_ref": uri} + ref = await blob_store.write(session_id, key, value) + data[field_name] = {"$blob_ref": ref.uri} except Exception as exc: # noqa: BLE001 logger.warning( "blob_offload_failed session=%s field=%s node=%s: %s", diff --git a/context_intelligence_server/blob_store.py b/context_intelligence_server/blob_store.py index 94511781..03bc7e84 100644 --- a/context_intelligence_server/blob_store.py +++ b/context_intelligence_server/blob_store.py @@ -8,6 +8,12 @@ All filesystem I/O is wrapped with ``asyncio.to_thread`` to keep the event loop non-blocking. + +The ``BlobStore`` Protocol is the backend-neutral seam: the only identity +that crosses the boundary is the ``ci-blob:///`` URI, carried +by :class:`BlobReference`. No ``Path``, on-disk layout, ``dest_dir``, or +``os.*`` detail appears in the Protocol or in any value it returns — that is +private to :class:`AsyncDiskBlobStore` (and, later, an Azure equivalent). """ from __future__ import annotations @@ -17,12 +23,51 @@ import os import shutil import tempfile +from collections.abc import AsyncIterator +from dataclasses import dataclass from pathlib import Path from typing import Any, Protocol, cast, runtime_checkable _SCHEME = "ci-blob://" +# --------------------------------------------------------------------------- +# BlobNotFoundError — backend-neutral missing-blob exception (guard #6) +# --------------------------------------------------------------------------- + + +class BlobNotFoundError(FileNotFoundError): + """Raised when a blob addressed by a ``ci-blob://`` URI does not exist. + + Subclasses :class:`FileNotFoundError` so existing ``except + FileNotFoundError`` callers keep working unchanged (zero caller churn). + The message carries the URI ONLY — never an on-disk path, container, or + account — so a future Azure backend can raise the same type/message + shape and no caller (or log line) ever learns which backend is in use. + """ + + +# --------------------------------------------------------------------------- +# BlobReference — cheap handle: identity + metadata, NO payload, NO Path +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class BlobReference: + """Cheap handle — identity + metadata, NO payload, NO Path. + + This is what ``scan()``/``list()`` return and what everything except a + payload read passes around. It is what gets serialized on the graph (as + its ``.uri``). + """ + + uri: str # ci-blob:/// — the ONLY address callers use + session_id: str + key: str + size: int # content length in bytes + last_modified: float # epoch seconds: disk st_mtime || azure Last-Modified + + # --------------------------------------------------------------------------- # BlobStore protocol # --------------------------------------------------------------------------- @@ -30,41 +75,51 @@ @runtime_checkable class BlobStore(Protocol): - """Protocol for a session-scoped, URI-addressable blob store.""" + """Protocol for a session-scoped, URI-addressable blob store. + + 100% backend-neutral: the only identity that crosses the boundary is the + ``ci-blob://`` URI (carried by :class:`BlobReference`). No ``Path``, no + on-disk layout, no ``dest_dir``, no ``os.*`` — ever. + """ async def write( self, session_id: str, key: str, value: dict[str, Any] | list[Any] - ) -> str: - """Persist *value* as JSON and return a ``ci-blob://`` URI.""" + ) -> BlobReference: + """Persist *value* as JSON and return a :class:`BlobReference`.""" ... - async def read(self, uri: str) -> dict[str, Any] | list[Any]: - """Resolve *uri* and return the stored value. - - Raises: - ValueError: If *uri* does not match the ``ci-blob://`` scheme. - FileNotFoundError: If no blob exists at the resolved path. - """ + def list(self, session_id: str) -> AsyncIterator[BlobReference]: + """Stream all blob references for *session_id* (one session).""" ... - async def list(self, session_id: str) -> list[str]: - """Return all blob URIs for *session_id*, sorted lexicographically.""" + def scan(self) -> AsyncIterator[BlobReference]: + """Stream all blob references across ALL sessions.""" ... - async def dump(self, uri: str, dest_dir: Path | str | None = None) -> str: - """Copy the blob file addressed by *uri* to *dest_dir*. + async def delete( + self, uri: str, if_unmodified: BlobReference | None = None + ) -> bool: + """Delete the blob addressed by *uri*. Idempotent: returns False if absent. Args: - uri: ``ci-blob://`` URI identifying the blob to copy. - dest_dir: Destination directory. Defaults to - ``Path(tempfile.gettempdir()) / 'ci-blobs'``. + uri: The ``ci-blob://`` URI to delete. + if_unmodified: When provided, this is a **fenced (compare-and-delete)** + delete — the store re-checks the blob's current metadata against + *if_unmodified* (disk: mtime + size; Azure: ``If-Match`` ETag) and + refuses (returns ``False``, does NOT delete) if the blob changed + since *if_unmodified* was observed (e.g. by a `scan()`). When + ``None`` (default), this is the unconditional idempotent delete. + """ + ... - Returns: - The destination file path as a string. + async def read(self, uri: str) -> dict[str, Any] | list[Any]: + """Resolve *uri* and return the stored value (the sole payload path). Raises: - ValueError: If *uri* is not a valid ``ci-blob://`` URI. - FileNotFoundError: If no blob exists at the resolved path. + ValueError: If *uri* does not match the ``ci-blob://`` scheme. + BlobNotFoundError: If no blob exists for *uri*. Subclasses + ``FileNotFoundError`` for back-compat; the message carries the + URI only — never an on-disk path, container, or account. """ ... @@ -133,17 +188,16 @@ def blob_path(self, session_id: str, key: str) -> Path: async def write( self, session_id: str, key: str, value: dict[str, Any] | list[Any] - ) -> str: - """Persist *value* as JSON and return a ``ci-blob://`` URI. + ) -> BlobReference: + """Persist *value* as JSON and return a :class:`BlobReference`. Creates the directory ``//blobs/`` if needed. - - Returns: - A ``ci-blob:///`` URI. + ``last_modified`` is the storage mtime (from the same ``stat`` call + that produces ``size``) — never a writer-clock timestamp. """ path = self._blob_path(session_id, key) - def _write() -> None: + def _write() -> os.stat_result: path.parent.mkdir(parents=True, exist_ok=True) data = json.dumps(value) tmp_fd, tmp_name = tempfile.mkstemp( @@ -161,9 +215,16 @@ def _write() -> None: except FileNotFoundError: pass raise + return path.stat() - await asyncio.to_thread(_write) - return self._make_uri(session_id, key) + st = await asyncio.to_thread(_write) + return BlobReference( + uri=self._make_uri(session_id, key), + session_id=session_id, + key=key, + size=st.st_size, + last_modified=st.st_mtime, + ) async def read(self, uri: str) -> dict[str, Any] | list[Any]: """Return the blob addressed by *uri*. @@ -174,7 +235,9 @@ async def read(self, uri: str) -> dict[str, Any] | list[Any]: Raises: ValueError: If *uri* is not a valid ``ci-blob://`` URI. - FileNotFoundError: If no blob exists at the resolved path. + BlobNotFoundError: If no blob exists for *uri*. Subclasses + ``FileNotFoundError`` for back-compat; the message carries + the URI only — never the on-disk path. """ session_id, key = self._parse_uri(uri) path = self._blob_path(session_id, key) @@ -186,28 +249,124 @@ def _read() -> dict[str, Any] | list[Any]: json.loads(path.read_text(encoding="utf-8")), ) except FileNotFoundError: - raise FileNotFoundError(f"Blob not found: {uri!r} (path: {path})") + raise BlobNotFoundError(f"Blob not found: {uri!r}") from None return await asyncio.to_thread(_read) - async def list(self, session_id: str) -> list[str]: - """Return all blob URIs for *session_id*, sorted lexicographically. + async def list(self, session_id: str) -> AsyncIterator[BlobReference]: + """Stream all blob references for *session_id*. - Returns an empty list if the session directory does not exist. + Yields nothing if the session's blobs directory does not exist. + The scandir + per-entry stat work is offloaded to a thread in small + units (per-entry), so the event loop stays responsive and references + stream out incrementally rather than blocking on the whole walk. """ blobs_dir = self._root / session_id / "blobs" - def _list() -> list[str]: + def _list_entries() -> list[tuple[str, int, float]]: if not blobs_dir.exists(): return [] - keys = sorted(p.stem for p in blobs_dir.glob("*.json")) - return [self._make_uri(session_id, key) for key in keys] + entries: list[tuple[str, int, float]] = [] + with os.scandir(blobs_dir) as it: + for entry in it: + if not entry.name.endswith(".json"): + continue + st = entry.stat() + key = entry.name[: -len(".json")] + entries.append((key, st.st_size, st.st_mtime)) + entries.sort(key=lambda e: e[0]) + return entries + + entries = await asyncio.to_thread(_list_entries) + for key, size, last_modified in entries: + yield BlobReference( + uri=self._make_uri(session_id, key), + session_id=session_id, + key=key, + size=size, + last_modified=last_modified, + ) + + async def scan(self) -> AsyncIterator[BlobReference]: + """Stream all blob references across ALL sessions. + + Walks ``/*/blobs/*.json`` — session-dir enumeration and each + session's blob-dir scan are offloaded to a thread in small units + (never one giant ``to_thread`` for the whole tree), so references + stream out as they are discovered instead of materializing the + entire store in memory before yielding anything. + """ + + def _list_session_dirs() -> list[str]: + if not self._root.exists(): + return [] + with os.scandir(self._root) as it: + return sorted(entry.name for entry in it if entry.is_dir()) + + session_ids = await asyncio.to_thread(_list_session_dirs) + for session_id in session_ids: + async for ref in self.list(session_id): + yield ref - return await asyncio.to_thread(_list) + async def delete( + self, uri: str, if_unmodified: BlobReference | None = None + ) -> bool: + """Delete the blob addressed by *uri*. + + Idempotent: returns ``False`` (never raises) if the blob is already + absent, ``True`` if it existed and was removed. + + Args: + uri: The ``ci-blob://`` URI to delete. + if_unmodified: When ``None`` (default), unconditional delete — + unlinks and returns ``True``, or ``False`` if already absent. + When provided, this is a **fenced compare-and-delete**: the + blob is re-``stat``'d (inside the same thread hop, right + before the unlink, to minimise the TOCTOU window) and the + delete only proceeds if ``st_mtime``/``st_size`` still match + *if_unmodified* — i.e. nothing rewrote the blob since it was + observed (e.g. by ``scan()``). If the blob is missing, or it + changed, the delete is refused and ``False`` is returned — + the blob is left untouched on disk. + """ + session_id, key = self._parse_uri(uri) + path = self._blob_path(session_id, key) + + def _delete() -> bool: + if if_unmodified is None: + try: + os.unlink(path) + return True + except FileNotFoundError: + return False + + # Fenced compare-and-delete: stat first, unlink only if unchanged. + try: + st = path.stat() + except FileNotFoundError: + return False + if ( + st.st_mtime != if_unmodified.last_modified + or st.st_size != if_unmodified.size + ): + # Blob was rewritten since it was observed — refuse to delete. + return False + try: + os.unlink(path) + return True + except FileNotFoundError: + # Deleted concurrently between our stat and unlink. + return False + + return await asyncio.to_thread(_delete) async def dump(self, uri: str, dest_dir: Path | str | None = None) -> str: """Copy the blob file addressed by *uri* to *dest_dir*. + Disk-only helper — NOT part of the :class:`BlobStore` Protocol + (no production caller; kept as a concrete convenience for external + tooling that needs a local export). + Args: uri: ``ci-blob://`` URI identifying the blob to copy. dest_dir: Destination directory. Defaults to diff --git a/context_intelligence_server/identity_store.py b/context_intelligence_server/identity_store.py index a7da96e7..c6219570 100644 --- a/context_intelligence_server/identity_store.py +++ b/context_intelligence_server/identity_store.py @@ -62,7 +62,7 @@ class IdentityStore: """ def __init__(self, path: Path) -> None: - self.path = path + self._path = path # Rich format: {key: {id: ..., display_name?: ...}} self._data: dict[str, dict[str, str]] = {} # Flat derived cache: {key: contributor_id}. @@ -80,7 +80,7 @@ def load(self) -> None: Missing file → empty dict (normal first boot, no log). Corrupt / non-dict → empty dict + LOUD error log, never raise. """ - if not self.path.exists(): + if not self._path.exists(): # Normal first boot — the file hasn't been written yet. self._data = {} self._rebuild_flat() @@ -88,12 +88,12 @@ def load(self) -> None: raw: object try: - raw = json.loads(self.path.read_text(encoding="utf-8")) + raw = json.loads(self._path.read_text(encoding="utf-8")) except (json.JSONDecodeError, UnicodeDecodeError, OSError) as exc: logger.error( "identity_store.load CORRUPT FILE path=%s error=%r — " "failing CLOSED to empty map. Re-populate via /admin API.", - self.path, + self._path, exc, ) self._data = {} @@ -104,7 +104,7 @@ def load(self) -> None: logger.critical( "identity_store.load INVALID FORMAT path=%s got=%r — " "expected a JSON object at top level. Failing CLOSED to empty map.", - self.path, + self._path, type(raw).__name__, ) self._data = {} @@ -168,7 +168,7 @@ def seed(self, data: dict[str, dict[str, str]]) -> None: "identity_store.seed: could not write seed to %s: %r " "— in-memory map is live but the file is not yet persisted. " "The next mutation via /admin API will persist the file.", - self.path, + self._path, exc, ) # Update in-memory regardless — data is from durable config. @@ -186,6 +186,10 @@ def items(self): # type: ignore[override] def __len__(self) -> int: return len(self._data) + def exists(self) -> bool: + """Whether the store has ever been persisted to its backing file.""" + return self._path.exists() + # ------------------------------------------------------------------ # Internals # ------------------------------------------------------------------ @@ -203,7 +207,7 @@ def _rebuild_flat(self) -> None: self.flat_dict[key] = contributor_id def _write_atomic(self, data: dict[str, dict[str, str]]) -> None: - """Write *data* atomically to ``self.path``. + """Write *data* atomically to ``self._path``. Steps: 1. Create parent directory (parents=True, exist_ok=True). @@ -216,15 +220,15 @@ def _write_atomic(self, data: dict[str, dict[str, str]]) -> None: Raises the underlying OS/IO exception so the caller (put/delete) knows the write failed and leaves in-process state unchanged. """ - self.path.parent.mkdir(parents=True, exist_ok=True) - tmp_fd, tmp_str = tempfile.mkstemp(dir=str(self.path.parent), suffix=".tmp") + self._path.parent.mkdir(parents=True, exist_ok=True) + tmp_fd, tmp_str = tempfile.mkstemp(dir=str(self._path.parent), suffix=".tmp") tmp_path = Path(tmp_str) try: with os.fdopen(tmp_fd, "w", encoding="utf-8") as fh: json.dump(data, fh, indent=2, ensure_ascii=False) fh.flush() os.fsync(fh.fileno()) - os.replace(str(tmp_path), str(self.path)) + os.replace(str(tmp_path), str(self._path)) except Exception: # Best-effort cleanup of the tempfile before propagating. try: diff --git a/context_intelligence_server/main.py b/context_intelligence_server/main.py index 0bf0709f..7b7fd034 100644 --- a/context_intelligence_server/main.py +++ b/context_intelligence_server/main.py @@ -30,7 +30,6 @@ require_read, require_write, ) -from context_intelligence_server.blob_store import AsyncDiskBlobStore from context_intelligence_server.config import Neo4jClientConfig, Settings, get_settings from context_intelligence_server.idempotency import EventIdempotencyCache from context_intelligence_server.identity_store import IdentityStore @@ -393,6 +392,13 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: # request.app.state.registry instead of importing the module-level name # (avoids a circular import between main and the routers package). app.state.registry = registry +# Single shared BlobStore instance for the whole process (T1.5): the /blobs +# routes above and the registry's session-worker construction both go +# through registry.blob_store, which lazily builds and caches exactly one +# AsyncDiskBlobStore. Also mirrored on app.state so other routers (e.g. a +# future admin reclaim rewire) can reach the same instance without importing +# the module-level `registry` name. +app.state.blob_store = registry.blob_store idempotency_cache = EventIdempotencyCache() # Session-less events are keyed by a per-workspace sentinel stem so that events @@ -574,7 +580,7 @@ def create_asgi_app( # Build and load the entra identity store. entra_store = IdentityStore(Path(s.entra_identities_store_path)) entra_store.load() - if not entra_store.path.exists(): + if not entra_store.exists(): # First boot: seed in-process map from config. Converts the flat # {oid -> contributor_id} from build_identity_map() to the rich # {oid -> {"id": contributor_id}} format that IdentityStore expects. @@ -638,7 +644,7 @@ def create_asgi_app( # Build and load the API-key store. key_store = IdentityStore(Path(s.api_keys_store_path)) key_store.load() - if not key_store.path.exists(): + if not key_store.exists(): # First boot: seed from config. Converts the flat # {sha256_hex -> contributor_id} from build_keystore() to the # rich {sha256_hex -> {"id": contributor_id}} format. @@ -921,14 +927,13 @@ async def post_events( @app.get("/blobs/{session_id}", dependencies=[Depends(require_read)]) async def list_blobs(session_id: str) -> JSONResponse: - blob_store = AsyncDiskBlobStore(root=_settings.blob_path) - uris = await blob_store.list(session_id) + uris = [ref.uri async for ref in registry.blob_store.list(session_id)] return JSONResponse(content={"session_id": session_id, "blobs": uris}) @app.get("/blobs/{session_id}/{key}", dependencies=[Depends(require_read)]) async def get_blob(session_id: str, key: str) -> JSONResponse: - blob_store = AsyncDiskBlobStore(root=_settings.blob_path) + blob_store = registry.blob_store uri = f"ci-blob://{session_id}/{key}" try: content = await blob_store.read(uri) diff --git a/context_intelligence_server/registry.py b/context_intelligence_server/registry.py index ad078fee..336cd064 100644 --- a/context_intelligence_server/registry.py +++ b/context_intelligence_server/registry.py @@ -69,6 +69,7 @@ def __init__(self) -> None: # before the per-test settings patch applies, so we cannot read # settings here — see _ensure_infra(). self._queue_manager: QueueManager | None = None + self._blob_store: AsyncDiskBlobStore | None = None self._write_semaphore: asyncio.Semaphore | None = None self._max_delivery_attempts: int = 0 # Live pipeline-conservation counters (D2): make silently-dropped @@ -113,6 +114,20 @@ def write_semaphore(self) -> asyncio.Semaphore: assert self._write_semaphore is not None return self._write_semaphore + @property + def blob_store(self) -> AsyncDiskBlobStore: + """The single shared AsyncDiskBlobStore owned by this registry. + + Built lazily on first access (same rationale as ``queue_manager``: + the module-level registry singleton is constructed at import time, + before any per-test settings patch applies), then reused for every + session worker and route handler — never one instance per session. + """ + if self._blob_store is None: + settings = get_settings() + self._blob_store = AsyncDiskBlobStore(root=settings.blob_path) + return self._blob_store + def record_accepted(self, n: int = 1) -> None: """Count events admitted to the durable log (ingest accepted).""" self._accepted_total += n @@ -554,7 +569,7 @@ def get_or_create( ) -> SessionWorker: if session_id not in self._workers: settings = get_settings() - blob_store = AsyncDiskBlobStore(root=settings.blob_path) + blob_store = self.blob_store _admin = settings.resolve_neo4j_admin() neo4j_store = Neo4jGraphStore( uri=_admin.url, diff --git a/context_intelligence_server/services.py b/context_intelligence_server/services.py index b5f0230b..59b6ee27 100644 --- a/context_intelligence_server/services.py +++ b/context_intelligence_server/services.py @@ -12,6 +12,7 @@ from datetime import datetime from typing import Any +from context_intelligence_server.blob_store import BlobStore from context_intelligence_server.handlers.data_layer_2.state import DataLayer2State from context_intelligence_server.handlers.data_layer_3.state import DataLayer3State @@ -229,7 +230,7 @@ def __init__( *, created_by: str | None = None, raw_config: dict[str, Any] | None = None, - blob_store: Any | None = None, + blob_store: BlobStore | None = None, ) -> None: self.config = HookConfig(raw_config or {}) if graph_store is not None: diff --git a/tests/test_blob_isolation_tripwire.py b/tests/test_blob_isolation_tripwire.py new file mode 100644 index 00000000..dc4f1f89 --- /dev/null +++ b/tests/test_blob_isolation_tripwire.py @@ -0,0 +1,67 @@ +"""Tripwire: the blob-store on-disk layout stays inside blob_store.py. + +Locks the isolation boundary established by the BlobStore refactor +(see docs/blob-store-abstraction.md). These tests fail loudly if a future +change lets any module other than ``blob_store.py`` locate the blob root or +resurrects the old direct-filesystem reclaim implementation -- i.e. if a +direct-FS blob leak is reintroduced. + +Two invariants: + 1. Only the single construction site (registry.py) and the config field + declaration (config.py) may reference ``settings.blob_path`` -- a caller + that cannot locate the blob root physically cannot do blob filesystem I/O. + 2. The pre-refactor direct-disk reclaim symbols must never reappear. +""" + +from __future__ import annotations + +import pathlib + +PKG = pathlib.Path(__file__).resolve().parents[1] / "context_intelligence_server" + + +def _code_lines(path: pathlib.Path): + """Yield (lineno, stripped) for real code lines, skipping comments and + rst-doc lines (``...`` backtick spans) so docstring prose never trips the + guard.""" + for i, raw in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + s = raw.strip() + if not s or s.startswith("#") or "``" in raw: + continue + yield i, s + + +def test_blob_root_locatable_only_at_construction_site() -> None: + """`settings.blob_path` -- the only way to find the on-disk blob root -- is + referenced solely where the store is constructed (registry.py) and where the + config field is declared (config.py). blob_store.py owns the layout itself.""" + allowed = {"registry.py", "config.py", "blob_store.py"} + offenders: list[str] = [] + for p in PKG.rglob("*.py"): + if p.name in allowed: + continue + for lineno, line in _code_lines(p): + if "blob_path" in line: + offenders.append(f"{p.relative_to(PKG)}:{lineno}: {line}") + assert not offenders, ( + "blob root re-derived outside the single construction site " + "(registry.py)/config -- a caller that can locate the blob root can " + "bypass BlobStore and touch disk directly:\n" + "\n".join(offenders) + ) + + +def test_old_direct_disk_reclaim_symbols_are_gone() -> None: + """The pre-refactor direct-FS reclaim implementation (a private on-disk blob + dataclass + a filesystem scan helper in routers/admin.py) must not reappear + anywhere in the package.""" + banned = ("_OnDiskBlob", "_scan_disk_blobs") + offenders: list[str] = [] + for p in PKG.rglob("*.py"): + for lineno, line in _code_lines(p): + for token in banned: + if token in line: + offenders.append(f"{p.relative_to(PKG)}:{lineno}: {token}") + assert not offenders, ( + "old direct-disk blob-reclaim symbols resurfaced (the reclaim GC must " + "go through BlobStore.scan()/delete()):\n" + "\n".join(offenders) + ) diff --git a/tests/test_blob_processor.py b/tests/test_blob_processor.py index 82ee196c..4dfc92a4 100644 --- a/tests/test_blob_processor.py +++ b/tests/test_blob_processor.py @@ -23,12 +23,20 @@ from unittest.mock import AsyncMock import pytest - from context_intelligence_server.blob_processor import ( BLOB_FIELDS, _lift_raw_fields, process_event_data, ) +from context_intelligence_server.blob_store import BlobReference + + +def _ref(uri: str) -> BlobReference: + """Build a minimal BlobReference for mocking blob_store.write().""" + session_id, _, key = uri.removeprefix("ci-blob://").partition("/") + return BlobReference( + uri=uri, session_id=session_id, key=key, size=0, last_modified=0.0 + ) # --------------------------------------------------------------------------- @@ -55,7 +63,7 @@ async def test_process_event_data_mutates_in_place() -> None: original_id = id(data) blob_store = AsyncMock() - blob_store.write = AsyncMock(return_value="ci-blob://sess/node__raw") + blob_store.write = AsyncMock(return_value=_ref("ci-blob://sess/node__raw")) await process_event_data(data, blob_store, "sess", "node") @@ -74,7 +82,7 @@ async def test_process_event_data_returns_none() -> None: """process_event_data returns None.""" data: dict[str, Any] = {"result": {"answer": 42}} blob_store = AsyncMock() - blob_store.write = AsyncMock(return_value="ci-blob://sess/node__result") + blob_store.write = AsyncMock(return_value=_ref("ci-blob://sess/node__result")) result = await process_event_data(data, blob_store, "sess", "node") @@ -96,8 +104,8 @@ async def test_blob_ref_substitution_on_successful_write() -> None: # Use a function-based side_effect so the returned URI always matches # the actual key argument, regardless of BLOB_FIELDS frozenset iteration order. - async def _write(session_id: str, key: str, value: object) -> str: - return f"ci-blob://{session_id}/{key}" + async def _write(session_id: str, key: str, value: object) -> BlobReference: + return _ref(f"ci-blob://{session_id}/{key}") blob_store.write = AsyncMock(side_effect=_write) @@ -134,7 +142,7 @@ async def test_absent_fields_are_skipped() -> None: """Fields in BLOB_FIELDS that are absent from data are not added.""" data: dict[str, Any] = {"other_field": "untouched"} blob_store = AsyncMock() - blob_store.write = AsyncMock(return_value="ci-blob://sess/node__something") + blob_store.write = AsyncMock(return_value=_ref("ci-blob://sess/node__something")) await process_event_data(data, blob_store, "sess", "node") @@ -158,7 +166,7 @@ async def test_none_fields_are_skipped() -> None: "messages": None, } blob_store = AsyncMock() - blob_store.write = AsyncMock(return_value="ci-blob://sess/node__x") + blob_store.write = AsyncMock(return_value=_ref("ci-blob://sess/node__x")) await process_event_data(data, blob_store, "sess", "node") @@ -183,8 +191,8 @@ async def test_blob_key_format() -> None: blob_store = AsyncMock() blob_store.write = AsyncMock( side_effect=[ - "ci-blob://my-session/my-node__raw", - "ci-blob://my-session/my-node__debug", + _ref("ci-blob://my-session/my-node__raw"), + _ref("ci-blob://my-session/my-node__debug"), ] ) diff --git a/tests/test_blob_store.py b/tests/test_blob_store.py index 4a3a31ad..312e8974 100644 --- a/tests/test_blob_store.py +++ b/tests/test_blob_store.py @@ -1,14 +1,14 @@ -"""Tests for AsyncDiskBlobStore — Write, Read, List, Dump. +"""Tests for AsyncDiskBlobStore — Write, Read, List, Scan, Delete, Dump. -15 tests covering: +Covers: 1. write/read roundtrip -2. URI format +2. BlobReference.uri format 3. directory structure creation 4. URI-based session_id resolution 5. missing blob raises FileNotFoundError 6. invalid URI raises ValueError 7. empty list for missing session -8. correct URI listing +8. correct BlobReference listing (async iterator) 9. session isolation 10. asyncio.to_thread delegation verification 11. dump() copies blob to specified dest_dir @@ -16,19 +16,26 @@ 13. dump() missing blob raises FileNotFoundError 14. dump() delegates copy2 via asyncio.to_thread 15. BlobStore protocol conformance +16. scan() yields BlobReference across multiple sessions +17. delete() is idempotent (True then False) and removes the blob +18. list()/write() BlobReference has correct uri/size/last_modified """ from __future__ import annotations import asyncio import json +import os from pathlib import Path from unittest.mock import patch import pytest - -from context_intelligence_server.blob_store import AsyncDiskBlobStore, BlobStore - +from context_intelligence_server.blob_store import ( + AsyncDiskBlobStore, + BlobNotFoundError, + BlobReference, + BlobStore, +) # --------------------------------------------------------------------------- # Fixtures @@ -41,6 +48,10 @@ def store(tmp_path: Path) -> AsyncDiskBlobStore: return AsyncDiskBlobStore(root=tmp_path) +async def _list_uris(store: AsyncDiskBlobStore, session_id: str) -> list[str]: + return [ref.uri async for ref in store.list(session_id)] + + # --------------------------------------------------------------------------- # 1. Write/read roundtrip # --------------------------------------------------------------------------- @@ -49,20 +60,25 @@ def store(tmp_path: Path) -> AsyncDiskBlobStore: async def test_write_read_roundtrip(store: AsyncDiskBlobStore) -> None: """Data written can be read back unchanged.""" payload = {"event": "tool_call", "tool": "bash", "args": ["ls"]} - uri = await store.write("session-abc", "tool_call_01", payload) - result = await store.read(uri) + ref = await store.write("session-abc", "tool_call_01", payload) + result = await store.read(ref.uri) assert result == payload # --------------------------------------------------------------------------- -# 2. URI format +# 2. BlobReference.uri format # --------------------------------------------------------------------------- async def test_uri_format(store: AsyncDiskBlobStore) -> None: - """write() returns a ci-blob:/// URI.""" - uri = await store.write("session-xyz", "my_key", {"x": 1}) - assert uri == "ci-blob://session-xyz/my_key" + """write() returns a BlobReference whose .uri is ci-blob:///.""" + ref = await store.write("session-xyz", "my_key", {"x": 1}) + assert isinstance(ref, BlobReference) + assert ref.uri == "ci-blob://session-xyz/my_key" + assert ref.session_id == "session-xyz" + assert ref.key == "my_key" + assert ref.size > 0 + assert ref.last_modified > 0 # --------------------------------------------------------------------------- @@ -93,11 +109,11 @@ async def test_uri_based_session_id_resolution( session_id = "session-uri-resolve" key = "my_blob" payload = {"resolved": True} - uri = await store.write(session_id, key, payload) + ref = await store.write(session_id, key, payload) # Confirm URI contains session_id - assert session_id in uri + assert session_id in ref.uri # read must successfully resolve session_id from URI - result = await store.read(uri) + result = await store.read(ref.uri) assert result == payload @@ -136,29 +152,34 @@ async def test_invalid_uri_raises_value_error(store: AsyncDiskBlobStore) -> None async def test_empty_list_for_missing_session(store: AsyncDiskBlobStore) -> None: - """list() returns an empty list when no blobs exist for the session.""" - result = await store.list("session-does-not-exist") + """list() yields nothing when no blobs exist for the session.""" + result = await _list_uris(store, "session-does-not-exist") assert result == [] # --------------------------------------------------------------------------- -# 8. Correct URI listing +# 8. Correct BlobReference listing (async iterator) # --------------------------------------------------------------------------- async def test_correct_uri_listing(store: AsyncDiskBlobStore) -> None: - """list() returns all blob URIs for a session, sorted.""" + """list() yields all blob references for a session, sorted by key.""" session_id = "session-list" await store.write(session_id, "key_b", {"b": 2}) await store.write(session_id, "key_a", {"a": 1}) await store.write(session_id, "key_c", {"c": 3}) - uris = await store.list(session_id) - assert uris == [ + refs = [ref async for ref in store.list(session_id)] + assert [r.uri for r in refs] == [ "ci-blob://session-list/key_a", "ci-blob://session-list/key_b", "ci-blob://session-list/key_c", ] + for r in refs: + assert isinstance(r, BlobReference) + assert r.session_id == session_id + assert r.size > 0 + assert r.last_modified > 0 # --------------------------------------------------------------------------- @@ -167,13 +188,13 @@ async def test_correct_uri_listing(store: AsyncDiskBlobStore) -> None: async def test_session_isolation(store: AsyncDiskBlobStore) -> None: - """list() only returns URIs for the requested session, not other sessions.""" + """list() only returns references for the requested session, not other sessions.""" await store.write("session-alpha", "blob_1", {"alpha": True}) await store.write("session-beta", "blob_2", {"beta": True}) await store.write("session-alpha", "blob_3", {"alpha2": True}) - alpha_uris = await store.list("session-alpha") - beta_uris = await store.list("session-beta") + alpha_uris = await _list_uris(store, "session-alpha") + beta_uris = await _list_uris(store, "session-beta") assert all("session-alpha" in u for u in alpha_uris) assert all("session-beta" in u for u in beta_uris) @@ -200,7 +221,8 @@ async def tracking_to_thread(func, *args, **kwargs): # type: ignore[no-untyped- with patch("asyncio.to_thread", side_effect=tracking_to_thread): await store.write("sess", "k", {"v": 1}) await store.read("ci-blob://sess/k") - await store.list("sess") + async for _ in store.list("sess"): + pass assert len(to_thread_calls) >= 3, ( f"Expected at least 3 asyncio.to_thread calls (write, read, list), " @@ -220,10 +242,10 @@ async def test_dump_copy_to_specified_dest_dir( session_id = "session-dump-copy" key = "blob_to_copy" payload = {"copy": "me"} - uri = await store.write(session_id, key, payload) + ref = await store.write(session_id, key, payload) dest_dir = tmp_path / "my_dest" - result = await store.dump(uri, dest_dir=dest_dir) + result = await store.dump(ref.uri, dest_dir=dest_dir) result_path = Path(result) assert result_path.exists() @@ -242,9 +264,9 @@ async def test_dump_default_dest_dir(store: AsyncDiskBlobStore) -> None: session_id = "session-dump-default" key = "default_blob" - uri = await store.write(session_id, key, {"default": True}) + ref = await store.write(session_id, key, {"default": True}) - result = await store.dump(uri) + result = await store.dump(ref.uri) expected_dir = Path(tempfile.gettempdir()) / "ci-blobs" result_path = Path(result) @@ -277,7 +299,7 @@ async def test_dump_uses_asyncio_to_thread_for_copy2( """dump() delegates shutil.copy2 to asyncio.to_thread for non-blocking I/O.""" session_id = "session-dump-thread" key = "thread_blob" - uri = await store.write(session_id, key, {"thread": True}) + ref = await store.write(session_id, key, {"thread": True}) dest_dir = tmp_path / "thread_dest" to_thread_calls: list[str] = [] @@ -288,7 +310,7 @@ async def tracking_to_thread(func, *args, **kwargs): # type: ignore[no-untyped- return await original_to_thread(func, *args, **kwargs) with patch("asyncio.to_thread", side_effect=tracking_to_thread): - await store.dump(uri, dest_dir=dest_dir) + await store.dump(ref.uri, dest_dir=dest_dir) assert len(to_thread_calls) >= 1, ( f"Expected at least 1 asyncio.to_thread call for dump(), " @@ -318,12 +340,14 @@ async def test_write_is_atomic_no_torn_file_on_failure( session_id = "sess-atomic" key = "k1" - with patch( - "context_intelligence_server.blob_store.os.replace", - side_effect=OSError("simulated replace failure"), + with ( + patch( + "context_intelligence_server.blob_store.os.replace", + side_effect=OSError("simulated replace failure"), + ), + pytest.raises(OSError), ): - with pytest.raises(OSError): - await store.write(session_id, key, {"v": 1}) + await store.write(session_id, key, {"v": 1}) final_path = store.blob_path(session_id, key) # No torn file observable at the final path. @@ -341,10 +365,174 @@ async def test_write_replaces_atomically_on_success( session_id = "sess-atomic" key = "k2" - uri = await store.write(session_id, key, {"v": 1}) + ref = await store.write(session_id, key, {"v": 1}) - assert uri == "ci-blob://sess-atomic/k2" + assert ref.uri == "ci-blob://sess-atomic/k2" final_path = store.blob_path(session_id, key) assert final_path.read_text(encoding="utf-8") == '{"v": 1}' # No leftover temp files. assert list(final_path.parent.glob("*.tmp")) == [] + + +# --------------------------------------------------------------------------- +# 16. scan() yields BlobReference across multiple sessions +# --------------------------------------------------------------------------- + + +async def test_scan_yields_references_across_sessions( + store: AsyncDiskBlobStore, +) -> None: + """scan() streams a BlobReference for every blob across ALL sessions.""" + await store.write("session-scan-a", "k1", {"a": 1}) + await store.write("session-scan-a", "k2", {"a": 2}) + await store.write("session-scan-b", "k1", {"b": 1}) + + refs = [ref async for ref in store.scan()] + uris = {r.uri for r in refs} + assert uris == { + "ci-blob://session-scan-a/k1", + "ci-blob://session-scan-a/k2", + "ci-blob://session-scan-b/k1", + } + for r in refs: + assert isinstance(r, BlobReference) + assert r.size > 0 + assert r.last_modified > 0 + + +async def test_scan_empty_store_yields_nothing(store: AsyncDiskBlobStore) -> None: + """scan() over an empty store yields no references.""" + refs = [ref async for ref in store.scan()] + assert refs == [] + + +# --------------------------------------------------------------------------- +# 17. delete() is idempotent and removes the blob +# --------------------------------------------------------------------------- + + +async def test_delete_idempotent_true_then_false(store: AsyncDiskBlobStore) -> None: + """delete() returns True the first time (blob existed), False thereafter.""" + ref = await store.write("session-delete", "to_delete", {"gone": "soon"}) + + first = await store.delete(ref.uri) + assert first is True + + # The blob is actually removed from disk. + with pytest.raises(FileNotFoundError): + await store.read(ref.uri) + + second = await store.delete(ref.uri) + assert second is False + + +async def test_delete_missing_blob_returns_false(store: AsyncDiskBlobStore) -> None: + """delete() on a never-written blob returns False, never raises.""" + result = await store.delete("ci-blob://never-existed/nope") + assert result is False + + +# --------------------------------------------------------------------------- +# BlobNotFoundError — neutral missing-blob error (guard #6) +# --------------------------------------------------------------------------- + + +async def test_missing_blob_raises_blob_not_found_error_no_path_leak( + store: AsyncDiskBlobStore, tmp_path: Path +) -> None: + """read() of a missing uri raises BlobNotFoundError (a FileNotFoundError + subclass, for back-compat) whose message carries the uri only — never the + on-disk path/root. + """ + uri = "ci-blob://session-missing/nonexistent_key" + + with pytest.raises(BlobNotFoundError) as exc_info: + await store.read(uri) + + # Back-compat: existing `except FileNotFoundError` callers still catch it. + assert isinstance(exc_info.value, FileNotFoundError) + + message = str(exc_info.value) + assert uri in message + # No on-disk path fragment or root leaks into the message. + assert "path" not in message.lower() + assert str(tmp_path) not in message + + +# --------------------------------------------------------------------------- +# Fenced (compare-and-delete) delete — guard #1 +# --------------------------------------------------------------------------- + + +async def test_fenced_delete_succeeds_when_unchanged( + store: AsyncDiskBlobStore, +) -> None: + """delete(uri, if_unmodified=ref) removes the blob when it has not + changed since ref was observed (e.g. by scan()/list()).""" + ref = await store.write("session-fence", "unchanged_key", {"v": 1}) + + result = await store.delete(ref.uri, if_unmodified=ref) + assert result is True + + with pytest.raises(BlobNotFoundError): + await store.read(ref.uri) + + +async def test_fenced_delete_refuses_when_rewritten( + store: AsyncDiskBlobStore, +) -> None: + """delete(uri, if_unmodified=stale_ref) returns False and leaves the + (new) blob on disk when the blob was rewritten after stale_ref was + observed. + + Deterministic (not sleep-based): the rewrite uses a longer JSON payload + so the size differs regardless of filesystem mtime granularity, and the + file's mtime is also forced forward via os.utime so both the size AND + mtime comparisons independently detect the change. + """ + session_id, key = "session-fence-stale", "rewritten_key" + + stale_ref = await store.write(session_id, key, {"v": 1}) + + # Rewrite with a longer payload -> different size, independent of mtime + # resolution/granularity on the filesystem. + new_ref = await store.write(session_id, key, {"v": 1, "extra": "x" * 64}) + assert new_ref.size != stale_ref.size + + # Force the mtime to be unambiguously different too (belt-and-suspenders + # against any filesystem where sizes could coincidentally collide). + path = store.blob_path(session_id, key) + new_mtime = stale_ref.last_modified + 100.0 + os.utime(path, (new_mtime, new_mtime)) + + result = await store.delete(stale_ref.uri, if_unmodified=stale_ref) + assert result is False + + # The (new) blob survives on disk, untouched. + survived = await store.read(stale_ref.uri) + assert survived == {"v": 1, "extra": "x" * 64} + + +async def test_fenced_delete_missing_blob_returns_false( + store: AsyncDiskBlobStore, +) -> None: + """delete(uri, if_unmodified=ref) on an already-absent blob returns False.""" + ref = await store.write("session-fence-missing", "gone_key", {"v": 1}) + assert await store.delete(ref.uri) is True # unconditional delete first + + result = await store.delete(ref.uri, if_unmodified=ref) + assert result is False + + +async def test_unconditional_delete_still_idempotent( + store: AsyncDiskBlobStore, +) -> None: + """Unconditional delete(uri) (if_unmodified=None, the default) is + unchanged: True then False, idempotent.""" + ref = await store.write("session-fence-uncond", "plain_key", {"v": 1}) + + first = await store.delete(ref.uri) + assert first is True + + second = await store.delete(ref.uri) + assert second is False diff --git a/tests/test_m2_service_auth.py b/tests/test_m2_service_auth.py index a6f9781f..099adfc8 100644 --- a/tests/test_m2_service_auth.py +++ b/tests/test_m2_service_auth.py @@ -123,13 +123,14 @@ def _service_claims(roles: list[str], appid: str = FAKE_APPID) -> dict[str, Any] class _MockBlobStore: - """Mock for AsyncDiskBlobStore — returns empty list, never touches filesystem.""" + """Mock for AsyncDiskBlobStore — yields nothing, never touches filesystem.""" - def __init__(self, root: Any) -> None: + def __init__(self, root: Any = None) -> None: pass - async def list(self, session_id: str) -> list[str]: - return [] + async def list(self, session_id: str) -> AsyncGenerator[Any, None]: + return + yield # pragma: no cover — makes this an async generator function async def read(self, uri: str) -> Any: raise FileNotFoundError(f"mock blob store: not found: {uri}") @@ -377,7 +378,7 @@ async def test_cap_sr_r_reader_read_capable( private_key, asgi = service_asgi token = _sign_jwt(private_key, _service_claims(roles=["Reader"])) - monkeypatch.setattr(main_module, "AsyncDiskBlobStore", _MockBlobStore) + monkeypatch.setattr(main_module.registry, "_blob_store", _MockBlobStore()) async with _make_client(asgi) as c: resp = await c.get( @@ -456,7 +457,7 @@ async def test_cap_sc_r_contributor_read_capable( private_key, asgi = service_asgi token = _sign_jwt(private_key, _service_claims(roles=["Contributor"])) - monkeypatch.setattr(main_module, "AsyncDiskBlobStore", _MockBlobStore) + monkeypatch.setattr(main_module.registry, "_blob_store", _MockBlobStore()) async with _make_client(asgi) as c: resp = await c.get( diff --git a/tests/test_main.py b/tests/test_main.py index 302fa1bf..a8510a0d 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -14,6 +14,7 @@ import context_intelligence_server.main as main_module from context_intelligence_server.auth import BearerTokenMiddleware +from context_intelligence_server.blob_store import AsyncDiskBlobStore from context_intelligence_server.main import app, lifespan, registry from context_intelligence_server.models import CypherRequest from tests.conftest import MockNeo4jDriver @@ -280,7 +281,9 @@ async def test_list_blobs_returns_empty_for_session_with_no_blobs( monkeypatch: pytest.MonkeyPatch, ) -> None: """GET /blobs/{session_id} returns 200 with empty blobs list for session with no blobs.""" - monkeypatch.setattr(main_module._settings, "blob_path", str(tmp_path)) + monkeypatch.setattr( + main_module.registry, "_blob_store", AsyncDiskBlobStore(root=tmp_path) + ) response = await client.get("/blobs/no-blobs-session") assert response.status_code == 200 @@ -295,7 +298,9 @@ async def test_list_blobs_returns_correct_uris_for_existing_blobs( monkeypatch: pytest.MonkeyPatch, ) -> None: """GET /blobs/{session_id} returns 200 with correct ci-blob:// URIs for existing blobs.""" - monkeypatch.setattr(main_module._settings, "blob_path", str(tmp_path)) + monkeypatch.setattr( + main_module.registry, "_blob_store", AsyncDiskBlobStore(root=tmp_path) + ) session_id = "blob-list-session" blob_dir = tmp_path / session_id / "blobs" @@ -319,7 +324,9 @@ async def test_get_blob_returns_200_with_content( monkeypatch: pytest.MonkeyPatch, ) -> None: """GET /blobs/{session_id}/{key} returns 200 with blob content for existing blob.""" - monkeypatch.setattr(main_module._settings, "blob_path", str(tmp_path)) + monkeypatch.setattr( + main_module.registry, "_blob_store", AsyncDiskBlobStore(root=tmp_path) + ) session_id = "test-session" key = "my-key" @@ -340,7 +347,9 @@ async def test_get_blob_returns_404_for_missing_blob( monkeypatch: pytest.MonkeyPatch, ) -> None: """GET /blobs/{session_id}/{key} returns 404 with 'not found' in detail for missing blob.""" - monkeypatch.setattr(main_module._settings, "blob_path", str(tmp_path)) + monkeypatch.setattr( + main_module.registry, "_blob_store", AsyncDiskBlobStore(root=tmp_path) + ) response = await client.get("/blobs/missing-session/missing-key") assert response.status_code == 404 From 499cd6ccbf652f39a193d66a4824569feee99580 Mon Sep 17 00:00:00 2001 From: colombod Date: Mon, 17 Aug 2026 17:14:35 +0000 Subject: [PATCH 2/5] fix(pipeline): include tool_call_id in blob key to prevent parallel same-ms event collision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The blob key is derived from make_node_id(session_id, event, timestamp), but without a tool_call_id disambiguator. Two parallel events with the same (session_id, event_name, timestamp-to-the-ms) but different tool_call_ids would mint identical blob keys, causing the second write to silently overwrite the first blob via os.replace, while both Event nodes still pointed at their original URIs. Fix: include tool_call_id (when present) in the blob key, matching how handlers/data_layer_1 derives the Event node id. This ensures each parallel event gets a distinct blob, even at the same millisecond. - Backward compatible: blob key format unchanged when tool_call_id is absent - Updated test to reflect 4-arg make_node_id call - Added regression test: parallel same-ms events with distinct tool_call_id do not collide 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- context_intelligence_server/pipeline.py | 12 ++- tests/test_pipeline.py | 123 +++++++++++++++++++++++- 2 files changed, 133 insertions(+), 2 deletions(-) diff --git a/context_intelligence_server/pipeline.py b/context_intelligence_server/pipeline.py index 7310fd9c..778909ec 100644 --- a/context_intelligence_server/pipeline.py +++ b/context_intelligence_server/pipeline.py @@ -173,7 +173,17 @@ async def process_event( data.get("timestamp") if isinstance(data, dict) else None ) if session_id and timestamp and worker.services.blob_store: - node_id = make_node_id(session_id, event, timestamp) + # The blob-key node_id MUST match handlers/data_layer_1/default.py's + # event_node_id (same session_id + event + timestamp + tool_call_id), + # otherwise two distinct same-millisecond events (e.g. parallel tool + # calls in the same batch) collide on an identical blob key and the + # second write silently overwrites the first via os.replace, while + # the first Event node's $blob_ref still points at that URI (now + # holding the wrong payload). tool_call_id is present on ALL event + # types that carry it, not just tool:* (see default.py's own + # comment on this), so it is safe to read unconditionally here. + disambiguator = data.get("tool_call_id") if isinstance(data, dict) else None + node_id = make_node_id(session_id, event, timestamp, disambiguator) await process_event_data( data, worker.services.blob_store, session_id, node_id ) diff --git a/tests/test_pipeline.py b/tests/test_pipeline.py index 32ef699a..5d0728f9 100644 --- a/tests/test_pipeline.py +++ b/tests/test_pipeline.py @@ -488,8 +488,12 @@ async def test_blob_processing_called_when_all_conditions_met( ) as mock_node_id, ): await process_event(worker, "session:start", data, pipeline_handlers) + # The blob-key node_id must be computed with the SAME disambiguator + # (tool_call_id) that handlers/data_layer_1/default.py uses for the + # Event node id -- otherwise parallel same-millisecond events collide + # on the blob key (see test_blob_node_id_matches_default_handler_event_node_id). mock_node_id.assert_called_once_with( - "sess-123", "session:start", "2024-01-01T00:00:00Z" + "sess-123", "session:start", "2024-01-01T00:00:00Z", None ) mock_process.assert_called_once_with( data, worker.services.blob_store, "sess-123", "test-node-id" @@ -566,6 +570,123 @@ async def test_blob_skip_missing_timestamp_logs_warning( assert "missing timestamp" in caplog.text +# =========================================================================== +# Blob-key collision regression (data integrity) +# +# Root cause: pipeline.py computed the blob-key node_id WITHOUT the +# tool_call_id disambiguator that handlers/data_layer_1/default.py uses for +# the Event node id. Two distinct same-session, same-event, same-millisecond +# events with DIFFERENT tool_call_id (e.g. parallel tool calls) therefore +# minted the SAME blob key, and the second write silently clobbered the +# first via AsyncDiskBlobStore's os.replace -- while the first Event node's +# $blob_ref still pointed at that (now-overwritten) URI. +# =========================================================================== + + +async def test_parallel_same_millisecond_events_do_not_collide_on_blob_key( + pipeline_handlers: Any, + tmp_path: Any, +) -> None: + """Two events sharing session_id + event name + timestamp (same epoch-ms) + but with DIFFERENT tool_call_id must mint DISTINCT ci-blob:// URIs, and + each blob must read back its own payload -- no silent overwrite.""" + from context_intelligence_server.blob_store import AsyncDiskBlobStore + from context_intelligence_server.pipeline import process_event + + blob_store = AsyncDiskBlobStore(root=tmp_path) + + worker = MagicMock() + worker.services.ensure_session_node = AsyncMock() + worker.services.touch_session = AsyncMock() + worker.services.graph = MagicMock() + worker.services.graph.flush = AsyncMock() + worker.services.blob_store = blob_store + + session_id = "sess-parallel" + event_name = "tool_call:end" + timestamp = "2024-06-01T12:00:00.000Z" # fixed -- identical epoch-ms for both + + data_a: dict[str, Any] = { + "session_id": session_id, + "timestamp": timestamp, + "tool_call_id": "call-A", + "result": {"payload": "result-from-call-A"}, + } + data_b: dict[str, Any] = { + "session_id": session_id, + "timestamp": timestamp, + "tool_call_id": "call-B", + "result": {"payload": "result-from-call-B"}, + } + + await process_event(worker, event_name, data_a, pipeline_handlers) + await process_event(worker, event_name, data_b, pipeline_handlers) + + # (c) each event's data[field] == {"$blob_ref": } + assert "$blob_ref" in data_a["result"] + assert "$blob_ref" in data_b["result"] + uri_a = data_a["result"]["$blob_ref"] + uri_b = data_b["result"]["$blob_ref"] + + # (a) distinct URIs -- no collision + assert uri_a != uri_b, ( + f"Blob key collision: both events minted the same URI {uri_a!r} -- " + "the second write silently overwrote the first's blob." + ) + + # (b) both blobs exist and each reads back its OWN distinct payload + read_a = await blob_store.read(uri_a) + read_b = await blob_store.read(uri_b) + assert read_a == {"payload": "result-from-call-A"} + assert read_b == {"payload": "result-from-call-B"} + + +async def test_blob_node_id_matches_default_handler_event_node_id( + pipeline_handlers: Any, +) -> None: + """Pins the invariant: the blob-key node_id pipeline.process_event computes + must EQUAL the event_node_id handlers/data_layer_1/default.py computes for + the same event (make_node_id(session_id, event, timestamp, + data.get("tool_call_id"))). If these ever drift apart again, the + collision this test suite guards against reappears.""" + from context_intelligence_server.pipeline import process_event + from context_intelligence_server.utils import make_node_id + + worker = MagicMock() + worker.services.ensure_session_node = AsyncMock() + worker.services.touch_session = AsyncMock() + worker.services.graph = MagicMock() + worker.services.graph.flush = AsyncMock() + worker.services.blob_store = MagicMock() # truthy blob_store + + session_id = "sess-invariant" + event_name = "tool_call:end" + timestamp = "2024-06-01T12:00:00.000Z" + tool_call_id = "call-invariant" + + data = { + "session_id": session_id, + "timestamp": timestamp, + "tool_call_id": tool_call_id, + } + + expected_event_node_id = make_node_id( + session_id, event_name, timestamp, tool_call_id + ) + + with patch( + "context_intelligence_server.pipeline.process_event_data", + new_callable=AsyncMock, + ) as mock_process: + await process_event(worker, event_name, data, pipeline_handlers) + actual_blob_node_id = mock_process.call_args.args[3] + + assert actual_blob_node_id == expected_event_node_id, ( + "Blob-key node_id has drifted from default.py's event_node_id -- " + "this reintroduces the same-millisecond blob-key collision." + ) + + # =========================================================================== # process_event — touch_session call site # =========================================================================== From 65d28414c56c8dde6f20622a6be7d005b2559329 Mon Sep 17 00:00:00 2001 From: colombod Date: Mon, 17 Aug 2026 18:04:27 +0000 Subject: [PATCH 3/5] refactor(storage): obtain all storages from config-driven factories behind protocols MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Blob store redesign — package split:** - Moved BlobStore Protocol, BlobReference, BlobNotFoundError to blob_store/protocol.py - Renamed AsyncDiskBlobStore → FileSystemBlobStore; moved to blob_store/filesystem.py - Added blob_store/factory.py with create_blob_store(settings) → BlobStore - Reads settings.blob_backend to select backend ("filesystem" → FileSystemBlobStore, "azure" → NotImplementedError for now) - This is the ONLY site where the concrete backend is constructed - Enables Azure implementation as: one new file + one factory branch; zero consumer changes - blob_store/__init__.py re-exports the protocol and factory function **Config-driven backend selection:** - Added blob_backend field to config.py (defaults to "filesystem") - Registry now receives BlobStore via create_blob_store(settings), typed to the Protocol - Registry no longer imports FileSystemBlobStore or touches settings.blob_path - Consumers (registry and above) are entirely backend-agnostic **Factory pattern applied uniformly to all storages:** - queue_manager.py: added create_queue_manager(settings: Settings) → QueueManager - Registry calls this instead of constructing QueueManager directly - identity_store.py: added create_identity_store(settings: Settings, kind: str) → IdentityStore - Selects entra or api_key backend by kind parameter - main.py bootstrap uses this instead of direct construction - All storage objects now obtained from factories; never instantiated by consumers **Protocol-typed DI:** - registry.blob_store now typed to BlobStore (the Protocol), not the concrete class - This ensures type safety and makes backend swaps transparent to consumers **Test isolation tightened:** - test_blob_isolation_tripwire.py: concrete FileSystemBlobStore + settings.blob_path only inside blob_store package and config.py - Verified non-vacuous: full suite 1873 passed, 4 skipped - Rename refactor across all test files (AsyncDiskBlobStore → FileSystemBlobStore) - conftest.py: added blob_backend to _SettingsProxy **Design principle honored:** Storages obtained from factories with configuration, not direct construction, so that all consumers are implementation-agnostic. This enables the Azure backend and any future backend to be added without touching consumer code. Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- README.md | 2 +- .../blob_store/__init__.py | 29 ++++ .../blob_store/factory.py | 46 ++++++ .../filesystem.py} | 144 +++--------------- .../blob_store/protocol.py | 109 +++++++++++++ context_intelligence_server/config.py | 6 + context_intelligence_server/identity_store.py | 31 ++++ context_intelligence_server/main.py | 17 ++- context_intelligence_server/queue_manager.py | 15 +- context_intelligence_server/registry.py | 30 ++-- tests/conftest.py | 1 + tests/test_blob_isolation_tripwire.py | 57 +++++-- tests/test_blob_store.py | 68 ++++----- tests/test_m2_service_auth.py | 2 +- tests/test_main.py | 10 +- tests/test_pipeline.py | 6 +- tests/test_registry.py | 22 +-- 17 files changed, 387 insertions(+), 208 deletions(-) create mode 100644 context_intelligence_server/blob_store/__init__.py create mode 100644 context_intelligence_server/blob_store/factory.py rename context_intelligence_server/{blob_store.py => blob_store/filesystem.py} (65%) create mode 100644 context_intelligence_server/blob_store/protocol.py diff --git a/README.md b/README.md index 01b95ffb..7b389c22 100644 --- a/README.md +++ b/README.md @@ -516,7 +516,7 @@ amplifier-context-intelligence/ │ ├── pipeline.py # Per-event dispatch spine (invoked by the drainer) │ ├── neo4j_store.py # Neo4jGraphStore (managed-tx writes) │ ├── graph_store.py # Graph store protocol / abstraction -│ ├── blob_store.py # AsyncDiskBlobStore +│ ├── blob_store/ # BlobStore Protocol + FileSystemBlobStore + config-driven factory │ ├── idempotency.py # Idempotent MERGE / dedupe helpers │ ├── auth.py # Bearer-token API authentication │ ├── status.py # Status/version plumbing (EventRingBuffer, build_status_response, SERVER_VERSION) diff --git a/context_intelligence_server/blob_store/__init__.py b/context_intelligence_server/blob_store/__init__.py new file mode 100644 index 00000000..0a9ecdac --- /dev/null +++ b/context_intelligence_server/blob_store/__init__.py @@ -0,0 +1,29 @@ +"""blob_store \u2014 session-scoped, URI-addressable blob storage. + +The public surface is the backend-neutral :class:`BlobStore` Protocol plus +:class:`BlobReference` / :class:`BlobNotFoundError` and the +:func:`create_blob_store` factory. Consumers should depend on these, never on +a concrete backend class. + +Package layout: + protocol.py BlobStore Protocol, BlobReference, BlobNotFoundError \u2014 the + backend-neutral seam (no filesystem imports). + filesystem.py FileSystemBlobStore \u2014 the disk-backed implementation. + factory.py create_blob_store(settings) \u2014 the ONLY place a backend is + selected and the ONLY place (besides config.py) that reads + settings.blob_path. +""" + +from __future__ import annotations + +from .factory import create_blob_store +from .filesystem import FileSystemBlobStore +from .protocol import BlobNotFoundError, BlobReference, BlobStore + +__all__ = [ + "BlobNotFoundError", + "BlobReference", + "BlobStore", + "FileSystemBlobStore", + "create_blob_store", +] diff --git a/context_intelligence_server/blob_store/factory.py b/context_intelligence_server/blob_store/factory.py new file mode 100644 index 00000000..adebde1b --- /dev/null +++ b/context_intelligence_server/blob_store/factory.py @@ -0,0 +1,46 @@ +"""Config-driven BlobStore factory \u2014 the ONLY place a blob-store backend is selected. + +This is the single seam through which the concrete backend is chosen. Adding +a new backend (e.g. Azure) means: one new module implementing +:class:`~.protocol.BlobStore`, one new branch here, and a config value \u2014 +zero changes to :mod:`context_intelligence_server.registry` or any consumer. + +This module (and :mod:`~context_intelligence_server.config`) are the only +places ``settings.blob_path`` is read \u2014 the on-disk root is a filesystem- +backend concern, resolved here and handed to the concrete backend at +construction time. Callers only ever see the :class:`~.protocol.BlobStore` +Protocol. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from .filesystem import FileSystemBlobStore +from .protocol import BlobStore + +if TYPE_CHECKING: + from context_intelligence_server.config import Settings + + +def create_blob_store(settings: Settings) -> BlobStore: + """Build the configured :class:`~.protocol.BlobStore` backend. + + Reads ``settings.blob_backend`` (default ``"filesystem"``) to select the + implementation: + + - ``"filesystem"``: :class:`~.filesystem.FileSystemBlobStore` rooted at + ``settings.blob_path``. + - ``"azure"``: not yet implemented. + - anything else: rejected as an unknown backend. + + Raises: + NotImplementedError: If ``blob_backend == "azure"`` (not yet built). + ValueError: If ``blob_backend`` names an unknown backend. + """ + backend = settings.blob_backend + if backend == "filesystem": + return FileSystemBlobStore(root=settings.blob_path) + if backend == "azure": + raise NotImplementedError("azure blob backend not yet implemented") + raise ValueError(f"Unknown blob_backend: {backend!r}") diff --git a/context_intelligence_server/blob_store.py b/context_intelligence_server/blob_store/filesystem.py similarity index 65% rename from context_intelligence_server/blob_store.py rename to context_intelligence_server/blob_store/filesystem.py index 03bc7e84..505508e2 100644 --- a/context_intelligence_server/blob_store.py +++ b/context_intelligence_server/blob_store/filesystem.py @@ -1,4 +1,4 @@ -"""AsyncDiskBlobStore — async, disk-backed blob storage with ci-blob:// URIs. +"""FileSystemBlobStore \u2014 async, disk-backed blob storage with ci-blob:// URIs. Disk layout: //blobs/.json @@ -9,11 +9,10 @@ All filesystem I/O is wrapped with ``asyncio.to_thread`` to keep the event loop non-blocking. -The ``BlobStore`` Protocol is the backend-neutral seam: the only identity -that crosses the boundary is the ``ci-blob:///`` URI, carried -by :class:`BlobReference`. No ``Path``, on-disk layout, ``dest_dir``, or -``os.*`` detail appears in the Protocol or in any value it returns — that is -private to :class:`AsyncDiskBlobStore` (and, later, an Azure equivalent). +This is a concrete implementation of the :class:`~context_intelligence_server.blob_store.protocol.BlobStore` +Protocol. No ``Path``, on-disk layout, ``dest_dir``, or ``os.*`` detail +appears in the Protocol or in any value it returns \u2014 those details are +private to this class (and, later, an Azure equivalent). """ from __future__ import annotations @@ -24,113 +23,16 @@ import shutil import tempfile from collections.abc import AsyncIterator -from dataclasses import dataclass from pathlib import Path -from typing import Any, Protocol, cast, runtime_checkable +from typing import Any, cast -_SCHEME = "ci-blob://" - - -# --------------------------------------------------------------------------- -# BlobNotFoundError — backend-neutral missing-blob exception (guard #6) -# --------------------------------------------------------------------------- - - -class BlobNotFoundError(FileNotFoundError): - """Raised when a blob addressed by a ``ci-blob://`` URI does not exist. - - Subclasses :class:`FileNotFoundError` so existing ``except - FileNotFoundError`` callers keep working unchanged (zero caller churn). - The message carries the URI ONLY — never an on-disk path, container, or - account — so a future Azure backend can raise the same type/message - shape and no caller (or log line) ever learns which backend is in use. - """ - - -# --------------------------------------------------------------------------- -# BlobReference — cheap handle: identity + metadata, NO payload, NO Path -# --------------------------------------------------------------------------- - - -@dataclass(frozen=True) -class BlobReference: - """Cheap handle — identity + metadata, NO payload, NO Path. - - This is what ``scan()``/``list()`` return and what everything except a - payload read passes around. It is what gets serialized on the graph (as - its ``.uri``). - """ - - uri: str # ci-blob:/// — the ONLY address callers use - session_id: str - key: str - size: int # content length in bytes - last_modified: float # epoch seconds: disk st_mtime || azure Last-Modified - - -# --------------------------------------------------------------------------- -# BlobStore protocol -# --------------------------------------------------------------------------- - - -@runtime_checkable -class BlobStore(Protocol): - """Protocol for a session-scoped, URI-addressable blob store. - - 100% backend-neutral: the only identity that crosses the boundary is the - ``ci-blob://`` URI (carried by :class:`BlobReference`). No ``Path``, no - on-disk layout, no ``dest_dir``, no ``os.*`` — ever. - """ - - async def write( - self, session_id: str, key: str, value: dict[str, Any] | list[Any] - ) -> BlobReference: - """Persist *value* as JSON and return a :class:`BlobReference`.""" - ... +from .protocol import BlobNotFoundError, BlobReference - def list(self, session_id: str) -> AsyncIterator[BlobReference]: - """Stream all blob references for *session_id* (one session).""" - ... - - def scan(self) -> AsyncIterator[BlobReference]: - """Stream all blob references across ALL sessions.""" - ... - - async def delete( - self, uri: str, if_unmodified: BlobReference | None = None - ) -> bool: - """Delete the blob addressed by *uri*. Idempotent: returns False if absent. - - Args: - uri: The ``ci-blob://`` URI to delete. - if_unmodified: When provided, this is a **fenced (compare-and-delete)** - delete — the store re-checks the blob's current metadata against - *if_unmodified* (disk: mtime + size; Azure: ``If-Match`` ETag) and - refuses (returns ``False``, does NOT delete) if the blob changed - since *if_unmodified* was observed (e.g. by a `scan()`). When - ``None`` (default), this is the unconditional idempotent delete. - """ - ... - - async def read(self, uri: str) -> dict[str, Any] | list[Any]: - """Resolve *uri* and return the stored value (the sole payload path). - - Raises: - ValueError: If *uri* does not match the ``ci-blob://`` scheme. - BlobNotFoundError: If no blob exists for *uri*. Subclasses - ``FileNotFoundError`` for back-compat; the message carries the - URI only — never an on-disk path, container, or account. - """ - ... - - -# --------------------------------------------------------------------------- -# AsyncDiskBlobStore -# --------------------------------------------------------------------------- +_SCHEME = "ci-blob://" -class AsyncDiskBlobStore: - """Async, disk-backed implementation of :class:`BlobStore`. +class FileSystemBlobStore: + """Async, disk-backed implementation of :class:`~.protocol.BlobStore`. Args: root: Root directory under which all session blobs are stored. @@ -155,10 +57,10 @@ def _parse_uri(self, uri: str) -> tuple[str, str]: """ if not uri.startswith(_SCHEME): raise ValueError( - f"Invalid URI scheme — expected '{_SCHEME}...', got: {uri!r}" + f"Invalid URI scheme \u2014 expected '{_SCHEME}...', got: {uri!r}" ) remainder = uri[len(_SCHEME) :] - # remainder must be "/" — both parts non-empty + # remainder must be "/" \u2014 both parts non-empty if "/" not in remainder: raise ValueError(f"URI missing key component: {uri!r}") session_id, _, key = remainder.partition("/") @@ -193,7 +95,7 @@ async def write( Creates the directory ``//blobs/`` if needed. ``last_modified`` is the storage mtime (from the same ``stat`` call - that produces ``size``) — never a writer-clock timestamp. + that produces ``size``) \u2014 never a writer-clock timestamp. """ path = self._blob_path(session_id, key) @@ -229,7 +131,7 @@ def _write() -> os.stat_result: async def read(self, uri: str) -> dict[str, Any] | list[Any]: """Return the blob addressed by *uri*. - The session_id is resolved from the URI itself — callers do not + The session_id is resolved from the URI itself \u2014 callers do not supply it separately (avoids the bundle footgun where the wrong session_id is passed). @@ -237,7 +139,7 @@ async def read(self, uri: str) -> dict[str, Any] | list[Any]: ValueError: If *uri* is not a valid ``ci-blob://`` URI. BlobNotFoundError: If no blob exists for *uri*. Subclasses ``FileNotFoundError`` for back-compat; the message carries - the URI only — never the on-disk path. + the URI only \u2014 never the on-disk path. """ session_id, key = self._parse_uri(uri) path = self._blob_path(session_id, key) @@ -290,7 +192,7 @@ def _list_entries() -> list[tuple[str, int, float]]: async def scan(self) -> AsyncIterator[BlobReference]: """Stream all blob references across ALL sessions. - Walks ``/*/blobs/*.json`` — session-dir enumeration and each + Walks ``/*/blobs/*.json`` \u2014 session-dir enumeration and each session's blob-dir scan are offloaded to a thread in small units (never one giant ``to_thread`` for the whole tree), so references stream out as they are discovered instead of materializing the @@ -318,15 +220,15 @@ async def delete( Args: uri: The ``ci-blob://`` URI to delete. - if_unmodified: When ``None`` (default), unconditional delete — + if_unmodified: When ``None`` (default), unconditional delete \u2014 unlinks and returns ``True``, or ``False`` if already absent. When provided, this is a **fenced compare-and-delete**: the blob is re-``stat``'d (inside the same thread hop, right before the unlink, to minimise the TOCTOU window) and the delete only proceeds if ``st_mtime``/``st_size`` still match - *if_unmodified* — i.e. nothing rewrote the blob since it was + *if_unmodified* \u2014 i.e. nothing rewrote the blob since it was observed (e.g. by ``scan()``). If the blob is missing, or it - changed, the delete is refused and ``False`` is returned — + changed, the delete is refused and ``False`` is returned \u2014 the blob is left untouched on disk. """ session_id, key = self._parse_uri(uri) @@ -349,7 +251,7 @@ def _delete() -> bool: st.st_mtime != if_unmodified.last_modified or st.st_size != if_unmodified.size ): - # Blob was rewritten since it was observed — refuse to delete. + # Blob was rewritten since it was observed \u2014 refuse to delete. return False try: os.unlink(path) @@ -363,9 +265,9 @@ def _delete() -> bool: async def dump(self, uri: str, dest_dir: Path | str | None = None) -> str: """Copy the blob file addressed by *uri* to *dest_dir*. - Disk-only helper — NOT part of the :class:`BlobStore` Protocol - (no production caller; kept as a concrete convenience for external - tooling that needs a local export). + Disk-only helper \u2014 NOT part of the :class:`~.protocol.BlobStore` + Protocol (no production caller; kept as a concrete convenience for + external tooling that needs a local export). Args: uri: ``ci-blob://`` URI identifying the blob to copy. diff --git a/context_intelligence_server/blob_store/protocol.py b/context_intelligence_server/blob_store/protocol.py new file mode 100644 index 00000000..a48c1eaa --- /dev/null +++ b/context_intelligence_server/blob_store/protocol.py @@ -0,0 +1,109 @@ +"""BlobStore Protocol \u2014 the backend-neutral seam. + +The only identity that crosses the boundary is the ``ci-blob:///`` +URI, carried by :class:`BlobReference`. No ``Path``, on-disk layout, ``dest_dir``, +or ``os.*`` detail appears here or in any value the Protocol returns \u2014 that is +private to a concrete backend (:class:`~context_intelligence_server.blob_store.filesystem.FileSystemBlobStore` +and, later, an Azure equivalent). + +This module is backend-neutral by construction: it imports nothing from +``os``, ``pathlib``, or any filesystem library. +""" + +from __future__ import annotations + +from collections.abc import AsyncIterator +from dataclasses import dataclass +from typing import Any, Protocol, runtime_checkable + +# --------------------------------------------------------------------------- +# BlobNotFoundError \u2014 backend-neutral missing-blob exception (guard #6) +# --------------------------------------------------------------------------- + + +class BlobNotFoundError(FileNotFoundError): + """Raised when a blob addressed by a ``ci-blob://`` URI does not exist. + + Subclasses :class:`FileNotFoundError` so existing ``except + FileNotFoundError`` callers keep working unchanged (zero caller churn). + The message carries the URI ONLY \u2014 never an on-disk path, container, or + account \u2014 so a future Azure backend can raise the same type/message + shape and no caller (or log line) ever learns which backend is in use. + """ + + +# --------------------------------------------------------------------------- +# BlobReference \u2014 cheap handle: identity + metadata, NO payload, NO Path +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class BlobReference: + """Cheap handle \u2014 identity + metadata, NO payload, NO Path. + + This is what ``scan()``/``list()`` return and what everything except a + payload read passes around. It is what gets serialized on the graph (as + its ``.uri``). + """ + + uri: str # ci-blob:/// \u2014 the ONLY address callers use + session_id: str + key: str + size: int # content length in bytes + last_modified: float # epoch seconds: disk st_mtime || azure Last-Modified + + +# --------------------------------------------------------------------------- +# BlobStore protocol +# --------------------------------------------------------------------------- + + +@runtime_checkable +class BlobStore(Protocol): + """Protocol for a session-scoped, URI-addressable blob store. + + 100% backend-neutral: the only identity that crosses the boundary is the + ``ci-blob://`` URI (carried by :class:`BlobReference`). No ``Path``, no + on-disk layout, no ``dest_dir``, no ``os.*`` \u2014 ever. + """ + + async def write( + self, session_id: str, key: str, value: dict[str, Any] | list[Any] + ) -> BlobReference: + """Persist *value* as JSON and return a :class:`BlobReference`.""" + ... + + def list(self, session_id: str) -> AsyncIterator[BlobReference]: + """Stream all blob references for *session_id* (one session).""" + ... + + def scan(self) -> AsyncIterator[BlobReference]: + """Stream all blob references across ALL sessions.""" + ... + + async def delete( + self, uri: str, if_unmodified: BlobReference | None = None + ) -> bool: + """Delete the blob addressed by *uri*. Idempotent: returns False if absent. + + Args: + uri: The ``ci-blob://`` URI to delete. + if_unmodified: When provided, this is a **fenced (compare-and-delete)** + delete \u2014 the store re-checks the blob's current metadata against + *if_unmodified* (disk: mtime + size; Azure: ``If-Match`` ETag) and + refuses (returns ``False``, does NOT delete) if the blob changed + since *if_unmodified* was observed (e.g. by a `scan()`). When + ``None`` (default), this is the unconditional idempotent delete. + """ + ... + + async def read(self, uri: str) -> dict[str, Any] | list[Any]: + """Resolve *uri* and return the stored value (the sole payload path). + + Raises: + ValueError: If *uri* does not match the ``ci-blob://`` scheme. + BlobNotFoundError: If no blob exists for *uri*. Subclasses + ``FileNotFoundError`` for back-compat; the message carries the + URI only \u2014 never an on-disk path, container, or account. + """ + ... diff --git a/context_intelligence_server/config.py b/context_intelligence_server/config.py index 48b20e97..21577c44 100644 --- a/context_intelligence_server/config.py +++ b/context_intelligence_server/config.py @@ -800,6 +800,12 @@ def resolve_neo4j_query(self) -> Neo4jClientConfig: # ------------------------------------------------------------------------- # Storage paths # ------------------------------------------------------------------------- + # blob_backend selects the BlobStore implementation via + # context_intelligence_server.blob_store.create_blob_store(); "filesystem" + # is the only backend implemented today. blob_path is that filesystem + # backend's own root -- it is read only by the factory (and here, at + # declaration) and is otherwise meaningless to any other backend. + blob_backend: str = "filesystem" blob_path: str = "/data/blobs" queues_path: str = "/data/queues" diff --git a/context_intelligence_server/identity_store.py b/context_intelligence_server/identity_store.py index c6219570..7b230af3 100644 --- a/context_intelligence_server/identity_store.py +++ b/context_intelligence_server/identity_store.py @@ -42,11 +42,17 @@ } """ +from __future__ import annotations + import json import logging import os import tempfile from pathlib import Path +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from context_intelligence_server.config import Settings logger = logging.getLogger(__name__) @@ -236,3 +242,28 @@ def _write_atomic(self, data: dict[str, dict[str, str]]) -> None: except Exception: pass raise + + +def create_identity_store(settings: Settings, kind: str) -> IdentityStore: + """Build an ``IdentityStore`` rooted at the configured path for *kind*. + + Construction only \u2014 callers are responsible for ``load()``/``seed()`` + and any auth-mode wiring (this mirrors ``blob_store.factory.create_blob_store``: + a single-backend, config-reading seam that keeps the store paths out of + consumers such as ``main.py``). + + Args: + settings: The active ``Settings``. + kind: ``"entra"`` for the OID identity map, ``"api_key"`` for the + SHA-256 digest keystore. + + Raises: + ValueError: If *kind* is neither ``"entra"`` nor ``"api_key"``. + """ + if kind == "entra": + path = settings.entra_identities_store_path + elif kind == "api_key": + path = settings.api_keys_store_path + else: + raise ValueError(f"Unknown identity store kind: {kind!r}") + return IdentityStore(Path(path)) diff --git a/context_intelligence_server/main.py b/context_intelligence_server/main.py index 7b7fd034..49a3ae2d 100644 --- a/context_intelligence_server/main.py +++ b/context_intelligence_server/main.py @@ -11,7 +11,6 @@ from collections.abc import AsyncGenerator from contextlib import asynccontextmanager, suppress from datetime import datetime -from pathlib import Path from typing import Any from fastapi import Depends, FastAPI, HTTPException, Request @@ -32,7 +31,10 @@ ) from context_intelligence_server.config import Neo4jClientConfig, Settings, get_settings from context_intelligence_server.idempotency import EventIdempotencyCache -from context_intelligence_server.identity_store import IdentityStore +from context_intelligence_server.identity_store import ( + IdentityStore, + create_identity_store, +) from context_intelligence_server.logging_config import setup_logging from context_intelligence_server.models import ( CypherRequest, @@ -395,9 +397,10 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: # Single shared BlobStore instance for the whole process (T1.5): the /blobs # routes above and the registry's session-worker construction both go # through registry.blob_store, which lazily builds and caches exactly one -# AsyncDiskBlobStore. Also mirrored on app.state so other routers (e.g. a -# future admin reclaim rewire) can reach the same instance without importing -# the module-level `registry` name. +# BlobStore (backend selected by create_blob_store() from config). Also +# mirrored on app.state so other routers (e.g. a future admin reclaim +# rewire) can reach the same instance without importing the module-level +# `registry` name. app.state.blob_store = registry.blob_store idempotency_cache = EventIdempotencyCache() @@ -578,7 +581,7 @@ def create_asgi_app( if s.auth_mode == "entra": # Build and load the entra identity store. - entra_store = IdentityStore(Path(s.entra_identities_store_path)) + entra_store = create_identity_store(s, "entra") entra_store.load() if not entra_store.exists(): # First boot: seed in-process map from config. Converts the flat @@ -642,7 +645,7 @@ def create_asgi_app( admin_api_key_digest = None else: # Build and load the API-key store. - key_store = IdentityStore(Path(s.api_keys_store_path)) + key_store = create_identity_store(s, "api_key") key_store.load() if not key_store.exists(): # First boot: seed from config. Converts the flat diff --git a/context_intelligence_server/queue_manager.py b/context_intelligence_server/queue_manager.py index 6fba186e..85bf993b 100644 --- a/context_intelligence_server/queue_manager.py +++ b/context_intelligence_server/queue_manager.py @@ -31,7 +31,10 @@ import time from dataclasses import dataclass from pathlib import Path -from typing import Any +from typing import TYPE_CHECKING, Any + +if TYPE_CHECKING: + from context_intelligence_server.config import Settings # Fixed buffer size for streaming scans over a session ``.log`` (last-newline # search and newline counting). Bounds boot-time and /status memory to O(chunk) @@ -729,3 +732,13 @@ def _reconcile() -> int: return total_skipped return await asyncio.to_thread(_reconcile) + + +def create_queue_manager(settings: Settings) -> QueueManager: + """Build the durable ``QueueManager`` from config. + + Single backend today (on-disk), so this is a thin config-reading seam + rather than a multi-backend dispatcher \u2014 but it keeps ``settings.queues_path`` + out of consumers (mirrors ``blob_store.factory.create_blob_store``). + """ + return QueueManager(queues_dir=Path(settings.queues_path)) diff --git a/context_intelligence_server/registry.py b/context_intelligence_server/registry.py index 336cd064..03fc2f12 100644 --- a/context_intelligence_server/registry.py +++ b/context_intelligence_server/registry.py @@ -6,16 +6,19 @@ import time from collections import deque from dataclasses import dataclass, field -from pathlib import Path from typing import Any -from context_intelligence_server.blob_store import AsyncDiskBlobStore +from context_intelligence_server.blob_store import BlobStore, create_blob_store from context_intelligence_server.config import get_settings -from context_intelligence_server.status import EventRecord, ring_buffer from context_intelligence_server.neo4j_store import Neo4jGraphStore from context_intelligence_server.pipeline import process_event, setup_handlers -from context_intelligence_server.queue_manager import Batch, QueueManager +from context_intelligence_server.queue_manager import ( + Batch, + QueueManager, + create_queue_manager, +) from context_intelligence_server.services import HookStateService +from context_intelligence_server.status import EventRecord, ring_buffer logger = logging.getLogger("context_intelligence_server") @@ -69,7 +72,7 @@ def __init__(self) -> None: # before the per-test settings patch applies, so we cannot read # settings here — see _ensure_infra(). self._queue_manager: QueueManager | None = None - self._blob_store: AsyncDiskBlobStore | None = None + self._blob_store: BlobStore | None = None self._write_semaphore: asyncio.Semaphore | None = None self._max_delivery_attempts: int = 0 # Live pipeline-conservation counters (D2): make silently-dropped @@ -96,7 +99,7 @@ def _ensure_infra(self) -> None: """ if self._queue_manager is None: settings = get_settings() - self._queue_manager = QueueManager(queues_dir=Path(settings.queues_path)) + self._queue_manager = create_queue_manager(settings) self._write_semaphore = asyncio.Semaphore(settings.write_concurrency) self._max_delivery_attempts = settings.max_delivery_attempts @@ -115,17 +118,22 @@ def write_semaphore(self) -> asyncio.Semaphore: return self._write_semaphore @property - def blob_store(self) -> AsyncDiskBlobStore: - """The single shared AsyncDiskBlobStore owned by this registry. + def blob_store(self) -> BlobStore: + """The single shared BlobStore owned by this registry. Built lazily on first access (same rationale as ``queue_manager``: the module-level registry singleton is constructed at import time, before any per-test settings patch applies), then reused for every session worker and route handler — never one instance per session. + + The concrete backend is selected by ``create_blob_store()`` from + config (``settings.blob_backend``) — this registry never references + a concrete backend class or ``settings.blob_path`` directly, so + swapping backends (e.g. adding Azure) touches only the factory. """ if self._blob_store is None: settings = get_settings() - self._blob_store = AsyncDiskBlobStore(root=settings.blob_path) + self._blob_store = create_blob_store(settings) return self._blob_store def record_accepted(self, n: int = 1) -> None: @@ -446,7 +454,9 @@ async def _process_batch( ) -> bool: """Dispatch each line in the batch; return True if it contained a terminal (session:end) event.""" - from context_intelligence_server.pipeline import TERMINAL_EVENTS # noqa: PLC0415 + from context_intelligence_server.pipeline import ( + TERMINAL_EVENTS, + ) saw_terminal = False for raw in batch.lines: diff --git a/tests/conftest.py b/tests/conftest.py index ecfd94d9..c36fd9b9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -107,6 +107,7 @@ def safe_settings(tmp_path: Any) -> Generator[None, None, None]: _real = _Settings() class _SettingsProxy: + blob_backend: str = _real.blob_backend blob_path: str = _real.blob_path queues_path: str = str(tmp_path / "queues") # Redirect identity-store paths so the registry proxy never touches the diff --git a/tests/test_blob_isolation_tripwire.py b/tests/test_blob_isolation_tripwire.py index dc4f1f89..bd8289fa 100644 --- a/tests/test_blob_isolation_tripwire.py +++ b/tests/test_blob_isolation_tripwire.py @@ -1,15 +1,18 @@ -"""Tripwire: the blob-store on-disk layout stays inside blob_store.py. +"""Tripwire: the blob-store on-disk layout stays inside the blob_store package. Locks the isolation boundary established by the BlobStore refactor (see docs/blob-store-abstraction.md). These tests fail loudly if a future -change lets any module other than ``blob_store.py`` locate the blob root or -resurrects the old direct-filesystem reclaim implementation -- i.e. if a -direct-FS blob leak is reintroduced. +change lets any module other than the ``blob_store/`` package locate the blob +root or resurrects the old direct-filesystem reclaim implementation -- i.e. +if a direct-FS blob leak is reintroduced. Two invariants: - 1. Only the single construction site (registry.py) and the config field - declaration (config.py) may reference ``settings.blob_path`` -- a caller - that cannot locate the blob root physically cannot do blob filesystem I/O. + 1. Only the ``blob_store/`` package (specifically ``factory.py``, the single + construction site) and the config field declaration (config.py) may + reference ``settings.blob_path`` -- a caller that cannot locate the blob + root physically cannot do blob filesystem I/O. Since the config-driven + factory refactor, ``registry.py`` no longer references it either -- it + goes through ``create_blob_store(settings)`` and never sees the path. 2. The pre-refactor direct-disk reclaim symbols must never reappear. """ @@ -33,20 +36,46 @@ def _code_lines(path: pathlib.Path): def test_blob_root_locatable_only_at_construction_site() -> None: """`settings.blob_path` -- the only way to find the on-disk blob root -- is - referenced solely where the store is constructed (registry.py) and where the - config field is declared (config.py). blob_store.py owns the layout itself.""" - allowed = {"registry.py", "config.py", "blob_store.py"} + referenced solely inside the blob_store/ package (which owns the layout) + and where the config field is declared (config.py). registry.py goes + through create_blob_store(settings) and never sees the path itself.""" + blob_store_pkg = PKG / "blob_store" + allowed_top_level = {"config.py"} offenders: list[str] = [] for p in PKG.rglob("*.py"): - if p.name in allowed: + # Anything inside the blob_store/ package owns the layout -- allowed. + if blob_store_pkg in p.parents: + continue + if p.name in allowed_top_level: continue for lineno, line in _code_lines(p): if "blob_path" in line: offenders.append(f"{p.relative_to(PKG)}:{lineno}: {line}") assert not offenders, ( - "blob root re-derived outside the single construction site " - "(registry.py)/config -- a caller that can locate the blob root can " - "bypass BlobStore and touch disk directly:\n" + "\n".join(offenders) + "blob root re-derived outside the blob_store/ package/config -- a " + "caller that can locate the blob root can bypass BlobStore and touch " + "disk directly:\n" + "\n".join(offenders) + ) + + +def test_concrete_impl_referenced_only_inside_the_package() -> None: + """The concrete ``FileSystemBlobStore`` must appear ONLY inside the + ``blob_store/`` package. Production consumers depend on the ``BlobStore`` + Protocol and obtain instances via ``create_blob_store(settings)`` -- never + by naming a concrete backend -- so a disk->Azure swap touches only the + package. (Tests are permitted to construct a concrete backend directly.)""" + blob_store_pkg = PKG / "blob_store" + offenders: list[str] = [] + for p in PKG.rglob("*.py"): + if blob_store_pkg in p.parents: + continue + for lineno, line in _code_lines(p): + if "FileSystemBlobStore" in line: + offenders.append(f"{p.relative_to(PKG)}:{lineno}: {line}") + assert not offenders, ( + "concrete FileSystemBlobStore named outside the blob_store/ package -- " + "consumers must use the BlobStore Protocol + create_blob_store(settings), " + "not a concrete backend:\n" + "\n".join(offenders) ) diff --git a/tests/test_blob_store.py b/tests/test_blob_store.py index 312e8974..13884795 100644 --- a/tests/test_blob_store.py +++ b/tests/test_blob_store.py @@ -1,4 +1,4 @@ -"""Tests for AsyncDiskBlobStore — Write, Read, List, Scan, Delete, Dump. +"""Tests for FileSystemBlobStore — Write, Read, List, Scan, Delete, Dump. Covers: 1. write/read roundtrip @@ -31,10 +31,10 @@ import pytest from context_intelligence_server.blob_store import ( - AsyncDiskBlobStore, BlobNotFoundError, BlobReference, BlobStore, + FileSystemBlobStore, ) # --------------------------------------------------------------------------- @@ -43,12 +43,12 @@ @pytest.fixture -def store(tmp_path: Path) -> AsyncDiskBlobStore: - """Return a fresh AsyncDiskBlobStore rooted at a temporary directory.""" - return AsyncDiskBlobStore(root=tmp_path) +def store(tmp_path: Path) -> FileSystemBlobStore: + """Return a fresh FileSystemBlobStore rooted at a temporary directory.""" + return FileSystemBlobStore(root=tmp_path) -async def _list_uris(store: AsyncDiskBlobStore, session_id: str) -> list[str]: +async def _list_uris(store: FileSystemBlobStore, session_id: str) -> list[str]: return [ref.uri async for ref in store.list(session_id)] @@ -57,7 +57,7 @@ async def _list_uris(store: AsyncDiskBlobStore, session_id: str) -> list[str]: # --------------------------------------------------------------------------- -async def test_write_read_roundtrip(store: AsyncDiskBlobStore) -> None: +async def test_write_read_roundtrip(store: FileSystemBlobStore) -> None: """Data written can be read back unchanged.""" payload = {"event": "tool_call", "tool": "bash", "args": ["ls"]} ref = await store.write("session-abc", "tool_call_01", payload) @@ -70,7 +70,7 @@ async def test_write_read_roundtrip(store: AsyncDiskBlobStore) -> None: # --------------------------------------------------------------------------- -async def test_uri_format(store: AsyncDiskBlobStore) -> None: +async def test_uri_format(store: FileSystemBlobStore) -> None: """write() returns a BlobReference whose .uri is ci-blob:///.""" ref = await store.write("session-xyz", "my_key", {"x": 1}) assert isinstance(ref, BlobReference) @@ -87,7 +87,7 @@ async def test_uri_format(store: AsyncDiskBlobStore) -> None: async def test_directory_structure_creation( - store: AsyncDiskBlobStore, tmp_path: Path + store: FileSystemBlobStore, tmp_path: Path ) -> None: """write() creates //blobs/.json on disk.""" await store.write("session-123", "blob_key", {"data": "value"}) @@ -103,7 +103,7 @@ async def test_directory_structure_creation( async def test_uri_based_session_id_resolution( - store: AsyncDiskBlobStore, tmp_path: Path + store: FileSystemBlobStore, tmp_path: Path ) -> None: """read() resolves the session_id from the URI, not from a parameter.""" session_id = "session-uri-resolve" @@ -122,7 +122,7 @@ async def test_uri_based_session_id_resolution( # --------------------------------------------------------------------------- -async def test_missing_blob_raises_file_not_found(store: AsyncDiskBlobStore) -> None: +async def test_missing_blob_raises_file_not_found(store: FileSystemBlobStore) -> None: """read() raises FileNotFoundError for a URI pointing to a non-existent blob.""" uri = "ci-blob://session-missing/nonexistent_key" with pytest.raises(FileNotFoundError): @@ -134,7 +134,7 @@ async def test_missing_blob_raises_file_not_found(store: AsyncDiskBlobStore) -> # --------------------------------------------------------------------------- -async def test_invalid_uri_raises_value_error(store: AsyncDiskBlobStore) -> None: +async def test_invalid_uri_raises_value_error(store: FileSystemBlobStore) -> None: """read() raises ValueError for URIs that don't match the ci-blob:// scheme.""" with pytest.raises(ValueError): await store.read("not-a-ci-blob-uri") @@ -151,7 +151,7 @@ async def test_invalid_uri_raises_value_error(store: AsyncDiskBlobStore) -> None # --------------------------------------------------------------------------- -async def test_empty_list_for_missing_session(store: AsyncDiskBlobStore) -> None: +async def test_empty_list_for_missing_session(store: FileSystemBlobStore) -> None: """list() yields nothing when no blobs exist for the session.""" result = await _list_uris(store, "session-does-not-exist") assert result == [] @@ -162,7 +162,7 @@ async def test_empty_list_for_missing_session(store: AsyncDiskBlobStore) -> None # --------------------------------------------------------------------------- -async def test_correct_uri_listing(store: AsyncDiskBlobStore) -> None: +async def test_correct_uri_listing(store: FileSystemBlobStore) -> None: """list() yields all blob references for a session, sorted by key.""" session_id = "session-list" await store.write(session_id, "key_b", {"b": 2}) @@ -187,7 +187,7 @@ async def test_correct_uri_listing(store: AsyncDiskBlobStore) -> None: # --------------------------------------------------------------------------- -async def test_session_isolation(store: AsyncDiskBlobStore) -> None: +async def test_session_isolation(store: FileSystemBlobStore) -> None: """list() only returns references for the requested session, not other sessions.""" await store.write("session-alpha", "blob_1", {"alpha": True}) await store.write("session-beta", "blob_2", {"beta": True}) @@ -209,7 +209,7 @@ async def test_session_isolation(store: AsyncDiskBlobStore) -> None: async def test_asyncio_to_thread_delegation(tmp_path: Path) -> None: """All filesystem I/O is delegated to asyncio.to_thread for non-blocking I/O.""" - store = AsyncDiskBlobStore(root=tmp_path) + store = FileSystemBlobStore(root=tmp_path) to_thread_calls: list[str] = [] original_to_thread = asyncio.to_thread @@ -236,7 +236,7 @@ async def tracking_to_thread(func, *args, **kwargs): # type: ignore[no-untyped- async def test_dump_copy_to_specified_dest_dir( - store: AsyncDiskBlobStore, tmp_path: Path + store: FileSystemBlobStore, tmp_path: Path ) -> None: """dump() copies the blob file to the specified dest_dir and returns the path.""" session_id = "session-dump-copy" @@ -258,7 +258,7 @@ async def test_dump_copy_to_specified_dest_dir( # --------------------------------------------------------------------------- -async def test_dump_default_dest_dir(store: AsyncDiskBlobStore) -> None: +async def test_dump_default_dest_dir(store: FileSystemBlobStore) -> None: """dump() uses Path(tempfile.gettempdir()) / 'ci-blobs' when dest_dir is None.""" import tempfile @@ -280,7 +280,7 @@ async def test_dump_default_dest_dir(store: AsyncDiskBlobStore) -> None: async def test_dump_missing_blob_raises_file_not_found( - store: AsyncDiskBlobStore, + store: FileSystemBlobStore, ) -> None: """dump() raises FileNotFoundError with 'Blob not found' message for missing blob.""" uri = "ci-blob://session-nonexistent/missing_blob" @@ -294,7 +294,7 @@ async def test_dump_missing_blob_raises_file_not_found( async def test_dump_uses_asyncio_to_thread_for_copy2( - store: AsyncDiskBlobStore, tmp_path: Path + store: FileSystemBlobStore, tmp_path: Path ) -> None: """dump() delegates shutil.copy2 to asyncio.to_thread for non-blocking I/O.""" session_id = "session-dump-thread" @@ -323,8 +323,8 @@ async def tracking_to_thread(func, *args, **kwargs): # type: ignore[no-untyped- # --------------------------------------------------------------------------- -def test_blob_store_protocol_conformance(store: AsyncDiskBlobStore) -> None: - """AsyncDiskBlobStore conforms to the BlobStore protocol.""" +def test_blob_store_protocol_conformance(store: FileSystemBlobStore) -> None: + """FileSystemBlobStore conforms to the BlobStore protocol.""" assert isinstance(store, BlobStore) @@ -334,7 +334,7 @@ def test_blob_store_protocol_conformance(store: AsyncDiskBlobStore) -> None: async def test_write_is_atomic_no_torn_file_on_failure( - store: AsyncDiskBlobStore, tmp_path: Path + store: FileSystemBlobStore, tmp_path: Path ) -> None: """A failure during os.replace leaves no torn final file and no temp siblings.""" session_id = "sess-atomic" @@ -342,7 +342,7 @@ async def test_write_is_atomic_no_torn_file_on_failure( with ( patch( - "context_intelligence_server.blob_store.os.replace", + "context_intelligence_server.blob_store.filesystem.os.replace", side_effect=OSError("simulated replace failure"), ), pytest.raises(OSError), @@ -359,7 +359,7 @@ async def test_write_is_atomic_no_torn_file_on_failure( async def test_write_replaces_atomically_on_success( - store: AsyncDiskBlobStore, + store: FileSystemBlobStore, ) -> None: """On success the final file has the exact JSON, no temp remains, URI is correct.""" session_id = "sess-atomic" @@ -380,7 +380,7 @@ async def test_write_replaces_atomically_on_success( async def test_scan_yields_references_across_sessions( - store: AsyncDiskBlobStore, + store: FileSystemBlobStore, ) -> None: """scan() streams a BlobReference for every blob across ALL sessions.""" await store.write("session-scan-a", "k1", {"a": 1}) @@ -400,7 +400,7 @@ async def test_scan_yields_references_across_sessions( assert r.last_modified > 0 -async def test_scan_empty_store_yields_nothing(store: AsyncDiskBlobStore) -> None: +async def test_scan_empty_store_yields_nothing(store: FileSystemBlobStore) -> None: """scan() over an empty store yields no references.""" refs = [ref async for ref in store.scan()] assert refs == [] @@ -411,7 +411,7 @@ async def test_scan_empty_store_yields_nothing(store: AsyncDiskBlobStore) -> Non # --------------------------------------------------------------------------- -async def test_delete_idempotent_true_then_false(store: AsyncDiskBlobStore) -> None: +async def test_delete_idempotent_true_then_false(store: FileSystemBlobStore) -> None: """delete() returns True the first time (blob existed), False thereafter.""" ref = await store.write("session-delete", "to_delete", {"gone": "soon"}) @@ -426,7 +426,7 @@ async def test_delete_idempotent_true_then_false(store: AsyncDiskBlobStore) -> N assert second is False -async def test_delete_missing_blob_returns_false(store: AsyncDiskBlobStore) -> None: +async def test_delete_missing_blob_returns_false(store: FileSystemBlobStore) -> None: """delete() on a never-written blob returns False, never raises.""" result = await store.delete("ci-blob://never-existed/nope") assert result is False @@ -438,7 +438,7 @@ async def test_delete_missing_blob_returns_false(store: AsyncDiskBlobStore) -> N async def test_missing_blob_raises_blob_not_found_error_no_path_leak( - store: AsyncDiskBlobStore, tmp_path: Path + store: FileSystemBlobStore, tmp_path: Path ) -> None: """read() of a missing uri raises BlobNotFoundError (a FileNotFoundError subclass, for back-compat) whose message carries the uri only — never the @@ -465,7 +465,7 @@ async def test_missing_blob_raises_blob_not_found_error_no_path_leak( async def test_fenced_delete_succeeds_when_unchanged( - store: AsyncDiskBlobStore, + store: FileSystemBlobStore, ) -> None: """delete(uri, if_unmodified=ref) removes the blob when it has not changed since ref was observed (e.g. by scan()/list()).""" @@ -479,7 +479,7 @@ async def test_fenced_delete_succeeds_when_unchanged( async def test_fenced_delete_refuses_when_rewritten( - store: AsyncDiskBlobStore, + store: FileSystemBlobStore, ) -> None: """delete(uri, if_unmodified=stale_ref) returns False and leaves the (new) blob on disk when the blob was rewritten after stale_ref was @@ -514,7 +514,7 @@ async def test_fenced_delete_refuses_when_rewritten( async def test_fenced_delete_missing_blob_returns_false( - store: AsyncDiskBlobStore, + store: FileSystemBlobStore, ) -> None: """delete(uri, if_unmodified=ref) on an already-absent blob returns False.""" ref = await store.write("session-fence-missing", "gone_key", {"v": 1}) @@ -525,7 +525,7 @@ async def test_fenced_delete_missing_blob_returns_false( async def test_unconditional_delete_still_idempotent( - store: AsyncDiskBlobStore, + store: FileSystemBlobStore, ) -> None: """Unconditional delete(uri) (if_unmodified=None, the default) is unchanged: True then False, idempotent.""" diff --git a/tests/test_m2_service_auth.py b/tests/test_m2_service_auth.py index 099adfc8..3ccb00e0 100644 --- a/tests/test_m2_service_auth.py +++ b/tests/test_m2_service_auth.py @@ -123,7 +123,7 @@ def _service_claims(roles: list[str], appid: str = FAKE_APPID) -> dict[str, Any] class _MockBlobStore: - """Mock for AsyncDiskBlobStore — yields nothing, never touches filesystem.""" + """Mock for FileSystemBlobStore — yields nothing, never touches filesystem.""" def __init__(self, root: Any = None) -> None: pass diff --git a/tests/test_main.py b/tests/test_main.py index a8510a0d..1ed0180a 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -14,7 +14,7 @@ import context_intelligence_server.main as main_module from context_intelligence_server.auth import BearerTokenMiddleware -from context_intelligence_server.blob_store import AsyncDiskBlobStore +from context_intelligence_server.blob_store import FileSystemBlobStore from context_intelligence_server.main import app, lifespan, registry from context_intelligence_server.models import CypherRequest from tests.conftest import MockNeo4jDriver @@ -282,7 +282,7 @@ async def test_list_blobs_returns_empty_for_session_with_no_blobs( ) -> None: """GET /blobs/{session_id} returns 200 with empty blobs list for session with no blobs.""" monkeypatch.setattr( - main_module.registry, "_blob_store", AsyncDiskBlobStore(root=tmp_path) + main_module.registry, "_blob_store", FileSystemBlobStore(root=tmp_path) ) response = await client.get("/blobs/no-blobs-session") @@ -299,7 +299,7 @@ async def test_list_blobs_returns_correct_uris_for_existing_blobs( ) -> None: """GET /blobs/{session_id} returns 200 with correct ci-blob:// URIs for existing blobs.""" monkeypatch.setattr( - main_module.registry, "_blob_store", AsyncDiskBlobStore(root=tmp_path) + main_module.registry, "_blob_store", FileSystemBlobStore(root=tmp_path) ) session_id = "blob-list-session" @@ -325,7 +325,7 @@ async def test_get_blob_returns_200_with_content( ) -> None: """GET /blobs/{session_id}/{key} returns 200 with blob content for existing blob.""" monkeypatch.setattr( - main_module.registry, "_blob_store", AsyncDiskBlobStore(root=tmp_path) + main_module.registry, "_blob_store", FileSystemBlobStore(root=tmp_path) ) session_id = "test-session" @@ -348,7 +348,7 @@ async def test_get_blob_returns_404_for_missing_blob( ) -> None: """GET /blobs/{session_id}/{key} returns 404 with 'not found' in detail for missing blob.""" monkeypatch.setattr( - main_module.registry, "_blob_store", AsyncDiskBlobStore(root=tmp_path) + main_module.registry, "_blob_store", FileSystemBlobStore(root=tmp_path) ) response = await client.get("/blobs/missing-session/missing-key") diff --git a/tests/test_pipeline.py b/tests/test_pipeline.py index 5d0728f9..36c97d97 100644 --- a/tests/test_pipeline.py +++ b/tests/test_pipeline.py @@ -578,7 +578,7 @@ async def test_blob_skip_missing_timestamp_logs_warning( # the Event node id. Two distinct same-session, same-event, same-millisecond # events with DIFFERENT tool_call_id (e.g. parallel tool calls) therefore # minted the SAME blob key, and the second write silently clobbered the -# first via AsyncDiskBlobStore's os.replace -- while the first Event node's +# first via FileSystemBlobStore's os.replace -- while the first Event node's # $blob_ref still pointed at that (now-overwritten) URI. # =========================================================================== @@ -590,10 +590,10 @@ async def test_parallel_same_millisecond_events_do_not_collide_on_blob_key( """Two events sharing session_id + event name + timestamp (same epoch-ms) but with DIFFERENT tool_call_id must mint DISTINCT ci-blob:// URIs, and each blob must read back its own payload -- no silent overwrite.""" - from context_intelligence_server.blob_store import AsyncDiskBlobStore + from context_intelligence_server.blob_store import FileSystemBlobStore from context_intelligence_server.pipeline import process_event - blob_store = AsyncDiskBlobStore(root=tmp_path) + blob_store = FileSystemBlobStore(root=tmp_path) worker = MagicMock() worker.services.ensure_session_node = AsyncMock() diff --git a/tests/test_registry.py b/tests/test_registry.py index c31efeb4..44390a0a 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -15,7 +15,7 @@ import pytest import context_intelligence_server.registry as registry_module -from context_intelligence_server.blob_store import AsyncDiskBlobStore +from context_intelligence_server.blob_store import FileSystemBlobStore from context_intelligence_server.config import get_settings from context_intelligence_server.queue_manager import QueueManager from context_intelligence_server.registry import ( @@ -245,13 +245,13 @@ async def test_worker_has_workspace_attribute( assert worker.workspace == workspace @pytest.mark.asyncio - async def test_worker_services_blob_store_is_async_disk_blob_store( + async def test_worker_services_blob_store_is_filesystem_blob_store( self, registry: SessionRegistry ) -> None: - """worker.services.blob_store is an AsyncDiskBlobStore instance.""" + """worker.services.blob_store is a FileSystemBlobStore instance.""" worker = registry.get_or_create("session-1", "/workspace/test") - assert isinstance(worker.services.blob_store, AsyncDiskBlobStore) + assert isinstance(worker.services.blob_store, FileSystemBlobStore) @pytest.mark.asyncio async def test_worker_services_blob_store_root_matches_settings_blob_path( @@ -262,7 +262,7 @@ async def test_worker_services_blob_store_root_matches_settings_blob_path( settings = get_settings() blob_store = worker.services.blob_store - assert isinstance(blob_store, AsyncDiskBlobStore) + assert isinstance(blob_store, FileSystemBlobStore) assert blob_store._root == Path(settings.blob_path) @@ -1988,7 +1988,7 @@ def test_get_or_create_accepts_created_by_kwarg(self) -> None: with ( patch("context_intelligence_server.registry.Neo4jGraphStore") as MockStore, patch( - "context_intelligence_server.registry.AsyncDiskBlobStore" + "context_intelligence_server.registry.create_blob_store" ) as MockBlob, patch( "context_intelligence_server.registry.HookStateService" @@ -2019,7 +2019,7 @@ def test_get_or_create_default_created_by_is_none(self) -> None: with ( patch("context_intelligence_server.registry.Neo4jGraphStore") as MockStore, patch( - "context_intelligence_server.registry.AsyncDiskBlobStore" + "context_intelligence_server.registry.create_blob_store" ) as MockBlob, patch( "context_intelligence_server.registry.HookStateService" @@ -2045,7 +2045,7 @@ class TestSessionOwnershipInvariant: - log nothing at ERROR when the same (or None) created_by arrives; - log an ERROR and preserve the bound id when a different created_by arrives. - Mocking strategy: patch Neo4jGraphStore / AsyncDiskBlobStore / HookStateService + Mocking strategy: patch Neo4jGraphStore / create_blob_store / HookStateService exactly as TestGetOrCreateCreatedBy does. After the first get_or_create call (which stores a worker whose .services is the mock_svc MagicMock), we manually set mock_svc.graph.created_by = "alice" to simulate the bound state — the real @@ -2069,7 +2069,7 @@ def test_get_or_create_reuse_keeps_bound_created_by( "context_intelligence_server.registry.Neo4jGraphStore" ) as MockStore, patch( - "context_intelligence_server.registry.AsyncDiskBlobStore" + "context_intelligence_server.registry.create_blob_store" ) as MockBlob, patch( "context_intelligence_server.registry.HookStateService" @@ -2111,7 +2111,7 @@ def test_get_or_create_reuse_ignores_new_created_by_no_error( "context_intelligence_server.registry.Neo4jGraphStore" ) as MockStore, patch( - "context_intelligence_server.registry.AsyncDiskBlobStore" + "context_intelligence_server.registry.create_blob_store" ) as MockBlob, patch( "context_intelligence_server.registry.HookStateService" @@ -2152,7 +2152,7 @@ def test_invariant_violation_is_observed_and_not_overwritten( "context_intelligence_server.registry.Neo4jGraphStore" ) as MockStore, patch( - "context_intelligence_server.registry.AsyncDiskBlobStore" + "context_intelligence_server.registry.create_blob_store" ) as MockBlob, patch( "context_intelligence_server.registry.HookStateService" From ab4830cdb97a24a91daf76340b61b5dba41d24f7 Mon Sep 17 00:00:00 2001 From: colombod Date: Mon, 17 Aug 2026 19:15:12 +0000 Subject: [PATCH 4/5] refactor(identity): protocol-ize identity store behind a config-driven factory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Split context_intelligence_server/identity_store.py into a package: - protocol.py: IdentityStore runtime_checkable Protocol with load/put/delete/seed/get/items/__len__/exists + flat_dict - filesystem.py: FileSystemIdentityStore (renamed from concrete IdentityStore) - factory.py: create_identity_store(settings, kind) -> IdentityStore (single backend-selection seam) - __init__.py: re-exports Protocol + factory - Consumers (main.py, routers/admin.py, tests) now obtain instances via factory and type against Protocol - entra_identities_store_path and api_keys_store_path now read only by factory.py + config.py - Reworded 3 startup diagnostic log messages to reference config settings instead of raw paths - Added tests/test_identity_isolation_tripwire.py (non-vacuous enforcement) - Suite: 1875 passed, 4 skipped 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- .../identity_store/__init__.py | 29 +++++ .../identity_store/factory.py | 49 ++++++++ .../filesystem.py} | 100 ++++++--------- .../identity_store/protocol.py | 117 ++++++++++++++++++ context_intelligence_server/main.py | 15 ++- context_intelligence_server/routers/admin.py | 2 +- tests/test_identity_isolation_tripwire.py | 91 ++++++++++++++ tests/test_identity_store.py | 48 +++---- 8 files changed, 359 insertions(+), 92 deletions(-) create mode 100644 context_intelligence_server/identity_store/__init__.py create mode 100644 context_intelligence_server/identity_store/factory.py rename context_intelligence_server/{identity_store.py => identity_store/filesystem.py} (73%) create mode 100644 context_intelligence_server/identity_store/protocol.py create mode 100644 tests/test_identity_isolation_tripwire.py diff --git a/context_intelligence_server/identity_store/__init__.py b/context_intelligence_server/identity_store/__init__.py new file mode 100644 index 00000000..1cf81b75 --- /dev/null +++ b/context_intelligence_server/identity_store/__init__.py @@ -0,0 +1,29 @@ +"""identity_store -- durable, write-through key -> contributor-identity map. + +The public surface is the backend-neutral :class:`IdentityStore` Protocol plus +the :func:`create_identity_store` factory. Consumers should depend on these, +never on a concrete backend class. + +Package layout: + protocol.py IdentityStore Protocol -- the backend-neutral seam (no + filesystem imports). AUTH-CRITICAL commit-order and + fail-closed-load guarantees are documented here. + filesystem.py FileSystemIdentityStore -- the JSON-file-backed + implementation. + factory.py create_identity_store(settings, kind) -- the ONLY place a + backend is selected and the ONLY place (besides config.py) + that reads settings.entra_identities_store_path / + settings.api_keys_store_path. +""" + +from __future__ import annotations + +from .factory import create_identity_store +from .filesystem import FileSystemIdentityStore +from .protocol import IdentityStore + +__all__ = [ + "FileSystemIdentityStore", + "IdentityStore", + "create_identity_store", +] diff --git a/context_intelligence_server/identity_store/factory.py b/context_intelligence_server/identity_store/factory.py new file mode 100644 index 00000000..e2d64c66 --- /dev/null +++ b/context_intelligence_server/identity_store/factory.py @@ -0,0 +1,49 @@ +"""Config-driven IdentityStore factory -- the ONLY place a backend is selected. + +This is the single seam through which the concrete backend is chosen. Adding +a new backend (e.g. Azure) means: one new module implementing +:class:`~.protocol.IdentityStore`, one new branch here -- zero changes to +:mod:`context_intelligence_server.main` or any other consumer. + +This module (and :mod:`~context_intelligence_server.config`) are the only +places ``settings.entra_identities_store_path`` / ``settings.api_keys_store_path`` +are read -- the on-disk location is a filesystem-backend concern, resolved +here and handed to the concrete backend at construction time. Callers only +ever see the :class:`~.protocol.IdentityStore` Protocol. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import TYPE_CHECKING + +from .filesystem import FileSystemIdentityStore +from .protocol import IdentityStore + +if TYPE_CHECKING: + from context_intelligence_server.config import Settings + + +def create_identity_store(settings: Settings, kind: str) -> IdentityStore: + """Build an :class:`~.protocol.IdentityStore` rooted at the configured path for *kind*. + + Construction only -- callers are responsible for ``load()`` / ``seed()`` + and any auth-mode wiring (this mirrors + ``blob_store.factory.create_blob_store``: a single-backend, config-reading + seam that keeps the store paths out of consumers such as ``main.py``). + + Args: + settings: The active ``Settings``. + kind: ``"entra"`` for the OID identity map, ``"api_key"`` for the + SHA-256 digest keystore. + + Raises: + ValueError: If *kind* is neither ``"entra"`` nor ``"api_key"``. + """ + if kind == "entra": + path = settings.entra_identities_store_path + elif kind == "api_key": + path = settings.api_keys_store_path + else: + raise ValueError(f"Unknown identity store kind: {kind!r}") + return FileSystemIdentityStore(Path(path)) diff --git a/context_intelligence_server/identity_store.py b/context_intelligence_server/identity_store/filesystem.py similarity index 73% rename from context_intelligence_server/identity_store.py rename to context_intelligence_server/identity_store/filesystem.py index 7b230af3..c18d92d6 100644 --- a/context_intelligence_server/identity_store.py +++ b/context_intelligence_server/identity_store/filesystem.py @@ -1,19 +1,29 @@ -"""Durable identity-map store for the Context Intelligence Server. +"""FileSystemIdentityStore -- disk-backed identity map with atomic writes. -Each ``IdentityStore`` wraps ONE JSON file and keeps an in-process dict -(``_data``) that IS the live source of truth for the single-replica process. -A second derived dict, ``flat_dict``, exposes ``{key: contributor_id}`` and is -kept in-sync with ``_data`` via in-place mutations so that any object holding a -reference to ``flat_dict`` always sees the latest state without a restart. +Each ``FileSystemIdentityStore`` wraps ONE JSON file and keeps an in-process +dict (``_data``) that IS the live source of truth for the single-replica +process. A second derived dict, ``flat_dict``, exposes ``{key: contributor_id}`` +and is kept in-sync with ``_data`` via in-place mutations so that any object +holding a reference to ``flat_dict`` always sees the latest state without a +restart. -**Commit order (ROB F2 — NON-NEGOTIABLE)** +This is a concrete implementation of the +:class:`~context_intelligence_server.identity_store.protocol.IdentityStore` +Protocol. No ``Path``, on-disk layout, or ``os.*`` detail appears in the +Protocol or in any value it returns -- those details are private to this +class (and, later, an Azure equivalent). + +**Commit order (ROB F2 -- NON-NEGOTIABLE)** On every mutation (put / delete): 1. Build the new data dict (do NOT touch ``_data`` yet). -2. Serialize and write to a tempfile **in the same directory** as the target file. -3. ``os.replace()`` the tempfile onto the target (atomic rename on POSIX / Azure Files). -4. **ONLY IF the above succeeds**: update ``_data`` and ``flat_dict`` in-place. +2. Serialize and write to a tempfile **in the same directory** as the target + file. +3. ``os.replace()`` the tempfile onto the target (atomic rename on POSIX / + Azure Files). +4. **ONLY IF the above succeeds**: update ``_data`` and ``flat_dict`` + in-place. If the file write raises for any reason, ``_data`` and ``flat_dict`` are **unchanged** and the exception propagates to the caller (who returns 5xx). @@ -23,13 +33,13 @@ On ``load()``: -- Missing file → empty dict (normal first boot). No log, no raise. -- Corrupt / torn / partial / invalid-JSON file → **empty dict + a LOUD - ``logger.error`` / ``logger.critical``**. The server MUST NOT crash-loop on - a bad store file. An empty map means "nobody is bound yet" — every auth +- Missing file -> empty dict (normal first boot). No log, no raise. +- Corrupt / torn / partial / invalid-JSON file -> **empty dict + a LOUD + ``logger.error`` / ``logger.critical``**. The server MUST NOT crash-loop on + a bad store file. An empty map means "nobody is bound yet" -- every auth attempt then fails normally until an admin re-populates via the /admin API. -File format (both modes share the same abstraction):: +File format (both modes share the same on-disk shape):: # api-keys.json { @@ -49,21 +59,18 @@ import os import tempfile from pathlib import Path -from typing import TYPE_CHECKING - -if TYPE_CHECKING: - from context_intelligence_server.config import Settings logger = logging.getLogger(__name__) -class IdentityStore: +class FileSystemIdentityStore: """Durable, write-through identity map backed by a single JSON file. - See module docstring for the commit-order contract and fail-closed guarantees. + See module docstring for the commit-order contract and fail-closed + guarantees. Args: - path: Absolute path to the JSON store file. The parent directory is + path: Absolute path to the JSON store file. The parent directory is created automatically on the first write. """ @@ -83,11 +90,11 @@ def __init__(self, path: Path) -> None: def load(self) -> None: """Read the file and populate the in-process map. - Missing file → empty dict (normal first boot, no log). - Corrupt / non-dict → empty dict + LOUD error log, never raise. + Missing file -> empty dict (normal first boot, no log). + Corrupt / non-dict -> empty dict + LOUD error log, never raise. """ if not self._path.exists(): - # Normal first boot — the file hasn't been written yet. + # Normal first boot -- the file hasn't been written yet. self._data = {} self._rebuild_flat() return @@ -97,7 +104,7 @@ def load(self) -> None: raw = json.loads(self._path.read_text(encoding="utf-8")) except (json.JSONDecodeError, UnicodeDecodeError, OSError) as exc: logger.error( - "identity_store.load CORRUPT FILE path=%s error=%r — " + "identity_store.load CORRUPT FILE path=%s error=%r -- " "failing CLOSED to empty map. Re-populate via /admin API.", self._path, exc, @@ -108,7 +115,7 @@ def load(self) -> None: if not isinstance(raw, dict): logger.critical( - "identity_store.load INVALID FORMAT path=%s got=%r — " + "identity_store.load INVALID FORMAT path=%s got=%r -- " "expected a JSON object at top level. Failing CLOSED to empty map.", self._path, type(raw).__name__, @@ -124,15 +131,15 @@ def load(self) -> None: self._rebuild_flat() def put(self, key: str, value: dict[str, str]) -> None: - """Upsert *key* → *value*. + """Upsert *key* -> *value*. - Commit order (F2): write tempfile → os.replace → update in-process. + Commit order (F2): write tempfile -> os.replace -> update in-process. Raises on file-write failure; in-process state is UNCHANGED. """ new_data = dict(self._data) new_data[key] = value self._write_atomic(new_data) - # File write succeeded — now update in-process state. + # File write succeeded -- now update in-process state. self._data[key] = value contributor_id = value.get("id", "") if contributor_id: @@ -143,13 +150,13 @@ def put(self, key: str, value: dict[str, str]) -> None: def delete(self, key: str) -> None: """Remove *key* from the store. - Commit order (F2): write tempfile → os.replace → update in-process. + Commit order (F2): write tempfile -> os.replace -> update in-process. Raises on file-write failure; in-process state is UNCHANGED. No-op if *key* is not present. """ new_data = {k: v for k, v in self._data.items() if k != key} self._write_atomic(new_data) - # File write succeeded — now update in-process state. + # File write succeeded -- now update in-process state. self._data.pop(key, None) self.flat_dict.pop(key, None) @@ -172,12 +179,12 @@ def seed(self, data: dict[str, dict[str, str]]) -> None: except Exception as exc: logger.warning( "identity_store.seed: could not write seed to %s: %r " - "— in-memory map is live but the file is not yet persisted. " + "-- in-memory map is live but the file is not yet persisted. " "The next mutation via /admin API will persist the file.", self._path, exc, ) - # Update in-memory regardless — data is from durable config. + # Update in-memory regardless -- data is from durable config. self._data = dict(data) self._rebuild_flat() @@ -242,28 +249,3 @@ def _write_atomic(self, data: dict[str, dict[str, str]]) -> None: except Exception: pass raise - - -def create_identity_store(settings: Settings, kind: str) -> IdentityStore: - """Build an ``IdentityStore`` rooted at the configured path for *kind*. - - Construction only \u2014 callers are responsible for ``load()``/``seed()`` - and any auth-mode wiring (this mirrors ``blob_store.factory.create_blob_store``: - a single-backend, config-reading seam that keeps the store paths out of - consumers such as ``main.py``). - - Args: - settings: The active ``Settings``. - kind: ``"entra"`` for the OID identity map, ``"api_key"`` for the - SHA-256 digest keystore. - - Raises: - ValueError: If *kind* is neither ``"entra"`` nor ``"api_key"``. - """ - if kind == "entra": - path = settings.entra_identities_store_path - elif kind == "api_key": - path = settings.api_keys_store_path - else: - raise ValueError(f"Unknown identity store kind: {kind!r}") - return IdentityStore(Path(path)) diff --git a/context_intelligence_server/identity_store/protocol.py b/context_intelligence_server/identity_store/protocol.py new file mode 100644 index 00000000..3182eb69 --- /dev/null +++ b/context_intelligence_server/identity_store/protocol.py @@ -0,0 +1,117 @@ +"""IdentityStore Protocol -- the backend-neutral seam. + +Each ``IdentityStore`` wraps a durable ``key -> {"id": contributor_id, ...}`` +map and keeps an in-process, live-mutated view of it (:attr:`flat_dict`) so +that any object holding a reference (e.g. an auth resolver's keystore) always +sees the latest state without a restart. No ``Path``, on-disk layout, or +``os.*`` detail appears here or in any value the Protocol returns -- that is +private to a concrete backend +(:class:`~context_intelligence_server.identity_store.filesystem.FileSystemIdentityStore` +and, later, an Azure equivalent). + +This is AUTH-CRITICAL surface. The following guarantees are part of the +Protocol's contract and every backend MUST uphold them: + +**Commit order (ROB F2 -- NON-NEGOTIABLE)** + +On every mutation (``put`` / ``delete``): + +1. The durable store is written FIRST (whatever "durable" means for the + backend -- a file, a blob, etc.). +2. **ONLY IF** that write succeeds does in-process state (``flat_dict`` and + any internal map) get updated, IN-PLACE, so existing references to + ``flat_dict`` observe the change immediately. +3. If the durable write fails, in-process state is **UNCHANGED** and the + exception propagates to the caller (who returns 5xx). The durable store + and in-process memory are never out of sync. + +**Fail-CLOSED load()** + +On ``load()``: + +- Missing / never-persisted store -> empty map (normal first boot). No log, + no raise. +- Corrupt / unreadable store -> empty map + a LOUD error log. The server + MUST NOT crash-loop on a bad store. An empty map means "nobody is bound + yet" -- every auth attempt then fails normally until an admin re-populates + via the /admin API. + +**flat_dict is a live, shared view** + +``flat_dict`` is ``{key: contributor_id}``, mutated IN-PLACE on every +``put`` / ``delete`` / ``seed`` / ``load`` so any object holding a reference +to it always sees the latest state. + +This module is backend-neutral by construction: it imports nothing from +``os``, ``pathlib``, or any filesystem library. +""" + +from __future__ import annotations + +from collections.abc import ItemsView +from typing import Protocol, runtime_checkable + + +@runtime_checkable +class IdentityStore(Protocol): + """Protocol for a durable, write-through identity map. + + 100% backend-neutral: no ``Path``, no on-disk layout, no ``os.*`` -- + ever. + """ + + flat_dict: dict[str, str] + """Live derived view: ``{key: contributor_id}``. Shared BY REFERENCE with + consumers (e.g. auth resolvers) so mutations are visible with no restart.""" + + def load(self) -> None: + """Read the durable store and populate the in-process map. + + Fail-closed: missing store -> empty map (silent, normal first boot); + corrupt store -> empty map + a loud error log. Never raises. + """ + ... + + def put(self, key: str, value: dict[str, str]) -> None: + """Upsert ``key`` -> ``value``. + + Commit order (F2): durable write -> in-process update. Raises on + durable-write failure; in-process state is left unchanged. + """ + ... + + def delete(self, key: str) -> None: + """Remove ``key`` from the store. + + Commit order (F2): durable write -> in-process update. Raises on + durable-write failure; in-process state is left unchanged. No-op if + ``key`` is not present. + """ + ... + + def seed(self, data: dict[str, dict[str, str]]) -> None: + """Bulk-seed from config on first boot. + + Unlike ``put()`` (which enforces F2 write-before-memory strictly), + in-process state is updated even if the durable write fails -- the + data came from durable config, so memory-ahead-of-durable-store is + safe (a restart re-seeds from config again). A warning is logged if + the durable write fails. + """ + ... + + def get(self, key: str) -> dict[str, str] | None: + """Return the value for ``key``, or ``None`` if not present.""" + ... + + def items(self) -> ItemsView[str, dict[str, str]]: + """Iterate over ``(key, value)`` pairs in the store.""" + ... + + def __len__(self) -> int: + """Return the number of entries currently in the store.""" + ... + + def exists(self) -> bool: + """Whether the store has ever been persisted to its backing store.""" + ... diff --git a/context_intelligence_server/main.py b/context_intelligence_server/main.py index 49a3ae2d..9ea6e379 100644 --- a/context_intelligence_server/main.py +++ b/context_intelligence_server/main.py @@ -34,7 +34,7 @@ from context_intelligence_server.identity_store import ( IdentityStore, create_identity_store, -) +) # IdentityStore here is the Protocol (see identity_store/protocol.py) from context_intelligence_server.logging_config import setup_logging from context_intelligence_server.models import ( CypherRequest, @@ -605,8 +605,8 @@ def create_asgi_app( "is UP and serving, but every delegated (human) token will " "receive 403 until identities are onboarded. Bind the first user " "with an IdentityAdmin-role token via PUT /admin/identities/{oid} " - "(store=%s). This is expected on a fresh /data volume.", - s.entra_identities_store_path, + "(see the configured entra identities store-path setting). This " + "is expected on a fresh /data volume." ) # B4: boot disjointness invariant — each oid must belong to exactly one @@ -667,9 +667,8 @@ def create_asgi_app( "static keystore is EMPTY at startup (0 bound keys) — server " "is UP but fail-CLOSED; every request will 401 until keys are " "onboarded. Add the first key with the admin token via " - "PUT /admin/keys/{sha256hash} (store=%s). Expected on a fresh " - "/data volume.", - s.api_keys_store_path, + "PUT /admin/keys/{sha256hash} (see the configured " + "api keys store-path setting). Expected on a fresh /data volume." ) else: logger.warning( @@ -679,8 +678,8 @@ def create_asgi_app( "/admin API is unreachable without an admin key: every token " "401s at the middleware before require_admin runs). Set " "admin_api_key/admin_api_key_sha256 to enable runtime " - "onboarding, or add api_keys in config and restart. (store=%s)", - s.api_keys_store_path, + "onboarding, or add api_keys in config and restart. (see the " + "configured api keys store-path setting)" ) # Pass key_store.flat_dict (the LIVE dict) so the resolver sees any diff --git a/context_intelligence_server/routers/admin.py b/context_intelligence_server/routers/admin.py index 4606a303..57bbbf67 100644 --- a/context_intelligence_server/routers/admin.py +++ b/context_intelligence_server/routers/admin.py @@ -40,7 +40,7 @@ from pydantic import BaseModel, field_validator from context_intelligence_server.config import _ALL_ZEROS_GUID, _GUID_RE -from context_intelligence_server.identity_store import IdentityStore +from context_intelligence_server.identity_store import IdentityStore # Protocol # --------------------------------------------------------------------------- # Validation constants diff --git a/tests/test_identity_isolation_tripwire.py b/tests/test_identity_isolation_tripwire.py new file mode 100644 index 00000000..79636e90 --- /dev/null +++ b/tests/test_identity_isolation_tripwire.py @@ -0,0 +1,91 @@ +"""Tripwire: identity-store on-disk location stays inside the identity_store package. + +Locks the isolation boundary established by the IdentityStore refactor +(mirrors ``tests/test_blob_isolation_tripwire.py`` for the blob store). These +tests fail loudly if a future change lets any module other than the +``identity_store/`` package locate an identity-store's backing file, or +resurrects a concrete-class reference at a consumer site -- i.e. if a +direct-FS identity leak is reintroduced. + +Two invariants: + 1. Only the ``identity_store/`` package (specifically ``factory.py``, the + single construction site) and the config field declarations (config.py) + may reference ``settings.entra_identities_store_path`` / + ``settings.api_keys_store_path`` -- a caller that cannot locate the + backing path physically cannot do identity-store filesystem I/O + directly. ``main.py`` and ``routers/admin.py`` go through + ``create_identity_store(settings, kind)`` and never see the paths. + 2. The concrete ``FileSystemIdentityStore`` must appear ONLY inside the + ``identity_store/`` package (production consumers depend on the + ``IdentityStore`` Protocol and obtain instances via + ``create_identity_store(settings, kind)``). Tests are permitted to + construct a concrete backend directly. +""" + +from __future__ import annotations + +import pathlib + +PKG = pathlib.Path(__file__).resolve().parents[1] / "context_intelligence_server" + + +def _code_lines(path: pathlib.Path): + """Yield (lineno, stripped) for real code lines, skipping comments and + rst-doc lines (``...`` backtick spans) so docstring prose never trips the + guard.""" + for i, raw in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + s = raw.strip() + if not s or s.startswith("#") or "``" in raw: + continue + yield i, s + + +def test_identity_store_paths_locatable_only_at_construction_site() -> None: + """`settings.entra_identities_store_path` / `settings.api_keys_store_path` -- + the only way to find an identity store's backing file -- are referenced + solely inside the identity_store/ package (which owns the on-disk layout) + and where the config fields are declared (config.py). main.py and + routers/admin.py go through create_identity_store(settings, kind) and + never see the paths themselves.""" + identity_store_pkg = PKG / "identity_store" + allowed_top_level = {"config.py"} + banned = ("entra_identities_store_path", "api_keys_store_path") + offenders: list[str] = [] + for p in PKG.rglob("*.py"): + # Anything inside the identity_store/ package owns the layout -- allowed. + if identity_store_pkg in p.parents: + continue + if p.name in allowed_top_level: + continue + for lineno, line in _code_lines(p): + for token in banned: + if token in line: + offenders.append(f"{p.relative_to(PKG)}:{lineno}: {line}") + assert not offenders, ( + "identity-store backing path re-derived outside the identity_store/ " + "package/config -- a caller that can locate the path can bypass " + "IdentityStore and touch disk directly:\n" + "\n".join(offenders) + ) + + +def test_concrete_impl_referenced_only_inside_the_package() -> None: + """The concrete ``FileSystemIdentityStore`` must appear ONLY inside the + ``identity_store/`` package. Production consumers depend on the + ``IdentityStore`` Protocol and obtain instances via + ``create_identity_store(settings, kind)`` -- never by naming a concrete + backend -- so a disk->Azure swap touches only the package. (Tests are + permitted to construct a concrete backend directly.)""" + identity_store_pkg = PKG / "identity_store" + offenders: list[str] = [] + for p in PKG.rglob("*.py"): + if identity_store_pkg in p.parents: + continue + for lineno, line in _code_lines(p): + if "FileSystemIdentityStore" in line: + offenders.append(f"{p.relative_to(PKG)}:{lineno}: {line}") + assert not offenders, ( + "concrete FileSystemIdentityStore named outside the identity_store/ " + "package -- consumers must use the IdentityStore Protocol + " + "create_identity_store(settings, kind), not a concrete backend:\n" + + "\n".join(offenders) + ) diff --git a/tests/test_identity_store.py b/tests/test_identity_store.py index 61733c91..bc8cd6e9 100644 --- a/tests/test_identity_store.py +++ b/tests/test_identity_store.py @@ -14,7 +14,7 @@ import pytest -from context_intelligence_server.identity_store import IdentityStore +from context_intelligence_server.identity_store import FileSystemIdentityStore # --------------------------------------------------------------------------- @@ -42,7 +42,7 @@ def _bob_entry() -> dict[str, str]: class TestPutGetRoundtrip: def test_put_then_get_returns_value(self, tmp_path: Path) -> None: """put(key, value) → get(key) returns that value immediately.""" - store = IdentityStore(path=tmp_path / "store.json") + store = FileSystemIdentityStore(path=tmp_path / "store.json") store.load() store.put(FAKE_HASH_A, _alice_entry()) @@ -53,25 +53,25 @@ def test_put_then_get_returns_value(self, tmp_path: Path) -> None: def test_put_persists_to_file_and_loads_fresh(self, tmp_path: Path) -> None: """Round-trip: put → create new store → load → value is present.""" store_path = tmp_path / "store.json" - store = IdentityStore(path=store_path) + store = FileSystemIdentityStore(path=store_path) store.load() store.put(FAKE_HASH_A, _alice_entry()) # New store instance reads from disk - store2 = IdentityStore(path=store_path) + store2 = FileSystemIdentityStore(path=store_path) store2.load() assert store2.get(FAKE_HASH_A) == _alice_entry() def test_get_missing_key_returns_none(self, tmp_path: Path) -> None: """get() returns None for a key that was never put.""" - store = IdentityStore(path=tmp_path / "store.json") + store = FileSystemIdentityStore(path=tmp_path / "store.json") store.load() assert store.get(FAKE_HASH_A) is None def test_delete_removes_key(self, tmp_path: Path) -> None: """delete(key) removes the entry from in-process dict AND file.""" store_path = tmp_path / "store.json" - store = IdentityStore(path=store_path) + store = FileSystemIdentityStore(path=store_path) store.load() store.put(FAKE_HASH_A, _alice_entry()) store.delete(FAKE_HASH_A) @@ -79,13 +79,13 @@ def test_delete_removes_key(self, tmp_path: Path) -> None: assert store.get(FAKE_HASH_A) is None # Verify file is also updated - store2 = IdentityStore(path=store_path) + store2 = FileSystemIdentityStore(path=store_path) store2.load() assert store2.get(FAKE_HASH_A) is None def test_items_returns_all_entries(self, tmp_path: Path) -> None: """items() yields all key-value pairs currently in the store.""" - store = IdentityStore(path=tmp_path / "store.json") + store = FileSystemIdentityStore(path=tmp_path / "store.json") store.load() store.put(FAKE_HASH_A, _alice_entry()) store.put(FAKE_HASH_B, _bob_entry()) @@ -95,7 +95,7 @@ def test_items_returns_all_entries(self, tmp_path: Path) -> None: def test_upsert_overwrites_existing(self, tmp_path: Path) -> None: """put() on an existing key overwrites the value.""" - store = IdentityStore(path=tmp_path / "store.json") + store = FileSystemIdentityStore(path=tmp_path / "store.json") store.load() store.put(FAKE_HASH_A, _alice_entry()) store.put(FAKE_HASH_A, {"id": "alice-updated"}) @@ -105,12 +105,12 @@ def test_upsert_overwrites_existing(self, tmp_path: Path) -> None: def test_sequential_puts_all_persist(self, tmp_path: Path) -> None: """Multiple sequential puts all persist correctly (each write is the full map).""" store_path = tmp_path / "store.json" - store = IdentityStore(path=store_path) + store = FileSystemIdentityStore(path=store_path) store.load() store.put(FAKE_HASH_A, _alice_entry()) store.put(FAKE_HASH_B, _bob_entry()) - store2 = IdentityStore(path=store_path) + store2 = FileSystemIdentityStore(path=store_path) store2.load() assert store2.get(FAKE_HASH_A) == _alice_entry() assert store2.get(FAKE_HASH_B) == _bob_entry() @@ -124,7 +124,7 @@ def test_sequential_puts_all_persist(self, tmp_path: Path) -> None: class TestLoadFailClosed: def test_missing_file_yields_empty_dict(self, tmp_path: Path) -> None: """Missing store file → load() yields empty dict (normal first boot), no raise.""" - store = IdentityStore(path=tmp_path / "nonexistent.json") + store = FileSystemIdentityStore(path=tmp_path / "nonexistent.json") store.load() # must not raise assert store.get(FAKE_HASH_A) is None assert list(store.items()) == [] @@ -136,7 +136,7 @@ def test_corrupt_json_yields_empty_dict_and_logs_error( store_path = tmp_path / "store.json" store_path.write_text("{{{{not valid json at all}}}}", encoding="utf-8") - store = IdentityStore(path=store_path) + store = FileSystemIdentityStore(path=store_path) with caplog.at_level(logging.ERROR): store.load() # must NOT raise @@ -156,7 +156,7 @@ def test_valid_json_but_not_dict_yields_empty_and_logs( json.dumps([{"id": "alice"}]), encoding="utf-8" ) # list, not dict - store = IdentityStore(path=store_path) + store = FileSystemIdentityStore(path=store_path) with caplog.at_level(logging.ERROR): store.load() # must NOT raise @@ -171,7 +171,7 @@ def test_partial_write_torn_file_loads_empty( store_path = tmp_path / "store.json" store_path.write_bytes(b'{"aaa": {"id": "al') # truncated mid-write - store = IdentityStore(path=store_path) + store = FileSystemIdentityStore(path=store_path) with caplog.at_level(logging.ERROR): store.load() # must NOT raise @@ -188,7 +188,7 @@ def test_partial_write_torn_file_loads_empty( class TestWriteFileThenSwapMemory: def test_put_write_failure_leaves_dict_unchanged(self, tmp_path: Path) -> None: """If os.replace raises, the in-process dict is UNCHANGED (F2 contract).""" - store = IdentityStore(path=tmp_path / "store.json") + store = FileSystemIdentityStore(path=tmp_path / "store.json") store.load() # Establish an existing entry store.put(FAKE_HASH_A, _alice_entry()) @@ -208,7 +208,7 @@ def test_put_write_failure_leaves_dict_unchanged(self, tmp_path: Path) -> None: def test_delete_write_failure_leaves_dict_unchanged(self, tmp_path: Path) -> None: """If delete's file write fails, the in-process dict is UNCHANGED.""" - store = IdentityStore(path=tmp_path / "store.json") + store = FileSystemIdentityStore(path=tmp_path / "store.json") store.load() store.put(FAKE_HASH_A, _alice_entry()) @@ -221,7 +221,7 @@ def test_delete_write_failure_leaves_dict_unchanged(self, tmp_path: Path) -> Non def test_failed_write_leaves_no_torn_tempfile(self, tmp_path: Path) -> None: """A failed os.replace must clean up the tempfile — no orphaned .tmp files.""" - store = IdentityStore(path=tmp_path / "store.json") + store = FileSystemIdentityStore(path=tmp_path / "store.json") store.load() with patch("os.replace", side_effect=OSError("simulated disk full")): @@ -235,7 +235,7 @@ def test_failed_write_leaves_no_torn_tempfile(self, tmp_path: Path) -> None: def test_atomic_write_uses_tempfile_in_same_dir(self, tmp_path: Path) -> None: """Writes use a temp file in the same directory (then os.replace).""" store_path = tmp_path / "store.json" - store = IdentityStore(path=store_path) + store = FileSystemIdentityStore(path=store_path) store.load() replaced_from: list[str] = [] @@ -266,13 +266,13 @@ class TestFlatDictLiveReference: def test_flat_dict_empty_on_new_store(self, tmp_path: Path) -> None: """flat_dict is empty after load() with no file.""" - store = IdentityStore(path=tmp_path / "store.json") + store = FileSystemIdentityStore(path=tmp_path / "store.json") store.load() assert store.flat_dict == {} def test_flat_dict_updated_after_put(self, tmp_path: Path) -> None: """flat_dict is updated immediately after put().""" - store = IdentityStore(path=tmp_path / "store.json") + store = FileSystemIdentityStore(path=tmp_path / "store.json") store.load() store.put(FAKE_HASH_A, {"id": "alice"}) @@ -280,7 +280,7 @@ def test_flat_dict_updated_after_put(self, tmp_path: Path) -> None: def test_flat_dict_updated_after_delete(self, tmp_path: Path) -> None: """flat_dict removes key immediately after delete().""" - store = IdentityStore(path=tmp_path / "store.json") + store = FileSystemIdentityStore(path=tmp_path / "store.json") store.load() store.put(FAKE_HASH_A, {"id": "alice"}) store.delete(FAKE_HASH_A) @@ -290,7 +290,7 @@ def test_flat_dict_updated_after_delete(self, tmp_path: Path) -> None: def test_flat_dict_is_same_object_across_puts(self, tmp_path: Path) -> None: """flat_dict is the SAME dict object before and after put() (so a shared reference to flat_dict stays live).""" - store = IdentityStore(path=tmp_path / "store.json") + store = FileSystemIdentityStore(path=tmp_path / "store.json") store.load() flat_ref = store.flat_dict # capture the reference @@ -309,7 +309,7 @@ def test_flat_dict_populated_from_file_on_load(self, tmp_path: Path) -> None: json.dumps({FAKE_HASH_A: {"id": "alice"}, FAKE_HASH_B: {"id": "bob"}}), encoding="utf-8", ) - store = IdentityStore(path=store_path) + store = FileSystemIdentityStore(path=store_path) store.load() assert store.flat_dict[FAKE_HASH_A] == "alice" From 7c0c29c2dcb75f0762a97b94c9fefa2ba9fbf8e5 Mon Sep 17 00:00:00 2001 From: colombod Date: Mon, 17 Aug 2026 20:41:30 +0000 Subject: [PATCH 5/5] refactor(queue): protocol-ize queue store behind a config-driven factory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Queue store is now obtained via create_queue_manager(settings) factory behind the QueueManager Protocol, matching blob and identity storages. The concrete FileSystemQueueManager is isolated to the queue_manager/ package and constructed only in factory.py, with no filesystem paths leaking to consumers. - Split context_intelligence_server/queue_manager.py into a package: - protocol.py: QueueManager runtime_checkable Protocol + Batch value type - filesystem.py: FileSystemQueueManager (renamed, behavior identical) - factory.py: create_queue_manager(settings) -> QueueManager (only reader of queues_path) - __init__.py: re-exports Protocol, concrete, factory - registry.py now types against QueueManager Protocol and builds via factory - Consumers never see FileSystemQueueManager or settings.queues_path - Non-vacuous tripwire added to catch isolation violations 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- .../queue_manager/__init__.py | 27 ++++ .../queue_manager/factory.py | 35 +++++ .../filesystem.py} | 48 ++----- .../queue_manager/protocol.py | 121 ++++++++++++++++++ tests/integration/test_crash_recovery.py | 6 +- tests/neo4j/test_concurrent_flush.py | 6 +- tests/neo4j/test_oom_regression.py | 4 +- tests/neo4j/test_orphan_visibility.py | 4 +- tests/neo4j/test_queues_actions.py | 4 +- tests/routers/test_queues.py | 6 +- tests/test_main.py | 12 +- tests/test_queue_isolation_tripwire.py | 88 +++++++++++++ tests/test_queue_manager.py | 20 +-- tests/test_registry.py | 4 +- 14 files changed, 318 insertions(+), 67 deletions(-) create mode 100644 context_intelligence_server/queue_manager/__init__.py create mode 100644 context_intelligence_server/queue_manager/factory.py rename context_intelligence_server/{queue_manager.py => queue_manager/filesystem.py} (95%) create mode 100644 context_intelligence_server/queue_manager/protocol.py create mode 100644 tests/test_queue_isolation_tripwire.py diff --git a/context_intelligence_server/queue_manager/__init__.py b/context_intelligence_server/queue_manager/__init__.py new file mode 100644 index 00000000..0de017af --- /dev/null +++ b/context_intelligence_server/queue_manager/__init__.py @@ -0,0 +1,27 @@ +"""queue_manager — durable, per-session append-only queue for the event-write pipeline. + +The public surface is the backend-neutral :class:`QueueManager` Protocol plus +:class:`Batch` and the :func:`create_queue_manager` factory. Consumers should +depend on these, never on a concrete backend class. + +Package layout: + protocol.py QueueManager Protocol, Batch — the backend-neutral seam (no + filesystem imports). + filesystem.py FileSystemQueueManager — the on-disk implementation. + factory.py create_queue_manager(settings) — the ONLY place a backend is + selected and the ONLY place (besides config.py) that reads + settings.queues_path. +""" + +from __future__ import annotations + +from .factory import create_queue_manager +from .filesystem import FileSystemQueueManager +from .protocol import Batch, QueueManager + +__all__ = [ + "Batch", + "FileSystemQueueManager", + "QueueManager", + "create_queue_manager", +] diff --git a/context_intelligence_server/queue_manager/factory.py b/context_intelligence_server/queue_manager/factory.py new file mode 100644 index 00000000..fadf960e --- /dev/null +++ b/context_intelligence_server/queue_manager/factory.py @@ -0,0 +1,35 @@ +"""Config-driven QueueManager factory — the ONLY place a queue backend is selected. + +This is the single seam through which the concrete backend is chosen. Adding +a new backend (e.g. Azure) means: one new module implementing +:class:`~.protocol.QueueManager`, one new branch here, and a config value — +zero changes to :mod:`context_intelligence_server.registry` or any consumer. + +This module (and :mod:`~context_intelligence_server.config`) are the only +places ``settings.queues_path`` is read — the on-disk root is a filesystem- +backend concern, resolved here and handed to the concrete backend at +construction time. Callers only ever see the :class:`~.protocol.QueueManager` +Protocol. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import TYPE_CHECKING + +from .filesystem import FileSystemQueueManager +from .protocol import QueueManager + +if TYPE_CHECKING: + from context_intelligence_server.config import Settings + + +def create_queue_manager(settings: Settings) -> QueueManager: + """Build the durable ``QueueManager`` from config. + + Single backend today (on-disk), so this is a thin config-reading seam + rather than a multi-backend dispatcher — but it keeps ``settings.queues_path`` + out of consumers (mirrors ``blob_store.factory.create_blob_store`` and + ``identity_store.factory.create_identity_store``). + """ + return FileSystemQueueManager(queues_dir=Path(settings.queues_path)) diff --git a/context_intelligence_server/queue_manager.py b/context_intelligence_server/queue_manager/filesystem.py similarity index 95% rename from context_intelligence_server/queue_manager.py rename to context_intelligence_server/queue_manager/filesystem.py index 85bf993b..5f058a26 100644 --- a/context_intelligence_server/queue_manager.py +++ b/context_intelligence_server/queue_manager/filesystem.py @@ -1,4 +1,4 @@ -"""On-disk durable queue manager for the event-write pipeline. +"""FileSystemQueueManager — on-disk durable queue for the event-write pipeline. Disk layout (one set of files per session, keyed by ``session_id``): @@ -20,6 +20,12 @@ it is empty or contains a path separator (``/`` or ``\\``) or a null byte. The ``session_id`` is used raw as the filename stem, so it must be a safe, single path component. + +This is a concrete implementation of the +:class:`~context_intelligence_server.queue_manager.protocol.QueueManager` +Protocol. No ``Path``, on-disk layout, or ``os.*`` detail appears in the +Protocol or in any value it returns — those details are private to this +class (and, later, an Azure equivalent). """ from __future__ import annotations @@ -29,12 +35,10 @@ import json import os import time -from dataclasses import dataclass from pathlib import Path -from typing import TYPE_CHECKING, Any +from typing import Any -if TYPE_CHECKING: - from context_intelligence_server.config import Settings +from .protocol import Batch # Fixed buffer size for streaming scans over a session ``.log`` (last-newline # search and newline counting). Bounds boot-time and /status memory to O(chunk) @@ -44,28 +48,12 @@ _SCAN_CHUNK_BYTES = 1 << 20 -@dataclass(frozen=True) -class Batch: - """A contiguous batch of log lines read from a session's append-only log. +class FileSystemQueueManager: + """Manages per-session append-only queues on disk. - Attributes: - session_id: The session the lines belong to. - lines: Raw, complete log lines WITHOUT their trailing newline. - start_offset: Byte position in the log where this batch begins. - end_offset: Byte position in the log AFTER the last returned line. - This is the value passed to ``commit``. When no complete lines - are available, ``end_offset == start_offset``. + Implements :class:`~.protocol.QueueManager`. """ - session_id: str - lines: list[bytes] - start_offset: int - end_offset: int - - -class QueueManager: - """Manages per-session append-only queues on disk.""" - def __init__(self, queues_dir: Path): self._dir = Path(queues_dir) self._dir.mkdir(parents=True, exist_ok=True) @@ -492,7 +480,7 @@ async def spool_stats(self) -> dict[str, int]: aggregate. A ``-1`` in either field is the operator-visible "spool footprint - temporarily unavailable" signal -- distinct from a real ``0`` -- and + temporarily unavailable" signal -- distinct from a real ``0``, and never leaks any identifier. """ now = time.monotonic() @@ -732,13 +720,3 @@ def _reconcile() -> int: return total_skipped return await asyncio.to_thread(_reconcile) - - -def create_queue_manager(settings: Settings) -> QueueManager: - """Build the durable ``QueueManager`` from config. - - Single backend today (on-disk), so this is a thin config-reading seam - rather than a multi-backend dispatcher \u2014 but it keeps ``settings.queues_path`` - out of consumers (mirrors ``blob_store.factory.create_blob_store``). - """ - return QueueManager(queues_dir=Path(settings.queues_path)) diff --git a/context_intelligence_server/queue_manager/protocol.py b/context_intelligence_server/queue_manager/protocol.py new file mode 100644 index 00000000..ff4a09d8 --- /dev/null +++ b/context_intelligence_server/queue_manager/protocol.py @@ -0,0 +1,121 @@ +"""QueueManager Protocol — the backend-neutral seam. + +A ``QueueManager`` manages a durable, per-session append-only queue for the +event-write pipeline: events are appended as opaque ``bytes`` lines, read back +in batches, and the committed offset advances only once a batch has been +durably processed (the "ack"). Lines that cannot be processed after +exhausting retries are dead-lettered rather than silently dropped. + +No ``Path``, on-disk layout, or ``os.*`` detail appears here or in any value +the Protocol returns — that is private to a concrete backend +(:class:`~context_intelligence_server.queue_manager.filesystem.FileSystemQueueManager` +and, later, an Azure equivalent). + +This module is backend-neutral by construction: it imports nothing from +``os``, ``pathlib``, or any filesystem library. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Any, Protocol, runtime_checkable + +# --------------------------------------------------------------------------- +# Batch — a contiguous slice of a session's durable log +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class Batch: + """A contiguous batch of log lines read from a session's append-only log. + + Attributes: + session_id: The session the lines belong to. + lines: Raw, complete log lines WITHOUT their trailing newline. + start_offset: Byte position in the log where this batch begins. + end_offset: Byte position in the log AFTER the last returned line. + This is the value passed to ``commit``. When no complete lines + are available, ``end_offset == start_offset``. + """ + + session_id: str + lines: list[bytes] + start_offset: int + end_offset: int + + +# --------------------------------------------------------------------------- +# QueueManager protocol +# --------------------------------------------------------------------------- + + +@runtime_checkable +class QueueManager(Protocol): + """Protocol for a durable, per-session append-only queue. + + 100% backend-neutral: no ``Path``, no on-disk layout, no ``os.*`` — ever. + ``session_id`` (and, where relevant, ``worker_key``) is the only identity + that crosses the boundary. + """ + + async def append(self, session_id: str, raw: bytes) -> None: + """Append one raw record to *session_id*'s durable log.""" + ... + + async def read_batch(self, session_id: str, max_items: int) -> Batch: + """Return up to *max_items* complete, uncommitted lines for *session_id*.""" + ... + + async def commit(self, session_id: str, new_offset: int) -> None: + """Durably persist *new_offset* as the committed (acked) position.""" + ... + + async def dead_letter(self, session_id: str, raw: bytes, error: str) -> None: + """Record one unprocessable line for *session_id*, with its *error*.""" + ... + + async def delete_drained(self, session_id: str) -> None: + """Remove the drained log/offset for a fully-finalized *session_id*. + + Dead-letter records (if any) are intentionally retained. + """ + ... + + async def read_dead_letters(self, session_id: str) -> list[dict[str, Any]]: + """Return all dead-letter records for *session_id*, in append order.""" + ... + + async def active_sessions(self) -> list[str]: + """Return sorted session_ids with undrained (uncommitted) data.""" + ... + + async def recover(self) -> list[str]: + """Return sorted session_ids that have at least one complete unprocessed line.""" + ... + + async def derive_all_stats(self) -> dict[str, Any]: + """Derive live queue stats (per-key + aggregate) purely from durable state.""" + ... + + async def spool_stats(self) -> dict[str, int]: + """Return a cheap, aggregate-only spool footprint (health-endpoint safe).""" + ... + + async def dead_letter_keys(self) -> list[str]: + """Return sorted worker keys that have at least one dead-letter record.""" + ... + + async def purge_dead_letters(self, worker_key: str) -> int: + """Delete all dead-letter records for *worker_key*; return the count removed.""" + ... + + async def recovery_seed_counts(self) -> tuple[int, int]: + """Return ``(accepted_seed, written_seed)`` derived from durable state at boot.""" + ... + + async def recovery_reconcile_dead(self) -> int: + """Advance committed offsets past leading already-dead pending lines. + + Returns the total number of lines skipped across all keys. + """ + ... diff --git a/tests/integration/test_crash_recovery.py b/tests/integration/test_crash_recovery.py index 61fb544a..f4f7e9de 100644 --- a/tests/integration/test_crash_recovery.py +++ b/tests/integration/test_crash_recovery.py @@ -9,7 +9,7 @@ from unittest.mock import AsyncMock, patch from context_intelligence_server import registry as registry_module -from context_intelligence_server.queue_manager import QueueManager +from context_intelligence_server.queue_manager import FileSystemQueueManager from context_intelligence_server.registry import SessionRegistry, SessionWorker from context_intelligence_server.services import HookStateService @@ -23,7 +23,7 @@ async def test_no_loss_after_crash_mid_drain() -> None: registry_module.get_settings() ) # queues_path patched to tmp_path by safe_settings sid = "crash-sess" - qm = QueueManager(queues_dir=Path(settings.queues_path)) + qm = FileSystemQueueManager(queues_dir=Path(settings.queues_path)) K = 5 for i in range(K): @@ -94,7 +94,7 @@ async def test_offset_never_advances_over_undurable_data() -> None: buffer-restore-on-failure (neo4j_store.py:686-696).""" settings = registry_module.get_settings() sid = "flush-fail-sess" - qm = QueueManager(queues_dir=Path(settings.queues_path)) + qm = FileSystemQueueManager(queues_dir=Path(settings.queues_path)) await qm.append(sid, _line("e0", "/ws", {"session_id": sid})) reg = SessionRegistry() diff --git a/tests/neo4j/test_concurrent_flush.py b/tests/neo4j/test_concurrent_flush.py index 470aa83d..d9742c35 100644 --- a/tests/neo4j/test_concurrent_flush.py +++ b/tests/neo4j/test_concurrent_flush.py @@ -25,7 +25,7 @@ Neo4jGraphStore, ensure_neo4j_schema, ) -from context_intelligence_server.queue_manager import QueueManager +from context_intelligence_server.queue_manager import FileSystemQueueManager from context_intelligence_server.registry import SessionRegistry, SessionWorker from context_intelligence_server.services import HookStateService from context_intelligence_server.utils import make_node_id @@ -226,7 +226,7 @@ async def test_durable_drain_multi_writer_zero_loss( await driver.close() reg = SessionRegistry() - reg._queue_manager = QueueManager(queues_dir=tmp_path / "queues") + reg._queue_manager = FileSystemQueueManager(queues_dir=tmp_path / "queues") reg._write_semaphore = asyncio.Semaphore(2) reg._max_delivery_attempts = 5 qm = reg._queue_manager @@ -327,7 +327,7 @@ async def test_durable_poison_isolation_no_contamination( await driver.close() reg = SessionRegistry() - reg._queue_manager = QueueManager(queues_dir=tmp_path / "queues") + reg._queue_manager = FileSystemQueueManager(queues_dir=tmp_path / "queues") reg._write_semaphore = asyncio.Semaphore(2) reg._max_delivery_attempts = 2 qm = reg._queue_manager diff --git a/tests/neo4j/test_oom_regression.py b/tests/neo4j/test_oom_regression.py index b7fd2e1b..4fb8228b 100644 --- a/tests/neo4j/test_oom_regression.py +++ b/tests/neo4j/test_oom_regression.py @@ -306,7 +306,7 @@ async def test_finalization_path_freezes_then_restart_then_drains( from unittest.mock import AsyncMock from context_intelligence_server.pipeline import setup_handlers - from context_intelligence_server.queue_manager import QueueManager + from context_intelligence_server.queue_manager import FileSystemQueueManager from context_intelligence_server.registry import SessionRegistry, SessionWorker from context_intelligence_server.services import HookStateService @@ -320,7 +320,7 @@ async def test_finalization_path_freezes_then_restart_then_drains( # fixture) patches into get_settings().queues_path for every new # SessionRegistry() instance constructed in this test. queues_dir = tmp_path / "queues" - qm = QueueManager(queues_dir=queues_dir) + qm = FileSystemQueueManager(queues_dir=queues_dir) async def _make( rows: int, byts: int diff --git a/tests/neo4j/test_orphan_visibility.py b/tests/neo4j/test_orphan_visibility.py index a2ced1e0..0799579b 100644 --- a/tests/neo4j/test_orphan_visibility.py +++ b/tests/neo4j/test_orphan_visibility.py @@ -45,7 +45,7 @@ from context_intelligence_server.status import build_status_response from context_intelligence_server.neo4j_store import Neo4jGraphStore -from context_intelligence_server.queue_manager import QueueManager +from context_intelligence_server.queue_manager import FileSystemQueueManager from context_intelligence_server.registry import SessionRegistry, SessionWorker from context_intelligence_server.services import HookStateService @@ -145,7 +145,7 @@ async def test_finalization_orphan_surfaces_on_status( # guarantees this QueueManager and the registry's queue_manager point at # the same on-disk queue. queues_dir = tmp_path / "queues" - qm = QueueManager(queues_dir=queues_dir) + qm = FileSystemQueueManager(queues_dir=queues_dir) # ----------------------------------------------------------------------- # Seed the durable queue in exact order (line counts are load-bearing). diff --git a/tests/neo4j/test_queues_actions.py b/tests/neo4j/test_queues_actions.py index 6276bbc1..dd57e3e5 100644 --- a/tests/neo4j/test_queues_actions.py +++ b/tests/neo4j/test_queues_actions.py @@ -34,7 +34,7 @@ Neo4jGraphStore, ensure_neo4j_schema, ) -from context_intelligence_server.queue_manager import QueueManager +from context_intelligence_server.queue_manager import FileSystemQueueManager from context_intelligence_server.registry import SessionRegistry, SessionWorker from context_intelligence_server.services import HookStateService @@ -96,7 +96,7 @@ async def test_replay_rewrites_through_real_drainer( # worker is pre-registered, so get_or_create never builds a settings-derived # store). reg = SessionRegistry() - reg._queue_manager = QueueManager(queues_dir=tmp_path / "queues") + reg._queue_manager = FileSystemQueueManager(queues_dir=tmp_path / "queues") reg._write_semaphore = asyncio.Semaphore(1) reg._max_delivery_attempts = 3 qm = reg._queue_manager diff --git a/tests/routers/test_queues.py b/tests/routers/test_queues.py index c8909e57..09bb096a 100644 --- a/tests/routers/test_queues.py +++ b/tests/routers/test_queues.py @@ -9,15 +9,15 @@ import pytest from context_intelligence_server.main import registry -from context_intelligence_server.queue_manager import QueueManager +from context_intelligence_server.queue_manager import FileSystemQueueManager -def _point_registry_at(tmp_path: Path) -> QueueManager: +def _point_registry_at(tmp_path: Path) -> FileSystemQueueManager: """Point the shared registry's durable infra at a tmp_path queues dir. Returns the QueueManager so tests can seed dead-letter records directly. """ - qm = QueueManager(queues_dir=tmp_path / "queues") + qm = FileSystemQueueManager(queues_dir=tmp_path / "queues") registry._queue_manager = qm registry._write_semaphore = asyncio.Semaphore(2) registry._max_delivery_attempts = 5 diff --git a/tests/test_main.py b/tests/test_main.py index 1ed0180a..0ead9449 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -164,11 +164,13 @@ async def test_post_events_increments_accepted_counter( monkeypatch: pytest.MonkeyPatch, ) -> None: """A durably-accepted event increments the registry accepted_total (D2).""" - from context_intelligence_server.queue_manager import QueueManager + from context_intelligence_server.queue_manager import FileSystemQueueManager # Point the registry at a tmp queue dir so the durable append is isolated. monkeypatch.setattr( - main_module.registry, "_queue_manager", QueueManager(queues_dir=tmp_path) + main_module.registry, + "_queue_manager", + FileSystemQueueManager(queues_dir=tmp_path), ) monkeypatch.setattr( main_module.registry, "get_or_create", lambda *args, **kwargs: MagicMock() @@ -1314,11 +1316,11 @@ async def test_lifespan_seeds_counters_from_disk(tmp_path: Path) -> None: zero residual: 1 committed + 1 pending line yields accepted=2, written=1, in_queue=1, residual=0 after reconcile -> seed_counts -> seed_counters. """ - from context_intelligence_server.queue_manager import QueueManager + from context_intelligence_server.queue_manager import FileSystemQueueManager from context_intelligence_server.registry import SessionRegistry # Seed a queue dir with one committed line and one still-pending line. - seed_qm = QueueManager(queues_dir=tmp_path) + seed_qm = FileSystemQueueManager(queues_dir=tmp_path) sid = "sess-seed" line1 = json.dumps({"event": "a", "workspace": "/ws", "data": {}}).encode("utf-8") line2 = json.dumps({"event": "b", "workspace": "/ws", "data": {}}).encode("utf-8") @@ -1329,7 +1331,7 @@ async def test_lifespan_seeds_counters_from_disk(tmp_path: Path) -> None: # Fresh registry reusing the same on-disk queue dir. reg = SessionRegistry() - reg._queue_manager = QueueManager(queues_dir=tmp_path) + reg._queue_manager = FileSystemQueueManager(queues_dir=tmp_path) # Production order: reconcile dead lines BEFORE seeding the counts. await reg.queue_manager.recovery_reconcile_dead() diff --git a/tests/test_queue_isolation_tripwire.py b/tests/test_queue_isolation_tripwire.py new file mode 100644 index 00000000..e3f12387 --- /dev/null +++ b/tests/test_queue_isolation_tripwire.py @@ -0,0 +1,88 @@ +"""Tripwire: the queue on-disk location stays inside the queue_manager package. + +Locks the isolation boundary established by the QueueManager refactor +(mirrors ``tests/test_blob_isolation_tripwire.py`` and +``tests/test_identity_isolation_tripwire.py``). These tests fail loudly if a +future change lets any module other than the ``queue_manager/`` package +locate the durable queue's backing directory, or resurrects a concrete-class +reference at a consumer site -- i.e. if a direct-FS queue leak is +reintroduced. + +Two invariants: + 1. Only the ``queue_manager/`` package (specifically ``factory.py``, the + single construction site) and the config field declaration (config.py) + may reference ``settings.queues_path`` -- a caller that cannot locate + the queue root physically cannot do queue filesystem I/O directly. + ``registry.py`` goes through ``create_queue_manager(settings)`` and + never sees the path itself. + 2. The concrete ``FileSystemQueueManager`` must appear ONLY inside the + ``queue_manager/`` package (production consumers depend on the + ``QueueManager`` Protocol and obtain instances via + ``create_queue_manager(settings)``). Tests are permitted to construct a + concrete backend directly. +""" + +from __future__ import annotations + +import pathlib + +PKG = pathlib.Path(__file__).resolve().parents[1] / "context_intelligence_server" + + +def _code_lines(path: pathlib.Path): + """Yield (lineno, stripped) for real code lines, skipping comments and + rst-doc lines (``...`` backtick spans) so docstring prose never trips the + guard.""" + for i, raw in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + s = raw.strip() + if not s or s.startswith("#") or "``" in raw: + continue + yield i, s + + +def test_queues_path_locatable_only_at_construction_site() -> None: + """`settings.queues_path` -- the only way to find the on-disk queue root + -- is referenced solely inside the queue_manager/ package (which owns + the layout) and where the config field is declared (config.py). + registry.py goes through create_queue_manager(settings) and never sees + the path itself.""" + queue_manager_pkg = PKG / "queue_manager" + allowed_top_level = {"config.py"} + offenders: list[str] = [] + for p in PKG.rglob("*.py"): + # Anything inside the queue_manager/ package owns the layout -- allowed. + if queue_manager_pkg in p.parents: + continue + if p.name in allowed_top_level: + continue + for lineno, line in _code_lines(p): + if "queues_path" in line: + offenders.append(f"{p.relative_to(PKG)}:{lineno}: {line}") + assert not offenders, ( + "queue root re-derived outside the queue_manager/ package/config -- a " + "caller that can locate the queue root can bypass QueueManager and " + "touch disk directly:\n" + "\n".join(offenders) + ) + + +def test_concrete_impl_referenced_only_inside_the_package() -> None: + """The concrete ``FileSystemQueueManager`` must appear ONLY inside the + ``queue_manager/`` package. Production consumers depend on the + ``QueueManager`` Protocol and obtain instances via + ``create_queue_manager(settings)`` -- never by naming a concrete backend + -- so a disk->Azure swap touches only the package. (Tests are permitted + to construct a concrete backend directly.)""" + queue_manager_pkg = PKG / "queue_manager" + offenders: list[str] = [] + for p in PKG.rglob("*.py"): + if queue_manager_pkg in p.parents: + continue + for lineno, line in _code_lines(p): + if "FileSystemQueueManager" in line: + offenders.append(f"{p.relative_to(PKG)}:{lineno}: {line}") + assert not offenders, ( + "concrete FileSystemQueueManager named outside the queue_manager/ " + "package -- consumers must use the QueueManager Protocol + " + "create_queue_manager(settings), not a concrete backend:\n" + + "\n".join(offenders) + ) diff --git a/tests/test_queue_manager.py b/tests/test_queue_manager.py index bd270601..35d4e824 100644 --- a/tests/test_queue_manager.py +++ b/tests/test_queue_manager.py @@ -6,18 +6,18 @@ import pytest -from context_intelligence_server.queue_manager import Batch, QueueManager +from context_intelligence_server.queue_manager import Batch, FileSystemQueueManager @pytest.fixture def qm(tmp_path): - return QueueManager(queues_dir=tmp_path / "queues") + return FileSystemQueueManager(queues_dir=tmp_path / "queues") def test_constructor_creates_queues_dir(tmp_path): target = tmp_path / "nested" / "queues" assert not target.exists() - QueueManager(queues_dir=target) + FileSystemQueueManager(queues_dir=target) assert target.is_dir() @@ -145,12 +145,12 @@ async def test_commit_advances_offset(qm): async def test_commit_persists_across_a_new_instance(tmp_path): qdir = tmp_path / "queues" - qm1 = QueueManager(queues_dir=qdir) + qm1 = FileSystemQueueManager(queues_dir=qdir) await qm1.append("s1", b"a") await qm1.append("s1", b"b") batch = await qm1.read_batch("s1", max_items=1) await qm1.commit("s1", batch.end_offset) - qm2 = QueueManager(queues_dir=qdir) # simulate restart + qm2 = FileSystemQueueManager(queues_dir=qdir) # simulate restart resumed = await qm2.read_batch("s1", max_items=10) assert resumed.lines == [b"b"] @@ -227,9 +227,9 @@ async def test_read_dead_letters_rejects_unsafe_session_id(qm, bad_id): async def test_delete_drained_removes_log_and_offset_keeps_dead(tmp_path) -> None: - from context_intelligence_server.queue_manager import QueueManager + from context_intelligence_server.queue_manager import FileSystemQueueManager - qm = QueueManager(queues_dir=tmp_path) + qm = FileSystemQueueManager(queues_dir=tmp_path) await qm.append("s", b"line") await qm.commit("s", 5) await qm.dead_letter("s", b"bad\n", "boom") @@ -613,7 +613,7 @@ def test_complete_data_end_newline_on_chunk_boundary(qm, tmp_path, monkeypatch): """The backward scan reads fixed non-overlapping windows; a newline landing exactly on a chunk boundary must still be found (regression guard for the streaming rewrite).""" - import context_intelligence_server.queue_manager as qm_mod + import context_intelligence_server.queue_manager.filesystem as qm_mod monkeypatch.setattr(qm_mod, "_SCAN_CHUNK_BYTES", 8) log = tmp_path / "queues" / "s1.log" @@ -628,7 +628,7 @@ def test_complete_data_end_newline_on_chunk_boundary(qm, tmp_path, monkeypatch): def test_count_newlines_matches_naive_across_ranges(qm, tmp_path, monkeypatch): """_count_newlines(start,end) == data[start:end].count(b'\\n') for arbitrary ranges, including across a small chunk size (multi-chunk streaming).""" - import context_intelligence_server.queue_manager as qm_mod + import context_intelligence_server.queue_manager.filesystem as qm_mod monkeypatch.setattr(qm_mod, "_SCAN_CHUNK_BYTES", 4) log = tmp_path / "queues" / "s1.log" @@ -652,7 +652,7 @@ def test_count_newlines_missing_and_empty_range(qm): def test_count_dead_matches_naive_and_streams(qm, tmp_path, monkeypatch): """_count_dead == old data.count(b'\\n') for empty / multi-record / missing, including a newline on a chunk boundary (streamed, not read_bytes).""" - import context_intelligence_server.queue_manager as qm_mod + import context_intelligence_server.queue_manager.filesystem as qm_mod monkeypatch.setattr(qm_mod, "_SCAN_CHUNK_BYTES", 8) dead = tmp_path / "queues" / "s1.dead.jsonl" diff --git a/tests/test_registry.py b/tests/test_registry.py index 44390a0a..79780081 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -17,7 +17,7 @@ import context_intelligence_server.registry as registry_module from context_intelligence_server.blob_store import FileSystemBlobStore from context_intelligence_server.config import get_settings -from context_intelligence_server.queue_manager import QueueManager +from context_intelligence_server.queue_manager import FileSystemQueueManager from context_intelligence_server.registry import ( CompletedSession, SessionRegistry, @@ -804,7 +804,7 @@ def test_queue_manager_is_lazy_and_rooted_at_settings(self) -> None: """queue_manager is a QueueManager rooted at settings.queues_path, idempotent.""" reg = SessionRegistry() qm = reg.queue_manager - assert isinstance(qm, QueueManager) + assert isinstance(qm, FileSystemQueueManager) # Built from the (patched) settings the registry sees. settings = registry_module.get_settings()