From 3192a2932c356f4308fe798036c1d578494b3d65 Mon Sep 17 00:00:00 2001 From: Marco Moauro Date: Thu, 3 Sep 2026 15:27:31 +0200 Subject: [PATCH] Fix export_subscribers aborting on the export's pending poll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `export_subscribers` failed on every call, with any arguments (#25). The cause is the poll, not the query. While Substack generates the file, `GET /subscriber_set/export/` answers `400 {"error":"Export not ready"}` — not a 200 with an absent `url`, which is what this repo believed and what the suite mocked. Since the tool's first poll is immediate and generation takes ~3s even for 6 rows, every export hit that 400 and `readBody` turned it into a fatal error. Measured live 2026-09-03, polling a fresh 6-row export at 250ms: +0.17s 400 Export not ready ... 400 Export not ready (7 polls) +2.71s 400 Export not ready +3.29s 200 {"url":"…/file"} A 200 without a `url` was never observed at any point. `getSubscriberSetExport` now translates that one body into `{pending: true}` and rethrows every other 400 — an export that will never arrive has to fail now rather than poll out the caller's wait budget. The tool needed no change: `{pending: true}` carries no `url`, so the existing loop, backoff, budget and timeout message work unchanged, and an absent `url` still means retry in case that state ever exists. `readBody` also attaches `status` and `body` to the error it throws. The message carried only the status, so Substack's own explanation of a refusal was readable in the log and nowhere else, and no caller could tell one 400 from another — which is why this presented as a bare `400 Bad Request` naming none of the four steps. Note what hid it: the pending state was mocked as `200 {}`, so 748 tests stayed green while the tool failed 100% of the time in production. The mock described the wrong universe, so no assertion over it could have failed. CLAUDE.md gains the two rules that follow — a fixture representing an API state must cite where that state was observed, and a time-dependent flow is verified by running the real handler, never by hand, because curl-by-curl runs at human pace and cannot enter a 3-second window. Verified live end to end after the fix: two pending polls, then the CSV. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 41 ++++++++++++++++++++++++-- src/api/substack/SubstackApi.js | 43 ++++++++++++++++++++++------ src/api/substack/SubstackApi.spec.js | 39 +++++++++++++++++++++++++ src/tools/export_subscribers.spec.js | 29 +++++++++++++++++++ 4 files changed, 142 insertions(+), 10 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6aeb0dc..0175e09 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -119,8 +119,24 @@ GET → ``` Verified end to end. The dashboard's polling backoff is `1, 5, 10, 30`, then `60` repeated; -`EXPORT_POLL_BACKOFF_SECONDS` mirrors it, except the first poll happens immediately because a small -export is usually already done. Four things this flow gets wrong if taken at face value: +`EXPORT_POLL_BACKOFF_SECONDS` mirrors it, and the first poll happens immediately — which is free +now but was not always: see the pending state below. Five things this flow gets wrong if taken at +face value: + +- **The pending poll is a `400`, not a 200 with an absent `url`.** While the file is generating, + `GET /subscriber_set/export/` answers `400 {"error":"Export not ready","type":"single"}`; + a 200 without a `url` was **never observed at any point**. Measured 2026-09-03 at 250ms + intervals: ~3s of 400s on a *6-row* export, then `200 {"url": "…/file"}`. This file previously + claimed the opposite, and the cost of that was total: since the first poll is immediate, every + export aborted on its first request with a bare `400 Bad Request` (issue #25). Note what made it + invisible — the suite mocked the pending state as `200 {}`, so 748 tests stayed green while the + tool failed 100% of the time in production. **A mock of a state nobody has observed is an + assumption wearing a test's clothes.** `SubstackApi.getSubscriberSetExport` now translates that + one body into `{pending: true}` and rethrows every other 400, so the tool's loop is unchanged — + an absent `url` still means retry, which costs nothing and covers the state if it ever exists. + This is also why `readBody` attaches `status` and `body` to the error it throws: the message + carries only the status, so the body explaining the refusal was readable in the log and nowhere + else, and no caller could tell one 400 from another. - **The CSV header carries human LABELS, not column keys** — `Emails opened (6mo)`, not `num_email_opens`. `COLUMN_KEY_BY_LABEL` is the reverse map; it works because the labels are @@ -554,6 +570,27 @@ a logging assertion. **Check the mutation actually landed** before trusting a gr that asserts nothing. Grep the file for a marker first. The whole suite runs in well under a second, so a mutation costs nothing. +**A fixture that represents an API *state* must cite where that state was observed** — a date, a +captured log line — and anything not observed has to say so. The mocks are written by us, so the +suite measures this repo's model of Substack and never Substack; a bug of the form "the API does +something we have not seen" is *structurally invisible* to it, however thorough it is. Issue #25 +is the demonstration: the export's pending state was mocked as `200 {}`, the real state is a +`400`, and no assertion over that mock could have failed because it described the wrong universe. +Note which mocks are exposed. A happy-path fixture is corrected by real use sooner or later; a +fixture for a **transient or error** state is corrected by nobody, because nothing exercises it +until something exercises it constantly. Suspect those first. + +**Verify a time-dependent flow by running the real handler, never by hand.** Polling, retry, +backoff, TTL and expiry all mean the code and a human occupy different timing regimes: hand-issued +requests land seconds apart, `export_subscribers` polls 200ms after asking for the file, and the +window it lands in is ~3s wide. A manual walk through the endpoints cannot enter that window and +the code cannot avoid it, so curl-by-curl "verified end to end" is not a verification of the same +program. The technique that works is already in this file — the token check drove +`create_draft_post` → `update_draft` → `get_draft` → `delete_draft` **through the real handlers** +with `SUBSTACK_MCP_LOG_LEVEL=debug` and read the log. Do that once per tool whose flow is +multi-step or timed, and keep the log: it is the only check that can contradict an assumption +rather than confirm it. + ## Style Two-space indent, semicolons, single quotes in code (imports use double), compact object diff --git a/src/api/substack/SubstackApi.js b/src/api/substack/SubstackApi.js index 2158feb..635e500 100644 --- a/src/api/substack/SubstackApi.js +++ b/src/api/substack/SubstackApi.js @@ -42,7 +42,15 @@ export default class SubstackApi { body, }); - throw new Error(`SubstackAPIException: ${response.status} ${response.statusText}`); + // The message carries only the status, which is all a caller ever saw: issue #25 surfaced as + // a bare "400 Bad Request" that named neither the failing step nor Substack's own + // explanation. The status and body ride along so a caller can tell one 400 from another + // without re-reading the log. + const error = new Error(`SubstackAPIException: ${response.status} ${response.statusText}`); + error.status = response.status; + error.body = body; + + throw error; } return response.text(); @@ -504,15 +512,34 @@ export default class SubstackApi { } /** - * Polls one export. Answers `{url}` once the file is ready; the url is absent while it is still - * being generated. + * Polls one export. Answers `{url}` once the file is ready and `{pending: true}` while Substack + * is still generating it. + * + * **The pending state is a 400, not a 200 with an absent url**, which is the opposite of what + * every other endpoint here does and is why this method exists rather than the caller polling + * `request` directly. Measured live 2026-09-03 on a 6-row export: ~3s of + * `400 {"error":"Export not ready","type":"single"}`, then `200 {url}`. A 200 without a url was + * never observed at any point — the earlier reading of this endpoint had it backwards, and since + * the tool's first poll is immediate, every export aborted on its first request (issue #25). + * + * Only that one body is a state. Any other refusal still throws: an export that will never + * arrive has to fail now rather than poll until the caller's wait budget runs out. */ async getSubscriberSetExport(export_id) { - return this.request({ - method: 'GET', - path: `/subscriber_set/export/${export_id}`, - referer: '/publish/subscribers', - }); + try { + return await this.request({ + method: 'GET', + path: `/subscriber_set/export/${export_id}`, + referer: '/publish/subscribers', + }); + } catch (error) { + if (error.status === 400 && /Export not ready/i.test(error.body ?? '')) { + logger.debug('substack.export.pending', {export_id}); + return {pending: true}; + } + + throw error; + } } /** diff --git a/src/api/substack/SubstackApi.spec.js b/src/api/substack/SubstackApi.spec.js index 383fda7..5ee7f30 100644 --- a/src/api/substack/SubstackApi.spec.js +++ b/src/api/substack/SubstackApi.spec.js @@ -328,6 +328,45 @@ describe('SubstackApi — subscriber set and export', () => { assert.equal(new URL(msw.requests[0].url).pathname, `/api/v1/subscriber_set/export/${EXPORT_ID}`); }); + // Measured live 2026-09-03: while Substack is still generating the file the poll answers + // 400 {"error":"Export not ready"} — NOT a 200 with an absent url. It stayed there for ~3s on a + // 6-row export. Reported as issue #25, where it aborted every single export. + test('getSubscriberSetExport reports a not-ready export as pending instead of throwing', async () => { + msw.server.use(msw.exportStatusHandler(() => + HttpResponse.json({error: 'Export not ready', type: 'single'}, {status: 400}) + )); + + const result = await createApi().getSubscriberSetExport(EXPORT_ID); + + assert.deepEqual(result, {pending: true}); + }); + + // Only that one 400 is a state; every other refusal is still a refusal. Swallowing them all + // would turn a rejected set id into an export that polls until the wait budget runs out. + test('getSubscriberSetExport still throws on a 400 that is not the pending state', async () => { + msw.server.use(msw.exportStatusHandler(() => + HttpResponse.json({error: 'Subscriber set not found', type: 'single'}, {status: 400}) + )); + + const error = await createApi().getSubscriberSetExport(EXPORT_ID).catch((e) => e); + + assert.match(error.message, /^SubstackAPIException: 400\b/); + }); + + // The thrown message carries only the status, so without these the body Substack sent to explain + // the refusal is readable in the log and nowhere else — which is what made #25 present as a bare + // "400 Bad Request" with no hint of which of the four steps failed. + test('a failing status attaches its status and body to the error', async () => { + msw.server.use(msw.exportStatusHandler(() => + HttpResponse.json({error: 'Export not ready', type: 'single'}, {status: 418}) + )); + + const error = await createApi().getSubscriberSetExport(EXPORT_ID).catch((e) => e); + + assert.equal(error.status, 418); + assert.match(error.body, /Export not ready/); + }); + // The export answers with a relative url, so it has to be resolved against the publication host // rather than concatenated onto publication_url — which already ends in /api/v1 and would produce // /api/v1/api/v1/... diff --git a/src/tools/export_subscribers.spec.js b/src/tools/export_subscribers.spec.js index 972f35b..3bee63b 100644 --- a/src/tools/export_subscribers.spec.js +++ b/src/tools/export_subscribers.spec.js @@ -260,6 +260,35 @@ describe('exportSubscribersHandler — polling', () => { assert.deepEqual(clock.slept, [EXPORT_POLL_BACKOFF_SECONDS[0], EXPORT_POLL_BACKOFF_SECONDS[1]]); }); + // Issue #25: the live pending state is a 400, and the first poll is immediate, so every export + // hit it and aborted before the file was ever ready. Verified against the real API on + // 2026-09-03: ~3s of 400 {"error":"Export not ready"}, then 200 {url}. + test('keeps polling through the 400 the live API answers while the file is generating', async () => { + msw.server.use(msw.exportStatusHandler((exportId, attempt) => + attempt < 3 + ? HttpResponse.json({error: 'Export not ready', type: 'single'}, {status: 400}) + : HttpResponse.json({url: EXPORT_FILE_PATH}, {status: 200}) + )); + + const clock = fakeClock(); + const result = await run({}, clock); + + assert.equal(result.count, 2); + assert.deepEqual(clock.slept, [EXPORT_POLL_BACKOFF_SECONDS[0], EXPORT_POLL_BACKOFF_SECONDS[1]]); + }); + + // A 400 that is not the pending state means the export will never arrive, so polling it to the + // end of the wait budget would replace a clear refusal with a slow timeout. + test('aborts on a 400 that is not the pending state', async () => { + msw.server.use(msw.exportStatusHandler(() => + HttpResponse.json({error: 'Subscriber set not found', type: 'single'}, {status: 400}) + )); + + const error = await run({}, fakeClock()).catch((e) => e); + + assert.match(error.message, /^SubstackAPIException: 400\b/); + }); + test('follows the documented backoff rather than a fixed interval', async () => { assert.deepEqual(EXPORT_POLL_BACKOFF_SECONDS.slice(0, 4), [1, 5, 10, 30]); assert.equal(EXPORT_POLL_BACKOFF_SECONDS.at(-1), 60);