From bcd3361bed977130c96945bfc60e52b9d2ed2a37 Mon Sep 17 00:00:00 2001 From: Karn Date: Mon, 10 Aug 2026 01:55:26 +0530 Subject: [PATCH] fix(relayui): trust our own deploy over a health read the edge has not caught up with MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A deploy returns when Cloudflare's API accepts the new Worker, but the edge keeps serving the previous one for seconds — occasionally longer. During that window the relay's /api/health still answers with the old FLUE_VERSION stamp, so Status(), which compared deployStamp() against that live read, put the "update the relay" card right back under the checkmarks the deploy just earned. The user obliges and redeploys identical bytes; time was the actual fix. The daemon knows what it just shipped — stop throwing that away. relayUIService now remembers (origin, stamp) after a successful Provision or Update, and Status answers from that memory instead of the health read, but only while the memory stays true: a failed deploy sets nothing, a binary whose own deployStamp changed falls back to the health read so a genuinely newer build still gets its card, and the memory is keyed to the origin it shipped to, so SetAddress (or a re-join under a running daemon) self-invalidates it. A daemon restart drops the memory, deliberately — propagation is long done by then. The CLI paths (flue relay setup / update) run in their own process, print, and exit; they never serve Status, so they need no memory. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/flue/relayui.go | 64 +++++++++++++++++- cmd/flue/relayui_test.go | 134 +++++++++++++++++++++++++++++++++++++ internal/daemon/relayui.go | 10 ++- 3 files changed, 203 insertions(+), 5 deletions(-) diff --git a/cmd/flue/relayui.go b/cmd/flue/relayui.go index ee9ccb3..50fc08f 100644 --- a/cmd/flue/relayui.go +++ b/cmd/flue/relayui.go @@ -49,6 +49,21 @@ type relayUIService struct { // log hears what the deploy did — worker names and outcomes, never a // token and never the secret. Nil is quiet, which is what tests want. log *slog.Logger + + // shippedOrigin and shippedStamp remember the last deploy this process + // completed: where it shipped and what stamp it shipped. They exist for + // the minutes right after a deploy — Cloudflare's API accepts a new + // Worker before every edge serves it, and until propagation finishes the + // relay's /api/health still answers with the previous stamp. A Status + // that trusted only the health read would put the update card back under + // the checkmarks the deploy just earned, and taking that offer redeploys + // identical bytes. Their own mutex, not mu: Status must render during a + // deploy, not queue behind one. Lost on a daemon restart, deliberately — + // by the time a daemon comes back, propagation is long done and the + // health read is telling the truth again. + shippedMu sync.Mutex + shippedOrigin string + shippedStamp string } func (s *relayUIService) logf() *slog.Logger { @@ -140,7 +155,17 @@ func (s *relayUIService) Status(ctx context.Context) daemon.RelayUIStatus { if w, err := updateWorkerName("", cfg); err == nil { st.Worker = w } - st.DeployedVersion = deployedVersion(ctx, cfg.Origin) + // This process's own deploy outranks the health read: right after one, + // the edge can keep serving the previous Worker — previous stamp and + // all — for seconds, occasionally longer, and believing /api/health in + // that window turns a finished deploy into an update offer. The memory + // declines to answer whenever it has gone stale (see shippedVersion), + // and the health read decides as before. + if stamp, ok := s.shippedVersion(cfg.Origin); ok { + st.DeployedVersion = stamp + } else { + st.DeployedVersion = deployedVersion(ctx, cfg.Origin) + } return st } @@ -171,6 +196,31 @@ func deployedVersion(ctx context.Context, origin string) string { return health.Version } +// recordShipped is the deploy paths' success line: this process just put its +// own bytes behind origin, whatever /api/health says for the next while. A +// deploy that failed must never reach here — it changed nothing at the edge, +// and the card has to keep offering what the health read supports. +func (s *relayUIService) recordShipped(origin string) { + s.shippedMu.Lock() + defer s.shippedMu.Unlock() + s.shippedOrigin, s.shippedStamp = origin, deployStamp() +} + +// shippedVersion answers Status from memory, but only while the memory still +// speaks for the question being asked: the same origin, carrying the stamp +// this binary would ship again. Anything else is stale and must lose to a +// live health read — a relay.json repointed or re-joined elsewhere names a +// deploy this process never performed, and a binary whose own stamp moved on +// has a genuinely newer build whose update card must not be swallowed. +func (s *relayUIService) shippedVersion(origin string) (string, bool) { + s.shippedMu.Lock() + defer s.shippedMu.Unlock() + if s.shippedStamp == "" || s.shippedOrigin != origin || s.shippedStamp != deployStamp() { + return "", false + } + return s.shippedStamp, true +} + // resolveToken decides what credential a deploy runs with: the request's // token when one was typed, the stored one otherwise, a refusal when there is // neither. It reports whether the token came from the request — the ones that @@ -328,6 +378,10 @@ func (s *relayUIService) Provision(ctx context.Context, req daemon.RelayUIDeploy } else if started { res.Steps = append(res.Steps, "daemon connecting to the relay") } + // Remember what shipped and where: Status answers from this while the + // edge catches up, instead of trusting a health read that briefly still + // says the previous deploy. + s.recordShipped(res.Origin) s.logf().Info("relay deployed from the UI", "worker", worker, "host", host, "restartNeeded", res.RestartNeeded) return res, nil } @@ -345,7 +399,10 @@ func joinCommand(host, secret string) string { // any existing pairing — the daemon serves exactly the origin it dials, so // every browser paired on the old one must pair again on the new address // (see runRelayAddress), and the second step line carries that truth to the -// card. +// card. The shipped-deploy memory keys on the origin, so a repoint sends +// Status back to asking the relay itself: the Worker behind the new name +// should be the same one, but that is the health read's fact to confirm, +// not memory's to assume. func (s *relayUIService) SetAddress(ctx context.Context, address string) (daemon.RelayUIDeployResult, error) { s.mu.Lock() defer s.mu.Unlock() @@ -458,6 +515,9 @@ func (s *relayUIService) Update(ctx context.Context, req daemon.RelayUIDeployReq steps = append(steps, "token stored for one-click updates") } } + // Same note as Provision's: the deploy succeeded, so Status may say so + // without waiting for the edge to agree. + s.recordShipped(cfg.Origin) s.logf().Info("relay updated from the UI", "worker", worker) return daemon.RelayUIDeployResult{Steps: steps, Origin: cfg.Origin}, nil } diff --git a/cmd/flue/relayui_test.go b/cmd/flue/relayui_test.go index 7a0af40..4b60f4f 100644 --- a/cmd/flue/relayui_test.go +++ b/cmd/flue/relayui_test.go @@ -218,6 +218,140 @@ func TestRelayUIStatusReportsTheDeployedVersion(t *testing.T) { } } +// staleHealth is a relay edge mid-propagation: whatever was just deployed, +// /api/health still answers with the previous deploy's stamp — which is what +// a real relay does for seconds, occasionally longer, after every deploy. +func staleHealth(t *testing.T) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/health" { + http.NotFound(w, r) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{"ok": true, "version": "0.0.0-previous"}) + })) + t.Cleanup(srv.Close) + return srv +} + +// seedRelayAt writes the relay.json of a machine already joined to a relay +// whose origin is the given server — the state the update card renders from. +func seedRelayAt(t *testing.T, origin string) { + t.Helper() + if err := config.SaveRelay(config.Relay{ + URL: "wss://flue-relay.karn.workers.dev", + Secret: "s", + Origin: origin, + MachineID: "m-1", + Worker: relayScriptName, + }); err != nil { + t.Fatalf("SaveRelay: %v", err) + } +} + +// TestRelayUIStatusTrustsItsOwnDeployWhileTheEdgeCatchesUp is the repro that +// earned the service its memory: deploy, watch every checkmark land, and the +// card underneath still says "update the relay" — because Cloudflare's API +// accepted the new Worker while the edge kept serving the old one, previous +// stamp and all, and Status believed the edge. The user obliges and +// redeploys identical bytes; time was the actual fix. After a successful +// deploy the service's own memory answers instead. +func TestRelayUIStatusTrustsItsOwnDeployWhileTheEdgeCatchesUp(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + f := newFakeCloudflare(t, oneAccount(), "karn") + stale := staleHealth(t) + seedRelayAt(t, stale.URL) + svc := uiService(f, &relayRuntime{running: true}) + + // Before this process has deployed anything, the health read decides — + // its differing stamp is what puts the update card up at all. + if st := svc.Status(context.Background()); st.DeployedVersion != "0.0.0-previous" { + t.Fatalf("deployed version before deploying = %q, want the health read's 0.0.0-previous", st.DeployedVersion) + } + + if _, err := svc.Update(context.Background(), daemon.RelayUIDeployRequest{Token: setupToken}); err != nil { + t.Fatalf("Update: %v", err) + } + + // The edge still answers the previous stamp; Status must not believe it. + if st := svc.Status(context.Background()); st.DeployedVersion != deployStamp() { + t.Fatalf("deployed version right after a successful deploy = %q, want this binary's %q", st.DeployedVersion, deployStamp()) + } + + // Provision remembers the same way: a first deploy's Status answers from + // memory too, without dialling the fresh workers.dev origin at all. + if _, err := svc.Provision(context.Background(), daemon.RelayUIDeployRequest{Token: setupToken}); err != nil { + t.Fatalf("Provision: %v", err) + } + if st := svc.Status(context.Background()); st.DeployedVersion != deployStamp() { + t.Fatalf("deployed version right after a provision = %q, want %q", st.DeployedVersion, deployStamp()) + } +} + +// TestRelayUIFailedUpdateLeavesTheHealthReadInCharge: a deploy the API +// refused changed nothing at the edge, so it earns no memory — the card +// keeps offering exactly what the health read supports. +func TestRelayUIFailedUpdateLeavesTheHealthReadInCharge(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + f := newFakeCloudflare(t, oneAccount(), "karn") + f.reject["/scripts/"+relayScriptName] = "computer says no" + stale := staleHealth(t) + seedRelayAt(t, stale.URL) + svc := uiService(f, &relayRuntime{running: true}) + + if _, err := svc.Update(context.Background(), daemon.RelayUIDeployRequest{Token: setupToken}); err == nil { + t.Fatal("the rejected deploy reported success") + } + if st := svc.Status(context.Background()); st.DeployedVersion != "0.0.0-previous" { + t.Fatalf("deployed version after a failed deploy = %q, want the health read's 0.0.0-previous", st.DeployedVersion) + } +} + +// TestRelayUIStaleShipMemoryLosesToTheHealthRead pins the two ways the +// memory expires. A binary whose stamp changed no longer ships what the +// memory says was shipped — in practice a rebuilt daemon, whose restart +// drops the memory anyway; the guard states the invariant without leaning on +// the restart. And an origin that moved on names a relay this process never +// deployed to. Both must yield to the health read, so a genuinely newer +// build still gets its update card. +func TestRelayUIStaleShipMemoryLosesToTheHealthRead(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + f := newFakeCloudflare(t, oneAccount(), "karn") + stale := staleHealth(t) + seedRelayAt(t, stale.URL) + svc := uiService(f, &relayRuntime{running: true}) + + if _, err := svc.Update(context.Background(), daemon.RelayUIDeployRequest{Token: setupToken}); err != nil { + t.Fatalf("Update: %v", err) + } + + // The rebuilt-binary shape: the memory holds a stamp this binary would + // not ship. (A test cannot rebuild itself, so it plants the mismatch.) + svc.shippedMu.Lock() + svc.shippedStamp = "dev-some-other-build" + svc.shippedMu.Unlock() + if st := svc.Status(context.Background()); st.DeployedVersion != "0.0.0-previous" { + t.Fatalf("deployed version under a stale stamp = %q, want the health read's 0.0.0-previous", st.DeployedVersion) + } + + // The moved-origin shape, through the real path: deploy again (memory + // back in force), then repoint the address. The new origin answers no + // health read — port 1 refuses instantly — and the memory, keyed to the + // origin it shipped to, must not answer for it. + if _, err := svc.Update(context.Background(), daemon.RelayUIDeployRequest{Token: setupToken}); err != nil { + t.Fatalf("second Update: %v", err) + } + if st := svc.Status(context.Background()); st.DeployedVersion != deployStamp() { + t.Fatalf("deployed version after re-deploying = %q, want %q", st.DeployedVersion, deployStamp()) + } + if _, err := svc.SetAddress(context.Background(), "wss://127.0.0.1:1"); err != nil { + t.Fatalf("SetAddress: %v", err) + } + if st := svc.Status(context.Background()); st.DeployedVersion != "" { + t.Fatalf("deployed version after repointing = %q, want empty: neither memory nor a health read can speak for the new origin", st.DeployedVersion) + } +} + // twoAccounts mirrors oneAccount for the picker tests. func twoAccounts() []cloudflare.Account { return []cloudflare.Account{ diff --git a/internal/daemon/relayui.go b/internal/daemon/relayui.go index 41ec439..b4a97e1 100644 --- a/internal/daemon/relayui.go +++ b/internal/daemon/relayui.go @@ -57,9 +57,13 @@ type RelayUIStatus struct { // the sentence the UI shows instead of a button. CanDeploy bool `json:"can_deploy"` CanDeployReason string `json:"can_deploy_reason,omitempty"` - // Version is this binary's; DeployedVersion is what the relay's - // /api/health reported, empty when unreachable or unstamped. The UI - // offers an update when the two differ. + // Version is this binary's; DeployedVersion is what the relay serves. + // Normally that is what its /api/health reported — empty when + // unreachable or unstamped — except right after a deploy this daemon + // performed, when it is the stamp the daemon shipped: the edge keeps + // serving the previous Worker for a while after the API accepts a new + // one, and a health read taken in that window would re-offer an update + // that just succeeded. The UI offers an update when the two differ. Version string `json:"version"` DeployedVersion string `json:"deployed_version,omitempty"` // HasToken says a Cloudflare token is stored (config/cloudflare.json), so