Skip to content

Add explicit error logging for MQTT publish attempts before/after connection availability - #169

Merged
mwfarb merged 2 commits into
mainfrom
copilot/arenaxrarena-unity-120-add-logging-pre-connection
Jul 31, 2026
Merged

Add explicit error logging for MQTT publish attempts before/after connection availability#169
mwfarb merged 2 commits into
mainfrom
copilot/arenaxrarena-unity-120-add-logging-pre-connection

Conversation

Copilot AI commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Publish calls were silently accepted when the MQTT session was not actually open, making pre-connection and post-disconnect failures hard to diagnose. This change adds an explicit connection-state gate in the publish path and emits a clear error when a publish is attempted while disconnected.

  • Publish-path connection guard

    • Updated ArenaMqttClient.Publish(...) to fail fast unless the broker session is open.
    • Guard conditions now check:
      • client == null
      • !mqttClientConnected
      • !client.IsConnected
  • Operational visibility for dropped publishes

    • Added error logging for blocked publish attempts, including the MQTT topic, so logs clearly indicate why a message was not sent.
  • Behavior when connected remains unchanged

    • When connected, publish and existing message logging behavior continue as before.
public void Publish(string topic, byte[] payload)
{
    if (client == null || !mqttClientConnected || !client.IsConnected)
    {
        Debug.LogError($"MQTT publish request failed; connection is not open. Topic: {topic}");
        return;
    }

    client.Publish(topic, payload);
    // existing logging behavior...
}

Copilot AI linked an issue Jul 18, 2026 that may be closed by this pull request
Copilot AI changed the title [WIP] Add logging for publish requests on mqtt connection state Add explicit error logging for MQTT publish attempts before/after connection availability Jul 18, 2026
Copilot AI requested a review from mwfarb July 18, 2026 21:42
@mwfarb

mwfarb commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Architectural Code Review — PR #169

Linked Issue

Issue #120"add logging pre-connection"

"We should log errors when publish requests are made and the mqtt connection has not opened or has closed."

Summary

Replaces the silent if (client != null) guard in ArenaMqttClient.Publish() with an explicit triple-check connection gate that logs an error and returns early when disconnected. Single file change: Runtime/ArenaMqttClient.cs (+7 / -1).

Issue → Implementation Gap Analysis

Issue Requirement Implemented?
Log errors when publishing pre-connection client == null and !mqttClientConnected checks
Log errors when publishing post-close !client.IsConnected check
Error should indicate what was attempted ✅ Includes the topic in the log message

No gaps. The PR fully satisfies Issue #120.

CONTRIBUTING.md Compliance

Rule Status
1. MQTT Topics — Use TOPICS constructor ✅ N/A (no topic changes)
2. Dependencies — Pin versions ✅ N/A (no new deps)
3. Component instantiation pattern ✅ N/A (no component changes)
4. Coordinate systems — Use ArenaUnity.cs ✅ N/A (no coordinate data)

Architecture Assessment

✅ Triple-check is defense-in-depth. The three conditions catch distinct failure modes:

  • client == nullAwake() not yet run or teardown complete
  • !mqttClientConnected — lifecycle flag managed by M2MqttUnityClient (base class, set in OnConnected/OnDisconnected)
  • !client.IsConnected — live socket-level check from the M2Mqtt library

✅ Fail-fast with early return. Previously, if client != null but the connection was closed, client.Publish() would throw an unhandled exception inside the M2Mqtt library. The early return converts a crash into a clean error log.

✅ Existing logging preserved. The LogMessage("Sending", ...) call below the publish is unchanged — connected-path behavior is identical.

✅ Correctly uses Debug.LogError. This is an error condition (attempted publish while disconnected), not a warning.

Observations (non-blocking)

  1. No throttling on the error log. If user code has a publish loop that runs every frame while disconnected, this will emit LogError every frame. Unlike Add throttled high-rate MQTT publish warnings in runtime client #164 which adds cooldown-based throttling for warnings, this error path has no rate limiting. In practice this is acceptable — LogError during disconnect is a "fix your code" signal, not a sustained runtime condition. If it becomes noisy, Add throttled high-rate MQTT publish warnings in runtime client #164's throttle pattern could be adapted here.

  2. ArenaObject already guards on the caller side. Components like ArenaObject.cs check mqttClientConnected before calling publish (4 call sites found). This PR adds a second safety net at the Publish() method itself — belt-and-suspenders, which is appropriate.

Verdict: ✅ APPROVE

No blocking issues. Clean, minimal change that converts a silent failure (or potential crash) into a diagnosable error log. Fully satisfies Issue #120.

@mwfarb
mwfarb marked this pull request as ready for review July 31, 2026 20:01
@mwfarb
mwfarb merged commit b5ecfa5 into main Jul 31, 2026
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.

add logging pre-connection

2 participants