Skip to content

fix(gmail): circuit-break watch rate limits#665

Merged
steipete merged 1 commit into
mainfrom
fix/gmail-watch-retry-after-643
May 30, 2026
Merged

fix(gmail): circuit-break watch rate limits#665
steipete merged 1 commit into
mainfrom
fix/gmail-watch-retry-after-643

Conversation

@steipete
Copy link
Copy Markdown
Collaborator

Summary

  • persist a per-account Gmail watch rateLimitedUntilMs circuit when Gmail push handling hits 429
  • skip Gmail API fetches while the circuit is open and return 503 with Retry-After
  • surface the circuit timestamp in gmail watch status and add regression coverage for history/message 429s

Fixes #643.

Testing

  • go test ./internal/cmd -run 'GmailWatchServer_(History429|Message429|OpenCircuit)|UpdateStateAfterHistoryKeepsConcurrentRateLimitCircuit'
  • go test ./internal/cmd -run 'GmailWatchServer_(History429|Message429|OpenCircuit)'
  • go test ./internal/cmd -run 'GmailWatch(Server|RateLimit|WatchState|ServeHTTP)'
  • go test ./internal/cmd ./internal/googleapi
  • make test
  • make lint
  • autoreview --mode local clean
  • Live clawdbot smoke: gmail watch status returned stored watch state and real gmail labels list returned 16 labels

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 30, 2026

Codex review: needs maintainer review before merge. Reviewed May 30, 2026, 2:51 PM ET / 18:51 UTC.

Summary
The PR persists a per-account Gmail watch rate-limit timestamp, skips Gmail API work while the circuit is open, returns 503 with Retry-After, shows the timestamp in watch status, and adds regression tests.

Reproducibility: yes. Source inspection on current main shows Gmail 429s from history/message fetches flow back as generic handler errors and would be retried without a persisted per-account circuit.

Review metrics: 2 noteworthy metrics.

  • Changed surface: 5 files, +350/-4. The patch is concentrated in Gmail watch state, push handling, status output, tests, and one release-note line.
  • Rate-limit coverage: 4 new tests. The added tests exercise the two Gmail API 429 paths and the open-circuit behavior that caused the report.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
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] The PR body reports normal live Gmail smoke, but the actual 429 Retry-After path is covered by httptest regression tests rather than a forced live Gmail 429 response.

Maintainer options:

  1. Decide the mitigation before merge
    Land the focused circuit-breaker after maintainer review, keeping the per-account persisted state and regression coverage as the durable fix for the linked watcher bug.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No automated repair is needed because diff review found no narrow blocking defect; the remaining action is maintainer review and merge gating.

Security
Cleared: No concrete security or supply-chain issue was found; the diff adds local retry-state handling and no new dependency, secret, permission, or code-execution surface.

Review details

Best possible solution:

Land the focused circuit-breaker after maintainer review, keeping the per-account persisted state and regression coverage as the durable fix for the linked watcher bug.

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

Yes. Source inspection on current main shows Gmail 429s from history/message fetches flow back as generic handler errors and would be retried without a persisted per-account circuit.

Is this the best way to solve the issue?

Yes. The PR is a narrow maintainable fix: persist the Retry-After deadline, check it before Gmail API calls, return 503/Retry-After while open, and cover the history and message 429 paths.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against e4941e776504.

Label changes

Label changes:

  • add P2: This fixes a bounded Gmail watcher reliability bug that can repeatedly consume Gmail API quota and delay message delivery.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate is not applicable to this collaborator PR; the body still lists tests and a live smoke check for normal Gmail access.

Label justifications:

  • P2: This fixes a bounded Gmail watcher reliability bug that can repeatedly consume Gmail API quota and delay message delivery.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate is not applicable to this collaborator PR; the body still lists tests and a live smoke check for normal Gmail access.
Evidence reviewed

What I checked:

  • Current main lacks a watch-level 429 circuit: Current main returns Gmail history/message fetch errors directly and ServeHTTP maps them to a generic 500, so repeated Pub/Sub redeliveries can call Gmail again. (internal/cmd/gmail_watch_server.go:190, e4941e776504)
  • PR adds circuit-break handling: The PR diff adds gmailWatchRateLimitError, persisted RateLimitedUntilMs state, pre-fetch circuit checks, Retry-After parsing, and 503/Retry-After responses for open circuits. (internal/cmd/gmail_watch_server.go:94, 1370b13cb826)
  • PR adds targeted regression coverage: The new test file covers history.list 429, messages.get 429, already-open circuits avoiding Gmail service calls, and preserving a concurrent circuit during state updates. (internal/cmd/gmail_watch_rate_limit_test.go:1, 1370b13cb826)
  • Repository policy read: AGENTS.md was read fully and the review stayed in PR-review mode without switching branches or editing files. (AGENTS.md:1, e4941e776504)
  • Feature history provenance: The Gmail watch server, state, and status output in current main trace to the v0.19.0 release commit by Peter Steinberger. (internal/cmd/gmail_watch_server.go:1, b25a3c029b37)

Likely related people:

  • steipete: Current-main blame and file history show the Gmail watch server, state model, and status output were introduced in the v0.19.0 release commit by Peter Steinberger; the same handle authored this focused follow-up. (role: feature-history owner; confidence: high; commits: b25a3c029b37, 1370b13cb826; files: internal/cmd/gmail_watch_server.go, internal/cmd/gmail_watch_cmds.go, internal/cmd/gmail_watch_types.go)
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.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. labels May 30, 2026
@steipete steipete merged commit 2c844b1 into main May 30, 2026
9 checks passed
@steipete steipete deleted the fix/gmail-watch-retry-after-643 branch May 30, 2026 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gmail watcher should honor 429 retry-after and circuit-break per account

1 participant