feat!: Multiple improvements - #288
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughBumps Go to 1.25.6; replaces pool min/max model with replicas and refactors pool lifecycle (Start→Run, machineMetadata, Set/GetReplicas); adds containerd-backed imageManager with pull deduplication; restructures CLI (pools group, ps, login); updates server APIs, types, metrics, tests, docs, CI, and Makefile. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Client
participant Server
participant SSH
CLI->>Client: GetMicroVM(ctx, vmid)
Client->>Server: query pools for vmid
Server-->>Client: MicroVM {VMID, IPAddr, Pool, CreatedAt}
Client-->>CLI: MicroVM
CLI->>CLI: validate IPAddr
alt has IP
CLI->>SSH: exec ssh -o StrictHostKeyChecking=no root@<IP>
SSH-->>CLI: interactive shell
else no IP
CLI-->>User: error "does not have an IP address yet"
end
sequenceDiagram
participant Pool as Pool.Run()
participant ImageMgr as ImageManager
participant Containerd
participant Firecracker
participant GitHub
Pool->>Pool: observe config.Replicas target, compute delta
alt Scale Up
Pool->>ImageMgr: ensureImage(ctx, imageRef, pullPolicy)
ImageMgr->>Containerd: pullImage (deduped)
Containerd-->>ImageMgr: image
ImageMgr-->>Pool: image
Pool->>Firecracker: create VM
Firecracker-->>Pool: VM instance
Pool->>Pool: store machineMetadata
else Scale Down
Pool->>Firecracker: stop VM
Firecracker-->>Pool: stopped
Pool->>GitHub: deleteGitHubRunner(runnerName, runnerID)
GitHub-->>Pool: deletion result
Pool->>Pool: cleanup metadata and resources
end
sequenceDiagram
participant Client
participant ImageManager
participant InflightMap
participant Containerd
Client->>ImageManager: ensureImage(ctx, imageRef, pullPolicy)
ImageManager->>ImageManager: decide policy
ImageManager->>InflightMap: check in-flight[cacheKey]
alt not in-flight
ImageManager->>InflightMap: mark in-flight, create request.done
ImageManager->>Containerd: pullImage(ctx, imageRef)
Containerd-->>ImageManager: image or error
ImageManager->>InflightMap: store result, close request.done
ImageManager-->>Client: return image or error
else in-flight
ImageManager->>InflightMap: wait on existing request.done
ImageManager-->>Client: return image or error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
90ae853 to
742f8a3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
server/convert.go (1)
7-38: Guard against nil Pool/config to prevent panic.
The pipeline failure indicates a nil pointer inconvertPool.porp.configcan be nil, so this should be handled defensively to avoid crashing list APIs.🛠️ Proposed fix (nil-guard + skip)
func convertPool(p *Pool) *fireactions.Pool { + if p == nil || p.config == nil { + return nil + } pool := &fireactions.Pool{ Name: p.config.Name, Replicas: p.config.Replicas, CurrentReplicas: p.GetCurrentSize(), DesiredReplicas: p.config.Replicas, Organization: p.config.Runner.Organization, GroupID: p.config.Runner.GroupID, Labels: p.config.Runner.Labels, Image: p.config.Runner.Image, } @@ func convertPools(pools []*Pool) fireactions.Pools { convertedPools := make(fireactions.Pools, 0, len(pools)) for _, pool := range pools { - convertedPools = append(convertedPools, convertPool(pool)) + if converted := convertPool(pool); converted != nil { + convertedPools = append(convertedPools, converted) + } } return convertedPools }commands/pools.go (1)
60-68: Copy-paste error: Incorrect error message in resume command.The error message says "pause pool" but this is the resume command handler.
Suggested fix
func runPoolsResumeCmd(cmd *cobra.Command, args []string) error { _, err := client.ResumePool(cmd.Context(), args[0]) if err != nil { - return fmt.Errorf("pause pool \"%s\": %w", args[0], err) + return fmt.Errorf("resume pool \"%s\": %w", args[0], err) } fmt.Printf("Pool \"%s\" resumed\n", args[0]) return nil }docs/user-guide/monitoring.md (1)
66-66: Typo: "vizualisation" should be "visualization".-Example Grafana dashboard for vizualisation of Fireactions metrics: +Example Grafana dashboard for visualization of Fireactions metrics:server/pool.go (1)
314-317: Data race:GetCurrentSize()readsmachinesmap without holdingmachinesMu.This method is called from
Run()andScale()without synchronization, while other methods modifyp.machinesundermachinesMu. This can cause a data race.🐛 Proposed fix
// GetCurrentSize returns the current size of the pool. func (p *Pool) GetCurrentSize() int { + p.machinesMu.Lock() + defer p.machinesMu.Unlock() return len(p.machines) }server/handlers_test.go (3)
164-188: ScalePool test doesn't send required JSON body.The
scalePoolHandlernow requires a JSON body withreplicasfield (as seen inserver/handlers.golines 65-72), but the test sends an empty request body. This will cause a400 Bad Requestinstead of the expected200 OK.🐛 Proposed fix
+import ( + "bytes" + "strings" + // ... other imports +) + t.Run("Success", func(t *testing.T) { m := newMockPoolManager(mockCtrl) m.EXPECT().ScalePool(gomock.Any(), "test", 1).Return(nil) router := gin.New() router.POST("/api/v1/pools/:id/scale", scalePoolHandler(m)) - req, err := http.NewRequest("POST", "/api/v1/pools/test/scale", nil) + body := strings.NewReader(`{"replicas": 1}`) + req, err := http.NewRequest("POST", "/api/v1/pools/test/scale", body) + req.Header.Set("Content-Type", "application/json") if err != nil { t.Fatal(err) }Similarly update the Error test case:
t.Run("Error", func(t *testing.T) { m := newMockPoolManager(mockCtrl) m.EXPECT().ScalePool(gomock.Any(), "test", 1).Return(errors.New("error")) router := gin.New() router.POST("/api/v1/pools/:id/scale", scalePoolHandler(m)) - req, err := http.NewRequest("POST", "/api/v1/pools/test/scale", nil) + body := strings.NewReader(`{"replicas": 1}`) + req, err := http.NewRequest("POST", "/api/v1/pools/test/scale", body) + req.Header.Set("Content-Type", "application/json")
184-187: Expected response message doesn't match updated handler.The handler now returns
"Pool replicas updated successfully"(seeserver/handlers.goline 79), but the test expects"Pool scaled successfully".🐛 Proposed fix
- expectedBody := `{"message":"Pool scaled successfully"}` + expectedBody := `{"message":"Pool replicas updated successfully"}`
105-136: Test mock Pool is missing requiredRunnerfield, and expected body doesn't match new JSON structure.The mock Pool at line 107-114 does not initialize
config.Runner, butconvertPool(server/convert.go:13-16) accessesp.config.Runner.Organization,GroupID,Labels, andImagewithout nil checks. This will cause a nil pointer dereference.Additionally, the
expectedBodyat line 132 references old field names (max_runners,min_runners,cur_runners) that no longer exist. The actual JSON response will contain:name,replicas,current_replicas,desired_replicas,organization,group_id,labels,image, andstatus.🐛 Proposed fix
m.EXPECT().GetPool(gomock.Any(), "test").Return(&Pool{ machines: make(map[string]*machineMetadata), config: &PoolConfig{ Name: "test", Replicas: 0, + Runner: &RunnerConfig{ + Organization: "test-org", + GroupID: 0, + Labels: []string{}, + Image: "", + }, }, isActive: false, }, nil) // ... router setup ... - expectedBody := `{"pool":{"name":"test","max_runners":0,"min_runners":0,"cur_runners":0,"status":{"state":"Paused","message":"Pool is paused"}}}` + expectedBody := `{"pool":{"name":"test","replicas":0,"current_replicas":0,"desired_replicas":0,"organization":"test-org","group_id":0,"labels":[],"image":"","status":{"state":"Paused","message":"Pool is paused"}}}`server/server.go (2)
237-250: Data race:pool.config.Replicasmodified without synchronization.
ScalePooldirectly writes topool.config.Replicaswhile the pool'sRun()loop may be concurrently reading it. This can cause a data race. The pool has its own mutex (pool.l) that should be used for synchronization.Proposed fix: Acquire pool lock before modifying config
func (s *Server) ScalePool(ctx context.Context, id string, replicas int) error { metricPoolScaleRequests.WithLabelValues(id).Inc() pool, err := s.GetPool(ctx, id) if err != nil { return err } // Update the pool config with the new replicas value // The Run() loop will handle the actual scaling + pool.l.Lock() pool.config.Replicas = replicas + pool.l.Unlock() return nil }
286-302: Same data race concern inReload():pool.configassigned without pool lock.On line 289,
pool.config = poolConfigis assigned while holdings.lbut not the pool's internal lock (pool.l). The pool'sRun()loop may be concurrently readingpool.config, causing a data race.Proposed fix: Acquire pool lock when updating config
for _, poolConfig := range s.config.Pools { pool, ok := s.pools[poolConfig.Name] if ok { + pool.l.Lock() pool.config = poolConfig + pool.l.Unlock() s.logger.Info().Msgf("Pool %s reloaded", poolConfig.Name) continue }
🤖 Fix all issues with AI agents
In `@commands/login_test.go`:
- Around line 13-35: TestLoginCmd_WithVMID sets a gomock expectation for
fireactionsClient.GetMicroVM but never executes the command, leaving the mock
unmet; either remove the mock setup from TestLoginCmd_WithVMID if you only
intend to assert command metadata (Use/Short), or actually execute the command
to satisfy the expectation by calling cmd.Execute() (or cmd.RunE()) inside the
test and handling the expected SSH failure (e.g., assert the returned error or
stub SSH behavior). Locate the mock creation (mocks.NewMockfireactionsClient),
the Expect().GetMicroVM(...) call, and the newLoginCmd() invocation to implement
the chosen fix.
In `@server/image_manager.go`:
- Around line 52-73: The switch on pullPolicy in server/image_manager.go uses
capitalized cases ("Always","Never","IfNotPresent") but config provides
lowercase values, so normalize the policy before the switch (e.g., set
pullPolicy = strings.ToLower(p.config.Runner.ImagePullPolicy) or call
strings.ToLower on the variable used by the switch) and update the case labels
to match the normalized form ("always","never","ifnotpresent"); ensure you add
the strings import if missing so functions like im.pullImageWithDedup and
im.getLocalImage continue to be called correctly after normalization.
In `@server/microvm.go`:
- Around line 22-28: The loop building MicroVM instances accesses
metadata.machine.Cfg.NetworkInterfaces[0] which can panic if the slice is empty;
update the code in the loop that constructs the MicroVM (the block creating
&MicroVM{ VMID, Pool, IPAddr, CreatedAt }) to defensively check
len(metadata.machine.Cfg.NetworkInterfaces) > 0 before reading index 0 and use a
safe fallback (e.g., set IPAddr to "" or "unknown", or skip adding that machine)
when no interfaces exist; ensure you reference
metadata.machine.Cfg.NetworkInterfaces and the MicroVM creation site when making
the change so the behavior is consistent.
In `@server/server.go`:
- Around line 74-91: The imageManager is created with the nop logger (logger
variable) before the Server options (including WithLogger) are applied, so its
logger never gets updated; fix by delaying creation of imageManager until after
you apply options (i.e., construct Server options loop then call newImageManager
with the final logger) or add an explicit method to update its logger (e.g.,
imageManager.SetLogger) and call that after WithLogger is applied; reference
newImageManager, imageManager, WithLogger and the Server struct to locate the
code to change.
In `@types.go`:
- Around line 153-164: The current KV() builds "Created" using
formatDuration(time.Since(vm.CreatedAt)) but time.Since can be negative if
CreatedAt is in the future; update formatDuration to detect negative durations
at its start (check for d < 0) and return a clear future-oriented string (e.g.,
"in X" or "X from now") by formatting the absolute value, so callers like KV()
display future timestamps correctly instead of "just now".
♻️ Duplicate comments (2)
.github/workflows/test.yaml (1)
14-17: Same Go version verification as lint workflow..github/workflows/release.yaml (1)
23-26: Same Go version verification as lint workflow.
🧹 Nitpick comments (12)
client_test.go (1)
29-41: Assert ScalePool request body includes replicas.
Right now the test only checks method/path. Sincereplicasis now required, asserting the request body will prevent regressions.✅ Suggested test enhancement
func TestClient_ScalePool(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != "POST" || r.URL.Path != "/api/v1/pools/test/scale" { t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) } + body, _ := io.ReadAll(r.Body) + assert.Contains(t, string(body), `"replicas":1`) w.WriteHeader(http.StatusOK) })) defer server.Close()client.go (1)
278-287: Consider validatingreplicasis non-negative.The function accepts any
intvalue, including negative numbers. While the server should validate this, adding client-side validation could provide faster feedback and clearer error messages.♻️ Optional: Add client-side validation
// ScalePool sets the desired number of replicas for a pool. func (c *Client) ScalePool(ctx context.Context, id string, replicas int) (*Response, error) { + if replicas < 0 { + return nil, fmt.Errorf("replicas must be non-negative, got %d", replicas) + } body := map[string]int{"replicas": replicas} req, err := c.newRequestWithContext(ctx, "POST", fmt.Sprintf("/api/v1/pools/%s/scale", id), body)commands/ps.go (1)
25-30: Usecmd.Context()instead ofcontext.Background().Using the command's context allows proper cancellation propagation (e.g., Ctrl+C handling) and is the idiomatic pattern for Cobra commands.
♻️ Proposed fix
func runPsCmd(cmd *cobra.Command) error { // List all VMs across all pools using the new endpoint - microvms, _, err := client.ListMicroVMs(context.Background(), "") + microvms, _, err := client.ListMicroVMs(cmd.Context(), "") if err != nil { return fmt.Errorf("failed to list VMs: %w", err) }server/microvm.go (1)
16-16: Thepoolparameter is unused in this method implementation, but removing it requires interface and API updates.The parameter is never referenced in the function body;
p.config.Nameis used instead (line 25). However, this parameter is part of theMicroVMManagerinterface (server/interface.go) and is used by other implementations likeServer.ListMicroVMs(). Removing it would require updating the interface definition, all implementing methods, and all call sites throughout the codebase.Either refactor the interface and all implementations consistently, or document why this parameter is retained for API uniformity.
commands/login.go (1)
27-31: Usecmd.Context()instead ofcontext.Background()for consistency.Other commands in this codebase (e.g.,
runPoolsScaleCmd,runPoolsResumeCmdincommands/pools.go) usecmd.Context(). This ensures proper context propagation and cancellation support.Suggested fix
func runLoginCmd(cmd *cobra.Command, vmID string) error { - vm, _, err := client.GetMicroVM(context.Background(), vmID) + vm, _, err := client.GetMicroVM(cmd.Context(), vmID) if err != nil { return fmt.Errorf("failed to get VM details: %w", err) }server/metrics.go (1)
56-60: Consider addingorganizationlabel topool_statusfor consistency.Other pool-level metrics (
pool_runners_current,pool_runners_desired) include anorganizationlabel. Ifpool_statusis intended to be joined with these metrics in queries, having consistent labels would simplify PromQL expressions.Suggested change (if consistency is desired)
metricPoolStatus = promauto.NewGaugeVec(prometheus.GaugeOpts{ Name: "pool_status", Namespace: namespace, Help: "Status of a pool. 0 is paused, 1 is active.", - }, []string{"pool"}) + }, []string{"pool", "organization"}) )server/image_manager.go (1)
133-166: Consider adding context cancellation check before the pull operation.The
pullImagemethod holdscontainerdMufor the entire duration of the pull operation. If the context is cancelled, the pull may continue unnecessarily while holding the lock.♻️ Suggested improvement
func (im *imageManager) pullImage(ctx context.Context, ref string) (containerd.Image, error) { + // Early exit if context is already cancelled + select { + case <-ctx.Done(): + return nil, ctx.Err() + default: + } + im.containerdMu.Lock() defer im.containerdMu.Unlock()server/pool.go (3)
159-161: Avoidtime.Sleepfor synchronization.Using
time.Sleep(100 * time.Millisecond)to wait for theRun()loop to exit is fragile. If the loop takes longer to release the lock, this could still cause contention. Consider using async.WaitGroupor a done channel to properly signal completion.♻️ Suggested approach
Add a
runDonechannel toPoolthatRun()closes when it exits:// In Pool struct runDone chan struct{} // In NewPool runDone: make(chan struct{}), // In Run() defer close(p.runDone) // In Stop() select { case <-p.runDone: case <-time.After(5 * time.Second): p.logger.Warn().Msg("Timed out waiting for Run() to exit") }
501-550: Lock held during network operations inscaleDown.The
machinesMulock is held for the entirescaleDownoperation, includingdeleteGitHubRunnerwhich makes network calls to GitHub API (with a 10-second timeout). This can cause significant lock contention if other operations need to accessmachines.♻️ Suggested approach
Extract the metadata and release the lock before cleanup operations:
func (p *Pool) scaleDown(ctx context.Context) error { p.machinesMu.Lock() - defer p.machinesMu.Unlock() // Find a machine to remove (pick the first one) var targetMetadata *machineMetadata var targetName string for name, metadata := range p.machines { targetMetadata = metadata targetName = name break } if targetMetadata == nil { + p.machinesMu.Unlock() return fmt.Errorf("no machines available to scale down") } + + // Remove from map before releasing lock + delete(p.machines, targetName) + p.machinesMu.Unlock() // Stop the machine err := targetMetadata.machine.StopVMM() // ... rest of cleanup without holding lock
456-496: Complex goroutine pattern for machine wait.The inline channel creation and goroutine pattern is hard to follow. Consider extracting this into a helper method or simplifying the structure.
♻️ Simplified approach
go func() { - // Wait for machine to exit or pool to be stopped - select { - case <-p.ctx.Done(): - // Pool is stopping, don't clean up here - Stop() will handle it - return - case <-func() chan struct{} { - done := make(chan struct{}) - go func() { - _ = md.machine.Wait(context.Background()) - close(done) - }() - return done - }(): - // Machine exited normally - p.logger.Debug().Msgf("Firecracker VM %s exited", runnerName) - } + // Create a channel to signal machine exit + machineDone := make(chan struct{}) + go func() { + _ = md.machine.Wait(context.Background()) + close(machineDone) + }() + + // Wait for machine to exit or pool to be stopped + select { + case <-p.ctx.Done(): + // Pool is stopping, Stop() will handle cleanup + return + case <-machineDone: + p.logger.Debug().Msgf("Firecracker VM %s exited", runnerName) + }server/handlers.go (1)
35-38: Consider nil check before accessingconfig.Name.If any pool in the slice has a nil
config, this sort will panic. While this shouldn't happen in practice, defensive coding could prevent potential crashes.♻️ Defensive approach
// Sort pools by name sort.Slice(pools, func(i, j int) bool { + if pools[i].config == nil || pools[j].config == nil { + return pools[i].config != nil + } return pools[i].config.Name < pools[j].config.Name })server/server.go (1)
309-326: Redundant parameter inpool.ListMicroVMscall.On line 315,
pool.config.Nameis passed topool.ListMicroVMs(), but the pool already has access to its own name internally. Consider whether this parameter is necessary.
36b7560 to
89d2abb
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Makefile (1)
16-18: Clean target doesn't remove the build output.The build target outputs
./fireactions, but the clean target only removes thedistdirectory. This leaves the binary behind aftermake clean.Proposed fix
.PHONY: clean clean: - @ rm -rf dist + @ rm -rf dist fireactionsclient.go (1)
133-133: Address pipeline failure: unchecked error return.The linter reports that the error return value of
rsp.Body.Closeis not checked. While ignoring this error in adeferis common practice, it's causing CI to fail.Proposed fix
- defer rsp.Body.Close() + defer func() { _ = rsp.Body.Close() }()server/server.go (1)
237-250: Data race confirmed:pool.config.Replicasis accessed without synchronization.
ScalePoolwrites topool.config.Replicas(server/server.go:247) whilePool.Run()reads it (server/pool.go:129, 137) in a continuous loop, both without synchronization. This causes a data race.The proposed fix protects the write side. However, the reads in
Pool.Run()are also unprotected and occur frequently in a hot loop. For this access pattern—infrequent writes and frequent reads—consider usingsync/atomic.Int32instead, which would be more efficient than holding a lock for every read. Otherwise, protect both reads and writes withpool.l.Recommended fix: Use sync/atomic for efficient concurrent access
Replace
config.Replicaswith an atomic field:
- Change field type in pool config to
atomic.Int32- Use
atomic.StoreInt32()inScalePool- Use
atomic.LoadInt32()inPool.Run()and other read locationsAlternatively, if keeping the current lock approach, protect reads in
Pool.Run()as well.
🤖 Fix all issues with AI agents
In `@docs/api/index.md`:
- Around line 249-261: The example response contains a `runner_id` field that
isn't present on the MicroVM API type and uses snake_case keys that don't match
the MicroVM struct JSON tags; either remove `runner_id` from the JSON example or
add a `RunnerID` field to the MicroVM struct (e.g., add RunnerID int
`json:"runner_id"` in types.go and propagate it in server/microvm.go response
creation, ensuring server/pool.go maps it where appropriate), and update the
example's JSON keys to exactly match the MicroVM struct's json tags (or update
the struct tags to produce the documented snake_case) so the docs and
implementation are consistent.
In `@docs/user-guide/monitoring.md`:
- Around line 42-45: The PromQL example can divide by zero when
fireactions_pool_runners_desired{pool="default"} is 0; update the example in
monitoring.md to guard against that by using clamp_min on
fireactions_pool_runners_desired{pool="default"} (e.g., clamp_min(..., 1)) or by
adding a filter that excludes zero desired replicas, so the query never performs
division by zero; update the shown expression and briefly note why the guard is
needed.
In `@server/handlers.go`:
- Around line 140-148: listMicroVMsHandler currently returns []*server.MicroVM
which JSON-serializes with lowercase field names, causing inconsistency with
getMicroVMHandler that uses convertMicroVM to produce fireactions.MicroVM; add a
helper convertMicroVMs that takes []*server.MicroVM and returns
[]fireactions.MicroVM by mapping each element through convertMicroVM, then
replace the direct ctx.JSON call in listMicroVMsHandler to call
ctx.JSON(http.StatusOK, gin.H{"micro_vms": convertMicroVMs(microVMs)}) so both
endpoints serialize the same struct shape.
In `@server/image_manager.go`:
- Around line 133-166: The mutex containerdMu is held across the long-running
im.containerd.Pull call in pullImage, which blocks other containerd ops; change
pullImage so it only holds containerdMu to perform the quick GetImage check
(using im.containerd.GetImage) and then unlocks before doing heavy work
(resolver creation and im.containerd.Pull), and finally re-acquire the lock if
you need to access shared state after a successful pull; ensure error paths
return correctly if Pull fails and reference pullImageWithDedup (which handles
dedup) when coordinating concurrent pulls.
In `@server/pool.go`:
- Around line 410-413: p.installationID is accessed concurrently (written in
scaleUp and read in deleteGitHubRunner) causing a data race; fix by making
installationID concurrency-safe — either change the field to an atomic type
(e.g., atomic.Int64) and replace the write (p.installationID =
installation.GetID()) with Store and the read in deleteGitHubRunner with Load,
or protect both accesses with the Pool's mutex (lock before assigning in scaleUp
and lock before reading in deleteGitHubRunner). Update all references to
p.installationID accordingly so both write and read use the chosen synchronized
access.
- Around line 160-161: The Stop() method’s use of time.Sleep(100 *
time.Millisecond) to wait for the Run() loop is fragile; replace this with a
proper synchronization channel: add a doneCh field to the Pool struct, have
Run() close(doneCh) (or send a signal) when it exits, and have Stop() wait on
that channel (with optional timeout) instead of sleeping; update Start(), Run(),
and Stop() to initialize and use doneCh so the lock is released reliably and
Stop() blocks until Run() has finished.
- Around line 501-550: The function scaleDown currently holds p.machinesMu for
the whole routine; change it to only hold the lock long enough to pick and
remove the target from p.machines (locate targetMetadata/targetName, then
delete(p.machines, targetName)) and then unlock before performing blocking work.
Replace the defer p.machinesMu.Unlock() with an explicit unlock immediately
after removal so you can call targetMetadata.machine.StopVMM(),
targetMetadata.machine.Wait(...), p.deleteGitHubRunner(targetName,...),
targetMetadata.leaseCancel(...), and close targetMetadata.logFile without
holding the lock, keeping all references to those symbols unchanged.
- Around line 456-496: The current cleanup goroutine spawns a helper goroutine
that calls md.machine.Wait(context.Background()), which can leak if the pool
context (p.ctx) is canceled; change the wait to be context-aware by calling
md.machine.Wait(p.ctx) (or, if Wait only accepts no-ctx, wrap it in a goroutine
but cancel/stop the machine on p.ctx.Done) so the done channel will close when
the pool context is canceled; specifically modify the anonymous func that
returns the done chan to call _ = md.machine.Wait(p.ctx) (or invoke machine.Stop
on p.ctx) instead of using context.Background(), keeping the outer select on
p.ctx.Done() and the done channel and preserving the existing metadata cleanup
logic for runnerName and metadata.
♻️ Duplicate comments (4)
types.go (1)
166-170: Guard against negative durations when system clock is ahead ofCreatedAt.If
CreatedAtis in the future (due to clock skew),time.Since()returns a negative duration, which would incorrectly display "just now". Add a check at the start offormatDuration().Suggested guard
func formatDuration(d time.Duration) string { + if d < 0 { + return "in the future" + } if d < time.Second { return "just now" }server/microvm.go (1)
22-28: Potential index out of bounds onNetworkInterfaces[0].If
metadata.machine.Cfg.NetworkInterfacesis empty, accessing index[0]will panic. Add a defensive bounds check.🐛 Proposed defensive check
for _, metadata := range p.machines { + var ipAddr string + if len(metadata.machine.Cfg.NetworkInterfaces) > 0 && + metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration != nil && + metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration != nil { + ipAddr = metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration.IPAddr.IP.String() + } vm := &MicroVM{ VMID: metadata.machine.Cfg.VMID, Pool: p.config.Name, - IPAddr: metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration.IPAddr.IP.String(), + IPAddr: ipAddr, CreatedAt: metadata.createdAt, }server/server.go (1)
74-91: imageManager initialized with Nop logger beforeWithLoggeroption is applied.This was flagged in a previous review. The
imageManageris created on line 80 using&logger(which iszerolog.Nop()at this point). TheWithLoggeroption is applied later in the loop on lines 93-95, but this doesn't update the imageManager's logger reference.server/image_manager.go (1)
52-73: Pull policy case mismatch with config validation will break all image pulls.This was flagged in a previous review. The switch statement checks for capitalized values (
"Always","Never","IfNotPresent"), but based on the past review analysis, if the configuration validation enforces lowercase values, this will cause all cases to fail and fall through to the default error.Verify the actual values used in configuration:
#!/bin/bash # Check config validation for image_pull_policy rg -n "image_pull_policy|oneof" server/config.go -A 2 -B 2
🧹 Nitpick comments (8)
commands/ps.go (1)
25-38: Usecmd.Context()instead ofcontext.Background().Using
context.Background()loses the ability to cancel the request when the user interrupts (Ctrl+C). Cobra commands have a context that propagates cancellation signals.Also, the static analysis tool flagged the unchecked error from
fmt.Fprintlnon line 33.Proposed fix
func runPsCmd(cmd *cobra.Command) error { // List all VMs across all pools using the new endpoint - microvms, _, err := client.ListMicroVMs(context.Background(), "") + microvms, _, err := client.ListMicroVMs(cmd.Context(), "") if err != nil { return fmt.Errorf("failed to list VMs: %w", err) } if len(*microvms) == 0 { - fmt.Fprintln(cmd.OutOrStdout(), "No running VMs found") + _, _ = fmt.Fprintln(cmd.OutOrStdout(), "No running VMs found") return nil } printer.PrintText(microvms, cmd.OutOrStdout(), nil) return nil }client_test.go (1)
29-44: Consider validating the request body contains the replicas value.The test verifies the HTTP method and path but doesn't confirm that the
replicasvalue is correctly sent in the request body. Since the server now expects a JSON body withreplicas, consider enhancing the test to verify the payload.💡 Suggested enhancement
func TestClient_ScalePool(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != "POST" || r.URL.Path != "/api/v1/pools/test/scale" { t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) } + + var body map[string]int + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Errorf("failed to decode request body: %v", err) + } + if body["replicas"] != 1 { + t.Errorf("expected replicas=1, got %d", body["replicas"]) + } w.WriteHeader(http.StatusOK) }))types.go (1)
119-132: Simplify withstrings.Join.The manual string concatenation can be replaced with
strings.Joinfor cleaner, more efficient code.♻️ Suggested simplification
+import "strings" + // formatLabels converts a slice of labels to a comma-separated string func formatLabels(labels []string) string { - if len(labels) == 0 { - return "" - } - result := "" - for i, label := range labels { - if i > 0 { - result += ", " - } - result += label - } - return result + return strings.Join(labels, ", ") }docs/user-guide/monitoring.md (1)
53-57: Minor: Use hyphenated "scale-up" as a compound modifier.-Average scale up duration: +Average scale-up duration:commands/login.go (2)
28-28: Usecmd.Context()instead ofcontext.Background().For consistency with Cobra patterns and to support context cancellation from the CLI, consider using the command's context.
♻️ Suggested fix
func runLoginCmd(cmd *cobra.Command, vmID string) error { - vm, _, err := client.GetMicroVM(context.Background(), vmID) + vm, _, err := client.GetMicroVM(cmd.Context(), vmID) if err != nil { return fmt.Errorf("failed to get VM details: %w", err) }
37-37: Address unchecked error fromfmt.Fprintf(static analysis).The linter flags the unchecked return value. For informational output like this, you can suppress the warning with a blank identifier.
♻️ Suggested fix
- fmt.Fprintf(cmd.OutOrStdout(), "Connecting to VM %s at %s...\n", vmID, vm.IPAddr) + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Connecting to VM %s at %s...\n", vmID, vm.IPAddr)commands/login_test.go (1)
13-27: Remove unnecessary mock setup in structure-only test.The test creates a mock client and assigns it to
client, but no expectations are set and the command is never executed. This mock setup serves no purpose for verifying command structure.♻️ Simplified test
func TestLoginCmd_WithVMID(t *testing.T) { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - mockClient := mocks.NewMockfireactionsClient(ctrl) - client = mockClient - cmd := newLoginCmd() - cmd.SetArgs([]string{"test-vm-1"}) // We can't actually test SSH execution, but we can verify the command structure assert.NotNil(t, cmd) assert.Equal(t, "login <vmid>", cmd.Use) assert.Equal(t, "SSH into a running VM as root user", cmd.Short) }server/server.go (1)
342-361: Nested locking: server lock held while acquiring pool locks.
GetMicroVMholdss.l(line 343-344) while iterating pools and acquiringpool.machinesMu(line 347). While this pattern is consistent (always server lock → pool lock), ensure no code path reverses this order to avoid deadlocks.The early
pool.machinesMu.Unlock()on line 356 before returning is good practice.Consider releasing the server lock before iterating pools since you only need read access to
s.pools:func (s *Server) GetMicroVM(ctx context.Context, vmid string) (*MicroVM, error) { s.l.Lock() - defer s.l.Unlock() + pools := make([]*Pool, 0, len(s.pools)) + for _, pool := range s.pools { + pools = append(pools, pool) + } + s.l.Unlock() - for _, pool := range s.pools { + for _, pool := range pools { pool.machinesMu.Lock()
89d2abb to
190a3ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
commands/pools.go (1)
61-64: Fix incorrect error label in resume flow.Line 63 returns “pause pool” while resuming.
🩹 Proposed fix
- return fmt.Errorf("pause pool \"%s\": %w", args[0], err) + return fmt.Errorf("resume pool \"%s\": %w", args[0], err)
🤖 Fix all issues with AI agents
In `@docs/user-guide/monitoring.md`:
- Around line 47-51: The success-rate PromQL can divide by zero when there are
no scale operations; update the expression that divides
sum(rate(fireactions_scale_operations_total{status="success"}[5m])) by
sum(rate(fireactions_scale_operations_total[5m])) to protect the
denominator—e.g., wrap the denominator with clamp_min(..., 1e-10) or add a
filter so sum(rate(fireactions_scale_operations_total[5m])) by (pool,direction)
is never zero; ensure you modify the expression using the metric name
fireactions_scale_operations_total and the same label grouping (by (pool,
direction)) so the numerator and protected denominator align.
In `@server/server.go`:
- Around line 246-249: The write to pool.config.Replicas is racy with the Run()
loop reading it; fix by performing the update while holding the pool's
synchronization lock (the same mutex used by Run()), e.g. acquire and release
the pool lock around setting pool.config.Replicas so readers in Run() see a
synchronized access; alternatively add a setter method on Pool (e.g.,
SetReplicas) that takes the lock, updates pool.config.Replicas, and returns, and
update callers to use that method.
♻️ Duplicate comments (11)
docs/api/index.md (1)
248-257: Align MicroVM response fields with implementation.This still documents
runner_idand snake_case keys that previously didn’t match the MicroVM struct’s JSON tags. Either update the struct/response to includerunner_idand matching tags, or remove it and align the example keys to the actual JSON tags.Also applies to: 287-295
docs/user-guide/monitoring.md (2)
42-45: PromQL example may cause division by zero.This concern was already raised in a previous review. When
desiredreplicas is 0 (pool scaled down completely), this query will result in division by zero.
53-57: Average scale-up duration query may divide by zero.When there are no scale operations in the window,
rate(..._count{...}[5m])will be 0, causing division by zero. Consider usinghistogram_quantileor adding a guard.💡 Suggested improvement
-Average scale up duration: +Average scale-up duration: ```promql -rate(fireactions_scale_duration_seconds_sum{direction="up"}[5m]) -/ rate(fireactions_scale_duration_seconds_count{direction="up"}[5m]) +rate(fireactions_scale_duration_seconds_sum{direction="up"}[5m]) +/ clamp_min(rate(fireactions_scale_duration_seconds_count{direction="up"}[5m]), 1e-10)Or use `histogram_quantile` for p50/p95: ```promql histogram_quantile(0.95, rate(fireactions_scale_duration_seconds_bucket{direction="up"}[5m]))server/image_manager.go (1)
133-136: Lock held during potentially slow image pull operation.This concern was already raised in a previous review. The
containerdMuis held during the entirePulloperation which can take minutes for large images.server/handlers.go (1)
140-148: Inconsistent JSON serialization between list and get MicroVM handlers.The
listMicroVMsHandlerreturns[]*MicroVMdirectly (line 148), whilegetMicroVMHandlerusesconvertMicroVM(line 163). This causes different JSON field naming between endpoints.This was flagged in a previous review - please address by adding a
convertMicroVMshelper and using it here.server/microvm.go (1)
22-26: Guard nil StaticConfiguration/IPConfiguration before dereferencing.Line 25 can panic when CNI is used (StaticConfiguration is often nil). Add nested nil checks and fall back to empty IP.
🐛 Proposed fix
- ipAddr := "" - if len(metadata.machine.Cfg.NetworkInterfaces) > 0 { - ipAddr = metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration.IPAddr.IP.String() - } + ipAddr := "" + if len(metadata.machine.Cfg.NetworkInterfaces) > 0 { + ni := metadata.machine.Cfg.NetworkInterfaces[0] + if ni.StaticConfiguration != nil && ni.StaticConfiguration.IPConfiguration != nil { + ipAddr = ni.StaticConfiguration.IPConfiguration.IPAddr.IP.String() + } + }#!/bin/bash # Check how network interfaces are configured across the repo (CNI vs static). rg -n 'NetworkInterfaces|CNIConfiguration|StaticConfiguration|IPConfiguration' -C2server/server.go (1)
347-356: Guard NetworkInterfaces[0] access in GetMicroVM.Line 355 can panic if the interface slice is empty or uses CNI without StaticConfiguration.
🐛 Proposed fix
- ip := metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration.IPAddr.IP.String() - vm := &MicroVM{VMID: vmid, Pool: pool.config.Name, IPAddr: ip, CreatedAt: metadata.createdAt} + ip := "" + if len(metadata.machine.Cfg.NetworkInterfaces) > 0 { + ni := metadata.machine.Cfg.NetworkInterfaces[0] + if ni.StaticConfiguration != nil && ni.StaticConfiguration.IPConfiguration != nil { + ip = ni.StaticConfiguration.IPConfiguration.IPAddr.IP.String() + } + } + vm := &MicroVM{VMID: vmid, Pool: pool.config.Name, IPAddr: ip, CreatedAt: metadata.createdAt}#!/bin/bash # Check network interface setup and StaticConfiguration usage. rg -n 'NetworkInterfaces|CNIConfiguration|StaticConfiguration|IPConfiguration' -C2server/pool.go (4)
150-161: Avoid time.Sleep for Run/Stop synchronization.Line 160 uses a fixed sleep to wait for Run to exit — this is brittle. Prefer a done channel closed by Run and awaited by Stop.
🔧 Sketch of fix
- // Give Start() loop a moment to exit cleanly and release the lock - time.Sleep(100 * time.Millisecond) + // Wait for Run() loop to exit + <-p.doneCh#!/bin/bash # Locate Run/Stop to wire a doneCh signal. rg -n 'func \(p \*Pool\) (Run|Stop)\(' -C2
410-413: Protect installationID against concurrent read/write.Line 411 writes
installationIDwhile deleteGitHubRunner (Line 608) reads it without synchronization. Use atomic or a mutex.#!/bin/bash # List all installationID accesses. rg -n '\binstallationID\b' -C2
455-469: Cleanup goroutine can leak when Wait uses Background context.Line 465 waits on
context.Background()even after pool cancellation. Usep.ctx(if supported) or stop the machine on cancel.🩹 Proposed adjustment (if Wait accepts context)
- go func() { - _ = md.machine.Wait(context.Background()) - close(done) - }() + go func() { + _ = md.machine.Wait(p.ctx) + close(done) + }()firecracker-go-sdk Machine Wait context signature
501-539: Release machinesMu before blocking scale-down work.
scaleDownholdsmachinesMuacross StopVMM/Wait/GitHub/lease calls, which can block other operations and cause contention.🧩 Suggested refactor
- p.machinesMu.Lock() - defer p.machinesMu.Unlock() + p.machinesMu.Lock() // Find a machine to remove (pick the first one) var targetMetadata *machineMetadata var targetName string for name, metadata := range p.machines { targetMetadata = metadata targetName = name break } if targetMetadata == nil { + p.machinesMu.Unlock() return fmt.Errorf("no machines available to scale down") } + // Remove from map before blocking operations + delete(p.machines, targetName) + p.machinesMu.Unlock() - - // Stop the machine + // Stop the machine err := targetMetadata.machine.StopVMM()#!/bin/bash # Inspect scaleDown body for lock scope and blocking calls. rg -n 'func \(p \*Pool\) scaleDown' -A80
🧹 Nitpick comments (5)
Makefile (1)
1-4: Consider defaulting GOOS/GOARCH to host values.Hard-coding linux/amd64 makes
make buildproduce non-native binaries on most dev machines. Prefer host defaults with override support.♻️ Proposed refactor
-GOOS := linux -GOARCH := amd64 +GOOS ?= $(shell $(GO) env GOOS) +GOARCH ?= $(shell $(GO) env GOARCH)Also applies to: 14-14
commands/login.go (1)
39-51: Consider validating IP address format before SSH execution.While
vm.IPAddrcomes from a trusted server API, validating the IP format provides defense-in-depth against potential command injection if the server response is compromised or malformed.💡 Suggested improvement
+import "net" + func runLoginCmd(cmd *cobra.Command, vmID string) error { vm, _, err := client.GetMicroVM(context.Background(), vmID) if err != nil { return fmt.Errorf("failed to get VM details: %w", err) } if vm.IPAddr == "" { return fmt.Errorf("VM %s does not have an IP address yet (still starting)", vmID) } + + if net.ParseIP(vm.IPAddr) == nil { + return fmt.Errorf("VM %s has invalid IP address: %s", vmID, vm.IPAddr) + }server/image_manager.go (1)
16-26: Consider documenting thread-safety guarantees.The struct has two mutexes serving different purposes. A brief comment clarifying which mutex protects which state would improve maintainability.
💡 Suggested improvement
// imageManager manages container images with deduplication and caching. // It ensures that multiple pools pulling the same image will only trigger one pull operation. +// +// Thread-safety: containerdMu guards all containerd client operations; +// pullsMu guards the pullsInFlight map for deduplication tracking. type imageManager struct { - containerd *containerd.Client - containerdMu *sync.Mutex - logger *zerolog.Logger - - // Track in-progress pulls to avoid duplicate pulls - pullsMu sync.Mutex - pullsInFlight map[string]*imagePullRequest + containerd *containerd.Client // containerd client for image operations + containerdMu *sync.Mutex // guards containerd client calls + logger *zerolog.Logger + + pullsMu sync.Mutex // guards pullsInFlight map + pullsInFlight map[string]*imagePullRequest // tracks in-progress pulls for deduplication }types.go (1)
119-132: Usestrings.Joinfor cleaner and more efficient label formatting.The current implementation manually concatenates strings in a loop. Using
strings.Joinis more idiomatic and avoids repeated string allocations.♻️ Suggested refactor
+import "strings" + // formatLabels converts a slice of labels to a comma-separated string func formatLabels(labels []string) string { - if len(labels) == 0 { - return "" - } - result := "" - for i, label := range labels { - if i > 0 { - result += ", " - } - result += label - } - return result + return strings.Join(labels, ", ") }commands/ps.go (1)
25-30: Usecmd.Context()instead ofcontext.Background().Using
cmd.Context()enables proper context propagation and cancellation support when the command is interrupted (e.g., via Ctrl+C).Suggested fix
func runPsCmd(cmd *cobra.Command) error { // List all VMs across all pools using the new endpoint - microvms, _, err := client.ListMicroVMs(context.Background(), "") + microvms, _, err := client.ListMicroVMs(cmd.Context(), "") if err != nil { return fmt.Errorf("failed to list VMs: %w", err) }
9399947 to
cf31fb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
commands/pools.go (1)
60-68: Incorrect error message in resume command.The error message incorrectly says "pause pool" instead of "resume pool".
🐛 Proposed fix
func runPoolsResumeCmd(cmd *cobra.Command, args []string) error { _, err := client.ResumePool(cmd.Context(), args[0]) if err != nil { - return fmt.Errorf("pause pool \"%s\": %w", args[0], err) + return fmt.Errorf("resume pool \"%s\": %w", args[0], err) } fmt.Printf("Pool \"%s\" resumed\n", args[0]) return nil }
🤖 Fix all issues with AI agents
In `@docs/user-guide/monitoring.md`:
- Around line 53-57: The PromQL average-duration expression using
rate(fireactions_scale_duration_seconds_sum{direction="up"}[5m]) /
rate(fireactions_scale_duration_seconds_count{direction="up"}[5m]) can divide by
zero when no scale-up events occur; update the divisor to use
clamp_min(rate(fireactions_scale_duration_seconds_count{direction="up"}[5m]),
1e-10) to avoid division-by-zero and change the text/heading to use the compound
adjective "scale-up" instead of "scale up" wherever the metric/description
appears.
♻️ Duplicate comments (5)
docs/user-guide/monitoring.md (1)
42-45: Capacity utilization query can divide by zero.This PromQL query will produce NaN when
fireactions_pool_runners_desiredis 0 (pool scaled down). This was flagged in a previous review but appears unaddressed. Consider applying the sameclamp_minprotection used in the success rate query below.💡 Suggested fix
Monitor pool capacity utilization: ```promql -fireactions_pool_runners_current{pool="default"} / fireactions_pool_runners_desired{pool="default"} +fireactions_pool_runners_current{pool="default"} / clamp_min(fireactions_pool_runners_desired{pool="default"}, 1)</details> </blockquote></details> <details> <summary>server/microvm.go (1)</summary><blockquote> `22-26`: **Potential nil pointer dereference on `StaticConfiguration` or `IPConfiguration`.** While the bounds check on `NetworkInterfaces` was added, accessing `StaticConfiguration.IPConfiguration.IPAddr.IP.String()` can still panic if `StaticConfiguration` or `IPConfiguration` is nil. The past review suggested a more comprehensive defensive check. <details> <summary>🐛 Proposed defensive check</summary> ```diff for _, metadata := range p.machines { ipAddr := "" - if len(metadata.machine.Cfg.NetworkInterfaces) > 0 { - ipAddr = metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration.IPAddr.IP.String() + if len(metadata.machine.Cfg.NetworkInterfaces) > 0 && + metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration != nil && + metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration != nil { + ipAddr = metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration.IPAddr.IP.String() }docs/api/index.md (1)
249-257: MicroVM response examples don’t match actual JSON fields.Line 253 and Line 290 show
vmid,pool,ip_addr,created_at, andrunner_id, but theMicroVMAPI type uses json tagsVMID,Pool,IPAddr,CreatedAtand has norunner_id. The examples should align with the actual response shape (or update the struct/tags if the intent is snake_case + runner_id).📝 Proposed doc fix (align examples with current json tags)
- "vmid": "default-abc123", - "pool": "default", - "ip_addr": "192.168.1.100", - "runner_id": 456789, - "created_at": "2026-01-23T10:00:00Z" + "VMID": "default-abc123", + "Pool": "default", + "IPAddr": "192.168.1.100", + "CreatedAt": "2026-01-23T10:00:00Z"- "vmid": "default-abc123", - "pool": "default", - "ip_addr": "192.168.1.100", - "runner_id": 456789, - "created_at": "2026-01-23T10:00:00Z" + "VMID": "default-abc123", + "Pool": "default", + "IPAddr": "192.168.1.100", + "CreatedAt": "2026-01-23T10:00:00Z"Also applies to: 287-295
server/image_manager.go (1)
133-160: Avoid holdingcontainerdMuacross the long‑running Pull.Line 155–160 still holds the mutex during
containerd.Pull, which can block unrelated image operations. Consider unlocking before the pull and re‑checking if needed.♻️ Proposed adjustment
- im.containerdMu.Lock() - image, err = im.containerd.Pull(ctx, ref, - containerd.WithPullUnpack, - containerd.WithResolver(resolver), - containerd.WithPullSnapshotter(defaultSnapshotter)) - im.containerdMu.Unlock() + image, err = im.containerd.Pull(ctx, ref, + containerd.WithPullUnpack, + containerd.WithResolver(resolver), + containerd.WithPullSnapshotter(defaultSnapshotter))Does the containerd Go client require external synchronization for concurrent Pull/GetImage calls, or is it safe to run Pull without a global mutex?server/pool.go (1)
152-163: Replace the fixed sleep with a proper Run() completion signal.Line 162 still uses
time.Sleepas a synchronization proxy; this is brittle.
🧹 Nitpick comments (5)
server/config.go (1)
105-107: Defer pattern is verbose but acceptable.The anonymous function wrapper explicitly discards the
Close()error. For a read-only operation this is fine. A simplerdefer file.Close()would suffice, but the current approach makes the intent explicit.♻️ Optional simplification
- defer func() { - _ = file.Close() - }() + defer file.Close()client_test.go (1)
29-44: Test could be strengthened to validate request body.The test correctly updates the
ScalePoolcall signature with the newreplicasparameter. However, the mock server doesn't verify that the request body contains the expected{"replicas": 1}payload. This is a minor test coverage gap.💡 Optional: Validate request body in test
func TestClient_ScalePool(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != "POST" || r.URL.Path != "/api/v1/pools/test/scale" { t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) } + + var body struct { + Replicas int `json:"replicas"` + } + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Errorf("failed to decode request body: %v", err) + } + if body.Replicas != 1 { + t.Errorf("unexpected replicas: got %d, want 1", body.Replicas) + } w.WriteHeader(http.StatusOK) }))commands/ps.go (2)
25-38: Usecmd.Context()for proper context propagation.Using
context.Background()at line 27 ignores the command's context, which means cancellation signals (e.g., Ctrl+C) won't be properly propagated to the API call.♻️ Proposed fix
func runPsCmd(cmd *cobra.Command) error { // List all VMs across all pools using the new endpoint - microvms, _, err := client.ListMicroVMs(context.Background(), "") + microvms, _, err := client.ListMicroVMs(cmd.Context(), "") if err != nil { return fmt.Errorf("failed to list VMs: %w", err) }
3-9: Unused import after suggested fix.If you adopt the fix to use
cmd.Context(), the"context"import on line 4 becomes unused and should be removed.♻️ Proposed fix
import ( - "context" "fmt" "github.com/hostinger/fireactions/helper/printer" "github.com/spf13/cobra" )server/server.go (1)
308-327: Reduce server‑lock contention during MicroVM scans.Line 310–326 and Line 344–361 hold
s.lwhile calling into per‑pool logic. Consider snapshottings.poolsunder lock, then releasing it before iterating.♻️ Suggested refactor
- s.l.Lock() - defer s.l.Unlock() - - var allVMs []*MicroVM - for _, pool := range s.pools { + s.l.Lock() + pools := make([]*Pool, 0, len(s.pools)) + for _, pool := range s.pools { + pools = append(pools, pool) + } + s.l.Unlock() + + var allVMs []*MicroVM + for _, pool := range pools { vms, err := pool.ListMicroVMs(ctx, pool.config.Name) // ... }- s.l.Lock() - defer s.l.Unlock() - - for _, pool := range s.pools { + s.l.Lock() + pools := make([]*Pool, 0, len(s.pools)) + for _, pool := range s.pools { + pools = append(pools, pool) + } + s.l.Unlock() + + for _, pool := range pools { pool.machinesMu.Lock() // ... }Also applies to: 344-362
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
commands/pools.go (1)
60-68: Fix the error message to say "resume" instead of "pause".The error message at line 63 incorrectly says "pause pool" when this is the resume operation.
🐛 Proposed fix
func runPoolsResumeCmd(cmd *cobra.Command, args []string) error { _, err := client.ResumePool(cmd.Context(), args[0]) if err != nil { - return fmt.Errorf("pause pool \"%s\": %w", args[0], err) + return fmt.Errorf("resume pool \"%s\": %w", args[0], err) } fmt.Printf("Pool \"%s\" resumed\n", args[0]) return nil }server/server.go (1)
287-303: Data race:pool.configis assigned without holding the pool's lock.Line 290 directly assigns
pool.config = poolConfigwhile the pool'sRun()loop may be concurrently reading config fields. This is the same class of data race that was previously identified forReplicas.Consider using a thread-safe method to update the config, or hold the pool's lock during assignment.
🔒 Proposed fix: Add synchronization for config update
for _, poolConfig := range s.config.Pools { pool, ok := s.pools[poolConfig.Name] if ok { + pool.l.Lock() pool.config = poolConfig + pool.l.Unlock() s.logger.Info().Msgf("Pool %s reloaded", poolConfig.Name) continue }Alternatively, consider adding a
SetConfig()method on thePooltype that handles synchronization internally, similar to howSetReplicas()was implemented.server/pool.go (1)
296-314: Data race:isActiveaccessed without synchronization.
p.isActiveis read inRun()(line 133) and written inPause()/Resume()from potentially different goroutines without synchronization.🐛 Proposed fix: Use atomic.Bool
type Pool struct { // ... - isActive bool + isActive atomic.Bool // ... } func NewPool(...) (*Pool, error) { // ... p := &Pool{ // ... - isActive: true, // ... } + p.isActive.Store(true) // ... } func (p *Pool) Pause() { - if !p.isActive { + if !p.isActive.Load() { return } p.logger.Debug().Msgf("Pool %s state changed to paused", p.config.Name) - p.isActive = false + p.isActive.Store(false) } func (p *Pool) Resume() { - if p.isActive { + if p.isActive.Load() { return } p.logger.Debug().Msgf("Pool %s state changed to active", p.config.Name) - p.isActive = true + p.isActive.Store(true) }Also update the read in
Run():- if !p.isActive { + if !p.isActive.Load() {
🤖 Fix all issues with AI agents
In `@commands/login.go`:
- Around line 29-35: After calling client.GetMicroVM in the login flow, add a
nil check for the returned vm before accessing vm.IPAddr (handle the case where
GetMicroVM returns (nil, nil)); if vm == nil return a clear error like "failed
to get VM details: received nil VM for <vmID>" (preserve existing error wrapping
for non-nil err), then proceed to check vm.IPAddr as before—update the code
around the GetMicroVM call and the vm.IPAddr check to first guard vm != nil.
In `@docs/api/index.md`:
- Around line 13-16: Update the Base URL section to avoid the inaccurate blanket
statement; clarify that only the versioned resource-management endpoints (e.g.,
routes handling "pools", "microvms", and "reload") are prefixed with /api/v1,
while utility endpoints like /healthz and /version are top-level routes outside
the /api/v1 prefix. Edit the sentence in docs/api/index.md to mention both
categories (versioned resource endpoints vs. unversioned utility routes) and
optionally give /healthz and /version as examples of the unversioned routes.
In `@docs/user-guide/monitoring.md`:
- Around line 53-57: Change the heading text "Average scale up duration" to use
a hyphenated compound adjective: "Average scale-up duration" so the heading
reads correctly; locate the heading line that currently contains the text
"Average scale up duration" in docs/user-guide/monitoring.md and replace it with
"Average scale-up duration".
In `@server/pool.go`:
- Around line 330-333: GetCurrentSize currently reads the map p.machines without
synchronization which races with scaleUp/scaleDown/Stop; update
Pool.GetCurrentSize to acquire the Pool.machinesMu read lock (e.g., RLock)
before computing len(p.machines) and release it (defer RUnlock) so the length
read is synchronized with writers in scaleUp, scaleDown and Stop.
♻️ Duplicate comments (4)
docs/user-guide/monitoring.md (1)
42-45: Guard capacity utilization query against zero desired replicas.When a pool is scaled to 0, this division can produce NaN/Inf. Consider guarding the denominator (e.g.,
clamp_min) or filtering zeros.💡 Suggested tweak
-fireactions_pool_runners_current{pool="default"} / fireactions_pool_runners_desired{pool="default"} +fireactions_pool_runners_current{pool="default"} / clamp_min(fireactions_pool_runners_desired{pool="default"}, 1)docs/api/index.md (1)
248-297: Verify MicroVM example fields match API JSON tags.The examples include
runner_idand snake_case keys; please confirm the MicroVM API response fields/tags align and update docs or the struct accordingly.#!/bin/bash # Locate MicroVM struct and JSON tags. rg -n "type MicroVM struct" --type go -A25 rg -n "runner_id|RunnerID" --type go -C3 rg -n "microvm" --type go -C3server/microvm.go (1)
23-26: Guard nested network config before dereference.Line 24-26 only checks slice length;
StaticConfigurationorIPConfigurationcan still be nil and panic. Add defensive checks before reading IP.🐛 Proposed fix
ipAddr := "" if len(metadata.machine.Cfg.NetworkInterfaces) > 0 { - ipAddr = metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration.IPAddr.IP.String() + ni := metadata.machine.Cfg.NetworkInterfaces[0] + if ni.StaticConfiguration != nil && ni.StaticConfiguration.IPConfiguration != nil { + ipAddr = ni.StaticConfiguration.IPConfiguration.IPAddr.IP.String() + } }server/pool.go (1)
162-163: Fragile synchronization:time.Sleepused to wait for Run() loop exit.This was previously flagged. A fixed sleep duration is unreliable—if the
Run()loop takes longer than 100ms to release the lock,Stop()may encounter contention.Consider using a done channel that
Run()closes when it exits, allowingStop()to wait deterministically.
🧹 Nitpick comments (2)
server/server.go (1)
310-327: Consider reducing lock scope when aggregating MicroVMs.The server lock is held for the entire duration of iterating pools and calling
pool.ListMicroVMs(). IfListMicroVMsperforms any I/O or lengthy operations, this could cause lock contention.Consider copying the pool references while holding the lock, then releasing it before iterating:
♻️ Proposed refactor to reduce lock contention
func (s *Server) ListMicroVMs(ctx context.Context, poolName string) ([]*MicroVM, error) { if poolName == "" { s.l.Lock() - defer s.l.Unlock() - - var allVMs []*MicroVM - for _, pool := range s.pools { + pools := make([]*Pool, 0, len(s.pools)) + for _, pool := range s.pools { + pools = append(pools, pool) + } + s.l.Unlock() + + var allVMs []*MicroVM + for _, pool := range pools { vms, err := pool.ListMicroVMs(ctx, pool.config.Name)server/pool.go (1)
214-219: Minor: Safety cleanup holds lock during potential network I/O.The safety loop calls
deleteGitHubRunner(10s timeout) while holdingmachinesMu. Since this is during shutdown and shouldn't normally execute (the main loop already cleaned up), the impact is minimal. Consider moving the deletion outside the lock for consistency.♻️ Suggested improvement
// Clean up any remaining runners (shouldn't happen but safety check) p.machinesMu.Lock() + remaining := make(map[string]int64) for runnerName, metadata := range p.machines { - p.deleteGitHubRunner(runnerName, metadata.runnerID) + remaining[runnerName] = metadata.runnerID } p.machinesMu.Unlock() + + for runnerName, runnerID := range remaining { + p.deleteGitHubRunner(runnerName, runnerID) + }
4baf8cc to
6353b5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/cli/index.md (1)
7-7: Minor wording polish“Firecracker based” → “Firecracker‑based”.
✏️ Suggested edit
-BYOM (Bring Your Own Metal) and run self-hosted GitHub runners in ephemeral, fast and secure Firecracker based virtual machines. +BYOM (Bring Your Own Metal) and run self-hosted GitHub runners in ephemeral, fast and secure Firecracker‑based virtual machines.server/handlers.go (1)
1-167: InitializemachinesMufield inTestGetPoolHandlerpool mock.The test panic in
TestGetPoolHandleroriginates from a nil pointer dereference whenGetCurrentSize()attempts to lock the uninitializedmachinesMufield. The Pool struct mock on lines 109–123 must includemachinesMu: &sync.Mutex{}alongside the existingmachines,l, and other fields.commands/pools.go (1)
60-67: Use Cobra’s output writer for success messages.
fmt.Printfwrites to stdout directly and bypassescmd.OutOrStdout(), which can break tests and output redirection.💡 Suggested change
- fmt.Printf("Pool \"%s\" resumed\n", args[0]) + fmt.Fprintf(cmd.OutOrStdout(), "Pool \"%s\" resumed\n", args[0]) @@ - fmt.Printf("Pool \"%s\" replicas set to %d\n", args[0], replicas) + fmt.Fprintf(cmd.OutOrStdout(), "Pool \"%s\" replicas set to %d\n", args[0], replicas) @@ - fmt.Printf("Pool \"%s\" paused\n", args[0]) + fmt.Fprintf(cmd.OutOrStdout(), "Pool \"%s\" paused\n", args[0])Also applies to: 84-93, 107-114
server/pool.go (1)
284-302: Data race:isActiveaccessed without synchronization.
isActiveis read inRun()(line 137) and written inPause()/Resume()without any synchronization. These can execute concurrently from different goroutines.🐛 Proposed fix: Use atomic.Bool for isActive
type Pool struct { // ...existing fields... - isActive bool + isActive atomic.Bool // ... } func NewPool(...) (*Pool, error) { // ... p := &Pool{ // ... - isActive: true, // ... } + p.isActive.Store(true) // ... } func (p *Pool) Run() { // ... - if !p.isActive { + if !p.isActive.Load() { p.logger.Debug().Msgf("Pool %s is paused, skipping scaling", p.config.Name) continue } // ... } func (p *Pool) Pause() { - if !p.isActive { + if !p.isActive.Load() { return } p.logger.Debug().Msgf("Pool %s state changed to paused", p.config.Name) - p.isActive = false + p.isActive.Store(false) } func (p *Pool) Resume() { - if p.isActive { + if p.isActive.Load() { return } p.logger.Debug().Msgf("Pool %s state changed to active", p.config.Name) - p.isActive = true + p.isActive.Store(true) }
🤖 Fix all issues with AI agents
In @.github/workflows/lint.yaml:
- Around line 19-27: Update acknowledges that actions/setup-go@v5 with
go-version: 1.25.6 and golangci/golangci-lint-action@v9 using version: v2.8.0
are compatible, but before merging run golangci-lint v2.8.0 locally or in a test
branch to surface new linter rules (e.g., godoc-lint require-stdlib-doclink,
gosec G116, modernize rules) and fix or suppress violations; if unacceptable
noise appears, either pin golangci-lint to the previous working version or add
targeted linter disables/linters-settings in the workflow/config to prevent CI
breakage.
In `@commands/login.go`:
- Around line 48-52: The SSH command construction (sshCmd) currently disables
host key verification by adding "-o StrictHostKeyChecking=no" and "-o
UserKnownHostsFile=/dev/null"; change this so strict host key checking is the
default and only disable it when an explicit opt-in is provided (e.g., add a
boolean flag like insecureSkipHostKeyCheck or --allow-insecure-hostkey to the
login command). Update the code that constructs sshCmd to conditionally append
the two insecure options only when that flag is set, and if you prefer an
interactive flow, prompt the user for confirmation before appending those
options; keep the target host formatting using fmt.Sprintf("root@%s", vm.IPAddr)
unchanged.
- Around line 42-52: The SSH target string is not handling IPv6 literals, so
build the target as "root@<addr>" for IPv4 but "root@[<addr>]" for IPv6: detect
IPv6 by checking vm.IPAddr for ':' or use net.ParseIP(vm.IPAddr).To4() == nil,
then format the user/host accordingly (replace fmt.Sprintf("root@%s", vm.IPAddr)
with a conditional that produces "root@[addr]" for IPv6 and "root@addr" for
IPv4) before creating exec.Command("ssh", ...), leaving other ssh options
unchanged.
In `@docs/cli/index.md`:
- Around line 36-40: The docs currently show inline credentials with the CLI
flags --username and --password which can leak secrets; update the examples in
docs/cli/index.md (and the other referenced example at lines ~168-172) to
demonstrate using environment variables (e.g. FIREACTIONS_USERNAME and
FIREACTIONS_PASSWORD) or an interactive prompt instead of passing secrets on the
command line, and add a short warning sentence advising users not to put
credentials in shell history or process listings; refer to the flags --username
and --password in the text so readers know the env-vars or prompt will populate
those values.
In `@server/convert.go`:
- Around line 8-18: The panic comes from dereferencing p.config and
p.config.Runner in convertPool when Runner is nil; update convertPool to
defensively handle missing config/Runner by checking if p.config == nil and if
p.config.Runner == nil before accessing Organization, GroupID, Labels, Image,
and Name, and supply safe defaults (empty string or empty map/slice) to the
fireactions.Pool fields (Name, Organization, GroupID, Labels, Image) and keep
CurrentReplicas/DesiredReplicas logic intact; modify places that call
p.GetReplicas() or p.GetCurrentSize() only if p is valid and prefer using helper
getters on p that return default values when config or Runner is absent to avoid
future nil derefs.
In `@server/pool.go`:
- Around line 318-323: Tests are constructing Pool values without initializing
internal mutexes, causing panics when methods like GetCurrentSize call
p.machinesMu.Lock(); update tests to either call the constructor NewPool(...) or
explicitly initialize the mutex fields on the test Pool literals (e.g., set
machinesMu = sync.Mutex{} and any other mutexes used), or add a small helper
factory in tests (e.g., newTestPool) that returns a fully initialized *Pool;
ensure all tests replace direct Pool struct literals with calls to
NewPool/newTestPool so methods like GetCurrentSize can safely Lock/Unlock
machinesMu.
♻️ Duplicate comments (2)
docs/api/index.md (1)
286-297: Samerunner_idinconsistency in single MicroVM response.This example also includes
runner_idwhich doesn't exist in theMicroVMstruct, and uses inconsistent JSON field casing.docs/user-guide/monitoring.md (1)
42-45: Guard capacity utilization query against zero desired replicas.When desired replicas is 0, this can divide by zero. (Duplicate of earlier review feedback.)
💡 Suggested improvement
-fireactions_pool_runners_current{pool="default"} / fireactions_pool_runners_desired{pool="default"} +fireactions_pool_runners_current{pool="default"} / clamp_min(fireactions_pool_runners_desired{pool="default"}, 1)
🧹 Nitpick comments (2)
commands/login.go (1)
28-58: Use the Cobra command context for cancellation propagation.This keeps VM lookup and SSH aligned with CLI cancellation (Ctrl+C, parent context).
♻️ Proposed refactor
-func runLoginCmd(cmd *cobra.Command, vmID string) error { - vm, _, err := client.GetMicroVM(context.Background(), vmID) +func runLoginCmd(cmd *cobra.Command, vmID string) error { + ctx := cmd.Context() + vm, _, err := client.GetMicroVM(ctx, vmID) if err != nil { return fmt.Errorf("failed to get VM details: %w", err) } @@ - sshCmd := exec.Command("ssh", + sshCmd := exec.CommandContext(ctx, "ssh", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "LogLevel=ERROR", fmt.Sprintf("root@%s", vm.IPAddr))server/image_manager.go (1)
133-142: Missing re-check after lock release in pullImage.After releasing
containerdMuat line 136 and before acquiring it again at line 155, another goroutine could have pulled the same image. WhilepullImageWithDedupprovides deduplication at thecacheKeylevel,pullImagecan still be called directly. Consider adding a re-check after re-acquiring the lock to avoid redundant pulls:♻️ Suggested improvement
im.containerdMu.Lock() + // Re-check if image exists (might have been pulled while lock was released) + image, err = im.containerd.GetImage(ctx, ref) + if err == nil { + im.containerdMu.Unlock() + return image, nil + } + image, err = im.containerd.Pull(ctx, ref,
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
docs/user-guide/concepts.md (1)
9-9: Fix plural agreement in paused pools sentence.“Pools” is plural but “it” is singular. Consider “which prevents them from scaling up.”
✏️ Suggested edit
-Pools can be paused via CLI, which prevents it from scaling up. +Pools can be paused via CLI, which prevents them from scaling up.Makefile (1)
16-18: Clean target does not remove the build artifact.The
buildtarget outputs to./fireactions, butcleanonly removes thedistdirectory. This inconsistency meansmake cleanwon't actually clean the build output.🔧 Proposed fix
.PHONY: clean clean: - @ rm -rf dist + @ rm -rf dist fireactionscommands/pools.go (1)
51-67: Fix resume error message (currently says “pause pool”).
This is misleading for users when resume fails.💡 Proposed fix
- return fmt.Errorf("pause pool \"%s\": %w", args[0], err) + return fmt.Errorf("resume pool \"%s\": %w", args[0], err)docs/user-guide/monitoring.md (1)
66-66: Typo: "vizualisation" should be "visualization".✏️ Suggested fix
-Example Grafana dashboard for vizualisation of Fireactions metrics: +Example Grafana dashboard for visualization of Fireactions metrics:server/server.go (1)
287-303: Potential data race when updating pool config during Reload.Line 290 directly assigns
pool.config = poolConfigwithout holding the pool's lock (pool.l). TheRun()loop reads fields frompool.config(e.g., viaGetReplicas(),config.Name,config.Runner.Organization). This can cause a data race ifReload()is called whileRun()is executing.🔧 Suggested fix
pool, ok := s.pools[poolConfig.Name] if ok { + pool.l.Lock() pool.config = poolConfig + pool.l.Unlock() s.logger.Info().Msgf("Pool %s reloaded", poolConfig.Name) continue }Alternatively, consider adding a
SetConfig()method on Pool similar toSetReplicas().server/pool.go (1)
284-302: Data race onisActivefield in Pause/Resume.
Pause()andResume()modifyp.isActivewithout holding a lock, whileRun()readsp.isActiveon line 137. This creates a potential data race.🔧 Suggested fix
// Pause pauses the pool. Pausing the pool will prevent the pool from scaling. func (p *Pool) Pause() { + p.l.Lock() + defer p.l.Unlock() + if !p.isActive { return } p.logger.Debug().Msgf("Pool %s state changed to paused", p.config.Name) p.isActive = false } // Resume resumes the pool. Resuming the pool will allow the pool to scale. func (p *Pool) Resume() { + p.l.Lock() + defer p.l.Unlock() + if p.isActive { return } p.logger.Debug().Msgf("Pool %s state changed to active", p.config.Name) p.isActive = true }And in
Run():+ p.l.Lock() + active := p.isActive + p.l.Unlock() - if !p.isActive { + if !active { p.logger.Debug().Msgf("Pool %s is paused, skipping scaling", p.config.Name) continue }
🤖 Fix all issues with AI agents
In `@docs/api/index.md`:
- Around line 341-346: Remove the `200 OK` item from the "Common HTTP status
codes:" list in the docs section (the list that currently includes `200 OK`,
`400 Bad Request`, `401 Unauthorized`, `404 Not Found`, `500 Internal Server
Error`), leaving only error-related codes; if needed, create or move a brief
"Status codes" note elsewhere for `200 OK` rather than listing it under the
error responses heading.
In `@server/image_manager.go`:
- Around line 84-116: The pull logic currently ignores isAlways; update
pullImageWithDedup to pass the isAlways flag into pullImage (call pullImage(ctx,
imageRef, isAlways)) and change pullImage's signature to accept isAlways (e.g.,
func (im *imageManager) pullImage(ctx context.Context, imageRef string, isAlways
bool) ...); inside pullImage, when isAlways is true, skip the early local
GetImage() short-circuit so you always call containerd.Pull() (still use
GetImage() when isAlways is false to preserve the cached-fast path), and ensure
error handling and returned containerd.Image/err remain consistent.
♻️ Duplicate comments (5)
docs/api/index.md (1)
248-259: Verify MicroVM response fields and JSON casing match the implementation.The examples include
runner_idand snake_case keys. Please confirm the MicroVM API type and JSON tags actually emit these fields and casing; otherwise update the examples or align the struct/tags.#!/bin/bash # Verify MicroVM struct fields + JSON tags vs docs examples rg -n "type MicroVM struct" -A 30 --type go rg -n "json:\".*\"" --type go -C 2 | rg -n "MicroVM|microvm|runner_id|vmid|ip_addr|created_at" rg -n "microvm" --type go -C 3 | rg -n "runner_id|RunnerID|vmid|ip_addr|created_at"Also applies to: 286-295
server/image_manager.go (1)
133-160: Avoid holdingcontainerdMuduring the long pull.The mutex is held across
containerd.Pull, which can take a long time and blocks other containerd operations.🐛 Proposed fix (release lock before pull)
- im.containerdMu.Lock() - image, err = im.containerd.Pull(ctx, ref, - containerd.WithPullUnpack, - containerd.WithResolver(resolver), - containerd.WithPullSnapshotter(defaultSnapshotter)) - im.containerdMu.Unlock() + image, err = im.containerd.Pull(ctx, ref, + containerd.WithPullUnpack, + containerd.WithResolver(resolver), + containerd.WithPullSnapshotter(defaultSnapshotter))Please confirm containerd client concurrency guarantees before removing the lock.
containerd Client thread safety; concurrent Pull and GetImage usageserver/microvm.go (1)
22-33: Still possible to panic on nil network config.
You now checklen(NetworkInterfaces), butStaticConfigurationorIPConfigurationcan still be nil. This is the same risk previously raised—please keep the defensive checks.🐛 Proposed fix
ipAddr := "" if len(metadata.machine.Cfg.NetworkInterfaces) > 0 { - ipAddr = metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration.IPAddr.IP.String() + ni := metadata.machine.Cfg.NetworkInterfaces[0] + if ni.StaticConfiguration != nil && ni.StaticConfiguration.IPConfiguration != nil { + ipAddr = ni.StaticConfiguration.IPConfiguration.IPAddr.IP.String() + } }docs/user-guide/monitoring.md (1)
42-45: Division by zero risk in capacity utilization query.When
fireactions_pool_runners_desiredis 0 (pool scaled to zero), this query will result in division by zero. Similar to the success rate query below, consider usingclamp_min.💡 Suggested fix
Monitor pool capacity utilization: ```promql -fireactions_pool_runners_current{pool="default"} / fireactions_pool_runners_desired{pool="default"} +fireactions_pool_runners_current{pool="default"} / clamp_min(fireactions_pool_runners_desired{pool="default"}, 1)</details> </blockquote></details> <details> <summary>server/pool.go (1)</summary><blockquote> `461-507`: **Inner goroutine in cleanup will leak if machine never exits.** The goroutine spawned on line 468 calls `md.machine.Wait(context.Background())`. Since `context.Background()` is never cancelled, this goroutine will remain blocked indefinitely if the machine process hangs and never exits—even after `p.ctx.Done()` is selected on line 476. Passing `p.ctx` to `Wait()` alone is insufficient: while it would allow `Wait()` to return when the pool context is cancelled, it does **not** stop the VMM process itself. The machine may continue running, and if it never exits, the goroutine will still leak. When the pool context is cancelled, you should call `StopVMM()` to force the machine to stop before—or instead of—proceeding with cleanup. <details> <summary>💡 Suggested fix</summary> ```diff select { case <-waitDone: // Machine exited normally case <-p.ctx.Done(): // Pool is stopping, forcibly stop the machine + _ = md.machine.StopVMM() }Alternatively, pass
p.ctxtoWait()and ensureStopVMM()is called:go func() { - _ = md.machine.Wait(context.Background()) + _ = md.machine.Wait(p.ctx) close(waitDone) }() select { case <-waitDone: // Machine exited normally case <-p.ctx.Done(): // Pool is stopping, forcibly stop the machine + _ = md.machine.StopVMM() }
🧹 Nitpick comments (2)
docs/api/index.md (1)
7-11: Add a HTTPS-only warning for Basic Auth usage.These examples use Basic Auth over HTTP; please add a short note that credentials must be sent over HTTPS in production to avoid cleartext exposure.
server/server.go (1)
346-370: GetMicroVM directly accesses pool internals; consider encapsulation.
GetMicroVMdirectly accessespool.machinesMuandpool.machinesfrom the server context. This tight coupling could be improved by delegating to a Pool method (e.g.,pool.GetMicroVM(vmid)), which would be consistent with howListMicroVMsdelegates topool.ListMicroVMs().
6353b5f to
276f5da
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
commands/pools.go (1)
60-68: Incorrect error message in resume command.The error message on line 63 says "pause pool" but this is the resume command. This will confuse users when errors occur.
🐛 Proposed fix
func runPoolsResumeCmd(cmd *cobra.Command, args []string) error { _, err := client.ResumePool(cmd.Context(), args[0]) if err != nil { - return fmt.Errorf("pause pool \"%s\": %w", args[0], err) + return fmt.Errorf("resume pool \"%s\": %w", args[0], err) } fmt.Printf("Pool \"%s\" resumed\n", args[0]) return nil }
🤖 Fix all issues with AI agents
In `@server/metrics.go`:
- Around line 37-41: metricPoolScaleRequests currently lacks the "organization"
label; update its declaration (metricPoolScaleRequests =
promauto.NewCounterVec(..., []string{"pool"})) to include "organization" (e.g.,
[]string{"pool","organization"}) and then update all places that record this
metric—specifically where metricPoolScaleRequests.WithLabelValues(...) is called
in ScalePool (and any usages in server/server.go) to pass pool.Organization (or
the appropriate field from the pool object) as the organization label value so
it matches the labeling used by metricScaleOperations, metricScaleDuration,
metricPoolRunnersCurrent, and metricPoolRunnersDesired.
In `@server/server.go`:
- Around line 355-370: The code in GetMicroVM iterates pool.machines and
unconditionally indexes NetworkInterfaces[0], risking a panic; modify the loop
(in GetMicroVM) to first check that len(metadata.machine.Cfg.NetworkInterfaces)
> 0 and that the chosen interface has a non-nil StaticConfiguration and
IPConfiguration (or iterate to find the first interface with a valid IP) before
accessing IP.String(); if no valid interface/IP is found, continue the loop
without returning. Ensure the pool.machinesMu locking/unlocking behavior remains
correct when you skip entries.
In `@types.go`:
- Around line 89-100: Pools.ColsMap() currently uses keys like "CurrentReplicas"
and "DesiredReplicas" that don't match the keys produced by Pools.KV(); update
the map in Pools.ColsMap() so its keys exactly match the column keys returned by
Pools.KV() (e.g., use the same "Current" and "Desired" keys or whatever exact
identifiers Pools.KV() emits) to ensure consistent lookups between
Pools.ColsMap() and Pools.KV().
- Around line 57-83: ColsMap keys for Pool and MicroVMs do not match the actual
column names returned by their KV() and Cols() methods, causing PrintText
validation failures; update the Pool ColsMap (function (p *Pool) ColsMap) to use
keys "Current", "Desired", and "Group ID" (instead of "CurrentReplicas",
"DesiredReplicas", "GroupID") to match Pool.KV() and Pool.Cols(), and also
update the MicroVMs ColsMap key from "IPAddr" to "IP Address" (or vice versa to
match MicroVMs.KV()/Cols()) so all ColsMap keys exactly match the strings
returned by the corresponding KV() and Cols() methods. Ensure each ColsMap entry
uses the exact column label used by its KV()/Cols() to fix the printer
validation.
♻️ Duplicate comments (7)
docs/api/index.md (2)
248-258: Verify MicroVM response fields/tags match the API.The examples show
runner_idand snake_case keys. Please confirm the MicroVM struct exposes those fields and JSON tags to avoid a docs/API mismatch.#!/bin/bash # Inspect MicroVM struct and JSON tags rg -n "type MicroVM struct" -A 30 --type go rg -n "RunnerID|runner_id" --type go -C3 rg -n 'json:"' --type go -C2 | rg -n "MicroVM|micro_vm|microvms"Also applies to: 288-295
341-346: Remove200 OKfrom the error-status list.This section is labeled “Error Responses,” so including
200 OKis misleading.📝 Suggested edit
-Common HTTP status codes: -- `200 OK` - Request succeeded +Common HTTP status codes:docs/cli/index.md (1)
36-40: Avoid inline credentials in CLI examples.Inline passwords leak via shell history/process listings; use env vars or prompting in examples.
📝 Suggested doc tweak
-fireactions --username admin --password secret pools list +FIREACTIONS_USERNAME=admin FIREACTIONS_PASSWORD=secret \ + fireactions pools list-fireactions -e https://fireactions.example.com -u admin -p secret pools list +FIREACTIONS_USERNAME=admin FIREACTIONS_PASSWORD=secret \ + fireactions -e https://fireactions.example.com pools listAlso applies to: 170-172
server/convert.go (1)
7-33: Fix nil pointer dereference inconvertPool.Accessing
p.configandp.config.Runner.*without nil checks will panic when either is unset (as seen in CI failures). Add defensive checks.🐛 Proposed fix with defensive nil checks
func convertPool(p *Pool) *fireactions.Pool { + if p == nil || p.config == nil { + return &fireactions.Pool{} + } + replicas := p.GetReplicas() + + var ( + org string + group int64 + labels []string + image string + ) + if p.config.Runner != nil { + org = p.config.Runner.Organization + group = p.config.Runner.GroupID + labels = p.config.Runner.Labels + image = p.config.Runner.Image + } + pool := &fireactions.Pool{ Name: p.config.Name, Replicas: replicas, CurrentReplicas: p.GetCurrentSize(), DesiredReplicas: replicas, - Organization: p.config.Runner.Organization, - GroupID: p.config.Runner.GroupID, - Labels: p.config.Runner.Labels, - Image: p.config.Runner.Image, + Organization: org, + GroupID: group, + Labels: labels, + Image: image, }commands/login_test.go (1)
13-27: Pipeline failure: Mock expectation set but command never executed.The test sets up a mock expectation for
GetMicroVMbut never callscmd.Execute()orcmd.RunE(), leaving the mock expectation unmet. This causes the pipeline failure.Remove the mock setup since this test only validates command structure:
🐛 Proposed fix
func TestLoginCmd_WithVMID(t *testing.T) { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - mockClient := mocks.NewMockfireactionsClient(ctrl) - client = mockClient - cmd := newLoginCmd() - cmd.SetArgs([]string{"test-vm-1"}) // We can't actually test SSH execution, but we can verify the command structure assert.NotNil(t, cmd) assert.Equal(t, "login <vmid>", cmd.Use) assert.Equal(t, "SSH into a running VM as root user", cmd.Short) }docs/user-guide/monitoring.md (1)
42-45: PromQL example may cause division by zero.When
fireactions_pool_runners_desiredis 0 (pool scaled down completely), this query will result in division by zero. Consider addingclamp_minsimilar to the other example queries.💡 Suggested improvement
Monitor pool capacity utilization: ```promql -fireactions_pool_runners_current{pool="default"} / fireactions_pool_runners_desired{pool="default"} +fireactions_pool_runners_current{pool="default"} / clamp_min(fireactions_pool_runners_desired{pool="default"}, 1)</details> </blockquote></details> <details> <summary>server/image_manager.go (1)</summary><blockquote> `52-73`: **Pull policy case mismatch may break image pulls.** The switch statement checks for capitalized values (`"Always"`, `"Never"`, `"IfNotPresent"`), but if the configuration validation enforces lowercase values, all cases will fail and fall through to the default error. Verify that the config values match these exact cases. ```shell #!/bin/bash # Check config validation for image pull policy values rg -n "image_pull_policy|ImagePullPolicy|oneof|validate" --type go -A 3 -B 3 | head -50
🧹 Nitpick comments (6)
commands/cmd_test.go (1)
30-31: Prefer asserting command names over count.A count-only check can be brittle if command order/feature flags change; consider asserting the expected subcommand names instead.
server/interface.go (1)
9-12: Document replicas semantics (absolute, non-negative).Consider adding a brief comment clarifying that
replicasis the absolute desired count and whether0is valid, to avoid confusion with the old delta behavior.docs/user-guide/configuration.md (1)
81-85: Consider noting that replicas can be 0.Since scale-down-to-zero is supported, adding a short note here would make the behavior explicit for users.
Makefile (1)
8-10: Consider UTC or SOURCE_DATE_EPOCH for BUILD_DATE.Using local time makes builds less reproducible. Consider defaulting to UTC and allowing overrides.
♻️ Optional tweak
-BUILD_DATE := $(shell date '+%FT%T') +BUILD_DATE ?= $(shell date -u '+%FT%TZ')docs/api/index.md (1)
7-11: Avoid inline credentials in curl examples.Inline passwords can leak via shell history and process listings. Prefer env vars in the example.
📝 Safer example
-curl -u username:password http://localhost:8080/api/v1/pools +USERNAME=admin PASSWORD=secret \ + curl -u "$USERNAME:$PASSWORD" http://localhost:8080/api/v1/poolsserver/convert.go (1)
44-62: Consider adding nil checks forconvertMicroVMandconvertMicroVMs.For consistency and defensive coding, consider adding nil checks to prevent panics if nil values are passed.
♻️ Suggested defensive checks
func convertMicroVM(m *MicroVM) *fireactions.MicroVM { + if m == nil { + return &fireactions.MicroVM{} + } microVM := &fireactions.MicroVM{ VMID: m.VMID, Pool: m.Pool, IPAddr: m.IPAddr, CreatedAt: m.CreatedAt, } return microVM } func convertMicroVMs(microVMs []*MicroVM) fireactions.MicroVMs { convertedMicroVMs := make(fireactions.MicroVMs, 0, len(microVMs)) for _, microVM := range microVMs { + if microVM == nil { + continue + } convertedMicroVMs = append(convertedMicroVMs, *convertMicroVM(microVM)) } return convertedMicroVMs }
8fa6c89 to
dc8d081
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/user-guide/concepts.md (1)
13-37: Mention that replicas can be set to 0 (scale‑down).Since the PR adds scale‑down to zero runners, please add a brief note near the example explaining that
replicascan be0to remove all runners from a pool.
♻️ Duplicate comments (9)
docs/user-guide/monitoring.md (1)
42-45: Guard capacity utilization query against zero desired replicas.
When desired replicas is 0, this yields division by zero.💡 Suggested fix
-fireactions_pool_runners_current{pool="default"} / fireactions_pool_runners_desired{pool="default"} +fireactions_pool_runners_current{pool="default"} / clamp_min(fireactions_pool_runners_desired{pool="default"}, 1)docs/cli/index.md (1)
36-40: Avoid inline passwords in CLI examples.
Prefer env vars (or prompting) to reduce accidental credential exposure.🔧 Suggested doc tweak
-If the Fireactions server is configured with basic authentication, you must include the username and password using the `--username` and `--password` flags. +If the Fireactions server is configured with basic authentication, provide credentials via flags or environment variables. Avoid placing passwords directly on the command line to reduce exposure in shell history and process listings. ```bash -fireactions --username admin --password secret pools list +FIREACTIONS_USERNAME=admin FIREACTIONS_PASSWORD=secret fireactions pools listAnd in the example: ```diff -fireactions -e https://fireactions.example.com -u admin -p secret pools list +FIREACTIONS_USERNAME=admin FIREACTIONS_PASSWORD=secret fireactions -e https://fireactions.example.com pools listAlso applies to: 168-172
docs/api/index.md (1)
341-343: Remove200 OKfrom the Error Responses list.
This section should only list error status codes.✏️ Suggested tweak
Common HTTP status codes: -- `200 OK` - Request succeeded - `400 Bad Request` - Invalid request parameters - `401 Unauthorized` - Authentication required or failed - `404 Not Found` - Resource not found (pool or VM doesn't exist) - `500 Internal Server Error` - Server errorserver/image_manager.go (2)
52-72: Normalize pullPolicy case to avoid invalid policy errors.
If config validation uses lowercase values, this switch will always hit default.#!/bin/bash # Verify the allowed/validated values for image pull policy. rg -n "image_pull_policy|ImagePullPolicy|pull policy" --type go -C 3
133-163: Avoid holdingcontainerdMuacrossPull().
Pull()can be long‑running and will block all other containerd ops while the mutex is held.🔧 Suggested refactor
- im.containerdMu.Lock() - image, err := im.containerd.Pull(ctx, ref, - containerd.WithPullUnpack, - containerd.WithResolver(resolver), - containerd.WithPullSnapshotter(defaultSnapshotter)) - im.containerdMu.Unlock() + image, err := im.containerd.Pull(ctx, ref, + containerd.WithPullUnpack, + containerd.WithResolver(resolver), + containerd.WithPullSnapshotter(defaultSnapshotter))Is the containerd Go client safe for concurrent Pull/GetImage calls, or does it require external serialization?server/server.go (1)
355-370: Potential index out of bounds onNetworkInterfaces[0]in GetMicroVM.Line 363 accesses
NetworkInterfaces[0]without checking if the slice is empty or if nested pointers are nil, which could cause a panic.🐛 Proposed defensive check
for _, pool := range pools { pool.machinesMu.Lock() for _, metadata := range pool.machines { if metadata.machine.Cfg.VMID != vmid { continue } - ip := metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration.IPAddr.IP.String() + var ip string + if len(metadata.machine.Cfg.NetworkInterfaces) > 0 && + metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration != nil && + metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration != nil { + ip = metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration.IPAddr.IP.String() + } vm := &MicroVM{VMID: vmid, Pool: pool.config.Name, IPAddr: ip, CreatedAt: metadata.createdAt}server/convert.go (1)
7-18: Nil dereference risk inconvertPoolwhen Runner is nil.Accessing
p.config.Runner.Organization,p.config.Runner.GroupID, etc. will panic ifp.config.Runneris nil. The past review flagged this issue, and CI showed a panic. Defensive checks should be added.🐛 Proposed fix with defensive defaults
func convertPool(p *Pool) *fireactions.Pool { + if p == nil || p.config == nil { + return &fireactions.Pool{} + } + replicas := p.GetReplicas() + var ( + org string + group int64 + labels []string + image string + ) + if p.config.Runner != nil { + org = p.config.Runner.Organization + group = p.config.Runner.GroupID + labels = p.config.Runner.Labels + image = p.config.Runner.Image + } pool := &fireactions.Pool{ Name: p.config.Name, Replicas: replicas, CurrentReplicas: p.GetCurrentSize(), DesiredReplicas: replicas, - Organization: p.config.Runner.Organization, - GroupID: p.config.Runner.GroupID, - Labels: p.config.Runner.Labels, - Image: p.config.Runner.Image, + Organization: org, + GroupID: group, + Labels: labels, + Image: image, }server/microvm.go (1)
22-32: Incomplete nil-pointer guard on nested network configuration.The check at line 24 handles an empty
NetworkInterfacesslice, butStaticConfigurationandIPConfigurationcould still be nil, causing a panic on line 25.🐛 Proposed defensive check
for _, metadata := range p.machines { ipAddr := "" - if len(metadata.machine.Cfg.NetworkInterfaces) > 0 { + if len(metadata.machine.Cfg.NetworkInterfaces) > 0 && + metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration != nil && + metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration != nil { ipAddr = metadata.machine.Cfg.NetworkInterfaces[0].StaticConfiguration.IPConfiguration.IPAddr.IP.String() }server/pool.go (1)
512-538: Potential race condition between scaleDown and cleanup goroutine.
scaleDownfinds a target machine, unlocksmachinesMu, then callsStopVMM(). However, the cleanup goroutine spawned duringscaleUpalso accessesp.machines[runnerName]and callsdelete. If both execute concurrently, the cleanup goroutine may have already removed the machine from the map before scaleDown's StopVMM completes, or StopVMM may fail on an already-stopped machine.Consider removing the target from the map while holding the lock to avoid the cleanup goroutine racing:
🔧 Suggested fix
func (p *Pool) scaleDown(ctx context.Context) error { p.machinesMu.Lock() // Find a machine to remove (pick the first one) var targetMetadata *machineMetadata var targetName string for name, metadata := range p.machines { targetMetadata = metadata targetName = name break } if targetMetadata == nil { p.machinesMu.Unlock() return fmt.Errorf("no machines available to scale down") } + // Remove from map while holding lock to prevent cleanup goroutine race + delete(p.machines, targetName) p.machinesMu.Unlock() err := targetMetadata.machine.StopVMM()
🧹 Nitpick comments (2)
Makefile (1)
6-10: Consider including timezone in BUILD_DATE for reproducibility.The current format
'+%FT%T'produces a timestamp without timezone information (e.g.,2026-01-23T10:30:45). For distributed teams or debugging across time zones, including the timezone would improve clarity.♻️ Suggested improvement
-BUILD_DATE := $(shell date '+%FT%T') +BUILD_DATE := $(shell date -u '+%FT%TZ')Using
-ufor UTC withZsuffix provides an unambiguous timestamp.commands/pools.go (1)
79-80: Default--replicas 0may cause unintended scale-to-zero.The flag defaults to
0, so if a user forgets to specify--replicas, the pool will scale to zero replicas despite the flag being marked as required. Cobra'sMarkFlagRequiredonly validates presence, not non-zero values.Consider adding validation to reject negative values or warn when scaling to zero:
♻️ Proposed validation
func runPoolsScaleCmd(cmd *cobra.Command, args []string) error { replicas, _ := cmd.Flags().GetInt("replicas") + if replicas < 0 { + return fmt.Errorf("replicas must be non-negative, got %d", replicas) + } _, err := client.ScalePool(cmd.Context(), args[0], replicas)
dc8d081 to
e5cae86
Compare
1 participant
This PR consolidates multiple improvements that we've gathered throughout our (Hostinger) usage of Fireactions:
min_runnersandmax_runnerstoreplicasSummary by CodeRabbit
New Features
Improvements
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.