amadan

carlos / platform Public

legacy-routes assign: audit rows in the apps' own trails (Plan 8 Task 5 fix round 1)

Spec §7 lists legacy-route-assign under what a member sees. The system row
stays in (ops account, "platform"); each change now also writes SD-N106's
row to the target app's trail, and on a re-assignment to the previous
owner's, through s.audit, which handleAppActivity reads back.

Tests: the audit rows where a member's activity listing reads them; the
non-busy re-assignment checks the file; the undecodable-record test checks
the file is unchanged; the concurrent test reports errors from the test
goroutine and releases every request at once. The m12 comment states both
orders of the race.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude e041cc1b95d479a17d25a4117ecdb6fe62893484 parent 4f92001
2 files changed, +173 −20
  • internal/console/apilegacyroutes.go +38 −14
  • internal/console/apilegacyroutes_test.go +135 −6

CI cancelled — superseded by a newer push — run details

diff --git a/internal/console/apilegacyroutes.go b/internal/console/apilegacyroutes.go
index 7c0e51057..ae6cf8175 100644
--- a/internal/console/apilegacyroutes.go
+++ b/internal/console/apilegacyroutes.go
@@ -79,7 +79,7 @@ func (s *Server) handleAPISystemLegacyRoutes(w http.ResponseWriter, r *http.Requ
ctx, cancel := context.WithTimeout(r.Context(), instanceStoreTimeout)
defer cancel()
want := seeds.LegacyOwner{Account: acct.Sqid, App: app}
- changed, err := assignLegacyRoute(ctx, s.cfg.Releases, name, want)
+ changed, prev, err := assignLegacyRoute(ctx, s.cfg.Releases, name, want)
switch {
case errors.Is(err, errLegacyRouteBusy):
jsonError(w, http.StatusConflict, fmt.Sprintf("%s has an import in flight under its current assignment. Commit or abort it first.", name))
@@ -96,48 +96,72 @@ func (s *Server) handleAPISystemLegacyRoutes(w http.ResponseWriter, r *http.Requ
return
}
if changed {
- s.auditSystem(addr, "legacy-route-assign", fmt.Sprintf("assigned the legacy routes named %s to %s/%s", name, want.Account, want.App))
+ detail := fmt.Sprintf("assigned the legacy routes named %s to %s/%s", name, want.Account, want.App)
+ s.auditSystem(addr, "legacy-route-assign", detail)
+ // Spec §7 lists the row under what a member sees: the app the name
+ // now belongs to, and the app it was taken from, each get it in the
+ // audit trail their activity tab reads.
+ s.audit(acct, app, addr, "legacy-route-assign", detail)
+ if prev != (seeds.LegacyOwner{}) {
+ s.auditPreviousLegacyOwner(prev, addr, detail)
+ }
s.bumpSerial(ctx)
}
writeJSON(w, http.StatusOK, map[string]string{"name": name, "account": want.Account, "app": want.App})
}
+// auditPreviousLegacyOwner writes the assignment's audit row to the app a
+// re-assignment moved the name away from. Best effort, as every audit row
+// is: an account that no longer exists has no trail to write to.
+func (s *Server) auditPreviousLegacyOwner(prev seeds.LegacyOwner, addr, detail string) {
+ acct, err := s.store.AccountBySqid(prev.Account)
+ if err != nil || acct == nil {
+ log.Printf("console: legacy routes: audit the previous owner %s/%s: account %v, %v", prev.Account, prev.App, acct, err)
+ return
+ }
+ s.audit(acct, prev.App, addr, "legacy-route-assign", detail)
+}
+
// assignLegacyRoute maps name to want in the assignment file. It reads the
// file through the swap's own strict reader, and writes it by CAS on the
// etag that read returned (create-only when the file was absent), so two
// assigns at once never lose one: the loser gets release.ErrCASConflict and
// nothing is retried behind the operator's back. The same mapping again is
-// no write at all (changed false).
+// no write at all (changed false). prev is the entry a write replaced, or
+// the zero owner when the name had none.
//
-// The in-flight check and the write are not one atomic step: an import of
-// the old mapping can reach ready between them. The swap then compares
-// against the new entry and refuses the row (SD-N94), which fails closed.
-func assignLegacyRoute(ctx context.Context, rs release.Store, name string, want seeds.LegacyOwner) (changed bool, err error) {
+// The in-flight check and the write are not one atomic step, and both
+// orders fail closed. An import of the old mapping that reaches ready
+// between them meets the new entry at its swap and refuses the row
+// (SD-N94). One whose box swaps between them read the old entry, which it
+// was authorized under, and nothing after the swap reads the file again.
+func assignLegacyRoute(ctx context.Context, rs release.Store, name string, want seeds.LegacyOwner) (changed bool, prev seeds.LegacyOwner, err error) {
owners, etag, err := seeds.ReadLegacyOwners(ctx, rs)
if err != nil {
- return false, fmt.Errorf("%w: %w", errLegacyRoutesUnread, err)
+ return false, seeds.LegacyOwner{}, fmt.Errorf("%w: %w", errLegacyRoutesUnread, err)
}
if cur, ok := owners.Lookup(name); ok {
if cur == want {
- return false, nil
+ return false, seeds.LegacyOwner{}, nil
}
busy, err := importInFlight(ctx, rs, cur)
if err != nil {
- return false, fmt.Errorf("%w: in-flight check of %s/%s: %w", errLegacyRoutesUnread, cur.Account, cur.App, err)
+ return false, seeds.LegacyOwner{}, fmt.Errorf("%w: in-flight check of %s/%s: %w", errLegacyRoutesUnread, cur.Account, cur.App, err)
}
if busy {
- return false, errLegacyRouteBusy
+ return false, seeds.LegacyOwner{}, errLegacyRouteBusy
}
+ prev = cur
}
owners.Names[name] = want
body, err := json.Marshal(owners)
if err != nil {
- return false, err
+ return false, seeds.LegacyOwner{}, err
}
if _, err := rs.Put(ctx, seeds.LegacyOwnersKey, body, etag); err != nil {
- return false, fmt.Errorf("write %s: %w", seeds.LegacyOwnersKey, err)
+ return false, seeds.LegacyOwner{}, fmt.Errorf("write %s: %w", seeds.LegacyOwnersKey, err)
}
- return true, nil
+ return true, prev, nil
}
// importInFlight reports whether any record of o's account and app holds an
diff --git a/internal/console/apilegacyroutes_test.go b/internal/console/apilegacyroutes_test.go
index cd8e684ac..f5878f12a 100644
--- a/internal/console/apilegacyroutes_test.go
+++ b/internal/console/apilegacyroutes_test.go
@@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
+ "io"
"net/http"
"strings"
"sync"
@@ -49,13 +50,19 @@ func (fx *legacyFx) assign(t *testing.T, name, to string) (int, map[string]any)
// legacyImportRecord files an import record of cdef/shop in state.
func legacyImportRecord(t *testing.T, rs release.Store, host, state string) {
+ t.Helper()
+ legacyImportRecordIn(t, rs, "cdef", host, state)
+}
+
+// legacyImportRecordIn files an import record of <acct>/shop in state.
+func legacyImportRecordIn(t *testing.T, rs release.Store, acct, host, state string) {
t.Helper()
id, err := seeds.NewID()
if err != nil {
t.Fatal(err)
}
now := time.Now().UTC()
- rec := provision.Record{Schema: 1, Host: host, Slug: strings.Split(host, ".")[0], App: "shop", Account: "cdef", Channel: "stable",
+ rec := provision.Record{Schema: 1, Host: host, Slug: strings.Split(host, ".")[0], App: "shop", Account: acct, Channel: "stable",
CreatedAt: now, Seed: &provision.SeedSource{ID: id, Kind: seeds.KindImport, State: state, By: "t", At: now}}
if err := provision.CreateRecord(context.Background(), rs, rec); err != nil {
t.Fatal(err)
@@ -176,9 +183,12 @@ func TestAssignRefusedWhileAnImportIsInFlight(t *testing.T) {
legacyImportRecord(t, fx.rel, "old.example.com", c.state)
st, out := fx.assign(t, "shop", "cdef/other")
if !c.busy {
- if st != http.StatusOK {
+ if st != http.StatusOK || out["app"] != "other" {
t.Fatalf("re-assign past an import in %s = %d %v", c.state, st, out)
}
+ if got, ok := readOwner(t, fx.rel, "shop"); !ok || got != (seeds.LegacyOwner{Account: "cdef", App: "other"}) {
+ t.Fatalf("re-assign past an import in %s left %+v %v", c.state, got, ok)
+ }
return
}
if st != http.StatusConflict || str(t, out, "error") != "shop has an import in flight under its current assignment. Commit or abort it first." {
@@ -208,6 +218,9 @@ func TestAssignCountsAnUndecodableRecordAsBusy(t *testing.T) {
if st, out := fx.assign(t, "shop", "cdef/other"); st != http.StatusConflict || !strings.Contains(str(t, out, "error"), "has an import in flight") {
t.Fatalf("re-assign past an undecodable record = %d %v", st, out)
}
+ if got, ok := readOwner(t, fx.rel, "shop"); !ok || got != (seeds.LegacyOwner{Account: "cdef", App: "shop"}) {
+ t.Fatalf("the file changed: %+v %v", got, ok)
+ }
}
// A file the strict reader refuses is never read as "no assignment".
@@ -284,17 +297,33 @@ func TestConcurrentAssignsLoseNoName(t *testing.T) {
msg string
}
results := make(chan result, len(names))
+ errs := make(chan error, len(names))
+ start := make(chan struct{}) // every request leaves at once
var wg sync.WaitGroup
for _, n := range names {
wg.Add(1)
go func() {
defer wg.Done()
- st, body := postRaw(t, fx.url, map[string]string{"name": n, "to": "cdef/" + n}, fx.owner)
+ <-start
+ // No t.Fatal off the test goroutine: errors are reported below.
+ st, body, err := postFromGoroutine(fx.url, fmt.Sprintf(`{"name":%q,"to":"cdef/%s"}`, n, n), fx.owner)
+ if err != nil {
+ errs <- fmt.Errorf("%s: %w", n, err)
+ return
+ }
results <- result{n, st, body}
}()
}
+ close(start)
wg.Wait()
close(results)
+ close(errs)
+ for err := range errs {
+ t.Error(err)
+ }
+ if t.Failed() {
+ t.FailNow()
+ }
o, _, err := seeds.ReadLegacyOwners(context.Background(), fx.rel)
if err != nil {
t.Fatal(err)
@@ -323,6 +352,24 @@ func TestConcurrentAssignsLoseNoName(t *testing.T) {
}
}
+// postFromGoroutine is postRaw for a goroutine that is not the test's: it
+// returns its error instead of failing the test.
+func postFromGoroutine(u, body, bearer string) (int, string, error) {
+ req, err := http.NewRequest(http.MethodPost, u, strings.NewReader(body))
+ if err != nil {
+ return 0, "", err
+ }
+ req.Header.Set("Content-Type", "application/json")
+ req.Header.Set("Authorization", "Bearer "+bearer)
+ res, err := http.DefaultClient.Do(req)
+ if err != nil {
+ return 0, "", err
+ }
+ defer res.Body.Close()
+ out, err := io.ReadAll(res.Body)
+ return res.StatusCode, string(out), err
+}
+
// interleaveStore runs between once, on the first read of the assignment
// file, before the caller's write: the shape of two assigns at once.
type interleaveStore struct {
@@ -351,11 +398,11 @@ func TestAssignLosesARaceByCAS(t *testing.T) {
}
rs := &interleaveStore{Store: fx.rel}
rs.between = func() {
- if _, err := assignLegacyRoute(ctx, fx.rel, "other", seeds.LegacyOwner{Account: "cdef", App: "other"}); err != nil {
+ if _, _, err := assignLegacyRoute(ctx, fx.rel, "other", seeds.LegacyOwner{Account: "cdef", App: "other"}); err != nil {
t.Errorf("the racing assign: %v", err)
}
}
- changed, err := assignLegacyRoute(ctx, rs, "shop", seeds.LegacyOwner{Account: "cdef", App: "shop"})
+ changed, _, err := assignLegacyRoute(ctx, rs, "shop", seeds.LegacyOwner{Account: "cdef", App: "shop"})
if changed || !errors.Is(err, release.ErrCASConflict) {
t.Fatalf("the losing assign = %v, want a CAS conflict", err)
}
@@ -414,7 +461,7 @@ func TestAssignFailsClosedOnAnIncompleteListing(t *testing.T) {
t.Fatalf("assign = %d %v", st, out)
}
legacyImportRecord(t, fx.rel, "old.example.com", seeds.StateCommitted)
- changed, err := assignLegacyRoute(ctx, c.rs(fx.rel), "shop", seeds.LegacyOwner{Account: "cdef", App: "other"})
+ changed, _, err := assignLegacyRoute(ctx, c.rs(fx.rel), "shop", seeds.LegacyOwner{Account: "cdef", App: "other"})
if changed || !errors.Is(err, errLegacyRoutesUnread) || errors.Is(err, errLegacyRouteBusy) {
t.Fatalf("re-assign over an incomplete listing = %v, want unread and not busy", err)
}
@@ -424,3 +471,85 @@ func TestAssignFailsClosedOnAnIncompleteListing(t *testing.T) {
})
}
}
+
+// Spec §7: legacy-route-assign is something a member sees. The row lands in
+// the audit trail the app's activity tab reads (Events(account, app), as
+// handleAppActivity calls it) for the app the name now belongs to and, on a
+// re-assignment, for the app it was taken from. The system row stays.
+func TestAssignAuditsTheAppsItMovesBetween(t *testing.T) {
+ fx := legacyFixture(t, "code-assign-audit")
+ const thirdID = 4343
+ if err := fx.st.CreateAccount(thirdID, "efgh", "Third", "third@keymail.dev", time.Now()); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := fx.st.CreateAppIn(thirdID, "shop", "third@keymail.dev", time.Now()); err != nil {
+ t.Fatal(err)
+ }
+ trail := func(acctID int64, app string) []string {
+ t.Helper()
+ events, err := fx.st.Events(acctID, app, activityCap)
+ if err != nil {
+ t.Fatal(err)
+ }
+ var out []string
+ for _, e := range events {
+ if e.Action == "legacy-route-assign" {
+ if e.Address != "paul@keymail.dev" {
+ t.Errorf("%d/%s: a row by %q", acctID, app, e.Address)
+ }
+ out = append(out, e.Detail)
+ }
+ }
+ return out
+ }
+ first := "assigned the legacy routes named shop to cdef/shop"
+ moved := "assigned the legacy routes named shop to efgh/shop"
+ if st, out := fx.assign(t, "shop", "cdef/shop"); st != http.StatusOK {
+ t.Fatalf("assign = %d %v", st, out)
+ }
+ if got := trail(legacyOtherID, "shop"); len(got) != 1 || got[0] != first {
+ t.Fatalf("cdef/shop's trail after the first assign = %q", got)
+ }
+ if got := trail(thirdID, "shop"); len(got) != 0 {
+ t.Fatalf("efgh/shop's trail before it is named = %q", got)
+ }
+
+ // The same mapping again writes no row anywhere.
+ if st, out := fx.assign(t, "shop", "cdef/shop"); st != http.StatusOK {
+ t.Fatalf("again = %d %v", st, out)
+ }
+ if got := trail(legacyOtherID, "shop"); len(got) != 1 {
+ t.Fatalf("the no-op wrote a row: %q", got)
+ }
+
+ // The move: the new owner and the previous owner both see it.
+ if st, out := fx.assign(t, "shop", "efgh/shop"); st != http.StatusOK {
+ t.Fatalf("move = %d %v", st, out)
+ }
+ if got := trail(thirdID, "shop"); len(got) != 1 || got[0] != moved {
+ t.Fatalf("efgh/shop's trail = %q, want the move", got)
+ }
+ if got := trail(legacyOtherID, "shop"); len(got) != 2 || got[0] != moved || got[1] != first {
+ t.Fatalf("cdef/shop's trail = %q, want the move above the first assign", got)
+ }
+ // No other app's trail, and no account-wide row, picks it up.
+ if got := trail(legacyOtherID, "other"); len(got) != 0 {
+ t.Fatalf("an unrelated app's trail = %q", got)
+ }
+ if got := trail(legacyOtherID, ""); len(got) != 0 {
+ t.Fatalf("the account-wide trail = %q", got)
+ }
+ // The system row stays, once per change.
+ if got := trail(OpsAccountID, "platform"); len(got) != 2 || got[0] != moved || got[1] != first {
+ t.Fatalf("the system trail = %q", got)
+ }
+
+ // A refused re-assignment writes no row.
+ legacyImportRecordIn(t, fx.rel, "efgh", "old.example.com", seeds.StateSwapped)
+ if st, out := fx.assign(t, "shop", "cdef/shop"); st != http.StatusConflict {
+ t.Fatalf("busy re-assign = %d %v", st, out)
+ }
+ if got := trail(thirdID, "shop"); len(got) != 1 {
+ t.Fatalf("a refusal wrote a row: %q", got)
+ }
+}