fix: close messagebus client before main() returns on shutdown - #870
fix: close messagebus client before main() returns on shutdown#870JarbasAl wants to merge 2 commits into
Conversation
skill_manager.shutdown() never closed the MessageBusClient, so the daemon thread started by bus.run_in_thread() stayed alive after main() returned. Any message it received during interpreter teardown could still call emitter.emit() -> executor.submit() on the bus client's pyee ExecutorEventEmitter after that executor was torn down, raising "cannot schedule new futures after shutdown" and leaving the process unable to exit cleanly on SIGTERM. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The bots have finished their work. Take a look! 🤖I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthI've performed a holistic audit of your project's soul. 🧘 ✅ All required files present. Latest Version: ✅ 🔎 Type CheckProcessing complete! Details follow. 📬 ❌ mypy: 300 error(s) found
Errors (showing first 10/300)📚 DocsA quick update on the status of your PR. 🔔 ✅ All required documentation files present. ✅ 🔒 Security (pip-audit)Shields up! Scanning for potential threats. 🛡️ ✅ No known vulnerabilities found (113 packages scanned). 📊 CoverageHow deep does the testing rabbit hole go? 🐇 Files below 80% coverage (8 files)
Full report: download the 🔌 Plugin DetectionIs this plugin ready for its debut in the Manager? 🎭 ❌ Plugin Status: ERRORS (1) Plugin Info:
OPM Detection:
Entry Point Validation:
⊘ No Issues:
🔌 Skill Tests (ovoscope)I've put the skill through its paces with live intent matching. 🏃 ✅ 13/37 passed ❌ **TestAdaptIntent** — 0/4
❌ **TestCancelIntentMidSentence** — 0/1
❌ **TestConverse** — 0/1
❌ **TestDeactivate** — 2/3
❌ **TestFallback** — 0/1
❌ **TestIntentPipelineRouting** — 0/4
❌ **TestLangDisambiguation** — 0/4
❌ **TestLegacyIntentIdBackCompat** — 0/2
❌ **TestNoSkills** — 0/2
❌ **TestPadatiousIntent** — 0/4
🚌 Bus CoverageChecking the insulation of our event handlers. 🏠 🔴 Coverage Summary
📊 Per-Skill Breakdown
🔍 Detailed Message Type Breakdown
|
| Python | Build | Install | Tests |
|---|---|---|---|
| 3.10 | ✅ | ✅ | ✅ |
| 3.11 | ✅ | ✅ | ✅ |
| 3.12 | ✅ | ✅ | ✅ |
| 3.13 | ✅ | ✅ | ✅ |
| 3.14 | ✅ | ✅ | ✅ |
⚖️ License Check
Ensuring our licenses are consistent and clear. 📄
✅ No license violations found.
Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed.
🌍 Locale Build
Another check completed successfully! 🏁
✅ Locale properly configured (64 files, 17 languages)
Locale directories found:
ovos_core/intent_services/locale
Localization coverage:
ovos_core/intent_services/locale: 64 files in 17 languages (en-us, ca-es, fa-ir, de-de, gl-es...)
pyproject.toml: ✅ [tool.setuptools.package-data.ovos_core] includes locale
intent_services/locale/*/*.voc
Build manifest: ✅ 31 locale files included in package
Processing... Done! Have a productive day! ☕
bus.close() is fire-and-forget: it signals run_forever to stop and closes the socket, but does not wait for the receiver thread spawned by bus.run_in_thread() to actually exit. Capture that thread and join it with a bounded timeout after close(), narrowing (not eliminating) the window where a buffered inbound frame can still hit the emitter's executor during interpreter teardown. Soften the code comment and test docstring accordingly: this is not a synchronous guarantee, since the join can time out. Verified ovos_bus_client.MessageBusClient.run_in_thread() returns the Thread object in the installed version. Updated the regression test to mock the returned thread and assert join() is called with a timeout, alongside the existing close() assertion; confirmed it fails without the join and passes with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This addresses the shutdown hang seen on
systemctl restartof ovos-core, where the service sits indeactivating (stop-sigterm)for 60+ seconds until systemd sends SIGKILL, withRuntimeError: cannot schedule new futures after shutdownin the logs. TheMessageBusClientused by the skills service is never closed anywhere inovos_core/__main__.py.bus.run_in_thread()starts a daemon thread that keeps receiving messages and dispatching them throughbus.emitter, which is apyee.ExecutorEventEmitterbacked by aThreadPoolExecutor. That thread is still running whenmain()returns and the interpreter starts tearing down, so a message arriving during that window can still callemitter.emit()->executor.submit()after the executor is gone, which is exactly the error text seen on the box.Sibling OVOS services (
ovos-dinkum-listener,mycroft-classic-listener,ovos-workshop'sOVOSAbstractApplication) all callbus.close()as part of their shutdown sequence;ovos-core's skills service was the one place that skipped it. The fix addsbus.close()right afterskill_manager.shutdown()inmain(), and now also captures the thread returned bybus.run_in_thread()and joins it with a bounded timeout.bus.close()alone is fire-and-forget: it signals the run_forever loop to stop and closes the socket, but does not wait for the receiver thread to exit. The join narrows the residual race further but is not a synchronous guarantee — it can itself time out, so this reduces rather than eliminates the window where a buffered inbound frame is dispatched during teardown.I added a regression test in
test/unittests/test_main.pythat mocks the bus (and the thread object it returns) and assertsbus.close()andbus_thread.join()(with a timeout) are both called beforemain()returns. I checked the join assertion fails against the code before this change and passes after.Full local
test/unittestssuite: 424 passed, 6 xfailed, no new failures, on a throwaway uv venv (--prerelease=allow) against ovos-core dev plus latest prerelease deps.