R-31: a second off-site Save while the first still provisions is refused, not raced

Both saves used to list no sub-account and create one (a dedicated box is a second bill). Per-customer in-memory claim; the second gets 409 and nothing is saved or created. The async save + status card stays open. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS
2026-10-06 21:04:21 +02:00
parent de3480fcd2
commit 6f8aa46cf7
4 changed files with 136 additions and 7 deletions
@@ -1,5 +1,6 @@
 ## Unreleased (2026-10-06 night) — a reinstall's new backup key no longer overwrites the old one in the escrow (R-366)

+- hub (R-31): a second Save of a customer's off-site settings while the first is still provisioning is refused at once (409, „already running — wait about a minute, then reload; do not save again") instead of racing it: both used to see no sub-account and create one (for a dedicated box, a second bill). Per customer, in memory; another customer is never blocked. Tests `TestProvision_R31_*` (red-proved: `documentation/audits/night-burndown-2026-10-06/hub/R-31-red.txt`). The async save with a status card stays open.
 - hub: the current escrow row is kept (retained, operator-only) when the whole-guest backup key's fingerprint changes, not only when the restic password changes. A reinstall mints a new PBS key while R-241 keeps the restic password, so the old rule overwrote the only copy of the old key and every pre-reinstall whole-guest archive became unopenable. An empty fingerprint on either side is unknown, not a change. Tests `TestSaveHostEscrow_R366_*` (red-proved: `documentation/audits/night-burndown-2026-10-06/r366/`).

 ## v0.140.0 — a failed operator mail is sent again; a Docker set is approved only after the engine showed it reports a memory kill (decision 157) (2026-10-06)
@@ -12,10 +12,12 @@ import (
 	"context"
 	"crypto/rand"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log"
 	"math/big"
 	"strings"
+	"sync"
 	"time"

 	"gitea.dooplex.hu/admin/felhom-hub/internal/hetznerapi"
@@ -29,8 +31,8 @@ const sftpPort = 23
 // the password or the SSH key.
 type Descriptor struct {
 	Enabled  bool   `json:"enabled"`
-	Type     string `json:"type,omitempty"`      // "shared" | "dedicated"
-	Host     string `json:"host,omitempty"`      // <user>.your-storagebox.de (dedicated) / <user>-subN… (shared)
+	Type     string `json:"type,omitempty"` // "shared" | "dedicated"
+	Host     string `json:"host,omitempty"` // <user>.your-storagebox.de (dedicated) / <user>-subN… (shared)
 	User     string `json:"user,omitempty"`
 	Port     int    `json:"port,omitempty"`      // 23
 	RepoPath string `json:"repo_path,omitempty"` // /home/<repo>
@@ -66,6 +68,35 @@ type Provisioner struct {
 	// creation by seconds-to-a-minute, so the first scan typically fails with "no such host"). nil → the
 	// default ~60s ladder. Tests inject zeros. The total must fit inside applyOffsite's 3-min detached ctx.
 	ScanBackoff []time.Duration
+
+	// inflight (R-31) holds the customers whose provisioning is running RIGHT NOW.
+	// A second Save while the first still runs used to race it: both listed no sub-account, both created
+	// one (a second dedicated box is a second bill). The second call now fails at once with
+	// ErrProvisionInProgress. Pinned by TestProvision_R31_ConcurrentSaveRefused.
+	inflightMu sync.Mutex
+	inflight   map[string]bool
+}
+
+// ErrProvisionInProgress (R-31): another provisioning for the same customer is still running.
+var ErrProvisionInProgress = errors.New("offsite: provisioning for this customer is already running — wait about a minute, then reload the page; do not save again")
+
+// begin claims customerID for one provisioning step; the returned func releases it.
+func (p *Provisioner) begin(customerID string) (func(), error) {
+	p.inflightMu.Lock()
+	defer p.inflightMu.Unlock()
+	if p.inflight == nil {
+		p.inflight = map[string]bool{}
+	}
+	if p.inflight[customerID] {
+		p.logf("[offsite] refused a second provisioning for %s while one is running (R-31)", customerID)
+		return nil, ErrProvisionInProgress
+	}
+	p.inflight[customerID] = true
+	return func() {
+		p.inflightMu.Lock()
+		delete(p.inflight, customerID)
+		p.inflightMu.Unlock()
+	}, nil
 }

 // defaultScanBackoff: 5 retries, ~60s total — sized to the observed DNS propagation lag.
@@ -78,8 +109,10 @@ func (p *Provisioner) logf(f string, a ...any) {
 }

 // customerLabel is the idempotency/teardown key.
-func customerLabel(customerID string) map[string]string { return map[string]string{"felhom-customer": customerID} }
-func customerSelector(customerID string) string         { return "felhom-customer=" + customerID }
+func customerLabel(customerID string) map[string]string {
+	return map[string]string{"felhom-customer": customerID}
+}
+func customerSelector(customerID string) string { return "felhom-customer=" + customerID }

 // repoPath is the controller-facing RepoPath — each account is chrooted, /home is writable (SPIKE).
 const repoPath = "/home/felhom-repo"
@@ -92,8 +125,12 @@ func (p *Provisioner) ProvisionOffsite(ctx context.Context, customerID string, i
 	if !in.Enabled {
 		return &Descriptor{Enabled: false}, nil
 	}
+	release, err := p.begin(customerID)
+	if err != nil {
+		return nil, err
+	}
+	defer release()
 	var d *Descriptor
-	var err error
 	switch in.Type {
 	case "shared":
 		d, err = p.provisionShared(ctx, customerID, in)
@@ -0,0 +1,81 @@
+package offsite
+
+import (
+	"context"
+	"errors"
+	"sync"
+	"testing"
+
+	"gitea.dooplex.hu/admin/felhom-hub/internal/hetznerapi"
+)
+
+// gatedAPI blocks the FIRST ListSubaccounts after it has listed, so a second Save can arrive in the
+// window between "no sub-account yet" and "create" — the race R-31 names.
+type gatedAPI struct {
+	*hetznerapi.Fake
+	once    sync.Once
+	listed  chan struct{}
+	release chan struct{}
+}
+
+func (g *gatedAPI) ListSubaccounts(ctx context.Context, boxID int64, sel string) ([]hetznerapi.Subaccount, error) {
+	out, err := g.Fake.ListSubaccounts(ctx, boxID, sel)
+	first := false
+	g.once.Do(func() { first = true })
+	if first {
+		close(g.listed)
+		<-g.release
+	}
+	return out, err
+}
+
+// R-31 — a second Save while the first provisioning still runs must not create a second sub-account
+// (for a dedicated box: a second bill). It is refused at once with ErrProvisionInProgress.
+//
+// COMPANION RED-PROOF (observed): remove the begin()/release() claim from ProvisionOffsite → this fails
+// with "the second Save must be refused while the first runs; got err=<nil>". Restored.
+func TestProvision_R31_ConcurrentSaveRefused(t *testing.T) {
+	p, fake, _ := newTestProvisioner(t)
+	g := &gatedAPI{Fake: fake, listed: make(chan struct{}), release: make(chan struct{})}
+	p.API = g
+
+	firstErr := make(chan error, 1)
+	go func() {
+		_, err := p.ProvisionOffsite(context.Background(), "cust-r31", Input{Enabled: true, Type: "shared", QuotaGB: 10})
+		firstErr <- err
+	}()
+	<-g.listed // the first Save has seen "no sub-account" and is about to create one
+
+	_, err := p.ProvisionOffsite(context.Background(), "cust-r31", Input{Enabled: true, Type: "shared", QuotaGB: 10})
+	close(g.release)
+	if e := <-firstErr; e != nil {
+		t.Fatalf("the first Save must succeed: %v", e)
+	}
+	if !errors.Is(err, ErrProvisionInProgress) {
+		t.Fatalf("the second Save must be refused while the first runs; got err=%v", err)
+	}
+	if fake.CreatedSubaccounts != 1 {
+		t.Fatalf("exactly one sub-account must exist, got %d", fake.CreatedSubaccounts)
+	}
+
+	// After the first finished, a Save is idempotent again (no refusal, no second create).
+	if _, err := p.ProvisionOffsite(context.Background(), "cust-r31", Input{Enabled: true, Type: "shared", QuotaGB: 10}); err != nil {
+		t.Fatalf("a Save after the first finished must pass: %v", err)
+	}
+	if fake.CreatedSubaccounts != 1 {
+		t.Fatalf("still exactly one sub-account, got %d", fake.CreatedSubaccounts)
+	}
+}
+
+// Another customer is never blocked by one customer's running provisioning.
+func TestProvision_R31_OtherCustomerNotBlocked(t *testing.T) {
+	p, _, _ := newTestProvisioner(t)
+	release, err := p.begin("cust-x")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer release()
+	if _, err := p.ProvisionOffsite(context.Background(), "cust-y", Input{Enabled: true, Type: "shared", QuotaGB: 10}); err != nil {
+		t.Fatalf("another customer must not be blocked: %v", err)
+	}
+}
@@ -779,7 +779,12 @@ func (s *Server) handleConfigCreate(w http.ResponseWriter, r *http.Request) {
 	}

 	// Offsite provisioning (fail-closed): a provisioning error must NOT save a half-enabled config.
-	if err := s.applyOffsite(r.Context(), r, cfg); err != nil {
+	if err := s.applyOffsite(r.Context(), r, cfg); errors.Is(err, offsite.ErrProvisionInProgress) {
+		// R-31: a second Save while the first still provisions — nothing saved, nothing created.
+		s.logger.Printf("[INFO] offsite provision for %s refused: one is already running (R-31)", customerID)
+		http.Error(w, err.Error(), http.StatusConflict)
+		return
+	} else if err != nil {
 		s.logger.Printf("[ERROR] offsite provision for %s: %v", customerID, err)
 		http.Error(w, "Offsite provisioning failed: "+err.Error(), http.StatusBadGateway)
 		return
@@ -859,7 +864,12 @@ func (s *Server) handleConfigUpdate(w http.ResponseWriter, r *http.Request, cust

 	cfg.ConfigJSON = buildConfigJSON(r)

-	if err := s.applyOffsite(r.Context(), r, cfg); err != nil {
+	if err := s.applyOffsite(r.Context(), r, cfg); errors.Is(err, offsite.ErrProvisionInProgress) {
+		// R-31: a second Save while the first still provisions — nothing saved, nothing created.
+		s.logger.Printf("[INFO] offsite provision for %s refused: one is already running (R-31)", customerID)
+		http.Error(w, err.Error(), http.StatusConflict)
+		return
+	} else if err != nil {
 		s.logger.Printf("[ERROR] offsite provision for %s: %v", customerID, err)
 		http.Error(w, "Offsite provisioning failed: "+err.Error(), http.StatusBadGateway)
 		return