fix(s3tables): answer 404 for a table name that was renamed away (#12258)

* s3tables: answer 404 for a table name that was renamed away

A rename leaves the source directory in place to hold the data under
it, with its catalog attributes cleared. UpdateTable, DeleteTable, the
table policy calls and the table maintenance calls read the metadata
attribute of that directory and reported its absence as an internal
error, so they answered 500, and GetTablePolicy answered NoSuchPolicy.

An absent metadata attribute now reads as a missing entry. tableMetadata
reports it as both filer_pb.ErrNotFound and the attribute error, so
callers that test for either keep working. The policy and maintenance
handlers read the attribute through the new readTableMetadataAttr, which
reports filer_pb.ErrNotFound, so each call answers NoSuchTable.

* s3tables: a view record without its metadata reads absent too

* s3tables: refuse a catalog write that lands on a metadata-less record

---------

Co-authored-by: user.email <chrislusf@users.noreply.github.com>

weed/s3api/s3tables/filer_ops.go

@@ -378,6 +378,16 @@ func (h *S3TablesHandler) loadNamespaceMetadata(ctx context.Context, filerClient
 	return &metadata, nil
 }
 
+// readTableMetadataAttr returns the table-metadata xattr at tablePath. A
+// directory without it is not a table, so its absence reads as a missing entry.
+func (h *S3TablesHandler) readTableMetadataAttr(ctx context.Context, client filer_pb.SeaweedFilerClient, tablePath string) ([]byte, error) {
+	data, err := h.getExtendedAttribute(ctx, client, tablePath, ExtendedKeyMetadata)
+	if errors.Is(err, ErrAttributeNotFound) {
+		return nil, fmt.Errorf("%w: %s", filer_pb.ErrNotFound, tablePath)
+	}
+	return data, err
+}
+
 // lookupEntry returns the filer entry at the given path.
 func (h *S3TablesHandler) lookupEntry(ctx context.Context, client filer_pb.SeaweedFilerClient, path string) (*filer_pb.Entry, error) {
 	dir, name := splitPath(path)
@@ -551,6 +561,9 @@ func (h *S3TablesHandler) mutateCatalogRecord(
 ) error {
 	return h.mutateEntryExtended(ctx, client, path, func(extended map[string][]byte) error {
 		if _, claimed := extended[ExtendedKeyDeleting]; !claimed {
+			if _, ok := extended[ExtendedKeyMetadata]; !ok {
+				return filer_pb.ErrNotFound
+			}
 			return mutate(extended)
 		}
 		if isUnfinishedCreate(extended) {

weed/s3api/s3tables/handler_maintenance.go

@@ -371,7 +371,7 @@ func (h *S3TablesHandler) authorizeMaintenanceTable(r *http.Request, filerClient
 	var bucketPolicy, tablePolicy string
 	var bucketTags, tableTags map[string]string
 	err := filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error {
-		data, err := h.getExtendedAttribute(r.Context(), client, tablePath, ExtendedKeyMetadata)
+		data, err := h.readTableMetadataAttr(r.Context(), client, tablePath)
 		if err != nil {
 			return err
 		}

weed/s3api/s3tables/handler_policy.go

@@ -349,7 +349,7 @@ func (h *S3TablesHandler) handlePutTablePolicy(w http.ResponseWriter, r *http.Re
 	var bucketPolicy, tablePolicy string
 	var bucketTags, tableTags map[string]string
 	err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error {
-		data, err := h.getExtendedAttribute(r.Context(), client, tablePath, ExtendedKeyMetadata)
+		data, err := h.readTableMetadataAttr(r.Context(), client, tablePath)
 		if err != nil {
 			return err
 		}
@@ -464,7 +464,7 @@ func (h *S3TablesHandler) handleGetTablePolicy(w http.ResponseWriter, r *http.Re
 
 	err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error {
 		// Get metadata for ownership check
-		data, err := h.getExtendedAttribute(r.Context(), client, tablePath, ExtendedKeyMetadata)
+		data, err := h.readTableMetadataAttr(r.Context(), client, tablePath)
 		if err != nil {
 			return err
 		}
@@ -568,7 +568,7 @@ func (h *S3TablesHandler) handleDeleteTablePolicy(w http.ResponseWriter, r *http
 	var bucketPolicy, tablePolicy string
 	var bucketTags, tableTags map[string]string
 	err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error {
-		data, err := h.getExtendedAttribute(r.Context(), client, tablePath, ExtendedKeyMetadata)
+		data, err := h.readTableMetadataAttr(r.Context(), client, tablePath)
 		if err != nil {
 			return err
 		}

weed/s3api/s3tables/handler_renamed_source_test.go

@@ -0,0 +1,180 @@
+package s3tables
+
+import (
+	"context"
+	"net/http"
+	"path"
+	"testing"
+
+	"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
+	"github.com/seaweedfs/seaweedfs/weed/s3api/s3tables/s3tablestest"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// extendedOf copies the attributes held on an entry, so a later write to the
+// entry shows up as a difference.
+func extendedOf(t *testing.T, fs *s3tablestest.MemFiler, dir, name string) map[string][]byte {
+	t.Helper()
+	entry := fs.Get(dir, name)
+	require.NotNil(t, entry)
+	copied := make(map[string][]byte, len(entry.Extended))
+	for key, value := range entry.Extended {
+		copied[key] = append([]byte(nil), value...)
+	}
+	return copied
+}
+
+// A rename leaves the source directory in place to hold the data under it, with
+// its catalog attributes cleared. That name is no longer a table, so every table
+// operation on it answers NoSuchTable and writes nothing to the directory.
+func TestRenamedAwayNameIsNoSuchTable(t *testing.T) {
+	fs, m := startRenameManager(t)
+	requireOK(t, m, fs, "CreateNamespace", &CreateNamespaceRequest{
+		TableBucketARN: mustBucketARN(t),
+		Namespace:      []string{"ns2"},
+	})
+	requireOK(t, m, fs, "RenameTable", &RenameTableRequest{
+		TableBucketARN:  mustBucketARN(t),
+		SourceNamespace: []string{"ns"},
+		SourceName:      "t",
+		DestNamespace:   []string{"ns2"},
+		DestName:        "t",
+	})
+	nsPath := GetNamespacePath(renameTestBucket, "ns")
+	before := extendedOf(t, fs, nsPath, "t")
+
+	bucketARN := mustBucketARN(t)
+	ops := []struct {
+		operation string
+		input     interface{}
+	}{
+		{"UpdateTable", &UpdateTableRequest{
+			TableBucketARN:   bucketARN,
+			Namespace:        []string{"ns"},
+			Name:             "t",
+			MetadataVersion:  4,
+			MetadataLocation: "s3://" + renameTestBucket + "/ns/t/metadata/v4.metadata.json",
+		}},
+		{"DeleteTable", deleteTableRequest(t, "t")},
+		{"PutTablePolicy", &PutTablePolicyRequest{
+			TableBucketARN: bucketARN,
+			Namespace:      []string{"ns"},
+			Name:           "t",
+			ResourcePolicy: `{"Statement":[{"Effect":"Allow","Principal":"writer","Action":"s3tables:*","Resource":"*"}]}`,
+		}},
+		{"GetTablePolicy", &GetTablePolicyRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "t"}},
+		{"DeleteTablePolicy", &DeleteTablePolicyRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "t"}},
+		{"PutTableMaintenanceConfiguration", &PutTableMaintenanceConfigurationRequest{
+			TableBucketARN: bucketARN,
+			Namespace:      []string{"ns"},
+			Name:           "t",
+			Type:           MaintenanceTypeIcebergCompaction,
+			Value:          &MaintenanceConfigurationValue{Status: MaintenanceStatusEnabled},
+		}},
+		{"GetTableMaintenanceConfiguration", &GetTableMaintenanceConfigurationRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "t"}},
+		{"GetTableMaintenanceJobStatus", &GetTableMaintenanceJobStatusRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "t"}},
+	}
+	for _, op := range ops {
+		t.Run(op.operation, func(t *testing.T) {
+			status, resp := runHandler(t, m, fs.Client, op.operation, op.input)
+			assert.Equal(t, http.StatusNotFound, status, "%+v", resp)
+			assert.Equal(t, ErrCodeNoSuchTable, resp.Type, "%+v", resp)
+		})
+	}
+
+	assert.Equal(t, before, extendedOf(t, fs, nsPath, "t"), "the renamed-away directory must not be written to")
+	status, resp := runHandler(t, m, fs.Client, "GetTable", &GetTableRequest{
+		TableBucketARN: bucketARN,
+		Namespace:      []string{"ns2"},
+		Name:           "t",
+	})
+	assert.Equal(t, http.StatusOK, status, "the renamed table must still resolve: %+v", resp)
+}
+
+// Views share the renamer, so a renamed-away view name answers NoSuchView the
+// same way, and the leftover view record still takes no writes.
+func TestRenamedAwayViewNameIsNoSuchView(t *testing.T) {
+	fs, m := startRenameManager(t)
+	seedView(t, fs, "v")
+	requireOK(t, m, fs, "RenameView", &RenameTableRequest{
+		TableBucketARN:  mustBucketARN(t),
+		SourceNamespace: []string{"ns"},
+		SourceName:      "v",
+		DestNamespace:   []string{"ns"},
+		DestName:        "v2",
+	})
+	nsPath := GetNamespacePath(renameTestBucket, "ns")
+	before := extendedOf(t, fs, nsPath, "v")
+
+	bucketARN := mustBucketARN(t)
+	ops := []struct {
+		operation string
+		input     interface{}
+	}{
+		{"GetView", &GetViewRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "v"}},
+		{"UpdateView", &UpdateViewRequest{
+			TableBucketARN:   bucketARN,
+			Namespace:        []string{"ns"},
+			Name:             "v",
+			MetadataLocation: "s3://" + renameTestBucket + "/ns/v/metadata/v2.metadata.json",
+		}},
+		{"DeleteView", &DeleteViewRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "v"}},
+	}
+	for _, op := range ops {
+		t.Run(op.operation, func(t *testing.T) {
+			status, resp := runHandler(t, m, fs.Client, op.operation, op.input)
+			assert.Equal(t, http.StatusNotFound, status, "%+v", resp)
+			assert.Equal(t, ErrCodeNoSuchView, resp.Type, "%+v", resp)
+		})
+	}
+
+	assert.Equal(t, before, extendedOf(t, fs, nsPath, "v"), "the renamed-away directory must not be written to")
+}
+
+// A record that keeps its view marker but lost the metadata — the state a
+// rename leaves behind while its clear is in flight — is not a view to its
+// readers either.
+func TestViewRecordWithoutMetadataIsNoSuchView(t *testing.T) {
+	fs, m := startRenameManager(t)
+	nsPath := GetNamespacePath(renameTestBucket, "ns")
+	fs.Put(nsPath, "half", map[string][]byte{ExtendedKeyEntryType: []byte(EntryTypeView)})
+
+	bucketARN := mustBucketARN(t)
+	ops := []struct {
+		operation string
+		input     interface{}
+	}{
+		{"GetView", &GetViewRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "half"}},
+		{"UpdateView", &UpdateViewRequest{
+			TableBucketARN:   bucketARN,
+			Namespace:        []string{"ns"},
+			Name:             "half",
+			MetadataLocation: "s3://" + renameTestBucket + "/ns/half/metadata/v2.metadata.json",
+		}},
+		{"DeleteView", &DeleteViewRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "half"}},
+	}
+	for _, op := range ops {
+		t.Run(op.operation, func(t *testing.T) {
+			status, resp := runHandler(t, m, fs.Client, op.operation, op.input)
+			assert.Equal(t, http.StatusNotFound, status, "%+v", resp)
+			assert.Equal(t, ErrCodeNoSuchView, resp.Type, "%+v", resp)
+		})
+	}
+}
+
+// A write landing after the record lost its metadata — a rename cleared it
+// between the handler's read and the mutation — must not write the leftover
+// directory.
+func TestMutateCatalogRecordRefusesClearedRecord(t *testing.T) {
+	fs, m := startRenameManager(t)
+	nsPath := GetNamespacePath(renameTestBucket, "ns")
+	fs.Put(nsPath, "gone", map[string][]byte{})
+
+	err := m.handler.mutateCatalogRecord(context.Background(), fs.Client, path.Join(nsPath, "gone"), func(extended map[string][]byte) error {
+		extended[ExtendedKeyPolicy] = []byte("x")
+		return nil
+	})
+	assert.ErrorIs(t, err, filer_pb.ErrNotFound)
+	assert.Empty(t, fs.Get(nsPath, "gone").Extended)
+}

weed/s3api/s3tables/handler_table.go

@@ -1298,14 +1298,17 @@ func (h *S3TablesHandler) readTableMetadata(ctx context.Context, client filer_pb
 // tableMetadata extracts the table-metadata xattr from an entry. A view stored
 // at the same name is reported absent, the way GetTable and the renamer's
 // entry-type check already treat kind mismatches, and so is a record its
-// create has not finished writing.
+// create has not finished writing. A directory without the record is no table
+// either, and reports the same absence.
 func tableMetadata(entry *filer_pb.Entry) ([]byte, error) {
 	if EntryType(entry.Extended) == EntryTypeView || isUnfinishedCreate(entry.Extended) {
 		return nil, filer_pb.ErrNotFound
 	}
 	data, ok := entry.Extended[ExtendedKeyMetadata]
 	if !ok {
-		return nil, fmt.Errorf("%w: %s", ErrAttributeNotFound, ExtendedKeyMetadata)
+		// A directory without the record is not a table (e.g. a renamed-away
+		// source), so it reads as a missing entry.
+		return nil, fmt.Errorf("%w: %w: %s", filer_pb.ErrNotFound, ErrAttributeNotFound, ExtendedKeyMetadata)
 	}
 	return data, nil
 }

weed/s3api/s3tables/handler_view.go

@@ -224,7 +224,7 @@ func (h *S3TablesHandler) handleGetView(w http.ResponseWriter, r *http.Request,
 		}
 		data, ok := entry.Extended[ExtendedKeyMetadata]
 		if !ok {
-			return fmt.Errorf("%w: %s", ErrAttributeNotFound, ExtendedKeyMetadata)
+			return fmt.Errorf("%w: %w: %s", filer_pb.ErrNotFound, ErrAttributeNotFound, ExtendedKeyMetadata)
 		}
 		return json.Unmarshal(data, &metadata)
 	})
@@ -470,7 +470,7 @@ func (h *S3TablesHandler) authorizeUpdateView(w http.ResponseWriter, r *http.Req
 		}
 		data, ok := entry.Extended[ExtendedKeyMetadata]
 		if !ok {
-			return fmt.Errorf("%w: %s", ErrAttributeNotFound, ExtendedKeyMetadata)
+			return fmt.Errorf("%w: %w: %s", filer_pb.ErrNotFound, ErrAttributeNotFound, ExtendedKeyMetadata)
 		}
 		return json.Unmarshal(data, &metadata)
 	})
@@ -594,7 +594,7 @@ func completedViewMetadata(entry *filer_pb.Entry, token string) (tableMetadataIn
 	}
 	data, ok := entry.Extended[ExtendedKeyMetadata]
 	if !ok {
-		return metadata, fmt.Errorf("%w: %s", ErrAttributeNotFound, ExtendedKeyMetadata)
+		return metadata, fmt.Errorf("%w: %w: %s", filer_pb.ErrNotFound, ErrAttributeNotFound, ExtendedKeyMetadata)
 	}
 	if err := json.Unmarshal(data, &metadata); err != nil {
 		return metadata, err