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 potential "infinite recursion" panics in runners #1670

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

ag-eitilt
Copy link
Collaborator

We've seen a couple problems where the proper reporting of an issue got masked by an "Infinite recursion detected" panic, and we verified that it was due to primJobFinish/primJobFailFinish being called before the respective primJobLaunch/primJobFailLaunch -- which for all their other benefits is a new risk with v44's flattened runners. Ultimately it would be best to implement an architecture solution for solving this, but in the mean time it's sufficient to patch the existing runners and to keep a careful eye on future code which touches them.

Copy link
Contributor

@colinschmidt colinschmidt left a comment

Choose a reason for hiding this comment

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

One question about the mkdir removal, otherwise LGTM

@@ -263,18 +263,14 @@ export def makeJSONRunner ((JSONRunnerPlan rawScript extraArgs extraEnv estimate
Nil
)

require Pass build =
mkdir ".build"
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this mkdir live now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Inside the write:

export def write (path: String) (content: String): Result Path Error =
    def spath = simplify path

    require Pass parent =
        simplify "{spath}/.."
        | mkdir # Note that this is recursive, as well.

    writeImp (parent, Nil) 0644 spath content

My guess is that the mkdir was originally split out purely for the error message, but the shell's mkdir already advertises the path it tried unsuccessfully to create, so keeping it separate didn't seem to have any benefit.

Also, CI apparently succeeded? I'll look into the supposedly end-of-lifed image in a bit.

@ag-eitilt
Copy link
Collaborator Author

Ah, apparently the deprecation is happening on jan 30, so we've got another month and a half. Merging!

@ag-eitilt ag-eitilt merged commit 72ff132 into master Dec 9, 2024
11 checks passed
@ag-eitilt ag-eitilt deleted the primJobFailLaunch-additions branch December 9, 2024 22:42
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