Log Errors from Targeted Sync, but don't stop the whole sync #474
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdjusts the main sync loop: when a Partial SyncTargetedResource operation returns a NotFound error and no retry is advised, the loop logs a targeted-resource not-found warning, finishes the state action, and continues to remaining actions instead of returning an error. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Syncer as Syncer Loop
participant Action as SyncTargetedResource Op
participant Retry as ShouldWaitAndRetry
participant Logger as Logger
Syncer->>Action: Execute Partial targeted-resource action
Action-->>Syncer: Error (NotFound)
Syncer->>Retry: ShouldWaitAndRetry(err)?
alt Retry advised
Retry-->>Syncer: true
Syncer->>Syncer: Wait and retry action
else No retry advised
Retry-->>Syncer: false
Syncer->>Logger: Log targeted-resource not-found warning
Syncer->>Syncer: Finish state action and continue loop
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)pkg/sync/syncer.go (1)
🪛 GitHub Check: go-lintpkg/sync/syncer.go[failure] 495-495: ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing touches
🧪 Generate unit tests
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 |
mj-palanker
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/sync/syncer.go(1 hunks)
⏰ 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). (1)
- GitHub Check: go-test (1.25.x, windows-latest)
pkg/sync/syncer.go
Outdated
| l.Error("max attempts reached for sync targeted resource action", zap.Any("stateAction", stateAction), zap.Error(err)) | ||
| // We want to continue here to make sure we finish other syncs | ||
| // we need a better solution for this. | ||
| continue |
There was a problem hiding this comment.
Finish the action (and record a warning) to avoid an infinite loop when swallowing the error.
You continue without s.state.FinishAction(ctx), so the same SyncTargetedResourceOp stays at the top of the stack and will be retried forever (since ShouldWaitAndRetry already returned false). Finish the action and optionally add the error to warnings so the global cap can trip if needed.
- if !retryer.ShouldWaitAndRetry(ctx, err) {
- l.Error("max attempts reached for sync targeted resource action", zap.Any("stateAction", stateAction), zap.Error(err))
- // We want to continue here to make sure we finish other syncs
- // we need a better solution for this.
- continue
- }
+ if !retryer.ShouldWaitAndRetry(ctx, err) {
+ l.Error("max attempts reached for sync targeted resource action", zap.Any("stateAction", stateAction), zap.Error(err))
+ // Treat as non-fatal: record and drop this action so the loop can progress.
+ warnings = append(warnings, err)
+ s.state.FinishAction(ctx)
+ continue
+ }📝 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.
| l.Error("max attempts reached for sync targeted resource action", zap.Any("stateAction", stateAction), zap.Error(err)) | |
| // We want to continue here to make sure we finish other syncs | |
| // we need a better solution for this. | |
| continue | |
| l.Error("max attempts reached for sync targeted resource action", zap.Any("stateAction", stateAction), zap.Error(err)) | |
| // Treat as non-fatal: record and drop this action so the loop can progress. | |
| warnings = append(warnings, err) | |
| s.state.FinishAction(ctx) | |
| continue |
🤖 Prompt for AI Agents
In pkg/sync/syncer.go around lines 496 to 499, the code currently continues
after logging max attempts reached which leaves the SyncTargetedResourceOp on
the stack and causes infinite retries; instead call s.state.FinishAction(ctx)
before continuing to remove the action from the stack, and record a warning
(e.g., append the error and context to the action/resource warnings list or call
the existing warnings API) so the failure is tracked and counts toward the
global warning cap.
| l.Warn("sync targeted resource not found, continuing to sync other resources", zap.Any("stateAction", stateAction), zap.Error(err)) | ||
| s.state.FinishAction(ctx) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Bug: Unreachable Error Handling in Sync Logic
The new if block for codes.NotFound errors in partial syncs is unreachable. The preceding isWarning(ctx, err) check already handles NotFound errors and continues, preventing this specific logic from executing. The codes.Code(status.Code(err)) conversion is also redundant.
Summary by CodeRabbit