diff --git a/CHANGELOG.md b/CHANGELOG.md index 855bbe6..38d1c5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Redeploying a proxied copy could leave the proxy with no copy at all.** The old earner and tunnel were removed before the new tunnel started, so a tunnel that could not start (an image missing locally, a proxy that does not answer) cost the running copy. The new tunnel now starts under a staged name next to the running copy, and the old pair goes only after the new tunnel has proved an exit. - **The Fleet page could draw over another view, and lose a half-typed proxy.** Fleet reads the backend twice before it draws, so a user who opened another view meanwhile got the Fleet page back. Any proxy action redraws the page, which also reset the open proxy editor and dropped unsaved values, a typed password included. A redraw now gives up when Fleet is no longer shown or a newer redraw started, and keeps the open editor when the same proxy's editor is drawn again. - **The Proxies section had no help link.** It now links to the published page on proxied copies, https://geiserx.github.io/CashPilot/proxies/. - **A proxied copy's stop that failed halfway could be undone.** The earner stopped first. If the tunnel then failed to stop, the next status read saw a ready tunnel beside a stopped earner and started the earner again. The tunnel now stops first, so a half-finished stop never leaves that state. diff --git a/internal/runtime/proxied.go b/internal/runtime/proxied.go index c64be83..2b5eb6f 100644 --- a/internal/runtime/proxied.go +++ b/internal/runtime/proxied.go @@ -36,6 +36,7 @@ import ( "sort" "strconv" "strings" + "sync" "time" "github.com/GeiserX/CashPilot-Desktop/internal/catalog" @@ -212,6 +213,25 @@ func tunnelName(slug string, proxyID int64) string { return fmt.Sprintf("%stunnel-%s-p%d", containerName(""), slug, proxyID) } +// stagedSuffix: a redeploy starts the new tunnel under its name plus this suffix, +// next to the copy that runs now, and gives it the final name only once the old pair +// is gone. The status read never reports a staged tunnel. +const stagedSuffix = "-next" + +// proxiedSwap makes deployProxied's swap of the old pair for the staged tunnel, and +// the status read, take turns: a status read must not start an earner that is being +// removed, or read a half-swapped pair. +var proxiedSwap sync.Mutex + +// proxiedDeploys runs one deploy per copy at a time: a second one would take the +// first one's staged tunnel for a leftover and remove it. +var proxiedDeploys sync.Map // "slug/id" -> *sync.Mutex + +func deployLock(slug string, proxyID int64) *sync.Mutex { + mu, _ := proxiedDeploys.LoadOrStore(fmt.Sprintf("%s/%d", slug, proxyID), &sync.Mutex{}) + return mu.(*sync.Mutex) +} + func proxiedName(slug string, proxyID int64) string { return fmt.Sprintf("%s-p%d", containerName(slug), proxyID) } @@ -395,7 +415,8 @@ func sidecarCreate(name, slug string, proxyID *int64, image, network string) cli if proxyID == nil { restart = container.RestartPolicy{Name: "no"} } - hostname := strings.TrimPrefix(name, containerName("")) + // A staged tunnel already gets the hostname of the name it is promoted to. + hostname := strings.TrimSuffix(strings.TrimPrefix(name, containerName("")), stagedSuffix) if len(hostname) > 63 { hostname = hostname[:63] } @@ -500,6 +521,7 @@ type tunnelDocker interface { stop(ctx context.Context, id string, timeout int) error restart(ctx context.Context, id string, timeout int) error remove(ctx context.Context, id string) error + rename(ctx context.Context, id, name string) error inspect(ctx context.Context, nameOrID string) (container.InspectResponse, bool, error) list(ctx context.Context, labels ...string) ([]container.Summary, error) readFile(ctx context.Context, id, path string) (string, error) @@ -560,6 +582,10 @@ func (d liveTunnelDocker) remove(ctx context.Context, id string) error { _, err := d.cli.ContainerRemove(ctx, id, client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}) return err } +func (d liveTunnelDocker) rename(ctx context.Context, id, name string) error { + _, err := d.cli.ContainerRename(ctx, id, client.ContainerRenameOptions{NewName: name}) + return err +} func (d liveTunnelDocker) inspect(ctx context.Context, nameOrID string) (container.InspectResponse, bool, error) { res, err := d.cli.ContainerInspect(ctx, nameOrID, client.ContainerInspectOptions{}) if err != nil { @@ -748,6 +774,9 @@ func pairOf(ctx context.Context, d tunnelDocker, slug string, proxyID int64) (pr } func deployProxied(ctx context.Context, d tunnelDocker, req ProxiedDeploy) (Exit, error) { + mu := deployLock(req.Slug, req.ProxyID) + mu.Lock() + defer mu.Unlock() facts := d.facts(ctx) if facts.OSType != "" && facts.OSType != "linux" { return Exit{}, proxyErr("Proxied copies need a Linux container runtime; this one runs %s containers.", facts.OSType) @@ -784,14 +813,40 @@ func deployProxied(ctx context.Context, d tunnelDocker, req ProxiedDeploy) (Exit if err != nil { return Exit{}, err } - if _, err := removeProxied(ctx, d, req.Slug, req.ProxyID, StopTimeoutSeconds(req.Service)); err != nil { + pid := req.ProxyID + final := tunnelName(req.Slug, pid) + staged := final + stagedSuffix + // Left by a deploy that died midway; replaced only if it is ours. + if ins, found, err := d.inspect(ctx, staged); err != nil { return Exit{}, err + } else if found { + if ins.Config == nil || !isOurs(ins.Config.Labels, req.Slug, pid) { + return Exit{}, proxyErr("A container named %s that CashPilot did not create is in the way", staged) + } + // One younger than the sweep age belongs to a deploy still running, in this + // app or another one on the same Docker host; removing it would cut that + // deploy's tunnel after it may already have removed the old pair. + if created, perr := time.Parse(time.RFC3339Nano, ins.Created); perr == nil && time.Since(created) < staleTestTunnel { + return Exit{}, proxyErr("Another deploy of %s through proxy %d is under way; try again in a few minutes", req.Slug, pid) + } + removeQuietly(d, ins.ID, 2) } - pid := req.ProxyID - sidecarID, exit, err := startSidecar(ctx, d, tunnelName(req.Slug, pid), req.Slug, network, &pid, req.Proxy, req.OwnIP) + // The new tunnel proves an exit while the old pair keeps running, so a tunnel that + // cannot start costs nothing. Only then does the old pair go. + sidecarID, exit, err := startSidecar(ctx, d, staged, req.Slug, network, &pid, req.Proxy, req.OwnIP) if err != nil { return Exit{}, err } + proxiedSwap.Lock() + _, err = removeProxied(ctx, d, req.Slug, req.ProxyID, StopTimeoutSeconds(req.Service)) + if err == nil { + err = d.rename(ctx, sidecarID, final) + } + proxiedSwap.Unlock() + if err != nil { + removeQuietly(d, sidecarID, 2) + return Exit{}, err + } opts, err := earnerCreate(req, sidecarID, image, facts.OSType) if err != nil { removeQuietly(d, sidecarID, 2) @@ -1015,8 +1070,27 @@ const testTunnelSlug = "proxy-test" // app that quit during the test. const staleTestTunnel = 5 * time.Minute -// sweepTestTunnels removes Test tunnels older than staleTestTunnel. +// sweepTestTunnels removes Test tunnels older than staleTestTunnel, and staged +// tunnels as old: a deploy is past its ready timeout by then, so the deploy that +// staged it died, and it holds the proxy login while no status read reports it. func sweepTestTunnels(ctx context.Context, d tunnelDocker, now time.Time) { + if staged, err := d.list(ctx, LabelManaged+"=true", LabelRole+"="+RoleTunnel, LabelProxy); err == nil { + for _, c := range staged { + if c.Labels[LabelProxy] != "" && strings.HasSuffix(summaryName(c), stagedSuffix) && now.Sub(time.Unix(c.Created, 0)) >= staleTestTunnel { + id, perr := strconv.ParseInt(c.Labels[LabelProxy], 10, 64) + if perr != nil { + continue + } + // A deploy of this copy in this app may still be waiting to swap it in. + mu := deployLock(c.Labels[LabelService], id) + if !mu.TryLock() { + continue + } + removeQuietly(d, c.ID, 2) + mu.Unlock() + } + } + } found, err := d.list(ctx, LabelManaged+"=true", LabelRole+"="+RoleTunnel, LabelService+"="+testTunnelSlug) if err != nil { return @@ -1034,6 +1108,8 @@ func sweepTestTunnels(ctx context.Context, d tunnelDocker, now time.Time) { func proxiedStatus(ctx context.Context, d tunnelDocker) ([]ProxiedCopy, error) { sweepTestTunnels(ctx, d, time.Now()) + proxiedSwap.Lock() + defer proxiedSwap.Unlock() found, err := d.list(ctx, LabelManaged+"=true", LabelProxy) if err != nil { return nil, err @@ -1044,8 +1120,8 @@ func proxiedStatus(ctx context.Context, d tunnelDocker) ([]ProxiedCopy, error) { } pairs := map[key]map[string]container.Summary{} for _, c := range found { - if c.Labels[LabelManaged] != "true" { - continue + if c.Labels[LabelManaged] != "true" || strings.HasSuffix(summaryName(c), stagedSuffix) { + continue // a staged tunnel is not part of a copy until it is promoted } id, err := strconv.ParseInt(c.Labels[LabelProxy], 10, 64) if err != nil { diff --git a/internal/runtime/proxied_test.go b/internal/runtime/proxied_test.go index fe95a92..eb31bd5 100644 --- a/internal/runtime/proxied_test.go +++ b/internal/runtime/proxied_test.go @@ -72,6 +72,8 @@ type fakeTunnelDocker struct { networkErr error // created is the Created time list reports for every container (unix seconds). created int64 + // renameErr makes every rename fail. + renameErr error // stopErr makes a stop of that container name fail. stopErr map[string]error } @@ -190,6 +192,25 @@ func (f *fakeTunnelDocker) remove(_ context.Context, id string) error { f.record("remove " + c.name) return nil } +func (f *fakeTunnelDocker) rename(_ context.Context, id, name string) error { + f.mu.Lock() + defer f.mu.Unlock() + c := f.find(id) + if c == nil { + return errors.New("no such container") + } + if f.renameErr != nil { + return f.renameErr + } + if f.containers[name] != nil { + return fmt.Errorf("name %s in use", name) + } + delete(f.containers, c.name) + f.record("rename " + c.name + " " + name) + c.name = name + f.containers[name] = c + return nil +} func (f *fakeTunnelDocker) inspect(_ context.Context, ref string) (container.InspectResponse, bool, error) { f.mu.Lock() defer f.mu.Unlock() @@ -198,10 +219,11 @@ func (f *fakeTunnelDocker) inspect(_ context.Context, ref string) (container.Ins return container.InspectResponse{}, false, nil } return container.InspectResponse{ - ID: c.id, - Name: "/" + c.name, - State: &container.State{Status: container.ContainerState(c.state), StartedAt: c.startedAt}, - Config: c.opts.Config, + ID: c.id, + Name: "/" + c.name, + State: &container.State{Status: container.ContainerState(c.state), StartedAt: c.startedAt}, + Config: c.opts.Config, + Created: time.Unix(f.created, 0).UTC().Format(time.RFC3339Nano), }, true, nil } func (f *fakeTunnelDocker) list(_ context.Context, _ ...string) ([]container.Summary, error) { @@ -416,7 +438,7 @@ func TestDeployStartsTheEarnerOnlyAfterTheTunnelIsReady(t *testing.T) { t.Fatalf("exit: %+v", exit) } calls := f.callList() - order := []string{"create cashpilot-tunnel-earner-p3", "copy cashpilot-tunnel-earner-p3 " + tunnelSecretDir, "start cashpilot-tunnel-earner-p3", "create cashpilot-earner-p3", "start cashpilot-earner-p3"} + order := []string{"create cashpilot-tunnel-earner-p3-next", "copy cashpilot-tunnel-earner-p3-next " + tunnelSecretDir, "start cashpilot-tunnel-earner-p3-next", "rename cashpilot-tunnel-earner-p3-next cashpilot-tunnel-earner-p3", "create cashpilot-earner-p3", "start cashpilot-earner-p3"} last := -1 for _, step := range order { i := slices.Index(calls, step) @@ -781,6 +803,186 @@ func TestAProbeAnswerThatIsNotAnIPIsNotReady(t *testing.T) { } } +// A redeploy whose new tunnel cannot start keeps the copy running now: the new tunnel +// proves its exit under a staged name before the old pair goes. +func TestARedeployWhoseTunnelCannotStartKeepsTheRunningCopy(t *testing.T) { + withTestTunnel(t) + f := newFakeTunnelDocker() + ctx := context.Background() + if _, err := deployProxied(ctx, f, deployRequest()); err != nil { + t.Fatal(err) + } + oldEarner, oldTunnel := f.containers["cashpilot-earner-p3"].id, f.containers["cashpilot-tunnel-earner-p3"].id + f.exit = "" // the new tunnel never sees an exit + if _, err := deployProxied(ctx, f, deployRequest()); err == nil { + t.Fatal("a tunnel with no exit must fail the deploy") + } + if e, tn := f.containers["cashpilot-earner-p3"], f.containers["cashpilot-tunnel-earner-p3"]; e == nil || tn == nil || e.id != oldEarner || tn.id != oldTunnel { + t.Fatalf("the running copy was replaced or removed: %v", f.callList()) + } + if f.containers["cashpilot-tunnel-earner-p3-next"] != nil { + t.Fatal("the staged tunnel was left behind") + } +} + +// The staged tunnel takes the final name only after the old pair is gone, and keeps +// the final hostname from the start. +func TestARedeployPromotesTheStagedTunnelAfterTheOldPairGoes(t *testing.T) { + withTestTunnel(t) + f := newFakeTunnelDocker() + ctx := context.Background() + if _, err := deployProxied(ctx, f, deployRequest()); err != nil { + t.Fatal(err) + } + before := len(f.callList()) + if _, err := deployProxied(ctx, f, deployRequest()); err != nil { + t.Fatal(err) + } + calls := f.callList()[before:] + ready, gone, renamed := slices.Index(calls, "start cashpilot-tunnel-earner-p3-next"), slices.Index(calls, "remove cashpilot-tunnel-earner-p3"), slices.Index(calls, "rename cashpilot-tunnel-earner-p3-next cashpilot-tunnel-earner-p3") + if ready < 0 || gone < ready || renamed < gone || slices.Index(calls, "remove cashpilot-earner-p3") > gone { + t.Fatalf("order: %v", calls) + } + if h := f.containers["cashpilot-tunnel-earner-p3"].opts.Config.Hostname; h != "tunnel-earner-p3" { + t.Fatalf("hostname %q", h) + } +} + +// A staged tunnel a dead deploy left is replaced, never a stranger under that name; +// the status read never reports one, and sweeps it once it is stale. +func TestAStagedTunnelIsNeverReportedAndAStrangerIsNeverRemoved(t *testing.T) { + withTestTunnel(t) + f := newFakeTunnelDocker() + ctx := context.Background() + if _, err := deployProxied(ctx, f, deployRequest()); err != nil { + t.Fatal(err) + } + pid := int64(3) + if _, err := f.create(ctx, sidecarCreate("cashpilot-tunnel-earner-p3-next", "earner", &pid, "example/cashpilot-tunnel:1.66.0", "")); err != nil { + t.Fatal(err) + } + f.created = time.Now().Unix() + status, err := proxiedStatus(ctx, f) + if err != nil || len(status) != 1 || status[0].TunnelContainerID != f.containers["cashpilot-tunnel-earner-p3"].id { + t.Fatalf("status reported the staged tunnel: %+v %v", status, err) + } + f.created = time.Now().Add(-2 * staleTestTunnel).Unix() + if _, err := proxiedStatus(ctx, f); err != nil { + t.Fatal(err) + } + if f.containers["cashpilot-tunnel-earner-p3-next"] != nil || f.containers["cashpilot-tunnel-earner-p3"] == nil { + t.Fatal("a stale staged tunnel is swept, and only it") + } + stranger := sidecarCreate("cashpilot-tunnel-earner-p3-next", "other", &pid, "example/cashpilot-tunnel:1.66.0", "") + if _, err := f.create(ctx, stranger); err != nil { + t.Fatal(err) + } + if _, err := deployProxied(ctx, f, deployRequest()); err == nil || !strings.Contains(err.Error(), "did not create") { + t.Fatalf("a stranger under the staged name must stop the deploy, got %v", err) + } + if f.containers["cashpilot-tunnel-earner-p3-next"] == nil { + t.Fatal("the stranger was removed") + } +} + +// A young staged tunnel belongs to a deploy still running, maybe in another app on +// the same Docker host: a second deploy refuses instead of removing it. +func TestADeployLeavesAnotherDeploysStagedTunnelAlone(t *testing.T) { + withTestTunnel(t) + f := newFakeTunnelDocker() + ctx := context.Background() + if _, err := deployProxied(ctx, f, deployRequest()); err != nil { + t.Fatal(err) + } + pid := int64(3) + if _, err := f.create(ctx, sidecarCreate("cashpilot-tunnel-earner-p3-next", "earner", &pid, "example/cashpilot-tunnel:1.66.0", "")); err != nil { + t.Fatal(err) + } + staged := f.containers["cashpilot-tunnel-earner-p3-next"].id + oldTunnel := f.containers["cashpilot-tunnel-earner-p3"].id + f.created = time.Now().Unix() + if _, err := deployProxied(ctx, f, deployRequest()); err == nil || !strings.Contains(err.Error(), "under way") { + t.Fatalf("a young staged tunnel must stop the deploy, got %v", err) + } + if c := f.containers["cashpilot-tunnel-earner-p3-next"]; c == nil || c.id != staged { + t.Fatal("the other deploy's staged tunnel was removed") + } + if c := f.containers["cashpilot-tunnel-earner-p3"]; c == nil || c.id != oldTunnel { + t.Fatal("the running pair was touched") + } + f.created = time.Now().Add(-2 * staleTestTunnel).Unix() + if _, err := deployProxied(ctx, f, deployRequest()); err != nil { + t.Fatalf("an abandoned staged tunnel is replaced: %v", err) + } +} + +// The sweep leaves a stale-looking staged tunnel alone while a deploy of its copy in +// this app still holds the copy's lock. +func TestTheSweepSkipsAStagedTunnelWhoseDeployIsRunning(t *testing.T) { + withTestTunnel(t) + f := newFakeTunnelDocker() + ctx := context.Background() + if _, err := deployProxied(ctx, f, deployRequest()); err != nil { + t.Fatal(err) + } + pid := int64(3) + if _, err := f.create(ctx, sidecarCreate("cashpilot-tunnel-earner-p3-next", "earner", &pid, "example/cashpilot-tunnel:1.66.0", "")); err != nil { + t.Fatal(err) + } + f.created = time.Now().Add(-2 * staleTestTunnel).Unix() + mu := deployLock("earner", 3) + mu.Lock() + sweepTestTunnels(ctx, f, time.Now()) + if f.containers["cashpilot-tunnel-earner-p3-next"] == nil { + mu.Unlock() + t.Fatal("the sweep removed a staged tunnel its deploy still holds") + } + mu.Unlock() + sweepTestTunnels(ctx, f, time.Now()) + if f.containers["cashpilot-tunnel-earner-p3-next"] != nil { + t.Fatal("once the deploy is done the stale staged tunnel is swept") + } +} + +// A swap that fails after the old pair went removes the staged tunnel, so nothing is +// left running under a name the status read never reports. +func TestAFailedPromotionRemovesTheStagedTunnel(t *testing.T) { + withTestTunnel(t) + f := newFakeTunnelDocker() + ctx := context.Background() + f.renameErr = errors.New("409 Conflict") + if _, err := deployProxied(ctx, f, deployRequest()); err == nil || !strings.Contains(err.Error(), "409") { + t.Fatalf("the failed rename must fail the deploy, got %v", err) + } + if f.containers["cashpilot-tunnel-earner-p3-next"] != nil || f.containers["cashpilot-earner-p3"] != nil { + t.Fatalf("left behind: %v", f.callList()) + } +} + +// Two deploys of one copy take turns, so the second never removes the first one's +// staged tunnel as a leftover. +func TestTwoDeploysOfOneCopyTakeTurns(t *testing.T) { + withTestTunnel(t) + f := newFakeTunnelDocker() + ctx := context.Background() + var wg sync.WaitGroup + errs := make([]error, 2) + for i := range errs { + wg.Add(1) + go func(i int) { + defer wg.Done() + _, errs[i] = deployProxied(ctx, f, deployRequest()) + }(i) + } + wg.Wait() + if errs[0] != nil || errs[1] != nil { + t.Fatalf("both deploys must succeed one after the other: %v", errs) + } + if f.containers["cashpilot-tunnel-earner-p3"] == nil || f.containers["cashpilot-earner-p3"] == nil || f.containers["cashpilot-tunnel-earner-p3-next"] != nil { + t.Fatalf("containers after two deploys: %v", f.callList()) + } +} + // The live test removes fixed names before it runs; a container of the operator's // under one of those names must stop it instead of being removed. func TestTheLiveTestNeverRemovesAContainerItDidNotCreate(t *testing.T) {