#Capturing error on Sync()

1 messages · Page 1 of 1 (latest)

hushed mason
#

What am I doing wrong?

func (m *Tests) CaptureErrorRepro(ctx context.Context) (*dagger.File, error) {
    ctr, err := dag.Container().From("alpine:latest").
        WithExec([]string{"sh", "-c", "echo 'foobar' > /tmp/foo"}).
        WithExec([]string{"sh", "-c", "exit 1"}).
        Sync(ctx)
    if err != nil {
        return nil, fmt.Errorf("expected error captured: %w", err)
    }
    return ctr.File("/tmp/foo"), nil
}

I want to capture any errors before returning something else, but that basic example shows how the error is never captured after the Sync(ctx)

dagger call -m pocs/tests capture-error-repro
✔ connect 0.2s
✔ load module: pocs/tests 0.7s
✔ parsing command line arguments 0.0s

✔ tests: Tests! 1.5s
✘ .captureErrorRepro: File! 0.5s ERROR
├╴✔ container: Container! 0.0s
├╴$ .from(address: "alpine:latest"): Container! 0.2s CACHED
├╴$ withExec sh -c 'echo '\''foobar'\'' > /tmp/foo' 0.0s CACHED
╰╴✘ withExec sh -c 'exit 1' 0.2s ERROR
  ! exit code: 1

What am I missing?
Seems like it's not capturing the error at the Sync()

Has this always worked this way?

strange python
#

Do you need Sync here?

func (m *Tests) CaptureErrorRepro(ctx context.Context) (*dagger.File, error) {
    ctr, err := dag.Container().From("alpine:latest").
        WithExec([]string{"sh", "-c", "echo 'foobar' > /tmp/foo"}).
        WithExec([]string{"sh", "-c", "exit 1"})
    
    
    if _, err := ctr.ExitCode(ctx); err != nil {
        return nil, fmt.Errorf("expected error captured: %w", err)
    }

    // Do anything else with ctr here
    
    return ctr.File("/tmp/foo"), nil
}
hushed mason
#

That still does not capture the error at ctr.ExitCode(ctx)
I would have to manually check the exitCode being != 0...

#

Which does not feel quite right... 🤔 I would swear this has worked "fine" for a long time
I added the Sync() so to have a reproducible case.

I am returning a file, and I expected to capture the error when I try to use the file (e.g. str, err := f.Contents(ctx) at this point I was expecting to capture the error, but that's not the case neither)

native temple
#

the workflow execution stops at the withExec exit 1

hushed mason
#

Yeah, I expect to see the "expected error captured".

# 
func (m *Tests) CaptureErrorRepro(ctx context.Context) (string, error) {
    ctr := dag.Container().From("alpine:latest").
        WithExec([]string{"sh", "-c", "echo 'foobar' > /tmp/foo"}).
        WithExec([]string{"sh", "-c", "exit 1"})
    str, err := ctr.File("/tmp/foo").Contents(ctx)
    if err != nil {
        return "", fmt.Errorf("failed to read file contents: %w", err)
    }
    return str, nil
}

Shouldn't this capture the error?

# 
✔ connect 1.4s
✔ load module: pocs/tests 6.8s
✔ parsing command line arguments 0.0s

✔ tests: Tests! 3.5s
✘ .captureErrorRepro: String! 0.6s ERROR
✘ withExec sh -c 'exit 1' 0.3s ERROR
! exit code: 1```
#

The fact that it doesn't has just broken some mental models of how the DAG is being executed 😅

#

Uhms... even more weird. If I don't embed the error:

func (m *Tests) CaptureErrorRepro(ctx context.Context) (string, error) {
    ctr := dag.Container().From("alpine:latest").
        WithExec([]string{"sh", "-c", "echo 'foobar' > /tmp/foo"}).
        WithExec([]string{"sh", "-c", "exit 1"})
    str, err := ctr.File("/tmp/foo").Contents(ctx)
    if err != nil {
        return "", fmt.Errorf("failed to read file contents")
    }
    return str, nil
}

Then it shows my custom error:

dagger call -m pocs/tests capture-error-repro
✔ connect 1.2s
✔ load module: pocs/tests 4.4s
✔ parsing command line arguments 0.0s

✔ tests: Tests! 3.2s
✘ .captureErrorRepro: String! 2.0s ERROR
! failed to read file contents
#

I'm... confused? 😕

#

😄

#

Mmmm...

func (m *Tests) CaptureErrorRepro(ctx context.Context) (string, error) {
    ctr := dag.Container().From("alpine:latest").
        WithExec([]string{"sh", "-c", "echo 'foobar' > /tmp/foo"}).
        WithExec([]string{"sh", "-c", "exit 1"})
    str, err := ctr.File("/tmp/foo").Contents(ctx)
    if err != nil {
        fmt.Println("Error Message: " + err.Error())
        return "", fmt.Errorf("failed to read file contents")
    }
    return str, nil
}

Outputs:

Error Message: exit code: 1 [traceparent:02dbd7497811e5e11970499e4f863ba5-ff1c8c8c0d7835ee]
#

Ok, so the difference comes from 0.19.16 to 0.19.17...
We had test cases where we check the error message containing an expected result...

#

All good, we may need some refactoring where we embed the errors coming out from dagger though 🤔

#

Would be good to be able to embed the errors though, we surface some of the errors with more context to what our modules are doing for the customers to understand the usage 🙂

native temple
#

@hushed mason FWIW if you return errors.New instead of wrapping the error with %w, I think that should work

coral spear
# native temple cc <@108011715077091328> mostly checking if wrapping errors changed somehow in 0...

Hmm yeah the TUI tries a lot harder to avoid spammy errors, which is normally what you get from error wrapping, which is usually done kind of blindly like doing foo: %w and ends up not being very helpful in most cases. Now when the TUI sees an error origin ([traceparent:02dbd7497811e5e11970499e4f863ba5-ff1c8c8c0d7835ee] which is the withExec span) it'll prioritize showing the error on its origin instead of showing all the various places it bubbled up.

coral spear
#

@hushed mason Depending on what you're trying to do, you could:

  • not wrap the error and just return your own error - it's the wrapping / err string concatenation that's triggering the error (over)simplification
  • check specifically for an *dagger.ExitError, and turn that into a higher level error, and otherwise bubble it up
  • instead of catching an error, e.g. if the command failing is "expected" (like running tests), you could use expect: ANY and explicitly check for the exit code
hushed mason
#

We are going to review all our error wrapping and refactor them to our custom error messages most likely. At least knowing how to handle them now we can move forward