carlos / platform Public

Merge pull request #115 from carlosframework/small-residuals

Small residuals: #58 sibling back-fill, syncable anti-drift, trash serial bumps, concurrent deploy watch
Paul Campbell 40469637189c8fb933396453fc0152f1a4e14478 parent 7a3d8c1parent cac9a67
10 files changed, +279 −56
  • deploy.go +82 −46
  • deploy_test.go +53 −0
  • internal/appconfig/sync.go +7 −7
  • internal/console/apicli.go +6 −0
  • internal/console/appsettings.go +7 −0
  • internal/console/deletion_test.go +8 −0
  • internal/console/ownership.go +11 −3
  • internal/console/serialbump_test.go +42 −0
  • internal/release/versions.go +22 −0
  • internal/release/versions_test.go +41 −0
diff --git a/deploy.go b/deploy.go
index b4158f024..797b786a1 100644
--- a/deploy.go
+++ b/deploy.go
@@ -17,6 +17,8 @@ import (
"os/exec"
"sort"
"strings"
+ "sync"
+ "sync/atomic"
"time"
)
@@ -253,66 +255,100 @@ var watchOrigin = func(host string) string { return "https://" + host }
// deploy as failed. The attempt counter is sufficient (no randomness
// needed): each request just needs to differ from the ones before it.
func watchVersion(w io.Writer, targets []watchTarget, ver string, timeout, interval time.Duration) error {
- pending := make(map[string]watchTarget, len(targets))
+ // pending is a slice, not a map, so target order stays stable across
+ // sweeps — the goroutines below write into an indexed result slice keyed
+ // off this same order, and the "live" lines print in that order once the
+ // whole sweep has finished, never interleaved mid-flight. A map would
+ // have de-duped repeated hosts for free, so do that explicitly here,
+ // keeping each host's first occurrence — otherwise a duplicate host gets
+ // polled (and printed "live") twice per sweep.
+ seen := make(map[string]bool, len(targets))
+ pending := make([]watchTarget, 0, len(targets))
for _, t := range targets {
- pending[t.Host] = t
+ if seen[t.Host] {
+ continue
+ }
+ seen[t.Host] = true
+ pending = append(pending, t)
}
last := make(map[string]string, len(targets))
client := &http.Client{Timeout: 10 * time.Second}
deadline := time.Now().Add(timeout)
- attempt := 0
+ var attempt atomic.Int64 // per-sweep-and-target: every goroutine gets a distinct probe value
for {
- for host, t := range pending {
- attempt++
- url := fmt.Sprintf("%s/?carlos-deploy-probe=%d", t.Origin, attempt)
- resp, err := client.Get(url)
- if err != nil {
- last[host] = "unreachable: " + err.Error()
- continue
- }
- io.CopyN(io.Discard, resp.Body, 4096)
- resp.Body.Close()
- got := resp.Header.Get("X-Carlos-Version")
- switch {
- case got == ver && resp.StatusCode/100 == 2:
+ type result struct {
+ live bool
+ diag string
+ }
+ results := make([]result, len(pending))
+ var wg sync.WaitGroup
+ for i, t := range pending {
+ wg.Add(1)
+ go func(i int, t watchTarget) {
+ defer wg.Done()
+ n := attempt.Add(1)
+ url := fmt.Sprintf("%s/?carlos-deploy-probe=%d", t.Origin, n)
+ resp, err := client.Get(url)
+ if err != nil {
+ results[i] = result{diag: "unreachable: " + err.Error()}
+ return
+ }
+ io.CopyN(io.Discard, resp.Body, 4096)
+ resp.Body.Close()
+ got := resp.Header.Get("X-Carlos-Version")
+ switch {
+ case got == ver && resp.StatusCode/100 == 2:
+ results[i] = result{live: true}
+ case got == ver:
+ // The edge stamps X-Carlos-Version before proxying to the
+ // origin, so a tenant that dies on boot can still answer
+ // with the new version header attached to a non-2xx (a 502,
+ // most commonly). That is the new build in a crash loop, not
+ // a live deploy, so the success arm has to gate on status
+ // too — matching the header alone let a dead tenant exit 0.
+ results[i] = result{diag: fmt.Sprintf("serving %s but HTTP %d — the app is failing", ver, resp.StatusCode)}
+ case got == "" && resp.StatusCode/100 != 2:
+ // A non-2xx with no header is not an old edge — it is an edge
+ // that answered and had nothing good to serve: an activator
+ // 503 because the wake failed or timed out, or a 404 because
+ // the route has not been minted yet. (A 502 from a tenant
+ // that died on boot carries the version header — the edge
+ // stamps it before proxying — so that shape lands in the
+ // case above, not here.) Reporting those as "edge predates
+ // this feature — the hourly pass will adopt it" sends the
+ // operator off to wait for a pass that will not help (review
+ // finding 2): the status code is the diagnostic, so print it.
+ results[i] = result{diag: fmt.Sprintf("not serving (HTTP %d) — instance failing to wake, or route not live yet", resp.StatusCode)}
+ case got == "":
+ // 2xx and no header: something IS being served here, it just
+ // isn't stamped — the only shape that really is an edge from
+ // before X-Carlos-Version existed.
+ results[i] = result{diag: "no X-Carlos-Version header (edge predates this feature — the hourly pass will adopt it)"}
+ default:
+ results[i] = result{diag: "still serving " + got}
+ }
+ }(i, t)
+ }
+ wg.Wait()
+
+ var stillPending []watchTarget
+ for i, r := range results {
+ host := pending[i].Host
+ if r.live {
fmt.Fprintf(w, "%s https://%s\n", colorFor(w).ok("live"), host)
- delete(pending, host)
- case got == ver:
- // The edge stamps X-Carlos-Version before proxying to the
- // origin, so a tenant that dies on boot can still answer
- // with the new version header attached to a non-2xx (a 502,
- // most commonly). That is the new build in a crash loop, not
- // a live deploy, so the success arm has to gate on status
- // too — matching the header alone let a dead tenant exit 0.
- last[host] = fmt.Sprintf("serving %s but HTTP %d — the app is failing", ver, resp.StatusCode)
- case got == "" && resp.StatusCode/100 != 2:
- // A non-2xx with no header is not an old edge — it is an edge
- // that answered and had nothing good to serve: an activator
- // 503 because the wake failed or timed out, or a 404 because
- // the route has not been minted yet. (A 502 from a tenant
- // that died on boot carries the version header — the edge
- // stamps it before proxying — so that shape lands in the
- // case above, not here.) Reporting those as "edge predates
- // this feature — the hourly pass will adopt it" sends the
- // operator off to wait for a pass that will not help (review
- // finding 2): the status code is the diagnostic, so print it.
- last[host] = fmt.Sprintf("not serving (HTTP %d) — instance failing to wake, or route not live yet", resp.StatusCode)
- case got == "":
- // 2xx and no header: something IS being served here, it just
- // isn't stamped — the only shape that really is an edge from
- // before X-Carlos-Version existed.
- last[host] = "no X-Carlos-Version header (edge predates this feature — the hourly pass will adopt it)"
- default:
- last[host] = "still serving " + got
+ continue
}
+ last[host] = r.diag
+ stillPending = append(stillPending, pending[i])
}
+ pending = stillPending
if len(pending) == 0 {
return nil
}
if time.Now().After(deadline) {
hosts := make([]string, 0, len(pending))
- for h := range pending {
- hosts = append(hosts, h)
+ for _, t := range pending {
+ hosts = append(hosts, t.Host)
}
sort.Strings(hosts)
var b strings.Builder
diff --git a/deploy_test.go b/deploy_test.go
index 1f7dac812..6cfeace25 100644
--- a/deploy_test.go
+++ b/deploy_test.go
@@ -7,6 +7,7 @@ import (
"net/http/httptest"
"strings"
"sync"
+ "sync/atomic"
"testing"
"time"
)
@@ -171,6 +172,58 @@ func TestWatchVersionNonOKNoHeaderDiagnostic(t *testing.T) {
}
}
+// TestWatchVersionSweepsTargetsConcurrently pins the concurrency fix: with N
+// targets, a single tick must probe them all in parallel, not one after
+// another. Each origin bumps a counter shared across all three servers on
+// entry and drops it on exit, tracking the high-water mark: sequential
+// probing can never push that mark above 1, while true concurrency pushes it
+// to len(targets). Asserting on the high-water mark rather than a wall-clock
+// budget means CI scheduler noise can slow the run down without flaking the
+// test — only true sequential probing can fail it. The 100ms sleep just
+// needs to be long enough that three goroutines' Gets overlap inside it.
+func TestWatchVersionSweepsTargetsConcurrently(t *testing.T) {
+ const delay = 100 * time.Millisecond
+ var inFlight atomic.Int64
+ var maxInFlight atomic.Int64
+ newSrv := func() *httptest.Server {
+ return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ n := inFlight.Add(1)
+ for {
+ cur := maxInFlight.Load()
+ if n <= cur || maxInFlight.CompareAndSwap(cur, n) {
+ break
+ }
+ }
+ time.Sleep(delay)
+ inFlight.Add(-1)
+ w.Header().Set("X-Carlos-Version", "new5678")
+ }))
+ }
+ srvs := []*httptest.Server{newSrv(), newSrv(), newSrv()}
+ defer func() {
+ for _, s := range srvs {
+ s.Close()
+ }
+ }()
+ targets := make([]watchTarget, len(srvs))
+ for i, s := range srvs {
+ targets[i] = watchTarget{Host: fmt.Sprintf("h%d.test", i), Origin: s.URL}
+ }
+ var out bytes.Buffer
+ err := watchVersion(&out, targets, "new5678", 5*time.Second, 10*time.Millisecond)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if got := maxInFlight.Load(); got != int64(len(targets)) {
+ t.Fatalf("max in-flight = %d, want %d — targets were probed sequentially, not concurrently", got, len(targets))
+ }
+ for _, target := range targets {
+ if !strings.Contains(out.String(), target.Host) {
+ t.Fatalf("live line missing for %s: %q", target.Host, out.String())
+ }
+ }
+}
+
// TestWatchVersionCacheBustsEveryPoll pins the fix for the edge's up-to-5min
// cache TTL on X-Carlos-Version (internal/edge/edge.go, versionHeader): a
// watch with no cache-bust could poll the same cached response for the whole
diff --git a/internal/appconfig/sync.go b/internal/appconfig/sync.go
index 246ec1d54..9738fc2f2 100644
--- a/internal/appconfig/sync.go
+++ b/internal/appconfig/sync.go
@@ -275,14 +275,14 @@ func lostKeys(cur []byte, m *Manifest) (lost []string, parsed bool) {
return lost, true
}
-// syncable mirrors release.Adoptable: lifecycle-managed instances of a
-// known app, excluding platform-minted alias routes (an alias shares its
-// canonical route's process and therefore its canonical route's config —
-// see release.Adoptable's own doc comment for why aliases are excluded
-// from adoption the same way).
+// syncable delegates to release.Adoptable, deliberately: config sync and
+// adopt must agree, to the character, on which routes are lifecycle-managed
+// (a known app, excluding platform-minted alias routes — see
+// release.Adoptable's own doc comment for why aliases are excluded), and a
+// second hand-copied predicate here is exactly the drift that would let the
+// two disagree.
func syncable(rt registry.Route) bool {
- return rt.Kind == "instance" && rt.App != "" && !rt.Platform &&
- (rt.Status == "" || rt.Status == "ready")
+ return release.Adoptable(rt)
}
// UnitConfigApps lists the distinct (account, app) pairs having at least
diff --git a/internal/console/apicli.go b/internal/console/apicli.go
index e9b0d65d1..db48921fd 100644
--- a/internal/console/apicli.go
+++ b/internal/console/apicli.go
@@ -591,6 +591,12 @@ func (s *Server) handleAPITrashApp(w http.ResponseWriter, r *http.Request) {
return
}
s.audit(acct, app, addr, "trash", "moved "+app+" to the trash (cli)")
+ // Box-consumed (carlos-deploy §3), symmetric with the restore door below:
+ // the trash marker is what stops instance minting and drops the app's
+ // routes (provision/reconcile.go, domains/reconcile.go). Without the bump
+ // a trash through this door sits invisible to the fleet until the hourly
+ // floor comes round, while restore already announces in seconds.
+ s.bumpSerial(ctx)
writeJSON(w, http.StatusOK, map[string]any{
"result": "trashed",
"restorable_until": deletedAt.Add(release.TrashCooloff).UTC().Format(time.RFC3339),
diff --git a/internal/console/appsettings.go b/internal/console/appsettings.go
index fcbac4414..3320245fe 100644
--- a/internal/console/appsettings.go
+++ b/internal/console/appsettings.go
@@ -324,6 +324,13 @@ func (s *Server) handleAppDelete(w http.ResponseWriter, r *http.Request) {
return
}
s.audit(acct, app, addr, "trash", "moved "+app+" to the trash")
+ // Box-consumed (carlos-deploy §3), symmetric with the web restore door
+ // (ownership.go, handleRestoreApp): the trash marker is what stops
+ // instance minting and drops the app's routes (provision/reconcile.go,
+ // domains/reconcile.go). Without the bump a trash through this door sits
+ // invisible to the fleet until the hourly floor comes round, while
+ // restore already announces in seconds.
+ s.bumpSerial(ctx)
s.setFlash(w, "ok", "Moved "+app+" to the trash — restorable there until "+
deletedAt.Add(release.TrashCooloff).UTC().Format("2006-01-02")+"; hosts stop serving it on their next pass.")
http.Redirect(w, r, "/a/"+url.PathEscape(acct.Sqid)+"/", http.StatusSeeOther)
diff --git a/internal/console/deletion_test.go b/internal/console/deletion_test.go
index 6e8813d5d..5ffffeaab 100644
--- a/internal/console/deletion_test.go
+++ b/internal/console/deletion_test.go
@@ -379,6 +379,11 @@ func TestDeleteNowDestroysEverythingAndFreesTheName(t *testing.T) {
t.Fatal("objects were destroyed before the passkey confirmation")
}
+ // Purge does not announce: by the time it runs, kass's routes and minting
+ // were already stopped at trash time — there is nothing left a box's fast
+ // pass converges on (see handlePurgeApp, ownership.go).
+ before := serial(t, fx)
+
// With it, the held action resumes and lands.
status, loc, body := completeStepUp(t, fx, c, loc)
if status != http.StatusSeeOther {
@@ -387,6 +392,9 @@ func TestDeleteNowDestroysEverythingAndFreesTheName(t *testing.T) {
if keys := appKeys(t, fx, "kass"); len(keys) != 0 {
t.Fatalf("the bucket still holds %v", keys)
}
+ if got := serial(t, fx); got != before {
+ t.Fatalf("serial after a purge = %d, want unchanged (%d) — purge announces nothing", got, before)
+ }
if has, err := fx.st.HasTrashedAppIn(OpsAccountID, "kass"); err != nil || has {
t.Fatalf("HasTrashedAppIn after purge = %v %v, want the row gone", has, err)
}
diff --git a/internal/console/ownership.go b/internal/console/ownership.go
index 799d8f017..4e91b8bf6 100644
--- a/internal/console/ownership.go
+++ b/internal/console/ownership.go
@@ -246,9 +246,10 @@ func (s *Server) handleRestoreApp(w http.ResponseWriter, r *http.Request) {
// Box-consumed (carlos-deploy §3): removing the trash marker re-enables
// instance minting for this app, and minting from records is leg 1 of the
// fast pass's reconcile. Without the bump a restore is invisible to the
- // fleet until the hourly floor comes round. Restore is the first door on
- // this page to announce its way out of that wait — the trash (purge) side
- // still relies on the next hourly pass; it does not bump.
+ // fleet until the hourly floor comes round. Trash (handleAppDelete) bumps
+ // the same way now — symmetric in and out; purge (handlePurgeApp, below)
+ // still does not, because by then the fleet has nothing left to converge
+ // on.
s.bumpSerial(r.Context())
// The anti-ghost rule, said where the person can read it: restoring brings
// back releases and channels and no routes at all (spec §3), so an app
@@ -316,6 +317,13 @@ func (s *Server) handlePurgeApp(w http.ResponseWriter, r *http.Request) {
fail(purgeAdvice(err, app))
return
}
+ // No bumpSerial here, deliberately: PurgeApp does delete box-parsed
+ // shapes (instance records, config objects) under apps/<sqid>/<app>/,
+ // but the fleet-visible teardown already happened, and was already
+ // announced, at TRASH time (domains/reconcile.go leg 1c,
+ // provision/reconcile.go's trashedFor bumped then). Once the trash
+ // marker itself is gone, a bump here buys nothing — reconcile's
+ // trashed-set no longer contains this app to converge on either way.
// The row LAST, mirroring the bucket-first rule the rest of this feature
// keeps: the row is the name reservation, and freeing a name while objects
// survived would let the next claim ship into another app's prefix.
diff --git a/internal/console/serialbump_test.go b/internal/console/serialbump_test.go
index af46aabff..a12eb61ab 100644
--- a/internal/console/serialbump_test.go
+++ b/internal/console/serialbump_test.go
@@ -248,6 +248,22 @@ func TestAPIRollbackBumpsSerial(t *testing.T) {
}
}
+// TestAPITrashBumpsSerial: trash through the CLI/API door writes the marker
+// that stops instance minting and drops the app's routes (leg 1 of the fast
+// pass's reconcile) — the same reason restore bumps, mirrored on the way in.
+func TestAPITrashBumpsSerial(t *testing.T) {
+ fx, token := trashFixture(t, "code-serial-trash")
+
+ before := serial(t, fx)
+ status, out := postBody(t, fx.ts.URL+"/api/cli/apps/bab/titogo/delete", token, "")
+ if status != http.StatusOK {
+ t.Fatalf("trash = %d %s", status, out)
+ }
+ if got := serial(t, fx); got != before+1 {
+ t.Fatalf("serial after an API trash = %d, want %d", got, before+1)
+ }
+}
+
// TestAPIRestoreBumpsSerial: restore through the CLI/API door re-enables
// instance minting for the app (removing the trash marker), which is leg 1 of
// the fast pass's reconcile — the same reason the web door (TestWebRestoreBumpsSerial)
@@ -405,6 +421,32 @@ func TestWebRestoreBumpsSerial(t *testing.T) {
}
}
+// TestWebTrashBumpsSerial: the web delete door writes the same trash marker
+// the API door does, behind its own passkey ceremony — the announcement must
+// survive the step-up round trip, not just the plain-write path.
+func TestWebTrashBumpsSerial(t *testing.T) {
+ fx := newConsoleFixture(t)
+ c := signInPaul(t, fx)
+ csrf := sessionToken(t, fx, c)
+
+ before := serial(t, fx)
+ status, loc, _ := postForm(t, c, fx.u("/apps/kass/delete"), url.Values{"csrf": {csrf}, "confirm": {"kass"}})
+ if status != http.StatusSeeOther || !strings.HasPrefix(loc, fx.fk.srv.URL) {
+ t.Fatalf("un-stepped-up delete = %d -> %q, want a redirect into Keymail", status, loc)
+ }
+ if got := serial(t, fx); got != before {
+ t.Fatalf("serial before the passkey confirmation = %d, want unchanged (%d)", got, before)
+ }
+
+ status, loc, body := completeStepUp(t, fx, c, loc)
+ if status != http.StatusSeeOther || loc != fx.path("/") {
+ t.Fatalf("resumed delete = %d -> %q, want 303 to the account home:\n%s", status, loc, body)
+ }
+ if got := serial(t, fx); got != before+1 {
+ t.Fatalf("serial after a web trash = %d, want %d", got, before+1)
+ }
+}
+
// cancelAwareStore is a bucket that honours context cancellation, the way the
// AWS SDK does and the DirStore (local disk, no ctx checks at all) does not —
// without it, a canceled-context test here would pass for the wrong reason.
diff --git a/internal/release/versions.go b/internal/release/versions.go
index 1f6063750..bc4f3d001 100644
--- a/internal/release/versions.go
+++ b/internal/release/versions.go
@@ -184,6 +184,28 @@ func StageManifest(optDir, acct, ver, app string, raw []byte) error {
if acct != "" && !ValidAccount(acct) {
return fmt.Errorf("invalid account %q", acct)
}
+ // A version staged by a pre-capability binary has no <acct>/ subdirectory
+ // at all (issue #58): WriteFileAtomic's tmp-file create fails ENOENT the
+ // first time a sibling is back-filled for it under a real account. Create
+ // the parent dir first, exactly as StageArtifact does for the binary —
+ // but only Chmod it when WE are the one creating it. The umask-masking
+ // problem StageArtifact's Chmod exists for (a tight operator umask
+ // leaving the dir non-traversable) only applies to a dir this call
+ // creates; Chmod-ing a dir that already existed is a new failure mode of
+ // its own — a non-owning uid that can still write into the dir (and so
+ // can legitimately reach this branch) gets EPERM trying to chmod it,
+ // and the sibling write below never runs.
+ dir := filepath.Dir(StagedManifestPath(optDir, acct, ver, app))
+ _, statErr := os.Stat(dir)
+ dirExisted := statErr == nil
+ if err := os.MkdirAll(dir, 0o755); err != nil {
+ return err
+ }
+ if !dirExisted {
+ if err := os.Chmod(dir, 0o755); err != nil {
+ return err
+ }
+ }
return WriteFileAtomic(StagedManifestPath(optDir, acct, ver, app), raw, 0o644)
}
diff --git a/internal/release/versions_test.go b/internal/release/versions_test.go
index f67043cda..ceb3c3437 100644
--- a/internal/release/versions_test.go
+++ b/internal/release/versions_test.go
@@ -8,6 +8,7 @@ import (
"os"
"path/filepath"
"strings"
+ "syscall"
"testing"
"time"
@@ -204,6 +205,46 @@ func TestStageManifestRoundTripsCapabilities(t *testing.T) {
}
}
+// TestStageManifestCreatesTheAccountDir pins issue #58: a version staged by
+// a pre-capability binary has NO <acct>/ subdirectory at all (the bare
+// no-account layout StageArtifact(optDir, "", ...) produces — exactly what
+// the old binary wrote). Back-filling a sibling for that version under a
+// real account must not assume the directory already exists; StageManifest
+// has to create it itself, the way StageArtifact already does for the
+// binary, or the write fails ENOENT on every affected box, forever (seen
+// live, hourly, on both production boxes).
+func TestStageManifestCreatesTheAccountDir(t *testing.T) {
+ // Tighten the umask so the account dir StageManifest creates would come
+ // out non-traversable (0700) if the Chmod fix didn't run — otherwise the
+ // perm assertion below would pass even with the umask-masking bug back.
+ old := syscall.Umask(0o077)
+ defer syscall.Umask(old)
+
+ opt := t.TempDir()
+ // Pre-capability layout: versions/<ver>/<app>, no account level at all.
+ if err := StageArtifact(opt, "", "v1abc", "myapp", []byte("binary")); err != nil {
+ t.Fatalf("StageArtifact: %v", err)
+ }
+ if err := StageManifest(opt, "bad", "v1abc", "myapp", []byte("{}")); err != nil {
+ t.Fatalf("StageManifest: %v", err)
+ }
+ p := StagedManifestPath(opt, "bad", "v1abc", "myapp")
+ st, err := os.Stat(p)
+ if err != nil {
+ t.Fatalf("staged manifest: %v", err)
+ }
+ if st.Mode().Perm() != 0o644 {
+ t.Fatalf("staged manifest mode = %v, want 0644", st.Mode().Perm())
+ }
+ dirSt, err := os.Stat(filepath.Dir(p))
+ if err != nil {
+ t.Fatalf("staged account dir: %v", err)
+ }
+ if dirSt.Mode().Perm() != 0o755 {
+ t.Fatalf("staged account dir mode = %v, want 0755", dirSt.Mode().Perm())
+ }
+}
+
// TestReadStagedManifestRunsTheValidationGate pins the Task-2-review
// backstop: a stale sibling written by a NEWER binary — one that understood
// a "v" this binary does not — must refuse cleanly here. This particular