Skip to content

fix(postgres): incrementalize change-triggered vector reconciliation - #1207

Merged
mjacobs merged 21 commits into
kenn-io:mainfrom
rodboev:pr/1197-pg-watch-vector-reconciliation
Jul 24, 2026
Merged

fix(postgres): incrementalize change-triggered vector reconciliation#1207
mjacobs merged 21 commits into
kenn-io:mainfrom
rodboev:pr/1197-pg-watch-vector-reconciliation

Conversation

@rodboev

@rodboev rodboev commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Change-triggered pg push --watch reconciliations can run every 30 to 40 seconds while one session is active, and the vector phase originally read state in proportion to the full generation. The deployment behind #1197 measured 6,034 pushes over 62.66 hours and about 17.1 MB received for one push, on the order of 100 GB on this PostgreSQL path. Those measurements come from @mudssky's issue report.

This keeps the branch's changed-session vector scoping and generation-wide maintenance, but rebuilds the local vectors.db export contract underneath it. Change-triggered pushes still derive their candidates from the relational push and read PostgreSQL vector state only for those sessions, while startup, the interval floor, shutdown flush, pg push --full, generation replacement, and recovery still reconcile the full generation. The local vector source now serves one vector phase through one SQLite read transaction. That snapshot establishes the active generation, the in-place-rebuild marker, and the scoped coverage before exposing hashes or documents, so a same-fingerprint rebuild can no longer be read as complete across separate queries and committed as a partial mirror.

A scoped push still promotes itself to generation-wide reconciliation whenever the immutable PostgreSQL generation id changes, a prior vector phase errored, or the watch process has not yet completed one full reconciliation, so a freshly created or recreated generation is never left partially populated. A generation whose local build is mid-flight is refused with ErrVectorSourceNotReady and retried after the build settles rather than exported partially. The per-session PostgreSQL ownership and replacement transaction, conflict and deferral rules, watch scheduling, session push phase, schemas, and non-PostgreSQL backends keep their existing behavior. The traffic measurements and the separation between PostgreSQL watch traffic and SSH remote-sync traffic come from @mudssky's issue report.

Closes #1197

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (ac6fe06)

Changes requested: two medium-severity performance issues remain in daemon-backed and change-scoped PostgreSQL watch pushes.

Medium

  • cmd/agentsview/archive_write_backend.go:329 — Daemon-backed pg watch discards the push reason and sends one-shot PGPush requests. Each request creates a fresh postgres.Sync, so ChangedVectorStateOnly is never enabled and its in-memory baseline cannot persist. Change events therefore continue performing full vector-state reconciliation when a local daemon is active.

    Fix: Propagate change-scoped intent through the daemon request, preserve a per-target vector baseline across daemon pushes, and add coverage for daemon-backed watch mode.

  • internal/postgres/vector_push.go:285 — The changed-scope path still calls SessionDocHashes, scanning and materializing every embedded document. It then walks the entire hash map and clones the full baseline around line 389, leaving every filesystem event O(total archive size) in local work and allocations.

    Fix: Carry changed session IDs or an incremental vector revision journal into this phase, query hashes only for candidates and deletions, update the baseline incrementally, and add a small-versus-large archive cardinality regression.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 5m32s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (2dbe4d9)

High-severity issue found: scoped vector reconciliation misses vector-only changes.

High

  • internal/postgres/push.go:399 — Scoped candidates are derived only from changed SQLite sessions. After the initial push, vector-source-only mutations produce no pending session IDs, causing reconciliation to return early. This breaks changed-scope updates, eviction, deferral, and conflict handling, leaving vectors stale until a full or interval push.
    • Fix: Queue affected session IDs whenever vector state changes, or require tests and callers to explicitly mark those sessions as changed candidates before scoped reconciliation.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 4m37s

@rodboev
rodboev force-pushed the pr/1197-pg-watch-vector-reconciliation branch from 2dbe4d9 to bebece9 Compare July 21, 2026 08:03
@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (bebece9)

Changes requested: one medium-severity reconciliation bug could silently omit sessions from later scoped pushes.

Medium

  • internal/postgres/vector_push.go:481 — If the vector source becomes unavailable mid-reconciliation, only already-deferred sessions are marked pending. Unvisited candidates are discarded while the reconciliation state is recorded as valid, so a same-fingerprint rebuild followed by change-scoped pushes can silently omit those sessions until a full reconciliation occurs.

    Suggested fix: Invalidate reconciliation state when processing stops early, or add all unvisited candidates to the pending set. Add a changed-scope regression test covering a same-fingerprint rebuild that begins mid-push.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 5m7s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (053280d)

The change introduces bounded-work regressions in PostgreSQL vector reconciliation that should be addressed before merging.

Medium

  • internal/postgres/vector_push.go:295 — An empty changed-session set falls back to scanning every local and PostgreSQL vector row. No-op watcher events and changes outside a project filter therefore remain O(total archive), violating bounded changed-batch behavior. It also conflicts with TestVectorPushChangedScopeReadsOnlyCandidates, where the second push can repair session B and push two sessions instead of the expected one.

    Suggested fix: Treat an empty pending set as a scoped no-op and rely on interval pushes for vector-only drift, or explicitly track vector-changed session IDs.

  • internal/postgres/vector_push.go:453 — Ownership conflicts and generic skipped outcomes remain in PendingSessionIDs indefinitely. Permanently foreign or PostgreSQL-excluded sessions will be retried on every filesystem event, allowing per-event work to scale with archive cardinality.

    Suggested fix: Persist only transiently deferred or failed candidates. Let later archive changes re-enqueue permanent skips, and use interval reconciliation for external ownership changes.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 4m33s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (b9bd972)

Verdict: One medium-severity test coverage flaw must be fixed; no critical or high-severity issues were identified.

Medium

  • Tests exercise unreachable vector-push pathsinternal/postgres/vector_push_pg_test.go:314, 349, 513, 932

    Several changed-scope tests mutate only the fake vector source without changing an archive session or adding a pending ID. After the initial push, pending IDs are empty, so pushVectorsWithOptions returns early at internal/postgres/vector_push.go:295. Consequently, the expected push, eviction, deferral, and conflict paths are never reached, causing the PostgreSQL tests to fail.

    Fix: Update the relevant local session so its push fingerprint changes before calling PushWithOptions, or explicitly seed mergeVectorPushPendingSessions when testing the vector phase directly. Tests for vector-only drift should instead assert the documented no-op behavior.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 5m46s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (0c29bdb)

No issues found.


Reviewers: 2 done | Synthesis: codex | Total: 5m12s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (3797afa)

Changes need a fix before approval due to a reconciliation correctness issue.

Medium

  • internal/postgres/vector_push.go:486 — If the vector source becomes unready during reconciliation, the persisted state may still be marked valid while already visited sessions have changed under the same fingerprint. Their PostgreSQL vectors can remain stale because subsequent change-scoped pushes will not revisit them until the next periodic full reconciliation. Invalidate reconciliation state whenever the source changes mid-phase, or requeue all in-scope candidates—including visited sessions—before persisting valid state. Apply the same handling before eviction.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 4m29s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (78c6e88)

Code review found one medium-severity correctness issue; no security regressions were identified.

Medium

  • internal/postgres/push.go:331purgePGExcludedPushSessions removes excluded sessions from sessionByID before changed-vector candidates are recorded. Because vector tables do not cascade from sessions, a change-triggered push can leave the excluded session’s vector documents, chunks, and push state behind until a later full or interval reconciliation.

    Suggested fix: Return the purged canonical and alias IDs from purgePGExcludedPushSessions and merge them into the pending vector candidate set. Add an integration test covering exclusion during a changed-scope push.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 7m20s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (528afe1)

Changes requested: two medium-severity issues could leave stale vector data and increase memory usage during full reconciliation.

Medium

  • internal/postgres/push.go:405 — Excluded sessions are queued as vector candidates, but their hashes remain in the local vector index. Equal hashes are treated as unchanged, while changed hashes are skipped after the PostgreSQL session row is deleted. This can leave vector state, documents, and chunks indefinitely, polluting semantic-search results. Carry excluded IDs through reconciliation as forced evictions until cleanup succeeds, and test with the excluded session still present in the local vector source.

  • internal/vector/export.go:89, internal/postgres/vector_push.go:417 — Full reconciliation retains a SHA-256 hasher for every session, and applyVectorDeltas allocates and fills an unused visited map. This adds memory proportional to archive cardinality during periodic scans. Hash and flush each session while consuming the ordered rows with one reusable hasher, and remove the unused map.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 4m38s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (cb093b6)

Changes need revision: two medium-severity issues remain in exclusion-driven PostgreSQL vector cleanup.

Medium

  • internal/postgres/vector_push.go:453 — Excluded sessions can remain indefinitely in the local vector index. When the PostgreSQL session row is deleted, the local-candidate path ignores sessionExists=false; equal hashes are treated as unchanged, full pushes no-op, and eviction skips IDs still present in local. Propagate excluded IDs as forced evictions regardless of local hashes, and test while retaining the local vector source entry.

  • internal/postgres/push.go:1550 — Exclusion reconciliation incorrectly depends on whether the local archive candidate list is empty. Empty lists repeatedly scan and materialize every historical tombstone, causing work proportional to archive size; non-empty lists ignore exclusions outside the candidate set, potentially delaying cleanup indefinitely. Reconcile newly pending exclusions independently using persisted revision/cursor state, and add a cardinality-scaling regression.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 5m58s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (9a51a2f)

Incremental exclusion processing has four medium-severity correctness and performance issues that should be addressed before merge.

Medium

  • Cursor advances before vector candidates are persistedinternal/postgres/push.go:1759
    A crash or fingerprint-preparation error after cursor advancement causes retries to skip purged IDs, leaving orphaned vector rows until full reconciliation. Persist candidate IDs before advancing the cursor, ideally in one coordinated local-state operation.

  • Failed sessions can still have vectors evictedinternal/postgres/vector_push.go:454
    Orphaned vector state is queued for eviction before consulting failedSessions. If the session-phase write failed, vectors are deleted instead of deferred. Check failedSessions first and retain those sessions in the pending set.

  • Timestamp cursor is not commit-safeinternal/postgres/push.go:1598
    PostgreSQL NOW() reflects transaction start time, so an earlier-started transaction can commit after the cursor passes a later transaction and then be permanently skipped. Use a commit-safe cursor, serialize creation and consumption, or retain a safely deduplicated overlap.

  • Incremental exclusion query lacks a supporting indexinternal/postgres/push.go:1594
    Without a (created_at, id) index, each watch-triggered push can scan and sort the continually growing tombstone table. Add the composite index through the idempotent schema-upgrade path, require it in pushSchemaCurrent, and use an index-compatible tuple range predicate.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 8m0s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (8a5e5b0)

Medium-severity issue found in PostgreSQL vector tombstone cleanup; no security vulnerabilities identified.

Medium

  • internal/postgres/push.go:1673vector_cleanup_done globally seals a tombstone after candidates are persisted only for the current sync scope and active vector generation. Other machines or generations then skip the tombstone, potentially leaving stale vector state and chunks that crowd valid semantic-search results until full reconciliation.
    • Suggested fix: Track cleanup acknowledgments per generation/pusher, or purge vector rows for all generations before marking the tombstone complete. Add a multi-generation regression test.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 7m41s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (d2a817f)

The change is functionally promising, but two medium-severity scalability regressions should be addressed.

Medium

  • Unbounded pending-session queue during skipped vector reconciliationinternal/postgres/push.go:415, internal/postgres/vector_push.go:252
    Changed sessions accumulate in PendingSessionIDs when reconciliation is unavailable or already invalid. Skip paths—such as missing pgvector, insufficient privileges, no active generation, or an unready source—leave the queue intact, causing watch events to eventually read, sort, and rewrite up to the entire archive. Internally forced full pushes also unnecessarily queue every session. Persist an invalid baseline with an empty queue on skipped phases, avoid merging IDs while the baseline is invalid, and require effective !full before enabling changed-only tracking. Add a cardinality-scaling regression test.

  • Per-session hashers increase full-scan memory usageinternal/vector/export.go:88
    Full hash scans retain a separate SHA-256 hasher for every session until the entire result set is consumed, replacing the previous constant-memory streaming approach and potentially adding hundreds of megabytes of transient memory on large archives. Stream ordered rows, finalize each session hash when its ID changes, and reuse a single hasher. Candidate chunks can be finalized independently because session IDs are deduplicated before chunking.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 7m59s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (4b87f30)

Medium-severity issues remain in PostgreSQL vector cleanup and tombstone processing.

Medium

  • internal/postgres/vector_push.go:1244 — If vector_push_state is missing, chunk deletions are rolled back but the function returns success. The caller then marks each candidate’s vector_cleanup_done as true, permanently leaving stale chunks/documents after a partial vector-schema reset. Do not swallow the undefined-table error after rollback; ensure or probe the vector schema before cleanup, or mark tombstones complete only after cleanup commits.

  • internal/postgres/push.go:1546 — Pending exclusions are processed as one unbounded batch, with completion flags written only after the entire batch succeeds. Following migration, a late timeout or error can cause every subsequent push to repeat already committed work and potentially starve the tail. Process a bounded number of tombstones per push and persist completion after each committed session, or record the successfully processed prefix before returning an error.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 8m32s

@rodboev
rodboev marked this pull request as draft July 21, 2026 15:36
@rodboev
rodboev force-pushed the pr/1197-pg-watch-vector-reconciliation branch from 4b87f30 to b58379a Compare July 21, 2026 16:50
@rodboev
rodboev marked this pull request as ready for review July 21, 2026 17:32
@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (6e2a6e2)

Semantic search can temporarily expose an incomplete vector generation after a scoped push.

Medium

  • internal/postgres/vector_push.go:256 — A scoped push can discover and register a newly active vector generation while pushing vectors only for relationally changed sessions. Because PostgreSQL serving treats the generation row and chunk table as ready, searches may use an incomplete generation until the next interval reconciliation.
    • Fix: Track the generation fingerprint that completed the last generation-wide reconciliation and force a full generation push whenever the active fingerprint changes. Add a regression test that switches generations before a scoped change push.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 7m13s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (147eeb8)

Changes requested: one medium-severity issue could expose incomplete semantic search results after PostgreSQL generation recreation.

Medium

  • internal/postgres/vector_push.go:262 — Promotion relies only on the local fingerprint. If the matching PostgreSQL generation is dropped or its vector tables are recreated while the watcher retains the same fingerprint, the next scoped push recreates the generation with only changed sessions. This exposes incomplete semantic results until interval reconciliation.

    Suggested fix: Also promote when the target generation or this machine’s reconciliation state is newly created or missing. Add a regression test covering deletion and recreation of the active generation with an unchanged fingerprint.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 5m54s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (a0ef276)

The change is generally sound, but a medium-severity concurrency gap remains in generation recreation.

Medium

  • internal/postgres/vector_push.go:284 — Generation recreation is detected only when the current push inserts the generation row. After an admin drop, concurrent watchers can race: the first recreates and reconciles its corpus, while another sees the same fingerprint and remains scoped, leaving most of its sessions absent until interval reconciliation. Track a stable generation identity/epoch—or promote reconciliation when the machine’s generation registration was absent—and add a concurrent multi-machine recreation test.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 5m33s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (8df9551)

Medium-severity issue found in cross-machine vector generation reconciliation.

Medium

  • internal/postgres/vector_push.go:284 — Recreated-generation detection only works when the current Sync inserts the generation row. If another machine first recreates a generation with the same fingerprint, generationCreated remains false and the fingerprint still matches the memo, causing a scoped push even though this machine’s prior vector state was deleted. Unchanged sessions remain absent until periodic generation-wide reconciliation.

    Suggested fix: Track and compare the immutable PostgreSQL generation ID, or promote to full reconciliation when this machine’s vector_generation_machines row must be recreated. Add a two-machine test where one machine recreates the dropped generation before the other performs a scoped push.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 6m48s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (f220d14)

Changes requested: one medium-severity scalability issue remains in scoped vector pushes.

Medium

  • internal/postgres/vector_push.go:241 — Every non-empty scoped push still calls Generation, whose GenerationByID coverage query counts all documents and stamps. This keeps each filesystem-triggered push O(total archive size), defeating bounded background reconciliation.

    Suggested fix: Make readiness checks scope-aware or persist an O(1) generation-complete marker, reserving full coverage scans for generation-wide pushes. Add a small-versus-large archive regression test asserting constant per-event work.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 7m26s

@rodboev

rodboev commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

This readiness scan is unchanged from the base branch. The PR scopes the two reads behind the 17.1 MB/push (SessionDocHashes and readVectorPushState); the Generation() coverage check was already there and I left it alone.

It's cheap on its own: two index-assisted counts returning a couple of integers, no rows built in Go, nothing read from Postgres. It's not the cost this PR set out to cut, and sizing it fairly needs a profile.

I can't just skip it for scoped pushes. A same-fingerprint rebuild clears the active generation's stamps in place, so a half-rebuilt session would look complete and we'd delete valid PG vectors. Doing it safely means a real scope-aware completeness check in internal/vector, which is its own change.

@rodboev
rodboev force-pushed the pr/1197-pg-watch-vector-reconciliation branch from f220d14 to 8f75e94 Compare July 23, 2026 02:48
@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (8f75e94)

Medium-severity issue found in vector reconciliation compatibility; no security regressions identified.

Medium

  • cmd/agentsview/pg_watch.go:143 — A successful generation-wide response with GenerationID == 0 clears vectorReconcileNeeded. Because API version 3 remains compatible with older daemons that omit this new field, an upgrade while the watch process is still running can leave the next change scoped without a trusted generation ID. Since pushVectors skips promotion for a zero memo, the new generation may remain partially populated until interval reconciliation. Preserve the reconcile flag after an unscoped push unless a nonzero generation ID is returned, or bump the daemon API version so older responses are rejected.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 7m41s

@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (fe5e47a)

Scoped vector reconciliation still has two medium-severity correctness and scalability issues.

Medium

  • internal/postgres/vector_push.go:241, cmd/agentsview/vector_push_source.go:160 — Every scoped push calls GenerationByID, whose coverage query counts all document and stamp rows. Watch-triggered work therefore remains proportional to total archive size despite later reads being scoped. Use scope-aware readiness checks or maintained build-state metadata for scoped pushes, reserving full coverage counts for generation-wide reconciliation. Add a small-versus-large archive regression test.

  • internal/postgres/vector_push.go:283 — A BIGSERIAL ID is not an immutable generation incarnation after vector tables are recreated: the new sequence can reuse the memoized generation ID. A scoped push can then miss the reset and populate only changed sessions, leaving semantic search incomplete until a generation-wide push. Compare a recreation-safe token, such as a randomly generated generation UUID, and test dropping and recreating all vector tables while retaining the relational sync marker.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 5m52s

@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (97b3dd0)

Medium-severity correctness issue found in PostgreSQL vector reconciliation; no security issues identified.

Medium

  • internal/postgres/vector_push.go:735 — The machine record is written before vector reconciliation succeeds, but later scoped pushes treat its existence as proof that the generation incarnation was fully reconciled. A canceled or failed generation-wide push can leave the record behind; if the recreated generation reuses the memoized ID, another watcher for the same machine may perform only a scoped push, leaving most vectors missing until interval reconciliation.
    • Suggested fix: Record successful generation-wide reconciliation only after applyVectorDeltas completes, or store a separate incarnation/completion marker. Add a regression test covering an aborted push that leaves a machine row before a scoped push.

Reviewers: 2 done | Synthesis: codex, 17s | Total: 4m18s

@rodboev

rodboev commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Both roborev findings addressed.

The BIGSERIAL id-reuse one is real and fixed in 97b3dd0f: recreating the vector tables restarts the sequence, so a single-generation reset can re-register the same fingerprint under an id a watch process already memoized, slipping past the id check. Keyed the incarnation check on this machine's vector_generation_machines record instead — wiped in the same reset, so its absence flags the recreation with no schema change. Regression drops all vector tables and confirms the scoped push promotes and refills.

roborev is right on the coverage count too: it leaves per-event work O(archive) on scoped pushes even though the hash and state reads are now scoped. Folding that fix in here rather than deferring — the readiness count takes the same changed-session scope the hash read already carries, with the full count reserved for generation-wide reconciliation. It's safe for the same reason the scoped push is: writes and eviction are already bounded to that set. Pushing shortly.

rodboev added 9 commits July 23, 2026 13:57
…n, and skip promotion without a fingerprint memo
… id so any recreation, by any machine, forces a generation-wide reconcile
…id, so a daemon predating the field cannot leave scoped pushes with a memo the vector phase refuses to promote
…record, since recreating the vector tables restarts the id sequence and can hand the recreated generation the memoized id
@rodboev
rodboev force-pushed the pr/1197-pg-watch-vector-reconciliation branch from 8a7bde0 to 467e881 Compare July 23, 2026 18:03
@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (467e881)

Medium

  • cmd/agentsview/vector_push_source.go:163 — The full-rebuild marker is checked only during Generation. A same-fingerprint rebuild can begin after that check but before SessionDocHashes or SessionDocs, allowing a scoped push to export and persist a partially refilled session if the partial aggregate remains stable long enough to pass hash comparison. Couple readiness and export through a consistent SQLite snapshot or rebuild epoch, or recheck the marker before hash and document exports and abort with ErrVectorSourceNotReady. Add a synchronized concurrent-rebuild regression test.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 9m56s

@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (fdca976)

Code changes look sound, but one stale PostgreSQL vector-push test must be updated for the new snapshot semantics.

Medium

  • internal/postgres/vector_push_pg_test.go:1310TestVectorPushEvictionSkippedWhenSourceTurnsUnready still expects a second generation probe, while unpromoted pushes now call BeginExport only once. Its call == 2 branch is unreachable, causing B to be evicted and invalidating the expected deferral/no-eviction assertions. Update the test to expect eviction from the stable export, and cover an unavailable source by returning ErrVectorSourceNotReady from the initial BeginExport.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 6m16s

@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (bec51e2)

No issues found.


Reviewers: 2 done | Synthesis: codex | Total: 9m49s

@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (4b30e3c)

Medium-severity issue found in scoped vector push promotion; no security vulnerabilities identified.

Medium

  • internal/postgres/vector_push.go:273 — A scoped push registers the local generation and creates its chunk table before determining whether it must promote to a generation-wide export. If the subsequent full BeginExport reports pending documents or a different generation, the function returns while leaving an empty generation registered. PostgreSQL semantic search may then treat the fingerprint and chunk table as available, causing empty or incomplete results until reconciliation; an abandoned initial generation may remain empty indefinitely.

    Suggested fix: Determine promotion using read-only generation and machine lookups, then acquire and validate the full export before registering the PostgreSQL generation or creating its chunk table. Add coverage for a previously unregistered generation whose promoted full export is not ready.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 7m17s

@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (183c609)

Medium-severity concurrency issue found in scoped vector push reconciliation.

Medium

  • internal/postgres/vector_push.go:334 — The scoped-push promotion decision occurs before resolveVectorGeneration. If another process drops and recreates the generation between lookup and resolution, the resolved generation may have a new ID or lack the expected machine witness, while the push remains scoped and only partially populates the recreated generation.

    Suggested fix: Revalidate the resolved generation against the memo and machine witness before applying scoped deltas. If they differ, promote the operation to a fresh generation-wide export and add a concurrent-recreation regression test.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 5m54s

@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (befbad3)

Medium findings remain in vector export readiness and PostgreSQL generation synchronization.

Medium

  • internal/vector/export.go:283 — The readiness query can treat a temporarily parked document (ordinal < 0) as complete when its old stamp still matches, even though hash and document exports exclude negative ordinals. Because Refresh parks rows using autocommit writes, an export may capture this intermediate state, produce matching reduced hashes/documents, and delete valid PostgreSQL vectors instead of deferring.

    • Fix: Treat any relevant d.ordinal < 0 row as ErrExportNotReady within the export transaction. Keep the duplicate readiness logic in MissingEmbeddedDocs aligned, or make the refresh parking sequence atomic.
  • internal/postgres/vector_push.go:315 — Generation identity is rechecked only immediately after the initial probe. A reset or recreation can still occur after the second lookup but before scoped reads and per-session writes, allowing the push to reconcile a stale generation ID—or a newly recreated generation reusing that ID—without promotion.

    • Fix: Synchronize generation reset/drop operations and scoped reconciliation with a shared advisory lock, or revalidate after scoped writes and immediately retry generation-wide before reporting success if the incarnation changed.

Reviewers: 2 done | Synthesis: codex, 12s | Total: 8m4s

@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (0c8cee6)

Overall verdict: One medium-severity correctness issue remains in filtered PostgreSQL vector synchronization.

Medium

  • internal/postgres/vector_push.go:465 — The generation-incarnation witness is keyed only by (generation_id, machine). Multiple filtered watchers can share a machine name, allowing one filter’s generation-wide push to create a witness that another filter incorrectly trusts after vector tables are recreated and a generation ID is reused. The second watcher may then perform only a scoped push without reconciling unchanged sessions, leaving vector search incomplete until interval reconciliation.

    Suggested fix: Key the witness by the local push marker and sync/filter scope, or use an immutable generation-incarnation token that cannot be satisfied by another filtered pusher. Add a regression test covering two project-filtered watchers across vector-table recreation.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 9m27s

@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (f1dc6da)

Code is generally sound, but one medium-severity concurrency issue should be addressed.

Medium

  • internal/postgres/vector_push.go:443 — Generation-wide reconciliation records its durable witness without verifying that the generation incarnation survived reconciliation. If another process recreates the vector tables after deltas are applied but before the witness insert, the restarted sequence can reuse the same generation ID and receive a witness for an incomplete generation. Subsequent change pushes may remain scoped until interval reconciliation.
    • Suggested fix: Guard full reconciliation with an incarnation token or advisory lock, revalidate it before recording the witness, and retry generation-wide reconciliation if it changed.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 8m48s

@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (b39673d)

Medium-severity concurrency issue found in PostgreSQL vector generation tracking.

Medium

  • internal/postgres/vector_push.go:458-482 — Generation identity is verified and its witness inserted in separate statements. A concurrent table reset between them can reuse the generation ID and receive a stale witness, causing later scoped pushes to trust an incomplete generation.
    • Suggested fix: Insert the witness conditionally from vector_generations, matching the verified ID, fingerprint, and created_at; retry generation-wide when no row is inserted.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 7m42s

@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (5daee9e)

Code is clean with no medium-or-higher severity findings.


Reviewers: 2 done | Synthesis: codex, 6s | Total: 10m12s

@mjacobs
mjacobs merged commit a2c1baa into kenn-io:main Jul 24, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

pg push --watch can read the full PostgreSQL vector state every 30 seconds and consume 100+ GB of network traffic

2 participants