Fail fast when the concurrent PATCH route is not ready - #2204
Conversation
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the readiness polling logic in the concurrentPatchMerge integration test. It introduces robust error handling, tracks the last observed status or error, cancels the response body to prevent resource leaks, and throws a detailed error if the service fails to become ready within the 30-second deadline. There are no review comments, and I have no feedback to provide.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const readinessURL = `${httpURL}/CollabDoc/`; | ||
| const readinessDeadlineMs = 30_000; | ||
| const deadline = Date.now() + readinessDeadlineMs; | ||
| let ready = false; | ||
| let lastObserved = 'no response'; | ||
| while (Date.now() < deadline) { | ||
| try { | ||
| const probe = await fetch(`${httpURL}/CollabDoc/`, { | ||
| const probe = await fetch(readinessURL, { | ||
| headers: { Authorization: auth }, | ||
| signal: AbortSignal.timeout(3_000), | ||
| }); | ||
| if (probe.status !== 404) break; | ||
| } catch { | ||
| /* not ready yet */ | ||
| lastObserved = `HTTP ${probe.status}`; | ||
| await probe.body?.cancel(); | ||
| if (probe.ok) { | ||
| ready = true; | ||
| break; | ||
| } | ||
| } catch (error) { | ||
| lastObserved = error instanceof Error ? `${error.name}: ${error.message}` : String(error); | ||
| } | ||
| await sleep(250); | ||
| } | ||
| if (!ready) | ||
| throw new Error( | ||
| `Harper did not become ready at ${readinessURL} within ${readinessDeadlineMs}ms; last observed ${lastObserved}` | ||
| ); |
There was a problem hiding this comment.
Suggestion (non-blocking): This re-implements the same poll/timeout/last-observed-error pattern as waitForRouteReady(client, probePath, timeoutMs) in integrationTests/apiTests/utils/lifecycle.mjs (extracted the same day in #1904), and client here (from createApiClient) already exposes the compatible reqRest() method. Reusing it would drop ~20 duplicated lines. The one real difference is the readiness predicate — the helper treats any non-404 as ready, while this test requires a true 2xx (probe.ok) — which the PR body already flags as an open decision. If that stricter predicate is needed here, consider adding an optional predicate parameter to the shared helper instead of a second bespoke implementation with different semantics.
|
Reviewed; no blockers found. Left one non-blocking suggestion inline about reusing the existing waitForRouteReady helper. |
1 participant
Summary
GET /CollabDoc/readiness probe to return a successful response before QA-328 starts.CI root cause
The failing Node 24 shard never returned from
setupHarperWithFixture; the later route poll did not run. The harness eventually reportedHarper produced no startup output for 150000ms before reporting ready.The server artifact shows initial HTTP thread IDs 1-4, then replacement IDs 5-7, with no
Harper successfully startedmessage.manageThreadsautomatically replaces a worker that exits before readiness, butsocketRouteronmainwaits on promises owned by the original Worker objects. A replacement creates a different promise, so the original startup barrier can never settle. The exit codes/triggers were not retained in that artifact, but the abandoned readiness barrier explains the 150-second hang after the exits.Fix HTTP startup after pre-ready worker restart #2129 already fixes that product-level lifecycle defect by owning readiness per logical worker slot and includes an end-to-end crash/replacement regression. Its CI passes across the Node, Windows, uWS, and integration matrices. This PR does not duplicate that open fix; the two changes are complementary.
Testing
npm run buildnpm run lint:requiredgit diff --checkIndependent review: Gemini + Cursor Composer + Harper-domain. Claude was attempted but returned provider HTTP 529 after its retry budget.
Human-Review-Need: 3 (decisions: inline-poll-vs-shared-helper, readiness-predicate-2xx-vs-non-404, duplicate-qa328-suites, fail-fast-vs-fixing-the-flake) @ 10d7283