fix(iceberg): keep a losing table create from deleting the winner's metadata (#12474)

* iceberg: keep a losing table create from deleting the winner's metadata

* iceberg: keep a staged file whose published record was renamed away

A create answers a conflict when its record was already visible and
another request changed it before the create could back it out. The
cleanup then looked only at the original name, so a record renamed in
between read as absent and the file it still points at was deleted.

S3 Tables now reports that case with a conflict of its own, and both
create paths keep the staged file on it instead of probing the name.
A conflict over a still-hidden record, lost to another create, still
removes the file.

* iceberg: discard a losing create's metadata only while it is still its own

The cleanup deleted the staged file by name. The path can be reused
before it runs: the table is dropped and purged, and another create
stages the same v1.metadata.json and registers it, which the losing
request then deleted.

Staged files now carry a per-request token in an internal extended
attribute, and the cleanup deletes with that token as the filer's
ExpectedExtended precondition, checked under the path lock the delete
runs under. A file that took the path since is left in place.

* iceberg: classify a typed S3 Tables error by its code, not its message

isS3TablesAlreadyExists matched "already exists" in the message before
looking at the code, so an InternalError whose text mentioned an
existing entry read as the name being taken: CreateTable answered 409
instead of 500, and registrationRefused discarded a staged file the
catalog may still point at. A typed error now decides by its code; only
an untyped one is matched by message.

* iceberg: pin stage-create on in the stage-create conflict test

Stage-create is on by default, but a shell that sets
ICEBERG_ENABLE_STAGE_CREATE=false turned the expected 409 into a 501.
Setting it in the test keeps the result independent of the environment.

* iceberg: test two creates overlapping at the canonical metadata name

The concurrent-creators test only lines the creators up before they
start, so whether two of them ever collide at v1.metadata.json is up to
the scheduler. The new test fixes the order: the second create runs
while the first has staged v1.metadata.json and not yet registered it.
It checks that every staging attempt is exclusive, that the second falls
back to v1-<uuid>, that both files hold their own metadata while both
are staged, and that the loser removes only its own file.

* s3tables: a create whose unmark write never went out stayed hidden

confirmChild counted any failure to take a create's mark off as possibly
published, the lookup that precedes the write included. That lookup
failing leaves the mark on, so the record stayed hidden; if another
create then adopted it, the back-out reported a published record that
changed, and the Iceberg catalog kept a staged file no record names.

The mark-removal calls now go through a wrapper that notes whether an
UpdateEntry went out without the filer refusing it. Only then may the
record have been seen. A refused compare, a missing entry, or a failure
before any write leaves the create's record hidden.

* s3tables: pass mayBeVisible into backOutChild

Fixes the build after rebasing onto the backOutChild split.

weed/s3api/s3tables/filer_ops.go

@@ -17,6 +17,7 @@ import (
 	"github.com/seaweedfs/seaweedfs/weed/glog"
 	"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
 	"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
+	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -26,6 +27,11 @@ var (
 	ErrConcurrentUpdate  = errors.New("attribute changed concurrently")
 	ErrNameTaken         = errors.New("catalog name is taken")
 	ErrLocationInUse     = errors.New("location is in use by another table or view")
+	// ErrPublishedRecordChanged is the ErrConcurrentUpdate a create returns
+	// when the record it wrote was already visible, and another caller changed
+	// it — updated it, or renamed it away — before the create could back it
+	// out. Whatever the create staged for that record may still be in use.
+	ErrPublishedRecordChanged = fmt.Errorf("published record changed before the create could back it out: %w", ErrConcurrentUpdate)
 )
 
 // extendedAttributeUpdateAttempts bounds the read-modify-write retry loop in
@@ -799,11 +805,18 @@ func (h *S3TablesHandler) confirmParentOnce(ctx context.Context, client filer_pb
 
 func (h *S3TablesHandler) confirmChild(ctx context.Context, client filer_pb.SeaweedFilerClient, parentPath string, isParent func(*filer_pb.Entry) bool, recheck bool, childPath string, written map[string][]byte, emptyDirs []string) error {
 	parent, err := h.openParent(ctx, client, parentPath, isParent)
-	confirmed := false
+	// mayBeVisible is whether the mark may have come off: it did, or a write
+	// taking it off was sent and failed without the filer saying it refused
+	// it. A failure before any such write — the lookup that precedes it, say —
+	// leaves the mark on, and the record as invisible as it was.
+	confirmed, mayBeVisible := false, false
 	if err == nil {
 		// The mark comes off only while the record still holds what this
 		// create wrote; a failed write backs the record out like a closed parent.
-		if err = h.removeExtendedAttributesIf(ctx, client, childPath, written, ExtendedKeyDeleting); err == nil {
+		writes := &markRemovalWrites{SeaweedFilerClient: client}
+		err = h.removeExtendedAttributesIf(ctx, writes, childPath, written, ExtendedKeyDeleting)
+		mayBeVisible = err == nil || writes.uncertain
+		if err == nil {
 			confirmed = true
 			if recheck {
 				err = h.parentStillOpen(ctx, client, parentPath, isParent, parent)
@@ -813,7 +826,7 @@ func (h *S3TablesHandler) confirmChild(ctx context.Context, client filer_pb.Seaw
 	if err == nil {
 		return nil
 	}
-	err = h.backOutChild(ctx, client, childPath, written, confirmed, emptyDirs, err)
+	err = h.backOutChild(ctx, client, childPath, written, confirmed, mayBeVisible, emptyDirs, err)
 	if errors.Is(err, errParentGone) {
 		h.removeLeftoverDirectory(context.WithoutCancel(ctx), client, parentPath)
 	}
@@ -821,8 +834,11 @@ func (h *S3TablesHandler) confirmChild(ctx context.Context, client filer_pb.Seaw
 }
 
 // backOutChild removes what a create wrote onto childPath, after the create
-// failed with err, and returns err, or why the record stays.
-func (h *S3TablesHandler) backOutChild(ctx context.Context, client filer_pb.SeaweedFilerClient, childPath string, written map[string][]byte, confirmed bool, emptyDirs []string, err error) error {
+// failed with err, and returns err, or why the record stays. mayBeVisible is
+// whether readers may have seen the record: a record that moved on is then
+// reported as ErrPublishedRecordChanged, since what was staged for it may
+// still be in use.
+func (h *S3TablesHandler) backOutChild(ctx context.Context, client filer_pb.SeaweedFilerClient, childPath string, written map[string][]byte, confirmed, mayBeVisible bool, emptyDirs []string, err error) error {
 	ctx = context.WithoutCancel(ctx)
 	expected := written
 	keys := make([]string, 0, len(written))
@@ -845,7 +861,12 @@ func (h *S3TablesHandler) backOutChild(ctx context.Context, client filer_pb.Seaw
 		glog.Warningf("back out %s: %v", childPath, backOutErr)
 		if errors.Is(backOutErr, ErrConcurrentUpdate) {
 			// The record moved past this create — adopted or rewritten — so
-			// there is nothing of its own left to remove.
+			// there is nothing of its own left to remove. While it was still
+			// marked, only another create could take it over; once readers
+			// could see it, an update or a rename may have built on it.
+			if mayBeVisible {
+				return fmt.Errorf("%w: %s", ErrPublishedRecordChanged, childPath)
+			}
 			return fmt.Errorf("%w: %s changed during create", ErrConcurrentUpdate, childPath)
 		}
 		return fmt.Errorf("%w: %s: %v (%v)", errBackOutFailed, childPath, backOutErr, err)
@@ -856,6 +877,24 @@ func (h *S3TablesHandler) backOutChild(ctx context.Context, client filer_pb.Seaw
 	return err
 }
 
+// markRemovalWrites passes a create's mark-removal calls through to the filer
+// and records whether an UpdateEntry was sent whose outcome is not a refusal:
+// it succeeded, or it failed in a way that does not say whether it landed. A
+// compare the filer refused (FailedPrecondition) or an entry it no longer has
+// (NotFound) changed nothing.
+type markRemovalWrites struct {
+	filer_pb.SeaweedFilerClient
+	uncertain bool
+}
+
+func (w *markRemovalWrites) UpdateEntry(ctx context.Context, in *filer_pb.UpdateEntryRequest, opts ...grpc.CallOption) (*filer_pb.UpdateEntryResponse, error) {
+	resp, err := w.SeaweedFilerClient.UpdateEntry(ctx, in, opts...)
+	if code := status.Code(err); code != codes.FailedPrecondition && code != codes.NotFound {
+		w.uncertain = true
+	}
+	return resp, err
+}
+
 // dirListPageSize is how many entries dirHoldsFiles reads per ListEntries call.
 const dirListPageSize = 1024
 
@@ -1055,7 +1094,8 @@ func (h *S3TablesHandler) claimCatalogEntryAt(ctx context.Context, client filer_
 		return err
 	}
 	if err := checkLocationUnclaimed(ctx, client, bucketName, path, metadataLocation); err != nil {
-		return h.backOutChild(ctx, client, path, attrs, false, emptyDirs, err)
+		// The claim still carries its mark, so no reader has seen the record.
+		return h.backOutChild(ctx, client, path, attrs, false, false, emptyDirs, err)
 	}
 	return nil
 }

weed/s3api/s3tables/handler_table.go

@@ -178,6 +178,10 @@ func (h *S3TablesHandler) handleCreateTable(w http.ResponseWriter, r *http.Reque
 			h.writeError(w, http.StatusConflict, ErrCodeTableAlreadyExists, fmt.Sprintf("table %s already exists", tableName))
 			return err
 		}
+		if errors.Is(err, ErrPublishedRecordChanged) {
+			h.writeError(w, http.StatusConflict, ErrCodeConflict, PublishedTableChangedMessage)
+			return err
+		}
 		if errors.Is(err, ErrConcurrentUpdate) {
 			h.writeError(w, http.StatusConflict, ErrCodeConflict, "table changed during create, retry the request")
 			return err

weed/s3api/s3tables/table_data_dir_test.go

@@ -24,6 +24,7 @@ func TestTableDataDirFromMetadataLocation(t *testing.T) {
 
 func TestMetadataVersionFromLocationUniqueSuffix(t *testing.T) {
 	cases := map[string]int{
+		"s3://bkt/ns/t/metadata/v1-0a1b2c3d-4e5f-6789-abcd-ef0123456789.metadata.json": 1,
 		"s3://bkt/ns/t/metadata/v4.metadata.json":                                      4,
 		"s3://bkt/ns/t/metadata/v4-0a1b2c3d-4e5f-6789-abcd-ef0123456789.metadata.json": 4,
 		"s3://bkt/ns/t/metadata/00007-0a1b2c3d.metadata.json":                          7,

weed/s3api/s3tables/types.go

@@ -2,6 +2,7 @@ package s3tables
 
 import (
 	"encoding/json"
+	"errors"
 	"strings"
 	"time"
 )
@@ -589,6 +590,23 @@ func (e *S3TablesError) Error() string {
 	return e.Message
 }
 
+// PublishedTableChangedMessage is the message of the conflict a CreateTable
+// answers when the table it registered was already visible, and another
+// caller updated or renamed it before the create could back it out. The
+// record survives, under this name or another, so whatever the caller staged
+// for it may still be in use.
+const PublishedTableChangedMessage = "table was registered and then changed by another request before the create finished"
+
+// IsPublishedTableChanged reports whether err, as a CreateTable caller
+// receives it, is the conflict PublishedTableChangedMessage describes.
+func IsPublishedTableChanged(err error) bool {
+	if errors.Is(err, ErrPublishedRecordChanged) {
+		return true
+	}
+	var tableErr *S3TablesError
+	return errors.As(err, &tableErr) && tableErr.Type == ErrCodeConflict && tableErr.Message == PublishedTableChangedMessage
+}
+
 // Table formats a catalog entry may declare.
 //
 // ICEBERG tables carry metadata the catalog maintains and the maintenance