Scheduler: "bad group" producer message is a hang guard, not a diagnostic

[dcwhite]

The message at WorkUnitProducer.h:119 still fires in real use, very infrequently:

producer is done with bad group, something went wrong. probably a race condition...

It dates to 47b5e39 (2015-01-30, "Mega threading improvement") and has never been chased down. Static analysis says it is not what it looks like.

What the message actually means

1. ParallelModuleExecutionOrder::minGroup() returns -1 only when the multimap is empty — group keys are topological distances and are never negative. GraphNetworkAnalyzer.cc:112 passes moduleCount_ as the explicit vertex count, so isolated modules are included. Empty map ⟺ the filtered graph has zero modules.

2. The producer's filter is context.addAdditionalFilter(ModuleWaitingFilter::Instance()). ModuleWaitingFilter is misnamed — it returns state != Completed, it never tests for Waiting.

Together: an empty graph means every module in scope is Completed, i.e. the run already finished successfully. The message is not reporting a scheduling failure.

Why the guard normally prevents it

enqueueReadyModules() is wrapped in if (!isDone()), where isDone() is doneCount_ >= numModules_, numModules_ = order.size() (a snapshot taken before the run) and doneCount_ counts distinct modules pushed. Enqueue strictly precedes execution, so when all modules are Completed, doneCount_ should already equal numModules_ and the body is skipped.

So the message firing implies doneCount_ < numModules_ permanently: a module counted in the pre-run order was never enqueued.

The part that matters

The producer loop is:

while (!badGroup_ && !isDone())

If doneCount_ can never reach numModules_, isDone() is never true and badGroup_ is the only exit. The producer thread is join()ed in run().

badGroup_ is currently acting as the termination condition that prevents a hang, not as a diagnostic. Removing the check or the message without first fixing the count mismatch would turn a rare warning into a rare deadlock.

Contributing ordering

Module.cc:492 transitions the module to Completed, but executeEnds_ — which calls enqueueReadyModules() — does not fire until Module.cc:503. A module is therefore observably Completed before the producer is told. schedule() reads all module states with no lock at all; enqueueLock_ guards only the counters. There is an existing comment directly above at Module.cc:491:

//TODO: brittle dependency on Completed with executor

Proposed work

• Diagnostic first. Extend the logCritical at WorkUnitProducer.h:119 to print doneCount_, numModules_ and doneIds_.size(). A constant gap of 1 points to one specific module being skipped; a varying gap points to the doneIds_ "wants to enqueue a second time" branch (~line 85), which declines to push and declines to increment doneCount_.
• Fix the data race. mutable bool badGroup_ (~line 134) is written under enqueueLock_ at line 73 but read unlocked at lines 112/118, while enqueueReadyModules() is invoked from worker threads via WorkUnitExecutor.h:52. Make it std::atomic<bool>. It currently works only because the seq_cst load of doneCount_ in the same loop condition prevents hoisting. Consider modernizing the neighbouring boost::atomic<int> doneCount_ to std::atomic<int> in the same pass.
• Fix the real count mismatch once the diagnostic identifies it, and only then reconsider the badGroup_ exit.
• Rename ModuleWaitingFilter to something honest such as ModuleNotCompletedFilter.

Verify against the scheduler unit tests in src/Dataflow/Engine/Scheduler/Tests/, especially SchedulingWithBoostGraph.cc.

Adjacent, lower priority

Noticed while tracing this, not necessarily part of the fix:

• In DynamicMultithreadedNetworkExecutor::execute, threadGroup_->clear() runs on the caller's thread outside executionLock_, while a previous run() may still be inside joinAll().
• static Mutex lock("live-scheduler") in the same function is function-static, shared across every network and every execution.