cac9a6724 · carlos/platform

diff --git a/deploy.go b/deploy.go

index 5eadd7776..797b786a1 100644

--- a/deploy.go

+++ b/deploy.go

@@ -258,9 +258,19 @@ func watchVersion(w io.Writer, targets []watchTarget, ver string, timeout, inter

// 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.

- pending := make([]watchTarget, len(targets))

- copy(pending, targets)

+ // 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 {

+ 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)

diff --git a/deploy_test.go b/deploy_test.go

index 500d2fc6f..6cfeace25 100644

--- a/deploy_test.go

+++ b/deploy_test.go

@@ -7,6 +7,7 @@ import (

"net/http/httptest"

"strings"

"sync"

+ "sync/atomic"

"testing"

"time"

)

@@ -173,15 +174,28 @@ 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. Three origins each sleep 500ms before answering (already live, so

-// one sweep is enough) — sequential probing would take >=1.5s for the sweep

-// alone, parallel finishes in ~500ms. The 1.2s budget leaves generous margin

-// for CI noise while still failing hard on a regression to sequential polling.

+// 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 = 500 * time.Millisecond

+ 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")

}))

}

@@ -196,14 +210,12 @@ func TestWatchVersionSweepsTargetsConcurrently(t *testing.T) {

targets[i] = watchTarget{Host: fmt.Sprintf("h%d.test", i), Origin: s.URL}

}

var out bytes.Buffer

- start := time.Now()

err := watchVersion(&out, targets, "new5678", 5*time.Second, 10*time.Millisecond)

- elapsed := time.Since(start)

if err != nil {

t.Fatal(err)

}

- if elapsed >= 1200*time.Millisecond {

- t.Fatalf("sweep took %v — targets were probed sequentially, not concurrently", elapsed)

+ 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) {

diff --git a/internal/console/ownership.go b/internal/console/ownership.go

index 13574dc54..4e91b8bf6 100644

--- a/internal/console/ownership.go

+++ b/internal/console/ownership.go

@@ -317,13 +317,13 @@ func (s *Server) handlePurgeApp(w http.ResponseWriter, r *http.Request) {

fail(purgeAdvice(err, app))

return

}

- // No bumpSerial here, deliberately: PurgeApp only deletes objects under

- // apps/<sqid>/<app>/ (releases, channels, tags, the production flag, the

- // trash marker itself) for an app whose routes were already torn down and

- // whose minting was already stopped at TRASH time (domains/reconcile.go

- // leg 1c, provision/reconcile.go's trashedFor). There is nothing left

- // here that a box's fast pass converges on — purge is bucket-and-name

- // cleanup, invisible to the fleet either way.

+ // 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/release/versions.go b/internal/release/versions.go

index ef244d415..bc4f3d001 100644

--- a/internal/release/versions.go

+++ b/internal/release/versions.go

@@ -187,14 +187,24 @@ func StageManifest(optDir, acct, ver, app string, raw []byte) error {

// 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

- // (and Chmod — umask masking, see StageArtifact's comment above) the

- // parent dir first, exactly as StageArtifact does for the binary.

+ // 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 err := os.Chmod(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 2599b78c1..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"

@@ -213,6 +214,12 @@ func TestStageManifestRoundTripsCapabilities(t *testing.T) {

// 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 {

@@ -229,6 +236,13 @@ func TestStageManifestCreatesTheAccountDir(t *testing.T) {

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

carlos / platform Public

review fixes: StageManifest chmod scope, purge comment, watch de-dup + structural test