Skip to content

fix(core): batch relation search refresh cleanup - #1391

Merged
phernandez merged 1 commit into
mainfrom
2589-batch-relation-refresh-delete
Aug 30, 2026
Merged

fix(core): batch relation search refresh cleanup#1391
phernandez merged 1 commit into
mainfrom
2589-batch-relation-refresh-delete

Conversation

@phernandez

Copy link
Copy Markdown
Member

Why

Production relation resolution failed when one tenant accumulated 44,926 durable relation-search refresh markers. clear_pending_search_refreshes() expanded every marker into one DELETE ... IN (...), exceeding asyncpg's hard 32,767 query-argument limit. The sibling generation-fenced completion path had the same unbounded statement shape.

Closes #1390. Tracks Logfire production issue #2589 (trace 01a05271a7f19527aad3e12f3621cd1a).

What Changed

  • Bound refresh-marker deletion to 500 IDs per SQL statement.
  • Apply the same bound to ordinary resolver cleanup and generation-fenced publication cleanup.
  • Add regression tests that compile and inspect each PostgreSQL delete statement, proving the parameter bound is applied.

Implementation Details

The methods continue to execute inside the existing caller-owned transaction, so splitting the SQL does not change atomicity. Project scoping and the accepted-generation predicate remain on every generated statement. A 500-ID batch stays comfortably below asyncpg's 32,767 parameter ceiling and older SQLite parameter ceilings.

Testing

Passed:

  • ./.venv/bin/pytest tests/repository/test_relation_search_refresh_batching.py tests/indexing/test_relation_search_refresh_retry.py -q (4 passed, SQLite)
  • BASIC_MEMORY_TEST_POSTGRES=1 ./.venv/bin/pytest tests/repository/test_relation_search_refresh_batching.py tests/indexing/test_relation_search_refresh_retry.py -q (4 passed, PostgreSQL)
  • uv run ruff check src tests test-int
  • uv run ruff format --check src/basic_memory/repository/relation_repository.py tests/repository/test_relation_search_refresh_batching.py
  • just typecheck
  • git diff --check

Load-bearing proof: temporarily replacing both batching loops with a single unbounded batch made both new tests fail with the observed five-ID statement instead of the required 2/2/1 split; restoring the fix returned them to green.

Risks / Follow-ups

This changes only statement partitioning; transaction ownership and deletion predicates are unchanged. After this reaches Basic Memory Cloud, rerun the affected tenant's resolver and verify Logfire #2589 does not recur before resolving it.

Signed-off-by: phernandez <paul@basicmachines.co>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-30T18:29:40.275874Z a6b7f44 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@phernandez phernandez added bug Something isn't working production Observed in production labels Aug 30, 2026
@phernandez
phernandez merged commit 8859774 into main Aug 30, 2026
25 checks passed
@phernandez
phernandez deleted the 2589-batch-relation-refresh-delete branch August 30, 2026 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working production Observed in production

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Relation search refresh cleanup exceeds asyncpg parameter limit (prod)

1 participant