Skip to content

fix: rollback pending state on tempfile failure in prepare_add#2689

Open
srpatcha wants to merge 2 commits intomozilla:mainfrom
srpatcha:fix/prepare-add-rollback
Open

fix: rollback pending state on tempfile failure in prepare_add#2689
srpatcha wants to merge 2 commits intomozilla:mainfrom
srpatcha:fix/prepare-add-rollback

Conversation

@srpatcha
Copy link
Copy Markdown

Summary

\prepare_add\ updated pending size before \ empfile_in(), which never rolled back on failure. This caused \make_space()\ to panic later when encountering negative available space.

Moved state update after successful tempfile creation and replaced .expect()\ with ?\ operator.

This is split from #2679 per reviewer feedback to keep changes focused.

Changes

  • \src/lru_disk_cache/mod.rs: Move pending size update after tempfile success
  • \src/server.rs: Replace .expect()\ with ?\ operator

Signed-off-by: Srikanth Patchava spatchava@meta.com

prepare_add updated pending size before tempfile_in(), which never
rolled back on failure. This caused make_space() to panic later when
encountering negative available space. Moved state update after
successful tempfile creation and replaced .expect() with ? operator.

Signed-off-by: Srikanth Patchava <spatchava@meta.com>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 5, 2026

Codecov Report

❌ Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.34%. Comparing base (d11e2e0) to head (0a5cfd8).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
src/lru_disk_cache/mod.rs 96.55% 1 Missing ⚠️
src/server.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2689      +/-   ##
==========================================
+ Coverage   74.17%   74.34%   +0.16%     
==========================================
  Files          70       70              
  Lines       39207    39396     +189     
==========================================
+ Hits        29083    29288     +205     
+ Misses      10124    10108      -16     

☔ View full report in Codecov by Sentry.
📢 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.

@sylvestre
Copy link
Copy Markdown
Collaborator

would it be possible to have a unit test to cover this change? thanks

Per @sylvestre review request on mozilla#2689, add a unit test that exercises
the failure path of prepare_add and asserts that pending_size is not
incremented when tempfile creation fails.

Before the fix, prepare_add updated pending state BEFORE calling
tempfile_in(), so any tempfile failure left pending_size > 0 forever,
which would later cause make_space() to panic on the next add.

The new test forces tempfile failure by removing the cache root after
construction, then verifies size() == baseline.

Signed-off-by: Srikanth Patchava <srpatcha@users.noreply.github.com>
@srpatcha
Copy link
Copy Markdown
Author

srpatcha commented May 6, 2026

Hi @sylvestre, thank you for the suggestion! Added a regression test in commit 0a5cfd8:

test_prepare_add_rollback_on_tempfile_failure exercises the failure path by:

  1. Creating an LruDiskCache normally
  2. Removing the cache root directory underneath it
  3. Calling prepare_add(), which now fails because tempfile_in() can't create a file in a missing dir
  4. Asserting that c.size() == baseline_size (no pending state leaked)

Before the fix, the test would fail with size() == 10 because the old code incremented pending_size before the tempfile creation. The test serves as a regression guard for #2689.

@sylvestre
Copy link
Copy Markdown
Collaborator

please run rustfmt on the change

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.

3 participants