fix: reconcile Hub command ACKs before retrying inverter writes

[mgazza]

Summary

Consume correlated Hub command acknowledgements and carry rejected/uncertain results back through standalone service dispatch to the inverter verification loops.

• Register waiters before publish, await on the MQTT owner loop, and use restart-safe command IDs.
• Match command ID, topic, command name and boolean result. Ignore retained/unrelated replies.
• Reject known offline, stale, missing, inactive or explicitly unmanaged targets without changing coordinator/AIO routing.
• Stop repeated writes after a NACK, missing ACK, disconnection or uncertain publish. Uncertain outcomes receive one bounded readback window, because a write may have partly applied.
• A positive ACK never replaces telemetry verification. Existing retries on positive ACK plus unchanged telemetry remain unchanged.
• Recover denied ACK subscriptions with bounded credential refresh/reconnect. Preserve telemetry while control is unavailable.
• Preserve legacy service return behavior and expose structured failures through the HTTP service API.

Proof and tests

New regressions run actual Gateway telemetry/configuration, standalone service dispatch, cross-loop MQTT publication and inverter number/switch/schedule verification. Only physical device/broker boundaries are simulated.

Before the fix, explicit NACKs and a disconnected coordinator cause ten writes and 20 simulated seconds of polling, ending in the observed didn't complete messages. A healthy single AIO completes in one attempt. This proves the mechanism can produce the symptom, not a unique cause for field Modbus failures.

• Initial baseline: 4 failures, 2 passes.
• Final new regression suite: 28/28 pass.
• Custom gateway suite: 305 checks pass.
• Inverter, control-ledger, write-poll and callback-success suites pass.
• Includes lost/misleading/delayed ACKs, disconnect/cancellation cleanup, malformed replies, partial application, schedule writes, HTTP serialization and ACL recovery.
• Explicit compatibility test preserves legacy boolean/None service results; only typed Hub outcomes alter verifier retries.

GitNexus marks the shared verifier change CRITICAL (20 affected flows). Those flows were reviewed and the broader regression suites above rerun. No change to coordinator selection, fanout or other integrations' legacy retry behavior is intended.

Rollout prerequisites

The instance MQTT token needs read access to predbat/devices/<paired-device>/ack/+. Existing one-level + status grants do not cover ACKs. Deploy token-issuer ACL support and firmware that truthfully reports failed setters before enabling this consumer. Old firmware can still falsely ACK failure, so telemetry verification remains essential.

No deployment, live device commands or provider changes are part of this PR.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Three moderate Gateway issues remain unresolved, along with an API documentation update.

Review effort: Lite
Findings: None

What changed in this PR

Adds correlated MQTT ACK handling and typed write outcomes to prevent unsafe inverter retries and expose structured failures.

Changes:

• Correlates ACKs, validates targets, and handles reconnects.
• Propagates rejected or uncertain outcomes through verification, standalone dispatch, and HTTP APIs.
• Adds regression coverage for ACK and retry behavior.

File	Summary
apps/​predbat/​web.py	Serializes structured failures; documentation should define the new response contract (nit, 1 vote).
apps/​predbat/​userinterface.py	Propagates typed standalone service results.
apps/​predbat/​tests/​test_gateway.py	Updates command-ID tests and registration.
apps/​predbat/​tests/​test_gateway_write_results.py	Adds Gateway write and failure regressions.
apps/​predbat/​inverter.py	Handles ACK-aware verification and retry behavior.
apps/​predbat/​gateway.py	Adds ACK correlation and control validation; three moderate findings remain (1 vote each) involving empty-status cache invalidation, owner-loop shutdown races, and unknown online-state handling.
apps/​predbat/​component_base.py	Defines structured write results and errors.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Out-of-order topology telemetry is compared by receipt time, which can incorrectly block or permit controls.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Use frame timestamps to detect out-of-order topology signals · New

[springfall2008]

Automated comment from the triage bot (pr-cleanup run).

I didn't push anything. This branch needs a manual rebase, because merging origin/main into it hits a conflict in design, not just in text.

What changed upstream: #5220 (merged 2026-09-26) added its own Hub command-ack handling to apps/predbat/gateway.py. It came in through commits 13e78d96 "send each hub command once and wait for its ack" through 756cb42f. That implementation has:

• _subscribe_acks(), which uses _ack_subscribed, _acks_seen and _ack_subscribe_warned. A denied subscription is optional there: writes carry on as they did before.
• _process_ack(), routed by _ack_topic_prefix.
• _send_control(), which tracks acks per entity in _pending_commands, with per-id outcomes, cache provenance and a bounded _COMMAND_ACK_IDS_KEPT.
• _next_command_id(), which gives clock-seeded incrementing PBAT ids.

This PR covers the same ground a different way: _subscribe_command_acks(), which reconnects with a credential refresh when the subscription is denied; _process_command_ack(); _publish_control_command() for _CONTROL_COMMANDS; and uuid4().hex command ids.

Why I didn't resolve it: git merge --no-edit origin/main stops with three conflict blocks in gateway.py: the runtime-state init, _on_message ack routing and publish_command id allocation/dispatch. The parts that merge automatically are wrong too. The connect loop would subscribe to acks twice, once through each mechanism, and topic_ack would be assigned twice. Getting to one correct result means deciding which ack model survives, and that affects:

• denied-subscription behaviour (optional, or reconnect/refresh)
• command id format (incrementing PBAT<n>, or uuid hex)
• where the per-write state lives (_pending_commands keyed by entity, or waiters keyed by command)

Then the typed-outcome plumbing this PR adds to the standalone service dispatch, the HTTP service API and the inverter verifier would have to be ported onto whichever model survives. That is a re-implementation, so I've left it for the author. The other files in the merge (component_base.py, test_gateway.py, userinterface.py, web.py) merged without conflict.

I didn't process the review threads or CI failures on this run, because they'd need re-checking against the rebased code anyway.

[springfall2008]

@mgazza #5231 merged this morning, and it overlaps most of this PR. I don't think this one can be rebased as it stands. It needs a rethink on top of #5231, or closing in favour of a smaller follow-up. Which would you prefer?

How the two overlap

Both PRs subscribe to ack/+ and handle the same "re-publish on every poll" symptom. They take opposite approaches:

• fix(gateway): send each hub command once and wait for its ack #5231 stays inside gateway.py. It publishes and returns at once. Once an ack has been seen on the connection, it won't publish the same value again for 30 s. An ok ack updates the cached slot/enable values. If there's no ack subscription, or no ack has been seen yet, control writes behave exactly as before.
• This PR waits for the ack. Each control write blocks for up to 30 s. A refusal or missing ack comes back as a failure result, and inverter.py stops retrying. If the ack subscription is unavailable, every control write is rejected.

What a trial rebase onto main showed

• There are 3 conflict hunks in gateway.py: the init state, the ack branch in _handle_message, and publish_command.
• The parts git merges without complaint are wrong too: the result subscribes to ack/+ twice, through _subscribe_acks and _subscribe_command_acks.
• Taking this PR's side in publish_command means commands go out with a uuid id. _send_control pre-allocates its own id, so none of fix(gateway): send each hub command once and wait for its ack #5231's acks would match and its dedup would silently stop working.

Two issues here that apply whatever happens with #5231

1. A refused subscription is never detected. _subscribe_command_acks expects a denied subscription to raise. In aiomqtt 2.5.1, client.subscribe() raises only on a local send error. A broker refusal comes back as a 0x80 reason code in the return value. So a refusal sets _ack_subscribed = True, the token-refresh recovery never runs, and every control write waits 30 s for an ack that won't arrive. The tests don't catch this because their stub subscribe raises. fix(gateway): send each hub command once and wait for its ack #5231's _subscribe_acks checks the return code.
2. A slow ack blocks the engine thread. The 30 s wait sits on the engine thread. On hub firmware that doesn't publish acks, a cycle with several control writes stalls for minutes. fix(gateway): send each hub command once and wait for its ack #5231 avoids this by only relying on acks once one has been seen on the connection.

Fallback policy

This needs deciding either way. #5231 says a denied or missing ack subscription leaves writes as they were. This PR blocks all inverter control in that case. The rollout notes here say instance tokens don't have read access to ack/+ yet. So as written, this PR would take inverter control away from every gateway user until the ACL change ships. I'd keep #5231's fallback.

What's still worth having from this PR

• A definite refusal ends write_and_poll_* early. After fix(gateway): send each hub command once and wait for its ack #5231, the loop still polls about 20 s and then logs a failure, although it no longer re-publishes.
• The checks in _check_control_target before sending: hub offline, stale telemetry, serial not found, not polled or not managed.
• Pending writes are failed promptly on disconnect.
• The structured {success, error, outcome_unknown} response on the HTTP service API.

If you want to keep going, I'd build it as a layer on #5231:

• Keep fix(gateway): send each hub command once and wait for its ack #5231's ids, _subscribe_acks and _process_ack.
• Resolve the waiting future from inside _process_ack, after the cache update.
• Only wait while _acks_seen is set.
• Keep the typed result through userinterface/inverter.py, plus the checks before sending.

That is probably small enough to be a fresh PR, and closing this one would keep the review history clean.