BUG: Fix ParallelSparseFieldLevelSet deadlock with PlatformMultiThreader (supersedes #6321)

[hjmjohnson]

Fixes the ParallelSparseFieldLevelSetImageFilter::Iterate() dispatch-starvation deadlock by forcing PlatformMultiThreader in the constructor (per @blowekamp's suggestion on #6321), rather than spawning explicit std::threads. Intended to supersede #6321.

⚠️ Stacked on #6320 (the robustness test). Merge #6320 first; this PR's only unique change is a 3-line constructor edit to itkParallelSparseFieldLevelSetImageFilter.hxx. After #6320 lands, rebase drops the test commit and the diff becomes just the fix.

Root cause and why this approach

Iterate() dispatches ThreadedApplyUpdate via mt->ParallelizeArray(); the body blocks in SignalNeighborsAndWait() until neighbor work units arrive. This neighbor-only protocol requires every work unit to hold its own concurrent OS thread for the full dispatch. Pool/TBB-backed ParallelizeArray queues N tasks onto a bounded worker set — CV-blocked tasks pin all workers, queued units never start, deadlock.

#6321 swapped only the one ParallelizeArray containing SignalNeighborsAndWait for explicit std::threads. @blowekamp noted that leaves other multi-threader/work-unit combinations potentially vulnerable, and suggested instead hard-coding PlatformMultiThreader to preserve the filter's original fixed-thread assumption — the same pattern itkSLICImageFilter uses:

this->DynamicMultiThreadingOff();
this->SetMultiThreader(PlatformMultiThreader::New());

PlatformMultiThreader::SingleMethodExecute spawns one dedicated OS thread per work unit (calling thread + N−1 spawned), and ParallelizeArrayHelper with range == work-unit count gives each thread exactly one index — so every ThreadedApplyUpdate/SignalNeighborsAndWait runs on its own concurrent thread. The neighbor-sync protocol and all data members are unchanged; the fix applies to every parallel section in the filter, not just one.

Local validation — 700 runs, 0 failures

Built with Module_ITKTBB=ON and exercised under both threaders:

Suite	Threader	Runs	Result
Robustness GTest (SweepRepeat + Determinism + ConcurrentMultiPipeline)	default (Pool)	×100	✅ 100/100, 0 failures
Robustness GTest	real TBB	×100	✅ 100/100, 0 failures
itkParallelSparseFieldLevelSetImageFilterTest (baseline image compare)	default	×100	✅ 100/100, 0 failures

700 total test executions, zero failures, zero deadlocks. Output bit-identical every run (the Determinism scenario and the MD5-pinned baseline both pass). Full ninja build (3345 targets) clean; pre-commit run --all-files clean.

Note: the original deadlock is timing-/core-count-dependent (needs work units > available TBB workers). On the 48-core validation host m_NumOfWorkUnits caps to ≤48, so the committed sweep does not starve there; the deterministic 100%-deadlock reproduction is on a 16-core box (see #6321) and CI's TBB legs.

[blowekamp]

LGTM

[greptile-apps]

Greptile Summary

Fixes the ParallelSparseFieldLevelSetImageFilter pool-starvation deadlock by forcing PlatformMultiThreader in the constructor — the same pattern used by itkSLICImageFilter — so every work unit gets its own dedicated OS thread, satisfying the neighbor-blocking inter-phase sync invariant.

• itkParallelSparseFieldLevelSetImageFilter.hxx: Three-line constructor addition (DynamicMultiThreadingOff() + SetMultiThreader(PlatformMultiThreader::New())) replaces the pool-backed default threader with one that spawns one OS thread per work unit.
• itkParallelSparseFieldLevelSetImageFilterRobustnessGTest.cxx: New GTest suite with SweepRepeat, Determinism, and ConcurrentMultiPipeline scenarios; uses a watchdog thread with std::abort() to turn a potential deadlock-hang into a timed test failure.
• CMakeLists.txt: Registers the new GTest binary via creategoogletestdriver; no explicit itk_add_test entries needed (auto-discovery handles registration).

Confidence Score: 3/5

The constructor fix is minimal and well-precedented; the main risk is in the new test infrastructure where an uncaught ITK exception would crash the entire GTest binary.

The three-line constructor change is clean and correct. The robustness test suite introduces a runWithDeadline helper that is not exception-safe: an uncaught ITK exception from body() leaves the watchdog thread unjoinable, causing std::terminate() to abort the test driver and silently drop all remaining test results.

itkParallelSparseFieldLevelSetImageFilterRobustnessGTest.cxx — the runWithDeadline exception-safety gap needs attention before CI can trust the test results.

Important Files Changed

Filename	Overview
Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx	3-line constructor change: DynamicMultiThreadingOff() + SetMultiThreader(PlatformMultiThreader::New()) fixes the pool-starvation deadlock; matches the established itkSLICImageFilter pattern. Public SetMultiThreader on the base class could silently undo the fix if called post-construction.
Modules/Segmentation/LevelSets/test/itkParallelSparseFieldLevelSetImageFilterRobustnessGTest.cxx	New GTest suite with SweepRepeat, Determinism, and ConcurrentMultiPipeline scenarios. runWithDeadline is not exception-safe: an uncaught ITK exception from body() leaves the watchdog thread unjoinable, causing std::terminate() to kill the entire test binary.
Modules/Segmentation/LevelSets/test/CMakeLists.txt	Adds creategoogletestdriver block for the new GTest file; follows existing ITK conventions with no explicit itk_add_test calls needed (gtest_discover_tests handles registration). Clean change.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Filter as ParallelSparseFieldLevelSetImageFilter
    participant PMT as PlatformMultiThreader
    participant T1 as WorkUnit Thread 1..N

    Caller->>Filter: New() [constructor]
    Filter->>Filter: DynamicMultiThreadingOff()
    Filter->>PMT: New()
    Filter->>Filter: SetMultiThreader(PMT)
    Caller->>Filter: Update()
    Filter->>PMT: ParallelizeArray(ThreadedApplyUpdate)
    PMT->>T1: Spawn N dedicated OS threads
    T1->>T1: ThreadedApplyUpdate + SignalNeighborsAndWait
    Note over T1: All N threads run concurrently
    T1-->>PMT: All threads complete
    PMT-->>Filter: Return
    Filter-->>Caller: Output ready

Loading

_{Reviews (1): Last reviewed commit: "BUG: Fix ParallelSparseFieldLevelSet dea..." | Re-trigger Greptile}

[greptile-apps]

runWithDeadline terminates the test driver on any pipeline exception

If body() throws (e.g., an itk::ExceptionObject from mf->Update()), the code after body() is never reached — done is never set, cv.notify_all() is never called, and watchdog.join() is never called. The watchdog destructor then invokes std::terminate() because the thread is still joinable, which kills the entire GTest binary. All subsequent test cases (Determinism, ConcurrentMultiPipeline) are silently lost rather than reported. A try/catch around body() that signals done and joins before rethrowing would let GTest report the failure normally.

Suggested change

	template <typename TFunctor>
	void
	runWithDeadline(unsigned int seconds, TFunctor && body)
	{
	std::mutex m;
	std::condition_variable cv;
	bool done = false;
	std::thread watchdog([&] {
	std::unique_lock<std::mutex> lk(m);
	if (!cv.wait_for(lk, std::chrono::seconds(seconds), [&] { return done; }))
	{
	std::cerr << "Deadline exceeded (" << seconds << "s): ParallelizeArray dispatch deadlock\n";
	std::abort();
	}
	});
	body();
	{
	const std::lock_guard<std::mutex> lk(m);
	done = true;
	}
	cv.notify_all();
	watchdog.join();
	}
	try
	{
	body();
	}
	catch (...)
	{
	{
	const std::lock_guard<std::mutex> lk(m);
	done = true;
	}
	cv.notify_all();
	watchdog.join();
	throw;
	}
	{
	const std::lock_guard<std::mutex> lk(m);
	done = true;
	}
	cv.notify_all();
	watchdog.join();
	}

[greptile-apps]

Post-construction SetMultiThreader silently re-introduces deadlock

SetMultiThreader is public on the base class, so a caller who runs filter->SetMultiThreader(TBBMultiThreader::New()) after construction replaces PlatformMultiThreader with a pool-backed threader and restores exactly the dispatch-starvation deadlock this PR fixes. Since the filter's synchronisation protocol mandates one dedicated OS thread per work unit, the constructor should at minimum document that overriding the threader breaks the filter — or note it in the class-level Doxygen rather than only in a constructor comment.

[greptile-apps]

File-level comment references a transient resource and exceeds the prose-budget cap

prose-budget.md caps in-source comments at one short line (≤ 100 chars) and explicitly forbids references to the PR ("See the PR for the full analysis") because the PR is a transient resource — "the link rots; the prose remains as noise." The second sentence and the "See the PR" cross-reference should be removed; the surviving single line should capture only the non-obvious invariant that the reader needs with the diff open.

Context Used: AGENTS.md (source)