fix(feishu): await HTTP server close before clearing state references by alex-xuweilong · Pull Request #48588 · openclaw/openclaw

alex-xuweilong · 2026-03-17T00:30:56Z

Summary

Prevents port leaks (EADDRINUSE) and memory leaks caused by premature HTTP server reference cleanup during Feishu plugin shutdown/restart.

Root Cause

stopFeishuMonitorState() calls server.close() then immediately deletes the server from the httpServers Map. Since server.close() is asynchronous, the port may still be bound when the reference is removed — causing EADDRINUSE on restart and leaking server objects. The same pattern exists in monitor.transport.ts's cleanup function.

Fix

Add closeHttpServer() helper: calls closeAllConnections() (Node 18.2+) then awaits server.close() callback, with a 5-second timeout fallback
Change stopFeishuMonitorState() to async: await server close before deleting Map entries (single-account and full-cleanup paths)
Fix monitor.transport.ts cleanup: await server.close() before clearing state
Backward compatible: callers can still fire-and-forget (no signature change needed at call sites)

Changes

extensions/feishu/src/monitor.state.ts: 22 lines added, 5 removed
extensions/feishu/src/monitor.transport.ts: 5 lines added, 6 removed

Fixes #48183

stopFeishuMonitorState() calls server.close() then immediately deletes the server reference from the httpServers Map. Since server.close() is asynchronous, the port may still be bound when the reference is removed — causing EADDRINUSE on restart and leaking server objects in memory. Await server.close() with a 5-second timeout before clearing Map entries. Also call closeAllConnections() (Node 18.2+) to force-close keep-alive connections. Fixes openclaw#48183

greptile-apps · 2026-03-17T00:33:29Z

Greptile Summary

This PR fixes a real and well-diagnosed race condition: stopFeishuMonitorState() previously deleted HTTP server references from the httpServers Map before server.close() finished, causing EADDRINUSE on restart and memory leaks. The fix in monitor.state.ts is solid — the new closeHttpServer() helper combines closeAllConnections() with a 5-second Promise.race timeout, and the function is correctly made async to sequence state cleanup after server teardown.

However, monitor.transport.ts re-implements the same close logic in its local cleanup() without the timeout guard, and the void cleanup().then(() => resolve()) pattern silently discards any rejection. Key concerns:

Missing timeout in monitor.transport.ts cleanup — on Node.js < 18.2 with persistent connections, server.close() never invokes its callback, causing monitorWebhook()'s outer Promise to hang forever.
Unhandled rejections — void cleanup().then(() => resolve()) (lines 243 and 247) swallows errors; reject should be passed as the second argument to .then().
stopFeishuMonitor discards the async result — the public wrapper in monitor.ts still returns void, meaning callers that fire-and-forget could still hit the original EADDRINUSE bug. Returning the Promise allows callers to opt in to awaiting shutdown.

Confidence Score: 2/5

The fix correctly addresses the root cause in monitor.state.ts, but monitor.transport.ts introduces a potential indefinite hang on cleanup and silently drops errors.
The state module changes are solid, but the transport cleanup is missing the timeout guard present in the new helper — on older Node.js or with persistent connections, the monitorWebhook Promise can hang forever. The void-then pattern also risks swallowing rejections permanently. These are genuine correctness issues that should be fixed before merging.
extensions/feishu/src/monitor.transport.ts — cleanup() lacks timeout protection and swallows rejections. extensions/feishu/src/monitor.ts — stopFeishuMonitor() discards the async result.

Comments Outside Diff (1)

extensions/feishu/src/monitor.ts, line 93-95 (link)

Public wrapper discards the async result

stopFeishuMonitorState is now async, but stopFeishuMonitor drops the returned Promise. Any caller that invokes stopFeishuMonitor() and then immediately restarts the monitor could still hit EADDRINUSE because the port is freed asynchronously but nothing awaits it.

Making the wrapper async (or at least returning the Promise) lets callers opt in to awaiting the shutdown:

Prompt To Fix With AI

This is a comment left during a code review.
Path: extensions/feishu/src/monitor.ts
Line: 93-95

Comment:
**Public wrapper discards the async result**

`stopFeishuMonitorState` is now `async`, but `stopFeishuMonitor` drops the returned Promise. Any caller that invokes `stopFeishuMonitor()` and then immediately restarts the monitor could still hit `EADDRINUSE` because the port is freed asynchronously but nothing awaits it.

Making the wrapper async (or at least returning the Promise) lets callers opt in to awaiting the shutdown:



How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: extensions/feishu/src/monitor.transport.ts
Line: 233-239

Comment:
**Cleanup can hang indefinitely — missing timeout**

`closeHttpServer()` in `monitor.state.ts` protects against a stuck server with a 5-second `Promise.race` timeout, but the `cleanup()` function here skips that guard. If `closeAllConnections` is unavailable (Node.js < 18.2) and there are persistent keep-alive connections, `server.close()` will never invoke its callback — it only stops *accepting* new connections, not closing existing ones. That means `cleanup()` hangs forever, `void cleanup().then(() => resolve())` never calls `resolve()`, and `monitorWebhook()`'s outer Promise never settles.

The simplest fix is to export `closeHttpServer` from `monitor.state.ts` and reuse it here, so both paths share the same timeout behaviour:

```
// In monitor.state.ts — add `export` keyword
export function closeHttpServer(server: http.Server): Promise<void> { ... }
```

```suggestion
    const cleanup = async () => {
      await closeHttpServer(server);
      httpServers.delete(accountId);
      botOpenIds.delete(accountId);
      botNames.delete(accountId);
    };
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: extensions/feishu/src/monitor.transport.ts
Line: 243-248

Comment:
**Cleanup rejections silently swallowed**

Using `void cleanup().then(() => resolve())` discards any rejection from `cleanup()`. If cleanup ever rejects (e.g., due to a future code change that can throw), the outer `monitorWebhook()` Promise will hang forever — `resolve()` is never called and `reject()` is never invoked.

Passing `reject` as the second argument to `.then()` ensures the outer Promise settles even on error:

```suggestion
      void cleanup().then(() => resolve(), reject);
```

And similarly for the already-aborted guard a few lines below:
```suggestion
      void cleanup().then(() => resolve(), reject);
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: extensions/feishu/src/monitor.ts
Line: 93-95

Comment:
**Public wrapper discards the async result**

`stopFeishuMonitorState` is now `async`, but `stopFeishuMonitor` drops the returned Promise. Any caller that invokes `stopFeishuMonitor()` and then immediately restarts the monitor could still hit `EADDRINUSE` because the port is freed asynchronously but nothing awaits it.

Making the wrapper async (or at least returning the Promise) lets callers opt in to awaiting the shutdown:

```suggestion
export function stopFeishuMonitor(accountId?: string): Promise<void> {
  return stopFeishuMonitorState(accountId);
}
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: 8fc7e52}

greptile-apps · 2026-03-17T00:33:32Z

+    const cleanup = async () => {
+      (server.closeAllConnections as (() => void) | undefined)?.();
+      await new Promise<void>((cb) => server.close(() => cb()));
      httpServers.delete(accountId);
      botOpenIds.delete(accountId);
      botNames.delete(accountId);
    };


Cleanup can hang indefinitely — missing timeout

closeHttpServer() in monitor.state.ts protects against a stuck server with a 5-second Promise.race timeout, but the cleanup() function here skips that guard. If closeAllConnections is unavailable (Node.js < 18.2) and there are persistent keep-alive connections, server.close() will never invoke its callback — it only stops accepting new connections, not closing existing ones. That means cleanup() hangs forever, void cleanup().then(() => resolve()) never calls resolve(), and monitorWebhook()'s outer Promise never settles.

The simplest fix is to export closeHttpServer from monitor.state.ts and reuse it here, so both paths share the same timeout behaviour:

// In monitor.state.ts — add `export` keyword export function closeHttpServer(server: http.Server): Promise<void> { ... }

Suggested change

const cleanup = async () => {

(server.closeAllConnections as (() => void) | undefined)?.();

await new Promise<void>((cb) => server.close(() => cb()));

httpServers.delete(accountId);

botOpenIds.delete(accountId);

botNames.delete(accountId);

};

const cleanup = async () => {

await closeHttpServer(server);

httpServers.delete(accountId);

botOpenIds.delete(accountId);

botNames.delete(accountId);

};

Prompt To Fix With AI

This is a comment left during a code review. Path: extensions/feishu/src/monitor.transport.ts Line: 233-239 Comment: **Cleanup can hang indefinitely — missing timeout** `closeHttpServer()` in `monitor.state.ts` protects against a stuck server with a 5-second `Promise.race` timeout, but the `cleanup()` function here skips that guard. If `closeAllConnections` is unavailable (Node.js < 18.2) and there are persistent keep-alive connections, `server.close()` will never invoke its callback — it only stops *accepting* new connections, not closing existing ones. That means `cleanup()` hangs forever, `void cleanup().then(() => resolve())` never calls `resolve()`, and `monitorWebhook()`'s outer Promise never settles. The simplest fix is to export `closeHttpServer` from `monitor.state.ts` and reuse it here, so both paths share the same timeout behaviour: ``` // In monitor.state.ts — add `export` keyword export function closeHttpServer(server: http.Server): Promise<void> { ... } ``` ```suggestion const cleanup = async () => { await closeHttpServer(server); httpServers.delete(accountId); botOpenIds.delete(accountId); botNames.delete(accountId); }; ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-17T00:33:33Z

+      void cleanup().then(() => resolve());
    };

    if (abortSignal?.aborted) {
-      cleanup();
-      resolve();
+      void cleanup().then(() => resolve());
      return;


Cleanup rejections silently swallowed

Using void cleanup().then(() => resolve()) discards any rejection from cleanup(). If cleanup ever rejects (e.g., due to a future code change that can throw), the outer monitorWebhook() Promise will hang forever — resolve() is never called and reject() is never invoked.

Passing reject as the second argument to .then() ensures the outer Promise settles even on error:

Suggested change

void cleanup().then(() => resolve());

};

if (abortSignal?.aborted) {

cleanup();

resolve();

void cleanup().then(() => resolve());

return;

void cleanup().then(() => resolve(), reject);

And similarly for the already-aborted guard a few lines below:

Suggested change

void cleanup().then(() => resolve());

};

if (abortSignal?.aborted) {

cleanup();

resolve();

void cleanup().then(() => resolve());

return;

void cleanup().then(() => resolve(), reject);

Prompt To Fix With AI

This is a comment left during a code review. Path: extensions/feishu/src/monitor.transport.ts Line: 243-248 Comment: **Cleanup rejections silently swallowed** Using `void cleanup().then(() => resolve())` discards any rejection from `cleanup()`. If cleanup ever rejects (e.g., due to a future code change that can throw), the outer `monitorWebhook()` Promise will hang forever — `resolve()` is never called and `reject()` is never invoked. Passing `reject` as the second argument to `.then()` ensures the outer Promise settles even on error: ```suggestion void cleanup().then(() => resolve(), reject); ``` And similarly for the already-aborted guard a few lines below: ```suggestion void cleanup().then(() => resolve(), reject); ``` How can I resolve this? If you propose a fix, please make it concise.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8fc7e52923

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-03-17T00:34:01Z

+    new Promise<void>((resolve) => {
+      setTimeout(() => {
+        console.warn("feishu: HTTP server close timed out, continuing cleanup");
+        resolve();
+      }, SERVER_CLOSE_TIMEOUT_MS);


Cancel server-close fallback timer after successful close

The new closeHttpServer timeout branch is never canceled when server.close() finishes first, so every normal shutdown still leaves a live 5s timer that keeps the event loop open and eventually prints a false timeout warning. In practice, stopping Feishu monitoring now adds an avoidable shutdown delay/noise on every close path. Please clear the timeout when the close callback runs (or otherwise make the fallback timer non-blocking).

Useful? React with 👍 / 👎.

…e cleanup errors

alex-xuweilong · 2026-03-17T00:49:14Z

Thanks for the thorough reviews! All three issues addressed:

Exported closeHttpServer and reused it in monitor.transport.ts — no more duplicated close logic
Added clearTimeout(timer) on successful close to prevent stale 5s timers
Changed both .then(() => resolve()) to .then(() => resolve(), reject) to propagate cleanup errors

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 284336a893

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-03-17T00:53:22Z

+      resolve();
+    }, SERVER_CLOSE_TIMEOUT_MS);
+
+    (server.closeAllConnections as (() => void) | undefined)?.();


Call server.close() before destroying all connections

closeHttpServer currently invokes closeAllConnections() before server.close(), which leaves a race window where new sockets can still be accepted between those two calls. Under shutdown traffic, that can keep server.close() from completing until the 5s timeout path resolves, reintroducing the same premature-cleanup behavior this patch is trying to eliminate (state is cleared while the port may still be bound).

Useful? React with 👍 / 👎.

Good catch — reordered to call server.close() first (stop accepting new connections), then closeAllConnections() (destroy existing sockets). This eliminates the race window where new sockets could be accepted during shutdown.

…socket race

clawsweeper · 2026-04-28T14:11:13Z

Codex review: needs changes before merge.

Summary
The PR adds a shared Feishu HTTP server close helper and awaits it before deleting Feishu monitor state in stop and webhook-abort cleanup paths.

Reproducibility: yes. at source level: current main calls server.close() and clears Feishu HTTP server state before the close callback in both stop and webhook-abort cleanup paths. I did not run a live restart loop in this read-only pass.

Next step before merge
A repair worker can add the missing changelog line without product, ownership, or security judgment.

Security
Cleared: The diff is limited to Feishu HTTP server shutdown sequencing and does not change dependencies, workflows, package resolution, secrets handling, permissions, or artifact execution.

Review findings

[P3] Add the required changelog entry — extensions/feishu/src/monitor.state.ts:146

Review details

Best possible solution:

Land this PR or a maintainer-owned equivalent after adding the required changelog entry and running merge-preview validation; keep the broader ResourceManager proposal in #48793 separate.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: current main calls server.close() and clears Feishu HTTP server state before the close callback in both stop and webhook-abort cleanup paths. I did not run a live restart loop in this read-only pass.

Is this the best way to solve the issue?

Yes. A Feishu-owned awaitable HTTP close helper shared by state cleanup and webhook abort cleanup is the narrowest maintainable fix; the remaining repair is the required changelog entry.

Full review comments:

[P3] Add the required changelog entry — extensions/feishu/src/monitor.state.ts:146
This is a user-facing Feishu fix, but the PR changes production monitor code without updating CHANGELOG.md. Add one single-line Unreleased Fixes entry referencing this PR and crediting the human contributor before merge.
Confidence: 0.9

Overall correctness: patch is correct
Overall confidence: 0.82

Acceptance criteria:

git diff --check
node scripts/check-changelog-attributions.mjs CHANGELOG.md
If implementation files are touched, run: pnpm test extensions/feishu/src/monitor.cleanup.test.ts extensions/feishu/src/monitor.webhook-e2e.test.ts extensions/feishu/src/monitor.webhook-security.test.ts

What I checked:

current-main-state-cleanup-still-sync: Current main still calls server.close() and immediately deletes or clears httpServers in stopFeishuMonitorState(). (extensions/feishu/src/monitor.state.ts:146, 89db1e5440f5)
current-main-webhook-abort-cleanup-still-sync: Current main's webhook abort cleanup calls server.close(), deletes Feishu state, and resolves without waiting for the close callback. (extensions/feishu/src/monitor.transport.ts:392, 89db1e5440f5)
pr-diff-addresses-feishu-owned-cleanup: The provided PR diff exports closeHttpServer(), awaits it in stopFeishuMonitorState(), and reuses it from webhook abort cleanup with rejection propagation. (extensions/feishu/src/monitor.state.ts:146, 80c074a12a83)
missing-required-changelog-entry: The current changelog has no entry for this Feishu cleanup fix or PR; Unreleased Fixes is the active placement. (CHANGELOG.md:46, 89db1e5440f5)
changelog-required-for-prod-code: Repo policy and merge gate require CHANGELOG.md for non-doc/test/workflow changed files, and this PR changes Feishu production code. (scripts/pr-lib/gates.sh:56, 89db1e5440f5)
related-tracking-context: The PR uses closing syntax for the still-open Feishu bug [Bug]: Feishu monitor state cleanup incomplete - potential memory leak in httpServers Map #48183; the broader ResourceManager proposal remains separately tracked in feat: centralized PluginResourceManager interface for consistent async cleanup across all channel plugins #48793 and should not block this narrow plugin-owned fix.

Likely related people:

steipete: Feature history connects this maintainer to the split Feishu monitor files and nearby webhook/body handling around this cleanup path. (role: recent maintainer and major Feishu monitor refactor owner; confidence: high; commits: f46bd2e0cc17, 8be3a4466cbc, 164f0feddff5; files: extensions/feishu/src/monitor.ts, extensions/feishu/src/monitor.state.ts, extensions/feishu/src/monitor.transport.ts)
vincentkoc: Recent Feishu state seam, runtime helper, and reconnect lifecycle commits touch adjacent monitor ownership and cleanup behavior. (role: recent Feishu lifecycle and state-seam maintainer; confidence: high; commits: 2fff6be4c393, 319aa2f1fe67, d93e6f61580a; files: extensions/feishu/src/monitor.state.ts, extensions/feishu/src/monitor.transport.ts, extensions/feishu/src/monitor.cleanup.test.ts)

Remaining risk / open question:

No live restart loop or targeted Vitest run was executed in this read-only review, so merge still needs focused Feishu cleanup/webhook validation.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 89db1e5440f5.

openclaw-barnacle Bot added channel: feishu Channel integration: feishu size: XS labels Mar 17, 2026

greptile-apps Bot reviewed Mar 17, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Mar 17, 2026

View reviewed changes

fix: reuse closeHttpServer helper, clear timeout on success, propagat…

284336a

…e cleanup errors

chatgpt-codex-connector Bot reviewed Mar 17, 2026

View reviewed changes

alex-xuweilong mentioned this pull request Mar 17, 2026

[Bug]: Feishu monitor state cleanup incomplete - potential memory leak in httpServers Map #48183

Open

fix: call server.close() before closeAllConnections() to prevent new …

80c074a

…socket race

alex-xuweilong mentioned this pull request Mar 17, 2026

feat: centralized PluginResourceManager interface for consistent async cleanup across all channel plugins #48793

Open

clawsweeper Bot mentioned this pull request Apr 26, 2026

fix(feishu): await HTTP server close during monitor cleanup #48329

Closed

3 tasks

vincentkoc added the triage: refactor-only Candidate: refactor/cleanup-only PR without maintainer context. label Apr 26, 2026

BingqingLyu mentioned this pull request Apr 27, 2026

fix(feishu): await HTTP server close before clearing state references BingqingLyu/openclaw#882

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(feishu): await HTTP server close before clearing state references#48588

fix(feishu): await HTTP server close before clearing state references#48588
alex-xuweilong wants to merge 3 commits intoopenclaw:mainfrom
alex-xuweilong:fix/feishu-monitor-state-cleanup

alex-xuweilong commented Mar 17, 2026

Uh oh!

greptile-apps Bot commented Mar 17, 2026 •

edited

Loading

Comments Outside Diff (1)

Uh oh!

greptile-apps Bot Mar 17, 2026

Uh oh!

greptile-apps Bot Mar 17, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Mar 17, 2026

Uh oh!

alex-xuweilong commented Mar 17, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Mar 17, 2026

Uh oh!

alex-xuweilong Mar 17, 2026

Uh oh!

clawsweeper Bot commented Apr 28, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

alex-xuweilong commented Mar 17, 2026

Summary

Root Cause

Fix

Changes

Uh oh!

greptile-apps Bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 2/5

Comments Outside Diff (1)

Uh oh!

greptile-apps Bot Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

alex-xuweilong commented Mar 17, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

alex-xuweilong Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

clawsweeper Bot commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

greptile-apps Bot commented Mar 17, 2026 •

edited

Loading

clawsweeper Bot commented Apr 28, 2026 •

edited

Loading