fix: [#726] scope log handler cache to the application - #1482
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1482 +/- ##
==========================================
- Coverage 69.40% 69.40% -0.01%
==========================================
Files 377 378 +1
Lines 30190 30199 +9
==========================================
+ Hits 20954 20959 +5
- Misses 8256 8258 +2
- Partials 980 982 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes an application-restart correctness issue in the logging subsystem by scoping the log channel handler cache to a log.Application instance, rather than using a package-level global cache that could outlive the container and retain stale telemetry resources.
Changes:
- Replaced the package-level handler cache with an
Application-scopedhandlerCacheshared acrossWithContext/Channel/Stackderived instances. - Updated
getHandlersto take an explicit cache parameter and thread it through stack recursion. - Removed the prior global-cache test helper and added a test ensuring cache sharing/leakage behavior matches the new scoping.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| log/application.go | Introduces per-Application handler cache and ensures derived instances share it while new root apps don’t. |
| log/application_test.go | Removes global cache helper; adds coverage verifying cache scope and updates getHandlers call sites. |
| log/writer_test.go | Removes now-unneeded cache clearing calls that were compensating for the prior global cache. |
| config config.Config | ||
| json foundation.Json | ||
| telemetryResolver contractstelemetry.Resolver | ||
| handlerCache *sync.Map |
There was a problem hiding this comment.
Is a pointer required?
| handlerCache *sync.Map | |
| channelToHandlers sync.Map |
There was a problem hiding this comment.
It is required, we want an application level cache here. Channel/WithContext/Stack create a new Application each call and all of them should share the same cache. If we make it a value, each instance gets a copy of the map, so if someone calls Channel multiple times it will create the handlers each time since whatever was cached stays in the copy. Also vet doesn't allow copying sync.Map by value, it has a mutex inside.
There was a problem hiding this comment.
But I have renamed handlerCache -> channelToHandlers
📑 Description
RelatedTo goravel/goravel#726
The log module caches channel handlers in a package level
sync.Map, so they survivefacades.App().Restart(). Handlers that hold per container resources keep using the old ones after the restart; for the otel channel this means emitting into a shut down logger provider, which silently drops every record.Scope the cache to the
Applicationinstead: instances derived viaWithContext/Channel/Stackshare it, and it dies with the container. Also removes theclearChannelCache()test helper that existed to work around the global.@coderabbitai summary
✅ Checks