Skip to content

fix(googlechat): bound control and media requests#102227

Merged
steipete merged 3 commits into
openclaw:mainfrom
hugenshen:fix/googlechat-api-request-timeout
Jul 9, 2026
Merged

fix(googlechat): bound control and media requests#102227
steipete merged 3 commits into
openclaw:mainfrom
hugenshen:fix/googlechat-api-request-timeout

Conversation

@hugenshen

@hugenshen hugenshen commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Outbound Google Chat requests had no owner-level deadline. A stalled Google endpoint, proxy, or response body could therefore hold a Gateway request lane indefinitely.

Why This Change Was Made

All Google Chat REST operations already share withGoogleChatResponse, so that remains the single deadline owner. The maintainer rewrite separates request classes instead of imposing one 30-second deadline on everything:

  • JSON/control operations use a 30-second total deadline.
  • Uploads and downloads get bounded, size-aware total budgets (30-second grace, 256 KiB/s planning floor, 15-minute cap).
  • JSON, error, and streamed media bodies independently fail after 30 seconds without a chunk.
  • Existing byte caps and guarded-fetch cleanup remain authoritative.

This distinction matters because Google documents Chat uploads up to 200 MB: https://developers.google.com/workspace/chat/api/reference/rest/v1/media/upload

User Impact

Stalled Google Chat sends, reactions, probes, uploads, downloads, and response bodies now terminate and release their request lane. Legitimate attachments retain substantially more than 30 seconds to transfer, so the timeout fix does not regress slower or larger media operations.

Evidence

Prepared head: 1a86c03fb9e528ab759e9c95d57af83aef23cef0

  • Blacksmith Testbox tbx_01kx3ejrk9vpfphhtfr48t0c95: pnpm test extensions/googlechat/src/targets.test.ts — 33/33 passed on the final rebased tree.
  • Blacksmith Testbox check:changed — extension/test typechecks, lint, dependency and package guards, database-first guard, media-helper roundtrip, and import-cycle check passed.
  • Controlled live socket — production fetchWithSsrFGuard aborted a non-responsive loopback HTTP server in 134 ms against a 100 ms deadline.
  • Live Google endpoint — chat.googleapis.com returned the expected unauthenticated 401 through the production guarded-fetch boundary.
  • Fresh branch autoreview — clean, no accepted or actionable findings.
  • git diff --check — clean.

Authenticated send/upload proof was unavailable because the Testbox has neither GOOGLE_CHAT_SERVICE_ACCOUNT nor GOOGLE_CHAT_SERVICE_ACCOUNT_FILE; no credential value was inspected.

  • AI-assisted
  • I understand what the code does
  • Change is focused and does not mix unrelated concerns

@openclaw-barnacle openclaw-barnacle Bot added channel: googlechat Channel integration: googlechat size: S labels Jul 8, 2026
@clawsweeper

clawsweeper Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 9, 2026, 9:44 AM ET / 13:44 UTC.

Summary
The PR adds bounded Google Chat REST deadlines for control calls, size-aware media transfers, 30-second idle body reads, focused tests, and a changelog note.

PR surface: Source +44, Tests +94, Docs +1. Total +139 across 3 files.

Reproducibility: yes. source inspection gives a high-confidence reproduction path: current main routes Google Chat API calls through fetchWithSsrFGuard without an owner deadline, so a stalled endpoint or body can hold the lane. The PR adds after-fix controlled-stall proof, but not an authenticated before/after Google Chat send.

Review metrics: 1 noteworthy metric.

  • Deadline policy surfaces: 3 added: control total, media total, body idle. These are the behavior-changing timeout classes maintainers should notice before accepting the Google Chat upgrade behavior.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
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:

  • none.

Risk before merge

  • [P1] New deadlines intentionally change upgrade behavior: Google Chat control requests now fail after 30 seconds and media after a bounded size-aware budget instead of waiting indefinitely.
  • [P1] Authenticated Google Chat send/upload proof is still unavailable; the available real proof covers a controlled production-guard stall and unauthenticated Google endpoint reachability.
  • [P1] Exact-head CI has an unrelated native-app-i18n shard failure that should be rerun or cleared before landing.

Maintainer options:

  1. Land the split deadline model (recommended)
    Accept the intentional upgrade behavior once CI is green because the PR now separates lightweight control calls from media transfer budgets and preserves large attachment headroom.
  2. Retune deadlines before merge
    If maintainers want a wider compatibility window, adjust the 30-second control deadline or the media planning floor/cap and keep the focused stall tests aligned.
  3. Wait for credentialed Google Chat proof
    If authenticated production send/upload evidence is required, pause landing until a maintainer can run a credentialed Google Chat proof path.

Next step before merge

  • No automated repair is indicated; maintainer landing should rerun or clear the unrelated CI failure and accept the timeout compatibility tradeoff.

Security
Cleared: The diff changes Google Chat request deadlines and body readers without adding dependencies, CI/workflow changes, secret handling, permissions, or package execution surfaces.

Review details

Best possible solution:

Land the split-deadline implementation after exact-head CI is green or the unrelated i18n failure is explicitly cleared, keeping short control deadlines and size-aware media budgets.

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

Yes, source inspection gives a high-confidence reproduction path: current main routes Google Chat API calls through fetchWithSsrFGuard without an owner deadline, so a stalled endpoint or body can hold the lane. The PR adds after-fix controlled-stall proof, but not an authenticated before/after Google Chat send.

Is this the best way to solve the issue?

Yes: the shared Google Chat response helper is the right owner boundary, and the final split between control deadlines, size-aware media budgets, and idle body guards addresses the earlier overbroad global timeout. Per-caller duplicated timeout logic would be more fragile.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. Sufficient: the PR body and follow-up comment include exact-head Testbox output, controlled live socket timeout through the production guarded-fetch path, and real unauthenticated Google endpoint reachability; private credentialed send/upload proof was unavailable.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit. Replaced prior rating: 🦪 silver shellfish.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

  • P2: This is a focused Google Chat channel availability fix with limited blast radius, but it affects real send, upload, download, and reaction paths.
  • merge-risk: 🚨 compatibility: Existing slow Google Chat operations that previously waited indefinitely can now fail at the new control or media deadline.
  • merge-risk: 🚨 message-delivery: The changed helper sits on outbound Google Chat message and attachment delivery paths, so timeout tuning can affect valid sends or uploads.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit. Replaced prior rating: 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Sufficient (live_output): Sufficient: the PR body and follow-up comment include exact-head Testbox output, controlled live socket timeout through the production guarded-fetch path, and real unauthenticated Google endpoint reachability; private credentialed send/upload proof was unavailable.
  • proof: sufficient: Contributor real behavior proof is sufficient. Sufficient: the PR body and follow-up comment include exact-head Testbox output, controlled live socket timeout through the production guarded-fetch path, and real unauthenticated Google endpoint reachability; private credentialed send/upload proof was unavailable.
Evidence reviewed

PR surface:

Source +44, Tests +94, Docs +1. Total +139 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 1 50 6 +44
Tests 1 99 5 +94
Docs 1 1 0 +1
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 150 11 +139

What I checked:

  • Current main has no Google Chat request deadline: On current main, the shared Google Chat helper calls fetchWithSsrFGuard with url, init, and auditContext, but without timeoutMs or a caller signal. (extensions/googlechat/src/api.ts:47, f6b990124353)
  • PR splits request deadline classes: The PR head adds a 30-second control deadline, size-aware media transfer budgets with a 15-minute cap, and passes the selected timeoutMs through the shared helper. (extensions/googlechat/src/api.ts:17, 1a86c03fb9e5)
  • PR covers media and control behavior: The updated Google Chat target tests assert size-derived media deadlines, stalled media/error/upload body cancellation, and the 30-second send deadline. (extensions/googlechat/src/targets.test.ts:345, 1a86c03fb9e5)
  • SDK body idle contract checked: readResponseWithLimit delegates chunk reads through readChunkWithIdleTimeout, which cancels the reader and rejects when a response body stops producing chunks. (src/infra/http-body.ts:131, f6b990124353)
  • Media size contract checked: Official Google Chat media upload docs describe attachments up to 200 MB, which supports treating media as a different timeout class from lightweight JSON/control requests. (developers.google.com)
  • PR proof and CI checked: The PR body and maintainer comment report exact-head Testbox coverage, controlled live socket timeout through production fetchWithSsrFGuard, and unauthenticated chat.googleapis.com reachability; live checks show one unrelated native-app-i18n CI shard failure on the same head. (1a86c03fb9e5)

Likely related people:

  • steipete: GitHub history shows repeated Google Chat API/media commits, and the final PR head was rewritten by this account with the split deadline approach. (role: recent area contributor and final rewrite author; confidence: high; commits: 6ecc18463753, 53d007bc878c, 58e82d91bafd; files: extensions/googlechat/src/api.ts, extensions/googlechat/src/actions.ts, extensions/googlechat/src/targets.test.ts)
  • vincentkoc: GitHub history shows prior Google Chat API JSON/error handling and SDK import work relevant to the response-reader and helper boundary touched here. (role: recent Google Chat API contributor; confidence: medium; commits: 60a6945a6e87, 2b67a3f76e90, 4f3d81b91855; files: extensions/googlechat/src/api.ts)
  • wangmiao0668000666: GitHub commit history shows the recent change that replaced unbounded Google Chat JSON/error reads with bounded provider helpers, adjacent to this PR's body-read timeout work. (role: recent adjacent contributor; confidence: medium; commits: 4f3d81b91855; files: extensions/googlechat/src/api.ts, extensions/googlechat/src/auth.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 (3 earlier review cycles)
  • reviewed 2026-07-08T15:46:56.125Z sha 498a5a2 :: needs real behavior proof before merge. :: [P2] Split the media timeout from lightweight API calls
  • reviewed 2026-07-08T15:54:18.874Z sha 7ca53fe :: needs real behavior proof before merge. :: [P2] Split the media timeout from lightweight API calls
  • reviewed 2026-07-08T16:03:13.376Z sha 7ca53fe :: needs real behavior proof before merge. :: [P2] Split the media timeout from lightweight API calls

@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. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jul 8, 2026
@steipete steipete self-assigned this Jul 9, 2026
@steipete
steipete force-pushed the fix/googlechat-api-request-timeout branch from 7ca53fe to 1a86c03 Compare July 9, 2026 13:15
@steipete steipete changed the title fix(googlechat): add 30 s request timeout to withGoogleChatResponse fix(googlechat): bound control and media requests Jul 9, 2026
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Land-ready maintainer rewrite at 1a86c03fb9e528ab759e9c95d57af83aef23cef0:

  • separated the 30 s total deadline for control/JSON requests from size-aware media deadlines (30 s grace, 256 KiB/s planning floor, 15 min ceiling)
  • added independent 30 s idle guards for successful JSON, error snippets, uploads, and streamed downloads while preserving the existing byte caps
  • consolidated the regression coverage into the owning Google Chat target tests and added the changelog credit
  • Testbox: 33 focused Google Chat assertions green; full changed gate green across extension tests, tsgo, lint, dependency/database/media guards, and import-cycle checks
  • controlled live loopback through production fetchWithSsrFGuard: a 100 ms deadline aborted in 134 ms with TimeoutError
  • real unauthenticated chat.googleapis.com request reached Google through the production guard and returned the expected 401; authenticated send/upload proof was not possible because the Testbox has no Google Chat service-account credential
  • fresh autoreview after the final rebase: no actionable findings

Exact-head hosted CI: https://github.com/openclaw/openclaw/actions/runs/29020719649 (still running at this update).

@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

CI follow-up: the Google Chat change remains land-ready. The only failing job is the unrelated native-app i18n inventory assertion in test/scripts/native-app-i18n.test.ts.

This is current-main baseline breakage from the Android orphan-screen removal, not a PR regression:

  • the test file, generated native-source inventory, and entire apps/android tree have identical Git object IDs at this PR head and current origin/main
  • a focused local rerun reproduces the same single failure with the PR untouched
  • the Google Chat-focused Testbox suite, changed-surface gate, controlled timeout loopback, and fresh autoreview remain green
  • the failed-only GitHub rerun repeated the same exact-head failure, confirming the deterministic baseline rather than a transient PR failure

The one-line baseline repair is isolated in #102857. Per the maintainer-requested unrelated-CI exception for this landing batch, I will not mix it into this Google Chat PR.

@steipete
steipete merged commit 2d5dd6c into openclaw:main Jul 9, 2026
145 of 151 checks passed
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 10, 2026
* fix(googlechat): add 30 s request timeout to withGoogleChatResponse

* fix(googlechat): satisfy ResolvedGoogleChatAccount in api.test.ts

* fix(googlechat): bound control and media requests

Co-authored-by: NIO <[email protected]>

---------

Co-authored-by: NIO <[email protected]>
Co-authored-by: Peter Steinberger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: googlechat Channel integration: googlechat merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants