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 <noreply@openai.com>
Co-authored-by: Claude <noreply@anthropic.com>

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

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)

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))

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)