[worker] introduce worker mode #187
Conversation
WalkthroughAdds a flag-driven CLI to run either the gateway or a new worker process; extracts background tasks into a standalone worker app (executor, locker, tasks); promotes several repositories/types to exported, restructures config and DI wiring, removes the cleaner subsystem, and adds worker deployment and observability artifacts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as cmd/sms-gateway/main.go
participant Parser as flag/parser
participant Gateway as smsgateway.Run()
participant Worker as worker.Run()
CLI->>Parser: parse flags & args
Parser-->>CLI: command (default "start")
alt command == "worker"
CLI->>Worker: worker.Run()
Worker->>Worker: build Fx app (logger, config, db, worker modules)
else
CLI->>Gateway: smsgateway.Run()
Gateway->>Gateway: build Fx app (gateway modules)
end
sequenceDiagram
autonumber
participant Exec as executor.Service
participant Task as PeriodicTask
participant Locker as Locker (MySQL)
participant Repo as messages.Repository
Exec->>Exec: Start() spawn goroutine per task
loop per task
Exec->>Exec: initial jitter then ticker
Exec->>Locker: AcquireLock(ctx, key)
alt lock acquired
Locker-->>Exec: success
Exec->>Task: Task.Run(ctx)
Task->>Repo: HashProcessed / Cleanup(ctx)
Repo-->>Task: rows affected
Task-->>Exec: result
Exec->>Locker: ReleaseLock(ctx, key)
else lock not acquired
Locker-->>Exec: ErrLockNotAcquired
Exec->>Exec: log & skip run
end
end
Exec->>Exec: Stop() -> shutdown goroutines
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
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: 1
🧹 Nitpick comments (5)
cmd/sms-gateway/main.go (1)
43-52: Consider aligning worker command with the existing CLI module pattern.The current implementation manually dispatches based on positional arguments, while
smsgateway.Run()uses theclimodule internally. This creates inconsistency in command handling approaches.Consider one of these approaches:
- Register the worker command using
cli.Register("worker", ...)similar to how "start" is registered (line 127 in internal/sms-gateway/app.go).- Add validation for the
cmdvalue to prevent silent fallthrough for typos.Current approach works but mixing command dispatch patterns may complicate future CLI expansion.
internal/worker/app.go (1)
15-24: Consider creating a worker-specific messages module to avoid loading unnecessary dependencies.The worker includes
messages.Module()which provides the full messages service stack (repository, cache, metrics, HTTP handlers). For a worker that only runs periodic tasks, this may include components that aren't needed.Consider one of these approaches:
- Create a separate
messages.WorkerModule()that provides only task-related dependencies.- Split the messages module into core and HTTP-handler modules.
- Verify and document which components from messages.Module() are actually required by the worker tasks.
This helps keep the worker process lightweight and reduces resource usage.
internal/sms-gateway/modules/messages/tasks/initial_hashing.go (2)
22-24: Consider adjusting the interval for the stub implementation.A 1-second interval is very frequent for a task that currently only logs a message. This will spam logs and waste CPU cycles.
For the stub implementation, consider using a longer interval (e.g., 30 seconds or 1 minute) or making it configurable. When the actual hashing logic is implemented, adjust based on the real operational requirements.
32-36: Stub implementation: Add TODO comment or issue reference.The
Run()method is currently a placeholder that only logs a message. The PR objectives mention migrating initial hashing from startup to a periodic task, but the hashing logic isn't implemented yet.Since this is a draft PR, consider:
- Adding a TODO comment referencing what needs to be implemented (e.g.,
// TODO: Implement actual message hashing logic)- Creating a tracking issue for the implementation work
Would you like me to help identify what hashing logic should be migrated here based on the codebase?
internal/worker/tasks/service.go (1)
11-29: Consider adding lifecycle state management.The Service lacks state tracking to prevent calling
Start()orStop()multiple times, which can lead to issues (e.g., double-close panic onstopChanifStop()is called twice). Consider adding a state field with sync primitives to ensure lifecycle methods are idempotent.Example:
type Service struct { tasks []PeriodicTask stopChan chan struct{} wg sync.WaitGroup + mu sync.Mutex + started bool logger *zap.Logger }Then check/set state in
Start()andStop().
📜 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 (10)
cmd/sms-gateway/main.go(2 hunks)go.mod(5 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/modules/messages/module.go(2 hunks)internal/sms-gateway/modules/messages/tasks.go(0 hunks)internal/sms-gateway/modules/messages/tasks/initial_hashing.go(1 hunks)internal/worker/app.go(1 hunks)internal/worker/tasks/module.go(1 hunks)internal/worker/tasks/service.go(1 hunks)internal/worker/tasks/types.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/messages/tasks.go
🧰 Additional context used
🧬 Code graph analysis (7)
internal/worker/app.go (3)
internal/sms-gateway/app.go (2)
Run(58-65)Module(33-56)internal/sms-gateway/modules/messages/module.go (1)
Module(13-38)internal/worker/tasks/module.go (1)
Module(10-28)
internal/sms-gateway/app.go (1)
internal/worker/tasks/module.go (1)
Module(10-28)
internal/worker/tasks/module.go (2)
internal/sms-gateway/modules/messages/module.go (1)
Module(13-38)internal/worker/tasks/service.go (2)
NewService(20-29)Service(11-18)
internal/worker/tasks/service.go (2)
internal/worker/tasks/types.go (1)
PeriodicTask(8-12)internal/sms-gateway/app.go (2)
Start(80-124)Run(58-65)
internal/sms-gateway/modules/messages/module.go (2)
internal/worker/tasks/module.go (1)
AsWorkerTask(30-32)internal/sms-gateway/modules/messages/tasks/initial_hashing.go (1)
NewInitialHashing(15-19)
internal/sms-gateway/modules/messages/tasks/initial_hashing.go (2)
internal/worker/tasks/types.go (1)
PeriodicTask(8-12)internal/worker/app.go (1)
Run(15-24)
cmd/sms-gateway/main.go (2)
internal/worker/app.go (1)
Run(15-24)internal/sms-gateway/app.go (1)
Run(58-65)
⏰ 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: Lint
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
internal/sms-gateway/modules/messages/module.go (1)
29-29: LGTM!The worker task integration follows clean DI patterns using fx group tags. The
AsWorkerTaskhelper appropriately tags theNewInitialHashingprovider for collection by the worker service.internal/worker/tasks/types.go (1)
8-12: LGTM!The
PeriodicTaskinterface is well-designed with clear semantics. It provides context-aware execution, proper error propagation, and configurable intervals using only standard library types.internal/worker/app.go (1)
26-43: LGTM!The worker module correctly wires the tasks module and adds appropriate lifecycle hooks for startup/shutdown logging.
internal/sms-gateway/app.go (1)
35-35: LGTM!The migration to
github.com/go-core-fx/loggersimplifies the logger configuration by replacing custom fxevent.Logger setup with the standardlogger.WithFxDefaultLogger(). This aligns with the worker app's logger integration.Also applies to: 63-63
internal/worker/tasks/module.go (2)
10-28: LGTM!The module correctly uses fx group tags to collect worker tasks and wires lifecycle hooks with proper error propagation. The named logger helps with observability.
30-32: LGTM!The
AsWorkerTaskhelper provides a clean abstraction for tagging task providers, making it easy to register new worker tasks across modules.go.mod (1)
12-12: Acknowledge that tagged releases don't exist; verify this pre-release dependency is intentional.The repository has no releases or tags on GitHub, so the pseudo-version is the only way to use this library. Ensure using an untagged, development commit is acceptable for your use case given the reproducibility and supply chain implications.
internal/worker/tasks/service.go (2)
54-72: Core task execution logic looks good.The ticker-based loop correctly handles context cancellation and task execution. Proper cleanup with
defer ticker.Stop(). The panic recovery mentioned in the previous comment will also protect this method.
41-49: Review comment is valid; developer should address or defer both concerns based on requirements.The review comment is accurate:
Panic recovery: Confirmed—if
task.Run(ctx)panics on line 63, the goroutine crashes silently. Whiledefer s.wg.Done()ensures the WaitGroup is marked, adding panic recovery is a reasonable defensive measure to log failures and prevent silent task termination.No immediate execution: Confirmed—
runTaskcreates a ticker first, then waits for the firstticker.Cfire. Tasks execute only after their interval elapses, not immediately on startup.Both concerns are design choices rather than bugs. The review appropriately asks for clarification on whether immediate execution is desired. The developer should decide whether:
- Panic recovery is needed (depends on whether tasks are trusted and if failures should be logged).
- Immediate execution is required (depends on the use case and interval values).
There was a problem hiding this comment.
Actionable comments posted: 5
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/messages/repository.go (1)
139-143: Fix unsafe MySQLError type assertion (can panic).err.(*mysql.MySQLError) will panic for non‑MySQL errors. Use errors.As instead.
Apply:
- if mysqlErr := err.(*mysql.MySQLError); mysqlErr != nil && mysqlErr.Number == 1062 { - return ErrMessageAlreadyExists - } + var mysqlErr *mysql.MySQLError + if errors.As(err, &mysqlErr) && mysqlErr.Number == 1062 { + return ErrMessageAlreadyExists + } return err
♻️ Duplicate comments (2)
internal/worker/executor/service.go (2)
14-22: Add lifecycle state and once semantics to avoid races and panics.Service lacks started/once state. This can double‑start tasks and panic on double Stop() due to closing a closed channel.
Apply:
type Service struct { tasks []PeriodicTask locker locker.Locker stopChan chan struct{} wg sync.WaitGroup logger *zap.Logger + + mu sync.Mutex + started bool + stopOnce sync.Once }
105-109: Protect Stop() against double close; make idempotent.Close stopChan exactly once and mark not started.
Apply:
func (s *Service) Stop() error { - close(s.stopChan) - s.wg.Wait() + s.mu.Lock() + if !s.started { + s.mu.Unlock() + return nil + } + s.started = false + s.mu.Unlock() + + s.stopOnce.Do(func() { + close(s.stopChan) + }) + s.wg.Wait() return nil }
🧹 Nitpick comments (8)
configs/config.example.yml (1)
34-36: Clarify the purpose ofmessages.hashing_interval_secondsvs worker task intervals.The configuration now has two hashing-related intervals:
messages.hashing_interval_seconds: 60(Line 36) - appears to be for gateway modetasks.messages_hashing.interval_hours: 168(Line 44) - for worker modeThese serve different purposes with vastly different default values (1 minute vs 1 week), but the distinction may not be immediately clear to users.
Consider adding comments to clarify:
- Which mode uses which configuration
- The purpose of each interval (e.g., "real-time hashing for gateway responses" vs "batch hashing for worker tasks")
+# Message cache and hashing configuration for gateway mode messages: cache_ttl_seconds: 300 # message cache TTL in seconds [MESSAGES__CACHE_TTL_SECONDS] + # Real-time hashing interval for gateway message processing hashing_interval_seconds: 60 # message hashing interval in seconds [MESSAGES__HASHING_INTERVAL_SECONDS]internal/worker/executor/module.go (1)
10-28: Remove unusedShutdownerparameter.The
fx.Shutdownerparametershon Line 17 is declared but never used in the lifecycle hook.- fx.Invoke(func(svc *Service, lc fx.Lifecycle, sh fx.Shutdowner) { + fx.Invoke(func(svc *Service, lc fx.Lifecycle) { lc.Append(fx.Hook{ OnStart: func(_ context.Context) error { return svc.Start() }, OnStop: func(_ context.Context) error { return svc.Stop() }, }) }),internal/config/module.go (1)
99-103: Consider documenting the config migration strategy.The use of
max()to select betweenTasks.Hashing.IntervalSeconds(deprecated) andMessages.HashingIntervalSecondshandles backward compatibility, but this behavior may not be obvious to users who have both configured. Consider adding a comment or logging a warning when both values are set to different values.Consider adding a comment to clarify the precedence:
fx.Provide(func(cfg Config) messages.Config { return messages.Config{ CacheTTL: time.Duration(cfg.Messages.CacheTTLSeconds) * time.Second, + // Use max of deprecated Tasks.Hashing.IntervalSeconds and new Messages.HashingIntervalSeconds for backward compatibility HashingInterval: time.Duration(max(cfg.Tasks.Hashing.IntervalSeconds, cfg.Messages.HashingIntervalSeconds)) * time.Second, } }),internal/worker/locker/mysql.go (1)
47-53: Consider checking RELEASE_LOCK return value.The current implementation doesn't verify that the lock was actually released. MySQL's
RELEASE_LOCKreturns 1 (released), 0 (not held by this thread), or NULL (doesn't exist). While the current idempotent behavior might be acceptable, checking the return value could help detect lock management issues.If you want stricter verification, consider checking the return value:
func (m *mySQLLocker) ReleaseLock(ctx context.Context, key string) error { name := m.prefix + key - _, err := m.db.ExecContext(ctx, "SELECT RELEASE_LOCK(?)", name) - return err + var result sql.NullInt64 + err := m.db.QueryRowContext(ctx, "SELECT RELEASE_LOCK(?)", name).Scan(&result) + if err != nil { + return fmt.Errorf("can't release lock: %w", err) + } + if !result.Valid || result.Int64 != 1 { + return fmt.Errorf("lock was not held or doesn't exist") + } + return nil }internal/worker/config/config.go (1)
14-21: Prefer duration types over raw hour counters.Using uint16 “hours” invites unit bugs and limits range; representing intervals as time.Duration (with YAML/env parsing as duration strings like “24h”, “7d”) is more idiomatic and flexible.
internal/config/config.go (1)
62-68: Marking hashing task deprecated is good; ensure it’s inert.If the worker owns hashing, keep this config unused and schedule removed from gateway. Consider adding a deprecation notice in docs.
internal/sms-gateway/modules/messages/repository.go (2)
170-187: Hashing SQL: ensure robust IN expansion and clarity.Current GORM Exec with ids slice is fine; optional: add explicit IN clause handling via gorm.Expr for clarity, and consider a small comment on MySQL 8 JSON_VALUE requirement.
30-101: Repository methods lack context; consider consistency.Select/Get/Insert/UpdateState don’t accept context while HashProcessed/Cleanup do. Unify to context‑aware APIs to enable request cancelation and deadlines.
Happy to draft the signature changes and call‑site adjustments.
Also applies to: 103-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
configs/config.example.yml(2 hunks)internal/config/config.go(2 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/modules/messages/config.go(1 hunks)internal/sms-gateway/modules/messages/module.go(1 hunks)internal/sms-gateway/modules/messages/repository.go(6 hunks)internal/sms-gateway/modules/messages/service.go(4 hunks)internal/sms-gateway/modules/messages/tasks.go(0 hunks)internal/sms-gateway/modules/messages/workers.go(1 hunks)internal/worker/app.go(1 hunks)internal/worker/config/config.go(1 hunks)internal/worker/config/module.go(1 hunks)internal/worker/executor/module.go(1 hunks)internal/worker/executor/service.go(1 hunks)internal/worker/executor/types.go(1 hunks)internal/worker/locker/locker.go(1 hunks)internal/worker/locker/module.go(1 hunks)internal/worker/locker/mysql.go(1 hunks)internal/worker/tasks/messages/cleanup.go(1 hunks)internal/worker/tasks/messages/config.go(1 hunks)internal/worker/tasks/messages/hashing.go(1 hunks)internal/worker/tasks/messages/module.go(1 hunks)internal/worker/tasks/module.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/messages/tasks.go
✅ Files skipped from review due to trivial changes (1)
- internal/sms-gateway/modules/messages/service.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/modules/messages/module.go
🧰 Additional context used
🧬 Code graph analysis (15)
internal/worker/tasks/module.go (3)
internal/worker/config/module.go (1)
Module(13-54)internal/worker/executor/module.go (1)
Module(10-28)internal/worker/tasks/messages/module.go (1)
Module(10-23)
internal/worker/tasks/messages/module.go (6)
internal/worker/config/module.go (1)
Module(13-54)internal/worker/executor/module.go (2)
Module(10-28)AsWorkerTask(30-32)internal/worker/tasks/module.go (1)
Module(9-15)internal/worker/tasks/messages/config.go (3)
Config(5-8)HashingConfig(10-12)CleanupConfig(14-17)internal/worker/tasks/messages/hashing.go (1)
NewInitialHashingTask(20-31)internal/worker/tasks/messages/cleanup.go (1)
NewCleanupTask(20-31)
internal/worker/executor/module.go (3)
internal/worker/locker/module.go (1)
Module(10-20)internal/worker/tasks/messages/module.go (1)
Module(10-23)internal/worker/executor/service.go (2)
NewService(24-34)Service(14-22)
internal/worker/executor/types.go (1)
internal/worker/app.go (1)
Run(16-24)
internal/worker/locker/mysql.go (1)
internal/worker/locker/locker.go (2)
Locker(10-13)ErrLockNotAcquired(8-8)
internal/worker/tasks/messages/cleanup.go (3)
internal/worker/tasks/messages/config.go (1)
CleanupConfig(14-17)internal/worker/executor/types.go (1)
PeriodicTask(8-12)internal/worker/app.go (1)
Run(16-24)
internal/sms-gateway/modules/messages/workers.go (2)
internal/sms-gateway/modules/messages/repository.go (1)
Repository(20-22)internal/sms-gateway/modules/messages/config.go (1)
Config(5-8)
internal/worker/locker/module.go (2)
internal/worker/locker/locker.go (1)
Locker(10-13)internal/worker/locker/mysql.go (1)
NewMySQLLocker(16-23)
internal/worker/config/config.go (2)
internal/config/config.go (1)
Database(43-54)internal/worker/tasks/messages/config.go (1)
Config(5-8)
internal/config/module.go (1)
internal/config/config.go (2)
Messages(74-77)Tasks(62-64)
internal/sms-gateway/modules/messages/repository.go (2)
internal/sms-gateway/modules/messages/repository_filter.go (3)
MessagesSelectFilter(16-23)MessagesSelectOptions(25-36)MessagesOrder(7-7)internal/sms-gateway/modules/messages/models.go (1)
Message(35-55)
internal/worker/config/module.go (4)
internal/worker/tasks/messages/module.go (1)
Module(10-23)internal/worker/config/config.go (5)
Config(5-8)Default(23-46)Tasks(10-13)MessagesHashing(14-16)MessagesCleanup(18-21)internal/worker/tasks/messages/config.go (3)
Config(5-8)HashingConfig(10-12)CleanupConfig(14-17)internal/config/config.go (1)
Database(43-54)
internal/worker/app.go (6)
internal/sms-gateway/app.go (2)
Run(58-65)Module(33-56)internal/worker/config/module.go (1)
Module(13-54)internal/worker/executor/module.go (1)
Module(10-28)internal/worker/locker/module.go (1)
Module(10-20)internal/worker/tasks/messages/module.go (1)
Module(10-23)internal/worker/tasks/module.go (1)
Module(9-15)
internal/worker/executor/service.go (2)
internal/worker/executor/types.go (1)
PeriodicTask(8-12)internal/worker/locker/locker.go (1)
Locker(10-13)
internal/worker/tasks/messages/hashing.go (2)
internal/worker/tasks/messages/config.go (1)
HashingConfig(10-12)internal/worker/executor/types.go (1)
PeriodicTask(8-12)
⏰ 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 (24)
configs/config.example.yml (1)
1-13: LGTM! Clean separation of common configuration.The database configuration has been properly extracted as a common section that can be shared between gateway and worker modes, avoiding duplication.
internal/worker/app.go (2)
16-24: LGTM! Clean worker initialization.The
Run()function properly initializes the worker with all necessary modules in the correct order: logger, config, database, and worker-specific components.
32-43: Lifecycle logging looks good.The lifecycle hooks provide clear visibility into worker startup and shutdown. The simple logging approach is appropriate for this use case.
internal/sms-gateway/modules/messages/config.go (1)
5-8: LGTM! Field rename improves clarity.Renaming
ProcessedLifetimetoHashingIntervalmakes the configuration more self-documenting and aligns with the new worker mode terminology.internal/worker/tasks/module.go (1)
9-15: LGTM! Clean module composition.The tasks module provides a clean aggregation point for worker tasks with proper logger namespacing. The structure allows easy addition of new task modules in the future.
internal/worker/tasks/messages/module.go (1)
10-23: LGTM! Proper task registration and dependency wiring.The module correctly:
- Wires the shared messages repository for worker tasks
- Registers hashing and cleanup tasks via
AsWorkerTask- Provides private config extraction for task-specific configuration
The cross-module dependency on
internal/sms-gateway/modules/messagesis appropriate for sharing the repository implementation between gateway and worker modes.internal/worker/executor/types.go (1)
8-12: LGTM! Well-designed task interface.The
PeriodicTaskinterface provides a clean contract for worker tasks with:
- Task identification via
Name()- Flexible scheduling via
Interval()- Context-aware execution via
Run(ctx context.Context) errorThis design enables proper task lifecycle management and graceful shutdown.
internal/worker/executor/module.go (1)
30-32: LGTM! Clean task registration helper.The
AsWorkerTaskhelper provides a convenient way to annotate task providers for group-based dependency injection, making task registration concise and consistent.internal/worker/tasks/messages/hashing.go (3)
1-31: LGTM!The task structure and constructor follow clean dependency injection patterns, and returning the interface type provides good abstraction.
57-57: Good practice: compile-time interface check.
44-48: No issues found. The nil filter behavior is intentional and correct.The
HashProcessed(ctx, nil)call hashes all unhashed messages without ID filtering. The method signature acceptsids []uint64, and whennil(or an empty slice) is passed, the conditional at line 175 (if len(ids) > 0) is false, so no ID filter is added to the WHERE clause. This allows the base query—which already filters foris_hashed = false—to process all eligible unhashed messages. This is the intended behavior for an initial hashing task during bootstrap.internal/worker/tasks/messages/cleanup.go (2)
1-41: LGTM!The cleanup task follows the same clean patterns as the hashing task, with proper dependency injection and interface implementation.
43-57: LGTM!The time calculation correctly computes the cutoff for cleanup, and error handling follows best practices with proper wrapping.
internal/worker/config/module.go (1)
13-54: LGTM!The worker config module is well-structured with proper error handling and clean dependency wiring. The hour-to-duration conversions are correct, and the empty DSN field is fine since other connection parameters are populated.
internal/worker/locker/locker.go (1)
1-13: LGTM!The Locker abstraction is clean and minimal, with a well-defined sentinel error for lock acquisition failures. This provides a good foundation for distributed task coordination.
internal/worker/tasks/messages/config.go (1)
1-17: LGTM!The configuration structs are clean and well-organized, with idiomatic use of
time.Durationfor time-based settings.internal/sms-gateway/modules/messages/workers.go (3)
1-31: LGTM!The worker structure is well-designed for concurrent access, with proper initialization and the idiomatic use of
map[uint64]struct{}for set semantics.
33-54: LGTM!The ticker pattern and mutex usage are correct. The
Enqueuemethod properly synchronizes access to the shared queue map.
56-72: LGTM! Good concurrent pattern.The implementation correctly minimizes lock holding time by copying and clearing the queue under lock, then processing without the lock. The use of
golang.org/x/exp/mapsforKeys()andClear()is acceptable, though note these are still experimental. Error logging without propagation is appropriate for a background worker.internal/worker/locker/mysql.go (2)
1-23: LGTM!The locker structure is clean, and the prefix parameter provides good namespacing for lock keys.
25-45: LGTM!The
AcquireLockimplementation correctly handles all return values from MySQL'sGET_LOCK:
- 1 (success) → lock acquired
- 0 (timeout) → returns
ErrLockNotAcquired- NULL (error) → Scan fails, error is wrapped and returned
internal/config/module.go (1)
43-43: The hardcoding of MySQL dialect is not a breaking change.Verification shows the codebase contains no references to other database dialects (PostgreSQL, SQLite, MariaDB) anywhere. The system was designed to support MySQL only, so hardcoding
db.DialectMySQLdoes not break existing deployments or remove previously supported database backends.Likely an incorrect or invalid review comment.
internal/worker/config/config.go (1)
23-46: Defaults look sane for now.Seven‑day hashing, daily cleanup, and 30‑day max age are reasonable starting points; DB defaults mirror global config.
internal/worker/executor/service.go (1)
3-12: Go version requirement for math/rand/v2 is satisfied.The main module specifies
go 1.24.3, which exceeds the minimum Go 1.22 requirement for math/rand/v2. The test module usesgo 1.23.0withtoolchain go1.23.2, which also meets the requirement. The import is correctly present ininternal/worker/executor/service.go. No action required—the toolchain versions are compatible with math/rand/v2 APIs.
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)
internal/sms-gateway/modules/messages/repository.go (1)
161-165: Bug confirmed: WHERE clause updates all recipients each iteration.The
Where("message_id = ?", message.ID)matches every recipient of that message, not just the current one. Each loop iteration overwrites all recipients with the currentvvalues. MessageRecipient has a primary keyIDthat is populated when recipients are loaded, so use it in the WHERE clause:for _, v := range message.Recipients { - if err := tx.Model(&v).Where("message_id = ?", message.ID).Select("State", "Error").Updates(&v).Error; err != nil { + if err := tx.Model(&MessageRecipient{}). + Where("id = ? AND message_id = ?", v.ID, message.ID). + Select("State", "Error"). + Updates(map[string]any{"State": v.State, "Error": v.Error}).Error; err != nil { return err } }
🧹 Nitpick comments (5)
internal/worker/config/types.go (1)
22-37: Consider simplifying by removing UnmarshalYAML.The
yaml.v3library automatically usesUnmarshalTextfor types implementingencoding.TextUnmarshaler, making the explicitUnmarshalYAMLimplementation redundant. However, keeping it provides explicit control and clarity about YAML unmarshalling behavior.If you prefer to simplify, you can remove the
UnmarshalYAMLmethod and theyaml.Unmarshalerinterface check:-func (d *Duration) UnmarshalYAML(value *yaml.Node) error { - var s string - if err := value.Decode(&s); err != nil { - return fmt.Errorf("can't unmarshal duration: %w", err) - } - - t, err := time.ParseDuration(s) - if err != nil { - return fmt.Errorf("can't parse duration: %w", err) - } - *d = Duration(t) - return nil -} - -var _ yaml.Unmarshaler = (*Duration)(nil) var _ encoding.TextUnmarshaler = (*Duration)(nil)configs/config.example.yml (1)
42-47: Consider clarifying time unit comments.The comments state "interval in hours" and "max age in hours", but the Duration type accepts any Go duration format (e.g., "168h", "7d" if days were supported, "10080m", etc.). While the example values are in hours, the comments might mislead users into thinking only hours are accepted.
Consider updating the comments to reflect the flexibility:
tasks: # tasks config messages_hashing: - interval: 168h # task execution interval in hours [TASKS__MESSAGES_HASHING__INTERVAL] + interval: 168h # task execution interval (Go duration format, e.g., 168h, 7d) [TASKS__MESSAGES_HASHING__INTERVAL] messages_cleanup: - interval: 24h # task execution interval in hours [TASKS__MESSAGES_CLEANUP__INTERVAL] - max_age: 720h # messages max age in hours [TASKS__MESSAGES_CLEANUP__MAX_AGE] + interval: 24h # task execution interval (Go duration format, e.g., 24h, 1d) [TASKS__MESSAGES_CLEANUP__INTERVAL] + max_age: 720h # messages max age (Go duration format, e.g., 720h, 30d) [TASKS__MESSAGES_CLEANUP__MAX_AGE]Note: Go's
time.ParseDurationdoesn't support "d" for days by default, so if you want to support that, you'd need to add custom parsing logic.pkg/mysql/errors.go (1)
5-10: Use errors.As to unwrap; consider clarifying the name.
- Robustly detect duplicated key by unwrapping with errors.As; direct type assertion misses wrapped errors.
- 1062 covers duplicate primary OR unique key. The name “IsPrimaryKeyViolation” is misleading; consider “IsDuplicateKey” or add a doc comment clarifying scope.
Apply:
-import "github.com/go-sql-driver/mysql" +import ( + "errors" + "github.com/go-sql-driver/mysql" +) -func IsPrimaryKeyViolation(err error) bool { - if mysqlErr, ok := err.(*mysql.MySQLError); ok { - return mysqlErr.Number == 1062 - } - return false -} +func IsPrimaryKeyViolation(err error) bool { + var me *mysql.MySQLError + if errors.As(err, &me) { + return me.Number == 1062 + } + return false +}internal/sms-gateway/modules/messages/repository.go (2)
139-144: Also check gorm.ErrDuplicatedKey for portability.Covers drivers that normalize duplicate errors to gorm.ErrDuplicatedKey.
- if mysql.IsPrimaryKeyViolation(err) { + if errors.Is(err, gorm.ErrDuplicatedKey) || mysql.IsPrimaryKeyViolation(err) { return ErrMessageAlreadyExists }
88-90: Prefer Preload for retrieving Device when WithDevice is true.Joins("Device") won’t populate the nested Device struct reliably; Preload loads the association.
- if filter.UserID == "" && options.WithDevice { - query = query.Joins("Device") - } + if options.WithDevice { + query = query.Preload("Device") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
configs/config.example.yml(2 hunks)internal/sms-gateway/modules/messages/repository.go(5 hunks)internal/worker/config/config.go(1 hunks)internal/worker/config/module.go(1 hunks)internal/worker/config/types.go(1 hunks)internal/worker/executor/module.go(1 hunks)internal/worker/locker/mysql.go(1 hunks)pkg/mysql/errors.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/worker/executor/module.go
- internal/worker/config/config.go
- internal/worker/locker/mysql.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/sms-gateway/modules/messages/repository.go (3)
internal/sms-gateway/modules/messages/repository_filter.go (3)
MessagesSelectFilter(16-23)MessagesSelectOptions(25-36)MessagesOrder(7-7)internal/sms-gateway/modules/messages/models.go (1)
Message(35-55)pkg/mysql/errors.go (1)
IsPrimaryKeyViolation(5-10)
internal/worker/config/module.go (4)
internal/worker/config/config.go (5)
Config(9-12)Default(27-50)Tasks(14-17)MessagesHashing(18-20)MessagesCleanup(22-25)internal/worker/tasks/messages/config.go (3)
Config(5-8)HashingConfig(10-12)CleanupConfig(14-17)internal/config/config.go (1)
Database(43-54)internal/worker/config/types.go (1)
Duration(11-11)
⏰ 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 (6)
internal/worker/config/types.go (1)
11-20: LGTM! Clean implementation of custom Duration unmarshalling.The Duration type wrapper and UnmarshalText implementation correctly handle text-based duration parsing with appropriate error handling.
internal/worker/config/module.go (3)
13-24: LGTM! Proper config loading with error handling.The Config provider correctly loads defaults, applies external configuration sources, and wraps errors appropriately. The
fx.Privateannotation ensures the Config is encapsulated within this module.
42-52: LGTM! Clean conversion from worker config to messages config.The type conversions from custom Duration to time.Duration are correct, and all configuration fields are properly mapped.
25-41: Verify DSN construction and connection timeout defaults with the external db package.The
dbpackage is an external dependency (github.com/capcom6/go-infra-fx/db), so I cannot verify its behavior from the codebase. Two concerns from the original review require manual verification:
Line 28 (DSN hardcoded to empty string): Confirm that the
dbpackage constructs the DSN from individual fields (Host, Port, User, etc.) when DSN is empty, rather than failing or using a fallback.Lines 36-37 (connection timeouts hardcoded to 0): Verify whether zero values trigger defaults in the
dbpackage or if these should be made configurable for production deployments to prevent connection leaks.Check the external package documentation or source to ensure these defaults align with your requirements.
configs/config.example.yml (1)
1-13: LGTM! Well-documented database configuration.The Common Config section provides clear database settings with environment variable mappings and helpful inline comments.
internal/sms-gateway/modules/messages/repository.go (1)
95-95: The code is correct; built-inmin()is available.The repository targets Go 1.24.3, which is well above Go 1.21—the version where
min()andmax()became built-in language functions. No local helper orcmp.Minis needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
configs/config.example.yml (1)
36-36: Typo already flagged in previous review.The typo "reasl-time" should be "real-time" as noted in the previous review comment.
internal/sms-gateway/modules/messages/repository.go (1)
174-191: Critical SQL issue flagged in previous review remains unfixed.The raw SQL UPDATE statement uses unqualified column names that will cause runtime errors. The
phone_numbercolumn exists only inmessage_recipients, notmessages, so the current query will fail when executed.As noted in the previous review, apply the qualification fixes:
rawSQL := "UPDATE `messages` `m`, `message_recipients` `r`\n" + - "SET `m`.`is_hashed` = true, `m`.`content` = SHA2(COALESCE(JSON_VALUE(`content`, '$.text'), JSON_VALUE(`content`, '$.data')), 256), `r`.`phone_number` = LEFT(SHA2(phone_number, 256), 16)\n" + + "SET `m`.`is_hashed` = true, `m`.`content` = SHA2(COALESCE(JSON_VALUE(`m`.`content`, '$.text'), JSON_VALUE(`m`.`content`, '$.data')), 256), `r`.`phone_number` = LEFT(SHA2(`r`.`phone_number`, 256), 16)\n" + "WHERE `m`.`id` = `r`.`message_id` AND `m`.`is_hashed` = false AND `m`.`is_encrypted` = false AND `m`.`state` <> 'Pending'"
🧹 Nitpick comments (1)
pkg/mysql/errors.go (1)
9-15: Function name may be misleading.MySQL error code 1062 (ER_DUP_ENTRY) applies to any duplicate key violation, including unique indexes, not just primary keys. Consider renaming to
IsDuplicateKeyViolationfor accuracy.Apply this diff if you want to improve the naming:
-func IsPrimaryKeyViolation(err error) bool { +func IsDuplicateKeyViolation(err error) bool { var me *mysql.MySQLError if errors.As(err, &me) { return me.Number == 1062 } return false }Note: This would require updating the call site in
internal/sms-gateway/modules/messages/repository.go.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
configs/config.example.yml(2 hunks)go.mod(6 hunks)internal/sms-gateway/modules/messages/repository.go(6 hunks)pkg/mysql/errors.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/modules/messages/repository.go (3)
internal/sms-gateway/modules/messages/repository_filter.go (3)
MessagesSelectFilter(16-23)MessagesSelectOptions(25-36)MessagesOrder(7-7)internal/sms-gateway/modules/messages/models.go (2)
Message(35-55)MessageRecipient(111-117)pkg/mysql/errors.go (1)
IsPrimaryKeyViolation(9-15)
⏰ 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 (9)
go.mod (5)
12-12: Verify pseudo-version pin forgo-core-fx/logger.The logger dependency uses a pseudo-version (dated 2025-10-28) rather than a released tag, which pins directly to the main branch. This could introduce instability if the upstream main branch receives breaking changes.
Consider whether this should reference a tagged release instead for reproducibility and stability.
The search didn't find the exactgo-core-fx/loggerpackage. Let me search more specifically:The package doesn't appear in standard registries. Given the PR is in draft and from capcom6, this might be an internal or early-stage package. Let me finalize the review with available information.
29-29: LGTM: YAML v3 addition aligns with configuration needs.The addition of
gopkg.in/yaml.v3as a direct dependency is appropriate for the worker mode's configuration requirements mentioned in the PR objectives.
70-70: LGTM: Clockwork dependency supports time-based testing and scheduling.The indirect dependency on
github.com/jonboulle/clockworkis a good addition for worker task testing and time-based fixtures, aligning with the periodic task and executor patterns mentioned in the PR objectives.
3-3: LGTM: Version bumps are reasonable.The Go toolchain bump to 1.24.3 (patch version) and
golang.org/x/sysupdate to v0.36.0 (minor version) are standard maintenance updates.Also applies to: 101-101
88-88: Review comment is incorrect—testify is properly classified as indirect.The test files use
github.com/go-playground/assert/v2, notgithub.com/stretchr/testify. Theassert.Equal()pattern matching produced a false positive since both libraries provide similar assertion APIs. Testify has no direct imports in the codebase and is correctly marked as an indirect dependency in go.mod.Likely an incorrect or invalid review comment.
internal/sms-gateway/modules/messages/repository.go (4)
20-28: LGTM!The Repository type is correctly exported with a standard constructor pattern, aligning well with the new public API design.
133-144: LGTM!The error handling correctly detects duplicate key violations using both GORM's error type and the MySQL-specific check, providing robust detection across different error patterns.
146-172: LGTM!The UpdateState transaction logic is correct, and the recipient update now uses precise field selection with proper filtering by message_id and phone_number.
193-200: LGTM!The Cleanup method is well-implemented with proper context awareness and appropriate filtering logic (preserving pending messages while cleaning up older processed ones).
capcom6
force-pushed
the
worker/initial-implementation
branch
from
be209fa to
a73cc5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
internal/config/config.go (1)
112-115: Verify hashing interval defaults between gateway and worker.The gateway defaults to a 60-second hashing interval while the worker (per
config.example.yml) uses a 168-hour (7-day) interval. If both run concurrently with defaults, this could result in redundant or conflicting hashing work. Consider:
- Disabling gateway hashing by default (
HashingIntervalSeconds: 0) if the worker is the intended owner of hashing tasks.- Or documenting the expected behavior when both are active.
internal/worker/executor/service.go (5)
65-66: Higher‑precision jitter without float math.Use rand/v2 Int64N over seconds‑truncation.
- initialDelay := time.Duration(math.Floor(rand.Float64()*task.Interval().Seconds())) * time.Second + iv := task.Interval() + initialDelay := time.Duration(rand.Int64N(iv.Nanoseconds()))
14-22: Add lifecycle state to prevent duplicate starts and enable safe stop.type Service struct { tasks []PeriodicTask locker locker.Locker stopChan chan struct{} wg sync.WaitGroup logger *zap.Logger + mu sync.Mutex + started bool + stopOnce sync.Once }
36-45: Guard Start against multiple calls.func (s *Service) Start() error { - ctx, cancel := context.WithCancel(context.Background()) + s.mu.Lock() + if s.started { + s.mu.Unlock() + return nil + } + s.started = true + s.mu.Unlock() + ctx, cancel := context.WithCancel(context.Background())
85-100: Lock contention is expected; defer release and recover panics.- if err := s.locker.AcquireLock(ctx, task.Name()); err != nil { - s.logger.Error("can't acquire lock", zap.String("name", task.Name()), zap.Error(err)) - continue - } + if err := s.locker.AcquireLock(ctx, task.Name()); err != nil { + if errors.Is(err, locker.ErrLockNotAcquired) || errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { + s.logger.Debug("lock not acquired", zap.String("name", task.Name()), zap.Error(err)) + } else { + s.logger.Error("can't acquire lock", zap.String("name", task.Name()), zap.Error(err)) + } + continue + } start := time.Now() s.logger.Info("running task", zap.String("name", task.Name())) - if err := task.Run(ctx); err != nil { + defer func(taskName string, started time.Time) { + if r := recover(); r != nil { + s.logger.Error("task panicked", zap.String("name", taskName), zap.Any("panic", r), zap.Duration("duration", time.Since(started))) + } + if err := s.locker.ReleaseLock(ctx, taskName); err != nil { + s.logger.Error("can't release lock", zap.String("name", taskName), zap.Error(err)) + } + }(task.Name(), start) + if err := task.Run(ctx); err != nil { s.logger.Error("task failed", zap.String("name", task.Name()), zap.Duration("duration", time.Since(start)), zap.Error(err)) } else { s.logger.Info("task succeeded", zap.String("name", task.Name()), zap.Duration("duration", time.Since(start))) } - - if err := s.locker.ReleaseLock(ctx, task.Name()); err != nil { - s.logger.Error("can't release lock", zap.String("name", task.Name()), zap.Error(err)) - } + // release handled in defer
105-110: Protect Stop from double‑close panic.func (s *Service) Stop() error { - close(s.stopChan) + s.mu.Lock() + if !s.started { + s.mu.Unlock() + return nil + } + s.started = false + s.mu.Unlock() + s.stopOnce.Do(func() { close(s.stopChan) }) s.wg.Wait() return nil }internal/sms-gateway/modules/messages/repository.go (1)
174-187: Qualify columns in UPDATE…JOIN to avoid runtime SQL errors.func (r *Repository) HashProcessed(ctx context.Context, ids []uint64) (int64, error) { rawSQL := "UPDATE `messages` `m`, `message_recipients` `r`\n" + - "SET `m`.`is_hashed` = true, `m`.`content` = SHA2(COALESCE(JSON_VALUE(`content`, '$.text'), JSON_VALUE(`content`, '$.data')), 256), `r`.`phone_number` = LEFT(SHA2(phone_number, 256), 16)\n" + + "SET `m`.`is_hashed` = true, `m`.`content` = SHA2(COALESCE(JSON_VALUE(`m`.`content`, '$.text'), JSON_VALUE(`m`.`content`, '$.data')), 256), `r`.`phone_number` = LEFT(SHA2(`r`.`phone_number`, 256), 16)\n" + "WHERE `m`.`id` = `r`.`message_id` AND `m`.`is_hashed` = false AND `m`.`is_encrypted` = false AND `m`.`state` <> 'Pending'"Optional: add a fallback to avoid NULL writes if both JSON paths are absent:
... SHA2(COALESCE(JSON_VALUE(`m`.`content`,'$.text'), JSON_VALUE(`m`.`content`,'$.data'), `m`.`content`), 256) ...
🧹 Nitpick comments (7)
internal/worker/config/module.go (1)
36-37: Consider setting connection timeouts for better resource management.Setting
ConnMaxIdleTimeandConnMaxLifetimeto 0 (unlimited) may not be optimal for production workloads. Consider setting reasonable values to ensure connection recycling and prevent stale connections.Example:
- ConnMaxIdleTime: 0, - ConnMaxLifetime: 0, + ConnMaxIdleTime: 5 * time.Minute, + ConnMaxLifetime: 1 * time.Hour,internal/sms-gateway/modules/messages/workers.go (3)
8-10: Prefer stdlib maps package.Use the standard library’s maps (Go 1.21+) instead of x/exp.
-import ( +import ( "context" "sync" "time" "go.uber.org/zap" - "golang.org/x/exp/maps" + "maps" )
39-47: Flush pending queue on shutdown.Avoid losing enqueued IDs when ctx cancels; process once before exit.
case <-ctx.Done(): t.logger.Info("Stopping hashing task...") + // best-effort final flush + t.process(context.Background()) return
68-71: Consider retry/backoff on hashing failures.Errors drop the whole batch; optionally re-enqueue or add simple backoff to avoid permanent loss under transient DB errors.
internal/worker/executor/service.go (1)
47-49: Skip tasks with non‑positive intervals.Treat <=0 as disabled to avoid panics and weird tickers.
- if task.Interval() == 0 { + if task.Interval() <= 0 { s.logger.Info("skipping task", zap.String("name", task.Name()), zap.Duration("interval", task.Interval())) continue }internal/sms-gateway/modules/messages/service.go (2)
19-20: Prefer stdlib maps package.Replace x/exp package with the standard maps (Go 1.21+).
- "golang.org/x/exp/maps" + "maps"
48-61: Minor naming mismatch.Param is named hashingTask but type is *hashingWorker. Rename for clarity.
- hashingTask *hashingWorker, + hashingWorker *hashingWorker, ... - hashingWorker: hashingTask, + hashingWorker: hashingWorker,
📜 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)
cmd/sms-gateway/main.go(2 hunks)configs/config.example.yml(2 hunks)go.mod(5 hunks)internal/config/config.go(2 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/modules/messages/config.go(1 hunks)internal/sms-gateway/modules/messages/module.go(1 hunks)internal/sms-gateway/modules/messages/repository.go(6 hunks)internal/sms-gateway/modules/messages/service.go(4 hunks)internal/sms-gateway/modules/messages/tasks.go(0 hunks)internal/sms-gateway/modules/messages/workers.go(1 hunks)internal/worker/app.go(1 hunks)internal/worker/config/config.go(1 hunks)internal/worker/config/module.go(1 hunks)internal/worker/config/types.go(1 hunks)internal/worker/executor/module.go(1 hunks)internal/worker/executor/service.go(1 hunks)internal/worker/executor/types.go(1 hunks)internal/worker/locker/locker.go(1 hunks)internal/worker/locker/module.go(1 hunks)internal/worker/locker/mysql.go(1 hunks)internal/worker/tasks/messages/cleanup.go(1 hunks)internal/worker/tasks/messages/config.go(1 hunks)internal/worker/tasks/messages/hashing.go(1 hunks)internal/worker/tasks/messages/module.go(1 hunks)internal/worker/tasks/module.go(1 hunks)pkg/mysql/errors.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/messages/tasks.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/sms-gateway/main.go
- internal/worker/locker/module.go
- internal/worker/tasks/messages/module.go
- internal/config/module.go
- go.mod
- internal/worker/config/types.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T11:12:35.670Z
Learnt from: capcom6
PR: android-sms-gateway/server#187
File: internal/worker/executor/service.go:64-75
Timestamp: 2025-10-28T11:12:35.670Z
Learning: As of Go 1.23, unreferenced unstopped timers created by time.After can be garbage collected. There is no need to prefer time.NewTimer over time.After for garbage collection efficiency in Go 1.23+.
Applied to files:
internal/worker/executor/service.go
🧬 Code graph analysis (16)
internal/worker/app.go (5)
internal/worker/config/module.go (1)
Module(13-54)internal/worker/executor/module.go (1)
Module(10-28)internal/worker/locker/module.go (1)
Module(10-20)internal/worker/tasks/messages/module.go (1)
Module(10-23)internal/worker/tasks/module.go (1)
Module(9-15)
internal/worker/tasks/messages/hashing.go (2)
internal/worker/tasks/messages/config.go (1)
HashingConfig(10-12)internal/worker/executor/types.go (1)
PeriodicTask(8-12)
internal/worker/executor/types.go (1)
internal/worker/config/types.go (1)
Duration(11-11)
internal/worker/config/config.go (3)
internal/config/config.go (1)
Database(44-55)internal/worker/config/types.go (1)
Duration(11-11)internal/worker/tasks/messages/config.go (1)
Config(5-8)
internal/sms-gateway/modules/messages/service.go (2)
internal/sms-gateway/modules/messages/repository.go (1)
Repository(20-22)internal/sms-gateway/modules/webhooks/service.go (1)
Service(28-37)
internal/worker/tasks/module.go (3)
internal/worker/config/module.go (1)
Module(13-54)internal/worker/executor/module.go (1)
Module(10-28)internal/worker/tasks/messages/module.go (1)
Module(10-23)
internal/worker/locker/mysql.go (1)
internal/worker/locker/locker.go (2)
Locker(10-13)ErrLockNotAcquired(8-8)
internal/sms-gateway/modules/messages/repository.go (3)
internal/sms-gateway/modules/messages/repository_filter.go (3)
MessagesSelectFilter(16-23)MessagesSelectOptions(25-36)MessagesOrder(7-7)internal/sms-gateway/modules/messages/models.go (2)
Message(35-55)MessageRecipient(111-117)pkg/mysql/errors.go (1)
IsDuplicateKeyViolation(9-15)
internal/sms-gateway/modules/messages/module.go (4)
internal/worker/tasks/module.go (1)
Module(9-15)internal/sms-gateway/modules/cleaner/module.go (1)
Module(28-36)internal/sms-gateway/modules/cleaner/service.go (1)
New(16-21)internal/sms-gateway/modules/messages/repository.go (1)
NewRepository(24-28)
internal/sms-gateway/modules/messages/workers.go (2)
internal/sms-gateway/modules/messages/repository.go (1)
Repository(20-22)internal/sms-gateway/modules/messages/config.go (1)
Config(5-8)
internal/sms-gateway/app.go (2)
internal/worker/tasks/module.go (1)
Module(9-15)internal/sms-gateway/modules/messages/module.go (1)
Module(10-25)
internal/worker/executor/service.go (2)
internal/worker/executor/types.go (1)
PeriodicTask(8-12)internal/worker/locker/locker.go (1)
Locker(10-13)
internal/worker/tasks/messages/config.go (1)
internal/worker/config/types.go (1)
Duration(11-11)
internal/worker/tasks/messages/cleanup.go (2)
internal/worker/tasks/messages/config.go (1)
CleanupConfig(14-17)internal/worker/executor/types.go (1)
PeriodicTask(8-12)
internal/worker/executor/module.go (2)
internal/worker/tasks/messages/module.go (1)
Module(10-23)internal/worker/executor/service.go (2)
NewService(24-34)Service(14-22)
internal/worker/config/module.go (4)
internal/worker/config/config.go (5)
Config(9-12)Default(27-50)Tasks(14-17)MessagesHashing(18-20)MessagesCleanup(22-25)internal/worker/tasks/messages/config.go (3)
Config(5-8)HashingConfig(10-12)CleanupConfig(14-17)internal/config/config.go (1)
Database(44-55)internal/worker/config/types.go (1)
Duration(11-11)
⏰ 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: Benchmark
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (27)
internal/worker/locker/locker.go (1)
1-13: LGTM!The locker abstraction is clean and idiomatic. The interface provides the essential lock/unlock operations with proper context support for timeouts and cancellation.
configs/config.example.yml (3)
3-12: LGTM!The database configuration is well-structured with clear comments and consistent environment variable naming.
36-36: Typo already fixed.The previous typo "reasl-time" has been corrected to "real-time" in the current version.
42-49: LGTM!The worker configuration section clearly defines periodic task schedules with appropriate intervals. The separation between gateway/server and worker configurations is clean.
internal/sms-gateway/app.go (2)
29-36: LGTM!The logger integration with go-core-fx is clean and follows the same pattern used in other modules throughout the codebase.
60-67: LGTM!The simplified logger wiring using
WithFxDefaultLogger()is cleaner than manual Zap configuration.internal/config/config.go (3)
44-55: LGTM!The Database struct now includes connection pool controls and removes the
Dialectfield, aligning with the fixed MySQL usage in the worker subsystem.
63-69: LGTM!The deprecation markers provide a clear migration path while maintaining backward compatibility.
75-78: LGTM!The
HashingIntervalSecondsfield provides clearer semantics than the previousProcessedLifetimeHours.pkg/mysql/errors.go (1)
1-15: LGTM!The helper function correctly identifies MySQL duplicate key violations (error code 1062) using proper error unwrapping. This is cleaner than string-based error checking.
internal/worker/tasks/module.go (1)
1-15: LGTM!The tasks module follows clean fx conventions with proper logger integration and clear module composition.
internal/sms-gateway/modules/messages/config.go (1)
1-8: LGTM!The simplified Config struct with
HashingIntervalprovides clearer semantics and aligns with the broader configuration refactor.internal/worker/executor/types.go (1)
1-12: LGTM!The
PeriodicTaskinterface is well-designed with clean separation of concerns. It provides name identification, interval scheduling, and context-aware execution—everything needed for the executor service to orchestrate periodic tasks.internal/worker/app.go (2)
16-24: LGTM! Clean worker bootstrap.The FX application setup follows standard patterns with proper module ordering and lifecycle management.
26-45: LGTM! Well-structured module wiring.The worker module properly integrates locker, tasks, and executor with appropriate lifecycle logging.
internal/worker/executor/module.go (2)
10-28: LGTM! Proper executor lifecycle management.The module correctly wires the executor service with lifecycle hooks. The ignored context parameters are appropriate since
Service.Start()andService.Stop()don't require context.
30-32: LGTM! Clean task grouping helper.The function provides a clean abstraction for registering worker tasks in the DI container.
internal/worker/config/module.go (2)
16-24: LGTM! Proper config loading with error handling.The configuration provider correctly loads defaults and merges with external configuration sources, with appropriate error wrapping.
42-52: LGTM! Clean configuration mapping.The messages configuration provider correctly converts duration types for task scheduling.
internal/worker/tasks/messages/hashing.go (2)
13-31: LGTM! Clean task constructor.The task struct and constructor follow standard Go patterns with proper dependency injection.
44-55: nil parameter correctly hashes all unprocessed messages without ID filtering.The
HashProcessedmethod signature showsids []uint64with logic that only applies an ID filter whenlen(ids) > 0. Passingnilresults in an empty slice, which skips the ID filter and hashes all messages matching the other criteria (is_hashed = false,is_encrypted = false,state <> 'Pending'). This is the intended behavior for theinitialHashingTask.internal/worker/tasks/messages/cleanup.go (2)
13-31: LGTM! Clean task constructor.The cleanup task follows the same well-structured pattern as the hashing task.
44-55: LGTM! Correct cleanup logic.The cleanup task correctly computes the cutoff time and handles errors appropriately.
internal/sms-gateway/modules/messages/module.go (1)
10-25: LGTM! Consistent logger wiring and provider updates.The changes align with the new logger pattern and maintain proper encapsulation with fx.Private providers.
internal/worker/tasks/messages/config.go (1)
5-17: LGTM! Clean configuration structures.The configuration types are well-organized and provide clear separation of concerns for hashing and cleanup task settings.
internal/worker/config/config.go (2)
9-25: LGTM! Well-structured configuration types.The configuration structs are properly organized with appropriate YAML and environment variable mappings.
27-50: LGTM! Sensible default configuration.The default values are reasonable for production use:
- 7-day hashing interval provides good balance
- 30-day message retention with daily cleanup is appropriate
- Database defaults are standard
There was a problem hiding this comment.
Actionable comments posted: 1
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/devices/service.go (1)
100-102: Stale cache after push-token update.
GetByTokenserves from cache; updatingpush_tokenleaves cached device stale for up to TTL. Invalidate (or refresh) the token cache after DB update.-func (s *Service) UpdatePushToken(deviceId string, token string) error { - return s.devices.UpdatePushToken(deviceId, token) -} +func (s *Service) UpdatePushToken(deviceID string, token string) error { + if err := s.devices.UpdatePushToken(deviceID, token); err != nil { + return err + } + // Best-effort cache invalidation to prevent stale data. + // Requires an ID filter; if unavailable, keep as a no-op and rely on TTL. + if dev, err := s.devices.Get(WithID(deviceID)); err == nil { + sum := sha256.Sum256([]byte(dev.AuthToken)) + key := hex.EncodeToString(sum[:]) + if err := s.tokensCache.Delete(key); err != nil { + s.logger.Error("can't invalidate token cache after push-token update", + zap.String("device_id", deviceID), + zap.String("cache_key", key), + zap.Error(err), + ) + } + } + return nil +}If
WithIDdoesn’t exist, I can add a minimal filter helper.
🧹 Nitpick comments (8)
internal/sms-gateway/modules/devices/repository.go (4)
55-70: Get loads all matches; limit to 2 to detect duplicates without scanning large sets.Current approach may allocate unnecessarily. Limit to 2 and decide based on length.
-func (r *Repository) Get(filter ...SelectFilter) (models.Device, error) { - devices, err := r.Select(filter...) - if err != nil { - return models.Device{}, err - } - - if len(devices) == 0 { - return models.Device{}, ErrNotFound - } - - if len(devices) > 1 { - return models.Device{}, ErrMoreThanOne - } - - return devices[0], nil -} +func (r *Repository) Get(filter ...SelectFilter) (models.Device, error) { + if len(filter) == 0 { + return models.Device{}, ErrInvalidFilter + } + f := newFilter(filter...) + devices := make([]models.Device, 0, 2) + if err := f.apply(r.db).Limit(2).Find(&devices).Error; err != nil { + return models.Device{}, err + } + switch len(devices) { + case 0: + return models.Device{}, ErrNotFound + case 1: + return devices[0], nil + default: + return models.Device{}, ErrMoreThanOne + } +}
76-78: UpdatePushToken should report not-found when no rows are updated.Currently returns nil even if
iddoesn’t exist. Check RowsAffected and map to ErrNotFound.-func (r *Repository) UpdatePushToken(id, token string) error { - return r.db.Model(&models.Device{}).Where("id = ?", id).Update("push_token", token).Error -} +func (r *Repository) UpdatePushToken(id, token string) error { + res := r.db.Model(&models.Device{}).Where("id = ?", id).Update("push_token", token) + if res.Error != nil { + return res.Error + } + if res.RowsAffected == 0 { + return ErrNotFound + } + return nil +}
96-103: Remove: clarify desired behavior when nothing matches.Currently returns nil on no-op deletes. If callers expect 404-like feedback, return ErrNotFound when RowsAffected==0. Otherwise, keep as-is for idempotency.
105-112: Cleanup: soft vs hard delete?
Deletewill soft-delete by default with GORM. If the intent is permanent purge, addUnscoped(). Please confirm.- res := r.db. + res := r.db. WithContext(ctx). - Where("last_seen < ?", until). + // Use Unscoped() if permanent purge is desired. + // Unscoped(). + Where("last_seen < ?", until). Delete(&models.Device{})internal/sms-gateway/modules/devices/service.go (1)
150-157: Constructor: consider making cache TTL configurable.Hardcoded
10 * time.Minutemay not suit all deployments. Expose via devices.Config (or worker/config) for consistency.internal/worker/tasks/devices/config.go (1)
5-12: Config types look fine. Add brief docs for units/semantics.Small docstrings for Interval/MaxAge improve readability (e.g., “polling interval,” “entries older than MaxAge are eligible for cleanup”).
internal/worker/tasks/devices/module.go (1)
3-8: Avoid package name shadowing for clarity.Alias the imported devices repo to prevent confusion with the current package name.
import ( - "github.com/android-sms-gateway/server/internal/sms-gateway/modules/devices" + devicesrepo "github.com/android-sms-gateway/server/internal/sms-gateway/modules/devices" "github.com/android-sms-gateway/server/internal/worker/executor" "github.com/go-core-fx/logger" "go.uber.org/fx" ) ... - fx.Provide(devices.NewRepository, fx.Private), + fx.Provide(devicesrepo.NewRepository, fx.Private),Also applies to: 17-17
internal/worker/config/module.go (1)
26-41: Optional: make DB lifetimes configurable.If infra supports it, consider adding ConnMaxIdleTime and ConnMaxLifetime to your Config and wiring them here (non-zero defaults help long‑running workers).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
configs/config.example.yml(2 hunks)internal/sms-gateway/app.go(3 hunks)internal/sms-gateway/modules/cleaner/interfaces.go(0 hunks)internal/sms-gateway/modules/cleaner/module.go(0 hunks)internal/sms-gateway/modules/cleaner/service.go(0 hunks)internal/sms-gateway/modules/devices/module.go(1 hunks)internal/sms-gateway/modules/devices/repository.go(6 hunks)internal/sms-gateway/modules/devices/service.go(3 hunks)internal/worker/config/config.go(1 hunks)internal/worker/config/module.go(1 hunks)internal/worker/tasks/devices/cleanup.go(1 hunks)internal/worker/tasks/devices/config.go(1 hunks)internal/worker/tasks/devices/module.go(1 hunks)internal/worker/tasks/messages/module.go(1 hunks)internal/worker/tasks/module.go(1 hunks)
💤 Files with no reviewable changes (3)
- internal/sms-gateway/modules/cleaner/module.go
- internal/sms-gateway/modules/cleaner/interfaces.go
- internal/sms-gateway/modules/cleaner/service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- configs/config.example.yml
- internal/worker/config/config.go
🧰 Additional context used
🧬 Code graph analysis (10)
internal/sms-gateway/modules/devices/service.go (1)
internal/sms-gateway/modules/devices/repository.go (1)
Repository(18-20)
internal/worker/tasks/devices/config.go (1)
internal/worker/config/types.go (1)
Duration(11-11)
internal/sms-gateway/modules/devices/module.go (4)
internal/sms-gateway/app.go (1)
Module(33-56)internal/sms-gateway/modules/messages/module.go (1)
Module(10-25)internal/sms-gateway/modules/devices/repository.go (1)
NewRepository(22-26)internal/sms-gateway/modules/devices/service.go (1)
NewService(150-158)
internal/worker/tasks/devices/module.go (6)
internal/sms-gateway/modules/devices/module.go (1)
Module(8-18)internal/worker/config/module.go (1)
Module(14-63)internal/worker/tasks/devices/config.go (2)
Config(5-7)CleanupConfig(9-12)internal/sms-gateway/modules/devices/repository.go (1)
NewRepository(22-26)internal/worker/executor/module.go (1)
AsWorkerTask(30-32)internal/worker/tasks/devices/cleanup.go (1)
NewCleanupTask(20-31)
internal/worker/tasks/module.go (4)
internal/worker/config/module.go (1)
Module(14-63)internal/worker/tasks/devices/module.go (1)
Module(10-22)internal/worker/tasks/messages/module.go (1)
Module(10-23)internal/worker/executor/module.go (1)
Module(10-28)
internal/worker/config/module.go (8)
internal/worker/tasks/devices/module.go (1)
Module(10-22)internal/worker/tasks/messages/module.go (1)
Module(10-23)internal/worker/tasks/module.go (1)
Module(10-17)internal/worker/config/config.go (6)
Config(9-12)Default(33-56)Tasks(14-18)MessagesHashing(19-21)MessagesCleanup(23-26)DevicesCleanup(28-31)internal/worker/tasks/devices/config.go (2)
Config(5-7)CleanupConfig(9-12)internal/worker/tasks/messages/config.go (3)
Config(5-8)HashingConfig(10-12)CleanupConfig(14-17)internal/config/config.go (1)
Database(44-55)internal/worker/config/types.go (1)
Duration(11-11)
internal/sms-gateway/app.go (1)
internal/sms-gateway/modules/devices/module.go (1)
Module(8-18)
internal/sms-gateway/modules/devices/repository.go (3)
internal/sms-gateway/modules/messages/repository.go (2)
NewRepository(24-28)Repository(20-22)internal/sms-gateway/modules/devices/repository_filter.go (1)
SelectFilter(9-9)internal/sms-gateway/models/models.go (1)
Device(25-36)
internal/worker/tasks/devices/cleanup.go (3)
internal/worker/tasks/devices/config.go (1)
CleanupConfig(9-12)internal/sms-gateway/modules/devices/repository.go (1)
Repository(18-20)internal/worker/executor/types.go (1)
PeriodicTask(8-12)
internal/worker/tasks/messages/module.go (6)
internal/worker/config/module.go (1)
Module(14-63)internal/worker/tasks/module.go (1)
Module(10-17)internal/worker/executor/module.go (2)
Module(10-28)AsWorkerTask(30-32)internal/worker/tasks/messages/config.go (3)
Config(5-8)HashingConfig(10-12)CleanupConfig(14-17)internal/worker/tasks/messages/hashing.go (1)
NewInitialHashingTask(20-31)internal/worker/tasks/messages/cleanup.go (1)
NewCleanupTask(20-31)
⏰ 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 (15)
internal/sms-gateway/modules/devices/repository.go (5)
18-26: Public Repository type and constructor look good.Exported type and NewRepository improve DI ergonomics. No issues.
28-37: Select: guard against empty filters — good.Prevents accidental full scans. Implementation is fine.
44-53: Exists: correct use of Take with ErrRecordNotFound.Simple and efficient. LGTM.
72-74: Insert: OK.Straightforward Create; no concerns.
80-94: SetLastSeen: monotonic update and no-op on stale/zero — good.Pattern avoids time regressions and treats stale timestamps as benign. LGTM.
internal/sms-gateway/modules/devices/service.go (3)
23-23: *Wiring to exported Repository: good migration.Type switch aligns with repo export. No issues.
Also applies to: 33-33
64-65: Exists pass-through with user scoping: LGTM.
74-75: Get pass-through with user scoping: LGTM.internal/worker/tasks/module.go (1)
10-17: FX tasks module wiring: LGTM.Named logger + nested modules are composed cleanly.
internal/worker/tasks/messages/module.go (1)
10-23: Messages worker module wiring: LGTM.Private config extraction and grouped task providers are set up correctly.
internal/worker/tasks/devices/module.go (1)
10-21: Devices worker task wiring looks good.Private scoping for config and repo, grouped task provider, and named logger are consistent.
internal/worker/config/module.go (1)
14-63: Config wiring LGTM.Private Config provider + derived configs for db/messages/devices are sound; duration conversions look correct.
internal/sms-gateway/modules/devices/module.go (1)
8-17: Devices module wiring LGTM.Scoped repo provider and named logger match the new module pattern.
internal/sms-gateway/app.go (2)
33-56: Server module list updates LGTM.Using logger.Module() and devices.Module() is consistent with the new modular pattern.
58-65: Logger wiring in Run LGTM.logger.WithFxDefaultLogger() cleanly replaces the previous fx logger setup.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/worker/tasks/devices/cleanup.go (1)
8-8: Remaining suggestions from previous review.The following improvements suggested in the previous review are still pending:
Import aliasing and field naming (lines 8, 15, 22): Aliasing the
devicesimport and renaming thedevicesfield torepowould reduce ambiguity when the package name matches the field name.Time source injection (line 45): Injecting a time source (
now func() time.Time) instead of callingtime.Now()directly would improve testability by allowing tests to control time.Also applies to: 15-15, 22-22, 45-45
🧹 Nitpick comments (7)
internal/sms-gateway/modules/devices/cache.go (2)
17-24: Make TTL configurable (inject via config) instead of hardcoding 10m.Parameterize TTL (e.g., from devices.Config) to tune cache behavior per env and ease tests.
26-33: Make Set atomic; rollback on partial write.Current errors.Join can leave one index populated if the other write fails. Use two-phase set with rollback.
Apply:
func (c *cache) Set(device models.Device) error { - err := errors.Join(c.byID.Set(device.ID, &device), c.byToken.Set(device.AuthToken, &device)) - if err != nil { - return fmt.Errorf("failed to cache device: %w", err) - } - return nil + if err := c.byID.Set(device.ID, &device); err != nil { + return fmt.Errorf("failed to cache device by id: %w", err) + } + if err := c.byToken.Set(device.AuthToken, &device); err != nil { + _ = c.byID.Delete(device.ID) // rollback + return fmt.Errorf("failed to cache device by token: %w", err) + } + return nil }internal/sms-gateway/modules/devices/service.go (3)
25-41: Prefer injecting cache (or TTL) via constructor to ease testing and tuning.Constructing cache internally hides configuration and complicates tests.
Example approach: accept a cache interface or TTL in NewService and call newCache(ttl).
99-112: Also refresh token-indexed cache entry after push-token update.You invalidate only by ID; the token-indexed entry can serve a stale PushToken until TTL.
Apply:
func (s *Service) UpdatePushToken(id string, token string) error { if err := s.cache.DeleteByID(id); err != nil { s.logger.Error("can't invalidate cache", zap.String("device_id", id), zap.Error(err), ) } if err := s.devices.UpdatePushToken(id, token); err != nil { return err } - return nil + // Optional: re-prime cache with fresh record to avoid subsequent misses/stale data. + if d, err := s.devices.Get(WithID(id)); err == nil { + if err := s.cache.Set(d); err != nil { + s.logger.Error("can't cache device after update", zap.String("device_id", id), zap.Error(err)) + } + } + return nil }If WithID is unavailable, fetch with a suitable filter that uniquely identifies the device.
141-163: Evict token-indexed entries during Remove to prevent stale token lookups.Deleting by ID alone won’t clear the token-indexed cache; add token eviction per device.
Apply:
- for _, device := range devices { - if err := s.cache.DeleteByID(device.ID); err != nil { + for _, device := range devices { + if err := s.cache.DeleteByID(device.ID); err != nil { s.logger.Error("can't invalidate cache", zap.String("device_id", device.ID), zap.Error(err), ) - } + } + if err := s.cache.DeleteByToken(device.AuthToken); err != nil { + s.logger.Error("can't invalidate token cache", + zap.String("device_id", device.ID), + zap.String("auth_token", device.AuthToken), + zap.Error(err), + ) + } }Requires the optional DeleteByToken helper proposed in cache.go.
internal/sms-gateway/modules/devices/repository.go (2)
56-71: Avoid full scans in Get; use LIMIT 2 to detect multiplicity.Load up to two rows and decide: 0 => not found, 1 => ok, 2 => ErrMoreThanOne.
Apply:
func (r *Repository) Get(filter ...SelectFilter) (models.Device, error) { - devices, err := r.Select(filter...) + f := newFilter(filter...) + devices := make([]models.Device, 0, 2) + err := f.apply(r.db).Limit(2).Find(&devices).Error if err != nil { return models.Device{}, fmt.Errorf("failed to get device: %w", err) } if len(devices) == 0 { return models.Device{}, ErrNotFound } if len(devices) > 1 { return models.Device{}, ErrMoreThanOne } return devices[0], nil }
113-120: Consider chunked cleanup for large tables.Long-running deletes can lock or bloat logs. Prefer batched deletes (LIMIT/OFFSET or PK ranges) with transaction/timeout tuning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/sms-gateway/modules/devices/cache.go(1 hunks)internal/sms-gateway/modules/devices/repository.go(7 hunks)internal/sms-gateway/modules/devices/service.go(3 hunks)internal/worker/tasks/devices/cleanup.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/sms-gateway/modules/devices/cache.go (1)
internal/sms-gateway/models/models.go (1)
Device(25-36)
internal/sms-gateway/modules/devices/service.go (4)
internal/sms-gateway/modules/devices/config.go (1)
Config(5-7)internal/sms-gateway/modules/devices/repository.go (1)
Repository(19-21)internal/sms-gateway/modules/db/module.go (1)
IDGen(8-8)internal/sms-gateway/modules/devices/repository_filter.go (1)
WithToken(17-21)
internal/worker/tasks/devices/cleanup.go (3)
internal/worker/tasks/devices/config.go (1)
CleanupConfig(9-12)internal/sms-gateway/modules/devices/repository.go (1)
Repository(19-21)internal/worker/executor/types.go (1)
PeriodicTask(8-12)
internal/sms-gateway/modules/devices/repository.go (4)
internal/sms-gateway/modules/messages/repository.go (2)
NewRepository(24-28)Repository(20-22)internal/sms-gateway/modules/devices/repository_filter.go (1)
SelectFilter(9-9)internal/sms-gateway/modules/webhooks/repository_filter.go (1)
SelectFilter(5-5)internal/sms-gateway/models/models.go (1)
Device(25-36)
⏰ 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/worker/tasks/devices/cleanup.go (1)
47-47: Log messages correctly reference "devices".The error and info log messages now correctly mention "devices" instead of "messages" as flagged in the previous review.
Also applies to: 51-51
internal/sms-gateway/modules/devices/cache.go (2)
35-42: LGTM: safe value return.Returning a value copy avoids unintended shared-state mutation.
44-51: LGTM: cache-first path with value copy.Works as intended; errors are wrapped consistently.
internal/sms-gateway/modules/devices/service.go (1)
84-97: LGTM: cache→repo fallback and cache fill.Flow and logging look good; error propagation is clear.
internal/sms-gateway/modules/devices/repository.go (5)
23-27: LGTM: exported constructor improves DI wiring.
29-38: LGTM: filter guard and straightforward select.Consistent invalid-filter check and filter application.
77-86: LGTM: precise update with error wrapping and not-found handling.Scope by PK; semantics are clear.
88-102: LGTM: idempotent last_seen update guarded by monotonicity.Zero-time ignored; UpdateColumn avoids hooks/defaults.
104-112: LGTM: guarded delete with filter presence check.
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/devices/cache.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/modules/devices/cache.go (2)
internal/sms-gateway/models/models.go (1)
Device(25-36)pkg/cache/errors.go (1)
ErrKeyNotFound(7-7)
⏰ 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 (4)
internal/sms-gateway/modules/devices/cache.go (4)
12-24: LGTM: dual-key cache design is appropriate.The dual-index design (byID and byToken) cleanly supports the lookup patterns for devices, and the 10-minute TTL is reasonable for an internal cache.
35-42: LGTM: clean retrieval with defensive copy.Returning the dereferenced device by value ensures callers receive an independent copy, preventing unintended mutations.
44-51: LGTM: consistent with GetByID pattern.The implementation correctly mirrors GetByID, maintaining consistency across lookup methods.
53-69: LGTM: correctly evicts from both caches.The implementation now properly fetches the device first to retrieve the AuthToken, then deletes from both indexes. This resolves the critical issue flagged in the previous review where
byToken.Delete(id)left stale entries.The idempotent handling of
ErrKeyNotFoundis appropriate for delete operations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/worker/executor/service.go (3)
36-62: Concurrent Start() calls will spawn duplicate task goroutines.This issue has already been flagged in previous review comments with a proposed fix using a mutex-guarded
startedboolean.
84-101: Lock handling needs defer and panic recovery.This issue has already been comprehensively flagged in previous review comments, including:
- Lock contention should be logged at debug/info level (not error)
- Lock release should be deferred to ensure it runs even if
task.Run()panics- Panic recovery should be added
105-110: Calling Stop() twice will panic on double-close.This issue has already been flagged in previous review comments with a proposed fix using either
sync.Onceor mutex-guarded state management.
🧹 Nitpick comments (1)
internal/worker/executor/service.go (1)
64-75: Initial delay jitter truncates to seconds.The current jitter calculation using
math.Floor(rand.Float64()*task.Interval().Seconds())truncates to whole seconds, which means:
- For intervals < 1 second,
initialDelaywill always be 0- For longer intervals, sub-second precision is lost
Consider using
rand.Int64N(task.Interval().Nanoseconds())for full-precision jitter across the entire interval range.Note: The
time.Afterusage is appropriate for Go 1.23+, as per retrieved learnings.Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/worker/executor/service.go(1 hunks)internal/worker/locker/mysql.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/worker/locker/mysql.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T11:12:35.670Z
Learnt from: capcom6
PR: android-sms-gateway/server#187
File: internal/worker/executor/service.go:64-75
Timestamp: 2025-10-28T11:12:35.670Z
Learning: As of Go 1.23, unreferenced unstopped timers created by time.After can be garbage collected. There is no need to prefer time.NewTimer over time.After for garbage collection efficiency in Go 1.23+.
Applied to files:
internal/worker/executor/service.go
🧬 Code graph analysis (1)
internal/worker/executor/service.go (2)
internal/worker/executor/types.go (1)
PeriodicTask(8-12)internal/worker/locker/locker.go (1)
Locker(10-13)
⏰ 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)
internal/worker/executor/service.go (2)
1-12: LGTM!Imports are appropriate for the task executor implementation.
24-34: LGTM!Constructor properly initializes all fields.
capcom6
force-pushed
the
worker/initial-implementation
branch
from
62c1bc7 to
79697df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
internal/sms-gateway/modules/devices/cache.go (1)
26-33: Critical: dangling pointer issue remains unresolved.Despite the previous review comment being marked as "Addressed in commit 92c60ce", the code still takes
deviceby value and stores&device, which creates a pointer to a stack-allocated local variable. OnceSetreturns, this pointer becomes invalid.Apply this diff to fix:
-func (c *cache) Set(device models.Device) error { - err := errors.Join(c.byID.Set(device.ID, &device), c.byToken.Set(device.AuthToken, &device)) +func (c *cache) Set(device *models.Device) error { + err := errors.Join(c.byID.Set(device.ID, device), c.byToken.Set(device.AuthToken, device)) if err != nil { return fmt.Errorf("failed to cache device: %w", err) } return nil }Update all call sites to pass
&deviceinstead ofdevice.internal/worker/executor/service.go (3)
14-61: Make Start idempotent and track service state.
Startcan be invoked multiple times, launching duplicate goroutines and reusing the same stop channel. Guard it with a mutex, track astartedflag, and resetstopChan/stopOnceon (re)start. This was already called out earlier and is still pending.Apply:
type Service struct { tasks []PeriodicTask locker locker.Locker + mu sync.Mutex + started bool + stopOnce sync.Once + stopChan chan struct{} wg sync.WaitGroup @@ func (s *Service) Start() error { - ctx, cancel := context.WithCancel(context.Background()) + s.mu.Lock() + if s.started { + s.mu.Unlock(); + return nil + } + s.stopChan = make(chan struct{}) + s.stopOnce = sync.Once{} + s.started = true + s.mu.Unlock() + + ctx, cancel := context.WithCancel(context.Background())
105-109: Stop must be safe to call twice.A second
Stopcall closes an already-closed channel and panics. Guard with the same mutex/state flag and wrap the close instopOnce.Do, then flipstartedback to false once shutdown finishes. This duplicates an unresolved earlier review.Apply:
func (s *Service) Stop() error { - close(s.stopChan) - s.wg.Wait() - - return nil + s.mu.Lock() + if !s.started { + s.mu.Unlock() + return nil + } + s.mu.Unlock() + + s.stopOnce.Do(func() { + close(s.stopChan) + }) + + s.wg.Wait() + + s.mu.Lock() + s.started = false + s.mu.Unlock() + + return nil }
79-101: Always release the lock (and recover) when tasks panic.If
task.Runpanics, we never reachReleaseLock, leaving the distributed lock held indefinitely. Wrap the run in a defer that recovers, logs, and releases the lock so tasks can't wedge the worker—this is the same unresolved feedback noted earlier.Apply:
@@ - case <-ticker.C: - if err := s.locker.AcquireLock(ctx, task.Name()); err != nil { + case <-ticker.C: + taskName := task.Name() + if err := s.locker.AcquireLock(ctx, taskName); err != nil { s.logger.Error("can't acquire lock", zap.String("name", task.Name()), zap.Error(err)) continue } start := time.Now() s.logger.Info("running task", zap.String("name", task.Name())) - if err := task.Run(ctx); err != nil { - s.logger.Error("task failed", zap.String("name", task.Name()), zap.Duration("duration", time.Since(start)), zap.Error(err)) - } else { - s.logger.Info("task succeeded", zap.String("name", task.Name()), zap.Duration("duration", time.Since(start))) - } - - if err := s.locker.ReleaseLock(ctx, task.Name()); err != nil { - s.logger.Error("can't release lock", zap.String("name", task.Name()), zap.Error(err)) - } + func() { + defer func() { + if r := recover(); r != nil { + s.logger.Error("task panicked", zap.String("name", taskName), zap.Any("panic", r), zap.Duration("duration", time.Since(start))) + } + if err := s.locker.ReleaseLock(ctx, taskName); err != nil { + s.logger.Error("can't release lock", zap.String("name", taskName), zap.Error(err)) + } + }() + + if err := task.Run(ctx); err != nil { + s.logger.Error("task failed", zap.String("name", taskName), zap.Duration("duration", time.Since(start)), zap.Error(err)) + return + } + + s.logger.Info("task succeeded", zap.String("name", taskName), zap.Duration("duration", time.Since(start))) + }()internal/sms-gateway/modules/messages/repository.go (1)
174-191: Critical: Unqualified columns will cause runtime SQL errors.The SQL statement still contains unqualified column references that were flagged in the previous review and remain unfixed:
- Line 176:
JSON_VALUE(\content`, ...)` resolves ambiguously- Line 176:
SHA2(phone_number, ...)will fail becausephone_numberexists only inmessage_recipientsThis will cause runtime failures when the query executes.
Apply the qualification fixes from the previous review:
rawSQL := "UPDATE `messages` `m`, `message_recipients` `r`\n" + - "SET `m`.`is_hashed` = true, `m`.`content` = SHA2(COALESCE(JSON_VALUE(`content`, '$.text'), JSON_VALUE(`content`, '$.data')), 256), `r`.`phone_number` = LEFT(SHA2(phone_number, 256), 16)\n" + + "SET `m`.`is_hashed` = true, `m`.`content` = SHA2(COALESCE(JSON_VALUE(`m`.`content`, '$.text'), JSON_VALUE(`m`.`content`, '$.data')), 256), `r`.`phone_number` = LEFT(SHA2(`r`.`phone_number`, 256), 16)\n" + "WHERE `m`.`id` = `r`.`message_id` AND `m`.`is_hashed` = false AND `m`.`is_encrypted` = false AND `m`.`state` <> 'Pending'"Note: The context handling and return value improvements in this refactor are otherwise well-implemented.
🧹 Nitpick comments (1)
internal/worker/config/module.go (1)
37-38: Consider making connection lifetime settings configurable.The zero values for
ConnMaxIdleTimeandConnMaxLifetimemean connections can idle indefinitely and have no maximum lifetime. While this may be acceptable for worker mode, consider exposing these as configurable parameters for better control over connection management.
📜 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 (38)
cmd/sms-gateway/main.go(2 hunks)configs/config.example.yml(2 hunks)go.mod(5 hunks)internal/config/config.go(2 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(3 hunks)internal/sms-gateway/modules/cleaner/interfaces.go(0 hunks)internal/sms-gateway/modules/cleaner/module.go(0 hunks)internal/sms-gateway/modules/cleaner/service.go(0 hunks)internal/sms-gateway/modules/devices/cache.go(1 hunks)internal/sms-gateway/modules/devices/module.go(1 hunks)internal/sms-gateway/modules/devices/repository.go(7 hunks)internal/sms-gateway/modules/devices/service.go(3 hunks)internal/sms-gateway/modules/messages/config.go(1 hunks)internal/sms-gateway/modules/messages/module.go(1 hunks)internal/sms-gateway/modules/messages/repository.go(6 hunks)internal/sms-gateway/modules/messages/service.go(4 hunks)internal/sms-gateway/modules/messages/tasks.go(0 hunks)internal/sms-gateway/modules/messages/workers.go(1 hunks)internal/worker/app.go(1 hunks)internal/worker/config/config.go(1 hunks)internal/worker/config/module.go(1 hunks)internal/worker/config/types.go(1 hunks)internal/worker/executor/module.go(1 hunks)internal/worker/executor/service.go(1 hunks)internal/worker/executor/types.go(1 hunks)internal/worker/locker/locker.go(1 hunks)internal/worker/locker/module.go(1 hunks)internal/worker/locker/mysql.go(1 hunks)internal/worker/tasks/devices/cleanup.go(1 hunks)internal/worker/tasks/devices/config.go(1 hunks)internal/worker/tasks/devices/module.go(1 hunks)internal/worker/tasks/messages/cleanup.go(1 hunks)internal/worker/tasks/messages/config.go(1 hunks)internal/worker/tasks/messages/hashing.go(1 hunks)internal/worker/tasks/messages/module.go(1 hunks)internal/worker/tasks/module.go(1 hunks)pkg/mysql/errors.go(1 hunks)
💤 Files with no reviewable changes (4)
- internal/sms-gateway/modules/cleaner/interfaces.go
- internal/sms-gateway/modules/cleaner/module.go
- internal/sms-gateway/modules/cleaner/service.go
- internal/sms-gateway/modules/messages/tasks.go
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/mysql/errors.go
- cmd/sms-gateway/main.go
- internal/worker/locker/module.go
- internal/worker/config/config.go
- internal/worker/executor/types.go
- internal/worker/executor/module.go
- internal/worker/tasks/module.go
- internal/sms-gateway/modules/messages/workers.go
- internal/worker/tasks/devices/cleanup.go
- internal/config/module.go
- internal/worker/app.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
PR: android-sms-gateway/server#178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
internal/sms-gateway/modules/devices/module.gointernal/sms-gateway/modules/messages/module.gointernal/sms-gateway/app.go
📚 Learning: 2025-10-28T11:12:35.670Z
Learnt from: capcom6
PR: android-sms-gateway/server#187
File: internal/worker/executor/service.go:64-75
Timestamp: 2025-10-28T11:12:35.670Z
Learning: As of Go 1.23, unreferenced unstopped timers created by time.After can be garbage collected. There is no need to prefer time.NewTimer over time.After for garbage collection efficiency in Go 1.23+.
Applied to files:
internal/worker/executor/service.go
🧬 Code graph analysis (17)
internal/worker/tasks/messages/module.go (6)
internal/worker/config/module.go (1)
Module(14-63)internal/worker/executor/module.go (2)
Module(10-28)AsWorkerTask(30-32)internal/worker/tasks/module.go (1)
Module(10-17)internal/worker/tasks/messages/config.go (3)
Config(5-8)HashingConfig(10-12)CleanupConfig(14-17)internal/worker/tasks/messages/hashing.go (1)
NewInitialHashingTask(20-31)internal/worker/tasks/messages/cleanup.go (1)
NewCleanupTask(20-31)
internal/worker/config/module.go (5)
internal/worker/config/config.go (6)
Config(9-12)Default(33-56)Tasks(14-18)MessagesHashing(19-21)MessagesCleanup(23-26)DevicesCleanup(28-31)internal/worker/tasks/devices/config.go (2)
Config(5-7)CleanupConfig(9-12)internal/worker/tasks/messages/config.go (3)
Config(5-8)HashingConfig(10-12)CleanupConfig(14-17)internal/config/config.go (1)
Database(44-55)internal/worker/config/types.go (1)
Duration(11-11)
internal/worker/tasks/messages/cleanup.go (2)
internal/worker/tasks/messages/config.go (1)
CleanupConfig(14-17)internal/worker/executor/types.go (1)
PeriodicTask(8-12)
internal/worker/locker/mysql.go (1)
internal/worker/locker/locker.go (2)
Locker(10-13)ErrLockNotAcquired(8-8)
internal/worker/tasks/messages/config.go (2)
internal/worker/tasks/devices/config.go (1)
CleanupConfig(9-12)internal/worker/config/types.go (1)
Duration(11-11)
internal/sms-gateway/modules/devices/module.go (4)
internal/sms-gateway/app.go (1)
Module(33-56)internal/sms-gateway/modules/messages/module.go (1)
Module(10-25)internal/sms-gateway/modules/devices/repository.go (1)
NewRepository(23-27)internal/sms-gateway/modules/devices/service.go (1)
NewService(25-41)
internal/sms-gateway/modules/messages/service.go (2)
internal/sms-gateway/modules/messages/repository.go (1)
Repository(20-22)internal/sms-gateway/modules/devices/service.go (1)
Service(14-23)
internal/worker/tasks/devices/module.go (7)
internal/worker/config/module.go (1)
Module(14-63)internal/worker/executor/module.go (2)
Module(10-28)AsWorkerTask(30-32)internal/worker/tasks/messages/module.go (1)
Module(10-23)internal/worker/tasks/module.go (1)
Module(10-17)internal/worker/tasks/devices/config.go (2)
Config(5-7)CleanupConfig(9-12)internal/worker/tasks/devices/cleanup.go (1)
NewCleanupTask(20-31)internal/worker/tasks/messages/cleanup.go (1)
NewCleanupTask(20-31)
internal/worker/tasks/messages/hashing.go (2)
internal/worker/tasks/messages/config.go (1)
HashingConfig(10-12)internal/worker/executor/types.go (1)
PeriodicTask(8-12)
internal/sms-gateway/modules/devices/cache.go (2)
internal/sms-gateway/models/models.go (1)
Device(25-36)pkg/cache/errors.go (1)
ErrKeyNotFound(7-7)
internal/sms-gateway/modules/devices/service.go (4)
internal/sms-gateway/modules/devices/config.go (1)
Config(5-7)internal/sms-gateway/modules/devices/repository.go (1)
Repository(19-21)internal/sms-gateway/modules/db/module.go (1)
IDGen(8-8)internal/sms-gateway/modules/devices/repository_filter.go (1)
WithToken(17-21)
internal/sms-gateway/modules/messages/module.go (5)
internal/sms-gateway/app.go (1)
Module(33-56)internal/config/module.go (1)
Module(22-127)internal/sms-gateway/modules/devices/module.go (1)
Module(8-18)internal/sms-gateway/cache/factory.go (1)
Factory(16-18)internal/sms-gateway/modules/messages/repository.go (1)
NewRepository(24-28)
internal/worker/tasks/devices/config.go (2)
internal/worker/tasks/messages/config.go (1)
CleanupConfig(14-17)internal/worker/config/types.go (1)
Duration(11-11)
internal/worker/executor/service.go (2)
internal/worker/executor/types.go (1)
PeriodicTask(8-12)internal/worker/locker/locker.go (1)
Locker(10-13)
internal/sms-gateway/modules/messages/repository.go (4)
internal/sms-gateway/modules/devices/repository.go (2)
NewRepository(23-27)Repository(19-21)internal/sms-gateway/modules/messages/repository_filter.go (3)
MessagesSelectFilter(16-23)MessagesSelectOptions(25-36)MessagesOrder(7-7)internal/sms-gateway/modules/messages/models.go (2)
Message(35-55)MessageRecipient(111-117)pkg/mysql/errors.go (1)
IsDuplicateKeyViolation(9-15)
internal/sms-gateway/modules/devices/repository.go (3)
internal/sms-gateway/modules/messages/repository.go (2)
NewRepository(24-28)Repository(20-22)internal/sms-gateway/modules/devices/repository_filter.go (1)
SelectFilter(9-9)internal/sms-gateway/models/models.go (1)
Device(25-36)
internal/sms-gateway/app.go (2)
internal/sms-gateway/modules/messages/module.go (1)
Module(10-25)internal/sms-gateway/modules/devices/module.go (1)
Module(8-18)
⏰ 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 (30)
go.mod (1)
3-3: Dependency updates look good.The toolchain bump to Go 1.24.3, addition of go-core-fx/logger for FX logging wiring, promotion of yaml.v3 to direct for config parsing, and indirect dependency updates all align with the worker mode introduction.
Also applies to: 11-11, 29-29, 70-70, 88-88, 101-101
internal/sms-gateway/modules/devices/cache.go (1)
53-69: DeleteByID token eviction fix looks good.The method now correctly fetches the device first and uses
device.AuthTokenfor the byToken deletion, resolving the previous issue whereidwas incorrectly used as the token key.internal/sms-gateway/app.go (1)
28-28: Logger wiring migration looks correct.The switch from FX event-based logging to go-core-fx logger, including
logger.Module()andlogger.WithFxDefaultLogger(), is properly implemented. The removal of the cleaner subsystem aligns with the PR's goal of extracting periodic tasks into standalone worker mode.Also applies to: 35-35, 52-52, 63-63
internal/sms-gateway/modules/devices/repository.go (3)
19-27: Public repository API looks good.Promoting
Repositoryto public with a standard constructor pattern enables the worker tasks to access device data. This aligns with the PR's goal of extracting periodic operations to a standalone worker.
77-86: Improved error handling in UpdatePushToken.The explicit error wrapping with
fmt.Errorfand theRowsAffectedcheck forErrNotFoundimprove error handling consistency and make the behavior more explicit.
113-120: Cleanup method signature is clear.Renaming from
removeUnused(since)toCleanup(until)and updating the parameter name improves clarity—untilsemantically matches theWHERE last_seen < ?condition.internal/worker/tasks/messages/module.go (1)
10-23: Module wiring is well-structured.The FX module correctly uses named logger for module scope, provides private repository and config extraction, and annotates tasks with
executor.AsWorkerTaskfor collection by the executor service. This follows established patterns.internal/worker/tasks/devices/config.go (1)
5-12: Config structure is clean and appropriate.The
ConfigandCleanupConfigtypes provide clear, type-safe configuration for the device cleanup task withIntervalandMaxAgefields matching the expected behavior.internal/worker/tasks/messages/hashing.go (2)
20-55: Hashing task implementation is solid.The periodic task correctly implements the
executor.PeriodicTaskinterface with proper error wrapping, conditional logging (only when rows > 0), and clean constructor pattern. TheHashProcessed(ctx, nil)call appropriately hashes all unprocessed messages.
57-57: Good use of compile-time interface assertion.The
var _ executor.PeriodicTask = (*initialHashingTask)(nil)assertion ensures the type implements the interface at compile time, catching errors early.internal/worker/tasks/messages/cleanup.go (2)
20-55: Cleanup task implementation is correct.The periodic task follows the same clean pattern as the hashing task, with proper time calculation (
time.Now().Add(-config.MaxAge)) for the cleanup cutoff, error wrapping, and conditional logging. The implementation is consistent and maintainable.
57-57: Compile-time interface check is good practice.The interface assertion ensures
cleanupTaskimplementsexecutor.PeriodicTaskat compile time.internal/sms-gateway/modules/messages/config.go (1)
5-8: LGTM! Clear semantic improvement.The rename from
ProcessedLifetimetoHashingIntervalbetter conveys the purpose of this configuration field.internal/worker/tasks/devices/module.go (1)
10-22: LGTM! Consistent module pattern.The devices task module follows the established pattern from messages.Module() with proper dependency wiring and appropriate use of fx.Private for internal providers.
internal/worker/config/types.go (1)
11-37: LGTM! Robust duration parsing.The custom Duration type with UnmarshalText and UnmarshalYAML methods enables flexible duration parsing from both text and YAML sources. The interface assertions ensure compile-time verification of the implementation.
internal/worker/config/module.go (1)
43-61: LGTM! Clean configuration mapping.The messages and devices configuration providers correctly map from the loaded config to task-specific config types with appropriate duration conversions.
configs/config.example.yml (1)
1-52: LGTM! Well-structured configuration.The configuration is well-organized with clear separation between common, server, and worker configs. The typo from the previous review has been fixed, and the duration formats align with the custom Duration type implementation.
internal/worker/tasks/messages/config.go (1)
5-17: LGTM! Clean configuration types.The configuration structs are well-defined and consistent with the devices config pattern. Proper use of time.Duration for all timing-related fields.
internal/sms-gateway/modules/messages/module.go (2)
13-13: LGTM! Correct logger pattern.The switch to
logger.WithNamedLogger("messages")follows the established pattern for module-scoped loggers.Based on learnings
19-20: LGTM! Provider updates align with worker mode.The provider updates (NewRepository exported, newHashingWorker replacing newHashingTask) align with the PR's introduction of worker mode and proper separation of concerns.
internal/worker/locker/locker.go (1)
8-13: LGTM! Clean locking abstraction.The Locker interface provides a simple, clean abstraction for distributed locking with proper context support for cancellation and timeouts. The sentinel error follows Go conventions.
internal/sms-gateway/modules/messages/repository.go (4)
20-28: LGTM! Standard public type pattern.The promotion to a public
Repositorytype with a standard constructor follows Go best practices and aligns with the pattern used in other modules.
133-144: LGTM! Comprehensive duplicate key detection.The dual check for both
gorm.ErrDuplicatedKeyandmysql.IsDuplicateKeyViolationensures robust duplicate detection across different error paths, and the enhanced error wrapping improves debuggability.
162-165: LGTM! More precise MessageRecipient targeting.The composite WHERE clause using both
message_idandphone_numberensures updates target the correct recipient records, improving correctness.
193-200: LGTM! Context-aware cleanup implementation.The method properly uses context, returns rows affected for observability, and correctly filters non-pending messages within the specified time range.
internal/sms-gateway/modules/devices/service.go (5)
14-23: LGTM! Proper encapsulation with private fields.The Service struct follows Go best practices by using private fields and including the cache as a managed dependency.
25-41: LGTM! Standard constructor pattern with cache initialization.The constructor properly initializes all dependencies including the new cache layer.
84-97: LGTM! Proper cache-aside pattern implementation.The method correctly implements the cache-aside pattern: check cache first, fall back to repository on miss, populate cache on success. Logging cache failures without propagating them ensures the cache remains optional and doesn't break the critical path.
99-112: LGTM! Cache invalidation before update.The method correctly invalidates the cache before updating the repository, preventing stale cache entries. Logging cache errors without failing the update is appropriate.
138-163: LGTM! Proper cache invalidation during bulk removal.The method correctly fetches devices first to invalidate their cache entries before removing them from the repository. The early return optimization and per-device error logging are well-implemented.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
internal/worker/executor/service.go (4)
14-36: Add state management to Service.The Service struct lacks a mutex and started flag to guard against concurrent or repeated Start/Stop calls, which can cause duplicate goroutines or double-close panics.
Add state management fields:
type Service struct { tasks []PeriodicTask locker locker.Locker + mu sync.Mutex + started bool stopChan chan struct{} wg sync.WaitGroup metrics *metrics logger *zap.Logger }
38-64: Guard Start against multiple calls.Calling Start() multiple times will launch duplicate task goroutines without the state guard suggested in the previous comment.
87-96: Fix lock handling: downgrade contention logging and ensure release on panic.Lock acquisition failures should be logged at debug/info level since contention is expected. Additionally, lock release should use defer to ensure it happens even if task.Run panics.
120-125: Protect against double-close panic.Calling Stop() twice will panic since closing an already-closed channel is invalid. This requires the state management suggested in the first comment.
📜 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 (9)
go.mod(6 hunks)internal/worker/app.go(1 hunks)internal/worker/config/config.go(1 hunks)internal/worker/config/module.go(1 hunks)internal/worker/executor/metrics.go(1 hunks)internal/worker/executor/module.go(1 hunks)internal/worker/executor/service.go(1 hunks)internal/worker/server/config.go(1 hunks)internal/worker/server/module.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/worker/config/module.go
- internal/worker/config/config.go
- internal/worker/app.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T11:12:35.670Z
Learnt from: capcom6
PR: android-sms-gateway/server#187
File: internal/worker/executor/service.go:64-75
Timestamp: 2025-10-28T11:12:35.670Z
Learning: As of Go 1.23, unreferenced unstopped timers created by time.After can be garbage collected. There is no need to prefer time.NewTimer over time.After for garbage collection efficiency in Go 1.23+.
Applied to files:
internal/worker/executor/service.go
🧬 Code graph analysis (3)
internal/worker/server/module.go (2)
internal/worker/config/module.go (1)
Module(15-70)internal/worker/server/config.go (1)
Config(3-6)
internal/worker/executor/service.go (2)
internal/worker/executor/types.go (1)
PeriodicTask(8-12)internal/worker/locker/locker.go (1)
Locker(10-13)
internal/worker/executor/module.go (5)
internal/worker/locker/module.go (1)
Module(10-20)internal/worker/tasks/devices/module.go (1)
Module(10-22)internal/worker/tasks/messages/module.go (1)
Module(10-23)internal/worker/tasks/module.go (1)
Module(10-17)internal/worker/executor/service.go (2)
NewService(25-36)Service(14-23)
🔇 Additional comments (13)
internal/worker/server/config.go (1)
3-6: LGTM!Clean configuration structure with appropriate types for HTTP server settings.
internal/worker/executor/module.go (2)
10-29: LGTM!The FX module wiring is well-structured with proper lifecycle management and dependency injection patterns.
31-33: LGTM!Clean helper for group-based task registration in the FX container.
internal/worker/server/module.go (2)
15-21: LGTM!Configuration mapping is clean and correctly sets up proxy header handling.
22-34: LGTM!Prometheus metrics and middleware are properly configured with correct ordering.
internal/worker/executor/metrics.go (3)
10-42: LGTM!Well-structured metrics with appropriate Prometheus naming conventions and histogram bucket distribution for task duration tracking.
44-50: LGTM!Gauge manipulation methods are correctly implemented.
52-55: LGTM!Correctly records both task result counts and duration observations with appropriate labels.
go.mod (5)
3-3: Go version patch bump is safe.Bumping from 1.24.1 to 1.24.3 is a patch release and is recommended for bug fixes and security updates.
14-14: Validator version update is safe.The validator bump from v10.26.0 to v10.28.0 is a minor version update and should be compatible.
27-27: Standard library and toolchain updates are appropriate.The updates to
golang.org/x/crypto, and thegolang.org/x/*packages (mod, net, sync, sys, text, tools) represent routine maintenance and align with the Go 1.24.3 toolchain bump. These are generally safe.Also applies to: 99-106
30-30: YAML v3 correctly promoted to direct dependency.Promoting
gopkg.in/yaml.v3from an indirect to a direct dependency makes sense given the new worker configuration handling and config-related modules introduced in this PR.
49-50: Original review comment contains unsupported claims.The dependencies
clockwork,testify, andfxutilare indirect transitive dependencies ingo.mod, but they are not directly used by the worker, executor, or locker modules. The worker implementation usestime.NewTickerrather thanclockwork, locker is a simple interface definition, andfxutilis not imported anywhere in the codebase.testifyappears only in converter tests, unrelated to worker implementation. The claimed "alignment with worker task/executor/locker modules" lacks code evidence.Likely an incorrect or invalid review comment.
capcom6
force-pushed
the
worker/initial-implementation
branch
from
c930566 to
21d8a82
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
configs/config.example.yml (1)
36-36: Typo in comment already flagged.The typo "reasl-time" → "real-time" was already identified in a previous review.
go.mod (1)
11-12: Pseudo-version dependencies already flagged.The use of pseudo-versions for
go-core-fxmodules was already identified in a previous review, noting thatloggerhas a released versionv0.0.1available.internal/config/config.go (1)
75-78: Conflicting hashing intervals already flagged.The concern about potentially conflicting hashing intervals between gateway (
HashingIntervalSeconds: 60) and worker (7-day periodic hashing) was already raised in a previous review.internal/worker/tasks/devices/cleanup.go (2)
44-55: Consider injecting a time source for better testability.Calling
time.Now()directly makes unit testing difficult. The past review suggested adding anow func() time.Timefield initialized totime.Nowin the constructor.
8-15: Alias the repository import and rename the field for clarity.The past review flagged this: using an unaliased import creates ambiguity when both the package name
devicesand a field nameddevicesexist in the same scope.Apply this diff:
import ( "context" "fmt" "time" - "github.com/android-sms-gateway/server/internal/sms-gateway/modules/devices" + devicesrepo "github.com/android-sms-gateway/server/internal/sms-gateway/modules/devices" "github.com/android-sms-gateway/server/internal/worker/executor" "go.uber.org/zap" ) type cleanupTask struct { config CleanupConfig - devices *devices.Repository + repo *devicesrepo.Repository logger *zap.Logger }Also update the constructor and Run method accordingly (lines 20-31, 44-55).
internal/sms-gateway/modules/messages/service.go (1)
114-117: PII concern: raw phone numbers may linger in cache until hashed.As flagged in the past review, caching recipients before the worker hashes them in the DB means the cache may expose raw phone numbers until TTL expires or the hashing worker processes the entry.
Consider:
- Invalidate or refresh cache for the affected message ID after
HashProcessedsucceeds- Avoid caching recipients when
IsHashedis false- Cache a redacted view until hashing completes
internal/sms-gateway/modules/messages/repository.go (1)
174-191: CRITICAL: Unqualified columns will cause runtime SQL errors.The past review identified this critical issue, but it remains unfixed. Line 176 uses unqualified
contentandphone_numberin the UPDATE statement. Sincephone_numberexists only inmessage_recipients, the query will fail with "Unknown column" errors at runtime.Apply this diff to qualify all column references:
- rawSQL := "UPDATE `messages` `m`, `message_recipients` `r`\n" + - "SET `m`.`is_hashed` = true, `m`.`content` = SHA2(COALESCE(JSON_VALUE(`content`, '$.text'), JSON_VALUE(`content`, '$.data')), 256), `r`.`phone_number` = LEFT(SHA2(phone_number, 256), 16)\n" + + rawSQL := "UPDATE `messages` `m`, `message_recipients` `r`\n" + + "SET `m`.`is_hashed` = true, `m`.`content` = SHA2(COALESCE(JSON_VALUE(`m`.`content`, '$.text'), JSON_VALUE(`m`.`content`, '$.data')), 256), `r`.`phone_number` = LEFT(SHA2(`r`.`phone_number`, 256), 16)\n" + "WHERE `m`.`id` = `r`.`message_id` AND `m`.`is_hashed` = false AND `m`.`is_encrypted` = false AND `m`.`state` <> 'Pending'"
📜 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 (41)
cmd/sms-gateway/main.go(2 hunks)configs/config.example.yml(2 hunks)go.mod(5 hunks)internal/config/config.go(2 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(3 hunks)internal/sms-gateway/modules/cleaner/interfaces.go(0 hunks)internal/sms-gateway/modules/cleaner/module.go(0 hunks)internal/sms-gateway/modules/cleaner/service.go(0 hunks)internal/sms-gateway/modules/devices/cache.go(1 hunks)internal/sms-gateway/modules/devices/module.go(1 hunks)internal/sms-gateway/modules/devices/repository.go(7 hunks)internal/sms-gateway/modules/devices/service.go(3 hunks)internal/sms-gateway/modules/messages/config.go(1 hunks)internal/sms-gateway/modules/messages/module.go(1 hunks)internal/sms-gateway/modules/messages/repository.go(6 hunks)internal/sms-gateway/modules/messages/service.go(4 hunks)internal/sms-gateway/modules/messages/tasks.go(0 hunks)internal/sms-gateway/modules/messages/workers.go(1 hunks)internal/worker/app.go(1 hunks)internal/worker/config/config.go(1 hunks)internal/worker/config/module.go(1 hunks)internal/worker/config/types.go(1 hunks)internal/worker/executor/metrics.go(1 hunks)internal/worker/executor/module.go(1 hunks)internal/worker/executor/service.go(1 hunks)internal/worker/executor/types.go(1 hunks)internal/worker/locker/locker.go(1 hunks)internal/worker/locker/module.go(1 hunks)internal/worker/locker/mysql.go(1 hunks)internal/worker/server/config.go(1 hunks)internal/worker/server/module.go(1 hunks)internal/worker/tasks/devices/cleanup.go(1 hunks)internal/worker/tasks/devices/config.go(1 hunks)internal/worker/tasks/devices/module.go(1 hunks)internal/worker/tasks/messages/cleanup.go(1 hunks)internal/worker/tasks/messages/config.go(1 hunks)internal/worker/tasks/messages/hashing.go(1 hunks)internal/worker/tasks/messages/module.go(1 hunks)internal/worker/tasks/module.go(1 hunks)pkg/mysql/errors.go(1 hunks)
💤 Files with no reviewable changes (4)
- internal/sms-gateway/modules/cleaner/module.go
- internal/sms-gateway/modules/messages/tasks.go
- internal/sms-gateway/modules/cleaner/service.go
- internal/sms-gateway/modules/cleaner/interfaces.go
🚧 Files skipped from review as they are similar to previous changes (17)
- internal/worker/tasks/module.go
- internal/worker/tasks/messages/module.go
- internal/worker/locker/module.go
- cmd/sms-gateway/main.go
- internal/worker/tasks/devices/config.go
- internal/worker/tasks/devices/module.go
- internal/worker/tasks/messages/cleanup.go
- pkg/mysql/errors.go
- internal/worker/config/module.go
- internal/worker/server/config.go
- internal/worker/app.go
- internal/sms-gateway/modules/messages/module.go
- internal/sms-gateway/modules/devices/cache.go
- internal/worker/executor/service.go
- internal/sms-gateway/modules/messages/workers.go
- internal/worker/config/config.go
- internal/worker/executor/module.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
internal/sms-gateway/app.go
🧬 Code graph analysis (12)
internal/worker/tasks/devices/cleanup.go (3)
internal/worker/tasks/devices/config.go (1)
CleanupConfig(9-12)internal/worker/tasks/messages/cleanup.go (1)
NewCleanupTask(20-31)internal/worker/executor/types.go (1)
PeriodicTask(8-12)
internal/worker/locker/mysql.go (1)
internal/worker/locker/locker.go (2)
Locker(10-13)ErrLockNotAcquired(8-8)
internal/sms-gateway/modules/devices/repository.go (4)
internal/sms-gateway/modules/messages/repository.go (2)
NewRepository(24-28)Repository(20-22)internal/sms-gateway/modules/devices/repository_filter.go (1)
SelectFilter(9-9)internal/sms-gateway/modules/webhooks/repository_filter.go (1)
SelectFilter(5-5)internal/sms-gateway/models/models.go (1)
Device(25-36)
internal/sms-gateway/app.go (2)
internal/sms-gateway/modules/messages/module.go (1)
Module(10-25)internal/sms-gateway/modules/devices/module.go (1)
Module(8-18)
internal/worker/server/module.go (2)
internal/worker/config/module.go (1)
Module(15-70)internal/worker/server/config.go (1)
Config(3-6)
internal/sms-gateway/modules/messages/repository.go (4)
internal/sms-gateway/modules/devices/repository.go (2)
NewRepository(23-27)Repository(19-21)internal/sms-gateway/modules/messages/repository_filter.go (3)
MessagesSelectFilter(16-23)MessagesSelectOptions(25-36)MessagesOrder(7-7)internal/sms-gateway/modules/messages/models.go (2)
Message(35-55)MessageRecipient(111-117)pkg/mysql/errors.go (1)
IsDuplicateKeyViolation(9-15)
internal/worker/tasks/messages/hashing.go (2)
internal/worker/tasks/messages/config.go (1)
HashingConfig(10-12)internal/worker/executor/types.go (1)
PeriodicTask(8-12)
internal/config/module.go (1)
internal/config/config.go (2)
Messages(75-78)Tasks(63-65)
internal/sms-gateway/modules/devices/service.go (3)
internal/sms-gateway/modules/devices/repository.go (1)
Repository(19-21)internal/sms-gateway/modules/db/module.go (1)
IDGen(10-10)internal/sms-gateway/modules/devices/repository_filter.go (1)
WithToken(17-21)
internal/sms-gateway/modules/devices/module.go (3)
internal/sms-gateway/app.go (1)
Module(33-56)internal/sms-gateway/modules/devices/repository.go (1)
NewRepository(23-27)internal/sms-gateway/modules/devices/service.go (1)
NewService(25-41)
internal/sms-gateway/modules/messages/service.go (1)
internal/sms-gateway/modules/messages/repository.go (1)
Repository(20-22)
internal/worker/tasks/messages/config.go (2)
internal/worker/tasks/devices/config.go (1)
CleanupConfig(9-12)internal/worker/config/types.go (1)
Duration(11-11)
⏰ 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: Lint
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (40)
internal/worker/locker/mysql.go (3)
63-86: LGTM!The
ReleaseLockimplementation correctly:
- Retrieves and removes the pinned connection from the map under mutex protection
- Always closes the connection (line 77) regardless of the RELEASE_LOCK result
- Returns appropriate errors for missing connections or failed releases
The ordering ensures the connection is cleaned up even if RELEASE_LOCK fails, preventing connection leaks.
88-88: LGTM!Compile-time interface assertion ensures type safety.
32-60: <!-- [scratchpad_start]
[task review_file_2/2 review_segment_2/4 verification_3/3]
ReleaseLock cleanup verification[observations]
- delete(m.conns, key) found at line 68 in mysql.go
- This is likely in the ReleaseLock implementation
- First search had no output (regex might have failed), but delete command confirms cleanup exists
- Map entry IS being deleted properly
[analysis]
- Lifecycle confirmed:
- AcquireLock: stores conn in map
- execute() runs with lock held
- ReleaseLock: closes conn AND deletes from map
- Loop continues, on next ticker AcquireLock is called
- Map should be empty on re-entry (no defensive cleanup triggered)
- Defensive cleanup at lines 53-55 should be unreachable in normal operation
- However, defensive cleanup is still a safeguard for:
- Caller bugs (acquiring twice without release)
- Race conditions
- Concurrent key acquisition attempts
- This is DEFENSIVE programming, not a sign of a bug
[conclusion]
- Original review comment's concern about "multiple times for same key without Release" is MISPLACED
- Lock is properly acquired and released in correct sequence
- Defensive cleanup is just a safety measure, not indicating a real problem
- Code is correct and well-designed
[done]
Verification complete: no issues found, original review was too cautious
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deployments/docker-compose/docker-compose.yml (1)
36-44: Consider using YAML anchors to reduce duplication.The environment variables for the worker service are identical to those of the backend service. Consider using YAML anchors to avoid duplication and improve maintainability.
Example refactor:
+x-common-env: &common-env + DEBUG: "" + CONFIG_PATH: config.yml + HTTP__LISTEN: 0.0.0.0:3000 + DATABASE__HOST: db + DATABASE__PORT: 3306 + DATABASE__USER: sms + DATABASE__PASSWORD: sms + DATABASE__DATABASE: sms + services: backend: ... - environment: - - DEBUG= - - CONFIG_PATH=config.yml - ... + environment: *common-env worker: ... - environment: - - DEBUG= - - CONFIG_PATH=config.yml - ... + environment: *common-env
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
build/package/Dockerfile.dev(0 hunks)deployments/docker-compose-proxy/docker-compose.yml(3 hunks)deployments/docker-compose/.env(0 hunks)deployments/docker-compose/docker-compose.dev.yml(0 hunks)deployments/docker-compose/docker-compose.yml(3 hunks)internal/sms-gateway/app.go(3 hunks)internal/sms-gateway/handlers/3rdparty.go(2 hunks)internal/sms-gateway/handlers/health.go(5 hunks)internal/sms-gateway/handlers/module.go(1 hunks)internal/sms-gateway/handlers/root.go(2 hunks)internal/sms-gateway/modules/db/health.go(1 hunks)internal/sms-gateway/modules/db/module.go(1 hunks)internal/worker/app.go(1 hunks)internal/worker/server/module.go(1 hunks)pkg/health/module.go(1 hunks)pkg/health/service.go(2 hunks)
💤 Files with no reviewable changes (3)
- deployments/docker-compose/.env
- build/package/Dockerfile.dev
- deployments/docker-compose/docker-compose.dev.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/worker/app.go
- internal/worker/server/module.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
pkg/health/module.gointernal/sms-gateway/app.go
🧬 Code graph analysis (7)
internal/sms-gateway/handlers/module.go (1)
internal/sms-gateway/handlers/health.go (1)
NewHealthHandler(19-30)
pkg/health/service.go (1)
pkg/health/types.go (1)
HealthProvider(66-72)
pkg/health/module.go (5)
internal/sms-gateway/app.go (1)
Module(33-56)internal/sms-gateway/handlers/module.go (1)
Module(15-39)internal/sms-gateway/modules/db/module.go (1)
Module(12-22)pkg/health/health.go (1)
NewHealth(11-13)pkg/health/service.go (1)
NewService(15-21)
internal/sms-gateway/app.go (2)
pkg/health/module.go (1)
Module(8-20)internal/sms-gateway/modules/devices/module.go (1)
Module(8-18)
internal/sms-gateway/handlers/root.go (2)
internal/sms-gateway/handlers/health.go (1)
HealthHandler(13-17)internal/sms-gateway/handlers/config.go (1)
Config(3-11)
internal/sms-gateway/handlers/3rdparty.go (1)
internal/sms-gateway/handlers/health.go (1)
HealthHandler(13-17)
internal/sms-gateway/handlers/health.go (2)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)pkg/health/service.go (1)
Service(9-13)
⏰ 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 (20)
deployments/docker-compose-proxy/docker-compose.yml (2)
58-70: Verify worker service has all required environment configuration.The worker service is missing
GATEWAY__PRIVATE_TOKEN, which the backend service includes. Verify whether the worker mode requires this token for API authentication or if it's intentionally omitted because the worker doesn't expose an HTTP API.If the worker needs this token, apply this diff:
worker: image: ghcr.io/android-sms-gateway/server:latest command: ["/app/app", "worker"] environment: - CONFIG_PATH=config.yml - DATABASE__PASSWORD=${MYSQL_PASSWORD} + - GATEWAY__PRIVATE_TOKEN=${GATEWAY_PRIVATE_TOKEN} configs: - source: config-yml target: /app/config.yml restart: unless-stopped depends_on: db: condition: service_healthy
83-83: Verify simplified healthcheck is sufficient for MariaDB.The healthcheck was simplified from a multi-line block (not shown) to a single-line format. Confirm that the new healthcheck reliably detects MariaDB availability and that the timeout and retry settings (5s timeout, 10 retries) remain appropriate.
internal/sms-gateway/app.go (4)
23-56: LGTM! Module integration updates are consistent.The health module integration and updated module wiring (converting static modules to factory function calls) align well with the broader architectural changes in this PR.
58-65: Logger wiring update is correct.The migration to
logger.WithFxDefaultLogger()is consistent with the new go-core-fx/logger pattern adopted across the codebase.
79-117: Start function correctly updated.The cleaner service goroutine has been removed consistently with the
StartParamschanges. The remaining background tasks (messages, push, server) are properly coordinated via context and waitgroup.
67-77: Cleaner functionality has been properly migrated to worker subsystem.Verification confirms the
CleanerServiceremoval is correct. Both devices and messages cleanup tasks have been successfully implemented as periodic workers in the worker subsystem:
internal/worker/tasks/devices/cleanup.go: ImplementscleanupTaskas anexecutor.PeriodicTaskinternal/worker/tasks/messages/cleanup.go: ImplementscleanupTaskas anexecutor.PeriodicTask- Both tasks are registered via
fx.Providewithexecutor.AsWorkerTaskin their respective modules- No remaining references to
CleanerServiceexist in the codebaseinternal/sms-gateway/modules/db/module.go (1)
1-22: LGTM! Health import path updated correctly.The health module import path has been updated to reflect its new location in
pkg/health. The usage pattern remains unchanged.internal/sms-gateway/modules/db/health.go (1)
1-60: LGTM! Health import path updated correctly.The health module import path has been updated consistently with the module relocation to
pkg/health.pkg/health/module.go (1)
8-20: Logger and DI wiring correctly migrated.The migration from manual zap logger decoration to
logger.WithNamedLogger("health")and the addition offx.AnnotatewithParamTagsfor group injection are correct implementations of the new patterns. Based on learnings, this creates a properly scoped logger for the health module.internal/sms-gateway/handlers/module.go (1)
26-38: LGTM! Constructor export is consistent.Changing to
NewHealthHandler(exported) is consistent with theHealthHandlertype now being public API.internal/sms-gateway/handlers/root.go (1)
11-59: LGTM! Type updates are consistent.The field and parameter type updates from
*healthHandlerto*HealthHandlerare correct and consistent with the type being exported.internal/sms-gateway/handlers/3rdparty.go (1)
18-81: LGTM! Type updates are consistent.The field and parameter type updates from
*healthHandlerto*HealthHandlerin bothThirdPartyHandlerParamsandthirdPartyHandlerare correct and consistent with the type being exported.pkg/health/service.go (2)
9-21: LGTM! Constructor signature updated correctly.The migration from fx.In-based
ServiceParamsto explicit parameters is well-executed. The field rename fromhealthProviderstoproviderssimplifies the code, and the constructor signature properly aligns with thefx.Annotatewiring inmodule.go.
23-57: Loop iteration correctly updated.The loop at line 28 has been correctly updated to use
s.providersfollowing the field rename.deployments/docker-compose/docker-compose.yml (3)
1-1: LGTM: Project name addition.Adding a top-level project name helps organize resources and is a good practice for Docker Compose configurations.
23-26: LGTM: Health-aware service dependency.The health check condition ensures the backend waits for the database to be ready before starting, preventing connection errors during initialization.
35-46: Worker HTTP server is necessary and appropriately configured.The worker service exposes two HTTP endpoints:
- Health checks via
HealthHandler(used by orchestration for service availability)- Prometheus metrics at
/metrics(enables monitoring of task execution: active task count, task results, and task duration)The
HTTP__LISTEN=0.0.0.0:3000and port3001:3000configuration correctly supports both operational health checks and observability. No changes are needed.Likely an incorrect or invalid review comment.
internal/sms-gateway/handlers/health.go (3)
7-7: LGTM: Health package promotion and type export.The import path change from
modules/healthtopkg/healthand the export of theHealthHandlertype align with the PR's objective of promoting types to exported APIs for broader reuse across the codebase.Also applies to: 13-17
41-43: LGTM: Method receivers updated consistently.The method receivers have been correctly updated from
*healthHandlerto*HealthHandlerfollowing the type export. The logic remains unchanged.Also applies to: 55-57, 68-70
19-30: Validator removal is safe.The
ValidateStructmethod inbase.Handlercontains a nil-check (if h.Validator != nilat line 45), so setting the Validator tonilwill not cause a nil pointer dereference. The validation logic is optional by design. Health endpoints don't require input validation and won't call validation methods anyway.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/worker/locker/mysql.go (1)
11-19: Consider adding a Close method for resource cleanup.The
mySQLLockerlacks aCloseorShutdownmethod to clean up unreleased locks and their pinned connections. While connections will eventually be cleaned up when the process exits, adding a cleanup method would:
- Enable graceful shutdown (iterate over
m.connsand close each)- Improve testability
- Follow resource management best practices
Example implementation:
// Close closes all remaining pinned connections. // Should be called during shutdown to clean up resources. func (m *mySQLLocker) Close() error { m.mu.Lock() defer m.mu.Unlock() for key, conn := range m.conns { if conn != nil { _ = conn.Close() } delete(m.conns, key) } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/worker/locker/locker.go(1 hunks)internal/worker/locker/mysql.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/worker/locker/locker.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/worker/locker/mysql.go (1)
internal/worker/locker/locker.go (2)
Locker(11-18)ErrLockNotAcquired(9-9)
⏰ 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 (3)
internal/worker/locker/mysql.go (3)
11-31: Previous feedback addressed: timeout now uses idiomatic time.Duration.The change from
uinttotime.Durationfor the timeout parameter resolves the previous documentation concern and makes the API more idiomatic. The constructor properly initializes all fields including the connection map.
33-62: Critical connection pooling issue from previous review is now resolved.The implementation correctly pins a dedicated
*sql.Connfor each lock and stores it in the map for use during release. The use ofm.timeout.Seconds()properly converts thetime.Durationto seconds as expected by MySQL'sGET_LOCK. Error paths properly close connections to prevent leaks.
64-88: ReleaseLock correctly uses pinned connection and ensures cleanup.The implementation properly retrieves the pinned connection for the key, executes
RELEASE_LOCKon that same connection, and always closes it (line 79) regardless of success or failure. This prevents connection leaks and satisfies the connection-scoped lock requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/worker/locker/mysql.go (3)
21-31: Consider documenting MySQL GET_LOCK timeout behavior.While
time.Durationis idiomatic and self-documenting, the comment could briefly mention how the timeout is used with MySQL'sGET_LOCK(e.g., that it's converted to seconds, and special values like 0 mean "no wait").Apply this diff to enhance documentation:
-// NewMySQLLocker creates a new MySQL-based distributed locker. +// NewMySQLLocker creates a new MySQL-based distributed locker. +// The timeout is passed to MySQL's GET_LOCK function (converted to seconds). +// A timeout of 0 returns immediately if the lock cannot be acquired. func NewMySQLLocker(db *sql.DB, prefix string, timeout time.Duration) Locker {
53-59: Consider logging the orphaned connection case for debugging.The defensive cleanup at lines 54-57 handles orphaned connections (e.g., from panics or caller bugs). While this makes the locker robust, silently closing a pre-existing connection could mask bugs.
Consider adding a log statement when an orphaned connection is found:
m.mu.Lock() // Should not exist; if it does, close previous to avoid leaks. if prev, ok := m.conns[key]; ok && prev != nil { + // This indicates a lock was acquired but not properly released. + // Log for debugging purposes. _ = prev.Close() } m.conns[key] = conn m.mu.Unlock()Note: This would require passing a logger to the locker, so defer if not already available in your DI setup.
90-102: Document that MySQL auto-releases locks on connection close.The
Close()implementation is correct—it closes all connections without explicitly callingRELEASE_LOCK. MySQL automatically releases locks when a connection closes, but documenting this behavior would clarify the design choice.Apply this diff to enhance documentation:
-// Close closes all remaining pinned connections. -// Should be called during shutdown to clean up resources. +// Close closes all remaining pinned connections. +// Locks are automatically released by MySQL when connections close. +// Should be called during shutdown to clean up resources. func (m *mySQLLocker) Close() error {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/worker/locker/locker.go(1 hunks)internal/worker/locker/module.go(1 hunks)internal/worker/locker/mysql.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/worker/locker/module.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/worker/locker/mysql.go (1)
internal/worker/locker/locker.go (2)
Locker(11-21)ErrLockNotAcquired(9-9)
⏰ 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)
internal/worker/locker/locker.go (1)
8-21: LGTM! Interface and documentation are well-designed.The Locker interface is clean, the documentation clearly explains the contract, and the previous review feedback has been properly addressed.
internal/worker/locker/mysql.go (1)
64-88: LGTM! ReleaseLock correctly manages connection lifecycle.The implementation properly retrieves the pinned connection, always closes it (even on errors), and provides appropriate error handling.
capcom6
force-pushed
the
worker/initial-implementation
branch
from
f67c1a6 to
c55b04f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
deployments/docker-swarm-terraform/worker.tf (1)
20-33: Redundant Prometheus labels defined at both container and service levels.The same labels (
prometheus.io/scrape,prometheus.io/port,prometheus.io/job) are defined incontainer_spec(lines 20–33) and again at the service level (lines 51–64) with identical values. Consider removing duplication: service-level labels should suffice for Prometheus discovery, unless container-level labels serve a specific purpose in your infrastructure.Also applies to: 51-64
deployments/docker-compose/docker-compose.yml (1)
28-49: Consider YAML anchors/aliases to reduce duplication.Backend and worker have nearly identical build configs and environment variables. While not critical for local dev compose, using YAML anchors could improve maintainability:
x-build-config: &build-config context: ../.. dockerfile: ./build/package/Dockerfile args: - APP=sms-gateway x-db-env: &db-env - DEBUG= - CONFIG_PATH=config.yml - HTTP__LISTEN=0.0.0.0:3000 - DATABASE__HOST=db # ... other common vars backend: build: *build-config environment: *db-env worker: build: *build-config environment: *db-envThis reduces maintenance burden if build or env changes are needed.
Also applies to: 3-26
internal/worker/locker/mysql.go (1)
44-44: Consider explicit integer conversion for MySQL GET_LOCK timeout.
m.timeout.Seconds()returnsfloat64, which MySQL will implicitly convert. For clarity and to avoid potential precision issues, consider casting toint:- if err := conn.QueryRowContext(ctx, "SELECT GET_LOCK(?, ?)", name, m.timeout.Seconds()).Scan(&res); err != nil { + if err := conn.QueryRowContext(ctx, "SELECT GET_LOCK(?, ?)", name, int(m.timeout.Seconds())).Scan(&res); err != nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
build/package/Dockerfile.dev(0 hunks)deployments/docker-compose-proxy/docker-compose.yml(3 hunks)deployments/docker-compose/.env(0 hunks)deployments/docker-compose/docker-compose.dev.yml(0 hunks)deployments/docker-compose/docker-compose.yml(3 hunks)deployments/docker-swarm-terraform/worker.tf(1 hunks)internal/worker/locker/locker.go(1 hunks)internal/worker/locker/module.go(1 hunks)internal/worker/locker/mysql.go(1 hunks)
💤 Files with no reviewable changes (3)
- deployments/docker-compose/.env
- deployments/docker-compose/docker-compose.dev.yml
- build/package/Dockerfile.dev
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/worker/locker/module.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/worker/locker/mysql.go (1)
internal/worker/locker/locker.go (2)
Locker(11-21)ErrLockNotAcquired(9-9)
⏰ 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 (12)
deployments/docker-swarm-terraform/worker.tf (2)
41-48: Memory reservation is hard-coded while limit is variable; verify alignment.The memory limit uses
var.memory-limit(line 42), but reservation is hard-coded to 32 MB (line 46). For consistency and operational clarity, consider whether the reservation should also be variable, scale proportionally, or be documented as a fixed minimum. This may impact scheduling behavior under load.
20-32: Prometheus port 3000 is correctly aligned across all deployment configurations.Verification confirms:
- Helm Chart: Worker container listens on port 3000 via
HTTP__LISTEN: "0.0.0.0:{{ .Values.service.port }}"(values.yaml line 31)- Docker-compose: Port mapping
"3000:3000"andlisten: 0.0.0.0:3000in both compose files- Worker.tf: Prometheus labels correctly target port 3000, matching the worker's actual listening port across all environments
No action needed.
deployments/docker-compose-proxy/docker-compose.yml (2)
45-45: Verify image registry change from capcom6 to ghcr.io.The backend (line 45) and worker (line 59) images now use
ghcr.io/android-sms-gateway/server:latestinstead ofcapcom6/sms-gateway:latest. Confirm:
- This registry change is intentional and documented in the PR.
- Old image is deprecated or a migration plan exists.
- All environments (proxy, compose, swarm) use consistent registries.
Ensure no downstream tooling (CI/CD, image scanning) is affected by this registry shift.
Also applies to: 59-59
58-71: Worker missingGATEWAY__PRIVATE_TOKENenvironment variable present in backend.The backend service includes
GATEWAY__PRIVATE_TOKEN(line 49), but the worker service does not (lines 61–63). If the worker processes gateway-related tasks (e.g., hashing, device management), it may need this token. If not, this is intentional and should be documented.Verify that the worker's environment variables are sufficient for its role and task execution.
deployments/docker-compose/docker-compose.yml (4)
1-1: LGTM.Adding the compose project name improves container naming and isolation.
23-26: LGTM.Backend now explicitly waits for db health, ensuring proper startup ordering and reducing race conditions on boot.
28-49: LGTM on worker service structure.The worker service is well-defined with appropriate isolation (port 3001), config mounting, and health-dependent startup. Port mapping (3001 → 3000) avoids local dev conflicts with backend.
However, verify environment variable alignment: the worker is missing
GATEWAY__PRIVATE_TOKEN(present in backend line 11–18). If worker processes gateway tasks, this may be needed; if not, document why it's omitted.
64-64: DB restart and healthcheck improvements look good.The quote-style update (line 64) and healthcheck configuration (lines 66–70) are consistent with backend/worker. The healthcheck intervals and retry logic are appropriate for local dev.
Also applies to: 54-70
internal/worker/locker/locker.go (1)
8-21: LGTM! Documentation addresses previous concerns.The interface and error are now properly documented. The contract is clear:
AcquireLockreturnsErrLockNotAcquiredon timeout,ReleaseLockhandles cleanup, andClosereleases all held locks. Well done addressing the previous review feedback.internal/worker/locker/mysql.go (3)
11-31: Excellent fix for connection-scoped lock issue.The connection-pinning approach with a per-key map (
conns map[string]*sql.Conn) correctly addresses the previous critical issue whereGET_LOCK/RELEASE_LOCKmust operate on the same connection. Usingtime.Durationfor the timeout is also clearer than the previousuint.
64-88: LGTM! Proper cleanup on release.The implementation correctly releases the lock on the same pinned connection and always closes the connection afterward (line 79), preventing leaks. Thread-safe map access is properly guarded.
90-102: LGTM! Proper resource cleanup.The
Closemethod correctly cleans up all pinned connections with proper mutex protection. This ensures clean shutdown and prevents connection leaks.
capcom6
force-pushed
the
worker/initial-implementation
branch
from
c55b04f to
1455213
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
deployments/docker-compose-proxy/docker-compose.yml (1)
58-70: Worker service configuration looks solid; consider adding resource limits.The new worker service is well-structured:
- Shares image and config with backend (appropriate)
- Depends on database health (correct)
- Runs with the worker subcommand
For production robustness, consider adding CPU and memory limits to prevent resource exhaustion if the worker performs heavy background processing:
worker: image: ghcr.io/android-sms-gateway/server:latest command: ["/app/app", "worker"] environment: - CONFIG_PATH=config.yml - DATABASE__PASSWORD=${MYSQL_PASSWORD} configs: - source: config-yml target: /app/config.yml restart: unless-stopped deploy: resources: limits: cpus: '1' memory: 512M depends_on: db: condition: service_healthydeployments/docker-compose/docker-compose.yml (1)
28-49: Consider using YAML anchors to reduce duplication between backend and worker services.The backend and worker services share identical
build,image,environment,volumes,restart, anddepends_onconfigurations. This duplication makes maintenance harder and increases the risk of configuration drift.Apply this refactor using YAML anchors and aliases to reduce duplication:
+x-common-config: &common-config + image: capcom6/sms-gateway + build: + context: ../.. + dockerfile: ./build/package/Dockerfile + args: + - APP=sms-gateway + environment: &common-env + - DEBUG= + - CONFIG_PATH=config.yml + - DATABASE__HOST=db + - DATABASE__PORT=3306 + - DATABASE__USER=sms + - DATABASE__PASSWORD=sms + - DATABASE__DATABASE=sms + volumes: + - ../../configs/config.yml:/app/config.yml:ro + restart: "unless-stopped" + depends_on: + db: + condition: service_healthy + services: backend: + <<: *common-config image: capcom6/sms-gateway - build: - context: ../.. - dockerfile: ./build/package/Dockerfile - args: - - APP=sms-gateway environment: + <<: *common-env - HTTP__LISTEN=0.0.0.0:3000 - - DEBUG= - - CONFIG_PATH=config.yml - - HTTP__LISTEN=0.0.0.0:3000 - - DATABASE__HOST=db - - DATABASE__PORT=3306 - - DATABASE__USER=sms - - DATABASE__PASSWORD=sms - - DATABASE__DATABASE=sms ports: - "3000:3000" - volumes: - - ../../configs/config.yml:/app/config.yml:ro - restart: "unless-stopped" - depends_on: - db: - condition: service_healthy worker: + <<: *common-config image: capcom6/sms-gateway - build: - context: ../.. - dockerfile: ./build/package/Dockerfile - args: - - APP=sms-gateway command: ["/app/app", "worker"] environment: + <<: *common-env - HTTP__LISTEN=0.0.0.0:3000 - - DEBUG= - - CONFIG_PATH=config.yml - - HTTP__LISTEN=0.0.0.0:3000 - - DATABASE__HOST=db - - DATABASE__PORT=3306 - - DATABASE__USER=sms - - DATABASE__PASSWORD=sms - - DATABASE__DATABASE=sms ports: - "3001:3000" - volumes: - - ../../configs/config.yml:/app/config.yml:ro - restart: "unless-stopped" - depends_on: - db: - condition: service_healthyNote: You can further simplify by extracting the
HTTP__LISTENenvironment variable if both services need it, or keep it service-specific if they differ.internal/worker/locker/mysql.go (1)
21-31: Consider validating the timeout parameter.The constructor accepts any
time.Durationvalue without validation. MySQL'sGET_LOCKinterprets zero as "try immediately" and negative values as "wait indefinitely". If these behaviors are not intended, consider adding validation.Apply this diff to add validation:
// NewMySQLLocker creates a new MySQL-based distributed locker. func NewMySQLLocker(db *sql.DB, prefix string, timeout time.Duration) Locker { + if timeout <= 0 { + panic("timeout must be positive") + } return &mySQLLocker{
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
build/package/Dockerfile.dev(0 hunks)deployments/docker-compose-proxy/docker-compose.yml(3 hunks)deployments/docker-compose/.env(0 hunks)deployments/docker-compose/docker-compose.dev.yml(0 hunks)deployments/docker-compose/docker-compose.yml(3 hunks)deployments/docker-swarm-terraform/worker.tf(1 hunks)internal/worker/locker/locker.go(1 hunks)internal/worker/locker/module.go(1 hunks)internal/worker/locker/mysql.go(1 hunks)
💤 Files with no reviewable changes (3)
- deployments/docker-compose/docker-compose.dev.yml
- build/package/Dockerfile.dev
- deployments/docker-compose/.env
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/worker/locker/module.go
- deployments/docker-swarm-terraform/worker.tf
- internal/worker/locker/locker.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/worker/locker/mysql.go (1)
internal/worker/locker/locker.go (2)
Locker(11-21)ErrLockNotAcquired(9-9)
⏰ 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)
deployments/docker-compose-proxy/docker-compose.yml (3)
37-42: Improved header formatting.The reproxy HEADER block is more readable with this indentation refactor. No semantic changes.
45-45: Verify backend image registry change.The backend image has migrated from
capcom6/sms-gateway:latesttoghcr.io/android-sms-gateway/server:latest. This is a registry/organization change that aligns with open-source best practices.Please confirm the new GHCR image contains both the gateway and worker binary at
/app/appand supports the expected command-line modes (gateway and worker).
83-83: Database healthcheck formatting is cleaner.The healthcheck command is now single-line; functionally equivalent to the prior multi-line format.
deployments/docker-compose/docker-compose.yml (2)
45-46: Clarify the purpose of the worker service's port exposure.The worker service exposes port 3001 and listens on
HTTP__LISTEN=0.0.0.0:3000, but if the worker is a background task executor (per the PR objectives), it likely shouldn't expose any HTTP ports.Verify whether the worker service should expose port 3001. If the worker doesn't need to listen on HTTP, consider:
- Removing the
portssection from the worker service, or- Clarifying its purpose if HTTP access is intentional (e.g., health checks, metrics).
35-35: No issues found — worker command is properly implemented.The codebase confirms the
workersubcommand is supported. Incmd/sms-gateway/main.go, the main entry point explicitly handles the "worker" command (lines 48–49) and routes toworker.Run(). Theinternal/workerpackage provides the complete implementation with config, executor, locker, server, and tasks modules. The docker-compose configuration is correct.internal/worker/locker/mysql.go (4)
11-19: LGTM: Struct design properly supports connection pinning.The struct includes all necessary fields for tracking per-key pinned connections with appropriate mutex protection.
64-88: LGTM: Correct release implementation with proper connection handling.The method correctly retrieves the pinned connection, issues
RELEASE_LOCKon it, and always closes the connection. Error handling covers both missing connections and failed releases.
90-102: LGTM: Proper cleanup on shutdown.The
Close()method correctly closes all remaining pinned connections and clears the map, preventing connection leaks during shutdown.
53-59: The silent close handling is intentional and correct—no changes needed.The connection closure at lines 54-57 is defensive programming for panic recovery. When a task execution panics (see
execute()recover block), the lock is not released, leaving a stale connection in the map. On the next acquisition attempt for the same key, the previous connection is silently closed to prevent leaks. This is the correct approach and doesn't warrant logging or errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
deployments/grafana/dashboards/worker.json (2)
360-363: Alert naming convention assumption.The "Active Worker Alerts" panel assumes worker-related alerts follow the
"Worker.*"naming pattern (line 360). Ensure that alerting rules define alerts with this naming convention, or adjust the regex pattern if alerts use a different format.Consider documenting the expected alert naming convention in a README or monitoring guide to maintain consistency with this dashboard.
35-46: Verify active task threshold values align with expected workload.The gauge for "Active Tasks" uses thresholds of 0 (green) and 10 (red). This assumes that 10+ concurrent tasks indicates unhealthy state. Adjust these thresholds based on actual capacity and expected workload characteristics.
deployments/prometheus/alerts/worker-alerts.yml (2)
5-5: Validate alert thresholds against expected worker capacity.The thresholds are set as follows:
- WorkerTaskFailure: > 1 error/min is a fairly low threshold. Consider whether transient errors should trigger a critical alert.
- LongRunningTask: 95th percentile > 30s is reasonable but depends on task complexity; confirm this aligns with expected workload.
- HighActiveTasks: > 10 active tasks may need tuning based on worker concurrency and resource limits.
Ensure these thresholds have been calibrated against observed production workloads or expected load patterns.
Also applies to: 14-14, 23-23
4-30: Consider alert coverage for critical worker conditions.The current alerts cover task failures, slow tasks, and queue depth. Consider adding alerts for:
- Worker process health/availability (e.g., no metrics received for X minutes)
- Task retry/backoff activity
- Worker initialization or startup failures
These conditions may be important for operational visibility, depending on the importance of worker reliability in your system.
deployments/helm-chart/templates/deployment.yaml (1)
170-180: Minor: Config volume and mount logic duplicated between backend and worker.The conditional config volume (lines 170–180) and volumeMounts (lines 215–220) are duplicated between backend and worker deployments. While this is acceptable for Helm templates (readability over DRY), consider documenting the deployment pattern or extracting via Helm sub-charts if more deployments are added in future.
Also applies to: 215-220
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deployments/grafana/dashboards/worker.json(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)deployments/prometheus/alerts/worker-alerts.yml(1 hunks)scripts/test-helm-minikube.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T08:37:28.077Z
Learnt from: eznix86
Repo: android-sms-gateway/server PR: 172
File: deployments/kubernetes/sms-gateway-server/Chart.yaml:9-10
Timestamp: 2025-09-05T08:37:28.077Z
Learning: In the android-sms-gateway/server project, the Helm chart's appVersion in Chart.yaml is set to "latest" as a placeholder because CI automatically updates it with the actual version from git tags during the release process via the bump-chart-app-version.yaml workflow.
Applied to files:
scripts/test-helm-minikube.sh
⏰ 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 (5)
deployments/grafana/dashboards/worker.json (1)
76-80: Metrics are properly exported by the worker module.All three core Prometheus metrics referenced in the dashboard are correctly defined and exported in
internal/worker/executor/metrics.go:
worker_executor_active_tasks_total(Gauge)worker_executor_task_result_total(CounterVec)worker_executor_task_duration_seconds(HistogramVec with appropriate buckets)The metrics use proper Prometheus naming conventions (namespace_subsystem_name) and are auto-registered via
promauto. The dashboard can safely reference these metrics.deployments/prometheus/alerts/worker-alerts.yml (1)
1-30: Metrics are properly defined and instrumented.All three metrics referenced in the alert rules are confirmed to exist in
internal/worker/executor/metrics.go:
worker_executor_active_tasks_total(Gauge)worker_executor_task_result_total(CounterVec)worker_executor_task_duration_seconds(HistogramVec)These metrics are already in production use in existing Grafana dashboards, confirming they are actively emitted by the worker implementation. The alert rules are correctly configured.
deployments/helm-chart/templates/deployment.yaml (2)
135-137: ✓ Backend tolerations block properly added.The tolerations block follows the existing template pattern and allows operators to customize pod placement via values.
139-232: All design concerns are verified and well-founded.The review comment accurately identifies four legitimate inconsistencies between the backend and worker deployments:
Hard-coded replicas and port: Confirmed. Backend uses
{{ .Values.replicaCount }}and{{ .Values.service.port }}; worker uses1and3000respectively.Missing health probes: Confirmed. Backend defines livenessProbe, readinessProbe, and startupProbe; worker has none.
Missing GATEWAY configuration: Confirmed. Backend conditionally sets
GATEWAY__MODEand FCM/token credentials based on.Values.gateway.privateTokenand.Values.gateway.fcmCredentials; worker omits these entirely.No container ports exposure: Confirmed. Backend declares a
ports:block; worker does not.These differences warrant explicit design decisions: either document why the worker intentionally differs (non-scaling, mode-agnostic, internal-only), or align configurations for consistency.
scripts/test-helm-minikube.sh (1)
37-37: ✓ Test script correctly enforces fresh image pulls.Setting
image.pullPolicy=Alwaysensures the Minikube test environment always fetches the latest image, preventing stale cached images from masking deployment or code issues during tests.
capcom6
force-pushed
the
worker/initial-implementation
branch
from
8000501 to
9a35d3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
deployments/docker-compose/docker-compose.yml (2)
3-26: Backend service now has explicit build and health-based dependency.The restructuring with build block and health-based depends_on is correct. However, notice that the worker service (lines 28–52) is nearly identical except for command and port. Consider extracting common configuration using YAML anchors to reduce duplication.
Example refactor using anchors:
+x-service-base: &service-base + image: capcom6/sms-gateway + build: + context: ../.. + dockerfile: ./build/package/Dockerfile + args: + - APP=sms-gateway + environment: + - DEBUG= + - CONFIG_PATH=config.yml + - HTTP__LISTEN=0.0.0.0:3000 + - DATABASE__HOST=db + - DATABASE__PORT=3306 + - DATABASE__USER=sms + - DATABASE__PASSWORD=sms + - DATABASE__DATABASE=sms + volumes: + - ../../configs/config.yml:/app/config.yml:ro + restart: "unless-stopped" + depends_on: + db: + condition: service_healthy + backend: - image: capcom6/sms-gateway - build: - context: ../.. - dockerfile: ./build/package/Dockerfile - args: - - APP=sms-gateway - environment: - - DEBUG= - - CONFIG_PATH=config.yml - - HTTP__LISTEN=0.0.0.0:3000 - - DATABASE__HOST=db - - DATABASE__PORT=3306 - - DATABASE__USER=sms - - DATABASE__PASSWORD=sms - - DATABASE__DATABASE=sms + <<: *service-base ports: - "3000:3000" - volumes: - - ../../configs/config.yml:/app/config.yml:ro - restart: "unless-stopped" - depends_on: - db: - condition: service_healthy worker: - image: capcom6/sms-gateway - build: - context: ../.. - dockerfile: ./build/package/Dockerfile - args: - - APP=sms-gateway + <<: *service-base command: ["/app/app", "worker"] - environment: - - DEBUG= - - CONFIG_PATH=config.yml - - HTTP__LISTEN=0.0.0.0:3000 - - DATABASE__HOST=db - - DATABASE__PORT=3306 - - DATABASE__USER=sms - - DATABASE__PASSWORD=sms - - DATABASE__DATABASE=sms ports: - "3001:3000" - volumes: - - ../../configs/config.yml:/app/config.yml:ro - restart: "unless-stopped" - depends_on: - db: - condition: service_healthy
28-52: Note: HTTP__LISTEN port configuration may cause confusion.Both backend and worker services set
HTTP__LISTEN=0.0.0.0:3000(the internal port), but they're exposed on different host ports (3000 and 3001). This is correct from a networking standpoint (internal port is the same, host ports differ), but the duplicate configuration makes maintenance harder. The YAML anchor refactor suggested above would help clarify this relationship.deployments/docker-swarm-terraform/worker.tf (1)
20-33: Remove duplicate Prometheus labels at container level.Prometheus labels appear at both the container_spec level (lines 21–32) and service level (lines 52–63). In Docker Swarm, Prometheus service discovery operates at the service level, making the container-level labels redundant and potentially confusing for future maintainers.
Unless the container-level labels serve a specific purpose (e.g., per-container scrape configuration), remove lines 20–33 to reduce duplication and clarify the configuration intent.
secrets { secret_id = docker_secret.config.id secret_name = docker_secret.config.name file_name = "/app/config.yml" file_mode = 384 file_uid = 405 file_gid = 100 } - - #region Prometheus support - labels { - label = "prometheus.io/scrape" - value = "true" - } - labels { - label = "prometheus.io/port" - value = "3000" - } - labels { - label = "prometheus.io/job" - value = "worker" - } - #endregion }Also applies to: 51-64
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
build/package/Dockerfile.dev(0 hunks)deployments/docker-compose-proxy/docker-compose.yml(3 hunks)deployments/docker-compose/.env(0 hunks)deployments/docker-compose/docker-compose.dev.yml(0 hunks)deployments/docker-compose/docker-compose.yml(3 hunks)deployments/docker-swarm-terraform/worker.tf(1 hunks)deployments/grafana/dashboards/worker.json(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)deployments/prometheus/alerts/worker-alerts.yml(1 hunks)scripts/test-helm-minikube.sh(1 hunks)
💤 Files with no reviewable changes (3)
- build/package/Dockerfile.dev
- deployments/docker-compose/.env
- deployments/docker-compose/docker-compose.dev.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/test-helm-minikube.sh
- deployments/grafana/dashboards/worker.json
⏰ 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 (12)
deployments/docker-compose/docker-compose.yml (3)
1-1: Added top-level compose name.Good addition for better project identification and container naming.
28-52: New worker service added with correct structure.The worker service is properly configured with health-based dependencies. Verify that the worker command syntax on line 35 is correct for the application's CLI.
Does your application binary (
/app/app) accept aworkersubcommand? If the actual CLI flag differs (e.g.,--mode=workeror similar), please update line 35 accordingly.
23-26: Good: Service restart and health-based dependencies are now consistent.All services use quoted
"unless-stopped"restart policy and proper health-check conditions for depends_on. This ensures robust startup ordering.Also applies to: 49-52, 64-64
deployments/docker-compose-proxy/docker-compose.yml (4)
36-42: Security headers formatting updated.The HEADER block has been reflowed for readability without changing content. The security headers are appropriate for this deployment (SAMEORIGIN framing policy, XSS protection, MIME-type sniffing prevention, HSTS, referrer policy, and CSP).
45-45: Image registry migrated to GHCR.Backend image successfully updated from Docker Hub (
capcom6/sms-gateway:latest) to GitHub Container Registry (ghcr.io/android-sms-gateway/server:latest). This aligns with the worker mode introduction and centralized artifact management.
58-70: Verify worker service environment configuration.The worker service has been added with appropriate structure (shared image, config mount, restart policy, and db health check dependency). However, the worker service is missing the
GATEWAY__PRIVATE_TOKENenvironment variable present in the backend service.Confirm whether this omission is intentional. If the worker process does not serve the gateway API and therefore does not require the token, this is correct. However, if any worker tasks require authenticated access to external services or the gateway's own endpoints, the token may be needed.
Additionally, verify that the shared
config.ymlcontains all necessary configuration sections for the worker (e.g., database connection, task queues, job concurrency settings, or any worker-specific subsystems).
83-83: MariaDB healthcheck format consolidated.The healthcheck has been refactored from multiline to single-line format using the standard
mysqladmin pingcommand, which is appropriate and functional.deployments/helm-chart/templates/deployment.yaml (4)
135-137: Tolerations block formatting looks good.The addition of the tolerations block to the backend Deployment follows the existing conditional pattern and aligns with the worker Deployment below.
148-148: Verify that worker replicas should be hardcoded to 1.The worker Deployment has
replicas: 1hardcoded. If this is by design (to avoid duplicate/concurrent task processing), it should be documented. However, if scalability or operational flexibility is needed, consider making this configurable via values.
191-192: WorkerHTTP__LISTENis hardcoded while backend uses configurable port.The worker container hardcodes
HTTP__LISTEN: "0.0.0.0:3000", whereas the backend uses.Values.service.port(line 54). This inconsistency raises questions:
- Does the worker expose HTTP endpoints? If not, why set
HTTP__LISTENat all?- If it does, should the port be configurable like the backend, or is 3000 intentionally fixed?
Please clarify the intent and adjust for consistency.
181-220: Worker container is missing health probes.Unlike the backend Deployment (lines 94–119), the worker container has no
livenessProbe,readinessProbe, orstartupProbe. Verify this is intentional (e.g., worker doesn't expose HTTP health endpoints) or add appropriate probes for cluster observability and failure detection.deployments/prometheus/alerts/worker-alerts.yml (1)
4-6: Document alert threshold rationale and reconsider severity level.The WorkerTaskFailure alert is configured as critical with a threshold of > 1 error per minute. However, examining similar alerts across the codebase reveals an inconsistent pattern: error rate alerts typically use percentage-based thresholds (5-10% in http-alerts.yml, sse-alerts.yml) rather than absolute counts. Other absolute-count alerts are marked as warning severity (HighActiveTasks > 10, online-alerts.yml > 10).
No documentation or ADR justifying this specific threshold exists in the repository. Either:
- Lower severity to warning (matching pattern of other low-threshold alerts), or
- Increase the threshold if 1 error/minute is expected under normal load, or
- Add documentation explaining why this threshold and severity are appropriate for your worker task volume
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
deployments/docker-swarm-terraform/worker.tf (1)
25-33: 🔴 CRITICAL (Duplicate): Resolve memory reservation vs. limit validation.This issue was flagged in a previous review but remains unresolved. Line 31 hardcodes a 32 MB memory reservation, but
var.memory-limit(line 27) has no validation. Ifvar.memory-limitis set below 32 MB, Docker Swarm will reject the deployment because reservation cannot exceed the limit.To fix this, either:
- Add a
validationblock to enforcevar.memory-limit >= 32 * 1024 * 1024and document the minimum, or- Compute the reservation dynamically (e.g., 1/4 of
var.memory-limitwith a 32 MB floor) while ensuring it never exceeds the limit.Please apply the fix to prevent deployment failures. For example, using option 2:
reservation { - memory_bytes = 32 * 1024 * 1024 + memory_bytes = min(var.memory-limit / 4, 32 * 1024 * 1024) }Or add validation to the variable definition (if using Terraform 1.0+):
variable "memory-limit" { type = number default = 64 * 1024 * 1024 description = "Worker memory limit in bytes. Minimum: 32 MB." validation { condition = var.memory-limit >= 32 * 1024 * 1024 error_message = "memory-limit must be at least 32 MB (33554432 bytes)." } }
🧹 Nitpick comments (1)
internal/worker/executor/metrics.go (1)
23-42: Consider adding Help text to metrics.While not critical, adding Help text to each metric is a Prometheus best practice that improves observability and documentation. The Help field describes what each metric measures.
Example for the active tasks gauge:
activeTasksCounter: promauto.NewGauge(prometheus.GaugeOpts{ Namespace: "worker", Subsystem: "executor", Name: "active_tasks", + Help: "Number of tasks currently being executed", }),Similarly, add Help text to the taskResult and taskDuration metrics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deployments/docker-swarm-terraform/worker.tf(1 hunks)deployments/grafana/dashboards/worker.json(1 hunks)deployments/prometheus/alerts/worker-alerts.yml(1 hunks)internal/worker/executor/metrics.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deployments/grafana/dashboards/worker.json
⏰ 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 (5)
deployments/docker-swarm-terraform/worker.tf (1)
36-48: Verify Prometheus port configuration.Line 43 hardcodes port 3000 for Prometheus scraping. Confirm this matches the worker application's metrics endpoint port, or make it configurable via a variable to align with the gateway service configuration.
internal/worker/executor/metrics.go (3)
10-15: LGTM! Type-safe result enum.The custom type with constants provides type safety for task results and follows idiomatic Go patterns.
17-21: LGTM! Appropriate metric types.The metrics struct correctly uses Gauge for active task counting, CounterVec for result tracking, and HistogramVec for duration measurement.
44-55: LGTM! Methods correctly instrument metrics.The methods properly manipulate the Prometheus metrics with appropriate label values and conversions (e.g., duration to seconds).
deployments/prometheus/alerts/worker-alerts.yml (1)
1-30: All metric names are correctly defined and match the worker implementation.The three metrics referenced in the alerts are properly defined:
worker_executor_active_tasksis a Gauge (correct, without_totalsuffix)worker_executor_task_result_totalis a CounterVecworker_executor_task_duration_secondsis a HistogramVecAll alert expressions correctly reference these metrics with appropriate aggregations (including the
by (le)clause for histogram_quantile). The configuration is valid and ready.
capcom6
force-pushed
the
worker/initial-implementation
branch
from
929c29a to
e69e762
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
internal/config/config.go (1)
112-115: Hashing interval conflict between gateway and worker.As noted in a previous review, the gateway's
HashingIntervalSeconds: 60(60 seconds) conflicts with the worker's 168h (7 days) default. This can cause duplicate hashing work if both are enabled.The previous review suggested either:
- Set gateway default to 0 (disable gateway hashing in favor of worker)
- Align both values
This ensures only one component performs the hashing task.
internal/sms-gateway/modules/messages/workers.go (1)
33-46: Guard against non-positive hashing interval before starting the ticker.
time.NewTickerpanics when invoked with an interval ≤ 0. The new config path leavesHashingIntervalat zero whenever bothcfg.Tasks.Hashing.IntervalSecondsandcfg.Messages.HashingIntervalSecondsare unset, so a misconfigured deployment will crash the worker on startup. Bail out early and log instead of panicking.func (t *hashingWorker) Run(ctx context.Context) { t.logger.Info("Starting hashing task...") + if t.interval <= 0 { + t.logger.Warn("hashing worker disabled: non-positive interval", zap.Duration("interval", t.interval)) + return + } ticker := time.NewTicker(t.interval) defer ticker.Stop()go.mod (1)
11-12: Pin logger to released tag v0.0.1.
github.com/go-core-fx/loggerstill points to the pseudo-versionv0.0.0-20251028014216-c34d2fb15ca2, even thoughv0.0.1is available. Please switch to the released tag (or document why the pseudo-version is required) so the build isn’t tied to an unreleased commit.-github.com/go-core-fx/logger v0.0.0-20251028014216-c34d2fb15ca2 +github.com/go-core-fx/logger v0.0.1internal/worker/executor/service.go (2)
38-61: Guard Start against duplicate invocations.
Startcan be called multiple times, launching duplicate goroutines and reusing a closedstopChanafterStop, which immediately cancels new runs. Add a mutex-protectedstartedflag so subsequentStartcalls are no-ops (or error) once running.type Service struct { tasks []PeriodicTask locker locker.Locker stopChan chan struct{} wg sync.WaitGroup + mu sync.Mutex + started bool metrics *metrics logger *zap.Logger } func (s *Service) Start() error { + s.mu.Lock() + if s.started { + s.mu.Unlock() + return nil + } + s.started = true + s.mu.Unlock() + ctx, cancel := context.WithCancel(context.Background())
124-128: Make Stop idempotent to avoid closing an already-closed channel.Calling
Stoptwice (directly or via deferred cleanup) panics becausestopChanis closed unguarded. Protect the close with async.Once(or the same mutex used inStart) so repeatedStopcalls are safe.type Service struct { tasks []PeriodicTask locker locker.Locker stopChan chan struct{} wg sync.WaitGroup + stopOnce sync.Once @@ func (s *Service) Stop() error { - close(s.stopChan) - s.wg.Wait() + s.stopOnce.Do(func() { + close(s.stopChan) + }) + s.wg.Wait() return nil }internal/sms-gateway/modules/messages/repository.go (1)
174-191: Qualify columns in raw SQL—this will cause runtime SQL errors.The unqualified column references in lines 176-177 will cause ambiguous column errors at runtime:
contentin JSON_VALUE calls must be qualified asm.contentphone_numberin the SHA2 call must be qualified asr.phone_numberApply this fix:
rawSQL := "UPDATE `messages` `m`, `message_recipients` `r`\n" + - "SET `m`.`is_hashed` = true, `m`.`content` = SHA2(COALESCE(JSON_VALUE(`content`, '$.text'), JSON_VALUE(`content`, '$.data')), 256), `r`.`phone_number` = LEFT(SHA2(phone_number, 256), 16)\n" + + "SET `m`.`is_hashed` = true, `m`.`content` = SHA2(COALESCE(JSON_VALUE(`m`.`content`, '$.text'), JSON_VALUE(`m`.`content`, '$.data')), 256), `r`.`phone_number` = LEFT(SHA2(`r`.`phone_number`, 256), 16)\n" + "WHERE `m`.`id` = `r`.`message_id` AND `m`.`is_hashed` = false AND `m`.`is_encrypted` = false AND `m`.`state` <> 'Pending'"Optional: Add a fallback to avoid NULL writes if both JSON keys are absent:
SHA2(COALESCE(JSON_VALUE(`m`.`content`, '$.text'), JSON_VALUE(`m`.`content`, '$.data'), ''), 256)
🧹 Nitpick comments (4)
deployments/docker-compose-proxy/docker-compose.yml (1)
58-71: Worker service deployment looks correct.The new worker service is properly configured with flag-driven CLI invocation (
command: ["/app/app", "worker"]), appropriate environment variables (notably excludingGATEWAY__PRIVATE_TOKEN), and correct database dependency. The configuration aligns well with the PR objectives for introducing worker mode.Minor consideration: The worker service lacks an explicit health check. Consider adding one to enable orchestration tooling (e.g., Portainer) to monitor worker readiness, especially since the PR mentions support for Portainer (v2.20.0+).
worker: image: ghcr.io/android-sms-gateway/server:latest command: ["/app/app", "worker"] environment: - CONFIG_PATH=config.yml - DATABASE__PASSWORD=${MYSQL_PASSWORD} configs: - source: config-yml target: /app/config.yml restart: unless-stopped depends_on: db: condition: service_healthy + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:3000/health"] # adjust endpoint as needed + interval: 30s + timeout: 5s + retries: 3deployments/helm-chart/templates/deployment.yaml (1)
148-148: Consider making worker replicas configurable.The worker replica count is hardcoded to 1, while the backend uses
.Values.replicaCount. Making this configurable would provide more deployment flexibility.- replicas: 1 + replicas: {{ .Values.workerReplicaCount | default 1 }}internal/worker/server/config.go (1)
3-6: Consider adding godoc for the exported Config type.While this is an internal package, adding a brief comment would improve clarity, especially for fields like
Proxies(e.g., what format is expected).Example:
+// Config holds server configuration for the worker HTTP server. type Config struct { + // Address is the HTTP listen address (e.g., ":8080"). Address string + // Proxies is a list of trusted proxy IP addresses or CIDR ranges. Proxies []string }internal/worker/locker/locker.go (1)
8-21: LGTM! Documentation significantly improved.The error and method documentation addresses the previous review feedback. The interface and its contract are now clear.
Optional: Consider adding a brief comment for the
Lockerinterface itself (line 11) to complete the documentation:+// Locker provides distributed locking using string keys. type Locker interface {
📜 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 (59)
build/package/Dockerfile.dev(0 hunks)cmd/sms-gateway/main.go(2 hunks)configs/config.example.yml(2 hunks)deployments/docker-compose-proxy/docker-compose.yml(3 hunks)deployments/docker-compose/.env(0 hunks)deployments/docker-compose/docker-compose.dev.yml(0 hunks)deployments/docker-compose/docker-compose.yml(3 hunks)deployments/docker-swarm-terraform/worker.tf(1 hunks)deployments/grafana/dashboards/worker.json(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)deployments/prometheus/alerts/worker-alerts.yml(1 hunks)go.mod(5 hunks)internal/config/config.go(2 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(3 hunks)internal/sms-gateway/handlers/3rdparty.go(2 hunks)internal/sms-gateway/handlers/health.go(5 hunks)internal/sms-gateway/handlers/module.go(1 hunks)internal/sms-gateway/handlers/root.go(2 hunks)internal/sms-gateway/modules/cleaner/interfaces.go(0 hunks)internal/sms-gateway/modules/cleaner/module.go(0 hunks)internal/sms-gateway/modules/cleaner/service.go(0 hunks)internal/sms-gateway/modules/db/health.go(1 hunks)internal/sms-gateway/modules/db/module.go(1 hunks)internal/sms-gateway/modules/devices/cache.go(1 hunks)internal/sms-gateway/modules/devices/module.go(1 hunks)internal/sms-gateway/modules/devices/repository.go(7 hunks)internal/sms-gateway/modules/devices/service.go(3 hunks)internal/sms-gateway/modules/messages/config.go(1 hunks)internal/sms-gateway/modules/messages/module.go(1 hunks)internal/sms-gateway/modules/messages/repository.go(6 hunks)internal/sms-gateway/modules/messages/service.go(4 hunks)internal/sms-gateway/modules/messages/tasks.go(0 hunks)internal/sms-gateway/modules/messages/workers.go(1 hunks)internal/worker/app.go(1 hunks)internal/worker/config/config.go(1 hunks)internal/worker/config/module.go(1 hunks)internal/worker/config/types.go(1 hunks)internal/worker/executor/metrics.go(1 hunks)internal/worker/executor/module.go(1 hunks)internal/worker/executor/service.go(1 hunks)internal/worker/executor/types.go(1 hunks)internal/worker/locker/locker.go(1 hunks)internal/worker/locker/module.go(1 hunks)internal/worker/locker/mysql.go(1 hunks)internal/worker/server/config.go(1 hunks)internal/worker/server/module.go(1 hunks)internal/worker/tasks/devices/cleanup.go(1 hunks)internal/worker/tasks/devices/config.go(1 hunks)internal/worker/tasks/devices/module.go(1 hunks)internal/worker/tasks/messages/cleanup.go(1 hunks)internal/worker/tasks/messages/config.go(1 hunks)internal/worker/tasks/messages/hashing.go(1 hunks)internal/worker/tasks/messages/module.go(1 hunks)internal/worker/tasks/module.go(1 hunks)pkg/health/module.go(1 hunks)pkg/health/service.go(2 hunks)pkg/mysql/errors.go(1 hunks)scripts/test-helm-minikube.sh(1 hunks)
💤 Files with no reviewable changes (7)
- deployments/docker-compose/.env
- internal/sms-gateway/modules/cleaner/service.go
- internal/sms-gateway/modules/cleaner/interfaces.go
- internal/sms-gateway/modules/messages/tasks.go
- build/package/Dockerfile.dev
- internal/sms-gateway/modules/cleaner/module.go
- deployments/docker-compose/docker-compose.dev.yml
🚧 Files skipped from review as they are similar to previous changes (17)
- internal/worker/locker/module.go
- internal/worker/executor/types.go
- internal/sms-gateway/modules/db/health.go
- internal/worker/tasks/devices/cleanup.go
- pkg/mysql/errors.go
- internal/worker/tasks/messages/module.go
- cmd/sms-gateway/main.go
- scripts/test-helm-minikube.sh
- internal/worker/tasks/devices/module.go
- internal/sms-gateway/modules/messages/config.go
- internal/worker/tasks/messages/hashing.go
- deployments/prometheus/alerts/worker-alerts.yml
- deployments/docker-swarm-terraform/worker.tf
- pkg/health/module.go
- internal/worker/tasks/messages/config.go
- internal/worker/locker/mysql.go
- pkg/health/service.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-28T11:12:35.699Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 187
File: internal/worker/executor/service.go:64-75
Timestamp: 2025-10-28T11:12:35.699Z
Learning: As of Go 1.23, unreferenced unstopped timers created by time.After can be garbage collected. There is no need to prefer time.NewTimer over time.After for garbage collection efficiency in Go 1.23+.
Applied to files:
internal/worker/executor/service.go
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
internal/sms-gateway/app.gointernal/sms-gateway/modules/messages/module.go
📚 Learning: 2025-09-05T08:37:28.077Z
Learnt from: eznix86
Repo: android-sms-gateway/server PR: 172
File: deployments/kubernetes/sms-gateway-server/Chart.yaml:9-10
Timestamp: 2025-09-05T08:37:28.077Z
Learning: In the android-sms-gateway/server project, the Helm chart's appVersion in Chart.yaml is set to "latest" as a placeholder because CI automatically updates it with the actual version from git tags during the release process via the bump-chart-app-version.yaml workflow.
Applied to files:
deployments/helm-chart/templates/deployment.yaml
🧬 Code graph analysis (23)
internal/sms-gateway/handlers/3rdparty.go (1)
internal/sms-gateway/handlers/health.go (1)
HealthHandler(13-17)
internal/sms-gateway/handlers/module.go (1)
internal/sms-gateway/handlers/health.go (1)
NewHealthHandler(19-30)
internal/worker/app.go (5)
internal/worker/locker/module.go (1)
Module(11-31)internal/worker/server/module.go (1)
Module(13-43)internal/worker/config/module.go (1)
Module(15-70)internal/worker/executor/module.go (1)
Module(10-29)internal/worker/tasks/module.go (1)
Module(10-17)
internal/sms-gateway/modules/devices/module.go (4)
internal/sms-gateway/app.go (1)
Module(33-56)internal/sms-gateway/modules/messages/module.go (1)
Module(10-25)internal/sms-gateway/modules/devices/repository.go (1)
NewRepository(23-27)internal/sms-gateway/modules/devices/service.go (1)
NewService(25-41)
internal/worker/executor/service.go (2)
internal/worker/executor/types.go (1)
PeriodicTask(8-12)internal/worker/locker/locker.go (1)
Locker(11-21)
internal/sms-gateway/modules/messages/service.go (1)
internal/sms-gateway/modules/messages/repository.go (1)
Repository(20-22)
internal/worker/server/module.go (2)
internal/worker/server/config.go (1)
Config(3-6)internal/sms-gateway/handlers/health.go (2)
NewHealthHandler(19-30)HealthHandler(13-17)
internal/worker/tasks/devices/config.go (1)
internal/worker/config/types.go (1)
Duration(11-11)
internal/worker/tasks/module.go (3)
internal/worker/executor/module.go (1)
Module(10-29)internal/worker/tasks/devices/module.go (1)
Module(10-22)internal/worker/tasks/messages/module.go (1)
Module(10-23)
internal/sms-gateway/app.go (2)
pkg/health/module.go (1)
Module(8-20)internal/sms-gateway/modules/devices/module.go (1)
Module(8-18)
internal/worker/tasks/messages/cleanup.go (2)
internal/worker/tasks/messages/config.go (1)
CleanupConfig(14-17)internal/worker/executor/types.go (1)
PeriodicTask(8-12)
internal/sms-gateway/handlers/root.go (1)
internal/sms-gateway/handlers/health.go (1)
HealthHandler(13-17)
internal/worker/config/config.go (4)
internal/config/config.go (2)
Database(44-55)HTTP(27-33)internal/worker/config/types.go (1)
Duration(11-11)internal/worker/tasks/devices/config.go (1)
Config(5-7)internal/worker/tasks/messages/config.go (1)
Config(5-8)
internal/sms-gateway/modules/messages/repository.go (4)
internal/sms-gateway/modules/devices/repository.go (2)
NewRepository(23-27)Repository(19-21)internal/sms-gateway/modules/messages/repository_filter.go (3)
MessagesSelectFilter(16-23)MessagesSelectOptions(25-36)MessagesOrder(7-7)internal/sms-gateway/modules/messages/models.go (2)
Message(35-55)MessageRecipient(111-117)pkg/mysql/errors.go (1)
IsDuplicateKeyViolation(9-15)
internal/worker/executor/module.go (4)
internal/worker/tasks/devices/module.go (1)
Module(10-22)internal/worker/tasks/messages/module.go (1)
Module(10-23)internal/worker/tasks/module.go (1)
Module(10-17)internal/worker/executor/service.go (2)
NewService(25-36)Service(14-23)
internal/sms-gateway/modules/messages/module.go (2)
internal/sms-gateway/modules/devices/module.go (1)
Module(8-18)internal/sms-gateway/modules/messages/repository.go (1)
NewRepository(24-28)
internal/config/module.go (1)
internal/config/config.go (2)
Messages(75-78)Tasks(63-65)
internal/sms-gateway/modules/devices/service.go (4)
internal/sms-gateway/modules/devices/config.go (1)
Config(5-7)internal/sms-gateway/modules/devices/repository.go (1)
Repository(19-21)internal/sms-gateway/modules/db/module.go (1)
IDGen(10-10)internal/sms-gateway/modules/devices/repository_filter.go (1)
WithToken(17-21)
internal/sms-gateway/handlers/health.go (2)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)pkg/health/service.go (1)
Service(9-13)
internal/sms-gateway/modules/messages/workers.go (2)
internal/sms-gateway/modules/messages/repository.go (1)
Repository(20-22)internal/sms-gateway/modules/messages/config.go (1)
Config(5-8)
internal/sms-gateway/modules/devices/repository.go (3)
internal/sms-gateway/modules/messages/repository.go (2)
NewRepository(24-28)Repository(20-22)internal/sms-gateway/modules/devices/repository_filter.go (1)
SelectFilter(9-9)internal/sms-gateway/models/models.go (1)
Device(25-36)
internal/worker/config/module.go (6)
internal/worker/config/config.go (6)
Config(9-13)Default(34-57)Tasks(15-19)MessagesHashing(20-22)MessagesCleanup(24-27)DevicesCleanup(29-32)internal/worker/server/config.go (1)
Config(3-6)internal/worker/tasks/devices/config.go (2)
Config(5-7)CleanupConfig(9-12)internal/worker/tasks/messages/config.go (3)
Config(5-8)HashingConfig(10-12)CleanupConfig(14-17)internal/config/config.go (2)
Database(44-55)HTTP(27-33)internal/worker/config/types.go (1)
Duration(11-11)
internal/sms-gateway/modules/devices/cache.go (2)
internal/sms-gateway/models/models.go (1)
Device(25-36)pkg/cache/errors.go (1)
ErrKeyNotFound(7-7)
🔇 Additional comments (35)
deployments/grafana/dashboards/worker.json (1)
1-389: Well-structured worker monitoring dashboard—verify metrics and job labels are properly implemented.The dashboard configuration is valid and well-designed. The panels logically cover worker task execution (active tasks, results, duration percentiles, health, and alerts). Metric names are specific and follow conventions. However, these are external dependencies that must be satisfied:
- Metrics:
worker_executor_active_tasks,worker_executor_task_result_total,worker_executor_task_duration_seconds_bucketmust be emitted by the worker process.- Prometheus job label: Line 307 queries
up{job="worker"}. Ensure your Prometheus scrape configuration labels the worker job as"worker".- Alert naming: Line 360 queries
ALERTS{alertname=~"Worker.*"}. Ensure your alerting rules define alerts matching this pattern.Please verify that:
- The worker application instrumentation emits the expected metrics.
- The Prometheus scrape config uses
job_name: "worker"for the worker job.- Alerting rules are defined with alert names starting with "Worker".
deployments/docker-compose-proxy/docker-compose.yml (3)
36-42: Verify HEADER environment variable formatting produces correct output.The multi-line YAML format may introduce unintended whitespace or newlines into the environment variable value. Docker-compose environment variables typically expect single-line values. Verify that the resulting
HEADERenvironment variable value matches the original format and is correctly parsed by reproxy.Please verify this is intentional and produces the same output. If reproxy expects headers as comma-separated on a single line, consider using:
- HEADER=X-Frame-Options:SAMEORIGIN,X-XSS-Protection:1; mode=block;,X-Content-Type-Options:nosniff,Strict-Transport-Security:max-age=31536000,Referrer-Policy:no-referrer,Content-Security-Policy:default-src 'self'; style-src 'self' 'unsafe-inline' https://fonts.googleapis.com/; font-src self https://fonts.gstatic.com/;Or if reproxy supports multiline headers, verify the YAML is properly parsed and doesn't include unintended indentation in the value.
45-45: Verify GHCR image is published and accessible.The backend service image has migrated from
capcom6/sms-gateway:latesttoghcr.io/android-sms-gateway/server:latest. Ensure this image is published, accessible, and that the deployment environment can pull from GitHub Container Registry (GHCR) without authentication issues. Note: Both backend and worker services use this same image, which correctly supports both deployment modes.
83-83: MariaDB healthcheck format change looks good.The healthcheck test has been reformatted from multiline to single-line format. The functionality is identical, and the syntax is valid for docker-compose.
internal/worker/tasks/devices/config.go (1)
1-12: LGTM!The configuration structs are well-structured and follow Go conventions. The use of
time.DurationforIntervalandMaxAgeis appropriate for the cleanup task configuration.configs/config.example.yml (1)
1-52: Configuration structure looks well-organized.The reorganization into Common, Server, and Worker sections provides clear separation of concerns. The typo mentioned in the previous review ("reasl-time") has been corrected to "real-time" at line 36.
deployments/helm-chart/templates/deployment.yaml (1)
134-137: LGTM!The tolerations block follows standard Helm templating patterns and enables proper node scheduling configuration.
deployments/docker-compose/docker-compose.yml (1)
1-70: LGTM!The docker-compose configuration properly separates the backend and worker services. Key improvements include:
- Health-based dependencies prevent premature service startup
- Worker service on port 3001 avoids conflicts with backend on port 3000
- Both services share the same configuration and database setup
internal/config/config.go (1)
63-69: Deprecation properly marked.The deprecated fields are clearly marked with comments, maintaining backward compatibility while signaling future removal.
internal/sms-gateway/modules/devices/repository.go (4)
19-27: LGTM!Exporting the
Repositorytype and constructor is consistent with the broader public API promotion across repository modules.
56-71: Good error handling improvement.The error wrapping with
fmt.Errorfand%wprovides better context while preserving the error chain forerrors.Isanderrors.As.
77-86: LGTM!The error handling is improved with proper wrapping and the
ErrNotFoundreturn when no rows are affected follows common repository patterns.
113-120: Well-designed cleanup method.The parameter rename to
untiland the querylast_seen < untilmake the cleanup semantics clear. Returning the affected count enables proper observability.internal/sms-gateway/modules/db/module.go (1)
7-7: LGTM!The health module import path update aligns with the reorganization that centralizes health providers under
pkg/health.internal/sms-gateway/handlers/module.go (1)
27-27: LGTM!The change to the exported
NewHealthHandlerconstructor is consistent with the broader health handler refactoring and public API promotion.internal/worker/tasks/module.go (1)
10-17: LGTM!The module composition is clean and follows the established FX module pattern used throughout the worker subsystem.
internal/sms-gateway/handlers/root.go (1)
14-14: LGTM!The type changes correctly align with the export of
HealthHandlerand maintain consistency across the handlers.Also applies to: 52-52
internal/worker/app.go (2)
19-28: LGTM!The FX application bootstrap follows the standard pattern and correctly wires all necessary modules for the worker subsystem.
30-51: LGTM!The worker module composition is well-structured, and the lifecycle hooks provide useful operational visibility.
internal/worker/executor/module.go (2)
10-29: LGTM!The executor module correctly wires the service with task group consumption and lifecycle management. The FX annotations properly configure dependency injection for the
worker:tasksgroup.
31-33: LGTM!The
AsWorkerTaskhelper function provides a clean abstraction for task registration and is correctly implemented using FX's result group annotation.internal/worker/config/module.go (1)
15-70: LGTM!The configuration module properly wires all necessary config providers with appropriate error handling and type transformations. The zero values for connection pool timeouts (lines 38-39) and empty DSN (line 30) are reasonable defaults—DSN will be constructed from individual connection parameters.
internal/sms-gateway/handlers/3rdparty.go (1)
21-21: LGTM!The type changes are consistent with the
HealthHandlerexport and properly maintain the handler wiring.Also applies to: 37-37
internal/worker/tasks/messages/cleanup.go (1)
44-55: Good use of repository cleanup hook.The task stays lean, wraps repository errors clearly, and only emits a log when work was done. Nicely shaped periodic worker.
internal/worker/server/module.go (1)
27-41: Prometheus wiring looks solid.Mounting the metrics endpoint inside the Fx invoke and layering the middleware keeps the worker’s HTTP surface consistent with the main gateway. Thumbs up.
internal/sms-gateway/modules/devices/module.go (1)
8-17: Module wiring remains coherent.Keeping the repository private while exposing the service maintains the same DI surface with the updated module function. Looks good.
internal/worker/config/types.go (1)
13-34: Duration helper fits config parsing nicely.The dual unmarshaler implementation keeps YAML/text inputs flexible while exposing plain durations to callers. Nice touch.
internal/sms-gateway/modules/devices/service.go (4)
14-41: LGTM! Clean service refactoring with proper cache integration.The Service struct and constructor are well-structured. The cache is properly initialized and all dependencies are clearly defined.
84-97: LGTM! Proper cache-aside pattern implementation.The GetByToken method correctly implements the cache-aside pattern with appropriate error logging for cache failures without blocking the primary operation.
99-112: LGTM! Cache invalidation properly handled.UpdatePushToken correctly invalidates the cache before updating the database. Cache invalidation failures are logged but don't prevent the update, which is the right approach for cache resilience.
141-163: LGTM! Batch cache invalidation correctly implemented.The Remove method properly invalidates cache entries for all devices being removed. The early return when no devices match prevents unnecessary operations.
internal/sms-gateway/modules/messages/repository.go (4)
20-28: LGTM! Clean repository export.The Repository type is now properly exported with a straightforward public constructor.
133-144: LGTM! Robust error handling for Insert.The enhanced error handling properly distinguishes duplicate key violations from other database errors and provides meaningful error wrapping.
162-165: LGTM! Modern GORM syntax.The UpdateState properly uses GORM's Model and Select methods for targeted updates.
193-200: LGTM! Clean Cleanup implementation.The Cleanup method is properly context-aware and returns both rows affected and error, which is good for observability.
capcom6
force-pushed
the
worker/initial-implementation
branch
from
e445f9d to
848a2de
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
deployments/helm-chart/templates/deployment.yaml (1)
215-222: Add readiness and startup probes to worker (duplicate of past feedback).The worker Deployment has only a liveness probe. Kubernetes needs readiness and startup probes to properly manage the worker lifecycle:
- Readiness probe tells Kubernetes when the worker is ready to process tasks
- Startup probe prevents liveness failures during initialization
The backend already includes all three probes (lines 94–119); apply the same pattern to the worker.
resources: {{- toYaml .Values.workerResources | nindent 12 }} livenessProbe: httpGet: path: /health/live port: 3000 initialDelaySeconds: 30 periodSeconds: 10 timeoutSeconds: 5 failureThreshold: 3 + readinessProbe: + httpGet: + path: /health/ready + port: 3000 + initialDelaySeconds: 10 + periodSeconds: 5 + timeoutSeconds: 3 + failureThreshold: 3 + startupProbe: + httpGet: + path: /health/startup + port: 3000 + initialDelaySeconds: 60 + periodSeconds: 10 + timeoutSeconds: 5 + failureThreshold: 6 {{- if .Values.gateway.config.enabled }}internal/worker/executor/service.go (3)
38-64: Protect Start() against concurrent or repeated invocations.Calling
Start()multiple times will spawn duplicate task goroutines, leading to unexpected behavior and resource leaks. Add a guard using a mutex-protected boolean flag to ensure the service starts only once.Apply this diff:
+type Service struct { + tasks []PeriodicTask + locker locker.Locker + + stopChan chan struct{} + wg sync.WaitGroup + + mu sync.Mutex + started bool + + metrics *metrics + logger *zap.Logger +} func (s *Service) Start() error { + s.mu.Lock() + if s.started { + s.mu.Unlock() + return nil + } + s.started = true + s.mu.Unlock() + ctx, cancel := context.WithCancel(context.Background())
124-129: Prevent double-close panic in Stop().Calling
Stop()multiple times will panic becauseclose(s.stopChan)cannot be called on an already-closed channel. Usesync.Onceto ensure the stop logic executes exactly once.Apply this diff:
type Service struct { tasks []PeriodicTask locker locker.Locker stopChan chan struct{} + stopOnce sync.Once wg sync.WaitGroup metrics *metrics logger *zap.Logger } func (s *Service) Stop() error { - close(s.stopChan) - s.wg.Wait() - + s.stopOnce.Do(func() { + close(s.stopChan) + s.wg.Wait() + }) return nil }
86-96: Ensure lock release even if task execution panics.If
s.execute(ctx, task)panics, the lock at line 87 is never released, causing the lock to remain held until the connection dies. Usedeferimmediately after acquiring the lock to guarantee release.Apply this diff:
case <-ticker.C: if err := s.locker.AcquireLock(ctx, task.Name()); err != nil { s.logger.Error("can't acquire lock", zap.String("name", task.Name()), zap.Error(err)) continue } + defer func(taskName string) { + if err := s.locker.ReleaseLock(ctx, taskName); err != nil { + s.logger.Error("can't release lock", zap.String("name", taskName), zap.Error(err)) + } + }(task.Name()) s.execute(ctx, task) - - if err := s.locker.ReleaseLock(ctx, task.Name()); err != nil { - s.logger.Error("can't acquire lock", zap.String("name", task.Name()), zap.Error(err)) - }Note: You may also want to downgrade the lock acquisition failure from
ErrortoDebugorInfolevel, as lock contention is expected in a distributed system.go.mod (1)
11-12: Pseudo-version concerns for go-core-fx modules already flagged.The use of pseudo-versions for
github.com/go-core-fx/fiberfxandgithub.com/go-core-fx/loggerhas already been identified in previous review comments. Please address the earlier feedback regarding using released versions where available.
🧹 Nitpick comments (10)
deployments/helm-chart/templates/deployment.yaml (1)
192-192: Inconsistency: worker hardcodes HTTP port while backend parameterizes it.The worker uses hardcoded port
3000forHTTP__LISTEN(line 192) and probe endpoint (line 218), whereas the backend uses{{ .Values.service.port }}(lines 54, 97, 105, 114). This creates a maintenance burden if port configuration changes.Consider parameterizing the worker port to match the backend pattern:
env: - name: CONFIG_PATH value: "/app/config.yml" - name: HTTP__LISTEN - value: "0.0.0.0:3000" + value: "0.0.0.0:{{ .Values.service.port }}"And update the probe:
livenessProbe: httpGet: path: /health/live - port: 3000 + port: httpAlso applies to: 218-218
internal/worker/config/config.go (1)
57-58: Consider explicit connection pool defaults.
MaxOpenConns: 0andMaxIdleConns: 0rely on database/sql defaults (unlimited open connections, 2 idle). Setting explicit values likeMaxOpenConns: 10andMaxIdleConns: 2would make the configuration more predictable and self-documenting.internal/worker/tasks/devices/config.go (1)
9-12: Consider extracting the shared CleanupConfig type.The
CleanupConfigstruct is identical to the one ininternal/worker/tasks/messages/config.go. To avoid duplication, consider extracting this common configuration type to a shared location, such asinternal/worker/tasks/config.goorinternal/worker/config/tasks.go.internal/worker/tasks/devices/cleanup.go (1)
8-8: Consider aliasing the repository import and renaming the field for clarity.For improved readability and to avoid ambiguity between the package name and the field:
- Alias the import:
devicesrepo "github.com/android-sms-gateway/server/internal/sms-gateway/modules/devices"- Rename the field from
devicestorepoAdditionally, for better testability, consider injecting a time source function (e.g.,
now func() time.Time) instead of callingtime.Now()directly at line 45.Also applies to: 15-15, 22-22, 27-27, 45-45
internal/worker/tasks/messages/cleanup.go (2)
8-8: Consider aliasing the repository import and injecting time for consistency.Similar to the devices cleanup task, consider:
- Aliasing the import:
messagesrepo "github.com/android-sms-gateway/server/internal/sms-gateway/modules/messages"- Renaming the field from
messagestorepo- Injecting a time source function for testability
This would improve consistency across cleanup tasks.
Also applies to: 15-15, 22-22, 27-27, 45-45
1-57: Note: Significant code duplication with devices cleanup task.The cleanup tasks for messages and devices are nearly identical. While the current duplication is manageable with just two tasks, if more cleanup tasks are added in the future, consider extracting a generic cleanup task abstraction.
internal/worker/tasks/messages/hashing.go (1)
8-8: Consider aliasing the repository import for consistency.For consistency with other task implementations and to avoid ambiguity:
- Alias the import:
messagesrepo "github.com/android-sms-gateway/server/internal/sms-gateway/modules/messages"- Consider renaming the field from
messagestorepoAlso applies to: 15-15, 22-22, 27-27
internal/worker/executor/service.go (1)
67-67: Consider using nanosecond-precision jitter.The current jitter calculation uses
math.Floor(rand.Float64()*task.Interval().Seconds())which truncates to seconds. For intervals shorter than a second or when more precise distribution is desired, consider usingrand.Int64N(task.Interval().Nanoseconds())for full nanosecond precision.Based on learnings
internal/worker/locker/mysql.go (1)
54-57: Consider logging when replacing an existing connection.If a connection already exists for the key (line 55-56), it's closed defensively. While this prevents leaks, it suggests a potential logic error (acquiring the same lock twice without releasing). Consider logging a warning when this occurs to help detect and diagnose such issues.
Apply this diff:
m.mu.Lock() // Should not exist; if it does, close previous to avoid leaks. if prev, ok := m.conns[key]; ok && prev != nil { + m.logger.Warn("replacing existing connection for lock", zap.String("key", key)) _ = prev.Close() } m.conns[key] = conn m.mu.Unlock()Note: This would require passing a logger to the
mySQLLockerstruct.cmd/sms-gateway/main.go (1)
41-52: Consider using a more robust CLI framework or explicitly defining the command structure.The current implementation calls
flag.Parse()without defining any flags, then manually extracts the command from positional arguments. This works but lacks common CLI features like help text, command validation, and structured subcommand handling.Consider one of these approaches:
- Use a CLI library (e.g.,
cobra,cli) for better structure and user experience:// Example with a simple approach if len(os.Args) > 1 && os.Args[1] == "worker" { worker.Run() } else { smsgateway.Run() }
- Or add explicit flag definition if flags are needed:
var command = flag.String("command", "start", "Command to run: start or worker") flag.Parse() if *command == "worker" { worker.Run() } else { smsgateway.Run() }
- Or remove the unused
flag.Parse()call if staying with positional arguments.
📜 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 (44)
build/package/Dockerfile.dev(0 hunks)cmd/sms-gateway/main.go(2 hunks)configs/config.example.yml(2 hunks)deployments/docker-compose-proxy/docker-compose.yml(3 hunks)deployments/docker-compose/.env(0 hunks)deployments/docker-compose/docker-compose.dev.yml(0 hunks)deployments/docker-compose/docker-compose.yml(3 hunks)deployments/docker-swarm-terraform/worker.tf(1 hunks)deployments/grafana/dashboards/worker.json(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)deployments/helm-chart/values.yaml(1 hunks)deployments/prometheus/alerts/worker-alerts.yml(1 hunks)go.mod(5 hunks)internal/sms-gateway/app.go(3 hunks)internal/sms-gateway/handlers/3rdparty.go(2 hunks)internal/sms-gateway/handlers/health.go(5 hunks)internal/sms-gateway/handlers/module.go(1 hunks)internal/sms-gateway/handlers/root.go(2 hunks)internal/sms-gateway/modules/db/health.go(1 hunks)internal/sms-gateway/modules/db/module.go(1 hunks)internal/worker/app.go(1 hunks)internal/worker/config/config.go(1 hunks)internal/worker/config/module.go(1 hunks)internal/worker/config/types.go(1 hunks)internal/worker/executor/metrics.go(1 hunks)internal/worker/executor/module.go(1 hunks)internal/worker/executor/service.go(1 hunks)internal/worker/executor/types.go(1 hunks)internal/worker/locker/locker.go(1 hunks)internal/worker/locker/module.go(1 hunks)internal/worker/locker/mysql.go(1 hunks)internal/worker/server/config.go(1 hunks)internal/worker/server/module.go(1 hunks)internal/worker/tasks/devices/cleanup.go(1 hunks)internal/worker/tasks/devices/config.go(1 hunks)internal/worker/tasks/devices/module.go(1 hunks)internal/worker/tasks/messages/cleanup.go(1 hunks)internal/worker/tasks/messages/config.go(1 hunks)internal/worker/tasks/messages/hashing.go(1 hunks)internal/worker/tasks/messages/module.go(1 hunks)internal/worker/tasks/module.go(1 hunks)pkg/health/module.go(1 hunks)pkg/health/service.go(2 hunks)scripts/test-helm-minikube.sh(1 hunks)
💤 Files with no reviewable changes (3)
- deployments/docker-compose/.env
- build/package/Dockerfile.dev
- deployments/docker-compose/docker-compose.dev.yml
🚧 Files skipped from review as they are similar to previous changes (21)
- internal/sms-gateway/modules/db/module.go
- internal/sms-gateway/modules/db/health.go
- internal/worker/server/config.go
- internal/worker/tasks/devices/module.go
- internal/worker/tasks/module.go
- internal/worker/locker/module.go
- internal/worker/tasks/messages/config.go
- deployments/prometheus/alerts/worker-alerts.yml
- configs/config.example.yml
- internal/worker/server/module.go
- internal/worker/config/module.go
- internal/worker/locker/locker.go
- internal/worker/app.go
- internal/worker/tasks/messages/module.go
- internal/sms-gateway/handlers/3rdparty.go
- internal/worker/executor/metrics.go
- deployments/grafana/dashboards/worker.json
- internal/sms-gateway/handlers/module.go
- deployments/docker-swarm-terraform/worker.tf
- internal/worker/config/types.go
- internal/worker/executor/types.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
internal/sms-gateway/app.gopkg/health/module.go
📚 Learning: 2025-09-05T08:37:28.077Z
Learnt from: eznix86
Repo: android-sms-gateway/server PR: 172
File: deployments/kubernetes/sms-gateway-server/Chart.yaml:9-10
Timestamp: 2025-09-05T08:37:28.077Z
Learning: In the android-sms-gateway/server project, the Helm chart's appVersion in Chart.yaml is set to "latest" as a placeholder because CI automatically updates it with the actual version from git tags during the release process via the bump-chart-app-version.yaml workflow.
Applied to files:
scripts/test-helm-minikube.sh
📚 Learning: 2025-10-28T11:12:35.699Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 187
File: internal/worker/executor/service.go:64-75
Timestamp: 2025-10-28T11:12:35.699Z
Learning: As of Go 1.23, unreferenced unstopped timers created by time.After can be garbage collected. There is no need to prefer time.NewTimer over time.After for garbage collection efficiency in Go 1.23+.
Applied to files:
internal/worker/executor/service.go
🧬 Code graph analysis (14)
cmd/sms-gateway/main.go (2)
internal/worker/app.go (1)
Run(19-28)internal/sms-gateway/app.go (1)
Run(58-65)
internal/worker/tasks/devices/config.go (2)
internal/worker/tasks/messages/config.go (1)
CleanupConfig(14-17)internal/worker/config/types.go (1)
Duration(11-11)
internal/worker/executor/module.go (4)
internal/worker/tasks/devices/module.go (1)
Module(10-22)internal/worker/tasks/messages/module.go (1)
Module(10-23)internal/worker/tasks/module.go (1)
Module(10-17)internal/worker/executor/service.go (2)
NewService(25-36)Service(14-23)
internal/worker/tasks/devices/cleanup.go (3)
internal/worker/tasks/devices/config.go (1)
CleanupConfig(9-12)internal/worker/tasks/messages/cleanup.go (1)
NewCleanupTask(20-31)internal/worker/executor/types.go (1)
PeriodicTask(8-12)
internal/worker/tasks/messages/cleanup.go (2)
internal/worker/tasks/messages/config.go (1)
CleanupConfig(14-17)internal/worker/executor/types.go (1)
PeriodicTask(8-12)
internal/worker/tasks/messages/hashing.go (2)
internal/worker/tasks/messages/config.go (1)
HashingConfig(10-12)internal/worker/executor/types.go (1)
PeriodicTask(8-12)
internal/sms-gateway/handlers/health.go (2)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)pkg/health/service.go (1)
Service(9-13)
internal/sms-gateway/handlers/root.go (1)
internal/sms-gateway/handlers/health.go (1)
HealthHandler(13-17)
internal/sms-gateway/app.go (3)
pkg/health/module.go (1)
Module(8-20)internal/sms-gateway/modules/messages/module.go (1)
Module(10-25)internal/sms-gateway/modules/devices/module.go (1)
Module(8-18)
pkg/health/module.go (5)
internal/sms-gateway/app.go (1)
Module(33-56)internal/sms-gateway/handlers/module.go (1)
Module(15-39)internal/sms-gateway/modules/db/module.go (1)
Module(12-22)pkg/health/health.go (1)
NewHealth(11-13)pkg/health/service.go (1)
NewService(15-21)
internal/worker/locker/mysql.go (1)
internal/worker/locker/locker.go (2)
Locker(11-21)ErrLockNotAcquired(9-9)
internal/worker/executor/service.go (2)
internal/worker/executor/types.go (1)
PeriodicTask(8-12)internal/worker/locker/locker.go (1)
Locker(11-21)
internal/worker/config/config.go (2)
internal/config/config.go (2)
Database(44-55)HTTP(27-33)internal/worker/config/types.go (1)
Duration(11-11)
pkg/health/service.go (1)
pkg/health/types.go (1)
HealthProvider(66-72)
⏰ 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: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (26)
scripts/test-helm-minikube.sh (1)
37-37: Good test environment configuration.Setting
image.pullPolicy=Alwaysensures fresh images are pulled during test runs, preventing issues with stale cached images when validating the new worker deployment. This is appropriate for test environments.deployments/helm-chart/values.yaml (1)
58-64: Clean separation of worker and backend resources.Adding
workerResourcesallows independent tuning of worker resource requests/limits without affecting the backend configuration. The default values are reasonable.deployments/helm-chart/templates/deployment.yaml (1)
135-137: Backend tolerations handling added consistently.The backend Deployment now includes tolerations configuration, following the same pattern as
nodeSelectorandaffinity. This allows pod scheduling flexibility via values.deployments/docker-compose-proxy/docker-compose.yml (3)
58-70: Worker service missing GATEWAY__PRIVATE_TOKEN—verify if intentional.The worker service is not provided
GATEWAY__PRIVATE_TOKEN, whereas the backend receives it. If the worker communicates with the gateway or validates requests, it may need this credential.Verify whether the worker service requires
GATEWAY__PRIVATE_TOKENor other credentials for operation. If needed, add it to the environment block:worker: image: ghcr.io/android-sms-gateway/server:latest command: ["/app/app", "worker"] environment: - CONFIG_PATH=config.yml - DATABASE__PASSWORD=${MYSQL_PASSWORD} + - GATEWAY__PRIVATE_TOKEN=${GATEWAY_PRIVATE_TOKEN} configs: - source: config-yml target: /app/config.yml restart: unless-stopped depends_on: db: condition: service_healthyAlternatively, confirm that the worker service is completely isolated and does not require gateway credentials.
82-85: Healthcheck formatting change is safe.The single-line YAML array format is functionally equivalent to multi-line format and preserves all arguments.
45-45: Image registry migration to ghcr.io—ensure users are informed.Both backend and worker now reference
ghcr.io/android-sms-gateway/server:latestinstead of the legacycapcom6/sms-gateway:latestregistry. Existing deployments using this compose file will pull from the new registry.Verify that:
- The new image is public and accessible
- Deprecation notices or migration docs are in place for users
Also applies to: 59-59
deployments/docker-compose/docker-compose.yml (3)
28-52: Worker service port mapping (3001) requires clarification.The worker service exposes port 3001:3000, suggesting it runs an HTTP service (likely for observability: metrics, health checks, or diagnostics). However, there is no
HTTP__LISTENenvironment override to set a custom port.Verify:
- What HTTP service does the worker expose on port 3001?
- Should the worker use a different port from the backend (3000) to avoid confusion, or is this for observability only?
- Is the app configured (via config.yml) to listen on a different internal port for the worker, or does it auto-detect from
HTTP__LISTEN?If the worker intentionally exposes metrics/health on 3001, consider adding a comment or documentation explaining this setup. If
HTTP__LISTENshould differ for the worker, add an environment override:worker: image: capcom6/sms-gateway build: context: ../.. dockerfile: ./build/package/Dockerfile args: - APP=sms-gateway command: ["/app/app", "worker"] environment: - DEBUG= - CONFIG_PATH=config.yml + - HTTP__LISTEN=0.0.0.0:3001 - DATABASE__HOST=db - DATABASE__PORT=3306 - DATABASE__USER=sms - DATABASE__PASSWORD=sms - DATABASE__DATABASE=sms ports: - "3001:3000" ...
10-18: GOOSE_DBSTRING environment variable removed from backend—verify migration strategy.The
GOOSE_DBSTRINGenvironment variable is no longer present in the backend service. This variable typically controlled database migration behavior (e.g., goose migrations).Verify that database migrations are still performed on app startup or via an alternative mechanism:
- If migrations are now always executed, ensure the database schema is initialized correctly.
- If migrations are now optional, confirm this doesn't cause schema version mismatches on first deployment.
- If migrations were entirely removed, document the new migration strategy.
For new deployments using this compose setup, confirm that the database schema is properly initialized. If needed, add
GOOSE_DBSTRINGback or document the new migration strategy in deployment docs.
1-1: Project naming and service orchestration improvements look good.
- Line 1: Adding
name: sms-gate-appallows running multiple stacks concurrently and improves clarity.- Lines 23-26: Backend now explicitly depends on db service health, ensuring proper startup order.
- Line 64: Consistent restart policy formatting across all services.
All are improvements that follow Docker Compose best practices.
Also applies to: 23-26, 64-64
internal/worker/config/config.go (1)
44-47: Previous concern addressed – DevicesCleanup is now properly initialized.The 365-day retention period for devices (vs. 30 days for messages) makes sense, as devices may remain inactive for longer periods while still being valid.
internal/worker/executor/module.go (1)
10-29: LGTM!The Fx module wiring is clean and follows best practices. The lifecycle hooks correctly wire Start/Stop methods, and the task grouping via
fx.AnnotatewithParamTagsis well-designed.internal/worker/locker/mysql.go (1)
34-62: LGTM! Connection pinning correctly implemented.The connection pinning approach correctly ensures that
GET_LOCKandRELEASE_LOCKoperate on the same database connection, addressing the previous review concerns.pkg/health/module.go (1)
8-20: LGTM! Clean migration to go-core-fx/logger pattern.The migration from
fx.Decoratewithzap.Logger.Namedtologger.WithNamedLoggerprovides a more consistent and standardized approach for module-scoped logging. The addition offx.Annotatewithfx.ParamTagsproperly wires the health-providers group injection forNewService.go.mod (2)
3-3: LGTM! Go toolchain update.Toolchain update from 1.24.1 to 1.24.3 is appropriate.
14-30: LGTM! Dependency updates.The dependency updates (validator, gopkg.in/yaml.v3 promotion to direct, golang.org/x/crypto) align with the new worker subsystem requirements.
internal/sms-gateway/handlers/root.go (2)
11-16: LGTM! Type update following HealthHandler export.The field and parameter type updates from
*healthHandlerto*HealthHandlercorrectly follow the export of the health handler type.
52-59: LGTM! Constructor parameter type updated.The constructor parameter type update aligns with the exported
*HealthHandlertype.pkg/health/service.go (2)
9-21: LGTM! Constructor simplified by removing ServiceParams.The refactor from using a
ServiceParamsstruct to direct parameters (providers []HealthProvider, logger *zap.Logger) simplifies the API and is more idiomatic. The field rename fromhealthProviderstoprovidersimproves brevity without losing clarity.
23-57: LGTM! Loop updated to use renamed field.The iteration correctly uses
s.providersfollowing the field rename.internal/sms-gateway/handlers/health.go (3)
1-17: LGTM! Handler type exported and import path updated.The export of
HealthHandler(previouslyhealthHandler) and the import path change frommodules/healthtopkg/healthalign with the modular restructuring in this PR.
32-104: LGTM! Method receivers updated consistently.All method receivers correctly updated from
*healthHandlerto*HealthHandlerfollowing the type export.
19-30: No issues found—Validator removal is intentional and safe.HealthHandler's methods (getLiveness, getReadiness, getStartup) never call validation methods, and the base.Handler methods defensively check
if h.Validator != nilbefore using it. SettingValidator: nilis the correct design choice for a health probe handler that doesn't perform request validation.internal/sms-gateway/app.go (4)
23-56: LGTM! Module wiring follows consistent patterns.The changes to use
Module()function calls (lines 35, 52) and the addition ofhealth.Module()(line 49) are consistent with the module patterns shown in the relevant code snippets.
67-117: Cleaner removal is complete and verified—no dangling references found.Comprehensive search across the codebase confirms all Cleaner-related code has been removed:
- No files named
*cleaner*or*Cleaner*exist- Zero "cleaner" references in any Go files (case-insensitive)
- No cleaner-related commits or recent file changes in git history
- The
app.gochanges (removal ofCleanerServiceand its goroutine) are consistent with a clean removalThe code change itself is correct. Please ensure this breaking change is documented in your PR description or migration notes for release tracking.
63-63: Verify the WithFxDefaultLogger() function is correct for your use case.The function
logger.WithFxDefaultLogger()is called from the external dependencygithub.com/go-core-fx/logger(v0.0.0-20251028014216-c34d2fb15ca2), which is a pre-release version. While the function is consistently used in bothinternal/worker/app.goandinternal/sms-gateway/app.go, public documentation for this function is unavailable. Manually verify that this function correctly configures the fx framework logger and maintains your expected logging behavior.
84-90: The review comment is incorrect. Background tasks are already properly separated by mode.The architecture shows that:
- Gateway mode (
cmd == "start"): RunsMessagesService.RunBackgroundTasksandPushService.Run- Worker mode (
cmd == "worker"): Uses a separate task execution system withtasks.Module()andexecutor.Module()The background tasks in
app.golines 84-90 execute exclusively in gateway mode through the gateway'sStart()function. The worker mode has its own independent task handling system. No changes needed—the PR correctly implements the separation.Likely an incorrect or invalid review comment.
capcom6
force-pushed
the
worker/initial-implementation
branch
from
ebe4350 to
39ea1eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
internal/worker/executor/service.go (3)
87-96: Reduce lock contention logging to debug level and use defer for release.Lock contention is expected in distributed systems; logging at
Errorlevel (line 88) creates noise. Additionally, using defer for lock release improves readability and ensures cleanup ordering is explicit.Apply this diff:
case <-ticker.C: if err := s.locker.AcquireLock(ctx, task.Name()); err != nil { - s.logger.Error("can't acquire lock", zap.String("name", task.Name()), zap.Error(err)) + if errors.Is(err, locker.ErrLockNotAcquired) { + s.logger.Debug("lock not acquired", zap.String("name", task.Name())) + } else { + s.logger.Error("can't acquire lock", zap.String("name", task.Name()), zap.Error(err)) + } continue } + defer func(taskName string) { + if err := s.locker.ReleaseLock(ctx, taskName); err != nil { + s.logger.Error("can't release lock", zap.String("name", taskName), zap.Error(err)) + } + }(task.Name()) s.execute(ctx, task) - - if err := s.locker.ReleaseLock(ctx, task.Name()); err != nil { - s.logger.Error("can't release lock", zap.String("name", task.Name()), zap.Error(err)) - } }Add the import:
import ( "context" + "errors" "math"
38-64: GuardStart()against multiple invocations.Calling
Start()multiple times will spawn duplicate task goroutines, leading to resource leaks and concurrent task execution without proper synchronization.Add a mutex-protected boolean field to prevent re-initialization:
type Service struct { tasks []PeriodicTask locker locker.Locker stopChan chan struct{} wg sync.WaitGroup + mu sync.Mutex + started bool metrics *metrics logger *zap.Logger } func (s *Service) Start() error { + s.mu.Lock() + if s.started { + s.mu.Unlock() + return nil + } + s.started = true + s.mu.Unlock() + ctx, cancel := context.WithCancel(context.Background())
124-129: ProtectStop()against double-close panic.Calling
Stop()twice will panic when attempting to close an already-closed channel at line 125.Use the
startedflag and mutex from the previous comment, or add async.Once:type Service struct { tasks []PeriodicTask locker locker.Locker stopChan chan struct{} wg sync.WaitGroup + stopOnce sync.Once metrics *metrics logger *zap.Logger } func (s *Service) Stop() error { - close(s.stopChan) - s.wg.Wait() - + s.stopOnce.Do(func() { + close(s.stopChan) + s.wg.Wait() + }) return nil }internal/worker/locker/module.go (1)
11-31: The hardcoded 10-second lock timeout remains unaddressed.The timeout is still hardcoded at line 12 and passed directly to
NewMySQLLockerat line 18. For long-running tasks, this fixed timeout can cause locks to expire prematurely, allowing duplicate task execution across worker instances.go.mod (1)
11-12: Update go.mod to use released version of logger instead of pseudo-version.The
github.com/go-core-fx/loggerdependency uses a pseudo-version but has a released versionv0.0.1available. Using released versions improves stability and makes dependency management clearer.Apply this change:
- github.com/go-core-fx/logger v0.0.0-20251028014216-c34d2fb15ca2 + github.com/go-core-fx/logger v0.0.1Then run
go mod tidyto update the module graph.deployments/docker-swarm-terraform/worker.tf (1)
25-33: Verify memory reservation does not exceed limit.The hardcoded 32 MB memory reservation can exceed
var.memory-limitif the variable is set below 32 MB, causing Docker Swarm to reject the deployment. Either add validation to enforce a minimum limit of 32 MB, document the minimum requirement, or make the reservation proportional to the limit.internal/sms-gateway/modules/messages/service.go (1)
108-119: PII exposure risk in cache before hashing.Recipients are cached before the worker hashes them (lines 114-116), potentially exposing raw phone numbers in the cache for the TTL duration (default 5 minutes). The
IsHashedflag indicates whether recipients have been hashed, but the cache stores unhashed data initially.Consider one of these mitigations:
- Invalidate or refresh cache entries after
HashProcessedsucceeds in the worker- Skip caching recipients when
IsHashedis false- Cache a redacted view until hashing completes
internal/config/config.go (1)
112-115: Conflicting hashing intervals between gateway and worker.The gateway's
HashingIntervalSecondsdefaults to 60 seconds while the worker defaults to 7 days. When both processes run, the gateway performs hashing every minute, duplicating the worker's task. Consider setting the gateway default to 0 to disable hashing there and let the worker handle it exclusively.Apply this diff to disable gateway hashing by default:
Messages: Messages{ CacheTTLSeconds: 300, // 5 minutes - HashingIntervalSeconds: 60, + HashingIntervalSeconds: 0, // Disabled; worker handles hashing },internal/sms-gateway/modules/messages/repository.go (1)
174-191: Unqualified columns will cause runtime SQL errors—this critical issue remains unfixed.At line 176, the unqualified column references will cause runtime errors:
JSON_VALUE(\content`, ...)resolves tomessages.contentbut should explicitly use`m`.`content``SHA2(phone_number, 256)will fail becausephone_numberexists only inmessage_recipientsand must be qualified as\r`.`phone_number``Apply this diff to fix the SQL:
- rawSQL := "UPDATE `messages` `m`, `message_recipients` `r`\n" + - "SET `m`.`is_hashed` = true, `m`.`content` = SHA2(COALESCE(JSON_VALUE(`content`, '$.text'), JSON_VALUE(`content`, '$.data')), 256), `r`.`phone_number` = LEFT(SHA2(phone_number, 256), 16)\n" + + rawSQL := "UPDATE `messages` `m`, `message_recipients` `r`\n" + + "SET `m`.`is_hashed` = true, `m`.`content` = SHA2(COALESCE(JSON_VALUE(`m`.`content`, '$.text'), JSON_VALUE(`m`.`content`, '$.data')), 256), `r`.`phone_number` = LEFT(SHA2(`r`.`phone_number`, 256), 16)\n" + "WHERE `m`.`id` = `r`.`message_id` AND `m`.`is_hashed` = false AND `m`.`is_encrypted` = false AND `m`.`state` <> 'Pending'"Optional: Add a COALESCE fallback to avoid NULL writes if both JSON keys are absent:
SHA2(COALESCE(JSON_VALUE(`m`.`content`, '$.text'), JSON_VALUE(`m`.`content`, '$.data'), ''), 256)
🧹 Nitpick comments (4)
internal/worker/tasks/messages/config.go (2)
10-12: LGTM!The
HashingConfigstructure is clean and appropriate for configuring periodic hashing tasks.Consider adding a godoc comment to document the purpose of this exported type:
+// HashingConfig configures the periodic hashing task for messages. type HashingConfig struct { Interval time.Duration }
14-17: Consider consolidating duplicateCleanupConfigdefinitions.The
CleanupConfigstruct is duplicated ininternal/worker/tasks/devices/config.gowith identical fields (IntervalandMaxAge). Consider extracting this to a shared location (e.g.,internal/worker/tasks/common/config.goorinternal/worker/config/types.go) to reduce duplication and improve maintainability.Example consolidation approach:
Create a shared cleanup config in
internal/worker/config/types.go:// CleanupConfig configures periodic cleanup tasks. type CleanupConfig struct { Interval time.Duration MaxAge time.Duration }Then import and reuse it in both
internal/worker/tasks/messages/config.goandinternal/worker/tasks/devices/config.go:+import "github.com/android-sms-gateway/server/internal/worker/config" + type Config struct { Hashing HashingConfig - Cleanup CleanupConfig + Cleanup config.CleanupConfig }deployments/helm-chart/templates/deployment.yaml (1)
215-228: Consider adding readiness and startup probes for complete health monitoring.The worker Deployment has a liveness probe and uses separate resource configuration (good!), but it's missing readiness and startup probes that the backend has (lines 103-119). Without these probes, Kubernetes cannot properly manage traffic routing during startup or detect when the worker is temporarily unavailable.
Apply this diff to add the missing probes:
failureThreshold: 3 + readinessProbe: + httpGet: + path: /health/ready + port: 3000 + initialDelaySeconds: 10 + periodSeconds: 5 + timeoutSeconds: 3 + failureThreshold: 3 + startupProbe: + httpGet: + path: /health/startup + port: 3000 + initialDelaySeconds: 60 + periodSeconds: 10 + timeoutSeconds: 5 + failureThreshold: 6 {{- if .Values.gateway.config.enabled }}internal/config/config.go (1)
63-69: Document migration from deprecated hashing config.Both
Tasks.HashingandHashingTask.IntervalSecondsare marked deprecated but retain defaults. Provide migration guidance in comments or documentation to help operators transition to the newMessages.HashingIntervalSecondsor worker-based configuration.
📜 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 (60)
build/package/Dockerfile.dev(0 hunks)cmd/sms-gateway/main.go(2 hunks)configs/config.example.yml(2 hunks)deployments/docker-compose-proxy/docker-compose.yml(3 hunks)deployments/docker-compose/.env(0 hunks)deployments/docker-compose/docker-compose.dev.yml(0 hunks)deployments/docker-compose/docker-compose.yml(3 hunks)deployments/docker-swarm-terraform/worker.tf(1 hunks)deployments/grafana/dashboards/worker.json(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)deployments/helm-chart/values.yaml(1 hunks)deployments/prometheus/alerts/worker-alerts.yml(1 hunks)go.mod(5 hunks)internal/config/config.go(2 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(3 hunks)internal/sms-gateway/handlers/3rdparty.go(2 hunks)internal/sms-gateway/handlers/health.go(5 hunks)internal/sms-gateway/handlers/module.go(1 hunks)internal/sms-gateway/handlers/root.go(2 hunks)internal/sms-gateway/modules/cleaner/interfaces.go(0 hunks)internal/sms-gateway/modules/cleaner/module.go(0 hunks)internal/sms-gateway/modules/cleaner/service.go(0 hunks)internal/sms-gateway/modules/db/health.go(1 hunks)internal/sms-gateway/modules/db/module.go(1 hunks)internal/sms-gateway/modules/devices/cache.go(1 hunks)internal/sms-gateway/modules/devices/module.go(1 hunks)internal/sms-gateway/modules/devices/repository.go(7 hunks)internal/sms-gateway/modules/devices/service.go(3 hunks)internal/sms-gateway/modules/messages/config.go(1 hunks)internal/sms-gateway/modules/messages/module.go(1 hunks)internal/sms-gateway/modules/messages/repository.go(6 hunks)internal/sms-gateway/modules/messages/service.go(4 hunks)internal/sms-gateway/modules/messages/tasks.go(0 hunks)internal/sms-gateway/modules/messages/workers.go(1 hunks)internal/worker/app.go(1 hunks)internal/worker/config/config.go(1 hunks)internal/worker/config/module.go(1 hunks)internal/worker/config/types.go(1 hunks)internal/worker/executor/metrics.go(1 hunks)internal/worker/executor/module.go(1 hunks)internal/worker/executor/service.go(1 hunks)internal/worker/executor/types.go(1 hunks)internal/worker/locker/locker.go(1 hunks)internal/worker/locker/module.go(1 hunks)internal/worker/locker/mysql.go(1 hunks)internal/worker/server/config.go(1 hunks)internal/worker/server/module.go(1 hunks)internal/worker/tasks/devices/cleanup.go(1 hunks)internal/worker/tasks/devices/config.go(1 hunks)internal/worker/tasks/devices/module.go(1 hunks)internal/worker/tasks/messages/cleanup.go(1 hunks)internal/worker/tasks/messages/config.go(1 hunks)internal/worker/tasks/messages/hashing.go(1 hunks)internal/worker/tasks/messages/module.go(1 hunks)internal/worker/tasks/module.go(1 hunks)pkg/health/module.go(1 hunks)pkg/health/service.go(2 hunks)pkg/mysql/errors.go(1 hunks)scripts/test-helm-minikube.sh(1 hunks)
💤 Files with no reviewable changes (7)
- internal/sms-gateway/modules/cleaner/interfaces.go
- internal/sms-gateway/modules/messages/tasks.go
- internal/sms-gateway/modules/cleaner/module.go
- deployments/docker-compose/.env
- build/package/Dockerfile.dev
- internal/sms-gateway/modules/cleaner/service.go
- deployments/docker-compose/docker-compose.dev.yml
✅ Files skipped from review due to trivial changes (1)
- internal/worker/tasks/messages/cleanup.go
🚧 Files skipped from review as they are similar to previous changes (22)
- cmd/sms-gateway/main.go
- internal/worker/tasks/devices/module.go
- internal/sms-gateway/modules/db/module.go
- internal/sms-gateway/handlers/module.go
- internal/worker/tasks/messages/hashing.go
- internal/worker/tasks/devices/cleanup.go
- pkg/mysql/errors.go
- internal/worker/server/config.go
- internal/worker/executor/module.go
- internal/worker/tasks/devices/config.go
- internal/sms-gateway/modules/db/health.go
- deployments/helm-chart/values.yaml
- scripts/test-helm-minikube.sh
- internal/sms-gateway/modules/messages/workers.go
- deployments/grafana/dashboards/worker.json
- internal/worker/app.go
- internal/worker/tasks/module.go
- internal/worker/tasks/messages/module.go
- internal/sms-gateway/modules/messages/config.go
- internal/worker/executor/types.go
- internal/worker/config/module.go
- internal/sms-gateway/modules/devices/cache.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-28T11:12:35.699Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 187
File: internal/worker/executor/service.go:64-75
Timestamp: 2025-10-28T11:12:35.699Z
Learning: As of Go 1.23, unreferenced unstopped timers created by time.After can be garbage collected. There is no need to prefer time.NewTimer over time.After for garbage collection efficiency in Go 1.23+.
Applied to files:
internal/worker/executor/service.go
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
pkg/health/module.gointernal/sms-gateway/modules/messages/module.gointernal/worker/locker/module.gointernal/sms-gateway/app.go
🧬 Code graph analysis (19)
internal/worker/executor/service.go (2)
internal/worker/executor/types.go (1)
PeriodicTask(8-12)internal/worker/locker/locker.go (1)
Locker(11-21)
pkg/health/module.go (5)
internal/sms-gateway/app.go (1)
Module(33-56)internal/sms-gateway/handlers/module.go (1)
Module(15-39)internal/sms-gateway/modules/db/module.go (1)
Module(12-22)pkg/health/health.go (1)
NewHealth(11-13)pkg/health/service.go (1)
NewService(15-21)
internal/worker/server/module.go (2)
internal/worker/server/config.go (1)
Config(3-6)internal/sms-gateway/handlers/health.go (2)
NewHealthHandler(19-30)HealthHandler(13-17)
internal/sms-gateway/handlers/health.go (2)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)pkg/health/service.go (1)
Service(9-13)
pkg/health/service.go (1)
pkg/health/types.go (1)
HealthProvider(66-72)
internal/sms-gateway/modules/devices/module.go (4)
internal/sms-gateway/app.go (1)
Module(33-56)internal/sms-gateway/modules/messages/module.go (1)
Module(10-25)internal/sms-gateway/modules/devices/repository.go (1)
NewRepository(23-27)internal/sms-gateway/modules/devices/service.go (1)
NewService(25-41)
internal/worker/config/config.go (2)
internal/config/config.go (2)
Database(44-55)HTTP(27-33)internal/worker/config/types.go (1)
Duration(11-11)
internal/sms-gateway/modules/messages/module.go (4)
internal/sms-gateway/app.go (1)
Module(33-56)internal/sms-gateway/handlers/module.go (1)
Module(15-39)internal/sms-gateway/cache/factory.go (1)
Factory(16-18)internal/sms-gateway/modules/messages/repository.go (1)
NewRepository(24-28)
internal/sms-gateway/handlers/3rdparty.go (1)
internal/sms-gateway/handlers/health.go (1)
HealthHandler(13-17)
internal/sms-gateway/modules/messages/repository.go (4)
internal/sms-gateway/modules/devices/repository.go (2)
NewRepository(23-27)Repository(19-21)internal/sms-gateway/modules/messages/repository_filter.go (3)
MessagesSelectFilter(16-23)MessagesSelectOptions(25-36)MessagesOrder(7-7)internal/sms-gateway/modules/messages/models.go (2)
Message(35-55)MessageRecipient(111-117)pkg/mysql/errors.go (1)
IsDuplicateKeyViolation(9-15)
internal/sms-gateway/modules/messages/service.go (2)
internal/sms-gateway/modules/messages/repository.go (1)
Repository(20-22)internal/sms-gateway/modules/devices/service.go (1)
Service(14-23)
internal/sms-gateway/handlers/root.go (3)
internal/sms-gateway/handlers/health.go (1)
HealthHandler(13-17)internal/sms-gateway/handlers/config.go (1)
Config(3-11)internal/sms-gateway/openapi/openapi.go (1)
Handler(12-13)
internal/sms-gateway/modules/devices/repository.go (4)
internal/sms-gateway/modules/messages/repository.go (2)
NewRepository(24-28)Repository(20-22)internal/sms-gateway/modules/devices/repository_filter.go (1)
SelectFilter(9-9)internal/sms-gateway/modules/webhooks/repository_filter.go (1)
SelectFilter(5-5)internal/sms-gateway/models/models.go (1)
Device(25-36)
internal/worker/locker/mysql.go (1)
internal/worker/locker/locker.go (2)
Locker(11-21)ErrLockNotAcquired(9-9)
internal/worker/locker/module.go (2)
internal/worker/locker/locker.go (1)
Locker(11-21)internal/worker/locker/mysql.go (1)
NewMySQLLocker(22-31)
internal/config/module.go (2)
internal/worker/config/types.go (1)
Duration(11-11)internal/config/config.go (2)
Messages(75-78)Tasks(63-65)
internal/worker/tasks/messages/config.go (2)
internal/worker/tasks/devices/config.go (1)
CleanupConfig(9-12)internal/worker/config/types.go (1)
Duration(11-11)
internal/sms-gateway/app.go (2)
internal/sms-gateway/modules/db/module.go (1)
Module(12-22)pkg/health/module.go (1)
Module(8-20)
internal/sms-gateway/modules/devices/service.go (4)
internal/sms-gateway/modules/devices/config.go (1)
Config(5-7)internal/sms-gateway/modules/devices/repository.go (1)
Repository(19-21)internal/sms-gateway/modules/db/module.go (1)
IDGen(10-10)internal/sms-gateway/modules/devices/repository_filter.go (1)
WithToken(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). (4)
- GitHub Check: Analyze (go)
- GitHub Check: E2E
- GitHub Check: Test
- GitHub Check: Benchmark
🔇 Additional comments (36)
internal/worker/tasks/messages/config.go (1)
5-8: LGTM!The
Configstruct appropriately aggregates the hashing and cleanup configurations for DI wiring.pkg/health/module.go (2)
4-4: LGTM! Cleaner logger setup.The switch to
logger.WithNamedLoggerreduces boilerplate while maintaining the same module-scoped logger behavior.Also applies to: 11-11
17-17: LGTM! Correct group dependency wiring.The
fx.ParamTagscorrectly consumes health providers registered with thehealth-providersgroup tag.pkg/health/service.go (1)
10-10: LGTM! Improved testability with explicit parameters.The refactoring from
ServiceParams(fx.In struct) to explicit constructor parameters makes the code cleaner and easier to test. The field rename toprovidersis also a good simplification.Also applies to: 15-20, 28-28
internal/worker/executor/metrics.go (1)
1-55: LGTM! Well-structured Prometheus metrics.The metrics follow Prometheus naming conventions, use appropriate bucket sizes (1ms to 10s) for task durations, and leverage thread-safe Prometheus types. The simple label set ("task", "result") provides good observability without label cardinality concerns.
internal/worker/locker/mysql.go (1)
11-102: LGTM! Connection-scoped locking correctly implemented.The implementation properly addresses the MySQL named lock connection-scoping requirements by:
- Pinning a dedicated
*sql.Connper lock key (lines 38, 58)- Storing pinned connections in a mutex-protected map (lines 17-18, 53-59)
- Releasing locks on the same connection that acquired them (lines 68-79)
- Cleaning up connections during
Close()(lines 92-102)This prevents the lock/release mismatch that would occur with pooled connections.
internal/worker/locker/locker.go (1)
8-21: LGTM! Exported API is well-documented.The interface and error variable now have clear godoc comments that describe the contract, return conditions, and expected behavior. This addresses the documentation concerns from previous reviews.
deployments/prometheus/alerts/worker-alerts.yml (1)
1-30: LGTM! Past issues have been addressed.The alerting rules are well-structured and the previously identified issues have been resolved:
- The
histogram_quantileat line 14 now correctly aggregates bylelabel- The active tasks metric at line 23 uses the correct gauge naming convention without the
_totalsuffixdeployments/docker-compose-proxy/docker-compose.yml (1)
58-70: LGTM! Worker service is properly configured.The new worker service correctly mirrors the backend configuration pattern with appropriate command override, environment variables, config mounting, and database health check dependency.
deployments/docker-compose/docker-compose.yml (1)
28-52: LGTM! Worker service is well-configured.The worker service includes proper build configuration, environment variables, volume mounts, restart policy, and database health check dependency. The port mapping (3001:3000) correctly avoids conflicts with the backend service.
internal/sms-gateway/modules/devices/service.go (3)
83-97: LGTM! Cache integration is well-implemented.The caching pattern correctly handles cache misses by falling back to the repository, caches retrieved devices for subsequent lookups, and logs cache errors without blocking the operation flow.
99-112: LGTM! Proper cache invalidation strategy.The cache invalidation correctly occurs before the database update, ensuring consistency. Cache errors are logged without blocking the primary operation, which is the right trade-off for cache resilience.
138-163: LGTM! Bulk removal handles cache invalidation correctly.The implementation properly invalidates the cache for all affected devices before performing the database removal. The early return when no devices match the filter prevents unnecessary operations.
configs/config.example.yml (1)
1-52: LGTM! Configuration structure is clear and well-documented.The config file is well-organized into Common, Server, and Worker sections. Each setting includes helpful comments with environment variable mappings, making it easy for users to understand and override configuration values.
internal/config/module.go (2)
100-105: Clarify migration path for deprecated hashing config.The
HashingIntervalcomputation usesmax()to reconcile two deprecated config sources (Tasks.Hashing.IntervalSecondsandMessages.HashingIntervalSeconds). Since both are marked deprecated, document which config operators should use going forward and ensure the gateway doesn't duplicate worker hashing work.
42-56: The review comment is incorrect.The original review assumes that "Previously used cfg.Database.Dialect dynamically," but the Database struct in config.go (lines 44-55) has never contained a Dialect field. A comprehensive search of the codebase confirms that
cfg.Database.Dialectis never referenced anywhere, indicating this configuration option never existed. Therefore, hardcodingdb.DialectMySQLis not a breaking change—there is no previous dialect configuration to break. The application either always used MySQL or this represents the first implementation of database support.Likely an incorrect or invalid review comment.
internal/sms-gateway/modules/messages/module.go (1)
10-25: LGTM on module refactoring.The migration to
Module() fx.Optionpattern withlogger.WithNamedLogger("messages")correctly provides module-scoped logging. ExportingNewRepositoryand renaming tonewHashingWorkeraligns with the worker extraction mentioned in the PR objectives.internal/worker/config/types.go (1)
1-37: LGTM on Duration type implementation.The custom
Durationtype withUnmarshalTextandUnmarshalYAMLmethods correctly parses duration strings viatime.ParseDuration. Error wrapping is clear, and compile-time interface checks ensure the type satisfies the required interfaces.internal/worker/config/config.go (1)
34-65: Verify device retention aligns with business requirements.
DevicesCleanup.MaxAgedefaults to 365 days whileMessagesCleanup.MaxAgeis 30 days. Confirm that retaining devices 12× longer than messages matches the intended data retention policy.internal/sms-gateway/modules/devices/module.go (1)
8-18: LGTM on consistent module pattern.The refactoring to
Module() fx.Optionwithlogger.WithNamedLogger("devices")and exportedNewRepositorymirrors the messages module changes. This consistency simplifies the codebase and enables worker reuse.internal/sms-gateway/modules/messages/service.go (1)
30-67: LGTM on service refactoring.Field renames (
*Repository,*hashingWorker) and constructor updates correctly align with the exported types frommodule.go. The changes are consistent and maintain proper encapsulation.internal/sms-gateway/app.go (2)
33-56: LGTM on cleaner removal and health integration.Removing the cleaner subsystem and adding
health.Module()aligns with the PR objectives. The module composition is consistent, with all modules now using theModule()function pattern.
58-117: LGTM on startup flow cleanup.The removal of
CleanerServicefromStartParamsand its startup goroutine is complete and consistent. Background tasks now only include messages and push services, aligning with the worker extraction.internal/sms-gateway/modules/devices/repository.go (6)
3-11: LGTM: Import addition supports error wrapping.The
fmtimport addition is appropriate for the error wrapping added in theGetandUpdatePushTokenmethods.
19-27: LGTM: Public API promotion follows established pattern.The type rename and constructor addition align with the PR's objective to expose repositories for worker use. The implementation follows the same pattern as the messages repository.
29-54: LGTM: Receiver type updates are consistent.The receiver type updates are consistent with the public API promotion.
56-71: LGTM: Error wrapping improves diagnostics.The addition of error wrapping with descriptive context is a good practice that will aid debugging.
77-84: LGTM: Error wrapping and explicit return improve clarity.The error wrapping and explicit nil return improve both error diagnostics and code clarity.
111-118: LGTM: Parameter rename improves semantic clarity.The rename from
sincetountilbetter conveys the cleanup intent: removing devices withlast_seenbefore the specified time.internal/sms-gateway/modules/messages/repository.go (6)
3-12: LGTM: Import refactoring improves abstraction.The switch to
pkg/mysqlprovides cleaner error-handling utilities and reduces direct database/sql dependencies.
20-28: LGTM: Public API promotion is consistent.The type rename and constructor follow the same pattern as the devices repository, maintaining consistency across the codebase.
30-131: LGTM: Receiver type updates are consistent.The receiver type updates are consistent with the public API promotion.
133-144: LGTM: Error handling improvements.The use of
mysql.IsDuplicateKeyViolationand error wrapping improves both error detection and diagnostics.
146-172: LGTM: Explicit field selection improves update safety.The use of
Updates(map[string]any{...})with explicit field selection ensures only the intended fields are updated.
193-200: LGTM: Cleanup logic correctly preserves pending messages.The dual WHERE clause ensures only non-pending messages older than the specified time are removed, which is the correct cleanup behavior.
internal/worker/server/module.go (1)
27-41: Health+metrics wiring looks solid.Registering the shared health handler alongside the fiberprometheus middleware ensures the worker exposes probes and Prom metrics immediately.
capcom6
added
deployed
Summary by CodeRabbit
New Features
Improvements
Configuration
Removals