GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — aclk: MQTT keepalive starved by traffic and tied to the wall clock by stelfrag · Pull Request #24427 · netdata/netdata

Conversation

@stelfrag

@stelfrag stelfrag commented Oct 25, 2026 •
edited by coderabbitai Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

The ACLK client requested its MQTT keepalive, and checked the PINGRESP deadline, only when poll() timed out with nothing to do. A link with steady inbound traffic never got there, so an unanswered PINGREQ went unnoticed indefinitely. The deadline, the keepalive base and the PUBACK deadlines also used the wall clock, so a clock step caused a false ping timeout (forward) or delayed detection (backward). Before the first NTP sync on a board without an RTC, PUBACK deadlines never expired at all.

Changes:

  • The keepalive is evaluated on every mqtt_wss_service() call, and poll() is bounded by its next due time.
  • The ping state moves from a global into the MQTT client.
  • The PINGRESP wait starts when the PINGREQ is handed to the WebSocket layer. While it is still queued behind a message being written, it expires only once that write stops making progress.
  • After a ping timeout, no further keepalive is sent during teardown.
  • PUBACK deadlines and their sweep use monotonic seconds.

Tested with a fake Cloud, same harness on master and this branch:

scenario master branch
PUBLISH every 0.5 s, PINGRESP withheld still connected after 220 s reconnects 60 s after PINGREQ
wall clock +3600 s, PINGRESP after 20 s false ping timeout stays connected
wall clock -3600 s, PINGRESP withheld not detected (160 s+) reconnects 60 s after PINGREQ
no traffic, PINGRESP withheld reconnects ~61 s after reconnects 60 s after
PUBACK withheld, clock -3600 s packets never time out timed out after ~110 s
PUBACK withheld, clock +3600 s timed out at ~100 s timed out at ~100 s

-W aclktimeouttest and -W mqttngtest pass on every commit; new unit tests cover the keepalive state machine and the poll bound.

Summary by CodeRabbit

  • Bug Fixes
    • Improved MQTT message acknowledgement and keepalive timeout tracking, reducing the chance of incorrect timeouts when messages take time to transmit or system clock changes occur.
    • Keepalive checks now account for queued transmission progress and connection state, helping prevent unnecessary disconnects and avoiding additional keepalive requests after a connection expires.
    • Polling now respects the next keepalive deadline, supporting more timely timeout handling.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
stelfrag added 3 commits October 25, 2026 18:47
@stelfrag
aclk: check the MQTT keepalive on every service call
a1e6bbb 
The PINGREQ was requested, and its PINGRESP deadline checked, only when
poll() timed out, so a link with steady inbound traffic never noticed an
unanswered ping. The deadline and the keepalive base also used the wall
clock, so a clock step fired a false timeout or postponed a real one.

Evaluate the keepalive on every call, bound poll() by its next due time,
use the monotonic clock, and keep the ping state in the MQTT client
instead of a global.
@stelfrag
aclk: use the monotonic clock for MQTT PUBACK deadlines
4036205 
The deadlines were wall-clock seconds since 2024: a backward step, or a
clock still before 2024 on a board without an RTC, kept them from ever
expiring.
@stelfrag
aclk: start the PINGRESP wait when the PINGREQ is submitted
573fd38 
A ping queued behind a long message could expire while that message was
still being written. Start the wait when the PINGREQ is handed to the
WebSocket layer; while it is still queued, expire it only if writing
stops making progress. Report a socket error seen at the deadline as
such, not as a ping timeout.
@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 74cd2a0e-5a47-4abd-ba0d-b9092f7bf507
Commits

Reviewing files that changed from the base of the PR and between c5aec5a and 573fd38.

Files selected for processing: 3
  • src/aclk/mqtt_websockets/mqtt_ng.c
  • src/aclk/mqtt_websockets/mqtt_ng.h
  • src/aclk/mqtt_websockets/mqtt_wss_client.c

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Walkthrough

The MQTT client now uses monotonic time for packet deadlines and send timestamps. The WebSocket client schedules keepalives per connection, bounds polling by the next ping deadline, and handles ping expiry by resetting the ping and requesting a disconnect.

Changes: MQTT Keepalive and Timeout Handling

Layer / File(s) Summary
MQTT monotonic timing and ping lifecycle
src/aclk/mqtt_websockets/mqtt_ng.h, src/aclk/mqtt_websockets/mqtt_ng.c		 PUBACK deadlines and send timestamps now use monotonic time. The client records ping request and submission times, exposes accessors and a reset function, and clears ping state on response or reconnect.
Per-connection keepalive scheduling and expiry
src/aclk/mqtt_websockets/mqtt_wss_client.c		 The client schedules keepalives at three-quarters of the interval and bounds poll() by the next deadline. It tracks transmit progress for queued pings and handles ping expiry with a ping reset and disconnect request.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant mqtt_wss_service
  participant mqtt_ng
  participant SSL_write
  participant ACLK
  mqtt_wss_service->>mqtt_ng: Queue a PINGREQ when a keepalive is due
  mqtt_wss_service->>SSL_write: Write queued MQTT data
  SSL_write-->>mqtt_wss_service: Report transmit progress
  mqtt_wss_service->>mqtt_ng: Read ping timestamps and reset an expired ping
  mqtt_wss_service->>ACLK: Request a ping-timeout disconnect
Loading

Suggested reviewers: thiagoftsm

Merge Risk: Minimal · up to 573fd

No identified keepalive or timeout issue remains to resolve before merging; normal checks still apply.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly identifies the two main changes: preventing MQTT keepalive starvation under traffic and removing wall-clock dependence from timing. It is concise, specific, and matches the pull requ…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title clearly identifies the two main changes: preventing MQTT keepalive starvation under traffic and removing wall-clock dependence from timing. It is concise, specific, and matches the pull request objectives.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
  • X
  • Mastodon
  • Reddit
  • LinkedIn

Comment @coderabbitai help to get the list of available commands.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the area/aclk label Oct 25, 2026
@sonarqubecloud

sonarqubecloud Bot commented Oct 25, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@stelfrag
stelfrag marked this pull request as ready for review October 25, 2026 17:49
@stelfrag
stelfrag requested a review from thiagoftsm October 25, 2026 17:50
thiagoftsm
thiagoftsm approved these changes Oct 25, 2026
View reviewed changes

@thiagoftsm thiagoftsm left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

All charts and communication with cloud are working as expected. LGTM!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@stelfrag
stelfrag merged commit a5f16c9 into netdata:master Oct 25, 2026
161 checks passed

Uh oh!

There was an error while loading. Please reload this page.
@stelfrag
stelfrag deleted the fix-mqtt-ping-expiry branch October 25, 2026 20:38
@stelfrag stelfrag mentioned this pull request Oct 26, 2026
Patch release 2.12.3 #24442
Merged
Ferroin pushed a commit that referenced this pull request Nov 4, 2026
@stelfrag @Ferroin
aclk: MQTT keepalive starved by traffic and tied to the wall clock (#…
8aa8acc 
…24427)

* aclk: check the MQTT keepalive on every service call

The PINGREQ was requested, and its PINGRESP deadline checked, only when
poll() timed out, so a link with steady inbound traffic never noticed an
unanswered ping. The deadline and the keepalive base also used the wall
clock, so a clock step fired a false timeout or postponed a real one.

Evaluate the keepalive on every call, bound poll() by its next due time,
use the monotonic clock, and keep the ping state in the MQTT client
instead of a global.

* aclk: use the monotonic clock for MQTT PUBACK deadlines

The deadlines were wall-clock seconds since 2024: a backward step, or a
clock still before 2024 on a board without an RTC, kept them from ever
expiring.

* aclk: start the PINGRESP wait when the PINGREQ is submitted

A ping queued behind a long message could expire while that message was
still being written. Start the wait when the PINGREQ is handed to the
WebSocket layer; while it is still queued, expire it only if writing
stops making progress. Report a socket error seen at the deadline as
such, not as a ping timeout.

(cherry picked from commit a5f16c9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thiagoftsm thiagoftsm thiagoftsm approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

area/aclk

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@stelfrag @thiagoftsm
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.