From 1af351a70295201078121af8c31fb3241f3b5107 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Tue, 4 Aug 2026 22:42:56 +0800 Subject: [PATCH] fix(storage): preserve ReadParts errors across keepalive responses ReadPartsHandler completed its keepalive stream before reporting storage failures, then tried to write an ordinary error response after the body was already owned. Clients consequently decoded the error text as msgpack and lost the real failure. Send failures through the keepalive completion channel and mark success only after ReadParts returns cleanly, preserving the existing wire framing and error identity. Co-authored-by: ChatGPT Co-authored-by: Claude --- cmd/storage-rest-server-guard.go | 6 ++--- cmd/storage-rest-server-guard_test.go | 34 ++++++++++++++++++++------- cmd/storage-rest-server.go | 7 ++++-- cmd/storage-rest-traversal_test.go | 14 +++++++---- 4 files changed, 42 insertions(+), 19 deletions(-) diff --git a/cmd/storage-rest-server-guard.go b/cmd/storage-rest-server-guard.go index 99efcb2fd..c3169b326 100644 --- a/cmd/storage-rest-server-guard.go +++ b/cmd/storage-rest-server-guard.go @@ -83,9 +83,9 @@ type guardedStorage struct { // short paths used here, so " " and "..." reach the syscall as an empty // component. On Unix they are ordinary filenames and legal object keys. // -// Note the second point is reasoned from documented Win32 behaviour, not from a +// Note the second point is reasoned from documented Win32 behavior, not from a // Windows test run: CI is Linux-only, so TestIsVolumeRootAliasIsPlatformCorrect -// pins both branches of the predicate rather than the syscall behaviour itself. +// pins both branches of the predicate rather than the syscall behavior itself. func isVolumeRootAlias(p string) bool { return isVolumeRootAliasOn(p, runtime.GOOS == globalWindowsOSName) } @@ -125,7 +125,7 @@ func isVolumeRootAliasOn(p string, windows bool) bool { // for them, so they say nothing about whether the metadata is sound. // // This is the boundary check; ShardFileSize's own zero-value guard stays as the -// last line of defence against a panic. +// last line of defense against a panic. func guardErasureParams(fi FileInfo) error { // Negative sizes share their rule with the storage layer, which also has to // cope with metadata already on disk; keep the two from drifting apart by diff --git a/cmd/storage-rest-server-guard_test.go b/cmd/storage-rest-server-guard_test.go index 152b2dcdf..0216a09d6 100644 --- a/cmd/storage-rest-server-guard_test.go +++ b/cmd/storage-rest-server-guard_test.go @@ -190,13 +190,22 @@ func TestIsVolumeRootAlias(t *testing.T) { want bool }{ // Collapse back to the volume directory. - {"", true}, {"/", true}, {"//", true}, + {"", true}, + {"/", true}, + {"//", true}, // Backslash is platform-dependent; see TestIsVolumeRootAliasIsPlatformCorrect. // Whitespace is NOT a separator. path.Clean leaves it alone, so these // name real directories and are legal S3 object keys. - {" ", false}, {" ", false}, {"\t", false}, {"\n", false}, - {" / ", false}, {"/ ", false}, - {"a", false}, {"/a", false}, {"..", false}, {" a ", false}, + {" ", false}, + {" ", false}, + {"\t", false}, + {"\n", false}, + {" / ", false}, + {"/ ", false}, + {"a", false}, + {"/a", false}, + {"..", false}, + {" a ", false}, {"obj/part.1", false}, } { if got := isVolumeRootAlias(tc.path); got != tc.want { @@ -239,12 +248,19 @@ func TestIsVolumeRootAliasIsPlatformCorrect(t *testing.T) { // Space and period: ordinary filename characters on Unix, but stripped // from a component by Win32 normalisation, so a component made only of // them vanishes and the path resolves to the volume root. - {" ", false, true}, {" ", false, true}, {" / ", false, true}, - {"...", false, true}, {". .", false, true}, + {" ", false, true}, + {" ", false, true}, + {" / ", false, true}, + {"...", false, true}, + {". .", false, true}, // Never an alias anywhere. - {"\t", false, false}, {"\n", false, false}, - {"a", false, false}, {"/a", false, false}, {"a ", false, false}, - {" a", false, false}, {"a.", false, false}, + {"\t", false, false}, + {"\n", false, false}, + {"a", false, false}, + {"/a", false, false}, + {"a ", false, false}, + {" a", false, false}, + {"a.", false, false}, } { if got := isVolumeRootAliasOn(tc.path, false); got != tc.unix { t.Errorf("isVolumeRootAliasOn(%q, unix) = %v, want %v", tc.path, got, tc.unix) diff --git a/cmd/storage-rest-server.go b/cmd/storage-rest-server.go index 2b0562b9b..0b65edd6b 100644 --- a/cmd/storage-rest-server.go +++ b/cmd/storage-rest-server.go @@ -597,11 +597,14 @@ func (s *storageRESTServer) ReadPartsHandler(w http.ResponseWriter, r *http.Requ done := keepHTTPResponseAlive(w) infos, err := s.getStorage().ReadParts(r.Context(), volume, preq.Paths...) - done(nil) if err != nil { - s.writeErrorResponse(w, err) + // The keep-alive stream owns the response body from here on, so the + // error has to travel through done(); writing a header afterwards is + // too late and leaves the client decoding the error text as msgpack. + done(err) return } + done(nil) presp := &ReadPartsResp{Infos: infos} storageLogIf(r.Context(), msgp.Encode(w, presp)) diff --git a/cmd/storage-rest-traversal_test.go b/cmd/storage-rest-traversal_test.go index cf34d65ef..d1bd4358f 100644 --- a/cmd/storage-rest-traversal_test.go +++ b/cmd/storage-rest-traversal_test.go @@ -469,10 +469,14 @@ func TestCheckPartsMalformedErasure(t *testing.T) { // Deleted short-circuits FileInfo.IsValid() to true, so an IsValid() // guard would not have caught this one. {Volume: "foo", Name: "obj", Deleted: true, Parts: []ObjectPartInfo{{Number: 1, Size: 4}}}, - {Volume: "foo", Name: "obj", Parts: []ObjectPartInfo{{Number: 1, Size: 4}}, - Erasure: ErasureInfo{DataBlocks: 4}}, // BlockSize still zero - {Volume: "foo", Name: "obj", Parts: []ObjectPartInfo{{Number: 1, Size: 4}}, - Erasure: ErasureInfo{BlockSize: blockSizeV2}}, // DataBlocks still zero + { + Volume: "foo", Name: "obj", Parts: []ObjectPartInfo{{Number: 1, Size: 4}}, + Erasure: ErasureInfo{DataBlocks: 4}, + }, // BlockSize still zero + { + Volume: "foo", Name: "obj", Parts: []ObjectPartInfo{{Number: 1, Size: 4}}, + Erasure: ErasureInfo{BlockSize: blockSizeV2}, + }, // DataBlocks still zero } { if _, err := restClient.CheckParts(ctx, "foo", "obj", fi); !errors.Is(err, errFileCorrupt) { t.Errorf("CheckParts(%+v): got %v, want %v", fi.Erasure, err, errFileCorrupt) @@ -584,7 +588,7 @@ func TestDeleteVersionsDeclaredCountIsNotTrusted(t *testing.T) { grew := after.TotalAlloc - before.TotalAlloc t.Logf("total-versions=%s -> err=%v, allocated %d bytes", total, err, grew) // 100000000 * sizeof(FileInfoVersions)(104) is ~9.7 GiB; anything in - // that neighbourhood means the declared count is still being trusted. + // that neighborhood means the declared count is still being trusted. if grew > 64<<20 { t.Errorf("total-versions=%s allocated %d bytes for an empty body - "+ "the declared count is sizing the allocation", total, grew)