Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ANE-2237] make the preflight check filename unique #1498

Merged
merged 3 commits into from
Jan 16, 2025

Conversation

spatten
Copy link
Contributor

@spatten spatten commented Jan 16, 2025

Overview

In ANE-2237, Lukas from SEW Eurodrive reported that the preflight check was failing around 10% of the time on their CI instance.

This was happening because they were running multiple runs of fossa in parallel, so the processes were sometimes deleting the preflight check file created by another instance of fossa, and it was raising an error when it tried to delete the nonexistent file.

Two obvious ways to fix this are to just not fail if you can't delete or to make the filename unique. I opted for the unique filename approach, but I have no strong opinion here.

Acceptance criteria

Testing plan

I was able to repro the initial problem by running this in one terminal:

while true; do rm -f $TMPDIR/preflight-check.txt; done

and then running fossa report a bunch of times in another:

repeat 20 { fossa report --format spdx-json --project zd-5344 --revision 2022-12-21T01:21:22Z attribution > /dev/null }

That's no longer going to reproduce the problem. So I just put in some logging and validated that the filename being returned by preflightCheckFilename was different between runs.

Risks

This is low risk

Metrics

References

ANE-2237

Checklist

  • I added tests for this PR's change (or explained in the PR description why tests don't make sense).
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • If this PR added docs, I added links as appropriate to the user manual's ToC in docs/README.ms and gave consideration to how discoverable or not my documentation is.
  • If this change is externally visible, I updated Changelog.md. If this PR did not mark a release, I added my changes into an ## Unreleased section at the top.
  • If I made changes to .fossa.yml or fossa-deps.{json.yml}, I updated docs/references/files/*.schema.json AND I have updated example files used by fossa init command. You may also need to update these if you have added/removed new dependency type (e.g. pip) or analysis target type (e.g. poetry).
  • If I made changes to a subcommand's options, I updated docs/references/subcommands/<subcommand>.md.

@spatten spatten requested a review from a team as a code owner January 16, 2025 20:44
@spatten spatten requested a review from nficca January 16, 2025 20:44
@spatten spatten requested a review from csasarak January 16, 2025 20:54
Copy link
Contributor

@csasarak csasarak left a comment

Choose a reason for hiding this comment

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

This is a nice find and fix!

@spatten spatten enabled auto-merge (squash) January 16, 2025 21:06
@spatten
Copy link
Contributor Author

spatten commented Jan 16, 2025

This is a nice find and fix!

I can't really take credit for the find. Lukas from SEW Eurodrive was the one who pointed out that it was because they are running multiple scans on the same machine.

I'm just glad he did. This could have taken a while to figure out otherwise!

@spatten spatten merged commit dee2229 into master Jan 16, 2025
18 of 19 checks passed
@spatten spatten deleted the ANE-2237-make-preflight-check-file-unique branch January 16, 2025 21:21
Copy link

@lpanni lpanni left a comment

Choose a reason for hiding this comment

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

I like the unique filename approach more than ignoring erorrs during deletion. Thanks for the quick fix!

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