Change `heroku run -x` implementation to use a Bash EXIT trap

[dzuelke]

Background

When -x is passed to heroku run, the given command is suffixed with a special echo that returns $?, the exit status of the program:

https://github.com/heroku/cli/blob/b451c6b3054dace0bc462eb778be8ba2d3f4d1ed/packages/run-v5/lib/dyno.js#L54

This \uFFFF heroku-command-exit-status: … output is later detected and used:

https://github.com/heroku/cli/blob/b451c6b3054dace0bc462eb778be8ba2d3f4d1ed/packages/run-v5/lib/dyno.js#L315-L328

Exit status reporting issues

Appending our own ; echo … to the command causes several bugs:

1. it does not run if the command contains an exit statement (simple example: heroku run -x 'exit 3'):

   $ heroku run -x -- 'exit 99' || echo $?
   Running exit 99 on ⬢ sushi... up, run.4144 (Standard-1X)
   
   1
   

   It must be wrapped in a bash -c instead, as a workaround:

   $ heroku run -x -- 'bash -c "exit 99"' || echo $?
   Running bash -c "exit 99" on ⬢ sushi... up, run.4799 (Standard-1X)
   
    ›   Error: Process exited with code 99
    ›   Code: 99
   99
   

2. it does not run if the shell itself exits, e.g. on commands that use set -e or set -u:

   $ heroku run -x 'set -e; false' || echo $?
   Running set -e; false on ⬢ sushi... up, run.7734 (Standard-1X)
   
   1
   

3. it breaks when the preceding command uses valid syntax that conflicts with the suffix (simple example: heroku run -x 'echo hello;'):

   $ heroku run 'echo "hello world";'
   Running echo "hello world"; on ⬢ sushi... up, run.9517 (Standard-1X)
   hello world
   
   $ heroku run -x 'echo "hello world";'
   Running echo "hello world"; on ⬢ sushi... up, run.2662 (Standard-1X)
   bash: -c: line 1: syntax error near unexpected token `;;'
   bash: -c: line 1: `echo "hello world";; echo " heroku-command-exit-status: $?"'
   

4. it causes the wrong exit status to be reported in heroku logs - it's always 0 (from the echo), not the correct value:

   $ heroku logs
   2023-09-01T00:00:20.512065+00:00 app[api]: Starting process with command `bash -c "exit 99"; echo " heroku-command-exit-status: $?"` by user [email protected]
   2023-09-01T00:00:41.461873+00:00 heroku[run.5368]: State changed from starting to up
   2023-09-01T00:00:41.519004+00:00 heroku[run.5368]: Awaiting client
   2023-09-01T00:00:41.533873+00:00 heroku[run.5368]: Starting process with command `bash -c "exit 99"; echo " heroku-command-exit-status: $?"`
   2023-09-01T00:00:45.174445+00:00 heroku[run.5368]: Client connection closed. Sending SIGHUP to all processes
   2023-09-01T00:00:45.695343+00:00 heroku[run.5368]: Process exited with status 0
   2023-09-01T00:00:45.719124+00:00 heroku[run.5368]: State changed from up to complete
   

Exit status reporting solution

Using a trap on EXIT instead works around all of these issues (in this example, the construction is a bit convoluted due to escaping challenges, but the principle is the same as the contents of this PR):

$ heroku run -- "trap '"'echo "'$(echo "\uFFFF")' heroku-command-exit-status: $?"'"' EXIT; exit 99;" || echo $?
Running trap 'echo " heroku-command-exit-status: $?"' EXIT; exit 99; on ⬢ sushi... up, run.1897 (Standard-1X)

 ›   Error: Process exited with code 99
 ›   Code: 99
99

Magic string handling issues

Additionally, there are problems with the matching and removal of the magic \uFFFF heroku-command-exit-status: … string.

First, the magic string replace anchors on the beginning of a line, but if the last line of output does not end with a newline, this will fail:

$ heroku run -x 'bash -c "echo foo; echo -n hi; exit 3"'
Running bash -c "echo foo; echo -n hi; exit 3" on ⬢ sushi... up, run.3301 (Standard-1X)
foo
hi heroku-command-exit-status: 3
 ›   Error: Process exited with code 3
 ›   Code: 3

Second, it always leaves behind a newline character. This means the output between heroku run and heroku run -x differs:

$ heroku run "echo hi"; echo ---
Running echo hi on ⬢ sushi... up, run.9696 (Standard-1X)
hi
---

$ heroku run -x "echo hi"; echo ---
Running echo hi on ⬢ sushi... up, run.9696 (Standard-1X)
hi

---

The reason is that the \n? is ungreedy due to the ?, so it will not actually match the newline character.

Magic string handling solution

The anchoring at the beginning of the line is easy to remove in the replace Regex.

For the trailing newline, the solution is to not have the newline optional in the Regex (our echo always prints it, after all), or to use a different trailing marker - I have re-used \uFFFF here, because that also allows removing multiline mode.

Ideally, we'd use a proper unique string - e.g. generate a GUID for the command, and match that exact GUID later. Could also use a different separator - \uFFFF is not legal unicode, but 💜 is ;) (that's \U1F49C in Bash, with uppercase "U", and \uD83D\uDC9C in JavaScript, expressed as a UTF-16 surrogate pair).

[dzuelke]

Just fwiw, don't immediately have to merge this; we can brainstorm the best approach for the magic string delimiter etc first. Feel free to leave open for a while.

[dzuelke]

Relevant: https://help.heroku.com/QUGCYVK6/how-do-i-run-one-off-dynos-with-exit-code-on-a-container-app-in-a-private-space

[lake-effect]

Just stopping by to say that this would probably solve a weird bug that may be incidental to heroku run: https://bugs.ruby-lang.org/issues/20514

[dzuelke]

I don't think that's related, per se, @lake-effect. The difference between system() and Open3.popen*() is that the former runs the given command in a subshell, while the latter IIRC does not, but I'd have to check. There might also be a difference in how the quotes and backslashes are escaped.