refactor(core): add shared runtime orchestration - #1002
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb65be817b
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
phernandez
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f338a6d382
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 703206a432
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
703206a to
07a8882
Compare
|
@codex review |
1 similar comment
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58e93ca7c8
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
3 similar comments
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
c3c5a7b to
bd4b25c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd4b25ca3f
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the five latest Codex P2 findings in 07c25b2:
Regression coverage was added for each bug. Verification:
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
…objects
to_response_payload emitted errors as plain reason strings, but the
MCP/CLI client validates the payload as DirectoryDeleteResult whose
errors field is a list of DirectoryDeleteError ({path, error}) objects.
Any partial failure (guarded skip or enqueue failure) therefore raised
a Pydantic validation error client-side instead of reporting the failed
paths.
Carry structured DirectoryDeleteFileFailure (path, reason) pairs
through the runner's skip/failure accounting and serialize errors as
objects matching the client schema. Adds a client-shape regression that
validates the payload with DirectoryDeleteResult.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
accept_directory_delete captures the surviving outside-directory sources whose relations pointed into the deleted set (relation_cleanup_entity_ids), but DirectoryDeleteService dropped them, leaving stale relation rows in the search index until an unrelated rebuild. The project-index path already repairs these; the API route path did not. Add a DirectoryDeleteRelationCleanupRefresher capability to the directory-delete runtime and reindex each surviving source after the delete acceptance. Sources deleted in the meantime are skipped, not fatal. The local runtime wires a repository/search-backed refresher; queued runtimes still receive the ids on the accepted result. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
LocalProjectIndexScheduler spawned a bare asyncio task per request, so every POST .../index and the startup scan started independent full coordinator runs over the same rows; overlapping runs are also the trigger for move/delete races. Coalesce per project: while a run is in flight, a new request marks the project dirty (keeping the strongest force_full) and exactly one trailing rerun starts when the current run finishes — matching the relation-resolution debounce in the same module. Test mode stays a no-op without leaking the pending marker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Port the relay-owned note persistence concurrency guard (cloud issue #1445) into the core accepted-note write path that PR #1002 moved out of basic-memory-cloud. Browser saves and the collaboration relay send the db_checksum they last synced as the x-bm-cloud-note-base-checksum header on note PUTs. When the accepted note_content row has advanced past that base, the update runner now rejects the write with a structured 409 whose detail body is {"message": "Note changed since your last sync", "db_checksum": <current or null>} so clients rebase instead of clobbering the newer write. - AcceptedNoteUpdateMutation gains optional base_checksum; the replace branch compares it against the current accepted db_checksum, and the create branch rejects outright (db_checksum null) because a precondition with no surviving entity means the note was deleted after the caller's pre-read - creating it would silently resurrect it. - Unlike cloud main's SELECT ... FOR UPDATE, core needs no row lock: the accept_write compare-and-set on db_version already prevents a write planned against a stale read from landing; a race between the check and the CAS trips the CAS and surfaces the concurrent-write 409. - The rejection detail is typed (AcceptedNoteBaseChecksumConflict) in the runner and serialized to its wire dict at the service boundary; NoteContentMutationServiceError.detail widens to str | dict so routes keep passing it verbatim to HTTPException. - The v2 PUT route reads the header (constant lives in the runtime note-content vocabulary) and threads it through NoteContentMutationService.update_note. An absent header keeps the existing upsert contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…rch refresh RepositoryProjectIndexMovedEntitySearchRefresher raised RuntimeError when any moved entity id no longer had a database row at refresh time. Move batches commit before the refresh runs, so a concurrent delete (file removed, note deleted via API) between move-batch commit and refresh is a benign race — but the raise aborted the whole coordinator run before delete batches and file indexing executed, stalling the scan. A missing entity at refresh time is now logged and skipped; the surviving moved entities are still refreshed. The vanished entity's search rows were already removed with its row, so there is nothing left to repair. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…can reconciliation os.walk's onerror only aborted the scan when the project root itself was unreadable; a deeper unreadable subdirectory was pruned from the observed snapshot with a warning. Delete reconciliation plans all_db_paths - storage_paths, so every indexed row under the vanished subtree was destroyed even though the files still exist. Following the unobservable-file carry-through pattern (46d0011), the walk now reports unreadable non-root directories in a LocalProjectIndexScan result, and the observed-file source carries every indexed path under them through the snapshot as present-with-unknown-checksum (RuntimeObservedIndexFile with checksum None). Those paths classify as modified, never deleted, and the batch planner re-checks them next pass. The indexed-path source is the already-loaded indexed-stat snapshot; the bare observed source (no stat source) has nothing to carry. A walk error without a directory attribution now fails the scan outright: it cannot be carried, and finishing the walk could still plan a mass delete for an unknown subtree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…n reconciliation Scan delete plans compare a storage snapshot against a later DB read (deleted = all_db_paths - storage_paths). A note accepted and materialized between snapshot capture and the DB read was planned as deleted, and the delete batch destroyed its entity, search, and vector rows with no re-verification. apply_project_index_delete_batch now re-confirms each planned path through a ProjectIndexDeletePathVerifier capability before touching the database. The local runtime injects LocalProjectIndexDeletePathVerifier, which probes the filesystem: a path present again (or whose probe fails) is skipped and logged — the next scan picks the file up as modified. Cloud/S3 runtimes keep current behavior through the explicit TrustPlannedProjectIndexDeleteVerifier pass-through default, since their storage listing is authoritative and they have no cheap per-path probe at apply time. Skipped paths are reported on the batch result and delete run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…nation entities Scan change planning only pairs a move with a destination that had no DB row at snapshot time, so an entity found at the destination during apply was created concurrently (e.g. an accepted write_note whose content may not be materialized yet). apply_project_index_move_batch deleted that entity unconditionally and re-pointed the stale source entity onto the path, destroying the accepted content. Scan runtimes now opt into verify_replaced_move_targets on the maintenance store: a replacement row is only deleted when its checksum equals the moved entity's indexed checksum — the checksum the move was planned against — which proves it merely indexes the moved bytes (a racing event index of the new path, a safe dedupe). Any other replacement drops that move from the batch with a warning; nothing is deleted or re-pointed, and the next scan reconciles the stale source path as deleted-or-present. Dropped source paths are reported on the batch result and move run. The unconditional-replace branch stays the default because the watcher flow legitimately moves onto an existing indexed file (mv over an indexed path) where the pre-existing destination row must be replaced; cloud scan runtimes also keep current behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…ommits apply_project_index_move_batch called the move content updater inside the open db.scoped_session, and the local updater wrote the moved file's permalink frontmatter to disk via update_frontmatter_with_result before the transaction committed. If the batch rolled back (e.g. an intra-batch permalink collision), the database reverted but the on-disk rewrites persisted, leaving files ahead of their indexed state. The ProjectIndexMoveContentUpdater capability is now split into plan_moved_file_content — which runs inside the transaction, resolves the permalink, and builds the updated markdown in memory without touching storage — and write_moved_file_content, which the store calls only after a successful commit. The database rows are stamped from the planned content, whose checksum matches the exact bytes the post-commit write persists, so DB and file agree when the write lands; a failed write (or formatter or platform newline divergence) is logged and surfaces on the next scan as a checksum-mismatch modified file, which re-indexes from disk. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…ails The trailing rerun sat after the try/finally, so an exception from index_project skipped it and the request that coalesced behind the failed run was silently dropped — exactly when it most needed a retry. Reschedule inside finally, after the pending marker clears. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
5073c83 to
c9980a6
Compare
The refreshed ty dev dependency enforces mutability on protocol members: a protocol that declares plain attributes (id: int, embeddings: bool, ...) now rejects frozen-dataclass implementers, because their attributes are read-only. The indexing test fakes were frozen dataclasses standing in for such protocols (OrphanIndexedEntity, IndexedNoteContentEntity, EmbeddingIndexBatchSummary, VectorSyncBatchSummary, ProjectIndexRequestSource, ProjectDeleteAcceptedProjectSource, UnresolvedRelation, ResolvedRelationTarget), which produced 15 invalid-argument-type diagnostics. Drop frozen=True from those fakes so they are structurally assignable to the protocols they implement, matching the writable shape of the real implementations (SQLAlchemy models and queue payloads). No test relies on the fakes being frozen or hashable, so runtime behavior is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53863eb833
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| parts = self.key.split("/", 1) | ||
| return parts[0] if len(parts) == 2 else "" |
There was a problem hiding this comment.
When a project storage prefix contains a slash, this parser keeps only the first segment as the project prefix. For example, storage_object_key_from_project_path("/app/data/team-a/research", "notes/a.md") produces team-a/research/notes/a.md, but this code routes the event as project team-a with relative path research/notes/a.md; the repository resolver never sees the intended team-a/research prefix, so storage events for nested cloud/workspace prefixes are skipped or can route to the wrong project. Preserve the full configured project prefix before deriving the relative path.
Useful? React with 👍 / 👎.
Path.exists() folds PermissionError into False, so an unreadable file counted as confirmed-absent and the planned index delete proceeded. The verifier now stats the path directly: FileNotFoundError (or a vanished parent) confirms absence, any other OSError skips the delete for the next scan to reconcile. Also records the same-path-move fail-fast decision at the rejection site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96533ef0f9
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| entity = entities_by_id.get(entity_id) | ||
| if entity is None: | ||
| continue | ||
| await self.entity_indexer.index_entity(entity) |
There was a problem hiding this comment.
When this refresher is invoked with relation_cleanup_entity_ids after a watcher/project-index delete removes a note that other notes link to, it rebuilds only the surviving source's FTS rows. Semantic chunks are stored separately and are only pruned by sync_entity_vectors_batch, so the old relation chunks for that surviving source remain in search_vector_chunks/search_vector_embeddings and semantic search can keep returning relations to the deleted note until a later full vector reindex. Include those cleanup ids in a vector sync path when semantic indexing is enabled.
Useful? React with 👍 / 👎.
DELETE /v2/projects/{id}/knowledge/entities/{id} used to return 500 for
note_type="file" entities (issue #1033) because the delete path did
markdown-specific cleanup. The #1002/#1054 refactor fixed this by routing
single-entity delete through the shared accepted-note delete path, but no
test covered the non-markdown case.
Add a regression test that indexes a real .csv file through the local
project indexer (note_type="file", no permalink, no note_content row),
deletes it via the v2 endpoint, and asserts the entity row and its search
index rows are gone.
Refs #1033
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
1 participant
Why
This PR started as the explicit-session repository refactor and grew into the core half of the local/cloud runtime parity work. The goal is for
basic-memoryto own portable orchestration while cloud and local runtimes provide their own dependency adapters.Paired cloud PR: basicmachines-co/basic-memory-cloud#1345.
Cloud parity document: basic-memory-cloud/docs/local-cloud-runtime-parity.md.
Closes #750.
What Changed
basic_memory.runtimefor storage events, note-content payloads, job identity/status, workflow metadata, materialization, and project/file indexing values.basic_memory.cloudfacades for note-content reads/writes, directory deletes, project deletes, and materialization so cloud/local APIs can share route-facing orchestration.basic_memory.indexas the event-based indexing orchestration layer for storage events, local filesystem watching, project fanout, and inline local processing.basic_memory.indexingwith provider-neutral runners for accepted-note mutation/write flows, file indexing, project indexing, directory deletes, relation/vector follow-ups, and progress/result accounting.basic_memory.synccode and tests out of the active package/test tree so it is reference material only while the new index path becomes the clear file-indexing path.Public Surface
Verification
Not rerun during PR publication. The branch has been validated incrementally during the refactor with focused core tests,
just fast-check,just package-check, and paired cloud checks. Final broad core and cloud checks should run before merge.Review Notes
This PR should be reviewed with the paired cloud PR because the ownership split is intentional: core owns portable orchestration; cloud owns hosted adapters and tenant/runtime wiring.