amadan

carlos / platform Public

instancectl: one POST /cmd per host at a time

A second concurrent start read the unit inactive, then saw the first
start's new invocation as its baseline, never saw a change, and stopped
the first request's command after the 10s wait. A per-host mutex now
spans the ActiveState read through the answer, so the second request
waits and is refused with 409.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 71135ab5de99bb8c3e1557f002c5cb38390eae84 parent c8a2639
3 files changed, +53 −0
  • internal/instancectl/cmd.go +7 −0
  • internal/instancectl/cmd_test.go +42 −0
  • internal/instancectl/server.go +4 −0

CI passed — run details

diff --git a/internal/instancectl/cmd.go b/internal/instancectl/cmd.go
index 5e3294ee2..3b5190f93 100644
--- a/internal/instancectl/cmd.go
+++ b/internal/instancectl/cmd.go
@@ -5,6 +5,7 @@ import (
"fmt"
"net/http"
"strings"
+ "sync"
"time"
"github.com/carlosframework/platform/internal/provision"
@@ -43,6 +44,12 @@ func (s *Server) handleCmd(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusBadRequest)
return
}
+ // One start per host at a time: a second request waits here, then reads
+ // the unit loaded and is refused, instead of racing the first one's
+ // invocation check and stopping its command.
+ lock, _ := s.cmdLocks.LoadOrStore(req.host, new(sync.Mutex))
+ lock.(*sync.Mutex).Lock()
+ defer lock.(*sync.Mutex).Unlock()
ctx := r.Context()
unit := UnitName(RoleCmd, req.host)
switch state := s.showValue(ctx, "ActiveState", unit); state {
diff --git a/internal/instancectl/cmd_test.go b/internal/instancectl/cmd_test.go
index 790b86c94..347c515d6 100644
--- a/internal/instancectl/cmd_test.go
+++ b/internal/instancectl/cmd_test.go
@@ -6,6 +6,7 @@ import (
"net/http"
"net/http/httptest"
"strings"
+ "sync"
"testing"
"time"
)
@@ -63,6 +64,47 @@ func TestCmdRefusesAUnitStillLoaded(t *testing.T) {
if strings.Contains(ctl.joined(), "start ") {
t.Errorf("%s: a loaded unit was started again:\n%s", state, ctl.joined())
}
+ if strings.Contains(ctl.joined(), "stop ") {
+ t.Errorf("%s: a loaded unit was stopped:\n%s", state, ctl.joined())
+ }
+ }
+}
+
+func TestCmdConcurrentStartsForOneHostAreOneStartAndOneRefusal(t *testing.T) {
+ ctl := &fakeCtl{answers: map[string]string{
+ "show -p ActiveState --value": "inactive\n",
+ "show -p InvocationID --value": "old\n",
+ }}
+ // The start loads the unit and gives it a new invocation, as systemd does.
+ ctl.onStart = func() {
+ ctl.set("show -p ActiveState --value", "active\n")
+ ctl.set("show -p InvocationID --value", "new\n")
+ }
+ _, c := newServer(t, ctl, fastCmd)
+ codes := make([]int, 2)
+ var wg sync.WaitGroup
+ for i := range codes {
+ wg.Add(1)
+ go func() {
+ defer wg.Done()
+ codes[i] = post(t, c, "/cmd?host=h.example.com&memory_max=536870912").StatusCode
+ }()
+ }
+ wg.Wait()
+ ok, conflict := 0, 0
+ for _, code := range codes {
+ switch code {
+ case 200:
+ ok++
+ case http.StatusConflict:
+ conflict++
+ }
+ }
+ if ok != 1 || conflict != 1 {
+ t.Fatalf("statuses %v, want one 200 and one 409", codes)
+ }
+ if strings.Contains(ctl.joined(), "stop ") {
+ t.Errorf("the started unit was stopped:\n%s", ctl.joined())
}
}
diff --git a/internal/instancectl/server.go b/internal/instancectl/server.go
index cd037ab6d..0df4f2be7 100644
--- a/internal/instancectl/server.go
+++ b/internal/instancectl/server.go
@@ -79,6 +79,10 @@ type Server struct {
// for the queued start to show a new invocation (10 s). Zero is the
// default; tests shorten them.
cmdPollEvery, cmdWaitBound, cmdStartBound time.Duration
+ // cmdLocks holds one *sync.Mutex per host, taken by handleCmd from its
+ // ActiveState read to its answer, so two starts for one host cannot both
+ // pass the check.
+ cmdLocks sync.Map
// Run executes one command as root and returns its combined output;
// production is exec.CommandContext, tests record. Only