ModelEngine-Group / ModelEngine-Group/nexent

GlobalThreadPool singleton: unsynchronised `__new__` and silently ignored `max_workers`

Open
#3,820 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
5.9k
Forks
731
Avg merge
19h 34m
Merged PRs (30d)
172

Description

backend/utils/thread_utils.py defines a process-wide thread pool used to fan out async work:

class GlobalThreadPool:
    _instance = None

    def __new__(cls, max_workers=10):
        if cls._instance is None:
            cls._instance = super().__new__(cls)
            cls._instance.pool = ThreadPoolExecutor(max_workers=max_workers)
            atexit.register(cls._instance.pool.shutdown)
        return cls._instance

Two distinct problems here:

1. The singleton is not thread-safe. Two threads racing into GlobalThreadPool(...) before _instance is set can both pass the if cls._instance is None check, each construct a ThreadPoolExecutor, and one of them silently overwrites the other. The discarded executor leaks until shutdown (and its atexit shutdown still fires against a dangling reference). Compare with AgentRunManager in backend/agents/agent_run_manager.py:13-21 which correctly uses a class-level lock.

2. max_workers is silently ignored after first construction. Anywhere in the codebase, GlobalThreadPool(max_workers=20) returns the previously-constructed instance with the old size. Today line 19 creates the singleton with max_workers=5, but the default argument is max_workers=10 — so even reading the file gives no obvious answer to "what is the pool size?". Any caller passing a different value will be confused when it has no effect.

Suggested fix
import threading

class GlobalThreadPool:
    _instance = None
    _lock = threading.Lock()

    def __new__(cls, max_workers=10):
        if cls._instance is None:
            with cls._lock:
                if cls._instance is None:
                    instance = super().__new__(cls)
                    instance.pool = ThreadPoolExecutor(max_workers=max_workers)
                    atexit.register(instance.pool.shutdown)
                    cls._instance = instance
        elif max_workers != cls._instance.pool._max_workers:
            logger.warning(
                "GlobalThreadPool already initialised with max_workers=%d; "
                "requested %d will be ignored",
                cls._instance.pool._max_workers, max_workers,
            )
        return cls._instance

Or — cleaner — drop the singleton-via-__new__ pattern in favour of a module-level instance constructed once at import time.

Severity: Medium (resource leak + footgun, not a correctness bug for the current single import site).

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with backend/utils/thread_utils.py and inspect the GlobalThreadPool construction and its current call at line 19. Compare the initialization approach with the class-level lock in backend/agents/agent_run_manager.py:13-21, then check callers for differing max_workers values. Done means concurrent initialization no longer creates a discarded executor and the handling of later max_workers requests is explicit and covered by relevant tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.