Skip to content

fix: two outputs that end the process in the middle of the work they announce - #269

Draft
Denis-hamon wants to merge 1 commit into
ovh:mainfrom
Denis-hamon:fix/logout-keeps-the-key-it-says-it-removed
Draft

fix: two outputs that end the process in the middle of the work they announce#269
Denis-hamon wants to merge 1 commit into
ovh:mainfrom
Denis-hamon:fix/logout-keeps-the-key-it-says-it-removed

Conversation

@Denis-hamon

Copy link
Copy Markdown
Contributor

What this fixes

display.OutputWarning and display.OutputError do not return.
OutputWithFormat finishes on ExitFunc(0) and ExitFunc(1), and ExitFunc is
os.Exit — so everything after such a call is dead code in the shipped binary.
An AST sweep of internal/ finds four such calls on main; both sites are here.

ovhcloud logout kept the key it said it removed

The remote revocation is best effort, and its own comment says so: "the
credentials may already be invalid, in which case we still want to clean up the
local configuration."
But each of its three unhappy paths called
OutputWarning, so the command stopped right there — exit 0 — having printed
🟠 credentials were already invalid or revoked, skipping remote revocation, a
sentence that reads as carrying on, while step 2 never ran.

Reproduced against the built binary, with a bogus key in ./ovh.conf:

$ ovhcloud logout --yes
🟠 credentials were already invalid or revoked, skipping remote revocation
$ echo $?
0
$ grep consumer_key ovh.conf
consumer_key=probe-consumer-key      # still there

After the fix, same input:

$ ovhcloud logout --yes
✅ Logged out: credentials removed from ./ovh.conf (remote revocation: already invalid or revoked, so nothing to revoke remotely)
$ grep consumer_key ovh.conf || echo removed
removed

The outcome of the revocation is now recorded and reported in the final message.
That also makes it one document instead of two: under -o json, a warning
followed by a result put two JSON documents on one stdout, which no parser
accepts.

One unreadable quota killed the whole registry plans listing

formatContainerRegistryPlans called OutputError when imageStorage would not
parse, and it runs once per plan inside the listing loop — so a single
malformed value ended the command with the other plans already collected and
never shown. A quota that will not parse is one unreadable cell: the raw value is
kept and the listing continues.

Verification

tests 5 (3 sub-tests on logout, 2 on the registry plans)
sabotages 3, 3 red, each on the intended test
gate go build, go vet, make wasm, go test ./..., docgen — all green
AST sweep 4 flagged on main, 0 after, out of 1252 calls examined

The logout tests use the real exit semantics rather than the suite's no-op
ExitFunc. That stub — internal/cmd/cmd_test.go replaces ExitFunc with a
no-op — is exactly what hid this class for as long as it existed: under test the
command carries on and prints what the operator will never see.

The registry test asserts on both halves for the same reason: with the no-op
stub, the old code fell through to the next line with the size still zero and
wrote "0" — a wrong quota wearing the shape of a real one.

…announce

`display.OutputWarning` and `display.OutputError` do not return.
`OutputWithFormat` finishes on `ExitFunc(0)` and `ExitFunc(1)`, and `ExitFunc` is
`os.Exit`, so everything after such a call is dead code in the shipped binary. An
AST sweep of internal/ finds four such calls on main; both sites are here.

**`logout` kept the key it said it removed.** The remote revocation is best effort
— its own comment says "the credentials may already be invalid, in which case we
still want to clean up the local configuration" — but each of its three unhappy
paths called OutputWarning. So the command stopped there, exit 0, having printed
"credentials were already invalid or revoked, skipping remote revocation", a
sentence that reads as carrying on, while the local cleanup never ran.

Reproduced against the built binary with a bogus key in ./ovh.conf: exit 0 and
consumer_key untouched. Verified after the fix: removed. Three tests, one per
unhappy path, using the real exit semantics rather than the suite's no-op
ExitFunc — that stub is exactly what hid this for as long as it existed.

The outcome of the revocation is now recorded and reported in the final message,
which also makes it one document instead of two: under -o json a warning followed
by a result put two JSON documents on one stdout, which no parser accepts.

**One unreadable quota killed the whole registry plans listing.**
formatContainerRegistryPlans called OutputError when imageStorage would not
parse, and it runs once per plan inside the listing loop — so a single malformed
value ended the command with the other plans already collected and never shown. A
quota that will not parse is one unreadable cell; the raw value is kept, and the
listing continues.

Under test that call looked harmless, because the no-op ExitFunc let execution
fall through to the next line with the size still zero — writing "0", a wrong
quota wearing the shape of a real one. The test asserts on both halves.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant