d83c1dd982
* feat(diff): use stored base_branch for diff stats + per-task compare picker
Task-card additions/deletions and the Changes panel were computed against a
hardcoded origin/main → master priority list inside agentctl. For tasks
branched off a release/staging branch this inflated the counts by every
commit since the actual fork point, and the Changes panel grouped unrelated
files alongside the task's real work.
This wires the per-repo task_repositories.base_branch end-to-end into the
agentctl WorkspaceTracker so each repo's diff stats resolve against the
recorded base instead of the integration-branch fallback. A picker on the
Changes panel branch hover card lets the user change the base mid-task; the
new value is persisted via PATCH /tasks/:id/repositories/:repo_id, pushed
live to the running agentctl instance, and the session's cached
base_commit_sha is cleared so the commits panel + cumulative diff refetch
against the new base without a session restart.
Falls back to the existing origin/main → master list for legacy tasks
(empty base_branch) and to the branch tip when no merge-base exists with
HEAD (unrelated histories — local rebase backups, etc.) so the stats and
commits panels stay anchored on the same SHA in every failure mode.
* fix(agentctl): sanitize user-controlled base_branch refs before git exec
CodeQL flagged the picker's user-controlled base_branch payload reaching
exec.Command("git", ..., baseBranch) inside workspace_git_cmd.go as a
command-built-from-user-controlled-source vulnerability. A malicious caller
could smuggle a leading-"-" argument like "-upload-pack=..." that git
itself reinterprets as a flag, even though exec.Command keeps args
separate from the shell.
Adds an IsSafeGitRef sanitizer at every boundary the value crosses on its
way to git: the service write path, the agentctl HTTP handlers
(/workspace/base-branches, /git/log, /git/cumulative-diff), and
WorkspaceTracker.SetBaseBranch itself. Unsafe values are rejected at the
service tier and silently downgraded to the no-override fallback inside
agentctl so a misbehaving caller can't half-break the tracker.
* fix(diff): address PR review blockers and CI lint
- Split UpdateRepositoryBaseBranch into validate/load/apply helpers to drop
cyclomatic complexity below the cyclop limit that gated Run Backend Tests.
- Replace direct m.cfg.BaseBranches reads/writes with getBaseBranches /
setBaseBranches under a dedicated RWMutex. UpdateBaseBranches used to
write under repoTrackersMu while rescan + lazy-subpath callers read under
workspaceTrackersMu, leaving an unsynchronized data race on the map.
- Drop the false 'PR target is auto-wired' claim from the MCP tool
description — GitOperator.CreatePR takes baseBranch as an explicit arg
and never reads the tracker's value.
- Picker no-op guard now compares against currentBase instead of storedBase
so selecting the displayed fallback (legacy task before any picker
change) is treated as no-op instead of triggering a spurious save.
- HTTP handler now maps non-validation service errors to 500 instead of
400, classifying via the existing isValidationError helper plus the new
unsafe-git-ref message.
* fix(diff): regex-based ref sanitiser at every git subprocess sink
CodeQL kept flagging workspace_git_cmd.go:43 even after SetBaseBranch
rejected unsafe values — the taint tracker doesn't follow sanitised stores
through struct fields, so it still saw user input flowing into
exec.Command("git", ..., baseBranch).
Switches IsSafeGitRef to a single regex check (a pattern CodeQL recognises
as a sanitiser barrier) and adds SanitizeGitRef calls inline at every
agentctl call site that splices a user-controlled ref into a git arg:
- workspace_git_status.resolveStoredRef (before the rev-parse probes)
- workspace_git_status.computeBaseCommit (before merge-base/rev-parse)
- workspace_git_status.getAheadBehindCounts (before rev-list)
- agentctl/server/api/git.go runGitLogForRepo + handleGitCumulativeDiff
(before computeMergeBase/GetRevParse on the HTTP TargetBranch)
Behaviour is unchanged for valid refs — only narrows the allowlist (now
[A-Za-z0-9_./-]) so the sanitiser barrier is recognisable to static
analysis. SetBaseBranch's field-store sanitiser stays as defence in depth.
* fix(diff): transform sanitiser at every map read for CodeQL taint flow
CodeQL still flagged workspace_git_cmd.go:43 because:
- handleSetBaseBranches used an IsSafeGitRef bool guard before a verbatim
map[k] = v copy. CodeQL recognises regex-transforming sanitisers but not
bool-guarded passthroughs.
- lookupBaseBranch returned m.cfg.BaseBranches[k] verbatim. Even though
the map was sanitised on write, taint flow across map stores + field
stores loses the sanitised state in the analyser.
Switches both sites to the transforming SanitizeGitRef helper so every
read produces a freshly-sanitised value before it flows to a downstream
git argument. Behaviour is unchanged for valid refs.
* fix(diff): regex transformer sanitiser + guard live push from empty map
CodeQL still flagged the previous SanitizeGitRef because it returned the
input string unchanged on success — an identity passthrough that the taint
tracker doesn't recognise as a sanitiser barrier.
Switches to safeGitRefPattern.FindString so the output comes from the
regex match (not the parameter). IsSafeGitRef now wraps SanitizeGitRef so
the predicate and the transformer share one allowlist.
Also addresses two Claude review blockers:
- collectTaskBaseBranches returns a nil/empty map when the task has no
recorded base_branches; pushing that to agentctl wipes every tracker's
override. Now skipped — the DB row is the source of truth for the next
session launch.
- applyBaseBranchSideEffects now runs on context.WithoutCancel so an HTTP
/ WS request cancel after the task_repositories UPDATE commits can't
leave session.base_commit_sha stale + the live agentctl tracker map
un-pushed, drifting from the persisted row.
* fix(diff): share securityutil.IsValidBranchName + harden error classification
CodeQL still flagged the picker's regex sanitiser as taint passthrough.
Refactors SanitizeGitRef / isSafeBaseBranchRef to delegate to the existing
securityutil.IsValidBranchName helper that agentctl's Rebase / Merge /
RenameBranch already use without being flagged. The shared allowlist also
fixes a Claude review blocker: 'feature/@2024' used to pass the service
validator but get dropped by the agentctl-side check at push time,
silently wiping every tracker's override.
Plus the two error-classification blockers Claude raised on the previous
HEAD:
- WS handler (task_ws_handlers.go wsUpdateTaskRepository) now distinguishes
validation errors (surfaced verbatim) from internal faults (opaque
500-style message). HTTP path already did this via the existing
isValidationError helper.
- MCP handler (handlers.handleUpdateRepositoryBaseBranch) gets the same
treatment via a new isValidationError local to the mcp/handlers
package so raw service error strings stop leaking across the MCP
boundary.
* fix(diff): inline securityutil.IsValidBranchName at every git subprocess sink
CodeQL's interprocedural taint tracking wasn't recognising SanitizeGitRef
as a sanitiser even after it delegated to securityutil.IsValidBranchName
— the wrapper helper + map/field hops between source and sink kept the
flow tainted in the analyser.
Inlines the IsValidBranchName allowlist check (already used inline by
Rebase/Merge in git.go without being flagged) at every call site that
splices a user-controlled ref into a git subprocess argument:
- workspace_git_status.resolveStoredRef (before rev-parse --verify)
- workspace_git_status.computeBaseCommit (before merge-base / rev-parse)
- workspace_git_status.getAheadBehindCounts (before rev-list)
- agentctl/server/api/git.go runGitLogForRepo + handleGitCumulativeDiff
- agentctl/server/api/workspace_base_branches.go handleSetBaseBranches
Behaviour unchanged: same allowlist as Rebase / Merge / RenameBranch
already use. SanitizeGitRef / IsSafeGitRef survive as defence-in-depth
helpers for callers that want a one-line check; they delegate to the same
securityutil helper so the rules stay aligned.
* fix(diff): inline regexp.MatchString at every git subprocess sink
CodeQL still flagged workspace_git_cmd.go:43 even with the inline
securityutil.IsValidBranchName helper — the taint tracker apparently does
not follow into helper functions for the go/command-injection rule on
this path.
Hoists the regex to package-scope vars (safeBranchRefPattern in both
process and api packages) and calls regexp.MatchString directly at each
sink site. Behaviour is identical to securityutil.IsValidBranchName; only
the call form changes.
* fix(diff): SHA-shape allowlist on BaseCommit before downstream git diff
CodeQL still flagged workspace_git_cmd.go:43 because update.BaseCommit
is the OUTPUT of an earlier merge-base / rev-parse call whose argument
list contained a user-controlled branch ref — the analyser treats that
command's output as still tainted even when the input ref was sanitised.
Adds an inline 40-hex SHA-shape allowlist check in enrichWithBranchDiff
right before `git diff --numstat <update.BaseCommit>` so the sanitiser
barrier sits in the same function as the next subprocess call. Behaviour
is unchanged: merge-base / rev-parse already produce SHA-shaped output.
* fix(diff): async tracker refresh + tighten init spread order
Two CodeRabbit/Claude review fixes:
- UpdateBaseBranches stamps each tracker's new baseBranch synchronously
then runs RefreshGitStatus on a background goroutine instead of inside
the picker-save HTTP handler. Per-tracker git subprocesses can each
take ~3-5 calls; serialising them on the request goroutine made the
PATCH response wait for every refresh to finish. The picker UI re-fetches
via the existing WS stream once Refresh emits, so the handler doesn't
need to block.
- updateTaskRepositoryBaseBranch now spreads options.init BEFORE the
required method/body fields so an upstream caller's init overrides
can't replace the PATCH method or the base_branch payload. Matches
the convention deleteTask / moveTask already follow.
* fix(diff): narrow mcp isValidationError + opaque 500 + post-sanitize log count
Three final Claude review threads on the latest commit:
- mcp/handlers isValidationError drops the broad 'invalid' substring so
DB / IO errors whose messages happen to contain 'invalid' don't
misclassify as ErrorCodeValidation.
- task_http_handlers httpUpdateTaskRepository now logs the raw service
error server-side and returns an opaque 'failed to update task
repository' on the 500 path. Stops potential DB / IO detail leakage
through the HTTP boundary.
- workspace_base_branches debug log reports accepted + rejected counts
(post-sanitisation) instead of the pre-filter total so the metric
actually reflects what hit UpdateBaseBranches.
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
414 lines
14 KiB
Go
414 lines
14 KiB
Go
// Package instance provides utilities for managing multi-agent instances.
|
||
package instance
|
||
|
||
import (
|
||
"context"
|
||
"errors"
|
||
"fmt"
|
||
"net"
|
||
"net/http"
|
||
"os/exec"
|
||
"strings"
|
||
"sync"
|
||
"syscall"
|
||
"time"
|
||
|
||
"github.com/google/uuid"
|
||
"github.com/kandev/kandev/internal/agentctl/server/config"
|
||
"github.com/kandev/kandev/internal/agentctl/server/process"
|
||
"github.com/kandev/kandev/internal/common/logger"
|
||
"github.com/kandev/kandev/pkg/agent"
|
||
"go.uber.org/zap"
|
||
)
|
||
|
||
// ServerFactory creates an HTTP handler for an instance given its config and process manager.
|
||
type ServerFactory func(cfg *config.InstanceConfig, procMgr *process.Manager, log *logger.Logger) http.Handler
|
||
|
||
// Manager manages multiple agent instances.
|
||
// It handles creation, tracking, and removal of agent instances,
|
||
// each with their own HTTP server on a dedicated port.
|
||
type Manager struct {
|
||
config *config.Config
|
||
logger *logger.Logger
|
||
instances map[string]*Instance
|
||
portAlloc *PortAllocator
|
||
serverFactory ServerFactory
|
||
mu sync.RWMutex
|
||
|
||
// reaperStop is closed by Shutdown to signal the idle reaper to exit.
|
||
// reaperStopOnce serializes the close so concurrent Shutdown calls can't
|
||
// double-close; sync.Once is used instead of m.mu because m.mu guards
|
||
// the instances map and shouldn't gate a one-shot lifecycle signal.
|
||
// reaperWG waits for the reaper goroutine to finish before Shutdown returns.
|
||
reaperStop chan struct{}
|
||
reaperStopOnce sync.Once
|
||
reaperWG sync.WaitGroup
|
||
}
|
||
|
||
// NewManager creates a new instance manager.
|
||
// If cfg.IdleTimeout > 0, a background goroutine periodically reaps
|
||
// instances that have been idle (no in-flight HTTP requests and no
|
||
// activity) for the configured duration.
|
||
func NewManager(cfg *config.Config, log *logger.Logger) *Manager {
|
||
// Clean up code-server processes orphaned by a previous session (safety net).
|
||
process.CleanupOrphanedCodeServers(log)
|
||
|
||
m := &Manager{
|
||
config: cfg,
|
||
logger: log.WithFields(zap.String("component", "instance-manager")),
|
||
instances: make(map[string]*Instance),
|
||
portAlloc: NewPortAllocator(cfg.Ports.Base, cfg.Ports.Max),
|
||
reaperStop: make(chan struct{}),
|
||
}
|
||
|
||
if cfg.IdleTimeout > 0 {
|
||
m.reaperWG.Add(1)
|
||
go m.runIdleReaper(cfg.IdleTimeout, cfg.IdleReaperInterval)
|
||
}
|
||
|
||
return m
|
||
}
|
||
|
||
// SetServerFactory sets the factory function for creating HTTP handlers for instances.
|
||
// This must be called before creating any instances.
|
||
func (m *Manager) SetServerFactory(factory ServerFactory) {
|
||
m.serverFactory = factory
|
||
}
|
||
|
||
// CreateInstance creates a new agent instance.
|
||
func (m *Manager) CreateInstance(ctx context.Context, req *CreateRequest) (*CreateResponse, error) {
|
||
m.mu.Lock()
|
||
defer m.mu.Unlock()
|
||
|
||
id := req.ID
|
||
if id == "" {
|
||
id = uuid.New().String()
|
||
}
|
||
if _, exists := m.instances[id]; exists {
|
||
return nil, fmt.Errorf("instance with ID %s already exists", id)
|
||
}
|
||
|
||
port, listener, err := m.allocatePortAndListener(id)
|
||
if err != nil {
|
||
return nil, err
|
||
}
|
||
|
||
agentCmd := m.resolveAgentCommand(req)
|
||
autoStart := req.AutoStart
|
||
mcpServers := m.buildMcpServerConfigs(req.McpServers)
|
||
|
||
m.logger.Info("CreateInstance: received request",
|
||
zap.String("req_protocol", req.Protocol),
|
||
zap.String("workspace_path", req.WorkspacePath))
|
||
|
||
overrides := &config.InstanceOverrides{
|
||
InstanceID: id,
|
||
Protocol: agent.Protocol(req.Protocol),
|
||
AgentCommand: agentCmd,
|
||
WorkDir: req.WorkspacePath,
|
||
AutoStart: &autoStart,
|
||
Env: config.CollectAgentEnv(req.Env),
|
||
AgentType: req.AgentType,
|
||
McpServers: mcpServers,
|
||
SessionID: req.SessionID,
|
||
TaskID: req.TaskID,
|
||
DisableAskQuestion: req.DisableAskQuestion,
|
||
AssumeMcpSse: req.AssumeMcpSse,
|
||
AssumeMcpHttp: req.AssumeMcpHttp,
|
||
McpMode: req.McpMode,
|
||
RequiresProcessKill: req.RequiresProcessKill,
|
||
BaseBranches: req.BaseBranches,
|
||
}
|
||
|
||
m.logger.Info("CreateInstance: applying overrides",
|
||
zap.String("override_protocol", string(overrides.Protocol)))
|
||
|
||
// Create instance config using the unified method
|
||
instanceCfg := m.config.NewInstanceConfig(port, overrides)
|
||
|
||
m.logger.Info("CreateInstance: instance config created",
|
||
zap.String("config_protocol", string(instanceCfg.Protocol)))
|
||
|
||
// Create process manager
|
||
procMgr := process.NewManager(instanceCfg, m.logger)
|
||
|
||
// Start root + per-repo trackers so file-change events fire even in passthrough mode.
|
||
procMgr.StartAllWorkspaceTrackers(context.Background())
|
||
|
||
// Create instance up-front so the activity middleware can reference it.
|
||
inst := &Instance{
|
||
ID: id,
|
||
Port: port,
|
||
Status: "running",
|
||
WorkspacePath: req.WorkspacePath,
|
||
AgentCommand: agentCmd,
|
||
Env: req.Env,
|
||
CreatedAt: time.Now(),
|
||
manager: procMgr,
|
||
}
|
||
inst.MarkActivity()
|
||
|
||
handler := activityMiddleware(inst)(m.buildHTTPHandler(instanceCfg, procMgr))
|
||
httpServer := m.startHTTPServer(port, listener, handler, id)
|
||
inst.server = httpServer
|
||
m.instances[id] = inst
|
||
|
||
m.logger.Info("created instance",
|
||
zap.String("instance_id", id),
|
||
zap.Int("port", port),
|
||
zap.String("workspace", req.WorkspacePath))
|
||
|
||
return &CreateResponse{
|
||
ID: id,
|
||
Port: port,
|
||
}, nil
|
||
}
|
||
|
||
// allocatePortAndListener allocates a free port and binds a TCP listener to it.
|
||
func (m *Manager) allocatePortAndListener(id string) (int, net.Listener, error) {
|
||
maxAttempts := m.config.Ports.Max - m.config.Ports.Base + 1
|
||
for attempt := 0; attempt < maxAttempts; attempt++ {
|
||
allocated, err := m.portAlloc.Allocate(id)
|
||
if err != nil {
|
||
return 0, nil, fmt.Errorf("failed to allocate port: %w", err)
|
||
}
|
||
ln, err := net.Listen("tcp", fmt.Sprintf(":%d", allocated))
|
||
if err != nil {
|
||
if errors.Is(err, syscall.EADDRINUSE) || strings.Contains(err.Error(), "address already in use") {
|
||
m.portAlloc.MarkUnavailable(allocated)
|
||
m.logger.Warn("port already in use; retrying",
|
||
zap.String("instance_id", id),
|
||
zap.Int("port", allocated))
|
||
continue
|
||
}
|
||
m.portAlloc.Release(allocated)
|
||
return 0, nil, fmt.Errorf("failed to bind instance port %d: %w", allocated, err)
|
||
}
|
||
return allocated, ln, nil
|
||
}
|
||
return 0, nil, fmt.Errorf("failed to allocate an available port for instance %s", id)
|
||
}
|
||
|
||
// resolveAgentCommand returns the effective agent command for a create request.
|
||
func (m *Manager) resolveAgentCommand(req *CreateRequest) string {
|
||
agentCmd := req.AgentCommand
|
||
if agentCmd == "" {
|
||
agentCmd = m.config.Defaults.AgentCommand
|
||
}
|
||
if req.WorkspacePath != "" && req.WorkspaceFlag != "" && !strings.Contains(agentCmd, req.WorkspaceFlag) {
|
||
agentCmd = agentCmd + " " + req.WorkspaceFlag + " " + req.WorkspacePath
|
||
}
|
||
return agentCmd
|
||
}
|
||
|
||
// buildMcpServerConfigs converts instance McpServerConfig entries to
|
||
// config.McpServerConfig. Stdio entries whose Command can't be resolved
|
||
// (binary missing from PATH, no longer installed, etc.) are dropped with
|
||
// a warning so the agent doesn't spawn a permanently-broken child for an
|
||
// MCP it can never invoke. See GH issue #1247 — the `/snap/bin/brave`
|
||
// stale-MCP repro.
|
||
func (m *Manager) buildMcpServerConfigs(mcpServers []McpServerConfig) []config.McpServerConfig {
|
||
result := make([]config.McpServerConfig, 0, len(mcpServers))
|
||
for _, mcp := range mcpServers {
|
||
if reason := mcpStdioValidationError(mcp); reason != "" {
|
||
m.logger.Warn("dropping MCP server: stdio command unavailable",
|
||
zap.String("mcp_name", mcp.Name),
|
||
zap.String("command", mcp.Command),
|
||
zap.String("reason", reason))
|
||
continue
|
||
}
|
||
result = append(result, config.McpServerConfig{
|
||
Name: mcp.Name,
|
||
URL: mcp.URL,
|
||
Type: mcp.Type,
|
||
Command: mcp.Command,
|
||
Args: mcp.Args,
|
||
Env: mcp.Env,
|
||
Headers: mcp.Headers,
|
||
})
|
||
}
|
||
return result
|
||
}
|
||
|
||
// mcpStdioValidationError returns an empty string when the MCP entry is
|
||
// either non-stdio (URL transport) or its stdio Command resolves on PATH.
|
||
// Otherwise it returns a human-readable reason the entry should be dropped.
|
||
//
|
||
// Caveat: PATH is resolved against the agentctl process's environment.
|
||
// In Docker and SSH executor modes the agent may launch in a different
|
||
// environment than agentctl, so a binary that lives only inside the
|
||
// container/remote host but not on the agentctl host will be dropped
|
||
// here even though it would have worked at agent runtime. For Standalone
|
||
// and Sprites this is unambiguously correct; for Docker/SSH it's an
|
||
// acceptable false positive — surfacing the warn log is better than
|
||
// spawning a permanently broken child every session (the `/snap/bin/brave`
|
||
// repro in GH issue #1247).
|
||
func mcpStdioValidationError(mcp McpServerConfig) string {
|
||
// Non-stdio transports (sse, http, streamable_http) carry their endpoint
|
||
// in URL — nothing to validate locally.
|
||
if mcp.URL != "" {
|
||
return ""
|
||
}
|
||
if mcp.Command == "" {
|
||
return "stdio MCP entry has neither URL nor Command"
|
||
}
|
||
// Tolerate a compound `Command` string like "python3 -m mcp_server"
|
||
// where the user collapsed Command+Args into Command. The first token
|
||
// is the actual binary to look up; everything else is argv that the
|
||
// agent will splice in later. This is more permissive than the
|
||
// schema strictly allows, but it matches what real configs look like.
|
||
bin := mcp.Command
|
||
if i := strings.IndexAny(bin, " \t"); i > 0 {
|
||
bin = bin[:i]
|
||
}
|
||
if _, err := exec.LookPath(bin); err != nil {
|
||
return err.Error()
|
||
}
|
||
return ""
|
||
}
|
||
|
||
// buildHTTPHandler creates the HTTP handler for an instance.
|
||
func (m *Manager) buildHTTPHandler(instanceCfg *config.InstanceConfig, procMgr *process.Manager) http.Handler {
|
||
if m.serverFactory != nil {
|
||
return m.serverFactory(instanceCfg, procMgr, m.logger)
|
||
}
|
||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||
w.WriteHeader(http.StatusServiceUnavailable)
|
||
if _, err := w.Write([]byte("server factory not configured")); err != nil {
|
||
m.logger.Debug("failed to write default response", zap.Error(err))
|
||
}
|
||
})
|
||
}
|
||
|
||
// startHTTPServer creates and starts an HTTP server on the given listener.
|
||
func (m *Manager) startHTTPServer(port int, listener net.Listener, handler http.Handler, id string) *http.Server {
|
||
httpServer := &http.Server{
|
||
Addr: fmt.Sprintf(":%d", port),
|
||
Handler: handler,
|
||
}
|
||
go func() {
|
||
if err := httpServer.Serve(listener); err != nil && err != http.ErrServerClosed {
|
||
m.logger.Error("instance server error",
|
||
zap.String("instance_id", id),
|
||
zap.Error(err))
|
||
}
|
||
}()
|
||
return httpServer
|
||
}
|
||
|
||
// GetInstance returns an instance by ID.
|
||
func (m *Manager) GetInstance(id string) (*Instance, bool) {
|
||
m.mu.RLock()
|
||
defer m.mu.RUnlock()
|
||
inst, ok := m.instances[id]
|
||
return inst, ok
|
||
}
|
||
|
||
// ListInstances returns info for all instances.
|
||
func (m *Manager) ListInstances() []*InstanceInfo {
|
||
m.mu.RLock()
|
||
defer m.mu.RUnlock()
|
||
|
||
result := make([]*InstanceInfo, 0, len(m.instances))
|
||
for _, inst := range m.instances {
|
||
result = append(result, inst.Info())
|
||
}
|
||
return result
|
||
}
|
||
|
||
// StopInstance stops and removes an instance by ID.
|
||
func (m *Manager) StopInstance(ctx context.Context, id string) error {
|
||
// Get instance and remove from map under lock (quick operation)
|
||
m.mu.Lock()
|
||
inst, ok := m.instances[id]
|
||
if !ok {
|
||
m.mu.Unlock()
|
||
return fmt.Errorf("instance %s not found", id)
|
||
}
|
||
// Remove from map immediately so new instances aren't blocked
|
||
delete(m.instances, id)
|
||
m.mu.Unlock()
|
||
|
||
m.logger.Debug("stopping instance", zap.String("instance_id", id))
|
||
|
||
// Stop the process manager (potentially slow, done without lock)
|
||
if inst.manager != nil {
|
||
if err := inst.manager.Stop(ctx); err != nil {
|
||
m.logger.Warn("error stopping process manager",
|
||
zap.String("instance_id", id),
|
||
zap.Error(err))
|
||
}
|
||
}
|
||
|
||
m.logger.Debug("StopInstance: shutting down HTTP server",
|
||
zap.String("instance_id", id),
|
||
zap.Int("port", inst.Port))
|
||
|
||
// Shutdown HTTP server (potentially slow, done without lock)
|
||
if inst.server != nil {
|
||
if err := inst.server.Shutdown(ctx); err != nil {
|
||
m.logger.Warn("error shutting down HTTP server",
|
||
zap.String("instance_id", id),
|
||
zap.Error(err))
|
||
}
|
||
}
|
||
|
||
m.logger.Debug("StopInstance: releasing port",
|
||
zap.String("instance_id", id),
|
||
zap.Int("port", inst.Port))
|
||
|
||
// Release port (quick operation, re-acquire lock)
|
||
m.mu.Lock()
|
||
m.portAlloc.Release(inst.Port)
|
||
m.mu.Unlock()
|
||
|
||
m.logger.Info("StopInstance completed",
|
||
zap.String("instance_id", id),
|
||
zap.Int("port", inst.Port))
|
||
|
||
return nil
|
||
}
|
||
|
||
// Shutdown stops all instances gracefully.
|
||
func (m *Manager) Shutdown(ctx context.Context) error {
|
||
// Stop the idle reaper first so it doesn't fire StopInstance concurrently
|
||
// with our explicit per-instance shutdown loop.
|
||
m.stopReaperOnce()
|
||
|
||
m.mu.Lock()
|
||
ids := make([]string, 0, len(m.instances))
|
||
for id := range m.instances {
|
||
ids = append(ids, id)
|
||
}
|
||
m.mu.Unlock()
|
||
|
||
var lastErr error
|
||
for _, id := range ids {
|
||
if err := m.StopInstance(ctx, id); err != nil {
|
||
m.logger.Error("error stopping instance during shutdown",
|
||
zap.String("instance_id", id),
|
||
zap.Error(err))
|
||
lastErr = err
|
||
}
|
||
}
|
||
|
||
return lastErr
|
||
}
|
||
|
||
// stopReaperOnce closes the reaper stop channel exactly once and waits for
|
||
// the reaper goroutine to drain. Safe to call multiple times.
|
||
//
|
||
// Note: reaperWG.Wait() blocks until the reaper finishes whatever sweep
|
||
// is currently in flight. Each instance in that sweep gets its own bounded
|
||
// context (idleReaperShutdownTimeout) independent of any caller deadline,
|
||
// so a Shutdown(ctx) caller with a tight deadline can find that the
|
||
// reaper drain consumed it before the main shutdown loop starts. The
|
||
// reaper polls reaperStop between instances, so the worst-case drain is
|
||
// one StopInstance round (15s), not N×timeout.
|
||
func (m *Manager) stopReaperOnce() {
|
||
m.reaperStopOnce.Do(func() {
|
||
close(m.reaperStop)
|
||
})
|
||
m.reaperWG.Wait()
|
||
}
|