fix(core): refresh linking notes' search rows on API note delete - #1368
Open
phernandez wants to merge 2 commits into
Open
fix(core): refresh linking notes' search rows on API note delete#1368phernandez wants to merge 2 commits into
phernandez wants to merge 2 commits into
Conversation
The API/MCP single-note delete (DELETE /v2/.../entities/{id} ->
run_accepted_note_delete -> delete_accepted_note) removed the deleted
note's own search_index rows and vectors but never re-indexed the
surviving notes that linked to it. Their relation search rows kept the
deleted entity's to_id and 'Alpha -> Beta' title, so
search_notes(..., entity_types=["relation"]) kept returning a permalink
to a note that no longer exists — derived-state drift with no repair
mechanism on this path, since nothing schedules a re-index of the
linking note. Every other delete path (watcher/external file delete,
directory delete, project scan) already snapshots surviving relation
sources before the delete and refreshes them afterwards.
Port that same sequence here:
- delete_accepted_note captures relation_cleanup_sources_for_deleted_entity
inside the delete transaction, after the NoteContent lock and before
session.delete(entity) — the SET NULL cascade would otherwise erase the
to_id evidence identifying which sources need repair. No new locking.
- RuntimeAcceptedNoteChange carries the ids as an additive
relation_cleanup_entity_ids field (default empty), so cloud and every
non-delete caller are untouched.
- LocalNoteContentMaterializationProvider.materialize_delete_change
re-indexes those sources post-commit through an optional
relation_cleanup_refresher (RepositoryProjectIndexMovedEntitySearchRefresher,
the same refresher the watcher path uses), wired in deps/services.py.
A refresh failure is logged and non-fatal: the delete already
committed, and the stale rows converge on the source's next edit or a
reindex. Cloud injects its own provider, leaves the field unset, and
keeps relying on its orphan sweeper.
Fixes #1351
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017STCpbNsYjZgUdftxgEAZ4
Signed-off-by: phernandez <paul@basicmachines.co>
Member
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 14a1c30dc3
ℹ️ 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".
Codex review on #1351: deleting a hub note with thousands of inbound links awaited the surviving-source reindex inline, and each source rereads its markdown and rewrites its search rows, so delete latency scaled with inbound degree and could time out the HTTP/MCP response after the DB delete had already committed. Hand the refresh to the same bounded worker pool that defers materialization; keep it inline only under test_mode so tests can assert synchronously. The search rows are eventually-consistent derived state, so deferring is safe. Refs #1351 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017STCpbNsYjZgUdftxgEAZ4 Signed-off-by: phernandez <paul@basicmachines.co>
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! 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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1351.
The gap
The API/MCP single-note delete (
DELETE /v2/.../entities/{id}→delete_note→run_accepted_note_delete→delete_accepted_note) removes the deleted note's ownsearch_indexrows and vectors, thensession.delete(entity). It never refreshes thesearch_indexrows of surviving notes that linked to the deleted one. Since #1358 the relation TABLE row for a linker (Alpha → Beta) correctly goesto_id=NULLon Beta's delete, but Alpha's relation SEARCH row still carries the oldto_idand "Alpha -> Beta" title, sosearch_notes(entity_types=["relation"])returns a permalink to a deleted note and it never self-heals.Every other delete path already handles this (watcher/external, directory, project scan): capture surviving relation sources before the delete, refresh their search rows after. This brings the API path in line.
Change (matches the external-delete precedent)
delete_accepted_note(accepted_note_write_runner.py): after the existing NoteContent lock and beforesession.delete(entity), call the reusablerelation_cleanup_sources_for_deleted_entity(fromexternal_file_delete_runner.py, not reimplemented) and attach the ids viadataclasses.replace. Capture must precede the delete because the SET NULL cascade erases the identifyingto_ids. No new locking.RuntimeAcceptedNoteChangegains an additiverelation_cleanup_entity_ids: frozenset = frozenset().LocalNoteContentMaterializationProvidergains an optionalrelation_cleanup_refresher;materialize_delete_changecallsrefresh_moved_entities(sorted(ids))after the guarded file delete, mirroringlocal_runtime.py's external-delete path. Refresh failure is logged and non-fatal — stale search rows are derived state that converge on the source's next edit or a reindex.deps/services.pybuilds the refresher from the existing entity-repository and search-service deps (the sameRepositoryProjectIndexMovedEntitySearchRefresherthe watcher uses).Cloud parity: both new fields default empty/None; cloud injects its own provider via the factory and is untouched, keeping its orphan-sweeper backstop. The only shared-path cost is one pre-delete SELECT.
Tests
test_delete_entity_refreshes_linking_notes_search_rows(real ASGI flow, fix(core): unresolve inbound relations on entity delete #1358 fixtures): before delete Alpha's relation row resolves to Beta; afterDELETE Beta, Alpha's relation search row is rebuilt withto_idNULL and no "Beta" in the title, and zero search rows reference Beta's id. Verified red on unpatched main, green with the fix (I re-confirmed by disabling the refresh: the test fails; restored: passes).tests/api tests/index tests/indexing tests/services tests/cloud: 1422 passed.ruff/tyclean (the onlytydiagnostics are the pre-existingpymilvusoptional-extra imports). Postgres not run locally (no Docker); the SQL is backend-neutral. MCPdelete_noterides the same route, covered transitively.🤖 Generated with Claude Code
https://claude.ai/code/session_017STCpbNsYjZgUdftxgEAZ4