feat(capture): memory embedding tolerates a busy local Ollama - #45
Conversation
ReviewVerdict: approve with should-fix. MERGEABLE on current Behavior matches CL-8615: Should-fix
Fixes for the tombstone filter and a single-flight background pass are in flight on this branch. |
Re-reviewVerdict: approve. SHA 8d2ae2f. Pending-chunk sweep inner-joins memory.version and skips tombstoned / non-live generations. Background embed is a process-wide serial drain with same-tenant coalesce. captureDocument still returns captured without awaiting embed. bun test ./src 490 pass, typecheck 0. |
Capture commits the rows and returns {status:"captured"} with no network
await; embedding runs detached with retry (1s, 5s) plus a bounded pending
sweep. Adds regression tests for the deferred pass, retry/sweep, and
embed-timeout signal threading.
… passes Join memory.version in findPendingChunks so a forget-then-add cannot sweep redacted placeholder text (tombstoned status and non-live generations). Coalesce overlapping background embed passes so concurrent adds share one in-flight drain instead of N retry storms against a busy Ollama.
8d2ae2f to
26340df
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
Review · Verdict: Approve (posted as Comment — same account as author)
captureDocument commits the row-store transaction and returns {status:"captured"} immediately; embedding of the fresh chunks now runs in a detached background pass with bounded retry (1s, 5s) plus a bounded pending-chunk sweep, so a busy local Ollama never stalls the awaiting caller.
Findings
src/services/capture.ts:50— the doc-comment line runs to 100 chars while every surrounding line wraps ~70-78 ("...HTTP caller). Unlike a fire-and-forget capture hook, this" trails onto the same line as the CL-8615 note). Cosmetic only, no behavior impact.
Notes
createBackgroundEmbedScheduler'schainis process-wide, so background embed passes for different tenants also serialize through the same chain, not just same-tenant coalescing — confirmed by the "passes for two tenants still never run concurrently" test. That's a reasonable choice for a single shared local Ollama (the stated scenario), but worth the author owning explicitly if this ever runs against per-tenant embed endpoints, since a slow/retrying pass for one tenant now adds up to ~6s of queuing delay to every other tenant's background embed.findPendingChunksjoiningmemory.versionand excludingtombstoned/non-live rows closes a real gap:tombstoneDocumentredacts chunk text to[redacted]without deleting the chunk row, so a pre-fix sweep on an unembedded chunk from a forgotten document would have embedded the placeholder text.- Diff is scoped to
src/services/capture.tsand its test file only; no migrations, no unrelated files. Did not find dead code, unused exports, unreachable defensive branches, comments restating code, or tests pinning a dependency's own behavior — the SQL-substring assertions in the new tests match an existing pattern already used intimeline.test.ts/search.test.ts, not new hedging.
Checks
bun install: clean.bun run typecheck: clean (tsc --noEmit, no output).bun run test: 457 pass, 0 fail, 44 files (DATABASE_URLunset locally, so the one DB-gated file skips — matches the repo's documented behavior; the PR's own 487/45 figure was run withDATABASE_URLset).- Commit messages: two commits, both match their diffs (
git showon each); no ticket references in either subject/body (git log --format='%B'scan clean); no filename tokens in subjects.
Finishes https://linear.app/abklabs/issue/CL-8615/memory-embedding-tolerates-a-busy-local-ollama.
Behavior:
Verification: