Skip to content

Commit cd41d0c

Browse files
authored
test(e2e): prove a dependent batch is woken by the merge ahead of it (#576)
## Summary ### Why? The parent change fixes a dropped wake-up: a merged batch fans out to speculate so its dependents can re-plan, and that message used to reuse the bare batch ID the batch controller had already published to the same topic and partition at creation. The queue deduplicates against rows it has not collected yet, consumed ones included, so the fan-out was reported as a success, stored nothing, and never arrived. That fix shipped with unit coverage on the message ID and integration coverage on the queue semantics, but nothing exercised the path the bug actually broke. It is also a path that hides easily: any other event re-plans the queue and moves the dependent along, so a naive two-request test passes with or without the fix. ### What? A new e2e case isolates the fan-out as the only possible wake-up, following the stop → observe → start shape `TestCancel_CaughtPreBatch_NeverLands` already uses: 1. Close the `runway-merge` gate for the queue before landing, so the lead batch cannot complete its merge. 2. Land the lead; wait for its merge to park, keyed by the lead's batch ID. 3. Land the dependent. Its batch serializes behind the lead's, which is in-flight (`Merging` is a dependency state). 4. Wait for the dependent to reach `speculated` — its speculative build has already passed, so its own build signals are finished and nothing else will wake it. 5. Open the gate. The lead merges and fans out. The dependent reaching `landed` is then attributable to the fan-out alone. Supporting changes: `e2e-chain-queue` is registered in `queues.yaml`. It is deliberately absent from the orchestrator's per-queue profiles so it falls through to the baseline profile and its `all` conflict analyzer, which serializes every new batch behind every in-flight one — that is what builds the chain. A new `awaitBatchID` harness helper resolves a request's batch ID from the operating store, since merge messages are keyed by batch rather than by the sqid a test holds. ## Test Plan - ✅ `bazel test //test/e2e/...` — 3 suites, including the new case (~33s) - ✅ `make lint-license`, `make lint-message-id`, `make lint-queue-shard` - **Confirmed the test fails against the unfixed code.** Reverting the parent's `mergesignal` message ID to the bare batch ID leaves the dependent stuck at `speculated` and the suite runs to Bazel's timeout (`TIMEOUT in 240.3s` with `--test_timeout=240`); with the fix it passes in 33s. A stalled pipeline surfaces as a test timeout rather than an assertion failure, which is how this harness reports non-convergence — `pollUntil` has no deadline of its own by design, so Bazel's timeout is the only one. ## Stack 1. #574 1. @ #576
1 parent e21ab03 commit cd41d0c

3 files changed

Lines changed: 107 additions & 0 deletions

File tree

service/submitqueue/gateway/server/queues.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,11 @@ queues:
77
- name: test-queue
88
- name: e2e-test-queue
99
- name: e2e-cancel-queue
10+
# Not listed in the orchestrator's per-queue profiles, so it falls through to
11+
# the baseline profile and its "all" conflict analyzer: every new batch
12+
# serializes behind every in-flight one. That is what lets e2e build a
13+
# dependency chain and exercise how a dependent is woken.
14+
- name: e2e-chain-queue
1015
# Routes to an analyzer that always errors (conflictfake.FailAlways) so e2e can
1116
# exercise the conflict-analysis error path. See newQueueRegistry in the
1217
# orchestrator example server.

test/e2e/submitqueue/harness_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,30 @@ func (s *E2EIntegrationSuite) assertStatusesNever(req request, banned ...entity.
183183
}
184184
}
185185

186+
// awaitBatchID polls the operating store until the request has been claimed by
187+
// a batch and returns that batch's ID.
188+
//
189+
// Messages about a batch are keyed by the batch ID, not the sqid the test
190+
// holds, so a test that wants to name one has to resolve it. Polling because
191+
// the claim happens asynchronously, several stages after Land returns.
192+
func (s *E2EIntegrationSuite) awaitBatchID(req request) string {
193+
t := s.T()
194+
store, err := s.appStorage.For(req.queue)
195+
require.NoError(t, err, "failed to resolve operating store for queue %s", req.queue)
196+
197+
var batchID string
198+
pollUntil(persistPollInterval, func() bool {
199+
associations, err := store.GetRequestBatchStore().GetByRequestID(s.ctx, req.sqid)
200+
if err != nil || len(associations) == 0 {
201+
return false
202+
}
203+
batchID = associations[0].BatchID
204+
return true
205+
})
206+
s.log.Logf("Request %s is carried by batch %s", req.sqid, batchID)
207+
return batchID
208+
}
209+
186210
// closeGate closes the consumer gate for the consumer group, scoped to one
187211
// partition (the queue name for pipeline topics). The gate must be closed
188212
// before the message that must be caught is published — that makes the stop

test/e2e/submitqueue/suite_test.go

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,84 @@ func (s *E2EIntegrationSuite) TestLand_HappyPath_ReachesLanded() {
276276
"operating store should show request %s in terminal state landed", req.sqid)
277277
}
278278

279+
// TestDependentBatch_IsWokenByTheMergeAhead proves that a batch waiting on
280+
// another is woken when that one merges — the edge CODEM-303 was silently
281+
// dropping.
282+
//
283+
// A merged batch fans out to speculate so its dependents can re-plan. That
284+
// message used to reuse the bare batch ID, which the batch controller had
285+
// already published to the same topic and partition when the batch was
286+
// created. The queue deduplicates against rows it has not collected yet,
287+
// consumed ones included, so the wake-up was reported as a success, stored
288+
// nothing, and never arrived.
289+
//
290+
// Ordinarily something else re-plans the queue soon enough to hide that. This
291+
// test removes every other source of a wake-up, as stop → observe → start:
292+
//
293+
// 1. Stop: close the gate for runway-merge on this queue, before landing, so
294+
// the lead batch cannot complete its merge.
295+
// 2. Land the lead. It runs to the merge hand-off and parks there.
296+
// 3. Land the dependent. The queue's analyzer serializes conservatively, so
297+
// its batch depends on the lead's, which is in-flight (Merging counts).
298+
// 4. Observe: wait for the dependent to reach "speculated" — its speculative
299+
// build has already passed, so its own build signals are finished. From
300+
// here the only thing that can advance it is the lead merging.
301+
// 5. Start: open the gate. The lead merges and fans out.
302+
//
303+
// The dependent reaching "landed" is therefore attributable to the fan-out
304+
// alone. Against the old code it stays at "speculated" and the suite runs to
305+
// Bazel's timeout, which is how the harness reports a pipeline that stalled.
306+
func (s *E2EIntegrationSuite) TestDependentBatch_IsWokenByTheMergeAhead() {
307+
t := s.T()
308+
309+
const queue = "e2e-chain-queue"
310+
const gateGroup = "runway-merge"
311+
gateTopic := runwaymq.TopicKeyMerge.String()
312+
313+
s.closeGate(gateGroup, queue, "e2e: hold the lead merge so the dependent finishes building first")
314+
// Reopen even if an assertion below fails, so teardown does not stop the
315+
// stack with a delivery still parked. Opening twice is a no-op.
316+
defer s.openGate(gateGroup, queue)
317+
318+
lead := s.land(queue, "github://github.example.com/uber/e2e-chain/pull/1/abcdef0123456789abcdef0123456789abcdef01")
319+
s.log.Logf("Landed lead request %s; awaiting its merge to park", lead.sqid)
320+
321+
// The merge request is keyed by batch, so name the batch to prove the
322+
// parked delivery is this request's merge and not some other.
323+
leadBatch := s.awaitBatchID(lead)
324+
parked := s.awaitParked(gateGroup, gateTopic, leadBatch)
325+
assert.Equal(t, queue, parked.PartitionKey, "merge request should be partitioned by queue")
326+
327+
// The lead is provably stopped mid-merge. A request landed now serializes
328+
// behind it.
329+
dependent := s.land(queue, "github://github.example.com/uber/e2e-chain/pull/2/1234567890abcdef1234567890abcdef12345678")
330+
dependentBatch := s.awaitBatchID(dependent)
331+
require.NotEqual(t, leadBatch, dependentBatch, "the two requests must be carried by different batches")
332+
333+
leadState, err := s.appStorage.For(queue)
334+
require.NoError(t, err)
335+
got, err := leadState.GetBatchStore().Get(s.ctx, dependentBatch)
336+
require.NoError(t, err, "failed to read the dependent batch")
337+
require.Contains(t, got.Dependencies, leadBatch,
338+
"batch %s must depend on the in-flight %s for this test to exercise anything", dependentBatch, leadBatch)
339+
340+
// Its speculative build passes while the lead is still parked, so by the
341+
// time the gate opens the dependent has no build signals left to wake it.
342+
s.awaitStatus(dependent, entity.RequestStatusSpeculated)
343+
s.log.Logf("Dependent %s is speculated and waiting only on %s", dependent.sqid, leadBatch)
344+
345+
// Start: the lead merges, and its fan-out is now the only thing that can
346+
// move the dependent.
347+
s.openGate(gateGroup, queue)
348+
s.awaitUnparked(gateGroup, gateTopic, leadBatch)
349+
350+
s.awaitStatus(lead, entity.RequestStatusLanded)
351+
s.awaitStatus(dependent, entity.RequestStatusLanded)
352+
353+
assert.Equal(t, entity.RequestStateLanded, s.terminalState(dependent),
354+
"the dependent must land once the batch it waited on merged")
355+
}
356+
279357
// TestReadAPIs validates all five request read endpoints against receipts
280358
// created through the public Land API.
281359
func (s *E2EIntegrationSuite) TestReadAPIs() {

0 commit comments

Comments
 (0)