Propagate persistence restore failures

[aidangarske]

F-8599

[Copilot]

Pull request overview

This PR improves broker persistence restore reliability by ensuring backend restore/iteration failures are surfaced to the caller and by adding rollback logic to discard partially restored in-memory state.

Changes:

• Propagate kv_iter/restore failures from BrokerPersist_Restore() instead of ignoring them, and rollback partially restored broker state on failure.
• Add deferred deletion handling for legacy/invalid persisted sessions (e.g., session-expiry=0) to avoid mutating persistence backends during iteration.
• Add unit tests covering iterator failure propagation, deferred delete behavior, and session key/body mismatch handling.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
wolfmqtt/mqtt_broker.h	Declares BrokerPersist_RestoreRollback() for use across persistence/restore code.
src/mqtt_broker.c	Implements restore rollback helper to free/clear partially restored broker state.
src/mqtt_broker_persist.c	Propagates iterator failures during restore, defers deletes until after iteration, and tightens session/sub restore validation.
tests/test_broker_connect.c	Adds persistence restore tests for error propagation and deferred delete semantics.

Suppressed comments (1)

src/mqtt_broker_persist.c:726

• When session expiry is 0, this path ignores any failure from BrokerPersist_DelSubs and returns only the result of BrokerPersist_DelSession. That can report success even if subs deletion failed, leaving stale persisted subscriptions behind.

    if (session_expiry_sec == 0) {
        (void)BrokerPersist_DelSubs(broker, client_id);
        return BrokerPersist_DelSession(broker, client_id);
    }

---

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

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #571

Scan targets checked: wolfmqtt-bugs, wolfmqtt-src

Findings: 8
8 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[embhorn]

Skoll Code Review

Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 15 total — 15 posted, 0 skipped
15 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [High] Restore silently drops persisted subscriptions whose SESSION record was skipped, breaking persistent-session resume — src/mqtt_broker_persist.c:1669-1674
• [Medium] Deferred zero-expiry cleanup turns a backend delete failure into a hard MqttBroker_Start() refusal — src/mqtt_broker_persist.c:2251-2256
• [Medium] "Put" functions now perform destructive cascading deletes; doc comments still describe them as write-only — src/mqtt_broker_persist.c:702-712,725-728
• [Medium] Cleanup reports success when the backend implements no kv_del, defeating the PR's failure-propagation goal — src/mqtt_broker_persist.c:1156-1162
• [Medium] BrokerPersist_RestoreRollback() discards all broker state, not just what restore loaded — src/mqtt_broker.c:6651-6684
• [Medium] New restore semantics and the rollback path are under-tested — tests/test_broker_connect.c:288-296,343-396
• [Low] RestoreRollback duplicates the subs-teardown loop from MqttBroker_Free, and the two copies already differ — src/mqtt_broker.c:6663-6677
• [Low] wmqb_log_client_id() duplicates BrokerLog_Sanitize() with a weaker, divergent escaping policy — src/mqtt_broker_persist.c:60-78
• [Low] #ifndef blocks wrap only the opening and closing brace of an if statement — src/mqtt_broker.c:5199-5207,5281-5289
• [Info] struct wmqb_restore_ctx::delete_count is written but never read — src/mqtt_broker_persist.c:1322
• [Info] Restore log reports deleted= before the deletes have run — src/mqtt_broker_persist.c:2241-2251
• [Info] Unreachable session_expiry_sec==0 rejection in wmqb_restore_create_orphan reports a misleading error code — src/mqtt_broker_persist.c:1418-1421
• [Info] New header declaration lacks a doc comment and the persist-hook contract is now stale — wolfmqtt/mqtt_broker.h:849
• [Info] 128-byte stack buffer populated even in no-log builds — src/mqtt_broker_persist.c:2061,2074
• [Info] RestoreRollback does not reset broker-next_packet_id — src/mqtt_broker.c:6655-6683

Review generated by Skoll