amadan

carlos / platform Public

console: a pending change executes against the CURRENT quorum

A proposal can wait a week, and both halves of "N distinct members" can
move underneath it. Execution counted the ROWS on the change against the
number STORED on it at propose time, so a promote proposed at
promote_approvals: 2 still landed after the channel's rule was raised to
3 — the stale proposal was the way past the new rule — and a
confirmation from somebody who had since left the account still carried
a change over the line, which made removing a member a way of leaving
their approvals working on their way out.

executePending now requires max(stored quorum, the channel's current
rule for this kind) and counts only confirmations from current members
(Store.MemberConfirms). Loosening a rule never lowers a stored quorum:
the confirmers agreed to something that needed N of them, and one person
lowering the rule afterwards is not the others' confirmation. Both are
refusals, not consumptions — the change stays listed, keeps every
confirmation row, and the remaining members can still meet it. A policy
edit needs no re-check: its SetAt pin already refuses any proposal
composed against an older pipeline.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell ce6ca583e47790a2843319f2d91945d622c7b7d5 parent da9554b
3 files changed, +311 −8
  • internal/console/pending.go +33 −0
  • internal/console/pendingflow_test.go +189 −0
  • internal/console/pendinghandlers.go +89 −8
diff --git a/internal/console/pending.go b/internal/console/pending.go
index 66dc5b326..db10fd140 100644
--- a/internal/console/pending.go
+++ b/internal/console/pending.go
@@ -254,6 +254,39 @@ func (s *Store) PendingChange(id string, now time.Time) (*PendingChange, error)
return pc, nil
}
+// MemberConfirms is a change's confirming addresses filtered to those that
+// are STILL members of the account — the tally EXECUTION must count (spec §4:
+// "quorum is N distinct MEMBERS"). A confirmation is an act of standing
+// authority, and an address that has left the account has none: without this
+// filter, removing somebody from an account would leave their approvals
+// working on their way out, and an account could be brought to quorum by
+// people who are no longer in it.
+//
+// The rows are KEPT, not deleted. Who confirmed what is a true fact about the
+// past and the trail says so; what changes is only whether it still counts.
+// Confirms (the display tally on the card and the CLI list) deliberately
+// keeps showing every row — the gap between the two numbers is the honest
+// picture, and a card that quietly dropped a name would make a stuck change
+// unexplainable.
+func (s *Store) MemberConfirms(accountID int64, id string) ([]string, error) {
+ rows, err := s.db.Query(`SELECT c.address FROM pending_confirms c
+JOIN memberships m ON m.address = c.address AND m.account_id = ?
+WHERE c.change_id = ? ORDER BY c.confirmed_at, c.address`, accountID, id)
+ if err != nil {
+ return nil, err
+ }
+ defer rows.Close()
+ var out []string
+ for rows.Next() {
+ var a string
+ if err := rows.Scan(&a); err != nil {
+ return nil, err
+ }
+ out = append(out, a)
+ }
+ return out, rows.Err()
+}
+
// MarkExecuted stamps a change as carried out, so it stops appearing as
// pending and can no longer be confirmed.
func (s *Store) MarkExecuted(id string, now time.Time) error {
diff --git a/internal/console/pendingflow_test.go b/internal/console/pendingflow_test.go
index 78c137977..9a355d20a 100644
--- a/internal/console/pendingflow_test.go
+++ b/internal/console/pendingflow_test.go
@@ -941,3 +941,192 @@ func TestHotfixBypassesOrderNotRules(t *testing.T) {
t.Fatalf("production serves %q, want abc1234", got.Version)
}
}
+
+// TestExecutionMeetsTheCURRENTQuorum: a proposal carries the quorum that was
+// in force when it was made, and a week can pass before it is confirmed. If
+// the channel's rule was TIGHTENED in between — 2 approvals to 3 — executing
+// at the stored 2 would let a stale proposal be the way past the new rule.
+// Execution therefore re-reads the pipeline and requires max(stored, current).
+// The refusal is an error, not a consumption: the change stays listed and
+// stays retryable, so the third member can still meet it.
+func TestExecutionMeetsTheCURRENTQuorum(t *testing.T) {
+ fx, c, csrf := memberFixture(t) // paul owns, kim is a member
+ if err := fx.st.AddMember(OpsAccountID, "alice@keymail.dev", RoleMember, time.Now()); err != nil {
+ t.Fatal(err)
+ }
+ pipelinedApp(t, fx, "widget", "abc1234",
+ release.PipelineChannel{Name: "edge"},
+ release.PipelineChannel{Name: "production", Rules: release.ChannelRules{PromoteApprovals: 2}})
+
+ status, loc, body := postGated(t, fx, c, fx.u("/apps/widget/promote"),
+ url.Values{"csrf": {csrf}, "channel": {"production"}, "version": {"abc1234"}, "confirmed": {"1"}})
+ if status != http.StatusSeeOther {
+ t.Fatalf("owner proposal = %d -> %q:\n%s", status, loc, body)
+ }
+ pending, err := fx.st.PendingChangesFor(OpsAccountID, "widget", time.Now())
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(pending) != 1 || pending[0].Quorum != 2 {
+ t.Fatalf("pending = %+v, want one promote needing 2", pending)
+ }
+ pc := pending[0]
+
+ // The rule is raised while the proposal waits.
+ setPipeline(t, fx, "widget",
+ release.PipelineChannel{Name: "edge"},
+ release.PipelineChannel{Name: "production", Rules: release.ChannelRules{PromoteApprovals: 3}})
+
+ kim := newBrowser(t)
+ fx.setUser("kim@keymail.dev")
+ signUp(t, fx, kim, "code-kim-tightened")
+ kimCSRF := sessionToken(t, fx, kim)
+ before := serial(t, fx)
+ status, loc, body = postGated(t, fx, kim,
+ fx.u("/apps/widget/pending/"+pc.ID+"/confirm"), url.Values{"csrf": {kimCSRF}})
+ if status != http.StatusSeeOther {
+ t.Fatalf("second confirmation = %d -> %q:\n%s", status, loc, body)
+ }
+ channelAbsent(t, fx, "widget", "production")
+ if got := serial(t, fx); got != before {
+ t.Fatalf("serial = %d, want it unmoved at %d — nothing may be written under a quorum that is no longer enough", got, before)
+ }
+ left, err := fx.st.PendingChangesFor(OpsAccountID, "widget", time.Now())
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(left) != 1 || left[0].ID != pc.ID {
+ t.Fatalf("pending after the refusal = %+v, want the change still listed and retryable", left)
+ }
+ if len(left[0].Confirms) != 2 {
+ t.Fatalf("confirmations = %v, want both kept — a refusal must not discard them", left[0].Confirms)
+ }
+
+ // The third member meets the new rule, and THAT lands it.
+ alice := newBrowser(t)
+ fx.setUser("alice@keymail.dev")
+ signUp(t, fx, alice, "code-alice-tightened")
+ aliceCSRF := sessionToken(t, fx, alice)
+ status, loc, body = postGated(t, fx, alice,
+ fx.u("/apps/widget/pending/"+pc.ID+"/confirm"), url.Values{"csrf": {aliceCSRF}})
+ if status != http.StatusSeeOther {
+ t.Fatalf("third confirmation = %d -> %q:\n%s", status, loc, body)
+ }
+ if got := fx.channelVersion(t, "widget", "production"); got.Version != "abc1234" {
+ t.Fatalf("production serves %q after the raised quorum was met, want abc1234", got.Version)
+ }
+}
+
+// TestLoosenedRuleDoesNotLowerAStoredQuorum: the ratchet only goes one way.
+// A proposal made at 3 approvals is not executable at 2 because somebody
+// lowered the rule afterwards — the confirmers agreed to a change that needed
+// three of them, and lowering the rule is not their confirmation.
+func TestLoosenedRuleDoesNotLowerAStoredQuorum(t *testing.T) {
+ fx, c, csrf := memberFixture(t) // paul owns, kim is a member
+ if err := fx.st.AddMember(OpsAccountID, "alice@keymail.dev", RoleMember, time.Now()); err != nil {
+ t.Fatal(err)
+ }
+ pipelinedApp(t, fx, "widget", "abc1234",
+ release.PipelineChannel{Name: "edge"},
+ release.PipelineChannel{Name: "production", Rules: release.ChannelRules{PromoteApprovals: 3}})
+
+ if status, loc, body := postGated(t, fx, c, fx.u("/apps/widget/promote"),
+ url.Values{"csrf": {csrf}, "channel": {"production"}, "version": {"abc1234"}, "confirmed": {"1"}}); status != http.StatusSeeOther {
+ t.Fatalf("owner proposal = %d -> %q:\n%s", status, loc, body)
+ }
+ pending, err := fx.st.PendingChangesFor(OpsAccountID, "widget", time.Now())
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(pending) != 1 || pending[0].Quorum != 3 {
+ t.Fatalf("pending = %+v, want one promote needing 3", pending)
+ }
+ pc := pending[0]
+
+ setPipeline(t, fx, "widget",
+ release.PipelineChannel{Name: "edge"},
+ release.PipelineChannel{Name: "production", Rules: release.ChannelRules{PromoteApprovals: 2}})
+
+ kim := newBrowser(t)
+ fx.setUser("kim@keymail.dev")
+ signUp(t, fx, kim, "code-kim-loosened")
+ kimCSRF := sessionToken(t, fx, kim)
+ if status, loc, body := postGated(t, fx, kim,
+ fx.u("/apps/widget/pending/"+pc.ID+"/confirm"), url.Values{"csrf": {kimCSRF}}); status != http.StatusSeeOther {
+ t.Fatalf("second confirmation = %d -> %q:\n%s", status, loc, body)
+ }
+ channelAbsent(t, fx, "widget", "production")
+ left, err := fx.st.PendingChangesFor(OpsAccountID, "widget", time.Now())
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(left) != 1 {
+ t.Fatalf("pending after the loosened rule = %+v, want the change still waiting on its own quorum of 3", left)
+ }
+}
+
+// TestDepartedMemberConfirmDoesNotCount: a quorum is N distinct MEMBERS. An
+// address that has since left the account is not one, so its row must not
+// carry a change over the line — otherwise removing somebody from an account
+// would leave their standing approvals working on their way out. The row is
+// kept (the trail of who confirmed what stays true); it simply stops counting.
+func TestDepartedMemberConfirmDoesNotCount(t *testing.T) {
+ fx, c, csrf := memberFixture(t) // paul owns, kim is a member
+ if err := fx.st.AddMember(OpsAccountID, "alice@keymail.dev", RoleMember, time.Now()); err != nil {
+ t.Fatal(err)
+ }
+ pipelinedApp(t, fx, "widget", "abc1234",
+ release.PipelineChannel{Name: "edge"},
+ release.PipelineChannel{Name: "production", Rules: release.ChannelRules{PromoteApprovals: 3}})
+
+ if status, loc, body := postGated(t, fx, c, fx.u("/apps/widget/promote"),
+ url.Values{"csrf": {csrf}, "channel": {"production"}, "version": {"abc1234"}, "confirmed": {"1"}}); status != http.StatusSeeOther {
+ t.Fatalf("owner proposal = %d -> %q:\n%s", status, loc, body)
+ }
+ pending, err := fx.st.PendingChangesFor(OpsAccountID, "widget", time.Now())
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(pending) != 1 {
+ t.Fatalf("pending = %+v, want one", pending)
+ }
+ pc := pending[0]
+
+ kim := newBrowser(t)
+ fx.setUser("kim@keymail.dev")
+ signUp(t, fx, kim, "code-kim-departed")
+ kimCSRF := sessionToken(t, fx, kim)
+ if status, loc, body := postGated(t, fx, kim,
+ fx.u("/apps/widget/pending/"+pc.ID+"/confirm"), url.Values{"csrf": {kimCSRF}}); status != http.StatusSeeOther {
+ t.Fatalf("kim's confirmation = %d -> %q:\n%s", status, loc, body)
+ }
+
+ // Kim leaves the account, with her confirmation still on the row.
+ if err := fx.st.RemoveMember(OpsAccountID, "kim@keymail.dev"); err != nil {
+ t.Fatal(err)
+ }
+
+ alice := newBrowser(t)
+ fx.setUser("alice@keymail.dev")
+ signUp(t, fx, alice, "code-alice-departed")
+ aliceCSRF := sessionToken(t, fx, alice)
+ before := serial(t, fx)
+ if status, loc, body := postGated(t, fx, alice,
+ fx.u("/apps/widget/pending/"+pc.ID+"/confirm"), url.Values{"csrf": {aliceCSRF}}); status != http.StatusSeeOther {
+ t.Fatalf("alice's confirmation = %d -> %q:\n%s", status, loc, body)
+ }
+ channelAbsent(t, fx, "widget", "production")
+ if got := serial(t, fx); got != before {
+ t.Fatalf("serial = %d, want it unmoved at %d — a departed member's row must not carry a change over the line", got, before)
+ }
+ left, err := fx.st.PendingChangesFor(OpsAccountID, "widget", time.Now())
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(left) != 1 || left[0].ID != pc.ID {
+ t.Fatalf("pending after the refusal = %+v, want the change still listed", left)
+ }
+ if len(left[0].Confirms) != 3 {
+ t.Fatalf("confirmations = %v, want all three rows KEPT — they simply do not all count", left[0].Confirms)
+ }
+}
diff --git a/internal/console/pendinghandlers.go b/internal/console/pendinghandlers.go
index 7e783e49e..69c56a180 100644
--- a/internal/console/pendinghandlers.go
+++ b/internal/console/pendinghandlers.go
@@ -273,30 +273,59 @@ func policyProposal(cur *release.Pipeline, next []release.PipelineChannel) polic
// caller must not mark it executed, and the member who confirmed it is told
// why rather than left believing it went through.
//
-// The first two checks are this function's OWN preconditions, restated rather
+// The first checks are this function's OWN preconditions, restated rather
// than assumed. Every caller today establishes them — quorum via
// ConfirmChange's `met`, the app via pendingScoped's 404 — but this is billed
// as the one place a gated write happens, and a future caller (the console's
// own pipeline page, Task 9) that got either wrong would otherwise turn a
// mistake into an unapproved signed write. They are errors, never panics: a
// bug here must refuse, not take the console down.
+//
+// The quorum precondition is not a restatement, though: ConfirmChange counts
+// ROWS against the number STORED on the proposal, and both of those can be
+// stale by the time anyone confirms. This is where the CURRENT rule and the
+// CURRENT membership are what the change has to meet — see the comment on the
+// check itself.
func (s *Server) executePending(ctx context.Context, acct *Account, app, actor string, pc *PendingChange) (string, error) {
if pc.AccountID != acct.ID || pc.App != app {
log.Printf("console: execute %s %s: change belongs to %d/%q, not %d/%q",
pc.Kind, pc.ID, pc.AccountID, pc.App, acct.ID, app)
return "", errors.New("that change belongs to a different app")
}
- if len(pc.Confirms) < pc.Quorum {
- log.Printf("console: execute %s %s: %d of %d confirmations", pc.Kind, pc.ID, len(pc.Confirms), pc.Quorum)
- return "", fmt.Errorf("that change has %d of %d confirmations", len(pc.Confirms), pc.Quorum)
+ // The quorum this change must meet NOW, not merely the one it was
+ // proposed under. A proposal can wait a week (PendingChangeTTL), and both
+ // halves of "N distinct members" can move underneath it:
+ //
+ // - the RULE can be tightened. Executing at the stored number would make
+ // a stale proposal the way past a raised rule — propose at 2, wait for
+ // somebody to raise it to 3, land it with 2. Required is therefore
+ // max(stored, current). Loosening never lowers the stored number: the
+ // confirmers agreed to something that needed N of them, and one person
+ // lowering the rule afterwards is not the others' confirmation.
+ // - the MEMBERSHIP can change. A confirmation is standing authority, and
+ // an address that has left the account has none, so its row stops
+ // counting (MemberConfirms) while staying on the record.
+ //
+ // Both are refusals, never consumptions: the change stays unexecuted,
+ // listed and retryable, and the remaining members can still meet it.
+ required, err := s.currentQuorum(ctx, acct, app, pc)
+ if err != nil {
+ return "", err
+ }
+ counted, err := s.store.MemberConfirms(pc.AccountID, pc.ID)
+ if err != nil {
+ log.Printf("console: execute %s %s: member confirms: %v", pc.Kind, pc.ID, err)
+ return "", errors.New("could not check who has confirmed this change")
+ }
+ if len(counted) < required {
+ log.Printf("console: execute %s %s: %d of %d confirmations counted (%d rows, quorum stored %d)",
+ pc.Kind, pc.ID, len(counted), required, len(pc.Confirms), pc.Quorum)
+ return "", fmt.Errorf("that change has %d of %d confirmations", len(counted), required)
}
if s.cfg.Key == nil {
return "", errors.New("no signing key on this host — the console is running read-only")
}
- var (
- msg string
- err error
- )
+ var msg string
switch pc.Kind {
case kindPromote:
msg, err = s.executePendingPromote(ctx, acct, app, actor, pc)
@@ -320,6 +349,58 @@ func (s *Server) executePending(ctx context.Context, acct *Account, app, actor s
return msg, nil
}
+// currentQuorum is how many confirmations this change must have RIGHT NOW:
+// the number it was proposed under, raised to the target channel's current
+// rule if that rule has since been tightened. Never lowered — see
+// executePending's comment for why a loosening is not a confirmation.
+//
+// It costs one pipeline read that the promote path then makes again a moment
+// later. That duplication is deliberate: this is the gate, and a gate that
+// depends on a value threaded down through three kind-specific functions is a
+// gate that a fourth kind can be added without. A gated execution is a rare,
+// human-paced action; the read is not a cost worth trading the shape for.
+//
+// A policy edit is not re-checked here and does not need to be: its quorum is
+// derived from the channels its own edit TOUCHES (guardedEdit), and
+// executePendingPolicy already refuses outright unless the stored pipeline is
+// byte-for-byte the one the proposal was composed against (the SetAt pin). A
+// rule that moved at all invalidates the proposal before any quorum question
+// is asked, so max(stored, current) could only ever equal stored.
+func (s *Server) currentQuorum(ctx context.Context, acct *Account, app string, pc *PendingChange) (int, error) {
+ var channel string
+ var rule func(release.ChannelRules) int
+ switch pc.Kind {
+ case kindPromote:
+ var p promotePayload
+ if err := json.Unmarshal(pc.Payload, &p); err != nil {
+ return 0, errors.New("this proposal's payload cannot be read — reject it and propose again")
+ }
+ channel, rule = p.Channel, func(r release.ChannelRules) int { return r.PromoteApprovals }
+ case kindFastTrack:
+ var p fastTrackPayload
+ if err := json.Unmarshal(pc.Payload, &p); err != nil {
+ return 0, errors.New("this proposal's payload cannot be read — reject it and propose again")
+ }
+ // A fast-track is gated by change_approvals, not promote_approvals
+ // (spec §2: one field covers editing a channel's entry AND overriding
+ // its bake), so it must be re-checked against that same field.
+ channel, rule = p.Channel, func(r release.ChannelRules) int { return r.ChangeApprovals }
+ default:
+ return pc.Quorum, nil
+ }
+ pipe, err := release.GetPipeline(ctx, s.cfg.Releases, s.cfg.ReleasePub, acct.Sqid, app)
+ if err != nil {
+ // Fail closed, like every other unreadable-pipeline path: a proposal
+ // whose governing rules cannot be read is not executable.
+ log.Printf("console: execute %s %s/%s: read pipeline: %v", pc.Kind, acct.Sqid, app, err)
+ return 0, errors.New("could not read the pipeline")
+ }
+ if n := rule(channelRules(pipe, channel)); n > pc.Quorum {
+ return n, nil
+ }
+ return pc.Quorum, nil
+}
+
// staleProposal is the one refusal every kind shares: the world the proposal
// pinned is not the world any more.
func staleProposal(detail string) error {