fix(google): give avatar HTTP client a timeout

[SebTardif]

What Problem This Solves

clawdex import google --avatars downloads each contact photo through
fetchAvatarURL. That helper used http.DefaultClient, which has no
Timeout. Production attachAvatar already wraps the call in a 20s
lookupCtx, so a stalling photo host does not hang forever on the current
call path.

DefaultClient can still stall when a caller passes an unbounded context
(tests use t.Context(), and fetchAvatarURL is the public import
path). The Google adapter should own a client timeout, the same
belt-and-suspenders pattern as sibling HTTP clients.

This change adds a package-level avatarHTTPClient with
Timeout: avatarLookupTimeout (20s) and uses it for the GET. Request
contexts still apply via http.NewRequestWithContext.

Evidence

Live go run against a TCP listener that accepts and never writes HTTP.
A 50ms client returns in 51ms. http.DefaultClient with only a 2s request
context waits the full deadline:

$ go run /tmp/clawdex-avatar-stall-proof.go
stall listener 127.0.0.1:65011 (accepts, never writes HTTP)
timed-50ms client elapsed=51ms err=Get "http://127.0.0.1:65011/stall": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
DefaultClient plus 2s ctx elapsed=2.002s err=Get "http://127.0.0.1:65011/stall": context deadline exceeded

After the patch, fetchAvatarURL uses that timed client. Grep of the
production helper:

$ rg -n 'DefaultClient|avatarHTTPClient' internal/google/gog.go
30:var avatarHTTPClient = &http.Client{Timeout: avatarLookupTimeout}
323:	resp, err := avatarHTTPClient.Do(req)

A stalled photo URL through fetchAvatarURL(context.Background(), ...)
now returns in 52ms with Client.Timeout exceeded while awaiting headers
instead of sitting until an outer 2s bound.

Real behavior proof

• Behavior or issue addressed: Google avatar HTTP GETs use a dedicated client with a 20s Timeout so an unbounded caller context cannot wait forever on a peer that accepts and never responds.

• Real environment tested: macOS Darwin 25.6.0 arm64, Go 1.27.0, clawdex worktree /tmp/clawdex-F001 at branch fix/google-avatar-http-timeout.

• Exact steps or command run after this patch:

  go run /tmp/clawdex-avatar-stall-proof.go
  rg -n 'DefaultClient|avatarHTTPClient' internal/google/gog.go
  go test -count=1 -timeout 15s -run TestFetchAvatarURLClientTimeout -v ./internal/google

• Evidence after fix: terminal output from the patched tree:

  $ go run /tmp/clawdex-avatar-stall-proof.go
  stall listener 127.0.0.1:65011 (accepts, never writes HTTP)
  timed-50ms client elapsed=51ms err=Get "http://127.0.0.1:65011/stall": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
  DefaultClient plus 2s ctx elapsed=2.002s err=Get "http://127.0.0.1:65011/stall": context deadline exceeded
  
  $ rg -n 'DefaultClient|avatarHTTPClient' internal/google/gog.go
  30:var avatarHTTPClient = &http.Client{Timeout: avatarLookupTimeout}
  323:	resp, err := avatarHTTPClient.Do(req)
  
  $ go test -count=1 -timeout 15s -run TestFetchAvatarURLClientTimeout -v ./internal/google
  === RUN   TestFetchAvatarURLClientTimeout
      gog_test.go:393: stalled fetch returned in 52.127167ms: Get "http://127.0.0.1:65123/stall": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
  --- PASS: TestFetchAvatarURLClientTimeout (0.05s)
  PASS

• Observed result after fix: A stalling photo host no longer depends on the caller to cancel. avatarHTTPClient returns Client.Timeout exceeded while awaiting headers in about 50ms when the client Timeout is 50ms. Production keeps the 20s avatarLookupTimeout. http.DefaultClient is gone from fetchAvatarURL.

• What was not tested: A live gog contacts raw call against a real Google account whose photo URL hangs. Redirect-heavy CDN photo URLs.

Summary

fetchAvatarURL now uses avatarHTTPClient (Timeout: 20s) instead of
http.DefaultClient. http.NewRequestWithContext is unchanged so caller
deadlines still apply. Success-path coverage in TestFetchAvatarURL still
passes through the new client.

Related:

• Same adapter hang stop: fix(google): cap contacts list pagination #7
• Avatar size cap (open): fix(avatar): cap manual avatar file reads #11
• Introduced with e419451 (feat: fetch Google contact avatars, 2026-05-08)
• Go http.Client.Timeout includes connect, redirects, and body read; DefaultClient is zero

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed September 4, 2026, 1:01 PM ET / 17:01 UTC.

ClawSweeper review

What this changes

The PR gives Google contact-avatar downloads their own 20-second HTTP timeout and adds a regression test for a server that accepts a connection but never responds.

Regression provenance

Possible regression — probable (reviewed change; failure trace). No predecessor PR is attributed.

Merge readiness

✅ Ready for maintainer review

The focused timeout change remains absent from current main, has direct stalled-connection proof, and introduces no actionable correctness or security defect.

Priority: P3
Reviewed head: 12b36434f6fb2d318420ddac22a6da60e6c35249

Review scores

Measure	Result	What it means
Overall readiness	🦞 diamond lobster (5/6)	A small, well-scoped reliability patch with direct runtime proof and targeted regression coverage.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (terminal): The changed production owner is fetchAvatarURL; the supplied terminal trace and added test exercise it against a local server that accepts a real HTTP connection and never returns headers, recording timeout recovery after the client timeout is set to 50 ms.
Patch quality	🦞 diamond lobster (5/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The changed production owner is fetchAvatarURL; the supplied terminal trace and added test exercise it against a local server that accepts a real HTTP connection and never returns headers, recording timeout recovery after the client timeout is set to 50 ms.
Evidence reviewed	4 items	Current-main gap: Current main still sends avatar requests through http.DefaultClient, so this timeout change is not already implemented. Introduced implementation: The branch creates a client using the existing 20-second avatar lookup timeout and routes fetchAvatarURL through it without changing request-context handling. Timeout regression coverage: The new test invokes the production fetch helper against a local HTTP server that stalls after accepting the connection.
Findings	None	None.
Security	None	None.

How this fits together

The Google import adapter lists contacts, obtains photo URLs through the gog CLI, and downloads images into imported contact records. The new HTTP-client boundary limits avatar-request wait time while preserving caller cancellation.

flowchart LR
  A[Google contact import] --> B[Contact and photo lookup]
  B --> C[Avatar URL]
  C --> D[Timed HTTP download]
  D --> E[Avatar byte limit]
  E --> F[Imported contact avatar]

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch scope	2 files affected; production +3/-1, tests +46	The change is narrowly limited to the Google avatar request and its regression coverage.

Technical review

Best possible solution:

Merge the focused timeout guard and retain the stalled-connection regression coverage.

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

Yes: the added regression exercises the production helper against a real local HTTP server that stalls after accepting a request, and the PR records the observed timeout result.

Is this the best way to solve the issue?

Yes: applying the existing 20-second adapter timeout at the HTTP-client boundary preserves caller cancellation and adds a narrow defensive limit to the image request.

AGENTS.md: not found in the target repository.

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

Labels

Label changes:

• add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
• remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

• P3: This is defensive reliability hardening for an optional avatar-download path with bounded user impact.
• rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The changed production owner is fetchAvatarURL; the supplied terminal trace and added test exercise it against a local server that accepts a real HTTP connection and never returns headers, recording timeout recovery after the client timeout is set to 50 ms.
• proof: sufficient: Contributor real behavior proof is sufficient. The changed production owner is fetchAvatarURL; the supplied terminal trace and added test exercise it against a local server that accepts a real HTTP connection and never returns headers, recording timeout recovery after the client timeout is set to 50 ms.

Evidence

What I checked:

• Current-main gap: Current main still sends avatar requests through http.DefaultClient, so this timeout change is not already implemented. (internal/google/gog.go:320, baa945d86da0)
• Introduced implementation: The branch creates a client using the existing 20-second avatar lookup timeout and routes fetchAvatarURL through it without changing request-context handling. (internal/google/gog.go:30, 12b36434f6fb)
• Timeout regression coverage: The new test invokes the production fetch helper against a local HTTP server that stalls after accepting the connection. (internal/google/gog_test.go:352, 12b36434f6fb)
• Feature history: Available commit metadata associates the initial avatar feature with this path; line blame could not complete because the partial clone's promisor remote was unavailable. (internal/google/gog.go:146, e41945159b1d)

Likely related people:

• Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• Sebastien Tardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

• 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.

History

Review history (5 earlier review cycles)

• reviewed 2026-09-02T22:37:35.318Z sha 12b3643 :: needs maintainer review before merge. :: none
• reviewed 2026-09-03T07:51:12.078Z sha 12b3643 :: needs maintainer review before merge. :: none
• reviewed 2026-09-03T15:03:39.033Z sha 12b3643 :: needs maintainer review before merge. :: none
• reviewed 2026-09-03T23:50:34.991Z sha 12b3643 :: needs maintainer review before merge. :: none
• reviewed 2026-09-04T04:59:33.999Z sha 12b3643 :: needs maintainer review before merge. :: none