Fix BuildPlanExecutor thread pool leak on constructor failure

[ulofiai]

Fixes #12599.

Build the initial plan before creating the PhasingExecutor. If plan construction throws, BuildContext now fails before allocating the executor, so cleanup cannot be skipped.

The change is limited to the initialization order in BuildPlanExecutor.

[gnodet]

Verdict: APPROVE ✅

Correct and minimal fix for a real thread pool resource leak. Reordering the executor creation to after buildInitialPlan() ensures that if plan initialization throws, no thread pool is leaked because it has not yet been allocated.

Key verification:

• buildInitialPlan() does not reference this.executor, so the reordering is safe and preserves identical behavior on the success path.
• Since BuildContext is used in a try-with-resources and Java only calls close() after a constructor completes successfully, the pre-fix code leaked the thread pool on any exception from buildInitialPlan().

Clean, surgical fix.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

APPROVE — Correct and minimal fix for a real thread pool resource leak.

Reordering executor creation to after buildInitialPlan() ensures no thread pool is allocated if plan construction throws, with no behavioral change on the success path.

Analysis:

• The bug is real: on master, the BuildContext constructor creates a PhasingExecutor (wrapping Executors.newFixedThreadPool) before calling buildInitialPlan(). Since the constructor is invoked inside execute()'s try-with-resources, a throw from buildInitialPlan() means the constructor never completes, Java never calls close(), and the thread pool leaks indefinitely.
• The reorder is safe: buildInitialPlan() does not reference this.executor, either directly or transitively. It only reads this.session (already assigned), outer-class fields, and the taskSegments parameter.
• No null-safety concern: close() does this.executor.close() without a null check, but close() is only reachable via try-with-resources after a successful constructor, so executor is always non-null at that point.

Minor observations (non-blocking, pre-existing):

1. buildInitialPlan() redundantly recalculates nThreads and calls session.setParallel(), duplicating the constructor setup.
2. close() has no null guard on this.executor, which would NPE if invoked on the unused no-arg constructor. Theoretical only.

This review was generated by an AI agent (Claude Code) and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet