Skip to content

Avoid stale lastUpdate cache during prewarm - #85

Open
bxclib2 wants to merge 1 commit into
mercuretechnologies:mainfrom
bxclib2:fix-prewarm-known-update
Open

Avoid stale lastUpdate cache during prewarm#85
bxclib2 wants to merge 1 commit into
mercuretechnologies:mainfrom
bxclib2:fix-prewarm-known-update

Conversation

@bxclib2

@bxclib2 bxclib2 commented Jul 13, 2026

Copy link
Copy Markdown

Summary

Fixes #84.

This changes post-publish cache prewarming so MarkUpdateAsChecked warms the manifest cache for the update that was just checked, instead of resolving "latest" again through GetLatestUpdateBundlePathForRuntimeVersion.

Why

GetLatestUpdateBundlePathForRuntimeVersion reads and writes the lastUpdate cache. Calling it from the post-publish prewarm path can race with stale Redis state and re-cache the previous update after a new update has already been marked checked.

That leaves storage/dashboard showing the new update while /manifest continues to serve the old one until the Redis TTL expires or the key is deleted manually.

Changes

  • Add PreWarmUpdateManifestCache(update, platform) for warming metadata/manifest cache for a known update.
  • Use that helper from MarkUpdateAsChecked after writing .check.
  • Keep PreWarmManifestCache(branch, runtimeVersion, platform) for callers that genuinely want to resolve latest.
  • Add a regression test asserting known-update prewarm does not populate lastUpdate.

Test

go test ./...

Summary by CodeRabbit

  • Bug Fixes

    • Improved update cache prewarming to target the correct platform-specific manifest.
    • Ensured manifest-only prewarming does not incorrectly populate the latest-update cache.
    • Added more reliable error handling and logging during cache prewarming.
  • Tests

    • Expanded coverage for metadata, manifest, and cache population behavior.

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1072a2d6-7ab7-4e8a-8814-be4815e48e7f

📥 Commits

Reviewing files that changed from the base of the PR and between 8d95400 and f1fa079.

📒 Files selected for processing (3)
  • internal/update/prewarm.go
  • internal/update/updates.go
  • test/manifest_test.go

📝 Walkthrough

Walkthrough

PreWarmUpdateManifestCache warms metadata and manifest caches for a specific update and platform. MarkUpdateAsChecked now invokes it for the stored platform, and tests cover both full latest-update prewarming and manifest-only prewarming.

Changes

Manifest cache prewarming

Layer / File(s) Summary
Targeted manifest prewarming
internal/update/prewarm.go
Adds update-scoped metadata retrieval and manifest composition with error logging and panic recovery, and updates the existing prewarming documentation.
Checked-update integration and cache coverage
internal/update/updates.go, test/manifest_test.go
Checked updates prewarm only the stored platform; tests verify full cache population and that update-specific prewarming does not populate lastUpdate.

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

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: preventing stale lastUpdate cache warming during prewarm.
Linked Issues check ✅ Passed The changes directly address issue #84 by prewarming the specific checked update instead of resolving and caching the latest update again.
Out of Scope Changes check ✅ Passed The changes stay focused on cache prewarming and regression tests, with no obvious unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

Comment @coderabbitai help to get the list of available commands.

@axelmarciano axelmarciano moved this from Backlog to In progress in expo-open-ota Jul 17, 2026
@axelmarciano

Copy link
Copy Markdown
Collaborator

Hi @bxclib2, sorry for the delay, I was heads down on v3. Good analysis, and you found the right culprit.

The race: v2 deletes the lastUpdate cache key before uploading .check. In that window, a concurrent /manifest (or the prewarm itself) re-resolves latest, cannot see the new update yet, and re-caches the old one for the full TTL.

This is fixed in v3.0.0 by inverting the order: .check is written first, then the cache key is deleted, so any cache miss can only resolve to the new update.

I'm happy to ship this in a v2.3.23 too. Your PR helps (the prewarm stops touching lastUpdate), but alone it leaves the window above open for concurrent requests, so it needs the same reorder on top: delete the cache key after the .check upload.
If you retarget this PR to release/v2 I'll merge it and add the reorder, or I can cherry-pick it myself, as you prefer.

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.

Redis lastUpdate cache can serve stale manifest after successful publish

2 participants