Skip to content

Fix/postchannel autoflush permanent stall#2746

Merged
hectorhdzg merged 2 commits into
microsoft:mainfrom
hectorhdzg:fix/postchannel-autoflush-permanent-stall
Jul 1, 2026
Merged

Fix/postchannel autoflush permanent stall#2746
hectorhdzg merged 2 commits into
microsoft:mainfrom
hectorhdzg:fix/postchannel-autoflush-permanent-stall

Conversation

@hectorhdzg

Copy link
Copy Markdown
Member

Problem

Under sustained intermittent send failures (e.g. a load balancer recycling
connections and returning occasional 503s), PostChannel could permanently stop
draining its in-memory queue. Once wedged, the queue saturates and telemetry is
silently dropped as QueueFull, and only a process restart recovers it.

Root cause

Auto flush is fire-and-forget, but it routed through flush(), whose async path
defers the send via a 0ms timer and then calls _waitForIdleManager(). That
re-arms _flushCallbackTimer and polls (every FlushCheckTimer) until the
HttpManager reports it is "completely idle" (no outstanding requests AND an empty
send queue) before clearing the timer.

While _flushCallbackTimer is non-null, BOTH the normal scheduled timer
(_scheduleTimer) and any further auto flush are suppressed. Under continuous
intermittent failures there is almost always a retry in-flight or a re-queued
batch, so the manager is rarely completely idle at the polling instant: the wait
never completes, _flushCallbackTimer stays set forever, and the channel stops
draining permanently.

Fix

Keep the original deferred fire-and-forget timing (so queue/discard semantics are
unchanged) but do not park the scheduler waiting for the manager to become idle:

  • _flushImpl() gains a waitForIdle parameter. When false (fire-and-forget auto
    flush), the completion logic runs immediately instead of via
    _waitForIdleManager(), so after the send the normal schedule resumes. Explicit,
    user-initiated flush() keeps its existing wait-for-idle behaviour.
  • _performAutoFlush() mirrors flush()'s async path (clear scheduled timer, queue
    the batches, send on a 0ms timer) but calls _flushImpl(..., waitForIdle=false).

A busy / non-idle HttpManager can therefore no longer wedge draining, while the
event ordering and QueueFull discard behaviour under overload are preserved.

Tests

Adds a regression test ("auto flush does not wedge behind the flush()
wait-for-idle timer") that fails on the old code (the residual wait-for-idle poll
timer suppresses the next auto flush) and passes with the fix. All existing
queueSize/discard/reset tests remain green (behaviour unchanged).

Bump the tar floor from ^7.5.13/>=7.5.13 to >=7.5.16 in the root npm 'overrides' and the Rush pnpm globalOverrides so installs resolve a patched tar (>=7.5.16). Mitigates the node-tar PAX size override file-smuggling vulnerability (GHSA-vmf3-w455-68vh).
…ind flush() wait-for-idle timer

Problem
-------
Under sustained intermittent send failures (e.g. a load balancer recycling
connections and returning occasional 503s), PostChannel could permanently stop
draining its in-memory queue. Once wedged, the queue saturates and telemetry is
silently dropped as QueueFull, and only a process restart recovers it.

Root cause
----------
Auto flush is fire-and-forget, but it routed through flush(), whose async path
defers the send via a 0ms timer and then calls _waitForIdleManager(). That
re-arms _flushCallbackTimer and polls (every FlushCheckTimer) until the
HttpManager reports it is "completely idle" (no outstanding requests AND an empty
send queue) before clearing the timer.

While _flushCallbackTimer is non-null, BOTH the normal scheduled timer
(_scheduleTimer) and any further auto flush are suppressed. Under continuous
intermittent failures there is almost always a retry in-flight or a re-queued
batch, so the manager is rarely completely idle at the polling instant: the wait
never completes, _flushCallbackTimer stays set forever, and the channel stops
draining permanently.

Fix
---
Keep the original deferred fire-and-forget timing (so queue/discard semantics are
unchanged) but do not park the scheduler waiting for the manager to become idle:

- _flushImpl() gains a `waitForIdle` parameter. When false (fire-and-forget auto
  flush), the completion logic runs immediately instead of via
  _waitForIdleManager(), so after the send the normal schedule resumes. Explicit,
  user-initiated flush() keeps its existing wait-for-idle behaviour.
- _performAutoFlush() mirrors flush()'s async path (clear scheduled timer, queue
  the batches, send on a 0ms timer) but calls _flushImpl(..., waitForIdle=false).

A busy / non-idle HttpManager can therefore no longer wedge draining, while the
event ordering and QueueFull discard behaviour under overload are preserved.

Tests
-----
Adds a regression test ("auto flush does not wedge behind the flush()
wait-for-idle timer") that fails on the old code (the residual wait-for-idle poll
timer suppresses the next auto flush) and passes with the fix. All existing
queueSize/discard/reset tests remain green (behaviour unchanged).
@hectorhdzg hectorhdzg requested a review from a team as a code owner July 1, 2026 20:51
Copilot AI review requested due to automatic review settings July 1, 2026 20:51

Copilot AI 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.

Pull request overview

Fixes a PostChannel auto-flush edge case where intermittent/continuous send failures could cause _flushCallbackTimer to remain set indefinitely (due to wait-for-idle polling), suppressing both scheduled sending and subsequent auto flushes and eventually leading to silent QueueFull drops.

Changes:

  • Adds a waitForIdle parameter to _flushImpl() so fire-and-forget auto flushes can complete without parking the scheduler behind _waitForIdleManager().
  • Reworks _performAutoFlush() to mirror the async flush scheduling behavior while calling _flushImpl(..., waitForIdle=false).
  • Adds a regression test to ensure auto flush continues draining across multiple bursts without wedging behind the wait-for-idle timer.

Reviewed changes

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

File Description
package.json Updates the tar override minimum version.
common/config/rush/pnpm-config.json Updates the Rush/pnpm tar globalOverride minimum version.
channels/1ds-post-js/src/PostChannel.ts Implements the non-wedging auto-flush behavior by avoiding wait-for-idle for fire-and-forget flushes.
channels/1ds-post-js/test/Unit/src/PostChannelTest.ts Adds a regression test covering the permanent-stall scenario.

Comment thread package.json
Comment on lines 79 to 83
"basic-ftp": ">=5.2.0",
"form-data": "^2.5.5",
"tar": "^7.5.13",
"tar": ">=7.5.16",
"glob": "^7.2.3",
"lodash": "^4.18.1",
Comment on lines 4 to 6
"minimatch": ">=3.1.5",
"tar": ">=7.5.13",
"tar": ">=7.5.16",
"glob": ">=7.2.3",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this a genuine concern?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not really, tar is only used internally not even a direct dependency in any code in this project, it get dragged with npm

Comment on lines +832 to +837
if (isAsync) {
// Clear the normal schedule timer as we are going to try and flush ASAP
_clearScheduledTimer();

// Move all queued events to the HttpManager so that we don't discard new events (Auto flush scenario)
_queueBatches(EventLatencyValue.Normal, EventSendType.Batched, SendRequestReason.MaxQueuedEvents);

@JacksonWeber JacksonWeber 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.

LGTM

@hectorhdzg hectorhdzg merged commit 634d976 into microsoft:main Jul 1, 2026
9 checks passed
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.

4 participants