Skip to content

fix: Fix restartRequired flag #31947

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
andrei-lazarov wants to merge 1 commit into
base: dev
Choose a base branch
from
+10 −3
Open

fix: Fix restartRequired flag #31947

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 10 additions & 3 deletions lib/extension/bridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,10 @@ export default class Bridge extends Extension {
}

const newSettings = message.options as Partial<Settings>;
this.restartRequired = settings.apply(newSettings);
const newRestartRequired = settings.apply(newSettings);
if (newRestartRequired) {
this.restartRequired = newRestartRequired;
}

// Apply some settings on-the-fly.
if (newSettings.homeassistant) {
Expand All @@ -262,9 +265,13 @@ export default class Bridge extends Extension {
logger.setDebugNamespaceIgnore(settings.get().advanced.log_debug_namespace_ignore);
}

logger.info("Successfully changed options");
if (newRestartRequired) {
logger.info("Changes require restart to take effect");
} else {
logger.info("Successfully changed options");
}
await this.publishInfo();
return utils.getResponse(message, {restart_required: this.restartRequired});
return utils.getResponse(message, {restart_required: newRestartRequired});
Copy link
Copy Markdown
Collaborator

@Nerivec Nerivec May 9, 2026

Choose a reason for hiding this comment

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

Suggested change
return utils.getResponse(message, {restart_required: newRestartRequired});
return utils.getResponse(message, {restart_required: this.restartRequired});

should stay required on consecutive saves if any previous save resulted in "required".
Looks like we don't have a test for this?

Copy link
Copy Markdown
Contributor Author

@andrei-lazarov andrei-lazarov May 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

To me it makes sense the response to changing an option should tell if that option needs a restart, not if the system does. (But I don't mind changing it)

For the tests just change some options and see if this.restartRequired is set correctly? settings.test.ts is fine?

Copy link
Copy Markdown
Collaborator

@Nerivec Nerivec May 9, 2026

Choose a reason for hiding this comment

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

I guess it depends if we want the bridge to keep in sync with the per-save response. I guess there's an argument for both 😅
We'd have to change the logic elsewhere too though:

zigbee2mqtt/lib/extension/bridge.ts

Line 475 in 94751ab

return utils.getResponse(message, {from: oldOptions, to: newOptions, id: ID, restart_required: this.restartRequired});

Frontend uses only bridge/info, so, no effect there.

We should test consecutive changes (through bridge topics) and match resulting bridge/info state and topic response state. In extensions/bridge.test.ts.

}

@bind async deviceRemove(message: string | KeyValue): Promise<Zigbee2MQTTResponse<"bridge/response/device/remove">> {
Expand Down
Loading