Skip to content

fix(config): report cache cleanup deletion failures#14358

Closed
solanaXpeter wants to merge 1 commit intofoundry-rs:masterfrom
solanaXpeter:fix/config-cache-cleanup-errors
Closed

fix(config): report cache cleanup deletion failures#14358
solanaXpeter wants to merge 1 commit intofoundry-rs:masterfrom
solanaXpeter:fix/config-cache-cleanup-errors

Conversation

@solanaXpeter
Copy link
Copy Markdown
Contributor

Motivation

Cache cleanup paths currently ignore all deletion errors.
That keeps missing paths best-effort, but it also hides permission and path-type failures while leaving stale cache state behind.

Solution

Ignore only io::ErrorKind::NotFound for cache cleanup removals and return all other deletion errors with path context from cleanup and the clean_foundry_* helpers.
Add focused unit tests for missing-path and wrong-path-type cases.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Copy Markdown
Collaborator

@mablr mablr left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

Copy link
Copy Markdown
Contributor

@decofe decofe left a comment

Choose a reason for hiding this comment

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

Cache cleanup is best-effort by design — this PR makes forge clean strictly worse by short-circuiting on the first deletion error (remaining dirs never get cleaned).

Instead of propagating errors, log them and continue. Since sh_warn! is a circular dependency in the config crate, use the same eprintln! + yansi::Paint::yellow pattern already used elsewhere in this file (see L1213):

// Instead of helper functions + Result propagation, just warn and continue:
if let Err(err) = fs::remove_file(&self.test_failures_file) {
    if err.kind() != io::ErrorKind::NotFound {
        eprintln!(
            "{}",
            yansi::Paint::yellow(&format!(
                "Warning: failed to remove test failures file {}: {err}",
                self.test_failures_file.display()
            ))
        );
    }
}

Same pattern for every remove_dir_all call. This way all cleanup steps always run, failures are visible to the user, and forge clean never aborts halfway through.

The helper functions (remove_file_if_exists, remove_dir_all_if_exists) and their tests can be dropped — they're not needed with this approach.

@zerosnacks
Copy link
Copy Markdown
Member

Hi @solanaXpeter, thanks for your PR!

I've decided to implement an alternative approach here: #14379 that I prefer and added you as a co-author

@zerosnacks zerosnacks closed this Apr 20, 2026
@github-project-automation github-project-automation Bot moved this to Done in Foundry Apr 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants