Skip to content

fix: send the buffered writer's remaining lines at shutdown - #146

Merged
ecbadeaux merged 1 commit into
Netflix:mainfrom
andersvw:writer-flush-on-shutdown
Oct 20, 2026
Merged

ecbadeaux merged 1 commit into
Netflix:mainfrom
andersvw:writer-flush-on-shutdown

Conversation

@andersvw

@andersvw andersvw commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Notes

With a buffer size set, the writer sends only full buffers, and ~Writer joined the sending thread and dropped whatever was left, so a process that exits before it fills one buffer never sends those meters. ~Writer now sends the rest of the buffer, under the write mutex, after the sending thread has joined. NDrive hit this with a startup-failure counter that its sidecar records a second before it exits.

Testing

$ ctest --output-on-failure   # clang 19, Debug
100% tests passed, 0 tests failed out of 17

Just the new test without the source code fix:

$ ./writer_test
test_writer.cpp:51: Failure
nothing was sent at exit: Resource temporarily unavailable
[  FAILED  ] WriterShutdownDeathTest.SendsBufferedLinesAtExit

🤖 Generated with Claude Code

@andersvw
andersvw marked this pull request as ready for review October 19, 2026 23:42
Comment thread libs/writer/writer_wrapper/writer.cpp Outdated
The buffered writer sends only full buffers. When it is destroyed at exit, the sending thread stops and whatever is still in the buffer is dropped, so a process that exits before it fills one buffer never sends those meters. NDrive hit this with a counter it records when startup fails a second after launch: it never reached spectatord. ~Writer now sends the rest of the buffer after it joins the sending thread.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@andersvw
andersvw force-pushed the writer-flush-on-shutdown branch from 1ff4613 to 5d9294a Compare October 20, 2026 18:49
@ecbadeaux
ecbadeaux merged commit fed96bf into Netflix:main Oct 20, 2026
1 check 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.

2 participants