Revert "db/snapshotsync/freezeblocks: run blocks snapshot merge off the shared build semaphore" (erigontech#22071)

Reverts erigontech#21526
because found races:
erigontech#22045 (comment)
and erigontech#21960

Author: AskAlexSharov

cmd/utils/app/snapshots_cmd.go

@@ -3511,16 +3511,6 @@ func doRetireCommand(cliCtx *cli.Context, dirs datadir.Dirs) error {
 		return err
 	}
 
-	for {
-		merged, err := br.MergeBlocks(ctx, log.LvlInfo, downloader.NoopSeederClient{})
-		if err != nil {
-			return err
-		}
-		if !merged {
-			break
-		}
-	}
-
 	if err := br.RemoveOverlaps(nil); err != nil {
 		return err
 	}

db/services/interfaces.go

@@ -127,7 +127,6 @@ type BlockRetire interface {
 	BuildMissedIndicesIfNeed(ctx context.Context, logPrefix string, notifier DBEventNotifier) error
 	SetWorkers(workers int)
 	GetWorkers() int
-	WaitForMerges(ctx context.Context)
 }
 
 type DBEventNotifier interface {

db/snapshotsync/freezeblocks/block_retire_merge_test.go

@@ -140,10 +140,6 @@ func chooseSegmentEnd(from, to uint64, snapType snaptype.Enum, snCfg *snapcfg.Cf
 type BlockRetire struct {
 	maxScheduledBlock  atomic.Uint64
 	working            atomic.Bool
-	merging            atomic.Bool
-	mergeWg            sync.WaitGroup
-	mergeMu            sync.Mutex
-	mergeClosing       bool
 	lastRetireGapStart atomic.Uint64
 
 	// shared semaphore with AggregatorV3 to allow only one type of snapshot building at a time
@@ -334,7 +330,8 @@ func (br *BlockRetire) retireBlocks(
 		}
 	}
 
-	return ok, nil
+	merged, err := br.MergeBlocks(ctx, lvl, seeder)
+	return ok || merged, err
 }
 
 func (br *BlockRetire) MergeBlocks(
@@ -439,18 +436,16 @@ func (br *BlockRetire) RetireBlocksInBackground(
 		defer onDone()
 		defer br.working.Store(false)
 
-		// Dump under the shared semaphore: the build phase is fast and is
-		// serialized against state-snapshot building to bound concurrent I/O.
-		err := func() error {
-			if br.snBuildAllowed != nil {
-				//we are inside own goroutine - it's fine to block here
-				if err := br.snBuildAllowed.Acquire(ctx, 1); err != nil {
-					return err
-				}
-				defer br.snBuildAllowed.Release(1)
+		if br.snBuildAllowed != nil {
+			//we are inside own goroutine - it's fine to block here
+			if err := br.snBuildAllowed.Acquire(ctx, 1); err != nil {
+				br.logger.Warn("[snapshots] retire blocks", "err", err)
+				return
 			}
-			return br.RetireBlocks(ctx, minBlockNum, maxBlockNum, lvl, seeder, onFinishRetire)
-		}()
+			defer br.snBuildAllowed.Release(1)
+		}
+
+		err := br.RetireBlocks(ctx, minBlockNum, maxBlockNum, lvl, seeder, onFinishRetire)
 		if errors.Is(err, heimdall.ErrHeimdallDataIsNotReady) {
 			br.borDataNotReadyBefore = time.Now().Add(BorDataNotReadyTimeout)
 			br.logger.Debug("[snapshots] bor data is not ready to be retired", "nextAttemptAt", br.borDataNotReadyBefore)
@@ -464,54 +459,11 @@ func (br *BlockRetire) RetireBlocksInBackground(
 			br.logger.Error("[snapshots] retire blocks", "err", err)
 			return
 		}
-
-		// Merge runs without the semaphore: it is slow and expensive, and
-		// holding the semaphore across it stalls state-snapshot collation/prune
-		// (which bounds chaindata size). Mirrors the aggregator's MergeLoop.
-		br.mergeBlocksInBackground(ctx, lvl, seeder)
 	}()
 
 	return true
 }
 
-// mergeBlocksInBackground runs block-snapshot merges off the shared build
-// semaphore. At most one merge runs at a time; a request arriving while one is
-// in flight is dropped and picked up by a later retire cycle.
-func (br *BlockRetire) mergeBlocksInBackground(ctx context.Context, lvl log.Lvl, seeder downloader.SeederClient) {
-	br.mergeMu.Lock()
-	defer br.mergeMu.Unlock()
-	// Once WaitForMerges has begun draining we must not start (and Add) a new merge.
-	if br.mergeClosing {
-		return
-	}
-	if !br.merging.CompareAndSwap(false, true) {
-		return
-	}
-	br.mergeWg.Add(1)
-	go func() {
-		defer br.mergeWg.Done()
-		defer br.merging.Store(false)
-		if _, err := br.MergeBlocks(ctx, lvl, seeder); err != nil {
-			br.logger.Error("[snapshots] merge blocks", "err", err)
-		}
-	}()
-}
-
-// WaitForMerges prevents new background merges from starting and waits for any
-// in-flight one to finish, or until ctx is done. Abandoning the wait is safe:
-// the merge touches neither chainDB nor the open snapshots.
-func (br *BlockRetire) WaitForMerges(ctx context.Context) {
-	br.mergeMu.Lock()
-	br.mergeClosing = true
-	br.mergeMu.Unlock()
-	done := make(chan struct{})
-	go func() { br.mergeWg.Wait(); close(done) }()
-	select {
-	case <-done:
-	case <-ctx.Done():
-	}
-}
-
 func (br *BlockRetire) RetireBlocks(
 	ctx context.Context,
 	requestedMinBlockNum uint64,

db/snapshotsync/freezeblocks/block_snapshots.go

@@ -1545,15 +1545,6 @@ func (s *Ethereum) Stop() error {
 		}
 	}
 
-	// Drain the fire-and-forget background block-snapshot merge goroutine for
-	// shutdown hygiene; the merge itself touches neither chainDB nor snapshots,
-	// so the wait is bounded.
-	if s.components != nil && s.components.Storage != nil && s.components.Storage.BlockRetire != nil {
-		mergeCtx, mergeCancel := context.WithTimeout(context.Background(), 30*time.Second)
-		s.components.Storage.BlockRetire.WaitForMerges(mergeCtx)
-		mergeCancel()
-	}
-
 	s.chainDB.Close()
 
 	if s.config.Downloader != nil {