fix(fleet): drop redundant instance.start() in runCreateContainer#132
fix(fleet): drop redundant instance.start() in runCreateContainer#132
Conversation
Fleet.create() enqueues \`bot.start\` which dockerManager.startBot() handles in a single create-AND-start step on the node-agent. The followup \`await instance.start()\` at line 399 always threw: Instance 6baaf660-...: Docker not available — use command bus The Instance constructed at fleet.ts:97-105 from the queue result is a value object for the core-side caller — no \`docker\` handle, no \`instanceRepo\`, no \`eventEmitter\`. start()/stop()/etc. call getDocker() and fail. That's intentional for everything EXCEPT this one redundant start that slipped in. Verified via prod logs (battleaxe → core): createContainer → queue dispatch → handler runs on core worker → fleet.create → bot.start op to node-agent succeeds → container up → instance.start() on core-side Instance → throw → op marked failed → holyship's tRPC surfaces the error. Deleting the line closes the last gap for engine→core fleet provision. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRemoves a redundant container start call in InstanceService.runCreateContainer and documents the correct lifecycle, ensuring core no longer tries to start containers directly when Docker is only available on node-agents. Sequence diagram for fleet.createContainer container lifecycle fixsequenceDiagram
participant Engine as Engine_holyship
participant CoreClient as Core_client
participant Core as Core_service
participant Queue as Core_queue
participant NodeAgent as Node_agent
participant DockerMgr as DockerManager
Engine->>CoreClient: call fleet.createContainer
CoreClient->>Core: fleet.createContainer.mutate(params)
Core->>Queue: enqueue instance.create_container (target: core)
Queue->>Core: deliver instance.create_container job
Core->>Core: InstanceService.runCreateContainer
Core->>Core: Fleet.create(params)
Core->>Queue: enqueue bot.start (target: null)
Queue->>NodeAgent: deliver bot.start job
NodeAgent->>DockerMgr: startBot(botId)
DockerMgr->>DockerMgr: create_and_start_container
DockerMgr-->>NodeAgent: containerId, nodeId
NodeAgent-->>Core: bot.start result
Core->>Core: construct Instance value (no docker field)
Note over Core,Core: Previously: await instance.start() here (now removed)
Core-->>CoreClient: CreatedBareContainer(id, url, containerId, gatewayKey)
CoreClient-->>Engine: return result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Core as InstanceService<br/>(Core)
participant Fleet as Fleet.create()
participant NodeAgent as Node-Agent
participant Docker as dockerManager
Note over Core,Docker: OLD FLOW (Failed)
Core->>Core: runCreateContainer()
Core->>Core: instance.start()
Core->>Docker: Access Docker
Note over Docker: ❌ Docker not accessible<br/>on core-side
Docker-->>Core: Error
Note over Core,Docker: NEW FLOW (Correct)
Core->>Fleet: runCreateContainer()
Core->>Fleet: Fleet.create()
Fleet->>Fleet: Enqueue bot.start
Fleet-->>Core: ✓ Returns (async)
NodeAgent->>NodeAgent: Process queued bot.start
NodeAgent->>Docker: dockerManager.startBot()
Docker->>Docker: Execute container start
Docker-->>NodeAgent: ✓ Success
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new comment block is quite long and implementation-specific; consider tightening it to a brief lifecycle invariant (e.g.
InstanceService assumes Fleet.create() enqueues bot.start and returns an already-started container on core) and moving any deeper explanation into a code comment near theInstanceconstruction or an internal design note.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new comment block is quite long and implementation-specific; consider tightening it to a brief lifecycle invariant (e.g. `InstanceService assumes Fleet.create() enqueues bot.start and returns an already-started container on core`) and moving any deeper explanation into a code comment near the `Instance` construction or an internal design note.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryRemoves a stray Confidence Score: 5/5Safe to merge — single-line deletion of a call that always threw in production, with no remaining P0/P1 issues. The fix is minimal, correct, and well-documented. Fleet.create() already starts the container via the bot.start queue job, and the Instance value object constructed on the core side has no docker field (confirmed in instance.ts line 53), so instance.start() always threw. Removing the call and documenting the architectural invariant is the right fix. Build confirmed clean. No edge cases missed. No files require special attention.
|
| Filename | Overview |
|---|---|
| core/platform-core/src/fleet/instance-service.ts | Removes the redundant await instance.start() (line 398) that always threw on the core side; adds a detailed comment explaining that bot.start via the queue already handles create+start atomically on the node-agent. |
Sequence Diagram
sequenceDiagram
participant holyship as holyship (engine)
participant core as core tRPC handler
participant queue as DB Queue
participant agent as node-agent
participant docker as Docker
holyship->>core: fleet.createContainer.mutate(...)
core->>queue: enqueue instance.create_container (target: "core")
queue->>core: QueueWorker picks up → runCreateContainer()
core->>queue: Fleet.create() → enqueue bot.start (target: null)
queue->>agent: agent claims bot.start job
agent->>docker: dockerManager.startBot() — create + start in one step
docker-->>agent: containerId, url
agent-->>queue: result {nodeId, containerId, url}
queue-->>core: BotStartResult
core->>core: construct Instance value object (no docker handle)
Note over core: ✅ AFTER FIX: skip instance.start()<br/>❌ BEFORE FIX: instance.start() → getDocker() → throws
core-->>holyship: CreatedBareContainer {id, url, containerId, gatewayKey}
Reviews (1): Last reviewed commit: "fix(fleet): drop redundant instance.star..." | Re-trigger Greptile
There was a problem hiding this comment.
Pull request overview
Removes a core-side instance.start() call from the bare-container creation path to avoid invoking Docker operations from the orchestrator (core), where Instance is intentionally constructed without a Docker handle and container start is already performed by the node-agent via the bot.start operation.
Changes:
- Drop redundant
await instance.start()inInstanceService.runCreateContainer(). - Add an explanatory comment documenting that
Fleet.create()enqueuesbot.startand the node-agent performs a single create-and-start step.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
The last gap for engine→core fleet provision.
Traced via prod logs (battleaxe → core VPS):
```
holyship: [fleet] calling core.fleet.createContainer
core: QueueWorker: handler failed
error: "Instance 6baaf660-...: Docker not available — use command bus"
workerId: core-e3f022ec-...
```
Sequence:
Step 6 is redundant. The container is already running from step 4. The stray `instance.start()` was left behind and breaks every bare-container creation path.
Fix
Delete the line. Add a comment explaining why the start is already handled.
Test plan
🤖 Generated with Claude Code
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit