fix: report a failed -o write instead of claiming success - #748
Open
VXNCXNX wants to merge 1 commit into
Open
Conversation
File write failures for -o and --format-multi now exit with status 1 and report errors to stderr, matching the behavior of --report, --hotspots, and other output paths. Adds regression test to verify this contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I explicitly licence this contribution under the MIT licence.
What's broken
-oreports success after a write that failed, and exits 0.--format-multihas the same hole, exiting 0 with the message going to stdout:The same mistakes on the sibling paths are already handled, which is what makes this look like an oversight:
Read-only directories and paths that are actually directories fail the same silent way. In a CI job that is a green build with no artifact.
The fix
processor.gohad_ = os.WriteFile(...)followed unconditionally by the success line. It now checks the error, prints it to stderr and exits 1, followingrunReporta few hundred lines above, which does exactly this.formatters.goalready built a message for the multi case but sent it to stdout and carried on, so it goes to stderr and exits too.After:
A successful write is unchanged, still
results written to ...and exit 0.os.Exit(1)rather than returning an error, becauseProcess()is called from cobra'sRun:, which has no error return, and the report path atprocessor.go:932already resolves that the same way. Happy to plumb an error through instead if you would rather change the signature.This changes an exit code, from 0 to 1, on a case that previously failed silently. That is the point of the change, but it is a visible difference for anyone whose script currently ignores it.
Verification
TestRegressionOutputWriteFailureExitsinregression_test.go, one subtest per path. It observes the real exit code:runSCCDirruns the test binary as a subprocess, soos.Exit(1)is a genuine child-process exit. It asserts non-zero exit, absence of the success message, presence of the reason, and that no file was created. With both source changes reverted and the test kept, both subtests fail.LANG=C go test ./...passes.A note in case you see it too: on a machine with a comma-decimal
LANG, four unrelated tests fail on master, because the formatters build their printer fromos.Getenv("LANG")while the fixtures spell numbers with a dot.LANG=Cis green before and after.On issue #510
I looked at whether this explains #510 and I do not think it does. That reporter got the tabular summary written instead of JSON, which is the unknown-format fallback from a
json2-less binary, matching your reply that the action needed updating. Their target directory was writable, so no write failed and nothing was swallowed. Same family of symptom, different cause.