fix(deploy): count a dropped connection as an absent OpenPlanner - #40
Conversation
The Knoxx health gate has failed every deploy since the upstream reachability probe landed, and because deploy-caddy needs deploy-knoxx, a red Knoxx silently freezes the ingress configuration too — no Caddyfile change can reach the host while this is broken. Run 30758885732 burned all 30 probes on: knoxx: host OpenPlanner API at http://host.docker.internal:7777 did not answer (UND_ERR_CONNECT_TIMEOUT); expected=false The classifier only accepted ECONNREFUSED as "deliberately not deployed". That is what a closed port answers on an unfiltered host, and this host is not unfiltered: bootstrap-host.sh runs `ufw default deny incoming`, bridge-to-host-gateway traffic traverses INPUT, and ufw DROPs it. An absent OpenPlanner can therefore only ever time out here, so the skip branch was unreachable by construction and KNOXX_EXPECT_OPENPLANNER_REST =false had no effect. Add the connect-phase codes to the absent set. Everything else still fails the gate: ENOTFOUND means the host.docker.internal mapping did not apply, EHOSTUNREACH/ENETUNREACH mean the route is broken, and a bare TimeoutError means the connection was established and the response never came — a deployed service hanging, which is what 727cabc exists to catch. Verified by running the amended gate on the production host against the live containers: it now skips the CMS surface and exits 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughKnoxx’s OpenPlanner verification now treats connection timeouts as expected host absence. DNS, routing, and response-hang failures still fail the health gate. The environment template documents firewall-blocked connections as possible timeouts. ChangesOpenPlanner availability
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
digitalocean/services/knoxx/verify.sh (1)
128-128: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for the classifier matrix.
Assert that
ECONNREFUSED,UND_ERR_CONNECT_TIMEOUT, andETIMEDOUTrepresent absence. Assert thatENOTFOUND,EHOSTUNREACH,ENETUNREACH, andTimeoutErrorremain failures. Exercise both values ofKNOXX_EXPECT_OPENPLANNER_RESTand the actual nested error shape.🤖 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 `@digitalocean/services/knoxx/verify.sh` at line 128, Add a regression test covering the classifier matrix around ABSENT: verify ECONNREFUSED, UND_ERR_CONNECT_TIMEOUT, and ETIMEDOUT classify as absence, while ENOTFOUND, EHOSTUNREACH, ENETUNREACH, and TimeoutError remain failures. Run the assertions with both values of KNOXX_EXPECT_OPENPLANNER_REST and use the actual nested error structure passed to the classifier.
🤖 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 `@digitalocean/services/knoxx/verify.sh`:
- Around line 123-127: Update the timeout explanation near the OpenPlanner probe
to remove the claim that a bare TimeoutError proves the connection was
established, unless the probe explicitly provides separate connect and response
timeout guarantees. Keep the remaining error classifications and
deployed-service hanging behavior description accurate for the actual fetch
signal used.
---
Nitpick comments:
In `@digitalocean/services/knoxx/verify.sh`:
- Line 128: Add a regression test covering the classifier matrix around ABSENT:
verify ECONNREFUSED, UND_ERR_CONNECT_TIMEOUT, and ETIMEDOUT classify as absence,
while ENOTFOUND, EHOSTUNREACH, ENETUNREACH, and TimeoutError remain failures.
Run the assertions with both values of KNOXX_EXPECT_OPENPLANNER_REST and use the
actual nested error structure passed to the classifier.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a11530d3-4224-4431-8be0-9b37913139b8
📒 Files selected for processing (2)
digitalocean/services/knoxx/env.templatedigitalocean/services/knoxx/verify.sh
CodeRabbit pointed out that the previous commit's justification did not
hold: it claimed a bare TimeoutError proves a connection was established
and the response never came, but AbortSignal.timeout aborts the whole
fetch. A dropped connect and a hung response can surface as the same
error depending only on which timer wins, and undici's own connect
timeout is not configurable through global fetch. Lower
BACKEND_PROBE_TIMEOUT_MS below undici's 10s connect timeout — the
variable is configurable, and accepts anything from 1ms — and a dropped
connect starts arriving as TimeoutError, which the classifier calls not
absent, reinstating exactly the failure this branch fixes.
Rather than soften the comment, make the phase distinction real. The
probe now attempts a bare TCP connect first, with its own timeout, and
only issues the HTTP request once a connection has demonstrably been
established. `absent` is set in the connect phase and nowhere else, so a
hung deployed service cannot be mistaken for one that was never
deployed. The failure phase is reported alongside the code and appears
in the gate's own log lines.
The probe moves out of an inline `node -e` string into
probe-openplanner.js, read on the host and evaluated in the container so
the container's network view still applies. Being a real file, CI can
`node --check` it and run its classifier matrix as a self-test — which
answers CodeRabbit's second point without the test drifting from the
code the gate actually executes.
Verified against real sockets:
refused {"phase":"connect","code":"ECONNREFUSED","absent":true}
dropped {"phase":"connect","code":"CONNECT_TIMEOUT","absent":true}
hung {"phase":"response","code":"TimeoutError","absent":false}
healthy {"reachable":true,"status":200}
and on the production host against the live containers, which skips the
CMS surface with connect/CONNECT_TIMEOUT and exits 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What is broken
Every Knoxx deploy fails its health gate, and has since the upstream
reachability probe landed. Run
30758885732 —
the
deploylabel on #38 — burned all 30 probes on:Both containers were
Up (healthy)the whole time. The gate, not the service,was wrong.
The second-order damage matters more than the red job:
deploy-caddyhasneeds: deploy-knoxx, so a red Knoxx skips the ingress deploy entirely. NoCaddyfile change can reach the host while this is broken, which is easy to
misread as an ingress bug.
Why
The classifier accepted only
ECONNREFUSEDas "deliberately not deployed":That is what a closed port answers on an unfiltered host. This host is not
unfiltered —
digitalocean/scripts/bootstrap-host.shrunsufw default deny incoming, bridge-to-host-gateway traffic traversesINPUT, and ufw DROPs it.Confirmed from inside the running backend container:
So an absent OpenPlanner can only ever time out on this host. The skip branch
was unreachable by construction, and
KNOXX_EXPECT_OPENPLANNER_REST=false— theflag added precisely to express "nothing is deployed here" — had no effect.
The change
Add the connect-phase codes to the absent set. Everything the previous commits
wanted to catch still fails the gate:
ECONNREFUSEDUND_ERR_CONNECT_TIMEOUT,ETIMEDOUTENOTFOUNDhost.docker.internalmapping did not applyEHOSTUNREACH,ENETUNREACHTimeoutErrorThe last row is the one 727cabc exists to catch, and it is preserved: a bare
TimeoutErroris theAbortSignalfiring after a connection was established,which is a different failure from never connecting at all.
Verification
The amended gate was run on the production host against the live containers:
Ordering
This unblocks the deploy; it does not by itself fix MCP OAuth. The remaining
blocker is an application bug — the two
/.well-known/oauth-*documents 500 —fixed in open-hax/knoxx#212. Because the label build always builds
knoxx-backendfromopen-hax/knoxx@main, that PR must merge before thedeploylabel goes on this one.Caddy ingress for MCP is already correct and already live:
/mcpreachesFastify and returns the bearer challenge, so no Caddyfile change is needed here.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation