fix(server): stop retrying permanent event sink rejections - #279
Merged
Conversation
A receiver that rejects the batch itself — a body it cannot parse, a credential it refuses, a route that holds no receiver — answers the same rejection to every identical retry. flushEventSink retried anyway, every two seconds, for the life of the process, and logged a warning on each pass: no record ever landed and the warning buried every other line an operator reads. The status is the only evidence available about whether a retry can succeed, so classification lives in the transport and the verdict travels as a sentinel. httpEventSink wraps server.ErrEventSinkPermanent for 400, 401, 403, 404, 409, 410, and 422. It leaves 408, 425, 429, 5xx, every unlisted status, and every transport failure retryable, because each of those is the receiver asking for the same batch later. flushEventSink now reports whether the pump may keep running. It detects the sentinel with errors.Is, logs one bounded warning that carries the scanned range, the status, and the receiver's sanitized machine code, and returns false. runEventSink returns on false, so the pump goroutine exits and sinkDone closes. Harness itself keeps running: sessions run, records still journal and still reach /event, and Drain still returns. Nothing closes and nothing crashes. The diagnostic path is unchanged — the error still carries the status and the bounded code only, never the receiver's free text and never the configured URL. Verification: TestEventSinkPermanentRejectionStopsThePumpWithoutStopping Harness drives the real pump in a testing/synctest bubble, moves the fake clock past two retry windows, and pins one delivery, one warning, a retired pump, and a server that still journals and still drains. TestEventSinkRetryableFailureIsNotPermanent pins the retry gap at exactly eventSinkRetryDelay on the same fake clock. The cmd/harness table covers every listed status plus 402 and 418 for the unlisted case, and separate tests pin the bounded diagnostic on a 403 and a refused connection as retryable. Red-verified: before the change the pump test reported "the pump is still running after a permanent rejection" and all seven permanent statuses reported errors.Is = false. go test -race ./..., go vet ./..., go build ./..., and gofmt all pass.
There was a problem hiding this comment.
🟡 Changes recommended
cmd/harness/eventsink.go constructs a permanent error using fmt.Errorf with two %w verbs, which is invalid and will panic at runtime on permanent rejections.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR prevents the server’s durable event-sink pump from retrying forever when the receiver returns a permanent HTTP rejection (e.g., 409 for a stale journal generation), while keeping Harness sessions and journaling healthy.
Changes:
- Introduces
server.ErrEventSinkPermanentand updatesflushEventSink/runEventSinkto retire the pump on permanent rejections (with a single bounded warning). - Adds synctest-backed coverage ensuring permanent failures stop the pump without breaking journaling/Drain, and retryable failures keep the exact retry delay.
- Updates the HTTP event sink to classify specific HTTP status codes as permanent vs retryable and wraps permanent rejections with the sentinel.
File summaries
| File | Description |
|---|---|
server/eventsink.go |
Adds the permanent-rejection sentinel and retires the pump when errors.Is(err, ErrEventSinkPermanent) is true. |
server/eventsink_test.go |
Adds synctest coverage for permanent-stop vs retry behavior and single-warning logging. |
cmd/harness/eventsink.go |
Implements status-based permanence classification and wraps permanent receiver errors with the sentinel. |
cmd/harness/eventsink_test.go |
Adds tests asserting status classification, bounded diagnostics, and transport failures staying retryable. |
docs/design/event-sink.md |
Updates the design doc to describe permanent rejection semantics and the status classification table. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review of #279 asked for a single-wrap error form on the permanent rejection path. The two-verb form built a multi-error whose Unwrap answers nil, so a caller that reaches for the one cause behind the message found nothing, and the shape of the error depended on a formatting detail rather than on the contract the pump matches against. Deliver now formats the receiver text with %v and wraps the sentinel alone. errors.Is keeps matching ErrEventSinkPermanent, errors.Unwrap now answers the sentinel itself, and the message is unchanged: the status and the bounded receiver code first, the sentinel appended once. Verification: TestHTTPEventSinkPermanentRejectionKeepsABoundedDiagnostic now pins the exact message text and asserts errors.Unwrap returns the sentinel; red-verified against the previous two-verb form, which failed with "errors.Unwrap(err) = <nil>". The status table pins the exact message for every listed status, permanent and retryable. go test -race ./..., go vet ./..., go build ./..., and gofmt all pass. Updates #279
There was a problem hiding this comment.
🟢 Approval recommended
The change is well-scoped, preserves existing retry semantics for transient failures, adds explicit classification for permanent rejections, and is backed by targeted synctest coverage and updated design documentation.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The sink retries every receiver error every two seconds. A stale journal generation receives a permanent 409, so a fenced old pod would post the same rejected batch indefinitely and compete with the current writer for admission capacity.
Change
Add a transport sentinel for permanent receiver rejections. HTTP 400, 401, 403, 404, 409, 410, and 422 retire only the sink pump; Harness sessions and journaling remain healthy. HTTP 408, 425, 429, 5xx, unlisted statuses, and transport failures retain the existing retry behavior.
Verification
go test -race ./...go vet ./...go build ./...