[pubsub] introduce memory and redis implementations #181
Conversation
WalkthroughAdds a Pub/Sub abstraction with in-memory and Redis backends, options and fx wiring; integrates Pub/Sub into sms-gateway. Events and push wrappers are JSON-serialized and delivered via Pub/Sub. Cache API gains Close semantics. Push and events modules refactored for serialized, cache-backed retry/blacklist delivery. Changes
Sequence Diagram(s)sequenceDiagram
participant Producer as Notify
participant PubSub as PubSub (memory/redis)
participant Consumer as Run
rect rgba(246,240,230,0.95)
Producer->>Producer: build eventWrapper\njson.Marshal (with timeout)
Producer->>PubSub: Publish(ctx, "events", data)
PubSub-->>Producer: return (err / nil)
end
rect rgba(234,247,239,0.95)
Consumer->>PubSub: Subscribe(ctx, "events")
PubSub-->>Consumer: Subscription (chan Message)
loop For each message
PubSub-->>Consumer: Message{Topic, Data}
Consumer->>Consumer: json.Unmarshal -> eventWrapper
Consumer->>Consumer: processEvent(&eventWrapper)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Points to review closely:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/pubsub/memory.go(1 hunks)pkg/pubsub/options.go(1 hunks)pkg/pubsub/pubsub.go(1 hunks)pkg/pubsub/redis.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/pubsub/memory.go (2)
pkg/pubsub/pubsub.go (5)
Message(13-16)ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)PubSub(45-50)pkg/pubsub/options.go (1)
Option(3-3)
pkg/pubsub/redis.go (2)
pkg/pubsub/options.go (1)
Option(3-3)pkg/pubsub/pubsub.go (5)
ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)Message(13-16)PubSub(45-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: E2E
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/pubsub/memory.go(1 hunks)pkg/pubsub/redis.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/pubsub/memory.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/pubsub/redis.go (2)
pkg/pubsub/options.go (1)
Option(3-3)pkg/pubsub/pubsub.go (5)
ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)Message(13-16)PubSub(45-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Benchmark
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
pkg/pubsub/redis.go (5)
1-22: LGTM!The imports and struct definition are well-organized with appropriate synchronization primitives and fields.
24-39: LGTM!The constructor properly applies the options pattern and initializes all struct fields correctly.
41-53: LGTM!The Publish method correctly validates the closed state and topic, and properly delegates to the Redis client.
82-112: LGTM!The goroutine implementation is correct with proper cleanup, comprehensive exit path handling, and safe message forwarding.
130-130: LGTM!The compile-time interface assertion correctly verifies that
redisPubSubimplements thePubSubinterface.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/pubsub/memory.go(1 hunks)pkg/pubsub/redis.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/pubsub/redis.go (2)
pkg/pubsub/options.go (1)
Option(3-3)pkg/pubsub/pubsub.go (5)
ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)Message(13-16)PubSub(45-50)
pkg/pubsub/memory.go (2)
pkg/pubsub/pubsub.go (5)
Message(13-16)ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)PubSub(45-50)pkg/pubsub/options.go (1)
Option(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build / Docker image (linux/amd64)
- GitHub Check: Build / Docker image (linux/arm64)
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
pkg/pubsub/memory.go (2)
33-73: LGTM! Publish synchronization is safe.The
RLockheld via defer (line 45) ensures the lock isn't released until afterwg.Wait()completes (line 70), preventing the race whereunsubscribecould remove and close a channel while publisher goroutines are sending. The past review concerns about concurrent map access have been properly addressed.
132-143: Close is idempotent and thread-safe.Returning
nilon repeated calls (line 135) matches the Redis implementation's behavior, resolving the API consistency concern from past reviews.pkg/pubsub/redis.go (2)
56-117: LGTM! Subscription lifecycle is correct.The Subscribe implementation properly waits for Redis confirmation (line 68) before proceeding, and handles errors by closing the pubsub object (line 70). The goroutine's defer block (lines 85-94) cleans up resources in the correct order: close Redis subscription, close local channel, remove tracking, signal completion.
119-130: Past review comment about Close behavior is now outdated.The past review suggested aligning Close behavior between memory and Redis implementations. Both now return
nilon repeated calls (memory.go line 135, redis.go line 122), making them consistent. The concern has been resolved.
capcom6
force-pushed
the
events/multi-instance-support
branch
from
84dfae9 to
737cc55
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/pubsub/memory.go (2)
121-130: Consider using defer for mutex unlock.The explicit unlock pattern works correctly, but using
deferwould be more idiomatic and maintainable.Apply this diff to use defer:
func (m *memoryPubSub) unsubscribe(id, topic string) { m.mu.Lock() + defer m.mu.Unlock() + subscriptions, ok := m.topics[topic] if !ok { - m.mu.Unlock() return } delete(subscriptions, id) - m.mu.Unlock() }
44-72: Document blocking Publish behavior.
Publishblocks until all subscribers receive the message or the context/close signal is triggered. This differs from the Redis implementation (which returns immediately after sending to Redis) and could surprise callers expecting fire-and-forget semantics.Consider adding a comment documenting this behavior:
// Publish sends a message to all subscribers of the given topic. // This method blocks until all subscribers have received the message // or until ctx is cancelled or the pubsub instance is closed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/pubsub/memory.go(1 hunks)pkg/pubsub/options.go(1 hunks)pkg/pubsub/pubsub.go(1 hunks)pkg/pubsub/redis.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/pubsub/pubsub.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/pubsub/memory.go (2)
pkg/pubsub/pubsub.go (5)
Message(13-16)ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)PubSub(45-50)pkg/pubsub/options.go (1)
Option(3-3)
pkg/pubsub/redis.go (2)
pkg/pubsub/options.go (1)
Option(3-3)pkg/pubsub/pubsub.go (5)
ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)Message(13-16)PubSub(45-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
pkg/pubsub/redis.go (1)
119-130: Close behavior is now consistent.Both memory and Redis implementations now return
nilon repeatedClose()calls, providing consistent idempotent semantics. This resolves the inconsistency mentioned in previous reviews.
capcom6
force-pushed
the
events/multi-instance-support
branch
from
66878bf to
56f4ebd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/pubsub/config.go(1 hunks)internal/sms-gateway/pubsub/module.go(1 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/sms-gateway/pubsub/module.go (3)
internal/config/module.go (1)
Module(22-131)internal/sms-gateway/app.go (1)
Module(36-60)internal/sms-gateway/pubsub/pubsub.go (1)
New(15-40)
internal/config/module.go (2)
internal/config/config.go (2)
Config(10-19)Cache(75-77)internal/sms-gateway/pubsub/config.go (1)
Config(4-7)
internal/sms-gateway/app.go (2)
internal/config/module.go (1)
Module(22-131)internal/sms-gateway/pubsub/module.go (1)
Module(8-16)
internal/sms-gateway/pubsub/pubsub.go (3)
internal/config/config.go (2)
Config(10-19)PubSub(79-81)internal/sms-gateway/pubsub/config.go (1)
Config(4-7)pkg/pubsub/options.go (1)
WithBufferSize(17-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
internal/config/config.go (3)
18-18: LGTM!The PubSub configuration field is correctly added to the Config struct, following the same pattern as the existing Cache configuration.
79-81: LGTM!The PubSub struct definition is consistent with the existing Cache struct pattern and correctly configured with appropriate YAML and environment variable tags.
111-113: LGTM!The default PubSub configuration with "memory://" URL is appropriate for development and testing, consistent with the Cache default.
internal/sms-gateway/pubsub/config.go (1)
3-7: LGTM!The Config struct is simple and clear, appropriately defining the parameters needed to control the PubSub backend via URL and configure the buffer size.
internal/sms-gateway/app.go (2)
24-24: LGTM!The import for the pubsub package is correctly added.
49-49: LGTM!The pubsub.Module() is correctly added to the fx.Module initialization, following the same pattern as other infrastructure modules.
internal/config/module.go (1)
14-14: LGTM!The import for the pubsub package is correctly added.
internal/sms-gateway/pubsub/module.go (1)
8-16: LGTM!The Module function correctly wires the pubsub subsystem into fx, with appropriate logger decoration and constructor provision.
internal/sms-gateway/pubsub/pubsub.go (4)
11-13: LGTM!The topicPrefix constant is appropriately defined to namespace Redis topics.
16-23: LGTM!URL defaulting and parsing logic is correct, with appropriate error handling and wrapping.
25-26: LGTM!Options are correctly built with the configured BufferSize.
28-39: LGTM!The scheme-based backend selection is clean and extensible. The Redis client lifecycle is properly managed by the NewRedis implementation (assuming it handles cleanup when the PubSub is closed).
capcom6
force-pushed
the
events/multi-instance-support
branch
from
bc2f812 to
c96b2b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/config/module.go (1)
125-130: Previous issue resolved; consider documenting BufferSize choice.The past review concern about using
cfg.Cache.URLinstead ofcfg.PubSub.URLhas been fixed - line 127 now correctly referencescfg.PubSub.URL.The hardcoded
BufferSize: 128could benefit from a brief comment explaining the rationale (e.g., "reasonable default for in-memory queuing") or extraction to a named constant if this value is significant.Apply this diff to add a clarifying comment:
fx.Provide(func(cfg Config) pubsub.Config { return pubsub.Config{ URL: cfg.PubSub.URL, - BufferSize: 128, + BufferSize: 128, // Default channel buffer size for pub/sub message queuing } }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/pubsub/config.go(1 hunks)internal/sms-gateway/pubsub/module.go(1 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sms-gateway/pubsub/config.go
- internal/sms-gateway/app.go
🧰 Additional context used
🧬 Code graph analysis (3)
internal/sms-gateway/pubsub/module.go (3)
internal/config/module.go (1)
Module(22-131)internal/sms-gateway/app.go (1)
Module(36-60)internal/sms-gateway/pubsub/pubsub.go (1)
New(15-40)
internal/config/module.go (2)
internal/config/config.go (2)
Config(10-19)PubSub(79-81)internal/sms-gateway/pubsub/config.go (1)
Config(4-7)
internal/sms-gateway/pubsub/pubsub.go (2)
internal/sms-gateway/pubsub/config.go (1)
Config(4-7)pkg/pubsub/options.go (1)
WithBufferSize(17-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/config/config.go (1)
18-18: LGTM! Configuration follows established patterns.The PubSub configuration is structured identically to the Cache configuration, with appropriate YAML and environment variable tags, and a sensible default of
memory://for local development.Also applies to: 79-82, 111-113
internal/sms-gateway/pubsub/module.go (1)
8-16: LGTM! FX module wiring is correct.The module follows the established pattern for FX modules in this codebase, with appropriate logger decoration and constructor provision.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/sms-gateway/pubsub/module.go (1)
18-24: PubSub.Close() is not context-aware; keep the call or change the API — rename the parameter to avoid shadowingVerified: pkg/pubsub/pubsub.go declares Close() error, so OnStop's context cannot be forwarded to Close() as-is. Keep the current call (returns error correctly) or change the PubSub API to Close(context.Context) error if you need context-aware shutdown. Minor nit: rename the parameter
pubsub(e.g.,psorpubsubSvc) to avoid shadowing the package.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/sms-gateway/pubsub/module.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/pubsub/module.go (4)
internal/sms-gateway/app.go (1)
Module(36-60)internal/config/module.go (1)
Module(22-131)internal/sms-gateway/pubsub/pubsub.go (1)
New(15-40)internal/config/config.go (1)
PubSub(79-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/sms-gateway/pubsub/module.go (2)
1-9: LGTM!Package declaration and imports are clean and appropriate for an fx module with lifecycle management.
11-16: LGTM!Module structure and logger decoration follow fx best practices. The named logger will provide clear context in log output.
capcom6
force-pushed
the
events/multi-instance-support
branch
from
c5c19f6 to
01f4557
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/sms-gateway/pubsub/config.go (1)
5-6: Consider adding field documentation and struct tags.The exported fields lack individual documentation. While the type-level comment is helpful, documenting each field improves maintainability and helps users understand the purpose and constraints of each field.
Additionally, if this Config is unmarshaled from YAML/JSON configuration files, consider adding appropriate struct tags.
Apply this diff to add field documentation:
// Config controls the PubSub backend via a URL (e.g., "memory://", "redis://..."). type Config struct { + // URL specifies the backend to use (e.g., "memory://", "redis://host:port"). + // Defaults to "memory://" if empty. URL string + // BufferSize sets the channel buffer size for the Pub/Sub implementation. BufferSize uint }If config unmarshaling is used, also add struct tags:
type Config struct { - URL string - BufferSize uint + URL string `yaml:"url" json:"url"` + BufferSize uint `yaml:"buffer_size" json:"buffer_size"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/pubsub/config.go(1 hunks)internal/sms-gateway/pubsub/module.go(1 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/config/config.go
- internal/sms-gateway/pubsub/module.go
- internal/sms-gateway/pubsub/pubsub.go
- internal/config/module.go
- internal/sms-gateway/app.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
internal/sms-gateway/pubsub/config.go (1)
6-6: No action needed: BufferSize is initialized to 128 ininternal/config/module.goand always passed viaWithBufferSize, so it never remains zero at runtime.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sms-gateway/modules/events/service.go (1)
97-142: Defensive check for nil event in consumed messages.eventWrapper.Event can be nil after unmarshal; prevent panic and record a failure.
func (s *Service) processEvent(wrapper *eventWrapper) { + if wrapper.Event == nil { + s.metrics.IncrementFailed("unknown", DeliveryTypeUnknown, FailureReasonSerializationError) + s.logger.Error("Received event wrapper with nil event", zap.String("user_id", wrapper.UserID)) + return + }
♻️ Duplicate comments (1)
internal/sms-gateway/pubsub/pubsub.go (1)
34-38: Ensure Redis client is closed by PubSub.Close() (connection leak risk).Confirm pkgpubsub.NewRedis’s Close() calls client.Close().
#!/bin/bash # Verify redis PubSub Close closes the underlying client rg -nP 'type\s+\w+\s+struct\s*\{[^}]*client[^\n]*\n[^}]*\}' --type=go -C3 rg -nP 'func\s*\(\s*\w+\s*\*\w+\s*\)\s*Close\(\)\s*error' --type=go -C3 rg -n 'client\.Close\(\)' --type=go -C3
🧹 Nitpick comments (6)
internal/sms-gateway/modules/events/metrics.go (1)
22-25: Consider adding a distinct deserialization failure reason.Consumer-side JSON parse failures are different from producer-side serialization. Add FailureReasonDeserializationError for clearer metrics and use it in the Run loop on unmarshal errors.
internal/sms-gateway/pubsub/pubsub.go (3)
7-9: Avoid same-name import shadowing for clarity.Alias the external pkg to reduce confusion within package pubsub.
-import ( - "fmt" - "net/url" - - "github.com/android-sms-gateway/core/redis" - "github.com/android-sms-gateway/server/pkg/pubsub" -) +import ( + "fmt" + "net/url" + + "github.com/android-sms-gateway/core/redis" + pkgpubsub "github.com/android-sms-gateway/server/pkg/pubsub" +) - type PubSub = pubsub.PubSub + type PubSub = pkgpubsub.PubSubAlso applies to: 15-15
27-29: Only set buffer size when > 0 (avoid forcing zero/unbuffered).Let backends keep their default when config.BufferSize == 0.
-opts := []pubsub.Option{} -opts = append(opts, pubsub.WithBufferSize(config.BufferSize)) +opts := []pkgpubsub.Option{} +if config.BufferSize > 0 { + opts = append(opts, pkgpubsub.WithBufferSize(config.BufferSize)) +}
30-41: Support rediss (TLS) scheme.Common Redis URLs use rediss://. Treat it like redis.
-switch u.Scheme { +switch u.Scheme { case "memory": - return pubsub.NewMemory(opts...), nil + return pkgpubsub.NewMemory(opts...), nil -case "redis": +case "redis", "rediss": client, err := redis.New(redis.Config{URL: config.URL}) if err != nil { return nil, fmt.Errorf("can't create redis client: %w", err) } - return pubsub.NewRedis(client, topicPrefix, opts...), nil + return pkgpubsub.NewRedis(client, topicPrefix, opts...), nilinternal/sms-gateway/modules/events/types.go (1)
9-18: Unify field name with push/SSE types for consistency.Consider renaming EventType -> Type (json:"event") to match push/sse.Event and reduce mental mapping across modules. If changed, adjust callers accordingly.
internal/sms-gateway/modules/events/service.go (1)
104-105: Avoid shadowing the imported package name.Rename local variable from devices to devs to keep readability.
- devices, err := s.deviceSvc.Select(wrapper.UserID, filters...) + devs, err := s.deviceSvc.Select(wrapper.UserID, filters...) @@ - if len(devices) == 0 { + if len(devs) == 0 { @@ - for _, device := range devices { + for _, device := range devs {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/sms-gateway/modules/events/metrics.go(1 hunks)internal/sms-gateway/modules/events/service.go(2 hunks)internal/sms-gateway/modules/events/types.go(1 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/sms-gateway/modules/events/types.go (3)
internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)
internal/sms-gateway/pubsub/pubsub.go (3)
internal/config/config.go (2)
PubSub(79-81)Config(10-19)internal/sms-gateway/pubsub/config.go (1)
Config(4-7)pkg/pubsub/options.go (1)
WithBufferSize(17-21)
internal/sms-gateway/modules/events/service.go (5)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(15-15)internal/sms-gateway/modules/events/types.go (1)
Event(9-12)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/events/metrics.go (6)
DeliveryTypeUnknown(20-20)FailureReasonSerializationError(22-22)FailureReasonPublishError(23-23)DeliveryTypePush(18-18)FailureReasonProviderFailed(24-24)DeliveryTypeSSE(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/sms-gateway/modules/events/module.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/modules/events/module.go (1)
internal/sms-gateway/modules/events/service.go (3)
Service(19-30)s(59-69)deviceSvc(13-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Test
- GitHub Check: Analyze (go)
capcom6
force-pushed
the
events/multi-instance-support
branch
from
a348cb4 to
4e59e2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/sms-gateway/modules/events/service.go (1)
101-146: Validate wrapper.Event to prevent nil-pointer dereference.The
processEventfunction accesseswrapper.Event.EventTypeandwrapper.Event.Data(lines 124-125, 137-138) without checking ifwrapper.Eventis nil. If deserialization produces a wrapper with a nil Event field, this will cause a panic.Add validation at the start of processEvent:
func (s *Service) processEvent(wrapper *eventWrapper) { + if wrapper.Event == nil { + s.logger.Error("Event wrapper has nil event", zap.String("user_id", wrapper.UserID)) + s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) + return + } + // Load devices from database filters := []devices.SelectFilter{}pkg/cache/redis.go (1)
191-197: Bug: HExpireAt missing field argument (compile/runtime error).HExpireAt requires the field; here it’s called without it. This will fail to compile or misbehave and leaves entries without TTL.
Apply this fix:
- if !options.validUntil.IsZero() { - if err := r.client.HExpireAt(ctx, r.key, options.validUntil).Err(); err != nil { - return fmt.Errorf("can't set cache item ttl: %w", err) - } - } + if !options.validUntil.IsZero() { + if err := r.client.HExpireAt(ctx, r.key, options.validUntil, key).Err(); err != nil { + return fmt.Errorf("can't set cache item ttl: %w", err) + } + }
🧹 Nitpick comments (2)
pkg/cache/redis.go (1)
175-197: Consider atomic SetNX + TTL in one step.Today, HSetNX then HExpireAt leaves a small window where the field exists without TTL. If you need strict atomicity, wrap both in a Lua script.
I can provide a compact script that performs HSETNX and, only on insert, applies HEXPIREAT in one EVAL. Interested?
internal/sms-gateway/cache/factory.go (1)
34-54: Supportrediss://URLs.Switch only accepts "redis". Consider handling "rediss" (TLS) the same way.
Example:
- switch u.Scheme { - case "memory": + switch u.Scheme { + case "memory": ... - case "redis": + case "redis", "rediss": ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/sms-gateway/cache/factory.go(1 hunks)internal/sms-gateway/modules/events/metrics.go(1 hunks)internal/sms-gateway/modules/events/module.go(1 hunks)internal/sms-gateway/modules/events/service.go(2 hunks)internal/sms-gateway/modules/events/types.go(1 hunks)internal/sms-gateway/modules/messages/service.go(2 hunks)internal/sms-gateway/modules/settings/service.go(1 hunks)internal/sms-gateway/modules/webhooks/service.go(1 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)pkg/cache/redis.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/sms-gateway/modules/messages/service.go
- internal/sms-gateway/modules/webhooks/service.go
- internal/sms-gateway/modules/settings/service.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T23:53:42.006Z
Learnt from: capcom6
PR: android-sms-gateway/server#178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Applied to files:
pkg/cache/redis.go
🧬 Code graph analysis (7)
internal/sms-gateway/modules/events/service.go (5)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)internal/sms-gateway/modules/events/types.go (1)
Event(9-12)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/events/metrics.go (7)
DeliveryTypeUnknown(20-20)FailureReasonSerializationError(22-22)FailureReasonPublishError(23-23)EventTypeUnknown(26-26)DeliveryTypePush(18-18)FailureReasonProviderFailed(24-24)DeliveryTypeSSE(19-19)
internal/sms-gateway/modules/events/metrics.go (1)
internal/sms-gateway/modules/sse/metrics.go (1)
m(85-87)
internal/sms-gateway/modules/events/types.go (3)
internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)
internal/sms-gateway/cache/factory.go (1)
pkg/cache/redis.go (2)
NewRedis(61-86)RedisConfig(37-51)
pkg/cache/redis.go (2)
internal/config/config.go (1)
Cache(75-77)pkg/cache/cache.go (1)
Cache(5-36)
internal/sms-gateway/modules/events/module.go (2)
internal/sms-gateway/modules/events/service.go (1)
Service(19-30)internal/sms-gateway/app.go (1)
Run(62-73)
internal/sms-gateway/pubsub/pubsub.go (4)
internal/config/config.go (2)
PubSub(79-81)Config(10-19)internal/sms-gateway/pubsub/config.go (1)
Config(4-7)pkg/pubsub/options.go (1)
WithBufferSize(17-21)pkg/cache/redis.go (1)
RedisConfig(37-51)
🪛 GitHub Check: Lint
internal/sms-gateway/pubsub/pubsub.go
[failure] 37-37:
not enough arguments in call to pubsub.NewRedis
[failure] 33-33:
undefined: pubsub.RedisConfig
[failure] 33-33:
not enough return values
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
internal/sms-gateway/modules/events/module.go (1)
17-28: LGTM! Well-structured error handling.The updated fx.Invoke properly injects logger and shutdowner dependencies, and the goroutine correctly handles service errors by logging and triggering graceful shutdown with an exit code.
internal/sms-gateway/modules/events/metrics.go (1)
22-26: LGTM! Constants align with pub/sub integration.The new failure reason constants (serialization_error, publish_error) and EventTypeUnknown appropriately categorize failures in the pub/sub-based event flow, while removing the now-unused FailureReasonQueueFull.
internal/sms-gateway/modules/events/types.go (2)
10-11: LGTM! Public fields enable JSON serialization.The Event struct now has public fields with appropriate JSON tags, supporting the pub/sub payload serialization requirements.
27-33: LGTM! Standard serialization methods.The serialize/deserialize methods correctly use
json.Marshalandjson.Unmarshalfor converting eventWrapper to/from JSON bytes.internal/sms-gateway/modules/events/service.go (1)
79-99: LGTM! Subscription loop handles channel closure correctly.The receive loop properly checks the
okvalue from the channel receive operation (line 85) and gracefully returns when the subscription channel closes (lines 86-89). Deserialization errors are correctly logged and counted without exposing PII.pkg/cache/redis.go (2)
103-122: Drain script LGTM; atomic and returns only non‑expired fields.EVAL with HGETALL + UNLINK/DEL is atomic and, with field TTLs, returns only live entries. Good alignment with Redis field‑TTL semantics.
Based on learnings
160-166: Pipelined HSET + HExpireAt is correct.Efficient and reduces round trips; TTL set only when configured. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/pubsub/redis.go (1)
45-56: Document Client/URL precedence behavior.When both
ClientandURLare provided inRedisConfig, theClienttakes precedence andURLis silently ignored (lines 49-56 only execute whenClient == nil). This precedence should be documented in theRedisConfigstruct orNewRedisfunction comments to avoid confusion.Consider adding a comment to the
RedisConfigstruct:type RedisConfig struct { // Client is the Redis client to use. - // If nil, a client is created from the URL. + // If nil, a client is created from the URL. + // If both Client and URL are provided, Client takes precedence. Client *redis.Client
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/pubsub/redis.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/pubsub/redis.go (2)
pkg/pubsub/options.go (1)
Option(3-3)pkg/pubsub/pubsub.go (5)
ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)Message(13-16)PubSub(45-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
pkg/pubsub/redis.go (2)
99-104: Resource leak fixed - LGTM!The subscription error path now properly closes the Redis subscription before returning, addressing the resource leak flagged in previous reviews. The use of
errors.Jointo combine both errors is appropriate.
117-126: Cleanup logic is well-structured.The deferred cleanup properly closes the Redis subscription, closes the channel, removes the subscriber from the registry, and signals completion via
wg.Done(). Ignoring the error fromps.Close()is acceptable in the cleanup path.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/pubsub/redis.go (2)
75-87: Document non-blocking Publish behavior.This is a previously identified issue that remains unaddressed. Redis
Publishis fire-and-forget (returns immediately after sending to Redis), while the memory implementation blocks until all subscribers receive. Add a comment documenting this behavioral difference.
160-171: Fix resource leak and align Close behavior with memory implementation.Two issues:
Resource leak (new issue): When
NewRediscreates a Redis client from URL (line 56), that client is never closed. This leaks connections and file descriptors.Inconsistent behavior (duplicate from past review): Returns
nilon repeated calls, butmemory.goreturnsErrPubSubClosed. This behavioral inconsistency across implementations could surprise users.To fix the resource leak, track client ownership:
type redisPubSub struct { prefix string bufferSize uint client *redis.Client + ownsClient bool wg sync.WaitGroup mu sync.Mutex subscribers map[string]context.CancelFunc closeCh chan struct{} }In
NewRedis, set ownership:if config.Client == nil { opt, err := redis.ParseURL(config.URL) if err != nil { return nil, fmt.Errorf("failed to parse redis url: %w", err) } config.Client = redis.NewClient(opt) + return &redisPubSub{ + // ... other fields ... + client: config.Client, + ownsClient: true, + }, nil } return &redisPubSub{ - prefix: config.Prefix, - bufferSize: o.bufferSize, - - client: config.Client, - - subscribers: make(map[string]context.CancelFunc), - closeCh: make(chan struct{}), + // ... other fields ... + client: config.Client, + ownsClient: false, }, nilIn
Close, close the client if owned:func (r *redisPubSub) Close() error { select { case <-r.closeCh: - return nil + return ErrPubSubClosed default: close(r.closeCh) } r.wg.Wait() + if r.ownsClient { + return r.client.Close() + } + return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/sms-gateway/cache/factory.go(1 hunks)internal/sms-gateway/modules/events/service.go(2 hunks)pkg/pubsub/redis.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/cache/factory.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/pubsub/redis.go (2)
pkg/pubsub/options.go (1)
Option(3-3)pkg/pubsub/pubsub.go (5)
ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)Message(13-16)PubSub(45-50)
internal/sms-gateway/modules/events/service.go (3)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)internal/sms-gateway/modules/events/types.go (1)
Event(9-12)internal/sms-gateway/modules/events/metrics.go (7)
EventTypeUnknown(26-26)DeliveryTypeUnknown(20-20)FailureReasonSerializationError(22-22)FailureReasonPublishError(23-23)DeliveryTypePush(18-18)FailureReasonProviderFailed(24-24)DeliveryTypeSSE(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
internal/sms-gateway/modules/events/service.go (4)
15-17: LGTM: Good practice to use a constant for the pub/sub topic.The constant makes the topic name maintainable and follows DRY principles.
32-44: LGTM: Clean dependency injection.The pub/sub dependency is properly injected through the constructor and stored in the Service struct.
77-104: LGTM: Proper subscription lifecycle management.The method correctly:
- Checks for subscription errors and returns early
- Defers subscription closure
- Handles the closed channel case with the
okcheck- Avoids PII leakage by not logging the full message payload
- Returns cleanly on context cancellation or channel close
This addresses the previous review comments about channel handling and PII logging.
106-151: LGTM: Proper event processing with metrics.The method correctly:
- Handles device selection with appropriate filters
- Implements fallback from push to SSE for devices without push tokens
- Increments metrics for both success and failure cases on each delivery path
- Logs errors with relevant context (user_id, device_id) without exposing sensitive event data
pkg/pubsub/redis.go (2)
100-105: LGTM! Subscription confirmation correctly implemented.The code now properly waits for the Redis subscription acknowledgment using
ps.Receive(ctx)before returning the subscription. The error handling on lines 103-104 correctly closes the subscription and combines both errors usingerrors.Join, which is an improvement over simply ignoring the close error.
117-155: LGTM! Goroutine correctly handles cleanup and prevents deadlock.The message forwarding goroutine is well-structured:
- Proper cleanup in defer (closes subscription, channel, removes from map)
- Responds to close signals (
r.closeChandsubCtx.Done())- The nested select (lines 143-152) prevents deadlock by checking close signals while attempting to send messages
This addresses the previously identified deadlock concern.
capcom6
force-pushed
the
events/multi-instance-support
branch
from
af41637 to
2038311
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cache/redis.go (1)
194-198: Fix TTL handling inSetOrFail.When a TTL is supplied, this path calls
HExpireAtwithout the hash field argument, so the command targets the whole hash (or fails with a Redis error). That breaks per-entry TTL semantics forSetOrFail, unlike theSetpath which passes the field. Add the field argument here as well.Apply this diff:
- if err := r.client.HExpireAt(ctx, r.key, options.validUntil).Err(); err != nil { + if err := r.client.HExpireAt(ctx, r.key, options.validUntil, key).Err(); err != nil {
♻️ Duplicate comments (2)
internal/sms-gateway/modules/events/service.go (2)
47-49: Add metric increment for validation failure.The validation check for empty
EventTypeis good, but it should increment a failure metric to maintain observability consistency with other error paths in this function.Apply this diff to add the metric increment:
if event.EventType == "" { + s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) return fmt.Errorf("event type is empty") }Note: Past review comments suggest using
FailureReasonValidationErrorinstead ofFailureReasonSerializationErrorfor better semantic clarity, but that would require adding the new constant first.
51-52: Accept parent context instead of using Background().Using
context.Background()ignores caller cancellation or timeout signals, potentially causing operations to hang for 5 seconds even when the caller has been cancelled. This can lead to resource leaks and degraded responsiveness during shutdown.Add a
context.Contextparameter toNotifyand use it as the parent context:-func (s *Service) Notify(userID string, deviceID *string, event Event) error { +func (s *Service) Notify(ctx context.Context, userID string, deviceID *string, event Event) error { if event.EventType == "" { s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) return fmt.Errorf("event type is empty") } - subCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + pubCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel() wrapper := eventWrapper{ UserID: userID, DeviceID: deviceID, Event: &event, } wrapperBytes, err := wrapper.serialize() if err != nil { s.metrics.IncrementFailed(string(event.EventType), DeliveryTypeUnknown, FailureReasonSerializationError) return fmt.Errorf("can't serialize event wrapper: %w", err) } - if err := s.pubsub.Publish(subCtx, pubsubTopic, wrapperBytes); err != nil { + if err := s.pubsub.Publish(pubCtx, pubsubTopic, wrapperBytes); err != nil { s.metrics.IncrementFailed(string(event.EventType), DeliveryTypeUnknown, FailureReasonPublishError) return fmt.Errorf("can't publish event: %w", err) } s.metrics.IncrementEnqueued(string(event.EventType)) return nil }Then update all call sites (e.g., in
internal/sms-gateway/modules/messages/service.golines 233 and 244) to pass a context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/cache/factory.go(1 hunks)internal/sms-gateway/modules/events/metrics.go(1 hunks)internal/sms-gateway/modules/events/module.go(1 hunks)internal/sms-gateway/modules/events/service.go(2 hunks)internal/sms-gateway/modules/events/types.go(1 hunks)internal/sms-gateway/modules/messages/service.go(2 hunks)internal/sms-gateway/modules/settings/service.go(1 hunks)internal/sms-gateway/modules/webhooks/service.go(1 hunks)internal/sms-gateway/pubsub/config.go(1 hunks)internal/sms-gateway/pubsub/module.go(1 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)pkg/cache/cache.go(1 hunks)pkg/cache/memory.go(2 hunks)pkg/cache/redis.go(2 hunks)pkg/pubsub/redis.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/config/module.go
- internal/sms-gateway/modules/settings/service.go
- pkg/cache/memory.go
- internal/sms-gateway/app.go
- internal/sms-gateway/pubsub/module.go
- internal/config/config.go
- internal/sms-gateway/pubsub/pubsub.go
- internal/sms-gateway/modules/events/metrics.go
- pkg/cache/cache.go
🧰 Additional context used
🧬 Code graph analysis (8)
pkg/pubsub/redis.go (3)
pkg/pubsub/options.go (1)
Option(3-3)pkg/pubsub/pubsub.go (5)
ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)Message(13-16)PubSub(45-50)internal/config/config.go (1)
PubSub(79-81)
internal/sms-gateway/modules/events/types.go (3)
internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)
internal/sms-gateway/modules/messages/service.go (1)
internal/sms-gateway/modules/events/events.go (1)
NewMessageEnqueuedEvent(9-11)
internal/sms-gateway/modules/events/module.go (2)
internal/sms-gateway/modules/events/service.go (1)
Service(19-30)internal/sms-gateway/app.go (1)
Run(62-73)
internal/sms-gateway/modules/events/service.go (5)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)internal/sms-gateway/modules/events/types.go (1)
Event(9-12)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/events/metrics.go (7)
DeliveryTypeUnknown(20-20)FailureReasonSerializationError(22-22)FailureReasonPublishError(23-23)EventTypeUnknown(26-26)DeliveryTypePush(18-18)FailureReasonProviderFailed(24-24)DeliveryTypeSSE(19-19)
pkg/cache/redis.go (2)
pkg/cache/cache.go (1)
Cache(5-40)internal/config/config.go (1)
Cache(75-77)
internal/sms-gateway/modules/webhooks/service.go (1)
internal/sms-gateway/modules/events/events.go (1)
NewWebhooksUpdatedEvent(13-15)
internal/sms-gateway/cache/factory.go (1)
pkg/cache/redis.go (2)
NewRedis(62-89)RedisConfig(37-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
internal/sms-gateway/modules/events/module.go (1)
17-36: LGTM! Error handling and shutdown flow are now correct.The lifecycle hooks properly capture errors from
svc.Run, log them, and trigger graceful shutdown viafx.Shutdowner. The OnStop hook correctly cancels the context.internal/sms-gateway/modules/messages/service.go (2)
232-236: LGTM! Correct adaptation to non-pointer Event parameter.The dereference of
events.NewMessageEnqueuedEvent()aligns with the updatedNotifysignature that now acceptsEventby value. The goroutine safely capturesuserIDanddeviceIDby value and logs errors appropriately.
242-244: LGTM! Correct adaptation to non-pointer Event parameter.The dereference of
events.NewMessagesExportRequestedEvent()aligns with the updatedNotifysignature that now acceptsEventby value.internal/sms-gateway/modules/events/service.go (6)
15-17: LGTM! Topic constant is well-defined.The
pubsubTopicconstant provides a centralized definition for the events topic used across Pub/Sub operations.
32-44: LGTM! Constructor correctly wires Pub/Sub dependency.The
NewServiceconstructor now includes thepubsub.PubSubparameter and properly initializes the service with all required dependencies.
54-64: LGTM! Wrapper creation and serialization error handling are correct.The wrapper construction and serialization with proper error handling and metrics tracking look good.
66-74: LGTM! Pub/Sub publishing with proper error handling.The publish operation correctly handles errors, tracks metrics, and returns descriptive error messages.
76-103: LGTM! Run loop correctly handles subscription lifecycle and closed channels.The subscription creation, message processing loop, and closed-channel check (
msg, ok := <-ch) are all correctly implemented. Deserialization failures properly increment metrics and continue processing.
105-150: LGTM! Event processing logic is sound.The
processEventfunction correctly dereferenceswrapper.Eventfields, routes to push or SSE services based on device configuration, and tracks metrics for both success and failure paths.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/sms-gateway/modules/events/types.go (2)
9-12: Extract a shared Event struct
TheEventtype is defined three times (inmodules/events/types.go,modules/sse/types.go, andmodules/push/types/types.gowith an alias inpush/types.go), varying only by field names/tags. Consolidate into one common definition—or document why they must differ—to eliminate duplication.
9-12: Unify JSON tag for Event.Type across modules.
internal/sms-gateway/modules/events/types.gousesjson:"event_type", butinternal/sms-gateway/modules/sse/types.gousesjson:"event". Align these tags (e.g. bothevent_typeor bothevent) to avoid serialization mismatches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(0 hunks)internal/sms-gateway/modules/events/events.go(2 hunks)internal/sms-gateway/modules/events/types.go(1 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (2)
internal/sms-gateway/modules/events/events.go (4)
internal/sms-gateway/modules/events/types.go (2)
Event(9-12)NewEvent(14-19)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
internal/sms-gateway/modules/events/types.go (3)
internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
internal/sms-gateway/modules/events/events.go (1)
9-29: LGTM! Return type changes align with the Event value type.The constructor functions now correctly return
Eventby value instead of*Event, consistent with the updated Event type definition intypes.go. The logic remains unchanged and correct.internal/sms-gateway/modules/events/types.go (4)
4-5: LGTM! Import needed for serialization.The
encoding/jsonimport is correctly added to support the newserialize()anddeserialize()methods.
14-19: LGTM! NewEvent correctly returns Event by value.The function now returns
Eventby value with exported fields properly initialized, aligning with the struct changes.
27-33: LGTM! Serialization methods are correctly implemented.The
serialize()anddeserialize()methods provide straightforward JSON marshaling/unmarshaling functionality for pub/sub transmission.
21-25: Event pointer usage is correct and consistent with JSON serialization requirements.The
Eventfield pointer usage is appropriate here. The eventWrapper is exclusively used for JSON serialization/deserialization via pub/sub (lines 27-32 in types.go), where theomitemptytag requires a pointer type. The code consistently creates the wrapper with&event(line 54-58 in service.go), and deserialization properly uses pointer receivers (lines 94-95 in service.go). No inconsistency issues found.
capcom6
force-pushed
the
events/multi-instance-support
branch
from
7257958 to
5e5ee38
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/sms-gateway/modules/events/service.go (2)
47-49: Add metrics increment for validation failure.The validation check correctly rejects empty event types, but it doesn't increment failure metrics as requested in the past review. This makes validation failures invisible in observability.
Add the metrics call (using the validation error constant from
metrics.go):if event.EventType == "" { + s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonValidationError) return fmt.Errorf("event type is empty") }
51-52: Accept parent context to respect caller cancellation.Using
context.Background()ignores caller cancellation/timeout, potentially causing operations to hang for 5 seconds even when the caller has been cancelled. This can lead to resource leaks and degraded responsiveness during shutdown.Update the signature to accept a context parameter:
-func (s *Service) Notify(userID string, deviceID *string, event Event) error { +func (s *Service) Notify(ctx context.Context, userID string, deviceID *string, event Event) error { if event.EventType == "" { s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonValidationError) return fmt.Errorf("event type is empty") } - subCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + pubCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel()Then update all callers to pass an appropriate context.
🧹 Nitpick comments (1)
pkg/cache/cache.go (1)
37-39: Document possible Close() errors in Cache interface. Update theClose()comment in pkg/cache/cache.go (lines 37–39) to describe when it can return errors (e.g. underlying Redis client.Close failures during connection shutdown).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
go.mod(0 hunks)internal/sms-gateway/cache/factory.go(1 hunks)internal/sms-gateway/modules/events/events.go(2 hunks)internal/sms-gateway/modules/events/metrics.go(1 hunks)internal/sms-gateway/modules/events/module.go(1 hunks)internal/sms-gateway/modules/events/service.go(2 hunks)internal/sms-gateway/modules/events/types.go(1 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)pkg/cache/cache.go(1 hunks)pkg/cache/memory.go(2 hunks)pkg/cache/redis.go(2 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/sms-gateway/pubsub/pubsub.go
- internal/sms-gateway/cache/factory.go
- pkg/cache/redis.go
🧰 Additional context used
🧬 Code graph analysis (5)
internal/sms-gateway/modules/events/module.go (2)
internal/sms-gateway/modules/events/service.go (1)
Service(19-30)internal/sms-gateway/app.go (1)
Run(62-73)
internal/sms-gateway/modules/events/types.go (3)
internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
internal/sms-gateway/modules/events/events.go (4)
internal/sms-gateway/modules/events/types.go (2)
Event(9-12)NewEvent(14-19)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
pkg/cache/memory.go (2)
internal/sms-gateway/cache/factory.go (1)
Cache(14-14)pkg/cache/cache.go (1)
Cache(5-40)
internal/sms-gateway/modules/events/service.go (5)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)internal/sms-gateway/modules/events/types.go (1)
Event(9-12)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/events/metrics.go (7)
DeliveryTypeUnknown(20-20)FailureReasonSerializationError(22-22)FailureReasonPublishError(23-23)EventTypeUnknown(26-26)DeliveryTypePush(18-18)FailureReasonProviderFailed(24-24)DeliveryTypeSSE(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
pkg/cache/memory.go (2)
172-176: LGTM: Close() implementation and compile-time assertion.The
Close()implementation correctly returnsnilsince an in-memory cache has no external resources to clean up. The compile-time assertionvar _ Cache = (*memoryCache)(nil)is good practice and ensuresmemoryCacheproperly implements theCacheinterface.
16-16: Returning memoryCache remains a valid, unexported implementation of Cache
*memoryCachestill implements theCacheinterface, callers can assign it toCachewithout issue, and internal fields stay unexported.Likely an incorrect or invalid review comment.
internal/sms-gateway/modules/events/module.go (1)
17-28: LGTM! Error handling properly implemented.The lifecycle hook correctly handles errors from
svc.Run, logs them, and initiates graceful shutdown with a non-zero exit code. The nested error handling for shutdown failures is also appropriate.internal/sms-gateway/modules/events/events.go (1)
9-29: LGTM! Consistent value semantics.The constructors now return
Eventby value, which is appropriate for small structs and aligns with the updatedNewEventsignature intypes.go. This eliminates unnecessary pointer allocations.internal/sms-gateway/modules/events/types.go (1)
4-19: LGTM! JSON serialization properly configured.The
Eventstruct now has exported fields with appropriate JSON tags, andNewEventcorrectly returns a value type. The serialization changes align well with the Pub/Sub integration requirements.internal/sms-gateway/modules/events/service.go (3)
32-44: LGTM! Constructor properly updated.The
NewServiceconstructor now accepts thepubsub.PubSubdependency, which is correctly stored in the service struct for use byNotifyandRunmethods.
60-71: LGTM! Serialization and publish error handling is robust.The code properly:
- Serializes the event wrapper with error handling
- Increments appropriate metrics for serialization failures
- Publishes to Pub/Sub with error handling
- Increments appropriate metrics for publish failures
- Increments enqueued metrics on success
- Wraps errors with context using
fmt.Errorfwith%w
76-103: LGTM! Subscription lifecycle properly managed.The
Runmethod correctly:
- Subscribes to the Pub/Sub topic
- Defers subscription cleanup
- Handles context cancellation
- Checks for closed channels to avoid tight loops
- Increments metrics on deserialization failures
- Logs errors without PII
Note: Validation for nil
wrapper.Eventshould be added as mentioned in the review oftypes.go(lines 21-33) to prevent panics inprocessEvent.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/sms-gateway/modules/events/service.go (1)
51-52: Accept parent context instead of using Background().Using
context.Background()ignores any caller's cancellation or timeout. This issue was previously flagged but remains unaddressed.-func (s *Service) Notify(userID string, deviceID *string, event Event) error { +func (s *Service) Notify(ctx context.Context, userID string, deviceID *string, event Event) error { if event.EventType == "" { return fmt.Errorf("event type is empty") } - subCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + pubCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel()
🧹 Nitpick comments (1)
internal/sms-gateway/modules/events/service.go (1)
47-49: Consider incrementing metrics for validation failures.The validation error here doesn't increment failed metrics, while serialization (line 62) and publish errors (line 67) do. For consistency and observability, consider adding a metrics call similar to other error paths.
if event.EventType == "" { + s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonValidationError) return fmt.Errorf("event type is empty") }Note: You would need to add the
FailureReasonValidationErrorconstant tometrics.goif not already present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/sms-gateway/modules/events/service.go(2 hunks)internal/sms-gateway/modules/events/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/modules/events/types.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/modules/events/service.go (6)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)internal/sms-gateway/modules/events/types.go (1)
Event(9-12)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/events/metrics.go (7)
DeliveryTypeUnknown(20-20)FailureReasonSerializationError(22-22)FailureReasonPublishError(23-23)EventTypeUnknown(26-26)DeliveryTypePush(18-18)FailureReasonProviderFailed(24-24)DeliveryTypeSSE(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/sms-gateway/modules/events/service.go (2)
76-103: LGTM! Pub/Sub integration properly implemented.The Run function correctly:
- Subscribes to the pubsub topic with proper error handling
- Handles channel close with the
okflag (line 89-93)- Avoids logging PII by only logging the error (line 97)
- Processes events through the wrapper pattern
The subscription lifecycle and error handling are well-implemented.
54-58: No issues found. TheEventis stored by value ineventWrapper(line 24 in types.go), which is intentional for immutability guarantees. The struct is small (~24 bytes: enum + map reference), so copying cost is negligible. JSON serialization handles value semantics correctly. This design choice is consistent with the serialize/deserialize implementation and prevents unintended modifications to events after wrapping.
capcom6
force-pushed
the
events/multi-instance-support
branch
from
2473277 to
adcbf48
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cache/redis.go (1)
156-201: <!-- [scratchpad]
[task final_analysis]
[observations]
- go-redis v9.9.0 is deployed; HExpireAt fully supported since v9.6.0
- HExpireAt calls in lines 166 and 195 are VALID Redis 7.4+ commands
- Lua scripts don't check expiry; rely on Redis auto-removal of expired fields
- Get() returns ErrKeyNotFound for both missing and expired keys (indistinguishable)
- GetAndDelete() Lua script returns false (mapped to ErrKeyNotFound) when field absent or expired
- Interface contract (cache.go:15) requires ErrKeyExpired to be returned for expired keys
[analysis]- Review comment's PRIMARY claim (HExpireAt doesn't exist) is INCORRECT
- Review comment's SECONDARY claim (ErrKeyExpired not returned) is VALID but for different reason
- With Redis 7.4 auto-expiry of hash fields, expired fields are auto-deleted by Redis
- This makes HGet return Nil for both expired and missing keys (semantically indistinguishable at API level)
- Current implementation violates interface contract which requires distinguishing expiry from non-existence
- The implementation relies on transparent auto-expiry but the interface doesn't support this model
[pending]- None; sufficient data to rewrite comment
[done]- Verified HExpireAt exists and is properly used
- Identified true semantic issue: interface contract violation for ErrKeyExpired distinction
Fix missing ErrKeyExpired distinction in Get() and GetAndDelete()—interface contract requires distinguishing expired from non-existent keys.
With Redis 7.4 auto-expiring hash fields, expired entries are transparently removed by Redis before retrieval. This makes HGet return Nil for both expired and missing keys, but the Cache interface contract (cache.go:15) requires returning ErrKeyExpired when a key has expired. Currently both Get() (line 128) and GetAndDelete() (line 142) return ErrKeyNotFound in both cases, violating the interface.
Solutions:
- A) Abandon per-field TTL; store each entry as a separate key with SET/GET/DEL+EX/EXAT for whole-key expiry, making expiration visible to the client.
- B) Keep the hash + add a sidecar sorted set (e.g., key + ":exp") tracking expiry times. In Get/GetAndDelete, use Lua to check if ZSCORE shows expiry < now; return ErrKeyExpired and clean up if so. Adjust Cleanup() and Drain() to also manage the sidecar.
- C) Clarify the interface: if auto-expiry is transparent, document that Get/GetAndDelete return ErrKeyNotFound for both missing and expired keys, and adjust memory.go to match (currently it returns ErrKeyExpired).
Note: HExpireAt (lines 166, 195) is correctly implemented—go-redis v9.9.0 fully supports this command.
♻️ Duplicate comments (2)
internal/sms-gateway/modules/events/metrics.go (1)
22-26: Add validation-specific failure reason constant.The validation check in
service.go(lines 47-49) should increment metrics with a distinct validation failure reason instead of lumping validation failures with serialization errors in metrics analysis.Apply this diff to add the constant:
FailureReasonSerializationError = "serialization_error" FailureReasonPublishError = "publish_error" + FailureReasonValidationError = "validation_error" FailureReasonProviderFailed = "provider_failed"Then update
service.goline 48 to increment metrics:if event.EventType == "" { s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonValidationError) return fmt.Errorf("event type is empty") }internal/sms-gateway/modules/events/service.go (1)
51-52: Accept parent context to respect caller cancellation.Using
context.Background()ignores any caller's cancellation or timeout, potentially causing operations to hang for 5 seconds even if the caller has been cancelled. This can lead to resource leaks and degraded responsiveness during shutdown or request cancellation.Add a
context.Contextparameter toNotify:-func (s *Service) Notify(userID string, deviceID *string, event Event) error { +func (s *Service) Notify(ctx context.Context, userID string, deviceID *string, event Event) error { if event.EventType == "" { return fmt.Errorf("event type is empty") } - subCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + pubCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel() wrapper := eventWrapper{ UserID: userID, DeviceID: deviceID, Event: event, } wrapperBytes, err := wrapper.serialize() if err != nil { s.metrics.IncrementFailed(string(event.EventType), DeliveryTypeUnknown, FailureReasonSerializationError) return fmt.Errorf("can't serialize event wrapper: %w", err) } - if err := s.pubsub.Publish(subCtx, pubsubTopic, wrapperBytes); err != nil { + if err := s.pubsub.Publish(pubCtx, pubsubTopic, wrapperBytes); err != nil { s.metrics.IncrementFailed(string(event.EventType), DeliveryTypeUnknown, FailureReasonPublishError) return fmt.Errorf("can't publish event: %w", err) } s.metrics.IncrementEnqueued(string(event.EventType)) return nil }
🧹 Nitpick comments (4)
pkg/cache/redis.go (3)
62-89: Constructor refactor is solid; consider returning the interface and fail-fast ping.Validation, URL parsing, prefix normalization, and ownership look good. Prefer returning Cache to avoid leaking the concrete type, and optionally Ping to fail fast on bad URLs.
Apply:
-func NewRedis(config RedisConfig) (*redisCache, error) { +func NewRedis(config RedisConfig) (Cache, error) { ... - return &redisCache{ + return &redisCache{ client: client, ownedClient: config.Client == nil, key: config.Prefix + redisCacheKey, ttl: config.TTL, }, nil }Optionally, after creating the client:
// _ := client.Ping(context.Background()).Err() // return wrapped error if non-nil
36-51: Clarify TTL semantics in the config.Document that TTL=0 disables per-entry expiry, and how overrides via options interact with TTL. This helps set expectations across memory and Redis backends.
203-209: Make Close idempotent and concurrency-safe.Multiple/concurrent Close calls can return errors from the underlying client. Guard with sync.Once or atomically nil out the client after the first close.
Example:
+import "sync" type redisCache struct { client *redis.Client ownedClient bool + closeOnce sync.Once key string ttl time.Duration } func (r *redisCache) Close() error { - if r.ownedClient { - return r.client.Close() - } - return nil + var err error + if r.ownedClient && r.client != nil { + r.closeOnce.Do(func() { err = r.client.Close(); r.client = nil }) + } + return err }pkg/pubsub/options.go (1)
9-15: *Simplify: apply doesn’t need to return options.Return value is unused; remove it to reduce noise.
-func (o *options) apply(opts ...Option) *options { +func (o *options) apply(opts ...Option) { for _, opt := range opts { opt(o) } - - return o }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
go.mod(0 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/cache/factory.go(1 hunks)internal/sms-gateway/modules/events/events.go(2 hunks)internal/sms-gateway/modules/events/metrics.go(1 hunks)internal/sms-gateway/modules/events/module.go(1 hunks)internal/sms-gateway/modules/events/service.go(2 hunks)internal/sms-gateway/modules/events/types.go(1 hunks)internal/sms-gateway/pubsub/config.go(1 hunks)internal/sms-gateway/pubsub/module.go(1 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)pkg/cache/cache.go(1 hunks)pkg/cache/memory.go(2 hunks)pkg/cache/redis.go(2 hunks)pkg/pubsub/memory.go(1 hunks)pkg/pubsub/options.go(1 hunks)pkg/pubsub/pubsub.go(1 hunks)pkg/pubsub/redis.go(1 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/sms-gateway/pubsub/config.go
- internal/config/module.go
- internal/sms-gateway/pubsub/module.go
- pkg/cache/cache.go
- internal/config/config.go
- pkg/pubsub/redis.go
- pkg/pubsub/pubsub.go
- internal/sms-gateway/modules/events/module.go
- internal/sms-gateway/modules/events/types.go
- internal/sms-gateway/pubsub/pubsub.go
- internal/sms-gateway/cache/factory.go
- pkg/cache/memory.go
🧰 Additional context used
🧬 Code graph analysis (5)
internal/sms-gateway/modules/events/service.go (3)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)internal/sms-gateway/modules/events/types.go (1)
Event(9-12)internal/sms-gateway/modules/events/metrics.go (7)
DeliveryTypeUnknown(20-20)FailureReasonSerializationError(22-22)FailureReasonPublishError(23-23)EventTypeUnknown(26-26)DeliveryTypePush(18-18)FailureReasonProviderFailed(24-24)DeliveryTypeSSE(19-19)
pkg/pubsub/memory.go (2)
pkg/pubsub/pubsub.go (5)
Message(13-16)ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)PubSub(45-50)pkg/pubsub/options.go (1)
Option(3-3)
internal/sms-gateway/modules/events/events.go (3)
internal/sms-gateway/modules/events/types.go (2)
Event(9-12)NewEvent(14-19)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
internal/sms-gateway/app.go (2)
internal/config/module.go (1)
Module(22-131)internal/sms-gateway/pubsub/module.go (1)
Module(11-26)
pkg/cache/redis.go (2)
pkg/cache/cache.go (1)
Cache(5-40)internal/config/config.go (1)
Cache(75-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
internal/sms-gateway/modules/events/service.go (2)
76-103: LGTM! Pub/Sub integration is well-implemented.The
Runfunction correctly:
- Subscribes to the Pub/Sub topic with proper error handling
- Handles channel closure (lines 89-93)
- Increments deserialization failure metrics with safe labels (line 96)
- Avoids logging sensitive payload data (line 97)
The past review concerns about channel handling and PII logging have been addressed.
105-150: LGTM! Event processing is correctly instrumented.The
processEventfunction properly:
- Handles both push and SSE delivery paths
- Increments success and failure metrics with appropriate labels
- Uses the wrapper's event type and data consistently
The implementation is clean and well-structured.
internal/sms-gateway/modules/events/events.go (1)
9-29: LGTM! Value semantics for Event is appropriate.The change from pointer to value return type aligns with the
Notify()method signature ininternal/sms-gateway/modules/events/service.go:46, which expectsevent Event(value type). All call sites acrosssettings/service.go,messages/service.go, andwebhooks/service.gocorrectly pass Event values directly toNotify()with no incompatibilities.pkg/cache/redis.go (1)
211-211: Nice interface assertion.Compile-time check is helpful.
internal/sms-gateway/app.go (1)
24-24: LGTM: pubsub module wired into app.Import and Module() registration order looks fine relative to config and events.
Also applies to: 49-49
pkg/pubsub/options.go (1)
5-7: ****The claim that "make(chan T, n) requires n to be int" is incorrect. Go's
make()built-in accepts any integer type for the buffer size parameter, includinguint. The code as written will compile without errors. Bothpkg/pubsub/memory.go:91andpkg/pubsub/redis.go:112successfully passuintvalues tomake(chan Message, ...).If there is a real type mismatch issue (e.g.,
config.BufferSizeisintbutWithBufferSizeexpectsuint), that is a separate concern and should be addressed at the call site, not by changing the buffer size type throughout the codebase.Likely an incorrect or invalid review comment.
pkg/pubsub/memory.go (2)
112-122: Update subscribe to new subscriber type.-func (m *memoryPubSub) subscribe(id, topic string, ch chan Message) { +func (m *memoryPubSub) subscribe(id, topic string, sub subscriber) { m.mu.Lock() defer m.mu.Unlock() subscriptions, ok := m.topics[topic] if !ok { - subscriptions = make(map[string]chan Message) + subscriptions = make(map[string]subscriber) m.topics[topic] = subscriptions } - subscriptions[id] = ch + subscriptions[id] = sub }Likely an incorrect or invalid review comment.
58-71: Skip canceled subscribers and guard mid-flight cancel in Publish.Pre-check sub.ctx.Done to avoid blocking, and include it in the send select.
- for _, ch := range subscribers { - wg.Add(1) - go func(ch chan Message) { + for _, sub := range subscribers { + // Skip already-canceled subscribers. + select { + case <-sub.ctx.Done(): + continue + default: + } + wg.Add(1) + go func(sub subscriber) { defer wg.Done() select { - case ch <- msg: + case sub.ch <- msg: case <-ctx.Done(): return case <-m.closeCh: return + case <-sub.ctx.Done(): + // Subscriber canceled mid-flight; drop. + return } - }(ch) + }(sub) }Likely an incorrect or invalid review comment.
capcom6
force-pushed
the
events/multi-instance-support
branch
from
adcbf48 to
fb29abc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
pkg/pubsub/redis.go (1)
78-90: Document Publish semantics (fire-and-forget vs memory).Add a note that Redis Publish returns when Redis accepts the message; it does not wait for subscribers to receive it (unlike memory, which blocks until delivery). Helps callers set expectations.
func (r *redisPubSub) Publish(ctx context.Context, topic string, data []byte) error { + // Note: Redis publish is fire-and-forget. It returns once Redis accepts the + // message and does not wait for subscribers to receive it. This differs from + // the in-memory implementation which blocks until delivery (or context/close). select {internal/sms-gateway/modules/events/metrics.go (1)
22-26: Add a validation-specific failure reason.Introduce a dedicated constant to distinguish validation failures from serialization errors in metrics.
FailureReasonSerializationError = "serialization_error" FailureReasonPublishError = "publish_error" FailureReasonProviderFailed = "provider_failed" + FailureReasonValidationError = "validation_error"internal/sms-gateway/modules/events/service.go (2)
47-49: Increment failure metrics on validation error.The validation check on line 47 returns an error but does not increment failure metrics, making it difficult to track validation failures in monitoring. Past review comments flagged this gap.
Apply this diff to increment metrics before returning:
if event.EventType == "" { + s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) return fmt.Errorf("event type is empty") }Note: If a
FailureReasonValidationErrorconstant is available inmetrics.go, prefer that overFailureReasonSerializationErrorfor clearer failure classification.
51-52: Accept parent context to honor caller cancellation.Using
context.Background()creates a detached context that ignores any cancellation or timeout from the caller. During shutdown or request cancellation, this can cause the publish operation to hang for up to 5 seconds, leading to resource leaks and degraded responsiveness.Update the
Notifysignature to accept acontext.Contextparameter and use it as the parent for the timeout:-func (s *Service) Notify(userID string, deviceID *string, event Event) error { +func (s *Service) Notify(ctx context.Context, userID string, deviceID *string, event Event) error { if event.EventType == "" { + s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) return fmt.Errorf("event type is empty") } - subCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + pubCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel()Then update all callers of
Notifyto pass an appropriate context.
🧹 Nitpick comments (4)
pkg/cache/cache.go (2)
39-39: Consider adding context parameter for timeout control.While
Close() errorfollows the standardio.Closerpattern, operations that close network resources (like Redis connections) could benefit from a context parameter for timeout and cancellation control. Consider whetherClose(ctx context.Context) errorwould better align with the other methods and provide better control over cleanup operations.
37-38: Enhance documentation with idempotency and error behavior.Consider documenting whether
Close()is idempotent (safe to call multiple times) and what specific error conditions callers should expect. This helps consumers understand proper usage patterns.internal/sms-gateway/pubsub/config.go (1)
3-7: Clarify BufferSize semantics (and consider int).Document that 0 means unbuffered; consider int for BufferSize to match common make(chan, n) usage and avoid ad-hoc casts elsewhere. Safe to keep uint if you prefer—just be consistent across options/backends.
pkg/pubsub/memory.go (1)
33-36: High fan-out: goroutine-per-subscriber delivery could be costly.If topics have many subscribers and events are frequent, consider bounded worker pool or direct synchronous sends with timeouts/backpressure to reduce goroutine churn.
If helpful, I can sketch benchmarks to gauge throughput/latency under fan-out.
Also applies to: 55-76
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
go.mod(0 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/cache/factory.go(1 hunks)internal/sms-gateway/modules/events/events.go(2 hunks)internal/sms-gateway/modules/events/metrics.go(1 hunks)internal/sms-gateway/modules/events/module.go(1 hunks)internal/sms-gateway/modules/events/service.go(2 hunks)internal/sms-gateway/modules/events/types.go(1 hunks)internal/sms-gateway/pubsub/config.go(1 hunks)internal/sms-gateway/pubsub/module.go(1 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)pkg/cache/cache.go(1 hunks)pkg/cache/memory.go(2 hunks)pkg/cache/redis.go(2 hunks)pkg/pubsub/memory.go(1 hunks)pkg/pubsub/options.go(1 hunks)pkg/pubsub/pubsub.go(1 hunks)pkg/pubsub/redis.go(1 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/pubsub/options.go
- internal/sms-gateway/app.go
- pkg/cache/memory.go
- internal/sms-gateway/pubsub/pubsub.go
- internal/config/module.go
- pkg/cache/redis.go
- internal/config/config.go
- pkg/pubsub/pubsub.go
- internal/sms-gateway/modules/events/module.go
- internal/sms-gateway/modules/events/types.go
- internal/sms-gateway/pubsub/module.go
- internal/sms-gateway/cache/factory.go
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/pubsub/redis.go (3)
pkg/pubsub/options.go (1)
Option(3-3)pkg/pubsub/pubsub.go (5)
ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)Message(13-16)PubSub(45-50)internal/config/config.go (1)
PubSub(79-81)
pkg/pubsub/memory.go (2)
pkg/pubsub/pubsub.go (5)
Message(13-16)ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)PubSub(45-50)pkg/pubsub/options.go (1)
Option(3-3)
internal/sms-gateway/modules/events/events.go (2)
internal/sms-gateway/modules/events/types.go (2)
Event(9-12)NewEvent(14-19)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)
internal/sms-gateway/modules/events/service.go (5)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)internal/sms-gateway/modules/events/types.go (1)
Event(9-12)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/events/metrics.go (7)
DeliveryTypeUnknown(20-20)FailureReasonSerializationError(22-22)FailureReasonPublishError(23-23)EventTypeUnknown(26-26)DeliveryTypePush(18-18)FailureReasonProviderFailed(24-24)DeliveryTypeSSE(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
pkg/cache/cache.go (1)
37-39: All implementations of theCacheinterface within this codebase have already been updated with theClose()method.Both
memoryCache(pkg/cache/memory.go:172) andredisCache(pkg/cache/redis.go:203) implementClose(), confirmed by compile-time assertions. No additional implementations exist that require updates.Note: This remains a breaking change for external consumers of the public
pkg/cachepackage, but the PR itself is complete for this codebase.internal/sms-gateway/modules/events/events.go (1)
9-11: LGTM – value returns align with NewEvent.Constructors now return Event by value consistently. Please ensure downstream call sites expect values (not pointers).
Also applies to: 13-15, 17-25, 27-29
internal/sms-gateway/modules/events/service.go (1)
76-103: LGTM—past review concerns addressed.The subscription lifecycle (lines 77-81), closed-channel handling (lines 89-93), and error logging without PII (line 97) correctly address previous review comments. The deserialization error flow with metrics and safe logging is well implemented.
capcom6
force-pushed
the
events/multi-instance-support
branch
from
a102d73 to
2ffc5a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/sms-gateway/modules/events/service.go (2)
51-52: Accept parent context to respect caller cancellation.Using
context.Background()ignores any caller's cancellation or timeout, potentially causing the operation to block for up to 5 seconds even when the caller has been cancelled. This can lead to resource leaks and degraded responsiveness during shutdown.Consider updating the
Notifysignature to accept a context parameter:-func (s *Service) Notify(userID string, deviceID *string, event Event) error { +func (s *Service) Notify(ctx context.Context, userID string, deviceID *string, event Event) error { if event.EventType == "" { s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) return fmt.Errorf("event type is empty") } - subCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + pubCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel()Then update all callers to pass an appropriate context.
46-49: Add failure metric for validation errors.The validation check on line 47-49 returns an error but doesn't increment a failure metric. This makes it difficult to monitor validation failures in production.
Apply this diff to add metric tracking:
func (s *Service) Notify(userID string, deviceID *string, event Event) error { if event.EventType == "" { + s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) return fmt.Errorf("event type is empty") }Note: If you add
FailureReasonValidationErroras suggested inmetrics.go, use that constant instead ofFailureReasonSerializationErrorfor clearer metrics distinction.
🧹 Nitpick comments (1)
internal/sms-gateway/modules/events/metrics.go (1)
22-26: Consider adding a validation-specific failure reason constant.The past review comments suggest adding
FailureReasonValidationError = "validation_error"to distinguish validation failures from serialization errors. Currently, validation failures inservice.go(line 48) don't increment any failure metric, and if they did, they would need a distinct constant to avoid conflating validation issues with actual serialization failures.Apply this diff to add the validation error constant:
FailureReasonSerializationError = "serialization_error" FailureReasonPublishError = "publish_error" + FailureReasonValidationError = "validation_error" FailureReasonProviderFailed = "provider_failed"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
go.mod(0 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/cache/factory.go(2 hunks)internal/sms-gateway/modules/events/events.go(2 hunks)internal/sms-gateway/modules/events/metrics.go(1 hunks)internal/sms-gateway/modules/events/module.go(1 hunks)internal/sms-gateway/modules/events/service.go(2 hunks)internal/sms-gateway/modules/events/types.go(1 hunks)internal/sms-gateway/pubsub/config.go(1 hunks)internal/sms-gateway/pubsub/module.go(1 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)pkg/cache/cache.go(1 hunks)pkg/cache/memory.go(2 hunks)pkg/cache/redis.go(2 hunks)pkg/pubsub/memory.go(1 hunks)pkg/pubsub/options.go(1 hunks)pkg/pubsub/pubsub.go(1 hunks)pkg/pubsub/redis.go(1 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/config/config.go
- internal/sms-gateway/pubsub/module.go
- pkg/pubsub/memory.go
- pkg/cache/cache.go
- pkg/pubsub/options.go
- pkg/pubsub/redis.go
🧰 Additional context used
🧬 Code graph analysis (10)
internal/sms-gateway/modules/events/module.go (2)
internal/sms-gateway/modules/events/service.go (1)
Service(19-30)internal/sms-gateway/app.go (1)
Run(62-73)
internal/sms-gateway/app.go (2)
internal/config/module.go (1)
Module(22-131)internal/sms-gateway/pubsub/module.go (1)
Module(10-29)
internal/sms-gateway/cache/factory.go (1)
pkg/cache/redis.go (2)
NewRedis(62-89)RedisConfig(37-51)
pkg/cache/memory.go (2)
internal/sms-gateway/cache/factory.go (1)
Cache(14-14)pkg/cache/cache.go (1)
Cache(5-40)
internal/sms-gateway/modules/events/service.go (5)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)internal/sms-gateway/modules/events/types.go (1)
Event(9-12)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/events/metrics.go (7)
DeliveryTypeUnknown(20-20)FailureReasonSerializationError(22-22)FailureReasonPublishError(23-23)EventTypeUnknown(26-26)DeliveryTypePush(18-18)FailureReasonProviderFailed(24-24)DeliveryTypeSSE(19-19)
internal/config/module.go (3)
internal/config/config.go (2)
Config(10-19)PubSub(79-81)internal/sms-gateway/pubsub/config.go (1)
Config(4-7)internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)
internal/sms-gateway/modules/events/events.go (1)
internal/sms-gateway/modules/events/types.go (2)
Event(9-12)NewEvent(14-19)
internal/sms-gateway/modules/events/types.go (3)
internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)
internal/sms-gateway/pubsub/pubsub.go (5)
internal/config/config.go (2)
PubSub(79-81)Config(10-19)internal/sms-gateway/pubsub/config.go (1)
Config(4-7)pkg/pubsub/options.go (1)
WithBufferSize(17-21)pkg/pubsub/memory.go (1)
NewMemory(24-36)pkg/pubsub/redis.go (2)
NewRedis(42-76)RedisConfig(15-27)
pkg/cache/redis.go (2)
pkg/cache/cache.go (1)
Cache(5-40)internal/config/config.go (1)
Cache(75-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
internal/sms-gateway/pubsub/config.go (1)
1-7: LGTM!The Config struct is clean and well-documented. The URL-based backend selection pattern is consistent with the cache factory approach elsewhere in the codebase.
internal/sms-gateway/cache/factory.go (1)
44-49: Good fix for the namespace collision issue.The Redis cache now correctly uses
keyPrefix + nameas the Prefix, which addresses the past review concern about all caches colliding on the same namespace.pkg/cache/redis.go (2)
36-89: LGTM! Clean config-based initialization with proper ownership tracking.The RedisConfig pattern and client ownership management are well-implemented:
- Clear validation that either Client or URL must be provided
- Proper prefix normalization
- Ownership tracking enables safe resource cleanup
203-211: LGTM! Proper resource cleanup with ownership check.The Close method correctly closes only owned clients, preventing double-close issues when an external client is provided. The compile-time assertion is good practice.
internal/sms-gateway/modules/events/events.go (1)
9-29: LGTM! Consistent value-type returns.The event constructors now return Event by value, which aligns with the updated Event type definition and eliminates pointer management overhead for these immutable data structures.
internal/sms-gateway/modules/events/service.go (2)
89-100: LGTM! Proper closed-channel handling.The select statement correctly checks the
okvalue from the channel receive, preventing tight loops when the subscription is closed. The error handling and metrics for deserialization failures are appropriate.
105-150: LGTM! Clean device-based event routing.The processEvent method properly handles device selection, routing to push or SSE based on token availability, and tracks success/failure metrics for each delivery type.
internal/config/module.go (1)
125-130: LGTM! Correct URL source for PubSub configuration.The provider now correctly reads from
cfg.PubSub.URLinstead ofcfg.Cache.URL, addressing the past review concern. The BufferSize of 128 provides reasonable capacity for the Pub/Sub channels.pkg/cache/memory.go (2)
16-23: LGTM! Constructor change aligns with lifecycle management.Returning the concrete
*memoryCachetype instead of theCacheinterface is appropriate here, especially with the compile-time check at Line 176 ensuring interface compliance.
172-176: LGTM! Close implementation and interface check are correct.The no-op
Close()is appropriate for the in-memory cache (no external resources to clean up), and the compile-time assertion ensures interface compliance.internal/sms-gateway/modules/events/module.go (1)
17-28: LGTM! Error handling and shutdown logic are well-structured.The addition of error handling for
svc.Run(ctx)with proper logging and graceful shutdown viafx.Shutdownerfollows fx lifecycle best practices.internal/sms-gateway/app.go (1)
24-24: LGTM! Clean integration of the pubsub module.The import and module registration are straightforward and follow the existing pattern.
Also applies to: 49-49
pkg/pubsub/pubsub.go (3)
8-16: LGTM! Clean error definitions and message structure.The sentinel errors and
Messagestruct provide a clear, simple foundation for the pub/sub abstraction.
18-31: LGTM! Well-designed subscription lifecycle management.The
Subscriptiontype properly encapsulates its state with unexported fields, exposes a read-only channel viaReceive(), and uses context cancellation for cleanup—following Go best practices.
33-50: LGTM! Well-structured interface design.The interface hierarchy follows good design principles:
- Segregates
PublisherandSubscriberconcerns- Combines them in the
PubSubinterface with lifecycle management- Clear documentation of behavior (fan-out, context cancellation)
internal/sms-gateway/pubsub/pubsub.go (1)
16-41: No issues found—Redis client closure is properly implemented.The verification confirms the factory implementation correctly manages the Redis client lifecycle:
- Factory passes
Client: nilto RedisConfig (line 20)- NewRedis() detects nil client and creates a new one via
redis.NewClient(opt)(line 59)ownedClientflag is correctly set totruewhen the client is created internally (line 71)- Close() properly guards the client closure with the
ownedClientcheck and callsr.client.Close()when owned (lines 173–174)The past concern about Redis client closure is not validated—the code already implements proper client lifecycle management.
capcom6
force-pushed
the
events/multi-instance-support
branch
from
7e48f90 to
4eef245
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
pkg/pubsub/redis.go (1)
112-112: Fix channel buffer size type mismatch.
make(chan Message, r.bufferSize)will not compile becausebufferSizeisuintand channel capacities must beint.Apply this diff:
- ch := make(chan Message, r.bufferSize) + ch := make(chan Message, int(r.bufferSize))Note: If unbounded
bufferSizevalues are expected, add bounds checking (e.g., clamping tomath.MaxInt).internal/sms-gateway/modules/events/service.go (3)
51-52: Accept parent context to respect caller cancellation.Using
context.Background()ignores the caller's cancellation signals, potentially causingNotifyto hang for up to 5 seconds even when the caller has been cancelled. This can accumulate resource leaks during shutdown or when upstream requests are cancelled.Based on past review comment: Add a
context.Contextparameter toNotifyand derive the timeout context from it instead ofBackground(). This ensures that caller cancellations propagate correctly and timeouts stack appropriately.
95-100: Validate EventType after deserialization.After successfully deserializing at line 95, the code doesn't verify that
wrapper.Event.EventTypeis non-empty before callingprocessEventat line 100. If the incoming JSON lacks an"event"field or contains"event": {}, the Event will have an empty EventType, which then propagates to metrics calls inprocessEvent(lines 132, 134, 145, 147), resulting in metrics labeled with empty strings and potential confusion.Add validation immediately after deserialization:
wrapper := new(eventWrapper) if err := wrapper.deserialize(msg.Data); err != nil { s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) s.logger.Error("Failed to deserialize event wrapper", zap.Error(err)) continue } + if wrapper.Event.EventType == "" { + s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) + s.logger.Error("Invalid event: missing EventType") + continue + } s.processEvent(wrapper)Based on learnings
47-49: Increment metrics on validation failure.When
EventTypeis empty, the function returns an error but doesn't increment failure metrics. This creates a blind spot in observability—validation failures won't be tracked or alerted on.Add metric tracking before returning:
if event.EventType == "" { + s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) return fmt.Errorf("event type is empty") }Note: Consider defining
FailureReasonValidationErroras suggested in past comments for more precise failure categorization.
🧹 Nitpick comments (3)
pkg/pubsub/redis.go (1)
163-178: Align Close behavior across implementations.The Redis Close method returns
nilon repeated calls (Line 166), while the memory implementation returnsErrPubSubClosed. Consider standardizing on one approach for API consistency across backends.If you prefer error semantics, apply this diff:
func (r *redisPubSub) Close() error { select { case <-r.closeCh: - return nil + return ErrPubSubClosed default: close(r.closeCh) } r.wg.Wait() if r.ownedClient { return r.client.Close() } return nil }internal/sms-gateway/modules/events/types.go (2)
10-10: Align JSON field naming with related Event types.The
EventTypefield usesjson:"event_type", butsse.Event.Typeusesjson:"event"(seeinternal/sms-gateway/modules/sse/types.goline 7). This inconsistency can cause confusion when reading logs or debugging serialized events across different modules.Consider standardizing on one naming convention. If you choose to align with SSE:
- EventType smsgateway.PushEventType `json:"event_type"` + EventType smsgateway.PushEventType `json:"event"`Or document the rationale for the different naming if intentional.
14-19: Validate eventType to prevent construction of invalid Events.
NewEventaccepts an emptyeventTypewithout validation, allowing callers to construct Events that will fail at publish time (seeNotifyvalidation at line 47-49 inservice.go). This defers error detection and makes debugging harder.Add validation at construction time:
func NewEvent(eventType smsgateway.PushEventType, data map[string]string) Event { + if eventType == "" { + panic("NewEvent: eventType cannot be empty") + } return Event{ EventType: eventType, Data: data, } }Alternatively, return an error:
func NewEvent(...) (Event, error).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
go.mod(0 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/cache/factory.go(2 hunks)internal/sms-gateway/modules/events/events.go(2 hunks)internal/sms-gateway/modules/events/metrics.go(1 hunks)internal/sms-gateway/modules/events/module.go(1 hunks)internal/sms-gateway/modules/events/service.go(2 hunks)internal/sms-gateway/modules/events/types.go(1 hunks)internal/sms-gateway/pubsub/config.go(1 hunks)internal/sms-gateway/pubsub/module.go(1 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)pkg/cache/cache.go(1 hunks)pkg/cache/memory.go(1 hunks)pkg/cache/redis.go(2 hunks)pkg/pubsub/memory.go(1 hunks)pkg/pubsub/options.go(1 hunks)pkg/pubsub/pubsub.go(1 hunks)pkg/pubsub/redis.go(1 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/sms-gateway/app.go
- pkg/cache/cache.go
- internal/config/module.go
- pkg/pubsub/pubsub.go
- internal/sms-gateway/modules/events/metrics.go
- pkg/pubsub/options.go
- internal/sms-gateway/pubsub/config.go
- internal/sms-gateway/cache/factory.go
🧰 Additional context used
🧬 Code graph analysis (9)
pkg/pubsub/memory.go (2)
pkg/pubsub/pubsub.go (5)
Message(13-16)ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)PubSub(45-50)pkg/pubsub/options.go (1)
Option(3-3)
internal/sms-gateway/pubsub/module.go (4)
internal/config/module.go (1)
Module(22-132)internal/sms-gateway/app.go (1)
Module(36-60)internal/sms-gateway/pubsub/pubsub.go (2)
New(16-41)PubSub(14-14)internal/config/config.go (1)
PubSub(85-87)
internal/sms-gateway/modules/events/module.go (2)
internal/sms-gateway/modules/events/service.go (1)
Service(19-30)internal/sms-gateway/app.go (1)
Run(62-73)
internal/sms-gateway/pubsub/pubsub.go (4)
internal/sms-gateway/pubsub/config.go (1)
Config(4-7)pkg/pubsub/options.go (1)
WithBufferSize(17-21)pkg/pubsub/memory.go (1)
NewMemory(24-36)pkg/pubsub/redis.go (2)
NewRedis(42-76)RedisConfig(15-27)
internal/config/config.go (1)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)
pkg/pubsub/redis.go (2)
pkg/pubsub/options.go (1)
Option(3-3)pkg/pubsub/pubsub.go (5)
ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)Message(13-16)PubSub(45-50)
internal/sms-gateway/modules/events/types.go (3)
internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
internal/sms-gateway/modules/events/service.go (3)
internal/sms-gateway/modules/messages/service.go (2)
Service(30-42)NewService(44-67)internal/sms-gateway/modules/settings/service.go (2)
Service(19-25)NewService(27-35)internal/sms-gateway/modules/webhooks/service.go (2)
Service(28-37)NewService(39-50)
internal/sms-gateway/modules/events/events.go (4)
internal/sms-gateway/modules/events/types.go (2)
Event(9-12)NewEvent(14-19)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/push/types.go (1)
Event(16-16)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
pkg/cache/memory.go (1)
195-197: LGTM! Appropriate no-op implementation.The no-op
Close()method is correct for an in-memory cache with no external resources to release. The memory will be garbage collected when the cache object is no longer referenced.internal/config/config.go (1)
19-19: LGTM! PubSub configuration follows existing patterns.The PubSub configuration structure mirrors the Cache configuration pattern with consistent YAML/envconfig tags and a sensible default (
memory://).Also applies to: 85-87, 121-123
pkg/cache/redis.go (2)
51-104: LGTM! Well-designed config pattern with proper ownership tracking.The
RedisConfigstruct and updatedNewRedisconstructor follow the options pattern effectively:
- Flexible initialization (accepts either a client or URL)
- Proper validation ensures at least one is provided
- Prefix normalization with trailing colon is a nice touch
- Ownership tracking (
ownedClient) enables correct resource cleanup
254-260: LGTM! Proper resource cleanup with ownership check.The
Closemethod correctly closes only owned clients, preventing resource leaks while respecting externally-provided clients.internal/sms-gateway/modules/events/events.go (1)
9-29: LGTM! Event constructors now return values for better serialization.The change from
*EventtoEventreturn types aligns with the updatedEventstruct (now with exported, JSON-tagged fields) and enables value semantics appropriate for serialization.pkg/pubsub/redis.go (3)
14-76: LGTM! Consistent config pattern with proper validation.The
RedisConfigandNewRedisconstructor follow the same solid design pattern as the cache implementation with proper ownership tracking and prefix normalization.
78-90: LGTM! Clean publish implementation.The
Publishmethod correctly validates state and topic before publishing. Note that this is fire-and-forget (returns once Redis accepts the message), which differs from the memory implementation's blocking behavior.
103-161: LGTM! Subscription handling with proper confirmation and cleanup.The Subscribe method correctly:
- Waits for subscription confirmation via
ps.Receive(ctx)before exposing the subscription- Cleans up the Redis subscription on error (Line 106)
- Uses nested select (Lines 146-155) to prevent deadlock with slow consumers
- Properly tracks and cleans up subscribers
internal/sms-gateway/modules/events/module.go (1)
17-36: LGTM! Proper error handling and graceful shutdown.The updated module wiring correctly:
- Injects logger and shutdowner for error handling
- Runs the service in a goroutine to avoid blocking startup
- Logs errors and triggers graceful shutdown on failure
- Handles shutdown errors appropriately
internal/sms-gateway/pubsub/module.go (1)
10-29: LGTM! Clean FX module with proper lifecycle management.The Module function follows the standard FX pattern with:
- Logger decoration for scoped logging
- Constructor provisioning via
fx.Provide- OnStop hook for proper resource cleanup
internal/sms-gateway/pubsub/pubsub.go (1)
16-41: LGTM! Well-structured factory with proper defaults.The
Newfactory function correctly:
- Defaults to
memory://when URL is empty- Parses and validates the URL with clear error messages
- Constructs backend-specific configurations
- Handles unknown schemes gracefully
pkg/pubsub/memory.go (4)
10-36: LGTM! Well-structured memory pub/sub with proper synchronization.The
memoryPubSubstruct and constructor are well-designed:
subscriberstruct tracks both channel and context for proper cancellation handling- Constructor applies options pattern for configurable buffer size
- Proper initialization of all fields
38-83: LGTM! Publish with proper synchronization and cancellation handling.The Publish method correctly:
- Holds RLock throughout the iteration (prevents concurrent map modification)
- Checks
sub.ctx.Done()(Line 74) to avoid blocking on cancelled subscribers- Uses per-subscriber goroutines with WaitGroup for blocking semantics
- Respects context cancellation and close signals
85-117: LGTM! Subscribe with correct cleanup ordering.The Subscribe method properly:
- Stores subscriber context for cancellation tracking (Line 100)
- Cleans up in the correct order: unsubscribe from map (Line 110) before closing channel (Line 111) to prevent send-on-closed panic
- Includes
idin the returned Subscription (Line 116) for API consistency
119-153: LGTM! Clean helper methods and idempotent Close.The helper methods and Close implementation are correct:
- Proper locking in subscribe/unsubscribe
- Idempotent Close that returns
nilon repeated calls- Waits for all goroutines before returning
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/cache/memory_bench_test.go (1)
72-77: Good benchmark isolation improvement, minor style inconsistency.The change correctly moves the []byte conversion outside the benchmark timer, which better isolates SetOrFail performance. However, other benchmarks in this file (e.g., BenchmarkMemoryCache_Set at line 25) still convert strings inside their loops.
For consistency, consider applying the same pattern to other benchmarks:
func BenchmarkMemoryCache_Set(b *testing.B) { cache := cache.NewMemory(0) ctx := context.Background() key := "benchmark-key" - value := "benchmark-value" + value := []byte("benchmark-value") b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - cache.Set(ctx, key, []byte(value)) + cache.Set(ctx, key, value) } }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(2 hunks)Makefile(3 hunks)pkg/cache/memory_bench_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 10-10: Target "all" should be declared PHONY.
(phonydeclared)
[warning] 81-81: Missing required phony target "all"
(minphony)
[warning] 81-81: Missing required phony target "clean"
(minphony)
🔇 Additional comments (2)
Makefile (2)
38-38: Confirm "init" target dependency is correct.Line 38 defines
init: deps, which is a good pattern. Verify that this dependency structure aligns with your development workflow, as developers may expectinitto include all initial setup tasks (similar to theinit-devtarget on line 40).
20-21: Test target flags are well-configured.The test target includes
-race,-shuffle=on,-count=1,-covermode=atomic, and-coverpkg=./..., which are solid practices for comprehensive test coverage and concurrency checking.
capcom6
force-pushed
the
events/multi-instance-support
branch
from
02fdc09 to
1068b66
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(2 hunks)Makefile(3 hunks)pkg/cache/memory_bench_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cache/memory_bench_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E
- GitHub Check: Test
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
.gitignore (2)
2-3: Header and footer updates are consistent.The terraform template additions to the header and footer are appropriate for maintaining consistency with generated .gitignore content.
Also applies to: 160-160
164-165: Standard build and test artifact patterns added appropriately.The new ignore patterns (
dist/,tmp/,coverage.*,benchmark.*) are sensible additions for a Go project with build artifacts and test outputs.Also applies to: 171-172
capcom6
force-pushed
the
events/multi-instance-support
branch
from
0323ab9 to
081c21a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/cache/memory_bench_test.go (1)
15-469: Consider standardizing value types across all benchmarks.While the updated benchmarks (Set, SetOrFail, TTLOverhead) now use
[]bytedirectly, many other benchmarks still declare string values and convert them at the call site (e.g., lines 35, 58, 92, 111, 165, 216, 257, 289, 397, 414, 434, 456). For consistency and to eliminate conversion overhead from all measurements, consider updating all benchmarks to use[]bytevalues directly.Example for BenchmarkMemoryCache_Get:
func BenchmarkMemoryCache_Get(b *testing.B) { cache := cache.NewMemory(0) ctx := context.Background() key := "benchmark-key" - value := "benchmark-value" + value := []byte("benchmark-value") // Pre-populate the cache - cache.Set(ctx, key, []byte(value)) + cache.Set(ctx, key, value)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(2 hunks)Makefile(3 hunks)pkg/cache/memory_bench_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/cache/memory_bench_test.go (1)
pkg/cache/options.go (1)
WithTTL(22-30)
🔇 Additional comments (5)
Makefile (2)
9-10: PHONY declarations now complete—all previous concerns resolved.The
.PHONYdeclaration now comprehensively includes all non-file-producing targets:all,fmt,lint,test,benchmark,deps,clean, and the build/docker targets. This fully addresses the prior review feedback on missing PHONY declarations forall, the utility targets (fmt,lint, test group,deps,clean), and the docker targets (docker-build).Also applies to: 81-81
10-10: Target structure and dependency chain is sound.The reorganization provides a clean separation of concerns:
all(line 10) aggregates the primary workflow (fmt, lint, test, benchmark)init(line 38) depends ondepsto ensure dependency resolutioninit-dev(line 40) extendsinitwith development toolstest-e2e(line 60) naturally chains aftertestThe
cleantarget (lines 32–34) now explicitly removes the artifacts (coverage.out,benchmark.txt) generated bytestandbenchmark, maintaining a tidy workspace.Also applies to: 38-40, 60-60
pkg/cache/memory_bench_test.go (3)
20-20: LGTM! Value type aligned with API.The change from string to
[]byteeliminates the conversion overhead from the benchmark measurement, making it more accurate for testing the actual Set operation performance.Also applies to: 25-25
72-72: LGTM! Consistent with Set benchmark changes.The value type change aligns with the API update and eliminates conversion overhead from the benchmark measurement.
Also applies to: 77-77
313-313: LGTM! TTL benchmark updated consistently.The value type change ensures that both TTL and non-TTL code paths are measured without string-to-[]byte conversion overhead, providing a fair comparison.
Also applies to: 330-330, 332-332
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.mod(1 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/modules/push/module.go(1 hunks)internal/sms-gateway/modules/push/service.go(6 hunks)internal/sms-gateway/modules/push/types.go(2 hunks)internal/sms-gateway/modules/push/types/types.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/sms-gateway/app.go (2)
internal/sms-gateway/pubsub/module.go (1)
Module(10-29)internal/config/module.go (1)
Module(22-132)
internal/sms-gateway/modules/push/types.go (3)
internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/events/types.go (1)
Event(9-12)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)
internal/sms-gateway/modules/push/module.go (5)
internal/sms-gateway/app.go (1)
Module(36-60)internal/sms-gateway/modules/push/service.go (2)
Config(21-28)New(41-71)internal/sms-gateway/modules/push/types.go (3)
Mode(10-10)ModeFCM(13-13)ModeUpstream(14-14)internal/sms-gateway/modules/push/fcm/client.go (1)
New(21-25)internal/sms-gateway/modules/push/upstream/client.go (1)
New(26-30)
internal/sms-gateway/modules/push/service.go (4)
internal/sms-gateway/modules/auth/service.go (3)
New(54-68)Config(22-25)Service(39-52)internal/sms-gateway/cache/factory.go (1)
Factory(16-18)internal/sms-gateway/modules/push/metrics.go (1)
BlacklistOperationSkipped(19-19)pkg/cache/options.go (1)
WithTTL(22-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
go.mod (1)
21-21: Dependency verification passed.The version v1.52.0 is the latest stable release (October 8, 2025), and no security advisories have been published for it. The library is actively used in the new pub/sub implementation—confirmed usage of
lo.MapEntries()andlo.MapValues()ininternal/sms-gateway/modules/push/service.go.internal/sms-gateway/modules/push/types/types.go (1)
7-10: LGTM! JSON tags correctly added for serialization.The JSON tags are properly formatted and will enable correct marshaling/unmarshaling of Event instances.
internal/sms-gateway/app.go (1)
24-24: LGTM! Module wiring correctly updated.The addition of
pubsub.Module()and the update ofpush.Moduletopush.Module()correctly follows the function-based module pattern established in this PR.Also applies to: 46-49
internal/sms-gateway/modules/push/types.go (3)
5-5: LGTM! Standard library import for serialization.The
encoding/jsonimport is necessary for the new serialization helpers.
19-22: LGTM! Client interface correctly updated.The Send signature now uses the Event type alias consistently.
25-41: LGTM! Event wrapper with proper serialization support.The
eventWrapperstruct is well-designed with exported fields, JSON tags, and serialization helpers. Thekey()method creates a composite key combining token and event type.internal/sms-gateway/modules/push/module.go (1)
13-52: LGTM! Module refactored to function-based pattern.The module correctly follows the function-based pattern, preserving all wiring and lifecycle management while improving consistency across the codebase.
internal/sms-gateway/modules/push/service.go (3)
16-19: LGTM! Clear cache prefix constants.The cache prefixes are well-named and clearly distinguish between events and blacklist caches.
89-116: LGTM! Enqueue correctly uses composite key.The enqueue logic properly checks the blacklist, serializes the wrapper, and stores it using
wrapper.key()which creates a composite key.
58-59: Verify whether min() capping is intentional or should be max() for enforcing minimums.The review comment correctly identifies that
min()enforces maximum caps rather than minimums. However, without tests, documentation, or design rationale in the codebase, I cannot definitively determine if this is:
- Intentional safety behavior: Capping timeout at 1s and debounce at 5s prevents overly aggressive configurations
- A logic error: Should use
max()to enforce minimum thresholds insteadThe capping pattern is unusual—typically timeout and debounce validation enforces minimum bounds to prevent values that are too small. However, enforcing maximum caps could be a deliberate safety measure.
Please verify this is the intended behavior based on your service design requirements.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/sms-gateway/modules/push/service.go (1)
195-195: Previous critical issue resolved!The key mismatch issue flagged in the previous review has been fixed. Line 195 now correctly uses
wrapper.key()instead oftoken, ensuring consistency with the Enqueue logic (line 109).
🧹 Nitpick comments (1)
internal/sms-gateway/modules/push/service.go (1)
130-143: Consider cleaning up malformed cache entries.The current implementation logs and skips deserialization errors but leaves the malformed data in the cache. This means future
Drain()calls will repeatedly encounter and log the same errors. While this preserves data for debugging, it could lead to noise in logs and metrics.If you want to clean up bad entries, consider tracking failed keys and removing them:
var failedKeys []string wrappers := lo.MapEntries( rawEvents, func(key string, value []byte) (string, *eventWrapper) { wrapper := new(eventWrapper) if err := wrapper.deserialize(value); err != nil { s.metrics.IncError(1) s.logger.Error("Failed to deserialize event wrapper", zap.String("key", key), zap.Binary("value", value), zap.Error(err)) failedKeys = append(failedKeys, key) return "", nil } return wrapper.Token, wrapper }, ) delete(wrappers, "") // Clean up failed entries for _, key := range failedKeys { if err := s.events.Delete(ctx, key); err != nil { s.logger.Warn("Failed to delete malformed cache entry", zap.String("key", key), zap.Error(err)) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/sms-gateway/modules/push/service.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/modules/push/service.go (4)
internal/sms-gateway/cache/factory.go (1)
Factory(16-18)internal/sms-gateway/modules/push/metrics.go (1)
BlacklistOperationSkipped(19-19)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)pkg/cache/options.go (1)
WithTTL(22-30)
🔇 Additional comments (6)
internal/sms-gateway/modules/push/service.go (6)
3-14: LGTM!The new imports are appropriate for the cache factory pattern, option functions, and functional programming utilities introduced in this refactor.
16-19: LGTM!The cache prefix constants provide clear namespacing for separating events and blacklist data stores.
30-39: LGTM!The Service struct refactor properly separates concerns with dedicated caches and adds observability dependencies.
119-129: LGTM!The drain logic properly handles errors and avoids unnecessary processing when the cache is empty.
145-156: LGTM!The functional approach to building the messages map is clean and type-safe.
176-176: Both constants are properly defined in consts.go.The constants
maxRetriesandblacklistTimeoutare already defined at lines 6 and 7 ofinternal/sms-gateway/modules/push/consts.gorespectively. No action is required.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
internal/sms-gateway/modules/push/service.go (4)
196-197: LGTM: fixed cache key mismatch in retry (uses wrapper.key()).
This resolves earlier mismatch between enqueue and retry keys.
104-107: Count serialization failures in metrics.
For parity with other error paths, increment the error metric before returning.wrapperData, err := wrapper.serialize() if err != nil { + s.metrics.IncError(1) return fmt.Errorf("can't serialize event wrapper: %w", err) }
176-179: Use a fresh timeout context for blacklist.Set.
Parent ctx may be cancelled/expired post-send, causing blacklist writes to be skipped.- if err := s.blacklist.Set(ctx, token, []byte{}, cacheImpl.WithTTL(blacklistTimeout)); err != nil { + blCtx, cancel2 := context.WithTimeout(context.Background(), s.config.Timeout) + defer cancel2() + if err := s.blacklist.Set(blCtx, token, []byte{}, cacheImpl.WithTTL(blacklistTimeout)); err != nil { s.logger.Warn("Can't add to blacklist", zap.String("token", token), zap.Error(err)) }
90-97: Handle cache errors distinctly from “not found” in blacklist check.
Don’t treat backend errors as “not blacklisted.” Use errors.Is with the cache’s sentinel.Apply:
+ // import "errors" ctx, cancel := context.WithTimeout(context.Background(), s.config.Timeout) defer cancel() - if _, err := s.blacklist.Get(ctx, token); err == nil { + if _, err := s.blacklist.Get(ctx, token); err == nil { s.metrics.IncBlacklist(BlacklistOperationSkipped) s.logger.Debug("Skipping blacklisted token", zap.String("token", token)) return nil + } else if err != nil && !errors.Is(err, cacheImpl.ErrKeyNotFound) { + s.logger.Warn("Error checking blacklist", zap.String("token", token), zap.Error(err)) + // proceed with enqueue despite cache error }Also add:
import ( + "errors" "context" "fmt" "time"
🧹 Nitpick comments (2)
internal/sms-gateway/modules/push/service.go (2)
189-194: Log level: serialization failure during retry should be Error.
It drops the message for this iteration; elevate severity.wrapperData, err := wrapper.serialize() if err != nil { s.metrics.IncError(1) - s.logger.Info("Can't serialize event wrapper", zap.Error(err)) + s.logger.Error("Can't serialize event wrapper", zap.Error(err)) continue }
197-197: Use Warn level for cache write failure.
Failure to requeue is notable and should not be Info.- s.logger.Info("Can't set message to cache", zap.Error(setErr)) + s.logger.Warn("Can't set message to cache", zap.Error(setErr))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/sms-gateway/modules/push/service.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/modules/push/service.go (3)
internal/sms-gateway/modules/auth/service.go (3)
New(54-68)Config(22-25)Service(39-52)internal/sms-gateway/cache/factory.go (1)
Factory(16-18)pkg/cache/options.go (1)
WithTTL(22-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
internal/sms-gateway/modules/push/service.go (6)
16-19: LGTM: cache key prefixes.
Consistent, clear naming for events/blacklist namespaces.
33-39: LGTM: Service field split into events/blacklist caches.
Improves clarity and enforces separate concerns.
48-56: LGTM: cache factory wiring and error propagation.
Good: fail fast with wrapped errors.
120-128: LGTM: drain short-circuit.
Clean early exit when empty or on drain errors.
153-156: LGTM: scoped send timeout and batch send.
Appropriate timeout scoping for the send operation.
58-59: Confirm Go version or use portable comparisons (built-in max requires Go ≥ 1.21).
If the repo targets Go < 1.21, these won’t compile. Either confirm Go 1.21+ or replace with explicit comparisons.Run:
If Go < 1.21, replace with:
- config.Timeout = max(config.Timeout, time.Second) - config.Debounce = max(config.Debounce, 5*time.Second) + if config.Timeout < time.Second { + config.Timeout = time.Second + } + if config.Debounce < 5*time.Second { + config.Debounce = 5*time.Second + }
capcom6
force-pushed
the
events/multi-instance-support
branch
from
0530daf to
e5ce8da
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/sms-gateway/modules/push/types.go (1)
25-29: Consider unexported struct for internal use.The struct definition is clean with proper JSON tags. However, since
eventWrapperappears to be used only internally within the push module, consider making it unexported (eventWrapper→eventWrapper) to clarify its scope.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.mod(1 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/modules/push/module.go(1 hunks)internal/sms-gateway/modules/push/service.go(6 hunks)internal/sms-gateway/modules/push/types.go(2 hunks)internal/sms-gateway/modules/push/types/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/sms-gateway/modules/push/types/types.go
- go.mod
- internal/sms-gateway/modules/push/service.go
- internal/sms-gateway/modules/push/module.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/sms-gateway/modules/push/types.go (1)
internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)
internal/sms-gateway/app.go (2)
internal/sms-gateway/pubsub/module.go (1)
Module(10-29)internal/config/module.go (1)
Module(22-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build / Docker image (linux/arm64)
- GitHub Check: Build / Docker image (linux/amd64)
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
internal/sms-gateway/app.go (3)
24-24: LGTM! Import addition is correct.The pubsub package import is properly added and necessary for the new Pub/Sub functionality.
46-46: LGTM! Refactoring to factory function is appropriate.The change from
push.Moduletopush.Module()aligns with the factory function pattern used by other modules in the codebase.
49-49: LGTM! Pub/Sub module integration is well-structured.The
pubsub.Module()is correctly added to the fx module wiring. The placement betweencache.Module()andevents.Moduleis appropriate, ensuring proper dependency resolution since events likely depends on pubsub for delivery.internal/sms-gateway/modules/push/types.go (4)
5-5: LGTM: Import added for JSON serialization.The
encoding/jsonimport is correctly added to support the new serialization helpers.
21-21: LGTM: Simplified signature using type alias.The signature now uses the
Eventalias defined on line 17, which improves readability.
35-37: LGTM: Clean serialization helper.The method provides a clear interface for marshaling
eventWrapperto JSON.
39-41: LGTM: Correct deserialization with pointer receiver.The method correctly uses a pointer receiver to unmarshal JSON data into the
eventWrapper.
capcom6
force-pushed
the
events/multi-instance-support
branch
from
457c7f2 to
756af3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
pkg/pubsub/redis.go (1)
112-112: Fix type mismatch in channel capacity.
r.bufferSizeis of typeuint(line 31), butmake(chan Message, ...)requires anintcapacity. This code will not compile.Apply this diff to cast the buffer size:
- ch := make(chan Message, r.bufferSize) + ch := make(chan Message, int(r.bufferSize))Note: For production code, consider guarding against overflow if
r.bufferSize > math.MaxInt, though this is unlikely with typical buffer sizes.internal/sms-gateway/modules/events/metrics.go (1)
22-27: Add a distinct validation failure reason constant.This keeps metrics for validation separate from serialization errors.
const ( DeliveryTypePush = "push" DeliveryTypeSSE = "sse" DeliveryTypeUnknown = "unknown" FailureReasonSerializationError = "serialization_error" FailureReasonPublishError = "publish_error" + FailureReasonValidationError = "validation_error" FailureReasonProviderFailed = "provider_failed" EventTypeUnknown = "unknown" )Also update call sites currently using serialization_error for validation to use validation_error. Based on learnings.
internal/sms-gateway/pubsub/pubsub.go (1)
33-37: Re‑verify Redis backend closes the owned client on Close().Prevent connection leaks when New created the client.
#!/usr/bin/env bash # Show redis Close method and confirm client.Close() is called when owned. rg -nP --type=go -C4 '^func\s+\(r\s+\*redisPubSub\)\s+Close\(\)\s+error' pkg/pubsub/ rg -nP --type=go -C2 'ownedClient\s*:\s*config\.Client\s*==\s*nil' pkg/pubsub/redis.go rg -n 'client.*Close\(' --type=go -C2 pkg/pubsub/internal/sms-gateway/modules/events/service.go (3)
47-49: Add metric increment on validation failure.The validation error on line 48 doesn't increment a failed metric, which is inconsistent with other error paths in this function (lines 62, 67). This makes validation failures invisible in metrics.
As noted in the past review, add the metric increment:
if event.EventType == "" { + s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) return fmt.Errorf("event type is empty") }Note: Consider using a more specific failure reason like
FailureReasonValidationErrorinstead ofFailureReasonSerializationErrorif such a constant exists or can be added.
51-52: Accept parent context to respect caller cancellation.Using
context.Background()ignores any caller's cancellation or timeout. If the caller is cancelled, this operation will still wait up to 5 seconds, potentially causing resource leaks during shutdown.As noted in the past review, update the function signature:
-func (s *Service) Notify(userID string, deviceID *string, event Event) error { +func (s *Service) Notify(ctx context.Context, userID string, deviceID *string, event Event) error { if event.EventType == "" { return fmt.Errorf("event type is empty") } - subCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + pubCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel()
94-100: Validate EventType after deserialization.After successful deserialization (line 95), the code immediately calls
processEventwithout validating thatwrapper.Event.EventTypeis non-empty. JSON deserialization can produce zero-valued fields if the incoming data is missing or malformed, leading to empty EventType strings that will pollute metrics and cause silent failures inprocessEvent.As noted in the past review, add validation after deserialization:
wrapper := new(eventWrapper) if err := wrapper.deserialize(msg.Data); err != nil { s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) s.logger.Error("Failed to deserialize event wrapper", zap.Error(err)) continue } + if wrapper.Event.EventType == "" { + s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) + s.logger.Error("Invalid event: missing EventType") + continue + } s.processEvent(wrapper)internal/sms-gateway/modules/push/service.go (5)
58-59: Enforce minimum values for Timeout and Debounce.Using
max()only enforces maximum values, allowing zero or negative values. A zeroconfig.Timeoutwould causecontext.WithTimeout()calls (line 91, 156) to create immediately-cancelled contexts, leading to operation failures.As noted in the past review, add minimum validation:
+ if config.Timeout <= 0 { + config.Timeout = 100 * time.Millisecond + } config.Timeout = max(config.Timeout, time.Second) + if config.Debounce <= 0 { + config.Debounce = 5 * time.Second + } config.Debounce = max(config.Debounce, 5*time.Second)
94-98: Distinguish cache errors from "not found" in blacklist check.The current code treats all
blacklist.Get()errors as "not blacklisted". If the cache experiences a network outage or timeout, this would bypass the blacklist and potentially spam previously blacklisted tokens.As noted in the past review, distinguish between cache miss and cache error. However, the suggested constant
cache.ErrNotFoundis incorrect. The correct constant frompkg/cache/errors.goiscache.ErrKeyNotFound:+import "errors" + if _, err := s.blacklist.Get(ctx, token); err == nil { s.metrics.IncBlacklist(BlacklistOperationSkipped) s.logger.Debug("Skipping blacklisted token", zap.String("token", token)) return nil + } else if err != nil && !errors.Is(err, cache.ErrKeyNotFound) { + s.logger.Warn("Error checking blacklist", zap.String("token", token), zap.Error(err)) + // Continue with enqueue despite cache error to avoid blocking legitimate messages }
89-90: Update docstring to reflect enforced minimum debounce.The comment states "immediately sends all messages if the debounce is 0", but the
New()constructor enforces a minimum debounce of 5 seconds (line 59), making immediate send unreachable.Update the comment to reflect the actual behavior:
-// Enqueue adds the data to the cache and immediately sends all messages if the debounce is 0. +// Enqueue adds the data to the cache. Messages are sent on the next ticker interval (minimum 5 seconds). func (s *Service) Enqueue(token string, event types.Event) error {
133-153: Nondeterministic event selection causes event loss.When multiple events exist for the same token, the code collapses them using
lo.MapEntries, which relies on Go's randomized map iteration order. This means which event "wins" for a given token is unpredictable, and other events are silently dropped afterDrain().As noted in the past review, deterministically select one event per token. For example, pick the lexicographically maximum cache key per token:
- wrappers := lo.MapEntries( - rawEvents, - func(key string, value []byte) (string, *eventWrapper) { - wrapper := new(eventWrapper) - if err := wrapper.deserialize(value); err != nil { - s.metrics.IncError(1) - s.logger.Error("Failed to deserialize event wrapper", zap.String("key", key), zap.Binary("value", value), zap.Error(err)) - return "", nil - } - - return wrapper.Token, wrapper - }, - ) - delete(wrappers, "") + selected := make(map[string]*eventWrapper) + selectedKey := make(map[string]string) + for key, value := range rawEvents { + w := new(eventWrapper) + if err := w.deserialize(value); err != nil { + s.metrics.IncError(1) + s.logger.Error("Failed to deserialize event wrapper", zap.String("key", key), zap.Int("value_len", len(value)), zap.Error(err)) + continue + } + if k, ok := selectedKey[w.Token]; !ok || key > k { + selected[w.Token] = w + selectedKey[w.Token] = key + } + } + if len(selected) == 0 { + return + } messages := lo.MapValues( - wrappers, + selected, func(value *eventWrapper, key string) Event { return value.Event }, )Also update line 176:
- wrapper := wrappers[token] + wrapper := selected[token]
180-182: Use fresh context for blacklist operations.The
blacklist.Setcall uses the parentctxfromsendAll, which may already be cancelled or timing out. If the send operation exhausted the context deadline, the blacklist update will fail silently, leaving the token retryable.As noted in the past review, use a fresh context:
- if err := s.blacklist.Set(ctx, token, []byte{}, cacheImpl.WithTTL(blacklistTimeout)); err != nil { + blacklistCtx, cancel := context.WithTimeout(context.Background(), s.config.Timeout) + defer cancel() + if err := s.blacklist.Set(blacklistCtx, token, []byte{}, cacheImpl.WithTTL(blacklistTimeout)); err != nil { s.logger.Warn("Can't add to blacklist", zap.String("token", token), zap.Error(err)) }
🧹 Nitpick comments (7)
pkg/cache/memory_bench_test.go (1)
1-469: Consider applying the same pattern file-wide for consistency.While the changes to the three benchmarks are correct, the file now has mixed patterns—some benchmarks declare
[]bytevalues upfront while others convert strings at the call site. For consistency and clarity, consider updating the remaining benchmarks to follow the same direct[]bytedeclaration pattern.pkg/pubsub/redis.go (2)
164-166: Align Close behavior with memory implementation for API consistency.The Redis implementation returns
nilon repeatedClose()calls (line 166), while the memory implementation returnsErrPubSubClosed. This behavioral inconsistency can confuse users who switch between backends or write tests against different implementations.Based on learnings
Apply this diff to match the memory implementation's behavior:
func (r *redisPubSub) Close() error { select { case <-r.closeCh: - return nil + return ErrPubSubClosed default: close(r.closeCh) }
78-90: Document fire-and-forget Publish semantics.The Redis
Publishimplementation returns immediately after sending to Redis (fire-and-forget), whereas the memory implementation blocks until all subscribers receive the message. This behavioral difference should be documented to set correct expectations.Based on learnings
Consider adding documentation above the method:
// Publish sends a message to the Redis channel for the given topic. // This is a fire-and-forget operation that returns once Redis accepts // the message, without waiting for subscribers to receive it. // This differs from the memory implementation which blocks until delivery. func (r *redisPubSub) Publish(ctx context.Context, topic string, data []byte) error {internal/sms-gateway/modules/push/module.go (1)
21-29: Include the invalid mode value in the error.Helps ops debug misconfig quickly.
-import ( - "context" - "errors" +import ( + "context" + "errors" + "fmt" @@ - default: - return nil, errors.New("invalid push mode") + default: + return nil, fmt.Errorf("invalid push mode: %q", cfg.Mode)internal/sms-gateway/pubsub/pubsub.go (1)
3-8: Normalize URL scheme and support rediss.Accept mixed‑case schemes and TLS Redis URLs.
-import ( - "fmt" - "net/url" - - "github.com/android-sms-gateway/server/pkg/pubsub" -) +import ( + "fmt" + "net/url" + "strings" + + "github.com/android-sms-gateway/server/pkg/pubsub" +) @@ - switch u.Scheme { + scheme := strings.ToLower(u.Scheme) + switch scheme { case "memory": return pubsub.NewMemory(opts...), nil - case "redis": + case "redis", "rediss": return pubsub.NewRedis(pubsub.RedisConfig{ Client: nil, URL: config.URL, Prefix: topicPrefix, }, opts...) default: - return nil, fmt.Errorf("invalid scheme: %s", u.Scheme) + return nil, fmt.Errorf("invalid scheme: %s", u.Scheme) }Also applies to: 21-40
pkg/pubsub/memory.go (1)
131-140: Remove empty topic maps on last unsubscribe.Prevents unbounded growth when topics churn.
func (m *memoryPubSub) unsubscribe(id, topic string) { m.mu.Lock() defer m.mu.Unlock() subscriptions, ok := m.topics[topic] if !ok { return } delete(subscriptions, id) + if len(subscriptions) == 0 { + delete(m.topics, topic) + } }internal/sms-gateway/modules/events/events.go (1)
17-24: Normalize timestamps to UTC for stable payloads.Reduces consumer ambiguity across timezones.
map[string]string{ - "since": since.Format(time.RFC3339), - "until": until.Format(time.RFC3339), + "since": since.UTC().Format(time.RFC3339), + "until": until.UTC().Format(time.RFC3339), },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
.gitignore(2 hunks)Makefile(3 hunks)configs/config.example.yml(1 hunks)go.mod(1 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/cache/factory.go(2 hunks)internal/sms-gateway/modules/events/events.go(2 hunks)internal/sms-gateway/modules/events/metrics.go(1 hunks)internal/sms-gateway/modules/events/module.go(1 hunks)internal/sms-gateway/modules/events/service.go(2 hunks)internal/sms-gateway/modules/events/types.go(1 hunks)internal/sms-gateway/modules/push/module.go(1 hunks)internal/sms-gateway/modules/push/service.go(6 hunks)internal/sms-gateway/modules/push/types.go(2 hunks)internal/sms-gateway/modules/push/types/types.go(1 hunks)internal/sms-gateway/pubsub/config.go(1 hunks)internal/sms-gateway/pubsub/module.go(1 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)pkg/cache/cache.go(1 hunks)pkg/cache/memory.go(1 hunks)pkg/cache/memory_bench_test.go(4 hunks)pkg/cache/redis.go(2 hunks)pkg/pubsub/memory.go(1 hunks)pkg/pubsub/redis.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/cache/cache.go
- configs/config.example.yml
- go.mod
- internal/sms-gateway/cache/factory.go
- pkg/cache/memory.go
- pkg/cache/redis.go
- internal/sms-gateway/modules/push/types.go
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (14)
internal/sms-gateway/pubsub/module.go (4)
internal/sms-gateway/app.go (1)
Module(36-60)internal/config/module.go (1)
Module(22-132)internal/sms-gateway/pubsub/pubsub.go (2)
New(16-41)PubSub(14-14)internal/config/config.go (1)
PubSub(85-87)
internal/config/config.go (1)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)
internal/sms-gateway/modules/events/service.go (5)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/events/types.go (1)
Event(9-12)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/events/metrics.go (7)
DeliveryTypeUnknown(20-20)FailureReasonSerializationError(22-22)FailureReasonPublishError(23-23)EventTypeUnknown(26-26)DeliveryTypePush(18-18)FailureReasonProviderFailed(24-24)DeliveryTypeSSE(19-19)
pkg/pubsub/memory.go (2)
pkg/pubsub/pubsub.go (5)
Message(13-16)ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)PubSub(45-50)pkg/pubsub/options.go (1)
Option(3-3)
internal/config/module.go (3)
internal/config/config.go (2)
Config(10-20)PubSub(85-87)internal/sms-gateway/pubsub/config.go (1)
Config(4-7)internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)
internal/sms-gateway/modules/push/module.go (6)
internal/sms-gateway/app.go (1)
Module(36-60)internal/sms-gateway/modules/messages/module.go (1)
Module(11-35)internal/sms-gateway/modules/push/service.go (2)
Config(21-28)New(41-71)internal/sms-gateway/modules/push/types.go (3)
Mode(10-10)ModeFCM(13-13)ModeUpstream(14-14)internal/sms-gateway/modules/push/fcm/client.go (1)
New(21-25)internal/sms-gateway/modules/push/upstream/client.go (1)
New(26-30)
pkg/cache/memory_bench_test.go (1)
pkg/cache/options.go (1)
WithTTL(22-30)
internal/sms-gateway/pubsub/pubsub.go (5)
internal/config/config.go (2)
PubSub(85-87)Config(10-20)internal/sms-gateway/pubsub/config.go (1)
Config(4-7)pkg/pubsub/options.go (1)
WithBufferSize(17-21)pkg/pubsub/memory.go (1)
NewMemory(24-36)pkg/pubsub/redis.go (2)
NewRedis(42-76)RedisConfig(15-27)
internal/sms-gateway/modules/events/module.go (2)
internal/sms-gateway/modules/events/service.go (1)
Service(19-30)internal/sms-gateway/app.go (1)
Run(62-73)
internal/sms-gateway/modules/events/types.go (3)
internal/sms-gateway/modules/push/types.go (1)
Event(17-17)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)
internal/sms-gateway/app.go (3)
internal/sms-gateway/modules/push/module.go (1)
Module(13-52)internal/config/module.go (1)
Module(22-132)internal/sms-gateway/pubsub/module.go (1)
Module(10-29)
internal/sms-gateway/modules/push/service.go (4)
internal/sms-gateway/cache/factory.go (1)
Factory(16-18)internal/sms-gateway/modules/push/metrics.go (1)
BlacklistOperationSkipped(19-19)internal/sms-gateway/modules/push/types.go (1)
Event(17-17)pkg/cache/options.go (1)
WithTTL(22-30)
pkg/pubsub/redis.go (3)
pkg/pubsub/options.go (1)
Option(3-3)pkg/pubsub/pubsub.go (5)
ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)Message(13-16)PubSub(45-50)internal/config/config.go (1)
PubSub(85-87)
internal/sms-gateway/modules/events/events.go (2)
internal/sms-gateway/modules/push/types.go (1)
Event(17-17)internal/sms-gateway/modules/events/types.go (2)
Event(9-12)NewEvent(14-19)
🔇 Additional comments (15)
Makefile (1)
9-10: Excellent—past review feedback fully addressed.The PHONY declaration now includes all non-file-producing targets (all, fmt, lint, test, benchmark, deps, clean, ngrok, docker-build, etc.), resolving all prior issues. The Makefile is well-structured with:
- A sensible default "all" target tying fmt, lint, test, and benchmark
- Clear separation of concerns (formatting, linting, testing, benchmarking, dependencies, cleanup)
- Proper test and e2e sequencing
- Complete and accurate PHONY declarations
No concerns.
Also applies to: 12-34, 38-38, 60-60, 81-81
pkg/cache/memory_bench_test.go (3)
16-28: LGTM! Improved benchmark accuracy.Using
[]bytedirectly removes the string-to-byte-slice conversion overhead from the benchmark measurement, providing a more accurate assessment of the cache Set operation itself.
68-80: LGTM! Consistent improvement.The same optimization applied to SetOrFail improves benchmark precision.
309-338: LGTM! TTL comparison is now cleaner.Both branches now pass the value directly, ensuring the benchmark measures only the TTL overhead without conversion noise.
internal/config/config.go (1)
19-19: LGTM! Clean configuration additions.The PubSub configuration follows the same pattern as the existing Cache configuration, with a sensible default of
"memory://"that aligns with the in-memory backend.Also applies to: 85-87, 121-123
internal/sms-gateway/pubsub/config.go (1)
1-7: LGTM! Clean configuration structure.The
Configstruct is well-defined with clear field names and a helpful comment explaining the URL-based backend selection pattern.internal/config/module.go (1)
14-14: LGTM! Correct configuration mapping.The pubsub.Config provider correctly maps
cfg.PubSub.URL(the previous copy-paste error usingcfg.Cache.URLhas been fixed) and uses a reasonable fixed buffer size of 128.Also applies to: 126-131
internal/sms-gateway/modules/push/types/types.go (1)
8-9: LGTM! Proper JSON serialization support.The JSON tags enable proper marshaling/unmarshaling of Event structs for transport over the Pub/Sub layer, with clear and appropriate field names.
internal/sms-gateway/modules/events/module.go (1)
17-28: LGTM! Robust error handling with proper shutdown.The enhanced lifecycle management properly logs errors from
svc.Runand triggers a controlled shutdown viafx.Shutdownerwhen the events service fails, ensuring the application doesn't continue in a degraded state.internal/sms-gateway/app.go (1)
24-24: LGTM! Proper module wiring.The pubsub module is correctly integrated into the FX application, and the change to
push.Module()(function invocation) maintains consistency with other module patterns in the application.Also applies to: 46-46, 49-49
internal/sms-gateway/pubsub/module.go (1)
10-28: LGTM! Well-structured FX module with proper lifecycle management.The module correctly provides the pubsub constructor and ensures graceful shutdown via the
OnStophook. Error logging on close failure provides good visibility into potential issues during application shutdown.pkg/pubsub/memory.go (1)
52-83: Publish concurrency looks solid.RLock held during snapshot/send with per‑sub goroutines and sub.ctx handling avoids send‑on‑closed and deadlocks. LGTM.
internal/sms-gateway/modules/events/events.go (2)
9-15: Switch to value return: LGTM.Constructors returning Event by value align with types.NewEvent. Minimal and clear.
Also applies to: 27-29
9-29: No issues found—all callers already aligned with value semantics.All four call sites (messages/service.go:243, messages/service.go:252, webhooks/service.go:125, settings/service.go:91) correctly pass
Eventvalues toNotify(), which expectsevent Event(value type). The type system confirms consistency:NewEvent()returnsEvent, and no*Eventpointers remain in the module.internal/sms-gateway/modules/push/module.go (1)
48-51: The review comment is incorrect.The fx documentation states "Constructors are called only if one or more of their returned types are needed" and "fx.Invoke forces the execution of dependencies". In this codebase,
*push.Servicehas an explicit consumer: theevents.Serviceconstructor requires it as a parameter (events/service.go:32). Sinceevents.Serviceis explicitly invoked viafx.Invokein events/module.go:17, fx will construct it, which requires satisfying its dependency on*push.Service. Therefore,push.New()will be called and*push.Servicewill be constructed.The provided code at push/module.go:48-51 is correct.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/pubsub/memory.go (1)
85-117: Implementation is correct - consider adding documentation.The Subscribe method properly addresses all critical issues from past reviews:
- Subscriber is registered with both channel and context (line 100)
- Cleanup goroutine unsubscribes before closing the channel (lines 110-111), preventing send-on-closed panics
- Subscription includes the
idfield (line 116), maintaining API consistency with the Redis implementationThe resource management and cleanup sequence are correct.
Consider adding a doc comment similar to Publish for consistency:
+// Subscribe creates a new subscription to the given topic. +// The returned Subscription will receive messages published to the topic +// until the context is cancelled or the pubsub instance is closed. func (m *memoryPubSub) Subscribe(ctx context.Context, topic string) (*Subscription, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/sms-gateway/modules/push/module.go(1 hunks)pkg/pubsub/memory.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/sms-gateway/modules/push/module.go (6)
internal/sms-gateway/app.go (1)
Module(36-60)internal/config/module.go (1)
Module(22-132)internal/sms-gateway/modules/push/service.go (2)
Config(21-28)New(41-71)internal/sms-gateway/modules/push/types.go (3)
Mode(10-10)ModeFCM(13-13)ModeUpstream(14-14)internal/sms-gateway/modules/push/fcm/client.go (1)
New(21-25)internal/sms-gateway/modules/push/upstream/client.go (1)
New(26-30)
pkg/pubsub/memory.go (2)
pkg/pubsub/pubsub.go (5)
Message(13-16)ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)PubSub(45-50)pkg/pubsub/options.go (1)
Option(3-3)
🔇 Additional comments (9)
internal/sms-gateway/modules/push/module.go (3)
5-5: LGTM!The
fmtimport is appropriately added to support the improved error formatting on line 28.
13-52: LGTM!The conversion from a module variable to a function-based approach is well-executed. The internal wiring logic is preserved, and the change aligns with the broader pattern adopted across other modules (as seen with
pubsub.Module()andonline.Module()in app.go). All callers have been updated to usepush.Module().
28-28: Enhanced error message!The error message now includes the actual mode value, making it much easier to diagnose configuration issues at runtime.
pkg/pubsub/memory.go (6)
10-22: LGTM! Solid concurrent design.The struct definitions correctly implement the fixes from previous reviews. The
subscriberstruct now stores both the channel and context, which prevents the deadlock scenario where Publish could block indefinitely on cancelled subscribers.
24-36: Constructor looks good.Proper initialization with sensible defaults. The unbuffered channel default (bufferSize: 0) works correctly with the goroutine-based broadcast pattern in Publish.
41-83: Excellent implementation - all critical issues resolved.The Publish method correctly addresses all previously identified critical issues:
- RLock is held throughout iteration (deferred unlock on line 53), preventing concurrent map modification panics
- Each subscriber's context is checked (line 74-75), preventing deadlocks when subscribers cancel
- Per-subscriber goroutines with WaitGroup ensure reliable delivery while respecting cancellation
The broadcast semantics are correct and thread-safe.
119-143: Helper methods are correct and well-structured.Both
subscribeandunsubscribeproperly synchronize access to the topics map. The cleanup of empty topics (lines 140-142) prevents memory leaks when all subscribers leave a topic.
145-156: Clean shutdown implementation.The Close method correctly handles:
- Concurrent close attempts (lines 146-150 prevent double-close panics)
- Graceful shutdown by signaling all operations via closeCh
- Waiting for all subscription cleanup goroutines to complete
158-158: Good practice - compile-time interface check.
capcom6
force-pushed
the
events/multi-instance-support
branch
from
098e2c1 to
ed271b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
internal/sms-gateway/modules/push/service.go (6)
179-181: Use a fresh, time‑bounded context for blacklist Set.Avoid reusing a possibly canceled/long‑lived ctx; ensures blacklist is attempted reliably.
- if wrapper.Retries >= maxRetries { - if err := s.blacklist.Set(ctx, token, []byte{}, cacheImpl.WithTTL(blacklistTimeout)); err != nil { + if wrapper.Retries >= maxRetries { + blCtx, cancel := context.WithTimeout(context.Background(), s.config.Timeout) + defer cancel() + if err := s.blacklist.Set(blCtx, token, []byte{}, cacheImpl.WithTTL(blacklistTimeout)); err != nil { s.logger.Warn("Can't add to blacklist", zap.String("token", token), zap.Error(err)) }Based on learnings.
133-145: Deterministic selection when multiple events per token.Collapsing via map loses all-but-one event with random choice (map iteration order). Pick deterministically or handle multiple.
- wrappers := lo.MapEntries( - rawEvents, - func(key string, value []byte) (string, *eventWrapper) { - wrapper := new(eventWrapper) - if err := wrapper.deserialize(value); err != nil { - s.metrics.IncError(1) - s.logger.Error("Failed to deserialize event wrapper", zap.String("key", key), zap.Binary("value", value), zap.Error(err)) - return "", nil - } - - return wrapper.Token, wrapper - }, - ) - delete(wrappers, "") + selected := make(map[string]*eventWrapper) + selectedKey := make(map[string]string) + for key, value := range rawEvents { + w := new(eventWrapper) + if err := w.deserialize(value); err != nil { + s.metrics.IncError(1) + s.logger.Error("Failed to deserialize event wrapper", zap.String("key", key), zap.Int("value_len", len(value)), zap.Error(err)) + continue + } + if k, ok := selectedKey[w.Token]; !ok || key > k { // deterministic: lexicographic max key wins + selected[w.Token] = w + selectedKey[w.Token] = key + } + } + if len(selected) == 0 { + return + } @@ - messages := lo.MapValues( - wrappers, + messages := lo.MapValues( + selected, func(value *eventWrapper, key string) types.Event { return value.Event }, ) @@ - wrapper := wrappers[token] + wrapper := selected[token]Based on learnings.
Also applies to: 173-179
94-98: Do not treat cache errors as “not blacklisted.”Differentiate “key not found” from real cache errors to avoid bypassing blacklist during outages/timeouts.
+ // import "errors" if _, err := s.blacklist.Get(ctx, token); err == nil { s.metrics.IncBlacklist(BlacklistOperationSkipped) s.logger.Debug("Skipping blacklisted token", zap.String("token", token)) return nil + } else if err != nil && !errors.Is(err, cacheImpl.ErrKeyNotFound) { + s.logger.Warn("Error checking blacklist", zap.String("token", token), zap.Error(err)) + // Proceed with enqueue to avoid blocking legitimate messages }Also add
errorsto imports.
89-90: Comment drift about debounce==0.New enforces Debounce ≥ 5s, so “immediately sends if debounce is 0” is unreachable. Update the comment or support 0 explicitly.
137-141: Avoid logging raw payload bytes (privacy).
zap.Binary("value", value)may leak sensitive data. Log length or a hash instead.- s.logger.Error("Failed to deserialize event wrapper", zap.String("key", key), zap.Binary("value", value), zap.Error(err)) + s.logger.Error("Failed to deserialize event wrapper", zap.String("key", key), zap.Int("value_len", len(value)), zap.Error(err))
148-153: Fix compile error: use types.Event.
Eventis undefined in this scope; you importedtypes. Usetypes.Event.- messages := lo.MapValues( - wrappers, - func(value *eventWrapper, key string) Event { - return value.Event - }, - ) + messages := lo.MapValues( + wrappers, + func(value *eventWrapper, key string) types.Event { + return value.Event + }, + )pkg/pubsub/redis.go (1)
112-113: Compile error: channel capacity must be int.
r.bufferSizeisuint;make(chan _, cap)requiresint. Cast (and optionally guard).Apply this minimal fix:
- ch := make(chan Message, r.bufferSize) + ch := make(chan Message, int(r.bufferSize))(Optional guard against overflow if ever user-configurable.)
internal/sms-gateway/modules/events/service.go (1)
47-49: Increment failure metric before returning validation error.The validation at line 48 returns an error without incrementing a failure metric, creating an observability gap. For consistency with serialization (line 62) and publish (line 67) error handling, add a metric increment:
if event.EventType == "" { + s.metrics.IncrementFailed(EventTypeUnknown, DeliveryTypeUnknown, FailureReasonSerializationError) return fmt.Errorf("event type is empty") }Note: Consider adding a distinct
FailureReasonValidationErrorconstant to differentiate validation failures from serialization errors in metrics.
🧹 Nitpick comments (5)
internal/config/config.go (1)
19-20: Make pubsub buffer size configurable (optional).Consider exposing a Pub/Sub buffer size in config (e.g.,
pubsub.buffer_size, envPUBSUB__BUFFER_SIZE) instead of hardcoding via wiring. Helps tune backpressure without code changes. Based on learnings.Also applies to: 85-87, 121-123
pkg/pubsub/redis.go (2)
163-169: Align Close() semantics with memory backend.Memory returns
ErrPubSubClosedon repeat Close; Redis returns nil. Standardize for API consistency.Apply:
func (r *redisPubSub) Close() error { select { case <-r.closeCh: - return nil + return ErrPubSubClosed default: close(r.closeCh) }Based on learnings.
78-90: Document non-blocking Publish semantics.Add a short comment noting fire-and-forget behavior vs memory backend’s blocking delivery, or unify semantics later.
- func (r *redisPubSub) Publish(ctx context.Context, topic string, data []byte) error { +// Publish sends to Redis and returns once Redis accepts the message. +// It does not wait for subscribers to receive it (fire-and-forget). +// NOTE: This differs from the in-memory backend which can block until delivery. +func (r *redisPubSub) Publish(ctx context.Context, topic string, data []byte) error {Based on learnings.
internal/sms-gateway/pubsub/config.go (1)
3-7: Clarify defaults and guard zero BufferSize.Document the default URL (“memory://”) and apply a non-zero default for BufferSize at construction time to avoid passing 0 to the backend.
Consider:
type Config struct { - URL string - BufferSize uint + // URL selects backend: e.g. "memory://", "redis://host[:port]/db" + URL string + // BufferSize caps per-topic buffers; 0 means “use module default”. + BufferSize uint }And in the constructor (internal/sms-gateway/pubsub/pubsub.go), before WithBufferSize:
+ if config.BufferSize == 0 { + config.BufferSize = 128 + }internal/sms-gateway/pubsub/module.go (1)
10-29: Add an OnStart connectivity check for Redis to fail fast.Validate the backend at startup to surface misconfig early, especially for Redis URLs.
- fx.Invoke(func(ps PubSub, logger *zap.Logger, lc fx.Lifecycle) { + fx.Invoke(func(ps PubSub, logger *zap.Logger, lc fx.Lifecycle) { lc.Append(fx.Hook{ + OnStart: func(ctx context.Context) error { + // Optional: only ping for redis-backed implementations if exposed + if pinger, ok := interface{ Ping(ctx context.Context) error }(ps); ok { + if err := pinger.Ping(ctx); err != nil { + logger.Error("pubsub ping failed", zap.Error(err)) + return err + } + } + return nil + }, OnStop: func(_ context.Context) error { if err := ps.Close(); err != nil { logger.Error("pubsub close failed", zap.Error(err)) return err } return nil }, }) }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
.gitignore(2 hunks)Makefile(3 hunks)configs/config.example.yml(1 hunks)go.mod(1 hunks)internal/config/config.go(3 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/cache/factory.go(2 hunks)internal/sms-gateway/modules/events/events.go(2 hunks)internal/sms-gateway/modules/events/metrics.go(1 hunks)internal/sms-gateway/modules/events/module.go(1 hunks)internal/sms-gateway/modules/events/service.go(2 hunks)internal/sms-gateway/modules/events/types.go(1 hunks)internal/sms-gateway/modules/push/module.go(1 hunks)internal/sms-gateway/modules/push/service.go(6 hunks)internal/sms-gateway/modules/push/types.go(2 hunks)internal/sms-gateway/modules/push/types/types.go(1 hunks)internal/sms-gateway/pubsub/config.go(1 hunks)internal/sms-gateway/pubsub/module.go(1 hunks)internal/sms-gateway/pubsub/pubsub.go(1 hunks)pkg/cache/cache.go(1 hunks)pkg/cache/memory.go(1 hunks)pkg/cache/memory_bench_test.go(4 hunks)pkg/cache/redis.go(2 hunks)pkg/pubsub/memory.go(1 hunks)pkg/pubsub/options.go(1 hunks)pkg/pubsub/pubsub.go(1 hunks)pkg/pubsub/redis.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/pubsub/options.go
- internal/sms-gateway/modules/events/metrics.go
- internal/sms-gateway/cache/factory.go
- internal/config/module.go
- pkg/cache/memory_bench_test.go
- pkg/cache/memory.go
- configs/config.example.yml
- internal/sms-gateway/modules/push/types.go
🧰 Additional context used
🧬 Code graph analysis (12)
internal/sms-gateway/modules/events/module.go (2)
internal/sms-gateway/modules/events/service.go (1)
Service(19-30)internal/sms-gateway/app.go (1)
Run(62-73)
internal/sms-gateway/pubsub/module.go (4)
internal/sms-gateway/app.go (1)
Module(36-60)internal/config/module.go (1)
Module(22-132)internal/sms-gateway/pubsub/pubsub.go (2)
New(16-41)PubSub(14-14)internal/config/config.go (1)
PubSub(85-87)
internal/sms-gateway/app.go (3)
internal/sms-gateway/modules/push/module.go (1)
Module(13-52)internal/config/module.go (1)
Module(22-132)internal/sms-gateway/pubsub/module.go (1)
Module(10-29)
internal/sms-gateway/modules/push/module.go (5)
internal/sms-gateway/app.go (1)
Module(36-60)internal/sms-gateway/modules/push/service.go (2)
Config(21-28)New(41-71)internal/sms-gateway/modules/push/types.go (3)
Mode(10-10)ModeFCM(13-13)ModeUpstream(14-14)internal/sms-gateway/modules/push/fcm/client.go (1)
New(21-25)internal/sms-gateway/modules/push/upstream/client.go (1)
New(26-30)
internal/config/config.go (1)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)
pkg/pubsub/memory.go (2)
pkg/pubsub/pubsub.go (5)
Message(13-16)ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)PubSub(45-50)pkg/pubsub/options.go (1)
Option(3-3)
internal/sms-gateway/modules/events/events.go (1)
internal/sms-gateway/modules/events/types.go (2)
Event(9-12)NewEvent(14-19)
internal/sms-gateway/modules/push/service.go (4)
internal/sms-gateway/cache/factory.go (1)
Factory(16-18)internal/sms-gateway/modules/push/metrics.go (1)
BlacklistOperationSkipped(19-19)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)pkg/cache/options.go (1)
WithTTL(22-30)
internal/sms-gateway/modules/events/service.go (5)
internal/sms-gateway/pubsub/pubsub.go (1)
PubSub(14-14)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/events/types.go (1)
Event(9-12)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)internal/sms-gateway/modules/events/metrics.go (7)
DeliveryTypeUnknown(20-20)FailureReasonSerializationError(22-22)FailureReasonPublishError(23-23)EventTypeUnknown(26-26)DeliveryTypePush(18-18)FailureReasonProviderFailed(24-24)DeliveryTypeSSE(19-19)
internal/sms-gateway/pubsub/pubsub.go (4)
internal/sms-gateway/pubsub/config.go (1)
Config(4-7)pkg/pubsub/options.go (1)
WithBufferSize(17-21)pkg/pubsub/memory.go (1)
NewMemory(24-36)pkg/pubsub/redis.go (2)
NewRedis(42-76)RedisConfig(15-27)
internal/sms-gateway/modules/events/types.go (3)
internal/sms-gateway/modules/push/types.go (1)
Event(17-17)internal/sms-gateway/modules/push/types/types.go (1)
Event(7-10)internal/sms-gateway/modules/sse/types.go (1)
Event(7-10)
pkg/pubsub/redis.go (2)
pkg/pubsub/options.go (1)
Option(3-3)pkg/pubsub/pubsub.go (5)
ErrPubSubClosed(9-9)ErrInvalidTopic(10-10)Subscription(18-23)Message(13-16)PubSub(45-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (21)
.gitignore (1)
2-3: Reasonable gitignore updates for project expansion.The changes appropriately extend .gitignore to reflect the project's growing scope:
- Updated headers and end marker to include Terraform templates (consistent with infrastructure-as-code additions in this PR)
- Added
dist/andtmp/for build and temporary artifacts- Expanded
go.worktogo.work*to capture versioned workspace files- Added
coverage.*andbenchmark.*for test artifactsAll patterns are sensible and follow standard practices for Go/Terraform projects.
Also applies to: 160-160, 164-172
Makefile (1)
9-10: Excellent refactor—all previous feedback addressed.The Makefile reorganization successfully resolves all prior PHONY declaration issues. The new composite
alltarget, explicitfmt,lint,test,benchmark,deps, andcleantargets, plus the expanded.PHONYdeclaration now includes every non-file-producing target (all, fmt, lint, test, benchmark, deps, clean, init, init-dev, air, ngrok, db-upgrade, db-upgrade-raw, run, test-e2e, build, install, docker-build, docker, docker-dev, docker-clean). Target dependencies are sensible (initdepends ondeps,test-e2edepends ontest), supporting the structured initialization and testing workflows needed for the new pubsub implementations.Also applies to: 12-13, 15-17, 19-21, 23-25, 27-29, 31-34, 36-81
pkg/cache/cache.go (1)
37-39: LGTM! Close method addition enables proper resource cleanup.The new
Close()method is a necessary addition for graceful shutdown of cache backends, especially for Redis connections. The documentation is clear and consistent with other methods in the interface.pkg/cache/redis.go (5)
51-66: LGTM! Flexible configuration design.The
RedisConfigstruct provides a clean API that supports both externally-managed clients and automatic client creation from URL. The documentation clearly explains the mutually-exclusive nature ofClientandURLfields.
68-75: LGTM! Ownership tracking enables proper resource management.The addition of the
ownedClientfield allows the cache to correctly determine whether it should close the Redis client during cleanup, preventing double-close scenarios when clients are shared.
77-104: LGTM! Well-structured constructor with proper validation.The refactored constructor handles both client provisioning scenarios correctly:
- Validates that at least one of Client or URL is provided
- Normalizes the prefix to ensure consistent key formatting
- Tracks ownership appropriately for cleanup
The error handling properly wraps URL parsing errors with context.
254-260: LGTM! Close implementation correctly handles ownership.The implementation properly checks ownership before closing the client, preventing double-close issues when clients are externally managed. The underlying
redis.Client.Close()method should handle concurrent calls safely.
262-262: LGTM! Good practice for interface conformance.The compile-time assertion ensures that
redisCachecorrectly implements theCacheinterface, including the newly addedClose()method. This catches any implementation gaps at compile time rather than runtime.go.mod (2)
21-21: Dependency addition looks good.The v1.52.0 release is the latest version, bringing major improvements including a new it/ sub-package for Go 1.23's iter package. The library is v1 and follows SemVer strictly with no breaking changes before v2.0.0, and has no dependencies outside the Go standard library. The addition aligns with the Pub/Sub refactoring for functional utilities.
Verify that
samber/lois actually used in the Pub/Sub implementation files to ensure the dependency is not unnecessary. You can search the codebase for imports of this library.
21-21: Dependency addition is appropriate and stable.The
github.com/samber/lo v1.52.0addition is a stable SemVer v1 release with no breaking changes planned. It's already imported and actively used ininternal/sms-gateway/modules/push/service.gofor functional utilities (lo.MapEntries()andlo.MapValues()), and Go 1.24.1 meets the library's requirement of Go 1.18+. No further action needed.internal/sms-gateway/modules/push/types/types.go (1)
8-9: LGTM: JSON tags added for wire format stability.pkg/pubsub/redis.go (1)
103-109: Good: subscription handshake and cleanup.Waiting for
ps.Receive(ctx)and closingpson error avoids missed messages and leaks. Nice.pkg/pubsub/pubsub.go (1)
45-50: LGTM: clear, minimal Pub/Sub interface and Subscription API.internal/sms-gateway/app.go (2)
24-24: LGTM: pubsub import added where used.
46-49: Wiring order verified; lifecycle shutdown order is correct.The full module wiring confirms pubsub.Module() at line 49 precedes events.Module at line 50. In Fx framework's reverse shutdown order, events stops before pubsub closes—exactly as your review comment described. The approval reasoning is sound and no further changes are needed.
internal/sms-gateway/modules/push/module.go (1)
13-52: LGTM: module refactor to function with clearer error handling.Mode switching, lifecycle hooks, and constructor visibility look good.
internal/sms-gateway/modules/events/module.go (1)
24-25: I found the inconsistency you mentioned. The search results confirm thatShutdown()is called both with and without arguments across the codebase. Let me verify the actual fx.Shutdowner interface signature to determine if this is a real compilation error.Review comment is incorrect—no compile error exists.
The Shutdowner interface defines
Shutdown(...ShutdownOption) error, using variadic parameters that accept zero or more options. Both call patterns—Shutdown()andShutdown(fx.ExitCode(1))—are valid Go and compile successfully. Tests in the fx library confirm both patterns work:s.Shutdown()ands.Shutdown(fx.ExitCode(2)). The codebase correctly uses optional exit codes; this is not a signature mismatch.Likely an incorrect or invalid review comment.
internal/sms-gateway/pubsub/pubsub.go (1)
16-41: LGTM! Factory function is well-structured.The implementation correctly:
- Defaults to
memory://for empty URL- Validates URL parsing with proper error wrapping
- Selects the appropriate backend based on scheme
- Handles unknown schemes with clear error messages
internal/sms-gateway/modules/events/events.go (1)
9-29: LGTM! Consistent refactor to value-based Event constructors.The change from returning
*EventtoEventaligns with the updatedNewEventsignature and eliminates potential nil pointer issues. All four constructors follow the same pattern consistently.pkg/pubsub/memory.go (1)
10-156: LGTM! Memory Pub/Sub implementation is correct and well-synchronized.All previously identified concurrency issues have been addressed:
- Read lock is held throughout subscriber iteration (lines 52-53)
- Unsubscribe occurs before channel close (lines 110-111)
- Subscriber context is tracked and checked during publish (lines 19-22, 74, 100)
- Subscription struct includes the id field (line 116)
The implementation properly handles:
- Concurrent access with RWMutex protection
- Graceful shutdown via closeCh and WaitGroup
- Context cancellation at multiple levels
- Resource cleanup in the subscription goroutine
internal/sms-gateway/modules/events/types.go (1)
9-33: LGTM! Event serialization changes are correct.The refactor to exported fields with JSON tags enables proper serialization for Pub/Sub delivery:
Eventchanged from pointer to value type eliminates nil pointer risksNewEventreturns by value, consistent with the updated typeeventWrapperserialize/deserialize methods enable JSON round-tripping- Field names and JSON tags are appropriate for the internal events module
Note: The JSON field naming differs from push and SSE modules (
event_typevs.type/event), but this is intentional—service.go explicitly converts between these distinct types (lines 127-129, 140-142), so they are not meant to be directly interchangeable.
|
Deployed... |
Summary by CodeRabbit
New Features
Improvements
API
Chores