Skip to content

Fix storage cache stamps - #2505

Closed
Promansis wants to merge 5 commits into
Pasta-Devs:refactorfrom
Promansis:fix/storage-cache-stamps-2497-2498
Closed

Fix storage cache stamps#2505
Promansis wants to merge 5 commits into
Pasta-Devs:refactorfrom
Promansis:fix/storage-cache-stamps-2497-2498

Conversation

@Promansis

Copy link
Copy Markdown
Collaborator

Linked issue

Closes #2497
Closes #2498

Why this change

  • Storage reads by id and projected-list reads could reuse stale or overly broad cache state after disk-backed collection access. The fixes keep cached id lookups narrow and make projected-list cache validation use the intended metadata stamp while atomic write conflict checks keep content-aware protection.

What changed

  • Added a cached id-index path for disk reads so repeated get and projected get calls can reuse indexed record ranges without loading full pretty JSON collections.
  • Split storage cache stamps so projected list caches use length/mtime metadata, while atomic update conflict detection still uses a content signature.
  • Added focused Rust storage regression tests for id-index reuse, projected id lookup behavior, and projected-list cache stamp behavior.

Refactor impact

Primary owner:

Rust storage

Impact areas reviewed:

  • src-tauri/crates/storage/src/lib.rs
  • Disk-backed collection reads by id
  • Projected collection list cache invalidation
  • Atomic collection update conflict checks

Boundary notes:

  • Change stays inside the Rust storage capability crate.
  • No React, engine, shared API, Tauri command registration, or remote-runtime routing boundaries changed.

Pressure points touched:

  • None. No ModeSurface, GameSurface, shared mode UI, src-tauri/src/lib.rs command registration, or import modules touched.

Validation

  • Matching validation command passes locally (for example pnpm typecheck, pnpm build, pnpm check:architecture, pnpm check:docs, or full pnpm check when warranted)
  • Full pnpm check passes before PR push/handoff
  • Human/manual validation completed by contributor or reviewer

Manual verification notes

  • cargo test --manifest-path src-tauri/crates/storage/Cargo.toml repeated_get_uses_cached_id_index_after_disk_read passed.
  • cargo test --manifest-path src-tauri/crates/storage/Cargo.toml repeated_projected_get_uses_cached_id_index_after_disk_read passed.
  • cargo test --manifest-path src-tauri/crates/storage/Cargo.toml projected_get_id_index_avoids_caching_full_pretty_rows passed.
  • cargo test --manifest-path src-tauri/crates/storage/Cargo.toml projected_collection_stamp_uses_metadata_without_content_signature passed.
  • cargo test --manifest-path src-tauri/crates/storage/Cargo.toml list_projected_cache_detects_same_length_rewrite_when_mtime_changes passed.
  • pnpm check passed.
  • Durable test rationale: these are storage cache regressions that can silently return stale or overly broad data; existing command-level proof is not enough to guard future cache changes; the tests are narrow storage-crate unit tests around the affected cache paths.

Feature Discoverability

Check exactly one:

  • Updated src/features/shell/discovery/ because this PR adds or materially changes a user-discoverable feature, workflow, setting, mode, panel, import path, agent, media capability, or advanced tool.
  • N/A because this PR is only a bugfix, refactor, test, docs, internal wiring, visual polish, copy edit, or compatibility fix and does not add a new thing users need to find.

Reason:

  • Storage cache bugfix only; no new user-discoverable feature, setting, mode, panel, import path, agent, media capability, or advanced tool.

Docs and release impact

  • No docs changes needed
  • Updated README.md
  • Updated CONTRIBUTING.md
  • Updated docs/developer/
  • Updated repo skills or AGENTS.md
  • Confirmed this PR does not restore old staging/package-workspace/release claims

UI evidence

N/A. Rust storage-only change; no visible UI behavior changed.

@github-actions github-actions Bot added the bugfix Bug fix label Jun 6, 2026
@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

✅ Bunny Review Completed

Tip

Review posted. The specimen has left the observation table.

  • Commit: d0a0897 - Retry unstable storage id index builds

@Pasta-Devs Pasta-Devs deleted a comment from coderabbitai Bot Jun 6, 2026
@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🐰 Bunny Review

Bunny Merge Signal: Ready With Notes

Warning

READY WITH NOTES
No actionable defects were isolated, but non-blocking notes remain.

Findings Nitpicks Controls Reviewed Head Mode
0 0 2 warn, 1 unknown, 2 pass d0a08977 incremental

Note

Mode: incremental
Head: d0a0897 - Retry unstable storage id index builds
Base: bc97677 - Harden storage read cache stamps

🧭 Specimen Summary

  • The specimen replaces the old ID-index collapse with a bounded retry and an uncached disk-read escape hatch. It samples content-bearing stamps before and after index construction, refusing to cache ranges whose observed bytes no longer match the specimen on disk. A cfg(test) hook now forces the file to mutate mid-build, which is a rather efficient way to make the old ghost show itself under glass.

🔎 Isolated Defects

Tip

No actionable defects isolated.

✅ Resolved Since Last Review

  • src-tauri/crates/storage/src/lib.rs:1853 - Unstable index build escapes as a read failure
  • src-tauri/crates/storage/src/lib.rs:1322 - Indexed ID cache contract remains unproven
  • src-tauri/crates/storage/src/lib.rs:1371 - Projected indexed range failure still hides behind fallback
  • src-tauri/crates/storage/src/lib.rs:1863 - ID index can be cached for bytes it did not prove
  • src-tauri/crates/storage/src/lib.rs:2677 - Projection cache now trusts metadata-only identity

🧹 Nitpicks

  • None recorded.

✅ Control Checks

Status Type Check Detail
⚠️ WARN CI Timing CI Status Expected CI controls were missing or incomplete when Bunny posted; verify the control path before merge.
✅ PASS Non-blocking Coverage Prior ID index instability contracts Contracts 1, 2, and 4 appear sutured: the ID-index path captures a pre-scan stamp, refuses mismatched scans, retries with a bound, and falls back uncached rather than preserving a contaminated index.
✅ PASS Non-blocking Coverage Projection cache identity contract Contract 5 appears satisfied in the inspected context: projected-list cache entries compare a content-bearing CollectionContentStamp rather than a metadata-only disguise.
⚠️ UNKNOWN Review Limitation Projected range comma contracts Contracts 3 and 6 concern read_pretty_projected_record_range, whose body is outside the supplied changed hunks. No fresh defect is invented from an unseen organ.
⚠️ WARN Proof Gap Proof The new scan-time rewrite test proves get returns the current row after a mid-build mutation. A direct assertion that the first mismatched scan never populated id_indexes would make the proof less inferential, though the changed control flow caches only after matching stamps.

🧪 Observations

  • Established the supplied incremental base bc97677 and head d0a0897 from the packet.
  • Inspected AGENTS.md and the architecture guard; this Rust storage change remains inside src-tauri without crossing UI, engine, feature, or remote-runtime seams.
  • Examined indexed_record_by_id_from_disk around cache lookup, pre/post CollectionContentStamp comparison, bounded retry, cache insertion, and uncached fallback.

🧰 CI Status

  • warning: required check Frontend, Architecture, and Organization did not appear before Bunny posted.
  • warning: required check Rust Capability Layer did not appear before Bunny posted.
  • warning: required check Browser Smoke and Performance did not appear before Bunny posted.
  • note: Bunny gates only required job conclusions from ci-checks.json; advisory reports such as continue-on-error dependency checks are not included in this status.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bunny Review inline findings

Comment thread src-tauri/crates/storage/src/lib.rs Outdated
Comment thread src-tauri/crates/storage/src/lib.rs Outdated
@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 28243249-5686-4ea7-9533-9823e90fe6db

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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 and usage tips.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bunny Review inline findings

Comment thread src-tauri/crates/storage/src/lib.rs

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bunny Review inline findings

Comment thread src-tauri/crates/storage/src/lib.rs Outdated
};
let refreshed_stamp = collection_content_stamp(&path)?;
if refreshed_stamp != stamp {
return Err(AppError::new(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ MEDIUM: Unstable index build escapes as a read failure

Location: src-tauri/crates/storage/src/lib.rs:1853

A fascinating cure: the index no longer caches bytes it failed to prove, but the mechanism now exports an ordinary concurrent collection rewrite as storage_index_unstable. A get or projected get racing another writer can fail outright even though a bounded retry or uncached read of the refreshed file could produce the correct answer. The stale-cache invariant is protected; the read contract is merely shoved off the table.

Tip

Suggested fix: On stamp mismatch, refuse the cache entry but retry once or fall back to an uncached read of the current disk bytes instead of presenting transient instability as the lookup result.

@Promansis Promansis mentioned this pull request Jun 6, 2026
11 tasks
@Promansis Promansis closed this Jun 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant