feat(api): thread context.Context through the client; cancel cleanly on SIGINT - #48
Merged
Merged
Conversation
Execute() now runs the command tree under signal.NotifyContext, so the first SIGINT/SIGTERM cancels cmd.Context() instead of killing the process. Requester and every api.Client method take a context as their first parameter, requests are built with NewRequestWithContext, and all ~150 call sites pass cmd.Context() (or a threaded ctx) through commands, the sip/tendlc/customerprofile service layers, and the --wait helpers. Every cmdutil.Poll site now sets PollConfig.Context, so the cancellation plumbing added in the 10DLC foundation work finally has a producer: Ctrl-C during --wait aborts the in-flight request and exits through the receipt-emitting paths — tendlc's awaitTerminal prints the accepted resource's bandwidthId instead of dying with nothing on stdout. A second Ctrl-C is untrapped (NotifyContext stops after one signal) and hard-exits. Requester's signatures changed in place rather than growing parallel ...Ctx methods — the call sites are mechanical and this is pre-1.0, per the discussion in Bandwidth#25. Token exchange (auth.TokenManager.GetToken) still runs without a context; that path has its own timeout and is left for the auth-status work. New tests: a client test proving a context deadline aborts an in-flight HTTP request, and a cmd test proving a real SIGINT delivered to the process cancels the command context and Execute returns. AGENTS.md's interim notes about the untrappable Ctrl-C are replaced with the new guarantee. Fixes Bandwidth#25. Fixes Bandwidth#34.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
joshraub-bw
approved these changes
Aug 25, 2026
kshahbw
approved these changes
Aug 25, 2026
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.
Fixes #25. Fixes #34.
What this does
One PR for both issues because #34's complete fix requires #25's plumbing — a signal handler without request contexts only cancels between polls, not during one.
#34 — signal handling.
Execute()now runs the command tree undersignal.NotifyContext(SIGINT, SIGTERM)viarootCmd.ExecuteContext. The first Ctrl-C cancelscmd.Context()everywhere: the in-flight HTTP request aborts,--waitloops exit through their cancellation branches, and tendlc'sawaitTerminalemits the receipt withbandwidthIdbefore the process exits — the accepted-but-unprinted brand ID from the issue's reproduction is no longer lost. A second Ctrl-C is untrapped (NotifyContextstops after one signal) and hard-exits the Go-default way.#25 — context threading.
Requesterand everyapi.Clientmethod takectx context.Contextas the first parameter, and requests are built withhttp.NewRequestWithContext. On the open design question, this changes the signatures in place rather than adding parallel…Ctxmethods, as the issue leans: one mechanical diff, no lingering dual API, pre-1.0. All ~150 call sites migrated: commands passcmd.Context(); theinternal/sip,internal/tendlc, andinternal/customerprofileservice layers thread ctx through their methods;--waithelper functions take ctx; thetestutil.FakeClientand test fakes match the new interface.Poll wiring. Every
cmdutil.Pollsite (13 of them) now setsPollConfig.Context, so theContextfield added in the 10DLC foundation work finally has a producer.Scope note
auth.TokenManager.GetToken(token exchange) still runs without a context — it has its own timeout, and that path is entangled with the #24 auth-status work, so it's deliberately left out.Tests
TestClient_ContextCancelsInFlightRequest— a context deadline aborts an in-flight HTTP request against a deliberately-hanging server (fails against the oldhttp.NewRequestbuild).TestExecuteCancelsCommandContextOnSIGINT— sends a realSIGINTto the test process and asserts the command's context cancels andExecutereturns (//go:build !windows). The issue's "second Ctrl-C hard-exits" behavior comes fromNotifyContextsemantics and can't be asserted in-process.go build,go vet,gofmtclean; full tests pass;internal/api,internal/cmdutil, andcmdrace-clean over-count=3.Docs
AGENTS.md's two interim notes — the untrappable-Ctrl-C caveat on the receipt guarantee (brands and campaigns) — are replaced with the new guarantee: a single Ctrl-C emits the receipt; only a second one kills the process without output.