[Eliot5566]

Description

Fixes #6209.

CachingStateSync.get_snapshots() computed expire_at before calling the underlying state sync. When that call took longer than the TTL, the snapshots it returned went into the cache already expired, so the next lookup fetched them again.

expire_at is now recomputed once the fetch returns and used for everything that fetch resolved, including the False placeholders for snapshots that do not exist. Cache hits keep the deadline they already had. The placeholders are still written before the fetch, so behaviour after a failed backend read is unchanged.

One thing for reviewers to decide: the issue asks whether expiry relative to fetch start was intentional. I assumed it was not, since it makes a slow cold load uncacheable.

Test Plan

Added test_cache_ttl_starts_after_slow_fetch. It uses a fake state sync that advances the clock by 11 s during the fetch with ttl=10, then checks that an immediate second lookup does not hit the backend and that a lookup after the TTL does. It fails on main (2 backend reads instead of 1).

tests/core/state_sync/test_state_sync.py and tests/core/test_context.py give the same results before and after the change in my environment, apart from the new test. 48 tests in those files fail or error for me on main as well because examples/custom_materializations is not installed locally, so I am relying on CI for the full suite.

Checklist

• I have run make style and fixed any issues
• I have added tests for my changes (if applicable)
• All existing tests pass (make fast-test)
• My commits are signed off (git commit -s) per the DCO

I ran ruff, ruff format and mypy on the changed files only, so the first box is unticked. I used Claude to help write and test this change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Hq1ACTsK85bRrY28fUQ7b3

Summary by CodeRabbit

• Bug Fixes
  • Cached snapshot results now remain available for the configured duration after fetching completes, even when the fetch takes longer than that duration.
  • Requests for snapshot IDs that are not returned by the fetch are also cached, reducing repeated lookups for missing snapshots.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

CachingStateSync.get_snapshots now starts cache expiration after the underlying fetch completes. It also caches requested snapshot IDs absent from the fetch result with that expiration. A regression test covers immediate reuse and expiration.

Changes: Snapshot cache expiration

Layer / File(s)	Summary
Fetch snapshots and set cache expiration sqlmesh/core/state_sync/cache.py, tests/core/state_sync/test_state_sync.py	get_snapshots sets expiration after the fetch for returned and absent IDs. The regression test verifies cache reuse after a slow fetch and a new fetch after the TTL.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: mday-io

Merge Risk: Minimal · up to d81ba

This change makes the snapshot cache start its expiry after a slow fetch finishes, so an immediate repeat lookup reuses the result. No merge-blocking risk was found.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title "fix: start the snapshot cache TTL after the fetch completes" matches the main change. CachingStateSync.get_snapshots() now computes expire_at after the underlying fetch returns, so the TTL …
Description check		The description follows the repository template. It has a Description section that explains the bug, the fix, and the reviewer question about intent. It has a Test Plan section that names the new test…
Linked Issues check		[#6209] The issue requires that a successfully fetched snapshot stay reusable for an immediate lookup when the fetch exceeds the TTL. The diff in sqlmesh/core/state_sync/cache.py recomputes expire_at …
Out of Scope Changes check		The whole-PR diff against the merge base touches sqlmesh/core/state_sync/cache.py (a six-line change in CachingStateSync.get_snapshots) and the regression test in tests/core/state_sync/test_state_sync…

Full details: Title check

Explanation

The title "fix: start the snapshot cache TTL after the fetch completes" matches the main change. CachingStateSync.get_snapshots() now computes expire_at after the underlying fetch returns, so the TTL starts at fetch completion. The title is concise, specific, and follows a conventional commit style.

Full details: Description check

Explanation

The description follows the repository template. It has a Description section that explains the bug, the fix, and the reviewer question about intent. It has a Test Plan section that names the new test, the failure on main, and the test files run. The Checklist is present. Some items are unticked: make style and make fast-test are not reported as run, and the author states that ruff and mypy ran only on changed files. The author explains these gaps. The description is mostly complete, so the check passes.

Full details: Linked Issues check

Explanation

[#6209] The issue requires that a successfully fetched snapshot stay reusable for an immediate lookup when the fetch exceeds the TTL. The diff in sqlmesh/core/state_sync/cache.py recomputes expire_at as now_timestamp() + self.ttl * 1000 after self.state_sync.get_snapshots(missing) returns. The fetched snapshots are then written with that deadline in the existing-entry loop. The fix keeps the required behaviors: cache hits keep their existing deadlines (the hit path continues before any write), placeholders for missing snapshots are written before the fetch, and a failed fetch raises before the post-fetch code, so retry behavior is unchanged. Seed hydration logic (the isinstance SeedModel and is_hydrated check) is unchanged. The new test test_cache_ttl_starts_after_slow_fetch, as described in the PR, covers the slow-fetch scenario. The issue's open question about whether pre-fetch expiry was intentional, and whether a supported workaround exists for repeated cold loads, is a question rather than a coding requirement. The PR states its assumption and does not implement a workaround. This does not block PASS. Test execution was not verified in this review; the verdict rests on the source diff and the PR's description of the test.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff against the merge base touches sqlmesh/core/state_sync/cache.py (a six-line change in CachingStateSync.get_snapshots) and the regression test in tests/core/state_sync/test_state_sync.py. The code change is limited to the post-fetch expiry and the negative-result placeholders for IDs missing from the fetch, which is the behavior the issue describes. No unrelated changes were found in the reviewed diff. The Context.audit() path mentioned in the issue was not changed, which is consistent with the stated scope.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 86bf1d23-6b39-4bdb-a64c-b0a8f1939d15

Commits

Reviewing files that changed from the base of the PR and between d943714 and d81bad9.

Files selected for processing: 2

• sqlmesh/core/state_sync/cache.py
• tests/core/state_sync/test_state_sync.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.