test(backends): live-substrate conformance module for pgvector - #1769
Conversation
Mirrors the portable fake-client arms of test_pgvector_backend.py
against a real PostgreSQL+pgvector server and adds live-only arms the
in-memory fake cannot exercise: real <=> operator ground truth, JSONB
pushdown vs local-fallback equivalence, cross-namespace isolation on
real tables, 8-connection concurrent writers, and the advisory-lock
serialization of run_maintenance('reindex') under a 2-connection race.
Gated on MEMPALACE_PGVECTOR_LIVE_DSN (same pattern as the qdrant live
gate); skips cleanly when unset. First run: 15/15 pass on PostgreSQL
16.10 + pgvector 0.8.2 (+AGE 1.6.0 in the same server), psycopg 3.3.4.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of live-substrate conformance tests for the pgvector backend in tests/test_live_pgvector_conformance.py. The feedback highlights critical concurrency issues, including a race condition when multiple backend instances concurrently write to the same marker file, and a thread-safety issue in the live fixture when appending to a shared list. Additionally, several improvements are suggested to tighten test assertions, such as verifying exact ordered query results and asserting precise execution counts in the reindexing race test.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def writer(worker): | ||
| backend = PgVectorBackend() | ||
| try: | ||
| col = make(tmp_path, backend_=backend) | ||
| for i in range(25): |
There was a problem hiding this comment.
In test_live_concurrent_writers_distinct_connections, 8 concurrent threads are calling col.upsert(...) on collections sharing the same tmp_path. Each upsert call internally invokes _write_marker which opens and writes to tmp_path / "pgvector_backend.json" using "w" mode.
This concurrent write to the same file path from multiple backend instances causes a race condition, which will lead to PermissionError (file sharing violations) on Windows and potential file corruption on Unix. Since the marker is already written during the initial sequential seed step, we can safely stub out _write_marker on the concurrent backend instances to avoid this race.
def writer(worker):
backend = PgVectorBackend()
# Stub out _write_marker to avoid concurrent file-write races on the same tmp_path
backend._write_marker = lambda *args, **kwargs: None
try:
col = make(tmp_path, backend_=backend)
for i in range(25):There was a problem hiding this comment.
Applied in 47bddb0 — the 8 concurrent backends now stub _write_marker (the seed step already wrote it). Worth noting for a possible follow-up: this race isn't only a test artifact — upsert() rewrites the marker on every call via a plain open(path, 'w'), so two real daemon processes writing the same palace path can tear the marker JSON (and hit sharing violations on Windows). An atomic tempfile + os.replace write in _write_marker would close that for production writers too — happy to send that separately if wanted.
| @pytest.fixture | ||
| def live(request, tmp_path): | ||
| """Backend + collection on the live server, namespaced per test.""" | ||
| namespace = "conf_" + request.node.name.replace("[", "_").replace("]", "")[:40] | ||
| backend = PgVectorBackend() | ||
| created = [] | ||
|
|
||
| def make(path, name="drawers", create=True, ns=namespace, dsn=LIVE_DSN, backend_=None): | ||
| b = backend_ or backend | ||
| ref = PalaceRef(id=str(path), local_path=str(path), namespace=ns) | ||
| col = b.get_collection( | ||
| palace=ref, collection_name=name, create=create, options={"dsn": dsn, "namespace": ns} | ||
| ) | ||
| created.append(col) | ||
| return col | ||
|
|
||
| yield backend, make, namespace | ||
| for col in created: | ||
| try: | ||
| col._client.drop_table(col._table) | ||
| except Exception: | ||
| pass | ||
| backend.close() |
There was a problem hiding this comment.
The live fixture defines a created list that is mutated concurrently via created.append(col) inside the make helper function when called from multiple threads in concurrent tests (such as test_live_concurrent_writers_distinct_connections and test_live_reindex_advisory_lock_race).
Standard Python lists are not guaranteed to be thread-safe across all Python implementations or free-threaded builds without synchronization. Adding a threading.Lock ensures safe concurrent mutation of the created list.
@pytest.fixture
def live(request, tmp_path):
"""Backend + collection on the live server, namespaced per test."""
namespace = "conf_" + request.node.name.replace("[", "_").replace("]", "")[:40]
backend = PgVectorBackend()
created = []
lock = threading.Lock()
def make(path, name="drawers", create=True, ns=namespace, dsn=LIVE_DSN, backend_=None):
b = backend_ or backend
ref = PalaceRef(id=str(path), local_path=str(path), namespace=ns)
col = b.get_collection(
palace=ref, collection_name=name, create=create, options={"dsn": dsn, "namespace": ns}
)
with lock:
created.append(col)
return col
yield backend, make, namespace
for col in created:
try:
col._client.drop_table(col._table)
except Exception:
pass
backend.close()There was a problem hiding this comment.
Applied in 47bddb0 — created.append is now guarded by a threading.Lock in the fixture.
| assert result.ids[0][0] == "a" | ||
| assert set(result.ids[0]) == {"a", "b", "c"} |
There was a problem hiding this comment.
Since col.query guarantees ordering by distance (ORDER BY distance ASC), the test should assert the exact ordered list ["a", "b", "c"] to verify that the backend correctly ranks results by distance, rather than just checking the first element and using a set for the rest.
| assert result.ids[0][0] == "a" | |
| assert set(result.ids[0]) == {"a", "b", "c"} | |
| assert result.ids[0] == ["a", "b", "c"] |
There was a problem hiding this comment.
Applied in 47bddb0 — asserting the exact distance-ordered list ["a", "b", "c"].
| ranked = col.query(query_embeddings=[[1, 0]], n_results=3, where={"rank": {"$gte": 2}}) | ||
| assert set(ranked.ids[0]) == {"b", "c"} |
There was a problem hiding this comment.
Since col.query guarantees ordering by distance (ORDER BY distance ASC), the test should assert the exact ordered list ["b", "c"] to verify that the backend correctly ranks filtered results by distance, rather than using a set.
| ranked = col.query(query_embeddings=[[1, 0]], n_results=3, where={"rank": {"$gte": 2}}) | |
| assert set(ranked.ids[0]) == {"b", "c"} | |
| ranked = col.query(query_embeddings=[[1, 0]], n_results=3, where={"rank": {"$gte": 2}}) | |
| assert ranked.ids[0] == ["b", "c"] |
There was a problem hiding this comment.
Applied in 47bddb0 — ranked.ids[0] == ["b", "c"].
| t.join(timeout=60) | ||
|
|
||
| assert errors == [], f"reindex race raised: {errors}" | ||
| assert statuses.count("ran") <= 1 |
There was a problem hiding this comment.
Since the index does not exist initially, exactly one thread must acquire the lock first, build the index, and return "ran". The other thread will either get "already_running" or "noop". Therefore, exactly one thread will have "ran". Asserting == 1 is more precise and robust than <= 1.
| assert statuses.count("ran") <= 1 | |
| assert statuses.count("ran") == 1 |
There was a problem hiding this comment.
Applied in 47bddb0 — statuses.count("ran") == 1, with the docstring updated to match (index absent beforehand, so the advisory-lock winner must build). Re-ran live after all changes: 15/15 pass.
…actly-one-ran asserts
- Stub _write_marker on the 8 concurrent writer backends: upsert()
rewrites the marker on every call with a plain open('w'), so backends
sharing one local_path race on the same file (sharing violations on
Windows) — a test-design artifact, not the contract under test
- Guard the fixture's created list with a lock for the threaded tests
- Assert exact distance-ordered ids in the query/filter arms
- Reindex race: exactly one 'ran' (index absent beforehand, so the
advisory-lock winner must build)
Re-run live after changes: 15/15 pass (PG 16.10, pgvector 0.8.2).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Delivers the offer from #1679 (comment): run the conformance arms against a live PG16 + pgvector + AGE server and report.
What
tests/test_live_pgvector_conformance.py— gated onMEMPALACE_PGVECTOR_LIVE_DSN(same shape as the qdrantMEMPALACE_QDRANT_LIVE_URLgate; 15 tests skip cleanly when unset). Point it at a scratch database and it exercises:test_pgvector_backend.py, mirrored 1:1 against the real_PgVectorClient(add/query/filter/lexical/marker, explicit-embeddings, dimension mismatch, duplicate ids, $or/$contains local-fallback vs equality/$gte JSONB pushdown, marker target-change + backend-mismatch, pure-remote refusal, missing-table-after-marker, cross-palaceassert_partition_isolation)<=>cosine ground truth (0.0/1.0/2.0 for identical/orthogonal/opposite), 8-connection concurrent writers, and a 2-connectionrun_maintenance("reindex")race against the feat(backends): observable maintenance hooks + pgvector indexed-build path (RFC 001) #1732 advisory lock (at most oneran, loser seesalready_running/noop, nobody raises, index present after)First run (production substrate, scratch DB)
PostgreSQL 16.10 (Debian) · pgvector 0.8.2 · AGE 1.6.0 + pg_trgm in the same server · psycopg 3.3.4 ·
develop@ f124bd2 → 15/15 pass. Full report in the #1679 comment thread.🤖 Generated with Claude Code