Skip to content

fix(stovepipe): stop wrapping the buildsignal re-poll publish as retryable - #469

Open
behinddwalls wants to merge 1 commit into
preetam/stovepipe-record-request-idfrom
preetam/stovepipe-publish-classification
Open

fix(stovepipe): stop wrapping the buildsignal re-poll publish as retryable#469
behinddwalls wants to merge 1 commit into
preetam/stovepipe-record-request-idfrom
preetam/stovepipe-publish-classification

Conversation

@behinddwalls

@behinddwalls behinddwalls commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Why?

buildsignal wrapped a failed reschedule publish in errs.NewRetryableError. platform/errs rule 4 says not to do that: a failed queue publish must not be made retryable just because replaying it is convenient, because that turns a permanent enqueue failure into an infinite retry instead of dead-lettering. It was also inconsistent with the sibling path — the publish to record, one branch above, already returns raw.

What?

Returns the raw error, leaving the classifier's verdict to stand, and drops the now-unused errs dependency.

Merge ordering

This should merge only after the build and buildsignal stages have DLQ consumers. Today only stovepipe-process-dlq is registered, and MoveToDLQ routes by topic-suffix convention, so anything dead-lettered from buildsignal lands on buildsignal_dlq and is never drained — leaving the request non-terminal with its build slot held, which is the wedge the reconciler exists to prevent.

This change does not create that hole; every other non-retryable failure in the stage already falls into it. But it does add one more path in, so the ordering is worth respecting rather than discovering later. If the DLQ work slips, the alternative is to land this anyway and accept parity with the existing failure modes.

Test Plan

bazel test //stovepipe/... — the reschedule-publish-failure case now asserts a non-retryable classification.

bazel test //test/e2e/stovepipe/... — the poll loop is unaffected on the success path.

Stack

  1. test(stovepipe): add build-slow marker to the fake build runner #464
  2. fix(stovepipe): mint a distinct message id for each buildsignal re-poll #465
  3. refactor(stovepipe): replace recorded greenness states with build outcomes #466
  4. feat(stovepipe): record the build outcome on the request and free its slot #467
  5. feat(stovepipe)!: key the record stage on the request id #468
  6. @ fix(stovepipe): stop wrapping the buildsignal re-poll publish as retryable #469

…yable

## Summary

### Why?

`buildsignal` wrapped a failed reschedule publish in `errs.NewRetryableError`. `platform/errs` rule 4 says not to do that: a failed queue publish must not be made retryable just because replaying it is convenient, because that turns a permanent enqueue failure into an infinite retry instead of dead-lettering. It was also inconsistent with the sibling path — the publish to `record`, one branch above, already returns raw.

### What?

Returns the raw error, leaving the classifier's verdict to stand, and drops the now-unused `errs` dependency.

### Merge ordering

**This should merge only after the build and buildsignal stages have DLQ consumers.** Today only `stovepipe-process-dlq` is registered, and `MoveToDLQ` routes by topic-suffix convention, so anything dead-lettered from `buildsignal` lands on `buildsignal_dlq` and is never drained — leaving the request non-terminal with its build slot held, which is the wedge the reconciler exists to prevent.

This change does not create that hole; every other non-retryable failure in the stage already falls into it. But it does add one more path in, so the ordering is worth respecting rather than discovering later. If the DLQ work slips, the alternative is to land this anyway and accept parity with the existing failure modes.

## Test Plan

✅ `bazel test //stovepipe/...` — the reschedule-publish-failure case now asserts a non-retryable classification.

✅ `bazel test //test/e2e/stovepipe/...` — the poll loop is unaffected on the success path.
@behinddwalls
behinddwalls force-pushed the preetam/stovepipe-publish-classification branch from 2ae86d2 to 71366e3 Compare July 30, 2026 07:51
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.

2 participants