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

fix infinite loop due to panic from bare .unwrap of a Result in create_digests intrinsic #21746

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Dec 11, 2024

While investigating #21709, the investigation was obscured by the fact that the issue manifested as a panic in the create_digests intrinsic. The error was in the Err variant of a Result which the create_digests blindly called .unwrap() on, which of course panics by design.

This panic resulted in the engine retrying the build graph node at issue and hitting the panic again ... and again ... and again ...

The engine should really stop if it encounters a panic from a node computation, but in the meantime, the create_digests should just return errors in Result instances instead of panicking.

@tdyas tdyas added category:bugfix Bug fixes for released features release-notes:not-required PR doesn't require mention in release notes labels Dec 11, 2024
@tdyas tdyas requested review from benjyw and huonw December 11, 2024 19:14
@tdyas tdyas changed the title fix infinite loop due to panic from bare .unwrapp of a Result in create_digests intrinsic fix infinite loop due to panic from bare .unwrap of a Result in create_digests intrinsic Dec 11, 2024
@tdyas
Copy link
Contributor Author

tdyas commented Dec 11, 2024

fyi @cburroughs

Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Makes sense, good find.

@tdyas tdyas merged commit 52e3797 into pantsbuild:main Dec 11, 2024
24 checks passed
@tdyas tdyas deleted the engine/fix_unwrap_infinite_loop branch December 11, 2024 21:14
@WorkerPants
Copy link
Member

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

❌ 2.24.x

I was unable to cherry-pick this PR to 2.24.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

  1. (Ensure your git working directory is clean)
  2. Run the following script to reproduce the merge-conflicts:
    git fetch https://github.com/pantsbuild/pants main \
      && git fetch https://github.com/pantsbuild/pants 2.24.x \
      && git checkout -b cherry-pick-21746-to-2.24.x FETCH_HEAD \
      && git cherry-pick 52e3797a2fb98ae3381422a9ee18f3e5b25878b7
  3. Fix the merge conflicts and commit the changes
  4. Run build-support/cherry_pick/make_pr.sh "21746" "2.24.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.


When you're done manually cherry-picking, please remove the needs-cherrypick label on this PR.

Thanks again for your contributions!

🤖 Beep Boop here's my run link

@WorkerPants WorkerPants added the auto-cherry-picking-failed Auto Cherry-Picking Failed label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-cherry-picking-failed Auto Cherry-Picking Failed category:bugfix Bug fixes for released features needs-cherrypick release-notes:not-required PR doesn't require mention in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants