mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-01 15:35:12 +01:00
fix(workspace-runtime): keep the real readiness failure when the deadline-clamped probe aborts (#11575)
## Thinking Path > - Paperclip manages AI agents and the services they use for work. > - An agent can wait for a workspace service to become ready. > - Each readiness probe has a time limit. > - A final probe can expire and hide an earlier HTTP failure. > - This pull request keeps that observed failure in the reported error. > - Operators can then see why the service was not ready. ## Linked Issues or Issue Description Refs #11525. Related: #11494. **What happened?** A deadline-limited final readiness probe replaced an observed HTTP 503 with an abort message. **Expected behavior** Keep an earlier service failure when only the readiness deadline aborts the last probe. Report normal connection failures and probe timeouts as before. **Steps to reproduce** Return HTTP 503 from the first probe. Leave one millisecond in the readiness budget. Abort the last probe. The reported error must still contain HTTP 503. ## What Changed - Track whether a readiness probe observed a service failure. - Preserve that observation when the final probe expires at the readiness deadline. - Add a deterministic regression with an injected clock. - Keep probe scheduling, request count and timeout limits unchanged. ## Verification - Author verification: the readiness suite passed all nine tests; server typecheck passed. The regression failed before the fix and passed after it. - The later task review recorded 172 passing workspace tests and seven additional readiness checks. Historical author baseline failures are not claimed as passing local runs. - Latest head `936a374a08d0e74d87d588ba034f674388e8d508`: CI passed with 54 successful checks and two intentional skips. Fresh Apex review is 5/5 with no unresolved findings or recommendations. - A test merge with current master is clean. ## Risks - Error text changes when a service first fails and then hangs. The earlier observed failure is retained. - A hang without a prior service failure still reports the abort. - No data model, API or permission change. ## Model Used Anthropic Claude Opus 5 (`claude-opus-5`) authored the fix through Claude Code with reasoning, file editing and test tools. OpenAI GPT-5.6 assisted the original diagnosis. OpenAI GPT-6 (`gpt-6`) through Codex assisted this final review and PR preparation with repository and terminal tools. This session does not expose an exact context-window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
473c80d13f
commit
be3e614d61
2 files changed
+58
-8
No files matched your search
@@ -120,6 +120,44 @@ describe("managed runtime start terminality", () => {
|
||||
expect(probes).toBeLessThan(12);
|
||||
});
|
||||
|
||||
it("preserves the last HTTP failure when the deadline-clamped probe aborts", async () => {
|
||||
// The readiness budget (1s) is smaller than the per-probe timeout, so every probe is bounded
|
||||
// by the deadline. The first probe observes the real failure (503); the second is admitted
|
||||
// with 1ms left and aborts immediately. That abort says the budget ran out, not that the
|
||||
// service misbehaved, so it must not overwrite the 503 in the thrown message.
|
||||
let nowMs = 0;
|
||||
let probes = 0;
|
||||
const fetchImpl = (async (_url: string | URL | Request, init?: RequestInit) => {
|
||||
probes += 1;
|
||||
if (probes === 1) {
|
||||
nowMs = 999;
|
||||
return new Response(null, { status: 503 });
|
||||
}
|
||||
return await new Promise<Response>((_resolve, reject) => {
|
||||
const rejectOnAbort = () => {
|
||||
nowMs = 1_000;
|
||||
reject(new Error("The operation was aborted"));
|
||||
};
|
||||
if (init?.signal?.aborted) {
|
||||
rejectOnAbort();
|
||||
return;
|
||||
}
|
||||
init?.signal?.addEventListener("abort", rejectOnAbort, { once: true });
|
||||
});
|
||||
}) as unknown as typeof fetch;
|
||||
|
||||
await expect(
|
||||
waitForRuntimeServiceReadiness({
|
||||
service: { readiness: { type: "http", timeoutSec: 1, intervalMs: 100 } },
|
||||
url: "http://127.0.0.1:45439/",
|
||||
readinessUrl: null,
|
||||
fetchImpl,
|
||||
now: () => nowMs,
|
||||
}),
|
||||
).rejects.toThrow("Readiness check failed for http://127.0.0.1:45439/: received HTTP 503");
|
||||
expect(probes).toBe(2);
|
||||
});
|
||||
|
||||
it("hands two concurrent starts distinct ports even when the kernel repeats a candidate", async () => {
|
||||
// The kernel really does hand the same ephemeral port to two `listen(0)` probes once the
|
||||
// first probe socket has closed, which is exactly the reallocation collision in the report.
|
||||
|
||||
@@ -5276,21 +5276,33 @@ export async function waitForRuntimeServiceReadiness(input: {
|
||||
let lastError = "service did not become ready";
|
||||
let lastCause: unknown;
|
||||
let probes = 0;
|
||||
let sawServiceFailure = false;
|
||||
while (now() < deadline) {
|
||||
const probeBudgetMs = Math.max(1, Math.min(RUNTIME_SERVICE_READINESS_PROBE_TIMEOUT_MS, deadline - now()));
|
||||
const remainingMs = deadline - now();
|
||||
// When the readiness budget is what bounds this probe, aborting it only means we ran out of
|
||||
// time — it is not an observation about the service, so it must not replace one we already have.
|
||||
const boundedByDeadline = remainingMs <= RUNTIME_SERVICE_READINESS_PROBE_TIMEOUT_MS;
|
||||
const probeBudgetMs = Math.max(1, Math.min(RUNTIME_SERVICE_READINESS_PROBE_TIMEOUT_MS, remainingMs));
|
||||
let probeSignal: AbortSignal | undefined;
|
||||
probes += 1;
|
||||
try {
|
||||
const response = await fetchImpl(readinessUrl, { signal: AbortSignal.timeout(probeBudgetMs) });
|
||||
probeSignal = AbortSignal.timeout(probeBudgetMs);
|
||||
const response = await fetchImpl(readinessUrl, { signal: probeSignal });
|
||||
if (response.ok) return;
|
||||
lastError = `received HTTP ${response.status}`;
|
||||
lastCause = undefined;
|
||||
sawServiceFailure = true;
|
||||
} catch (err) {
|
||||
lastCause = err;
|
||||
lastError = err instanceof Error ? err.message : String(err);
|
||||
// Node fetch hides connection errors behind "fetch failed". Retain the
|
||||
// transport cause so a refused port is distinguishable from a timeout.
|
||||
if (err instanceof Error && err.cause instanceof Error) {
|
||||
lastError += `: ${err.cause.message}`;
|
||||
const abortedByDeadline = Boolean(probeSignal?.aborted) && boundedByDeadline;
|
||||
if (!abortedByDeadline || !sawServiceFailure) {
|
||||
lastCause = err;
|
||||
lastError = err instanceof Error ? err.message : String(err);
|
||||
// Node fetch hides connection errors behind "fetch failed". Retain the
|
||||
// transport cause so a refused port is distinguishable from a timeout.
|
||||
if (err instanceof Error && err.cause instanceof Error) {
|
||||
lastError += `: ${err.cause.message}`;
|
||||
}
|
||||
sawServiceFailure = true;
|
||||
}
|
||||
}
|
||||
if (now() >= deadline) break;
|
||||
|
||||
Reference in new issue
Block a user