JS: Sentry CLI wrapper does not propagate errors

[spanferov-figma]

Environment

JavaScript @sentry/cli 2.19.1

Steps to Reproduce

Related to #1644. When invoked like this:

await sentryReleases.uploadSourceMaps(args.commit, {
  include: [tmpdir],
});

this promise never fails even if the underlying CLI operation failed.

This is related to the fact that the promise is never rejected in the execute wrapper here in the live == true branch:

return new Promise((resolve, reject) => {
    if (live === true) {
      const output = silent ? 'ignore' : 'inherit';
      const pid = childProcess.spawn(getPath(), args, {
        env,
        // stdin, stdout, stderr
        stdio: ['ignore', output, output],
      });
      pid.on('exit', () => {
        resolve();
      });
    } else {
      childProcess.execFile(getPath(), args, { env }, (err, stdout) => {
        if (err) {
          reject(err);
        } else {
          resolve(stdout);
        }
      });
    }
  });

Expected/Actual Result

Errors should be surfaced correctly, giving users a chance to handle them.

[kamilogorek]

This is currently implemented like this to make some commands, like upload itself, not fail the CI when it's not successful. We did this on purpose long time ago, as it's already exposing the stderr and logs would tell you that something is wrong. I agree that we could change this behavior and handle "background upload" in the plugins utililzing sentry-cli itself, however changing this now would be a breaking change and make any code relying on blocking await uploadSourceMaps() flow (without handling error case) break. I'm happy to mark this as the improvement for the future major though.

[spanferov-figma]

@kamilogorek we currently experience an influx of 408 errors from Sentry while uploading source maps and we wanted to implement automatic retries. Unfortunately, it's quite hard to do without the error propagation here.

Maybe a good solution to keep it backward compatible might be an optional onError callback. Or we can always make it opt-in by having a propagateErrors: boolean flag or something like this.

[kamilogorek]

@spanferov-figma see my answer here https://github.com/getsentry/sentry-cli/issues/1644#issuecomment-1594681585

As for retries, you are totally right, and we're already on it. We do retry on the CLI side, however only for 502|503|504, not for 408. We are changing this, as from the outside view, 408 should be translated to 504, which will be handled correctly. Bear with us a moment and we'll get it fixed without need to update any code :)

[spanferov-figma]

@kamilogorek can you please provide more context about retries? Were those implemented? Do you have an issue reference? Thanks!

[kamilogorek]

Hey, yes, they were all implemented. All 408s are now translated into 504s in most cases, which CLI will handle correctly. There's issue reference I can provide here however, as it'd done direclty in our ops stack, which is not open source.

[github-actions[bot]]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀