From 18252cd17e56fbd9287e2009fc9459dde54e47b3 Mon Sep 17 00:00:00 2001 From: Geoff Turk Date: Mon, 19 Oct 2026 18:18:17 +0200 Subject: [PATCH] fix(connection): settle legacy reservations and end failed grants A session whose latest mobile_connections row was written by the 000039 flow ('pending' or 'accepted') answered 409 to every QR code create or refresh: 000041 left those rows in place, and change() treats a status it does not know as still active. Migration 000044 moves them to their final claim-flow equivalents, 'lapsed' and 'released'. A failed additional-administrator grant only recorded provision_error and stayed confirmed/provisioning. That blocked new QR codes the same way, and the recovery queue re-ran the grant every 10 seconds, restarting the VTA each time. connection.MarkFailed now moves it to 'failed', which is final, and the API returns the error for failed requests. Signed-off-by: Geoff Turk --- internal/apidocs/openapi.yaml | 8 +- internal/connection/store.go | 10 ++ internal/connection/store_test.go | 97 +++++++++++++++++++ internal/handler/mobile_additional.go | 4 +- internal/handler/mobile_connection.go | 2 +- ..._settle_legacy_mobile_connections.down.sql | 4 + ...44_settle_legacy_mobile_connections.up.sql | 12 +++ 7 files changed, 130 insertions(+), 7 deletions(-) create mode 100644 migrations/000044_settle_legacy_mobile_connections.down.sql create mode 100644 migrations/000044_settle_legacy_mobile_connections.up.sql diff --git a/internal/apidocs/openapi.yaml b/internal/apidocs/openapi.yaml index c550bb0..77df10b 100644 --- a/internal/apidocs/openapi.yaml +++ b/internal/apidocs/openapi.yaml @@ -94,7 +94,7 @@ components: request_id: {type: string, format: uuid} status: type: string - enum: [open, claimed, confirmed, provisioning, released, rejected, lapsed] + enum: [open, claimed, confirmed, provisioning, released, rejected, lapsed, failed] vta_did: {type: string} operation: {type: string, enum: [provision_vta, grant_acl]} expires_at: {type: string, format: date-time} @@ -110,7 +110,9 @@ components: candidate_did: type: string description: Proof-bound candidate administrator DID; present only while confirmation is required. - error: {type: string} + error: + type: string + description: Why adding the administrator has not completed, or why it failed. A failed request is final; generate a new QR code to try again. MobileConnectionState: type: object required: [enabled, server_time, connection] @@ -259,7 +261,7 @@ components: description: True only for an unclaimed session that is prebuilt and waiting or whose preparation failed claim_state: type: string - enum: [open, claimed, confirmed, provisioning, released, rejected, lapsed] + enum: [open, claimed, confirmed, provisioning, released, rejected, lapsed, failed] required: [session_id, user_id, vta_name, status, url, claimed, cleanable] PoolBatch: diff --git a/internal/connection/store.go b/internal/connection/store.go index d6e74c8..7352418 100644 --- a/internal/connection/store.go +++ b/internal/connection/store.go @@ -459,6 +459,16 @@ func MarkReleased(ctx context.Context, db *gorm.DB, requestID string) error { }) } +// MarkFailed ends an additional-administrator request whose grant did not +// complete. The status must be final: anything else keeps the request active, +// which blocks every new QR code for the session and has the recovery queue +// retry the grant, restarting the VTA each time. +func MarkFailed(ctx context.Context, db *gorm.DB, requestID, reason string) error { + return db.WithContext(ctx).Model(&model.MobileConnection{}). + Where("id = ? AND operation = ? AND status IN ?", requestID, OperationGrantACL, []string{"confirmed", "provisioning"}). + Updates(map[string]any{"status": "failed", "provision_error": reason}).Error +} + // Reconcile expires unconfirmed reservations and marks initial provisioning as // released once the existing setup pipeline reports the VTA running. func Reconcile(ctx context.Context, db *gorm.DB, r *model.MobileConnection, s *model.SetupSession) error { diff --git a/internal/connection/store_test.go b/internal/connection/store_test.go index e94649d..08403a5 100644 --- a/internal/connection/store_test.go +++ b/internal/connection/store_test.go @@ -3,11 +3,13 @@ package connection import ( "context" "errors" + "os" "strings" "sync" "testing" "time" + "github.com/google/uuid" "github.com/ic3software/vtafarm-api/internal/didkey" "github.com/ic3software/vtafarm-api/internal/model" "github.com/ic3software/vtafarm-api/internal/testutil" @@ -257,3 +259,98 @@ func TestTeardownRejectsClaim(t *testing.T) { t.Fatalf("claim after teardown: %v", err) } } + +func TestFailedGrantFreesSession(t *testing.T) { + db := testutil.Postgres(t) + ctx := context.Background() + s := testutil.SetupSession(t, db, model.ModeVtaOnly) + if err := db.Model(&s).Update("status", "running").Error; err != nil { + t.Fatal(err) + } + r := mustRequest(t, db, s) + if _, err := Claim(ctx, db, *r.ClaimHandle, newDID(t)); err != nil { + t.Fatal(err) + } + if _, err := Confirm(ctx, db, s.ID, r.ID); err != nil { + t.Fatal(err) + } + if _, err := StartProvisioning(ctx, db, r.ID); err != nil { + t.Fatal(err) + } + if _, err := Change(ctx, db, s.ID, "refresh", r.ID); !errors.Is(err, ErrConflict) { + t.Fatalf("refresh during grant: %v", err) + } + if err := MarkFailed(ctx, db, r.ID, "grant failed"); err != nil { + t.Fatal(err) + } + var failed model.MobileConnection + db.First(&failed, "id = ?", r.ID) + if failed.Status != "failed" || failed.ProvisionError != "grant failed" { + t.Fatalf("not failed: %+v", failed) + } + if err := db.Model(&failed).Update("created_at", failed.CreatedAt.Add(-time.Minute)).Error; err != nil { + t.Fatal(err) + } + next, err := Change(ctx, db, s.ID, "refresh", r.ID) + if err != nil { + t.Fatalf("refresh after failure: %v", err) + } + if next.ID == r.ID || next.Status != "open" || next.Operation != OperationGrantACL { + t.Fatalf("no fresh reservation: %+v", next) + } +} + +func TestLegacyReservationsSettled(t *testing.T) { + db := testutil.Postgres(t) + ctx := context.Background() + pending := testutil.SetupSession(t, db, model.ModeVtaOnly) + accepted := testutil.SetupSession(t, db, model.ModeVtaOnly) + connectedAt := time.Now().Add(-time.Hour).UTC().Truncate(time.Microsecond) + for _, row := range []struct { + id string + session model.SetupSession + status string + connected *time.Time + }{ + {uuid.NewString(), pending, "pending", nil}, + {uuid.NewString(), accepted, "accepted", &connectedAt}, + } { + if err := db.Model(&row.session).Update("status", "running").Error; err != nil { + t.Fatal(err) + } + if err := db.Exec(`INSERT INTO mobile_connections (id, session_id, vta_did, status, created_at, expires_at, connected_at) + VALUES (?, ?, ?, ?, now() - interval '2 days', now() - interval '1 day', ?)`, row.id, row.session.ID, row.session.VtaDid, row.status, row.connected).Error; err != nil { + t.Fatal(err) + } + if _, err := Change(ctx, db, row.session.ID, "create", ""); !errors.Is(err, ErrConflict) { + t.Fatalf("%s row before migration: %v", row.status, err) + } + } + + up, err := os.ReadFile("../../migrations/000044_settle_legacy_mobile_connections.up.sql") + if err != nil { + t.Fatal(err) + } + if err := db.Exec(string(up)).Error; err != nil { + t.Fatal(err) + } + + for _, want := range []struct { + session model.SetupSession + status string + }{{pending, "lapsed"}, {accepted, "released"}} { + legacy, err := Current(db, want.session.ID) + if err != nil { + t.Fatal(err) + } + if legacy.Status != want.status { + t.Fatalf("legacy row = %s, want %s", legacy.Status, want.status) + } + if want.status == "released" && (legacy.ReleasedAt == nil || !legacy.ReleasedAt.Equal(connectedAt)) { + t.Fatalf("released_at = %v, want %v", legacy.ReleasedAt, connectedAt) + } + if _, err := Change(ctx, db, want.session.ID, "create", ""); err != nil { + t.Fatalf("%s row after migration: %v", want.status, err) + } + } +} diff --git a/internal/handler/mobile_additional.go b/internal/handler/mobile_additional.go index 3ab964e..dfc17a8 100644 --- a/internal/handler/mobile_additional.go +++ b/internal/handler/mobile_additional.go @@ -122,9 +122,7 @@ func (h *SetupHandler) runAdditionalMobile(ctx context.Context, requestID string func (h *SetupHandler) failAdditionalMobile(ctx context.Context, requestID string, err error) { log.Printf("[mobile-connection] additional request %s failed: %v", requestID, err) - if updateErr := h.db.WithContext(ctx).Model(&model.MobileConnection{}). - Where("id = ? AND operation = ? AND status IN ?", requestID, connection.OperationGrantACL, []string{"confirmed", "provisioning"}). - Update("provision_error", additionalMobileFailure).Error; updateErr != nil { + if updateErr := connection.MarkFailed(ctx, h.db, requestID, additionalMobileFailure); updateErr != nil { log.Printf("[mobile-connection] could not record failure for request %s: %v", requestID, updateErr) } } diff --git a/internal/handler/mobile_connection.go b/internal/handler/mobile_connection.go index 3e82a67..8a38e93 100644 --- a/internal/handler/mobile_connection.go +++ b/internal/handler/mobile_connection.go @@ -124,7 +124,7 @@ func (h *MobileConnectionHandler) view(r *model.MobileConnection) *mobileView { v.ClaimCheck = connection.ClaimCheck(r.AdminDid) v.CandidateDID = r.AdminDid } - if r.ProvisionError != "" && (r.Status == "confirmed" || r.Status == "provisioning") { + if r.ProvisionError != "" && (r.Status == "confirmed" || r.Status == "provisioning" || r.Status == "failed") { v.Error = r.ProvisionError } return v diff --git a/migrations/000044_settle_legacy_mobile_connections.down.sql b/migrations/000044_settle_legacy_mobile_connections.down.sql new file mode 100644 index 0000000..e7229d6 --- /dev/null +++ b/migrations/000044_settle_legacy_mobile_connections.down.sql @@ -0,0 +1,4 @@ +-- Nothing to undo: the settled rows can no longer be told apart from ones the +-- claim flow wrote, and 000041's down maps lapsed and released back to the +-- 000039 statuses anyway. +SELECT 1; diff --git a/migrations/000044_settle_legacy_mobile_connections.up.sql b/migrations/000044_settle_legacy_mobile_connections.up.sql new file mode 100644 index 0000000..d225902 --- /dev/null +++ b/migrations/000044_settle_legacy_mobile_connections.up.sql @@ -0,0 +1,12 @@ +-- 000041 added the claim-reservation statuses but left rows written by the +-- 000039 flow as they were. The reservation code knows neither 'pending' nor +-- 'accepted' and treats an unknown latest row as still active, so creating or +-- refreshing a QR code on such a session answered 409 forever. +UPDATE mobile_connections +SET status = 'lapsed' +WHERE status = 'pending'; + +UPDATE mobile_connections +SET status = 'released', + released_at = COALESCE(connected_at, provisioned_at, accepted_at, created_at) +WHERE status = 'accepted';