Remove read-path cache writes for query-backed models (closes #74)#79
Conversation
Cache fields (columns, backing_query_sql, data_source) on a query-backed SlayerModel are no longer refreshed as a side effect of engine.execute, inspect_model, or get_column_types — only engine.save_model and create_model_from_query(save=True) populate them. Eliminates the lost-update race where a parallel edit_model write was clobbered by the read path's full-model rewrite. Trade-off: a backing-query rewrite that bypasses engine.save_model won't be reflected in cached fields until the next save through the engine. Documented in docs/concepts/models.md. - Delete _refresh_cache_after_resolution and all three call sites (one in _execute_by_name, two in _expand_query_backed_model). - Drop the now-vestigial refresh_cache parameter and the canonical-second-render branch. - Invert TestCacheRefreshOnExecute to assert read paths never write, with cases for populated/empty/stale cache and outer-query variables. - Update TestInspectModelQueryBacked._setup to route through engine.save_model so the cache is warmed at save time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR eliminates cache-refresh logic from read operations in the query engine. Query-backed model caches ( ChangesCache Refresh Removal from Read Paths
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related Issues
Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 4 minutes and 59 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/concepts/models.md (1)
327-334:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument
data_sourceas part of the persisted cache.This section still calls out only
columnsandbacking_query_sql, butengine.save_model()also refreshes the storeddata_sourcefrom the resolved virtual model. That field is part of the same engine-managed cache contract now, and downstream read paths likeget_column_types()rely on it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/concepts/models.md` around lines 327 - 334, The docs omit that engine.save_model also persists model.data_source into the engine-managed cache; update the prose under "What gets cached" to list `model.data_source` alongside `model.columns` and `model.backing_query_sql`, and state that `engine.save_model` (and REST MCP create/edit) refreshes `model.data_source` from the resolved virtual model so read paths like `get_column_types`, `inspect_model`, and REST `GET /models/{name}` can rely on it; keep the note that read operations (e.g., `engine.execute`, `inspect_model`, `get_column_types`, MCP `query`, REST `/query`) do not write to storage and that direct writes outside `engine.save_model` leave `model.columns`, `model.backing_query_sql`, and now `model.data_source` stale until the next engine save.
🧹 Nitpick comments (1)
tests/test_mcp_server.py (1)
327-331: ⚡ Quick winKeep one MCP-level cache-warming regression test.
This helper now primes the cache by calling
SlayerQueryEngine.save_model(...)directly, so it no longer exercises the MCP write path itself. I'd add a tool-levelcreate_model(query=...)oredit_model(source_queries=...)assertion that the persisted model hasbacking_query_sql, so a regression inslayer/mcp/server.pycan't slip past while this test still stays green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_server.py` around lines 327 - 331, The test currently primes the cache by calling SlayerQueryEngine.save_model directly (SlayerQueryEngine.save_model and SlayerModel), so it no longer verifies the MCP write path in slayer/mcp/server.py; update the test to also exercise the MCP-level create/edit methods by invoking the MCP tool API (e.g., call create_model(query=...) or edit_model(source_queries=...)) and then retrieve the persisted model and assert that its backing_query_sql field is present and populated; this ensures a regression in the MCP handlers (create_model/edit_model in slayer/mcp/server.py) will be caught while keeping the existing cache-warming behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/concepts/models.md`:
- Around line 327-334: The docs omit that engine.save_model also persists
model.data_source into the engine-managed cache; update the prose under "What
gets cached" to list `model.data_source` alongside `model.columns` and
`model.backing_query_sql`, and state that `engine.save_model` (and REST MCP
create/edit) refreshes `model.data_source` from the resolved virtual model so
read paths like `get_column_types`, `inspect_model`, and REST `GET
/models/{name}` can rely on it; keep the note that read operations (e.g.,
`engine.execute`, `inspect_model`, `get_column_types`, MCP `query`, REST
`/query`) do not write to storage and that direct writes outside
`engine.save_model` leave `model.columns`, `model.backing_query_sql`, and now
`model.data_source` stale until the next engine save.
---
Nitpick comments:
In `@tests/test_mcp_server.py`:
- Around line 327-331: The test currently primes the cache by calling
SlayerQueryEngine.save_model directly (SlayerQueryEngine.save_model and
SlayerModel), so it no longer verifies the MCP write path in
slayer/mcp/server.py; update the test to also exercise the MCP-level create/edit
methods by invoking the MCP tool API (e.g., call create_model(query=...) or
edit_model(source_queries=...)) and then retrieve the persisted model and assert
that its backing_query_sql field is present and populated; this ensures a
regression in the MCP handlers (create_model/edit_model in slayer/mcp/server.py)
will be caught while keeping the existing cache-warming behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 079b8351-5170-4d12-9b03-c9d8108ca4a4
📒 Files selected for processing (4)
docs/concepts/models.mdslayer/engine/query_engine.pytests/test_mcp_server.pytests/test_query_backed_models.py
Cover the MCP write paths (`create_model(query=...)` and `edit_model(source_queries=...)`) end-to-end: assert the persisted model has `backing_query_sql` populated. Read paths no longer warm the cache after #74, so a regression in slayer/mcp/server.py that bypassed `engine.save_model` would otherwise silently leave the cache empty without any test catching it. Addresses CodeRabbit nitpick on PR #79. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
1 participant
Summary
engine.execute,inspect_model,get_column_types, MCPquery, and REST/queryfrom writing to storage as a side effect — eliminates the lost-update race in Lost-update race in _refresh_cache_after_resolution (read-path side effect) #74 where a paralleledit_modelwas clobbered by the read path's full-model rewrite.columns,backing_query_sql,data_source) on a query-backedSlayerModelare now populated only byengine.save_model/create_model_from_query(save=True)(and the REST/MCP/CLI write paths that route through them)._refresh_cache_after_resolutionand the now-vestigialrefresh_cacheparameter + canonical-second-render branch on_expand_query_backed_model.Trade-off (explicit): a backing-query rewrite that bypasses
engine.save_model(e.g., rawstorage.save_model) won't be reflected in cached fields until the next save through the engine. Documented indocs/concepts/models.md. Full CAS /patch_modelis the layer-2 path from the issue and is deferred.Test plan
poetry run pytest— 1382 passed, 0 failedpoetry run ruff check slayer/ tests/— cleanTestCacheRefreshOnExecuteto assert read paths never callstorage.save_model, with cases for populated, empty, and stale cache plus outer-query variables.TestInspectModelQueryBacked._setupto route throughengine.save_modelso the cache is warmed at save time (was relying on the removed read-path side effect).🤖 Generated with Claude Code
Summary by CodeRabbit