Skip to content

Commit f347c79

Browse files
os-billclaude
andauthored
docs(spec): SYNC_ARCHITECTURE stops teaching retryConfig as the rate-limit remedy (#18979)
Fixes #18794 Clause-②: no ## What changed `packages/spec/docs/SYNC_ARCHITECTURE.md` taught authors that a rate-limited upstream is answered by `retryConfig`, printed its `retryableStatusCodes` defaults (429 included), and listed it as a reason to pick L3 — while nothing reads the key. Five passages in that one file now say what is measurably true today: **declared but currently unimplemented**, each pointing at `packages/spec/liveness/connector.json`. This is option **A** from the card, and only A. No declaration, schema or accept set is touched, so the ADR-0049 ruling on these keys is deliberately not prejudged. The keys are **not** described as retired (they are still declared and still parse, so an author writing them still sees no error) and **not** as the host's job (falsified below). | Site (line at base `106717c3aa`) | Was | Now | |---|---|---| | `:157` blockquote | "what L3 does declare for a rate-limited upstream is `retryConfig`" | declared-but-unimplemented, ledger cited, host-seam falsification stated inline | | `:299` example comment | "Retry Configuration — for the connector's own outbound requests" | DECLARED BUT CURRENTLY UNIMPLEMENTED, ledger cited | | `:312` example timeouts | bare `connectionTimeoutMs` / `requestTimeoutMs` | annotated declared-but-unimplemented, ledger cited | | `:336` Best Practices | "`retryConfig` handles the `429` you get for exceeding a limit" | it does not; ledger cited; retrying is the provider's to implement | | `:360` decision row | "**Yes** → L3 (Connector) — `retryConfig`, `health.circuitBreaker`" | "Not a reason to pick a level"; the row's existing #4911 sentence is left byte-identical | ## Measurements — re-taken on this branch, not inherited **Consumer probe, fold-proof predicate, firing control in the same run.** Comment leaders are stripped first, then every whitespace run (newlines included) is collapsed and glued to the access punctuation, so a folded access cannot hide from it. Read-shaped access only (`x.KEY`, `x?.KEY`, `x["KEY"]`, destructure). 8763 tracked files, tree `106717c3aa`: ``` .retryConfig OUTSIDE packages/spec 0 hits in any packages/ or examples/ file (2 hits total, both in the GENERATED reference page content/docs/references/integration/connector.mdx) .connectionTimeoutMs OUTSIDE packages/spec 0 .requestTimeoutMs OUTSIDE packages/spec 0 .providerConfig OUTSIDE packages/spec 15 hits across 9 files (FIRING CONTROL) connector-mcp / connector-openapi / connector-rest providers, service-automation plugin.ts, app-showcase tests ``` **Second control, reachability.** A bare-identifier census in the same run proves the scan surface reaches the files where these keys actually live: `connectionTimeoutMs` occurs in 9 files outside `packages/spec` (the four connector packages, `plugin.ts`, a test) and `requestTimeoutMs` in 8 — every one of them a WRITE of the literal into a def so it satisfies the post-parse type, never a read. So the zeros read as "no reader", not "the probe never looked". **Host seam, re-read verbatim.** `packages/spec/src/integration/connector-provider.ts:57` declares `ConnectorProviderContext` with exactly `name`, `label`, `description?`, `icon?`, `type`, `providerConfig`, `auth?`, `loadPackageFile?`. None of the three keys is among them, so a provider factory is never handed them and has no way to honour them. "Left to the host" is false. **Changeset, measured rather than assumed.** The trigger is "can a consumer read a change", not "did bytes move". `npm pack --dry-run --json --ignore-scripts` on `packages/spec`: 275 entries, `docs/SYNC_ARCHITECTURE.md` **absent**, and **zero** `docs/` paths at all, while the positive controls `liveness/connector.json` and `src/integration/connector.zod.ts` are both present. `files[]` is `dist`, `json-schema`, `liveness`, `prompts`, `llms.txt`, `README.md`, `src/**/*.zod.ts`, `CHANGELOG.md`, `api-surface`, `spec-changes.json` — no `docs` entry. The edited file publishes to nobody, so `skip-changeset`. ## Verification - **Gate families.** `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 48 commands against commit `c4c09b853e`. All 48 were run, exit codes landed to disk first, then reconciled: *"48 derived, 48 run, 0 NOT-MEASURED, 0 UNRUN"* — a derived zero, every family carrying a recorded exit code. Four of them (`check:dts-closure`, `check:dual-build-cjs-loads`, `check:lean-entry-closure`, `check:sourcemap-no-sources-content`) first exited **3 = PREREQUISITE NOT MET**, which is neither a pass nor a failure; after `pnpm build` (73/73 tasks successful) all four re-ran green. - **DARK leg.** `pnpm --filter @objectstack/spec test` — 488 test files, 14182 tests, all pass. `pnpm --filter @objectstack/spec check:generated` — "All 15 generated artifacts are up to date". `pnpm --filter @objectstack/spec typecheck` — clean. - **This document sits behind a compile gate.** `packages/spec/src/integration/connector-author-shape.test.ts` extracts the file's typescript-fenced blocks and compiles them verbatim against the real schema, pinning the fence count at 2, the elision-sketch count at 0, and every block to compile clean. 14/14 pass with the edit in place. - **Reverse verification, direction stated before running.** Renaming the edited block's `retryConfig` key to `retryConfigBogus` had to turn that gate RED on the block I touched. Observed: RED, `TS2561 Object literal may only specify known properties` — TypeScript's did-you-mean form of the excess-property error, since the bogus name is one edit from the real one — on the assertion "L3 example #0 must compile clean"; 1 failed, 13 passed. Mutation landing was proved on disk (anchor 1 to 0, blob `bd3c6b895c2a` to `c02cf5c9c5e3`) and the restore was proved independently of the tool's own claim: blob back to `bd3c6b895c2a` equals HEAD, and `git diff HEAD` empty. This is what shows the comments added inside the fence are inside the compiled region and compile clean, rather than sitting outside it. - **Lint narrowing, declared.** (1) The population is read from eslint's own config: every `files:` selector in `eslint.config.mjs` targets `{ts,tsx,mts,cts,js,jsx,mjs,cjs}`, and no markdown selector or processor exists. (2) The count is read from `--format json`: the one changed file yields `"File ignored because no matching configuration was supplied."` with 0 errors, so 0 files of the lint population are touched. (3) Invariance: the file is outside the lint population entirely, so this diff cannot move any untouched file's verdict. ## One widening the reviewer should confirm The acceptance line for `:360` reads "it must not present an unimplemented key as a selection criterion". That row paired `retryConfig` with `health.circuitBreaker`, and the `:157` blockquote pairs them too. `health.circuitBreaker` is unimplemented on the same evidence: `packages/spec/liveness/connector.json` records every one of its sub-keys as `dead`, and the same probe run shows 0 read-shaped consumers outside `packages/spec` in any `packages/` or `examples/` file (its only hits are the generated reference page and `content/docs/references/system/cache.mdx`, which is the CACHE's own breaker, a different subject). Correcting only the `retryConfig` half would have left the row still presenting an unimplemented key as a selection criterion. So both halves are corrected, citing the ledger's existing verdict rather than making a new one. Flagged because it is one key wider than the card's three. ## Acceptance notes Noted, not filed. The first two are real sites carrying the same prescription, each behind a read-only fence this round: - `content/docs/automation/flows.mdx` (now `:1595`; the card said `:1588`) is held by open PR #18420 and is untouched here. Its passage is about reconciling retry-COUNT conventions between block kinds (`maxAttempts` includes the first attempt), not the "use `retryConfig` for upstream rate limiting" prescription, so excluding it does not damage the card's thesis. Follow-up: sweep it once PR #18420 lands. Successor: the seat that picks up that sweep. - `packages/spec/src/integration/connector.zod.ts:44` carries the **same sentence verbatim** ("What L3 does declare for a rate-limited upstream is `retryConfig` — whose `retryableStatusCodes` default ... includes `429`"), and `content/docs/references/integration/connector.mdx:42` is that same comment regenerated by `packages/spec/scripts/build-docs.ts`. Both are behind this round's fence on `packages/spec/src/integration/**`. So after this PR the repo still teaches the falsehood in those two places, from one source. Follow-up: correct that TSDoc block and regenerate, which is a prose-only change plus `gen:docs`. Successor: whoever takes the ADR-0049 ruling card, since it lands in the same file. - `SYNC_ARCHITECTURE.md:192` also names `connectionTimeoutMs` / `requestTimeoutMs`, but only to state that keys carrying a `.default()` are optional in the author shape — a true statement about `z.input` that makes no efficacy claim. Left alone deliberately. - The Best Practices bullet "**Error Handling**: Implement comprehensive retry logic with exponential backoff" is left as-is: under this change it reads correctly as advice to IMPLEMENT retry yourself, which is now the only true reading. _Generated by [Claude Code](https://claude.ai/code/session_01JbZnqu8bt6YqfJsr9vaFb3)_ --- _Generated by [Claude Code](https://claude.ai/code/session_01JbZnqu8bt6YqfJsr9vaFb3)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent d8b12fc commit f347c79

1 file changed

Lines changed: 35 additions & 7 deletions

File tree

‎packages/spec/docs/SYNC_ARCHITECTURE.md‎

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -153,9 +153,25 @@ Complete, production-grade integration with external systems. Includes authentic
153153
> nothing throttles the calls a connector makes *out*. Do **not** substitute
154154
> `shared`'s `RateLimitConfig` — that is the inbound limiter and would cap the
155155
> wrong direction. **Until an outbound throttle exists, rate-limit at the
156-
> connector provider or upstream gateway.** What L3 does declare for a
157-
> rate-limited upstream is `retryConfig` — whose `retryableStatusCodes` default
158-
> `[408, 429, 500, 502, 503, 504]` includes `429` — and `health.circuitBreaker`.
156+
> connector provider or upstream gateway.** **And do not reach for
157+
> `retryConfig` instead.** This paragraph used to end "what L3 does declare for
158+
> a rate-limited upstream is `retryConfig` — whose `retryableStatusCodes`
159+
> default `[408, 429, 500, 502, 503, 504]` includes `429` — and
160+
> `health.circuitBreaker`", which reads as a remedy. It is not one: both keys
161+
> are **declared but currently unimplemented**.
162+
> `packages/spec/liveness/connector.json` records every `retryConfig` sub-key
163+
> and every `health.circuitBreaker` sub-key as `dead`, and outside
164+
> `packages/spec` nothing reads either — no retry loop consumes a strategy, a
165+
> backoff, a jitter or that status-code list, so the `429` in it never causes a
166+
> retry, and no breaker ever opens. They are **not retired**: both are still
167+
> declared and still parse, so an author can write them and see no error. They
168+
> are **not left to the host** either — `ConnectorProviderContext`
169+
> (`src/integration/connector-provider.ts`) carries exactly `name`, `label`,
170+
> `description`, `icon`, `type`, `providerConfig`, `auth` and
171+
> `loadPackageFile`, so a provider factory is never handed either key and has
172+
> no way to honour it. ADR-0049 owes these keys a decision (retire / implement /
173+
> declare as a host contract); until it rules, the advice above is the whole
174+
> advice — retry and throttle **at the connector provider or upstream gateway**.
159175
160176
> **Field mapping does not transform values.** The ticked line above used to read
161177
> "With transformations and data type conversion". Only the second half was ever
@@ -297,7 +313,12 @@ const sapConnector: Connector = {
297313
// (`rateLimitConfig` sat here until #4911 retired it — no outbound
298314
// rate-limiting engine ever existed. Throttle at the provider/gateway.)
299315

300-
// Retry Configuration — for the connector's own outbound requests
316+
// Retry Configuration — DECLARED BUT CURRENTLY UNIMPLEMENTED. The block
317+
// below parses and is stored, and nothing reads it: no retry loop exists, so
318+
// the `retryableStatusCodes` list — 429 included — never causes a retry.
319+
// Every sub-key is recorded `dead` in `packages/spec/liveness/connector.json`,
320+
// and ADR-0049 owes it a decision. It is shown because this example is a tour
321+
// of the surface, not because authoring it buys behaviour.
301322
retryConfig: {
302323
strategy: 'exponential_backoff',
303324
maxAttempts: 5,
@@ -309,6 +330,10 @@ const sapConnector: Connector = {
309330
jitter: true
310331
},
311332

333+
// Also declared but currently unimplemented, and `dead` in the same
334+
// ledger (`packages/spec/liveness/connector.json`):
335+
// both timeouts parse and default, and no fetch, transport or handler reads
336+
// either, so a connector call is unbounded whatever is written here.
312337
connectionTimeoutMs: 30000,
313338
requestTimeoutMs: 60000,
314339
status: 'active',
@@ -333,8 +358,11 @@ const sapConnector: Connector = {
333358
- **Security First**: Always use encrypted credentials and secure storage
334359
- **Rate Limiting**: Respect the upstream API's limits — and enforce that at the
335360
connector provider or upstream gateway, since the connector shape declares no
336-
outbound throttle (#4911). `retryConfig` handles the `429` you get for exceeding a
337-
limit; it does not keep you under one
361+
outbound throttle (#4911). This bullet used to add that `retryConfig` handles
362+
the `429` you get for exceeding a limit. It does not: `retryConfig` is
363+
declared but currently unimplemented — every sub-key is `dead` in
364+
`packages/spec/liveness/connector.json` — so nothing retries that `429`
365+
either. Both the throttling and the retrying are the provider's to implement
338366
- **Error Handling**: Implement comprehensive retry logic with exponential backoff
339367
- **Monitoring**: Set up health checks and alerting for connector failures
340368
- **Testing**: Test authentication, sync, and webhook flows thoroughly
@@ -357,7 +385,7 @@ mostly answers "which surface", and — for the two questions that used to route
357385
| Do you need multi-source aggregation? | **Same answer**, and for the same reason — see [Retired: L2 ETL Pipeline](#retired-l2-etl-pipeline-v17) |
358386
| Do you need real-time webhooks? | **Yes** → L3 (Connector) |
359387
| Do you need advanced authentication (OAuth2, SAML)? | **Yes** → L3 (Connector) |
360-
| Do you need retry policies and circuit breaking? | **Yes** → L3 (Connector) — `retryConfig`, `health.circuitBreaker`. Outbound **rate limiting** is not a reason to pick any level: no level provides it (#4911); throttle at the provider or gateway |
388+
| Do you need retry policies and circuit breaking? | **Not a reason to pick a level.** L3 *declares* `retryConfig` and `health.circuitBreaker`, but both are **declared but currently unimplemented** — every sub-key of each is `dead` in `packages/spec/liveness/connector.json`, nothing outside `packages/spec` reads either, and ADR-0049 owes them a decision — so neither is a capability you can select for. Implement retry and circuit breaking in the connector provider. Outbound **rate limiting** is not a reason to pick any level: no level provides it (#4911); throttle at the provider or gateway |
361389
| Is it a simple point-to-point sync with an external system? | **Yes** → L3 (Connector) with `syncConfig` |
362390
| Are you building a data warehouse pipeline? | The extraction half is L3 (`syncConfig`); the warehouse-side transformation is the warehouse's own tooling. There is no ObjectStack pipeline protocol (#6414) |
363391
| Are you integrating with an enterprise system? | **Yes** → L3 (Connector) |

0 commit comments

Comments
 (0)