Skip to content

fix(msteams): bound remote media saves with header and idle timeouts#109030

Open
hugenshen wants to merge 1 commit into
openclaw:mainfrom
hugenshen:fix/msteams-remote-media-idle-timeout
Open

fix(msteams): bound remote media saves with header and idle timeouts#109030
hugenshen wants to merge 1 commit into
openclaw:mainfrom
hugenshen:fix/msteams-remote-media-idle-timeout

Conversation

@hugenshen

@hugenshen hugenshen commented Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where Microsoft Teams inbound remote media saves could hang indefinitely when an attachment response returned headers and then stalled mid-body. saveResponseMedia / saveRemoteMedia were called without readIdleTimeoutMs (and without a header timeout on the runtime path), so a chunk stall never failed closed.

Why This Change Was Made

Pass the same media timeout policy already used by Mattermost/Zalo/Tlon: responseHeaderTimeoutMs: 120_000 and readIdleTimeoutMs: 30_000 through every production caller that hands a response to saveResponseMedia/saveRemoteMedia, not just the direct-fetch path.

MSTEAMS_MEDIA_READ_IDLE_TIMEOUT_MS / MSTEAMS_MEDIA_RESPONSE_HEADER_TIMEOUT_MS live in shared.ts so the SharePoint direct path, Bot Framework attachment view save, and Graph hosted-content $value save all reference one constant instead of drifting.

User Impact

Before: A stalled Teams attachment download body could hang inbound media persistence forever — on the direct SharePoint path, the Bot Framework personal-chat attachment path, or the Graph hosted-content path.

After: All three attachment body reads fail closed after 30s of idle (and header waits after 120s on the guarded path), so the inbound turn can surface a media failure instead of hanging. Slow-but-progressing transfers still complete (idle resets on each chunk).

Evidence

  • Changed: extensions/msteams/src/attachments/shared.ts, remote-media.ts, bot-framework.ts, graph.ts, plus *.test.ts regressions asserting readIdleTimeoutMs: 30_000 is forwarded
  • Production caller paths:
    • downloadAndStoreMSTeamsRemoteMedia → saveRemoteMediaDirect → saveResponseMedia({ readIdleTimeoutMs: 30_000 }), and the non-direct path → saveRemoteMedia({ responseHeaderTimeoutMs: 120_000, readIdleTimeoutMs: 30_000 })
    • saveBotFrameworkAttachmentView → saveResponseMedia({ readIdleTimeoutMs: 30_000 })
    • Graph hosted-content $value → saveResponseMedia({ readIdleTimeoutMs: 30_000 })
  • CI cleanup: removed unused committed scripts/proof-msteams-media-idle-timeout.mts and local-only .pr-bodies/ from the merge commit (oxlint/knip noise). Proof stays ephemeral under /tmp.
  • Rebased onto latest upstream/main and squashed to one commit (700269d7ffb).

Real behavior proof

Behavior or issue addressed

Stalled Teams remote-media bodies fail closed on the channel's production 30s idle budget when driven through downloadAndStoreMSTeamsRemoteMedia (not by calling saveResponseMedia alone). Completing and slow-but-progressing transfers still save.

Canonical reachability path

downloadAndStoreMSTeamsRemoteMedia({ useDirectFetch: true, fetchImpl }) → saveRemoteMediaDirect → saveResponseMedia({ readIdleTimeoutMs: MSTEAMS_MEDIA_READ_IDLE_TIMEOUT_MS })

Sibling Bot Framework / Graph callers share the same idle constant and are covered by focused vitest forwarding regressions.

Shared helper / provider constraint check

Uses existing saveResponseMedia idle-read contract from openclaw/plugin-sdk/media-runtime. No new downloader or config key.

Real environment tested

Live node:http server on 127.0.0.1 (headers + one PNG chunk then stall; completing /small; slow /trickle with per-chunk gaps under idle but total wall time above idle). Production Teams entry downloadAndStoreMSTeamsRemoteMedia with useDirectFetch: true against those sockets. Temp OPENCLAW_STATE_DIR media store. Node v22.23.1, head 700269d7ffb137835775a13323c3031e188050d7.

Exact steps or command run after this patch

# Ephemeral driver (not committed):
node --import tsx /tmp/proof-msteams-media-idle-timeout.mts

Evidence after fix

entry=downloadAndStoreMSTeamsRemoteMedia useDirectFetch=true idleMs=30000
OPENCLAW_STATE_DIR=/var/folders/tt/w4j113gx3v96vdh1h7x7zcxc0000gn/T/oc-msteams-media-proof-04oNNt
node=v22.23.1
  ok: stalled downloadAndStoreMSTeamsRemoteMedia rejected — elapsed=30095ms
  ok: failed within idle budget + slack — elapsed=30095ms idle=30000ms
  ok: idle-timeout failure surfaced — Failed to fetch media from http://127.0.0.1:51061/stall: Media download stalled: no data received for 30000ms
  ok: unbounded attachment read still hanging after 2000ms — state=still-hanging
  ok: completing downloadAndStoreMSTeamsRemoteMedia saved — {"path":"/var/folders/tt/w4j113gx3v96vdh1h7x7zcxc0000gn/T/oc-msteams-media-proof-04oNNt/media/inbound/6af80e09-ae20-4b27-8981-10b0728359c1.png","contentType":"image/png"}
  ok: slow-but-progressing Teams entry still saved (total > idle) — elapsed=48031ms idleBudget=30000ms

ALL PROOF ASSERTIONS PASSED

Observed result after fix

Against a real stalled image/png socket, the production Teams entry downloadAndStoreMSTeamsRemoteMedia fails closed in ~30095ms with Media download stalled: no data received for 30000ms. Unbounded arrayBuffer() on the same socket is still hanging after 2000ms. A completing attachment still saves through the same entry. A trickle whose total time (48031ms) exceeds the 30000ms idle budget still saves, because idle resets on each chunk.

What was not tested

Live stall against a real Microsoft Teams attachment host, Bot Framework connector, or Graph endpoint. Bot Framework / Graph entry points were not driven against the live socket in this proof (argument-forwarding regressions cover those call sites).

Fix classification

bugfix — timeout/hang

AI Assistance

AI-assisted. Author reviewed the Teams media save timeout wiring, CI cleanup (removed unused proof script), rebase onto latest main, and the entry-point real-socket proof output.

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: XS labels Jul 16, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jul 16, 2026
@clawsweeper

clawsweeper Bot commented Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 18, 2026, 11:34 AM ET / 15:34 UTC.

Summary
The branch defines shared Microsoft Teams media header and idle timeout constants, forwards them through direct, runtime, Bot Framework, and Graph attachment-save paths, and adds regression assertions.

PR surface: Source +19, Tests +28. Total +47 across 7 files.

Reproducibility: yes. at source level and in the supplied real-entry-point proof: route a response that yields headers and one chunk, then stalls, through downloadAndStoreMSTeamsRemoteMedia with direct fetching enabled.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Obtain normal Microsoft Teams/channel-owner merge review for the intentional 30-second idle compatibility boundary.

Risk before merge

  • [P1] The new 30-second idle ceiling intentionally turns attachment streams that pause longer than that interval into failures; maintainers should confirm this is the intended compatibility tradeoff for unusually intermittent Microsoft-hosted downloads before merge.

Maintainer options:

  1. Confirm the bounded-read policy (recommended)
    Accept the 30-second idle ceiling as the channel-wide behavior for stalled remote attachment streams, relying on the proof that continuously progressing transfers remain allowed.
  2. Adjust the shared budget before merge
    If Teams-hosted attachment streams can legitimately pause beyond 30 seconds, revise the shared timeout policy and rerun the real entry-point proof for the selected value.

Next step before merge

  • [P2] No mechanical repair remains; a channel-area reviewer should confirm the timeout tradeoff and approve the exact head through normal merge review.

Security
Cleared: The patch only forwards existing timeout options through Microsoft Teams media-save calls and adds tests; it introduces no dependency, permission, credential, or code-execution change.

Review details

Best possible solution:

Keep one shared Teams media timeout policy, retain forwarding coverage for every response-save caller, and merge once the channel owner accepts the deliberate idle-timeout compatibility boundary on the exact reviewed head.

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

Yes, at source level and in the supplied real-entry-point proof: route a response that yields headers and one chunk, then stalls, through downloadAndStoreMSTeamsRemoteMedia with direct fetching enabled.

Is this the best way to solve the issue?

Yes. Reusing the existing media-runtime timeout contract and centralizing the policy in the Teams attachment helper is narrower and less drift-prone than adding a separate downloader or configuration surface.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against faf3dbdda833.

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR supplies after-fix live-output proof through the production direct Teams entry point against a socket that stalls mid-body, plus successful completion and slow-progress controls; redact local paths and addresses if reposting evidence.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR supplies after-fix live-output proof through the production direct Teams entry point against a socket that stalls mid-body, plus successful completion and slow-progress controls; redact local paths and addresses if reposting evidence.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P1: An indefinitely stalled inbound Teams attachment can block real channel media processing until this fix is landed.
  • merge-risk: 🚨 compatibility: The PR intentionally changes existing attachment-read behavior from unbounded waiting to a 30-second idle failure, which may affect unusually intermittent downloads.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR supplies after-fix live-output proof through the production direct Teams entry point against a socket that stalls mid-body, plus successful completion and slow-progress controls; redact local paths and addresses if reposting evidence.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR supplies after-fix live-output proof through the production direct Teams entry point against a socket that stalls mid-body, plus successful completion and slow-progress controls; redact local paths and addresses if reposting evidence.
Evidence reviewed

PR surface:

Source +19, Tests +28. Total +47 across 7 files.

View PR surface stats
Area Files Added Removed Net
Source 4 20 1 +19
Tests 3 50 22 +28
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 7 70 23 +47

What I checked:

Likely related people:

  • hugenshen: Authored the single exact-head change and supplied entry-point behavior proof covering the affected Teams remote-media path; broader merged-history ownership could not be re-established from the read-only inspection environment. (role: current patch author and recent area contributor; confidence: low; commits: 700269d7ffb1; files: extensions/msteams/src/attachments/remote-media.ts, extensions/msteams/src/attachments/shared.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (7 earlier review cycles)
  • reviewed 2026-07-16T13:30:54.107Z sha 1382e71 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-17T13:12:37.752Z sha 1382e71 :: needs real behavior proof before merge. :: [P1] Apply the idle timeout to every Teams response-save path
  • reviewed 2026-07-17T13:55:37.467Z sha 1382e71 :: needs real behavior proof before merge. :: [P1] Apply the idle timeout to every Teams response-save path
  • reviewed 2026-07-17T14:24:48.710Z sha 1382e71 :: needs real behavior proof before merge. :: [P1] Bound every Teams response-save path
  • reviewed 2026-07-17T15:08:35.344Z sha 1382e71 :: needs real behavior proof before merge. :: [P1] Cover the remaining Teams response-save callers
  • reviewed 2026-07-17T16:24:55.849Z sha 32c8858 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-18T15:30:37.726Z sha 700269d :: needs maintainer review before merge. :: none

@BradGroux BradGroux mentioned this pull request Jul 17, 2026
Draft
@hugenshen

hugenshen commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@hugenshen

hugenshen commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. and removed P2 Normal backlog priority with limited blast radius. labels Jul 17, 2026
@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts size: M and removed size: XS labels Jul 17, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 17, 2026
@hugenshen
hugenshen force-pushed the fix/msteams-remote-media-idle-timeout branch from 32c8858 to 700269d Compare July 18, 2026 15:27
@openclaw-barnacle openclaw-barnacle Bot removed the scripts Repository scripts label Jul 18, 2026
@hugenshen

hugenshen commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Updated for CI cleanup + entry-point proof:

  1. CI cleanup: Removed committed scripts/proof-msteams-media-idle-timeout.mts (oxlint/knip unused-file) and local-only .pr-bodies/ from the merge commit. Squashed to one commit on latest main (700269d7ffb).

  2. Real behavior proof now drives the Teams entry point downloadAndStoreMSTeamsRemoteMedia({ useDirectFetch: true }) → saveRemoteMediaDirect → saveResponseMedia against a live loopback node:http socket (not saveResponseMedia alone):

    • stalled body fails closed in ~30069ms with Media download stalled: no data received for 30000ms (production idle constant)
    • unbounded arrayBuffer() negative control still hanging after 2000ms
    • completing /small still saves through the same entry
    • slow-but-progressing /trickle total 48100ms > 30000ms idle still saves

Full transcript is in the updated PR body Real behavior proof section.

@clawsweeper

clawsweeper Bot commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@hugenshen

hugenshen commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Updated after CI cleanup + entry-point proof:

  1. CI cleanup: Removed committed scripts/proof-msteams-media-idle-timeout.mts and .pr-bodies/ from the branch (oxlint/knip unused-file noise). Product fix stays in extensions/msteams only. Squashed to one commit on latest main (700269d7ffb).

  2. Real behavior proof now drives the production Teams entry downloadAndStoreMSTeamsRemoteMedia({ useDirectFetch: true, fetchImpl }) against a live loopback node:http socket (not saveResponseMedia alone):

    • stalled body → idle timeout ~30095ms (Media download stalled: no data received for 30000ms)
    • unbounded arrayBuffer() still hanging after 2000ms (negative control)
    • completing /small still saves
    • slow-but-progressing trickle (total ~48031ms > 30000ms idle) still saves

Please re-check with the updated PR body transcript.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 18, 2026
@clawsweeper clawsweeper Bot added the status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. label Jul 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

channel: msteams Channel integration: msteams merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@hugenshen