fix(core): prevent worker metrics loss when cache_tracker init fails - #5071
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces changes to improve robustness and error handling during model registration and cache synchronization across the supervisor and workers. Key changes include adding defensive checks for _cache_tracker_ref being None, introducing a _registered flag to track worker registration state, and logging warnings instead of failing when auxiliary cache operations fail. However, two critical issues were identified in the review: first, suppressing failures in _sync_register_model on the supervisor can lead to inconsistent cluster states and silent launch failures, so registration should be rolled back and the error propagated; second, removing asyncio.to_thread from the requests.get call in worker.py blocks the main event loop, which could trigger heartbeat timeouts and falsely mark workers as dead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
5b6f888 to
27f7866
Compare
qinxuye
left a comment
There was a problem hiding this comment.
Two issues need cleanup before this can land.
Worker metrics (workers_total, _worker_status-based gauges) were persistently empty because get_supervisor_ref coupled add_worker registration with record_model_version cache sync in a single try block. When record_model_version failed, _clear_supervisor_refs wiped the supervisor_ref, so report_status never ran. Changes: - worker.py: track _registered flag separately from _supervisor_ref; move record_model_version into its own try block with defensive _cache_tracker_ref None guard; _clear_supervisor_refs resets _registered. - cache_tracker.py: replace AssertionError on version-count mismatch with a warning + skip, so cache sync no longer crashes the worker. - supervisor.py: isolate cache_tracker.record_model_version and _sync_register_model into separate try blocks so auxiliary-cache failures don't roll back the already-successful local register_fn. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
27f7866 to
51e7650
Compare
Summary
workers_total,_worker_status-based gauges) were persistently empty becauseget_supervisor_refcoupledadd_workerregistration withrecord_model_versioncache sync inside a single try block. Whenrecord_model_versionraised,_clear_supervisor_refswipedself._supervisor_ref, soreport_status/heartbeatcould never send worker status — even though the supervisor sawworkers_total=4._registeredflag, cache sync is isolated in its own try block and treated as an auxiliary feature whose failure must not tear down the worker↔supervisor channel.cache_trackerso a version-count mismatch between supervisor and worker becomes a logged skip rather than anAssertionErrorthat aborts the whole sync path.record_model_versioninsupervisor.register_model/worker.register_modelso an auxiliary-cache failure does not roll back the already-successfulregister_fn.register_fnfailures do roll back viaunregister_fn(preserving the origin/main safety guarantee), and_sync_register_modelfailures do roll back + propagate (preserving cluster-state consistency).Changes
xinference/core/worker.pyself._registered: boolflag tracking whethersupervisor.add_worker()has succeeded.get_supervisor_refcache-hit path returns early only when no registration is required, or the worker is already registered; otherwise falls through to runadd_worker.record_model_versionmoved into its own try block with a defensive_cache_tracker_ref is Noneguard; failures are logged as warnings, not propagated._clear_supervisor_refsresets_registeredto keep it in sync with the_supervisor_reflifecycle.register_model:register_fnfailure rolls back viaunregister_fn(restored per maintainer feedback);record_model_versionfailure is warning-only.xinference/core/cache_tracker.pyrecord_model_version: replaceAssertionErroron version-count mismatch with alogger.warning+continue, so cache sync degrades instead of crashing.xinference/core/supervisor.pyregister_model: split the single try block into (1) localregister_fn(rolls back on failure), (2)record_model_versioncache sync (warning-only, no rollback), (3)_sync_register_modelworker propagation (rolls backregister_fnand re-raises on failure).xinference/core/tests/test_worker.pyset_supervisor_ref_for_testnow also sets_registered = Trueso tests that inject a fake supervisor ref (without going throughadd_worker) still hit the cache-hit path inget_supervisor_ref(add_worker=True).Root cause
get_supervisor_refpreviously did:So any failure in
record_model_version(e.g. transientAssertionErrorfrom a version-count mismatch on a re-reporting worker) cleared_supervisor_ref, blockingheartbeatandreport_statusfor the worker.workers_totalwould still count the worker (becauseadd_workerhad already run), but every gauge sourced from_worker_statuswould stay empty.Test plan
black --check/flake8/isorton modified files — clean.pytest xinference/core/tests/test_worker.py -k test_report_status— verifies the injected-ref cache-hit path works with_registered=True. (Cannot run locally due to missingtorchvision; CI will validate.)get_supervisor_refwithadd_worker=Truecallssupervisor.add_workerexactly once across retries whenrecord_model_versionraises._cache_tracker_ref is Nonedoes not raiseAttributeErrorfromget_supervisor_ref.cache_tracker.record_model_versionwith mismatched version counts logs a warning and continues (noAssertionError).supervisor.register_model/worker.register_modelrolls back local registration (callsunregister_fn) and re-raises whenregister_fnfails.supervisor.register_modelrolls back and re-raises when_sync_register_modelfails.cache_tracker; verify worker status gauges stay populated and supervisor logs a warning rather than dropping the worker.Dependencies
None. This PR is independent of the dashboard-split work tracked separately.
🤖 Generated with Claude Code