Skip to content

fix: /healthz and /readyz wait for initial device connection (SOFIE-4325) #1730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jstarpl wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: /healthz and /readyz wait for initial device connection (SOFIE-4325) #1730

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d126baa
feat(mos-gateway): move synchronous updateDevices out of init
jstarpl Apr 27, 2026
de1fece
Merge branch 'main' into contrib/mos-gw-init
jstarpl Apr 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 11 additions & 12 deletions packages/mos-gateway/src/mosHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ export class MosHandler {
private _logger: Winston.Logger
private _disposed = false
private _settings?: MosGatewayConfig
private _coreHandler: CoreHandler | undefined
private _coreHandler!: CoreHandler
private _observers: Array<Observer<any>> = []
private _triggerupdateDevicesTimeout: any = null
private _triggerUpdateDevicesTimeout: any = null
private mosTypes: MosTypes

public static async create(
Expand Down Expand Up @@ -119,9 +119,6 @@ export class MosHandler {
}
}
*/
if (!coreHandler) {
throw Error('coreHandler is undefined!')
}

if (!coreHandler.core) {
throw Error('coreHandler.core is undefined!')
Expand All @@ -133,15 +130,14 @@ export class MosHandler {

this.mosTypes = getMosTypes(this.strict)

await this._updateDevices()

if (!this._coreHandler) throw Error('_coreHandler is undefined!')
this._coreHandler.onConnected(() => {
coreHandler.onConnected(() => {
// This is called whenever a connection to Core has been (re-)established
this.setupObservers()
this.sendStatusOfAllMosDevices()
})
this.setupObservers()

this.triggerUpdateDevices()
Comment on lines +133 to +140
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Ordering regression: setupObservers() now runs before _initMosConnection() and can NPE on this.mos.

Previously init() awaited _updateDevices() (which awaits _initMosConnection()), so this.mos was always defined before any observer/_deviceOptionsChanged could run. After this change, the call order in init() is:

  1. setupObservers() (line 137) — synchronously invokes _deviceOptionsChanged() at line 172.
  2. triggerUpdateDevices() (line 139) — schedules _updateDevices() (and therefore _initMosConnection()) ~20 ms later.

If the peripheral device's deviceSettings.debugLogging differs from the default this.debugLogging = false, _deviceOptionsChanged() enters the branch at line 186 and reaches line 191:

if (!this.mos) {
    throw Error('mos is undefined!')
}
this.mos.setDebug(this.debugLogging)

this.mos is still undefined here, so init() will throw and MosHandler.create() will reject — which is exactly the boot-failure mode this PR is trying to remove for /healthz and /readyz. The same race exists for any peripheralDevice observer event that fires inside the 20 ms window before _updateDevices() initializes this.mos.

Either initialize the MOS connection eagerly (without awaiting device connections) before observers run, or guard _deviceOptionsChanged() so it tolerates this.mos being undefined at this stage. Sketch:

🛡️ Suggested guard in _deviceOptionsChanged 
 			if (this.debugLogging !== settings.debugLogging) {
 				this._logger.info('Changing debugLogging to ' + settings.debugLogging)

 				this.debugLogging = !!settings.debugLogging

-				if (!this.mos) {
-					throw Error('mos is undefined!')
-				}
-				this.mos.setDebug(this.debugLogging)
+				// `this.mos` may not yet exist if observers fire before _initMosConnection completes;
+				// the value will be applied on next change once mos is initialized.
+				this.mos?.setDebug(this.debugLogging)
🤖 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 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).

}
Comment on lines +140 to 141
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Startup now hides fatal MOS init/config errors

init() returns before the first _updateDevices() runs, and Line 215–217 only logs failures. That means _initMosConnection() errors (eg. invalid mosId/ports) no longer fail MosHandler.create(), so the process can report initialized while MOS is broken.

Suggested fix 
 private async init(config: MosConfig, coreHandler: CoreHandler): Promise<void> {
   this.mosOptions = config
   this._coreHandler = coreHandler
   ...
   this.mosTypes = getMosTypes(this.strict)

+  // Fail fast on invalid MOS configuration, but still avoid waiting for device connections
+  await this._initMosConnection()
+
   coreHandler.onConnected(() => {
     this.setupObservers()
     this.sendStatusOfAllMosDevices()
   })
   this.setupObservers()

   this.triggerUpdateDevices()
 }

Also applies to: 214-217

🤖 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 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).

async dispose(): Promise<void> {
this._disposed = true
Expand Down Expand Up @@ -209,10 +205,13 @@ export class MosHandler {
this._logger.debug('test log debug')
}
}
if (this._triggerupdateDevicesTimeout) {
clearTimeout(this._triggerupdateDevicesTimeout)
this.triggerUpdateDevices()
}
private triggerUpdateDevices() {
if (this._triggerUpdateDevicesTimeout) {
clearTimeout(this._triggerUpdateDevicesTimeout)
}
this._triggerupdateDevicesTimeout = setTimeout(() => {
this._triggerUpdateDevicesTimeout = setTimeout(() => {
this._updateDevices().catch((e) => {
this._logger.error(stringifyError(e))
})
Expand Down
Loading