feat: implement lifecycle supervisor and mock backend driver#10
Merged
Conversation
- Add Supervisor state machine and lifecycle management (Start, RunCommand, Stop). - Implement MockBackend for testing the orchestration layer without a real VM. - Add comprehensive unit tests covering state transitions, concurrency safety (sync.RWMutex), and policy enforcement. Resolves: #9
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces a mock sandbox backend and a supervisor orchestrator that manages sandbox lifecycle with state transitions. The ChangesSandbox Backend and Supervisor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
yanurag-dev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/backend/mock.go (1)
33-38: ⚡ Quick winMock backend should fail on unknown handles.
ExecandDestroySandboxcurrently succeed for nonexistent handles. That can let supervisor lifecycle bugs pass tests undetected.Suggested fix
func (m *MockBackend) Exec(handle string, req api.ExecRequest) (api.ExecResult, error) { + m.mu.RLock() + _, exists := m.sandboxes[handle] + m.mu.RUnlock() + if !exists { + return api.ExecResult{}, fmt.Errorf("sandbox handle not found: %s", handle) + } + return api.ExecResult{ ExitCode: 0, Stdout: fmt.Sprintf("mock output for %v", req.Command), }, nil } @@ func (m *MockBackend) DestroySandbox(handle string) error { m.mu.Lock() defer m.mu.Unlock() + if _, exists := m.sandboxes[handle]; !exists { + return fmt.Errorf("sandbox handle not found: %s", handle) + } delete(m.sandboxes, handle) return nil }Also applies to: 44-48
🤖 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/backend/mock.go` around lines 33 - 38, The MockBackend methods Exec and DestroySandbox currently always succeed; update both implementations on MockBackend to first check whether the provided handle exists in the mock's internal registry/state (e.g., the map or slice that tracks created sandboxes) and, if not present, return a non-nil error (for example fmt.Errorf("unknown handle: %s", handle)) instead of a successful api.ExecResult or nil error; keep the existing successful return behavior only when the handle is found. Ensure you modify the Exec and DestroySandbox methods on MockBackend accordingly.internal/supervisor/supervisor_test.go (1)
11-86: ⚡ Quick winAdd a contention test for concurrent lifecycle calls.
Please add a test that runs
RunCommandconcurrently (and/orRunCommandracing withStop) to lock in expected state-machine behavior and catch regressions in synchronization.🤖 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/supervisor/supervisor_test.go` around lines 11 - 86, Add a new subtest inside TestSupervisorLifecycle that stresses concurrent lifecycle operations: spawn multiple goroutines that call sup.RunCommand(id, req) concurrently (and another set that calls sup.Stop(id) racing with RunCommand) using a sync.WaitGroup and channels to coordinate start, then assert that no data race occurs, RunCommand returns either a valid ExecResult or a well-typed error, and that supervisor.instances map and instance.State remain consistent (use sup.instances, StateReady, sup.RunCommand, sup.Stop to locate code). Ensure the test waits for all goroutines to finish, checks for expected outcomes (allowed exit codes or expected error when stopped), and runs with -race in CI to catch synchronization regressions.
🤖 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.
Inline comments:
In `@internal/backend/mock.go`:
- Around line 24-25: The handle generation in the mock backend uses
len(m.sandboxes) which can produce duplicate handles after deletions; add an
integer counter field (e.g., nextID) to the mock backend struct and use it to
build handles (fmt.Sprintf("mock-%d", m.nextID)) instead of len(m.sandboxes),
incrementing nextID after each creation; also ensure the counter is initialized
where the mock backend is constructed (e.g., NewMockBackend or the mock struct
initializer) so handles are always unique regardless of map deletions and no
existing sandbox entries are overwritten.
In `@internal/supervisor/supervisor.go`:
- Around line 74-85: The SandboxInstance field writes/reads (State, Handle,
Error) are done without synchronization after s.mu is released, causing races in
Start, RunCommand and inconsistent behavior with Stop; fix by adding
synchronization to SandboxInstance (e.g., add an embedded sync.Mutex or
sync.RWMutex on SandboxInstance and provide methods like SetState, GetState,
SetError, SetHandle that lock/unlock appropriately) and update Supervisor
methods (Start, RunCommand, Stop) to use those instance-level methods so the
ready-check-to-transition in RunCommand is performed atomically under the
instance lock (or alternatively hold s.mu for the entire create/update sequence)
and remove unsynchronized direct field assignments/read accesses.
- Around line 43-48: NewSupervisor currently accepts backend and engine without
validation which can cause panics later; change its signature to return
(*Supervisor, error), validate that backend != nil and engine != nil inside
NewSupervisor, and return a descriptive error (e.g. fmt.Errorf("NewSupervisor:
backend is nil") / fmt.Errorf("NewSupervisor: policy engine is nil")) if either
is nil; on success return the constructed &Supervisor{instances: make(...),
backend: backend, policy: engine}, nil and update callers to handle the error.
- Around line 132-148: In Stop, avoid holding s.mu across the
backend.DestroySandbox call: acquire s.mu, fetch instance from s.instances[id],
set instance.State = StateDestroying, then unlock s.mu before calling
s.backend.DestroySandbox(instance.Handle); if DestroySandbox returns an error,
re-lock s.mu to set instance.State = StateError and instance.Error = err (and
return), otherwise re-lock s.mu to remove the instance from the instances map
and perform any final cleanup; ensure all reads/writes to s.instances and
instance fields occur while s.mu is held.
---
Nitpick comments:
In `@internal/backend/mock.go`:
- Around line 33-38: The MockBackend methods Exec and DestroySandbox currently
always succeed; update both implementations on MockBackend to first check
whether the provided handle exists in the mock's internal registry/state (e.g.,
the map or slice that tracks created sandboxes) and, if not present, return a
non-nil error (for example fmt.Errorf("unknown handle: %s", handle)) instead of
a successful api.ExecResult or nil error; keep the existing successful return
behavior only when the handle is found. Ensure you modify the Exec and
DestroySandbox methods on MockBackend accordingly.
In `@internal/supervisor/supervisor_test.go`:
- Around line 11-86: Add a new subtest inside TestSupervisorLifecycle that
stresses concurrent lifecycle operations: spawn multiple goroutines that call
sup.RunCommand(id, req) concurrently (and another set that calls sup.Stop(id)
racing with RunCommand) using a sync.WaitGroup and channels to coordinate start,
then assert that no data race occurs, RunCommand returns either a valid
ExecResult or a well-typed error, and that supervisor.instances map and
instance.State remain consistent (use sup.instances, StateReady, sup.RunCommand,
sup.Stop to locate code). Ensure the test waits for all goroutines to finish,
checks for expected outcomes (allowed exit codes or expected error when
stopped), and runs with -race in CI to catch synchronization regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b0d90e3-f992-4126-9ecc-6bfb3becca50
📒 Files selected for processing (3)
internal/backend/mock.gointernal/supervisor/supervisor.gointernal/supervisor/supervisor_test.go
- Add per-instance RWMutex to SandboxInstance to prevent data races. - Implement atomic state transitions in Supervisor. - Update NewSupervisor to validate inputs and return errors. - Improve MockBackend with unique handle generation and existence checks. - Add concurrent stress tests to supervisor_test.go.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements the core orchestration layer for Sandforge.
Changes:
Ref: #9
Summary by CodeRabbit
Tests
Chores