execution/stagedsync: follow-ups for fee-merge write-set reclaim (#23099 / #23106)

[yperbasis]

Follow-ups from a review of #23106 (main pick of #23099, which is merged to release/3.6). That change moves the release of superseded fee-merge write sets off the hot loop into a background releaser (recordFeeMerge / queueMapRelease / mapReleases in execution/stagedsync/exec3_parallel.go).

The review confirmed the core of the change is safe: the superseded temp is unreachable once queued, queueing happens only on the exec loop, there is no double-release path, and the touched tests pass under -race. The items below are robustness, efficiency and convention follow-ups, roughly in priority order. Scope is main; the pool-level item was explicitly out of scope for 3.6.

Test shield (one-line fix)

• In exec3_fee_merge_temp_test.go, the round-1 negative assertion (txOut must survive the fee merge) is not shielded by be.awaitMapReleases(). If a future bug wrongly queues a live set for release, the release waits in the channel while the test reads Count(), so the test would still pass most of the time — exactly the regression class it exists to catch. Add be.awaitMapReleases() after the first recordFeeMerge; Wait on a zero counter returns immediately.

Memory retention and lifecycle

• Each queued mapRelease carries &be.mapReleasing. This interior pointer keeps the whole blockExecutor (results, tasks, blockIO, caches) reachable until the global releaser drains the entry, heaviest under memory pressure — and the WaitGroup has no production waiter, only the tests call awaitMapReleases. A separately allocated WaitGroup or a done-channel would pin only itself.
• The releaser is a process-global goroutine over a never-closed 4096-slot channel: no Stop/drain hook (unfriendly to goleak), and one serial consumer shared by every parallelExecutor in the process, so one storage-heavy release delays all instances. A per-executor releaser managed like the commitment calculator would give lifecycle, isolation and a natural drain point — or see the simplification below.

Efficiency

• When the channel is full, the fallback runs the O(entries) map clear inline on the exec loop — exactly when release traffic peaks (revalidation storms). Skipping the release and letting GC take the set (Done() only) is correctness-neutral and keeps the hot path flat; the stale-temp path after re-execution already GC-drops without releasing.
• The dominant release still runs synchronously on the serial apply loop: ReleaseOutputMaps walks every tx's write set for every block, and the root cost (vwMapPool clear-on-put with no size cap) sits in the state package where a fix would help all callers. A size cap for pooled maps or a cheaper clear may shrink or remove the need for the queue.

Robustness parity (future-conditional — verified not reachable today)

• The queued path runs ReleaseMaps on a bare goroutine without recover. A recoverable panic there kills the process, while the same panic inline is contained by the exec loop's deferred recover. A recover-and-log wrapper in the consumer restores parity between the two branches.
• WriteSet has no released-tripwire: a future use-after-supersede reader would hit a nondeterministic concurrent map read and map write throw instead of the deterministic empty reads it got before. The sibling ReleaseOutputMaps path guards this class with outputsReleased + assertOutputsLive under dbg.AssertEnabled; the fee-merge temps deserve the same guard.

Coverage gap (pre-existing, from #22883)

• The finalize-merge site also supersedes the just-recorded fee-merge temp (existingWrites replaced by the merged set) but is not routed through recordFeeMerge, so those temps are never pooled and the feeMergeTemp entry goes stale. The ownership argument is identical to the covered site.

Simplification (may resolve several items above at once)

• recordFeeMerge runs only on the exec loop, so a plain per-executor pending slice, drained at the existing block-end release point next to ReleaseOutputMaps, gives the same offload with zero new concurrency primitives — and makes the tests synchronous again, which also resolves the retention, lifecycle and test-shield items. Whether releasing pays at all versus a plain GC drop is still being measured (alex/exec_dbg_37 gates it behind a RELEASE_FEE_MERGE_MAPS switch for A/B); if GC-drop wins, the whole mechanism can go away.

Conventions

• The new comments say the release is kept off "the apply loop", but recordFeeMerge runs on the exec loop (processResults); the apply loop is the applyResults consumer. Fix the wording — the PR body itself has it right.
• The rationale comment for queueMapRelease is attached to type mapRelease and starts with a different symbol's name ("ReleaseMaps clears every map…"). Move it onto queueMapRelease; give the type a one-liner starting with its name, or none.
• The channel-full inline fallback and the WaitGroup handshake have no test — nothing in the tests ever fills mapReleases. A small test that fills the channel and asserts the inline release + Done would close the gap.

cc @AskAlexSharov

[VBulikov]

yperbasis added Imp2 as Importance