Skip to content

fix(submitqueue): keep internal merge stage topic distinct from runway merge queue#348

Open
sbalabanov wants to merge 1 commit into
mainfrom
fix-merge-topic-collision
Open

fix(submitqueue): keep internal merge stage topic distinct from runway merge queue#348
sbalabanov wants to merge 1 commit into
mainfrom
fix-merge-topic-collision

Conversation

@sbalabanov

Copy link
Copy Markdown
Contributor

Summary

The orchestrator registers its internal merge pipeline stage and the runway-owned merge queue in one topic registry over one shared queue backend, and both used the topic key "merge" (topickey.TopicKeyMerge == runwaymq.TopicKeyMerge). The publish-only runway entry silently shadowed the internal stage entry (last write wins on the registry maps), collapsing the two topics into one:

  • runway consumed the internal batch-ID messages as zero-step MergeRequests and reported SUCCEEDED without ever seeing the real steps;
  • the orchestrator merge controller's real MergeRequest publish was then silently dropped by the queue's (topic, partition_key, id) dedupe, because it reuses the batch ID already published to the same topic.

The pipeline still reached landed only because the zero-step result correlates by batch ID — with a real merger, nothing would ever have been merged. Found by the new e2e harness (stacked PR) asserting that runway's merge signal carries the request's step IDs.

Changes

  • submitqueue/core/topickey: rename the internal merge stage key (and its orchestrator wire name) to mergebatch, with a comment explaining why it must not be "merge".
  • platform/consumer: NewTopicRegistry now rejects duplicate topic keys, so a future collision fails at startup instead of silently misrouting messages.
  • service/submitqueue/orchestrator: register the internal stage under the new wire name.

Test plan

  • bazel test //... --test_tag_filters=-manual,-integration — 75/75 pass (includes the updated dlq controller test).
  • bazel test //test/integration/... — 8/8 pass.
  • e2e suite (with the stacked harness PR): runway's merge signal now carries the request step IDs and the full land pipeline completes in ~2s.

🤖 Generated with Claude Code

…y merge queue

The orchestrator registers its internal merge pipeline stage and the
runway-owned merge queue in one topic registry over one shared queue
backend, and both used the topic key "merge" (topickey.TopicKeyMerge ==
runwaymq.TopicKeyMerge). The publish-only runway entry silently shadowed
the internal stage entry (last write wins), collapsing the two topics into
one:

- runway consumed the internal batch-ID messages as zero-step
  MergeRequests and reported SUCCEEDED without ever seeing the real steps;
- the orchestrator merge controller's real MergeRequest publish was then
  silently dropped by the queue's (topic, partition_key, id) dedupe,
  because it reuses the batch ID already published to the same topic.

The pipeline still reached "landed" only because the zero-step result
correlates by batch ID — with a real merger nothing would ever have been
merged. Found by the e2e harness asserting runway's merge signal carries
the request's step IDs.

Rename the internal stage key and wire name to "mergebatch", and make
consumer.NewTopicRegistry reject duplicate topic keys so a future
collision fails at startup instead of silently misrouting messages.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

// PublishAfter when the build has not yet reached a terminal state.
TopicKeyBuildSignal TopicKey = "buildsignal"
// TopicKeyMerge is the pipeline stage where speculated batches are published for merging.
TopicKeyMerge TopicKey = "merge"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we namespace these to say runway-merge vs just merge and same from SQ stuff to be submitqueue-merge in keys?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants