Skip to content

Commit 7a44443

Browse files
committed
test(speculate): cover speculation across an unresolved dependency
## Summary ### Why? Nothing in `test/integration/` touches the speculate pipeline — the orchestrator integration suite is `TestPingAPI` and nothing else — so the only end-to-end coverage was the happy path, which has no dependencies and therefore never speculates across one. The events this stack introduces had unit coverage only. ### What? `e2e-respeculate-queue` is registered in the gateway's queue list. It takes no profile of its own: falling through to the baseline is what gives it the `all` analyzer, which serializes the queue so a second request becomes a batch depending on the first. The new e2e test forces the wait rather than racing it. Batch IDs come from a per-queue counter as `<queue>/batch/<n>`, so on a fresh queue the leader is `batch/1`, and the build topic partitions by batch — closing the consumer gate on that partition before anything is published holds the leader's build and nothing else. The follower then reaches a passed path while its dependency is still outstanding, reports `waiting`, and is asserted to still be `speculating`. Releasing the gate fails the leader, and the follower re-plans and lands with `speculating` and `speculated` recorded exactly once each. Two harness helpers come with it: `awaitEvent`, since an event is never a current status and the history is its only witness, and `assertStatusCount`, which is what pins the no-oscillation property the status change is for. A unit test covers the case e2e cannot reach deterministically: a dependency turning terminal in the same run that walks the head resting on it, so the break is seen by a later generation of the finalize loop rather than by the read. `invalidated` is deliberately not asserted end to end. A passed path stops occupying build budget, so by the time the leader fails the follower has usually funded the other side of the guess as well; it never loses its last live passed path, which is the state `invalidated` reports. Forcing that end to end would mean starving the queue's budget, which cannot be done without also starving the follower's first build. ## Test Plan ✅ `bazel test //submitqueue/... //platform/... //service/...` — 73 tests pass ✅ `bazel test //test/e2e/...` — 3/3 pass, including the new scenario # Conflicts: # service/submitqueue/gateway/server/queues.yaml # test/e2e/submitqueue/harness_test.go # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # interactive rebase in progress; onto bcea46e # Last commands done (2 commands done): # pick e407612 # feat(speculate): hold speculating until the batch can be sent to merge # pick 14839e5 # test(speculate): cover speculation across an unresolved dependency # No commands remaining. # You are currently rebasing branch 'preetam/codem-443-speculation-events' on 'bcea46ec'. # # Changes to be committed: # modified: service/submitqueue/gateway/server/queues.yaml # modified: submitqueue/orchestrator/controller/speculate/run_test.go # modified: test/e2e/submitqueue/harness_test.go # modified: test/e2e/submitqueue/suite_test.go # # Conflicts: # service/submitqueue/gateway/server/queues.yaml # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # interactive rebase in progress; onto 42d1cb7 # Last commands done (2 commands done): # pick cfaa178 # feat(speculate): hold speculating until the batch can be sent to merge # pick 0f79d0b # test(speculate): cover speculation across an unresolved dependency # No commands remaining. # You are currently rebasing branch 'preetam/codem-443-speculation-events' on '42d1cb72'. # # Changes to be committed: # modified: service/submitqueue/gateway/server/queues.yaml # modified: submitqueue/orchestrator/controller/speculate/run_test.go # modified: test/e2e/submitqueue/harness_test.go # modified: test/e2e/submitqueue/suite_test.go #
1 parent 5732f0f commit 7a44443

4 files changed

Lines changed: 148 additions & 0 deletions

File tree

service/submitqueue/gateway/server/queues.yaml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,3 +23,7 @@ queues:
2323
# pipeline runs against a real repository. See
2424
# service/submitqueue/demo/provider and doc/howto/PROVIDER-E2E.md.
2525
- name: demo-queue
26+
# Inherits the baseline "all" analyzer, which serializes the queue, so a
27+
# second request lands as a batch depending on the first. e2e uses that to
28+
# exercise speculation across an unresolved dependency.
29+
- name: e2e-respeculate-queue

submitqueue/orchestrator/controller/speculate/run_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1469,6 +1469,54 @@ func TestRun_ReportsInvalidatedWhenPassedPathBreaks(t *testing.T) {
14691469
assert.Equal(t, entry.ID, h.logs[0].Metadata["path_id"])
14701470
}
14711471

1472+
// The e2e shape: the dependency turns terminal in the same run that walks the
1473+
// head resting on it, so the break is seen by a later generation of the loop
1474+
// rather than by the read.
1475+
func TestRun_ReportsInvalidatedWhenTheDependencyFailsInTheSameRun(t *testing.T) {
1476+
ctrl := gomock.NewController(t)
1477+
1478+
leader := entity.Batch{ID: dep1, Queue: "q", State: entity.BatchStateSpeculating, Version: 1}
1479+
followerBatch := entity.Batch{
1480+
ID: head, Queue: "q", Contains: []string{"q/1"},
1481+
State: entity.BatchStateSpeculating, Dependencies: []string{dep1}, Version: 1,
1482+
}
1483+
1484+
h := newRunHarness(t, ctrl, &scriptedSpeculator{}, []entity.Batch{leader, followerBatch})
1485+
h.noBuildsDispatched()
1486+
1487+
// The leader has nothing left that can pass, so this run fails it.
1488+
h.pathSets.EXPECT().Get(gomock.Any(), dep1).Return(entity.SpeculationPathSet{
1489+
Head: dep1,
1490+
Paths: []entity.SpeculationPathEntry{entryFor(entity.SpeculationPath{Head: dep1}, entity.SpeculationPathStatusFailed)},
1491+
Version: 1,
1492+
}, nil).AnyTimes()
1493+
h.batches.EXPECT().
1494+
Update(gomock.Any(), updateTo{id: dep1, state: entity.BatchStateFailed}, int32(1), int32(2)).Return(nil)
1495+
1496+
// The follower passed on the guess that the leader would succeed.
1497+
passed := entity.SpeculationPath{
1498+
Head: head,
1499+
Dependencies: []entity.PathDependency{{Batch: dep1, Assumption: entity.DependencyAssumptionSucceeds}},
1500+
}
1501+
entry := entryFor(passed, entity.SpeculationPathStatusPassed)
1502+
h.pathSets.EXPECT().Get(gomock.Any(), head).Return(entity.SpeculationPathSet{
1503+
Head: head,
1504+
Paths: []entity.SpeculationPathEntry{entry},
1505+
Version: 1,
1506+
}, nil).AnyTimes()
1507+
h.pathSets.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
1508+
1509+
require.NoError(t, h.run(dep1))
1510+
1511+
var got []entity.RequestEvent
1512+
for _, entry := range h.logs {
1513+
if entry.Type == entity.RequestLogTypeEvent {
1514+
got = append(got, entry.Event)
1515+
}
1516+
}
1517+
assert.Contains(t, got, entity.RequestEventInvalidated)
1518+
}
1519+
14721520
// A merge is decided on the same live passed path a wait would be reported
14731521
// from, so without the gate every landed request would carry a wait it never
14741522
// had.

test/e2e/submitqueue/harness_test.go

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,46 @@ func (s *E2EIntegrationSuite) awaitBatchID(req request) string {
207207
return batchID
208208
}
209209

210+
// mustStatus reads the current status and fails the test if it is unreadable.
211+
func (s *E2EIntegrationSuite) mustStatus(req request) entity.RequestStatus {
212+
t := s.T()
213+
got, err := s.currentStatus(req)
214+
require.NoError(t, err, "GetRequestSummaryByID failed for %s", req.sqid)
215+
return got
216+
}
217+
218+
// awaitEvent polls GetRequestHistoryByID until want appears in the request's
219+
// event timeline. Unlike a status, an event is never the current position, so
220+
// there is nothing to poll on the summary — the history is the only witness.
221+
func (s *E2EIntegrationSuite) awaitEvent(req request, want entity.RequestEvent) {
222+
pollUntil(persistPollInterval, func() bool {
223+
got := s.eventTimeline(req)
224+
s.log.Logf("events(%s) = %v (want %q)", req.sqid, got, want)
225+
for _, e := range got {
226+
if e == want {
227+
return true
228+
}
229+
}
230+
return false
231+
})
232+
}
233+
234+
// assertStatusCount asserts how many times a status appears in the timeline.
235+
// A status that recurs is not merely noisy: the client renders each entry as a
236+
// fresh step, so a stage revisited reads as the pipeline going backwards.
237+
func (s *E2EIntegrationSuite) assertStatusCount(req request, status entity.RequestStatus, want int) {
238+
t := s.T()
239+
got := s.timeline(req)
240+
seen := 0
241+
for _, st := range got {
242+
if st == status {
243+
seen++
244+
}
245+
}
246+
assert.Equalf(t, want, seen,
247+
"GetRequestHistoryByID for %s should record %q %d time(s); got %v", req.sqid, status, want, got)
248+
}
249+
210250
// closeGate closes the consumer gate for the consumer group, scoped to one
211251
// partition (the queue name for pipeline topics). The gate must be closed
212252
// before the message that must be caught is published — that makes the stop

test/e2e/submitqueue/suite_test.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,62 @@ func (s *E2EIntegrationSuite) TestReadAPIs() {
432432
assert.Equal(t, secondSummary.Request.LastError, secondEvents[len(secondEvents)-1].LastError)
433433
}
434434

435+
// TestLand_DependentBatch_StaysSpeculatingAcrossAnUnresolvedDependency covers
436+
// the oscillation the request log used to report as a regression: a head
437+
// speculates while a dependency is unresolved, the dependency then fails, and
438+
// the head re-plans and lands anyway. Throughout, it is speculating exactly
439+
// once — the trail must never revisit a stage.
440+
//
441+
// The wait is forced rather than raced. Batch IDs come from a per-queue counter
442+
// as "<queue>/batch/<n>", so the leader on a fresh queue is batch/1, and the
443+
// build topic partitions by batch — closing the gate on that partition before
444+
// anything is published holds the leader's build and nothing else, so the
445+
// follower reaches a passed path while its dependency is still outstanding.
446+
//
447+
// No invalidated event is asserted. A passed path stops occupying build budget,
448+
// so by the time the leader fails the follower has usually funded the other side
449+
// of the guess too; it never loses its last live passed path, which is what
450+
// invalidated reports. The unit tests cover that state directly.
451+
func (s *E2EIntegrationSuite) TestLand_DependentBatch_StaysSpeculatingAcrossAnUnresolvedDependency() {
452+
const queue = "e2e-respeculate-queue"
453+
const gateGroup = "orchestrator"
454+
leaderBatch := queue + "/batch/1"
455+
456+
s.closeGate(gateGroup, leaderBatch, "e2e: hold the leader's build so its dependent speculates first")
457+
defer s.openGate(gateGroup, leaderBatch)
458+
459+
leader := s.land(queue, "github://github.example.com/uber/e2e-respeculate/pull/1/1111111111111111111111111111111111111111?sq-fake=build-fail")
460+
follower := s.land(queue, "github://github.example.com/uber/e2e-respeculate/pull/2/2222222222222222222222222222222222222222")
461+
s.log.Logf("Landed leader=%s (build held) follower=%s", leader.sqid, follower.sqid)
462+
463+
// The baseline analyzer serializes the queue, so the follower depends on the
464+
// leader and speculates on it succeeding. That build passes while the leader
465+
// is still held: the follower's own work is done and only the leader is
466+
// outstanding, which is the wait.
467+
s.awaitEvent(follower, entity.RequestEventWaiting)
468+
assert.Equal(s.T(), entity.RequestStatusSpeculating, s.mustStatus(follower),
469+
"a head waiting on its dependency has not finished speculating")
470+
471+
// Release the leader. Its build fails, contradicting the guess the follower
472+
// speculated on, and the follower has to reach the trunk another way.
473+
s.openGate(gateGroup, leaderBatch)
474+
assert.Equal(s.T(), entity.RequestStatusError, s.awaitTerminal(leader),
475+
"the leader's build carries a failure marker, so it must not land")
476+
477+
s.awaitStatus(follower, entity.RequestStatusLanded)
478+
s.assertStatusesInOrder(follower,
479+
entity.RequestStatusSpeculating,
480+
entity.RequestStatusSpeculated,
481+
entity.RequestStatusLanding,
482+
entity.RequestStatusLanded,
483+
)
484+
485+
// The point of the exercise: one trip through speculation, however many
486+
// guesses it took. A second entry renders as the pipeline going backwards.
487+
s.assertStatusCount(follower, entity.RequestStatusSpeculating, 1)
488+
s.assertStatusCount(follower, entity.RequestStatusSpeculated, 1)
489+
}
490+
435491
// TestCancelRequest_InvalidSqid verifies the gateway rejects an empty sqid
436492
// synchronously before publishing anything to the cancel queue.
437493
func (s *E2EIntegrationSuite) TestCancelRequest_InvalidSqid() {

0 commit comments

Comments
 (0)