Propagate bacerrors over nclprotocol #4774
Conversation
WalkthroughThe pull request introduces a comprehensive update to error handling across multiple packages in the Bacalhau project. The changes primarily focus on implementing a new error serialization mechanism using the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Encoder
participant ErrorHandler
participant Responder
Client->>Encoder: Send Request
Encoder->>ErrorHandler: Process Request
alt Request Successful
ErrorHandler-->>Encoder: Return Response
else Request Fails
ErrorHandler->>Responder: Create Bacalhau Error
Responder->>Encoder: Serialize Error
Encoder->>Client: Return Serialized Error
end
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
🔭 Outside diff range comments (2)
pkg/lib/ncl/encoder.go (1)
Line range hint
44-48: Fix error handling logicThe error handling seems incorrect. The condition checks for
ErrAlreadyRegisteredbut returns an error in that case while ignoring other errors.if err := config.messageRegistry.Register(BacErrorMessageType, bacerrors.New("")); err != nil { - if errors.Is(err, envelope.ErrAlreadyRegistered{}) { + if !errors.Is(err, envelope.ErrAlreadyRegistered{}) { return nil, fmt.Errorf("failed to register error response type: %w", err) } }pkg/lib/ncl/responder.go (1)
Line range hint
146-162: Fix error message formatting.The error message in
bacerrors.Newuses a format string but the arguments are passed incorrectly.Apply this fix:
- r.sendErrorResponse(requestMsg, bacerrors.New("no handler found for message type: %s", messageType). + r.sendErrorResponse(requestMsg, bacerrors.Newf("no handler found for message type: %s", messageType).
🧹 Nitpick comments (9)
pkg/bacerrors/json.go (2)
7-17: LGTM! Consider enhancing documentation.The
JSONErrorstruct is well-structured with appropriate fields and JSON tags. Consider adding field-level documentation to describe the purpose of each field, especially for fields likeFailsExecutionwhere the implications might not be immediately obvious.
33-50: Consider adding validation for critical fields.While the unmarshaling implementation is correct, consider adding validation for critical fields like
HTTPStatusCode(should be > 0) andCode(should be a validErrorCode). This would prevent invalid error states.func (e *errorImpl) UnmarshalJSON(data []byte) error { var je JSONError if err := json.Unmarshal(data, &je); err != nil { return err } + + // Validate critical fields + if je.HTTPStatusCode <= 0 { + return fmt.Errorf("invalid HTTP status code: %d", je.HTTPStatusCode) + } + if !je.Code.IsValid() { // Assuming ErrorCode has a validation method + return fmt.Errorf("invalid error code: %s", je.Code) + } e.cause = je.Cause e.hint = je.Hintpkg/lib/ncl/error_response.go (1)
21-29: Consider using a consistent time source.The
BacErrorToEnvelopefunction usestime.Now()directly, which could lead to inconsistencies in distributed systems. Consider accepting the event time as a parameter or using a centralized time source.-func BacErrorToEnvelope(err bacerrors.Error) *envelope.Message { +func BacErrorToEnvelope(err bacerrors.Error, eventTime time.Time) *envelope.Message { errMsg := envelope.NewMessage(err) errMsg.WithMetadataValue(envelope.KeyMessageType, BacErrorMessageType) errMsg.WithMetadataValue(KeyStatusCode, fmt.Sprintf("%d", err.HTTPStatusCode())) errMsg.WithMetadataValue(KeyErrorCode, string(err.Code())) - errMsg.WithMetadataValue(KeyEventTime, time.Now().Format(time.RFC3339)) + errMsg.WithMetadataValue(KeyEventTime, eventTime.Format(time.RFC3339)) return errMsg }pkg/transport/nclprotocol/orchestrator/errors.go (1)
8-14: LGTM! Consider adding error documentation.The implementation correctly extends the base error with component information. Consider adding a package-level documentation comment explaining the error types and their usage patterns in this package.
pkg/bacerrors/json_test.go (2)
75-81: Enhance invalid JSON test casesThe current invalid JSON test only covers one scenario. Consider adding more test cases:
- Missing required fields
- Invalid field types for other fields
- Malformed JSON syntax
- Empty JSON object
func TestErrorJSONMarshallingInvalid(t *testing.T) { - // Test unmarshalling invalid JSON - invalidJSON := []byte(`{"Cause": "test", "Retryable": "invalid"}`) - var unmarshaled errorImpl - err := json.Unmarshal(invalidJSON, &unmarshaled) - assert.Error(t, err, "Should fail to unmarshal invalid JSON") + testCases := []struct { + name string + json string + }{ + { + name: "invalid boolean", + json: `{"Cause": "test", "Retryable": "invalid"}`, + }, + { + name: "missing required cause", + json: `{"Retryable": true}`, + }, + { + name: "malformed json", + json: `{"Cause": "test" "Retryable": true}`, + }, + { + name: "empty object", + json: `{}`, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var unmarshaled errorImpl + err := json.Unmarshal([]byte(tc.json), &unmarshaled) + assert.Error(t, err, "Should fail to unmarshal invalid JSON") + }) + } }
13-47: Add test for error chain serializationThe current test doesn't verify how wrapped errors are handled during serialization. Consider adding test cases for:
- Wrapped errors with multiple levels
- Circular error references
pkg/lib/ncl/encoder.go (1)
44-44: Reconsider using empty string in bacerrors.NewUsing
bacerrors.New("")for type registration seems questionable. Consider:
- Using a meaningful placeholder message
- Creating a dedicated method for type registration that doesn't require a message
- if err := config.messageRegistry.Register(BacErrorMessageType, bacerrors.New("")); err != nil { + if err := config.messageRegistry.Register(BacErrorMessageType, bacerrors.New("type_registration_placeholder")); err != nil {pkg/lib/ncl/publisher.go (1)
98-98: Avoid using empty string in bacerrors.NewSimilar to the encoder, using
bacerrors.New("")for type checking seems questionable.- if errorResponse, ok := message.GetPayload(bacerrors.New("")); ok { + if errorResponse, ok := message.GetPayload(bacerrors.New("type_check_placeholder")); ok {pkg/transport/nclprotocol/compute/controlplane.go (1)
Line range hint
109-113: Consider enhancing error handling in the heartbeat method.While the handshake error check is improved, consider wrapping the heartbeat error with bacerrors for consistency.
Consider applying this enhancement:
- log.Error().Err(err).Msg("Failed to send heartbeat") + log.Error().Err(bacerrors.Wrap(err, "heartbeat failed")).Msg("Failed to send heartbeat")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pkg/bacerrors/json.go(1 hunks)pkg/bacerrors/json_test.go(1 hunks)pkg/lib/ncl/encoder.go(2 hunks)pkg/lib/ncl/encoder_test.go(2 hunks)pkg/lib/ncl/error_response.go(1 hunks)pkg/lib/ncl/publisher.go(2 hunks)pkg/lib/ncl/responder.go(5 hunks)pkg/transport/nclprotocol/compute/controlplane.go(2 hunks)pkg/transport/nclprotocol/orchestrator/errors.go(1 hunks)pkg/transport/nclprotocol/orchestrator/manager.go(3 hunks)
🔇 Additional comments (9)
pkg/bacerrors/json.go (1)
19-31: LGTM!
The MarshalJSON implementation correctly maps all fields from errorImpl to JSONError for serialization.
pkg/lib/ncl/encoder_test.go (2)
11-11: LGTM: Import aligns with the new error handling approach.
The addition of the bacerrors package import supports the transition to structured error handling.
181-192: LGTM: Test updates properly validate the new error handling mechanism.
The test changes effectively verify:
- Error creation with specific error codes
- Proper encoding/decoding of structured errors
- Correct error code and message preservation
pkg/lib/ncl/responder.go (2)
187-196: LGTM: Improved error handling for response encoding failures.
The changes properly handle encoding failures differently for error responses vs normal responses, preventing infinite error response loops.
213-214: LGTM: Clean signature update for error handling.
The method signature change to use bacerrors.Error aligns with the new error handling approach.
pkg/transport/nclprotocol/compute/controlplane.go (1)
11-11: LGTM: Imports support improved error handling.
The addition of bacerrors and nodes packages properly supports the new error handling mechanism.
Also applies to: 16-16
pkg/transport/nclprotocol/orchestrator/manager.go (3)
251-251: LGTM: Improved error handling for missing data plane
The change from string formatting to NewErrHandshakeRequired provides more structured error handling while maintaining the same logical flow.
274-274: LGTM: Consistent error handling approach
The change maintains consistency with other handlers by using NewErrHandshakeRequired for missing data plane scenarios.
296-296: Verify handshake requirement during shutdown
While the change to NewErrHandshakeRequired maintains consistency with other handlers, please verify if requiring a handshake during shutdown is the intended behavior. A node might legitimately need to send a shutdown notice even if its data plane is no longer active.
Let's verify the shutdown behavior in the codebase:
Summary by CodeRabbit
New Features
bacerrorspackage.Bug Fixes
Tests
Chores
bacerrorspackage.