Skip to content

Commit 696d71b

Browse files
phernandezclaude
andcommitted
perf(core): defer local note materialization off the accept path
The local accepted-note write materialized the markdown file AND re-indexed it inline (awaited in the v2 routes), so the accept paid for the DB-first accept plus the heavy file write + index. A write-load benchmark showed this made local writes ~3x slower than main with no offsetting benefit, because the "offline queue" win is cloud/PGQ-specific. Bring local into parity with cloud: cloud's materialize_write_change enqueues a PGQ job and returns immediately; local now schedules the file write + index as an in-process background task and returns the accepted note_content state at once. note_content remains the write/read-through cache that serves reads; the markdown file stays the source of truth, written just after the 202. Test mode keeps materialization inline so existing tests assert file/search state synchronously. Clear PARITY INVARIANT comments guard the flow on both the materializer and the router follow-up scheduler so a later change does not silently re-inline it. Refs benchmarks/docs/write-load-benchmark.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
1 parent 7ecb672 commit 696d71b

4 files changed

Lines changed: 118 additions & 2 deletions

File tree

src/basic_memory/api/v2/routers/knowledge_router.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,14 @@ def _schedule_post_write_followups(
8989
always runs so a newly written note back-resolves inbound forward references
9090
that name it — parity with the watcher's relation repair, which the inline
9191
write path does not otherwise trigger (#1015). Both are no-ops in test mode.
92+
93+
CLOUD/LOCAL PARITY (do not break): every follow-up here is a core *router
94+
scheduler* that the runtime overrides via dependency injection — local runs
95+
it in-process (debounced), cloud overrides each to enqueue a PGQ job. Cloud
96+
must override BOTH get_entity_vector_sync_scheduler AND
97+
get_relation_resolution_scheduler; if a new follow-up scheduler is added
98+
here, cloud needs a matching override or it will run the local in-process
99+
work on the cloud API server. See basic_memory_cloud api/deps/cloud_overrides.
92100
"""
93101
if app_config.semantic_search_enabled:
94102
vector_sync_scheduler.schedule_entity_vector_sync(

src/basic_memory/cloud/note_content_materialization.py

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,12 @@
22

33
from __future__ import annotations
44

5+
import asyncio
56
from collections.abc import Mapping
67
from dataclasses import dataclass, replace
78
from uuid import UUID
89

10+
from loguru import logger
911
from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker
1012

1113
from basic_memory import db, file_utils
@@ -40,6 +42,11 @@
4042

4143
LOCAL_NOTE_CONTENT_TENANT_ID = UUID(int=0)
4244

45+
# Strong references to in-flight deferred materialization tasks. asyncio only
46+
# keeps weak references to tasks, so without this a fire-and-forget background
47+
# materialization could be garbage-collected mid-write.
48+
_pending_materializations: set[asyncio.Task[None]] = set()
49+
4350

4451
def note_content_payload_file_path(
4552
payload: RuntimeNoteContentResponsePayload,
@@ -197,15 +204,58 @@ class LocalNoteContentMaterializationProvider:
197204
session_maker: async_sessionmaker[AsyncSession]
198205
file_service: FileService
199206
file_indexer: IndexFileExecutor | None = None
207+
test_mode: bool = False
200208

201209
async def materialize_write_change(
202210
self,
203211
accepted: RuntimeAcceptedNoteChange[RuntimeNoteContentResponsePayload],
204212
) -> RuntimeAcceptedNoteChange[RuntimeNoteContentResponsePayload]:
205-
"""Write accepted note content to the local filesystem when needed."""
213+
"""Materialize an accepted note write OFF the accept path.
214+
215+
Cloud/local parity (DO NOT UNDO): cloud's materialize_write_change
216+
enqueues a PGQ job and returns immediately, letting Tigris object storage
217+
+ indexing catch up asynchronously because S3 writes are slow. Locally we
218+
mirror that with an in-process background task. The accept has already
219+
persisted note_content (the write/read-through cache that serves reads);
220+
here we only schedule writing the markdown file (the source of truth) and
221+
indexing it. Writing + indexing the file is the heavy part of a write, so
222+
doing it inline reintroduces a ~3x write-load regression
223+
(benchmarks/docs/write-load-benchmark.md).
224+
225+
PARITY INVARIANT: production must defer. Test mode runs inline ONLY so
226+
tests can assert file/search state synchronously — never make the
227+
production path synchronous to "simplify" this.
228+
"""
206229
if accepted.materialization is None:
207230
return accepted
231+
if self.test_mode:
232+
return await self._materialize_write_now(accepted)
233+
self._schedule_materialization(accepted)
234+
return accepted
235+
236+
def _schedule_materialization(
237+
self,
238+
accepted: RuntimeAcceptedNoteChange[RuntimeNoteContentResponsePayload],
239+
) -> None:
240+
task = asyncio.create_task(self._materialize_write_logged(accepted))
241+
_pending_materializations.add(task)
242+
task.add_done_callback(_pending_materializations.discard)
243+
244+
async def _materialize_write_logged(
245+
self,
246+
accepted: RuntimeAcceptedNoteChange[RuntimeNoteContentResponsePayload],
247+
) -> None:
248+
try:
249+
await self._materialize_write_now(accepted)
250+
except Exception: # pragma: no cover - defensive background guard
251+
logger.exception("Local note materialization failed")
208252

253+
async def _materialize_write_now(
254+
self,
255+
accepted: RuntimeAcceptedNoteChange[RuntimeNoteContentResponsePayload],
256+
) -> RuntimeAcceptedNoteChange[RuntimeNoteContentResponsePayload]:
257+
if accepted.materialization is None: # pragma: no cover - guarded by caller
258+
return accepted
209259
storage = LocalNoteContentStorage(self.file_service)
210260
cleanup_enqueuer = InlineNoteFileDeleteEnqueuer(storage)
211261
result = await run_note_materialization(

src/basic_memory/deps/services.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -732,12 +732,19 @@ async def get_note_content_materialization_provider(
732732
file_service: FileServiceV2ExternalDep,
733733
file_indexer: IndexFileExecutorV2ExternalDep,
734734
session_maker: SessionMakerDep,
735+
app_config: AppConfigDep,
735736
) -> LocalNoteContentMaterializationProvider:
736-
"""Create the local inline materializer for accepted-note route writes."""
737+
"""Create the local materializer for accepted-note route writes.
738+
739+
test_mode keeps materialization inline so tests can assert file/search state
740+
synchronously; production defers the file write + index off the accept path
741+
for cloud parity (see LocalNoteContentMaterializationProvider).
742+
"""
737743
return LocalNoteContentMaterializationProvider(
738744
session_maker=session_maker,
739745
file_service=file_service,
740746
file_indexer=file_indexer,
747+
test_mode=app_config.is_test_env,
741748
)
742749

743750

tests/cloud/test_note_content_materialization.py

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
from __future__ import annotations
44

5+
import asyncio
56
from datetime import UTC, datetime
67
from hashlib import sha256
78
from typing import Any, cast
@@ -86,11 +87,16 @@ def accepted_materialization_change() -> RuntimeAcceptedNoteChange[
8687

8788
def local_materialization_provider(
8889
indexer: RecordingFileIndexer,
90+
*,
91+
test_mode: bool = True,
8992
) -> LocalNoteContentMaterializationProvider:
93+
# test_mode=True keeps materialization inline so these tests can assert the
94+
# result synchronously; production defers it to a background task.
9095
return LocalNoteContentMaterializationProvider(
9196
session_maker=cast(async_sessionmaker[AsyncSession], object()),
9297
file_service=cast(FileService, object()),
9398
file_indexer=indexer,
99+
test_mode=test_mode,
94100
)
95101

96102

@@ -153,3 +159,48 @@ async def fake_run_note_materialization(
153159
assert response_payload["last_materialization_error"] == "Refusing to overwrite notes/test.md"
154160
assert response_payload["file_checksum"] == "external-checksum"
155161
assert response_payload["sync_error"] == NOTE_CONTENT_EXTERNAL_CHANGE_SYNC_ERROR
162+
163+
164+
@pytest.mark.asyncio
165+
async def test_local_materialization_defers_write_off_the_accept_path(
166+
monkeypatch: pytest.MonkeyPatch,
167+
) -> None:
168+
"""Production (test_mode=False) returns the accepted DB state at once, writes async.
169+
170+
Cloud parity: the accept persists note_content and returns 202; the markdown
171+
file (source of truth) and its index are written off the request path.
172+
"""
173+
requests: list[RuntimeNoteMaterializationJobRequest] = []
174+
175+
async def fake_run_note_materialization(
176+
request: RuntimeNoteMaterializationJobRequest,
177+
**_: Any,
178+
) -> RuntimeNoteMaterializationResult:
179+
requests.append(request)
180+
return RuntimeNoteMaterializationResult(
181+
entity_id=42,
182+
status=RuntimeNoteMaterializationStatus.conflict,
183+
reason="deferred",
184+
file_path="notes/test.md",
185+
file_checksum="c",
186+
)
187+
188+
monkeypatch.setattr(
189+
note_content_materialization,
190+
"run_note_materialization",
191+
fake_run_note_materialization,
192+
)
193+
indexer = RecordingFileIndexer()
194+
accepted = accepted_materialization_change()
195+
196+
result = await local_materialization_provider(
197+
indexer, test_mode=False
198+
).materialize_write_change(accepted)
199+
200+
# Returned immediately with the accepted DB state — no inline write yet.
201+
assert result is accepted
202+
assert requests == []
203+
204+
# The write happens off the accept path; drain the background task to confirm.
205+
await asyncio.gather(*list(note_content_materialization._pending_materializations))
206+
assert len(requests) == 1

0 commit comments

Comments
 (0)