Skip to content

fix(extensions/feishu/src/reply-dispatcher.ts): missing cleanup in error path#50680

Closed
Maple778 wants to merge 1 commit intoopenclaw:mainfrom
Maple778:fix/agent-missing-cleanup-in-error-path-290
Closed

fix(extensions/feishu/src/reply-dispatcher.ts): missing cleanup in error path#50680
Maple778 wants to merge 1 commit intoopenclaw:mainfrom
Maple778:fix/agent-missing-cleanup-in-error-path-290

Conversation

@Maple778
Copy link
Copy Markdown
Contributor

@Maple778 Maple778 commented Mar 19, 2026

The `closeStreaming` function in `extensions/feishu/src/reply-dispatcher.ts` could skip cleanup if any async operation threw an exception.

Changes:

  • Wrapped async operations in `try` block
  • Moved all variable nullifications and state resets to `finally` block
  • Guarantees cleanup (`streaming = null`, `streamingStartPromise = null`, state resets) executes even if errors occur
  • Prevents memory leaks and state corruption in retry scenarios
  • Pattern from PR fix(gateway): pin plugin webhook route registry #47902
  • No semantic changes—only added error-path safety

@Maple778
fix(extensions/feishu/src/reply-dispatcher.ts): missing cleanup in er…
398fc51 
…ror path

Pattern from PR openclaw#47902

The bug pattern 'missing cleanup in error path' describes a scenario where a resource cleanup routine (`releasePluginRouteRegistry`) is skipped if an error occurs during the shutdown sequence. The provided code context for `startStreaming` in `extensions/feishu/src/reply-dispatcher.ts` contains a function `closeStreaming` (lines 290-308) which acts as the shutdown sequence for this component.

`closeStreaming` performs multiple asynchronous operations:
1. `await streamingStartPromise;` (Line 292)
2. `await partialUpdateQueue;` (Line 294)
3. `await streaming.close(...)` (Line 301)

If any of these `await` operations throw an exception (e.g., network error during `streaming.close`), the execution halts immediately. The subsequent cleanup lines (`streaming = null`, `streamingStartPromise = null`, and state resets) are skipped. Consequently, the `streaming` object is not properly dereferenced/nullified, and internal state variables (`streamText`, `reasoningText`) retain stale data, leading to potential memory leaks and state corruption in subsequent retry attempts. The fix requires wrapping the body of `closeStreaming` in a `try...finally` block and moving all variable nullifications/reset operations into the `finally` block to guarantee execution regardless of failure.
@openclaw-barnacle openclaw-barnacle Bot added channel: feishu Channel integration: feishu size: XS labels Mar 19, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 19, 2026

Greptile Summary

This PR adds try/finally error-path safety to the closeStreaming function in extensions/feishu/src/reply-dispatcher.ts. Previously, if any awaited operation inside closeStreaming (e.g. partialUpdateQueue, streaming.close(...)) threw an exception, the five state-reset lines at the end of the function would be skipped, leaving stale references and potentially causing memory leaks or corrupted state in retry scenarios. Moving the resets into a finally block guarantees they always execute.

  • The fix is minimal and correct: the finally block contains exactly the same five cleanup assignments as before; no new logic is introduced.
  • Error propagation behaviour is unchanged — errors still bubble up to callers (onIdle, onError, and the deliver callback).
  • The startStreaming IIFE already catches its own errors and resolves cleanly, so await streamingStartPromise in the try block will not reject; the main guarded paths are await partialUpdateQueue and await streaming.close(...).
  • partialUpdateQueue is not reset in the finally block (it wasn't before either), but since streaming is nulled out, any subsequent enqueued updates will be no-ops, so this is not a concern.

Confidence Score: 5/5

  • This PR is safe to merge — it is a minimal, correct defensive fix with no semantic changes in the happy path.
  • The change wraps an async function's body in try/finally so that five state-reset assignments are guaranteed to run on any code path. The cleanup assignments are identical to the originals; error propagation to callers is unchanged; and the happy path is functionally equivalent. No new logic, no new dependencies, no behavioural regressions are introduced.
  • No files require special attention.

Last reviewed commit: "fix(extensions/feish..."

chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed Mar 19, 2026
View reviewed changes
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 398fc511df

ℹ️ 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".

Comment thread extensions/feishu/src/reply-dispatcher.ts
Comment on lines +304 to +306
} finally {
streaming = null;
streamingStartPromise = null;
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep the streaming session until close actually succeeds

This finally block clears streaming/streamingStartPromise even when FeishuStreamingSession.close() throws. In the info.kind === "final" path, that rejection makes deliver() fail and createReplyDispatcher() immediately invokes onError, but the retry at extensions/feishu/src/reply-dispatcher.ts can no longer close the existing card because the session was already nulled out here. A transient Feishu/network failure now leaves the card stuck in streaming mode and drops the final reply instead of allowing the existing error path to retry it.

Useful? React with 👍 / 👎.

Comment thread extensions/feishu/src/reply-dispatcher.ts
Comment on lines +305 to +309
streaming = null;
streamingStartPromise = null;
streamText = "";
lastPartial = "";
reasoningText = "";
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reset partialUpdateQueue when running error cleanup

The new cleanup resets the text buffers but leaves partialUpdateQueue pointing at the same promise that just failed. If an earlier streaming.update() or partialUpdateQueue await rejected, the next flushStreamingCardUpdate() still does partialUpdateQueue = partialUpdateQueue.then(...) on a rejected promise, so its callback never runs and later closeStreaming() calls fail immediately again. Because this patch is explicitly trying to make post-error retries work, omitting the queue reset leaves every subsequent streamed reply in this dispatcher broken after the first update/close error.

Useful? React with 👍 / 👎.

@vincentkoc vincentkoc self-assigned this Apr 25, 2026
@vincentkoc vincentkoc mentioned this pull request Apr 25, 2026
Merged
@vincentkoc vincentkoc added dedupe:child Duplicate issue/PR child in dedupe cluster close:superseded PR close reason labels Apr 25, 2026
@vincentkoc
Copy link
Copy Markdown
Member

vincentkoc commented Apr 25, 2026

Closing as superseded by #71542. The merged follow-up wraps Feishu streaming close cleanup in a finally path, resets queued/update state after close failures, and adds regression coverage for a throwing close.

Thanks for flagging the missing cleanup path; the safer current-main version is now landed.

@vincentkoc vincentkoc closed this Apr 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

Assignees

@vincentkoc vincentkoc

Labels

channel: feishu Channel integration: feishu close:superseded PR close reason dedupe:child Duplicate issue/PR child in dedupe cluster size: XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Maple778 @vincentkoc