fix(smr): surface upload failures instead of silently succeeding#85
Open
mellox wants to merge 1 commit into
Open
fix(smr): surface upload failures instead of silently succeeding#85mellox wants to merge 1 commit into
mellox wants to merge 1 commit into
Conversation
`ficsit smr upload` had several paths that swallowed errors and reported success, so a failed upload could publish a broken/incomplete version while the command still exited 0. - Chunk upload discarded the HTTP response entirely: status code, GraphQL `errors`, and the `uploadVersionPart` result were all unchecked, and the body was never closed. A failed chunk was silently ignored and finalize proceeded. Extracted `uploadChunk`/`sendChunkOnce`, which now check HTTP status, the GraphQL `errors` array, and the `uploadVersionPart` boolean, and close the body. - Added per-chunk retry for transient failures (transport errors such as a dropped/corrupted TLS record, and 5xx) with exponential backoff via avast/retry-go (already a dependency). 4xx, GraphQL errors, and an explicit `uploadVersionPart: false` are treated as permanent and not retried. - Chunk read used a bare `f.Read` (not guaranteed to fill the buffer, so it could ship a zero-padded/corrupt chunk) and reopened + leaked the file each iteration; now uses `io.ReadFull` and opens the file once with a single defer. - Validate the upload file is a real `.smod`/`.zip` archive before creating a version (previously a TODO). - The post-finalize state check returned `nil` on error, so the command exited 0 even when the server rejected the version; it now returns the error. Adds unit tests for the chunk error/response handling, the retry path, the short-read fix, and archive validation.
There was a problem hiding this comment.
Pull request overview
This PR hardens ficsit smr upload so chunk upload and finalize failures are surfaced (non-zero exit) instead of being silently ignored, reducing the risk of publishing incomplete/corrupt versions.
Changes:
- Refactors chunk uploading into helpers that validate HTTP status + GraphQL
errors+uploadVersionPartresult and always close response bodies. - Adds per-chunk retries with exponential backoff for transient transport/5xx failures.
- Fixes chunk reading (no silent short reads), validates upload archives as real
.smod/.zip, and adds unit test coverage for these paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/smr/upload.go | Adds validated chunk upload + retry/backoff, fail-closed response handling, robust chunk reads, archive validation, and proper error surfacing on finalize state-check. |
| cmd/smr/upload_test.go | Adds unit tests covering chunk upload error surfacing, retry behavior, multipart parity, short-read handling, and archive validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bufferSize = stat.Size() - offset | ||
| } | ||
|
|
||
| chunk := make([]byte, bufferSize) |
Comment on lines
+81
to
+83
| if err := validateSmodFile(filePath); err != nil { | ||
| return err | ||
| } |
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
ficsit smr uploadhad several paths that swallowed errors and reported success, so a failed upload could publish a broken or incomplete version while the command still exited0.Changes
*http.Responseentirely — HTTP status, the GraphQLerrorsarray, and theuploadVersionPartresult were all unchecked, and the body was never closed. A failed chunk (bad token,413, a GraphQL error) was silently ignored andfinalizeproceeded on a broken upload. ExtracteduploadChunk/sendChunkOnce, which now check the HTTP status, the GraphQLerrorsarray, and theuploadVersionPartboolean, and always close the body.5xxresponses are now retried with exponential backoff viaavast/retry-go(already a dependency). Permanent failures —4xx, a GraphQLerrorsarray, or an explicituploadVersionPart: false— are markedretry.Unrecoverableand not retried.f.Read, which is not guaranteed to fill the buffer and could ship a zero-padded (corrupt) chunk; the file was also opened once per chunk and never closed. Now usesio.ReadFulland opens the file once with a singledefer Close..smod/.ziparchive before creating a version (replaces the existingTODO).nilon error, so the command exited0even when the server rejected the version. It now returns the error.Testing
cmd/smr/upload_test.go) cover: HTTP error status surfacing, a GraphQLerrorsarray surfacing, explicit success, a missing success field (fail-closed), explicitfalserejection, a wire-level multipart parity check, the retry path (transient5xxretried then succeeds), the short-read fix, and archive validation (accept/reject by extension and by zip validity).tls: bad record MACand a connection reset on individual chunks) and completed all chunks — where the previous code would have silently dropped a chunk and finalized a corrupt version. The finalize state-check change also surfaced a real server-side finalize error that the old code swallowed as a successful (exit 0) run.Notes
avast/retry-gowas already listed ingo.mod; no new dependency added.go build,go vet,gofmt, andgo test ./...are clean.