Introduce pausable service manager with tests#3335
Conversation
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| def release(self, name: str) -> None: | ||
| """Decrease reference count and pause service when unused.""" | ||
| with self._lock: | ||
| entry = self._services.get(name) | ||
| if not entry: | ||
| return | ||
| svc, count = entry | ||
| count -= 1 | ||
| if count <= 0: | ||
| self._services[name] = (svc, 0) | ||
| svc.pause() | ||
| else: | ||
| self._services[name] = (svc, count) |
There was a problem hiding this comment.
[P1] Pause services even when references remain
The new ServiceManager.release pauses the service regardless of the remaining reference count. When a service is acquired by multiple clients, the first release call decrements the count to >0 but still invokes svc.pause(), leaving the thread suspended while other clients still expect it to run. Those clients never resume the service because they already hold their reference, so their work stalls until they call acquire again. Pausing should only occur when the reference count reaches zero.
Useful? React with 👍 / 👎.
Summary
tools.service_managerto pause and resume service threads instead of killing themTesting
pytest tests/thread_manager/test_service_manager.py -qpytest --maxfail=1 -q(fails: tests/governance/test_governance_cut_paste.py::test_cut_paste_task_between_governance_diagrams)python tools/metrics_generator.py --path . --output metrics.jsonhttps://chatgpt.com/codex/tasks/task_b_68b19f73453483279b7a5aaa9935c7c8