Blob storage API isolation (stacked on #70) - #75
Closed
Diego Colombo (colombod) wants to merge 23 commits into
Closed
Blob storage API isolation (stacked on #70)#75Diego Colombo (colombod) wants to merge 23 commits into
Diego Colombo (colombod) wants to merge 23 commits into
Conversation
…longer MERGEs across orchestrator runs (also fixes I3)
- Iteration.node_id is now composed with the active orchestrator-run identifier: {session_id}::orch_run::{ts}::iteration::{N}, using the run disambiguator OrchestratorRun already tracks. Previously node_id was {session_id}::iteration::{N} with N restarting per run, so one Iteration node MERGEd across up to 15 runs (last-write-wins garbage on usage_input/usage_output/usage_cache_write/message_count; count(DISTINCT Iteration) undercount; run→Iteration→ToolCall overcount).
- The HAS_PART edge and the tool_call / content_block / skill_load cursors carry the new id (they treat it as opaque). The per-session iteration counter is intentionally NOT reset per run — that would collide ContentBlock node_ids ({session_id}::block::{N}::{idx}); run-uniqueness comes from the ::orch_run:: segment.
- This ends the last-write-wins collisions, so I3 (usage_cache_write junk) is fixed as a direct consequence. Forward-only: already-merged historical Iteration nodes are not retroactively split.
- Validated: server unit suite green (1772 passed / 2 skipped) + the delegation/skill integration test (6 passed); previously DTU-proven (new session max run-parents-per-Iteration = 1 vs 2 pre-fix in the same graph).
Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…operty (populate-if-missing) - EventRequest gains an optional top-level working_dir; post_events lifts it into the event data so it rides the existing pipeline to ensure_session_node, which sets Session.working_dir. Pairs with the bundle hook + upload tool now emitting working_dir (Phase-1 branch). - Populate-if-missing: fills the property from any event that carries a non-empty working_dir — including re-imports of pre-existing sessions via the upload tool — and never clobbers an already-set value; idempotent. Forward-only: sessions with no working_dir-bearing event stay null. - Startup is unchanged from base (no migration/rectification added here — the schema-version-awareness and server-side migration workflow are deferred to branch spike/self-service-upgrade for redesign). - Validated: server unit suite green (1772 passed / 2 skipped); previously DTU-proven end to end (live session populates Session.working_dir; imported JSONL populates it; re-import of an existing NULL session backfills it with no-clobber + positive control). Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…des (I5 remediation, non-destructive) Adds scripts/tag_legacy_pooled_iterations.py: a standalone, idempotent, dry-run+apply maintenance script that marks ONLY the confirmed-corrupt pre-fix Iteration nodes with `data_quality='legacy_pooled_pre_fix'`. Confirmed-corrupt = bare node_id (no `::orch_run::`) AND >=2 distinct OrchestratorRun HAS_PART parents (i.e. actually MERGEd across runs by the I5 bug). It deliberately does NOT touch single-run bare-id nodes (clean, ~93-95% of legacy), no-run-edge nodes (unconfirmable), or run-scoped nodes. This puts the corruption signal in the data itself, so a naive `MATCH (i:Iteration)` consumer can see it — not only skill users routing around it. Why tag and not delete: a council review + live-data verification (private-home-server 2,960/66,131 = 4.48%; team-shared 7,467/181,778 = 4.11%) showed a shape-based delete (`NOT node_id CONTAINS '::orch_run::'`) would destroy ~95% VALID single-run data — "old" != "corrupt". Only ~4% of legacy nodes are actually corrupt. The destructive DETACH DELETE of the tagged subset is a deferred, gated follow-up (dry-run count, all-producers-rolled, degree-batched, ContentBlock-orphan aware), not part of this commit. Adds a regression assertion to test_iteration.py: a multi-run session must produce Iteration nodes that each have exactly ONE distinct HAS_PART OrchestratorRun parent and distinct run-scoped node_ids (fails loudly if run-scoping is ever reverted, which would re-MERGE across runs). This is the recurrence guard — a test, not a runtime server check. Adds CHANGELOG.md (newest-first) documenting the data-quality work on this branch (I5 run-scoped node_id forward-only; I1 working_dir lift populate-if-missing forward-only; the tag script). No SCHEMA_VERSION/ledger/migration-runner machinery — the general self-service migration mechanism stays deferred to branch spike/self-service-upgrade. Validation: script --help ok; iteration tests 22 passed; full unit suite 1772 passed / 2 skipped. DTU-validated on synthetic data across all four categories (5/5 PASS): tags exactly the confirmed-corrupt nodes, leaves clean/no-run-edge/run-scoped untouched, idempotent (0 rows on re-apply). Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
… rebuild (+ iteration_scope, schema_version baseline)
I5 gave Iteration.node_id a run-scoped shape, but that shape depended on
in-memory DataLayer2State (execution_start_ts, iteration_count) that was
rebuilt EMPTY on any worker (re)creation. Two paths triggered this: a
process crash-restart with an undrained tail, and a stale-session reap
(which had no recovery path at all). Either one regressed node_id back to
the pre-fix bare shape and re-pooled Iteration nodes across orchestrator
runs, reopening I5 and I3.
Fix: persist the full DataLayer2State + DataLayer3State cursor ATOMICALLY
with the queue offset — folded into the .offset record as a single
os.replace, with the legacy bare-int .offset still read for back-compat —
and restore it exactly once at the top of drain_worker, the single
worker-construction path that covers both crash-restart and stale-reap.
This also preserves E09/E14/E15/E10-E11 edges across a rebuild.
Phantom-cursor guard: a dead-lettered line's in-memory mutations are now
rolled back before commit, so the persisted cursor can never point at a
discarded node. Written failing-test-first.
iteration_scope ("run" | "unscoped") is stamped at all three Iteration
upsert sites, so a node can never be created without a scope value; logged
at INFO.
schema_version baseline (data points only — no upgrade/handling logic):
a SCHEMA_VERSION=1 constant is exposed on /version, and a
(:SchemaMeta{id:'singleton'}) node is created create-if-absent (ON CREATE
only) behind a uniqueness constraint. This write is STARTUP-ONLY — moved
out of ensure_neo4j_schema (which runs per-worker-flush) into
ensure_schema_version_baseline(), called once from the lifespan so it is
single-writer with no concurrency.
Validated: 1841 non-neo4j tests pass; the full tests/neo4j/ suite (85
passed) ran against a real Neo4j, including end-to-end crash-restart +
stale-reap (duplicate Iteration=0, run-scoped continuity, edge parity,
iteration_scope=run, exactly one SchemaMeta) and a 20-way concurrent
schema_version race resolving to a single node.
Co-authored-by: Amplifier <amplifier@microsoft.com>
…-once-at-end race
IncompleteSession was written once at session:end when the Session node
had no type label yet, and never revised. Forked sub-sessions drain in
independent per-session queues with no cross-session ordering, so a
child's session:end is often processed before its session:fork/
session:start → classify() sees no type → stamps IncompleteSession →
the later fork/start add the real terminal but never clear the stale
marker. Live bisect: ~52.8% of sessions carry the label, ~99.4% are
false positives (the node carries its own linked start/fork event);
genuine loss ~0.5%.
Part 1 (heal-forward, code): classify() now strips IncompleteSession on
EVERY start/fork transition via a single _heal_forward() normalizer
(invariant applied to the return value, not per-branch, so a future
branch can't skip it); the end branch is unchanged (still the real
signal for the genuine ~0.5%). Reuses existing set_labels remove
plumbing — no store/Cypher change. Order-independent. Stale comments
in session.py/_handle_end and delegation.py updated.
Part 2 (one-off backfill, scripts/relabel_incomplete_sessions.py):
standalone, out-of-band, idempotent script that clears IncompleteSession
from provably-false-positive nodes (real terminal type OR a linked
SessionStartEvent/SessionForkEvent), leaving the genuine ~0.5% untouched.
--apply is hard-gated behind a read-only reconciliation diagnostic
(refuses if any linked-but-untyped nodes exist, so the selector
assumption is verified per-DB before mutating). Batched CALL{} IN
TRANSACTIONS; touched-id undo-log with --restore; before/after
population summary; POST-DEPLOY GATE in the docstring (run only after
Part 1 is deployed+verified). SCHEMA_VERSION unchanged (no version
bump; handling deferred).
Validated: 1844 non-neo4j tests pass; full tests/neo4j/ 94 passed
against real Neo4j incl. 3 heal-forward (out-of-order end→fork/start
strips the label physically, no lattice disturbance) and 6 backfill
(clears false positives, retains genuine, idempotent, diagnostic-gate
refuses, restore round-trip).
Tracking: (internal tracker).
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…ment IncompleteSession reconciliation Dockerfile now COPYs scripts/ into the runtime image (/app/scripts/). Previously the image copied only context_intelligence_server/, so the standalone out-of-band data-rectification scripts (relabel_incomplete_sessions.py, tag_legacy_pooled_iterations.py, repair_dual_labels.py, ...) were unreachable from a running container -- they could not be run against a live cloud/VM deployment as the migration model requires. Verified: image builds and `docker exec ... python3 scripts/relabel_incomplete_sessions.py --help` works in-image (runtime has python3, not python). CHANGELOG.md now carries a prominent "ACTION REQUIRED ON UPGRADE IF LEGACY DATA IS PRESENT" notice: this release changes IncompleteSession labeling; new events self-heal but historical graphs carry ~52.8% stale markers (~99% false positive) that are NOT auto-corrected; a one-off, out-of-band reconciliation (scripts/relabel_incomplete_sessions.py --dry-run then --apply, gated + idempotent) must be run once after deploy; migrations never run at startup; fresh graphs need no action. Tracking (internal tracker). 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…(POST /admin/blobs/reclaim) The blob store (AsyncDiskBlobStore) has no delete/GC; disk under blob_path grows forever. This adds an admin-only, on-demand reclaim of blob files not referenced by any node in the graph. Single POST /admin/blobs/reclaim on the /admin router (require_admin), dry_run defaults TRUE — preview shows exactly what would be deleted; apply requires an explicit max_delete cap. One shared _select_orphans path for preview and apply; apply performs its own authoritative fresh scan. Orphan = on-disk ci-blob://<sid>/<key> absent from a GLOBAL, all-workspace scan of :Event.data (blobs are session-scoped, nodes are (node_id, workspace)-scoped — a per-workspace scan could delete another workspace's live data). Council-gated design (docs/plans/2026-08-12-blob-reclaim-endpoint-spec.md); three blockers closed: - B1 pinned the Event.data reference-carrier invariant with a regression test. - B2 replaced a broken URI regex (silently truncated on a `"`/non-ASCII session_id → happy-path data loss) with structural json.loads walk of $blob_ref, and removed the APOC dual-path. - B3 based in-flight safety on a durable undrained-queue gate (new QueueManager.is_fully_drained, survives kill -9) plus a live-worker check and a min_age_minutes floor of 15. Audit line per delete; idempotent. Validated: 1848 non-neo4j tests pass; tests/neo4j/test_blob_reclaim.py 12 passed against real Neo4j (cross-workspace safety, special-char extraction, undrained-queue/live-worker skip, dry/apply parity, idempotence, max-delete-required, B1 carrier invariant). 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…ps on un-migrated/unreachable graph state (6.7.1) INCIDENT: deploying the server (a restart) crash-looped the live host under systemd Restart=always because cold-start raised fatally on 1 un-migrated (:Node-lacking) legacy node (a PR #67 guard). On Azure Container Apps the private graph is unreachable to run `doctor --fix`, so this class of failure is an unacceptable hard outage. FIX (boot must NEVER crash-loop, council-gated): 1. ONE loud try/except boundary wraps the ENTIRE lifespan startup body -- setup_logging, Neo4j driver construction, schema init, untagged probe, schema_version baseline, recovery -- so no startup step can crash the worker; on any failure it logs loudly, records degraded/unknown health, and boots anyway. 2. setup_logging() is now resilient: configures a stdout console handler first/unconditionally, file handler + mkdir best-effort, never raises. 3. ensure_neo4j_schema no longer fatal at boot (fail_on_data_conflict=False) and reorders so the :Node index is dropped only AFTER the uniqueness constraint is created; if the constraint can't be created (duplicates) it creates a fallback idx_node_universal so the hot write-path MERGE keeps a NodeIndexSeek (degraded mode costs atomicity only, never the seek). 4. removed the two fatal raises (constraint conflict + untagged-count). 5. recovery iterates sessions defensively -- a corrupt .offset/dead-letter quarantines that one session, boot continues. 6. tri-state schema health (healthy|degraded|unknown; probe failure -> unknown, never coerced to green) surfaced on GET /status with schema_checked_at + degraded_reason; explicit prohibition (code comment + docs) against wiring it to a liveness/readiness probe. Write path UNCHANGED. Migration stays out-of-band (doctor --fix for reachable deployments; maintenance-mode/in-place-fix tracked in (internal tracker) for ACA). VERSION: 6.7.0 -> 6.7.1. VALIDATED: 1853 non-neo4j tests pass; neo4j EXPLAIN tests confirm fallback-index NodeIndexSeek in degraded mode and constraint-backed seek when healthy; NEW real-process boot tests (subprocess against unreachable Neo4j + unwritable log path) prove the server boots, serves GET /status (schema_health unknown/degraded), and stays up -- no crash loop. Independently re-verified via a real server process against an unreachable Neo4j. Tracking: relates to (internal tracker) (maintenance-mode/in-place-fix) and (internal tracker) (schema_version handling). 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…h-wide carrier walk, not Event.data-only
POST /admin/blobs/reclaim decided orphans from Event.data ONLY. ci-blob://
refs also live on tool_input (ToolPreEvent/ToolPostEvent/ToolCall), prompt
(PromptSubmitEvent/PromptCompleteEvent/Prompt), and response
(OrchestratorRun). Those were safe today only by an UNENFORCED
pipeline-ordering invariant (every ref also landed in Event.data); a future
ingest change could silently make the GC delete a live blob.
FIX: compute the referenced set graph-wide over the known carrier
properties {data, tool_input, prompt, response} via a UNION ALL of four
single-property predicates (each touching exactly ONE property per row --
deliberately NOT an all-keys/all-node walk, to avoid the AllNodesScan stall
class this codebase is sensitive to). Each value is JSON-parsed and walked
structurally for $blob_ref (wrapper carriers); on parse failure, bare
ci-blob:// tokens are regex-extracted (plain-string carriers). Strict
SUPERSET of the old scan -- can only ever protect MORE blobs, never delete
more. Carrier list is an explicit module constant
(_BLOB_REF_CARRIER_PROPERTIES): adding a new carrier property in future
requires adding it here.
Makes orphan-detection correct by construction, not by pipeline ordering.
Closes the residual risk behind the endpoint spec's original "B1 --
Event.data is the complete reference carrier" (test-pinned) assumption.
VALIDATED: tests/neo4j/test_blob_reclaim.py 16 passed against real Neo4j
(12 original + 4 new: a blob referenced ONLY on ToolCall.tool_input / a
plain-string tool_input / Prompt.prompt / OrchestratorRun.response is now
protected -- each FAILS the old Event.data-only scan, PASSES now); 1853
non-neo4j pass. Equivalence dry-run on the live 242k-blob graph:
referenced_uris 242436 >= prior 242185 (superset), orphans_found 0
(non-regressive), scan completed in 19s (no stall).
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
max_delete had no validator, so a negative value would invert the cap via list slicing: candidates[: body.max_delete] with max_delete=-1 deletes all-but-one instead of enforcing a limit. This is a silent inversion of security intent, not caught by the apply-mode guard (which only checks if max_delete is None at apply-time). Fix adds Field(ge=1) to max_delete so <1 values are rejected at the schema boundary with 422 before the handler is invoked. None is still allowed (represents 'not yet decided', used in dry-run exploration). Tests cover: - max_delete=0 -> 422 (schema floor) - max_delete=-1 -> 422 (regression: inverted-cap blocker) - max_delete=1 -> accepted (the floor value is valid) From the PR review (blocking B-3). Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
The drain_worker retry loop (budget NOT exhausted, common path) replayed a failed batch without restoring cursor state. IterationHandler mutates iteration_count BEFORE its graph write, deriving iteration_id from the current counter value. On a transient flush failure (e.g. DeadlockDetected), the retry would replay the same batch without resetting the cursor, so iteration_count would advance again, creating a new (duplicate) ::iteration::N node for a batch that only ever committed once. This caused permanent counter skew: the counter was 2 but only 1 Iteration node was part of the committed graph. The existing snapshot_cursor()/restore_cursor() guard was wired only into the _handle_exhausted_batch path (budget-spent, give-up branch). Fix extends it to the common retry branch: snapshot the pre-batch cursor once per batch attempt (attempts==0) and restore it before each failed-attempt replay in the budget-not-exhausted retry branch. Tests drive the real process_event loop with a transient-then-success flush failure and assert exactly one Iteration node + unchanged iteration_count; also test a chained-failure (N>=2) case. Before fix: tests fail with duplicate node creation. After fix: tests pass. Proven non-vacuous. From the PR review (blocking B-2). Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…tenance channel Server now refuses ingest AND query with a structured maintenance-503 (Retry-After + JSON reason) while the :Node uniqueness constraint is absent. Gate lives in the drain loop (never latches; offset never advances while gated). Live TTL constraint probe self-clears without restart. /status advertises mode/started_at/elapsed. Allow-list keeps /status, /version, /admin/maintenance reachable with startup assertion. /admin/maintenance (POST triggers run_repair via private admin-driver path with atomic CAS single-flight, returns promptly; GET reports progress). Docs auth-fold (main:app→main:asgi_app). Stale AGENTS.md refreshed for this engagement. Addresses code review blocking item B-1 and must-fix items #1,#2,#3,#4,#5. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Add a validator rejecting blank/whitespace working_dir (None still allowed, unlike workspace). Session-node MERGE now uses coalesce to ensure an existing working_dir value is never clobbered across concurrent writers. The generic hot-path _NODE_MERGE_CYPHER is untouched (working_dir only flows through the Session inline MERGE), preserving the index seek optimization. Addresses code review W-3. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…list Add a mint-site tripwire (assert_carrier_registered) that fails closed if a ci-blob:// ref would be written to an unregistered carrier property. Make the allowlist the single source of truth shared by the mint path (blob_processor) and the reclaim scan (admin.py), removing a second hand-duplicated copy. Addresses code review W-5 (ship-now per human decision). 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Bump server 6.7.1 -> 6.8.0 (minor: new /admin/maintenance endpoint). SCHEMA_VERSION stays 1 (no stored-shape change; the rectification dedup/backfill/constraint is structural, not a schema-version step). Establish the Amplifier-consumable, OUT-OF-BAND upgrade path (first schema-adjacent change to need it): - migrations/run.py: standalone CLI. --status (read-only: constraint present? untagged/duplicate counts? server + schema version) and --apply (idempotent rectification via the SAME neo4j_store.run_repair() that /admin/maintenance calls -- one mechanism, two transports). Self-declares from->to, never runs at startup. - migrations/manifest.yaml: lean machine-readable entry (server_version 6.8.0, schema_version 1, schema_affecting false, scripts, gating, verify). - CHANGELOG.md 6.8.0 entry; README "Upgrading" section (healthy = no action; degraded = run migrations/run.py --apply OR POST /admin/maintenance, self-clears without restart), README-reachable per the discoverability rule. Tests: tests/test_migrations_run.py (13 passed). Full non-neo4j suite 1939 passed. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Product doc for the WS-3 maintenance mode: mode state machine, structured 503 + Retry-After contract, /status maintenance fields, allow-list blast radius (which routes stay up vs 503 during maintenance, and why it is intentional), enter/complete log events, the live POST/GET /admin/maintenance endpoint contract, how to clear maintenance (POST /admin/maintenance or migrations/run.py --apply -- both call run_repair; server self-clears via live re-probe, no restart), and the hard warning against wiring the maintenance signal to a k8s/ACA liveness/readiness probe. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
degraded_reason was read from app.state.schema_degraded_reason (written once at boot) while mode/schema_health already came from the live TTL constraint probe. After an in-server repair the gate/mode de-latched correctly but degraded_reason kept asserting the stale "constraint absent" sentence -- a user-facing string that contradicted the live mode. Source it from the coordinator's live reason (the same value that feeds the 503 body), so /status and the 503 stay consistent and the reason clears when the graph is healthy again. No gate/mode behavior change. Found in WS-3 DTU validation. Unit test proves it self-clears across a live constraint-absent -> present flip with no restart. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Align AGENTS.md with what this branch actually implements: "out-of-band" migration/ rectification now has two explicitly-triggered channels sharing one mechanism (run_repair) -- the standalone migrations/run.py --apply and the admin-authenticated POST /admin/maintenance -- while the server still performs zero migration work automatically (assess + advertise + gate only). 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…drop Close two coverage gaps from the review: - tag_legacy_pooled_iterations.py (374-line live-graph mutation, previously ZERO coverage): new tests/neo4j/test_tag_legacy_pooled_iterations.py proves the >=2-distinct-parents selector tags a pooled Iteration but never a clean single-parent one, that the --apply gate blocks writes on a dry run, and idempotence (a second apply tags zero additional rows). - idx_node_universal: assert run_repair DROPS the redundant fallback index after it successfully establishes the :Node uniqueness constraint (non-vacuous: the index existed before the call). Real Neo4j: 9 targeted tests pass; full tests/neo4j/ suite 119 passed, no regression. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
apply_relabel() commits per batch (CALL {} IN TRANSACTIONS), and the undo log
was written only after it returned -- so a mid-run crash left committed removals
with no undo record. Now run_apply() reads the full candidate node_id set
(read-only, same _FALSE_POSITIVE_MATCH selector), writes the undo log, THEN
mutates. The pre-mutation set is a superset of what gets removed and restore_ids
is idempotent over it, so a crash leaves a complete, safe undo record; on a clean
run candidate == touched so the reported count is unchanged.
Real Neo4j: new tests prove the undo log equals the pre-mutation candidate set
(not apply_relabel's return value) and that restore heals every node. 8 passed.
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…le (W-4)
/version returned only the compiled-in SCHEMA_VERSION and nothing ever read the
stored :SchemaMeta, so a server/graph schema-version mismatch was undetectable.
Add a read-only neo4j_store.read_graph_schema_version() (separate from the write
path -- ensure_schema_version_baseline is untouched) and surface it on GET /status
as additive advisory fields: graph_schema_version (stored) and
schema_version_current (stored == compiled expected, or null). ADVISORY telemetry
only -- it does not gate, refuse, or migrate; full mismatch handling stays in
(internal tracker). /version is unchanged (still the cheap compiled constant).
Also fixes a stale test: test_allow_listed_paths_bypass_the_gate asserted
/admin/maintenance is "!= 503", which was a false alarm once WS-3c added the route
(an unconfigured admin key legitimately 503s). It now asserts specifically that the
maintenance GATE (Retry-After + {"status":"maintenance"}) never intercepts the
allow-listed route -- order-independent.
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Scrub private issue-tracker references and a reviewer's name from code comments, CHANGELOG, AGENTS.md, and test docstrings. Substance (PR #67, commit 14a6d30, "tracked separately") is preserved; only the private tracker identifiers and personal name are removed. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…async Protocol
Isolates blob storage and OID/identity access patterns behind backend-neutral
API surfaces, enabling safe swap from disk to Azure Blob Storage without
changing callers or response contracts.
Blob storage isolation:
- Define BlobReference (uri, session_id, key, size, last_modified) as the
cheap handle; separate from actual payload (which is deserialized only
when genuinely needed).
- Redefine BlobStore Protocol with backend-neutral surface:
* write(session_id, key, value) -> BlobReference (no extra I/O)
* list(session_id) -> AsyncIterator[BlobReference] (streaming, no full-list
materialization)
* scan() -> AsyncIterator[BlobReference] (cross-session enumeration)
* delete(uri, if_unmodified=ref) -> bool (fenced compare-and-delete inside
the store, prevents TOCTOU races)
* read(uri) -> value (unchanged, bare payload for back-compat)
- AsyncDiskBlobStore implements the Protocol:
* list/scan are async generators (yield refs as os.scandir produces them;
no full-list await)
* write offloads to thread, returns ref stamped from storage stat (not
writer clock)
* delete is idempotent; conditional version re-stats before unlink in same
thread hop to minimize TOCTOU
* BlobNotFoundError (subclasses FileNotFoundError) carries only the URI, no
on-disk path (neutral for all backends)
- Collapse 3 independent AsyncDiskBlobStore construction sites into one
registry.blob_store (lazy, cached), wired to app.state for reclaim.
- Type services.py blob_store field: BlobStore | None (was Any).
Reclaim GC (POST /admin/blobs/reclaim) rewired as pure storage-API consumer:
- Deleted _OnDiskBlob and _scan_disk_blobs (direct-FS duplication).
- Uses registry.blob_store.scan() to enumerate references (no glob/stat/Path).
- Uses registry.blob_store.delete(uri, if_unmodified=ref) for fenced removal.
- No Path(settings.blob_path) re-derivation, no os.unlink outside the class.
- Added single-flight guard (409 'blob reclaim apply already running' on
concurrent apply).
OID/identity isolation:
- Privatize IdentityStore.path (self._path) and expose public exists()
method.
- Update main.py boot checks (entra_store/key_store) to use exists() instead
of reaching through .path attribute.
- Zero .path leaks repo-wide after this change.
Tests:
- test_blob_store.py: prove streaming list/scan, fenced delete, neutral
errors, reference composition.
- test_blob_processor.py: prove write returns BlobReference and URI is
correctly captured in the blob carrier.
- test_blob_isolation_tripwire.py: fail-fast test locking the boundary (no
blob-path FS op or path-in-error outside blob_store.py).
- tests/routers/test_blob_reclaim_single_flight.py: prove apply guard (409 on
concurrent, scan runs exactly once).
- test_identity_store.py, test_main.py: identity isolation (exists() call,
no .path reach-through).
Non-Neo4j suites: 339 passed. Neo4j suite (test_blob_reclaim.py, 16 tests)
deferred to CI (requires real Neo4j; proves end-to-end reclaim parity).
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Blob storage and OID/identity isolation work, stacked on #70 because that PR already modifies the same files and introduces the reclaim endpoint this work builds on.
What this does
Isolates blob storage and OID/identity subsystems behind backend-neutral API surfaces, enabling safe swap from disk to Azure Blob Storage later without changing any callers or response contracts.
Blob storage isolation:
BlobReference(uri, session_id, key, size, last_modified) as the cheap handle (no payload).BlobStoreProtocol:write() -> BlobReference(no extra I/O beyond the write itself)list() / scan() -> AsyncIterator[BlobReference](streaming, no full-list await)delete(uri, if_unmodified=ref) -> bool(fenced compare-and-delete inside the store, prevents TOCTOU)read(uri) -> value(unchanged, back-compat)BlobNotFoundError(no on-disk path in errors — neutral for all backends)registry.blob_storeinstance (collapsed 3 construction sites).POST /admin/blobs/reclaim) rewired as pure storage-API consumer (no direct FS access).OID/identity isolation:
IdentityStore.pathand expose publicexists()method..pathreach-throughs repo-wide.Data integrity fix:
tool_call_iddisambiguator (matches graph Event node id), preventing parallel same-millisecond events from silently overwriting each other's blobs.Test status
Non-Neo4j suites: 339 passed (blob_store, blob_processor, blob_carrier_allowlist, services, registry, m2_service_auth, main, pipeline, routers/test_blob_reclaim_single_flight).
tests/neo4j/test_blob_reclaim.py(16 tests) requires a real Neo4j instance and MUST pass in CI before merge to prove end-to-end reclaim parity.Re-targeting
Once #70 merges to main, re-target this PR's base from
fix/context-intelligence-data-quality-phase2tomainso the diff shows only these isolation changes (not #70's).Related
Depends on: #70 (reclaim endpoint, same-file modifications)