Skip to content

perf(sources): bound the receipt migration scan and detect shared plugin drift - #2148

Merged
hidb4ai merged 2 commits into
masterfrom
fix/2084-correctness-and-bounds
Oct 21, 2026
Merged

hidb4ai merged 2 commits into
masterfrom
fix/2084-correctness-and-bounds

Conversation

@AlexStocks

Copy link
Copy Markdown
Contributor

Which issue or RFC does this PR close?

Refs #2084. This is the second of two PRs for that risk collection. It fixes the two items with a
single correct answer, and records two items that the investigation found already resolved or
incorrect.

Fixed here:

  • R2: the receipt migration rescanned every visited row on each page.
  • R14 (shared sources): nothing detected drift between byte-identical host copies, and one had
    already diverged.

Investigated, not changed:

  • R10 (both bullets): not a defect. ArtifactProcessingLeaseRepository.load(..., for_update=True)
    already takes SQLite's write lock through a no-op UPDATE before reading the term, so both
    topic_memory_budget._lock and start_single_process_term validate the fence under the write
    lock. Details below.
  • R9 (N+1): already fixed on master by perf(search): simplify retrieval queries and lifecycle checks #2095. _load_related_items now authorizes and reads in
    pages of 100 through one read_many per page instead of one transaction and one get per hit.
  • R14 (Kimi idempotency): not changed. tests/plugin.test.mjs:112 asserts two submissions of the
    same prompt produce different source ids, so making the id content-addressed is a semantic
    decision about whether a repeated prompt is one memory or two events, not a defect fix.

Still open in the issue, with the blocking reason:

Item Why it is not in this PR
R1 The rebuild is now gated on a projection version (also #2095); what remains is the work still executed inside BEGIN IMMEDIATE. Removing it needs a startup-time measurement first, and the pc_artifacts(family) counts need an index decision.
R3 Shrinking the flush lock needs a benchmark proving the CAS path already covers the removed window.
R4 Merging owner establishment into the artifact transaction changes the authorization write path and needs a backfill for existing databases.
R5 (behavior) Not advancing the cursor on HOLD changes flush semantics for every caller.
R6 Needs a trust-tier design for external Sources.
R7 A body limit needs a configured maximum; the rate-limit and MCP-session parts need a policy.
R9 (limit/ANN) The missing candidate limit is documented as deliberate, and the OceanBase APPROXIMATE filter order needs a live tenant to confirm.
R11 Both serialization points are documented as intentional; the dirty-only scheduler needs a correct "has work" predicate.
R12 (remaining) Streaming the snapshot hash and resuming the rebuild are independent changes.
R13 (mutex) Startup mutual exclusion for OceanBase needs GET_LOCK semantics verified against a live tenant.
R14 (remaining) The WorkBuddy request path needs the same error taxonomy and exit-code mapping as the other hosts, which is its own change.

Rationale for this change

R2. migrate_handoff_receipts paged with (scope_id, source_id) > after while the primary key is
(scope_id, source_type, source_id). The cursor could not steer that index, so SQLite produced:

SEARCH pc_sources USING COVERING INDEX sqlite_autoindex_pc_sources_1 (scope_id>?)
USE TEMP B-TREE FOR LAST TERM OF ORDER BY

The temporary B-tree means every page materialized all remaining rows before LIMIT applied, so one
Scope with N content Sources cost O(N²/batch) index reads on every startup with access control
enabled. Adding the missing key column to the cursor is not enough — the equality on source_type
still forces the sort. Paging inside one Scope instead gives the first two key columns as equality
and source_id as a range:

page:       SEARCH pc_sources USING COVERING INDEX sqlite_autoindex_pc_sources_1 (scope_id=? AND source_type=? AND source_id>?)
next scope: SEARCH pc_sources USING COVERING INDEX sqlite_autoindex_pc_sources_1 (scope_id>?)

No schema change and no new index: the existing primary key is enough once the query matches it.

R14. Four hosts ship the same powercontext_client_config.py and three ship the same
hooks/prepared_context.py. Nothing compared them, and the WorkBuddy copy had already fallen behind
on bounded_recall_query and the MAX_QUERY_* bounds that Claude Code and Codex share. This PR
aligns that copy and adds a test that fails on any future divergence, with the drifted paths in the
message.

What changes are included in this PR?

  • receipt_migration.py: pagination is now per Scope (_next_content_scope, _page_content_sources),
    each page seeks the primary key. Classification, attestation, review-table handling, and the
    missing-row error are unchanged; batch_size still bounds one page.
  • integrations/workbuddy/.../hooks/prepared_context.py: same content as the Claude Code and Codex
    copies.
  • tests/test_integration_shared_files.py: new guard for both shared sources.

Are there any user-facing changes?

migrate_handoff_receipts returns the same counts and writes the same rows; only the query shape
changed. The WorkBuddy plugin now carries an unused-by-it helper in a shared module, so its behavior
is unchanged until its caller adopts it.

How was this change tested?

pytest tests/builtin/persistence/test_records.py -q     # 17 passed (includes the batching test at batch_size=1)
pytest tests/test_integration_shared_files.py -q        # 2 passed
ruff check / ruff format --check on touched Python files

The new guard was validated by reverting the WorkBuddy copy: the test failed with
prepared_context.py differs from integrations/claude-code/... in: integrations/workbuddy/..., then
passed after the realignment.

Query plans above were captured with EXPLAIN QUERY PLAN on a 5,000-row SQLite table using the
production primary key. They are reported here rather than asserted in a test, because the plan text
is SQLite-version-specific.

AI usage statement

Built with WorkBuddy (DeepSeek V4.1 Flash). The R10 and R9 conclusions above come from reading
supervision.py and atomic_memory_processing.py on master at 30e3f1b1, not from a test run —
please re-check the reasoning if you disagree with the for_update claim.

The migration cursor spanned Scopes, so SQLite could not steer the
(scope_id, source_type, source_id) primary key: every page used a temporary B-tree for
the ORDER BY and rescanned the rows already visited, costing O(N^2) index reads per
Scope at startup. Paging inside one Scope turns both statements into bounded seeks.
Four hosts ship the same client configuration and three ship the same
prepared-context helper, with nothing detecting divergence. The WorkBuddy copy had
already fallen behind on the bounded recall query, so this aligns it and adds a guard.
Copilot AI balanced review requested due to automatic review settings October 21, 2026 04:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

@PsiACE PsiACE left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@hidb4ai hidb4ai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@hidb4ai
hidb4ai merged commit ae9deb3 into master Oct 21, 2026
43 checks passed
@hidb4ai hidb4ai added the 1.4.1 v1.4.1 release tracking label Oct 21, 2026
@PsiACE
PsiACE deleted the fix/2084-correctness-and-bounds branch October 22, 2026 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1.4.1 v1.4.1 release tracking

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants