s3tables: fix create-after-rename overwriting the renamed table (#10091)

* s3tables: purge decoupled table data without deleting the reused name path

A renamed or created-over-leftover table keeps its data at a location that
differs from its catalog name path. Drop now purges that data location and
clears the marker, instead of recursively deleting the name path, which may
still hold another table's data.

* iceberg: route a table created over a leftover to a unique location

When the default location is occupied by a leftover directory (data kept when
another table was renamed to this name), create the new table at a unique
location so it cannot overwrite that table's metadata. Common case is unchanged.

* iceberg: fail table create when the leftover-path check errors

A transient filer lookup error fell through as "not occupied", routing the
new table back to the default path and risking the very overwrite this check
guards against. Propagate the error and return 500 instead.

* s3tables: assert all catalog xattrs cleared on decoupled drop

Seed the full marker set so the test catches a regression that leaves the
policy, tags, version, or entry-type attribute on the reused name path.

* s3tables: refuse to drop a table whose data path is an ancestor

Corrupt metadata can resolve the data path to the bucket or namespace root,
which the bucket-scope check still admits; a recursive purge there would wipe
sibling tables. Reject an ancestor data path before deleting.

weed/s3api/iceberg/handlers_table.go

@@ -89,6 +89,34 @@ func (s *Server) handleListTables(w http.ResponseWriter, r *http.Request) {
 	writeJSON(w, http.StatusOK, result)
 }
 
+// tablePathOccupied reports whether a filer entry already exists at the catalog
+// name path. A leftover directory there (e.g. data kept when another table was
+// renamed to this name) means a table created at the default location would
+// overwrite it, so the caller routes the new table to a unique location. A
+// lookup failure other than not-found is returned so the caller can fail the
+// create rather than fall back to the default path on a transient filer error.
+func (s *Server) tablePathOccupied(ctx context.Context, bucketName, tablePath string) (bool, error) {
+	full := path.Join(s3tables.TablesPath, bucketName, tablePath)
+	dir, name := path.Split(full)
+	dir = strings.TrimSuffix(dir, "/")
+	occupied := false
+	err := s.filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error {
+		_, err := filer_pb.LookupEntry(ctx, client, &filer_pb.LookupDirectoryEntryRequest{
+			Directory: dir,
+			Name:      name,
+		})
+		if errors.Is(err, filer_pb.ErrNotFound) {
+			return nil
+		}
+		if err != nil {
+			return err
+		}
+		occupied = true
+		return nil
+	})
+	return occupied, err
+}
+
 // handleCreateTable creates a new table.
 func (s *Server) handleCreateTable(w http.ResponseWriter, r *http.Request) {
 	vars := mux.Vars(r)
@@ -124,6 +152,17 @@ func (s *Server) handleCreateTable(w http.ResponseWriter, r *http.Request) {
 	tablePath := path.Join(flattenNamespacePath(namespace), req.Name)
 	location := strings.TrimSuffix(req.Location, "/")
 	if location == "" {
+		// If a leftover directory already occupies the default location (data
+		// kept when another table was renamed to this name), route this table
+		// to a unique location so it cannot overwrite that table's files.
+		occupied, err := s.tablePathOccupied(r.Context(), bucketName, tablePath)
+		if err != nil {
+			writeError(w, http.StatusInternalServerError, "InternalServerError", "Failed to check table location: "+err.Error())
+			return
+		}
+		if occupied {
+			tablePath = path.Join(flattenNamespacePath(namespace), req.Name+"-"+tableUUID.String())
+		}
 		if req.Properties != nil {
 			if warehouse := strings.TrimSuffix(req.Properties["warehouse"], "/"); warehouse != "" {
 				location = fmt.Sprintf("%s/%s", warehouse, tablePath)

weed/s3api/s3tables/handler_delete_decouple_test.go

@@ -0,0 +1,94 @@
+package s3tables
+
+import (
+	"context"
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func runDeleteTable(t *testing.T, m *Manager, fs *memFilerServer, namespace, name string) error {
+	t.Helper()
+	return m.Execute(context.Background(), NewManagerClient(fs.client), "DeleteTable", &DeleteTableRequest{
+		TableBucketARN: mustBucketARN(t),
+		Namespace:      []string{namespace},
+		Name:           name,
+	}, nil, "")
+}
+
+// A table whose data was decoupled from its name (created over a leftover from a
+// rename): catalog marker at ns/newt, data at ns/newt-x, and ns/newt still holds
+// another table's leftover data. Dropping it must purge only its own data and
+// clear the marker, never the data under the reused name path.
+func TestDeleteTableDecoupledKeepsReusedNamePath(t *testing.T) {
+	fs, m := startRenameManager(t)
+
+	newtMeta, _ := json.Marshal(tableMetadataInternal{
+		Name:             "newt",
+		Namespace:        "ns",
+		Format:           "ICEBERG",
+		OwnerAccountID:   DefaultAccountID,
+		MetadataLocation: "s3://" + renameTestBucket + "/ns/newt-x/metadata/v1.metadata.json",
+	})
+	markerKeys := []string{ExtendedKeyMetadata, ExtendedKeyMetadataVersion, ExtendedKeyPolicy, ExtendedKeyTags, ExtendedKeyEntryType}
+	fs.putEntry(GetNamespacePath(renameTestBucket, "ns"), "newt", map[string][]byte{
+		ExtendedKeyMetadata:        newtMeta,
+		ExtendedKeyMetadataVersion: []byte("v1"),
+		ExtendedKeyPolicy:          []byte(`{"Version":"2012-10-17"}`),
+		ExtendedKeyTags:            []byte(`{"k":"v"}`),
+		ExtendedKeyEntryType:       []byte(EntryTypeTable),
+	})
+	fs.putEntry(GetTablePath(renameTestBucket, "ns", "newt"), "leftover", nil) // another table's data under the name path
+	fs.putEntry(GetNamespacePath(renameTestBucket, "ns"), "newt-x", nil)       // this table's own (decoupled) data
+	fs.putEntry(GetTablePath(renameTestBucket, "ns", "newt-x"), "metadata", nil)
+
+	require.NoError(t, runDeleteTable(t, m, fs, "ns", "newt"))
+
+	assert.Nil(t, fs.getEntry(GetNamespacePath(renameTestBucket, "ns"), "newt-x"),
+		"the table's own data location must be purged")
+	assert.NotNil(t, fs.getEntry(GetTablePath(renameTestBucket, "ns", "newt"), "leftover"),
+		"data under the reused name path must survive")
+	marker := fs.getEntry(GetNamespacePath(renameTestBucket, "ns"), "newt")
+	require.NotNil(t, marker)
+	for _, key := range markerKeys {
+		_, present := marker.Extended[key]
+		assert.Falsef(t, present, "catalog attribute %s must be cleared", key)
+	}
+}
+
+// A table whose MetadataLocation resolves to an ancestor of its own name path
+// (here the namespace root, e.g. from corrupt metadata) must not be deleted: a
+// recursive purge of that data path would wipe sibling tables. The delete is
+// refused and the namespace's other tables survive.
+func TestDeleteTableRefusesAncestorDataPath(t *testing.T) {
+	fs, m := startRenameManager(t)
+
+	badMeta, _ := json.Marshal(tableMetadataInternal{
+		Name:             "badt",
+		Namespace:        "ns",
+		Format:           "ICEBERG",
+		OwnerAccountID:   DefaultAccountID,
+		MetadataLocation: "s3://" + renameTestBucket + "/ns/metadata/v1.metadata.json",
+	})
+	fs.putEntry(GetNamespacePath(renameTestBucket, "ns"), "badt", map[string][]byte{ExtendedKeyMetadata: badMeta})
+
+	require.Error(t, runDeleteTable(t, m, fs, "ns", "badt"))
+
+	// The sibling table seeded by startRenameManager and its data must survive.
+	assert.NotNil(t, fs.getEntry(GetNamespacePath(renameTestBucket, "ns"), "t"),
+		"sibling table marker must survive a refused delete")
+	assert.NotNil(t, fs.getEntry(GetTablePath(renameTestBucket, "ns", "t"), "data"),
+		"sibling table data must survive a refused delete")
+}
+
+// A normal colocated table (data under its own name path) is removed wholesale.
+func TestDeleteTableColocatedRemovesData(t *testing.T) {
+	fs, m := startRenameManager(t)
+
+	require.NoError(t, runDeleteTable(t, m, fs, "ns", "t"))
+
+	assert.Nil(t, fs.getEntry(GetNamespacePath(renameTestBucket, "ns"), "t"), "colocated table entry must be deleted")
+	assert.Nil(t, fs.getEntry(GetTablePath(renameTestBucket, "ns", "t"), "metadata"), "colocated table data must be deleted")
+}

weed/s3api/s3tables/handler_table.go

@@ -1182,18 +1182,28 @@ func (h *S3TablesHandler) handleDeleteTable(w http.ResponseWriter, r *http.Reque
 
 	// Delete the table
 	err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error {
-		if err := h.deleteDirectory(r.Context(), client, tablePath); err != nil {
-			return err
-		}
-		// A renamed table keeps its data at the original location, so the catalog
-		// path no longer holds it; purge the data directory too when it differs.
 		dataPath := tableDataDirFromMetadataLocation(metadata.MetadataLocation)
 		if dataPath != "" && dataPath != tablePath && strings.HasPrefix(dataPath+"/", GetTableBucketPath(bucketName)+"/") {
+			// Refuse to purge a data path that is an ancestor of the table's own
+			// name path (e.g. corrupt metadata resolving to the bucket or
+			// namespace root): the bucket-scope check above still admits the
+			// bucket root, and a recursive delete there would take out unrelated
+			// tables.
+			if strings.HasPrefix(tablePath+"/", dataPath+"/") {
+				return fmt.Errorf("refusing to delete table %s: data path %q is an ancestor of catalog path %q", tableName, dataPath, tablePath)
+			}
+			// Decoupled table (renamed, or created over a leftover): its data
+			// lives elsewhere. Purge the data, then clear the catalog marker
+			// without deleting the name path -- it may still hold another
+			// table's data that was left when this name was reused.
 			if err := h.deleteDirectory(r.Context(), client, dataPath); err != nil {
 				return err
 			}
+			return h.removeExtendedAttributes(r.Context(), client, tablePath,
+				ExtendedKeyMetadata, ExtendedKeyMetadataVersion, ExtendedKeyPolicy, ExtendedKeyTags, ExtendedKeyEntryType)
 		}
-		return nil
+		// Colocated table: the name path holds the data.
+		return h.deleteDirectory(r.Context(), client, tablePath)
 	})
 
 	if err != nil {