Harden Copilot api-proxy startup: verify listener accept readiness and absorb first-request ECONNREFUSED#52619
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52619 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review analysis; submitting review via safeoutputs tools.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This patch improves startup diagnostics, but it still leaves a real correctness hole in the readiness gate.
Blocking theme
The new TCP probe treats any successful connect as proof that the provider is ready, but for https:// providers it never performs a TLS handshake. A listener can accept TCP before TLS is configured, which means the harness can still proceed straight into the same first-request failure this change is supposed to eliminate.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 6.44 AIC · ⌖ 6.36 AIC · ⊞ 6.5K
Comment /review to run again
| settle(false); | ||
| }, perAttemptTimeoutMs); | ||
| const clear = () => clearTimeout(timer); | ||
| socket.once("connect", () => { |
There was a problem hiding this comment.
This readiness check is too weak for https:// providers: a raw TCP connect can succeed before the TLS listener is actually usable, so the harness can still advance into the same first-request failure path this patch is meant to prevent.
💡 Why this needs to change
waitForProviderListenerReady() derives the port from the URL, but it always probes with net.connect() and treats any accepted socket as success. For HTTPS endpoints that only proves something is listening on the port; it does not prove TLS negotiation will succeed or that the HTTP stack is ready to serve requests. In practice that leaves a race where the new gate reports ready and the first SDK call still dies during handshake/startup.
A safer approach is to probe at the protocol level: use tls.connect() for https: URLs (or perform a short /health/HEAD request through the same stack the real client uses), and only mark readiness once the secure handshake completes successfully.
const socket = parsed.protocol === "https:"
? tls.connect({ host, port, servername: host })
: net.connect({ host, port });That keeps the readiness gate aligned with the actual failure mode instead of only checking that the TCP port opened.
There was a problem hiding this comment.
Pull request overview
Hardens Copilot SDK startup against api-proxy listener races and transient connection refusals.
Changes:
- Adds TCP listener readiness probing before SDK traffic.
- Retries first-attempt connection refusals with a short backoff.
- Adds readiness and error-detection tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/awf_reflect.cjs |
Implements provider listener readiness probing. |
actions/setup/js/awf_reflect.test.cjs |
Tests readiness outcomes and constants. |
actions/setup/js/copilot_harness.cjs |
Gates startup and adds refusal retry handling. |
actions/setup/js/copilot_harness.test.cjs |
Tests connection-refusal detection. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
| const uniqueProviderBaseUrls = [...new Set(multiProvider.providers.map(provider => String(provider.baseUrl || "").trim()).filter(Boolean))]; | ||
| for (const providerBaseUrlToProbe of uniqueProviderBaseUrls) { |
| const timer = setTimeout(() => { | ||
| clear(); | ||
| lastError = `connect attempt timed out after ${perAttemptTimeoutMs}ms`; | ||
| socket.destroy(); | ||
| settle(false); | ||
| }, perAttemptTimeoutMs); |
| if (attempt === 0 && isConnectionRefused && maxRetries > 0) { | ||
| useContinueOnRetry = false; | ||
| log(`attempt ${attempt + 1}: connection refused on first request path — retrying as fresh run with short backoff (${FIRST_CONNECTION_REFUSED_RETRY_DELAY_MS}ms) (attempt ${attempt + 2}/${maxRetries + 1})`); | ||
| return { action: "retry", nextDelayMs: FIRST_CONNECTION_REFUSED_RETRY_DELAY_MS }; |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 83/100 — Excellent
📊 Metrics (6 tests)
NotesTest inflation: Mock usage: Coverage quality: The three Verdict
|
There was a problem hiding this comment.
Review: Harden Copilot api-proxy startup
The changes are well-structured and correctly address the api-proxy startup race condition. The TCP readiness probe with timeout, retry, and per-attempt timeout is sound, and the first-request ECONNREFUSED retry on attempt 0 is a practical defense-in-depth layer.
One minor observation (non-blocking):
The CONNECTION_REFUSED_ERROR_PATTERN (/connection refused|ECONNREFUSED/i) matches any connection-refused signal in the full process output, not just errors targeting the api-proxy provider. If Copilot SDK or an MCP tool logs ECONNREFUSED to an unrelated service, this could trigger the attempt-0 retry unnecessarily. Since the retry is bounded to attempt === 0 && maxRetries > 0 and uses only a 1-second delay, the blast radius is small — but worth tracking if false-positive retries are observed in practice.
Summary: Tests cover the happy path, timeout, and invalid-URL cases. Constants are exported cleanly. The settled guard prevents double-resolution. No blocking issues found. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 37.2 AIC · ⌖ 7.07 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Generated by ✂️ Ponytail Reviewer for #52619 · auto · 37.3 AIC · ⌖ 3.11 AIC · ⊞ 6.8K
Comment /ponytail to run again
| * }} options | ||
| * @returns {Promise<{ ok: true } | { ok: false, reason: "invalid_base_url" | "timeout", error: string }>} | ||
| */ | ||
| async function waitForProviderListenerReady(options) { |
There was a problem hiding this comment.
L392-455: yagni: hand-rolled retry loop with manual timers/socket state machine reimplements the withRetry(operation, config) helper already imported in this file (used at line 183). withRetry + a plain net.connect promise wrapper would cut this to ~15 lines.
| const timeoutMs = options?.timeoutMs ?? AWF_PROVIDER_LISTENER_READY_TIMEOUT_MS; | ||
| const retryDelayMs = options?.retryDelayMs ?? AWF_PROVIDER_LISTENER_READY_RETRY_MS; | ||
| const perAttemptTimeoutMsRaw = options?.perAttemptTimeoutMs ?? AWF_PROVIDER_LISTENER_READY_PROBE_TIMEOUT_MS; | ||
| const perAttemptTimeoutMs = Number.isFinite(perAttemptTimeoutMsRaw) && perAttemptTimeoutMsRaw > 0 ? perAttemptTimeoutMsRaw : AWF_PROVIDER_LISTENER_READY_PROBE_TIMEOUT_MS; |
There was a problem hiding this comment.
L396-397: yagni: defensive Number.isFinite/>0 validation on an internal-only perAttemptTimeoutMs option nobody sets from outside this repo. Drop the guard; trust the constant default.
| log(`copilot-sdk driver mode: multi-provider config resolved (${multiProvider.providers.length} providers, ${multiProvider.models.length} models, model=${resolvedModel})`); | ||
|
|
||
| const uniqueProviderBaseUrls = [...new Set(multiProvider.providers.map(provider => String(provider.baseUrl || "").trim()).filter(Boolean))]; | ||
| for (const providerBaseUrlToProbe of uniqueProviderBaseUrls) { |
There was a problem hiding this comment.
L1035-1042: yagni: loops over uniqueProviderBaseUrls but in practice this is almost always a single provider; the Set/map/filter/loop machinery for the common one-provider case adds indirection for a rare multi-provider path. A simple for-of over multiProvider.providers with baseUrl dedup inline would read the same in fewer moving parts.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the implementation is well-reasoned and the test coverage is solid for the happy path. Commenting (not blocking) with a few correctness and observability suggestions.
📋 Key Themes & Highlights
Key Themes
- Socket cleanup on success —
socket.end()without removing theerrorlistener leaves a window where a trailing RST can invoke the already-settled handler (comment #2). - Per-attempt timeout path untested — the
setTimeoutbranch inwaitForProviderListenerReadyhas no corresponding test case (comment #1). - Silent skip on empty provider URLs — an empty
uniqueProviderBaseUrlsbypasses the probe without any log trace (comment #3). - Undocumented invariant on
attempt === 0retry — the interaction between the readiness probe and the first-request ECONNREFUSED retry deserves a comment explaining why only attempt 0 is eligible (comment #4).
Positive Highlights
- ✅ Excellent use of injected
connectImplfor testability — the mock socket pattern is clean and reusable. - ✅ Right call to use
infrastructure-incompletestructured output andprocess.exit(1)on readiness failure — prevents burning model API quota. - ✅ De-duplicating provider URLs before probing (
[...new Set(...)]) is a nice touch. - ✅
FIRST_CONNECTION_REFUSED_RETRY_DELAY_MSconstant well-named and easy to tune.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 39.3 AIC · ⌖ 8.76 AIC · ⊞ 7K
Comment /matt to run again
| while (Date.now() - startedAt < timeoutMs) { | ||
| const ready = await new Promise(resolve => { | ||
| const socket = connectImpl({ host, port }); | ||
| let settled = false; |
There was a problem hiding this comment.
[/tdd] The per-attempt connect-timeout branch (where the setTimeout fires before connect/error) is not tested — only the error event path is covered.
💡 Suggested test sketch
it("returns timeout when per-attempt timer fires (hung connect)", async () => {
const probingConnect = vi.fn().mockImplementation(() => {
// never fire connect or error
return {
once(event, cb) { return this; },
end() {},
destroy() {},
};
});
const result = await waitForProviderListenerReady({
baseUrl: "(apiproxy/redacted)
timeoutMs: 50,
retryDelayMs: 1,
perAttemptTimeoutMs: 5,
connectImpl: probingConnect,
logger: () => {},
});
expect(result.ok).toBe(false);
expect(result.reason).toBe("timeout");
expect(result.error).toContain("timed out after 5ms");
});This branch sets lastError to the per-attempt timeout message; without a test, a refactor could silently break it.
@copilot please address this.
|
|
||
| let parsed; | ||
| try { | ||
| parsed = new URL(baseUrl); |
There was a problem hiding this comment.
[/diagnosing-bugs] socket.end() is called on a successful connect, but the error listener is not removed before end(). If the socket emits a late error after end() (common on abrupt server closes), the error handler will fire and set lastError even though the probe already settled true.
💡 Suggested fix
socket.once("connect", () => {
clear();
socket.removeAllListeners("error");
socket.destroy(); // destroy instead of end — we only need the handshake
settle(true);
});Using destroy() after a successful connect avoids the half-open window where a trailing RST could trigger the error listener.
@copilot please address this.
| }); | ||
| if (!readiness.ok) { | ||
| emitInfrastructureIncomplete(`api-proxy provider listener was not ready at ${providerBaseUrlToProbe} before first Copilot SDK request (${readiness.error}).`); | ||
| log(`copilot-sdk driver mode: provider listener readiness probe failed for ${providerBaseUrlToProbe}: ${readiness.error}`); |
There was a problem hiding this comment.
[/diagnosing-bugs] If uniqueProviderBaseUrls is empty (e.g., all providers had falsy baseUrl), the loop is skipped silently. The readiness check then offers no protection for misconfigured providers.
💡 Suggested guard
if (uniqueProviderBaseUrls.length === 0) {
log("copilot-sdk driver mode: no provider baseUrls to probe — skipping listener readiness check");
}A debug log here makes it visible in traces that the probe was bypassed, which helps when diagnosing future ECONNREFUSED incidents.
@copilot please address this.
| } | ||
| } | ||
|
|
||
| if (attempt === 0 && isConnectionRefused && maxRetries > 0) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The ECONNREFUSED retry only fires when attempt === 0, but the listener readiness probe already runs before the first request. If the probe passes yet the first request still gets ECONNREFUSED (e.g., provider crashes immediately after probe), the one-shot retry helps; however this intent isn't documented in a comment or test.
💡 Suggestion
Add a comment explaining the invariant:
// The readiness probe ensures the listener is up before attempt 0.
// A ECONNREFUSED here means the provider died in the narrow window
// between the probe and the first request — retry once as a fresh run.
if (attempt === 0 && isConnectionRefused && maxRetries > 0) {Also add a test that verifies attempt > 0 ECONNREFUSED does not trigger this path, to prevent the logic from accidentally expanding.
@copilot please address this.
|
@copilot Quick nudge: there is still a blocking review about the readiness gate not proving TLS-ready state for
|
… budget, tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the outstanding review feedback:
All 398 test files (12999 tests) pass in |
Triage: bug / risk: mediumScore: 68/100 (impact+urgency+quality) · Priority: high · Action: Fixes daily Assign-Issue-To-User failure: hardens Copilot api-proxy startup listener readiness / first-request ECONNREFUSED. Automated PR triage — see full report issue for details.
|
PR Triage\n\n- Category: bug\n- Risk: medium\n- Priority: high\n- Score: 70/100 (impact 35 + urgency 25 + quality 10)\n- Recommended action:
|
Daily Assign Issue To User failed because the Copilot sidecar reported healthy/key-valid but refused all
:10002chat-completion connections during initial harness attempts. This change targets that startup race by requiring real listener readiness before first SDK traffic and softening first-request refusal handling.Readiness: probe actual TCP accept on provider listeners
waitForProviderListenerReady(...)inactions/setup/js/awf_reflect.cjs.host:portderived from providerbaseUrluntil connect succeeds or timeout.Copilot SDK startup gate: block until provider listeners are actually accepting
actions/setup/js/copilot_harness.cjs, after multi-provider resolution, the harness now probes each unique providerbaseUrllistener.Retry behavior: tolerate first-request connection refusal
ECONNREFUSED/connection refuseddetection path in the Copilot harness.--continuedisablement.Regression coverage
awf_reflecttests for listener readiness success/timeout/invalid URL behavior and new constants.