Skip to content

cloudwatchlogs: buffer resetTimerCh to prevent lost wakeup in force-flush timer#2166

Merged
jefchien merged 2 commits into
aws:mainfrom
musa-asad:fix/flush-timer-lost-wakeup
Jul 2, 2026
Merged

cloudwatchlogs: buffer resetTimerCh to prevent lost wakeup in force-flush timer#2166
jefchien merged 2 commits into
aws:mainfrom
musa-asad:fix/flush-timer-lost-wakeup

Conversation

@musa-asad

@musa-asad musa-asad commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

A one-line concurrency fix that prevents the force-flush timer from permanently stalling on low-volume log streams.

Background

The CloudWatch Agent reads log files and publishes events to CloudWatch Logs. Each log stream has a force-flush timer (default 5 seconds) that sends accumulated events even when the batch is not full. This keeps delivery timely for low-volume streams.

The flush timer is one-shot: after it fires, resetFlushTimer() must be called to start it again. If the reset is dropped, the timer stays dead and events only leave when the batch fills up, which for a quiet stream may take minutes or longer.

Problem

resetFlushTimer() uses a non-blocking send:

select {
case q.resetTimerCh <- struct{}{}:
default:
}

When manageFlushTimer is blocked sending on flushCh (waiting for the main loop to accept a flush signal), it cannot receive from resetTimerCh. With an unbuffered channel, the rearm hits the default branch and is silently dropped. After a size-based send, this can leave the force-flush timer permanently un-rearmed. A low-volume stream's trailing batch then sits unsent with no error or retry logged.

Fix

// Before
resetTimerCh: make(chan struct{}),

// After
resetTimerCh: make(chan struct{}, 1),

A buffer of 1 parks the rearm signal so manageFlushTimer picks it up on its next loop iteration. The signal is idempotent (presence means "please reset"), so a buffer of 1 is sufficient.

Tests

Three new tests exercise the fix:

Test What it proves
TestResetTimerChIsBuffered resetTimerCh has buffer capacity 1; cap 0 reintroduces the bug.
TestForceFlushTimerRearmsAfterHeldSend A trailing event is flushed within 3× flushTimeout even when a size-based send held the flush path.
TestForceFlushTimer_SingleLowVolumeEventFlushes A single event on a quiet stream is flushed by the timer alone.
$ go test -count=1 -race -run "TestResetTimerCh|TestForceFlush" -timeout 30s ./plugins/outputs/cloudwatchlogs/internal/pusher/
ok  	github.com/aws/amazon-cloudwatch-agent/plugins/outputs/cloudwatchlogs/internal/pusher	4.060s

@musa-asad musa-asad requested review from jefchien and okankoAMZ June 19, 2026 00:16
@musa-asad musa-asad self-assigned this Jun 19, 2026
@musa-asad musa-asad force-pushed the fix/flush-timer-lost-wakeup branch from 976f9fe to 950d65b Compare June 19, 2026 00:17
@musa-asad musa-asad added the ready for testing Indicates this PR is ready for integration tests to run label Jun 19, 2026
@musa-asad musa-asad marked this pull request as ready for review June 19, 2026 00:18
@musa-asad musa-asad requested a review from a team as a code owner June 19, 2026 00:18
@musa-asad musa-asad force-pushed the fix/flush-timer-lost-wakeup branch 6 times, most recently from 3872ff5 to c690a7c Compare June 22, 2026 22:20
@musa-asad musa-asad changed the title cloudwatchlogs: fix lost wakeup that leaves the force-flush timer dead cloudwatchlogs:adding reslience lost wakeup that leaves the force-flush timer dead Jun 22, 2026
@musa-asad musa-asad changed the title cloudwatchlogs:adding reslience lost wakeup that leaves the force-flush timer dead cloudwatchlogs: adding reslience to lost wakeup that leaves the force-flush timer dead Jun 22, 2026
@musa-asad musa-asad force-pushed the fix/flush-timer-lost-wakeup branch 6 times, most recently from 461771e to 70af2ad Compare June 23, 2026 04:27
@musa-asad musa-asad changed the title cloudwatchlogs: adding reslience to lost wakeup that leaves the force-flush timer dead cloudwatchlogs: buffer resetTimerCh to prevent lost wakeup in force-flush timer Jun 23, 2026
@musa-asad musa-asad force-pushed the fix/flush-timer-lost-wakeup branch 2 times, most recently from 127bb7f to bcf8894 Compare June 23, 2026 05:09
Comment thread plugins/outputs/cloudwatchlogs/internal/pusher/queue.go Outdated
Comment thread plugins/outputs/cloudwatchlogs/internal/pusher/queue.go Outdated
Comment thread plugins/outputs/cloudwatchlogs/internal/pusher/queue.go Outdated
Comment thread plugins/outputs/cloudwatchlogs/internal/pusher/queue.go Outdated
Comment thread plugins/outputs/cloudwatchlogs/internal/pusher/lostwakeup_test.go Outdated
Comment thread plugins/outputs/cloudwatchlogs/internal/pusher/lostwakeup_test.go Outdated
Comment thread plugins/outputs/cloudwatchlogs/internal/pusher/queue.go Outdated
Comment thread plugins/outputs/cloudwatchlogs/internal/pusher/queue.go Outdated
@musa-asad musa-asad force-pushed the fix/flush-timer-lost-wakeup branch 3 times, most recently from cf9fdeb to 1d86fc7 Compare June 23, 2026 19:41
@musa-asad musa-asad requested a review from jefchien June 23, 2026 20:05
@musa-asad musa-asad force-pushed the fix/flush-timer-lost-wakeup branch from 1d86fc7 to 84cad0b Compare June 23, 2026 20:17
@musa-asad musa-asad force-pushed the fix/flush-timer-lost-wakeup branch 3 times, most recently from 2bf2aa3 to 5bdd682 Compare June 30, 2026 04:16
@musa-asad musa-asad marked this pull request as ready for review June 30, 2026 14:46
Comment thread plugins/outputs/cloudwatchlogs/internal/pusher/queue.go Outdated
Comment thread plugins/outputs/cloudwatchlogs/internal/pusher/queue.go
logLines := logSink.Lines()
lastLine := logLines[len(logLines)-1]
expected := fmt.Sprintf("All %v retries to G/S failed for PutLogEvents, request dropped.", cnt.Load()-1)
require.True(t, strings.HasSuffix(lastLine, expected), fmt.Sprintf("Expecting error log to end with request dropped, but received '%s' in the log", logSink.String()))

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.

why are we no longer checking for last line? why do we check ever line instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So we can find the drop message regardless of its position.

Comment thread plugins/outputs/cloudwatchlogs/internal/pusher/queue.go Outdated
@musa-asad musa-asad force-pushed the fix/flush-timer-lost-wakeup branch 2 times, most recently from b908524 to d2f703e Compare June 30, 2026 18:11
@musa-asad musa-asad marked this pull request as draft June 30, 2026 18:20
@musa-asad musa-asad force-pushed the fix/flush-timer-lost-wakeup branch 2 times, most recently from a9c3c14 to 816e057 Compare June 30, 2026 19:12
@musa-asad musa-asad requested a review from okankoAMZ June 30, 2026 19:24
@musa-asad musa-asad marked this pull request as ready for review June 30, 2026 19:25
@musa-asad musa-asad force-pushed the fix/flush-timer-lost-wakeup branch 12 times, most recently from 4851433 to ef0e56a Compare July 1, 2026 19:59
@musa-asad musa-asad force-pushed the fix/flush-timer-lost-wakeup branch from ef0e56a to 722ebc0 Compare July 1, 2026 19:59
@jefchien jefchien merged commit a53d263 into aws:main Jul 2, 2026
19 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for testing Indicates this PR is ready for integration tests to run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants