Skip to content

fix(knowledge): delete the document directory even before it is indexed - #456

Merged
gloryfromca merged 4 commits into
mainfrom
fix/knowledge-delete-before-index
Sep 24, 2026
Merged

gloryfromca merged 4 commits into
mainfrom
fix/knowledge-delete-before-index

Conversation

@gloryfromca

Copy link
Copy Markdown
Member

Summary

DELETE /api/v2/knowledge/documents/{doc_id} resolved the document directory through its SQLite row. The cascade writes that row a second or more after POST /documents has returned, so a delete that followed a create closely (an undo, a client retry) found no row, removed nothing, answered 204 — and the markdown left on disk was indexed right back in: the "deleted" document reappeared in GET /documents a few seconds later.

Markdown is the source of truth, so when the index has no row the directory is located on disk instead: its name ends in the doc_id (knowledge_writer.py: dir_name = f"{title_slug}_{root.doc_id}"), so a */*_<doc_id> glob under the scope's knowledge dir finds it. Response semantics are unchanged (204 when no indexed topics were removed, as documented).

Area

  • architecture / correctness (knowledge subsystem)

Verification

  • Reproduced on a live Tier-3 server before the fix: POST /documents (201) → DELETE 3 ms later (204) → GET /documents lists the document again, GET /documents/{id} → 200, directory still on disk (.work_context/agent_sim/logs/trace.jsonl, local).
  • New unit test test_delete_document_removes_dir_before_the_index_has_the_row (real tmp root, repo mocked to return no row) — red without the disk lookup (assert not doc_dir.exists() fails), green with it. The existing idempotent test now pins EVEROS_ROOT to a tmp dir so it never touches ~/.everos.
  • tests/unit/test_service/test_knowledge_crud.py + test_knowledge_api.py: 57 passed. make lint green.

Checklist

  • tests added
  • lint green
  • no API contract change

Notes for Reviewers

_find_doc_dir runs in a worker thread (anyio.to_thread.run_sync), same as the existing rmtree. The glob is bounded to <knowledge_dir>/<category>/ — two levels, no recursion.

🤖 Generated with Claude Code

`delete_document` located the directory through the SQLite row and returned
`deleted_topics=0` when the row was missing. The row is written by the
cascade a second or more after `POST /documents` returns, so a delete that
follows a create closely (an undo, a client retry) found no row, touched
nothing, and answered 204 — while the markdown stayed on disk and the
cascade indexed the "deleted" document right back in. Reproduced against a
live server: create → delete 3 ms later → the document listed and readable
again a few seconds on.

Markdown is the source of truth, so the disk is consulted when the index has
no row: the directory name ends in the doc_id (`knowledge_writer`), and a
`*/*_<doc_id>` glob under the scope's knowledge dir finds it. The response
stays 204 in that case (no indexed topics were removed), as documented.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@gloryfromca

Copy link
Copy Markdown
Member Author

Live before/after on the same repro (create → DELETE 3 ms later → wait 15 s → look):

DELETE GET /documents/{id} after 15 s listed directory on disk
before (main worktree, run 2 of the agent-sim driver) 204 200 yes yes — resurrected
after (this branch, fresh root, port 8124) 204 404 no no — GONE

Script: .work_context/agent_sim/repro_delete_before_index.py (local).

gloryfromca pushed a commit that referenced this pull request Sep 24, 2026
PR #456 changes what a delete-before-index reports; the wording added
here would be wrong once it lands, so keep the sentence as on main.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@gloryfromca

Copy link
Copy Markdown
Member Author

Adversarial review round (subagent, read-only) found two should-fix items; both addressed in the follow-up commit:

  1. doc_id reached the glob unvalidated. delete_document is exported from everos.service; only the HTTP route validates the id's shape, and delete_document("*", …) removed the first document of the first category. The directory name is now compared literally (name.endswith(f"_{doc_id}")), every match is removed.
  2. 204 for a real deletion. The no-row branch reported a hard-coded deleted_topics=0, so a delete that beat the index answered 204 — indistinguishable from "never existed" and at odds with docs/knowledge.md. It now counts the topic files on disk (N_*.md, markdown is the truth) and answers 200 with that count; 204 remains for "nothing existed".

Test hardening: the fixture plants a sibling document and .taxonomy.md and asserts they survive (an rmtree(knowledge_dir) implementation passed the old test, fails this one), plus a parametrised case for an absent id, * and */_original. 60 tests green, lint green.

Adversarial review of the previous commit found two gaps. The doc_id went
into a glob pattern unvalidated — `delete_document` is a public service
function and only the HTTP route checks the id's shape, so `"*"` removed
the first document of the first category. The directory name is now
compared literally with `endswith`, every matching directory is removed,
and the response reports the topic files that were on disk instead of a
hard-coded 0, so a delete that beats the index answers 200 with the real
count as the docs describe (204 stays for "nothing existed").

The test now plants a sibling document and the taxonomy file and asserts
they survive — an `rmtree(knowledge_dir)` implementation passed the old
test and fails this one — and a parametrised case checks that an absent
id, `*` and `*/_original` remove nothing.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@gloryfromca
gloryfromca force-pushed the fix/knowledge-delete-before-index branch from 9a97816 to 825d83c Compare September 24, 2026 05:56
gloryfromca pushed a commit that referenced this pull request Sep 24, 2026
Adversarial review of the eventual-consistency paragraph against everalgo:
the gate that actually decides under EverOS defaults is
`min_tool_call_rounds=3` — no case below three tool-call rounds, which also
makes the previously listed "assistant turn under ~200 tokens" gate
unreachable — and the EverOS `agent_case_skipped_by_algo` event carries ids
only; the reason is on everalgo's own log line. Also: `quiesce` is safe to
call twice rather than "idempotent" (it reports `quiesced=True` both times),
knowledge GET reads SQLite rather than "the index", the ~1 s figure is
hedged like the others, the PATCH sentence names why it escapes the lag,
and the delete status-code sentence is left as on main because PR #456
changes what a delete-before-index reports.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@arelchan
arelchan self-requested a review September 24, 2026 08:45
@gloryfromca
gloryfromca enabled auto-merge (squash) September 24, 2026 08:54
@gloryfromca
gloryfromca merged commit 401fbf8 into main Sep 24, 2026
10 checks passed
@gloryfromca
gloryfromca deleted the fix/knowledge-delete-before-index branch September 24, 2026 08:55
This was referenced Sep 24, 2026
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.

2 participants