Skip to content

Expose remote cache read errors and fail on server disconnects - #2779

Closed
seungpyoson wants to merge 11 commits into
mozilla:mainfrom
seungpyoson:agent/fail-closed-cache-errors
Closed

seungpyoson wants to merge 11 commits into
mozilla:mainfrom
seungpyoson:agent/fail-closed-cache-errors

Conversation

@seungpyoson

@seungpyoson seungpyoson commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • in the direct single-cache daemon path, surface non-NotFound remote read failures through the existing cache-error statistics path (MissType::CacheReadError, "Cache errors") instead of counting them as ordinary misses
  • make post-CompileStarted response I/O failures follow the documented default-fatal policy (README: "By default, sccache will fail your build if it fails to successfully communicate with its associated server") while preserving SCCACHE_IGNORE_SERVER_IO_ERROR=1

Related: #2415 (the "read failures are invisible" part; the token-refresh and skip-write parts of that issue are not addressed here).

Why these are the odd ones out

  • RemoteStorage::get_raw already returns an error for every non-NotFound backend failure ("Return error instead of silently returning None"). Only the parsed read used by the daemon path still turned the same failures into a miss.
  • Client-side mode already fails when the compile body ends without CompileFinished (compile_direct). Only the daemon-mode client still compiled locally on an unexpected EOF, contrary to the README default.

After this change both read paths and both client modes behave the same way.

Deliberately unchanged

  • startup rate-limit tolerance
  • automatic read-only downgrade when a configured backend cannot write
  • explicit read-only and public-cache configurations
  • client-side IPC and multi-level cache fallback policies (a multi-level chain still logs a level error and tries the next level)

Scope

The change touches src/cache/cache.rs and src/commands.rs only, squash-ready. There are no documentation, protocol, cache-format, dependency, disk-cache, multi-level-cache, or client-side-storage changes. Existing documentation already describes the server-I/O policy.

Synced with main at c2a187d (2026-10-22): the decode helper now backs get_with_raw (#2835), uses the any_cache_remote cfg alias (#2905), and the ArtifactNotification loop (#2875) is kept as is; only its error arm changed.

Validation

Focused regression tests cover unexpected direct remote read errors, default-fatal post-start disconnects, and the existing opt-in local fallback. On the merged head the three tests pass locally (cargo test --lib -- remote_storage test_handle_compile_response_disconnect), cargo fmt --check is clean, and clippy reports nothing in the touched files. Upstream CI and maintainer review remain required before merge; the fork workflow runs need maintainer approval.

@seungpyoson seungpyoson reopened this Jul 23, 2026
@seungpyoson
seungpyoson force-pushed the agent/fail-closed-cache-errors branch from e14c22c to 5abf492 Compare July 24, 2026 08:11
@seungpyoson seungpyoson changed the title Fix cache and server error propagation Fail closed on remote cache and server I/O errors Jul 24, 2026
@seungpyoson
seungpyoson marked this pull request as ready for review July 24, 2026 08:22
@seungpyoson
seungpyoson force-pushed the agent/fail-closed-cache-errors branch from 5abf492 to 83b66fd Compare July 24, 2026 08:38
@seungpyoson
seungpyoson force-pushed the agent/fail-closed-cache-errors branch from 83b66fd to a35fe90 Compare July 24, 2026 08:59
@seungpyoson seungpyoson changed the title Fail closed on remote cache and server I/O errors Expose remote cache read errors and fail on server disconnects Jul 24, 2026
@codecov-commenter

codecov-commenter commented Jul 28, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.29730% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.21%. Comparing base (ffac4a5) to head (d048ff9).

Files with missing lines Patch % Lines
src/commands.rs 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2779      +/-   ##
==========================================
+ Coverage   70.91%   73.21%   +2.30%     
==========================================
  Files          72       72              
  Lines       37684    37695      +11     
==========================================
+ Hits        26723    27600     +877     
+ Misses      10961    10095     -866     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codspeed

codspeed Bot commented Jul 28, 2026 •

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 58 untouched benchmarks
⏩ 6 skipped benchmarks1


Comparing seungpyoson:agent/fail-closed-cache-errors (d048ff9) with main (2a40bc3)2

Open in CodSpeed

Footnotes

  1. 6 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

  2. No successful run was found on main (ffac4a5) during the generation of this report, so 2a40bc3 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@seungpyoson

Copy link
Copy Markdown
Contributor Author

Synced this PR with current main at 918b8ef. New CI, integration-test, and benchmark workflows are waiting for maintainer approval. Could a maintainer please approve the workflows and re-review the PR after they complete? The sync includes main's libc pin for the prior dist-test failure and allows CodSpeed to compare against the current runner generation.

@seungpyoson

Copy link
Copy Markdown
Contributor Author

@sylvestre Could you take a look at sccache PR #2779 (#2779) when you have a chance? I synced it with current main today. The underlying bugs are still present, the patch merges cleanly, and the fork workflow runs are waiting for maintainer approval.

If the approach looks good, could you approve the workflows and merge it?

@seungpyoson

Copy link
Copy Markdown
Contributor Author

Quick update: the sole non-successful check on d048ff9 was the CUDA 11.8 job, which was canceled after Ubuntu package mirror requests stalled during NVCC installation; it never reached tests. I pushed empty commit acfbdca to retrigger the workflows without source changes. The new CI, integration-test, and benchmark runs are waiting for maintainer approval.

@seungpyoson

Copy link
Copy Markdown
Contributor Author

@sylvestre a gentle reminder for your review

@seungpyoson

Copy link
Copy Markdown
Contributor Author

@sylvestre @Xuanwo Small fail-closed fix, +84/−39, CI green, rebased on main — happy to split or adjust if anything blocks review.

seungpyoson and others added 2 commits August 31, 2026 18:16
Resolve conflicts with mozilla/sccache main (c2a187d):
- src/cache/cache.rs: the fail-closed decode now backs get_with_raw (mozilla#2835)
  and uses the any_cache_remote cfg alias (mozilla#2905); the test follows.
- src/commands.rs: keep the ArtifactNotification loop (mozilla#2875); the read
  error arm collapses to the SCCACHE_IGNORE_SERVER_IO_ERROR check.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019VNNohpFLCBvQm2aB5ZHtA
@seungpyoson

Copy link
Copy Markdown
Contributor Author

Superseded by #2928, which carries only the remote read error fix. The server-disconnect change will be proposed separately.

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.

2 participants