amadan
console: a policy attempt's cancel runs before it unregisters (#328, #329)
Review fix round 2 for Task 8. The returned cancel unregistered the attempt and only then cancelled it, so a holding ending in between could release its lease under a live attempt. It now cancels first. A test seam pauses the cancel between its two steps while the holding ends.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+62
−10
internal/console/policylease.go+7 −1internal/console/policylease_test.go+44 −0internal/console/server.go+11 −9
diff --git a/internal/console/policylease.go b/internal/console/policylease.go| index 7b8a5eba0..bb5f1a49f 100644 |
| --- a/internal/console/policylease.go |
| +++ b/internal/console/policylease.go |
| @@ -257,11 +257,17 @@ func (s *Server) policyAttempt(ctx context.Context, h policyHolding) (context.Co |
| case <-actx.Done(): |
| } |
| }() |
| + // Cancel first, then unregister: at no instant is an attempt neither |
| + // registered with its holding nor already cancelled, so a holding that |
| + // ends meanwhile can never release its lease under a live attempt. |
| return actx, func() { |
| + cancel() |
| + if s.policyAttemptEnding != nil { |
| + s.policyAttemptEnding() |
| + } |
| s.policyStateMu.Lock() |
| delete(cur.attempts, id) |
| s.policyStateMu.Unlock() |
| - cancel() |
| }, nil |
| } |
diff --git a/internal/console/policylease_test.go b/internal/console/policylease_test.go| index 4fb319d82..2c5d1e1b4 100644 |
| --- a/internal/console/policylease_test.go |
| +++ b/internal/console/policylease_test.go |
| @@ -499,6 +499,50 @@ func TestPolicyHoldingEndCancelsItsAttemptsFirst(t *testing.T) { |
| } |
| } |
| +// Review fix round 2: a caller ending its own attempt while the holding |
| +// ends. The attempt's cancel is paused between its two steps (cancel and |
| +// unregister) by a seam; the holding ends during the pause. Whichever order |
| +// the steps run in, the lease must not be released while the attempt is |
| +// live: with unregister first, termination would find nothing to cancel. |
| +func TestPolicyAttemptCancelRacingTheHoldingEnd(t *testing.T) { |
| + t.Parallel() |
| + bg := context.Background() |
| + f := &fakePolicyLeaser{} |
| + s, _ := policyLeaseServer(f) |
| + paused, resume := make(chan struct{}), make(chan struct{}) |
| + s.policyAttemptEnding = func() { |
| + close(paused) |
| + <-resume |
| + } |
| + |
| + h, _ := s.policyWriter(bg) |
| + actx, cancel, ref := s.policyAttempt(bg, h) |
| + if ref != nil { |
| + t.Fatalf("refused: %+v", ref) |
| + } |
| + liveAtRelease := make(chan bool, 1) |
| + f.lease(0).setOnRelease(func() { liveAtRelease <- actx.Err() == nil }) |
| + |
| + ended := make(chan struct{}) |
| + go func() { |
| + cancel() |
| + close(ended) |
| + }() |
| + <-paused |
| + s.ReleasePolicyWriter(bg) |
| + close(resume) |
| + <-ended |
| + |
| + select { |
| + case live := <-liveAtRelease: |
| + if live { |
| + t.Fatal("the lease was released while the attempt was live") |
| + } |
| + default: |
| + t.Fatal("the stand-down did not release the lease") |
| + } |
| +} |
| + |
| // Review fix round 1, item 2: a stand-down while an acquisition is in flight |
| // cancels it, waits for it, and releases the lease it won anyway before |
| // returning; and nothing acquires afterwards. The leaser blocks until its |
diff --git a/internal/console/server.go b/internal/console/server.go| index 14ee6ced2..1fdec2e29 100644 |
| --- a/internal/console/server.go |
| +++ b/internal/console/server.go |
| @@ -984,15 +984,17 @@ type Server struct { |
| // ReleasePolicyWriter never wait on an acquisition: policyN is the last |
| // holding's number, policyCur the current holding (nil when none), |
| // policyAcquiring the acquisition in flight, policyStopping set for good |
| - // by ReleasePolicyWriter. policyLog is a test seam (nil: log.Printf). The |
| - // zero Server needs no setup. |
| - policyLeaseMu sync.Mutex |
| - policyStateMu sync.Mutex |
| - policyN uint64 |
| - policyCur *policyHold |
| - policyAcquiring *policyAcquisition |
| - policyStopping bool |
| - policyLog func(format string, args ...any) |
| + // by ReleasePolicyWriter. policyLog is a test seam (nil: log.Printf), and |
| + // so is policyAttemptEnding, run inside an attempt's cancel between its |
| + // two steps (nil in production). The zero Server needs no setup. |
| + policyLeaseMu sync.Mutex |
| + policyStateMu sync.Mutex |
| + policyN uint64 |
| + policyCur *policyHold |
| + policyAcquiring *policyAcquisition |
| + policyStopping bool |
| + policyLog func(format string, args ...any) |
| + policyAttemptEnding func() |
| // sendApp is how the alert watcher (alertwatch.go) delivers keymail: |
| // km.SendApp in production, a capture in tests. A field rather than a |
| // direct call so an alertPass assertion needs no keymail server. |