mirror of
https://github.com/Control-D-Inc/ctrld.git
synced 2026-09-04 13:36:35 +02:00
Cuong Manh Le
4f730167d4
processCDFlags retries the resolver-config fetch indefinitely by design: a device that has no working network at boot must eventually come up. The loop had no cancellation, so a stop request arriving while the API is unreachable was ignored - the process kept retrying long after the service reported itself stopped, doing work on behalf of a service the OS considers stopped. Thread a context through processCDFlags and derive it from p.stopCh, in both the startup preflight and the config-reload path. The loop now returns as soon as the context is cancelled, checked both before a retry and after backoff returns (backoff can wake up on cancellation). A stop during preflight exits the way a normal stop does, without Fatal, so the service manager does not treat it as a failed start and apply its restart policy to a service the operator just asked to stop. Bind the two API requests themselves as well, so a stop does not have to wait out an in-flight request. Without this the loop honours a stop only between attempts, which leaves up to defaultTimeout (20s) of a request the service is no longer interested in - the same "still working after Service stopped" the loop change exists to end, one layer down. Doing so means a context parameter on FetchResolverConfig, FetchResolverUID, UpdateCustomLastFailed and SendLogs, since all four reach a request builder. The callers that have no context pass context.Background(), which is what master effectively does at those sites: its loggerCtx carries a logger, not cancellation. doWithFallback needs no parameter, because it clones the request with req.Context() and so inherits the binding. This also repairs internal/controld/controld_test.go, which is behind //go:build controld and had already been written against the context-taking signature, so it could not compile. Cover the cancellation paths; removing either check makes the tests hang until timeout. Sampling the stop state is the whole point of runAPIPreflight rather than doing this inline. A stop and a failure need opposite handling - one exits quietly, the other self-uninstalls a deleted device, surfaces the error to a mobile app, and reports a failed start - so the two must not be confused. Reading it from the context after cancelling would report "stopped" for every failure, since CancelFunc sets ctx.Err() regardless of whether anyone asked to stop; the stop channel is read directly instead, which also does not depend on the context watcher goroutine having been scheduled.
99 lines
3.0 KiB
Go
99 lines
3.0 KiB
Go
package controld
|
|
|
|
import (
|
|
"encoding/json"
|
|
"net/http"
|
|
"strings"
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
)
|
|
|
|
func Test_parseUID(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
uid string
|
|
wantUID string
|
|
wantClientID string
|
|
}{
|
|
{"empty", "", "", ""},
|
|
{"only uid", "abcd1234", "abcd1234", ""},
|
|
{"with client id", "abcd1234/clientID", "abcd1234", "clientID"},
|
|
{"with empty clientID", "abcd1234/", "abcd1234", ""},
|
|
}
|
|
|
|
for _, tc := range tests {
|
|
tc := tc
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
t.Parallel()
|
|
gotUID, gotClientID := ParseRawUID(tc.uid)
|
|
assert.Equal(t, tc.wantUID, gotUID)
|
|
assert.Equal(t, tc.wantClientID, gotClientID)
|
|
})
|
|
}
|
|
}
|
|
|
|
// 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("<html>502 Bad Gateway</html>"))
|
|
if _, err := apiErrorFromResponse(http.StatusBadGateway, d); err == nil {
|
|
t.Error("expected a decode error for a non-JSON body")
|
|
}
|
|
})
|
|
}
|