Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds access-log cleanup: Store API to delete old logs, Manager methods for immediate and periodic cleanup with background goroutine and cancel control, config fields for retention/interval, boot wiring to start cleanup, and corresponding unit/integration test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Boot as Boot Process
participant Mgr as AccessLogsManager
participant Store as Store
participant DB as Database
Boot->>Mgr: StartPeriodicCleanup(ctx, retentionDays, intervalHours)
activate Mgr
Mgr->>Mgr: validate inputs, create cancelable ctx
Mgr->>Mgr: spawn background goroutine & run immediate cleanup
deactivate Mgr
Mgr->>Store: DeleteOldAccessLogs(ctx, cutoffTime)
activate Store
Store->>DB: DELETE ... WHERE timestamp < cutoffTime
DB-->>Store: rowsAffected
Store-->>Mgr: rowsAffected, error
deactivate Store
loop Periodic Cleanup (ticker)
Mgr->>Store: DeleteOldAccessLogs(ctx, cutoffTime)
activate Store
Store->>DB: DELETE ... WHERE timestamp < cutoffTime
DB-->>Store: rowsAffected
Store-->>Mgr: rowsAffected, error
deactivate Store
end
Boot->>Mgr: StopPeriodicCleanup()
activate Mgr
Mgr->>Mgr: cancel context -> stop goroutine and ticker
deactivate Mgr
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go (2)
16-70: Missing error-path test case forCleanupOldAccessLogs.The table has no case where
DeleteOldAccessLogsreturns an error, leaving the branch at manager.go lines 92–94 untested.✅ Suggested additional test case
+ { + name: "store returns error", + retentionDays: 30, + setupMock: func(mockStore *store.MockStore) { + mockStore.EXPECT(). + DeleteOldAccessLogs(gomock.Any(), gomock.Any()). + Return(int64(0), errors.New("db connection error")) + }, + expectedCount: 0, + expectedError: true, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go` around lines 16 - 70, Add a test case exercising the error path of CleanupOldAccessLogs by extending the tests table with a case (e.g., name "delete returns error") where retentionDays > 0, setupMock configures mockStore.EXPECT().DeleteOldAccessLogs(gomock.Any(), gomock.Any()).Return(int64(0), errors.New("delete failed")), and the test asserts that the returned count is 0 and expectedError is true; this ensures the manager.CleanupOldAccessLogs code path that handles errors from store.DeleteOldAccessLogs is executed and validated.
140-242:time.Sleep-based synchronisation is flaky; the periodic-cleanup path is never exercised.All
StartPeriodicCleanupsub-tests rely on fixed sleeps (100–200 ms) to allow the goroutine to run. Under CI load these windows can be too short, causing intermittent failures. Additionally, because all tests use 12 h or 24 h intervals, the ticker never fires — only the immediate startup cleanup is exercised. The periodic code path (theticker.Cbranch) has zero test coverage.Consider replacing sleeps with
assert.Eventuallyor a done-channel injected via an option, and add at least one sub-test with a very short interval (e.g.,1 * time.Millisecond) to cover the periodic branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go` around lines 140 - 242, The tests use brittle time.Sleep and long ticker intervals so the ticker.C branch in StartPeriodicCleanup on managerImpl is never reliably exercised; replace the sleeps with deterministic synchronization (e.g., testing/assert.Eventually or waiting on a done channel injected via an option) and add a sub-test that starts manager.StartPeriodicCleanup with a very short interval (e.g., 1 millisecond) so the periodic ticker fires; update the mocks (store.NewMockStore) expectations to account for multiple DeleteOldAccessLogs calls and call manager.StopPeriodicCleanup (or cancel the context) to cleanly stop the goroutine after the assertion.management/internals/modules/reverseproxy/accesslogs/manager/manager.go (1)
105-149:StartPeriodicCleanupcalled twice leaks the first goroutine and creates a concurrent duplicate cleanup.If
StartPeriodicCleanupis invoked again (e.g. config reload),m.cleanupCancelis silently overwritten. The previous goroutine can no longer be stopped viaStopPeriodicCleanup; it only exits when the parentctxis cancelled. Both goroutines now perform simultaneous cleanup.Additionally,
m.cleanupCancelis written and read from different call sites without synchronization; the Go race detector will flag this if callers overlap.A minimal fix: stop any existing routine before starting a new one, and protect the field with a mutex:
♻️ Proposed fix
type managerImpl struct { store store.Store permissionsManager permissions.Manager geo geolocation.Geolocation + mu sync.Mutex cleanupCancel context.CancelFunc }func (m *managerImpl) StartPeriodicCleanup(ctx context.Context, retentionDays, cleanupIntervalHours int) { if retentionDays <= 0 { log.WithContext(ctx).Debug("periodic access log cleanup disabled: retention days is 0 or negative") return } + // Stop any previously running cleanup before starting a new one. + m.StopPeriodicCleanup() + if cleanupIntervalHours <= 0 { cleanupIntervalHours = 24 } + m.mu.Lock() cleanupCtx, cancel := context.WithCancel(ctx) m.cleanupCancel = cancel + m.mu.Unlock() // ... } func (m *managerImpl) StopPeriodicCleanup() { + m.mu.Lock() + cancel := m.cleanupCancel + m.cleanupCancel = nil + m.mu.Unlock() + if cancel != nil { + cancel() + } - if m.cleanupCancel != nil { - m.cleanupCancel() - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/accesslogs/manager/manager.go` around lines 105 - 149, StartPeriodicCleanup currently overwrites m.cleanupCancel and can leak the previous goroutine and race on the field; modify managerImpl to guard cleanup lifecycle with a mutex (e.g., add a sync.Mutex or sync.RWMutex on managerImpl) and in StartPeriodicCleanup acquire the lock, call the existing cancel (if non-nil) to stop any running cleanup goroutine, then set m.cleanupCancel to the new cancel; ensure StopPeriodicCleanup also acquires the same mutex before reading/calling m.cleanupCancel and clearing it to avoid races and duplicate routines (references: StartPeriodicCleanup, StopPeriodicCleanup, m.cleanupCancel, managerImpl).management/server/store/sql_store.go (1)
5103-5115: Batching recommended for large DELETE operations; timestamp is already indexed.The single unbatched
DELETE … WHERE timestamp < ?can still cause lock contention and replication lag on MySQL/Aurora when operating at scale. However, theTimestampfield inAccessLogEntryalready carries agorm:"index"tag (line 17 of accesslogentry.go), so full table scans are not a concern.Consider chunking large deletes to bound lock duration:
♻️ Suggested batched-delete approach
-func (s *SqlStore) DeleteOldAccessLogs(ctx context.Context, olderThan time.Time) (int64, error) { - result := s.db.WithContext(ctx). - Where("timestamp < ?", olderThan). - Delete(&accesslogs.AccessLogEntry{}) - - if result.Error != nil { - log.WithContext(ctx).Errorf("failed to delete old access logs: %v", result.Error) - return 0, status.Errorf(status.Internal, "failed to delete old access logs") - } - - return result.RowsAffected, nil -} +func (s *SqlStore) DeleteOldAccessLogs(ctx context.Context, olderThan time.Time) (int64, error) { + const batchSize = 1000 + var totalDeleted int64 + for { + result := s.db.WithContext(ctx). + Where("timestamp < ?", olderThan). + Limit(batchSize). + Delete(&accesslogs.AccessLogEntry{}) + if result.Error != nil { + log.WithContext(ctx).Errorf("failed to delete old access logs: %v", result.Error) + return totalDeleted, status.Errorf(status.Internal, "failed to delete old access logs") + } + totalDeleted += result.RowsAffected + if result.RowsAffected < batchSize { + break + } + } + return totalDeleted, nil +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/store/sql_store.go` around lines 5103 - 5115, The DeleteOldAccessLogs function performs a single large DELETE which can cause lock/replication issues for big datasets; modify SqlStore.DeleteOldAccessLogs (and use the accesslogs.AccessLogEntry model) to delete in bounded batches: loop issuing repeated deletes scoped to the same WHERE "timestamp < ?" but with an ORDER BY timestamp and a LIMIT (e.g., 500–5000) using s.db.WithContext(ctx).Where(...).Order("timestamp").Limit(batchSize).Delete(...), accumulate RowsAffected until the last batch returns zero, and return the total deleted count; ensure you preserve error handling (log the error and return status.Internal) and that the loop breaks on result.Error or when rows deleted < batchSize to avoid infinite loops.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go`:
- Around line 29-36: In the DoAndReturn callback used in the tests (e.g., inside
the TestCleanup test and the identical callback in TestCleanupWithExactBoundary)
replace t.Errorf(...) with t.Fatalf(...) so the test aborts immediately on the
cutoff-time assertion failure; update the anonymous DoAndReturn funcs that
currently compute expectedCutoff/timeDiff and call t.Errorf to call t.Fatalf
with the same message, ensuring the callback does not return a success value
after a failed assertion.
In `@management/internals/server/boot.go`:
- Around line 197-207: AccessLogsManager currently starts a long‑running cleanup
goroutine with StartPeriodicCleanup(context.Background(), ...) and never wires
StopPeriodicCleanup into shutdown, causing goroutine leaks; fix it by adding an
accessLogManager field to BaseServer, have AccessLogsManager() store the created
accesslogsmanager.Manager instance into that field (instead of only returning
it), and call its StopPeriodicCleanup(ctx) from BaseServer.Stop() (following the
proxyAuthClose pattern) so the cleanup goroutine is cancelled during graceful
shutdown and in tests; reference BaseServer, AccessLogsManager(),
StartPeriodicCleanup, StopPeriodicCleanup, proxyAuthClose, and Stop().
---
Nitpick comments:
In
`@management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go`:
- Around line 16-70: Add a test case exercising the error path of
CleanupOldAccessLogs by extending the tests table with a case (e.g., name
"delete returns error") where retentionDays > 0, setupMock configures
mockStore.EXPECT().DeleteOldAccessLogs(gomock.Any(),
gomock.Any()).Return(int64(0), errors.New("delete failed")), and the test
asserts that the returned count is 0 and expectedError is true; this ensures the
manager.CleanupOldAccessLogs code path that handles errors from
store.DeleteOldAccessLogs is executed and validated.
- Around line 140-242: The tests use brittle time.Sleep and long ticker
intervals so the ticker.C branch in StartPeriodicCleanup on managerImpl is never
reliably exercised; replace the sleeps with deterministic synchronization (e.g.,
testing/assert.Eventually or waiting on a done channel injected via an option)
and add a sub-test that starts manager.StartPeriodicCleanup with a very short
interval (e.g., 1 millisecond) so the periodic ticker fires; update the mocks
(store.NewMockStore) expectations to account for multiple DeleteOldAccessLogs
calls and call manager.StopPeriodicCleanup (or cancel the context) to cleanly
stop the goroutine after the assertion.
In `@management/internals/modules/reverseproxy/accesslogs/manager/manager.go`:
- Around line 105-149: StartPeriodicCleanup currently overwrites m.cleanupCancel
and can leak the previous goroutine and race on the field; modify managerImpl to
guard cleanup lifecycle with a mutex (e.g., add a sync.Mutex or sync.RWMutex on
managerImpl) and in StartPeriodicCleanup acquire the lock, call the existing
cancel (if non-nil) to stop any running cleanup goroutine, then set
m.cleanupCancel to the new cancel; ensure StopPeriodicCleanup also acquires the
same mutex before reading/calling m.cleanupCancel and clearing it to avoid races
and duplicate routines (references: StartPeriodicCleanup, StopPeriodicCleanup,
m.cleanupCancel, managerImpl).
In `@management/server/store/sql_store.go`:
- Around line 5103-5115: The DeleteOldAccessLogs function performs a single
large DELETE which can cause lock/replication issues for big datasets; modify
SqlStore.DeleteOldAccessLogs (and use the accesslogs.AccessLogEntry model) to
delete in bounded batches: loop issuing repeated deletes scoped to the same
WHERE "timestamp < ?" but with an ORDER BY timestamp and a LIMIT (e.g.,
500–5000) using
s.db.WithContext(ctx).Where(...).Order("timestamp").Limit(batchSize).Delete(...),
accumulate RowsAffected until the last batch returns zero, and return the total
deleted count; ensure you preserve error handling (log the error and return
status.Internal) and that the loop breaks on result.Error or when rows deleted <
batchSize to avoid infinite loops.
Comment on lines
+29
to
+36
| DoAndReturn(func(ctx context.Context, olderThan time.Time) (int64, error) { | ||
| expectedCutoff := time.Now().AddDate(0, 0, -30) | ||
| timeDiff := olderThan.Sub(expectedCutoff) | ||
| if timeDiff.Abs() > time.Second { | ||
| t.Errorf("cutoff time not as expected: got %v, want ~%v", olderThan, expectedCutoff) | ||
| } | ||
| return 5, nil | ||
| }) |
Contributor
There was a problem hiding this comment.
Use t.Fatalf instead of t.Errorf inside DoAndReturn to stop execution on assertion failure.
t.Errorf marks the test failed but allows the callback to return 5, nil, which makes subsequent assertions pass even when the cutoff check failed. Use t.Fatalf to abort immediately.
🐛 Proposed fix
- if timeDiff.Abs() > time.Second {
- t.Errorf("cutoff time not as expected: got %v, want ~%v", olderThan, expectedCutoff)
- }
+ if timeDiff.Abs() > time.Second {
+ t.Fatalf("cutoff time not as expected: got %v, want ~%v", olderThan, expectedCutoff)
+ }Apply the same fix to the identical pattern at Line 108 in TestCleanupWithExactBoundary.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| DoAndReturn(func(ctx context.Context, olderThan time.Time) (int64, error) { | |
| expectedCutoff := time.Now().AddDate(0, 0, -30) | |
| timeDiff := olderThan.Sub(expectedCutoff) | |
| if timeDiff.Abs() > time.Second { | |
| t.Errorf("cutoff time not as expected: got %v, want ~%v", olderThan, expectedCutoff) | |
| } | |
| return 5, nil | |
| }) | |
| DoAndReturn(func(ctx context.Context, olderThan time.Time) (int64, error) { | |
| expectedCutoff := time.Now().AddDate(0, 0, -30) | |
| timeDiff := olderThan.Sub(expectedCutoff) | |
| if timeDiff.Abs() > time.Second { | |
| t.Fatalf("cutoff time not as expected: got %v, want ~%v", olderThan, expectedCutoff) | |
| } | |
| return 5, nil | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go`
around lines 29 - 36, In the DoAndReturn callback used in the tests (e.g.,
inside the TestCleanup test and the identical callback in
TestCleanupWithExactBoundary) replace t.Errorf(...) with t.Fatalf(...) so the
test aborts immediately on the cutoff-time assertion failure; update the
anonymous DoAndReturn funcs that currently compute expectedCutoff/timeDiff and
call t.Errorf to call t.Fatalf with the same message, ensuring the callback does
not return a success value after a failed assertion.
Comment on lines
197
to
207
| func (s *BaseServer) AccessLogsManager() accesslogs.Manager { | ||
| return Create(s, func() accesslogs.Manager { | ||
| accessLogManager := accesslogsmanager.NewManager(s.Store(), s.PermissionsManager(), s.GeoLocationManager()) | ||
| accessLogManager.StartPeriodicCleanup( | ||
| context.Background(), | ||
| s.Config.ReverseProxy.AccessLogRetentionDays, | ||
| s.Config.ReverseProxy.AccessLogCleanupIntervalHours, | ||
| ) | ||
| return accessLogManager | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the directory structure and locate the file
find . -name "boot.go" -type f | head -20Repository: netbirdio/netbird
Length of output: 98
🏁 Script executed:
# Search for StopPeriodicCleanup usage in the codebase
rg -n "StopPeriodicCleanup" --type go -A 2 -B 2Repository: netbirdio/netbird
Length of output: 3862
🏁 Script executed:
# Check the boot.go file around the AccessLogsManager function and line 126 for proxyAuthClose
rg -n "AccessLogsManager|proxyAuthClose" management/internals/server/boot.go -A 5 -B 2Repository: netbirdio/netbird
Length of output: 2221
🏁 Script executed:
# Look at the BaseServer struct definition
rg -n "type BaseServer struct" --type go -A 20Repository: netbirdio/netbird
Length of output: 1564
🏁 Script executed:
# Search for BaseServer Close or Shutdown methods
rg -n "func.*BaseServer.*Close|func.*BaseServer.*Shutdown" --type go -A 10Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Check the full boot.go file to understand the context better
wc -l management/internals/server/boot.goRepository: netbirdio/netbird
Length of output: 100
🏁 Script executed:
# Check the full BaseServer struct definition
cat -n management/internals/server/server.go | head -100Repository: netbirdio/netbird
Length of output: 3834
🏁 Script executed:
# Search for BaseServer Close method
rg -n "func.*Close|func.*Shutdown" management/internals/server/server.go -A 15Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Search for where proxyAuthClose is actually used/called
rg -n "proxyAuthClose" management/internals/server/ -A 3 -B 3Repository: netbirdio/netbird
Length of output: 1754
🏁 Script executed:
# Look at the full AccessLogsManager function in context
sed -n '195,210p' management/internals/server/boot.goRepository: netbirdio/netbird
Length of output: 610
🏁 Script executed:
# Get full Stop method
sed -n '218,235p' management/internals/server/server.goRepository: netbirdio/netbird
Length of output: 460
🏁 Script executed:
# Check the full boot.go to see how Create pattern works
rg -n "func Create" management/internals/server/boot.go -A 10Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Check if AccessLogsManager is cached/stored
rg -n "accessLogsManager|AccessLogsManager" management/internals/server/ --type goRepository: netbirdio/netbird
Length of output: 950
context.Background() + missing StopPeriodicCleanup() wiring causes a goroutine leak
StartPeriodicCleanup is passed context.Background(), so its internal goroutine can only be stopped via StopPeriodicCleanup(). However, StopPeriodicCleanup is never called in any production shutdown path. This results in:
- Goroutine leak during graceful server shutdown — the cleanup ticker goroutine runs until the OS kills the process.
- Test interference — each test that initializes
AccessLogsManagerspawns a goroutine that outlives the test.
The proxyAuthClose pattern at line 126 demonstrates the existing precedent for wiring cleanup into server shutdown. StopPeriodicCleanup must follow the same pattern: store the manager instance and call it in the Stop() method.
🔧 Suggested fix
Add an accessLogManager field to BaseServer and wire StopPeriodicCleanup into the shutdown sequence:
type BaseServer struct {
// ... existing fields
proxyAuthClose func()
+ accessLogManager accesslogs.Manager
}Then in AccessLogsManager():
func (s *BaseServer) AccessLogsManager() accesslogs.Manager {
return Create(s, func() accesslogs.Manager {
+ if s.accessLogManager == nil {
accessLogManager := accesslogsmanager.NewManager(s.Store(), s.PermissionsManager(), s.GeoLocationManager())
accessLogManager.StartPeriodicCleanup(
context.Background(),
s.Config.ReverseProxy.AccessLogRetentionDays,
s.Config.ReverseProxy.AccessLogCleanupIntervalHours,
)
+ s.accessLogManager = accessLogManager
+ }
- return accessLogManager
+ return s.accessLogManager
})
}And in the Stop() method (after ReverseProxyGRPCServer().Close()):
if s.proxyAuthClose != nil {
s.proxyAuthClose()
s.proxyAuthClose = nil
}
+if s.accessLogManager != nil {
+ s.accessLogManager.StopPeriodicCleanup()
+}
_ = s.Store().Close(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/internals/server/boot.go` around lines 197 - 207,
AccessLogsManager currently starts a long‑running cleanup goroutine with
StartPeriodicCleanup(context.Background(), ...) and never wires
StopPeriodicCleanup into shutdown, causing goroutine leaks; fix it by adding an
accessLogManager field to BaseServer, have AccessLogsManager() store the created
accesslogsmanager.Manager instance into that field (instead of only returning
it), and call its StopPeriodicCleanup(ctx) from BaseServer.Stop() (following the
proxyAuthClose pattern) so the cleanup goroutine is cancelled during graceful
shutdown and in tests; reference BaseServer, AccessLogsManager(),
StartPeriodicCleanup, StopPeriodicCleanup, proxyAuthClose, and Stop().
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
management/server/http/handlers/proxy/auth_callback_integration_test.go (1)
160-170: Nit: align unused-parameter naming and drop superfluousreturnstatements.The pre-existing mock methods in this struct (lines 172, 176) blank-identify every unused parameter with
_. The two new methods that acceptctxneither use it nor blank it, creating a minor style inconsistency. The barereturnat the end of void functions is also redundant.♻️ Proposed cleanup
-func (m *testAccessLogManager) CleanupOldAccessLogs(ctx context.Context, retentionDays int) (int64, error) { +func (m *testAccessLogManager) CleanupOldAccessLogs(_ context.Context, _ int) (int64, error) { return 0, nil } -func (m *testAccessLogManager) StartPeriodicCleanup(ctx context.Context, retentionDays, cleanupIntervalHours int) { - return +func (m *testAccessLogManager) StartPeriodicCleanup(_ context.Context, _, _ int) { } func (m *testAccessLogManager) StopPeriodicCleanup() { - return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/proxy/auth_callback_integration_test.go` around lines 160 - 170, Align unused-parameter naming and drop redundant returns: update testAccessLogManager methods to blank-identify unused ctx and other unused params (e.g., change CleanupOldAccessLogs(ctx context.Context, retentionDays int) to CleanupOldAccessLogs(_ context.Context, _ int) and StartPeriodicCleanup(ctx context.Context, retentionDays, cleanupIntervalHours int) to StartPeriodicCleanup(_ context.Context, _ int, _ int)), and remove the superfluous bare "return" statements from StartPeriodicCleanup and StopPeriodicCleanup while keeping the necessary return in CleanupOldAccessLogs (return 0, nil).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@management/server/http/handlers/proxy/auth_callback_integration_test.go`:
- Around line 160-170: Align unused-parameter naming and drop redundant returns:
update testAccessLogManager methods to blank-identify unused ctx and other
unused params (e.g., change CleanupOldAccessLogs(ctx context.Context,
retentionDays int) to CleanupOldAccessLogs(_ context.Context, _ int) and
StartPeriodicCleanup(ctx context.Context, retentionDays, cleanupIntervalHours
int) to StartPeriodicCleanup(_ context.Context, _ int, _ int)), and remove the
superfluous bare "return" statements from StartPeriodicCleanup and
StopPeriodicCleanup while keeping the necessary return in CleanupOldAccessLogs
(return 0, nil).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
proxy/management_integration_test.go (1)
172-178: Redundant barereturnin void stubs.In Go, a bare
returnat the end of a function with no return values is unnecessary. Removing it is more idiomatic.♻️ Remove redundant `return` statements
func (m *testAccessLogManager) StartPeriodicCleanup(ctx context.Context, retentionDays, cleanupIntervalHours int) { - return } func (m *testAccessLogManager) StopPeriodicCleanup() { - return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/management_integration_test.go` around lines 172 - 178, Remove the redundant bare "return" statements from the void stub methods on testAccessLogManager: delete the trailing "return" in StartPeriodicCleanup(ctx context.Context, retentionDays, cleanupIntervalHours int) and in StopPeriodicCleanup() so the functions are empty bodies (just {}). This makes the implementations idiomatic Go while keeping the method signatures and behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@proxy/management_integration_test.go`:
- Around line 172-178: Remove the redundant bare "return" statements from the
void stub methods on testAccessLogManager: delete the trailing "return" in
StartPeriodicCleanup(ctx context.Context, retentionDays, cleanupIntervalHours
int) and in StopPeriodicCleanup() so the functions are empty bodies (just {}).
This makes the implementations idiomatic Go while keeping the method signatures
and behavior unchanged.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
proxy/management_integration_test.go (1)
168-178: LGTM — correct no-op stubs to satisfy the expanded interface.The three new methods are minimal, consistent with each other, and pair correctly:
StartPeriodicCleanupstarts nothing, soStopPeriodicCleanuphaving a no-op body is sound.CleanupOldAccessLogsreturning(0, nil)is the appropriate zero-value stub for anint64, errorreturn.One cosmetic note: the existing
SaveAccessLogandGetAllAccessLogsuse blank identifiers (_) for unused parameters, while the new methods name them (ctx,retentionDays, etc.). Either style is fine in Go, but aligning with the convention already established in this type would improve consistency.🔧 Optional: align with existing blank-identifier convention
-func (m *testAccessLogManager) CleanupOldAccessLogs(ctx context.Context, retentionDays int) (int64, error) { +func (m *testAccessLogManager) CleanupOldAccessLogs(_ context.Context, _ int) (int64, error) { return 0, nil } -func (m *testAccessLogManager) StartPeriodicCleanup(ctx context.Context, retentionDays, cleanupIntervalHours int) { +func (m *testAccessLogManager) StartPeriodicCleanup(_ context.Context, _, _ int) { // noop }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/management_integration_test.go` around lines 168 - 178, Align parameter naming with existing methods that use blank identifiers: update the stub methods CleanupOldAccessLogs, StartPeriodicCleanup, and StopPeriodicCleanup in testAccessLogManager to use blank identifiers for unused parameters (e.g., replace ctx, retentionDays, cleanupIntervalHours with _ where applicable) so they match the style used by SaveAccessLog and GetAllAccessLogs and keep bodies as no-ops/zero-values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@proxy/management_integration_test.go`:
- Around line 168-178: Align parameter naming with existing methods that use
blank identifiers: update the stub methods CleanupOldAccessLogs,
StartPeriodicCleanup, and StopPeriodicCleanup in testAccessLogManager to use
blank identifiers for unused parameters (e.g., replace ctx, retentionDays,
cleanupIntervalHours with _ where applicable) so they match the style used by
SaveAccessLog and GetAllAccessLogs and keep bodies as no-ops/zero-values.
|
crn4
approved these changes
Feb 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Configuration
Tests