amadan
Review fixes: the sweep could strand every label, and delete real teams
Both reviewers found the same critical defect independently, and one found it was worse than the first reading. Sweep ran in ONE transaction: purge expired requests, then delete the teams they stranded. A team whose only instance is 'disabled' satisfied the old "no live instance" test, so the delete was attempted and team_instances.ON DELETE RESTRICT refused it — rolling back the purge too. One such team stopped every expired request in the database from ever being swept, so no label was ever released again: precisely the failure §7 exists to prevent. Sweep now purges in its own transaction and releases each team in its own, so a team that refuses to go is skipped rather than fatal. Worse, its candidate set was wrong in the other direction. It deleted any team with no live instance, and operator seed writes a team and its owner in one command and the instance in the NEXT one — so a sweep timed between them destroyed a legitimate team and its memberships. It now only considers a team it actually stranded: one whose expiring request it just purged, which has no team_instances row at all. 'disabled' counts as in use. The rate limit counted rows, so a six-app signup spent six of a budget meant to count signups; it now counts DISTINCT team_id. A limit configured with no window silently did not exist, and a ttl shorter than the window let a request expire while still inside it — both are refused. A total queue-depth cap sits beside the per-requester one, because per-subject limiting alone is defeated by cheap identities. MarkProvisioned stored the drain's origin unchecked, and Choices refuses a member's ENTIRE chooser if any stored origin fails validation — so one bad drain input would have blanked every member's app list for that team. It is validated where it is written now. Tests for each, including the three the review identified as weaker than their names, and the isUnique comment is corrected: the driver does export the code, matching the message is a deliberate trade to avoid the dependency.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 files changed,
+245
−33
internal/home/directorydata/provision.go+110 −27internal/home/directorydata/provision_test.go+125 −0internal/home/directorydata/provisionids.go+10 −6
diff --git a/internal/home/directorydata/provision.go b/internal/home/directorydata/provision.go| index fce43d9..fbecf2f 100644 |
| --- a/internal/home/directorydata/provision.go |
| +++ b/internal/home/directorydata/provision.go |
| @@ -16,6 +16,9 @@ import ( |
| // sealedFloor mirrors the sealed_payload and sealed_name CHECK minimums. |
| const sealedFloor = 29 |
| +// queueDepthCap bounds the whole queue, not one requester's share of it. |
| +const queueDepthCap = 5000 |
| + |
| var ErrLabelTaken = errors.New("directory team label already reserved") |
| // ErrRateLimited is returned when one requester has asked too often inside |
| @@ -62,6 +65,14 @@ func (s *Store) CreateTeam(ctx context.Context, subject, label, name string, app |
| if len(payload) < sealedFloor || len(payload) > 4096 || ttl <= 0 || now.IsZero() { |
| return Team{}, ErrInvalid |
| } |
| + // A rate limit with no window is not a limit: `requested_at > now-0` |
| + // counts nothing and the check silently passes. And ttl must cover the |
| + // window, because Sweep deletes an expired request — if a request could |
| + // expire while still inside the rate window, its requester's history |
| + // would vanish early and the burst could repeat every ttl. |
| + if maxPerWindow > 0 && (window <= 0 || ttl < window) { |
| + return Team{}, ErrInvalid |
| + } |
| seen := make(map[string]bool, len(apps)) |
| for _, app := range apps { |
| if !s.apps[app] || seen[app] { |
| @@ -78,15 +89,29 @@ func (s *Store) CreateTeam(ctx context.Context, subject, label, name string, app |
| rate := RateKey(subject) |
| if maxPerWindow > 0 { |
| + // COUNT(DISTINCT team_id), not count(*): one CreateTeam writes one |
| + // row per app, so counting rows would charge a request for six apps |
| + // six times against a budget meant to count requests. |
| var recent int |
| if err := tx.QueryRowContext(ctx, |
| - `SELECT count(*) FROM provisioning_queue WHERE rate_key = ? AND requested_at > ?`, |
| + `SELECT count(DISTINCT team_id) FROM provisioning_queue WHERE rate_key = ? AND requested_at > ?`, |
| rate, now.Add(-window).Unix()).Scan(&recent); err != nil { |
| return Team{}, ErrUnavailable |
| } |
| if recent >= maxPerWindow { |
| return Team{}, ErrRateLimited |
| } |
| + // A total depth cap as well as a per-requester one (§7). Per-subject |
| + // limiting alone is defeated by cheap identities, which is exactly |
| + // how a shared DNS namespace gets squatted. |
| + var depth int |
| + if err := tx.QueryRowContext(ctx, |
| + `SELECT count(*) FROM provisioning_queue WHERE state = 'queued' AND expires_at > ?`, now.Unix()).Scan(&depth); err != nil { |
| + return Team{}, ErrUnavailable |
| + } |
| + if depth >= queueDepthCap { |
| + return Team{}, ErrRateLimited |
| + } |
| } |
| // The sqid is derived from the row id, so the row has to exist before |
| @@ -119,9 +144,6 @@ func (s *Store) CreateTeam(ctx context.Context, subject, label, name string, app |
| return Team{}, err |
| } |
| if _, err := tx.ExecContext(ctx, `UPDATE teams SET sqid = ?, sealed_name = ? WHERE id = ?`, sqid, sealed, id); err != nil { |
| - if isUnique(err) { |
| - return Team{}, ErrUnavailable |
| - } |
| return Team{}, ErrUnavailable |
| } |
| // The requester owns the team they just asked for. Ruling 23 is what |
| @@ -183,7 +205,7 @@ func (s *Store) Queued(ctx context.Context, now time.Time, limit int) ([]Request |
| // <team>.<app>.oficina.works resolves against, so it and the state change |
| // commit together or neither does. |
| func (s *Store) MarkProvisioned(ctx context.Context, id int64, origin, consumerID string, now time.Time) error { |
| - if id <= 0 || origin == "" || consumerID == "" { |
| + if id <= 0 || !ValidOrigin(origin) || !validConsumer(consumerID) { |
| return ErrInvalid |
| } |
| tx, err := s.db.BeginTx(ctx, nil) |
| @@ -200,6 +222,16 @@ func (s *Store) MarkProvisioned(ctx context.Context, id int64, origin, consumerI |
| } |
| return ErrUnavailable |
| } |
| + // Choices refuses a member's WHOLE chooser if any stored origin fails |
| + // this check, so a route that would not survive reading is refused at |
| + // the point it is written instead. |
| + var sqid, label string |
| + if err := tx.QueryRowContext(ctx, `SELECT sqid, label FROM teams WHERE id = ?`, teamID).Scan(&sqid, &label); err != nil { |
| + return ErrUnavailable |
| + } |
| + if !s.validateRoute(Route{TeamID: teamID, TeamSQID: sqid, Label: label, App: app, Origin: origin, ConsumerID: consumerID}) { |
| + return ErrInvalid |
| + } |
| if _, err := tx.ExecContext(ctx, |
| `INSERT INTO team_instances (team_id, app, origin, consumer_id, state) VALUES (?, ?, ?, ?, 'live')`, |
| teamID, app, origin, consumerID); err != nil { |
| @@ -216,40 +248,91 @@ func (s *Store) MarkProvisioned(ctx context.Context, id int64, origin, consumerI |
| // Sweep releases what expired. §7: "a queue entry that is purged or times |
| // out must release its label, or a request that never became a team holds a |
| -// name nobody can claim." Deleting the expired request is not enough on its |
| -// own — the label lives on the teams row — so a team left with no live |
| -// instance and no live request goes too, and its memberships with it. |
| +// name nobody can claim." |
| // |
| -// A team that HAS a live instance is never touched, however old its other |
| -// requests are: that is a real team with a name in use. |
| +// Two properties this had to be rewritten to get right, both found in review. |
| +// |
| +// It runs in SEPARATE transactions, never one. The first commits the purge |
| +// of expired requests. Only then does it consider the teams those requests |
| +// stranded, one team per transaction, skipping any that refuses to go. A |
| +// single transaction meant one undeletable team rolled back the purge too, |
| +// so one bad row stopped every label in the database from ever being |
| +// released — the exact failure §7 exists to prevent. |
| +// |
| +// And it only ever considers a team that HAS a provisioning_queue row and |
| +// NO team_instances row at all. The earlier version deleted any team with no |
| +// live instance, which caught two kinds of team it had no business |
| +// touching: one whose only instance is 'disabled' (that team is real and its |
| +// name is in use), and one an operator seeded, because operator seed writes |
| +// the team and its owner in one command and the instance in the next — so a |
| +// sweep timed between them destroyed a legitimate team and its memberships. |
| func (s *Store) Sweep(ctx context.Context, now time.Time) (requests, labels int, err error) { |
| - tx, err := s.db.BeginTx(ctx, nil) |
| + // Which teams the purge is about to strand has to be read BEFORE the |
| + // purge: afterwards the expired rows are gone and nothing links the |
| + // team to a request that ever existed. This is also what scopes the |
| + // release to teams this sweep stranded, rather than to every team that |
| + // happens to have no instance right now. |
| + rows, err := s.db.QueryContext(ctx, |
| + `SELECT DISTINCT team_id FROM provisioning_queue WHERE state = 'queued' AND expires_at <= ? LIMIT 1000`, |
| + now.Unix()) |
| if err != nil { |
| return 0, 0, ErrUnavailable |
| } |
| - defer tx.Rollback() |
| - res, err := tx.ExecContext(ctx, |
| + var candidates []int64 |
| + for rows.Next() { |
| + var id int64 |
| + if err := rows.Scan(&id); err != nil { |
| + rows.Close() |
| + return 0, 0, ErrUnavailable |
| + } |
| + candidates = append(candidates, id) |
| + } |
| + closeErr := rows.Err() |
| + rows.Close() |
| + if closeErr != nil { |
| + return 0, 0, ErrUnavailable |
| + } |
| + |
| + res, err := s.db.ExecContext(ctx, |
| `DELETE FROM provisioning_queue WHERE state = 'queued' AND expires_at <= ?`, now.Unix()) |
| if err != nil { |
| return 0, 0, ErrUnavailable |
| } |
| n, _ := res.RowsAffected() |
| - if _, err := tx.ExecContext(ctx, `DELETE FROM memberships WHERE team_id IN ( |
| - SELECT t.id FROM teams t |
| - WHERE NOT EXISTS (SELECT 1 FROM team_instances i WHERE i.team_id = t.id AND i.state != 'disabled') |
| - AND NOT EXISTS (SELECT 1 FROM provisioning_queue q WHERE q.team_id = t.id AND q.state = 'queued'))`); err != nil { |
| - return 0, 0, ErrUnavailable |
| + |
| + for _, id := range candidates { |
| + var blockers int |
| + if err := s.db.QueryRowContext(ctx, |
| + `SELECT (SELECT count(*) FROM team_instances WHERE team_id = ?1) |
| + + (SELECT count(*) FROM provisioning_queue WHERE team_id = ?1 AND state = 'queued')`, |
| + id).Scan(&blockers); err != nil { |
| + continue |
| + } |
| + if blockers != 0 { |
| + continue |
| + } |
| + if s.releaseTeam(ctx, id) { |
| + labels++ |
| + } |
| } |
| - res, err = tx.ExecContext(ctx, `DELETE FROM teams WHERE id IN ( |
| - SELECT t.id FROM teams t |
| - WHERE NOT EXISTS (SELECT 1 FROM team_instances i WHERE i.team_id = t.id AND i.state != 'disabled') |
| - AND NOT EXISTS (SELECT 1 FROM provisioning_queue q WHERE q.team_id = t.id AND q.state = 'queued'))`) |
| + return int(n), labels, nil |
| +} |
| + |
| +// releaseTeam drops one stranded team and its memberships, and reports |
| +// whether it went. A team that refuses — a foreign key from the roster or an |
| +// operator receipt still pointing at it — is left alone and does not stop |
| +// the next one. Its own transaction is what makes that containment true. |
| +func (s *Store) releaseTeam(ctx context.Context, id int64) bool { |
| + tx, err := s.db.BeginTx(ctx, nil) |
| if err != nil { |
| - return 0, 0, ErrUnavailable |
| + return false |
| } |
| - m, _ := res.RowsAffected() |
| - if err := tx.Commit(); err != nil { |
| - return 0, 0, ErrUnavailable |
| + defer tx.Rollback() |
| + if _, err := tx.ExecContext(ctx, `DELETE FROM memberships WHERE team_id = ?`, id); err != nil { |
| + return false |
| + } |
| + if _, err := tx.ExecContext(ctx, `DELETE FROM teams WHERE id = ?`, id); err != nil { |
| + return false |
| } |
| - return int(n), int(m), nil |
| + return tx.Commit() == nil |
| } |
diff --git a/internal/home/directorydata/provision_test.go b/internal/home/directorydata/provision_test.go| index 9c88dfb..13f0d58 100644 |
| --- a/internal/home/directorydata/provision_test.go |
| +++ b/internal/home/directorydata/provision_test.go |
| @@ -158,3 +158,128 @@ func TestCreateTeamRateLimitsOneRequester(t *testing.T) { |
| // "three" was refused, so nothing may be holding it. |
| mustCreate(t, s, personB, "three", []string{"docs"}, now.Add(2*time.Hour)) |
| } |
| + |
| +// Regression: a team whose only instance is 'disabled' used to satisfy |
| +// Sweep's "no live instance" test, so Sweep deleted its memberships and then |
| +// tried to delete the team — which team_instances.ON DELETE RESTRICT |
| +// refuses. The whole transaction rolled back, so ONE such team stopped every |
| +// expired request in the database from ever being swept, and no label was |
| +// ever released again. |
| +func TestSweepIsNotBlockedByADisabledInstance(t *testing.T) { |
| + s, d, _ := fixture(t) |
| + now := time.Unix(1_700_000_000, 0).UTC() |
| + blocker := mustCreate(t, s, personA, "blocker", []string{"docs"}, now) |
| + got, err := s.Queued(context.Background(), now, 10) |
| + if err != nil || len(got) != 1 { |
| + t.Fatal(err) |
| + } |
| + if err := s.MarkProvisioned(context.Background(), got[0].ID, "https://blocker.docs.oficina.works", "c1", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + exec(t, d.Writer(), `UPDATE team_instances SET state='disabled' WHERE team_id=?`, blocker.ID) |
| + |
| + // An unrelated request expires and must still be swept. |
| + if _, err := s.CreateTeam(context.Background(), personB, "victim", privateName, []string{"docs"}, sealedPayload, now, time.Minute, 0, time.Minute); err != nil { |
| + t.Fatal(err) |
| + } |
| + reqs, _, err := s.Sweep(context.Background(), now.Add(2*time.Minute)) |
| + if err != nil { |
| + t.Fatalf("sweep refused to run: %v", err) |
| + } |
| + if reqs != 1 { |
| + t.Fatalf("swept %d expired requests, want 1 — a disabled instance blocked the sweep", reqs) |
| + } |
| + // And the victim's label is genuinely free again. |
| + mustCreate(t, s, personA, "victim", []string{"docs"}, now.Add(3*time.Minute)) |
| +} |
| + |
| +// A team with no provisioning_queue row was never this sweep's business. |
| +// operator seed writes a team and its owner in one command and the instance |
| +// in the NEXT one, so a sweep timed between them used to delete a legitimate |
| +// team and its memberships outright. |
| +func TestSweepNeverTouchesATeamItDidNotStrand(t *testing.T) { |
| + s, d, _ := fixture(t) |
| + now := time.Unix(1_700_000_000, 0).UTC() |
| + exec(t, d.Writer(), `INSERT INTO teams (sqid, label, sealed_name, created_at) VALUES ('seeded', 'seeded', ?, ?)`, |
| + make([]byte, 29), now.Unix()) |
| + exec(t, d.Writer(), `INSERT INTO memberships (team_id, person_ref, role) |
| + SELECT id, ?, 'owner' FROM teams WHERE label='seeded'`, personA) |
| + // Something unrelated expires, so the sweep definitely runs. |
| + if _, err := s.CreateTeam(context.Background(), personB, "other", privateName, []string{"docs"}, sealedPayload, now, time.Minute, 0, time.Minute); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, _, err := s.Sweep(context.Background(), now.Add(2*time.Minute)); err != nil { |
| + t.Fatal(err) |
| + } |
| + var teams, members int |
| + if err := d.Writer().QueryRow(`SELECT count(*) FROM teams WHERE label='seeded'`).Scan(&teams); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := d.Writer().QueryRow(`SELECT count(*) FROM memberships`).Scan(&members); err != nil { |
| + t.Fatal(err) |
| + } |
| + if teams != 1 || members != 1 { |
| + t.Fatalf("sweep destroyed an operator-seeded team: %d teams, %d memberships", teams, members) |
| + } |
| +} |
| + |
| +// One request for several apps is ONE request. Counting rows charged it once |
| +// per app, so a six-app signup burned six of a budget meant to count signups. |
| +func TestRateLimitCountsRequestsNotApps(t *testing.T) { |
| + s, _, _ := fixture(t) |
| + now := time.Unix(1_700_000_000, 0).UTC() |
| + if _, err := s.CreateTeam(context.Background(), personA, "acme", privateName, []string{"docs", "sheets"}, sealedPayload, now, time.Hour, 2, time.Hour); err != nil { |
| + t.Fatal(err) |
| + } |
| + // That was one request against a budget of two, so a second must pass. |
| + if _, err := s.CreateTeam(context.Background(), personA, "beta", privateName, []string{"docs", "sheets"}, sealedPayload, now, time.Hour, 2, time.Hour); err != nil { |
| + t.Fatalf("second request refused, so the limit is counting apps: %v", err) |
| + } |
| + if _, err := s.CreateTeam(context.Background(), personA, "gamma", privateName, []string{"docs"}, sealedPayload, now, time.Hour, 2, time.Hour); !errors.Is(err, ErrRateLimited) { |
| + t.Fatalf("third request: %v, want ErrRateLimited", err) |
| + } |
| +} |
| + |
| +// A limit with no window counts `requested_at > now`, which is nothing, so |
| +// the limit silently does not exist. And a ttl shorter than the window lets |
| +// a request expire while still inside it, erasing the history the limit is |
| +// counting. Both are configuration, so both are refused at the door. |
| +func TestRateLimitRefusesAConfigurationThatCannotLimit(t *testing.T) { |
| + s, _, _ := fixture(t) |
| + now := time.Unix(1_700_000_000, 0).UTC() |
| + for _, c := range []struct { |
| + name string |
| + ttl time.Duration |
| + window time.Duration |
| + }{ |
| + {"no window", time.Hour, 0}, |
| + {"negative window", time.Hour, -time.Minute}, |
| + {"ttl shorter than window", time.Minute, time.Hour}, |
| + } { |
| + if _, err := s.CreateTeam(context.Background(), personA, "acme", privateName, []string{"docs"}, sealedPayload, now, c.ttl, 5, c.window); !errors.Is(err, ErrInvalid) { |
| + t.Fatalf("%s: %v, want ErrInvalid", c.name, err) |
| + } |
| + } |
| +} |
| + |
| +// The drain hands back an origin and a consumer id. Choices refuses a |
| +// member's ENTIRE chooser if any stored origin fails validation, so one bad |
| +// drain input would blank every member's app list for that team. Refuse it |
| +// where it is written instead. |
| +func TestMarkProvisionedRefusesARouteThatWouldNotSurviveReading(t *testing.T) { |
| + s, _, _ := fixture(t) |
| + now := time.Unix(1_700_000_000, 0).UTC() |
| + mustCreate(t, s, personA, "acme", []string{"docs"}, now) |
| + got, err := s.Queued(context.Background(), now, 10) |
| + if err != nil || len(got) != 1 { |
| + t.Fatal(err) |
| + } |
| + for _, bad := range []string{"https://attacker.example", "https://acme.sheets.oficina.works", "not-a-url", ""} { |
| + if err := s.MarkProvisioned(context.Background(), got[0].ID, bad, "consumer-1", now); !errors.Is(err, ErrInvalid) { |
| + t.Fatalf("origin %q accepted: %v", bad, err) |
| + } |
| + } |
| + if err := s.MarkProvisioned(context.Background(), got[0].ID, "https://acme.docs.oficina.works", "consumer-1", now); err != nil { |
| + t.Fatalf("the correct route was refused: %v", err) |
| + } |
| +} |
diff --git a/internal/home/directorydata/provisionids.go b/internal/home/directorydata/provisionids.go| index 464c5dd..2ae7675 100644 |
| --- a/internal/home/directorydata/provisionids.go |
| +++ b/internal/home/directorydata/provisionids.go |
| @@ -50,12 +50,16 @@ var pendingSeal = make([]byte, 29) |
| // isUnique reports whether err is a UNIQUE constraint violation. |
| // |
| -// modernc.org/sqlite returns extended result codes on its own error type, |
| -// but the constant for SQLITE_CONSTRAINT_UNIQUE is not exported in a form |
| -// this package can depend on without pinning the driver's internals. The |
| -// message is the stable surface: SQLite has spelled it "UNIQUE constraint |
| -// failed" since 3.8. A false negative here costs a clear ErrLabelTaken and |
| -// returns ErrUnavailable instead, which is the safe direction to fail. |
| +// This matches on the message rather than the code, and that is a |
| +// deliberate trade rather than a limitation: modernc.org/sqlite DOES export |
| +// (*sqlite.Error).Code() and sqlite3.SQLITE_CONSTRAINT_UNIQUE, but reaching |
| +// them coerces this package into a driver dependency it otherwise does not |
| +// have. SQLite has spelled the message "UNIQUE constraint failed" since 3.8. |
| +// |
| +// The trade is safe in one direction only, which is the one that matters: a |
| +// false negative turns a real collision into ErrUnavailable — a confusing |
| +// 5xx and a retry — and never into a duplicate reservation, because the |
| +// constraint still refused the write. |
| func isUnique(err error) bool { |
| return err != nil && strings.Contains(err.Error(), "UNIQUE constraint failed") |
| } |