amadan
fleet bindings: same-box clock guard, fleet-prefixed tunnel name required
Fix round 2, addressing 2 new Importants from Opus re-review of the previous fix round: - bindingBlocked's BoundAt guard now only arbitrates a box against its OWN past writes (existing.Box == b.Box). BoundAt is each box's own wall clock, not a shared one — comparing it across boxes was comparing skew, not recency, and could have permanently wedged a surviving box's lease-backed takeover behind a dead box's clock-ahead stale binding (the startup pass can't help either, since it's deliberately non-stealing). - resolveFleetIdentity's CARLOS_TUNNEL_NAME fallback now requires the "<fleet>/" prefix (strings.CutPrefix, not TrimPrefix) and refuses loudly when it's absent, instead of silently accepting the bare legacy tunnel name as the label — a binding published under that label is one the edge can never dial (fleetChannelFor's rule 1 would check a channel name the tunnel hub never actually presents).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 files changed,
+109
−14
internal/host/fleetbind.go+18 −5internal/host/fleetbind_test.go+33 −0internal/host/fleetidentity.go+20 −9internal/host/fleetidentity_test.go+38 −0
diff --git a/internal/host/fleetbind.go b/internal/host/fleetbind.go| index 9c03b3f6c..adbed21e0 100644 |
| --- a/internal/host/fleetbind.go |
| +++ b/internal/host/fleetbind.go |
| @@ -70,12 +70,25 @@ func fleetBindingKey(fleet, host string) string { |
| // (takeover=true) may take over — it is backed by having just won the |
| // per-host S3 lease, which is the actual arbiter of who may serve a |
| // host, so it is allowed to restamp regardless of who was there before. |
| -// - Logical clock: a binding with a strictly greater BoundAt already |
| -// stored must never be overwritten by an older one — this holds |
| -// regardless of takeover, and fixes a physical-write-race hazard: two |
| -// concurrent Puts to the same key (a retried wake publish racing its own |
| +// - Logical clock, SAME BOX ONLY: a binding already stored FROM THE SAME |
| +// BOX with a strictly greater BoundAt must never be overwritten by an |
| +// older write from that same box — this holds regardless of takeover, |
| +// and fixes a physical-write-race hazard: two concurrent Puts to the |
| +// same key from one box (a retried wake publish racing its own |
| // predecessor, or any other overlap) must not let whichever one's HTTP |
| // round-trip happens to land second win just because it arrived later. |
| +// Scoped to existing.Box == b.Box deliberately (fix round 2, Opus |
| +// review): BoundAt is each BOX's OWN wall clock, never a shared one — |
| +// comparing it across two DIFFERENT boxes compares clock skew, not |
| +// recency. Before this scoping, a dead box A's stale binding stamped |
| +// with a clock running ahead of surviving box B's would silently and |
| +// PERMANENTLY block B's lease-backed takeover write (the startup pass |
| +// can't help either — it's non-stealing by the guard above), stranding |
| +// the edge on its sorted-fallback channel instead of the real lease |
| +// holder for as long as the skew persists. A cross-box binding is |
| +// already fully governed by the non-stealing guard above (blocked |
| +// unless takeover=true); the clock only ever needs to arbitrate a BOX |
| +// against its own past self. |
| func bindingBlocked(existing *FleetBinding, b FleetBinding, takeover bool) bool { |
| if existing == nil { |
| return false |
| @@ -83,7 +96,7 @@ func bindingBlocked(existing *FleetBinding, b FleetBinding, takeover bool) bool |
| if !takeover && existing.Box != b.Box { |
| return true |
| } |
| - return existing.BoundAt > b.BoundAt |
| + return existing.Box == b.Box && existing.BoundAt > b.BoundAt |
| } |
| // getExistingBinding reads key's current binding + etag for a replace-only |
diff --git a/internal/host/fleetbind_test.go b/internal/host/fleetbind_test.go| index a8854b727..ea8c24e57 100644 |
| --- a/internal/host/fleetbind_test.go |
| +++ b/internal/host/fleetbind_test.go |
| @@ -183,6 +183,39 @@ func TestWriteFleetBindingTakeoverClaimsFromAnotherBox(t *testing.T) { |
| } |
| } |
| +// Fix round 2 (Opus re-review): the logical-clock guard must be scoped to |
| +// the SAME box only — BoundAt is each box's own wall clock, never a shared |
| +// one. A dead box A's binding stamped with a clock running ahead of |
| +// surviving box B's must not permanently block B's lease-backed takeover |
| +// write: box-a's stale binding here carries a BoundAt in the FUTURE |
| +// relative to box-b's claim, and the takeover must still go through, |
| +// because a cross-box BoundAt comparison is comparing skew, not recency. |
| +func TestWriteFleetBindingTakeoverIgnoresOtherBoxsFutureBoundAt(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + stale := FleetBinding{Host: "one.example.com", Fleet: "pi", Box: "box-a", BoundAt: 9_000_000_000_000} |
| + if err := WriteFleetBinding(ctx, s, stale, true); err != nil { |
| + t.Fatalf("seed box-a's (clock-ahead) binding: %v", err) |
| + } |
| + |
| + claim := FleetBinding{Host: "one.example.com", Fleet: "pi", Box: "box-b", BoundAt: 1_000_000_000_000} |
| + if err := WriteFleetBinding(ctx, s, claim, true); err != nil { |
| + t.Fatalf("takeover write: %v", err) |
| + } |
| + |
| + body, _, err := s.Get(ctx, "fleets/pi/bindings/one.example.com.json") |
| + if err != nil { |
| + t.Fatalf("Get: %v", err) |
| + } |
| + var got FleetBinding |
| + if err := json.Unmarshal(body, &got); err != nil { |
| + t.Fatalf("unmarshal: %v", err) |
| + } |
| + if got.Box != "box-b" { |
| + t.Fatalf("Box = %q, want box-b — a lease-backed takeover must not be blocked by a DIFFERENT box's clock running ahead", got.Box) |
| + } |
| +} |
| + |
| // Important 5 (Opus review, fix round): a stored binding with a strictly |
| // greater BoundAt must never be overwritten by an older one, even by the |
| // SAME box, and even with takeover=true — a physical write race must not |
diff --git a/internal/host/fleetidentity.go b/internal/host/fleetidentity.go| index d121c8ce6..3018b93b1 100644 |
| --- a/internal/host/fleetidentity.go |
| +++ b/internal/host/fleetidentity.go |
| @@ -64,16 +64,27 @@ func resolveFleetIdentity(getenv func(string) string) (FleetIdentity, error) { |
| // A fleet box's CARLOS_TUNNEL_NAME is minted as "<fleet>/<label>" |
| // (plan doc: "FleetChannelAuth: channel name '<fleet>/<label>'"), so |
| // stripping this fleet's own prefix recovers the label without a |
| - // second env var in the common case. TrimPrefix no-ops when |
| - // CARLOS_TUNNEL_NAME is unset (label stays "" — caught below) or |
| - // doesn't carry this exact prefix; ValidTunnelName below is the |
| - // backstop, but note it cannot detect every malformed input this way |
| - // (a CARLOS_TUNNEL_NAME from a WRONG fleet, or one with no "/" at |
| - // all, can still leave behind a string that happens to validate) — |
| - // CARLOS_FLEET_BOX is the reliable path for anything but the |
| - // standard provisioning shape. |
| + // second env var in the common case. CutPrefix — not TrimPrefix — is |
| + // load-bearing here (fix round 2, Opus re-review): TrimPrefix |
| + // no-ops when CARLOS_TUNNEL_NAME doesn't actually carry this exact |
| + // prefix, silently leaving the WHOLE tunnel name behind as the |
| + // "label" — and a bare legacy tunnel name (CARLOS_TUNNEL_NAME= |
| + // "pi-box", no "/" at all — the live shape in today's runbook) |
| + // still passes ValidTunnelName, so that silent fallback would have |
| + // published a Box the edge could never dial: fleetChannelFor's rule |
| + // 1 checks hub.State(fleet+"/"+boxLabel), but the hub only ever |
| + // knows this box's channel by its RAW CARLOS_TUNNEL_NAME — "pi-box", |
| + // not "pi/pi-box". A fleet box's channel MUST be "<fleet>/<label>" |
| + // for FleetChannelAuth to key it at all, so a tunnel name missing |
| + // that exact prefix is refused here rather than silently accepted. |
| tunnelName := strings.TrimSpace(getenv("CARLOS_TUNNEL_NAME")) |
| - label = strings.TrimPrefix(tunnelName, fleet+"/") |
| + derived, ok := strings.CutPrefix(tunnelName, fleet+"/") |
| + if !ok { |
| + return FleetIdentity{}, fmt.Errorf( |
| + "fleet box label could not be derived from CARLOS_TUNNEL_NAME=%q (want the %q prefix) — set CARLOS_FLEET_BOX explicitly", |
| + tunnelName, fleet+"/") |
| + } |
| + label = derived |
| } |
| if label == "" || !registry.ValidTunnelName(label) { |
| return FleetIdentity{}, fmt.Errorf( |
diff --git a/internal/host/fleetidentity_test.go b/internal/host/fleetidentity_test.go| index f55ec6580..471cb68c7 100644 |
| --- a/internal/host/fleetidentity_test.go |
| +++ b/internal/host/fleetidentity_test.go |
| @@ -64,6 +64,44 @@ func TestResolveFleetIdentityExplicitBoxLabelWinsOverTunnelName(t *testing.T) { |
| } |
| } |
| +// Fix round 2 (Opus re-review): a bare legacy tunnel name — the LIVE shape |
| +// in today's runbook, e.g. CARLOS_TUNNEL_NAME="pi-box" with no "<fleet>/" |
| +// prefix at all — must be refused LOUDLY when no CARLOS_FLEET_BOX is set, |
| +// not silently accepted as the label. Before this fix, TrimPrefix would |
| +// no-op (the string doesn't start with "pi/") and leave "pi-box" itself as |
| +// the "label" — which still passes ValidTunnelName, so it slipped through |
| +// silently. That would have published a Box the edge could never dial: |
| +// fleetChannelFor's rule 1 checks hub.State("pi" + "/" + "pi-box"), but the |
| +// tunnel hub only ever knows this box's channel by its raw |
| +// CARLOS_TUNNEL_NAME, "pi-box" — never "pi/pi-box". |
| +func TestResolveFleetIdentityRefusesBareLegacyTunnelNameWithoutFleetPrefix(t *testing.T) { |
| + _, err := resolveFleetIdentity(envFunc(map[string]string{ |
| + "CARLOS_FLEET": "pi", |
| + "CARLOS_TUNNEL_NAME": "pi-box", // no "pi/" prefix — the legacy shape |
| + })) |
| + if err == nil || errors.Is(err, errNotFleetBox) { |
| + t.Fatalf("err = %v, want a loud, non-errNotFleetBox refusal", err) |
| + } |
| +} |
| + |
| +// An explicit CARLOS_FLEET_BOX must still work regardless of |
| +// CARLOS_TUNNEL_NAME's shape — including the bare legacy form the previous |
| +// test refuses to derive a label from. The explicit override never goes |
| +// anywhere near the prefix-stripping path. |
| +func TestResolveFleetIdentityExplicitBoxLabelWorksWithLegacyTunnelName(t *testing.T) { |
| + id, err := resolveFleetIdentity(envFunc(map[string]string{ |
| + "CARLOS_FLEET": "pi", |
| + "CARLOS_FLEET_BOX": "amber-gliding", |
| + "CARLOS_TUNNEL_NAME": "pi-box", // legacy shape; must be ignored entirely |
| + })) |
| + if err != nil { |
| + t.Fatalf("resolveFleetIdentity: %v", err) |
| + } |
| + if id.Label != "amber-gliding" { |
| + t.Fatalf("Label = %q, want amber-gliding (explicit CARLOS_FLEET_BOX must work regardless of tunnel-name form)", id.Label) |
| + } |
| +} |
| + |
| func TestResolveFleetIdentityInvalidFleetNameRefuses(t *testing.T) { |
| _, err := resolveFleetIdentity(envFunc(map[string]string{ |
| "CARLOS_FLEET": "Not Valid!", |