engine: add Engine.Shutdown honoring shutdownAction#32
Merged
Conversation
Engine.Down stays the explicit teardown path: callers calling
Down still get unconditional stop (and Remove) regardless of the
spec field. Engine.Shutdown is the new spec-aware companion for
editor-close / idle-shutdown semantics:
- "none": no-op, container left running.
- "stop" / "stopContainer" / unset (image/build): stop without
remove.
- "stopCompose" / unset (compose): stop without remove (project-
level Stop is approximated with primary-container stop until #10
adds a ComposeStop primitive).
Closes #22.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds Engine.Shutdown(ctx, ws) which validates inputs, resolves an effective shutdownAction (from config or defaulting: stopCompose for compose workspaces, stopContainer otherwise), and dispatches to shutdownStopContainer or shutdownStopCompose (ShutdownNone is a no-op). Unit and integration tests exercise behavior and input validation. ChangesEngine.Shutdown Implementation
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@down.go`:
- Around line 142-144: The Shutdown path currently only checks ws for nil but
later dereferences ws.Container.ID and can panic; update Engine.Shutdown (and
the similar check around the second dispatch at the 170-172 area) to validate
that ws.Container is non-nil before proceeding and return a clear error (e.g.,
fmt.Errorf("Engine.Shutdown: Workspace.Container is required") or similar)
instead of continuing; locate the checks around ws == nil and add a guard that
checks if ws.Container == nil and fails fast, referencing Workspace.Container
and ws.Container.ID in your change so you prevent nil-pointer dereferences.
- Around line 185-189: The current shutdownAction=stopCompose path only calls
shutdownStopContainer(ctx, ws) which stops just the primary container and leaves
sibling services running; update the implementation so stopCompose performs a
project-level stop by iterating all containers/services in the workspace or
invoking the Compose runtime's project-stop API (e.g., call a
ComposeRuntime.StopProject/Stop(ctx, project) if available) instead of
shutdownStopContainer; modify or add a function (e.g.,
shutdownStopComposeProject or extend shutdownStopContainer) to enumerate ws
services/containers and stop each one (or call the compose project stop API) to
ensure all sibling services are stopped.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 708d21e0-4a3e-4e7d-8a57-383d00de130a
📒 Files selected for processing (3)
down.goshutdown_test.gotest/integration/shutdown_action_test.go
Validates ws.Container and ws.Container.ID up front so the dispatch paths can dereference safely (CodeRabbit #32). Tests cover both nil-Container and empty-ID cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@down.go`:
- Around line 130-132: The doc comment for "stopCompose" incorrectly claims it
performs a project-level `docker compose stop`; update the comment to state that
the current implementation (stopCompose) only stops the primary container as an
approximation of a full compose stop, and mention that callers should use Down
with Remove=true for full teardown; optionally add a TODO in the comment
pointing to implementing full project-level stopping in the future.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52e12cd4-0e38-4813-afa4-b07e28eab898
📒 Files selected for processing (2)
down.goshutdown_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shutdown_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.
Summary
Mapping (per spec)
Closes #22.
Test plan
Unit (`shutdown_test.go`): all six branches exercised against the in-memory `fakeRuntime` — `none` no-op, explicit `stopContainer`, default for image, `stopCompose` for compose, `stopCompose` fallback when set on a non-compose workspace, nil-workspace rejection.
Integration (`test/integration/shutdown_action_test.go`): Up against the alpine test image with each action, then assert container state via `InspectContainer`:
The latter also serves as a regression test for the "stop without remove" contract.
Why a new method instead of changing `Down`?
`Down` already has callers (DAP, the integration suite) that expect unconditional teardown. Making it silently no-op for `shutdownAction: none` would surprise them. `Shutdown` is the spec-faithful complement; consumers who want spec semantics call it; consumers who want "gone now" call `Down`.
Summary by CodeRabbit
New Features
Tests