multi: preserve action order across SQL migration

Migrate KV firewall actions by traversing the global actions-index
instead of iterating per-session buckets, so SQL action IDs follow the
same global creation order as the legacy KV store.

Also stabilize SQL action listing by ordering on created_at and id,
using id as a deterministic tie-breaker when multiple actions share the
same timestamp.

This is needed because KV actions have a real global sequence in
actions-index, while the old migration assigned SQL IDs based on bucket
traversal order. That could reorder legacy actions during migration.
Separately, ordering by created_at alone was not stable for equal
timestamps, so action queries could return different orders for the
same data.

NOTE: This commit explicitly edits the previous migration instead of
adding a new one. This is ok as SQL dbs are not yet supported in
production, so there are no live deployments to worry about.

db/sqlc/actions_custom.go

@@ -175,7 +175,7 @@ func buildListActionsQuery(params ListActionsParams) (string, []interface{}) {
 	if params.Reversed {
 		order = "DESC"
 	}
-	query += " ORDER BY a.created_at " + order
+	query += " ORDER BY a.created_at " + order + ", a.id " + order
 
 	// Maybe paginate.
 	if params.Pagination != nil {

db/sqlc/migrations/000005_actions.down.sql

@@ -1,5 +1,5 @@
 DROP INDEX IF NOT EXISTS actions_state_idx;
 DROP INDEX IF NOT EXISTS actions_session_id_idx;
 DROP INDEX IF NOT EXISTS actions_feature_name_idx;
-DROP INDEX IF NOT EXISTS actions_created_at_idx;
+DROP INDEX IF EXISTS actions_created_at_id_idx;
 DROP TABLE IF EXISTS actions;

db/sqlc/migrations/000005_actions.up.sql

@@ -49,4 +49,8 @@ CREATE TABLE IF NOT EXISTS actions(
 CREATE INDEX IF NOT EXISTS actions_state_idx ON actions(action_state);
 CREATE INDEX IF NOT EXISTS actions_session_id_idx ON actions(session_id);
 CREATE INDEX IF NOT EXISTS actions_feature_name_idx ON actions(feature_name);
-CREATE INDEX IF NOT EXISTS actions_created_at_idx ON actions(created_at);
+-- Actions are commonly queried in chronological order. Include the primary key
+-- as a stable tie-breaker so callers can order by (created_at, id) and still
+-- get deterministic results when multiple actions share the same timestamp.
+CREATE INDEX IF NOT EXISTS actions_created_at_id_idx
+    ON actions(created_at, id);

db/sqlcmig6/actions_custom.go

@@ -175,7 +175,7 @@ func buildListActionsQuery(params ListActionsParams) (string, []interface{}) {
 	if params.Reversed {
 		order = "DESC"
 	}
-	query += " ORDER BY a.created_at " + order
+	query += " ORDER BY a.created_at " + order + ", a.id " + order
 
 	// Maybe paginate.
 	if params.Pagination != nil {

firewalldb/sql_migration.go

@@ -930,32 +930,42 @@ func migrateActionsToSQL(ctx context.Context, kvStore *bbolt.DB,
 			return fmt.Errorf("actions bucket not found")
 		}
 
+		actionsIndexBucket := actionsBucket.Bucket(actionsIndex)
+		if actionsIndexBucket == nil {
+			return fmt.Errorf("actions->actions-index bucket not " +
+				"found")
+		}
+
 		sessionsBucket := actionsBucket.Bucket(actionsKey)
 		if sessionsBucket == nil {
 			return fmt.Errorf("actions->sessions bucket not found")
 		}
 
-		// Iterate over session ID buckets (i.e. what we should name
-		// macaroon IDs).
-		//
-		// nolint:ll
-		return sessionsBucket.ForEach(func(macID []byte, v []byte) error {
-			if v != nil {
-				return fmt.Errorf("expected only sub-buckets " +
-					"in sessions bucket")
+		// Iterate over the global actions index so that SQL insertion
+		// order follows the legacy KVDB action order rather than bucket
+		// traversal order.
+		return actionsIndexBucket.ForEach(func(seqNo []byte,
+			locatorBytes []byte) error {
+
+			if locatorBytes == nil {
+				return fmt.Errorf("unexpected nested bucket " +
+					"under actions-index")
 			}
 
-			sessBucket := sessionsBucket.Bucket(macID)
-			if sessBucket == nil {
-				return fmt.Errorf("session bucket for %x not "+
-					"found", macID)
+			locator, err := deserializeActionLocator(
+				bytes.NewReader(locatorBytes),
+			)
+			if err != nil {
+				return fmt.Errorf("unable to deserialize "+
+					"action locator for seq %x: %w",
+					seqNo, err)
 			}
 
 			// fetch the full macaroon root key ID based on the
 			// macaroon identifier for the action (the last 4 bytes
 			// of the root key ID).
 			var macIDArr [4]byte
-			copy(macIDArr[:], macID)
+			copy(macIDArr[:], locator.sessionID[:])
 
 			macRootKeyID, ok := macMap[macIDArr]
 			if !ok {
@@ -970,65 +980,43 @@ func migrateActionsToSQL(ctx context.Context, kvStore *bbolt.DB,
 				// backends.
 				log.Warnf("No macaroon root key ID found for "+
 					"macaroon ID %x, using zeroes for "+
-					"the first 4 bytes", macID)
+					"the first 4 bytes",
+					locator.sessionID[:])
 
 				macRootKeyID = make([]byte, 8)
 				copy(macRootKeyID[4:], macIDArr[:])
 			}
 
-			// Iterate over the actions inside each session/macaroon
-			// ID.
-			return sessBucket.ForEach(func(actionID,
-				actionBytes []byte) error {
+			action, err := getAction(sessionsBucket, locator)
+			if err != nil {
+				return fmt.Errorf("unable to deserialize "+
+					"action for locator %+v: %w",
+					locator, err)
+			}
 
-				if actionBytes == nil {
-					return fmt.Errorf("unexpected nested "+
-						"bucket under session %x",
-						macID)
-				}
+			log.Tracef("Migrated Action: SeqNo: %x, Macaroon ID: "+
+				"%x, ActionID: %d, Actor: %s, Feature: %s",
+				seqNo, locator.sessionID[:], locator.actionID,
+				action.ActorName, action.FeatureName)
 
-				sessionID, err := session.IDFromBytes(macID)
-				if err != nil {
-					// This should be unreachable, as the
-					// macID should always be 4 bytes long.
-					return fmt.Errorf("invalid session ID "+
-						"format %x: %v", macID, err)
-				}
+			// Now proceed to migrate the action, and also validate
+			// that the action was correctly migrated.
+			err = migrateActionToSQL(
+				ctx, sqlTx, acctsMap, sessMap, action,
+				macRootKeyID,
+			)
+			if err != nil {
+				return fmt.Errorf("migrating action to SQL "+
+					"failed: %w", err)
+			}
 
-				action, err := DeserializeAction(
-					bytes.NewReader(actionBytes), sessionID,
-				)
-				if err != nil {
-					return fmt.Errorf("unable to "+
-						"deserialize action in "+
-						"session %x: %w", macID, err)
-				}
+			migCount++
+			if migCount%migrationProgressLogInterval == 0 {
+				log.Infof("Migrated %d actions from KV to SQL",
+					migCount)
+			}
 
-				log.Tracef("Migrated Action: Macaroon ID: %x, "+
-					"ActionID: %x, Actor: %s, Feature: %s",
-					macID, actionID, action.ActorName,
-					action.FeatureName)
-
-				// Now proceed to migrate the action, and also
-				// validate that the action was correctly
-				// migrated.
-				err = migrateActionToSQL(
-					ctx, sqlTx, acctsMap, sessMap, action,
-					macRootKeyID,
-				)
-				if err != nil {
-					return fmt.Errorf("migrating action "+
-						"to SQL failed: %w", err)
-				}
-
-				migCount++
-				if migCount%migrationProgressLogInterval == 0 {
-					log.Infof("Migrated %d actions from "+
-						"KV to SQL", migCount)
-				}
-
-				return nil
-			})
+			return nil
 		})
 	})
 	if err != nil {

firewalldb/sql_migration_test.go

@@ -479,6 +479,10 @@ func TestFirewallDBMigration(t *testing.T) {
 			name:       "action with no session or account",
 			populateDB: actionNoSessionOrAccount,
 		},
+		{
+			name:       "actions with same timestamp",
+			populateDB: actionsWithSameTimestamp,
+		},
 		{
 			name:       "action with empty RPCParamsJson",
 			populateDB: actionEmptyRPCParamsJson,
@@ -1281,6 +1285,36 @@ func actionNoSessionOrAccount(t *testing.T, ctx context.Context,
 	}
 }
 
+// actionsWithSameTimestamp adds multiple actions at the same timestamp and
+// returns them in insertion order. This makes sure migration validation also
+// covers the SQL (created_at, id) tie-breaker.
+func actionsWithSameTimestamp(t *testing.T, ctx context.Context,
+	boltDB *BoltDB, _ session.Store, _ accounts.Store,
+	rStore *rootKeyMockStore) *expectedResult {
+
+	actions := make([]*Action, 0, 3)
+	for i := range 3 {
+		rootKey := rStore.addRandomRootKey()
+
+		actionReq := testActionReq
+		actionReq.MacaroonRootKeyID = fn.Some(rootKey)
+		actionReq.SessionID = fn.None[session.ID]()
+		actionReq.AccountID = fn.None[accounts.AccountID]()
+		actionReq.FeatureName = fmt.Sprintf("same-time-%d", i+1)
+
+		actions = append(
+			actions,
+			addActionAtCurrentTime(t, ctx, boltDB, &actionReq),
+		)
+	}
+
+	return &expectedResult{
+		kvEntries: []*kvEntry{},
+		privPairs: make(privacyPairs),
+		actions:   actions,
+	}
+}
+
 // actionEmptyRPCParamsJson adds an action which has no RPCParamsJson set.
 func actionEmptyRPCParamsJson(t *testing.T, ctx context.Context,
 	boltDB *BoltDB, _ session.Store, _ accounts.Store,
@@ -1986,6 +2020,14 @@ func addAction(t *testing.T, ctx context.Context, boltDB *BoltDB,
 	// of a session or account that it might be linked to.
 	boltDB.clock = clock.NewTestClock(boltDB.clock.Now().Add(time.Second))
 
+	return addActionAtCurrentTime(t, ctx, boltDB, actionReq)
+}
+
+// addActionAtCurrentTime adds an action using the bolt DB's current clock
+// value without advancing it first.
+func addActionAtCurrentTime(t *testing.T, ctx context.Context, boltDB *BoltDB,
+	actionReq *AddActionReq) *Action {
+
 	aLocator, err := boltDB.AddAction(ctx, actionReq)
 	require.NoError(t, err)