feat(connectors): execute a connector's declared retryConfig and requestTimeoutMs at the one platform fetch site - #19388
Conversation
…uest timeout WIP: wrapper knobs, the single mapping, the provider context widening and the three built-in HTTP providers. Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm Co-authored-by: Claude <noreply@anthropic.com>
…te named Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm Co-authored-by: Claude <noreply@anthropic.com>
… policy Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm Co-authored-by: Claude <noreply@anthropic.com>
…nnector-retry-timeouts
…ntry `connectorFetchOptions` returns it, so a consumer writing an un-annotated `export const opts = connectorFetchOptions(...)` hit TS2883. check:entry-nameability named it; one declaration, re-exported. Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm Co-authored-by: Claude <noreply@anthropic.com>
…nnector-retry-timeouts
Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm Co-authored-by: Claude <noreply@anthropic.com>
📓 Docs Drift CheckThis PR changes 4 package(s): 3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 136 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 4bd3b8be1f2a8a4619abbcc70bd5d0cc249b5c6b && git checkout 4bd3b8be1f2a8a4619abbcc70bd5d0cc249b5c6b
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f9e16d856b278d5cd45807991c7a3b44db511698 13987f1b69e5ca0b5902d639ab8ff8fe09b6b14a && git checkout -B drift-repro f9e16d856b278d5cd45807991c7a3b44db511698 && git merge --no-ff 13987f1b69e5ca0b5902d639ab8ff8fe09b6b14a
node scripts/docs-audit/affected-docs.mjs --json f9e16d856b278d5cd45807991c7a3b44db511698
|
The Docs Drift Check pointed at content/docs/automation/flows.mdx, which documents this key's counting base for authors and contrasts it with maxRetries. The page is right and the first mapping was wrong: retries = maxAttempts, one to one, with resilientFetch's own floor covering maxAttempts: 0. Pinned by the case that tells the two readings apart. Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm Co-authored-by: Claude <noreply@anthropic.com>
…mplemented behaviour #18794's bleed-stop text said retryConfig was declared but unimplemented and that ConnectorProviderContext could never carry it. Both are now false, so the ruling's third bullet applies: correct it rather than absorb that card. health.circuitBreaker and connectionTimeoutMs stay named as still inert. Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm Co-authored-by: Claude <noreply@anthropic.com>
Contract reviewServed-tier: 1/1 Isolated at-tier reviewer, spawned by the ① Derived judgmentsSTEP ZERO — holds, and it is what keeps the PR small. Read at the head: ZONE 1 — nine rows, not ten: the reasoning is right; the ZONE 2 — the counting-base correction is right and the pin discriminates. ZONE 3 — jitter-then-cap is RIGHT; the ZONE 4 — taking openapi in place was right; leaving it unpinned was not. Right to take: the openapi def already advertised ZONE 5 — the corrected text is true of the implementation; the .mdx is generated. ZONE 6 — gates. Derivation reproduced in my worktree: 109 families ( ② Semver level
③ Boundary flags
What to change — exactly, then the same head rule applies to the new push
Nothing else is owed. The nine-row flip, the widening, the materializer parse-and-fail-by-name, the signature inclusion, the docs corrections and the gate ledger are all as claimed. Implemented-by: VERDICT: FAIL Generated by Claude Code |
One open item on the review record is already discharged: the
|
…outing Review record 5751411253, two FAIL items. 1. maxDelayMs was not a maximum: Retry-After was exempt, so maxDelayMs 1000 against retry-after 3600 slept 3600000ms while the ledger row read live. A Retry-After past the ceiling now ends the retry loop and returns the response - sleeping it out would make the key not a maximum, and retrying sooner than asked is the abuse Retry-After prevents. 2. The openapi routing had no pin of its own; restoring the naked fetch left all 34 openapi tests green. Pinned through the provider factory on call counts, the one thing a naked fetch cannot produce. Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm Co-authored-by: Claude <noreply@anthropic.com>
|
Contract reviewServed-tier: 1/1 Round 2 by the same isolated at-tier reviewer (spawned by the ① Derived judgmentsFAIL 1 (openapi routing unpinned) — DISCHARGED, and the dev's precision claim is true on both halves. Two cases in FAIL 2 (
One consequence to name so nobody reads it as a regression later: an author who declares any Text follows the code. The wrapper's Round-1 findings carried on unchanged code (the only source file changed between the two heads is Round-2 re-measured at this head (every exit captured before any pipe): spec mapping + wrapper 29/29; CI on this head, read from the check-runs and the job log, not adopted. Required contexts: ② Semver levelUnchanged from round 1 and still right: ③ Boundary flags
Implemented-by: VERDICT: PASS Generated by Claude Code |
…nnector-retry-timeouts
Contract reviewServed-tier: Round 3 by an isolated at-tier reviewer (spawned by the ① Derived judgmentsZ1 — the merge is what it claims. Z2 — neither guard was undone by the merge. Both files carry the same blobs at this head as at
Z3 — the gate family at this head. Re-derived with Z4 — generated artifacts. Z5 — CI on this head. Check-runs read per NAME, newest-run-wins, on Z6 — the PR body at this head. Every number the body keys to a head reproduces where re-measured here: openapi 36/36, rest 26/26, slack 10/10, spec wrapper + mapping 29/29, service-automation ② Semver levelUnchanged from rounds 1 and 2: ③ Boundary flags
Implemented-by: VERDICT: PASS Generated by Claude Code |
Fixes #18975
Clause-②: yes
Ruling of record: comment
5729479418— director seat summon #24, batch #159 item 5, maintainer 「同意」 2026-09-18T11:42Z, letter 实现. Not re-adjudicated here. The spec declarations do not move: the connector schema keeps every key, every bound and every default it had.STEP ZERO — where the platform-owned connector fetch actually lives
Measured before anything was written, on
origin/main=be7382d77e(2026-09-20T14:05Z), re-confirmed after the merge toada70122.The ruling's wrapper already exists.
packages/spec/src/shared/resilient-fetch.ts— exported asresilientFetchfrom@objectstack/spec/shared— is the platform's outbound-HTTP call: it already gave every attempt a 30s timeout and a bounded exponential backoff with jitter andRetry-Afterhandling. So "land the wrapper there once, no gateway, no new subsystem" was satisfiable without building anything new.What the connectors do with it, measured per package:
connector-restrest-connector.tsrequest()resilientFetch(...)connector-slackslack-connector.tscallSlack()resilientFetch(...)connector-openapiopenapi-connector.tsrequest()fetch— unbounded, never retriedconnector-mcpclient.callToolfetchat all; the MCP SDK owns the transport, with a hardcoded 30stimeoutSo the fetch site is one shared wrapper plus one bypass to fold in, not several independent paths and not something that needed restructuring. The gap was never "there is no wrapper" — it was that the wrapper could not express the declared policy, and that no authored value could reach it:
ConnectorProviderContextcarried none of the three keys.Holder check at claim time: across all 30 open PRs, zero touch any file matching
connector,resilient-fetchorservice-automation(lit control on the same scan:packages/spechits 9, 22 and 9 files on PRs #19364 / #19363 / #19335).What landed
One wrapper, extended by exactly what was missing.
ResilientFetchOptionsgainsstrategy,backoffMultiplier,maxDelayMs,jitterandretryOnNetworkError. Each defaults to the behaviour the wrapper already had, so a caller that passes none is byte-identical to before.One mapping.
connectorFetchOptions()(packages/spec/src/integration/connector-fetch-policy.ts) is the single place a connector's declared policy becomes wrapper options — one execution site, not one per connector package.One contract widening.
ConnectorProviderContextgainsretryConfig,connectionTimeoutMsandrequestTimeoutMs, read-only and resolved: the materializer parsesretryConfigthroughRetryConfigSchema, so a factory reads real values instead of re-deriving the schema's defaults. The policy also joins the instance signature, so editing it re-materializes the connector instead of leaving the old policy serving until restart.The built-in HTTP providers honour it by construction.
restandopenapipass the context's policy into their connector builders.✅ RESOLVED at 2026-09-20T17:02:28Z — the work is on the branch; the push simply lagged the report by about two minutes. Kept in full rather than deleted, because the sequence is worth more than the tidy version. At 17:00:04Z the remote tip was
4432f967e3with an 18-file diff and ⛔ none of the three files below; the dev's report already described them at head5911c8cf. The seat held the review and struck this paragraph. At 17:02:28Zgit ls-remotereports the tip as5911c8cf664f534823d598c801f852c346be8075—5911c8cf docs(spec): the connector header and SYNC_ARCHITECTURE describe the implemented behavioursitting on top of4432f967— 21 files, all three present. ⇒ the 17:00Z reading was true when taken and is now superseded; the report was accurate about content and early about the push. ⭐ The rule that survives, and it is not 「the check was wasted」:git ls-remoteis the authority and the PR object is not — while this was being checked the PR object was still serving the stale 18-file count. ⛔ A conclusion drawn from a summary face has a shelf life; one drawn from the ref does not.The teaching text #18794 narrowed is corrected to describe the implemented behaviour —
packages/spec/docs/SYNC_ARCHITECTURE.mdin five places, plus theconnector.zod.tsheader TSDoc it renders from (content/docs/references/integration/connector.mdxfollows bygen:docs, ⛔ never hand-edited). Those passages asserted the keys were 「declared but currently unimplemented」 and thatConnectorProviderContextcould never carry them; both are now false. This is the ruling's third bullet, ⛔ not an absorption of #18794. ⭐health.circuitBreakerandconnectionTimeoutMsare explicitly kept named as still inert in every corrected passage.SYNC_ARCHITECTURE.mdstates the rule by its inputs, so it shares no identifier with the emitter this diff changed and ⛔ no run could ever have listed it. The bot's three named pages were each hand-verified and two were ACCURATE and left untouched —error-catalog.mdx'sno_retryis the API error-envelope enum fromapi/errors.zod.ts, a different enum this diff never touches, andjobs.mdxisjob.retryPolicyfromshared/retry-policy.zod.ts, likewise untouched. The third was accurate too, and it is the one that falsified the code.Two interpretive calls, both stated rather than assumed:
maxAttemptscounts TOTAL calls, the first included — the contrastcontent/docs/automation/flows.mdxalready draws againstmaxRetries, and it is what corrected this implementation.min(0)and ashared/retry-policy.zod.tscomment the dev has since said it over-read (that comment is about opt-in vs opt-out defaults, ⛔ not the counting base). The first reading reached a pushed commit; it was falsified by a documentation page, and the implementation was changed to match the page — ⛔ not the other way round. New pin:maxAttempts: 3must make three calls, ⛔ not four, the case that tells the two readings apart. Ablation: restoring+ 1turns 3 mapping tests red.maxDelayMsis applied after jitter. Jitter is additive, so capping first would let a delay land up to 99ms above the declared ceiling.The seat's assumption 4 is falsified, and that is the one thing the ruling asked me to report rather than invent
AbortSignal.timeoutis available (Node 22 or newer, which the rootenginesfield pins; already used atpackages/drivers/driver-turso/src/turso-driver.ts). The connection-vs-request distinction is not.A connector's call is a WHATWG
fetch, whose only cancellation surface is oneAbortSignalover the whole operation; nothing in that interface observes the connection phase separately. Bounding time-to-response withconnectionTimeoutMswould kill a slow-but-connected upstream that the author meant to allow with a largerequestTimeoutMs— breaking the very promise the key makes. Node's undici exposesconnectTimeoutthrough a custom dispatcher, which is Node-only and a new subsystem underneath every connector: the same ruling forbids it.So
connectionTimeoutMsis carried ontoConnectorProviderContext(a custom provider on a transport that can separate the phases may honour it) and not enforced by the platform.packages/spec/liveness/connector.jsonkeeps that one rowdead, with the measurement written into it, and a pin inconnector-fetch-policy.test.tsgoes red if anyone aliases it ontotimeoutMs. Nine of the ten rows flip, not ten. The tenth is owed a second, narrower ADR-0049 decision — see the acceptance notes.Verification
Full pipeline at the final commit
956fdb10.Gate family, re-derived in this worktree from the real changed paths (
node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack), every command run with its exit code captured before any pipe, then reconciled with--ran:The three NOT MEASURED are
PREREQUISITE NOT MET, not findings:check:dual-build-cjs-loadsandcheck:type-check-debtboth need a whole-repo build closure (CI builds it before those steps), andcheck-plugin-teardown-shape.mjs --self-testcannot reach its commit-pinned positive control on a shallow clone. All three are CI's to run.Tests (all on the merged tree):
typecheckgreen for all six.pnpm --filter @objectstack/spec check:generated: 15 of 15 artifacts up to date (api-surface/andexport-origins/regenerated after a real build — the two new exports plus theResilientFetchOptionsre-export).Lint — measured whole, not narrowed.
eslint . --no-inline-config --format jsonover the repo root: 6939 files, 0 errors, 0 warnings, exit 0.Ablation — both pins proved able to fail, through
scripts/ablation-replace.mjs(anchor must hit, blob hash must move, restore proved againstHEAD):connector-fetch-policy.ts: invert the early return so a declaredretryConfigis never mappedrest-provider.ts: stop passingctx.retryConfiginto the connectorThe pins assert call counts and delay sequences, not the presence of a field: a pin on the def's
retryConfigcould not have failed here, because the key was already storable and served back before any of this landed. The sharpest one is the narrowing case — an authoredretryableStatusCodes: [429]must leave a 500 unretried, which only passes if the authored list is the one executed (500 is retryable under both the wrapper's own default and the schema default).Acceptance notes
To file (class (c), an authoring trap that survives this PR):
connector.connectionTimeoutMsstill parses, still stores, is still served back by/meta/connector, and is enforced by nothing — for the measured reason above, which is a property offetch, not an omission here. It now needs a decision this card's ruling did not answer: retire it, or re-describe it as something the platform can enforce (its siblingrequestTimeoutMsalready is). Reproduction: declare aconnectors:entry withprovider: 'rest'andconnectionTimeoutMs: 1000, pointproviderConfig.baseUrlat an endpoint that takes 5s, and dispatch therequestaction — it completes normally. Dedupe words:connectionTimeoutMs declared unenforced·connector connect timeout AbortSignal fetch·ADR-0049 connectionTimeoutMs second decision·connector.json connectionTimeoutMs dead row·retire or redescribe connect timeout.Fixed in place, declared here rather than filed:
connector-openapi's generated actions went through a nakedfetch— unbounded, never retried, and the one built-in HTTP path an authored policy could never reach. It is the same defect on the same measured site as this card's, the fix is mechanical and its shape was already pinned by two sibling connectors, no other open PR holds the file, and it adds no new gate family. Those actions now go through the same wrapper asconnector-restandconnector-slack. Evidence:openapi-connector.tscreateOpenApiConnector—doFetch(url, init)becameresilientFetch(url, init, fetchOptions);@objectstack/connector-openapi34 tests still pass.Noted, not filed:
ConnectorProviderContext.iconand.typeare set by the materializer and read by none of the three shipped provider factories, so an authoredicon:ortype:on a declarative instance never reachesGET /api/v1/automation/connectors. Already recorded per-row inpackages/spec/liveness/connector.json, with what is owed already stated there. Next toucher: whoever adds or changes a provider factory.connector-slackships no provider factory, so nothing authored can reach it — only the plugin door, hand-wired by its host. Not silent: an unknownprovideris a loud, named boot failure that lists the installed ones. Next toucher: whoever adds aslackprovider key.connector-rate-limit-config-removedcomment inpackages/spec/src/conversions/registry.tssaysretryConfigand the timeouts "are live". It was wrong when written (the ledger'sretryConfig.strategyrow corrects it by name), and this PR makes nine tenths of it accidentally true. A stale comment, no behaviour. Next toucher: whoever edits that conversion entry.health.circuitBreaker's sub-keys aredeadon the same schema and the same ADR-0049 worklist. Out of this card's scope by the card's own words ("本卡只管这三个"), and its teaching text was already stanched by [finding]connector.zod.ts:44-47的 TSDoc 仍逐字教着「限流上游的解法是retryConfig与health.circuitBreaker」—— 那是 PR #18979 刚在 SYNC_ARCHITECTURE.md 收掉的同一句话的**源头**,且有一个生成的下游 #18983. Next toucher: the next ADR-0049 connector sweep.Round 2 — both at-tier FAIL items fixed, at head
4a9b3480f2Review record
5751411253FAILed this PR on two items. Both are fixed, pinned and ablated; ⛔ nothing else was widened, and the two optional notes the review offered (the.describe('Maximum retry attempts')counting-base wording, and the pre-existingRetry-After-on-any-retryable-status and unbounded-body-read observations) were deliberately not acted on.FAIL 1 — the openapi routing is now pinned
The review's ablation proved this PR's own justification false: 「shape already pinned by two sibling connectors」 did not hold for this file — restoring the naked fetch left 34/34 openapi tests green.
Two cases added in
openapi-provider.test.ts, through the factory withretryConfigon ctx, mirroringrest-provider.test.ts: scripted fetch[503, 200],{strategy: 'fixed_delay', maxAttempts: 2, initialDelayMs: 100, retryableStatusCodes: [503], jitter: false}, asserting exactly 2 upstream calls and a 200. The review's own ablation reproduces on the same blobs (a0172843ccb9→22b1470c9170) and now turns the retry pin RED where it measured 34/34 green; restore proved blob == HEAD.FAIL 2 —
maxDelayMsis now a maximum. Route (a), and the reasonThe review offered two routes. Route (a) was taken: a
Retry-Afterlonger thanmaxDelayMsnow ends the retry loop and returns the response.⭐ Why (a) and not (b): this card exists to make a declaration equal its enforcement, so making 「Maximum retry delay in ms」 true beats documenting an exception to it. Route (b) would have left a key whose name says maximum with an upstream-controlled way past it — which is the exact shape #19410 was filed for earlier today.
The three alternatives, and why returning wins: sleeping it out makes the key not a maximum; retrying sooner than asked is the abuse
Retry-Afterexists to prevent; returning hands the caller the real status and its header. Only aRetry-Aftercan reach that branch, becausebackoffMscaps its own output — so the review's jitter-cap lit control is untouched.Pinned at
maxDelayMs: 1000+retry-after: 3600→ 1 call, the 429 returned,sleepnever called, with a control that aRetry-Afterwithin the ceiling is still honoured and still retried. Ablation: deleting the guard (bab28fcb1bde→9564b3126ef7) turns it RED; restore proved blob == HEAD.⇒ the
retryConfig.maxDelayMsledger note and the changeset sentence were both corrected, so ⛔ no artefact still claims a ceiling the code ignores.Verification at the pushed head
Gate family re-derived on the pushed tip and again on the fix commit — identical 109 families both times: 107 green / 2 NOT-MEASURED / 0 red / 0 unrun. The two NOT-MEASURED are the shallow-clone self-test and
check:dual-build-cjs-loadsneeding the full build closure — ⛔ exit 3 is a prerequisite, ⛔ not a red.check:generated15/15 with no regeneration owed (route (a) moved no.describe(), soconnector.mdxdid not move). Tests: openapi 36 (was 34), rest 26, slack 10, spec wrapper+mapping 29. Full-repo lint 6,945 files, 0 errors, 0 warnings.⏹⚠️ Overtaken and corrected 2026-09-20T20:37Z — the merge WAS taken. The paragraph below was true when written and is spent; kept struck rather than deleted, because the reason it gave is the reason round 3 exists.
Round 3 —
origin/mainmerged, the head re-reviewed, and the base-drift cost paidOnce round 2 cleared, the base was 21 commits stale and the landing needed a fresh CI run, so
origin/main576d5df6was merged once as13987f1b. ⛔ No rebase, ⛔ no amend, ⛔ no force-push, ⛔ no empty commit.The merge is provably automatic:
13987f1bhas exactly two parents (4a9b3480,576d5df6), andgit merge-tree --write-tree 4a9b3480 576d5df6yields tree8350d10d, which equals13987f1b^{tree}⇒ no hand resolution existed. The two sides are disjoint — this PR 22 files, main 83, intersection 0 (lit control: both lists non-empty).This PR's own delta did not move: 22 files, +1197/−132, the same file list as before the merge.
Neither guard was undone. Both blobs are byte-identical to round 2, and both ablations still give exactly 1 red — the openapi routing pin (35 passed) and
maxDelayMs bounds a Retry-After by STOPPING(18 passed, the within-ceiling and jitter-cap controls green by name). Restores proved blob-equal to HEAD.Gates: 110 derived / 109 green / 1 NOT-MEASURED / 0 red / 0 unrun. One family appeared with the incoming commits (
check:issue-citations, wired in by5a5e710f); a true set comparison against a round-2 derivation gives only-in-r3 = that one, only-in-r2 = none. The NOT-MEASURED ischeck-plugin-teardown-shape --self-testat exit 3 — a shallow-clone prerequisite, ⛔ not a red.check:generated15/15 with no regeneration owed, the migration registry included after main deleted ten entry files. Whole-repo lint 6,943 files, 0 errors, 0 warnings.⭐ Honest cost, stated because the reviewer found it and the dev's number alone would have hidden it: the first gate reading on a turbo-cache-restored closure was 108 green + 2 exit 3, not 109 + 1 —
check:type-check-debtrefused on a dist whose mtimes predated its sources, and reached exit 0 only after a real rebuild. Same conclusion, named with what it cost.CI on⚠️ A green re-run corroborates that the earlier red was not this PR's; it ⛔ does not prove it. The load-bearing evidence is still the mechanism filed as #19424 (
13987f1b: 35 names, 33 success + 2 skipped, 0 failing, 0 pending — all sixTest Coreshards green, including the 2/6 shard that was red before.80 × 0.25 s = 20 sagainst an observed 20,999 ms, the same titled assertion passing in 299 ms in the same run).⭐ And one thing this PR previously could not establish, now established from a different door:
GET /branches/main/protectionanswers 403, butGET /rules/branches/mainanswers 200 and lists seven required contexts —Test Coreamong them, all sevensuccesshere. A 403 on one endpoint is a fact about that endpoint, ⛔ not about the question.Round-3 at-tier review: PASS — record
5752480809, keyed to this head.check-clause2-carriers --pair 19388exits 0 with zero ✗ rows, run after the record existed.Generated by Claude Code