amadan
fleetapi: fix Delete opacity and artifact-download timeout (review C1/C2)
C1: Delete no longer folds a 404 into success. The door answers 204 for every in-scope key, present or absent — that's where Delete's idempotency lives. A 404 is only ever a refusal (bad credential, bad key shape, ACL, misrouted base), never "already gone", so it must surface as an error. C2: replaced http.Client.Timeout (a whole-exchange wall clock) with a Transport carrying dial/TLS/ResponseHeaderTimeout at 30s and no overall deadline. Get is the artifact-download path release adoption uses; a whole-body 30s cap would fail every large-artifact pull on the home-broadband/Pi class of box this spec targets, deterministically, every adoption pass. A wedged edge that never starts answering still fails fast. Added TestDeleteWithWrongCredentialErrors (own + another tenant's key) and a white-box TestClientHasNoWholeExchangeTimeoutButBoundsResponseHeaders pinning the Transport construction.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 files changed,
+108
−21
internal/fleetapi/fleetapi.go+41 −21internal/fleetapi/fleetapi_internal_test.go+39 −0internal/fleetapi/fleetapi_test.go+28 −0
diff --git a/internal/fleetapi/fleetapi.go b/internal/fleetapi/fleetapi.go| index dd1baddb6..73f1e0402 100644 |
| --- a/internal/fleetapi/fleetapi.go |
| +++ b/internal/fleetapi/fleetapi.go |
| @@ -17,6 +17,7 @@ import ( |
| "errors" |
| "fmt" |
| "io" |
| + "net" |
| "net/http" |
| "net/url" |
| "os" |
| @@ -26,10 +27,18 @@ import ( |
| "github.com/carlosframework/platform/internal/release" |
| ) |
| -// callTimeout bounds every single HTTP call this store makes (spec: 30s per |
| -// call). It is set on the *http.Client itself rather than derived from the |
| -// caller's context, so a call this box makes with no context deadline of its |
| -// own still cannot hang forever against a wedged edge. |
| +// callTimeout bounds connect, TLS handshake, and the wait for the response |
| +// headers to start arriving — the "is the edge even alive and answering" |
| +// phase of a call. It deliberately does NOT bound the rest of the exchange |
| +// (reading/writing the body): Get is the artifact-download path (release |
| +// adoption fetches whole app.tar.gz bodies through this store), and this |
| +// spec's own target box class — home broadband, a Pi's uplink — can take |
| +// well over 30s to pull a large artifact while still making steady |
| +// progress. Putting this on http.Client.Timeout instead (a whole-exchange |
| +// wall clock) would fail every such transfer deterministically, on every |
| +// adoption pass, forever; S3Store has no equivalent cap, so this store must |
| +// not invent a stricter one. A wedged edge that never starts answering still |
| +// fails fast — that's what ResponseHeaderTimeout below is for. |
| const callTimeout = 30 * time.Second |
| // store is the client half of the door: release.Store spoken over base's |
| @@ -62,10 +71,22 @@ func New(base, name, token string) (release.Store, error) { |
| return nil, fmt.Errorf("fleetapi: invalid base %q: want an absolute URL", base) |
| } |
| return &store{ |
| - base: strings.TrimSuffix(base, "/"), |
| - name: name, |
| - token: token, |
| - client: &http.Client{Timeout: callTimeout}, |
| + base: strings.TrimSuffix(base, "/"), |
| + name: name, |
| + token: token, |
| + // No Client.Timeout (see callTimeout's comment): only the connect/TLS/ |
| + // response-header phase is bounded, so a slow-but-progressing artifact |
| + // transfer can still finish. |
| + client: &http.Client{Transport: &http.Transport{ |
| + Proxy: http.ProxyFromEnvironment, |
| + DialContext: (&net.Dialer{ |
| + Timeout: callTimeout, |
| + KeepAlive: 30 * time.Second, |
| + }).DialContext, |
| + TLSHandshakeTimeout: callTimeout, |
| + ResponseHeaderTimeout: callTimeout, |
| + ExpectContinueTimeout: 1 * time.Second, |
| + }}, |
| }, nil |
| } |
| @@ -168,13 +189,14 @@ func (s *store) Put(ctx context.Context, key string, body []byte, ifMatch string |
| } |
| } |
| -// Delete is idempotent, matching release.Store's contract (and the door's: |
| -// storedoor.go always answers 204 for an in-scope key, deleted or not). A |
| -// 404 is folded into success too: the door's opacity means "out of scope" |
| -// and "already gone" are the SAME response by design (spec's central |
| -// security invariant), and the Store contract only ever asks Delete to be |
| -// idempotent — it has no notion of "delete refused", so there is nothing |
| -// more honest to report here than success. |
| +// Delete is idempotent, matching release.Store's contract: the door |
| +// (storedoor.go) always answers 204 for an in-scope key, deleted or not, |
| +// which is where the idempotency lives — NOT in this client folding a 404 |
| +// into success. A 404 here is only ever a refusal (bad token, bad name, an |
| +// out-of-scope key, a malformed key, a misrouted base): it is never "already |
| +// gone", because "already gone" is exactly the case the door answers 204. |
| +// Treating it as success would make a mis-credentialled client report every |
| +// delete as having worked. |
| func (s *store) Delete(ctx context.Context, key string) error { |
| req, err := s.newRequest(ctx, http.MethodDelete, s.objectURL(key), nil) |
| if err != nil { |
| @@ -185,13 +207,11 @@ func (s *store) Delete(ctx context.Context, key string) error { |
| return errorf("delete %s: %w", key, err) |
| } |
| defer resp.Body.Close() |
| - io.Copy(io.Discard, resp.Body) //nolint:errcheck |
| - switch resp.StatusCode { |
| - case http.StatusNoContent, http.StatusNotFound: |
| - return nil |
| - default: |
| - return errorf("delete %s: unexpected status %d", key, resp.StatusCode) |
| + respBody, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) |
| + if resp.StatusCode != http.StatusNoContent { |
| + return errorf("delete %s: unexpected status %d: %s", key, resp.StatusCode, strings.TrimSpace(string(respBody))) |
| } |
| + return nil |
| } |
| // List is the door's /store/list — see listDecode. Note the spec's callout: |
diff --git a/internal/fleetapi/fleetapi_internal_test.go b/internal/fleetapi/fleetapi_internal_test.go| new file mode 100644 |
| index 000000000..ea1dada3e |
| --- /dev/null |
| +++ b/internal/fleetapi/fleetapi_internal_test.go |
| @@ -0,0 +1,39 @@ |
| +package fleetapi |
| + |
| +// White-box (package fleetapi, not fleetapi_test): pins the transport |
| +// construction New() does, which no exported surface exposes — C2's fix |
| +// shape (a Transport with dial/TLS/response-header timeouts and no |
| +// Client.Timeout) is otherwise invisible to a caller until an actual slow |
| +// transfer either survives or doesn't. |
| + |
| +import ( |
| + "net/http" |
| + "testing" |
| +) |
| + |
| +func TestClientHasNoWholeExchangeTimeoutButBoundsResponseHeaders(t *testing.T) { |
| + s, err := New("https://tunnel.example.com", "pi/box-a", "tok") |
| + if err != nil { |
| + t.Fatalf("New: %v", err) |
| + } |
| + st, ok := s.(*store) |
| + if !ok { |
| + t.Fatalf("New returned %T, want *store", s) |
| + } |
| + if st.client.Timeout != 0 { |
| + t.Errorf("client.Timeout = %v, want 0 (a whole-exchange deadline would fail slow-but-progressing artifact downloads)", st.client.Timeout) |
| + } |
| + tr, ok := st.client.Transport.(*http.Transport) |
| + if !ok { |
| + t.Fatalf("client.Transport = %T, want *http.Transport", st.client.Transport) |
| + } |
| + if tr.ResponseHeaderTimeout != callTimeout { |
| + t.Errorf("ResponseHeaderTimeout = %v, want %v (a wedged edge must still fail fast)", tr.ResponseHeaderTimeout, callTimeout) |
| + } |
| + if tr.TLSHandshakeTimeout != callTimeout { |
| + t.Errorf("TLSHandshakeTimeout = %v, want %v", tr.TLSHandshakeTimeout, callTimeout) |
| + } |
| + if tr.DialContext == nil { |
| + t.Error("DialContext = nil, want a dialer with a connect timeout set") |
| + } |
| +} |
diff --git a/internal/fleetapi/fleetapi_test.go b/internal/fleetapi/fleetapi_test.go| index 1584a0bcc..4956c9b18 100644 |
| --- a/internal/fleetapi/fleetapi_test.go |
| +++ b/internal/fleetapi/fleetapi_test.go |
| @@ -275,6 +275,34 @@ func TestDeleteIsIdempotent(t *testing.T) { |
| } |
| } |
| +// TestDeleteWithWrongCredentialErrors pins C1: a 404 on DELETE is only ever |
| +// a refusal (the door answers 204 for every in-scope key, present or |
| +// absent — that's where Delete's idempotency lives). A mis-credentialled |
| +// client must see an error, not silent "success" for a delete that never |
| +// happened. |
| +func TestDeleteWithWrongCredentialErrors(t *testing.T) { |
| + d := newTestDoor(t) |
| + key := "fleets/" + testFleet + "/status/boxes/" + testLabel + ".json" |
| + seed(t, d.store, key, "{}") |
| + |
| + wrongToken := d.client(t, testFleet+"/"+testLabel, "not-the-real-token") |
| + if err := wrongToken.Delete(context.Background(), key); err == nil { |
| + t.Fatal("Delete with wrong token = nil error, want an error (the key must still exist)") |
| + } |
| + if _, _, err := d.box(t).Get(context.Background(), key); err != nil { |
| + t.Fatalf("the refused Delete removed the object anyway: %v", err) |
| + } |
| + |
| + otherAccountKey := "apps/zz99/web/channels/stable.json" |
| + seed(t, d.store, otherAccountKey, "someone else's") |
| + if err := d.box(t).Delete(context.Background(), otherAccountKey); err == nil { |
| + t.Fatal("Delete of another tenant's key = nil error, want an error") |
| + } |
| + if _, _, err := d.store.Get(context.Background(), otherAccountKey); err != nil { |
| + t.Fatalf("the refused Delete removed another tenant's object anyway: %v", err) |
| + } |
| +} |
| + |
| // ---- Auth failure is opaque, matching the door ----------------------- |
| // TestAuthFailureSurfacesAsNotFound pins the door's central opacity |