From 96f17adb501e7adc5c06843957a1e9d74754c877 Mon Sep 17 00:00:00 2001 From: mnoah1 Date: Mon, 5 Oct 2026 20:06:32 +0000 Subject: [PATCH] fix(id): preserve legacy IDs across decimal migration --- platform/base/id/id.go | 29 +++++++++++++++-- platform/base/id/id_test.go | 24 +++++++++++++- stovepipe/controller/ingest_test.go | 37 ++++++++++++++++++++++ stovepipe/controller/record/record_test.go | 5 +++ stovepipe/entity/request_id.go | 4 ++- 5 files changed, 94 insertions(+), 5 deletions(-) diff --git a/platform/base/id/id.go b/platform/base/id/id.go index 71ed9d28..d9e2eee5 100644 --- a/platform/base/id/id.go +++ b/platform/base/id/id.go @@ -18,6 +18,7 @@ package id import ( "fmt" "strconv" + "strings" ) // FromCounter returns the canonical decimal resource ID for value. @@ -44,17 +45,39 @@ func Validate(id string) error { return nil } -// Compare compares two canonical resource IDs numerically. +// Compare orders legacy IDs by their numeric suffix, followed by decimal 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) { - aValue, err := parseResourceID(a) + 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) if err != nil { return 0, fmt.Errorf("invalid first resource ID %q: %w", a, err) } - bValue, err := parseResourceID(b) + bValue, err := parseResourceID(bCounter) 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: diff --git a/platform/base/id/id_test.go b/platform/base/id/id_test.go index a6b0b08d..5652a716 100644 --- a/platform/base/id/id_test.go +++ b/platform/base/id/id_test.go @@ -88,8 +88,30 @@ func TestCompare(t *testing.T) { {name: "older", a: "9", b: "10", want: -1}, {name: "equal", a: "42", b: "42"}, {name: "newer", a: "10", b: "9", want: 1}, - {name: "invalid first", a: "queue/9", b: "10", wantErr: true}, + {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}, } for _, tt := range tests { diff --git a/stovepipe/controller/ingest_test.go b/stovepipe/controller/ingest_test.go index 78283488..5ab426eb 100644 --- a/stovepipe/controller/ingest_test.go +++ b/stovepipe/controller/ingest_test.go @@ -184,6 +184,43 @@ func TestIngestController_Ingest(t *testing.T) { }, wantID: "7", }, + { + name: "new ID advances legacy latest pointer and publishes", + queue: testQueue, + setup: func(m ingestMocks) { + expectResolve(m) + m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testURI).Return("", storage.ErrNotFound) + m.counter.EXPECT().Next(gomock.Any(), counterDomainRequest).Return(int64(1), nil) + m.uriStore.EXPECT().Create(gomock.Any(), testURI, "1").Return(nil) + m.reqStore.EXPECT().Get(gomock.Any(), "1").Return(entity.Request{}, storage.ErrNotFound) + m.reqStore.EXPECT().Create(gomock.Any(), acceptedRequest("1")).Return(nil) + expectMaterializeAccepted(m, "1") + 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", + }, + { + name: "retry repairs accepted decimal request behind legacy pointer", + queue: testQueue, + setup: func(m ingestMocks) { + expectResolve(m) + m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testURI).Return("1", nil) + m.reqStore.EXPECT().Get(gomock.Any(), "1").Return(acceptedRequest("1"), nil) + expectMaterializeAccepted(m, "1") + 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", + }, { name: "dedup with existing accepted request republishes without minting", queue: testQueue, diff --git a/stovepipe/controller/record/record_test.go b/stovepipe/controller/record/record_test.go index 871cdeda..b12b1e37 100644 --- a/stovepipe/controller/record/record_test.go +++ b/stovepipe/controller/record/record_test.go @@ -291,6 +291,11 @@ 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 { diff --git a/stovepipe/entity/request_id.go b/stovepipe/entity/request_id.go index def123da..023cf0b1 100644 --- a/stovepipe/entity/request_id.go +++ b/stovepipe/entity/request_id.go @@ -17,8 +17,10 @@ package entity 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. -// IDs are canonical decimal strings whose scope is carried separately. +// Decimal IDs are newer than all legacy request// IDs. +// Within each format, ordering is by numeric counter. func CompareRequestID(a, b string) (int, error) { return id.Compare(a, b) }