fix(mcp): phantom child rows in cache — filter tombstones + reconcile on parent sync#825
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes “phantom” (stale/duplicated) cached child rows for several entities by (1) filtering soft-deleted rows out of list responses and (2) reconciling cached children against the authoritative nested row list returned by the parent endpoint during sync.
Changes:
- Add
deleted_at IS NULLfiltering to both eager-loaded row reads (selectinload) and correlatedrow_countsubqueries for SO/PO/Stock Transfers. - Introduce
EntitySpec.reconcile_childrenand implement per-parent child reconciliation deletes during sync for SO/PO/Stock Adjustments/Stock Transfers. - Add regression + end-to-end tests validating both the row filtering and reconciliation behavior.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Bumps katana-mcp-server version (lockfile update). |
katana_mcp_server/src/katana_mcp/tools/foundation/sales_orders.py |
Filters soft-deleted SO rows in both include_rows and row_count paths. |
katana_mcp_server/src/katana_mcp/tools/foundation/purchase_orders.py |
Filters soft-deleted PO rows in both include_rows and row_count paths. |
katana_mcp_server/src/katana_mcp/tools/foundation/stock_transfers.py |
Filters soft-deleted transfer rows in both include_rows and row_count paths. |
katana_mcp_server/src/katana_mcp/typed_cache/sync.py |
Adds reconcile_children to EntitySpec and performs per-parent child reconciliation deletes after upserts. |
katana_mcp_server/tests/tools/test_sales_orders.py |
Adds tests pinning SO row filtering behavior in both query paths. |
katana_mcp_server/tests/tools/test_purchase_orders.py |
Adds tests pinning PO row filtering behavior in both query paths. |
katana_mcp_server/tests/tools/test_stock_transfers.py |
Adds test ensuring stock transfer soft-deleted rows are excluded. |
katana_mcp_server/tests/test_typed_cache_invalidation.py |
Adds reconciliation-focused tests + an end-to-end phantom-row regression test. |
dougborg
force-pushed
the
fix/803-phantom-child-rows
branch
from
3d0cf25 to
57f82f0
Compare
#803) The cache-backed list paths for sales orders, purchase orders, and stock transfers eager-load child rows via ``selectinload`` (or count them via a correlated subquery when ``include_rows=False``). Both shapes lacked the ``deleted_at IS NULL`` predicate, so once the row-sync correctly tombstoned a row, ``list_*`` still surfaced it as a phantom — diverging from ``get_<entity>``, which reads live from Katana and is correct. Apply the predicate inside the loader option for the eager-load case (via ``relationship.and_(...)``) and in the ``where`` clause of the count subquery. Stock-adjustment row reads aren't affected here: ``CachedStockAdjustmentRow`` doesn't carry ``deleted_at`` — its sync spec hard-deletes via the reconciliation pass landing in a follow-up commit. Regression coverage seeds a mixed cache (one live row + one tombstoned row) and asserts both the ``include_rows=True`` payload and the ``row_count`` summary drop the tombstone.
dougborg
force-pushed
the
fix/803-phantom-child-rows
branch
from
57f82f0 to
6f9209c
Compare
dougborg
force-pushed
the
fix/803-phantom-child-rows
branch
from
6f9209c to
65936c8
Compare
The watermarked row-sync (``_<entity>_ROW_SPEC``) misses any row deleted upstream without a ``deleted_at`` propagation AND an ``updated_at`` bump. UI deletions, color-swap edits, and bad-import cleanup all fall in that gap — the row vanishes from ``find_<entity>`` *and* never returns from the dedicated ``/.../rows`` feed, so the cache holds it forever until a manual ``rebuild_cache``. This was the second root cause of the phantom- row repros on #WEB20523 / #WEB20561 / #WEB20518 / #WEB20484. Treat the parent payload's nested row list as authoritative. After the bulk-upsert of parents and children lands, fire one ``DELETE FROM <child> WHERE fk = parent_id AND id NOT IN (seen_ids)`` per parent in the batch. Seeding ``seen_by_parent`` from ``cached_parents`` (not ``cached_children``) is load-bearing: a parent in the response with zero children still wipes the cache for that parent, which the cached-children-only walk would silently skip. ``synchronize_session=False`` keeps aiosqlite happy — the ORM-aware sync strategy holds a SELECT cursor open through commit, which the single-cursor backend refuses. New ``EntitySpec.reconcile_children`` opts in. Enabled on SO, PO, stock_adjustment, and stock_transfer — every spec whose parent endpoint returns the full current row set. Left ``False`` on manufacturing orders (no inline rows; recipe rows live behind their own watermark). ``__post_init__`` validates the flag requires the child-cls triple, so misconfiguration surfaces at import time. Coverage: - ``test_reconcile_deletes_rows_missing_from_parent_response`` - ``test_reconcile_handles_empty_rows_array`` (the empty-rows seed case) - ``test_reconcile_does_not_touch_unrelated_parents`` (scoped WHERE) - ``test_reconcile_disabled_leaves_extra_rows`` (default-off contract, via the MO spec) - ``test_reconcile_post_init_requires_children`` - ``test_phantom_rows_cleared_on_next_list_sales_orders`` (end-to-end through ``_list_sales_orders_impl``)
Catch up the lock with pyproject.toml — v0.93.1 release commit (288b1ec) only updated pyproject.toml, leaving uv.lock pinned at v0.93.0.
dougborg
force-pushed
the
fix/803-phantom-child-rows
branch
from
65936c8 to
dbc670c
Compare
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
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.
Closes #803
Summary
list_sales_orders(include_rows=true)andlist_purchase_orderswere returning line items — sometimes duplicated — that didn't exist on the order in Katana, whileget_<entity>(which bypasses the cache) showed the truth. Repros on 4 sales orders (#WEB20523, #WEB20561, #WEB20518, #WEB20484) all traced to a row deleted in the Katana UI that never propagated to the cache.Two independent root causes, two layered fixes shipped together as the issue prescribes.
Fix A — filter
deleted_at IS NULLon row readsThe eager-load and count-subquery paths in
_list_<entity>_impllacked the predicate, so tombstoned children leaked through even after the row sync had correctly tombstoned them. Applied in 3 files:sales_orders.py(selectinload + row_count_subq)purchase_orders.py(same pair)stock_transfers.py(same pair)Stock-adjustment row reads aren't affected here —
CachedStockAdjustmentRowdoesn't carrydeleted_at, so Fix B alone handles its dropped rows via hard-delete reconciliation.Fix B — reconcile children on parent sync
The watermarked row sync (
_SALES_ORDER_ROW_SPECand friends) misses any row deleted upstream without both adeleted_atpropagation and anupdated_atbump. UI deletions, color-swap edits, and bad-import cleanup all fall in that gap — the row vanishes fromfind_<entity>and never returns from the/.../rowsfeed, so the cache holds it forever until manualrebuild_cache.New
EntitySpec.reconcile_childrenopts in. After the parent batch's bulk upsert lands, the sync fires oneDELETE FROM <child> WHERE fk = parent_id AND id NOT IN (seen_ids)per parent — treating the parent's nested row list as authoritative.Enabled on the 4 specs whose parent payload returns the full current row set: SO, PO, stock_adjustment, stock_transfer. Left
Falseon manufacturing orders (recipe rows live behind a separate watermarked endpoint).Key invariants pinned in tests:
seen_by_parentfromcached_parents(notcached_children) so empty-rows responses still wipe the cache for that parent.synchronize_session=Falsebecause aiosqlite refuses to commit with the ORM-aware SELECT cursor still open.WHERE fk = parent_idper parent keeps the DELETE scoped — unrelated parents are untouched.__post_init__validates the flag requires the child-cls triple.Test plan
uv run poe checkpasses (3553 unit/integration + 21 browser tests, lint, typecheck, format)include_rows=Truepayload androw_countexclude soft-deleted rowstest_phantom_rows_cleared_on_next_list_sales_ordersexercises both fixes through_list_sales_orders_impl(the actual user-facing surface)Out of scope (filed if needed)
Per the issue's follow-ups:
_<entity>_ROW_SPECpolling now that Fix B covers the gap — verify in a week of real use first.include_deletedinto Fix A's row filter so opt-in callers see deleted rows too.verify_order_documentand other non-list consumers ofselectinload(<rows>)for the same filter.🤖 Generated with Claude Code