diff --git a/changelog.d/notify-webhook-duplicate.fixed.md b/changelog.d/notify-webhook-duplicate.fixed.md new file mode 100644 index 000000000..c83ca7136 --- /dev/null +++ b/changelog.d/notify-webhook-duplicate.fixed.md @@ -0,0 +1 @@ +A Feishu webhook message whose answer was lost is no longer posted a second time: a timed-out or aborted send gets one attempt and says the message may already be in the chat, instead of claiming Feishu refused it ([#2660](https://github.com/ThinkFlowLab/FlyBot/issues/2660)). diff --git a/core/src/host/connectors.js b/core/src/host/connectors.js index 097dfe729..32eecde39 100644 --- a/core/src/host/connectors.js +++ b/core/src/host/connectors.js @@ -275,6 +275,25 @@ function withDeadline(promise, ms) { return Promise.race([promise, deadline]).finally(() => clearTimeout(timer)) } +// A request that never reached Feishu wrote nothing, so notify.js may safely send it again. A +// request that timed out, was aborted, or failed in a way we cannot read may already be in the +// chat, and a custom-bot webhook has no idempotency field: repeating it would post it twice. +// `feishu/no-answer` is deliberately absent from notify.js's `RETRYABLE`, so it gets one attempt. +const UNSENT_CODES = new Set(['ECONNREFUSED', 'ENOTFOUND', 'EAI_AGAIN']) +const NO_ANSWER = 'feishu/no-answer' + +function connectionNeverOpened(error) { + const seen = new Set() + const inspect = cause => { + if (!cause || typeof cause !== 'object' || seen.has(cause)) return false + seen.add(cause) + if (UNSENT_CODES.has(String(cause.code))) return true + if (inspect(cause.cause)) return true + return Array.isArray(cause.errors) && cause.errors.some(inspect) + } + return inspect(error) +} + // One text message through a custom-bot webhook, answered in notify.js's vocabulary: `{ ok, ts }` // or `{ ok: false, error }`, where `rate_limited` and `service_unavailable` may pass on their own // and every other error is a shape Feishu will refuse again. The URL is never part of the answer. @@ -288,8 +307,9 @@ export async function feishuPost(webhook, text, { fetch: request = fetch, timeou redirect: 'error', signal: AbortSignal.timeout(Math.max(1, timeoutMs)), }), timeoutMs) - } catch { - return { ok: false, error: 'network' } + } catch (error) { + // Only a connection that never opened is retried; anything else is an unconfirmed delivery. + return connectionNeverOpened(error) ? { ok: false, error: 'network' } : { ok: false, error: NO_ANSWER } } const body = await response.json().catch(() => ({})) if (response.status === 429) { @@ -311,6 +331,7 @@ export async function feishuPost(webhook, text, { fetch: request = fetch, timeou export async function feishuCheck(webhook, { api = FEISHU_DEFAULTS.api, fetch: request = fetch, timeoutMs = CHECK_TIMEOUT_MS } = {}) { const answer = await feishuPost(webhookAddress(webhook, api), FEISHU_TEST_TEXT, { fetch: request, timeoutMs }) if (answer.ok) return {} + if (answer.error === NO_ANSWER) throw new Error('Feishu did not answer in time — the test message may already be in the chat; check before connecting again') if (answer.error === 'network' || answer.error === 'service_unavailable') throw new Error('Could not reach Feishu') if (answer.error === 'rate_limited') throw new Error('Feishu is rate-limiting this webhook; try again in a minute') throw new Error(`Feishu did not accept the webhook (${answer.error})`) @@ -1018,7 +1039,9 @@ export function install(rt) { // A fresh key per call: two messages with the same text are two messages, not a duplicate. const result = await notifier.send({ key: randomUUID(), text }) const sent = result.state === 'sent' || result.state === 'duplicate' - const failure = `Feishu refused the message (${result.error ?? result.reason ?? 'no answer'})` + const failure = result.error === NO_ANSWER + ? 'Feishu did not answer in time — the message may already be in the chat; check before sending again' + : `Feishu refused the message (${result.error ?? result.reason ?? 'no answer'})` await keep(c, { ...saved(c), ...(sent ? { lastOkAt: Date.now() } : { lastError: failure }) }) if (!sent) throw new Error(failure) return connectorsView() diff --git a/core/src/host/notify.js b/core/src/host/notify.js index 6784f77d7..12ab00e42 100644 --- a/core/src/host/notify.js +++ b/core/src/host/notify.js @@ -1,10 +1,11 @@ // Out-of-app delivery, first half (#355): the adapter that will carry a reminder to Slack. // A reminder has to reach the owner when the desktop app is closed, so delivery cannot be a // client toast. The transport is injected; what this module owns is the policy around it: one -// idempotency key per reminder, so a retried send cannot arrive twice; exponential backoff that -// honours `Retry-After`; three attempts at most; no retry at all for a failure that will not -// change (the channel is gone, the app was removed). A channel switched off makes zero transport -// calls, including one that is switched off while it waits to retry. +// idempotency key per reminder, deduplicated inside this process only, and a send that may already +// have left the process is never repeated, because the outbound transport cannot dedupe it; +// exponential backoff that honours `Retry-After`; three attempts at most; no retry at all for a +// failure that will not change (the channel is gone, the app was removed). A channel switched off +// makes zero transport calls, including one that is switched off while it waits to retry. // // The real Slack transport waits for the owner's credential, so it is not here: the module speaks // Slack's `chat.postMessage` shape and ships a fake transport that records every call and scripts diff --git a/core/test/feishu.test.js b/core/test/feishu.test.js index c0662ce91..25c2cbdc1 100644 --- a/core/test/feishu.test.js +++ b/core/test/feishu.test.js @@ -142,7 +142,8 @@ test('#1263: a webhook Feishu refuses stays on the row as an error that names no test('#1263: a Feishu that never answers fails on the deadline instead of hanging', async () => { const started = Date.now() const answerOrNothing = await feishuPost(webhookOf('x'), 'hello', { fetch: () => new Promise(() => {}), timeoutMs: 25 }) - assert.deepEqual(answerOrNothing, { ok: false, error: 'network' }) + // #2660: the POST may already be in the chat, so it is an unconfirmed delivery, not a retryable link. + assert.deepEqual(answerOrNothing, { ok: false, error: 'feishu/no-answer' }) assert.ok(Date.now() - started < 2_000, 'the deadline, not the fetch, ends the wait') }) diff --git a/core/test/notify-webhook-once.test.js b/core/test/notify-webhook-once.test.js new file mode 100644 index 000000000..0014e76e9 --- /dev/null +++ b/core/test/notify-webhook-once.test.js @@ -0,0 +1,84 @@ +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { feishuCheck, feishuTransport } from '../src/host/connectors.js' +import { createNotifier } from '../src/host/notify.js' + +// #2660: a Feishu custom-bot webhook has no idempotency field, so a POST whose answer was lost +// cannot be sent again: the receiver would show the same message twice. Notify.js retries +// `network`, which is right for a connection that never opened and wrong for a request that may +// already have been written — the transport has to tell those two apart. + +const WEBHOOK = 'https://open.feishu.cn/open-apis/bot/v2/hook/unit-test' +const ok = data => ({ status: 200, ok: true, headers: { get: () => null }, json: async () => ({ code: 0, data }) }) +const status = (code, headers = {}) => ({ + status: code, + ok: code >= 200 && code < 300, + headers: { get: name => headers[String(name).toLowerCase()] ?? null }, + json: async () => ({}), +}) +const notifier = (fetch, extra = {}) => createNotifier({ + channel: 'feishu', + transport: feishuTransport(WEBHOOK, { fetch, timeoutMs: 40 }), + log: () => {}, + ...extra, +}) + +test('#2660: a POST whose answer was lost is not sent a second time', async () => { + const posts = [] + const fetch = async (url, init) => { + posts.push(JSON.parse(init.body)) + return new Promise(() => {}) // accepted, but the answer never comes back + } + + const result = await notifier(fetch).send({ key: 'k1', text: 'standup in 5 minutes' }) + + assert.equal(posts.length, 1, 'one send makes exactly one POST when the answer is lost') + assert.deepEqual(posts[0], { msg_type: 'text', content: { text: 'standup in 5 minutes' } }) + assert.deepEqual(result, { state: 'failed', error: 'feishu/no-answer', attempts: 1, permanent: true }) + assert.equal(result.error, 'feishu/no-answer', 'the error names the unconfirmed delivery') +}) + +test('#2660: a connection that never opened still retries three times', async () => { + let calls = 0 + const fetch = async () => { + calls += 1 + const cause = Object.assign(new Error('connect ECONNREFUSED'), { code: 'ECONNREFUSED' }) + throw new TypeError('fetch failed', { cause }) + } + + const result = await notifier(fetch, { delay: async () => {} }).send({ key: 'k2', text: 'hello' }) + + assert.equal(calls, 3, 'a refused connection wrote nothing, so the three attempts stay') + assert.deepEqual(result, { state: 'failed', error: 'network', attempts: 3, permanent: false }) +}) + +test('#2660: a 429 still retries and waits what Retry-After asks', async () => { + let calls = 0 + const waits = [] + const fetch = async () => { + calls += 1 + return calls === 1 ? status(429, { 'retry-after': '2' }) : ok({ message_id: 'om_1' }) + } + + const result = await notifier(fetch, { delay: async ms => { waits.push(ms) } }).send({ key: 'k3', text: 'hello' }) + + assert.equal(result.state, 'sent') + assert.equal(result.attempts, 2, 'a rate limit is transient: the same call is tried again') + assert.ok(waits[0] >= 2_000, 'Retry-After is a floor, not a suggestion') +}) + +test('#2660: the connect sentence says the test message may already be posted', async () => { + let calls = 0 + const fetch = async () => { calls += 1; return new Promise(() => {}) } + + await assert.rejects( + feishuCheck(WEBHOOK, { fetch, timeoutMs: 40 }), + error => { + assert.equal(calls, 1, 'the probe is one POST, never a retry') + assert.match(error.message, /did not answer in time/i) + assert.match(error.message, /may already be in the chat/i) + assert.ok(!error.message.includes('open.feishu.cn'), 'the sentence never carries the webhook URL') + return true + }, + ) +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 6ab0cb245..5f62801cb 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -481,7 +481,7 @@ A group room takes the same 60s line, through the room's own path (#1650). A per `src/host/connectors.js` mounts one MCP server per connector through DSH's own MCP client, so a Bot's tools are `mcp__github__*` and `mcp__composio__*` **for the owner's own use of every Bot, and for other people's use of a Bot only when the owner allowed it** (per-Bot access, below). Composio is a gateway: one API key (`COMPOSIO_API_KEY`) and a list of apps the user picked; the host asks Composio's tool-router for a session limited to those apps (workbench off) and mounts the session's MCP address. The session is made once per connect or app-list change and kept as `state.connectors.composio.session` (`{ id, apps }`: never the key and, since #335, never the session's address): the key check, the mount and Try again reuse it while the apps are the same and `GET /tool_router/session/{id}` (`x-api-key`) still finds it, else a new one is made. The mount's address is the `mcp.url` that answer carries (Composio's reference lists it as required in both the create and the get answer), taken only when it is https on the API's own host and held in memory for that mount; a session whose answer names no usable address is treated as gone. Why the address is not stored (#335): Composio's docs show the session address always used together with the key header and say to treat the exported address-and-headers config as a secret, but they do not say the address alone cannot authenticate, so it is not kept in `state.json` (backups, exports and anything that reads the profile) and is rebuilt from the id and the key at each mount. A record an older version wrote with `url` is rewritten without it at start (`scrubAddresses`), and the in-memory address is also taken out of a connector error (`[session address]`) before it is shown or logged. A superseded session (the old one after Choose apps or a new key, the new one when saving it failed, the kept one on Disconnect) is deleted with `DELETE /tool_router/session/{id}`, using the key it was made with; the delete is best effort and never an error for the user, and stopping the host leaves the session for the next start. Composio asks for each app's sign-in the first time a Bot needs it, in the chat. A stable `flybot-` user id kept in `state.connectors.composio` (kept across Disconnect) is what Composio ties signed-in apps to. Rules: the key is a secret kept for all of the owner's Bots like any key the owner saves (#545; until then a connector saved it with scope `[]`, no Bot's shell), sent only as the `x-api-key` MCP header and to the API address, never written to `state.json`, a profile or an error (errors go through `redactSecrets`); the API address must be https, the answer's MCP address must be https on the API's own host, and the session request does not follow redirects; the key is proved by creating the session before it is saved; `connector-apps` changes the app list without the key and keeps the old list when Composio refuses the new one; `connector-app-search { id, query, token? }` searches Composio's app list (`GET /toolkits?search=&sort_by=usage&limit=20`, `x-api-key`, no redirects) with the pasted `token` when given (not saved) or the saved key, and returns only `{ slug, name }` with slugs a session can take; `connector-app-status { id }` reads the kept session's apps (`GET /tool_router/session/{id}/toolkits?toolkits=…`) and returns `{ [slug]: 'ready' | 'sign-in' }`, ready when the app needs no sign-in or has an `ACTIVE` connected account (#359); `connector-*` writes are `SHARED_BLOCKED`; `github` and `composio` are reserved capability names; `composio: false` hides the connector. Which Composio tools run without asking is DSH's normal tool approval, not a FlyBot gate (a per-app approval is a later step). -Feishu (Lark) started as the first **sender** connector (#1263), and its webhook mode still is one: a group's custom-bot webhook is one HTTPS POST into the instance's own group, so nothing is mounted, no Bot's tool list changes, and `connector-access.js`'s `CONNECTOR_IDS` does not list it. The definition is a channel today, not a sender (#1266, below): the view reports the kind and the client reads it instead of hard-coding an id, and the channel kind is what makes the row offer Choose Bots and `reconcile()` open the socket, while webhook mode is the same row with nothing to dial. A webhook stops at the status: a saved webhook reads `ready` without posting, because the test POST belongs to the **connect** step, so a restart never messages the group; `connector-retry` ("Try again") is the one path that proves it again. The webhook is `FEISHU_WEBHOOK`, saved through `secret-set` **before** the probe — so a refusal stays on the row as `error` with the reason and `redactSecrets` can mask it, the way a failed mount does for GitHub — and it must be https on the configured `feishu.api` origin, else `A Feishu webhook must be https on `: the URL is the credential, so it may never be posted to another host and never appears in `state.json`, the view or a warn line. `connector-send { id, text }` posts one message on demand and records `lastOkAt` / `lastError` in `state.connectors.feishu`; nothing posts room traffic by itself. Feishu answers HTTP 200 with the verdict in the body (`code: 0` success, `9499` a rate limit, `19021` a refused signature), so `feishuPost` maps the body into notify.js's vocabulary and the retry policy stays `notify.js`'s `createNotifier` (3 attempts, backoff, honour `retry-after`, structural errors not retried) rather than a second one written here; `connector-send` uses a fresh idempotency key per call, so two messages with the same text are two messages. The connect probe and a send both carry a deadline (15 s and 60 s) and `withDeadline` races the fetch, so a fetch that ignores its abort signal cannot hang either. `feishu: false` hides the row; `connector-send` is in `SHARED_BLOCKED` like the other connector writes. +Feishu (Lark) started as the first **sender** connector (#1263), and its webhook mode still is one: a group's custom-bot webhook is one HTTPS POST into the instance's own group, so nothing is mounted, no Bot's tool list changes, and `connector-access.js`'s `CONNECTOR_IDS` does not list it. The definition is a channel today, not a sender (#1266, below): the view reports the kind and the client reads it instead of hard-coding an id, and the channel kind is what makes the row offer Choose Bots and `reconcile()` open the socket, while webhook mode is the same row with nothing to dial. A webhook stops at the status: a saved webhook reads `ready` without posting, because the test POST belongs to the **connect** step, so a restart never messages the group; `connector-retry` ("Try again") is the one path that proves it again. The webhook is `FEISHU_WEBHOOK`, saved through `secret-set` **before** the probe — so a refusal stays on the row as `error` with the reason and `redactSecrets` can mask it, the way a failed mount does for GitHub — and it must be https on the configured `feishu.api` origin, else `A Feishu webhook must be https on `: the URL is the credential, so it may never be posted to another host and never appears in `state.json`, the view or a warn line. `connector-send { id, text }` posts one message on demand and records `lastOkAt` / `lastError` in `state.connectors.feishu`; nothing posts room traffic by itself. Feishu answers HTTP 200 with the verdict in the body (`code: 0` success, `9499` a rate limit, `19021` a refused signature), so `feishuPost` maps the body into notify.js's vocabulary and the retry policy stays `notify.js`'s `createNotifier` (3 attempts, backoff, honour `retry-after`, structural errors not retried) rather than a second one written here; `connector-send` uses a fresh idempotency key per call, so two messages with the same text are two messages. The connect probe and a send both carry a deadline (15 s and 60 s) and `withDeadline` races the fetch, so a fetch that ignores its abort signal cannot hang either. A POST that ran out of time, was aborted, or failed in a way the code cannot read is an **unconfirmed delivery** (#2660): it may already be in the chat, and a custom-bot webhook has no idempotency field, so `feishuPost` answers `feishu/no-answer` — deliberately absent from `notify.js`'s `RETRYABLE` — and the message gets exactly one attempt; the row then says the message may already have been posted instead of claiming Feishu refused it. Only a connection that never opened (`ECONNREFUSED` / `ENOTFOUND` / `EAI_AGAIN`) is still retried to three attempts, and 429/5xx keep their retry. `feishu: false` hides the row; `connector-send` is in `SHARED_BLOCKED` like the other connector writes. The same row carries a Feishu message coming **in** (#1266): `inbound` reports `off|connecting|ready|error`, the last event, the last error and whether the app pair is `set` or `missing` — never a value. `src/host/feishu-inbound.js` owns the policy: off unless the row has app mode connected with both halves saved (`mode: 'app'` on the record and an app pair `appOf()` can assemble, below — no config switch of its own), so an off connector dials nothing. The owner is the only sender that may start a turn: `feishu.ownerOpenId` from config is one half of that gate and the record's `allowed` guest list (below) the other, and an empty `open_id` never compares equal to an unset owner. A bounded ring of 512 `event_id`s makes a replay land once — the id enters it only after `deliver` has returned, so a handover that threw is retried instead of being refused as a duplicate for ever — and one socket per connector shares a start. The socket itself is injected as `rt.feishuTransport` (`connect(onEvent) -> { close }`) so the client behind it is one adapter, and it is read again on every start rather than copied while the modules install — an adapter that arrives later comes up on the row's **Try again**, which retries the sender and the socket together; a stop that lands while a socket dials hangs up the one that arrives instead of adopting it. `src/host/feishu-sdk.js` is the default adapter over the official `@larksuiteoapi/node-sdk`, imported only when a socket is dialled and reading the app pair at that moment, so a host that never turns the long connection on pays nothing for the package; the message lands through `remote-chat.deliver`, the seam a friend's Bot already uses, and the answer leaves again through the sender this module registers in that seam under the `feishu` plugin, posting on the same `FEISHU_WEBHOOK` `feishuPost` has used since phase 1 (a chat an older build opened without the plugin is refreshed on its next message) — one path in, one path out. `FEISHU_APP_SECRET` is the only half read as a secret through `rt.secretValue`, the App ID travels in the connector record (below), and a half-saved pair reports `missing` and dials nothing. diff --git a/patches/ds-bot/0001-core-fixes.patch b/patches/ds-bot/0001-core-fixes.patch index f46dab35f..175f7e225 100644 --- a/patches/ds-bot/0001-core-fixes.patch +++ b/patches/ds-bot/0001-core-fixes.patch @@ -4027,7 +4027,7 @@ diff --git a/src/host/connectors.js b/src/host/connectors.js let response try { response = await request(`${api.replace(/\/$/, '')}/user`, { -@@ -38,132 +38,991 @@ export async function signIn(token, { api = DEFAULTS.api, fetch: request = fetch +@@ -38,132 +38,1014 @@ export async function signIn(token, { api = DEFAULTS.api, fetch: request = fetch return { login: user.login } } @@ -4234,6 +4234,25 @@ diff --git a/src/host/connectors.js b/src/host/connectors.js + return Promise.race([promise, deadline]).finally(() => clearTimeout(timer)) +} + ++// A request that never reached Feishu wrote nothing, so notify.js may safely send it again. A ++// request that timed out, was aborted, or failed in a way we cannot read may already be in the ++// chat, and a custom-bot webhook has no idempotency field: repeating it would post it twice. ++// `feishu/no-answer` is deliberately absent from notify.js's `RETRYABLE`, so it gets one attempt. ++const UNSENT_CODES = new Set(['ECONNREFUSED', 'ENOTFOUND', 'EAI_AGAIN']) ++const NO_ANSWER = 'feishu/no-answer' ++ ++function connectionNeverOpened(error) { ++ const seen = new Set() ++ const inspect = cause => { ++ if (!cause || typeof cause !== 'object' || seen.has(cause)) return false ++ seen.add(cause) ++ if (UNSENT_CODES.has(String(cause.code))) return true ++ if (inspect(cause.cause)) return true ++ return Array.isArray(cause.errors) && cause.errors.some(inspect) ++ } ++ return inspect(error) ++} ++ +// One text message through a custom-bot webhook, answered in notify.js's vocabulary: `{ ok, ts }` +// or `{ ok: false, error }`, where `rate_limited` and `service_unavailable` may pass on their own +// and every other error is a shape Feishu will refuse again. The URL is never part of the answer. @@ -4247,8 +4266,9 @@ diff --git a/src/host/connectors.js b/src/host/connectors.js + redirect: 'error', + signal: AbortSignal.timeout(Math.max(1, timeoutMs)), + }), timeoutMs) -+ } catch { -+ return { ok: false, error: 'network' } ++ } catch (error) { ++ // Only a connection that never opened is retried; anything else is an unconfirmed delivery. ++ return connectionNeverOpened(error) ? { ok: false, error: 'network' } : { ok: false, error: NO_ANSWER } + } + const body = await response.json().catch(() => ({})) + if (response.status === 429) { @@ -4270,6 +4290,7 @@ diff --git a/src/host/connectors.js b/src/host/connectors.js +export async function feishuCheck(webhook, { api = FEISHU_DEFAULTS.api, fetch: request = fetch, timeoutMs = CHECK_TIMEOUT_MS } = {}) { + const answer = await feishuPost(webhookAddress(webhook, api), FEISHU_TEST_TEXT, { fetch: request, timeoutMs }) + if (answer.ok) return {} ++ if (answer.error === NO_ANSWER) throw new Error('Feishu did not answer in time — the test message may already be in the chat; check before connecting again') + if (answer.error === 'network' || answer.error === 'service_unavailable') throw new Error('Could not reach Feishu') + if (answer.error === 'rate_limited') throw new Error('Feishu is rate-limiting this webhook; try again in a minute') + throw new Error(`Feishu did not accept the webhook (${answer.error})`) @@ -4916,8 +4937,8 @@ diff --git a/src/host/connectors.js b/src/host/connectors.js + } + await retire(c, { gone: previous?.session, kept: record.session, keys: [oldToken, token] }) + await sync(c) - return connectorsView() - }, ++ return connectorsView() ++ }, + // Changes which apps a connected gateway offers, without asking for the key again. + async 'connector-apps'(body) { + const c = known(body.id) @@ -4936,8 +4957,8 @@ diff --git a/src/host/connectors.js b/src/host/connectors.js + } + await retire(c, { gone: previous?.session, kept: record.session, keys: [token] }) + await sync(c) -+ return connectorsView() -+ }, + return connectorsView() + }, + // Searches Composio's app list: with the saved key, or before connecting with the key being pasted + // (sent only to Composio, never saved by this call). + async 'connector-app-search'(body) { @@ -5039,7 +5060,9 @@ diff --git a/src/host/connectors.js b/src/host/connectors.js + // A fresh key per call: two messages with the same text are two messages, not a duplicate. + const result = await notifier.send({ key: randomUUID(), text }) + const sent = result.state === 'sent' || result.state === 'duplicate' -+ const failure = `Feishu refused the message (${result.error ?? result.reason ?? 'no answer'})` ++ const failure = result.error === NO_ANSWER ++ ? 'Feishu did not answer in time — the message may already be in the chat; check before sending again' ++ : `Feishu refused the message (${result.error ?? result.reason ?? 'no answer'})` + await keep(c, { ...saved(c), ...(sent ? { lastOkAt: Date.now() } : { lastError: failure }) }) + if (!sent) throw new Error(failure) return connectorsView() diff --git a/patches/ds-bot/files/src/host/notify.js b/patches/ds-bot/files/src/host/notify.js index 6784f77d7..12ab00e42 100644 --- a/patches/ds-bot/files/src/host/notify.js +++ b/patches/ds-bot/files/src/host/notify.js @@ -1,10 +1,11 @@ // Out-of-app delivery, first half (#355): the adapter that will carry a reminder to Slack. // A reminder has to reach the owner when the desktop app is closed, so delivery cannot be a // client toast. The transport is injected; what this module owns is the policy around it: one -// idempotency key per reminder, so a retried send cannot arrive twice; exponential backoff that -// honours `Retry-After`; three attempts at most; no retry at all for a failure that will not -// change (the channel is gone, the app was removed). A channel switched off makes zero transport -// calls, including one that is switched off while it waits to retry. +// idempotency key per reminder, deduplicated inside this process only, and a send that may already +// have left the process is never repeated, because the outbound transport cannot dedupe it; +// exponential backoff that honours `Retry-After`; three attempts at most; no retry at all for a +// failure that will not change (the channel is gone, the app was removed). A channel switched off +// makes zero transport calls, including one that is switched off while it waits to retry. // // The real Slack transport waits for the owner's credential, so it is not here: the module speaks // Slack's `chat.postMessage` shape and ships a fake transport that records every call and scripts diff --git a/patches/ds-bot/files/test/feishu.test.js b/patches/ds-bot/files/test/feishu.test.js index c0662ce91..25c2cbdc1 100644 --- a/patches/ds-bot/files/test/feishu.test.js +++ b/patches/ds-bot/files/test/feishu.test.js @@ -142,7 +142,8 @@ test('#1263: a webhook Feishu refuses stays on the row as an error that names no test('#1263: a Feishu that never answers fails on the deadline instead of hanging', async () => { const started = Date.now() const answerOrNothing = await feishuPost(webhookOf('x'), 'hello', { fetch: () => new Promise(() => {}), timeoutMs: 25 }) - assert.deepEqual(answerOrNothing, { ok: false, error: 'network' }) + // #2660: the POST may already be in the chat, so it is an unconfirmed delivery, not a retryable link. + assert.deepEqual(answerOrNothing, { ok: false, error: 'feishu/no-answer' }) assert.ok(Date.now() - started < 2_000, 'the deadline, not the fetch, ends the wait') }) diff --git a/patches/ds-bot/files/test/notify-webhook-once.test.js b/patches/ds-bot/files/test/notify-webhook-once.test.js new file mode 100644 index 000000000..0014e76e9 --- /dev/null +++ b/patches/ds-bot/files/test/notify-webhook-once.test.js @@ -0,0 +1,84 @@ +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { feishuCheck, feishuTransport } from '../src/host/connectors.js' +import { createNotifier } from '../src/host/notify.js' + +// #2660: a Feishu custom-bot webhook has no idempotency field, so a POST whose answer was lost +// cannot be sent again: the receiver would show the same message twice. Notify.js retries +// `network`, which is right for a connection that never opened and wrong for a request that may +// already have been written — the transport has to tell those two apart. + +const WEBHOOK = 'https://open.feishu.cn/open-apis/bot/v2/hook/unit-test' +const ok = data => ({ status: 200, ok: true, headers: { get: () => null }, json: async () => ({ code: 0, data }) }) +const status = (code, headers = {}) => ({ + status: code, + ok: code >= 200 && code < 300, + headers: { get: name => headers[String(name).toLowerCase()] ?? null }, + json: async () => ({}), +}) +const notifier = (fetch, extra = {}) => createNotifier({ + channel: 'feishu', + transport: feishuTransport(WEBHOOK, { fetch, timeoutMs: 40 }), + log: () => {}, + ...extra, +}) + +test('#2660: a POST whose answer was lost is not sent a second time', async () => { + const posts = [] + const fetch = async (url, init) => { + posts.push(JSON.parse(init.body)) + return new Promise(() => {}) // accepted, but the answer never comes back + } + + const result = await notifier(fetch).send({ key: 'k1', text: 'standup in 5 minutes' }) + + assert.equal(posts.length, 1, 'one send makes exactly one POST when the answer is lost') + assert.deepEqual(posts[0], { msg_type: 'text', content: { text: 'standup in 5 minutes' } }) + assert.deepEqual(result, { state: 'failed', error: 'feishu/no-answer', attempts: 1, permanent: true }) + assert.equal(result.error, 'feishu/no-answer', 'the error names the unconfirmed delivery') +}) + +test('#2660: a connection that never opened still retries three times', async () => { + let calls = 0 + const fetch = async () => { + calls += 1 + const cause = Object.assign(new Error('connect ECONNREFUSED'), { code: 'ECONNREFUSED' }) + throw new TypeError('fetch failed', { cause }) + } + + const result = await notifier(fetch, { delay: async () => {} }).send({ key: 'k2', text: 'hello' }) + + assert.equal(calls, 3, 'a refused connection wrote nothing, so the three attempts stay') + assert.deepEqual(result, { state: 'failed', error: 'network', attempts: 3, permanent: false }) +}) + +test('#2660: a 429 still retries and waits what Retry-After asks', async () => { + let calls = 0 + const waits = [] + const fetch = async () => { + calls += 1 + return calls === 1 ? status(429, { 'retry-after': '2' }) : ok({ message_id: 'om_1' }) + } + + const result = await notifier(fetch, { delay: async ms => { waits.push(ms) } }).send({ key: 'k3', text: 'hello' }) + + assert.equal(result.state, 'sent') + assert.equal(result.attempts, 2, 'a rate limit is transient: the same call is tried again') + assert.ok(waits[0] >= 2_000, 'Retry-After is a floor, not a suggestion') +}) + +test('#2660: the connect sentence says the test message may already be posted', async () => { + let calls = 0 + const fetch = async () => { calls += 1; return new Promise(() => {}) } + + await assert.rejects( + feishuCheck(WEBHOOK, { fetch, timeoutMs: 40 }), + error => { + assert.equal(calls, 1, 'the probe is one POST, never a retry') + assert.match(error.message, /did not answer in time/i) + assert.match(error.message, /may already be in the chat/i) + assert.ok(!error.message.includes('open.feishu.cn'), 'the sentence never carries the webhook URL') + return true + }, + ) +})