Skip to content

Commit 604ed57

Browse files
os-zhuangclaude
andcommitted
feat(spec): resilientFetch — timeout + backoff for outbound HTTP (P1-1)
Connectors/embedder used naked fetch with no timeout or retry, so a slow or rate-limited external API could hang an agent turn with no recovery. New shared resilientFetch (@objectstack/spec/shared): 30s per-attempt timeout (AbortController) + exponential backoff w/ jitter (3 tries) on network errors / 429 / 5xx, honouring Retry-After; never retries a caller-initiated abort. Wired into connector-rest, connector-slack, embedder-openai. connector-mcp uses the MCP SDK transport, so it gets a 30s per-request timeout on callTool/listTools instead. (Also added the missing @objectstack/spec/shared alias to embedder's vitest config so the subpath value-import resolves.) Circuit breaker deliberately deferred (stateful/per-host); timeout+backoff already removes the hang/no-recovery risk. +13 tests (helper 9, connector retry 1, existing suites green). docs/launch-readiness.md P1-1 updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 0a6438e commit 604ed57

11 files changed

Lines changed: 295 additions & 11 deletions

File tree

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
---
2+
'@objectstack/spec': minor
3+
'@objectstack/connector-rest': patch
4+
'@objectstack/connector-slack': patch
5+
'@objectstack/embedder-openai': patch
6+
'@objectstack/connector-mcp': patch
7+
---
8+
9+
feat(spec): resilientFetch — timeout + backoff for outbound HTTP (P1-1)
10+
11+
Outbound calls in the connectors/embedder were naked `fetch` with no timeout or
12+
retry, so a slow or rate-limited external API could hang an agent turn with no
13+
recovery.
14+
15+
New shared `resilientFetch` (`@objectstack/spec/shared`):
16+
- per-attempt timeout via `AbortController` (default 30s);
17+
- exponential backoff with jitter, up to 3 attempts, on network errors / 429 / 5xx;
18+
- honours a `Retry-After` header on 429;
19+
- never retries a caller-initiated abort (intentional cancellation).
20+
21+
Wired into `connector-rest`, `connector-slack`, and `embedder-openai`.
22+
`connector-mcp` talks through the MCP SDK transport, so it gets a 30s per-request
23+
`timeout` on `callTool` / `listTools` instead.
24+
25+
A stateful per-host **circuit breaker** is deliberately left as a follow-up:
26+
timeout + backoff already removes the hang/no-recovery risk.

docs/launch-readiness.md

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,18 @@ fix or acceptance.**
110110
rate-limited external API hangs the entire agent turn with no recovery.
111111
- **Action:** Add a default request timeout (e.g. 30s, configurable) + exponential
112112
backoff (3 tries) + a circuit breaker; tests for 429 / timeout paths.
113-
- **Owner:** _______ · Verify ☐ · Sign-off ☐ · Notes: _______
113+
- **Fix:** New shared `resilientFetch` (`@objectstack/spec/shared`) — 30s per-attempt
114+
timeout (AbortController) + exponential backoff with jitter (3 tries) on
115+
network errors / 429 / 5xx, honouring `Retry-After`; never retries a
116+
caller-initiated abort. Wired into `connector-rest`, `connector-slack`,
117+
`embedder-openai`. `connector-mcp` uses the MCP SDK transport, so it gets a 30s
118+
per-request `timeout` on `callTool` / `listTools` instead. +13 tests (helper 9,
119+
connector retry 1, plus existing suites green).
120+
- **Deferred (follow-up, not blocking):** a **circuit breaker** — it's stateful
121+
and per-host; timeout + backoff already removes the "hangs the agent turn / no
122+
recovery" risk. Making timeout/retry **per-call configurable** (currently
123+
sensible defaults) is a small follow-up.
124+
- **Owner:** _______ · Verify ✅ (confirmed real @ `main`) · Sign-off ☐ · Notes: Timeout + backoff shipped across all 4 paths; circuit breaker deferred (rationale above). Awaiting human sign-off.
114125

115126
### P1-2 — Unbounded growth: execution logs, job runs, event log
116127
- **Area:** `service-automation` (in-memory exec logs, hard 1000 cap),

packages/connectors/connector-mcp/src/mcp-connector.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,13 @@ function normalizeResult(raw: unknown): Record<string, unknown> {
149149
return out;
150150
}
151151

152+
/**
153+
* Default per-request timeout (ms) for MCP calls (P1-1). Without it, a hung or
154+
* unresponsive MCP server stalls the agent turn indefinitely. The SDK aborts the
155+
* request once this elapses.
156+
*/
157+
const MCP_REQUEST_TIMEOUT_MS = 30_000;
158+
152159
/**
153160
* The default {@link McpClientLike} — lazily imports the official MCP SDK so it
154161
* is only loaded when a real connection is made (tests inject their own client).
@@ -182,11 +189,11 @@ async function defaultClientFactory(
182189

183190
return {
184191
async listTools() {
185-
const res = await client.listTools();
192+
const res = await client.listTools(undefined, { timeout: MCP_REQUEST_TIMEOUT_MS });
186193
return (res.tools ?? []) as McpToolDescriptor[];
187194
},
188195
async callTool(name, args) {
189-
return client.callTool({ name, arguments: args });
196+
return client.callTool({ name, arguments: args }, undefined, { timeout: MCP_REQUEST_TIMEOUT_MS });
190197
},
191198
async close() {
192199
await client.close();

packages/connectors/connector-rest/src/rest-connector.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,27 @@ describe('createRestConnector — request action', () => {
4848
expect(out).toEqual({ status: 200, ok: true, body: { id: 1, name: 'Ada' } });
4949
});
5050

51+
it('retries a transient 503 then returns the success (P1-1)', async () => {
52+
let n = 0;
53+
const calls: number[] = [];
54+
const impl = (async () => {
55+
calls.push(1);
56+
const status = n++ === 0 ? 503 : 200;
57+
return {
58+
status,
59+
ok: status >= 200 && status < 300,
60+
headers: { get: (h: string) => (h.toLowerCase() === 'content-type' ? 'application/json' : null) },
61+
json: async () => ({ ok: status === 200 }),
62+
text: async () => '',
63+
};
64+
}) as unknown as typeof fetch;
65+
const { handlers } = createRestConnector({ baseUrl: 'https://api.example.com', fetchImpl: impl });
66+
67+
const out = await handlers.request({ path: '/x' }, {});
68+
expect(calls.length).toBe(2); // retried the 503 once
69+
expect(out.status).toBe(200);
70+
});
71+
5172
it('JSON-encodes the body and sets Content-Type for non-GET', async () => {
5273
const { impl, calls } = stubFetch();
5374
const { handlers } = createRestConnector({ baseUrl: 'https://api.example.com', fetchImpl: impl });

packages/connectors/connector-rest/src/rest-connector.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

33
import type { Connector } from '@objectstack/spec/integration';
4+
import { resilientFetch } from '@objectstack/spec/shared';
45

56
/**
67
* Generic REST connector — the reference *concrete* connector (ADR-0018
@@ -97,7 +98,6 @@ function applyAuth(
9798
export function createRestConnector(opts: RestConnectorOptions): RestConnectorBundle {
9899
const name = opts.name ?? 'rest';
99100
const auth: RestAuth = opts.auth ?? { type: 'none' };
100-
const doFetch = opts.fetchImpl ?? fetch;
101101

102102
const def: Connector = {
103103
name,
@@ -154,11 +154,11 @@ export function createRestConnector(opts: RestConnectorOptions): RestConnectorBu
154154
headers['Content-Type'] = 'application/json';
155155
}
156156

157-
const response = await doFetch(url, {
157+
const response = await resilientFetch(url, {
158158
method,
159159
headers,
160160
body: hasBody ? JSON.stringify(req.body) : undefined,
161-
});
161+
}, { fetchImpl: opts.fetchImpl });
162162

163163
// Parse JSON when advertised; fall back to text so non-JSON endpoints
164164
// don't throw.

packages/connectors/connector-slack/src/slack-connector.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

33
import type { Connector } from '@objectstack/spec/integration';
4+
import { resilientFetch } from '@objectstack/spec/shared';
45

56
/**
67
* Slack connector — a *concrete* connector (ADR-0018 §Addendum) and the second
@@ -78,7 +79,6 @@ export interface SlackConnectorBundle {
7879
export function createSlackConnector(opts: SlackConnectorOptions): SlackConnectorBundle {
7980
const name = opts.name ?? 'slack';
8081
const baseUrl = (opts.baseUrl ?? 'https://slack.com/api').replace(/\/+$/, '');
81-
const doFetch = opts.fetchImpl ?? fetch;
8282

8383
const def: Connector = {
8484
name,
@@ -151,11 +151,11 @@ export function createSlackConnector(opts: SlackConnectorOptions): SlackConnecto
151151
Authorization: `Bearer ${opts.token}`,
152152
};
153153

154-
const response = await doFetch(`${baseUrl}/${method}`, {
154+
const response = await resilientFetch(`${baseUrl}/${method}`, {
155155
method: 'POST',
156156
headers,
157157
body: JSON.stringify(params),
158-
});
158+
}, { fetchImpl: opts.fetchImpl });
159159

160160
// The Slack Web API always answers with JSON; `ok` is the real outcome.
161161
const body = (await response.json()) as Record<string, unknown>;

packages/plugins/embedder-openai/src/index.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
*/
2121

2222
import type { IEmbedder } from '@objectstack/spec/contracts';
23+
import { resilientFetch } from '@objectstack/spec/shared';
2324

2425
export interface OpenAIEmbedderOptions {
2526
/** Bearer token sent as `Authorization: Bearer <apiKey>`. Required. */
@@ -154,15 +155,15 @@ export class OpenAIEmbedder implements IEmbedder {
154155
if (texts.length === 0) return [];
155156
const body: Record<string, unknown> = { model: this.model, input: texts };
156157
if (this.requestedDims) body.dimensions = this.requestedDims;
157-
const res = await this.fetchImpl(`${this.baseUrl}/embeddings`, {
158+
const res = await resilientFetch(`${this.baseUrl}/embeddings`, {
158159
method: 'POST',
159160
headers: {
160161
'content-type': 'application/json',
161162
authorization: `Bearer ${this.apiKey}`,
162163
...this.extraHeaders,
163164
},
164165
body: JSON.stringify(body),
165-
});
166+
}, { fetchImpl: this.fetchImpl });
166167
if (!res.ok) {
167168
const text = await res.text().catch(() => '');
168169
throw new Error(

packages/plugins/embedder-openai/vitest.config.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ export default defineConfig({
1111
resolve: {
1212
alias: {
1313
'@objectstack/spec/contracts': path.resolve(__dirname, '../../spec/src/contracts/index.ts'),
14+
'@objectstack/spec/shared': path.resolve(__dirname, '../../spec/src/shared/index.ts'),
1415
'@objectstack/spec': path.resolve(__dirname, '../../spec/src/index.ts'),
1516
},
1617
},

packages/spec/src/shared/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,3 +18,4 @@ export * from './metadata-collection.zod';
1818
export * from './lazy-schema';
1919
export * from './expression.zod';
2020
export * from './protection.zod';
21+
export * from './resilient-fetch';
Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect, vi } from 'vitest';
4+
import { resilientFetch } from './resilient-fetch';
5+
6+
/** Minimal Response stand-in — resilientFetch only reads `.status` + `.headers.get`. */
7+
function resp(status: number, headers: Record<string, string> = {}): Response {
8+
return {
9+
status,
10+
ok: status < 400,
11+
headers: { get: (k: string) => headers[k.toLowerCase()] ?? null },
12+
} as unknown as Response;
13+
}
14+
15+
/** A fetch that returns the scripted statuses in order (last one repeats). */
16+
function scripted(statuses: Array<number | Record<string, string> | [number, Record<string, string>]>) {
17+
let i = 0;
18+
return vi.fn(async () => {
19+
const s = statuses[Math.min(i++, statuses.length - 1)];
20+
if (Array.isArray(s)) return resp(s[0], s[1]);
21+
return resp(s as number);
22+
});
23+
}
24+
25+
const noSleep = async () => {};
26+
27+
describe('resilientFetch', () => {
28+
it('returns a successful response without retrying', async () => {
29+
const fetchImpl = scripted([200]);
30+
const res = await resilientFetch('http://x', {}, { fetchImpl, sleep: noSleep });
31+
expect(res.status).toBe(200);
32+
expect(fetchImpl).toHaveBeenCalledTimes(1);
33+
});
34+
35+
it('retries a 429 then returns the success', async () => {
36+
const fetchImpl = scripted([429, 200]);
37+
const res = await resilientFetch('http://x', {}, { fetchImpl, sleep: noSleep, retries: 3 });
38+
expect(res.status).toBe(200);
39+
expect(fetchImpl).toHaveBeenCalledTimes(2);
40+
});
41+
42+
it('retries a 5xx then returns the success', async () => {
43+
const fetchImpl = scripted([503, 500, 200]);
44+
const res = await resilientFetch('http://x', {}, { fetchImpl, sleep: noSleep, retries: 3 });
45+
expect(res.status).toBe(200);
46+
expect(fetchImpl).toHaveBeenCalledTimes(3);
47+
});
48+
49+
it('gives up after `retries` attempts and returns the last response', async () => {
50+
const fetchImpl = scripted([500, 500, 500]);
51+
const res = await resilientFetch('http://x', {}, { fetchImpl, sleep: noSleep, retries: 3 });
52+
expect(res.status).toBe(500);
53+
expect(fetchImpl).toHaveBeenCalledTimes(3);
54+
});
55+
56+
it('does NOT retry a non-retryable status (4xx other than 429)', async () => {
57+
const fetchImpl = scripted([404, 200]);
58+
const res = await resilientFetch('http://x', {}, { fetchImpl, sleep: noSleep, retries: 3 });
59+
expect(res.status).toBe(404);
60+
expect(fetchImpl).toHaveBeenCalledTimes(1);
61+
});
62+
63+
it('honours a numeric Retry-After header on a 429', async () => {
64+
const fetchImpl = scripted([[429, { 'retry-after': '2' }], 200]);
65+
const sleep = vi.fn(noSleep);
66+
await resilientFetch('http://x', {}, { fetchImpl, sleep, retries: 3 });
67+
expect(sleep).toHaveBeenCalledWith(2000);
68+
});
69+
70+
it('times out a hung request and surfaces the error', async () => {
71+
const fetchImpl = vi.fn(
72+
(_url: any, init: any) =>
73+
new Promise<Response>((_, reject) => {
74+
init.signal.addEventListener('abort', () => reject(new Error('aborted')), { once: true });
75+
}),
76+
);
77+
await expect(
78+
resilientFetch('http://x', {}, { fetchImpl, sleep: noSleep, retries: 1, timeoutMs: 10 }),
79+
).rejects.toThrow();
80+
expect(fetchImpl).toHaveBeenCalledTimes(1);
81+
});
82+
83+
it('retries a network error before succeeding', async () => {
84+
let n = 0;
85+
const fetchImpl = vi.fn(async () => {
86+
if (n++ === 0) throw new Error('ECONNRESET');
87+
return resp(200);
88+
});
89+
const res = await resilientFetch('http://x', {}, { fetchImpl, sleep: noSleep, retries: 3 });
90+
expect(res.status).toBe(200);
91+
expect(fetchImpl).toHaveBeenCalledTimes(2);
92+
});
93+
94+
it('does not retry when the caller aborts', async () => {
95+
const ac = new AbortController();
96+
ac.abort();
97+
const fetchImpl = vi.fn(async (_u: any, init: any) => {
98+
if (init.signal.aborted) throw new Error('aborted by caller');
99+
return resp(200);
100+
});
101+
await expect(
102+
resilientFetch('http://x', { signal: ac.signal }, { fetchImpl, sleep: noSleep, retries: 3 }),
103+
).rejects.toThrow(/aborted/);
104+
expect(fetchImpl).toHaveBeenCalledTimes(1);
105+
});
106+
});

0 commit comments

Comments
 (0)