increase timeout and retry #37
Conversation
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 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: 2
🧹 Nitpick comments (1)
pkg/servicenow/client.go (1)
604-606: Respect context cancellation during backoff waits.
time.Sleepignoresctxcancel/deadline signals, so a canceled sync still waits the full backoff interval (up to 6s here) before honoring the cancellation. Please gate the sleep with aselectonctx.Done()(same change for each retry loop).A possible update:
- delay := baseDelay * time.Duration(1<<attempt) - time.Sleep(delay) + delay := baseDelay * time.Duration(1<<attempt) + select { + case <-time.After(delay): + case <-ctx.Done(): + return nil, "", ctx.Err() + }Also applies to: 639-641, 672-674
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/connector/connector.go(2 hunks)pkg/servicenow/client.go(4 hunks)
🧰 Additional context used
🪛 GitHub Check: go-lint
pkg/connector/connector.go
[failure] 124-124:
File is not properly formatted (goimports)
pkg/servicenow/client.go
[failure] 598-598:
File is not properly formatted (goimports)
🪛 GitHub Actions: ci
pkg/connector/connector.go
[error] 124-124: golangci-lint: File is not properly formatted (goimports). Run 'goimports -w' or 'gofmt -w' to fix formatting.
⏰ 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: Cursor Bugbot
|
|
||
| httpClient.Timeout = 10 * time.Minute | ||
|
|
There was a problem hiding this comment.
Fix gofmt/goimports formatting before merging.
CI failed because this file is not gofmt/goimports formatted. Please run goimports -w pkg/connector/connector.go (or gofmt) and re-push so the pipeline passes.
🧰 Tools
🪛 GitHub Check: go-lint
[failure] 124-124:
File is not properly formatted (goimports)
🪛 GitHub Actions: ci
[error] 124-124: golangci-lint: File is not properly formatted (goimports). Run 'goimports -w' or 'gofmt -w' to fix formatting.
🤖 Prompt for AI Agents
In pkg/connector/connector.go around lines 124 to 126, the file is not formatted
with gofmt/goimports which breaks CI; run goimports -w
pkg/connector/connector.go (or gofmt -w pkg/connector/connector.go), ensure
imports are organized and the file is saved, then re-commit and push the change
so the pipeline can pass.
| for attempt := 0; attempt < maxRetries; attempt++ { | ||
| next, err := c.get(ctx, fmt.Sprintf(VariableSetM2MBaseUrl, c.deployment), &resp, req...) | ||
| if err != nil { | ||
| if status.Code(err) == codes.DeadlineExceeded || strings.Contains(err.Error(), "timeout") || strings.Contains(err.Error(), "deadline exceeded") { | ||
| if attempt < maxRetries-1 { | ||
| delay := baseDelay * time.Duration(1<<attempt) | ||
| time.Sleep(delay) | ||
| continue | ||
| } | ||
| } | ||
| return nil, "", err |
There was a problem hiding this comment.
Retry condition misses HTTP gateway/request timeouts.
doRequest wraps HTTP failures with status.Error(codes.Code(status), "Request failed"). For 504/408 responses, status.Code(err) becomes codes.Code(504)/codes.Code(408) and the message lacks "timeout" wording, so these new retries never trigger and we still fail immediately on the common ServiceNow timeout scenarios. Please treat those HTTP statuses as retryable (and apply the fix to all three functions).
Apply this diff (pattern repeated in the other two retry blocks):
- if err != nil {
- if status.Code(err) == codes.DeadlineExceeded || strings.Contains(err.Error(), "timeout") || strings.Contains(err.Error(), "deadline exceeded") {
+ if err != nil {
+ code := status.Code(err)
+ if code == codes.DeadlineExceeded ||
+ code == codes.Code(http.StatusGatewayTimeout) ||
+ code == codes.Code(http.StatusRequestTimeout) ||
+ strings.Contains(err.Error(), "timeout") ||
+ strings.Contains(err.Error(), "deadline exceeded") {Also applies to: 634-644, 667-677
🤖 Prompt for AI Agents
In pkg/servicenow/client.go around lines 599-609 (and similarly apply to 634-644
and 667-677): the retry condition only checks for gRPC DeadlineExceeded or text
containing "timeout"/"deadline exceeded", but doRequest wraps HTTP 504/408
responses into status.Errors with numeric codes (e.g. codes.Code(504),
codes.Code(408)) and no "timeout" text, so those timeouts never trigger a retry.
Update the conditional to also treat HTTP 504 and 408 as retryable by checking
status.Code(err) against codes.Code(504) and codes.Code(408) (in addition to
codes.DeadlineExceeded and the existing string checks), then keep the existing
exponential backoff and continue logic; apply the same change to the two other
retry blocks at the specified lines.
Summary by CodeRabbit