Review of 083: a message the store cannot take is held and retried on a ticker, not slept on, so enrolments are answered meanwhile; a newer one per subject supersedes; the password is replaced after the spend; the same presenter may finish after a lost answer; upgrades retry only on the store

internal/inventory/nodes.go

@@ -203,15 +203,6 @@ func (i *Inventory) IssueToken(ctx context.Context, nodeName string, validFor ti
 // token that had expired.
 var ErrTokenRefused = errors.New("that token cannot be used")
 
-// Redeem spends a token and reports which node it was for.
-//
-// It does not issue an identity. What a node presents afterwards to prove it is that node is not
-// decided anywhere (novox/hq ADR 0004 names the property, not the mechanism), and guessing at it
-// in a migration is the most expensive guess available here.
-//
-// The update is the check: one statement that both finds a live token and marks it used, so two
-// simultaneous redemptions of one secret cannot both succeed. Reading first and writing second
-// would leave exactly that gap.
 // ClaimLease is how long a claimed token is held for the one presenter that claimed it. Long
 // enough for an enrolment to be tried again through a store restart; short enough that a host
 // which gave up and was started over, with keys of its own, is not kept waiting long.
@@ -224,12 +215,17 @@ var ErrTokenInUse = errors.New("the token is being used by another enrolment")
 // Claim takes a token for one presenter — `by`, which names the key presenting it — for the length
 // of a lease, and says which node it enrols. The same presenter may claim it again, as may anyone
 // once the lease has lapsed; nothing is spent until Spend (novox/hq 04-ISSUES/083).
+//
+// A token this same presenter already spent is claimed again too: its spend reached the store and
+// the answer did not reach the node, which asked again. Refusing it then would lock out a machine
+// the mesh holds as enrolled — with the key it is still presenting.
 func (i *Inventory) Claim(ctx context.Context, secret, by string) (Node, error) {
 	var id string
 	err := i.store.Pool().QueryRow(ctx,
 		`update enrolment_token set claimed_by = $2, claimed_until = now() + $3::interval
-		  where secret = $1 and redeemed is null and expires > now()
-		    and (claimed_by is null or claimed_by = $2 or claimed_until < now())
+		  where secret = $1 and expires > now()
+		    and ((redeemed is null and (claimed_by is null or claimed_by = $2 or claimed_until < now()))
+		      or (redeemed is not null and claimed_by = $2))
 		  returning node`, hashSecret(secret), by, ClaimLease.String()).Scan(&id)
 	if errors.Is(err, pgx.ErrNoRows) {
 		// Unusable, or held by someone else — told apart, because the second passes.
@@ -237,8 +233,12 @@ func (i *Inventory) Claim(ctx context.Context, secret, by string) (Node, error)
 		probe := i.store.Pool().QueryRow(ctx,
 			`select true from enrolment_token
 			  where secret = $1 and redeemed is null and expires > now()`, hashSecret(secret)).Scan(&held)
-		if probe == nil && held {
+		switch {
+		case probe == nil && held:
 			return Node{}, ErrTokenInUse
+		case probe != nil && !errors.Is(probe, pgx.ErrNoRows):
+			// The store went away between the two questions: "not now", not a refusal.
+			return Node{}, probe
 		}
 		return Node{}, ErrTokenRefused
 	}
@@ -251,12 +251,13 @@ func (i *Inventory) Claim(ctx context.Context, secret, by string) (Node, error)
 	return n, err
 }
 
-// Spend makes a claimed token used, only for the presenter holding the claim. The last write of an
-// enrolment, so a token is spent exactly when the node it enrolled is complete.
+// Spend makes a claimed token used, only for the presenter holding the claim. The last write to the
+// store in an enrolment, so a token is spent exactly when the node it enrolled is complete. Spent
+// again by the same presenter is not an error: an answer lost after the first spend.
 func (i *Inventory) Spend(ctx context.Context, secret, by string) error {
 	tag, err := i.store.Pool().Exec(ctx,
-		`update enrolment_token set redeemed = now()
-		  where secret = $1 and redeemed is null and claimed_by = $2`, hashSecret(secret), by)
+		`update enrolment_token set redeemed = coalesce(redeemed, now())
+		  where secret = $1 and claimed_by = $2`, hashSecret(secret), by)
 	if err != nil {
 		return err
 	}
@@ -266,6 +267,16 @@ func (i *Inventory) Spend(ctx context.Context, secret, by string) error {
 	return nil
 }
 
+// Redeem spends a token in one step and reports which node it was for. Enrolment claims and then
+// spends (Claim, Spend); this is the one-step form, kept for what spends a token outright.
+//
+// It does not issue an identity. What a node presents afterwards to prove it is that node is not
+// decided anywhere (novox/hq ADR 0004 names the property, not the mechanism), and guessing at it
+// in a migration is the most expensive guess available here.
+//
+// The update is the check: one statement that both finds a live token and marks it used, so two
+// simultaneous redemptions of one secret cannot both succeed. Reading first and writing second
+// would leave exactly that gap.
 func (i *Inventory) Redeem(ctx context.Context, secret string) (Node, error) {
 	var id string
 	err := i.store.Pool().QueryRow(ctx,