Murex Integrations: preventing another yarn incident

[lmorg]

We recently had an issue where Murex would fail to start on some systems due to the yarn integration.

This is explained in more detail in the ArchLinux AUR comments, however it also impacted Homebrew of macOS and Linux too:

yarn is an optional integration that only gets imported if yarn is in $PATH. The issue seems to stem from a placeholder yarn executable existing on some systems without yarn (proper) being installed. And to compound things, this placeholder yarn assumes its run in a TTY even when it isn't.

We've refactored the yarn integration to solve this specific issue, however, we'll also be looking into ways to prevent this type of problem from occurring with any future integrations in the future.

So the questions I'd like to address would be:

1. We already have extensive build (CI/CD) and unit tests, yet this still slipped through. How can we improve our testing to catch these kinds of issues before new versions of Murex are released?
2. Is there any improvements we can make to integrations to reduce the impact of these kinds of issues in the future?

[lmorg]

I have some thoughts on point 2:

1. should be there a timeout for source files imported during the loading of Murex profiles, including internal ones? I'm thinking this can be user configurable via config (eg in .murex_preload) and maybe default to 10 seconds? That way even if an integration (or any other profile script) hangs, it's an irritation rather than a show-stopper bug.
2. should it be possible to enable/disable specific integrations via config? I see this as being significantly harder to implement in an intuitive way. But it's something we could investigate further if it proved a desirable feature.

[tiymat]

I think the key to avoiding another Yarnpocalypse is making sure integration logic is only loaded when the relevant programs are visible on $PATH. In this case we clearly had code that intended to do that (the leading !if block) which failed.

I guess step 0 is identifying the issue with that block and ensuring it can't recur in other integrations (it seems tied to the bg in particular? If so, bg doesn't seem to be invoked by any other integration, so good news on that front.)

Maybe going forward we could add some logic to defaults.go or elsewhere to ensure that integration code only gets run if the command is available on $PATH? Though I guess there'd need to be some sort of association between each integration script and the actual command to check for, as relying on the filename isn't ideal, so that could end up getting more complicated that it at first seems.

I like the idea of a timeout (though it might be more of a band-aid). It shouldn't matter for 99.5% of valid use cases and then for the pathological cases that do genuinely need to run longer, there could be an option to alter or disable the timeout.

[tiymat]

should it be possible to enable/disable specific integrations via config?

I think if we have an effective and consistent way to ensure integration code doesn't run when it's not needed, then configuring on/off specific integrations isn't that useful. I know that technically you can do a wide range of things with integrations, but in practice, as I glance through them now, it seems many (most?) of them are primarily about autocompletions and summaries. It seems unlikely that many people would dislike these so much as to want to turn them off, when they could just e.g. not engage the autocompletions for a given command.

[lmorg]

Is there definitely no executable on your system called ‘yarn’?

the system wouldn’t hang because of the absence of an executable. From what I’ve seen, some systems do bundle a placeholder ‘yarn’ even if the user hasn’t explicitly installed that executable. And it’s that placeholder ‘yarn’ that misbehaves.

[lmorg]

The !if syntax does work. There's lots of unit tests already for that piece of logic and a quick test in the command line confirms this too:

» !if { which cat } then { out THEN } else { out ELSE }
ELSE

» !if { which sdkfjslkjfhdsaf } then { out THEN } else { out ELSE }
THEN

so the following pattern should work too:

!if { which yarn } then {
    return
}

Which means the issue is that there's something else installed on the host called yarn but which blocks execution. @priscira discusses this in #956