Skip to content

fix(#56): hide documents via query_enabled instead of Marqo purge - #145

Open
nexus69420 wants to merge 6 commits into
OpenAgriNet:mainfrom
nexus69420:fix/issue-56-query-enabled-soft-disable
Open

nexus69420 wants to merge 6 commits into
OpenAgriNet:mainfrom
nexus69420:fix/issue-56-query-enabled-soft-disable

Conversation

@nexus69420

Copy link
Copy Markdown
Collaborator

Summary

Test plan

  • pytest tests/test_document_lifecycle.py tests/test_marqo_doc_scope.py tests/test_search_service.py tests/test_vector_store.py tests/test_tenant_isolation.py tests/test_db.py tests/test_activities.py (182 passed locally; the remaining failure is untracked scripts/_h100_reconcile_v2_marqo.py leaking a Marqo client, not this PR)
  • Include off: card stays, search misses the doc, Marqo records remain with query_enabled:false
  • Include on: search returns again without reingest
  • Delete: card hidden, same search hide as Include off; Restore unhides card only; Include on restores search
  • Chunk Include flips one record; chunk Delete still hard-deletes that Marqo/SQLite row
  • Re-chunk carries is_excluded by unchanged original_text; reingest uses include_excluded=True
  • 502 on a recorded index blocks the SQLite flip; historical indexes stay indexed (not removed)

…o purge

Include off and Delete flip the search flag so operators can restore without reingest.
@nexus69420
nexus69420 requested a review from KDwevedi August 30, 2026 01:26

@KDwevedi KDwevedi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address these blockers before merge:

  1. MarqoStore.set_query_enabled() discards the response from update_documents() and always reports every requested ID as updated. Marqo can return a successful HTTP response with errors: true and per-item failures; in that case the route proceeds to flip SQLite even though search visibility did not change. Please inspect the response, count only confirmed successes, and surface any failed items so the SQLite mutation is not performed.

  2. apply_document_query_enabled() mutates recorded indexes sequentially. If an early index succeeds and a later index fails, the helper raises before SQLite changes, but the earlier index remains flipped. This is especially unsafe on re-enable because a historical index can become searchable while the document remains disabled in SQLite. Please make failure handling restore already-mutated indexes (or otherwise preserve a consistent per-index/SQLite state) and add a regression where the later sorted index fails.

There is also an acceptance/scope mismatch to resolve explicitly: this PR says it closes #56 while intentionally dropping the required 409 for indexes without query_enabled and leaving the duplicate REVIEW/ADMIN privilege surface unchanged. Please either implement those criteria or update #56 and get agreement on the revised contract. The coordinated index recreation and chat/voice query_enabled:true rollout should remain a merge/deploy prerequisite.

Count only confirmed item successes, restore earlier indexes when a later one fails, and 409 when the index schema has no query_enabled field. SQLite is not flipped unless every target index succeeds.
@nexus69420

Copy link
Copy Markdown
Collaborator Author

Addressed the review blockers.

  1. MarqoStore.set_query_enabled() now unwraps the update_documents payload. Only status-200 items count as updated; errors: true with per-item failures (or no items) is reported in failed and does not flip SQLite.
  2. apply_document_query_enabled() records indexes already flipped and restores them (opposite flag) if a later sorted index fails. Regression: test_later_index_failure_restores_earlier_index.
  3. 409 when a target index schema has no query_enabled (never falls back to delete). Partial flips in the same call are restored first.

Privilege: chunk is_excluded stays review (PATCH .../chunks/{n}). Document Include / Delete stay admin lifecycle, same as before this PR — not a second chunk-toggle route. Commented on #56 with that contract.

Chat/voice query_enabled:true filter and coordinated index recreate remain a merge/deploy prerequisite.

@KDwevedi KDwevedi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The substantive review blockers are addressed: per-item Marqo failures now fail closed, earlier indexes are compensated on a later failure, the missing-schema path returns 409, and the privilege contract is documented.

One branch-specific suite regression remains. The full suite now fails test_disable_document_hides_in_own_tenant_index_not_legacy: its fake tenant index does not advertise query_enabled, so the route correctly returns the new 409 instead of exercising the tenant-index isolation assertion. Please update that non-legacy fake/schema fixture to include the new filterable field while keeping the legacy fake without it, then rerun the full suite.

Current full result: 879 passed, 2 failed; the other failure is the existing taxonomy-console assertion that also fails on main.

… fake

Disable/include now 409s when the index schema has no query_enabled. The tenant-isolation fake only advertised instance, so hide-in-own-index never reached the isolation assertion. Legacy fakes still omit the field.
@nexus69420

Copy link
Copy Markdown
Collaborator Author

The isolation fake now advertises query_enabled on non-legacy indexes (alongside instance). Indexes in _LEGACY_INDEXES still omit both fields, so hide/include 409s there instead of deleting.

test_disable_document_hides_in_own_tenant_index_not_legacy passes again. Full local suite: isolation + lifecycle + marqo-doc-scope green; remaining failures are the taxonomy-console assertions that also fail on main, plus a local untracked ops script tripping the Marqo-boundary scan (not in this PR).

Also merged upstream/main (includes #140).

@nexus69420
nexus69420 requested a review from KDwevedi September 1, 2026 10:00
Non-legacy hide now goes through the same update_documents item payload as production. A tenant index without query_enabled 409s and does not delete or disable.

@KDwevedi KDwevedi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suite regression is fixed and the substantive behaviour holds up, but re-reviewing the whole branch surfaced one new blocker in the chunk-level path.

Verified fixed:

  • test_disable_document_hides_in_own_tenant_index_not_legacy passes again. The fixture change is honest: non-legacy fakes advertise query_enabled alongside instance, _LEGACY_INDEXES omit both, and the new test_disable_document_409s_when_tenant_index_lacks_query_enabled proves the legacy path 409s instead of falling back to delete.
  • Local full suite on 736e135: 909 passed, 1 failed — test_taxonomy_admin.py::test_console_offers_tenants_to_a_pure_platform_admin, which also fails on main.
  • Per-item Marqo failures fail closed, earlier indexes are compensated on a later failure, and SQLite is flipped only after the Marqo update succeeds.

One P1 remains (new, in update_chunk) — details in the inline comment.

Also worth planning for before #153 lands: this PR adds set_query_enabled() to the VectorStore surface, and QdrantStore in #153 does not implement it. set_document_chunks_query_enabled() calls store.set_query_enabled(...) and only catches VectorStoreError, so under VECTOR_STORE_BACKEND=qdrant disable/query-off would raise AttributeError and 500. If this merges first, #153 must add the method; if #153 merges first, add it here.

Comment thread pipeline/routers/content.py Outdated
and bool(data.is_excluded) != bool(old_chunk.get("is_excluded", False))
and doc.get("stage") == "completed"
):
indexes.apply_document_query_enabled(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] This re-enables search for a chunk of a soft-deleted or query-disabled document. The flip is gated only on is_excluded changing and stage == "completed" — it never checks the parent document's is_disabled / query_enabled. Since record-level query_enabled:true is the only visibility gate (pipeline/services/search.py:339), un-excluding one chunk makes it retrievable again while the document is disabled.

Reproduced on this branch: disable_document(wf, remove_from_search=True) (row ends is_disabled=1, query_enabled=0), then PATCH /documents/{wf}/chunks/1 {"is_excluded": false} calls set_document_chunks_query_enabled(index='t-tenant-a-vet', enabled=True, chunk_num=1). Before this PR the records were deleted from Marqo on disable, so un-excluding a chunk could only mark reindex required — it could not resurrect search visibility.

Please only flip to enabled when the parent document is live and query-enabled (update SQLite only otherwise, or 409), and add a regression covering un-exclude on a disabled document and on a query_enabled=false document.

nexus69420 added a commit that referenced this pull request Sep 18, 2026
Pin the ingest tensor-refuse assertion, page filter-only scroll by offset, report missing_core from live payload schema, and audit every tenant collection before cutover. Also remap Indexes-summary live stats and add QdrantStore.set_query_enabled so a later #145 merge does not 500.
…en document

Chunk Include was flipping query_enabled=true whenever is_excluded cleared on a completed doc. After Delete or Include off that made a single chunk searchable while the parent stayed hidden. Skip the index write unless the parent is live and query-enabled; SQLite still updates.
@nexus69420
nexus69420 requested a review from KDwevedi September 18, 2026 11:39
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.

Chunk soft-disable, done properly: add query_enabled filter field before more indexes are created

2 participants