[Bug]: deal send-manual - stack trace on rejected deal

[ze42]

Description

When sending a deal manually (send-manual), and the deal get rejected, I get a stack-trace, and an exit code 0, making it harder to use and automate things.

Ideally, I would expect:
* A valid json output, with some error information in it
* A non-zero exit code

Steps to Reproduce

Send a deal, to a provider that will reject your deal.

Version

v0.4.1-a0e6ea6

Operating System

Linux

Database Backend

PostgreSQL

Additional context

$ singularity version
singularity v0.4.1-a0e6ea6
$ singularity -json --verbose deal send-manual --client f0xxx \
    --provider f0xxx --piece-cid bagaxxx --piece-size 32GiB \
    --url-template "https://xxx.tld/{PIECE_CID}.car" --verified=false 
2023-09-11T11:04:03.447+0200	INFO	database	database/connstring_cgo.go:35	Opening postgres database
2023-09-11T11:04:03.459+0200	INFO	replication	replication/makedeal.go:516	making deal	{"client": "f0xxx", "pieceCID": "bagaxxx", "provider": "f0xxx"}
deal rejected: Deal rejected | Error
(1) attached stack trace
  -- stack trace:
  | github.com/data-preservation-programs/singularity/cmd/deal.glob..func2
  | 	/home/ze/git/github/singularity/cmd/deal/send-manual.go:183
  | github.com/urfave/cli/v2.(*Command).Run
  | 	/home/ze/go/pkg/mod/github.com/urfave/cli/[email protected]/command.go:274
  | github.com/urfave/cli/v2.(*Command).Run
  | 	/home/ze/go/pkg/mod/github.com/urfave/cli/[email protected]/command.go:267
  | github.com/urfave/cli/v2.(*Command).Run
  | 	/home/ze/go/pkg/mod/github.com/urfave/cli/[email protected]/command.go:267
  | github.com/urfave/cli/v2.(*App).RunContext
  | 	/home/ze/go/pkg/mod/github.com/urfave/cli/[email protected]/app.go:332
  | main.main
  | 	/home/ze/git/github/singularity/singularity.go:54
  | runtime.main
  | 	/usr/lib/go-1.20/src/runtime/proc.go:250
Wraps: (2) attached stack trace
  -- stack trace:
  | github.com/data-preservation-programs/singularity/handler/deal.DefaultHandler.SendManualHandler
  | 	/home/ze/git/github/singularity/handler/deal/send-manual.go:142
  | [...repeated from below...]
Wraps: (3) attached stack trace
  -- stack trace:
  | github.com/data-preservation-programs/singularity/replication.DealMakerImpl.MakeDeal
  | 	/home/ze/git/github/singularity/replication/makedeal.go:609
  | github.com/data-preservation-programs/singularity/handler/deal.DefaultHandler.SendManualHandler
  | 	/home/ze/git/github/singularity/handler/deal/send-manual.go:140
  | github.com/data-preservation-programs/singularity/cmd/deal.glob..func2
  | 	/home/ze/git/github/singularity/cmd/deal/send-manual.go:181
  | github.com/urfave/cli/v2.(*Command).Run
  | 	/home/ze/go/pkg/mod/github.com/urfave/cli/[email protected]/command.go:274
  | github.com/urfave/cli/v2.(*Command).Run
  | 	/home/ze/go/pkg/mod/github.com/urfave/cli/[email protected]/command.go:267
  | github.com/urfave/cli/v2.(*Command).Run
  | 	/home/ze/go/pkg/mod/github.com/urfave/cli/[email protected]/command.go:267
  | github.com/urfave/cli/v2.(*App).RunContext
  | 	/home/ze/go/pkg/mod/github.com/urfave/cli/[email protected]/app.go:332
  | main.main
  | 	/home/ze/git/github/singularity/singularity.go:54
  | runtime.main
  | 	/usr/lib/go-1.20/src/runtime/proc.go:250
  | runtime.goexit
  | 	/usr/lib/go-1.20/src/runtime/asm_amd64.s:1598
Wraps: (4) deal rejected: Deal rejected | Error
Error types: (1) *withstack.withStack (2) *withstack.withStack (3) *withstack.withStack (4) *errutil.leafError

deal rejected: Deal rejected | Error
$ echo $?
0
$

[xinaxu]

This is a general issue not just for send-manual command but for all the rest too.
The expectation should be

• If --json is specified, stdout should only print out a JSON payload
• If --verbose is specified, extra logs and error trace should be printed out to stderr.
• If any error, the exit code should be non zero.

@ze42 would above make sense to you.

[ze42]

If I understand correctly:

• --verbose gives more trace in stderr, which can be used for debug purpose, and try to look more details on what is going wrong
• --json - always gives a JSON payload, including some base error message if any
• exit-code corresponding to 0 ok, non-zero error

If that's it, yeah, it looks good to me.

[xinaxu]

Putting into a matrix below

Success?	--verbose	--json	stdout	stderr	exitcode
✅	❌	❌	simple CLI table	none	0
✅	❌	✅	prettified JSON	none	0
✅	✅	❌	verbose CLI table	none	0
✅	✅	✅	prettified JSON	none	0
❌	❌	❌	simple error message	none	1
❌	❌	✅	error message wrapped in JSON	none	1
❌	✅	❌	simple error message	error message stack trace	1
❌	✅	✅	erorr message wrapped in JSON	error message stack trace	1

[ze42]

You often state stderr as none, though there are some stuffs getting to stderr in most case. Like debug info about connecting to db, and so on. Which I am completely fine with it. (maybe a --quiet that would suppress such output, but that's not what this issue is about)

I would usually expect stderr to also include some base output stating there were an error.
So, on error, to include at least let some logs get into the stderr, the same connecting to db and such get there (with timestamp and color when on a tty, ...)

Ie: would help if doing something like:

$ singularity -json deal send-manual > proposal.json

And have directly in the output the fact that it also failed, not having to double check the logfile to see if it failed or not.

So, just suggesting to have along what you suggest, some more log.

[xinaxu]

Oh yes, all logging are going to stderr and is controlled by https://github.com/ipfs/go-log
It has its own environment variables to control color, format, level, etc.

I want to decouple it from the --verbose flag in singularity library.
The --verbose only applies to the console output and error output from singularity only.

[ze42]

Yeah, so all fine... just make sure to include some logs about the errors, with something like the "simple error message" in all error case?

Such error cases should include any failure to validate arguments. (Had error with file-size malformated, and had to dig into stdout logs to find there was something wrong with it...)

[xinaxu]

Such error cases should include any failure to validate arguments. (Had error with file-size malformated, and had to dig into stdout logs to find there was something wrong with it...)

Most of the places where the error message is not clear have been fixed, i.e., instead of showing error: not found, it will show error: storage xxx not found. Or, instead of err: value invalid, it should show PieceSize ABCD is not a valid number.

Let me know if you encounter any unclear error message so I can make corresponding fixes