amadan
ship: fix round 1 — mint trap, anchored stamp parse, and three smaller edges
Five review Minors, all in the ship-door refusals.
1. A MINTED version is a dead end for a flagged app: the console chooses
it after the operator linked the binary, so no stamp can match and
rebuilding only loses to the next mint. The refusal now says so and
names -version as the way out. Reachable through `carlos ship` on a
flagged app with a version target; `carlos deploy` defaults -version to
the git short head and never lands here.
2. A MISSING row falls open (AppFlags maps sql.ErrNoRows to "not the
control plane") where an unreadable row fails closed. Comment only, at
the read site: apiAccountApp's 404 runs first, so reaching it means
losing a race with a purge of the app being shipped to, and any fix
belongs in AppFlags with its own callers considered.
3. embeddedBuild claimed a suffix match on the symbol path and implemented
an unanchored search of the whole field, so `-X main.Notes=internal/
version.Build=decoy` reported "decoy" as the build — a false MATCH,
the one direction these checks must not have. Each field is now cut at
its first `=` and the suffix test applies to the path. Table test, with
the decoy row as the regression guard.
4. All three refusals now pin that nothing was written, not just the arch
one: "refused before any byte lands" is the claim the placement makes.
5. elf.NewFile over a bytes.Reader has nothing to close, so that wrapped
refusal was unreachable — dropped. And the architecture sentence
compared a machine to a target name ("is a amd64 binary, not
linux-arm64"); it now compares machine to machine, which is what a
GOARCH fixes.
3 files changed,
+154
−16
internal/console/apicli.go+30 −1internal/console/controlplane.go+32 −15internal/console/controlplane_test.go+92 −0
diff --git a/internal/console/apicli.go b/internal/console/apicli.go| index 553534c94..5328156c1 100644 |
| --- a/internal/console/apicli.go |
| +++ b/internal/console/apicli.go |
| @@ -1415,6 +1415,11 @@ func (s *Server) handleAPIShip(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| ver := strings.TrimSpace(meta.Version) |
| + // Whether THIS console chose the version, rather than the caller. It |
| + // changes what a control-plane refusal can honestly advise below: a |
| + // version minted here cannot possibly be the one stamped into a binary |
| + // the operator built before the request. |
| + minted := ver == "" |
| if ver == "" { |
| // Mint from the app's version target (release versioning spec): no |
| // target, no mint — and the refusal names the fix. This runs inside |
| @@ -1565,6 +1570,18 @@ func (s *Server) handleAPIShip(w http.ResponseWriter, r *http.Request) { |
| // so a row unreadable HERE but readable THERE is a column-level fault on |
| // a console that started successfully — reachable in a test by dropping |
| // one column, not by an ordinary outage. |
| + // |
| + // One asymmetry to know about, and accepted rather than overlooked: a |
| + // MISSING row falls open where an UNREADABLE row fails closed. AppFlags |
| + // maps sql.ErrNoRows to (false, "", nil) — "not the control plane, no |
| + // probe", which is the right reading of an app that no longer exists — |
| + // so an app purged between the gate and this line would ship unchecked. |
| + // It is acceptable for the same reason that mapping exists: apiAccountApp |
| + // has already answered its byte-identical 404 for an app that is gone, so |
| + // reaching here with no row means losing a race with a purge of the very |
| + // app being shipped to, and the release lands in a prefix nothing points |
| + // at. Fixing it belongs in AppFlags with its own callers considered, not |
| + // here. |
| cp, probe, err := s.store.AppFlags(acct.ID, app) |
| if err != nil { |
| log.Printf("console: api ship %s/%s: read app flags: %v", acct.Sqid, app, err) |
| @@ -1586,7 +1603,19 @@ func (s *Server) handleAPIShip(w http.ResponseWriter, r *http.Request) { |
| // than discover the gap later. |
| if cp && meta.Kind != "static" { |
| if err := checkControlPlaneArtifacts(files, ver); err != nil { |
| - jsonError(w, http.StatusBadRequest, err.Error()) |
| + msg := err.Error() |
| + // A MINTED version is a dead end for these checks, and the |
| + // refusal has to say so or it sends the operator in a circle: |
| + // the version was chosen here, after their binary was linked, so |
| + // no stamp could have matched it — and rebuilding with the stamp |
| + // the message names only loses to the NEXT mint. `carlos deploy` |
| + // never lands here (it defaults -version to the git short head), |
| + // but a flagged app with a version target shipped through |
| + // `carlos ship` does. |
| + if minted { |
| + msg += " — and this version was minted at the door, so no build could have carried it: pass -version explicitly for a control-plane app" |
| + } |
| + jsonError(w, http.StatusBadRequest, msg) |
| return |
| } |
| } |
diff --git a/internal/console/controlplane.go b/internal/console/controlplane.go| index 01f1d7f1e..2d2cf265b 100644 |
| --- a/internal/console/controlplane.go |
| +++ b/internal/console/controlplane.go |
| @@ -312,11 +312,12 @@ var targetMachine = map[string]elf.Machine{ |
| "linux-amd64": elf.EM_X86_64, |
| } |
| -// buildLDFlag is the assignment the release recipe links in. Matched as a |
| -// SUFFIX of the symbol path rather than the whole of it, so a vendored or |
| -// renamed module prefix does not silently turn every artifact into "carries |
| -// no version.Build". |
| -const buildLDFlag = "internal/version.Build=" |
| +// buildSymbol is the symbol the release recipe stamps. Matched as a SUFFIX of |
| +// the symbol path rather than the whole of it, so a vendored or renamed module |
| +// prefix does not silently turn every artifact into "carries no |
| +// version.Build" — but as a suffix of the PATH only, never of the whole |
| +// argument (see embeddedBuild). |
| +const buildSymbol = "internal/version.Build" |
| // checkControlPlaneArtifacts refuses two ways a control-plane ship can be |
| // wrong in a manner nothing downstream would catch. |
| @@ -351,17 +352,19 @@ func checkControlPlaneArtifacts(files map[string][]byte, version string) error { |
| if !known { |
| continue |
| } |
| + // No Close: elf.NewFile over a bytes.Reader has nothing to close — |
| + // the file half of debug/elf is elf.Open's, and calling Close here |
| + // would only ever return nil. |
| f, err := elf.NewFile(bytes.NewReader(b)) |
| if err != nil { |
| return fmt.Errorf("artifact %s is not a readable ELF binary: %v", name, err) |
| } |
| - machine := f.Machine |
| - if err := f.Close(); err != nil { |
| - return fmt.Errorf("artifact %s could not be read: %v", name, err) |
| - } |
| - if machine != want { |
| - return fmt.Errorf("artifact %s is a %s binary, not %s — rebuild it with GOOS/GOARCH matching the target it is shipped under", |
| - name, machineName(machine), name) |
| + // Machine against machine, not machine against target name: the |
| + // operator's fix is a GOARCH, so the sentence has to compare the two |
| + // things a GOARCH can be. |
| + if f.Machine != want { |
| + return fmt.Errorf("artifact %s is %s machine code, but target %s wants %s — rebuild it with GOOS/GOARCH matching the target it is shipped under", |
| + name, machineName(f.Machine), name, machineName(want)) |
| } |
| bi, err := buildinfo.Read(bytes.NewReader(b)) |
| if err != nil { |
| @@ -388,17 +391,31 @@ func checkControlPlaneArtifacts(files map[string][]byte, version string) error { |
| // -ldflags, returning "" when the flag is absent — which is exactly the build |
| // that reports "dev" on the wire. |
| // |
| -// Scanned FIELD BY FIELD rather than with one Index over the whole string, so |
| +// Scanned FIELD BY FIELD rather than with one search over the whole string, so |
| // that both spellings the linker accepts are read the same way: `-X` and its |
| // assignment as separate arguments, and `-X=path=value` as one. |
| +// |
| +// The match is ANCHORED: each field is cut at its FIRST `=`, and the suffix |
| +// test applies to the symbol path on the left. Searching the raw field |
| +// instead — which an earlier version did — would let the needle be found |
| +// inside another flag's VALUE (`-X main.Notes=internal/version.Build=decoy`), |
| +// so a binary carrying no build stamp at all could report someone else's |
| +// string as its build. |
| func embeddedBuild(bi *debug.BuildInfo) string { |
| for _, s := range bi.Settings { |
| if s.Key != "-ldflags" { |
| continue |
| } |
| for _, f := range strings.Fields(s.Value) { |
| - if i := strings.Index(f, buildLDFlag); i >= 0 { |
| - return f[i+len(buildLDFlag):] |
| + // The one-argument spelling; the two-argument one arrives here |
| + // as the assignment on its own, with the bare `-X` skipped by |
| + // the Cut below. |
| + path, val, ok := strings.Cut(strings.TrimPrefix(f, "-X="), "=") |
| + if !ok { |
| + continue |
| + } |
| + if path == buildSymbol || strings.HasSuffix(path, "/"+buildSymbol) { |
| + return val |
| } |
| } |
| } |
diff --git a/internal/console/controlplane_test.go b/internal/console/controlplane_test.go| index f1938e034..8964a2108 100644 |
| --- a/internal/console/controlplane_test.go |
| +++ b/internal/console/controlplane_test.go |
| @@ -7,6 +7,7 @@ import ( |
| "os" |
| "os/exec" |
| "path/filepath" |
| + "runtime/debug" |
| "strings" |
| "sync" |
| "testing" |
| @@ -692,6 +693,11 @@ func TestControlPlaneShipRefusesAMismatchedEmbeddedBuild(t *testing.T) { |
| if msg := str(t, out, "error"); !strings.Contains(msg, "notthesha") || !strings.Contains(msg, "cp00004") { |
| t.Fatalf("refusal %q must name both builds", msg) |
| } |
| + // "Refused before any byte is written" is the load-bearing claim of where |
| + // this check sits, so every refusal pins it, not just the first. |
| + if _, err := release.GetManifest(context.Background(), fx.rel, release.OpsAccount, "console", "cp00004"); err == nil { |
| + t.Fatal("the release landed despite the refusal") |
| + } |
| } |
| // TestControlPlaneShipRefusesAMissingBuildStamp is the COMMON half of the |
| @@ -714,6 +720,9 @@ func TestControlPlaneShipRefusesAMissingBuildStamp(t *testing.T) { |
| if !strings.Contains(msg, "cp00006") { |
| t.Fatalf("refusal %q does not say what to stamp the build as", msg) |
| } |
| + if _, err := release.GetManifest(context.Background(), fx.rel, release.OpsAccount, "console", "cp00006"); err == nil { |
| + t.Fatal("the release landed despite the refusal") |
| + } |
| } |
| // TestControlPlaneShipAcceptsAMatchingArtifact. |
| @@ -772,3 +781,86 @@ func TestOrdinaryAppShipIsNotArchChecked(t *testing.T) { |
| t.Fatalf("ordinary app shipping a non-ELF = %d %v, want 201 (unaffected)", status, out) |
| } |
| } |
| + |
| +// TestEmbeddedBuildReadsOnlyTheBuildSymbol is the parser's own table. It |
| +// exists because the field it reads is a STRING the linker recorded, not a |
| +// structured value: everything about "which build is this" rests on cutting |
| +// that string in exactly the right place. |
| +// |
| +// The decoy row is the regression guard. An earlier version searched each |
| +// raw field for "internal/version.Build=" and returned whatever followed, so |
| +// a binary with NO build stamp could report another flag's value as its |
| +// build — a false MATCH, which is the one failure direction these checks |
| +// must never have. |
| +func TestEmbeddedBuildReadsOnlyTheBuildSymbol(t *testing.T) { |
| + const sym = "github.com/carlosframework/platform/internal/version.Build" |
| + cases := []struct { |
| + name string |
| + ldflags string |
| + want string |
| + }{ |
| + {"the release recipe", "-s -w -X " + sym + "=abc1234", "abc1234"}, |
| + {"one-argument spelling", "-X=" + sym + "=abc1234", "abc1234"}, |
| + {"no -X at all (the forgotten ldflag)", "-s -w", ""}, |
| + {"a vendored or renamed prefix still counts", "-X example.com/fork/internal/version.Build=abc1234", "abc1234"}, |
| + {"a decoy inside another flag's value", "-s -w -X main.Notes=internal/version.Build=decoy", ""}, |
| + {"a neighbouring symbol", "-X " + sym + "Date=2026-08-25", ""}, |
| + {"a foreign symbol that merely ends in Build", "-X example.com/app/internal/versionx.Build=nope", ""}, |
| + {"stamp among other stamps", "-X main.who=paul -X " + sym + "=abc1234 -X main.when=now", "abc1234"}, |
| + } |
| + for _, c := range cases { |
| + t.Run(c.name, func(t *testing.T) { |
| + bi := &debug.BuildInfo{Settings: []debug.BuildSetting{ |
| + {Key: "-compiler", Value: "gc"}, |
| + {Key: "-ldflags", Value: c.ldflags}, |
| + }} |
| + if got := embeddedBuild(bi); got != c.want { |
| + t.Fatalf("embeddedBuild(%q) = %q, want %q", c.ldflags, got, c.want) |
| + } |
| + }) |
| + } |
| + // No -ldflags setting at all — a `go build` with no flags, which is the |
| + // shape of every unstamped local build. |
| + if got := embeddedBuild(&debug.BuildInfo{Settings: []debug.BuildSetting{{Key: "-compiler", Value: "gc"}}}); got != "" { |
| + t.Fatalf("embeddedBuild with no -ldflags setting = %q, want \"\"", got) |
| + } |
| +} |
| + |
| +// TestControlPlaneShipRefusalNamesTheMintTrap: a version MINTED at the door |
| +// cannot be in a binary the operator linked before the request, so every |
| +// minted ship of a flagged app is refused — and the refusal has to say why, |
| +// or it sends them in a circle: rebuild with the stamp it names, and the next |
| +// mint moves the target again. |
| +func TestControlPlaneShipRefusalNamesTheMintTrap(t *testing.T) { |
| + fx, token := controlPlaneFixture(t, "code-cp-mint") |
| + liftShipBudget(fx) |
| + ctx := context.Background() |
| + if err := release.SetTarget(ctx, fx.rel, release.OpsAccount, "console", "0.5.0", "t", time.Now().UTC()); err != nil { |
| + t.Fatalf("set target: %v", err) |
| + } |
| + status, out := postShip(t, fx.ts.URL+"/api/cli/apps/bab/console/releases", token, |
| + []shipPart{metaPart(t, "", "binary"), {name: "linux-arm64", body: buildTestELF(t, "linux", "arm64", "cp00009")}}) |
| + if status != http.StatusBadRequest { |
| + t.Fatalf("minted ship of a control-plane app = %d %v, want 400", status, out) |
| + } |
| + msg := str(t, out, "error") |
| + if !strings.Contains(msg, "-version") || !strings.Contains(msg, "minted") { |
| + t.Fatalf("refusal %q must say the version was minted here and that -version is the way out", msg) |
| + } |
| + if _, err := release.GetManifest(ctx, fx.rel, release.OpsAccount, "console", "0.5.0-1"); err == nil { |
| + t.Fatal("the minted release landed despite the refusal") |
| + } |
| + // An ORDINARY app minting is untouched — the trap is the flag's, not the |
| + // mint's, and nothing about minting became worse for a tenant. |
| + if _, err := fx.st.CreateAppIn(OpsAccountID, "titogo", "paul@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := release.SetTarget(ctx, fx.rel, release.OpsAccount, "titogo", "0.5.0", "t", time.Now().UTC()); err != nil { |
| + t.Fatalf("set target: %v", err) |
| + } |
| + status, out = postShip(t, fx.ts.URL+"/api/cli/apps/bab/titogo/releases", token, |
| + []shipPart{metaPart(t, "", "binary"), {name: "linux-arm64", body: []byte("not an ELF at all")}}) |
| + if status != http.StatusCreated { |
| + t.Fatalf("minted ship of an ordinary app = %d %v, want 201", status, out) |
| + } |
| +} |