Repository navigation
Conversation
NewWriteResponse defaults the status code to 204, but the write handler only fell back to 500 when the status code was 0. A Store implementation returning an error together with a default (or nil) WriteResponse thus made the handler answer with "204 No Content", so the sender treated the batch as accepted and dropped it. Replace any status code below 400 with 500 when Store returns an error. Also default a zero status code to 204 on success, which previously made WriteHeader(0) panic for a zero value WriteResponse. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Manuel Rüger <manuel@rueg.eu>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation matches the stated behavior and includes focused regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Prevents failed remote writes from returning successful HTTP statuses, avoiding silent data loss.
Changes:
- Converts sub-400 statuses to 500 when storage fails.
- Defaults zero status to 204 on success.
- Adds table-driven regression coverage.
| File | Description |
|---|---|
exp/api/remote/remote_headers.go |
Documents status fallback behavior. |
exp/api/remote/remote_api.go |
Correctly normalizes success and error statuses. |
exp/api/remote/remote_api_test.go |
Tests default, nil, zero, and explicit statuses. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Summary
NewWriteResponse()defaults the status code to 204, but the write handler only fell back to 500 when the status code was 0. AStoreimplementation that returned an error together with a default (or nil)WriteResponsetherefore made the handler reply204 No Contentwith the error message as the body. The sender treats that as success and drops the batch, so data is lost silently.This change:
Storereturns an error. Explicit 4xx/5xx codes are kept.&WriteResponse{}madeWriteHeader(0)panic.SetStatusCodedoc comment accordingly.Behaviour change to note: a
Storereturning an error together with an explicit 2xx status now results in a 500. An error fromStoreshould never be reported as success to the sender.Testing
Added the table-driven
TestWriteHandler_StatusCode, covering default, nil, zero value and explicit status responses with and without a storage error. Before the fix it fails (204 on errors, panic on the zero value response); with the fix it passes, as doesgo test -race ./api/remote/inexp/.Note: #2153 touches
writeHeadersand the same test file; any conflict should be trivial.🤖 Generated with Claude Code