Skip to content

fix: defer retain completion outbox until store commit - #4203

Merged
nicoloboschi merged 2 commits into
vectorize-io:mainfrom
kyletser:fix/store-owned-retain-webhook
Sep 8, 2026
Merged

fix: defer retain completion outbox until store commit#4203
nicoloboschi merged 2 commits into
vectorize-io:mainfrom
kyletser:fix/store-owned-retain-webhook

Conversation

@kyletser

@kyletser kyletser commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #4189.

A store-owned retain session can buffer its facts until commit(). The completion outbox currently queries that store from inside the retain pipeline, before the session commits, so a successful retain can announce memory_unit_count: 0 even though the document ends up with four memories.

This change records the callbacks actually reached by the pipeline and runs them after successful session commit, in a fresh SQL outbox transaction. It preserves document-specific callback factories and keeps counts based on the document's stored total, not the IDs created by this retain. The SQL path and stores without a retain session keep their existing callback timing.

The SQL outbox and an external store are still separate transactional systems; this does not introduce cross-store atomicity. A failed retain/session commit does not publish a success event.

Tests

  • Regression tests cover direct callbacks, per-document factories, unchanged and zero-fact retains, and retain/commit failures.
  • Real PostgreSQL tests exercise both the SQL backend and a buffered store. The existing in-memory test fixture now consumes FactRecord correctly, preserves document fields and allocated IDs, and honors the document filter used by the count assertion.
  • Python 3.11: pytest tests/test_retain_outbox_session.py tests/test_webhooks.py tests/test_memories_extension.py238 passed, 67 skipped. Existing local-ML-dependent tests skip in the slim environment; the new count regressions run with deterministic embeddings and a real isolated pg0 database.
  • Final focused run after rebasing: 26 passed.
  • Mutation check: disabling deferral makes three database regressions fail again, including 0 != 4 and unchanged-retain counts [4, 0].
  • Ruff 0.14.9 API lint, formatting of changed files, changed-runtime-file typecheck with ty 0.0.8, and git diff --check passed.

Local validation limits

The complete API typecheck could not pass in the slim environment because optional ML/provider imports are missing. The all-workspace lint shell script could not start from the CRLF Windows checkout; the Python checks above were run directly. The full repository suite and full free-threaded API environment were not validated locally. A separate source-extracted lifecycle smoke check passed 96 threaded event-loop scenarios on Python 3.14t with the GIL disabled, but it is not a substitute for the free-threaded CI job.

Copilot AI lite review requested due to automatic review settings September 7, 2026 14:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@koriyoshi2041 koriyoshi2041 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The store/SQL boundary looks sound at d760a00: reached callbacks are deferred only for store-owned sessions, the store commit happens before the fresh SQL outbox transaction, and retain or commit failures cannot publish a success event. The factory path also preserves per-document callback contents, while unchanged retains count the committed document total rather than newly created IDs.

The focused matrix covers direct/factory callbacks, previous/committed counts, and both failure points. I also checked that the session path does not sub-batch, so the deferred list does not introduce duplicate per-document completion events. Approved.

kyletser and others added 2 commits September 8, 2026 17:26
Queue reached callbacks while a retain session buffers memories, then publish document counts after its successful commit. Cover document factories, unchanged and zero-fact retains, and commit failures.
…write fails

The deferred completion outbox now runs after the store session has committed,
so a failure there cannot be undone by failing the retain — it would only report
a stored document as lost and invite a duplicate re-submit. Log the dropped
retain.completed event and let the retain report the truth.
@nicoloboschi
nicoloboschi force-pushed the fix/store-owned-retain-webhook branch from ce02c54 to 7bfbacd Compare September 8, 2026 15:26
@nicoloboschi
nicoloboschi merged commit 511c86e into vectorize-io:main Sep 8, 2026
113 of 209 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

retain.completed reports memory_unit_count: 0 for a store-owned bank — the outbox callback counts before the store has written

4 participants