amadan

carlos / platform Public

create doors refuse a host that still holds a deleted record's replica (#199 stopgap)

A deleted instance or sidecar leaves its litestream replica at
instances/<host> and any move staging at moves/<host>/, both keyed by
host alone, so the next record created on that host inherits the old
database on wake or through carlos db pull. Until the full fix lands,
the instance and sidecar create doors list both prefixes in this
console's data bucket (levels 0..9 through the db door's replica seam,
stopping at the first LTX file) after every existing validation and
before the record write, and before the private-storage walk:

- any file: 409, no record written;
- a listing error: 503, never create on uncertainty; the store error is
  logged, not returned;
- no data bucket configured, a fleet placement, or a record homed in
  another region: created unchecked, as before. The gap is documented
  in createleftovers.go.

The check has its own instanceStoreTimeout. Updates, deletes and every
other path are unchanged. The four new user-facing strings are
placeholders pending copy review.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 6cc86d345ae52fd76759b226767807a6e7317186 parent da866a9
4 files changed, +489 −0
  • internal/console/apiinstances.go +13 −0
  • internal/console/apisidecars.go +9 −0
  • internal/console/createleftovers.go +120 −0
  • internal/console/createleftovers_test.go +347 −0

CI passed — run details

diff --git a/internal/console/apiinstances.go b/internal/console/apiinstances.go
index f63f73e21..c737bb25e 100644
--- a/internal/console/apiinstances.go
+++ b/internal/console/apiinstances.go
@@ -997,6 +997,19 @@ func (s *Server) handleAPIInstanceCreate(w http.ResponseWriter, r *http.Request)
jsonError(w, ref.Status, ref.Msg)
return
}
+ }
+ // The leftover-replica rail (#199, createleftovers.go): a host still
+ // holding a deleted instance's replica or move staging is refused, so
+ // the new record cannot inherit that database. Last of the checks, after
+ // every validation above, and BEFORE the store walk below, so a refused
+ // create never provisions a bucket either.
+ if ref := s.leftoverReplicaRefusal(r.Context(), "api instances create "+acct.Sqid+"/"+app, host, region,
+ host+" still holds data from a deleted instance. Choose another address, or ask an operator to purge it.",
+ "could not check whether "+host+" is free. Try again."); ref != nil {
+ jsonError(w, ref.Status, ref.Msg)
+ return
+ }
+ if m.PrivateStorage {
// r.Context(), not ctx: the walk takes storeGrantTimeout, which is
// longer than this handler's own deadline.
storeRec, ok := s.instanceStoreProvision(r.Context(), w, acct, app, rec.Environment, principal)
diff --git a/internal/console/apisidecars.go b/internal/console/apisidecars.go
index 558715819..e28374818 100644
--- a/internal/console/apisidecars.go
+++ b/internal/console/apisidecars.go
@@ -300,6 +300,15 @@ func (s *Server) handleAPISidecarCreate(w http.ResponseWriter, r *http.Request)
jsonError(w, http.StatusBadRequest, err.Error())
return
}
+ // The leftover-replica rail (#199, createleftovers.go): a sidecar's host
+ // is derived from its name, so a name reused after a delete lands on the
+ // deleted sidecar's replica. Refused, after every validation above.
+ if ref := s.leftoverReplicaRefusal(r.Context(), "api sidecars create "+acct.Sqid+"/"+app, rec.Host, region,
+ "the name "+name+" still holds data from a deleted sidecar. Choose another name, or ask an operator to purge it.",
+ "could not check whether the name "+name+" is free. Try again."); ref != nil {
+ jsonError(w, ref.Status, ref.Msg)
+ return
+ }
if err := provision.CreateSidecarRecord(ctx, s.cfg.Releases, rec); err != nil {
if errors.Is(err, release.ErrCASConflict) {
// A lost race against a second create for the same name, or a
diff --git a/internal/console/createleftovers.go b/internal/console/createleftovers.go
new file mode 100644
index 000000000..7391727ce
--- /dev/null
+++ b/internal/console/createleftovers.go
@@ -0,0 +1,120 @@
+// The leftover-replica rail on the instance and sidecar create doors
+// (discussion #199). This is a stopgap, not the fix.
+//
+// A hibernating record's replica lives at instances/<host> and a residence
+// move stages one at moves/<host>/. Both are keyed by HOST alone, and
+// deleting the record leaves both behind (deliberately: retiring a record is
+// not destroying its data). So whoever next creates a record on the same host
+// inherits the old database, on its first wake or through `carlos db pull`.
+// The full fix is being designed separately. Until it lands, the create
+// doors refuse a new record on a host whose replica or staging prefix in this
+// console's data bucket still holds an LTX file, and refuse on uncertainty
+// when the bucket cannot be read.
+//
+// The gap, stated plainly: the check only sees THIS console's region. A
+// record homed in another region, or placed on a customer fleet, is created
+// unchecked, because its data lives in a bucket this console's role cannot
+// read (resolveReplica's residency rule). Leftovers in this region under a
+// host created elsewhere are not seen either, and a later move of that
+// instance into this region would land on top of them. A console with no
+// data bucket configured checks nothing.
+
+package console
+
+import (
+ "context"
+ "fmt"
+ "log"
+ "net/http"
+
+ "github.com/carlosframework/platform/internal/activator"
+ "github.com/carlosframework/platform/internal/dbdoor"
+ "github.com/carlosframework/platform/internal/registry"
+)
+
+// leftoverCheckApplies reports whether a record homed in region can be
+// checked from here: the data bucket is configured, and the record's data
+// would live in it. The residency rule is resolveReplica's: a fleet
+// placement is never this console's to read, whatever its own region string
+// says, and otherwise the record's region must equal the console's ("" ==
+// "" on the home region and on a single-region deployment).
+func (s *Server) leftoverCheckApplies(region string) bool {
+ if s.cfg.HibernateBucket == "" {
+ return false
+ }
+ if _, fleet := registry.FleetPlacement(region); fleet {
+ return false
+ }
+ return region == s.cfg.Region
+}
+
+// hostHoldsReplicaData reports whether host's replica prefix or its move
+// staging prefix holds any LTX file, at any level from 0 to the snapshot
+// level. It stops at the first file. Every error is returned, never treated
+// as "empty": the caller refuses on uncertainty.
+//
+// The listing goes through s.dbReplica, the db door's cached
+// lease.ReadReplicaClient seam, so the reader here and the writer on the box
+// cannot disagree about the bucket's endpoint, region or layout.
+func (s *Server) hostHoldsReplicaData(ctx context.Context, host string) (bool, error) {
+ for _, prefix := range []string{
+ activator.HibernatePrefix(s.cfg.HibernatePrefix, host),
+ activator.MovesPrefix(host),
+ } {
+ rd, err := s.dbReplica(ctx, prefix)
+ if err != nil {
+ return false, fmt.Errorf("replica client for %s: %w", prefix, err)
+ }
+ for level := 0; level <= dbdoor.MaxLevel; level++ {
+ found, err := anyLTXFile(ctx, rd, level)
+ if err != nil {
+ return false, fmt.Errorf("list %s level %d: %w", prefix, level, err)
+ }
+ if found {
+ return true, nil
+ }
+ }
+ }
+ return false, nil
+}
+
+// anyLTXFile reports whether one level lists at least one file.
+func anyLTXFile(ctx context.Context, rd replicaReader, level int) (bool, error) {
+ itr, err := rd.LTXFiles(ctx, level, 0, false)
+ if err != nil {
+ return false, err
+ }
+ defer itr.Close()
+ if itr.Next() {
+ return true, nil
+ }
+ return false, itr.Err()
+}
+
+// leftoverReplicaRefusal is the rail as the doors call it. It answers nil
+// when the create may go ahead: the record is out of this console's sight
+// (leftoverCheckApplies), or the host holds no replica data. Otherwise it
+// answers 409 with taken, or 503 with unsure when the bucket could not be
+// read. The store's own error is logged under logTag and never returned: it
+// carries the bucket name.
+//
+// The check gets its own instanceStoreTimeout, so a slow bucket cannot run
+// on for ever, and runs off the request's context rather than the door's,
+// whose budget earlier reads have already spent some of.
+func (s *Server) leftoverReplicaRefusal(parent context.Context, logTag, host, region, taken, unsure string) *doorRefusal {
+ if !s.leftoverCheckApplies(region) {
+ return nil
+ }
+ ctx, cancel := context.WithTimeout(parent, instanceStoreTimeout)
+ defer cancel()
+ held, err := s.hostHoldsReplicaData(ctx, host)
+ if err != nil {
+ log.Printf("console: %s: leftover replica check for %s: %v", logTag, host, err)
+ return &doorRefusal{http.StatusServiceUnavailable, unsure}
+ }
+ if held {
+ log.Printf("console: %s: refused: %s still holds replica data from a deleted record (#199); purge it to free the host", logTag, host)
+ return &doorRefusal{http.StatusConflict, taken}
+ }
+ return nil
+}
diff --git a/internal/console/createleftovers_test.go b/internal/console/createleftovers_test.go
new file mode 100644
index 000000000..0e818c6ba
--- /dev/null
+++ b/internal/console/createleftovers_test.go
@@ -0,0 +1,347 @@
+package console
+
+// The leftover-replica rail on the two create doors (discussion #199). A
+// deleted instance or sidecar leaves its replica at instances/<host> and any
+// move staging at moves/<host>/, and both are keyed by HOST alone, so the next
+// record created on that host would inherit the old database on its first
+// wake or through `carlos db pull`. Until the full fix lands, both doors
+// refuse a create on a host whose replica or staging prefix in this
+// console's data bucket still holds an LTX file. These tests use real
+// litestream replicas on disk behind the db door's replica seam, so "holds an
+// LTX file" means exactly what the activator would restore from.
+
+import (
+ "context"
+ "errors"
+ "io"
+ "net/http"
+ "os"
+ "path/filepath"
+ "strings"
+ "sync/atomic"
+ "testing"
+
+ lsfile "github.com/benbjohnson/litestream/file"
+ "github.com/superfly/ltx"
+
+ "github.com/carlosframework/platform/internal/activator"
+ "github.com/carlosframework/platform/internal/dbdoor/dbdoortest"
+ "github.com/carlosframework/platform/internal/provision"
+)
+
+type leftoverFx struct {
+ *consoleFixture
+ token string
+ root string // the fake data bucket: one file replica per prefix
+ opens *atomic.Int32 // how many times a door reached for a replica client
+}
+
+// leftoverFixture is a console in region ie with a data bucket configured,
+// and titogo enabled for both instances and sidecars in ie.
+func leftoverFixture(t *testing.T, code string) *leftoverFx {
+ t.Helper()
+ fx, token := instanceFixture(t, code)
+ enableInstances(t, fx, "titogo", titoManifest())
+ enableSidecars(t, fx, "titogo", titoSidecarManifest())
+ root := t.TempDir()
+ fx.srv.cfg.HibernateBucket = "data-test"
+ fx.srv.cfg.HibernatePrefix = "instances"
+ fx.srv.cfg.Region = "ie"
+ opens := new(atomic.Int32)
+ fx.srv.dbReplica = func(_ context.Context, prefix string) (replicaReader, error) {
+ opens.Add(1)
+ return lsfile.NewReplicaClient(filepath.Join(root, prefix)), nil
+ }
+ return &leftoverFx{consoleFixture: fx, token: token, root: root, opens: opens}
+}
+
+// leaveReplica writes a real litestream replica where a deleted record's
+// activator left it.
+func (fx *leftoverFx) leaveReplica(t *testing.T, host string) {
+ t.Helper()
+ dbdoortest.Replicate(t, filepath.Join(fx.root, activator.HibernatePrefix("instances", host)), 1)
+}
+
+// leaveStaging writes one where an abandoned move staged it.
+func (fx *leftoverFx) leaveStaging(t *testing.T, host string) {
+ t.Helper()
+ dbdoortest.Replicate(t, filepath.Join(fx.root, activator.MovesPrefix(host)), 1)
+}
+
+// createDoor is one of the two doors, reduced to what the matrix needs.
+type createDoor struct {
+ name string
+ url func(fx *consoleFixture) string
+ body string // a create the door accepts on a clean host, in region ie
+ host string // the host that create claims
+ exists func(t *testing.T, fx *consoleFixture) bool
+}
+
+var leftoverDoors = []createDoor{
+ {
+ name: "instance",
+ url: func(fx *consoleFixture) string { return instancesURL(fx, "bab", "titogo") },
+ body: `{"host":"acme.go.tito.io","region":"ie"}`,
+ host: "acme.go.tito.io",
+ exists: func(t *testing.T, fx *consoleFixture) bool {
+ t.Helper()
+ _, err := provision.GetRecord(context.Background(), fx.rel, "bab", "titogo", "acme.go.tito.io")
+ if err != nil && !errors.Is(err, os.ErrNotExist) {
+ t.Fatal(err)
+ }
+ return err == nil
+ },
+ },
+ {
+ name: "sidecar",
+ url: func(fx *consoleFixture) string { return sidecarsURL(fx, "bab", "titogo") },
+ body: `{"name":"bot","region":"ie"}`,
+ host: botHost,
+ exists: func(t *testing.T, fx *consoleFixture) bool {
+ t.Helper()
+ _, err := provision.GetSidecarRecord(context.Background(), fx.rel, "bab", "titogo", "bot")
+ if err != nil && !errors.Is(err, os.ErrNotExist) {
+ t.Fatal(err)
+ }
+ return err == nil
+ },
+ },
+}
+
+// A host whose replica prefix still holds LTX files is refused with 409,
+// and the refusal writes no record.
+func TestCreateRefusesLeftoverReplica(t *testing.T) {
+ for _, d := range leftoverDoors {
+ t.Run(d.name, func(t *testing.T) {
+ fx := leftoverFixture(t, "code-left-rep-"+d.name)
+ fx.leaveReplica(t, d.host)
+ status, _, out := rawReq(t, http.MethodPost, d.url(fx.consoleFixture), fx.token, d.body)
+ if status != http.StatusConflict || !strings.Contains(out, "still holds data from a deleted") {
+ t.Fatalf("create = %d %s, want the leftover-data 409", status, out)
+ }
+ if d.exists(t, fx.consoleFixture) {
+ t.Fatal("a refused create wrote a record anyway")
+ }
+ })
+ }
+}
+
+// Move staging counts too: a host whose only leftover is an abandoned
+// moves/<host>/ is refused the same way, because a landing promotes that
+// staging into the instance's prefix.
+func TestCreateRefusesLeftoverMoveStaging(t *testing.T) {
+ for _, d := range leftoverDoors {
+ t.Run(d.name, func(t *testing.T) {
+ fx := leftoverFixture(t, "code-left-mov-"+d.name)
+ fx.leaveStaging(t, d.host)
+ status, _, out := rawReq(t, http.MethodPost, d.url(fx.consoleFixture), fx.token, d.body)
+ if status != http.StatusConflict || !strings.Contains(out, "still holds data from a deleted") {
+ t.Fatalf("create = %d %s, want the leftover-data 409", status, out)
+ }
+ if d.exists(t, fx.consoleFixture) {
+ t.Fatal("a refused create wrote a record anyway")
+ }
+ })
+ }
+}
+
+// A clean host is created as before, and the check really looked: both
+// prefixes were opened.
+func TestCreateAllowsACleanHost(t *testing.T) {
+ for _, d := range leftoverDoors {
+ t.Run(d.name, func(t *testing.T) {
+ fx := leftoverFixture(t, "code-left-clean-"+d.name)
+ // Another host's leftovers are not this host's business.
+ fx.leaveReplica(t, "other.go.tito.io")
+ status, _, out := rawReq(t, http.MethodPost, d.url(fx.consoleFixture), fx.token, d.body)
+ if status != http.StatusCreated {
+ t.Fatalf("create = %d %s, want 201", status, out)
+ }
+ if !d.exists(t, fx.consoleFixture) {
+ t.Fatal("the create answered 201 and wrote no record")
+ }
+ if n := fx.opens.Load(); n != 2 {
+ t.Fatalf("replica clients opened = %d, want 2 (the replica prefix and the staging prefix)", n)
+ }
+ })
+ }
+}
+
+// A record homed in another region is not checked: its data lives in that
+// region's bucket, which this console cannot read. The gap is documented at
+// the rail; this pins that the rail does not refuse what it cannot see.
+func TestCreateSkipsTheCheckInAnotherRegion(t *testing.T) {
+ for _, d := range leftoverDoors {
+ t.Run(d.name, func(t *testing.T) {
+ fx := leftoverFixture(t, "code-left-region-"+d.name)
+ fx.srv.cfg.Region = "uk"
+ fx.leaveReplica(t, d.host)
+ status, _, out := rawReq(t, http.MethodPost, d.url(fx.consoleFixture), fx.token, d.body)
+ if status != http.StatusCreated {
+ t.Fatalf("create = %d %s, want 201 (unchecked)", status, out)
+ }
+ if n := fx.opens.Load(); n != 0 {
+ t.Fatalf("replica clients opened = %d for an out-of-region record, want 0", n)
+ }
+ })
+ }
+}
+
+// A console with no data bucket creates as it always did, and never reaches
+// for a replica client.
+func TestCreateSkipsTheCheckWithNoDataBucket(t *testing.T) {
+ for _, d := range leftoverDoors {
+ t.Run(d.name, func(t *testing.T) {
+ fx := leftoverFixture(t, "code-left-nobucket-"+d.name)
+ fx.srv.cfg.HibernateBucket = ""
+ fx.leaveReplica(t, d.host)
+ status, _, out := rawReq(t, http.MethodPost, d.url(fx.consoleFixture), fx.token, d.body)
+ if status != http.StatusCreated {
+ t.Fatalf("create = %d %s, want 201 (unchecked)", status, out)
+ }
+ if n := fx.opens.Load(); n != 0 {
+ t.Fatalf("replica clients opened = %d with no bucket, want 0", n)
+ }
+ })
+ }
+}
+
+// failingReplica fails every listing, with a message carrying a bucket name
+// the way a real S3 error does.
+type failingReplica struct{}
+
+func (failingReplica) LTXFiles(context.Context, int, ltx.TXID, bool) (ltx.FileIterator, error) {
+ return nil, errors.New("list secret-bucket: connection reset")
+}
+
+func (failingReplica) OpenLTXFile(context.Context, int, ltx.TXID, ltx.TXID, int64, int64) (io.ReadCloser, error) {
+ return nil, errors.New("not used")
+}
+
+// erroringIterator fails on the first Next, the way the S3 client surfaces
+// a paging error.
+type erroringIterator struct{}
+
+func (erroringIterator) Close() error { return nil }
+func (erroringIterator) Next() bool { return false }
+func (erroringIterator) Err() error { return errors.New("page secret-bucket: timeout") }
+func (erroringIterator) Item() *ltx.FileInfo { return nil }
+
+type pagingFailReplica struct{ failingReplica }
+
+func (pagingFailReplica) LTXFiles(context.Context, int, ltx.TXID, bool) (ltx.FileIterator, error) {
+ return erroringIterator{}, nil
+}
+
+// A check that cannot complete refuses with 503: never create on
+// uncertainty. The store's own error, which names the bucket, stays in the
+// log.
+func TestCreateRefusesWhenTheCheckFails(t *testing.T) {
+ seams := map[string]func(context.Context, string) (replicaReader, error){
+ "client": func(context.Context, string) (replicaReader, error) {
+ return nil, errors.New("resolve region for secret-bucket: no credentials")
+ },
+ "listing": func(context.Context, string) (replicaReader, error) { return failingReplica{}, nil },
+ "paging": func(context.Context, string) (replicaReader, error) { return pagingFailReplica{}, nil },
+ }
+ for _, d := range leftoverDoors {
+ for seam, open := range seams {
+ t.Run(d.name+"/"+seam, func(t *testing.T) {
+ fx := leftoverFixture(t, "code-left-fail-"+d.name+"-"+seam)
+ fx.srv.dbReplica = open
+ status, _, out := rawReq(t, http.MethodPost, d.url(fx.consoleFixture), fx.token, d.body)
+ if status != http.StatusServiceUnavailable || !strings.Contains(out, "could not check whether") {
+ t.Fatalf("create = %d %s, want the 503 that refuses on uncertainty", status, out)
+ }
+ if strings.Contains(out, "secret-bucket") || strings.Contains(out, "data-test") {
+ t.Fatalf("the refusal leaked a bucket name: %s", out)
+ }
+ if d.exists(t, fx.consoleFixture) {
+ t.Fatal("a create that could not be checked wrote a record anyway")
+ }
+ })
+ }
+ }
+}
+
+// Every existing validation answers first: a create the door would refuse
+// anyway never reaches for the bucket, and keeps its own answer.
+func TestCreateValidatesBeforeTheCheck(t *testing.T) {
+ // consoleRegion is the region the rail would check, so a check run
+ // too early would reach for the bucket rather than skip.
+ cases := []struct{ door, url, body, consoleRegion string }{
+ {"instance", "instances", `{"host":"acme.elsewhere.io","region":"ie"}`, "ie"},
+ {"instance", "instances", `{"host":"acme.go.tito.io","region":"mars"}`, "mars"},
+ {"sidecar", "sidecars", `{"name":"../bot","region":"ie"}`, "ie"},
+ {"sidecar", "sidecars", `{"name":"bot","region":"mars"}`, "mars"},
+ }
+ for _, c := range cases {
+ t.Run(c.door+" "+c.body, func(t *testing.T) {
+ fx := leftoverFixture(t, "code-left-order")
+ fx.srv.cfg.Region = c.consoleRegion
+ fx.srv.dbReplica = func(context.Context, string) (replicaReader, error) {
+ fx.opens.Add(1)
+ return failingReplica{}, nil
+ }
+ u := fx.ts.URL + "/api/cli/apps/bab/titogo/" + c.url
+ status, _, out := rawReq(t, http.MethodPost, u, fx.token, c.body)
+ if status != http.StatusBadRequest {
+ t.Fatalf("create = %d %s, want the door's own 400", status, out)
+ }
+ if n := fx.opens.Load(); n != 0 {
+ t.Fatalf("an invalid create reached for the bucket %d time(s)", n)
+ }
+ })
+ }
+}
+
+// Paths that create no record are untouched: deleting a record whose host
+// holds a replica (the ordinary case) never consults the rail.
+func TestDeleteNeverConsultsTheLeftoverCheck(t *testing.T) {
+ for _, d := range leftoverDoors {
+ t.Run(d.name, func(t *testing.T) {
+ fx := leftoverFixture(t, "code-left-del-"+d.name)
+ if status, _, out := rawReq(t, http.MethodPost, d.url(fx.consoleFixture), fx.token, d.body); status != http.StatusCreated {
+ t.Fatalf("create = %d %s", status, out)
+ }
+ fx.leaveReplica(t, d.host)
+ fx.opens.Store(0)
+ target := d.url(fx.consoleFixture) + "/" + d.host
+ if d.name == "sidecar" {
+ target = d.url(fx.consoleFixture) + "/bot"
+ }
+ if status, _, out := rawReq(t, http.MethodDelete, target, fx.token, ""); status >= 300 {
+ t.Fatalf("delete = %d %s, want success", status, out)
+ }
+ if n := fx.opens.Load(); n != 0 {
+ t.Fatalf("a delete reached for the bucket %d time(s)", n)
+ }
+ })
+ }
+}
+
+// The residency rule the rail applies is resolveReplica's: in-region only,
+// and a fleet placement is never this console's to read, whatever its own
+// region string says.
+func TestLeftoverCheckResidency(t *testing.T) {
+ s := &Server{}
+ s.cfg.HibernateBucket = "data-test"
+ s.cfg.Region = "fleet/acme"
+ for _, c := range []struct {
+ region, console string
+ bucket string
+ want bool
+ }{
+ {"", "", "data-test", true},
+ {"ie", "ie", "data-test", true},
+ {"uk", "ie", "data-test", false},
+ {"", "ie", "data-test", false},
+ {"fleet/acme", "fleet/acme", "data-test", false},
+ {"ie", "ie", "", false},
+ } {
+ s.cfg.Region, s.cfg.HibernateBucket = c.console, c.bucket
+ if got := s.leftoverCheckApplies(c.region); got != c.want {
+ t.Errorf("record %q on console %q (bucket %q) = %v, want %v", c.region, c.console, c.bucket, got, c.want)
+ }
+ }
+}