[swarming] Refactor entity key <-> string ID conversions.

Clarify there are two flavors of string task IDs, but that it is
legacy leftovers.

Move conversions to a separate *.go file and rename them to
indicate they are not intrinsically tied to TaskRequest entity,
since string IDs are also technically representing TaskRunResult
entities.

R=jonahhooper@google.com, randymaldonado@google.com

Change-Id: Ic5065a5e4926faa675c732db3d9f8b4a36da28c3
Reviewed-on: https://chromium-review.googlesource.com/c/infra/luci/luci-go/+/5106608
Reviewed-by: Jonah Hooper <jonahhooper@google.com>
Commit-Queue: Vadim Shtayura <vadimsh@chromium.org>
Reviewed-by: Randy Maldonado <randymaldonado@google.com>
diff --git a/swarming/server/bq/bqconverter.go b/swarming/server/bq/bqconverter.go
index 38eadd0..b0f5986 100644
--- a/swarming/server/bq/bqconverter.go
+++ b/swarming/server/bq/bqconverter.go
@@ -162,7 +162,7 @@
 		Resultdb: &bqpb.ResultDBCfg{
 			Enable: tr.ResultDB.Enable,
 		},
-		TaskId: model.PackTaskRequestKey(tr.Key),
+		TaskId: model.RequestKeyToTaskID(tr.Key, model.AsRequest),
 		PubsubNotification: &bqpb.PubSub{
 			Topic:    tr.PubSubTopic,
 			Userdata: tr.PubSubUserData,
diff --git a/swarming/server/bq/bqconverter_test.go b/swarming/server/bq/bqconverter_test.go
index e228e5b..739c03e 100644
--- a/swarming/server/bq/bqconverter_test.go
+++ b/swarming/server/bq/bqconverter_test.go
@@ -232,7 +232,7 @@
 	Convey("Convert TaskRequest with empty parent task", t, func() {
 		ctx := memory.Use(context.Background())
 		taskID := "65aba3a3e6b99310"
-		key, err := model.UnpackTaskRequestKey(ctx, taskID)
+		key, err := model.TaskIDToRequestKey(ctx, taskID)
 		So(err, ShouldBeNil)
 		sampleRequest := createSampleTaskRequest(key, testTime)
 		expected := createSampleBQTaskRequest(taskID, testTime)
@@ -243,7 +243,7 @@
 	Convey("Converting empty EnvPrefixes works", t, func() {
 		ctx := memory.Use(context.Background())
 		taskID := "65aba3a3e6b99310"
-		key, err := model.UnpackTaskRequestKey(ctx, taskID)
+		key, err := model.TaskIDToRequestKey(ctx, taskID)
 		So(err, ShouldBeNil)
 		sampleRequest := createSampleTaskRequest(key, testTime)
 		// Set this field to zero
diff --git a/swarming/server/model/taskid.go b/swarming/server/model/taskid.go
new file mode 100644
index 0000000..7541071
--- /dev/null
+++ b/swarming/server/model/taskid.go
@@ -0,0 +1,89 @@
+// Copyright 2023 The LUCI Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package model
+
+import (
+	"context"
+	"fmt"
+	"strconv"
+
+	"go.chromium.org/luci/common/errors"
+	"go.chromium.org/luci/gae/service/datastore"
+	"go.chromium.org/luci/grpc/grpcutil"
+)
+
+// taskRequestIDMask is xored with TaskRequest entity ID.
+const taskRequestIDMask = 0x7fffffffffffffff
+
+// TaskIDVariant is an enum with possible variants of task ID encoding.
+type TaskIDVariant int
+
+const (
+	// AsRequest instructs RequestKeyToTaskID to produce an ID ending with `0`.
+	AsRequest TaskIDVariant = 0
+	// AsRunResult instructs RequestKeyToTaskID to produce an ID ending with `1`.
+	AsRunResult TaskIDVariant = 1
+)
+
+// RequestKeyToTaskID converts TaskRequest entity key to a string form used in
+// external APIs.
+//
+// For legacy reasons they are two flavors of string task IDs:
+//  1. A "packed TaskRequest key", aka "packed TaskResultSummary" key. It is
+//     a hex string ending with 0, e.g. `6663cfc78b41fb10`. Pass AsRequest as
+//     the second argument to request this variant.
+//  2. A "packed TaskRunResult key". It is a hex string ending with 1, e.g.
+//     `6663cfc78b41fb11`. Pass AsRunResult as the second argument to request
+//     this variant.
+//
+// Some APIs return the first form, others return the second. There's no clear
+// logical reason why they do so anymore. They do it for backward compatibility
+// with much older API, where these differences mattered.
+//
+// Panics if `key` is not a TaskRequest key.
+func RequestKeyToTaskID(key *datastore.Key, variant TaskIDVariant) string {
+	if key.Kind() != "TaskRequest" {
+		panic(fmt.Sprintf("expecting TaskRequest key, but got %q", key.Kind()))
+	}
+	switch variant {
+	case AsRequest:
+		return fmt.Sprintf("%x0", key.IntID()^taskRequestIDMask)
+	case AsRunResult:
+		return fmt.Sprintf("%x1", key.IntID()^taskRequestIDMask)
+	default:
+		panic(fmt.Sprintf("invalid variant %d", variant))
+	}
+}
+
+// TaskIDToRequestKey returns TaskRequest entity key given a task ID string.
+//
+// The task ID is something that looks like `6663cfc78b41fb10`, it is either
+// a "packed TaskRequest key" (when ends with 0) or "a packed TaskRunResult key"
+// (when ends with non-0). See RequestKeyToTaskID.
+//
+// Task request key is a root key of the hierarchy of entities representing
+// a particular task. All key constructor functions for such entities take
+// the request key as an argument.
+func TaskIDToRequestKey(ctx context.Context, taskID string) (*datastore.Key, error) {
+	if err := checkIsHex(taskID, 2); err != nil {
+		return nil, errors.Annotate(err, "bad task ID").Tag(grpcutil.InvalidArgumentTag).Err()
+	}
+	// Chop the suffix byte. It is TaskRunResult index, we don't care about it.
+	num, err := strconv.ParseInt(taskID[:len(taskID)-1], 16, 64)
+	if err != nil {
+		return nil, errors.Annotate(err, "bad task ID").Tag(grpcutil.InvalidArgumentTag).Err()
+	}
+	return datastore.NewKey(ctx, "TaskRequest", "", num^taskRequestIDMask, nil), nil
+}
diff --git a/swarming/server/model/taskid_test.go b/swarming/server/model/taskid_test.go
new file mode 100644
index 0000000..89948a4
--- /dev/null
+++ b/swarming/server/model/taskid_test.go
@@ -0,0 +1,67 @@
+// Copyright 2023 The LUCI Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package model
+
+import (
+	"context"
+	"testing"
+
+	"go.chromium.org/luci/gae/impl/memory"
+	"go.chromium.org/luci/gae/service/datastore"
+
+	. "github.com/smartystreets/goconvey/convey"
+	. "go.chromium.org/luci/common/testing/assertions"
+)
+
+func TestTaskID(t *testing.T) {
+	t.Parallel()
+
+	Convey("With datastore", t, func() {
+		ctx := memory.Use(context.Background())
+
+		Convey("Key to string", func() {
+			key := datastore.NewKey(ctx, "TaskRequest", "", 8787878774240697582, nil)
+			So("60b2ed0a43023110", ShouldEqual, RequestKeyToTaskID(key, AsRequest))
+			So("60b2ed0a43023111", ShouldEqual, RequestKeyToTaskID(key, AsRunResult))
+		})
+
+		Convey("String to key: AsRequest", func() {
+			key, err := TaskIDToRequestKey(ctx, "60b2ed0a43023110")
+			So(err, ShouldBeNil)
+			So(key.IntID(), ShouldEqual, 8787878774240697582)
+		})
+
+		Convey("String to key: AsRunResult", func() {
+			key, err := TaskIDToRequestKey(ctx, "60b2ed0a43023111")
+			So(err, ShouldBeNil)
+			So(key.IntID(), ShouldEqual, 8787878774240697582)
+		})
+
+		Convey("Bad hex", func() {
+			_, err := TaskIDToRequestKey(ctx, "60b2ed0a4302311z")
+			So(err, ShouldErrLike, "bad task ID: bad lowercase hex string")
+		})
+
+		Convey("Empty", func() {
+			_, err := TaskIDToRequestKey(ctx, "")
+			So(err, ShouldErrLike, "bad task ID: too small")
+		})
+
+		Convey("Overflow", func() {
+			_, err := TaskIDToRequestKey(ctx, "ff60b2ed0a4302311f")
+			So(err, ShouldErrLike, "value out of range")
+		})
+	})
+}
diff --git a/swarming/server/model/taskrequest.go b/swarming/server/model/taskrequest.go
index 31c3ceb..1a0301f 100644
--- a/swarming/server/model/taskrequest.go
+++ b/swarming/server/model/taskrequest.go
@@ -16,22 +16,15 @@
 
 import (
 	"context"
-	"fmt"
-	"strconv"
 	"time"
 
 	"go.chromium.org/luci/auth/identity"
-	"go.chromium.org/luci/common/errors"
 	"go.chromium.org/luci/gae/service/datastore"
-	"go.chromium.org/luci/grpc/grpcutil"
 
 	"go.chromium.org/luci/swarming/proto/api_v2"
 	configpb "go.chromium.org/luci/swarming/proto/config"
 )
 
-// taskRequestIDMask is xored with TaskRequest entity ID.
-const taskRequestIDMask = 0x7fffffffffffffff
-
 // TaskRequest contains a user request to execute a task.
 //
 // Key ID is a decreasing integer based on time plus some randomness on lower
@@ -486,42 +479,7 @@
 	return FromJSONProperty(prop, p)
 }
 
-////////////////////////////////////////////////////////////////////////////////
-
-// UnpackTaskRequestKey returns TaskRequest entity key given a task ID string.
-//
-// The task ID is something that looks like "60b2ed0a43023110", it is either
-// a "packed TaskResultSummary key" (when ends with 0) or "a packed
-// TaskRunResult key" (when ends with non-0).
-//
-// Task request key is a root key of the hierarchy of entities representing
-// a particular task. All key constructor functions for such entities take
-// the request key as an argument.
-func UnpackTaskRequestKey(ctx context.Context, taskID string) (*datastore.Key, error) {
-	if err := checkIsHex(taskID, 2); err != nil {
-		return nil, errors.Annotate(err, "bad task ID").Tag(grpcutil.InvalidArgumentTag).Err()
-	}
-	// Chop the suffix byte. It is TaskRunResult index, we don't care about it.
-	num, err := strconv.ParseInt(taskID[:len(taskID)-1], 16, 64)
-	if err != nil {
-		return nil, errors.Annotate(err, "bad task ID").Tag(grpcutil.InvalidArgumentTag).Err()
-	}
-	return datastore.NewKey(ctx, "TaskRequest", "", num^taskRequestIDMask, nil), nil
-}
-
 // NewTaskRequestID generates an ID for a new task.
 func NewTaskRequestID(ctx context.Context) int64 {
 	panic("not implemented")
 }
-
-// PackTaskRequestKey is the inverse of UnpackTaskRequestKey
-func PackTaskRequestKey(key *datastore.Key) string {
-	if key.Kind() != "TaskRequest" {
-		panic(fmt.Sprintf("expecting TaskRequest key, but got %q", key.Kind()))
-	}
-	integerID := key.IntID()
-	// We add a 0 to the end of the formatted string for legacy reasons
-	// This behaviour came from when swarming used to support retries
-	// See https://source.chromium.org/chromium/infra/infra/+/main:luci/appengine/swarming/server/task_pack.py;l=110;drc=361c3fbf33a686c792b3d148d67e346ac9b53523
-	return fmt.Sprintf("%x0", integerID^taskRequestIDMask)
-}
diff --git a/swarming/server/model/taskrequest_test.go b/swarming/server/model/taskrequest_test.go
index f4017f7..8f29059 100644
--- a/swarming/server/model/taskrequest_test.go
+++ b/swarming/server/model/taskrequest_test.go
@@ -25,66 +25,8 @@
 	configpb "go.chromium.org/luci/swarming/proto/config"
 
 	. "github.com/smartystreets/goconvey/convey"
-	. "go.chromium.org/luci/common/testing/assertions"
 )
 
-func TestUnpackTaskRequestKey(t *testing.T) {
-	t.Parallel()
-
-	Convey("With datastore", t, func() {
-		ctx := memory.Use(context.Background())
-
-		Convey("Good task ID: TaskResultSummary", func() {
-			key, err := UnpackTaskRequestKey(ctx, "60b2ed0a43023110")
-			So(err, ShouldBeNil)
-			So(key.IntID(), ShouldEqual, 8787878774240697582)
-		})
-
-		Convey("Good task ID: TaskRunResult", func() {
-			key, err := UnpackTaskRequestKey(ctx, "60b2ed0a43023111")
-			So(err, ShouldBeNil)
-			So(key.IntID(), ShouldEqual, 8787878774240697582)
-		})
-
-		Convey("Bad hex", func() {
-			_, err := UnpackTaskRequestKey(ctx, "60b2ed0a4302311z")
-			So(err, ShouldErrLike, "bad task ID: bad lowercase hex string")
-		})
-
-		Convey("Empty", func() {
-			_, err := UnpackTaskRequestKey(ctx, "")
-			So(err, ShouldErrLike, "bad task ID: too small")
-		})
-
-		Convey("Overflow", func() {
-			_, err := UnpackTaskRequestKey(ctx, "ff60b2ed0a4302311f")
-			So(err, ShouldErrLike, "value out of range")
-		})
-	})
-}
-
-func TestPackTaskRequestKey(t *testing.T) {
-	t.Parallel()
-
-	Convey("With datastore", t, func() {
-		ctx := memory.Use(context.Background())
-
-		Convey("Pack TaskRequestKey", func() {
-			expected := "60b2ed0a43023110"
-			key := datastore.NewKey(ctx, "TaskRequest", "", 8787878774240697582, nil)
-			So(expected, ShouldEqual, PackTaskRequestKey(key))
-		})
-
-		Convey("Round trip pack and unpack", func() {
-			expected := "65beeeafbd2a4b10"
-			key, err := UnpackTaskRequestKey(ctx, expected)
-			So(err, ShouldBeNil)
-			So(8765149556731894606, ShouldEqual, key.IntID())
-			So(expected, ShouldEqual, PackTaskRequestKey(key))
-		})
-	})
-}
-
 func TestTaskRequest(t *testing.T) {
 	t.Parallel()
 
@@ -158,7 +100,7 @@
 			}
 		}
 
-		key, err := UnpackTaskRequestKey(ctx, "65aba3a3e6b99310")
+		key, err := TaskIDToRequestKey(ctx, "65aba3a3e6b99310")
 		So(err, ShouldBeNil)
 
 		fullyPopulated := TaskRequest{
diff --git a/swarming/server/rbe/reservation.go b/swarming/server/rbe/reservation.go
index 08d1ec7..9d0dcda 100644
--- a/swarming/server/rbe/reservation.go
+++ b/swarming/server/rbe/reservation.go
@@ -405,7 +405,7 @@
 // newTaskToRunFromPayload returns an empty TaskToRun struct with populated
 // entity key using information from the TaskPayload proto.
 func newTaskToRunFromPayload(ctx context.Context, p *internalspb.TaskPayload) (*model.TaskToRun, error) {
-	taskReqKey, err := model.UnpackTaskRequestKey(ctx, p.TaskId)
+	taskReqKey, err := model.TaskIDToRequestKey(ctx, p.TaskId)
 	if err != nil {
 		return nil, err
 	}
diff --git a/swarming/server/rbe/reservation_test.go b/swarming/server/rbe/reservation_test.go
index 46885ed..f9cc4be 100644
--- a/swarming/server/rbe/reservation_test.go
+++ b/swarming/server/rbe/reservation_test.go
@@ -88,7 +88,7 @@
 			Priority: 123,
 		}
 
-		taskReqKey, err := model.UnpackTaskRequestKey(ctx, enqueueTask.Payload.TaskId)
+		taskReqKey, err := model.TaskIDToRequestKey(ctx, enqueueTask.Payload.TaskId)
 		So(err, ShouldBeNil)
 		taskToRun := &model.TaskToRun{
 			Key: model.TaskToRunKey(ctx, taskReqKey,
@@ -267,7 +267,7 @@
 				if reapable {
 					exp.Set(testclock.TestRecentTimeUTC.Add(time.Hour))
 				}
-				taskReqKey, _ := model.UnpackTaskRequestKey(ctx, taskID)
+				taskReqKey, _ := model.TaskIDToRequestKey(ctx, taskID)
 				So(datastore.Put(ctx, &model.TaskToRun{
 					Key:        model.TaskToRunKey(ctx, taskReqKey, taskToRunShard, taskToRunID),
 					Expiration: exp,