fix(holyship): spawn holyshipper with PORT=3100 to match fleet URL template#133
fix(holyship): spawn holyshipper with PORT=3100 to match fleet URL template#133
Conversation
…mplate
Core's Fleet.create constructs Instance.url as http://<name>:3100
(fleet.ts:101), but holyshipper was being spawned with PORT=8080.
waitForReady then pinged the wrong port, timed out after 30s every
time, and the worker tore down the freshly-started holyshipper.
Verified via battleaxe SSH on prod:
holyship-its-over-stack-overflowing-spit-on-that-thang:
[holyshipper] worker-runtime listening on :8080
But fleet's URL says 3100 — nothing answers, timeout, teardown.
Change PORT to 3100 to match the URL template. Holyshipper respects
the env var, so no holyshipper-side change needed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 32 minutes and 51 seconds. ⌛ 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 (1)
✨ 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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR fixes a port mismatch between the fleet URL template and holyshipper containers by updating the holyshipper process environment to listen on port 3100 instead of 8080, ensuring waitForReady health checks succeed. Sequence diagram for holyshipper provisioning and waitForReady health checksequenceDiagram
actor User
participant Core as Core_Service
participant Fleet as Fleet_Manager
participant Docker as Docker_Engine
participant Holy as Holyshipper_Container
User->>Core: Request new holyshipper instance
Core->>Fleet: Fleet.create(entityId)
Note over Core,Fleet: Core sets instance.url = http://<name>:3100
Fleet->>Docker: Start container holyship-<entityId>
activate Docker
Docker-->>Holy: Run holyshipper with env
Note over Holy: HOLYSHIP_GATEWAY_URL, HOLYSHIP_ENTITY_ID, PORT=3100
deactivate Docker
Core->>Holy: waitForReady GET /health on port 3100
alt PORT is 3100 (after fix)
Holy-->>Core: 200 OK
Core-->>User: Provisioning succeeds
else PORT was 8080 (before fix)
Holy-->>Holy: Listening on port 8080
Core-->>Core: Retries until 30s timeout
Core-->>User: Provisioning fails, teardown instance
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull request overview
Fixes a readiness/health-check failure when provisioning holyshipper worker containers via core’s fleet API by aligning the container’s listening port with core’s default Instance.url template (http://<name>:3100).
Changes:
- Set
PORT=3100for holyshipper containers spawned byHolyshipperFleetManager. - Add inline rationale documenting why the port must match core’s fleet URL template to avoid
waitForReadytimeouts.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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:
- Since
3100is now assumed both here and inFleet.create, consider extracting the port into a shared constant/config so that future changes to the default port don't silently desynchronize these code paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `3100` is now assumed both here and in `Fleet.create`, consider extracting the port into a shared constant/config so that future changes to the default port don't silently desynchronize these code paths.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryFixes a port mismatch where holyshipper containers were spawned with Confidence Score: 5/5Safe to merge — the fix is a targeted, correct one-line change with no side effects. The change aligns the container's listening port with what core's Fleet.create uses as the fallback URL (confirmed at fleet.ts:101). No other files have the stale port, the comment explains the reasoning clearly, and the production symptom exactly matches the described root cause. No files require special attention.
|
| Filename | Overview |
|---|---|
| platforms/holyship/src/fleet/holyshipper-fleet-manager.ts | PORT env var corrected from 8080 to 3100 to match core's Fleet.create URL template; change is minimal, well-commented, and directly resolves the described 30s timeout regression. |
Sequence Diagram
sequenceDiagram
participant H as HolyshipperFleetManager
participant C as Core (Fleet.create)
participant CT as Container (holyshipper)
H->>C: createContainer(name, image, env={PORT:"3100"})
C->>CT: docker run --env PORT=3100 ...
Note over CT: holyshipper listens on :3100
C-->>H: instance = { url: "http://hs-xxx:3100" }
H->>CT: GET http://hs-xxx:3100/health (waitForReady)
CT-->>H: 200 OK
H->>CT: POST /credentials
H->>CT: POST /checkout
H-->>H: ProvisionResult { runnerUrl: "http://hs-xxx:3100" }
Reviews (1): Last reviewed commit: "fix(holyship): spawn holyshipper with PO..." | Re-trigger Greptile
Summary
Port mismatch. Core's Fleet.create builds `Instance.url = http://:3100` (fleet.ts:101). But holyshipper containers were being spawned with `PORT=8080`. `waitForReady` pings http://:3100/health, nothing answers, 30s timeout, teardown.
Verified on prod (battleaxe → core VPS):
```
docker logs holyship-its-over-stack-overflowing-spit-on-that-thang
... [holyshipper] worker-runtime listening on :8080
```
Fix
Pass `PORT=3100` in the env for holyshipper. Holyshipper already reads `process.env.PORT`, so no holyshipper-side change needed.
Test plan
🤖 Generated with Claude Code
Summary by Sourcery
Bug Fixes: