Skip to content

feat: add simple retries for status requests #65

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pboros merged 1 commit into from
Jan 23, 2026
Merged

feat: add simple retries for status requests #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 28 additions & 8 deletions db/conn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package db
import (
"bufio"
"encoding/json"
"fmt"
"os"
"regexp"
"strconv"
Expand Down Expand Up @@ -100,7 +101,18 @@ func getFdb() fdb.Database {
return db
}

func GetStatus() (*models.FullStatus, error) {
func IsStatusIncomplete(status *models.FullStatus) bool {
if status.Cluster == nil ||
status.Cluster.DatabaseLockState == nil ||
status.Cluster.FaultTolerance == nil ||
status.Cluster.Data == nil ||
status.Cluster.DatabaseAvailable == false {
return true
}
return false
}

func getStatusOnce() (*models.FullStatus, error) {
start := time.Now()
conn := getFdb()
var status models.FullStatus
Expand All @@ -122,11 +134,7 @@ func GetStatus() (*models.FullStatus, error) {
}

if os.Getenv("DEBUG_LOG_INCOMPLETE_STATUS") == "true" {
if status.Cluster == nil ||
status.Cluster.DatabaseLockState == nil ||
status.Cluster.FaultTolerance == nil ||
status.Cluster.Data == nil ||
status.Cluster.DatabaseAvailable == false {
if IsStatusIncomplete(&status) {
statusString := string(statusJson.([]byte))
log.Debug().Str("status_json", statusString).Msg("status json is missing cluster fields")

Expand All @@ -135,13 +143,25 @@ func GetStatus() (*models.FullStatus, error) {
Dur("took", d).
Str("status_json", statusString).
Msg("status json is missing cluster fields")

return nil, fmt.Errorf("incomplete status")

@greptile-apps greptile-apps Bot Jan 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: incomplete status now returns error when DEBUG_LOG_INCOMPLETE_STATUS is set, which triggers retry - verify this behavior change is intentional since previously it would return the incomplete status without error. was the previous behavior (returning incomplete status without error) a bug, or is this new retry behavior a breaking change?

Prompt To Fix With AI 
This is a comment left during a code review.
Path: db/conn.go
Line: 146:146

Comment:
**style:** incomplete status now returns error when `DEBUG_LOG_INCOMPLETE_STATUS` is set, which triggers retry - verify this behavior change is intentional since previously it would return the incomplete status without error. was the previous behavior (returning incomplete status without error) a bug, or is this new retry behavior a breaking change?

How can I resolve this? If you propose a fix, please make it concise.

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This early return inside the DEBUG_LOG_INCOMPLETE_STATUS check means incomplete status is only treated as an error when debug logging is enabled. When the environment variable is not set to 'true', the function proceeds to return the incomplete status without error, which is inconsistent behavior. The incomplete status check and error return should be outside the debug logging conditional block.

Copilot uses AI. Check for mistakes.
}
}

return &status, nil
}

func GetStatus() (*models.FullStatus, error) {
status, err := getStatusOnce()
if err != nil {
log.Error().Err(err).Msg("failed to get status, retrying")
time.Sleep(1 * time.Second)

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry delay of 1 second is a magic number hardcoded in the implementation. Consider extracting this as a named constant or configurable parameter to improve maintainability and make it easier to adjust the retry behavior.

Copilot uses AI. Check for mistakes.
status, err = getStatusOnce()
if err != nil {
log.Error().Err(err).Msg("failed to get status, retry failed")
}
}
return status, err
}
Comment on lines +152 to +163

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new retry logic in GetStatus lacks test coverage. Tests should verify the retry behavior when getStatusOnce fails on the first attempt but succeeds on retry, and when both attempts fail.

Copilot uses AI. Check for mistakes.

func isTLSmode(c *string) bool {
// Find if must run in TLS mode
file, err := os.Open(*c)
Expand Down
4 changes: 1 addition & 3 deletions metrics/metric_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,8 @@ func (m *MetricProvider) Close() {
}
}

// Periodic data collection, called from main in a goroutine
func (mp *MetricProvider) Collect() {
// TODO make this configurable
interval := 10 * time.Second
interval := 4 * time.Second

@greptile-apps greptile-apps Bot Jan 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: collection interval reduced from 10s to 4s - combined with the 1s retry delay in GetStatus(), overlapping collection attempts could occur if status requests consistently take >4s

Suggested change
interval := 4 * time.Second
interval := 6 * time.Second
Prompt To Fix With AI 
This is a comment left during a code review.
Path: metrics/metric_provider.go
Line: 58:58

Comment:
**style:** collection interval reduced from 10s to 4s - combined with the 1s retry delay in `GetStatus()`, overlapping collection attempts could occur if status requests consistently take >4s

```suggestion
	interval := 6 * time.Second
```

How can I resolve this? If you propose a fix, please make it concise.

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The collection interval of 4 seconds is a magic number. While this replaces a previous TODO about making it configurable, consider implementing that configurability or at least extracting this as a named constant to document why this specific interval was chosen.

Copilot uses AI. Check for mistakes.
ticker := time.NewTicker(interval)

if err := mp.reporter.collectOnce(); err != nil {
Expand Down