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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dzuelke
Copy link

@dzuelke dzuelke commented Sep 1, 2023

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:

let command = this.opts['exit-code'] ? `${this.opts.command}; echo "\uFFFF heroku-command-exit-status: $?"` : this.opts.command

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

let exitCode = data.match(/\uFFFF heroku-command-exit-status: (\d+)/m)
if (exitCode) {
debug('got exit code: %d', exitCode[1])
this.push(data.replace(/^\uFFFF heroku-command-exit-status: \d+$\n?/m, ''))
let code = Number.parseInt(exitCode[1])
if (code === 0) this.resolve()
else {
let err = new Error(`Process exited with code ${cli.color.red(code)}`)
err.exitCode = code
this.reject(err)
}
return
}

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 dz@heroku.com
    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).

Appending our own `; echo …` to the command has several drawbacks:

- it does not run if the command contains an `exit` statement (simple example: `heroku run -x 'exit 3'`);
- it does not run if the shell itself exits, e.g. on commands that use `set -e` or `set -u`;
- it breaks when the preceding command uses valid syntax that conflicts with the suffix (simple example: `heroku run -x 'echo hello;'`);
- it causes the wrong exit status to be reported in `heroku logs` - it's always 0 (from the `echo`), not the correct value.

Using a `trap` on `EXIT` instead works around all of these issues.
The match call uses multiline mode for no reason (it contains no anchors).

The replace call anchors at the start of the string, but this is not a guarantee, and on failure, the magic string is visible in the output (simple example: `heroku run -x 'echo "hi"; echo -n "yo"'`).

Solution is to remove the leading anchor.

It also leaves behind a stray newline after replacing - the reason is that the `\n?` is ungreedy due to the `?`, so it will not actually match the newline character. The result is that using `heroku run -x` produces an extra blank line at the end of the output.

Solution is to not have the newline optional, 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 dzuelke requested a review from a team as a code owner September 1, 2023 01:17
@dzuelke dzuelke temporarily deployed to AcceptanceTests September 1, 2023 08:05 — with GitHub Actions Inactive
@dzuelke dzuelke temporarily deployed to AcceptanceTests September 1, 2023 08:05 — with GitHub Actions Inactive
@dzuelke
Copy link
Author

dzuelke commented Sep 1, 2023

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 dzuelke temporarily deployed to AcceptanceTests September 1, 2023 08:05 — with GitHub Actions Inactive
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.

None yet

1 participant