fix(swarm): close lifecycle admission on shutdown#776
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #771 (AUD-010) by making Swarm.Close a true shutdown barrier: it atomically closes lifecycle admission before cancelation, prevents post-shutdown lifecycle work (including retry and queue-drain paths), and waits for owned watcher goroutines to exit before returning.
Changes:
- Add swarm-level lifecycle admission gating (
closed+lifecycleMu) and watcher ownership tracking (watchers), and updateCloseto fail queued work and wait for watchers. - Make lifecycle entry points (
Spawn,Handoff,AdoptOrphans) and retry/queue-drain logic reject work after shutdown begins. - Make
Scheduler.Closeidempotent and concurrency-safe viasync.Once, and add regression tests covering shutdown admission, queue, retry, and concurrentClose.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/swarm/team.go | Adds swarm closed state + admission lock + watcher tracking; updates Close to fail queued work and wait for watchers; ensures scheduler access respects shutdown. |
| internal/swarm/scheduler.go | Makes Scheduler.Close a completion barrier via closeOnce and simplifies idempotent close behavior. |
| internal/swarm/lifecycle.go | Gates lifecycle entry points and retry/queue-drain behavior behind lifecycle admission; tracks watcher goroutines in a WaitGroup. |
| internal/swarm/lifecycle_test.go | Adds regression coverage for admission rejection, queued members not launching, retry prevention, and Close waiting for watchers/concurrent callers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (s *Swarm) beginLifecycleAdmission() error { | ||
| s.lifecycleMu.RLock() | ||
| if s.closed { | ||
| s.lifecycleMu.RUnlock() | ||
| return ErrSwarmClosed | ||
| } | ||
| return nil | ||
| } |
WalkthroughSwarm shutdown now closes lifecycle admission, cancels execution, fails queued tasks, prevents retries and queue draining, and waits for member watchers. Scheduler close operations are synchronized, and lifecycle tests cover admission rejection, queued work, retries, and concurrent shutdown. ChangesSwarm shutdown lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Swarm
participant Scheduler
participant Watchers
Caller->>Swarm: Close()
Swarm->>Swarm: Mark closed and cancel context
Swarm->>Scheduler: Close()
Swarm->>Swarm: Fail queued specs
Swarm->>Watchers: Wait for completion
Watchers-->>Swarm: Exit
Swarm-->>Caller: Return
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/swarm/lifecycle.go (1)
35-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the lock-transfer contract on
beginLifecycleAdmission. On success this returns while still holdinglifecycleMu.RLock(); on failure it releases and returnsErrSwarmClosed. Every caller silently depends on that asymmetry (defer s.lifecycleMu.RUnlock()only after a nil error). A doc comment stating "returns with the read lock held on success; released on error" prevents a future caller from adding its ownRUnlockor forgetting the deferred one.📝 Suggested doc
+// beginLifecycleAdmission gates a lifecycle entry point against shutdown. On +// success it returns nil WITH lifecycleMu held for reading; the caller must +// release it (e.g. via defer). On shutdown it releases the lock and returns +// ErrSwarmClosed. func (s *Swarm) beginLifecycleAdmission() error {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/swarm/lifecycle.go` around lines 35 - 42, Add a doc comment to Swarm.beginLifecycleAdmission documenting its lock-transfer contract: successful calls return with lifecycleMu's read lock held, while the closed/error path releases the lock before returning ErrSwarmClosed. Keep the existing locking behavior unchanged.internal/swarm/team.go (1)
177-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe close-after-create handoff is race-free — worth a one-liner so it stays that way. Because a concurrent
Close()can only takelifecycleMu.Lock()either fully before or fully after this method's read-lock section, eitherCloseobserves the freshly storeds.schedulerand closes it, or this method observesclosed == trueand closes it itself. No leak, no double-work window. A short comment on why theclosedsnapshot + post-unlocksched.Close()is safe would protect this from a future well-meaning refactor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/swarm/team.go` around lines 177 - 191, Add a concise comment in Swarm.Scheduler explaining that the closed snapshot is synchronized by lifecycleMu: Close either observes and closes the newly stored scheduler, or Scheduler sees closed and closes sched after releasing the locks. Preserve the existing locking and post-unlock sched.Close() behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/swarm/lifecycle.go`:
- Around line 35-42: Add a doc comment to Swarm.beginLifecycleAdmission
documenting its lock-transfer contract: successful calls return with
lifecycleMu's read lock held, while the closed/error path releases the lock
before returning ErrSwarmClosed. Keep the existing locking behavior unchanged.
In `@internal/swarm/team.go`:
- Around line 177-191: Add a concise comment in Swarm.Scheduler explaining that
the closed snapshot is synchronized by lifecycleMu: Close either observes and
closes the newly stored scheduler, or Scheduler sees closed and closes sched
after releasing the locks. Preserve the existing locking and post-unlock
sched.Close() behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76fca25e-776c-47e6-a683-cf49151ddb4f
📒 Files selected for processing (4)
internal/swarm/lifecycle.gointernal/swarm/lifecycle_test.gointernal/swarm/scheduler.gointernal/swarm/team.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Do not hold lifecycle admission while starting a member
internal/swarm/lifecycle.go:59
Spawn(and the retry and queue-drain paths) keepslifecycleMu.RLock()while invoking the injectedMemberLauncher.Launch.Closeneeds the write lock before it callss.cancel(), so a launcher that blocks during startup waiting for its supplied context can never be cancelled: launch holds the read lock, whileClosewaits for that lock before issuing the cancellation. This leaves both the spawn and every close caller hung. Release the admission lock before calling external launcher code, while retaining an ordering mechanism that prevents a launch from being admitted once shutdown starts; add a regression launcher whoseLaunchwaits onctx.Done(). -
[P3] Document the admission helper's lock-transfer contract
internal/swarm/lifecycle.go:35
beginLifecycleAdmissionreturns withlifecycleMuread-locked on success but unlocks it before returningErrSwarmClosed. Callers must know that asymmetric contract to avoid leaked read locks or an extra unlock, yet the helper has no comment describing it. This remains the open Copilot/CodeRabbit review request; document the ownership rule (or return an explicit unlock function). -
[P3] Explain the post-close scheduler handoff
internal/swarm/team.go:177
SchedulersnapshotsclosedunderlifecycleMu, installs/readss.scheduler, then closes the scheduler after releasing both locks. The correctness relies onCloseeither seeing and closing that stored scheduler orSchedulerseeing the closed snapshot and closing it itself. This is the unresolved CodeRabbit request; add a concise comment preserving that synchronization rationale.
Summary
ClosereturnsValidation
go test ./internal/swarm -count=1go test -race ./internal/swarm -count=1(WSL/Linux)go vet ./internal/swarmgo test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokegit diff --checkCloses #771
Summary by CodeRabbit
New Features
Bug Fixes