Skip to content

fix(aclk): prevent unbounded one-core CPU spins in the cloud-connection loops - #22879

Merged
ktsaou merged 2 commits into
netdata:masterfrom
ktsaou:fix-aclk-cpu-spin-hardening
Jun 27, 2026
Merged

fix(aclk): prevent unbounded one-core CPU spins in the cloud-connection loops#22879
ktsaou merged 2 commits into
netdata:masterfrom
ktsaou:fix-aclk-cpu-spin-hardening

Conversation

@ktsaou

@ktsaou ktsaou commented Jun 26, 2026

Copy link
Copy Markdown
Member

Problem

Two independent ACLK loops can peg one CPU core at 100% indefinitely (until a
manual restart) while the cloud link still looks healthy. Both were diagnosed on a
standalone Windows Server agent.

1. ACLKSYNC dispatch loop (sqlite_aclk.c)

aclk_synchronization_event_loop() keeps a pending_queries counter as an alias for
the number of queries held in aclk_query_execute->JudyL. If that counter ever drifts
above the actual queue length, the ACLK_DATABASE_NOOP -> ACLK_QUERY_EXECUTE rewrite
fires on every iteration over an empty queue, and the inner uv_run(UV_RUN_NOWAIT)
loop never blocks — a silent, work-free 100% CPU spin that cannot self-recover (the
spinning thread is even accounted idle, because worker_is_busy() excludes
ACLK_QUERY_EXECUTE).

Fix: before the rewrite can use it, reconcile pending_queries against
JudyLCount() (the source of truth) and log the discrepancy (rate-limited) if they
disagree. Inert when they match; self-heals any drift regardless of its origin.

2. mqtt_wss_service() and the https_client.c poll loops

When poll() keeps reporting readiness but no bytes/frames make progress (e.g. a
runtime poll()/SSL readiness quirk), the only escapes were gated behind poll() == 0
and therefore never fired, so the loop spins.

Fix:

  • mqtt_wss_service(): add a per-client no-progress watchdog (drop + reconnect after
    2 * PING_TIMEOUT, refreshed on a clean poll() timeout and on every byte moved,
    seeded at CONNACK, gated to an established connection), plus a terminal
    socket-revents check on POLLERR | POLLNVAL. POLLHUP intentionally falls through
    to SSL_read so a graceful close drains its final frame before the clean
    SSL_ERROR_ZERO_RETURN.
  • https_client.c: evaluate the existing request-timeout check at the top of all three
    poll loops (socket_write_all, ssl_write_all, read_parse_response), not only on
    poll() == 0.

Notes

  • All changes are additive and inert on healthy paths, single-threaded where they touch
    state, and platform-independent (no Windows-only special-casing).
  • The exact field trigger for the counter drift is not yet pinned down (suspected
    memory/Judy corruption on the MSYS2/Cygwin runtime); the reconcile is defense-in-depth
    that ends the spin for any cause and logs the condition so a recurrence is captured.

Summary by cubic

Prevents unbounded one-core CPU spins and stalled ACLK queues by fixing two cloud-connection loops. The agent now self-recovers instead of burning CPU when poll reports readiness without progress.

  • Bug Fixes
    • ACLKSYNC dispatch loop: on every idle pass, reconcile pending_queries with JudyLCount() before any rewrite; rate-limit a warning and self-correct drift to prevent both spins (over-count) and stalled queues (under-count).
    • mqtt_wss_service(): add a per-client no-progress watchdog (drop/reconnect after 2 * PING_TIMEOUT) and drop on POLLERR | POLLNVAL; POLLHUP still drains via SSL_read.
    • https_client.c: check the overall request timeout on every iteration in socket_write_all, ssl_write_all, and read_parse_response to break spin states.

Written for commit 975b281. Summary will update on new commits.

Review in cubic

…on loops

Two independent ACLK loops could peg one CPU core at 100% indefinitely
(until a manual restart) while the cloud link still looked healthy:

1) ACLKSYNC dispatch loop (sqlite_aclk.c): if the pending_queries counter
   ever drifts above the actual number of queued queries, the
   NOOP -> ACLK_QUERY_EXECUTE rewrite fires on every iteration over an
   empty queue and the inner UV_RUN_NOWAIT loop never blocks, producing a
   silent, work-free spin that cannot self-recover. Reconcile the counter
   against JudyLCount() (the source of truth) before it can gate the
   rewrite, and log the discrepancy (rate-limited) so the trigger is
   captured if it ever recurs.

2) mqtt_wss_service() and the https_client.c helper loops: when poll()
   keeps reporting readiness but no bytes/frames make progress, the only
   escapes were gated behind poll()==0 and therefore never fired. Add a
   per-client no-progress watchdog (drop and reconnect after
   2*PING_TIMEOUT) plus a terminal socket-revents check (POLLERR/POLLNVAL)
   to mqtt_wss_service(); POLLHUP intentionally falls through to SSL_read
   so a graceful close drains its final frame. Evaluate the request
   timeout on every iteration of the three https_client.c poll loops.

All changes are additive and inert on healthy paths, single-threaded where
they touch state, and platform-independent.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 issue found across 3 files

Confidence score: 2/5

  • In src/database/sqlite/sqlite_aclk.c, the reconcile logic skips the zero-counter case, so a drifted state where the queue has work but pending_queries == 0 is never corrected; merging as-is risks permanently stalled ACLK query processing until an unrelated enqueue happens. Add a recovery path for this state (or trigger reconcile when queue depth and counter disagree) and verify with a targeted test before merging.
Architecture diagram
sequenceDiagram
    participant Agent as Agent Main
    participant ACLK as ACLK Sync Loop
    participant JudyL as JudyL Queue
    participant MQTT as MQTT WSS Client
    participant HTTPS as HTTPS Client
    participant Cloud as Cloud Endpoint
    participant poll as poll() syscall

    Note over Agent,Cloud: ACLK Sync Loop – Query Dispatch (no-progress spin fix)

    loop Each iteration
        ACLK->>JudyL: dequeue next opcode
        alt opcode == ACLK_DATABASE_NOOP and pending_queries > 0
            ACLK->>JudyL: NEW: reconcile pending_queries with JudyLCount()
            alt pending_queries != JudyLCount()
                ACLK->>ACLK: NEW: log rate-limited warning, set pending_queries = JudyLCount()
            end
            ACLK->>ACLK: if pending_queries > 0 and queries_running < limit → rewrite to ACLK_QUERY_EXECUTE
        else opcode == ACLK_QUERY_EXECUTE
            ACLK->>JudyL: process query, decrement pending_queries
        end
    end

    Note over MQTT,Cloud: MQTT WSS Service – I/O Handling (watchdog + error check)

    MQTT->>poll: poll() with timeout
    alt poll() returns 0 (clean timeout)
        MQTT->>MQTT: NEW: update last_io_progress_ut
        MQTT->>MQTT: handle keepalive
    else poll() returns >0 (readiness)
        Note over MQTT: NEW: no-progress watchdog (only for established connection)
        MQTT->>MQTT: check revents for POLLERR | POLLNVAL
        alt revents matches error
            MQTT->>MQTT: NEW: return MQTT_WSS_ERR_CONN_DROP (break spin)
        else revents not error
            MQTT->>MQTT: check if no progress for MQTT_WSS_IO_WATCHDOG_SECS
            alt watchdog elapsed
                MQTT->>MQTT: NEW: return MQTT_WSS_ERR_CONN_DROP (break spin)
            end
            MQTT->>Cloud: attempt SSL read/write
            alt bytes moved
                MQTT->>MQTT: NEW: update last_io_progress_ut
            else SSL error
                MQTT->>MQTT: existing error handling
            end
        end
    end

    Note over HTTPS,Cloud: HTTPS Client Poll Loops (timeout check on every iteration)

    HTTPS->>HTTPS: enter write or read loop (socket_write_all / ssl_write_all / read_parse_response)
    loop Until I/O complete or error
        HTTPS->>HTTPS: NEW: check https_req_check_timedout()
        alt timed out
            HTTPS->>HTTPS: NEW: return timeout (break unbounded spin)
        else not timed out
            HTTPS->>poll: poll() with timeout
            alt poll() > 0 (ready)
                HTTPS->>Cloud: perform underlying I/O (send/recv)
                alt I/O progressed
                    HTTPS->>Cloud: continue loop
                else I/O error
                    HTTPS->>HTTPS: existing error handling
                end
            else poll() == 0 (timeout)
                HTTPS->>HTTPS: existing timeout handling (now also covered by top check)
            end
        end
    end
Loading

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/database/sqlite/sqlite_aclk.c Outdated
…n > 0

The reconcile only ran when pending_queries > 0, so it healed an over-count
(the CPU spin) but not an under-count: if the counter drifted to 0 while
aclk_query_execute->JudyL still held queued queries, the NOOP ->
ACLK_QUERY_EXECUTE rewrite never fired and those queries stalled until an
unrelated enqueue nudged execution. Run the reconcile on every idle (NOOP)
pass so drift in either direction self-heals (JudyLCount is O(1) on an
empty/small array).
@ktsaou

ktsaou commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

@cubic-dev-ai please review again

@cubic-dev-ai

cubic-dev-ai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai please review again

@ktsaou I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues found across 3 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.
Architecture diagram
sequenceDiagram
    participant SyncLoop as ACLK Sync Event Loop
    participant Judy as JudyL Query Queue
    participant Counter as pending_queries
    participant MQTT as MQTT WSS Client
    participant Poll as poll/SSL
    participant HTTPS as HTTPS Client
    participant Timeout as Request Timeout

    Note over SyncLoop,Judy: ACLK Sync Dispatch Loop (sqlite_aclk.c)

    loop Each iteration
        SyncLoop->>Counter: Read pending_queries
        alt opcode == ACLK_DATABASE_NOOP
            SyncLoop->>Judy: NEW: JudyLCount() – source of truth
            Judy-->>SyncLoop: queued count
            alt queued != pending_queries
                SyncLoop->>SyncLoop: NEW: log discrepancy, set pending_queries = queued
            end
        end
        alt pending_queries > 0 and slots available
            SyncLoop->>SyncLoop: Rewrite NOOP → ACLK_QUERY_EXECUTE
            SyncLoop->>Judy: Pop and dispatch query
        end
    end

    Note over MQTT,Poll: MQTT WSS Service (mqtt_wss_client.c)

    MQTT->>MQTT: On CONNACK – NEW: seed last_io_progress_ut
    loop mqtt_wss_service entered periodically
        MQTT->>Poll: poll(timeout_ms)
        alt poll returns 0
            MQTT->>MQTT: NEW: update last_io_progress_ut (clean idle)
            MQTT->>MQTT: Handle keepalive if needed
        else poll returns >0
            MQTT->>MQTT: NEW: check revents & POLLERR|POLLNVAL
            alt error present
                MQTT->>MQTT: Drop connection, return MQTT_WSS_ERR_CONN_DROP
            else
                MQTT->>MQTT: NEW: check watchdog (elapsed > 2*PING_TIMEOUT?)
                alt time exceeded
                    MQTT->>MQTT: Drop connection, return MQTT_WSS_ERR_CONN_DROP
                else
                    MQTT->>Poll: SSL_read / SSL_write
                    alt bytes transferred
                        MQTT->>MQTT: NEW: update last_io_progress_ut
                    end
                end
            end
        end
    end

    Note over HTTPS,Timeout: HTTPS Client Poll Loops (https_client.c)

    loop socket_write_all / ssl_write_all / read_parse_response
        HTTPS->>Timeout: NEW: https_req_check_timedout() on every iteration
        alt timed out
            HTTPS->>HTTPS: Return error (2 or HTTPS_CLIENT_RESP_TIMEOUT)
        else
            HTTPS->>Poll: poll(POLL_TO_MS)
            alt poll > 0
                HTTPS->>Poll: Read/write data
            else poll error
                HTTPS->>HTTPS: Handle poll error
            end
        end
    end
Loading

Re-trigger cubic

@thiagoftsm thiagoftsm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues found during runtime. LGTM!

@ktsaou
ktsaou merged commit b42dc40 into netdata:master Jun 27, 2026
155 of 157 checks passed
@sonarqubecloud

Copy link
Copy Markdown

stelfrag pushed a commit to stelfrag/netdata that referenced this pull request Jul 12, 2026
@stelfrag stelfrag mentioned this pull request Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants