iceberg maintenance: cache a remote-only file before reading it (#12191)

* iceberg maintenance: an unreadable parquet file fails the job, not the process parquet.NewReader panics on any file it cannot open (wrong magic, a truncated footer, zero bytes), and parquet-go's row decoder panics on a corrupt page under a valid footer. Maintenance runs in the worker's goroutines with no recover, so one bad delete or data file in any table ended the whole process. - Every parquet open in compaction goes through newParquetReader, which opens with parquet.OpenFile and returns the error. - A panic while decoding a delete file or a data file's rows is recovered into an error naming the file (recoverRead/guardRead). - As a backstop, detection recovers per table and Execute per operation: the table is skipped or the operation fails, and the worker keeps running. A failed delete file still fails the table's compaction (no commit), as a read error did before; a failed data file still skips only its bin. * iceberg maintenance: cache a remote-only file before reading it On a bucket mounted with remote.mount, a file that is not cached locally has no chunks and no inline content, only its RemoteEntry. Maintenance read it as an empty file: a parquet reader failed on zero bytes (before the previous change, it panicked and ended the process) and a manifest failed to parse, so a table whose files had been uncached could not be maintained. lookupFileByIcebergPath, the lookup every read goes through, now calls CacheRemoteObjectToLocalCluster for a remote-only entry, as the S3 gateway does on a read, and reads the entry it returns; the call is tried twice, and an entry still remote-only after caching is an error. The lookups that only resolve a path (orphan removal) do not cache. readEntryContent reports a file with no content and no chunks as empty instead of returning zero bytes. The fake volume server in the tests now serves chunk reads, so the remote-only tests read the cached file through the chunk reader as a real one is read. * iceberg maintenance: bound the metadata location read before caching a remote file * iceberg maintenance: bound the metadata read on the entry the cache returns * iceberg maintenance: enforce the metadata size bound inside the remote cache call The worker checks the resolved entry before asking the filer to cache a remote-only object, but the filer resolves the entry again at cache time and downloads whatever its remote size then says. A replacement landing in between could push an oversized object through the download even though the post-cache check would refuse to read it. CacheRemoteObjectToLocalClusterRequest gains an optional max_size; the filer rejects an over-limit remote object before the download starts. loadMetadata passes metadataFileMaxBytes; data-file reads leave it unset. * iceberg maintenance: key the remote cache singleflight by the size limit Callers sharing one flight share its outcome, including a bounded caller's over-limit rejection: an unrestricted reader joining the same path would fail even though its own bound allows the download. The limit joins the key so only callers with the same bound share a flight. * iceberg: gofmt compact_order_test.go * filer: bound remote cache callers before the shared download Each caller's max_size is now enforced on the entry the filer resolves before that caller joins the singleflight, which is keyed by path again. A bounded caller's rejection is never shared with other callers, and callers with different bounds share one download instead of pulling the same remote object twice. * filer: bind the remote cache size bound to the downloaded entry The pre-flight check and the shared download resolve the entry separately, so a remote metadata refresh landing in between lets a larger object through: the download ran to the refreshed size and only the reader's post-check refused it. The bound is now re-checked inside the flight on the entry the download streams. The rejection is typed so a co-tenant sharing the flight does not inherit it: callers whose own bound permits the reported size retry on a fresh flight instead of failing for another caller's limit. --------- Co-authored-by: user.email <chrislusf@users.noreply.github.com>
2026-10-30 08:50:48 +01:00 · 2026-10-29 11:13:23 -07:00
1 parent 9df7d62b4b
commit 4be28fef2d
@@ -31,42 +31,74 @@ func (fs *FilerServer) CacheRemoteObjectToLocalCluster(ctx context.Context, req
 	// This benefits all clients: S3 API, filer HTTP, Hadoop, etc.
 	cacheKey := req.Directory + "/" + req.Name

+	// A caller's size bound is enforced on the entry it resolves, before it
+	// joins the shared download: the rejection stays with the bounded caller
+	// while allowed downloads still dedupe by path across all callers.
+	if req.MaxSize > 0 {
+		if entry, _, err := fs.fencedFindEntry(ctx, util.JoinPath(req.Directory, req.Name)); err == nil &&
+			entry.IsInRemoteOnly() && entry.Remote.RemoteSize > req.MaxSize {
+			return nil, &remoteObjectTooLargeError{path: cacheKey, size: entry.Remote.RemoteSize, limit: req.MaxSize}
+		}
+	}
+
 	// Detach from caller ctx: on failure the error path deletes every chunk
 	// already written, so cancelling mid-download loses all progress. For
 	// blobs large enough that the download outlasts the caller's timeout
 	// the retry loop never converges.
 	bgCtx := context.WithoutCancel(ctx)

-	// DoChan (vs Do) so the caller can bail out on ctx.Done() while the
-	// singleflight goroutine keeps caching on bgCtx; otherwise this handler
-	// goroutine stays blocked for the full download after the client is gone.
-	ch := fs.remoteCacheGroup.DoChan(cacheKey, func() (interface{}, error) {
-		return fs.doCacheRemoteObjectToLocalCluster(bgCtx, req)
-	})
+	for {
+		// DoChan (vs Do) so the caller can bail out on ctx.Done() while the
+		// singleflight goroutine keeps caching on bgCtx; otherwise this handler
+		// goroutine stays blocked for the full download after the client is gone.
+		ch := fs.remoteCacheGroup.DoChan(cacheKey, func() (interface{}, error) {
+			return fs.doCacheRemoteObjectToLocalCluster(bgCtx, req)
+		})

-	select {
-	case <-ctx.Done():
-		// Caller gave up; the detached cache keeps running and a later
-		// request will find the entry cached (or join the same singleflight).
-		return nil, ctx.Err()
-	case res := <-ch:
-		if res.Shared {
-			glog.V(2).Infof("CacheRemoteObjectToLocalCluster: shared result for %s", cacheKey)
+		select {
+		case <-ctx.Done():
+			// Caller gave up; the detached cache keeps running and a later
+			// request will find the entry cached (or join the same singleflight).
+			return nil, ctx.Err()
+		case res := <-ch:
+			if res.Shared {
+				glog.V(2).Infof("CacheRemoteObjectToLocalCluster: shared result for %s", cacheKey)
+			}
+			if res.Err != nil {
+				var tooLarge *remoteObjectTooLargeError
+				if errors.As(res.Err, &tooLarge) && (req.MaxSize == 0 || tooLarge.size <= req.MaxSize) {
+					// The shared flight ran under a co-tenant's tighter bound; this
+					// caller's own bound allows the object, so it starts a fresh
+					// flight rather than share that rejection.
+					continue
+				}
+				return nil, cacheRemoteObjectError(res.Err)
+			}
+			if res.Val == nil {
+				return nil, fmt.Errorf("unexpected nil result from singleflight")
+			}
+			resp, ok := res.Val.(*filer_pb.CacheRemoteObjectToLocalClusterResponse)
+			if !ok {
+				return nil, fmt.Errorf("unexpected result type from singleflight")
+			}
+			return resp, nil
 		}
-		if res.Err != nil {
-			return nil, cacheRemoteObjectError(res.Err)
-		}
-		if res.Val == nil {
-			return nil, fmt.Errorf("unexpected nil result from singleflight")
-		}
-		resp, ok := res.Val.(*filer_pb.CacheRemoteObjectToLocalClusterResponse)
-		if !ok {
-			return nil, fmt.Errorf("unexpected result type from singleflight")
-		}
-		return resp, nil
 	}
 }

+// remoteObjectTooLargeError reports that the entry a shared download resolved
+// exceeds a bound. It travels through the singleflight result, so callers
+// whose own bound permits the size retry instead of inheriting the rejection.
+type remoteObjectTooLargeError struct {
+	path  string
+	size  int64
+	limit int64
+}
+
+func (e *remoteObjectTooLargeError) Error() string {
+	return fmt.Sprintf("remote object %s is %d bytes, over the %d byte limit", e.path, e.size, e.limit)
+}
+
 // doCacheRemoteObjectToLocalCluster performs the actual caching operation.
 // This is called from singleflight, so only one instance runs per object.
 func (fs *FilerServer) doCacheRemoteObjectToLocalCluster(ctx context.Context, req *filer_pb.CacheRemoteObjectToLocalClusterRequest) (*filer_pb.CacheRemoteObjectToLocalClusterResponse, error) {
@@ -94,6 +126,12 @@ func (fs *FilerServer) doCacheRemoteObjectToLocalCluster(ctx context.Context, re
 		return resp, nil
 	}

+	// Enforce the bound on the entry this flight downloads: the caller's
+	// pre-flight check read an entry a remote metadata refresh can replace.
+	if req.MaxSize > 0 && entry.Remote.RemoteSize > req.MaxSize {
+		return nil, &remoteObjectTooLargeError{path: string(lockPath), size: entry.Remote.RemoteSize, limit: req.MaxSize}
+	}
+
 	glog.V(1).Infof("CacheRemoteObjectToLocalCluster: caching %s/%s (remote size: %d)", req.Directory, req.Name, entry.Remote.RemoteSize)

 	readDir, readName := fs.filer.RemoteReadPath(entry, req.Directory, req.Name)
@@ -227,3 +227,69 @@ func TestPruneOnANonOwnerLeavesTheDeleteToTheOwner(t *testing.T) {
 	assert.False(t, fs.pruneEntryMissingFromRemote(context.Background(), entry.FullPath, entry, remote_storage.ErrRemoteObjectNotFound))
 	assert.Contains(t, store.entries, string(entry.FullPath), "only the write owner may delete the entry")
 }
+
+func pollFindCalls(store *renameTestStore, path string, n int) bool {
+	for i := 0; i < 4000; i++ {
+		store.mu.Lock()
+		calls := store.findCalls[path]
+		store.mu.Unlock()
+		if calls >= n {
+			return true
+		}
+		time.Sleep(time.Millisecond)
+	}
+	return false
+}
+
+func awaitFindCalls(t *testing.T, store *renameTestStore, path string, n int) {
+	t.Helper()
+	require.True(t, pollFindCalls(store, path, n), "entry %s saw fewer than %d finds", path, n)
+}
+
+// A remote metadata refresh can swap the entry between a caller's pre-flight
+// check and the lookup the shared download resolves: the bound must apply to
+// the latter, and a co-tenant sharing the flight must not inherit a rejection
+// its own bound does not call for.
+func TestCacheBoundAppliesToTheDownloadedEntry(t *testing.T) {
+	fs, store, _ := newRemoteMountTestServer(t)
+	store.findDelay = 500 * time.Millisecond
+
+	small := remoteOnlyFileEntry("/other/obj.bin")
+	small.Remote.RemoteSize = 4
+	store.entries[string(small.FullPath)] = small
+
+	big := small.ShallowClone()
+	big.Remote = proto.Clone(small.Remote).(*filer_pb.RemoteEntry)
+	big.Remote.RemoteSize = 100
+	go func() {
+		if !pollFindCalls(store, string(small.FullPath), 1) {
+			return
+		}
+		store.mu.Lock()
+		store.entries[string(small.FullPath)] = big
+		store.mu.Unlock()
+	}()
+
+	bounded := make(chan error, 1)
+	go func() {
+		_, err := fs.CacheRemoteObjectToLocalCluster(context.Background(), &filer_pb.CacheRemoteObjectToLocalClusterRequest{
+			Directory: "/other", Name: "obj.bin", MaxSize: 5,
+		})
+		bounded <- err
+	}()
+
+	awaitFindCalls(t, store, string(small.FullPath), 1)
+	time.Sleep(80 * time.Millisecond) // let the bounded flight reach its own find
+	_, joinErr := fs.CacheRemoteObjectToLocalCluster(context.Background(), &filer_pb.CacheRemoteObjectToLocalClusterRequest{
+		Directory: "/other", Name: "obj.bin",
+	})
+
+	err := <-bounded
+	var tooLarge *remoteObjectTooLargeError
+	require.ErrorAs(t, err, &tooLarge)
+	assert.Equal(t, int64(100), tooLarge.size)
+	assert.Equal(t, int64(5), tooLarge.limit)
+
+	require.Error(t, joinErr)
+	assert.False(t, errors.As(joinErr, &tooLarge), "unbounded joiner retries rather than sharing the rejection")
+}