Exit 124 when --timeout expires resolving a data source name - #47
Merged
Conversation
The name lookup was the one server call in internal/cmd that returned its error unwrapped by omission, so a --timeout expiry there exited 1 with a bare context error — the code documented as "a longer timeout will not help" — while the same endpoint under data-source list exited 124.
The buffered reached channel drops arrivals once it is full, so reading the absence of a request out of it took a paragraph of reasoning about which arrivals cannot be the dropped ones. The other endpoints already answer "did this arrive" with a field set under the mutex.
Merged
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.
Why
--timeoutexpiry is supposed to exit124, andskills/rdsh/SKILL.mdmaps that code to "re-run witha longer
--timeout" while1means "a longer timeout will not help". One request escaped that mapping.resolveDataSourceturns a--data-sourcename into an ID by callingGET /api/data_sources, andreturned its error unwrapped;
runRunandrunQueryCreatepassed it straight out. A deadline thatexpired during the lookup therefore exited
1with a barecontext deadline exceeded— pointing anagent away from the one recovery that would have worked. It was the only server call in
internal/cmdthat missed
timeoutOrby omission; the one other unwrapped call,query create's publish step, skipsit deliberately.
The result was that the same command answered differently depending only on whether
--data-sourcewasgiven an ID or a name, and that the same endpoint was a timeout under
rdsh data-source listand anordinary failure under
rdsh run. This is not a corner reserved for callers who type a name:rdsh auth loginprompts for a default data source by ID or name and stores the answer verbatim, so for a profilewhose default is a name, every
rdsh rungoes through this lookup.This changes an exit code: a run that exits
1today exits124after this change.CLAUDE.mdholds the exit codes to a higher bar than a human-facing CLI would, so it is worth stating rather than
letting it pass as an internal detail. The definition of
124is untouched, and the new code is the oneREADME.mdandskills/rdsh/SKILL.mdalready promise for this failure — the code was what disagreedwith them, which is why this is a bug fix rather than a documented promise being changed.
What
runRunandrunQueryCreatepassresolveDataSource's error throughtimeoutOr(err, timeout, "the data source lookup"). The operation name reads distinctly againstdata-source list'sthe data source listingandrun's ownquery.resolveDataSource's doc comment now states that resolving a name is a server call and that callerswrap what it returns, so the obligation sits next to the function rather than being copied into both
call sites.
124path for both commands, plus a pin that an all-digit--data-sourcestill sendsno listing request and still reports its expiry as the query's.
Only the deadline changes classification.
timeoutOrpasses anything that is notcontext.DeadlineExceededthrough untouched, so a name the server does not have still exits1with theexisting
data source %q not foundmessage, and a missing data source still exits1telling the callerto pass
--data-source.Alternative considered
Mapping
context.DeadlineExceededto124centrally, the way the sibling CLIs do in their owninternal/cmd/root.go, is the stronger design against this class of bug in general. It is not right forrdsh:
abandonJob(internal/redash/redash.go) derives its own 10 s cancellation timeout fromcontext.Background()and joins that path's error with the run's, so a centralerrors.Iswould alsofire for a cancellation that timed out — a failure the caller cannot fix by raising
--timeout.Wrapping per call site keeps the opt-in property #28 settled on, and is what #43 recommends.
Not in scope
data-source list,auth login,query update/list/show— all already wrapped, all untouched.query create's publish step, which reports a deadline as an ordinary failure on purpose:124wouldtell an agent to re-run, and re-running create saves a second query.
README.mdandskills/rdsh/SKILL.md, which already describe the behaviour this PR implements.Verification
Closes #43