Add dependabot grouping/auto-merge, unit-test CI gate, and status endpoint#382
Open
sallyom wants to merge 1 commit into
Open
Add dependabot grouping/auto-merge, unit-test CI gate, and status endpoint#382sallyom wants to merge 1 commit into
sallyom wants to merge 1 commit into
Conversation
Reviewer's GuideAdds an opt-in HTTP health/status endpoint integrated with method scheduling, introduces a fast Go vet + unit-test CI workflow, and configures Dependabot with grouped updates and auto-merge for minor/patch changes, along with README documentation for the new status server. Sequence diagram for the new HTTP health and status endpointssequenceDiagram
actor User
participant Fetchit as Fetchit.RunTargets
participant Status as statusRegistry
participant Scheduler as Scheduler
participant Method as Method
participant HTTP as HTTPServer
Note over Fetchit: Startup and scheduling
Fetchit->>Fetchit: RunTargets
Fetchit->>Fetchit: startStatusServer()
alt [FETCHIT_STATUS_ADDR set]
Fetchit->>HTTP: http.ListenAndServe(addr, mux)
else [FETCHIT_STATUS_ADDR unset]
Fetchit--xHTTP: no-op
end
loop for each method
Fetchit->>Status: status.register(method, schedule)
Fetchit->>Scheduler: Cron(schedule).Do(wrapper)
end
Note over Scheduler,Method: Scheduled execution
Scheduler->>Method: wrapper(ctx, conn, skew)
Method->>Status: status.recordRun(method)
Method->>Method: Process(ctx, conn, skew)
Note over User,HTTP: Status query
User->>HTTP: GET /status
HTTP->>Status: status.writeJSON(w)
Status-->>HTTP: JSON with uptime and methods
HTTP-->>User: 200 OK + status JSON
Note over User,HTTP: Health check
User->>HTTP: GET /healthz
HTTP-->>User: 200 OK
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 14 issues, and left some high level feedback:
- In
statusRegistry.writeJSON, if no methods have been registered yetr.startedremains the zero time, sotime.Since(started)will produce a large negative duration and odd uptime/startedAt values; consider guarding forstarted.IsZero()and returning a sensible default (e.g., uptime 0 and omitting/zeroingstartedAt) in that case.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `statusRegistry.writeJSON`, if no methods have been registered yet `r.started` remains the zero time, so `time.Since(started)` will produce a large negative duration and odd uptime/startedAt values; consider guarding for `started.IsZero()` and returning a sensible default (e.g., uptime 0 and omitting/zeroing `startedAt`) in that case.
## Individual Comments
### Comment 1
<location path="pkg/engine/status.go" line_range="69-78" />
<code_context>
+
+func (r *statusRegistry) writeJSON(w http.ResponseWriter) {
+ r.mu.Lock()
+ ms := make([]*methodStatus, 0, len(r.methods))
+ for _, s := range r.methods {
+ ms = append(ms, s)
+ }
+ started := r.started
+ r.mu.Unlock()
+
+ sort.Slice(ms, func(i, j int) bool {
+ if ms[i].Kind != ms[j].Kind {
+ return ms[i].Kind < ms[j].Kind
+ }
+ return ms[i].Name < ms[j].Name
+ })
+
+ resp := struct {
+ Status string `json:"status"`
+ StartedAt time.Time `json:"startedAt"`
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid data races between status mutation and JSON encoding
`writeJSON` collects `*methodStatus` under the mutex, then unlocks before sorting/encoding. While encoding, concurrent `register`/`recordRun` calls can mutate those structs, causing a data race. Either hold the lock through sorting and encoding, or copy each `methodStatus` to value types under the lock and encode the copies after unlocking.
</issue_to_address>
### Comment 2
<location path="pkg/engine/status.go" line_range="80-89" />
<code_context>
+ Methods []*methodStatus `json:"methods"`
+ }{
+ Status: "running",
+ StartedAt: started,
+ UptimeSeconds: int64(time.Since(started).Seconds()),
+ Methods: ms,
+ }
</code_context>
<issue_to_address>
**suggestion:** Handle the case where `started` is still zero in the status response
If `/status` is called before any method is registered, `r.started` will be the zero `time.Time`, so `time.Since(started)` will produce a huge duration (since year 1) and misleading `startedAt`/`uptimeSeconds` values. Please handle `started.IsZero()` explicitly (e.g., treat as "initializing" with zero/omitted uptime fields) until the first method is registered.
```suggestion
return ms[i].Name < ms[j].Name
})
status := "running"
var uptimeSeconds int64
if started.IsZero() {
status = "initializing"
uptimeSeconds = 0
} else {
uptimeSeconds = int64(time.Since(started).Seconds())
}
resp := struct {
Status string `json:"status"`
StartedAt time.Time `json:"startedAt"`
UptimeSeconds int64 `json:"uptimeSeconds"`
Methods []*methodStatus `json:"methods"`
}{
Status: status,
StartedAt: started,
UptimeSeconds: uptimeSeconds,
Methods: ms,
```
</issue_to_address>
### Comment 3
<location path="pkg/engine/status.go" line_range="104" />
<code_context>
+// startStatusServer launches the status/health HTTP server in a goroutine if
+// FETCHIT_STATUS_ADDR is set. It is a no-op otherwise.
+func startStatusServer() {
+ addr := os.Getenv(statusAddrEnv)
+ if addr == "" {
+ return
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The status server may unintentionally listen on all interfaces (e.g. with `:8080`), which is risky on multi-tenant or shared hosts; consider defaulting to localhost unless an explicit host is provided.
Because `FETCHIT_STATUS_ADDR` is passed directly to `http.ListenAndServe`, a common configuration like `:8080` will bind to all interfaces. On shared hosts or container platforms, this can expose `/status` and `/healthz` beyond the local machine, which is unnecessary for a liveness/status endpoint and can be considered a security hardening issue.
One way to make this safer by default while keeping the current flexibility is to treat a host-less address as localhost-only, and require an explicit host if an operator truly wants 0.0.0.0:
```go
func startStatusServer() {
addr := os.Getenv(statusAddrEnv)
if addr == "" {
return
}
// Hardening: default host-less addresses to localhost only.
if strings.HasPrefix(addr, ":") {
addr = "127.0.0.1" + addr
}
mux := http.NewServeMux()
mux.HandleFunc("/healthz", func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte("ok\n"))
})
mux.HandleFunc("/status", func(w http.ResponseWriter, _ *http.Request) {
status.writeJSON(w)
})
go func() {
logger.Infof("Status server listening on %s (/healthz, /status)", addr)
if err := http.ListenAndServe(addr, mux); err != nil {
logger.Errorf("status server error: %v", err)
}
}()
}
```
This preserves the current behavior when a full host:port is provided (e.g. `0.0.0.0:8080`), but avoids unintentionally exposing the status endpoints when users follow the `:8080` pattern from the README. If you prefer to keep the existing behavior, an alternative is to at least document that `:8080` will bind on all interfaces and recommend `127.0.0.1:8080` for local-only use.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Include comments that identify potential security risks and provide a safer alternative.
</details>
</issue_to_address>
### Comment 4
<location path="pkg/engine/status.go" line_range="96" />
<code_context>
+ }
+
+ w.Header().Set("Content-Type", "application/json")
+ enc := json.NewEncoder(w)
+ enc.SetIndent("", " ")
+ _ = enc.Encode(resp)
</code_context>
<issue_to_address>
**issue (review_instructions):** The error from enc.Encode(resp) is explicitly discarded, which can mask failures when writing the JSON response.
Currently the result of `enc.Encode(resp)` is ignored via `_ = enc.Encode(resp)`. This is similar in spirit to catching all exceptions and then continuing, because any failure to encode or write the response will be silently dropped.
To make the behavior clearer and safer, consider checking the returned error and either:
- logging it (at least at warning/error level), and/or
- returning an appropriate HTTP status code (e.g., 500) to the client.
That way, operational issues (e.g., a broken writer or encoding problem) are surfaced instead of being silently ignored.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Include comments that identify potential bug risks, such as catching all exceptions, which can mask important errors.
</details>
</issue_to_address>
### Comment 5
<location path="pkg/engine/status.go" line_range="109" />
<code_context>
+ return
+ }
+ mux := http.NewServeMux()
+ mux.HandleFunc("/healthz", func(w http.ResponseWriter, _ *http.Request) {
+ w.WriteHeader(http.StatusOK)
+ _, _ = w.Write([]byte("ok\n"))
</code_context>
<issue_to_address>
**issue (review_instructions):** The writes in the /healthz handler ignore write errors, which can silently hide I/O problems.
In the `/healthz` handler, the call to `w.Write` uses `_, _ = w.Write([]byte("ok\n"))`, which discards any error from writing the response. While health checks often are simple, silently ignoring I/O errors can hide underlying problems (e.g., client disconnects or broken connections), similar to catching all exceptions and continuing.
Consider at least logging the error from `w.Write` so that unexpected failures are visible. Alternatively, if you want to keep the handler minimal, a brief comment explaining why the error is intentionally ignored would clarify that this is a conscious choice rather than an oversight.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Include comments that identify potential bug risks, such as catching all exceptions, which can mask important errors.
</details>
</issue_to_address>
### Comment 6
<location path="pkg/engine/status.go" line_range="91" />
<code_context>
+ }{
+ Status: "running",
+ StartedAt: started,
+ UptimeSeconds: int64(time.Since(started).Seconds()),
+ Methods: ms,
+ }
</code_context>
<issue_to_address>
**issue (review_instructions):** `UptimeSeconds` is computed from `started` even when `started` is the zero time, so hitting `/status` before any methods are registered will return a huge, incorrect uptime value.
Because `started` is only set in `register`, if `/status` is called before any methods are registered then `r.started` is still the zero `time.Time`. In that case `time.Since(started)` is the duration since year 0, so `uptimeSeconds` will be an enormous, meaningless value rather than something close to zero.
Example: right after startup with `FETCHIT_STATUS_ADDR` set but before any method registration, a user hitting `/status` would see `uptimeSeconds` on the order of billions of seconds. You could guard against this by initializing `started` to `time.Now()` in `startStatusServer`, or by having `writeJSON` set `started` to `time.Now()` when it finds it zero before computing `UptimeSeconds`.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Include comments that identify potential bugs or risks in the code.
</details>
</issue_to_address>
### Comment 7
<location path="README.md" line_range="182" />
<code_context>
+ -p 8080:8080 \
+ -v fetchit-volume:/opt \
+ -v $HOME/.fetchit:/opt/mount \
+ -v /run/user/$(id -u)/podman//podman.sock:/run/podman/podman.sock \
+ --security-opt label=disable \
+ quay.io/fetchit/fetchit:latest
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The double slash in `podman//podman.sock` looks like a typographical error in the command and may confuse readers.
The path `podman//podman.sock` includes a double slash, which is unusual and likely a typo rather than intentional.
To make the example clearer and avoid confusion, consider correcting it to use a single slash:
```sh
- -v /run/user/$(id -u)/podman//podman.sock:/run/podman/podman.sock \
+ -v /run/user/$(id -u)/podman/podman.sock:/run/podman/podman.sock \
```
This keeps the path syntactically clean while preserving the intended mount point.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Identify and point out typographical errors in code, especially in commands or scripts.
</details>
</issue_to_address>
### Comment 8
<location path="pkg/engine/status.go" line_range="98" />
<code_context>
+ w.Header().Set("Content-Type", "application/json")
+ enc := json.NewEncoder(w)
+ enc.SetIndent("", " ")
+ _ = enc.Encode(resp)
+}
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The JSON encoding error is being ignored here, which weakens error handling and makes the status endpoint less robust.
Right now the status handler ignores any error from `json.Encoder.Encode`, which means failures to serialize the response (e.g., due to a write error on the `http.ResponseWriter`) will be silently dropped. For a status/health endpoint, it’s usually better to surface a clear HTTP error so callers know something went wrong.
You could tighten the error handling like this:
```go
w.Header().Set("Content-Type", "application/json")
enc := json.NewEncoder(w)
enc.SetIndent("", " ")
if err := enc.Encode(resp); err != nil {
logger.Errorf("status: failed to encode response: %v", err)
http.Error(w, "failed to encode status", http.StatusInternalServerError)
return
}
```
This keeps the implementation simple while making the status endpoint more robust and observable when write/encode errors occur.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
- The comment should address potential issues such as error handling or code robustness.
- Include specific code examples or modifications to illustrate the suggestion.
- Ensure the comment provides a clear suggestion for improvement or enhancement.
- The comment should be relevant to the context of the code and provide actionable advice.
</details>
</issue_to_address>
### Comment 9
<location path="pkg/engine/status.go" line_range="98" />
<code_context>
+ w.Header().Set("Content-Type", "application/json")
+ enc := json.NewEncoder(w)
+ enc.SetIndent("", " ")
+ _ = enc.Encode(resp)
+}
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Silently discarding JSON encoding errors can hide real issues; consider handling specific error types instead of ignoring them.
Assigning and discarding the result of `enc.Encode` means any encoding or write error is completely ignored, which can make diagnosing real failures difficult. Handling specific error types explicitly would make this safer.
For example:
```go
import (
"encoding/json"
"errors"
"net"
"net/http"
)
// ... inside writeJSON
w.Header().Set("Content-Type", "application/json")
enc := json.NewEncoder(w)
enc.SetIndent("", " ")
if err := enc.Encode(resp); err != nil {
// Treat transient network/client issues differently from internal bugs.
var netErr net.Error
if errors.As(err, &netErr) {
// likely client disconnect / timeout; optionally log at debug level
logger.Debugf("client disconnected while writing status response: %v", netErr)
} else {
// unexpected serialization or IO error
logger.Errorf("failed to encode status response: %v", err)
}
}
```
This keeps the handler resilient but adds more specific error-type handling instead of ignoring all failures. It requires adding `errors` and `net` to the imports.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
The comment should suggest using more specific exception types for better error handling.
</details>
</issue_to_address>
### Comment 10
<location path="pkg/engine/status.go" line_range="103" />
<code_context>
+
+// startStatusServer launches the status/health HTTP server in a goroutine if
+// FETCHIT_STATUS_ADDR is set. It is a no-op otherwise.
+func startStatusServer() {
+ addr := os.Getenv(statusAddrEnv)
+ if addr == "" {
</code_context>
<issue_to_address>
**issue (review_instructions):** If the status server is enabled but no methods are ever registered, `started` remains zero so `/status` reports a `StartedAt` of year 0001 and a huge uptime, which is likely unintended behavior.
As written, `status.started` is only initialized in `register`, so if `FETCHIT_STATUS_ADDR` is set but no methods are scheduled or registered (for example in a minimal configuration, or if scheduling fails before registration), then `writeJSON` will compute:
```go
started := r.started // time.Time{}
UptimeSeconds: int64(time.Since(started).Seconds()),
```
`time.Since(time.Time{})` is the duration since the zero time (year 0001), so `/status` will emit a `StartedAt` of `0001-01-01T00:00:00Z` and an extremely large `uptimeSeconds`. That’s misleading for operators and monitoring systems.
A concrete improvement is to ensure `started` is initialized when the status server starts, so the status endpoint always reports a sane start time even if no methods are registered:
```go
func startStatusServer() {
addr := os.Getenv(statusAddrEnv)
if addr == "" {
return
}
// Ensure the registry has a reasonable start time even if no methods
status.mu.Lock()
if status.started.IsZero() {
status.started = time.Now()
}
status.mu.Unlock()
mux := http.NewServeMux()
mux.HandleFunc("/healthz", func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte("ok\n"))
})
mux.HandleFunc("/status", func(w http.ResponseWriter, _ *http.Request) {
status.writeJSON(w)
})
go func() {
logger.Infof("Status server listening on %s (/healthz, /status)", addr)
if err := http.ListenAndServe(addr, mux); err != nil {
logger.Errorf("status server error: %v", err)
}
}()
}
```
This keeps the behavior consistent across configurations and avoids confusing monitoring data, which is particularly helpful in heterogeneous or multi-environment deployments.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Ensure the comment identifies a potential bug risk or issue in the code.
</details>
</issue_to_address>
### Comment 11
<location path="pkg/engine/status.go" line_range="67" />
<code_context>
+ s.LastRun = &now
+}
+
+func (r *statusRegistry) writeJSON(w http.ResponseWriter) {
+ r.mu.Lock()
+ ms := make([]*methodStatus, 0, len(r.methods))
</code_context>
<issue_to_address>
**issue (review_instructions):** If /status is hit before any method is registered, uptimeSeconds will be computed from a zero time, yielding a huge, misleading uptime value.
In `writeJSON`, `started` is copied from `r.started`, and then `uptimeSeconds` is calculated as:
```go
started := r.started
...
UptimeSeconds: int64(time.Since(started).Seconds()),
```
If the status server is started and `/status` is requested before any method has been registered via `register`, `r.started` will still be the zero time (`time.Time{}`). In that case, `time.Since(started)` will be the duration since year 0001, resulting in an enormous `uptimeSeconds` value that does not reflect the actual process uptime.
A simple fix is to ensure `started` is initialized to `time.Now()` if it is zero before computing uptime, for example:
```go
func (r *statusRegistry) writeJSON(w http.ResponseWriter) {
r.mu.Lock()
ms := make([]*methodStatus, 0, len(r.methods))
for _, s := range r.methods {
ms = append(ms, s)
}
started := r.started
if started.IsZero() {
started = time.Now()
}
r.mu.Unlock()
...
resp := struct {
// ...
UptimeSeconds int64 `json:"uptimeSeconds"`
}{
Status: "running",
StartedAt: started,
UptimeSeconds: int64(time.Since(started).Seconds()),
Methods: ms,
}
...
}
```
Alternatively, you could initialize `r.started` when the status server is started, but the key is to avoid using a zero `time.Time` to compute uptime to prevent misleading values.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Include comments that identify potential bugs or runtime errors and provide a clear solution.
</details>
</issue_to_address>
### Comment 12
<location path="pkg/engine/fetchit.go" line_range="372" />
<code_context>
logger.Infof("Processing git target: %s Method: %s Name: %s", method.GetTarget().url, mt, method.GetName())
- s.Cron(schedInfo.schedule).Tag(mt).Do(method.Process, ctx, f.conn, skew)
+ status.register(method, schedInfo.schedule)
+ m := method
+ s.Cron(schedInfo.schedule).Tag(mt).Do(func(ctx, conn context.Context, skew int) {
+ status.recordRun(m)
</code_context>
<issue_to_address>
**issue (review_instructions):** The anonymous function passed to `Do` has a parameter type mismatch with the arguments (`ctx` and `f.conn`), which will cause a compile-time error and incorrect types passed to `m.Process`.
The line below defines the function as `func(ctx, conn context.Context, skew int)`, but it's invoked with `ctx` (a `context.Context`) and `f.conn` (likely a `*grpc.ClientConn`). That won't compile because `conn` is declared as `context.Context` while the second argument is `*grpc.ClientConn`, and `m.Process` expects the connection argument, not another context.
You probably want the second parameter to be the connection, matching the original `method.Process` signature. For example:
```go
m := method
s.Cron(schedInfo.schedule).Tag(mt).Do(func(ctx context.Context, conn *grpc.ClientConn, skew int) {
status.recordRun(m)
m.Process(ctx, conn, skew)
}, ctx, f.conn, skew)
```
This preserves the previous behavior while adding the status tracking without introducing a type mismatch.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Identify and highlight potential bugs or risks in the code, such as missing syntax elements that could lead to unintended behavior.
</details>
</issue_to_address>
### Comment 13
<location path="pkg/engine/status.go" line_range="67" />
<code_context>
+ s.LastRun = &now
+}
+
+func (r *statusRegistry) writeJSON(w http.ResponseWriter) {
+ r.mu.Lock()
+ ms := make([]*methodStatus, 0, len(r.methods))
</code_context>
<issue_to_address>
**issue (review_instructions):** If /status is hit before any methods are registered, `started` remains zero, so `StartedAt` will be the zero time and `UptimeSeconds` will be an enormous value, which is likely unintended behavior.
Currently `r.started` is only set in `register`, so if the status server is enabled but no methods have been scheduled (or `/status` is hit very early), `writeJSON` will compute `time.Since(started)` where `started` is the zero value. That yields a huge uptime and a `0001-01-01...` `StartedAt`, which is misleading for operators.
A concrete fix would be to ensure `started` is initialized when the server is started or before computing the response, for example:
```go
func (r *statusRegistry) writeJSON(w http.ResponseWriter) {
r.mu.Lock()
if r.started.IsZero() {
r.started = time.Now()
}
ms := make([]*methodStatus, 0, len(r.methods))
for _, s := range r.methods {
ms = append(ms, s)
}
started := r.started
r.mu.Unlock()
// ... rest unchanged
}
```
Alternatively, you could special-case the zero-time case by setting `UptimeSeconds` to 0 and omitting/zeroing `StartedAt` until there is at least one registered method.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Identify potential bugs or risks in the code and provide a clear explanation of the issue.
</details>
</issue_to_address>
### Comment 14
<location path="pkg/engine/status.go" line_range="118" />
<code_context>
+ })
+ go func() {
+ logger.Infof("Status server listening on %s (/healthz, /status)", addr)
+ if err := http.ListenAndServe(addr, mux); err != nil {
+ logger.Errorf("status server error: %v", err)
+ }
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The HTTP status server uses http.ListenAndServe with default settings, which lacks timeouts and can be vulnerable to slow-client/DoS-style attacks; consider using an http.Server with sensible timeouts.
Right now the status server is created via `http.ListenAndServe(addr, mux)` with all default settings. The default `http.Server` has no read/write/idle timeouts, which can make it susceptible to resource exhaustion from slow clients (e.g. Slowloris-style behavior), even on an internal status port.
A more defensive pattern is to construct an `http.Server` with explicit timeouts, for example:
```go
srv := &http.Server{
Addr: addr,
Handler: mux,
ReadTimeout: 5 * time.Second,
WriteTimeout: 10 * time.Second,
IdleTimeout: 60 * time.Second,
}
if err := srv.ListenAndServe(); err != nil {
logger.Errorf("status server error: %v", err)
}
```
If you expect this endpoint to ever be exposed beyond localhost, these timeouts help reduce the risk of malicious or misbehaving clients tying up connections indefinitely.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Include comments that suggest security improvements, such as input validation, especially when they prevent potential runtime failures or malicious use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return | ||
| } | ||
| mux := http.NewServeMux() | ||
| mux.HandleFunc("/healthz", func(w http.ResponseWriter, _ *http.Request) { |
There was a problem hiding this comment.
issue (review_instructions): The writes in the /healthz handler ignore write errors, which can silently hide I/O problems.
In the /healthz handler, the call to w.Write uses _, _ = w.Write([]byte("ok\n")), which discards any error from writing the response. While health checks often are simple, silently ignoring I/O errors can hide underlying problems (e.g., client disconnects or broken connections), similar to catching all exceptions and continuing.
Consider at least logging the error from w.Write so that unexpected failures are visible. Alternatively, if you want to keep the handler minimal, a brief comment explaining why the error is intentionally ignored would clarify that this is a conscious choice rather than an oversight.
Review instructions:
Path patterns: *
Instructions:
Include comments that identify potential bug risks, such as catching all exceptions, which can mask important errors.
| logger.Infof("Processing git target: %s Method: %s Name: %s", method.GetTarget().url, mt, method.GetName()) | ||
| s.Cron(schedInfo.schedule).Tag(mt).Do(method.Process, ctx, f.conn, skew) | ||
| status.register(method, schedInfo.schedule) | ||
| m := method |
There was a problem hiding this comment.
issue (review_instructions): The anonymous function passed to Do has a parameter type mismatch with the arguments (ctx and f.conn), which will cause a compile-time error and incorrect types passed to m.Process.
The line below defines the function as func(ctx, conn context.Context, skew int), but it's invoked with ctx (a context.Context) and f.conn (likely a *grpc.ClientConn). That won't compile because conn is declared as context.Context while the second argument is *grpc.ClientConn, and m.Process expects the connection argument, not another context.
You probably want the second parameter to be the connection, matching the original method.Process signature. For example:
m := method
s.Cron(schedInfo.schedule).Tag(mt).Do(func(ctx context.Context, conn *grpc.ClientConn, skew int) {
status.recordRun(m)
m.Process(ctx, conn, skew)
}, ctx, f.conn, skew)This preserves the previous behavior while adding the status tracking without introducing a type mismatch.
Review instructions:
Path patterns: *
Instructions:
Identify and highlight potential bugs or risks in the code, such as missing syntax elements that could lead to unintended behavior.
| }) | ||
| go func() { | ||
| logger.Infof("Status server listening on %s (/healthz, /status)", addr) | ||
| if err := http.ListenAndServe(addr, mux); err != nil { |
There was a problem hiding this comment.
suggestion (review_instructions): The HTTP status server uses http.ListenAndServe with default settings, which lacks timeouts and can be vulnerable to slow-client/DoS-style attacks; consider using an http.Server with sensible timeouts.
Right now the status server is created via http.ListenAndServe(addr, mux) with all default settings. The default http.Server has no read/write/idle timeouts, which can make it susceptible to resource exhaustion from slow clients (e.g. Slowloris-style behavior), even on an internal status port.
A more defensive pattern is to construct an http.Server with explicit timeouts, for example:
srv := &http.Server{
Addr: addr,
Handler: mux,
ReadTimeout: 5 * time.Second,
WriteTimeout: 10 * time.Second,
IdleTimeout: 60 * time.Second,
}
if err := srv.ListenAndServe(); err != nil {
logger.Errorf("status server error: %v", err)
}If you expect this endpoint to ever be exposed beyond localhost, these timeouts help reduce the risk of malicious or misbehaving clients tying up connections indefinitely.
Review instructions:
Path patterns: *
Instructions:
Include comments that suggest security improvements, such as input validation, especially when they prevent potential runtime failures or malicious use.
…point - .github/dependabot.yml: group minor/patch Go bumps into one weekly PR, track github-actions; keep major bumps separate for review - dependabot-auto-merge.yml: auto-merge minor/patch dependabot PRs once CI passes - test.yml: fast go vet + unit test gate (build tags + CGO deps) - status.go: opt-in HTTP server (FETCHIT_STATUS_ADDR) serving /healthz and /status (uptime, per-method schedule/run-count/last-run); wired into RunTargets without changing the Method interface Signed-off-by: sallyom <somalley@redhat.com>
sallyom
force-pushed
the
improvements/ci-and-status
branch
from
86ce2df to
a68c5c3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add an opt-in HTTP health and status endpoint for FetchIt and introduce CI and dependency management automation.
New Features:
Enhancements:
CI:
Documentation: