feat(hosterrorscache): add Remove and MarkFailedOrRemove methods
#5984
Conversation
and also deprecating `MarkFailed` Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
WalkthroughThis pull request introduces comprehensive modifications to error handling and host error caching across multiple packages. The changes focus on refining the logic for marking host failures, removing redundant error checks, and ensuring consistent error tracking. The modifications span several files in the Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
🔇 Additional comments (11)
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (6)
pkg/protocols/common/hosterrorscache/hosterrorscache.go (1)
147-151: Consider logging cache removal errors in verbose mode.The implementation is clean, but for debugging purposes, it would be helpful to log removal errors when verbose mode is enabled.
func (c *Cache) Remove(ctx *contextargs.Context) { key := c.GetKeyFromContext(ctx, nil) - _ = c.failedTargets.Remove(key) // remove even the cache is not present + if err := c.failedTargets.Remove(key); err != nil && c.verbose { + gologger.Verbose().Msgf("Failed to remove %s from cache: %v", key, err) + } }pkg/protocols/common/hosterrorscache/hosterrorscache_test.go (2)
22-29: Enhance test readability with descriptive error messages.The test logic is correct, but the failure messages could be more descriptive.
- require.Falsef(t, got, "got %v in iteration %d", got, i) + require.Falsef(t, got, "expected host not to be flagged after %d failure(s), but it was", i)
89-101: Consider adding edge cases to the Remove test.While the current test is good, consider adding cases for:
- Removing a non-existent host
- Removing a host with permanent error
- Concurrent removal operations
pkg/tmplexec/generic/exec.go (1)
89-91: Consider extracting common error handling logic.The error marking logic is duplicated across workflow and generic executors. Consider extracting this into a common helper function to maintain consistency and reduce duplication.
Example helper function:
func markExecutorError(cache CacheInterface, protoType string, input *contextargs.Context, err error) { if cache != nil { cache.MarkFailed(protoType, input, err) } }pkg/templates/cluster.go (1)
312-314: Duplicate error handling pattern.This is a duplicate of the error handling pattern from the
Executemethod. Consider extracting this common logic into a shared helper method to maintain DRY principles.Apply this diff to extract the common logic:
+func (request *Request) markHostErrors(input *contextargs.Context, err error) { + if request.options.HostErrorsCache != nil { + request.options.HostErrorsCache.MarkFailed(request.options.ProtocolType.String(), input, err) + } +} + func (request *Request) Execute(ctx *scan.ScanContext) (bool, error) { // ... - if request.options.HostErrorsCache != nil { - request.options.HostErrorsCache.MarkFailed(request.options.ProtocolType.String(), ctx.Input, err) - } + request.markHostErrors(ctx.Input, err) return results, err } func (request *Request) ExecuteWithResults(ctx *scan.ScanContext) ([]*output.ResultEvent, error) { // ... - if request.options.HostErrorsCache != nil { - request.options.HostErrorsCache.MarkFailed(request.options.ProtocolType.String(), ctx.Input, err) - } + request.markHostErrors(ctx.Input, err) return scanCtx.GenerateResult(), err }pkg/protocols/network/request.go (1)
294-295: Improve comment accuracy.The comment "adds it to unresponsive address list if applicable" is outdated and doesn't reflect the new error handling approach.
Apply this diff to update the comment:
- // adds it to unresponsive address list if applicable + // marks host error in cache if applicable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/core/workflow_execute.go(1 hunks)pkg/protocols/common/hosterrorscache/hosterrorscache.go(4 hunks)pkg/protocols/common/hosterrorscache/hosterrorscache_test.go(5 hunks)pkg/protocols/http/request.go(5 hunks)pkg/protocols/http/request_fuzz.go(1 hunks)pkg/protocols/network/request.go(2 hunks)pkg/templates/cluster.go(2 hunks)pkg/tmplexec/generic/exec.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests (windows-latest)
🔇 Additional comments (7)
pkg/protocols/common/hosterrorscache/hosterrorscache.go (2)
24-29: LGTM! Well-structured interface changes.The interface changes are clean and maintain backward compatibility while introducing new functionality.
160-223: Excellent implementation with clear documentation and thread-safety!The implementation is robust with:
- Comprehensive documentation explaining the removal strategy
- Proper thread-safety using mutex
- Clear error handling logic
pkg/core/workflow_execute.go (1)
99-101: LGTM! Consistent error handling implementation.The error marking is correctly placed after executer execution, maintaining consistency with the new error handling strategy.
pkg/templates/cluster.go (1)
276-278: Verify error handling behavior.The error handling logic has been updated to mark host errors in the cache regardless of the error's presence. This change aligns with the PR's objective to enhance error handling, but we should verify that this doesn't lead to false positives.
Run the following script to check for similar patterns in other files:
✅ Verification successful
Error handling implementation is correct and safe
The error handling in HostErrorsCache is properly implemented with built-in safeguards:
- Only tracks errors for HTTP protocol after validation
- Automatically removes host from cache when error is nil
- Uses atomic operations for thread-safety
- Has protection against false positives through error type checking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent error handling patterns across the codebase # Look for similar patterns where HostErrorsCache.MarkFailed is called ast-grep --pattern 'if $cache != nil { $cache.MarkFailed($_, $_, $_) }'Length of output: 75
Script:
#!/bin/bash # Search for MarkFailed method calls and their context rg "MarkFailed" -A 3 -B 3 # Find HostErrorsCache type definition and usage rg "type.*HostErrorsCache" -A 5 # Look for similar error handling patterns with nil checks ast-grep --pattern 'if $_ != nil { $$$ }'Length of output: 65730
Script:
#!/bin/bash # Check MarkFailedOrRemove implementation and error handling rg "MarkFailedOrRemove" -A 10 -B 2 # Look for error handling in cache implementation rg "func.*Cache" -A 5 pkg/protocols/common/hosterrorscache/Length of output: 11817
pkg/protocols/http/request_fuzz.go (1)
228-230: Consistent error handling pattern.The error handling logic follows the same pattern as in
cluster.go, marking host errors in the cache regardless of the error's presence. This consistency is good for maintainability.pkg/protocols/network/request.go (1)
Line range hint
527-532: Consistent method naming and documentation.The method has been renamed from
markUnresponsiveAddresstomarkHostErrorto better reflect its purpose. The comment accurately describes the function's behavior.pkg/protocols/http/request.go (1)
Line range hint
1193-1198: Consistent method implementation.The implementation of
markHostErroris consistent with the renamed methods in other files, which is good for maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/protocols/common/hosterrorscache/hosterrorscache_test.go (2)
173-191: Consider enhancing concurrent test coverage.While the test effectively verifies concurrent marking and checking, consider adding assertions to verify the final error count after all goroutines complete.
} wg.Wait() + + // Verify final error count + normalizedCacheValue := cache.GetKeyFromContext(ctx, nil) + failedTarget, err := cache.failedTargets.Get(normalizedCacheValue) + require.Nil(t, err) + require.NotNil(t, failedTarget) + require.EqualValues(t, 100, failedTarget.errors.Load()) }
89-89: Consider adding edge case tests for Remove.Consider adding test cases for edge scenarios of the Remove method, such as:
- Removing a non-existent host
- Removing a host multiple times
- Concurrent removal of the same host
+func TestRemoveEdgeCases(t *testing.T) { + cache := New(3, DefaultMaxHostsCount, nil) + ctx := newCtxArgs(t.Name()) + + // Test removing non-existent host + cache.Remove(ctx) + require.False(t, cache.Check(protoType, ctx)) + + // Test multiple removals + cache.MarkFailed(protoType, ctx, errors.New("error")) + cache.Remove(ctx) + cache.Remove(ctx) + require.False(t, cache.Check(protoType, ctx)) + + // Test concurrent removal + cache.MarkFailed(protoType, ctx, errors.New("error")) + wg := sync.WaitGroup{} + for i := 0; i < 10; i++ { + wg.Add(1) + go func() { + defer wg.Done() + cache.Remove(ctx) + }() + } + wg.Wait() + require.False(t, cache.Check(protoType, ctx)) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/protocols/common/hosterrorscache/hosterrorscache.go(4 hunks)pkg/protocols/common/hosterrorscache/hosterrorscache_test.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: perf-test (150)
- GitHub Check: perf-test (100)
- GitHub Check: perf-test (50)
- GitHub Check: Tests (windows-latest)
🔇 Additional comments (8)
pkg/protocols/common/hosterrorscache/hosterrorscache.go (6)
24-29: LGTM! Interface changes are well-structured.The new methods
RemoveandMarkFailedOrRemoveare clearly named and maintain backward compatibility.
53-53: LGTM! Thread-safety improvement.The addition of
sync.Mutexensures thread-safe access to the cache item's fields.
133-138: LGTM! Thread-safety and logging improvements.The addition of mutex locking ensures thread-safe access to cache items, and the error message now uses the appropriate verbose level for expected skips.
152-156: LGTM! Clean implementation of Remove method.The implementation is simple, focused, and handles the cache miss case appropriately.
159-163: LGTM! Clean deprecation approach.The method is properly marked as deprecated and maintains backward compatibility by delegating to
MarkFailedOrRemove.
165-225: LGTM! Well-implemented and thoroughly documented.The method effectively handles both error and non-error cases, maintains thread-safety, and includes comprehensive documentation explaining the removal strategy rationale.
pkg/protocols/common/hosterrorscache/hosterrorscache_test.go (2)
22-46: LGTM! Comprehensive test coverage.The new subtests effectively cover the key scenarios: error increment, flagging, and removal functionality.
89-101: LGTM! Clear and focused test.The test effectively verifies both the marking of failed hosts and their subsequent removal.
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Proposed changes
The decision was made to completely remove the cached entry for the host instead of simply decrementing the error$$N-1$$ ). This approach was chosen because the error handling logic operates concurrently, and decrementing the count could lead to UB (unexpected behavior) even when the error is
count (using
(atomic.Int32).Swapto update the value tonil.To clarify, consider the following scenario where the error encountered does NOT belong to the permanent network error category (
errkit.ErrKindNetworkPermanent):This removal strategy ensures the cache is updated dynamically to reflect the current state of the host without persisting stale or irrelevant error counts that could interfere with future error handling and tracking logic.
Possibly closes #4266
What's changed
hosterrorscache*Cache.MarkFailed, now it calls*Cache.MarkFailedOrRemove.*Cache.Removemethod.**Cache.MarkFailedunconditionally.Proof
Checklist
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
Tests
The updates focus on more robust error tracking and management across different protocol and workflow execution contexts.