amadan

carlos / platform Public

instancectl: seed cleanup runs detached from the request after start

Fix round 1 on the plan 2 final-review commit (7576db8e): item 4 was
PARTIAL. The `show` call right after a successful `start`, and the
cleanup `stop` after that, still ran on the request context. Production's
Systemctl is exec.CommandContext, so a client that disconnected the
instant `start` answered cancelled that context, `show` failed with
"context canceled", the handler 502'd there, and the cleanup `stop` was
never reached, the stale-invocation guard only caught it on the NEXT
/seed.

The request context is now used only up to and including the first
`start`. From the moment it returns, one context detached from the
request (context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second))
carries every later systemctl call in handleSeed: the post-start
InvocationID read, the stale-invocation stop/retry-start/show, the final
show, the cleanup stop, and WaitCgroupEmpty. The retry-failure branch now
stops the unit on that context before returning its 502.

fakeCtl.run in server_test.go now honours ctx.Err(), matching what
exec.CommandContext actually does, and gained an onStart hook so
TestSeedStopRunsDespiteACancelledClientContext can cancel the request
context the instant start answers and assert the response still
succeeds with stop issued. Confirmed empirically that the test catches
the round-1 bug: temporarily swapping in 7576db8e's handleSeed and
running just this test failed with "status 502, body show: context
canceled"; reverted immediately after (see final-fix-report.md's "Fix
round 1" section for both runs).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YZ1ZvWj7naf73GjTX8MBv5
Paul Campbell pushed by paul@keymail.dev de37683ed0b6e7c90650725deb4e774cf2593283 parent 7576db8
2 files changed, +100 −55
  • internal/instancectl/server.go +46 −30
  • internal/instancectl/server_test.go +54 −25

CI passed — run details

diff --git a/internal/instancectl/server.go b/internal/instancectl/server.go
index 9558ed1f6..57ec1b0ce 100644
--- a/internal/instancectl/server.go
+++ b/internal/instancectl/server.go
@@ -288,19 +288,26 @@ func parseShow(out string) map[string]string {
}
// handleSeed runs the seed oneshot once and returns its retained result.
-// Every cleanup `stop` runs on a context detached from the request (never
-// ctx itself): a client that disconnects, or whose request context is
-// cancelled for any other reason, must not leave the oneshot's cgroup
-// running behind a response nobody reads (final review, plan 2, item 4).
-// Before starting, and again right after, the unit's InvocationID is
-// read: if start left the SAME invocation in place, an earlier request's
-// stop never ran (its client context was cancelled before this fix
-// existed, or the agent restarted mid-seed), so this handler stops the
-// stale run and starts again rather than reading and returning someone
-// else's result. The order after that is show (the fields RemainAfterExit
-// retains), then stop, then wait for the cgroup to empty: a populated
-// cgroup after the wait is still reported as a 502 naming the unit, the
-// same as before this reordering.
+// The unit's InvocationID is read before and after the first `start`: if
+// start left the SAME invocation in place, an earlier request's stop
+// never ran (its client disconnected before this fix existed, or the
+// agent restarted mid-seed), so this handler stops the stale run and
+// starts again rather than reading and returning someone else's result.
+//
+// The request context ctx is used only up to and including that first
+// `start`. From the moment it returns, the unit is this server's
+// responsibility to clean up, whether or not the client that asked for
+// it is still connected: `start` is the blocking call, and production's
+// Systemctl is exec.CommandContext, which kills the systemctl process
+// outright when its context is cancelled. A client that hung up while
+// `start` was running would otherwise cancel every systemctl call after
+// it too: the very next `show` would fail, the handler would 502, and
+// the cleanup `stop` after it would never run (fix round 1, item 4). So
+// every call from here to the end of the handler runs on ONE context
+// (dctx) detached from the request instead. The order is show (the
+// fields RemainAfterExit retains), then stop, then wait for the cgroup
+// to empty: a populated cgroup after the wait is still reported as a 502
+// naming the unit.
func (s *Server) handleSeed(w http.ResponseWriter, r *http.Request) {
req, err := parseRequest(r, nil, true)
if err != nil {
@@ -318,25 +325,36 @@ func (s *Server) handleSeed(w http.ResponseWriter, r *http.Request) {
// keeps the unit loaded with its result until the stop below.
invBefore := s.showValue(ctx, "InvocationID", unit)
startOut, startErr := s.Systemctl(ctx, "start", unit)
- invAfter := s.showValue(ctx, "InvocationID", unit)
+
+ // See the handler doc comment: everything past this point belongs to
+ // the server, not the client, so it runs on a context of the server's
+ // own rather than one the client's disconnect can cancel.
+ dctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second)
+ defer cancel()
+
+ invAfter := s.showValue(dctx, "InvocationID", unit)
if invBefore != "" && invBefore == invAfter {
- s.stopDetached(ctx, unit)
- startOut, startErr = s.Systemctl(ctx, "start", unit)
- if retry := s.showValue(ctx, "InvocationID", unit); retry == "" || retry == invAfter {
+ s.stopUnit(dctx, unit)
+ startOut, startErr = s.Systemctl(dctx, "start", unit)
+ if retry := s.showValue(dctx, "InvocationID", unit); retry == "" || retry == invAfter {
+ // Still this server's unit to clean up even though the request
+ // is about to fail: leaving it running would strand the NEXT
+ // /seed on this same stale-invocation guard forever.
+ s.stopUnit(dctx, unit)
http.Error(w, "seed: "+unit+" would not leave its stale invocation "+invAfter, http.StatusBadGateway)
return
}
}
- out, err := s.Systemctl(ctx, "show", "-p", "InvocationID,Result,ExecMainStatus,ExecMainCode", unit)
+ out, err := s.Systemctl(dctx, "show", "-p", "InvocationID,Result,ExecMainStatus,ExecMainCode", unit)
if err != nil {
http.Error(w, "show: "+err.Error(), http.StatusBadGateway)
return
}
m := parseShow(out)
- s.stopDetached(ctx, unit)
- wctx, cancel := context.WithTimeout(ctx, s.waitBudget())
- defer cancel()
+ s.stopUnit(dctx, unit)
+ wctx, waitCancel := context.WithTimeout(dctx, s.waitBudget())
+ defer waitCancel()
if err := WaitCgroupEmpty(wctx, s.CgroupRoot, SliceName(req.host), unit, 50*time.Millisecond); err != nil {
http.Error(w, "seed: "+err.Error(), http.StatusBadGateway)
return
@@ -359,15 +377,13 @@ func (s *Server) showValue(ctx context.Context, prop, unit string) string {
return strings.TrimSpace(out)
}
-// stopDetached issues `systemctl stop` for unit on a context that outlives
-// the request: context.WithoutCancel(ctx) keeps ctx's values (so logging
-// and tracing still see the request that triggered this) but not its
-// cancellation, bounded instead by its own 30-second timeout (final
-// review, plan 2, item 4).
-func (s *Server) stopDetached(ctx context.Context, unit string) {
- dctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second)
- defer cancel()
- if _, err := s.Systemctl(dctx, "stop", unit); err != nil {
+// stopUnit issues `systemctl stop` for unit on ctx, logging rather than
+// surfacing a failure to the caller: every call site in handleSeed is
+// already past the point where the unit is this server's responsibility,
+// not something a failed stop should turn into a different HTTP error for
+// the client.
+func (s *Server) stopUnit(ctx context.Context, unit string) {
+ if _, err := s.Systemctl(ctx, "stop", unit); err != nil {
s.logf("instance-control: stop %s: %v", unit, err)
}
}
diff --git a/internal/instancectl/server_test.go b/internal/instancectl/server_test.go
index a79af5173..2ccc339ee 100644
--- a/internal/instancectl/server_test.go
+++ b/internal/instancectl/server_test.go
@@ -21,13 +21,35 @@ type fakeCtl struct {
// answers maps a leading argument pattern ("show -p InvocationID") to output.
answers map[string]string
fail map[string]error
+ // onStart, if set, runs once, synchronously, the first time args[0] is
+ // "start", before run returns to its caller. A test uses it to
+ // simulate a client that disconnects the instant systemctl start
+ // answers (fix round 1, item 4).
+ onStart func()
}
-func (f *fakeCtl) run(_ context.Context, args ...string) (string, error) {
+func (f *fakeCtl) run(ctx context.Context, args ...string) (string, error) {
f.mu.Lock()
- defer f.mu.Unlock()
line := strings.Join(args, " ")
f.calls = append(f.calls, line)
+ var hook func()
+ if len(args) > 0 && args[0] == "start" && f.onStart != nil {
+ hook, f.onStart = f.onStart, nil
+ }
+ f.mu.Unlock()
+ if hook != nil {
+ hook()
+ }
+ // Production's Systemctl is exec.CommandContext, which fails outright
+ // once its context is done rather than completing anyway; the fake
+ // must behave the same way, or a test cannot tell a handler that used
+ // the request's context apart from one that used a detached one (fix
+ // round 1, item 4).
+ if ctx.Err() != nil {
+ return "", ctx.Err()
+ }
+ f.mu.Lock()
+ defer f.mu.Unlock()
for k, err := range f.fail {
if strings.HasPrefix(line, k) {
return "", err
@@ -201,31 +223,31 @@ func TestSeedFailsWhenTheCgroupStaysPopulated(t *testing.T) {
}
}
-// TestSeedStopRunsDespiteACancelledClientContext proves the cleanup stop
-// uses a context detached from the request: even when the client's
-// context is already cancelled by the time systemctl would run "stop",
-// the stop call still reaches systemctl with a context that is NOT
-// cancelled (final review, plan 2, item 4). A fake that ignored ctx
-// entirely could not tell the two cases apart, so this fake asserts on
-// the context it is handed.
+// TestSeedStopRunsDespiteACancelledClientContext proves the WHOLE
+// post-start sequence (the second InvocationID read, show, the cleanup
+// stop, WaitCgroupEmpty) runs on a context detached from the request, not
+// only the final stop (fix round 1, item 4). fakeCtl.run now honours
+// ctx.Err(), and its onStart hook cancels the request context the instant
+// the first "start" answers, simulating a client that hung up right then.
+// Before this fix, "show" ran on the request context: a cancelled one
+// failed it outright, the handler 502'd there, and the cleanup stop after
+// it was never reached. Confirmed empirically: temporarily swapping in
+// commit 7576db8e's handleSeed (the pre-fix-round-1 version, which used
+// ctx throughout after start) and running just this test failed with
+// "status 502, body show: context canceled" and never reached a stop
+// call, exactly the reported bug. The swap was reverted immediately
+// after, see the report for both runs.
func TestSeedStopRunsDespiteACancelledClientContext(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
- ctl := &fakeCtl{answers: map[string]string{
- "show -p InvocationID,Result,ExecMainStatus,ExecMainCode": "InvocationID=feed\nResult=success\nExecMainStatus=0\nExecMainCode=exited\n",
- }}
+ ctl := &fakeCtl{
+ answers: map[string]string{
+ "show -p InvocationID,Result,ExecMainStatus,ExecMainCode": "InvocationID=feed\nResult=success\nExecMainStatus=0\nExecMainCode=exited\n",
+ },
+ onStart: cancel,
+ }
root := t.TempDir()
- stoppedWithLiveContext := false
s := &Server{
- Systemctl: func(c context.Context, args ...string) (string, error) {
- out, err := ctl.run(c, args...)
- if len(args) > 0 && args[0] == "start" {
- cancel() // the client disconnected the instant start answered
- }
- if len(args) > 0 && args[0] == "stop" && c.Err() == nil {
- stoppedWithLiveContext = true
- }
- return out, err
- },
+ Systemctl: ctl.run,
PeerOK: func(net.Conn) error { return nil },
CgroupRoot: filepath.Join(root, "cgroup"),
Logf: t.Logf,
@@ -241,8 +263,15 @@ func TestSeedStopRunsDespiteACancelledClientContext(t *testing.T) {
r := httptest.NewRequest("POST", "/seed?host=h.example.com&memory_max=536870912", nil).WithContext(ctx)
w := httptest.NewRecorder()
s.handleSeed(w, r)
- if !stoppedWithLiveContext {
- t.Errorf("stop did not run on a context detached from the (cancelled) request:\n%s", ctl.joined())
+ if w.Code != http.StatusOK {
+ t.Fatalf("status %d, body %s, want 200 despite the cancelled client context", w.Code, w.Body.String())
+ }
+ var out SeedResponse
+ if err := json.NewDecoder(w.Body).Decode(&out); err != nil || out.InvocationID != "feed" || out.Result != "success" {
+ t.Errorf("response = %+v, %v", out, err)
+ }
+ if !strings.Contains(ctl.joined(), "stop carlos-instance-seed@h.example.com.service") {
+ t.Errorf("stop did not run despite the cancelled request context:\n%s", ctl.joined())
}
}