Add optional turbopuffer-style object-storage vector search backend

[JSv4]

Overview

Adds an opt-in, reversible vector search backend that keeps its index entirely in object storage, following turbopuffer's architecture: object storage as index source of truth, per-namespace WAL with strongly consistent reads (unindexed tail scanned as an overlay), async compaction into immutable centroid-ANN segment generations, and tiered caching for warm queries. Default behavior is unchanged — VECTOR_SEARCH_BACKEND=pgvector remains the default, and the object-storage path falls back to pgvector on unindexed namespaces, engine errors, or post-filter shortfalls.

Design doc: docs/architecture/object_storage_vector_search.md (includes the build-on-turbopuffer vs from-scratch analysis and prior-art comparison — OpenSearch searchable snapshots, Quickwit — from the referenced HN discussion).

Flag on feasibility (as requested): turbopuffer is closed-source SaaS, so there is nothing to self-host; this is a from-scratch implementation of the same architecture (~500 lines of numpy over blob-store primitives). A hosted-turbopuffer API driver could later slot into the same search_via_object_index() / enqueue_embedding_index_sync() seam.

How it works

Embedding.objects.store_embedding()  ── on_commit ──▶ Celery WAL append (one PUT)
  (+ worker_uploads bulk writes, hooked explicitly)
<prefix>/<ns>/wal/*.jsonl  ── auto-compaction (cache-lock, k-means, k=√n) ──▶
<prefix>/<ns>/index/segments/<gen>/{centroids.npy, cluster_*.npz} + manifest.json
VectorSearchViaEmbeddingMixin.search_by_embedding()  ◀── router probes nprobe
clusters + WAL tail, then RE-FILTERS candidate ids through the caller's queryset

• Integration seam: VectorSearchViaEmbeddingMixin.search_by_embedding (opencontractserver/shared/mixins.py). Scope: document, annotation, and note search — the parent kinds whose querysets inherit the mixin — so GraphQL semantic search, Discover, MCP search_corpus passages, agent tools, and hybrid RRF over those kinds inherit the toggle with zero caller changes. Conversation/ChatMessage/Relationship vector search deliberately stays on pgvector for now (their reads use separate inline implementations with a different result contract; documented follow-up — see EMBEDDABLE_PARENT_KINDS), and their writes are correspondingly not fanned out to the index.
• Permissions preserved by construction: the engine stores only (parent_pk, vector); candidate ids are re-filtered through the caller's already-scoped queryset (visible_to_user, MIN(document, corpus), structural rules), with capped oversampling and a shortfall rule that falls back to pgvector rather than under-filling. Deleted rows drop out at the same re-filter. The index blobs carry no ACL — system check opencontracts.W004 warns when the backend is enabled over storage with public-read signals (AWS + GCS).
• Write path: fan-out from Embedding.objects.store_embedding (the in-app pipeline chokepoint) plus explicit hooks in worker_uploads/tasks.py's bulk writes, via opencontractserver/tasks/vector_index_tasks.py, with autoretry and pending-marker-gated auto-compaction (cache-lock serialised, deferred GC by one generation); fire-and-forget — an index sync failure never breaks the Postgres write.
• Ops: manage.py rebuild_object_vector_index (batched, --dry-run, progress, lock-aware) replays Postgres embeddings into the index; system checks opencontracts.E002 (invalid backend value) and W003 (process-local cache vs compaction lock); tuning constants in opencontractserver/constants/search.py.

Testing

• 38 tests (opencontractserver/tests/test_object_storage_vector_backend.py): engine-level (WAL-only strong consistency, compaction parity with numpy brute force, tombstones, overwrite ordering, determinism, recall, manifest mid-overwrite retry, WAL/manifest read-ordering race, deferred/partial GC, put_bytes overwrite races) and Django integration (toggle both ways, queryset scoping, all fallback paths incl. filter shortfall and fetch-cap, end-to-end CoreAnnotationVectorStore caller, worker-upload direct-write fan-out, rebuild command incl. dry-run + lock contention, auto-compaction gating, read/write kind symmetry, E002/W003/W004 checks).
• Regression: existing search/embedding suites (incl. conversation, subtree-group, and worker-upload suites) and the 92 architecture-invariant tests — all pass.
• Real object-storage verification: engine exercised end-to-end against MinIO via the S3 API (docs/test_scripts/object_storage_vector_backend_minio.md) and fake-gcs-server via the GCS JSON API through django-storages GoogleCloudStorage (docs/test_scripts/object_storage_vector_backend_gcs.md) — all three supported storage backends (LOCAL/AWS/GCP) verified.

Limitations / future work (documented in the design doc)

Vector arm only (FTS/hybrid RRF unchanged in Postgres); document/annotation/note kinds only (conversation/message/relationship read paths are the named follow-up); async write-visibility window (Celery lag vs turbopuffer's synchronous ack); no group commit (~3 object-store roundtrips per online write — bulk jobs should use the rebuild command); lazy deletes; wall-clock WAL ordering under worker clock skew; non-atomic manifest overwrite (retry-mitigated; S3 conditional PUT named as the permanent fix); full recluster per compaction; per-process cache only; deployment-wide (not corpus-sharded) namespaces.

[claude]

Review: Object-storage vector search backend

Nicely scoped, opt-in feature with a clear single integration seam (search_by_embedding), good fallback design, and a strong test suite for the sequential-execution paths. One correctness issue stood out that I think is worth fixing before this ships, plus a few smaller notes.

Correctness: search()/compact() read order can silently lose recently-written data during concurrent compaction

opencontractserver/vector_search/engine.py:213-214 (and again at :259-260 in compact()):

manifest = self._load_manifest(namespace)
wal_names = self._list_wal(namespace)

compact() commits the new manifest first, then deletes the folded WAL files as best-effort GC (engine.py:319-333 — "Single PUT = the atomic commit point," GC happens after). That write order means a correct reader must observe the WAL before the manifest, not after — otherwise there's a window where:

1. A search() call reads the old manifest (generation G, just before compaction commits G+1).
2. Compaction completes: commits G+1 (which folds WAL files W1, W2), then deletes W1, W2.
3. The same search() call now lists the WAL directory — W1/W2 are already gone.

The reader ends up with old manifest G (missing the data that was only in W1/W2) and an empty WAL overlay (those files are deleted) — the ids in W1/W2 vanish from the result entirely, with no error, no fallback, no log. This directly contradicts the "strongly consistent... a write is searchable the moment its WAL PUT returns" claim in docs/architecture/object_storage_vector_search.md and the search() docstring.

Swapping the read order to list WAL first, then load the manifest fixes this: since the write path always commits the manifest strictly before deleting WAL files, a reader that lists the WAL before reading the manifest is guaranteed that either (a) the WAL files are still present and the manifest read is stale-or-current — both cases are covered by the overlay, or (b) the WAL files are already gone, which (by write ordering + read-after-write consistency of the underlying store) guarantees the manifest read must already reflect the generation that folded them. This is a small, local change but worth a targeted regression test — e.g. a test that manually interleaves _load_manifest/_list_wal/compact() calls (mock or call them out of the normal sequence) to lock in the fix, since the existing 18 tests are all single-threaded/sequential and don't exercise this interleaving.

Secondary notes (not blocking)

• Immediate GC of the old generation (engine.py:329-333) deletes the previous generation's segment files right after the manifest swap, with no grace period. Even after the ordering fix above, a reader that reads the old manifest just before the swap but then pauses (GC, scheduling) before fetching a cluster file can hit ObjectNotFound on that fetch. This is caught by router.search_via_object_index's broad except Exception and gracefully falls back to pgvector for that query, so it's not data-loss — but under sustained write/compaction load it could mean a steady trickle of full pgvector fallbacks, quietly undermining the perf benefit. Worth a comment noting this is accepted, or a short deferred-delete window if it turns out to matter in practice.
• Compaction lock relies on a shared cache backend (vector_index_tasks.py compact_object_vector_namespace, cache.add). This is correct given django_redis in production, but the compact() docstring's "NOT concurrency-safe with itself" invariant is silently violated if a deployment ever runs Celery workers as separate processes against LocMemCache (e.g. local.py falls back to LocMemCache when REDIS_URL is unset). Might be worth a startup check (similar to opencontracts.E002) or at least a doc callout that a shared cache backend is a hard requirement for VECTOR_SEARCH_BACKEND=object_storage.
• Sequential WAL GETs in _load_wal_overlay (engine.py:354-376) fetch each WAL file one at a time. Since the WAL tail is bounded by OBJECT_INDEX_COMPACT_MIN_WAL_FILES (16) before auto-compaction fires, every query near that threshold pays up to 16 sequential object-storage round trips plus nprobe cluster loads. Given these are I/O-bound GETs, fetching them concurrently (e.g., a small thread pool) would meaningfully cut tail latency — not urgent, but worth a follow-up if cold-query p50 turns out higher than the ~500ms–1s documented target.
• Minor DRY nit: the parent-kind string set (document, annotation, note, conversation, message, relationship) is duplicated as literals across router.PARENT_KIND_BY_MODEL_NAME and vector_index_tasks.PARENT_FK_TO_KIND. Different key spaces (model name vs. FK attribute), so full consolidation isn't trivial, but a shared constant for the kind values would guard against drift if a kind is ever renamed.

What looks solid

• Permission model is sound: the engine only ever stores (parent_pk, vector), and candidate ids are always re-filtered through the caller's already-scoped queryset before being returned — this correctly reuses the existing visible_to_user/MIN(document, corpus) machinery rather than reimplementing it, in line with the project's permissioning guide.
• Fallback semantics are consistent and well-tested: unindexed namespace, engine exception, and default-off all correctly avoid touching the object-storage path or (conversely) never touch pgvector's CosineDistance when the object index serves the hit — verified via mock.patch(..., side_effect=AssertionError) in the integration tests, a nice way to assert "the other path was never touched."
• store_embedding's write-path fan-out (Managers.py) is a clean single chokepoint — both the update and create/IntegrityError-retry branches now converge on one enqueue_embedding_index_sync call.
• System check opencontracts.E002 for invalid VECTOR_SEARCH_BACKEND values is a good catch-misconfiguration-at-boot pattern, consistent with the existing E001 check.
• Docs are thorough and honest about limitations (lazy deletes, no group commit, full recluster per compaction) — appreciated given CLAUDE.md's guidance to keep docs current and not oversell.

Overall: solid architecture and test coverage for the happy paths; the manifest/WAL read-order issue is the one item I'd want addressed (or explicitly accepted with reasoning) before enabling this in production, since it's a silent-correctness bug in a feature whose entire pitch is strong consistency.

[claude]

Review: Optional object-storage vector search backend

Solid piece of engineering — the mixin-level integration seam (VectorSearchViaEmbeddingMixin.search_by_embedding) means every caller (GraphQL, MCP, agent tools, hybrid RRF) inherits the toggle for free, permission re-filtering through the caller's own queryset is done correctly and matches the codebase's MIN(document, corpus) scoping rules, and the fail-safe design (any engine error or unindexed namespace transparently falls back to pgvector) means enabling the flag genuinely can't make things worse. Test coverage is good — engine-level (WAL consistency, compaction/brute-force parity, tombstones, determinism, recall) plus Django-integration (toggle, scoping preservation, both fallback paths, rebuild command, auto-compaction). Changelog fragment and design doc both follow repo conventions.

A few things worth a look before/after merge:

Correctness / design risks

1. Compaction GC races with concurrent readers (opencontractserver/vector_search/engine.py::compact). Right after the new manifest is PUT (the atomic commit point), the previous generation's centroids.npy/cluster_*.npz blobs are deleted immediately. A reader that loaded the old manifest a moment earlier (in search()) can then try to fetch a centroid/cluster blob for that now-deleted generation and get ObjectNotFound. It's caught by the blanket except Exception in router.search_via_object_index, so it degrades to a pgvector fallback rather than corrupting results — but since auto-compaction fires every OBJECT_INDEX_COMPACT_MIN_WAL_FILES (16) WAL files on active namespaces, this means every compaction cycle can produce a burst of spurious fallbacks + logger.exception noise for any query racing it. Worth considering a short grace period (e.g. only GC generation N-1 after starting generation N+1, or a brief delay before deleting) rather than immediate deletion.

2. "Strong consistency" framing is a bit optimistic for the write path. The design doc says a write is "searchable the moment its WAL PUT returns," mirroring turbopuffer's guarantee — but the WAL PUT itself happens inside a Celery task dispatched via transaction.on_commit(...).apply_async() (vector_search/hooks.py), not synchronously with store_embedding(). There's a real (queue-depth-dependent) window between the Postgres write returning and the vector actually landing in the WAL, during which a search against an already-indexed namespace will silently omit the just-written vector (no error — just a false negative for that one row). Might be worth an explicit callout in the design doc's "Current limitations" section, since it's a real divergence from turbopuffer's synchronous-client-write model.

3. Distributed WAL ordering relies on wall-clock timestamps (_append_wal uses time.time_ns()) for last-writer-wins semantics across Celery workers. test_later_wal_upsert_wins only exercises this in-process/sequentially, so clock skew between workers (which could let an older re-embed "win" over a newer one) isn't covered. Likely low real-world impact given embeddings are re-generated infrequently per parent, but worth a line in the limitations doc alongside the other documented caveats.

Minor

4. DjangoStorageObjectStore.put_bytes's collision-recovery path (exists → delete → save, then read-back/rewrite on a detected race) isn't airtight: if a third writer deletes the colliding blob between the read-back and rewrite, self._storage.open(saved_as, "rb") raises an uncaught FileNotFoundError. In practice this key is only mutated by the manifest write, which is normally serialized by the per-namespace compaction cache-lock — so the window only opens if a compactor overruns the 600s lock TTL and a second one starts concurrently. Edge case, but worth a guard/comment given it's presented as the safety net for concurrent writers.

5. No test for the new opencontracts.E002 system check (opencontractserver/shared/checks.py::check_vector_search_backend). The sibling E001 check has coverage per repo convention — a quick SimpleTestCase asserting an invalid VECTOR_SEARCH_BACKEND value trips the check would close the gap.

6. get_default_engine() (vector_search/router.py) is a lazy module-global singleton with no lock — a classic double-checked-locking gap. Harmless today (the engine is stateless besides its own thread-safe LRU cache), but flagging in case it grows meaningfully-stateful fields later.

7. Nit: the config/settings/base.py diff leaves a double blank line after the new VECTOR_INDEX_STORAGE_PREFIX block — pre-commit will likely reformat it.

None of the above block merging given the default-off posture and universal fallback-on-error behavior, but #1 and #2 seem worth at least a doc update, and #1 might be worth a small follow-up fix (delayed GC) if compaction ends up running frequently in practice.

[JSv4]

Both reviews addressed in the latest commits. The blocking finding — the manifest/WAL read-order race — was real and is fixed, along with the adjacent GC races:

Fixed

• search()/compact() now list the WAL before reading the manifest (the dangerous interleave was old-manifest + already-GC'd tail = silent data loss). Locked in by a regression test that runs two full compaction cycles between the two reads (test_search_survives_compaction_between_wal_list_and_manifest_read).
• The manifest now records folded_wals; readers/compactors skip those files when building the overlay, so a lingering folded file (failed or late GC) can never replay stale entries over newer segments or resurrect tombstoned ids (test_partial_gc_failure_cannot_resurrect_tombstoned_ids).
• Deferred GC by one compaction cycle (both reviews' old-generation-GC concern): committing generation N+1 deletes only what generation N superseded, so in-flight readers holding manifest N can still fetch every blob it references — no more ObjectNotFound → spurious pgvector fallback bursts per compaction. wal_tail_count now counts only unfolded files so deferred GC can't re-trigger auto-compaction forever (test_deferred_gc_reclaims_prior_cycle, test_wal_tail_count_ignores_folded_lingering_files).
• put_bytes collision recovery now tolerates a third writer deleting the uniquified blob mid-recovery (falls back to the payload in hand).
• New system check opencontracts.W003 warns when VECTOR_SEARCH_BACKEND=object_storage runs over a process-local cache (LocMemCache/dummy), where the compaction cache-lock can't serialise compactors across workers; E002 and W003 both now have tests.

Documented as accepted limitations (design doc "Current limitations"): the async write-visibility window (WAL PUT happens in an on-commit Celery task, unlike turbopuffer's synchronous ack), wall-clock WAL ordering under worker clock skew, and sequential WAL-tail GETs (concurrent fetch listed as follow-up).

Not taken: consolidating the parent-kind literals across router/vector_index_tasks (different key spaces, as noted — the values are pinned to Embedding's FK names and covered by tests); locking get_default_engine (documented why the unlocked lazy init is safe today). The base.py blank-line nit didn't materialize — pre-commit passes.

Suite is now 25 tests; all 154 pre-existing search/embedding regression tests and the MinIO real-S3 smoke run still pass.

---

Generated by Claude Code

[claude]

Review: Object-storage vector search backend (turbopuffer-style)

Overall this is a well-scoped, well-documented, opt-in feature. Default behavior is unchanged (VECTOR_SEARCH_BACKEND=pgvector), the single integration seam (VectorSearchViaEmbeddingMixin.search_by_embedding) is a good choke point, permission re-filtering through the caller's own queryset is the right approach, and the WAL/manifest read-ordering + deferred-GC design is subtle but backed by targeted regression tests (test_search_survives_compaction_between_wal_list_and_manifest_read, test_partial_gc_failure_cannot_resurrect_tombstoned_ids, test_deferred_gc_reclaims_prior_cycle) that clearly demonstrate the author understood the race conditions involved.

Correctness

• opencontractserver/vector_search/object_store.py::put_bytes — the uniquify-recovery branch is fragile under concurrency. When save() races and returns saved_as != name, the recovery deletes whatever now lives at name (which could be another writer's legitimate, already-committed blob), reads back the payload from saved_as, and does one more save(name, ...). That final save() is not re-checked — if a third concurrent writer creates name again between the recovery's delete and its final save, Django's storage will silently uniquify the name again and the intended overwrite is lost (the "true" name key ends up holding stale content). The recovery only handles one level of contention, not the general race. In practice this is mitigated by compact_object_vector_namespace's per-namespace cache lock (manifest.json is the only key this path really protects), but it's worth either looping until saved_as == name or explicitly documenting that only single-level contention is handled and why 2+-way races are believed impossible given the locking design.
• Related: the recovery reads the payload back from saved_as via self._storage.open(...) instead of just reusing the in-memory data it already has (the except FileNotFoundError: payload = data fallback shows data was available the whole time). This adds an unnecessary GET/round-trip and complexity without a corresponding benefit that I can see — consider simplifying to skip the read-back.
• "Flipping the flag can never make search worse than pgvector" is slightly overstated for heavily-filtered querysets. search_via_object_index does post-ANN permission filtering with a fixed oversample (OBJECT_INDEX_FILTER_OVERSAMPLE = 4, opencontractserver/constants/search.py), then returns whatever survives — it does not retry with a larger fetch on shortfall. For a namespace where the caller's queryset is very selective (e.g. a document visible to very few users within a large corpus), the object-storage path can legitimately return fewer than top_k results where pgvector's SQL-driven filter+limit would not. This is implicitly acknowledged in the docs ("oversampling ... to keep heavily filtered querysets full") but the stronger "never worse" claim in both the PR description and object_storage_vector_search.md could mislead an operator about recall guarantees. Worth softening the doc language or adding a fallback/retry when candidate count < top_k.

Performance

• OBJECT_INDEX_COMPACT_LOCK_TIMEOUT_SECONDS = 600 is a fixed 10-minute ceiling on the compaction mutex. The design doc notes the "full recluster per compaction ... fine up to ~10⁶ vectors, revisit beyond" — but doesn't connect that scaling limit to the lock timeout. If a namespace's k-means compaction ever exceeds 10 minutes, the lock expires and a second compactor can start concurrently, producing racing manifest writes that lean on the put_bytes recovery path flagged above. Worth a comment tying the two limits together, or making the timeout self-adjusting (e.g. proportional to vector count).
• Cold-path search() does the WAL-tail scan sequentially (one GET per unfolded WAL file) and the final ranking (sorted(scored.items(), ...)) is pure Python. This is already called out as future work in the design doc, so just flagging it's consistent with what's documented — no action needed unless nprobe/oversample defaults get tuned upward.
• Minor nit: fetch_n = max(top_k, top_k * OBJECT_INDEX_FILTER_OVERSAMPLE) (router.py) — since the oversample constant is always ≥ 1, the max() is redundant (top_k * OBJECT_INDEX_FILTER_OVERSAMPLE >= top_k always holds for non-negative top_k). Harmless, just dead code.

Test coverage

• The 18 new tests are thorough for the engine's core WAL/compaction/consistency guarantees and the Django integration (toggle, fallback, scoping, rebuild command, auto-compaction). Good use of SimpleTestCase for the pure-engine tests to keep them fast.
• Gap: DjangoStorageObjectStore.put_bytes's uniquify-recovery branch (the saved_as != name path flagged above) has no direct test — it's the most concurrency-sensitive piece of object_store.py and currently only exercised implicitly (if at all) via the higher-level engine tests, none of which appear to force a save() race. A test that mocks Storage.save to return a uniquified name once would pin down the intended last-writer-wins behavior and guard against regressions.
• Gap: no automated test for the "heavily filtered queryset returns fewer than top_k" scenario described above — would be a good regression test to document the actual (rather than idealized) behavior.

Security

• Permission model looks correct: the engine only ever stores (parent_pk, vector), and every read path re-filters candidate ids through the caller's already-scoped queryset before returning instances — so visible_to_user/MIN(document, corpus)/structural rules are enforced by the same code as the pgvector path. Deleted rows are naturally dropped by the same re-filter.
• build_namespace sanitizes embedder_path via regex + an MD5 digest suffix before it becomes part of a storage key, which avoids path-traversal via embedder path strings. All other key components (parent_kind, dimension, generation numbers, WAL filenames) are internally generated, not user-controlled.
• No new user-facing surface is added (management command + system checks are ops-only); GraphQL/MCP/agent callers are unaffected beyond inheriting the toggle.

Nits

• docs/test_scripts/object_storage_vector_backend_minio.md records "Last verified: 2026-07-07" — per repo convention this is a good practice to keep current; just flagging it'll need updating if the engine changes materially.
• check_vector_search_cache (opencontractserver/shared/checks.py) does a substring match ("locmem" in default_cache.lower()) rather than an exact class check — reasonable heuristic for a warning-level check, no change needed.

Nice work overall — the design doc is unusually thorough (including the "why not build on turbopuffer directly" analysis and prior-art comparison), the fallback-to-pgvector safety net is well-reasoned, and the WAL/manifest ordering tests show real care about the hard concurrency edges. The object_store.py recovery path is the one spot I'd want a second look at before this sees real multi-worker production traffic.

[JSv4]

Third-round findings addressed:

• put_bytes recovery — rewritten as a bounded delete-then-save retry loop: each attempt discards our stray uniquified blob and retries until the save lands at the exact key (terminal state is always exactly one blob at the key); after OBJECT_STORE_PUT_OVERWRITE_MAX_ATTEMPTS we concede last-writer-wins to the racing writer and log — contention on the manifest means the compaction lock was breached, and their blob is a valid manifest. The read-back GET is gone (you were right that it bought nothing). Direct test added (test_put_bytes_retries_when_racing_writer_uniquifies_save) forcing a save() uniquification and asserting final content + no strays.
• "Never worse than pgvector" for heavily-filtered querysets — now actually true rather than aspirational: if re-filtering consumes a truncated candidate set (engine returned a full fetch_n) without filling top_k, the router falls back to pgvector; a short result from an exhausted namespace is genuinely complete and returned as-is. Regression test swamps the candidate set with 10 other-user docs and asserts the caller still gets their 2 docs via the fallback. Docs updated to state the rule instead of hand-waving the oversample.
• Lock TTL ↔ recluster scaling — comment on OBJECT_INDEX_COMPACT_LOCK_TIMEOUT_SECONDS now ties the 600s ceiling to the ~1M-vectors-per-namespace note and names the failure mode if breached.
• Redundant max() in fetch_n dropped.

Suite is now 28 tests; all regression suites remain green.

---

Generated by Claude Code

[claude]

Review: Object-storage vector search backend

Went through engine.py, router.py, object_store.py, hooks.py, vector_index_tasks.py, checks.py, the Managers.py/mixins.py integration points, the rebuild command, and the design doc/tests. Overall this is a well-scoped, opt-in feature with a good safety net -- nice work on the 3 rounds of self-review that already hardened the WAL/manifest read-order race, deferred GC, put_bytes overwrite race, and the filter-shortfall fallback. A few things worth a look:

Security -- vector blobs bypass the permission layer if the storage bucket is ever exposed

docs/architecture/object_storage_vector_search.md's "Permissions (critical)" section documents the ORM re-filter that protects query results, but the raw index data now lives as directly-addressable object-storage keys (vector-index/<namespace>/wal/*.jsonl, .../segments/<gen>/*) with no ACL of its own -- by design, per engine.py's docstring ("stores only (parent_pk, vector) -- no ACLs"). That's fine as long as default_storage's bucket is never independently readable (e.g. via a public MEDIA_URL/CDN path used elsewhere for thumbnails, or a misconfigured bucket policy). If it ever is, anyone with bucket read access could enumerate/download raw vectors + parent_pk ids for every document/annotation/note in the system -- including private ones -- entirely outside GraphQL/Django auth, which is a materially larger blast radius than pgvector ever had (vectors never left Postgres+app layer there). Worth an explicit doc note ("this prefix MUST live in a non-public storage location") or, better, a startup check similar to W003 that warns if VECTOR_SEARCH_BACKEND=object_storage and the default storage backend is configured for public read (if that's inferable from settings).

Correctness -- Embedding-row deletes aren't tombstoned (documented, but worth double-checking)

router.py/design doc correctly handle the parent-object-deleted case (cascade delete removes the parent row, so the ORM re-filter drops the stale id -- covered by test_deleted_rows_dropped_by_orm_refilter). But if an Embedding row is ever deleted or replaced independent of its parent (e.g. a future "delete stale embeddings for an old embedder version" cleanup command, or a re-embed flow that removes rather than updates), the object index has no way to hear about it -- the stale vector keeps matching and the (still-alive) parent object would resurface in results indefinitely until a manual rebuild_object_vector_index. I confirmed no code path in this PR or the existing codebase currently deletes an Embedding row without cascading from its parent, so this isn't exploitable today -- but it's a silent trap for whoever adds that cleanup command later, since nothing here would signal that the index needs a corresponding tombstone. Might be worth a one-line comment on EmbeddingManager/wherever embeddings get deleted in the future, pointing back to this doc section.

Performance -- per-embedding write cost

sync_embedding_to_object_index does a WAL PUT, then unconditionally calls wal_tail_count() (a list_keys + manifest GET) to decide whether to trigger compaction -- so every single embedding write costs 3 object-store round trips, fired per-Celery-task per-embedding. This is already called out under "no group commit" in the limitations section, but worth flagging explicitly for anyone running a large backfill/bulk-embedding job through the normal write path (as opposed to rebuild_object_vector_index, which batches) -- expect meaningfully higher object-store request volume/cost than the pgvector path during those jobs.

Nit

_kmeans's empty-cluster reseed (engine.py:120-121) picks the single globally-worst-fit point (vectors[np.argmin(best_sims)]) each time it re-seeds -- if two clusters go empty in the same iteration they'll momentarily be reseeded to the identical point. Self-corrects within the fixed 8 iterations and doesn't affect correctness (just a slightly wasted iteration), so not blocking, just noting in case it ever bites at very small k.

What looks solid

• The WAL-before-manifest read ordering + folded_wals + deferred one-cycle GC in engine.py is a genuinely correct design for the described race, and the regression tests (test_search_survives_compaction_between_wal_list_and_manifest_read, test_partial_gc_failure_cannot_resurrect_tombstoned_ids, test_deferred_gc_reclaims_prior_cycle) target exactly the right scenarios.
• The fallback-to-None-on-any-exception pattern in router.search_via_object_index and the filter-shortfall rule together make the "never worse than pgvector" claim credible, and it's backed by a test (test_filter_shortfall_falls_back_to_pgvector).
• opencontracts.E002/W003 system checks are a nice touch for catching misconfiguration at boot rather than silently degrading.
• Single integration seam (VectorSearchViaEmbeddingMixin.search_by_embedding) keeps the blast radius of the whole feature to one call site, and the write-path fan-out through Embedding.objects.store_embedding similarly avoids scattering hooks across every embedding producer.

Nothing here should block merge -- the storage-exposure point is the one I'd actually want a confirmation on before flipping the flag on in production.

[JSv4]

Fourth-round findings addressed:

• Storage exposure (the confirmation you asked for) — the requirement is now explicit and enforced-as-warning: the design doc's Permissions section states the index prefix must live in non-publicly-readable storage and why (no per-blob ACL; a readable bucket leaks raw vectors + parent ids outside Django auth, a larger blast radius than pgvector). New system check opencontracts.W004 warns at boot when VECTOR_SEARCH_BACKEND=object_storage runs over an AWS config showing public-read signals (AWS_DEFAULT_ACL=public-read*, AWS_QUERYSTRING_AUTH=False, or AWS_S3_CUSTOM_DOMAIN), with tests covering each signal plus the private/LOCAL/pgvector negative cases. Note the repo's default AWS config (AWS_QUERYSTRING_AUTH=True, no default ACL) passes clean.
• Embedding-row deletion trap — flagged in both places a future author would look: EmbeddingManager's docstring now warns that deleting an Embedding row independent of its parent must tombstone the namespace entry (ObjectStorageVectorEngine.delete) or trigger a rebuild, and the design doc's lazy-deletes limitation spells out the same.
• Per-write cost — the "no group commit" limitation now quantifies it (~3 object-store roundtrips per store_embedding) and directs bulk backfills to rebuild_object_vector_index, which batches hundreds of vectors per PUT.
• k-means reseed nit — empty clusters now reseed from successive worst-fit points, so multiple empty clusters in one iteration can't collapse onto the same seed.

Suite is now 29 tests; regression suites remain green.

---

Generated by Claude Code

[claude]

Review: Optional turbopuffer-style object-storage vector search backend

This is a large, well-scoped addition (~2200 lines) that's opt-in and defaults to the existing pgvector path. I read through engine.py, object_store.py, router.py, hooks.py, the Managers.py/mixins.py integration points, the new system checks, the Celery tasks, the rebuild command, and the 18 new tests. Overall this is unusually thorough for a first pass — the WAL-before-manifest read ordering, deferred GC, and shortfall-fallback logic all have dedicated regression tests that specifically target the races they're designed to survive (test_search_survives_compaction_between_wal_list_and_manifest_read, test_partial_gc_failure_cannot_resurrect_tombstoned_ids, test_filter_shortfall_falls_back_to_pgvector). Nice work reasoning through the consistency model up front rather than discovering it via production incidents.

Correctness

• WAL-first read ordering is correctly implemented and tested. engine.search() and wal_tail_count() both list the WAL before reading the manifest, matching the documented invariant, and the deferred-GC-by-one-cycle logic in compact() (storing prior_generation in the new manifest, then deleting what the previous manifest superseded) is correctly wired and covered by test_deferred_gc_reclaims_prior_cycle.
• Permission preservation — the search_via_object_index() re-filter through the caller's own queryset (opencontractserver/vector_search/router.py) is the right design; it can't silently bypass visible_to_user/MIN(document, corpus) scoping since it reuses whatever queryset the caller already built. test_queryset_scoping_is_preserved and test_deleted_rows_dropped_by_orm_refilter back this up.
• Fallback semantics look sound: unindexed namespace returns None, engine exception is caught broadly and logged then returns None, and the 'filter shortfall' case (truncated candidate set + under-filled top_k) also falls back rather than silently returning fewer/worse results than pgvector would. Good defensive design, matches the 'never worse than pgvector' claim in the PR description.

Minor issues / things worth a second look

1. Compaction-storm potential under sustained write bursts (opencontractserver/tasks/vector_index_tasks.py, sync_embedding_to_object_index): a new compact_object_vector_namespace.apply_async() fires on every write once wal_tail_count >= OBJECT_INDEX_COMPACT_MIN_WAL_FILES, not just on the write that crosses the threshold. The cache lock makes this safe (redundant compactions just no-op), but a hot namespace during a bulk backfill through the normal write path (not rebuild_object_vector_index) will queue one compaction task per embedding write once past the threshold — needless Celery churn. Worth gating on 'only enqueue if this write was the one that crossed the threshold,' or explicitly accepting it as a tradeoff.
2. GCS/GoogleCloudStorage path is untested. The design doc and test plan verify local FileSystemStorage and S3 (via real MinIO), but object_store.py's docstring also advertises GCS support via STORAGE_BACKEND=GCP. listdir() semantics differ slightly across django-storages backends (GCS is a flat namespace with prefix-emulated 'directories'); worth at least a smoke test or an explicit doc note that GCS is unverified, since list_keys() is load-bearing for both wal_tail_count() and the WAL overlay.
3. opencontracts.W004 only checks AWS-specific signals. check_vector_index_storage_exposure gates on STORAGE_BACKEND == AWS and inspects AWS_DEFAULT_ACL/AWS_QUERYSTRING_AUTH/AWS_S3_CUSTOM_DOMAIN. If a deployment uses GCS (STORAGE_BACKEND=GCP) with a public bucket, this check silently passes even though the same 'unauthenticated read leaks raw vectors + parent ids' risk applies. Given the check exists specifically to catch this class of misconfiguration, an equivalent GCS signal (e.g. GS_DEFAULT_ACL, GS_QUERYSTRING_AUTH) would close the gap, or the docstring/warning text should note it's AWS-only today.
4. get_default_engine()'s unlocked lazy init is fine today but is a latent footgun if the engine ever grows real mutable state beyond the LRU cache — the comment calls this out explicitly, which is good, but a TODO/issue reference would help it survive future refactors.
5. rebuild_object_vector_index has no --dry-run or progress reporting for very large tables — it iterates the entire Embedding table with .iterator(chunk_size=batch_size) and only prints a summary at the very end per namespace. For a large production dataset this could run silently for a long time with no feedback. Not a correctness issue, just an operability nit given this is explicitly the tool operators run before flipping the flag in prod.

Security

• The permissions design (storage carries no ACL, all filtering is ORM-based post-ANN) is called out explicitly in the design doc, and the new opencontracts.W004 check is a good mitigation for the most likely misconfiguration (public/unsigned S3 bucket). See point 3 above for the GCS gap in that same check.
• hashlib.md5(..., usedforsecurity=False) for the namespace slug digest is appropriate — it's a collision-avoidance suffix, not a security boundary, and the usedforsecurity=False flag correctly documents that intent for FIPS-mode environments.

Test coverage

Coverage is strong for a first PR of this scope: WAL-only consistency, compaction/brute-force parity, tombstones, overwrite-race handling in DjangoStorageObjectStore.put_bytes, all three system checks, and the Django-level toggle/fallback/scoping integration tests. Two gaps worth calling out (not blocking):

• No test exercises sync_embedding_to_object_index racing a concurrent delete + re-create under the same (embedder_path, parent) — low priority given it's an edge case already handled defensively (row-not-found -> skip).
• No test for the GCS backend path (see point 2).

Style / conventions

• Follows CLAUDE.md conventions well: constants centralized in opencontractserver/constants/search.py (no magic numbers), changelog fragment added under changelog.d/, design doc added and cross-linked from two other docs, single integration seam (search_by_embedding) rather than scattering backend checks across callers.
• The 'late import' pattern in mixins.py and hooks.py to avoid import-order coupling is explained inline — appreciated, since this is exactly the kind of thing that's confusing without the comment.

Nothing here blocks merging; the flagged items are mostly hardening/operability suggestions for a backend that's disabled by default and falls back safely on any failure.

[claude]

Review: Object-storage vector search backend

Reviewed the diff (origin/main...PR head, 19 files, ~2200 additions) — engine.py, router.py, object_store.py, hooks.py, vector_index_tasks.py, checks.py, Managers.py, the rebuild command, settings, and the 18-test suite. Overall this is a very well-engineered, carefully documented piece of infrastructure.

Strengths

• The WAL-list-before-manifest-read ordering in engine.py::search() is subtle and exactly right, and is backed by a dedicated regression test (test_search_survives_compaction_between_wal_list_and_manifest_read) that actually interleaves two compaction cycles between the two reads — a genuinely hard race to test for, done well.
• Deferred GC (one compaction cycle of grace) avoids ObjectNotFound races for in-flight readers holding a stale manifest, covered by test_partial_gc_failure_cannot_resurrect_tombstoned_ids / test_deferred_gc_reclaims_prior_cycle.
• search_via_object_index's "shortfall rule" (fall back to pgvector if oversampled candidates were exhausted by post-ANN filtering without filling top_k) is the right call for preserving "enabling the backend never makes results worse," exercised by test_filter_shortfall_falls_back_to_pgvector with a deliberately adversarial fixture.
• The three new system checks (E002 invalid backend value, W003 process-local cache + compaction lock, W004 public-storage exposure of raw vectors) anticipate exactly the misconfigurations that would otherwise fail silently or leak data, and are unit tested.
• similarity_score clamping in router.py matches the existing pgvector path's max(0.0, min(1.0, ...)) convention in mixins.py — good consistency.
• Managers.py::store_embedding consolidates the fan-out call to a single call site covering both the "existing embedding" and "create / IntegrityError-race" paths, rather than duplicating it.
• The EmbeddingManager docstring proactively flags the one sharp edge for future work (independent Embedding row deletion needs manual tombstoning or a rebuild) instead of leaving a silent trap.

Minor observations (none blocking)

1. Compaction lock timeout vs. re-entrancy: compact_object_vector_namespace uses a cache.add lock with a 600s TTL. If a compaction genuinely exceeds that TTL, the lock expires and a second compactor can start concurrently — engine.compact() explicitly documents it is "NOT concurrency-safe with itself." This is called out as an intentional, documented tradeoff in the constant's comment ("raise this together with any namespace-size increase"), so not a bug — but it's the one place where namespace growth over time can silently invalidate a correctness assumption without a code change. Worth considering a compare-and-swap guard on the manifest write if this is expected to scale past the "~1M vectors" note in the design doc.
2. Global (non-corpus-scoped) namespaces: namespaces are keyed by (parent_kind, embedder_path, dimension) only, not by corpus/tenant, so a corpus-scoped query on a large multi-tenant deployment could need heavy oversampling before falling back to pgvector. Explicitly called out as out-of-scope in the design doc ("namespaces here are per-deployment"), so known/documented — flagging only because the backend's benefit may degrade toward "always falls back anyway" for narrow per-corpus searches at large multi-tenant scale.
3. DjangoStorageObjectStore.put_bytes overwrite dance: relies on Storage.save uniquifying names on collision to detect a losing race, then discards the stray and retries (bounded by OBJECT_STORE_PUT_OVERWRITE_MAX_ATTEMPTS = 3). Correct for FileSystemStorage and default S3Boto3Storage; contingent on the concrete backend's overwrite/consistency semantics rather than something the abstract Storage API itself guarantees. The PR does verify this against real MinIO per the test script, so likely fine.
4. rebuild_object_vector_index idempotency: always re-upserts every matching Embedding and compacts every touched namespace unconditionally — correct and simple, but on a large existing pgvector deployment this is a full WAL replay + full recluster per namespace. Might be worth a runtime expectation note in --help/docs for large corpora.

Security: search_via_object_index re-filters every candidate id through the caller's own queryset before returning results, correctly preserving the MIN(document, corpus) and structural-annotation invariants from CLAUDE.md. I didn't find a path where raw (parent_pk, vector) blobs reach a caller without going back through Django's permission-aware queryset. The new W004 check closes the remaining gap (bucket-level exposure bypassing Django auth entirely).

Test coverage: 18 new tests split cleanly between pure-engine (no DB) and Django-integration layers; both the default-off and toggled-on paths assert the other backend's call site raises AssertionError if touched, which is a nice way to prove exclusivity rather than just checking output.

No blocking issues found. This is a large, subtle feature (WAL/compaction/ANN over object storage) landed with race-condition test coverage well above what I'd expect for a first pass, and it's fully opt-in with automatic fallback, so the blast radius of shipping it is low.

[JSv4]

Rounds 5–6 addressed in the latest commit:

Fixed

• Compaction storm — sync_embedding_to_object_index now claims a cache pending-marker before enqueueing, so a sustained burst past the WAL threshold enqueues exactly one compaction task instead of one per write; the marker clears when compaction finishes (or by TTL if the queued task is lost, letting the next crossing re-trigger). Test: test_compaction_enqueued_once_per_threshold_crossing.
• W004 GCS gap — the exposure check now also covers STORAGE_BACKEND=GCP (GS_DEFAULT_ACL public values, GS_QUERYSTRING_AUTH=False), with tests for both signals plus the private-config negative case.
• Rebuild operability — rebuild_object_vector_index gained --dry-run (namespaces + counts, no writes), a progress line every 10k embeddings, and help text setting the runtime expectation (full WAL replay + full recluster per namespace; scope with --embedder-path).

Documented

• GCS is explicitly marked unverified in the design doc (listdir semantics are load-bearing for the WAL overlay) — run a smoke test before enabling on GCP.
• Limitations now note that deployment-wide namespaces can push narrow corpus-scoped queries onto the shortfall fallback at large multi-tenant scale, with corpus-sharded namespaces named as the follow-up.

Accepted as documented trade-offs (round 6): the 600s lock TTL vs. compaction duration coupling (a portable compare-and-swap doesn't exist in the abstract Django Storage API — S3 conditional PUTs would be a backend-specific enhancement; the constant's comment ties the two limits together), and put_bytes reliance on concrete-backend uniquify semantics (verified on FileSystemStorage + real MinIO).

Suite is now 30 tests; regression suites remain green.

---

Generated by Claude Code

[claude]

Review: Object-storage vector search backend

Overall this is a very well-engineered addition — opt-in, reversible by construction (a single string setting), with a clear write chokepoint (Embedding.objects.store_embedding), a single pgvector call site to hook (VectorSearchViaEmbeddingMixin.search_by_embedding), and permission enforcement that's correctly deferred to the caller's own already-scoped queryset rather than reimplemented. The design doc is candid about limitations (write-visibility lag, no group commit, GCS unverified, lazy deletes), the two new system checks (W003/W004) proactively guard against exactly the kind of misconfiguration that would silently corrupt data or leak vectors, and the test suite (18 new tests) exercises WAL ordering races, deferred GC, tombstones, and fallback semantics with real rigor — test_search_survives_compaction_between_wal_list_and_manifest_read and test_partial_gc_failure_cannot_resurrect_tombstoned_ids in particular are the kind of adversarial tests this concurrency model needs.

One correctness concern worth a look before this ships with the backend actually enabled in production:

Manifest read/write race can silently under-return results (no fallback safety net)

DjangoStorageObjectStore.put_bytes (opencontractserver/vector_search/object_store.py:57-85) overwrites by delete-then-save, since Django's Storage.save() never overwrites and this project explicitly sets file_overwrite = False on the S3 storage class (opencontractserver/utils/enhanced_storages.py:70) — so the delete-then-save dance is required, not incidental. The docstring only discusses writer-vs-writer contention (mitigated by the compaction cache-lock), but there's also a reader-vs-writer gap: between the delete() and save() calls, get_bytes() on that key raises ObjectNotFound.

The only key this repeatedly overwrites is manifest.json (ObjectStorageVectorEngine._load_manifest, engine.py:406-411), written once per compaction. If a concurrent search() (engine.py:218-271) calls _load_manifest during that exact window, it gets None back — structurally indistinguishable from "namespace never indexed." Tracing what search() does next:

• manifest is None, but wal_names is virtually always non-empty right after any compaction (the just-folded WAL files are retained for one more cycle, per the deferred-GC design), so the early-return-None guard (if manifest is None and not wal_names: return None) doesn't fire.
• The cluster-probe branch is skipped entirely (if manifest is not None and manifest["cluster_count"] > 0).
• The overlay is built from wal_names with exclude=self._folded_wals(manifest) → self._folded_wals(None) is set(), so nothing is excluded — but only the most recent compaction cycle's folded WAL files still exist in storage (older ones were already GC'd). Everything folded into segments more than one compaction ago is unreachable from this reconstruction.

Net effect: a search landing in this window returns a severely truncated candidate set (recent WAL tail only) instead of the full indexed namespace — and in router.search_via_object_index (router.py:159-174), the shortfall/fallback rule that's supposed to catch under-filled results only triggers when len(hits) >= fetch_n (i.e., "the engine returned a full oversampled batch but filtering ate it"). Here len(hits) will typically be small (just the WAL tail), so the code concludes the namespace was "genuinely exhausted" and returns the truncated results as final — silently violating the PR's own stated invariant ("enabling the backend never returns worse results than pgvector") for the duration of the race.

The window is narrow (one delete+save round trip, only during compaction commits) and self-healing on the next query, so this may be an acceptable tradeoff in practice — but it seems worth either an explicit call-out in the design doc's limitations section (it currently documents write-visibility lag but not this read-during-compaction-commit gap) or a small mitigation, e.g. retrying _load_manifest once on ObjectNotFound before treating the namespace as unindexed, or having search() distinguish "manifest key doesn't exist" from "manifest key exists but is mid-overwrite" (e.g. via namespace_exists()'s broader check, which already ORs in a WAL-files-exist test, as a signal to retry rather than proceed with cluster data silently dropped). Given test_search_survives_compaction_between_wal_list_and_manifest_read already tests a related-but-distinct manifest race, this specific one seems like a reasonable next addition to that test class.

Minor / non-blocking observations

• opencontractserver/vector_search/engine.py::compact() materializes the entire namespace's vectors into a single in-memory dict/array (state, then np.stack(...)) for the full k-means recluster. This is already flagged in the design doc as a known scaling limit ("fine up to ~10⁶ vectors per namespace, revisit beyond"), so no action needed — just confirming the tradeoff is intentional and documented, not missed.
• opencontractserver/shared/checks.py::check_vector_index_storage_exposure is a nice defense-in-depth addition, but it's inherently best-effort (e.g., it doesn't/can't detect a public bucket policy set out-of-band from Django settings, or MinIO/custom S3-compatible endpoints with public access). Worth a one-line caveat in the warning hint that this check covers common misconfiguration signals, not a guarantee — right now the docstring is clear about this but the runtime Warning text reads a bit more absolute.
• EmbeddingManager's docstring (opencontractserver/shared/Managers.py) proactively documents the "delete Embedding independent of parent" gap for future maintainers — good practice, no action needed.

Test coverage

Strong. Engine-level tests cover the interesting concurrency edge cases directly (WAL/manifest read ordering, deferred GC, partial-GC-failure-can't-resurrect-tombstones), and the Django integration tests cover the toggle, fallback paths (unindexed namespace, engine exception, filter shortfall), queryset scoping, and the rebuild command. The one gap is the manifest overwrite race described above, which isn't currently exercised (the existing race test interleaves two full compact() calls between _list_wal and _load_manifest, but doesn't simulate _load_manifest itself observing a mid-overwrite ObjectNotFound).

No security concerns beyond what's already been proactively addressed (post-ANN ORM re-filtering preserves permission scoping; W004 warns on public-bucket misconfiguration). No DB migrations needed since this only adds a parallel index, consistent with "Postgres remains ground truth." Default-off behavior and the E002 system check for typo'd VECTOR_SEARCH_BACKEND values are good guardrails for a change of this scope.

[JSv4]

Round 7's manifest read/write race is confirmed and fixed. You were right that it was worse than a doc-note case: a reader catching the delete-then-save window got manifest=None with a non-empty WAL, served only the WAL tail, and — as you traced — the shortfall rule read the small hit count as "namespace exhausted" and returned the truncated results as final, with no fallback.

Fix: search() and wal_tail_count() now read the manifest through a retry guard whenever the namespace demonstrably holds data (non-empty WAL): a None result is retried once after 50ms (OBJECT_INDEX_MANIFEST_RETRY_DELAY_SECONDS) before being believed. A genuinely pre-first-compaction namespace pays one extra GET; empty namespaces keep the fast single-read path. Regression test test_search_retries_manifest_read_hit_mid_overwrite simulates the mid-overwrite ObjectNotFound and asserts full results.

Documented: the design doc's limitations now describe the non-atomic manifest commit, the retry mitigation, the residual risk (retry also landing in the window degrades one query to WAL-tail-only and self-heals), and names S3 conditional PUTs as the backend-native way to remove the window entirely.

Also taken: W004's runtime hint now states it's best-effort (Django settings signals only — bucket policies set outside Django are invisible to it), matching the docstring's honesty.

The compact() full-materialization memory profile is intentional and documented as the ~1M-vectors-per-namespace scaling bound, as you suspected.

Suite is now 31 tests; regression suites remain green.

---

Generated by Claude Code

[claude]

Review: Optional turbopuffer-style object-storage vector search backend

Overall this is a well-scoped, well-documented, opt-in feature. Default behavior is unchanged (pgvector stays default), the single call-site design (VectorSearchViaEmbeddingMixin.search_by_embedding) means every caller (GraphQL, MCP, agent tools, hybrid RRF) picks up the toggle for free, and permission scoping is preserved correctly by re-filtering candidate ids through the caller's own queryset rather than reimplementing ACLs in the new engine. Test coverage (engine-level + Django integration, including fallback semantics and the filter-shortfall recall cliff) is thorough. The three new system checks (E002/W003/W004) are a nice touch — especially W004 flagging that the index blobs carry no ACL of their own.

A few things worth a look before/after merge:

Correctness / concurrency

• rebuild_object_vector_index bypasses the compaction lock. ObjectStorageVectorEngine.compact()'s docstring is explicit that it is "NOT concurrency-safe with itself" and must be serialized per namespace — which compact_object_vector_namespace (the Celery task) does via a cache lock, but the management command (opencontractserver/annotations/management/commands/rebuild_object_vector_index.py:527-528) calls engine.compact(namespace) directly with no lock. The docs say it's meant to be run "before flipping the flag," but nothing stops someone from re-running it later (e.g. drift repair) on a namespace that's concurrently being auto-compacted by the Celery task, which could race and clobber/lose the manifest. Worth having the command acquire the same lock (or invoke the task synchronously) before compacting.

• Silent data loss in DjangoStorageObjectStore.put_bytes after retries are exhausted (opencontractserver/vector_search/object_store.py:346-374). After OBJECT_STORE_PUT_OVERWRITE_MAX_ATTEMPTS (3) failed overwrite attempts, the function just logs a warning and returns — the caller's write is silently dropped with no exception. Since this is the only path that commits the manifest, a compaction whose write loses the race has no way to know its result never persisted. Given contention here is supposed to only happen if the per-namespace lock was already breached, this may be intentional, but consider raising (so the Celery task fails/retries visibly) rather than swallowing it.

Minor / DRY

• PARENT_FK_TO_KIND (opencontractserver/tasks/vector_index_tasks.py) and PARENT_KIND_BY_MODEL_NAME (opencontractserver/vector_search/router.py) encode the same parent-kind taxonomy under two different keys (FK attr name vs. model name) and have to be kept in sync by hand whenever a new embeddable parent type is added. A comment cross-referencing the other map (or a single shared source keyed by parent kind) would reduce the chance one gets updated without the other.

• fetch_n = top_k * OBJECT_INDEX_FILTER_OVERSAMPLE in router.search_via_object_index is unbounded — a caller passing a large top_k fans out into a correspondingly large in-memory sort plus a pk__in=candidate_ids query. Consider a hard cap similar to MAX_SELECT_ALL_DOCUMENT_IDS used elsewhere in constants/search.py, independent of what callers pass.

• The shortfall-fallback check (len(results) < top_k and len(hits) >= fetch_n) treats an exactly-exhausted namespace (len(hits) == fetch_n but genuinely complete) the same as a truncated one, causing an unnecessary — but harmless — pgvector fallback right at that boundary. Not a bug, just slightly under-serves the backend's benefit at that edge; a one-line comment noting it's an accepted false positive would help future readers.

• sync_embedding_to_object_index has no retry/backoff for transient object-storage errors (e.g. throttling); a failed sync silently drops that one embedding from the index until the next rebuild/compaction. Given the design already accepts eventual consistency, autoretry_for with a couple of retries would cheaply improve reliability for what's normally a single PUT.

Test coverage

The 18 new tests are solid at the engine/router/task boundary. Given the PR's headline claim is "GraphQL semantic search, MCP search_corpus, agent tools, and hybrid RRF all inherit the toggle with zero caller changes," it'd be worth locking that in with at least one thin end-to-end test that exercises the toggle through one of those actual call paths (not just Document.objects.search_by_embedding directly), since it currently holds by code inspection rather than by test.

Nothing above blocks merging given the feature is opt-in and defaults to unchanged behavior — the rebuild-command lock gap is the one I'd treat as worth fixing before anyone relies on rebuild_object_vector_index for live drift repair rather than only the documented pre-flip warm-up.

[JSv4]

Round 8 addressed — all items taken:

• Rebuild-command lock gap (the one you flagged as worth fixing) — rebuild_object_vector_index now acquires the same per-namespace cache lock as the auto-compaction task (shared compact_lock_key helper) before compacting. If a compaction is already running, it replays the WAL and defers folding to the next cycle with a warning instead of racing engine.compact(). Test: test_rebuild_command_respects_compaction_lock.
• put_bytes silent drop — now raises ObjectStoreWriteError after retries are exhausted, so a compaction whose manifest never persisted fails visibly in Celery / the command rather than reporting success. Test: test_put_bytes_raises_when_overwrite_retries_exhausted.
• Sync task retries — sync_embedding_to_object_index gained autoretry_for with backoff (3 tries, idempotent upsert), so transient object-store errors no longer silently drop an embedding until the next rebuild.
• fetch_n cap — bounded by new OBJECT_INDEX_MAX_FETCH_CANDIDATES (2048) independent of caller top_k; cap-truncated shortfalls take the pgvector fallback, so correctness is preserved.
• DRY / boundary comments — both parent-kind maps now cross-reference each other with an explicit "kind values must match exactly or reads and writes land in different namespaces" warning, and the shortfall rule documents the accepted exactly-fetch_n false positive.
• End-to-end caller test — test_toggle_serves_core_annotation_vector_store_callers drives a real CoreAnnotationVectorStore vector search (the store behind GraphQL semantic search / MCP / agents) with the toggle on and proves the results come from the object index with pgvector's CosineDistance patched to raise — the "zero caller changes" claim now holds by test, not just inspection.

Suite is now 34 tests; regression suites (142 in this round's run) remain green.

---

Generated by Claude Code

[claude]

Overview

This PR adds an opt-in, reversible object-storage vector search backend (turbopuffer-style: WAL + async k-means compaction + centroid-ANN, all on top of Django's default file storage), routed entirely through the single existing call site VectorSearchViaEmbeddingMixin.search_by_embedding. Default behavior (VECTOR_SEARCH_BACKEND=pgvector) is unchanged, and every failure mode (unindexed namespace, engine exception, post-ANN filter shortfall) falls back to pgvector. Write-side fan-out happens from the single Embedding.objects.store_embedding chokepoint via a fire-and-forget Celery task.

This is a large, well-engineered change (~2,500 lines) with unusually thorough documentation (docs/architecture/object_storage_vector_search.md honestly enumerates limitations rather than hiding them) and 18 new tests that specifically target the hard concurrency edge cases (WAL/manifest read-ordering race, deferred GC, manifest delete-then-save overwrite window, compaction-storm gating). Great use of docs/test_scripts/ for the MinIO manual verification per repo convention.

Strengths

• Correctness-by-construction on permissions: the engine only ever stores (parent_pk, vector) and candidate ids are re-filtered through the caller's own queryset (opencontractserver/vector_search/router.py, search_via_object_index), so visible_to_user/MIN(document, corpus)/structural scoping is inherited automatically rather than reimplemented. checks.py::check_vector_index_storage_exposure (W004) is a nice touch flagging the real residual risk (bucket-level exposure of raw vectors) that this re-filter can't cover.
• WAL/manifest read ordering (engine.py::search) is subtle and correctly reasoned about (list WAL before manifest, folded_wals prevents replay-over-newer-state), and it's the one thing I'd have been most worried about — it has dedicated regression tests (test_search_survives_compaction_between_wal_list_and_manifest_read, test_partial_gc_failure_cannot_resurrect_tombstoned_ids).
• Deferred GC by one compaction cycle avoids the classic "reader holds a manifest whose blobs just got deleted" race without needing distributed locking on reads.
• Good use of existing system-check infrastructure (E002/W003/W004) to fail loudly on foot-guns (invalid backend string, LocMemCache with multi-worker compaction, public-bucket exposure) rather than silently degrading.
• PARENT_FK_TO_KIND / PARENT_KIND_BY_MODEL_NAME keep-in-sync comments in both vector_index_tasks.py and router.py are a reasonable mitigation for the lack of a single shared source of truth between them — though see suggestion below.

Issues / Risks

Correctness (minor, already largely self-flagged)

• Concurrent compactors beyond the lock TTL can write into the same generation directory. If a compaction genuinely exceeds OBJECT_INDEX_COMPACT_LOCK_TIMEOUT_SECONDS (600s), the lock expires and a second compactor can start from the same base manifest, both computing new_generation = N+1 and writing cluster files under the same segments/000{N+1}/ prefix concurrently (not just racing the final manifest PUT, but potentially interleaving cluster files from two different k-means runs before the last manifest write wins). This is explicitly flagged in the constants comment (constants/search.py) as something to revisit before raising the per-namespace size ceiling, so I'm not asking for a fix here — just flagging it so it's tracked as a real (if narrow, opt-in, and self-documented) correctness gap rather than a purely theoretical one.
• int(doc_id) in engine.upsert/delete implicitly assumes integer PKs for every embeddable parent model. That holds today (Document/Annotation/Note/Conversation/ChatMessage/Relationship all use BaseOCModel's default AutoField id — verified, no override), but nothing enforces it if a future embeddable parent uses a non-integer PK (the repo does have UUID-PK models elsewhere, e.g. document_imports). Worth a one-line comment or assertion near PARENT_FK_TO_KIND/PARENT_KIND_BY_MODEL_NAME calling out the integer-PK assumption so it's not rediscovered the hard way.

Performance (documented, worth surfacing to reviewers who won't read the design doc)

• No group commit: store_embedding → enqueue_embedding_index_sync fans out one Celery task per embedding, and each task does a WAL PUT plus a wal_tail_count() check (list + manifest GET) — i.e., ~3 object-store round trips per embedding. For bulk pipelines that create many embeddings per document (e.g., per-annotation embeddings across a large corpus reprocess), this multiplies Celery queue depth and object-store request volume significantly versus the previous no-op. This is called out in the design doc's limitations section with the mitigation (rebuild_object_vector_index batches), but bulk online writes (not just backfills) will still hit this — worth confirming Celery queue capacity/object-store request pricing before enabling on a large deployment with heavy live-ingestion.
• OBJECT_INDEX_MANIFEST_RETRY_DELAY_SECONDS = 0.05 is a synchronous time.sleep() inside _load_manifest_expecting_data, which can execute on a request-serving thread (GraphQL resolver → search_by_embedding → search_via_object_index → engine.search) when a query lands in the rare delete-then-save overwrite window. Low probability and low absolute cost (50ms), but worth being aware it's not confined to Celery.

Security

• Nice defense-in-depth already in place: np.load() on WAL/segment blobs relies on numpy's default allow_pickle=False, so even if an attacker could write to the storage prefix, arbitrary object deserialization isn't possible via this path.
• The W004 check is a reasonable best-effort signal, and the doc is upfront that it can't see bucket policies set outside Django — good, avoids false confidence.

Test coverage

• Coverage is strong for the engine's concurrency edge cases and the Django integration toggle/fallback/scoping paths. One gap: I didn't see a test exercising rebuild_object_vector_index --dry-run output content, or a test for the ObjectStoreWriteError raised when put_bytes exhausts OBJECT_STORE_PUT_OVERWRITE_MAX_ATTEMPTS being surfaced correctly all the way up through compact() (only the object_store-level test_put_bytes_raises_when_overwrite_retries_exhausted covers it directly) — minor, not blocking.
• GCS is explicitly called out as unverified (docs/architecture/object_storage_vector_search.md) since list_keys() depends on Storage.listdir() semantics over GCS's flat namespace — good that this is surfaced rather than silently assumed to work; worth doing that smoke test before anyone flips the flag on STORAGE_BACKEND=GCP in production.

Suggestions

1. Consider deriving PARENT_FK_TO_KIND (in vector_index_tasks.py) and PARENT_KIND_BY_MODEL_NAME (in router.py) from a single shared mapping instead of two hand-synced dicts with a "KEEP IN SYNC" comment — a silent divergence here would misroute reads/writes into different namespaces with no error, just silently-empty search results. Even a small shared constant in opencontractserver/constants/search.py (e.g. EMBEDDABLE_PARENT_KINDS: dict[str, tuple[str, str]] mapping model name → (fk_attr, kind)) would remove the duplication per the repo's DRY guidance and eliminate the divergence risk entirely.
2. Given the "namespaces are deployment-wide, not corpus-scoped" limitation already called out in the design doc, it might be worth a follow-up issue reference (or a TODO with an issue number) so it doesn't get lost — this seems like the most likely thing to bite a real multi-tenant deployment first.

Overall this is a careful, well-tested, and honestly-documented piece of infrastructure that keeps the blast radius small (opt-in, pgvector remains ground truth and default, fails safe). The concerns above are mostly things the PR author already anticipated and documented — I'd call those out for reviewer awareness rather than treat them as blockers.

[JSv4]

Round 9 addressed:

• Single-source parent-kind taxonomy (your suggestion Bump postgres from 14.5 to 15.0 in /compose/production/postgres #1, taken as proposed) — new EMBEDDABLE_PARENT_KINDS constant in constants/search.py maps model_name -> (fk_attr, kind); both PARENT_KIND_BY_MODEL_NAME (router) and PARENT_FK_TO_KIND (tasks) are now derived from it, so the silent-divergence → different-namespaces failure mode is structurally impossible rather than comment-guarded.
• Integer-PK assumption — documented on that same constant: the engine stores ids as int64 (int(doc_id)), every listed parent uses an integer AutoField pk, and adding a UUID-pk parent requires changing the engine's id representation first.
• Request-thread sleep — noted on OBJECT_INDEX_MANIFEST_RETRY_DELAY_SECONDS that the retry sleep can run on a request-serving thread (rare mid-overwrite window only; keep it small).
• --dry-run coverage — test_rebuild_command_dry_run_reports_without_writing asserts the preview output and that nothing lands in the object store.

On the flagged-not-blocking items: the lock-TTL-expiry double-compactor scenario stays a documented trade-off (the constants comment ties the TTL to the namespace-size ceiling; same-generation cluster interleaving resolves at the manifest PUT via last-writer-wins + ObjectStoreWriteError on the loser since round 8). The np.load allow_pickle=False observation is correct — that default is relied upon deliberately. Corpus-sharded namespaces stay in the design doc's limitations as the named follow-up; happy to open a tracking issue if maintainers want one.

Suite is now 35 tests; regression suites remain green.

---

Generated by Claude Code

[claude]

Review: Object-storage vector search backend (turbopuffer-style)

Overall this is a very well-engineered, carefully documented PR. The design doc, in-code rationale comments (especially around WAL/manifest read ordering, deferred GC, and the compaction lock), and the 32-test suite (`opencontractserver/tests/test_object_storage_vector_backend.py`) all show real attention to the hard parts of building an eventually-async index on top of blob storage. The feature is genuinely opt-in (`VECTOR_SEARCH_BACKEND=pgvector` default) and fails safe (`search_via_object_index` falls back to pgvector on any exception, unindexed namespace, or filtered-shortfall).

Strengths

• Correctness of the read/write protocol is well-reasoned: the WAL-list-before-manifest-read ordering (`engine.py:230-250`), the deferred-GC-by-one-cycle scheme, and the `folded_wals` bookkeeping to avoid double-replay are all explained with the specific race they prevent, not just what the code does.
• Permission-preservation by construction — storing only `(parent_pk, vector)` and re-filtering candidate ids through the caller's already-scoped queryset (`router.py:149-183`) is the right way to bolt this onto the existing permission model without duplicating scoping logic, and it's consistent with the project's `CorpusDocumentService`/service-layer conventions.
• New system checks are a nice touch: `opencontracts.E002` (invalid backend value) and especially `opencontracts.W004` (public-bucket exposure warning, since the index blobs carry no ACL of their own) proactively catch misconfigurations that would otherwise fail silently or leak data.
• Shortfall/fallback logic (`router.py:164-182`) is honest about its one accepted false-positive edge case (exactly `fetch_n` matches) rather than hiding it.
• Good single-source-of-truth discipline: `EMBEDDABLE_PARENT_KINDS` in `constants/search.py` is shared by both the write-path (`vector_index_tasks.py`) and read-path (`router.py`) maps so they can't silently diverge.

Issues / suggestions

1. Dead code: `ObjectStorageVectorEngine.namespace_exists()` (`opencontractserver/vector_search/engine.py:201-205`) is defined but never called anywhere in the codebase (production code or tests) — `search()` already returns `None` for an unwritten namespace, which is what callers actually use. Per the repo's "no dead code" guidance, either wire it up somewhere useful or drop it.

2. Embedding row deletion isn't tombstoned — correctly documented as a known gap in the `EmbeddingManager` docstring (`opencontractserver/shared/Managers.py:1339-1347`), but worth calling out explicitly in review: any code path that deletes/replaces an `Embedding` row independent of its parent (e.g. a stale-embedder cleanup) will leave a stale vector matching a still-alive parent until a rebuild. I didn't find an existing call site that does this today, but it's a foot-gun for future code that could add one without realizing the index needs a tombstone — might be worth a lint/grep check or a helper that always goes through the engine.

3. WAL ordering relies on wall-clock (`time.time_ns()`) across processes/hosts (`engine.py:194-198`) — "later write wins" is really "write from the host with the later clock wins." With multiple Celery workers on machines with clock skew, a genuinely later write could lose to an earlier one if the losing host's clock is fast. This is a pretty standard last-writer-wins tradeoff (S3 has similar issues), and the blast radius is small (worst case: briefly serving a stale vector for one parent until the next write/compaction), but it's not called out in the design doc's limitations section alongside the other documented tradeoffs (lazy deletes, no group commit, etc.) — might be worth adding a line there for future readers.

4. Minor: `search_via_object_index` doesn't guard against `top_k <= 0`; Python list slicing degrades gracefully, but worth a sanity check if this ever becomes reachable from unvalidated caller input (today's callers all pass a positive int, so this is low priority).

Test coverage

Coverage is thorough — engine-level tests cover strong consistency, compaction/brute-force parity, tombstones, overwrite ordering, determinism, the manifest mid-overwrite race, and the deferred-GC/partial-GC-failure edge cases; integration tests cover the toggle, queryset scoping, both fallback paths, the rebuild command (including dry-run and lock contention), and auto-compaction triggering. I didn't spot an explicit test for the WAL-clock-skew scenario in point 3 above, but that's a hard thing to test deterministically and I wouldn't block on it.

Security

• Candidate re-filtering through the caller's queryset is the right call for IDOR prevention, consistent with `docs/permissioning/consolidated_permissioning_guide.md`.
• The new `opencontracts.W004` check for public-bucket exposure is a good defensive addition given the index has no per-object ACL.
• No obvious injection/XSS surface introduced — this is internal blob-storage plumbing with no new user-facing input parsing beyond `embedder_path` (already validated upstream) and numeric ids.

Nice work overall — no blocking issues, just the dead-code cleanup (#1) and the documentation nit (#3) worth a look before merge.

[JSv4]

Round 10 addressed:

• Bump postgres from 14.5 to 15.0 in /compose/production/postgres #1 dead code — confirmed and removed. namespace_exists() had no callers (search() returning None already serves the never-written signal); dropped per the no-dead-code rule.
• Bump responses from 0.21.0 to 0.22.0 #4 top_k <= 0 guard — added. search_via_object_index now returns None for degenerate top_k, deferring to pgvector so behavior stays byte-identical to the default backend for any caller quirk.
• Bump actions/setup-node from 3.4.1 to 3.5.1 #3 clock-skew doc note — already present. The design doc's limitations section has carried "WAL ordering across workers uses wall-clock names (time.time_ns()) … subject to clock skew … healed by the next rebuild" since an earlier round — no change needed.
• Bump traefik from v2.8.7 to v2.9.1 in /compose/production/traefik #2 embedding-deletion tombstone — stays a documented gap (EmbeddingManager docstring + design doc limitations, both pointing at ObjectStorageVectorEngine.delete/rebuild). A grep-based lint felt heavier than warranted for a path with zero call sites today; happy to add one if maintainers disagree.

Suite remains 35 tests, all green.

---

Generated by Claude Code

[claude]

Review: Object-storage vector search backend (turbopuffer-style)

This is a large, well-engineered addition — opt-in, defaults preserved, and the design doc + 10 rounds of self-review already closed out a long list of real WAL/manifest/GC race conditions (deferred GC, folded_wals, manifest retry-on-mid-overwrite, put_bytes overwrite races, compaction-storm gating, etc.). That groundwork is solid and the test suite (35 tests) is unusually thorough for a feature this concurrency-heavy.

One correctness bug survived that self-review, though, and it undercuts a guarantee the PR explicitly documents.

Bug: the fetch_n cap is bypassed for top_k above OBJECT_INDEX_MAX_FETCH_CANDIDATES

opencontractserver/vector_search/router.py:133-136:

```python
fetch_n = min(
top_k * OBJECT_INDEX_FILTER_OVERSAMPLE,
max(top_k, OBJECT_INDEX_MAX_FETCH_CANDIDATES),
)
```

OBJECT_INDEX_MAX_FETCH_CANDIDATES (constants/search.py) is documented as "Hard cap on the oversampled candidate fetch, independent of caller top_k — bounds the in-memory ranking and the pk__in re-filter query for abusive or buggy top_k values," and the inline comment above this line repeats that claim. But max(top_k, CAP) only changes the result of the outer min() when top_k > CAP — and in exactly that case it substitutes top_k back in, so fetch_n collapses to min(top_k * OVERSAMPLE, top_k) == top_k. The cap has no effect whenever it's actually needed:

• top_k = 5000 → fetch_n = min(20000, max(5000, 2048)) = min(20000, 5000) = 5000 (not 2048).
• top_k = 1_000_000 → fetch_n = 1_000_000.

top_k is caller-controlled and unbounded at at least one call site: search_conversations (config/graphql/conversation_queries.py:89-103) takes top_k=graphene.Int(default_value=100) straight from the GraphQL request with no upper-bound validation before it reaches CoreConversationVectorStore → search_by_embedding → search_via_object_index. So a client can pass an arbitrarily large top_k and force engine.search() to rank/sort an unbounded candidate set and router.py's queryset.filter(pk__in=candidate_ids) to build an unbounded IN (...) clause — precisely the DoS scenario the constant was introduced to prevent.

Fix is presumably just dropping the max(top_k, ...):
```python
fetch_n = min(top_k * OBJECT_INDEX_FILTER_OVERSAMPLE, OBJECT_INDEX_MAX_FETCH_CANDIDATES)
```
With that, top_k > CAP correctly truncates fetch_n to the cap, and the existing shortfall rule (len(results) < top_k and len(hits) >= fetch_n, router.py:178) already handles falling back to pgvector when the cap truncates — so this looks like the only change needed, and it matches what the round-8 commit message ("fetch_n capped by OBJECT_INDEX_MAX_FETCH_CANDIDATES independent of caller top_k") says was intended.

Worth adding a regression test alongside the fix (e.g. top_k=OBJECT_INDEX_MAX_FETCH_CANDIDATES * 10 should still result in a bounded engine.search call and fall back to pgvector) — the current suite exercises the shortfall path only for small top_k (test_filter_shortfall_falls_back_to_pgvector), so this slipped through.

Everything else

• Read/compaction ordering (WAL-before-manifest, folded_wals, deferred GC by one cycle) checks out against the tests that exercise it (test_search_survives_compaction_between_wal_list_and_manifest_read, test_partial_gc_failure_cannot_resurrect_tombstoned_ids, test_deferred_gc_reclaims_prior_cycle) — traced through the logic by hand and it holds.
• put_bytes's delete-then-save retry/uniquify handling and the manifest mid-overwrite retry are both correctly bounded and fail loudly (ObjectStoreWriteError) rather than silently reporting success, which matches the "Postgres stays ground truth" design.
• EMBEDDABLE_PARENT_KINDS FK-attribute names (document_id, annotation_id, note_id, conversation_id, message_id, relationship_id) match the actual Embedding model fields — no drift between the router's read-side map and the task's write-side map.
• Permission re-filtering through the caller's own queryset (rather than trusting the engine's stored ids) is the right call and keeps this consistent with the rest of the permission architecture in this repo.
• opencontracts.W003/W004 checks are a nice touch for catching the two most likely footguns (non-shared cache breaking the compaction lock; public-read storage leaking raw vectors) before they bite in production.

Minor / non-blocking

• rebuild_object_vector_index's manual compaction shares compact_lock_key with the auto-compaction task but not the write-path's pending-marker key — if a rebuild runs while a pending marker is set from a prior write burst, the next auto-compaction enqueue could be suppressed for up to OBJECT_INDEX_COMPACT_LOCK_TIMEOUT_SECONDS (600s) after the rebuild already compacted. Self-heals via TTL and doesn't affect correctness (just delays the next auto-compaction), so likely not worth the extra coordination, but flagging in case it's easy to close while this is fresh.
• GCS is explicitly called out as unverified in the design doc (list_keys/listdir semantics), which is the right way to ship this — just flagging that it's a real gap if anyone flips the flag on GCS before that smoke test happens.

[JSv4]

Round 11's cap-bypass finding is confirmed and fixed — good catch, and correct in every detail: max(top_k, CAP) neutralised the cap precisely when top_k > CAP, and top_k does reach this path unvalidated from GraphQL (searchConversations). It's now fetch_n = min(top_k * OBJECT_INDEX_FILTER_OVERSAMPLE, OBJECT_INDEX_MAX_FETCH_CANDIDATES), exactly as the round-8 commit message claimed it should have been; when the cap truncates below top_k, the existing shortfall rule falls back to pgvector (whose SQL LIMIT has no equivalent exposure). Regression test test_fetch_cap_binds_for_abusive_top_k spies on the engine call with top_k = 10× cap and asserts the bound holds.

The minor pending-marker interaction was also closed while fresh: rebuild_object_vector_index now clears the auto-compaction pending marker after a manual compaction (which just serviced it), so a stale marker can no longer delay the next threshold-crossing enqueue by up to the TTL.

Suite is now 36 tests; regression suites remain green.

---

Generated by Claude Code

[claude]

Review: Object-storage vector search backend (turbopuffer-style)

This is a well-documented, carefully reasoned addition — the design doc, deferred-GC/WAL-ordering rationale, and the limitations section are genuinely good engineering hygiene. Comments below are ordered by importance.

🔴 Read-path coverage doesn't match the "single call site" claim

The PR description states the integration seam is VectorSearchViaEmbeddingMixin.search_by_embedding and that "GraphQL semantic search, MCP search_corpus, agent tools, and hybrid RRF all inherit the toggle with zero caller changes." That's true for Document/Annotation/Note (their querysets in opencontractserver/shared/QuerySets.py genuinely inherit the mixin), but not for the other three kinds this PR wires into EMBEDDABLE_PARENT_KINDS (opencontractserver/constants/search.py):

• ConversationQuerySet.search_by_embedding (opencontractserver/conversations/models.py:322) and ChatMessageQuerySet.search_by_embedding (same file, :506) each define their own method with an inline CosineDistance query — they don't inherit VectorSearchViaEmbeddingMixin (the from ... import VectorSearchViaEmbeddingMixin at line 131 is just a stray import inside the class body; the class itself only extends SoftDeleteQuerySet). These are called directly from CoreConversationVectorStore/CoreChatMessageVectorStore (opencontractserver/llms/vector_stores/core_conversation_vector_stores.py:234,303,488,574).
• CoreRelationshipVectorStore.search (opencontractserver/llms/vector_stores/core_relationship_vector_store.py) does its own inline CosineDistance join too ("same idea as VectorSearchViaEmbeddingMixin but inlined" per its docstring).

None of these three call object_storage_backend_enabled() / search_via_object_index(). Meanwhile, the write path (enqueue_embedding_index_sync → sync_embedding_to_object_index) fans out to all six EMBEDDABLE_PARENT_KINDS, and rebuild_object_vector_index will happily replay + k-means-compact conversation/…, message/…, and relationship/… namespaces. So with the flag on, conversation/chat-message/relationship embeddings get durably written, WAL-compacted, and clustered in object storage, but every actual query for those three kinds still goes straight to pgvector regardless of VECTOR_SEARCH_BACKEND — pure write/compute amplification with no corresponding read benefit, and a behavior gap from what the PR describes.

Suggested fix: either (a) make ConversationQuerySet/ChatMessageQuerySet actually inherit VectorSearchViaEmbeddingMixin and route CoreRelationshipVectorStore through search_via_object_index too, or (b) scope EMBEDDABLE_PARENT_KINDS/the write-path hook down to document/annotation/note until the other three read paths are migrated, and note the gap explicitly in the design doc's limitations section. Right now the mismatch is easy to miss because the 18 new tests only exercise Document (via DocumentQuerySet) integration — none assert that conversation/message/relationship search is actually served (or not served) by the object index.

Other observations (minor / already well-handled)

• The WAL-before-manifest read ordering, deferred one-cycle GC, and the manifest delete-then-save retry dance are all reasoned through carefully and covered by targeted regression tests (test_search_survives_compaction_between_wal_list_and_manifest_read, test_partial_gc_failure_cannot_resurrect_tombstoned_ids, etc.) — nice work isolating these races.
• similarity_score clamping (max(0.0, min(1.0, similarity)) in router.py) matches the existing pgvector path's clamping (opencontractserver/shared/mixins.py), so behavior is consistent between backends.
• The shortfall/fallback rule in search_via_object_index (falling back to pgvector when a truncated candidate set doesn't fill top_k) is a nice touch for correctness under heavy per-caller filtering, and is tested (test_filter_shortfall_falls_back_to_pgvector).
• check_vector_index_storage_exposure (W004) and check_vector_search_cache (W003) are good defense-in-depth checks given the documented "no ACL at the storage layer" caveat — appreciate that the design doc calls out the blast-radius difference vs. pgvector explicitly rather than glossing over it.
• Minor: get_default_engine() is an unlocked lazy singleton (opencontractserver/vector_search/router.py) — documented as intentional since a racing double-init just orphans an empty LRU. Fine as long as the engine truly stays stateless beyond the cache; worth a comment/test if that assumption ever changes.
• Minor: DjangoStorageObjectStore.put_bytes's delete-then-save overwrite dance is inherently non-atomic for readers (acknowledged in the limitations section as "Manifest overwrite is not atomic for readers"). The retry-once-after-50ms-sleep mitigation is reasonable for the rare window, but note it's a blocking time.sleep that can execute on a request-serving thread (already flagged in the constants comment) — worth keeping an eye on tail latency in production if manifest contention turns out to be more frequent than expected.
• Test coverage for the engine/object-store/system-checks layers is strong (18 tests, including race-condition regressions). Given the finding above, I'd suggest adding an integration test that asserts conversation/message/relationship search behavior explicitly (either "yes it's served by the object index" once fixed, or "no, intentionally still pgvector" if that's the accepted scope for this PR).

Security

• Namespace construction (build_namespace) sanitizes embedder_path before using it in storage keys, so there's no path-traversal concern even though embedder_path strings ultimately flow from pipeline/embedder configuration.
• The "storage exposure" caveat in the design doc (no ACL at the blob layer, permissions enforced only by the ORM re-filter) is called out clearly and backed by a system check — good practice for an opt-in feature with this kind of blast radius.

Overall: solid opt-in, reversible architecture with real attention to consistency races and fallback safety. The main actionable item is closing (or explicitly scoping down/documenting) the gap between the "every producer/consumer inherits the toggle" claim and the three parent kinds whose reads still bypass it entirely.

[JSv4]

Round 12's read-path coverage finding is confirmed and addressed — you were right, and the original "single CosineDistance call site" premise was wrong: ConversationQuerySet.search_by_embedding, ChatMessageQuerySet.search_by_embedding, and CoreRelationshipVectorStore.search all carry their own inline implementations that never see the toggle (and notably with a different result contract — QuerySet + distance-valued similarity_score, ascending — so routing them through search_via_object_index is not a drop-in change).

Taken option (b), scoped down for symmetry:

• EMBEDDABLE_PARENT_KINDS now lists only document/annotation/note — the kinds whose querysets actually inherit the mixin — with the exclusion rationale and the migration recipe (route the read path through search_via_object_index first, then add the kind) documented on the constant.
• enqueue_embedding_index_sync skips non-indexed parent kinds before queuing any Celery task, so conversation/message/relationship writes no longer produce index writes nobody reads.
• Design doc: indexed kinds stated in the architecture section; new limitations bullet names the conversation/message/relationship read-path migration as the follow-up. The PR description's caller-coverage claim is corrected accordingly.
• New test test_non_indexed_parent_kinds_do_not_fan_out locks in the symmetry both ways (conversation embedding → no fan-out; document embedding → fan-out). Conversation and subtree-group (relationship) search suites verified green against the change.

Separately (from the session's ongoing goal): GCS is no longer unverified — the full engine lifecycle now runs green against the GCS JSON API via fake-gcs-server + django-storages GoogleCloudStorage (WAL over prefix-emulated listdir, manifest delete-then-save overwrite, multi-generation deferred GC, parity, tombstones), with zero adapter changes needed; steps recorded in docs/test_scripts/object_storage_vector_backend_gcs.md. All three storage backends (LOCAL/AWS via MinIO/GCP via fake-gcs-server) are now verified.

Suite is now 37 tests; regression suites remain green.

---

Generated by Claude Code

[claude]

Review: Object-storage vector search backend

Overall this is an impressively engineered, well-isolated feature. The opt-in flag, pgvector-as-permanent-fallback design, and the extensive self-review history (WAL/manifest read ordering, deferred GC, overwrite races, fetch-cap bypass, public-storage exposure checks) already cover most of the sharp edges I'd normally flag in an object-storage LSM-ish engine. I read engine.py, object_store.py, router.py, hooks.py, Managers.py, checks.py, vector_index_tasks.py, the management command, and the test suite directly (PR branch checked out locally). Two findings below are, I believe, genuinely new.

Correctness — the "single chokepoint" claim doesn't hold for all producers

The design doc and PR description state the write path is a single chokepoint: "Write path: fan-out from Embedding.objects.store_embedding (single chokepoint for all producers)." That's true for the in-app embedding pipeline (opencontractserver/tasks/embeddings_task.py → HasEmbeddingMixin.add_embedding), but opencontractserver/worker_uploads/tasks.py (_store_embeddings / _store_single_embedding, ~lines 476–565) writes Embedding rows directly via Embedding.objects.bulk_create(...) and Embedding.objects.update_or_create(...), bypassing EmbeddingManager.store_embedding() entirely. There's no post_save signal on Embedding to catch this as a safety net (confirmed — no signal handlers reference Embedding).

Effect: documents/annotations ingested through the external-worker upload pipeline will never call enqueue_embedding_index_sync(), so they silently never land in the object-storage index once VECTOR_SEARCH_BACKEND=object_storage is enabled — while the same rows are perfectly present in Postgres. Because the namespace itself is likely non-empty (populated by other, correctly-routed producers), this isn't caught by the "unindexed namespace → fallback to pgvector" safety net; it's a silent per-row gap that only rebuild_object_vector_index can repair, with no automatic staleness detection. Worth either wiring this second producer through store_embedding, or documenting it as a second known chokepoint gap (and running rebuild_object_vector_index on a schedule if that pipeline is in active use).

Correctness — write/read taxonomy mismatch for 3 of 6 "embeddable parent kinds"

EMBEDDABLE_PARENT_KINDS (opencontractserver/constants/search.py) lists six parent kinds — document, annotation, note, conversation, chatmessage, relationship — and the write-path hook (enqueue_embedding_index_sync) fires for all of them via the shared store_embedding() chokepoint. But on the read side, only DocumentQuerySet, AnnotationQuerySet, and NoteQuerySet actually mix in VectorSearchViaEmbeddingMixin and route through search_via_object_index(). The other three all hand-roll their own inline CosineDistance query and never call the router:

• ConversationQuerySet.search_by_embedding (opencontractserver/conversations/models.py)
• ChatMessageQuerySet.search_by_embedding (same file)
• CoreRelationshipVectorStore.search (opencontractserver/llms/vector_stores/core_relationship_vector_store.py), whose own docstring says "same idea as VectorSearchViaEmbeddingMixin but inlined because RelationshipManager isn't a from_queryset(...) shape today"

For relationship this isn't theoretical — relationship.add_embedding(...) is actively called in opencontractserver/tasks/embeddings_task.py:996, so relationship vectors are written into the object-storage index (WAL PUTs, compaction cycles, storage cost) but can never be served from it, since the relationship vector store never calls search_via_object_index. (Conversation/ChatMessage may currently have no live embedding producer at all, so that half of the mismatch is lower-impact today, but the taxonomy entry is still misleading.)

Given the "zero caller changes" framing in the PR description, worth either (a) scoping EMBEDDABLE_PARENT_KINDS/the write-path hook down to the three kinds that can actually be searched today (avoids paying WAL/compaction cost for data that can never be read back), or (b) wiring CoreRelationshipVectorStore (at least) through the same router seam, since its own docstring already flags the duplication as a stopgap.

Everything else I checked

• Engine internals (engine.py): WAL-before-manifest read ordering, folded_wals bookkeeping, deferred one-cycle GC, k-means empty-cluster reseeding bound (verified reseed_cursor can't exceed worst_fit_order length since empty clusters < k ≤ n) — all sound, each backed by a targeted regression test using deterministic mocking rather than real concurrency (good — avoids CI flakiness).
• object_store.py: delete-then-save overwrite race handling is correct and bounded; idempotent delete is right for the GC-failure-is-harmless design.
• router.py: fetch cap correctly bounds top_k * OVERSAMPLE independent of caller-supplied top_k (the round-11 fix), degenerate top_k <= 0 deferred to pgvector, namespace slug collision-proofed via md5 digest suffix (no path-traversal surface — parent_kind comes from a fixed dict, not user input).
• Permissions: post-ANN re-filter through the caller's own queryset is the right design and is exercised by test_queryset_scoping_is_preserved / test_toggle_serves_core_annotation_vector_store_callers. opencontracts.W004's public-bucket-exposure check is a nice defense-in-depth addition given the index has no per-blob ACL.
• Security: no obvious injection/SSRF surface; usedforsecurity=False on the md5 namespace digest is correctly scoped (collision-avoidance, not a security boundary).
• Ops: rebuild_object_vector_index correctly shares the compaction lock with the Celery task (round 8 fix) and clears the pending-marker after a manual compaction (round 11 fix) — good attention to the two-writer interaction.
• Test coverage: 35+ tests split cleanly between engine-only (SimpleTestCase, no DB) and Django integration (TestCase), covering the trickiest races via mocking rather than real threads/sleeps. Good coverage of the fallback semantics (unindexed namespace, engine error, filter shortfall). The two gaps above aren't covered because the tests only exercise the store_embedding/mixin paths — reasonable, since those are the "documented" surfaces, but worth a regression test if the worker-upload path is fixed.
• Docs: design doc is thorough, and its own "Current limitations" section is honest about real tradeoffs (write-visibility lag, no group commit, sequential WAL-tail GETs, deployment-wide namespaces). Good discipline keeping the changelog fragment in changelog.d/ per repo convention instead of touching CHANGELOG.md directly.

Minor / non-blocking

• EmbeddingManager.store_embedding's lookup dict doesn't include dimension, so (pre-existing, not from this PR) a single Embedding row could theoretically end up with more than one vector_<dim> column populated for the same (embedder_path, parent) if called with different dimensions over time. rebuild_object_vector_index already handles this correctly by iterating all DIM_TO_FIELD_MAP entries per row, so no action needed — just flagging that the multi-dimension case exists and is implicitly relied upon.
• This is a very large single PR (~2.7k lines / 20 files) for a repo whose CLAUDE.md calls for trunk-based, quickly-merged changes. Given it's fully flag-gated and defaults to unchanged behavior, that's a reasonable tradeoff, but worth confirming a human maintainer (not just the automated review rounds baked into the commit history) has read through engine.py end-to-end before merge.

Nice work on the fallback-safety framing throughout — "enabling the flag can never make search worse than pgvector" is a good design invariant, and it's actually upheld by the code (shortfall rule, engine-error catch-all, and unindexed-namespace check all funnel to the same fallback).

[JSv4]

Round 13:

Finding 1 (worker-upload bypass) — confirmed and fixed. Genuinely new and the more serious of the two: worker_uploads/tasks.py writes Embedding rows via bulk_create/update_or_create, bypassing store_embedding, so externally-ingested documents/annotations would silently never reach the object index (non-empty namespace ⇒ no fallback ⇒ a per-row gap only a manual rebuild repairs). _store_embeddings and _store_single_embedding now call enqueue_embedding_index_sync explicitly per row (no-op when the backend is off), the hooks.py docstring and design-doc diagram no longer overclaim a "single chokepoint" (in-app pipeline chokepoint + the worker-upload bulk path hooked explicitly, with a warning for future direct-write producers), and test_worker_upload_direct_writes_fan_out covers both paths end-to-end into the index. The 132 worker-upload regression tests stay green.

Finding 2 (six-kind taxonomy mismatch) — already fixed in round 12, which the checked-out branch appears to predate: EMBEDDABLE_PARENT_KINDS is now scoped to document/annotation/note only, the write hook skips non-indexed kinds (so relationship embeddings from embeddings_task.py no longer fan out), and test_non_indexed_parent_kinds_do_not_fan_out locks in the symmetry. The conversation/message/relationship read-path migration is the documented follow-up.

The multi-dimension-per-row observation is correct and pre-existing; the rebuild command indeed iterates all DIM_TO_FIELD_MAP entries per row deliberately. And agreed on the human-review note — the automated rounds are no substitute for a maintainer reading engine.py end-to-end before merge.

Suite is now 38 tests; all regression suites remain green.

---

Generated by Claude Code

[claude]

Review: Object-storage vector search backend (turbopuffer-style)

This is a genuinely impressive piece of engineering — the design doc, the deferred-GC/WAL-ordering reasoning, the 37-test suite, and the real MinIO/fake-gcs-server verification scripts all show unusual rigor for an opt-in, off-by-default feature. I traced through engine.py, router.py, object_store.py, hooks.py, vector_index_tasks.py, Managers.py, and checks.py in detail. Summary: no correctness bugs found in the core read/write/compaction paths. A few notes below, none blocking.

Things that look solid (worth calling out, not just "no bugs")

• WAL-before-manifest read ordering in engine.search/wal_tail_count is correctly reasoned through — I traced the deferred-GC interaction (compact() only deletes what the previous manifest superseded) against a reader holding a stale manifest, and the invariant holds: a reader can never observe a manifest paired with an already-GC'd WAL tail.
• Permission preservation: search_via_object_index re-filters entirely through the caller's own queryset (qs.filter(pk__in=...)), so visibility/corpus scoping is identical to the pgvector path by construction. The shortfall-triggers-fallback rule is a nice touch — it keeps "enabling the backend can't make results worse than pgvector" actually true rather than just documented.
• Write fan-out completeness: EMBEDDABLE_PARENT_KINDS as the single source of truth across router.py/vector_index_tasks.py/hooks.py prevents the read/write namespace-map divergence that would otherwise silently produce empty search results. I checked both bulk-write bypasses (worker_uploads/tasks.py's bulk_create/update_or_create paths) and confirmed enqueue_embedding_index_sync is called explicitly in both, with a docstring warning future bypass authors to do the same.
• Managers.py::store_embedding's update/create/race-recovery branches all reach enqueue_embedding_index_sync exactly once regardless of path (verified the create-then-IntegrityError-then-get branch falls through to the same call at the end rather than duplicating it).

Minor observations (non-blocking)

1. OBJECT_INDEX_MANIFEST_RETRY_DELAY_SECONDS sleep on request thread (engine.py::_load_manifest_expecting_data, flagged already in constants/search.py). This is really just confirming the tradeoff is reasonable: 50ms only fires in the rare mid-overwrite window, and only for namespaces with a non-empty WAL. Fine as-is, but worth a second pair of eyes if this path is ever hit from a synchronous GraphQL resolver under load — a pile-up of concurrent 50ms sleeps during a compaction burst could theoretically add up. Given it's bounded and rare, this is a non-issue for now, not a request for changes.

2. OBJECT_STORE_PUT_OVERWRITE_MAX_ATTEMPTS retry window in put_bytes (object_store.py): the exists() → delete() → save() sequence has a window where a racing writer's freshly-saved blob could get deleted by this process's delete(name) call (between our exists() check and our delete(), the racing writer could delete-then-resave in between). This is already called out as "contention here means the compaction lock was breached" and bounded by retries plus a hard failure (ObjectStoreWriteError) rather than silent data loss, so it's an acceptable, well-documented residual risk rather than a bug — just flagging that I traced it and it matches the stated design intent (lock-breach scenario only, not the happy path).

3. Test coverage looks comprehensive for the scenarios that matter (toggle both ways, shortfall fallback, deleted-row refilter, rebuild dry-run/lock-contention, E002/W003/W004 checks, GCS/MinIO real backends). One gap I didn't see covered: a test exercising sync_embedding_to_object_index's Celery autoretry_for=(Exception,) actually retrying on a transient engine failure (vs. just the fallback-on-error path tested on the read side). Low priority since the retry decorator is declarative, but worth a quick sanity test if you want the full write-path resilience story covered by this suite specifically.

4. Nit: shared/checks.py::check_vector_search_backend shadows the builtin Warning name via from django.core.checks import Error, Warning, register — this matches Django's own idiom for check functions, so not a real issue, just flagging in case a linter complains.

Security

The opencontracts.W004 check for public-read storage signals is a nice defense-in-depth addition given the design doc's explicit callout that index blobs carry no ACL of their own — this is exactly the kind of check I'd want to see for a feature that moves permission enforcement to query-time-only. The scope caveat in the docstring (best-effort, Django-settings-only, doesn't see bucket policies set via console/IaC) is honest and appropriately worded.

Nothing else stood out

No dead code, no magic numbers outside the new constants/search.py block, and the default-off toggle plus automatic pgvector fallback make this safe to merge without behavior change for existing deployments. Nice work.

[codecov]

Codecov Report

❌ Patch coverage is 94.75524% with 30 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
opencontractserver/vector_search/engine.py	94.71%	13 Missing ⚠️
opencontractserver/tasks/vector_index_tasks.py	88.00%	6 Missing ⚠️
...management/commands/rebuild_object_vector_index.py	93.33%	4 Missing ⚠️
opencontractserver/vector_search/router.py	95.16%	3 Missing ⚠️
opencontractserver/vector_search/object_store.py	96.22%	2 Missing ⚠️
opencontractserver/shared/checks.py	97.82%	1 Missing ⚠️
opencontractserver/vector_search/hooks.py	94.11%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!