Skip to content

fix(msteams): clear pending upload timeout on removal#32555

Merged
BradGroux merged 1 commit intoopenclaw:mainfrom
PinoHouse:fix/msteams-pending-upload-timeout-leak
Apr 1, 2026
Merged

fix(msteams): clear pending upload timeout on removal#32555
BradGroux merged 1 commit intoopenclaw:mainfrom
PinoHouse:fix/msteams-pending-upload-timeout-leak

Conversation

@PinoHouse
Copy link
Copy Markdown
Contributor

@PinoHouse PinoHouse commented Mar 3, 2026

Track setTimeout handles and clear them on removal/bulk-clear to prevent orphaned timer callbacks.

@openclaw-barnacle openclaw-barnacle bot added channel: msteams Channel integration: msteams size: XS labels Mar 3, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 3, 2026

Greptile Summary

This PR fixes orphaned setTimeout handles in the MS Teams pending-upload module by introducing a pendingTimeouts map that tracks every timer created in storePendingUpload and cancels it in both removePendingUpload and clearPendingUploads. The fix is correct and well-targeted for the two paths it addresses.

One small gap remains: the manual-expiry branch inside getPendingUpload (lines 63-65) deletes the entry from pendingUploads but does not cancel or remove the corresponding timer from pendingTimeouts. The timer's eventual callback is safe (no-op deletes), but the live handle lingers in pendingTimeouts until it fires, which is inconsistent with the cleanup discipline introduced elsewhere in this PR and means clearPendingUploads may needlessly iterate over an about-to-expire timer.

Confidence Score: 4/5

  • Safe to merge; the core fix is correct and the only gap is a minor style inconsistency in the manual-expiry path.
  • The change is small, well-scoped, and directly addresses the stated problem. All critical removal paths (removePendingUpload, clearPendingUploads) now properly cancel timers. The one omission — not cancelling the timer in getPendingUpload's expiry check — is a style-level inconsistency with no runtime risk, since the timer callback is idempotent.
  • No files require special attention beyond the minor inconsistency noted in extensions/msteams/src/pending-uploads.ts at line 63.

Last reviewed commit: 38d9f2b

greptile-apps[bot]
greptile-apps bot reviewed Mar 3, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 3, 2026

Additional Comments (1)

extensions/msteams/src/pending-uploads.ts
Timeout not cancelled on manual expiry

When getPendingUpload detects an expired entry and removes it from pendingUploads, it does not cancel the corresponding timeout or remove it from pendingTimeouts. This means a live timer handle stays in pendingTimeouts until the timeout eventually fires on its own. While the timer's callback is safe (it calls pendingUploads.delete and pendingTimeouts.delete on an already-absent entry), it is inconsistent with the rest of the PR, which explicitly cancels timers on all other removal paths.

To keep the cleanup uniform — and to prevent clearPendingUploads from iterating over a timer that is about to self-fire — consider cancelling and removing the timeout here as well:

  if (Date.now() - entry.createdAt > PENDING_UPLOAD_TTL_MS) {
    pendingUploads.delete(id);
    const timeout = pendingTimeouts.get(id);
    if (timeout) {
      clearTimeout(timeout);
      pendingTimeouts.delete(id);
    }
    return undefined;
  }
Prompt To Fix With AI 
This is a comment left during a code review.
Path: extensions/msteams/src/pending-uploads.ts
Line: 63-66

Comment:
**Timeout not cancelled on manual expiry**

When `getPendingUpload` detects an expired entry and removes it from `pendingUploads`, it does not cancel the corresponding timeout or remove it from `pendingTimeouts`. This means a live timer handle stays in `pendingTimeouts` until the timeout eventually fires on its own. While the timer's callback is safe (it calls `pendingUploads.delete` and `pendingTimeouts.delete` on an already-absent entry), it is inconsistent with the rest of the PR, which explicitly cancels timers on all other removal paths.

To keep the cleanup uniform — and to prevent `clearPendingUploads` from iterating over a timer that is about to self-fire — consider cancelling and removing the timeout here as well:

```suggestion
  if (Date.now() - entry.createdAt > PENDING_UPLOAD_TTL_MS) {
    pendingUploads.delete(id);
    const timeout = pendingTimeouts.get(id);
    if (timeout) {
      clearTimeout(timeout);
      pendingTimeouts.delete(id);
    }
    return undefined;
  }
```

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

@PinoHouse PinoHouse force-pushed the fix/msteams-pending-upload-timeout-leak branch from 38d9f2b to 69eeab0 Compare March 3, 2026 13:35
@PinoHouse @claude
fix(msteams): clear pending upload timeout on removal
7afb37b 
Track setTimeout handles in a separate map and clear them when uploads
are removed or the store is cleared, preventing orphaned timer callbacks.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@PinoHouse PinoHouse force-pushed the fix/msteams-pending-upload-timeout-leak branch from 69eeab0 to 7afb37b Compare March 3, 2026 13:40
@BradGroux BradGroux self-assigned this Mar 10, 2026
@BradGroux
Copy link
Copy Markdown
Contributor

BradGroux commented Mar 10, 2026

Hi @PinoHouse — thanks for the submission. I’m the new Microsoft Teams maintainer for OpenClaw. Please give me a day or two to work through the open Teams backlog. Also, join the Twitter community for daily MS Teams feedback + updates: https://x.com/i/communities/2031170403607974228

1 similar comment
@BradGroux
Copy link
Copy Markdown
Contributor

BradGroux commented Mar 10, 2026

Hi @PinoHouse — thanks for the submission. I’m the new Microsoft Teams maintainer for OpenClaw. Please give me a day or two to work through the open Teams backlog. Also, join the Twitter community for daily MS Teams feedback + updates: https://x.com/i/communities/2031170403607974228

@BradGroux BradGroux mentioned this pull request Mar 17, 2026
Draft
@johnsonshi
Copy link
Copy Markdown
Contributor

johnsonshi commented Mar 18, 2026

I took a pass through this one. The fix is nicely scoped and the main paths look right: timeout handles are now tracked and cleared on explicit removal and bulk clear, which addresses the orphaned-callback issue this PR is targeting.

One small gap I’d call out before merge: getPendingUpload() still has a manual-expiry branch that deletes the upload entry but does not clear/delete the matching timeout handle from pendingTimeouts. That seems low risk because the scheduled callback will later clean up the maps again, but it does leave a stale tracked timer around until then and is inconsistent with the cleanup discipline introduced elsewhere in this patch.

I also don’t see any test changes in this PR, so a small unit test around timer cleanup / manual expiry would be a good way to lock this behavior down. If you update that manual-expiry branch to clear the timeout as well, this looks ready to me.

@BradGroux BradGroux merged commit 83149ed into openclaw:main Apr 1, 2026
28 checks passed
@github-actions github-actions bot mentioned this pull request Apr 1, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@BradGroux BradGroux

Labels

channel: msteams Channel integration: msteams size: XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@PinoHouse @BradGroux @johnsonshi