fix: /healthz and /readyz wait for initial device connection (SOFIE-4325)#1730
Merged
Conversation
jstarpl
added
🐛🔨 bugfix
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughMosHandler startup now awaits MOS connection, registers coreHandler.onConnected, sets up observers, and schedules a debounced device refresh via triggerUpdateDevices(). ChangesInitialization & debounce refactor
sequenceDiagram
participant MosHandler
participant MOSConnection
participant CoreHandler
participant UpdateDevices
MosHandler->>MOSConnection: _initMosConnection()
MOSConnection-->>MosHandler: connected
MosHandler->>CoreHandler: register onConnected callback
MosHandler->>MosHandler: setupObservers()
MosHandler->>MosHandler: triggerUpdateDevices() (debounced)
MosHandler->>UpdateDevices: _updateDevices() (scheduled after debounce)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/mos-gateway/src/mosHandler.ts (2)
83-83: Non-null assertion makes existing_coreHandlerundefined-checks dead code.With
_coreHandler!: CoreHandlerthe field can no longer be typedundefined, so theif (!this._coreHandler) throw …branches at lines 156-158, 294, 453-455, and 599 are unreachable per the type system. They're harmless, but worth removing in a follow-up to keep the runtime invariant aligned with the type. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mos-gateway/src/mosHandler.ts` at line 83, The field _coreHandler is declared with a non-null assertion but code contains runtime undefined checks, so make the type reflect that by changing the declaration from "_coreHandler!: CoreHandler" to "private _coreHandler?: CoreHandler" so existing guards (the if (!this._coreHandler) throw ...) remain meaningful; update any call sites that currently rely on non-null typing to either keep the guards or use explicit non-null assertions where you truly know it is initialized (e.g., inside methods that run after initialization) so the type and runtime behavior stay aligned.
209-218: Clear_triggerUpdateDevicesTimeoutondispose().The deferred update is now scheduled via
setTimeoutand stored on the instance, butdispose()(lines 141-148) never clears it. The_disposedcheck inside_updateDevices()does prevent work from happening, but the pending timer still keeps the event loop alive and holds a closure reference tothis. Cheap to fix:♻️ Proposed cleanup
async dispose(): Promise<void> { this._disposed = true + if (this._triggerUpdateDevicesTimeout) { + clearTimeout(this._triggerUpdateDevicesTimeout) + this._triggerUpdateDevicesTimeout = null + } if (this.mos) { return this.mos.dispose() } else { return Promise.resolve() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mos-gateway/src/mosHandler.ts` around lines 209 - 218, The instance schedules a deferred timer in triggerUpdateDevices (stores it in _triggerUpdateDevicesTimeout) but dispose() does not clear it, leaving the timer alive and holding a reference to this; update dispose() to clearTimeout(this._triggerUpdateDevicesTimeout) and then null/undefined the property (or otherwise reset it) so any pending timer is cancelled, and ensure triggerUpdateDevices still overwrites the field when scheduling a new timer; reference functions/fields: triggerUpdateDevices, _triggerUpdateDevicesTimeout, dispose, and _updateDevices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/mos-gateway/src/mosHandler.ts`:
- Around line 132-139: The ordering regression causes setupObservers() to run
before _initMosConnection() and can NPE when _deviceOptionsChanged() calls
this.mos.setDebug; fix by guarding _deviceOptionsChanged() to tolerate a missing
mos (e.g. early-return or defer calling this.mos.setDebug if this.mos is
undefined) or by moving setupObservers() to run only after
_initMosConnection()/_updateDevices() has initialized this.mos; update the code
paths in setupObservers, triggerUpdateDevices, _deviceOptionsChanged,
_updateDevices and _initMosConnection so observers never call this.mos methods
when this.mos is undefined (prefer adding a simple if (!this.mos) return or
queue the change until after _initMosConnection completes).
---
Nitpick comments:
In `@packages/mos-gateway/src/mosHandler.ts`:
- Line 83: The field _coreHandler is declared with a non-null assertion but code
contains runtime undefined checks, so make the type reflect that by changing the
declaration from "_coreHandler!: CoreHandler" to "private _coreHandler?:
CoreHandler" so existing guards (the if (!this._coreHandler) throw ...) remain
meaningful; update any call sites that currently rely on non-null typing to
either keep the guards or use explicit non-null assertions where you truly know
it is initialized (e.g., inside methods that run after initialization) so the
type and runtime behavior stay aligned.
- Around line 209-218: The instance schedules a deferred timer in
triggerUpdateDevices (stores it in _triggerUpdateDevicesTimeout) but dispose()
does not clear it, leaving the timer alive and holding a reference to this;
update dispose() to clearTimeout(this._triggerUpdateDevicesTimeout) and then
null/undefined the property (or otherwise reset it) so any pending timer is
cancelled, and ensure triggerUpdateDevices still overwrites the field when
scheduling a new timer; reference functions/fields: triggerUpdateDevices,
_triggerUpdateDevicesTimeout, dispose, and _updateDevices.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ad57a02-ca62-4d6c-bc26-bb7fb90c58f3
📒 Files selected for processing (1)
packages/mos-gateway/src/mosHandler.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/mos-gateway/src/mosHandler.ts`:
- Around line 140-141: init() currently returns before the first
_updateDevices() runs and swallows _initMosConnection() failures, so
MosHandler.create() can appear successful while MOS is broken; update init() to
await the initial device update by returning/awaiting
this.triggerUpdateDevices() (or directly await this._updateDevices()) so init()
only resolves after the first successful update, and change the
_initMosConnection() error handling to rethrow or propagate errors instead of
only logging (so callers of init()/MosHandler.create() receive the failure).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1af185af-b884-4d8c-9a2b-ae15055f4973
📒 Files selected for processing (1)
packages/mos-gateway/src/mosHandler.ts
nytamin
approved these changes
May 18, 2026
jstarpl
force-pushed
the
contrib/mos-gw-init
branch
from
474e827 to
594ed23
Compare
Contributor
|
@jstarpl Not sure if this is an issue with our system but the MOS Gateway connections now show as disconnected all the time in the Sofie UI. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
About the Contributor
This pull request is posted on behalf of the NRK.
Type of Contribution
This is a:
Bug fix
Current Behavior
The Kubernetes
/healthzand/readyzdon't return an OK signal until devices configured at boot-up have connected.New Behavior
The device connection is detached from boot-up, since it's not really necessary for the boot-up process. This allows the instance to stay up instead of being restarted.
Testing
Affected areas
This PR affects the MOS Gateway bootup process.
Time Frame
Not urgent, but we would like to get this merged into the in-development release.
Other Information
Status