Conversation
Contributor
|
Droid finished @torcolvin's task —— View job Droid review (validator): Validated 0 inline comments (none proposed). Overall, the logger handoff looks like it fixes the InitLogging memory-buffer race without adding hot-path locking; removing the !race build tags for the audit tests matches that intent. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Concurrent logger replacement needs focused regression coverage and race-detector validation before approval.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
This PR fixes a race when Sync Gateway replaces startup memory loggers with configured loggers.
Changes:
- Synchronizes buffer transfer and forwards subsequent writes from retained memory loggers.
- Passes previous loggers through file and audit logger initialization.
- Re-enables audit tests under the race detector.
| File | Description |
|---|---|
| rest/cluster_compat_audit_test.go | Removes the race-test exclusion. |
| rest/audit_test.go | Removes the race-test exclusion. |
| base/logging_config.go | Passes previous logger instances during initialization. |
| base/logger_file.go | Synchronizes memory-log handoff and forwards later writes. |
| base/logger_audit.go | Passes the previous audit logger into file-logger initialization. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
InitLogging copied the memory loggers' buffers without a lock, which raced with any goroutine that was still logging to them. NewFileLogger now takes the logger it replaces. For a memory logger, it calls SetOutput to wait for in-flight writes and forward later writes to the new logger, then copies the buffer. Log writes take no new lock. Tests that call InitializeMemoryLoggers and then InitLogging can run under the race detector again, so remove the !race build tag from audit_test.go and cluster_compat_audit_test.go. The audit RestTester now disables log collation for every logger, because FlushLogBuffers waits on a collation WaitGroup while background goroutines log. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
InitLogging copied the memory loggers' buffers without a lock, which raced with any goroutine that was still logging to them. NewFileLogger now takes the logger it replaces. For a memory logger, it calls SetOutput to wait for in-flight writes and forward later writes to the new logger, then copies the buffer. Log writes take no new lock.
Tests that call InitializeMemoryLoggers and then InitLogging can run under the race detector again, so remove the !race build tag from audit_test.go and cluster_compat_audit_test.go. The audit RestTester now disables log collation for every logger, because FlushLogBuffers waits on a collation WaitGroup while background goroutines log.
Pre-review checklist
base.UD(docID),base.MD(dbName))docs/apiIntegration Tests