Skip to content

fix: re-enable async logging with proper shutdown flushing#2779

Draft
ivanauth wants to merge 3 commits intoauthzed:mainfrom
ivanauth:fix-async-logging-889
Draft

fix: re-enable async logging with proper shutdown flushing#2779
ivanauth wants to merge 3 commits intoauthzed:mainfrom
ivanauth:fix-async-logging-889

Conversation

@ivanauth
Copy link
Copy Markdown
Contributor

@ivanauth ivanauth commented Dec 17, 2025

Fixes #889

Async logging was disabled because nothing was flushing the diode.Writer buffer on shutdown - logs were getting lost. The cobrazerolog library hides the closer so we couldn't fix it there.

This creates our own logging setup that exposes the closer, stores it globally, and flushes it via defer log.Close() in main. Also restructured main() to return an int so defers actually run before exit.

@ivanauth ivanauth requested a review from a team as a code owner December 17, 2025 17:21
@github-actions github-actions Bot added the area/cli Affects the command line label Dec 17, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 17, 2025

Codecov Report

❌ Patch coverage is 85.61151% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.64%. Comparing base (fa85ab8) to head (96b842e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cmd/spicedb/main.go 0.00% 11 Missing ⚠️
pkg/cmd/server/logging_run_e.go 88.24% 4 Missing and 2 partials ⚠️
internal/logging/logger.go 95.84% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2779      +/-   ##
==========================================
- Coverage   73.66%   73.64%   -0.01%     
==========================================
  Files         499      500       +1     
  Lines       60165    60307     +142     
==========================================
+ Hits        44312    44409      +97     
- Misses      12666    12700      +34     
- Partials     3187     3198      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ivanauth ivanauth marked this pull request as draft December 17, 2025 17:40
@ivanauth ivanauth force-pushed the fix-async-logging-889 branch from d4136c5 to d5db6cf Compare December 17, 2025 18:01
@github-actions github-actions Bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Dec 17, 2025
@ivanauth ivanauth force-pushed the fix-async-logging-889 branch 6 times, most recently from acc473d to 3b4d309 Compare December 17, 2025 21:37
@ivanauth ivanauth force-pushed the fix-async-logging-889 branch 2 times, most recently from e15b28a to 31234ae Compare April 3, 2026 14:58
@ivanauth ivanauth force-pushed the fix-async-logging-889 branch from 31234ae to 19f4ae0 Compare April 7, 2026 15:25
@tstirrat15
Copy link
Copy Markdown
Contributor

We own cobrautil - it might make sense to fix it there.

@ivanauth
Copy link
Copy Markdown
Contributor Author

ivanauth commented Apr 8, 2026

Update: cobrautil fix submitted

Instead of duplicating cobrautil's logging setup, I've submitted a fix upstream: jzelinskie/cobrautil#66

That PR:

  • Exposes Builder.Close() to flush the async diode writer on shutdown
  • Fixes WithAsync(size, pollInterval) params being ignored (hardcoded to 1000/10ms)
  • Wraps os.Stderr to prevent diode.Writer.Close() from closing the fd
  • Guards against double RunE() leaking poll goroutines
  • Defaults size to 1000 if <= 0 to prevent panic

Once that's merged and tagged, this PR can be significantly simplified:

  • Delete logging_run_e.go and logging_run_e_test.go (no need to duplicate cobrautil's setup)
  • Delete noCloseWriter (cobrautil now handles this)
  • Simplify internal/logging/logger.go — store the *Builder ref, call builder.Close()
  • Keep the main()run() int restructuring (so defers run before os.Exit)
  • Bump cobrautil dependency

cc @tstirrat15 per your suggestion to fix it in cobrautil

@github-actions github-actions Bot added the area/dependencies Affects dependencies label Apr 10, 2026
@ivanauth ivanauth force-pushed the fix-async-logging-889 branch from 97e61d1 to 96b842e Compare April 16, 2026 15:22
@github-actions github-actions Bot removed the area/dependencies Affects dependencies label Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli Affects the command line area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

re-enable async logging

2 participants