Land and landsignal DLQs fail a `landing` batch that Runway goes on to merge

オープン
  • 何か問題がありますか？
#819 コメント 1 件 リアクション 0 件 担当者 0 名 GitHub で見る

メンテナーはふだん 2 日以内に返信

まだ誰も着手していません。

評価

難易度
5/5
見積もり時間
1週間以上
初心者へのやさしさ
35/100
issue の種類
バグ
明瞭さ
おおむね明確
活発さ
活発
技術スタック
go
領域
distributed-systems

調査の方向性

Start with submitqueue/orchestrator/controller/dlq/dlq.go and the interleaving described in the issue; then read spec/submitqueue/landoutcome/LandOutcome.tla and compare the design options. Add tests that interleave a DLQ with a late SUCCEEDED and confirm they fail before the fix. Done means the chosen design updates the model and meets all four acceptance criteria without allowing an incorrect terminal outcome.

索引モデルが issue の本文から書いたものです。

説明

bug

Problem

A batch is reported failed, and its requests error, even though Runway merged the change onto the target branch.

Cause

failBatch (submitqueue/orchestrator/controller/dlq/dlq.go) moves any non-terminal batch to failed, landing included. A landing batch has already handed its outcome to Runway, so the reconciler cannot know whether the merge happened. Two DLQs reach failBatch while the batch is landing:

  • submitqueue-land DLQ. The Runway publish is land's last step, so a lost ack, or an ambiguous publish error after the row was written, leaves Runway holding the MergeRequest. If later attempts then fail on a storage error, the message dead-letters.
  • landsignal DLQ. landsignal holds Runway's SUCCEEDED, but the batch compare-and-swap keeps failing on a storage fault. The DLQ payload contains the verdict, and the reconciler discards it.

When Runway's answer arrives later, landsignal sees a terminal batch, re-fans out, and acks. Nothing corrects the outcome.

Interleaving (TLC's shortest counterexample)

speculate:   batch speculating → landing, publish to submitqueue-land
land:        publish MergeRequest to runway-merge, then the attempt fails; retries exhaust → land DLQ
runway:      merges, publishes SUCCEEDED
land DLQ:    failBatch: landing → failed, fan out requests → error
landsignal:  batch already terminal → ack, outcome unchanged

Why the obvious fixes are not enough

Found by model checking spec/submitqueue/landoutcome/LandOutcome.tla, which checks 24 design combinations:

  • Skip landing in the DLQ and wait for Runway. This trades the wrong answer for a batch that never finishes, when Runway's answer is itself the message that dead-lettered.
  • Re-send the MergeRequest from the DLQ. This is unsound against today's Runway, for two reasons:
    • A stale FAILED can win. Runway's first attempt dead-letters without pushing, so its DLQ honestly answers FAILED. The re-send then merges on a second attempt, and the earlier FAILED is applied.
    • The replay is swallowed. A replayed answer reuses the request ID as its message ID and is deduped by the queue (the #352 class).
  • The only design that passes every property has four parts: the DLQ re-sends; Runway's DLQ checks the branch; Runway records its verdict per request ID; and answers do not dedup on the request ID alone. That gives Runway state, so cheaper designs are worth modelling first. Two candidates: the landsignal DLQ applies the verdict it holds, and land records "dispatched" before it publishes.

Acceptance criteria

  • A dead letter on submitqueue-land or landsignal never moves a landing batch to failed while the change can still land or has landed.
  • Every landing batch still reaches a terminal state.
  • Unit tests that interleave the DLQ with a late SUCCEEDED, confirmed to fail before the fix.
  • LandOutcome.tla updated to the chosen design.

Related: #818 (the same symptom through the speculate path).

主要言語
Go
スター
247
フォーク
16
平均マージ
2日 1時間
マージ済み PR（30日）
125

環境構築

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

uber/submitqueue のほかの issue

uber/submitqueue の issue をすべて見る

似ている issue

Go の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。