Skip to content

Ignore sync errors in a consistent way#403

Open
ianmiell wants to merge 1 commit into
mainfrom
imiell/ignore_sync_errors
Open

Ignore sync errors in a consistent way#403
ianmiell wants to merge 1 commit into
mainfrom
imiell/ignore_sync_errors

Conversation

@ianmiell
Copy link
Copy Markdown
Contributor

@ianmiell ianmiell commented May 25, 2026

Summary by CodeRabbit

  • Bug Fixes

    • Improved logging robustness to gracefully handle certain system-level synchronization errors during shutdown instead of treating them as failures.
  • Tests

    • Added tests to verify logging error handling behavior.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 51f966e3-cc38-4a4f-a384-fedeeff21cf9

📥 Commits

Reviewing files that changed from the base of the PR and between 535606d and 2a86d2e.

📒 Files selected for processing (7)
  • cmd/digest.go
  • cmd/riskdigest.go
  • cmd/run.go
  • cmd/users/create.go
  • cmd/users/update.go
  • internal/logging/sync.go
  • internal/logging/sync_test.go

📝 Walkthrough

Walkthrough

This PR introduces a centralized logger sync error filtering utility and applies it uniformly across five command modules. The IgnoreSyncError helper identifies benign Zap sync failures (nil, EINVAL, ENOTTY) that can be safely suppressed, replacing unconditional error checks in digest, risk-digest, server, and user commands.

Changes

Logger Sync Error Filtering

Layer / File(s) Summary
Logger sync utility and test coverage
internal/logging/sync.go, internal/logging/sync_test.go
New IgnoreSyncError(err error) bool function returns true for nil, syscall.EINVAL, and syscall.ENOTTY errors; comprehensive table-driven test validates behavior for both ignored and non-ignored error cases.
Digest command logger sync handling
cmd/digest.go
Digest command imports internal/logging and updates runDigestTest and runDigestPreview to filter deferred sugar.Sync() errors through IgnoreSyncError instead of checking err != nil.
Risk-digest command logger sync handling
cmd/riskdigest.go
Risk-digest command imports internal/logging and replaces direct error check with logging.IgnoreSyncError(err) in deferred sync handler.
Server command logger sync handling
cmd/run.go
Server command imports internal/logging and updates RunServer deferred logger sync handling to conditionally log errors using IgnoreSyncError filter.
User command logger sync handling
cmd/users/create.go, cmd/users/update.go
Create and update user commands import internal/logging and filter deferred logger sync errors through IgnoreSyncError instead of unconditional error reporting.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A sync error filtered clean and true,
Benign ENOTTY fades from view,
Five commands aligned, their logs run light,
The logger hops on, everything's right! 🎯

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: introducing consistent handling of sync errors across multiple command files via a new logging utility function.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

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.

1 participant