diff --git a/cmd/cli/cli.go b/cmd/cli/cli.go index eaee8ef..abc2558 100644 --- a/cmd/cli/cli.go +++ b/cmd/cli/cli.go @@ -316,23 +316,57 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { } if cdUID != "" { validateCdUpstreamProtocol() - if rc, err := processCDFlags(&cfg); err != nil { + // Bound API preflight by the service lifetime. Without this, a stop request + // arriving while the API is unreachable leaves this retry/backoff loop running + // after "Service stopped" was logged, so the process keeps working on behalf of + // a service the OS considers stopped. + pf := runAPIPreflight(p.stopCh, &cfg) + switch { + case pf.stopRequested: + // Stop requested during preflight, whether or not the fetch itself + // succeeded. A successful fetch does not entitle startup to continue: the + // operator asked for a stop, and carrying on would set up listeners and + // interception for a service the OS already considers stopping. + // + // Exit the way a normal stop does: no Fatal, so the OS service manager does + // not see a failed start and apply its restart policy to a service the + // operator just asked to stop. + p.Notice().Msg("Stop requested while fetching resolver config — shutting down") + notifyExitToLogServer() + return + case pf.err != nil: if isMobile() { - appCallback.Exit(err.Error()) + appCallback.Exit(pf.err.Error()) return } cdLogger := p.logger.Load().With().Str("mode", "cd") // Performs self-uninstallation if the ControlD device does not exist. var uer *controld.ErrorResponse - if errors.As(err, &uer) && uer.ErrorField.Code == controld.InvalidConfigCode { + if errors.As(pf.err, &uer) && uer.ErrorField.Code == controld.InvalidConfigCode { _ = uninstallInvalidCdUID(p, cdLogger, false) } + if rejection, ok := permanentAPIRejection(pf.err); ok { + // The API answered and rejected this request permanently. Restarting + // cannot change that answer, so exit cleanly rather than through Fatal: + // an abnormal exit spends one of the service manager's restart actions, + // and on Windows those are what bring enforcement back after a real + // crash. Burning that budget on a config problem also buries the API's + // reason under repeated start failures. + cdLogger.Error().Err(pf.err).Int("status", rejection.StatusCode).Msg("Failed to fetch resolver config; the API rejected this configuration") + notifyExitToLogServer() + return + } notifyExitToLogServer() - cdLogger.Fatal().Err(err).Msg("Failed to fetch resolver config") - } else { + // Everything else - a denied socket, an unreachable API, a proxy in the way, + // an API that is having a bad day - is a condition a later start may not hit, + // so keep the abnormal exit and let the service manager's recovery policy + // retry. That policy backs off (see ConfigureWindowsServiceFailureActions), so + // a short-lived block does not exhaust it. + cdLogger.Fatal().Err(pf.err).Msg("Failed to fetch resolver config") + default: p.mu.Lock() - p.rc = rc + p.rc = pf.rc p.mu.Unlock() } } @@ -682,28 +716,152 @@ func deactivationPinSet() bool { return cdDeactivationPin.Load() != defaultDeactivationPin } -// processCDFlags processes Control D related flags -func processCDFlags(cfg *ctrld.Config) (*controld.ResolverConfig, error) { +// fetchResolverConfig is a test seam for the ControlD resolver-config API call. +var fetchResolverConfig = controld.FetchResolverConfig + +// apiPreflight is the outcome of the API preflight fetch: the resolver config, the +// error if any, and whether the service was asked to stop while it ran. +type apiPreflight struct { + rc *controld.ResolverConfig + err error + stopRequested bool +} + +// runAPIPreflight fetches the ControlD resolver config bounded by the service +// lifetime, and reports whether a stop was requested while it ran. +// +// The distinction matters because the caller does very different things with it: a stop +// exits quietly, while a failure self-uninstalls a deleted device, surfaces the error to +// a mobile app, and reports a failed start to the service manager. +// +// stopRequested must not be derived from the context once it has been cancelled. +// context.CancelFunc sets ctx.Err() unconditionally, so reading it after the cancel +// classifies *every* failure - a deleted device, an exhausted retry, a mobile caller +// with no stop channel - as an operator stop. Reading the stop channel directly is also +// independent of whether the context's watcher goroutine has been scheduled yet. +func runAPIPreflight(stopCh <-chan struct{}, cfg *ctrld.Config) apiPreflight { + rc, err := fetchCDConfigBoundedBy(stopCh, cfg) + return apiPreflight{rc: rc, err: err, stopRequested: stopRequested(stopCh)} +} + +// permanentAPIRejection reports whether err is the API refusing this request in a way +// that a restart cannot change, and returns the rejection when it is. +// +// The type alone does not answer this. controld builds an *ErrorResponse for *any* +// non-200 whose body decodes, so a 502 from a load balancer and a 404 for a deleted +// device arrive as the same Go type. Treating both as permanent would let a few minutes +// of API trouble stop ctrld on every host with no service-manager retry behind it, which +// is strictly worse than the abnormal exit it replaced. +// +// So the HTTP status decides, and only a client-error status counts: +// +// - 4xx: the API examined this request and refused it - a deleted device, a revoked +// token, a malformed UID. The same request will be refused again. +// - 408 and 429 are the exceptions: they are the API asking for another attempt later. +// - 5xx, or no recorded status, says nothing about this configuration. Retry. +func permanentAPIRejection(err error) (*controld.ErrorResponse, bool) { + var uer *controld.ErrorResponse + if !errors.As(err, &uer) { + return nil, false + } + switch uer.StatusCode { + case http.StatusRequestTimeout, http.StatusTooManyRequests: + return nil, false + } + if uer.StatusCode < 400 || uer.StatusCode >= 500 { + return nil, false + } + return uer, true +} + +// processCDFlagsFn is the API fetch, indirected so the lifetime binding around it can be +// tested without reaching the network. +var processCDFlagsFn = processCDFlags + +// fetchCDConfigBoundedBy runs the API fetch bounded by stopCh, so a fetch that cannot +// reach the API stops when the service is asked to stop instead of working on behalf of a +// service the OS already considers stopped. The derived context is always cancelled, which +// releases the goroutine watching stopCh. +func fetchCDConfigBoundedBy(stopCh <-chan struct{}, cfg *ctrld.Config) (*controld.ResolverConfig, error) { + ctx, cancel := contextFromStopCh(stopCh) + defer cancel() + return processCDFlagsFn(ctx, cfg) +} + +// fetchCDConfigBoundedByLifetime is the reload path's fetch. Reload binds the same stop +// primitives as startup - it used to wire them up itself, where a dropped cancel or the +// wrong channel would have failed nothing. +func (p *prog) fetchCDConfigBoundedByLifetime(cfg *ctrld.Config) (*controld.ResolverConfig, error) { + return fetchCDConfigBoundedBy(p.stopCh, cfg) +} + +// stopRequested reports whether stopCh has been closed. A nil channel - mobile passes +// none - blocks forever, so the default case is taken and it reads as "no stop". +func stopRequested(stopCh <-chan struct{}) bool { + select { + case <-stopCh: + return true + default: + return false + } +} + +// contextFromStopCh returns a context that is cancelled when stopCh closes, so +// long-running startup work stops as soon as the service is asked to stop. The +// returned cancel func must be called to release the watcher goroutine. +func contextFromStopCh(stopCh <-chan struct{}) (context.Context, context.CancelFunc) { + ctx, cancel := context.WithCancel(context.Background()) + if stopCh == nil { + return ctx, cancel + } + go func() { + select { + case <-stopCh: + cancel() + case <-ctx.Done(): + } + }() + return ctx, cancel +} + +// processCDFlags fetches the ControlD configuration for cdUID and applies it to cfg. +// +// ctx bounds the whole operation, including the bootstrap-DNS retry loop below. That +// loop retries indefinitely by design (a device with no network yet must eventually +// come up), so it must be cancellable: otherwise a stop request during preflight is +// ignored and the process keeps retrying after the service reports itself stopped. +func processCDFlags(ctx context.Context, cfg *ctrld.Config) (*controld.ResolverConfig, error) { logger := mainLog.Load().With().Str("mode", "cd") logger.Info().Msgf("Fetching Controld D configuration from API: %s", cdUID) bo := backoff.NewBackoff("processCDFlags", logf, 30*time.Second) bo.LogLongerThan = 30 * time.Second - ctx := ctrld.LoggerCtx(context.Background(), logger) + if ctx == nil { + ctx = context.Background() + } + ctx = ctrld.LoggerCtx(ctx, logger) req := &controld.ResolverConfigRequest{ RawUID: cdUID, Version: appVersion, Metadata: ctrld.SystemMetadata(ctx), } - resolverConfig, err := controld.FetchResolverConfig(ctx, req, cdDev) + resolverConfig, err := fetchResolverConfig(ctx, req, cdDev) // Retry logic for network errors using bootstrap DNS // This is needed because the initial DNS resolution might fail due to network issues // or DNS server unavailability, but bootstrap DNS can provide alternative resolution for { + if ctxErr := ctx.Err(); ctxErr != nil { + logger.Debug().Msg("Resolver config fetch cancelled") + return nil, ctxErr + } if errUrlNetworkError(err) { bo.BackOff(ctx, err) + if ctxErr := ctx.Err(); ctxErr != nil { + logger.Debug().Msg("Resolver config fetch cancelled during backoff") + return nil, ctxErr + } logger.Warn().Msg("Could not fetch resolver using bootstrap DNS, retrying...") - resolverConfig, err = controld.FetchResolverConfig(ctx, req, cdDev) + resolverConfig, err = fetchResolverConfig(ctx, req, cdDev) continue } break diff --git a/cmd/cli/cli_preflight_test.go b/cmd/cli/cli_preflight_test.go new file mode 100644 index 0000000..c89b154 --- /dev/null +++ b/cmd/cli/cli_preflight_test.go @@ -0,0 +1,403 @@ +package cli + +import ( + "context" + "errors" + "fmt" + "net" + "net/http" + "net/url" + "sync/atomic" + "syscall" + "testing" + "time" + + "github.com/Control-D-Inc/ctrld" + "github.com/Control-D-Inc/ctrld/internal/controld" +) + +func TestContextFromStopCh(t *testing.T) { + t.Run("cancels when stopCh closes", func(t *testing.T) { + stopCh := make(chan struct{}) + ctx, cancel := contextFromStopCh(stopCh) + defer cancel() + + if ctx.Err() != nil { + t.Fatalf("context cancelled before the stop request: %v", ctx.Err()) + } + close(stopCh) + select { + case <-ctx.Done(): + case <-time.After(5 * time.Second): + t.Fatal("context was not cancelled after stopCh closed") + } + if !errors.Is(ctx.Err(), context.Canceled) { + t.Errorf("ctx.Err() = %v, want %v", ctx.Err(), context.Canceled) + } + }) + + t.Run("cancel releases the watcher", func(t *testing.T) { + // stopCh is never closed: cancel() must still end the goroutine watching it. + ctx, cancel := contextFromStopCh(make(chan struct{})) + cancel() + select { + case <-ctx.Done(): + case <-time.After(5 * time.Second): + t.Fatal("context was not cancelled by cancel()") + } + }) + + t.Run("nil stopCh is usable", func(t *testing.T) { + // Mobile callers have no stop channel; preflight must still run. + ctx, cancel := contextFromStopCh(nil) + defer cancel() + if ctx.Err() != nil { + t.Fatalf("context cancelled immediately: %v", ctx.Err()) + } + }) +} + +// retryableNetworkErr is the shape processCDFlags treats as "retry with bootstrap +// DNS": a url.Error wrapping a network failure. +func retryableNetworkErr() error { + return &url.Error{ + Op: "Post", + URL: "https://api.controld.com/utility", + Err: &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED}, + } +} + +func TestProcessCDFlagsStopsWhenCancelled(t *testing.T) { + oldFetch := fetchResolverConfig + oldUID := cdUID + t.Cleanup(func() { + fetchResolverConfig = oldFetch + cdUID = oldUID + }) + cdUID = "testuid" + + var calls atomic.Int64 + fetchResolverConfig = func(ctx context.Context, req *controld.ResolverConfigRequest, dev bool) (*controld.ResolverConfig, error) { + calls.Add(1) + return nil, retryableNetworkErr() + } + + // A stop request arriving while the API is unreachable. Before this was + // cancellable, the retry loop kept running after the service reported itself + // stopped, which is what kept the incident's process alive and enforcing. + stopCh := make(chan struct{}) + ctx, cancel := contextFromStopCh(stopCh) + defer cancel() + + done := make(chan error, 1) + go func() { + cfg := ctrld.Config{} + _, err := processCDFlags(ctx, &cfg) + done <- err + }() + + // Let it fail at least once and settle into backoff before stopping. + deadline := time.After(10 * time.Second) + for calls.Load() == 0 { + select { + case <-deadline: + t.Fatal("resolver config was never fetched") + case err := <-done: + t.Fatalf("processCDFlags returned before any fetch: %v", err) + default: + time.Sleep(5 * time.Millisecond) + } + } + close(stopCh) + + select { + case err := <-done: + if !errors.Is(err, context.Canceled) { + t.Errorf("processCDFlags err = %v, want it to report %v", err, context.Canceled) + } + case <-time.After(30 * time.Second): + t.Fatal("processCDFlags did not return after the stop request") + } +} + +func TestProcessCDFlagsReturnsImmediatelyWhenAlreadyCancelled(t *testing.T) { + oldFetch := fetchResolverConfig + oldUID := cdUID + t.Cleanup(func() { + fetchResolverConfig = oldFetch + cdUID = oldUID + }) + cdUID = "testuid" + + var calls atomic.Int64 + fetchResolverConfig = func(ctx context.Context, req *controld.ResolverConfigRequest, dev bool) (*controld.ResolverConfig, error) { + calls.Add(1) + return nil, retryableNetworkErr() + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + cfg := ctrld.Config{} + _, err := processCDFlags(ctx, &cfg) + if !errors.Is(err, context.Canceled) { + t.Errorf("processCDFlags err = %v, want %v", err, context.Canceled) + } + // One attempt is made before the loop notices; it must not retry past that. + if got := calls.Load(); got > 1 { + t.Errorf("fetched %d times with a cancelled context, want at most 1", got) + } +} + +// TestRunAPIPreflightClassification is the regression guard for classifying a preflight +// failure as an operator stop. +// +// runAPIPreflight cancels the context it derived from stopCh. Sampling the stop state +// from that context afterwards reports "stopped" unconditionally, because +// context.CancelFunc sets ctx.Err() whether or not anyone asked to stop. run() then +// takes the stop branch for every failure, which skips self-uninstalling a deleted +// device, skips the mobile exit callback, and tells the service manager a failed start +// was a clean exit. +func TestRunAPIPreflightClassification(t *testing.T) { + oldFetch := fetchResolverConfig + oldUID := cdUID + t.Cleanup(func() { + fetchResolverConfig = oldFetch + cdUID = oldUID + }) + cdUID = "testuid" + + // A deleted ControlD device: non-retryable, so preflight returns promptly. + deletedDevice := func() error { + e := &controld.ErrorResponse{} + e.ErrorField.Code = controld.InvalidConfigCode + e.ErrorField.Message = "device does not exist" + return e + } + + openCh := make(chan struct{}) + closedCh := make(chan struct{}) + close(closedCh) + + tests := []struct { + name string + stopCh <-chan struct{} + fetchErr func() error + wantStop bool + }{ + { + // The P1: no stop was requested, so this must reach the failure branch. + name: "api error with no stop request", + stopCh: openCh, + fetchErr: deletedDevice, + }, + { + // Mobile passes no stop channel at all, so it could never have stopped. + name: "api error with a nil stop channel", + stopCh: nil, + fetchErr: deletedDevice, + }, + { + name: "stop requested during preflight", + stopCh: closedCh, + fetchErr: func() error { return retryableNetworkErr() }, + wantStop: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + fetchResolverConfig = func(context.Context, *controld.ResolverConfigRequest, bool) (*controld.ResolverConfig, error) { + return nil, tc.fetchErr() + } + cfg := ctrld.Config{} + pf := runAPIPreflight(tc.stopCh, &cfg) + + if pf.err == nil { + t.Fatal("expected preflight to fail") + } + if pf.stopRequested != tc.wantStop { + t.Errorf("stopRequested = %v, want %v", pf.stopRequested, tc.wantStop) + } + }) + } +} + +// TestRunAPIPreflightPreservesAPIError verifies the error reaches the caller in a form +// the failure branch can still act on: self-uninstall keys off an *ErrorResponse with +// InvalidConfigCode, and it only runs if that error is both classified as a failure and +// still unwrappable. +func TestRunAPIPreflightPreservesAPIError(t *testing.T) { + oldFetch := fetchResolverConfig + oldUID := cdUID + t.Cleanup(func() { + fetchResolverConfig = oldFetch + cdUID = oldUID + }) + cdUID = "testuid" + + want := &controld.ErrorResponse{} + want.ErrorField.Code = controld.InvalidConfigCode + fetchResolverConfig = func(context.Context, *controld.ResolverConfigRequest, bool) (*controld.ResolverConfig, error) { + return nil, want + } + + cfg := ctrld.Config{} + pf := runAPIPreflight(make(chan struct{}), &cfg) + + if pf.stopRequested { + t.Error("a device-deleted failure must not be reported as an operator stop") + } + var got *controld.ErrorResponse + if !errors.As(pf.err, &got) { + t.Fatalf("error no longer unwraps to *controld.ErrorResponse: %v", pf.err) + } + if got.ErrorField.Code != controld.InvalidConfigCode { + t.Errorf("code = %d, want %d (self-uninstall would not trigger)", got.ErrorField.Code, controld.InvalidConfigCode) + } +} + +// TestPermanentAPIRejectionNarrowsToClientErrors is the regression guard for the clean +// exit added above. +// +// controld builds an *ErrorResponse for any non-200 whose body decodes, so the Go type +// says nothing about whether the API's answer will change on a retry. Keying the clean +// exit off the type alone meant a 502 from a load balancer, or an API having a bad ten +// minutes, stopped ctrld on every affected host with no service-manager retry behind it - +// worse than the abnormal exit it replaced, because a Fatal at least gets restarted. +// +// Only a client-error status may take that path. +func TestPermanentAPIRejectionNarrowsToClientErrors(t *testing.T) { + rejection := func(status, code int) error { + e := &controld.ErrorResponse{StatusCode: status} + e.ErrorField.Code = code + e.ErrorField.Message = "api said no" + return e + } + + tests := []struct { + name string + err error + wantPermanent bool + }{ + { + // The case the clean exit exists for: the device is gone, and every restart + // will be told the same thing. + name: "deleted device", + err: rejection(http.StatusNotFound, controld.InvalidConfigCode), + wantPermanent: true, + }, + {"revoked credentials", rejection(http.StatusUnauthorized, 0), true}, + {"forbidden", rejection(http.StatusForbidden, 0), true}, + {"malformed request", rejection(http.StatusBadRequest, 0), true}, + + // Server-side trouble. These must keep the abnormal exit so the service + // manager's recovery policy retries. + {"bad gateway", rejection(http.StatusBadGateway, 0), false}, + {"internal error", rejection(http.StatusInternalServerError, 0), false}, + {"service unavailable", rejection(http.StatusServiceUnavailable, 0), false}, + + // 4xx, but both are the API asking for a later attempt rather than refusing + // this configuration. + {"request timeout", rejection(http.StatusRequestTimeout, 0), false}, + {"rate limited", rejection(http.StatusTooManyRequests, 0), false}, + + // An *ErrorResponse built without a recorded status carries no verdict. A + // hand-constructed one, or a decode path that forgets to record the status, + // must not silently gain the clean exit. + {"no recorded status", rejection(0, controld.InvalidConfigCode), false}, + + // Not an API answer at all: the incident's denied socket reaches Fatal. + {"network failure", retryableNetworkErr(), false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, ok := permanentAPIRejection(tc.err) + if ok != tc.wantPermanent { + t.Errorf("permanentAPIRejection() = %v, want %v", ok, tc.wantPermanent) + } + if ok && got == nil { + t.Error("a permanent rejection must return the rejection for reporting") + } + }) + } + + // The wrapped form matters too: preflight composes the fetch error, and errors.As has + // to reach through that for either branch to be chosen correctly. + wrapped := fmt.Errorf("processCDFlags: %w", rejection(http.StatusNotFound, controld.InvalidConfigCode)) + if _, ok := permanentAPIRejection(wrapped); !ok { + t.Error("a wrapped API rejection must still be recognised") + } + wrappedTransient := fmt.Errorf("processCDFlags: %w", rejection(http.StatusBadGateway, 0)) + if _, ok := permanentAPIRejection(wrappedTransient); ok { + t.Error("a wrapped 502 must not be treated as a permanent rejection") + } +} + +func TestStopRequested(t *testing.T) { + closedCh := make(chan struct{}) + close(closedCh) + + if stopRequested(nil) { + t.Error("a nil stop channel must read as no stop (mobile passes none)") + } + if stopRequested(make(chan struct{})) { + t.Error("an open stop channel must read as no stop") + } + if !stopRequested(closedCh) { + t.Error("a closed stop channel must read as a stop") + } +} + +// TestReloadFetchIsBoundedByServiceLifetime covers the reload path's stop wiring. +// +// Reload fetches the ControlD config too, and it used to build the bounded context +// itself. Nothing tested that: the wrong channel, or a dropped cancel, would have left a +// reload retrying against an unreachable API after "Service stopped" was logged, and no +// test would have failed. Both paths now go through one bounded fetch, so this pins it. +func TestReloadFetchIsBoundedByServiceLifetime(t *testing.T) { + original := processCDFlagsFn + t.Cleanup(func() { processCDFlagsFn = original }) + + t.Run("a stop request cancels the reload fetch", func(t *testing.T) { + stopCh := make(chan struct{}) + close(stopCh) + + var sawCancelled bool + processCDFlagsFn = func(ctx context.Context, _ *ctrld.Config) (*controld.ResolverConfig, error) { + select { + case <-ctx.Done(): + sawCancelled = true + case <-time.After(2 * time.Second): + } + return nil, ctx.Err() + } + + p := &prog{stopCh: stopCh} + if _, err := p.fetchCDConfigBoundedByLifetime(&ctrld.Config{}); !errors.Is(err, context.Canceled) { + t.Errorf("reload fetch err = %v, want %v", err, context.Canceled) + } + if !sawCancelled { + t.Error("the reload fetch did not observe the stop request: it is not bound to the service lifetime") + } + }) + + t.Run("the derived context is always released", func(t *testing.T) { + // stopCh stays open: the fetch's own cancel is what must end the watcher, or + // every reload leaks a goroutine. + var captured context.Context + processCDFlagsFn = func(ctx context.Context, _ *ctrld.Config) (*controld.ResolverConfig, error) { + captured = ctx + return nil, nil + } + + p := &prog{stopCh: make(chan struct{})} + if _, err := p.fetchCDConfigBoundedByLifetime(&ctrld.Config{}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + select { + case <-captured.Done(): + case <-time.After(time.Second): + t.Error("the reload fetch left its context uncancelled") + } + }) +} diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index 47ced10..e8ed760 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -322,7 +322,7 @@ func (p *prog) runWait() { continue } if cdUID != "" { - rc, err := processCDFlags(newCfg) + rc, err := p.fetchCDConfigBoundedByLifetime(newCfg) if err != nil { p.Error().Err(err).Msg("Could not fetch controld config") waitOldRunDone() diff --git a/internal/controld/config.go b/internal/controld/config.go index df88609..f1bb861 100644 --- a/internal/controld/config.go +++ b/internal/controld/config.go @@ -61,12 +61,35 @@ type ErrorResponse struct { Message string `json:"message"` Code int `json:"code"` } `json:"error"` + // StatusCode is the HTTP status the API answered with. It is not part of the JSON + // body: this type is built for *any* non-200 whose body decodes, so the body alone + // cannot tell a permanent rejection of the request from a transient server-side + // failure, and callers that act differently on the two need the status to tell them + // apart. Zero means the status was not recorded. + StatusCode int `json:"-"` } func (u ErrorResponse) Error() string { return u.ErrorField.Message } +// apiErrorFromResponse builds the error for a non-200 API answer, recording the HTTP +// status alongside the decoded body. +// +// The status is what tells a caller whether the answer will change on a retry: this type +// is built for every non-200 whose body decodes, so a 502 from a load balancer and a 404 +// for a deleted device are otherwise indistinguishable. Both response paths go through +// here so neither can decode a body and forget to record it. +func apiErrorFromResponse(statusCode int, d *json.Decoder) (*ErrorResponse, error) { + errResp := &ErrorResponse{StatusCode: statusCode} + if err := d.Decode(errResp); err != nil { + return nil, err + } + // Decode fills exported fields from the body; StatusCode is json:"-", so it survives. + errResp.StatusCode = statusCode + return errResp, nil +} + type utilityRequest struct { UID string `json:"uid"` ClientID string `json:"client_id,omitempty"` @@ -171,7 +194,10 @@ func postUtilityAPI(ctx context.Context, version string, cdDev, lastUpdatedFaile } ctrld.Log(ctx, logger.Debug(), "Creating HTTP request") - req, err := http.NewRequest("POST", apiUrl, body) + // Context-bound so an in-flight request is abandoned when the caller is + // cancelled - a service stop during API preflight must not wait out the + // request timeout, let alone keep retrying. + req, err := http.NewRequestWithContext(ctx, "POST", apiUrl, body) if err != nil { ctrld.Log(ctx, logger.Error(), "Failed to create HTTP request: %v", err) return nil, fmt.Errorf("http.NewRequest: %w", err) @@ -206,8 +232,8 @@ func postUtilityAPI(ctx context.Context, version string, cdDev, lastUpdatedFaile ctrld.Log(ctx, logger.Debug(), "Processing API response") d := json.NewDecoder(resp.Body) if resp.StatusCode != http.StatusOK { - errResp := &ErrorResponse{} - if err := d.Decode(errResp); err != nil { + errResp, err := apiErrorFromResponse(resp.StatusCode, d) + if err != nil { ctrld.Log(ctx, logger.Error(), "Failed to decode error response: %v", err) return nil, err } @@ -237,7 +263,7 @@ func SendLogs(ctx context.Context, lr *LogsRequest, cdDev bool) error { } ctrld.Log(ctx, logger.Debug(), "Creating HTTP request for log upload") - req, err := http.NewRequest("POST", apiUrl, lr.Data) + req, err := http.NewRequestWithContext(ctx, "POST", apiUrl, lr.Data) if err != nil { ctrld.Log(ctx, logger.Error(), "Failed to create HTTP request: %v", err) return fmt.Errorf("http.NewRequest: %w", err) @@ -265,8 +291,8 @@ func SendLogs(ctx context.Context, lr *LogsRequest, cdDev bool) error { ctrld.Log(ctx, logger.Debug(), "Processing API response") d := json.NewDecoder(resp.Body) if resp.StatusCode != http.StatusOK { - errResp := &ErrorResponse{} - if err := d.Decode(errResp); err != nil { + errResp, err := apiErrorFromResponse(resp.StatusCode, d) + if err != nil { ctrld.Log(ctx, logger.Error(), "Failed to decode error response: %v", err) return err } diff --git a/internal/controld/config_test.go b/internal/controld/config_test.go index b266142..5973de0 100644 --- a/internal/controld/config_test.go +++ b/internal/controld/config_test.go @@ -1,6 +1,9 @@ package controld import ( + "encoding/json" + "net/http" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -29,3 +32,67 @@ func Test_parseUID(t *testing.T) { }) } } + +// TestAPIErrorRecordsHTTPStatus pins the plumbing the caller's exit decision rests on. +// +// cmd/cli treats a 4xx as "this configuration is refused, restarting cannot help" and +// exits cleanly, while a 5xx keeps the abnormal exit so the service manager retries. Both +// readings need the status, and it is not in the JSON body - so a decode path that +// forgets to record it would quietly send every API error down the retry branch, +// including a deleted device that should self-uninstall and stop. +func TestAPIErrorRecordsHTTPStatus(t *testing.T) { + tests := []struct { + name string + statusCode int + body string + wantCode int + wantMsg string + }{ + { + name: "deleted device", + statusCode: http.StatusNotFound, + body: `{"error":{"message":"device does not exist","code":40402}}`, + wantCode: InvalidConfigCode, + wantMsg: "device does not exist", + }, + { + // A gateway error body carries no error object at all, which decodes + // cleanly into the zero value - so the status is the only thing that + // distinguishes it from a real rejection. + name: "gateway error with an empty body", + statusCode: http.StatusBadGateway, + body: `{}`, + }, + { + name: "service unavailable", + statusCode: http.StatusServiceUnavailable, + body: `{"error":{"message":"try again later","code":0}}`, + wantMsg: "try again later", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + d := json.NewDecoder(strings.NewReader(tc.body)) + errResp, err := apiErrorFromResponse(tc.statusCode, d) + if err != nil { + t.Fatalf("unexpected decode error: %v", err) + } + if errResp.StatusCode != tc.statusCode { + t.Errorf("StatusCode = %d, want %d: the caller cannot tell a permanent rejection from a transient failure without it", errResp.StatusCode, tc.statusCode) + } + if errResp.ErrorField.Code != tc.wantCode { + t.Errorf("code = %d, want %d", errResp.ErrorField.Code, tc.wantCode) + } + if errResp.Error() != tc.wantMsg { + t.Errorf("message = %q, want %q", errResp.Error(), tc.wantMsg) + } + }) + } + + t.Run("an undecodable body is reported as a decode failure", func(t *testing.T) { + d := json.NewDecoder(strings.NewReader("502 Bad Gateway")) + if _, err := apiErrorFromResponse(http.StatusBadGateway, d); err == nil { + t.Error("expected a decode error for a non-JSON body") + } + }) +}