Skip to content

fix(mqtt): resync MQTT discovery after reconnect - #743

Merged
Starosdev merged 3 commits into
Starosdev:developfrom
YouCD:ycd
Sep 19, 2026
Merged

Starosdev merged 3 commits into
Starosdev:developfrom
YouCD:ycd

Conversation

@YouCD

@YouCD YouCD commented Aug 19, 2026

Copy link
Copy Markdown

Summary

When the initial MQTT connection fails (e.g., broker unreachable at startup), the paho client is configured with SetConnectRetry(true) and SetAutoReconnect(true), so it keeps retrying in the background. However, the application previously nulled out the publisher and skipped registering MqttPublisherMiddleware on the initial connect failure, so even a successful reconnect never restored publishing or discovery sync — a restart was required.

This PR keeps the MQTT integration alive across connect failures and re-syncs all discovery entities whenever the connection is (re)established.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Refactoring (no functional changes)
  • CI/CD or infrastructure change

Related Issues

None.

Changes Made

  • webapp/backend/pkg/web/server.go: do not null out the publisher when the initial connect fails; always register MqttPublisherMiddleware so publishing resumes automatically once paho reconnects
  • webapp/backend/pkg/web/server.go: register a reconnect hook before connecting so both the initial connection and every automatic reconnect trigger a full device sync
  • webapp/backend/pkg/mqtt/client.go: add SetOnReconnect callback invoked on every (re)established connection
  • webapp/backend/pkg/mqtt/publisher.go: expose SetOnReconnected to forward the callback
  • webapp/backend/pkg/web/server.go: guard loadInitialMqttData with an atomic.Bool to prevent concurrent duplicate syncs

Testing

  • go build ./... in webapp/backend

  • go test ./pkg/mqtt/... ./pkg/web/... and full go test ./... pass

  • go vet ./pkg/mqtt/... ./pkg/web/... clean

  • Behavior verified: after a broker outage the client reconnects in the background, publishes the online availability state, and re-syncs discovery/state for all devices without a restart

  • I have tested this locally

  • I have added/updated tests that prove my fix/feature works

  • All existing tests pass

Checklist

  • My code follows the project's code style
  • I have performed a self-review of my code
  • I have commented my code where necessary (particularly complex areas)
  • I have updated the documentation if needed
  • My changes generate no new warnings
  • No console.log, debug statements, or commented-out code

Screenshots (if applicable)

Not applicable.

Additional Notes

None.

- Initial connect failure no longer disables MQTT; paho retries
- Reconnect hook re-publishes discovery entities
- Atomic flag prevents overlapping discovery syncs
@YouCD
YouCD requested a review from Starosdev as a code owner August 19, 2026 04:55
@Starosdev

Copy link
Copy Markdown
Owner

Nice catch on the root cause. With SetConnectRetry(true), Connect() hands back a token that only completes once the connection succeeds, so token.WaitTimeout(defaultConnectTimeout) gives you an error while paho is still happily retrying in the background (see the RETRYCONN loop in paho v1.5.1 client.go). Setting ae.MqttPublisher = nil on that error is what killed MQTT for good. Keeping the publisher alive is the right call.

One thing needs fixing before this can merge. The rest are smaller.

Blocking: we now publish to a client that has no connection

While paho is retrying the initial connection, IsConnected() lies to you:

// paho.mqtt.golang v1.5.1 client.go:197
case s == connected:
    return true
case c.options.ConnectRetry && s == connecting:
    return true

Every method on Publisher uses that call as its only safety check. That was fine before, because a failed initial connect nulled the publisher and skipped the middleware, so c.Get("MQTT_PUBLISHER") came up empty and the handlers did nothing. Now that the middleware is always registered, those handlers go ahead and publish even though there is no socket.

Worse, Publish() has no early exit in the connecting state. The qos 0 shortcut at client.go:779 only covers reconnecting. So paho stores the packet, logs "storing publish message (connecting)", and never completes the token. Every call to Client.publish sits there for the full 5 second defaultPublishTimeout. That adds up fast:

  • POST /api/devices/register publishes discovery inline, 5 messages per device, so 25 seconds for every device the collector registers
  • DELETE /api/device/:id sends 5 remove messages, 5 legacy ones, and 2 state clears: 60 seconds
  • POST /api/health/mqtt-sync does the same walk over every device

ConnectRetryInterval is 30 seconds and there is no attempt limit, so this is not a brief startup hiccup. It lasts as long as the broker is down, and collector registration will blow past web.listen.write_timeout_seconds.

Those stored messages do not show up later either. Connect() wipes the store on a successful connect unless CleanSession is false, and paho defaults it to true (options.go:130), which we never override. Everything queued during the retry window gets dropped the moment we finally connect. The auto-reconnect path behaves differently, since reconnect() always calls c.resume().

One line in pkg/mqtt/client.go fixes it:

func (c *Client) IsConnected() bool {
	// IsConnectionOpen is only true when a connection is actually established.
	// IsConnected also returns true during ConnectRetry backoff.
	return c.client != nil && c.client.IsConnectionOpen()
}

IsConnectionOpen() is already on the paho Client interface, so nothing else has to change.

POST /api/health/mqtt-sync claims success while the broker is down

Same root cause. SyncAllDevices only returns its "MQTT client is not connected" error when IsConnected() is false, and during connecting it is not. So the sync runs, every publish times out into a Warnf, and you get back HTTP 200 with devices_published: N for messages nobody will ever receive.

The one-line fix above makes the response honest, but it comes back as a 500 from the generic error branch, where you used to get a 400. A 500 is a bit heavy for something we know is going to happen, so an explicit check returning 503 would read better.

Not blocking: a reconnect during a sync gets dropped

When the CAS fails, loadInitialMqttData just gives up and nothing retries later:

if !ae.mqttSyncInProgress.CompareAndSwap(false, true) {
	return
}

If the broker drops and comes back while a sync is already running, onReconnect hits the failed CAS and that connection never gets synced.

It is not as bad as it first looks. web.mqtt.qos defaults to 1, anything published during reconnecting gets persisted, and reconnect() calls c.resume() no matter what CleanSession says, so the individual messages still land. What you lose is the full sync, which is the only thing that removes archived devices and cleans up the legacy topics. Setting a pending flag and rerunning once the current sync finishes would cover it, and it is small enough to test.

Advisory: the flag is on the wrong object

handler.MqttSync calls pub.SyncAllDevices(deviceRepo, c) directly, so it never sees the guard and a manual sync can still overlap a reconnect sync. Nothing breaks, it is just duplicated work. Putting the flag on Publisher next to SyncAllDevices catches every caller in one spot and keeps AppEngine copyable.

Smaller stuff

  • SetOnReconnect writes c.onReconnect with no synchronization, and paho reads it from its own goroutine (go c.options.OnConnect(c), client.go:635). There is no race here today because the only caller runs before Connect(), but an exported setter does not promise that to whoever calls it next.
  • The commit headline has an emoji in it. AGENTS.md line 18 rules those out for commits and PRs, so either amend it or fix the squash-merge title.
  • No tests. pkg/mqtt already has in-package testify tests, and both the sync guard and the connected check are easy to cover.
  • No CI has reported on this PR, so none of it has actually been checked yet. Worth making sure the workflows run before merging.

@Starosdev
Starosdev merged commit b73c572 into Starosdev:develop Sep 19, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants