refactor(id): require canonical decimal request IDs

platform/base/id/id.go

@@ -18,7 +18,6 @@ package id
 import (
 	"fmt"
 	"strconv"
-	"strings"
 )
 
 // FromCounter returns the canonical decimal resource ID for value.
@@ -45,39 +44,18 @@ func Validate(id string) error {
 	return nil
 }
 
-// Compare orders legacy IDs by their numeric suffix, followed by decimal IDs numerically.
+// Compare compares two canonical positive decimal resource IDs numerically.
 // Callers must ensure both IDs belong to the same queue and resource kind.
-// This ordering requires a one-way writer switch from legacy to decimal IDs.
 func Compare(a, b string) (int, error) {
-	aCounter, bCounter := a, b
-	aSeparator := strings.LastIndexByte(a, '/')
-	bSeparator := strings.LastIndexByte(b, '/')
-	aLegacy, bLegacy := aSeparator >= 0, bSeparator >= 0
-	if aLegacy {
-		if aSeparator == 0 {
-			return 0, fmt.Errorf("invalid first resource ID %q: legacy prefix must not be empty", a)
-		}
-		aCounter = a[aSeparator+1:]
-	}
-	if bLegacy {
-		if bSeparator == 0 {
-			return 0, fmt.Errorf("invalid second resource ID %q: legacy prefix must not be empty", b)
-		}
-		bCounter = b[bSeparator+1:]
-	}
-	aValue, err := parseResourceID(aCounter)
+	aValue, err := parseResourceID(a)
 	if err != nil {
 		return 0, fmt.Errorf("invalid first resource ID %q: %w", a, err)
 	}
-	bValue, err := parseResourceID(bCounter)
+	bValue, err := parseResourceID(b)
 	if err != nil {
 		return 0, fmt.Errorf("invalid second resource ID %q: %w", b, err)
 	}
 	switch {
-	case aLegacy && !bLegacy:
-		return -1, nil
-	case !aLegacy && bLegacy:
-		return 1, nil
 	case aValue < bValue:
 		return -1, nil
 	case aValue > bValue:

platform/base/id/id_test.go

@@ -90,28 +90,22 @@ func TestCompare(t *testing.T) {
 		{name: "newer", a: "10", b: "9", want: 1},
 		{name: "invalid first", a: "bad", b: "10", wantErr: true},
 		{name: "invalid second", a: "9", b: "batch.10", wantErr: true},
-		{name: "legacy counters sort numerically", a: "queue/9", b: "queue/10", want: -1},
-		{name: "newer legacy counter", a: "queue/10", b: "queue/9", want: 1},
-		{name: "decimal is newer despite smaller counter", a: "1", b: "queue/42", want: 1},
-		{name: "legacy is older despite larger counter", a: "queue/42", b: "1", want: -1},
-		{name: "same counter across formats is not equal", a: "queue/42", b: "42", want: -1},
-		{name: "same legacy ID", a: "queue/42", b: "queue/42"},
-		{name: "queue containing slashes", a: "1", b: "request/monorepo/main/42", want: 1},
-		{name: "legacy queue containing slashes", a: "request/monorepo/main/9", b: "request/monorepo/main/10", want: -1},
-		{name: "maximum legacy counter is older than first decimal", a: "request/monorepo/main/9223372036854775807", b: "1", want: -1},
-		{name: "large legacy counters retain precision", a: "queue/9223372036854775806", b: "queue/9223372036854775807", want: -1},
-		{name: "invalid legacy suffix", a: "queue/bad", b: "queue/42", wantErr: true},
-		{name: "zero legacy suffix", a: "1", b: "queue/0", wantErr: true},
-		{name: "noncanonical legacy suffix", a: "queue/01", b: "queue/42", wantErr: true},
-		{name: "empty first legacy prefix", a: "/9", b: "10", wantErr: true},
-		{name: "empty second legacy prefix", a: "9", b: "/10", wantErr: true},
-		{name: "empty legacy suffix", a: "queue/", b: "10", wantErr: true},
-		{name: "overflow legacy suffix", a: "queue/9223372036854775808", b: "10", wantErr: true},
-		{name: "zero decimal against legacy", a: "0", b: "queue/42", wantErr: true},
-		{name: "noncanonical decimal against legacy", a: "queue/42", b: "01", wantErr: true},
-		{name: "negative decimal against legacy", a: "-1", b: "queue/42", wantErr: true},
-		{name: "empty decimal against legacy", a: "queue/42", wantErr: true},
-		{name: "overflow decimal against legacy", a: "9223372036854775808", b: "queue/42", wantErr: true},
+		{name: "large counters retain precision", a: "9223372036854775806", b: "9223372036854775807", want: -1},
+		{name: "legacy first ID", a: "queue/42", b: "1", wantErr: true},
+		{name: "legacy second ID", a: "1", b: "queue/42", wantErr: true},
+		{name: "both legacy IDs", a: "queue/9", b: "queue/10", wantErr: true},
+		{name: "same legacy ID", a: "queue/42", b: "queue/42", wantErr: true},
+		{name: "legacy queue containing slashes", a: "request/monorepo/main/9", b: "10", wantErr: true},
+		{name: "empty first ID", b: "1", wantErr: true},
+		{name: "zero first ID", a: "0", b: "1", wantErr: true},
+		{name: "negative first ID", a: "-1", b: "1", wantErr: true},
+		{name: "noncanonical first ID", a: "01", b: "1", wantErr: true},
+		{name: "overflow first ID", a: "9223372036854775808", b: "1", wantErr: true},
+		{name: "empty second ID", a: "1", wantErr: true},
+		{name: "zero second ID", a: "1", b: "0", wantErr: true},
+		{name: "negative second ID", a: "1", b: "-1", wantErr: true},
+		{name: "noncanonical second ID", a: "1", b: "01", wantErr: true},
+		{name: "overflow second ID", a: "1", b: "9223372036854775808", wantErr: true},
 	}
 
 	for _, tt := range tests {

stovepipe/controller/ingest_test.go

@@ -185,7 +185,7 @@ func TestIngestController_Ingest(t *testing.T) {
 			wantID: "7",
 		},
 		{
-			name:  "new ID advances legacy latest pointer and publishes",
+			name:  "legacy latest pointer prevents advancement and publish",
 			queue: testQueue,
 			setup: func(m ingestMocks) {
 				expectResolve(m)
@@ -198,14 +198,11 @@ func TestIngestController_Ingest(t *testing.T) {
 				m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
 					Name: testQueue, LatestRequestID: "request/" + testQueue + "/42", Version: 1,
 				}, nil)
-				updated := entity.Queue{Name: testQueue, LatestRequestID: "1", Version: 1}
-				updateCall := m.queueStore.EXPECT().Update(gomock.Any(), updated, int32(1), int32(2)).Return(nil)
-				m.publisher.EXPECT().Publish(gomock.Any(), "process", gomock.Any()).Return(nil).After(updateCall)
 			},
-			wantID: "1",
+			wantErr: true,
 		},
 		{
-			name:  "retry repairs accepted decimal request behind legacy pointer",
+			name:  "retry rejects legacy latest pointer",
 			queue: testQueue,
 			setup: func(m ingestMocks) {
 				expectResolve(m)
@@ -215,11 +212,8 @@ func TestIngestController_Ingest(t *testing.T) {
 				m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
 					Name: testQueue, LatestRequestID: "request/" + testQueue + "/42", Version: 1,
 				}, nil)
-				updated := entity.Queue{Name: testQueue, LatestRequestID: "1", Version: 1}
-				updateCall := m.queueStore.EXPECT().Update(gomock.Any(), updated, int32(1), int32(2)).Return(nil)
-				m.publisher.EXPECT().Publish(gomock.Any(), "process", gomock.Any()).Return(nil).After(updateCall)
 			},
-			wantID: "1",
+			wantErr: true,
 		},
 		{
 			name:  "dedup with existing accepted request republishes without minting",

stovepipe/controller/record/record_test.go

@@ -291,11 +291,6 @@ func TestProcess_AdvancesBookmarkOnSuccess(t *testing.T) {
 			stored:  queueRow("git://remote/monorepo/main/old", "3", 4),
 			wantURI: testURI,
 		},
-		{
-			name:    "stored legacy bookmark is older",
-			stored:  queueRow("git://remote/monorepo/main/old", "request/"+testQueue+"/42", 4),
-			wantURI: testURI,
-		},
 	}
 
 	for _, tt := range tests {
@@ -339,6 +334,22 @@ func TestProcess_AdvancesBookmarkOnSuccess(t *testing.T) {
 	}
 }
 
+func TestProcess_RejectsLegacyBookmark(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	c, m := newController(t, ctrl)
+
+	m.reqStore.EXPECT().Get(gomock.Any(), testID).
+		Return(requestWithState(entity.RequestStateSucceeded), nil)
+	var fact entity.ValidationFact
+	m.expectFactCreated(&fact)
+	m.queueStore.EXPECT().Get(gomock.Any(), testQueue).
+		Return(queueRow("git://remote/monorepo/main/old", "request/"+testQueue+"/42", 4), nil)
+
+	err := c.Process(queueContext(), delivery(t, ctrl, recordPayload(t, testID)))
+	require.Error(t, err)
+	assert.Empty(t, m.hooks.events)
+}
+
 func TestProcess_RecordsNamedProjectStatusResults(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	c, m := newController(t, ctrl)

stovepipe/entity/request_id.go

@@ -19,8 +19,7 @@ import "github.com/uber/submitqueue/platform/base/id"
 // CompareRequestID compares ingest order of two request IDs in the same queue.
 // Callers must ensure the IDs belong to the same queue.
 // Returns -1 if a is older than b, 0 if equal, 1 if a is newer than b.
-// Decimal IDs are newer than all legacy request/<queue>/<counter> IDs.
-// Within each format, ordering is by numeric counter.
+// IDs must be canonical positive decimal strings whose scope is carried separately.
 func CompareRequestID(a, b string) (int, error) {
 	return id.Compare(a, b)
 }

stovepipe/entity/request_id_test.go

@@ -54,6 +54,8 @@ func TestCompareRequestID(t *testing.T) {
 			want: -1,
 		},
 		{name: "prefixed ID", a: "request.1", b: "2", wantErr: true},
+		{name: "legacy first ID", a: "request/monorepo/main/1", b: "2", wantErr: true},
+		{name: "legacy second ID", a: "1", b: "request/monorepo/main/2", wantErr: true},
 	}
 
 	for _, tt := range tests {