Skip to content

[codex] acknowledge terminal Gmail OAuth watch failures#930

Open
litang9 wants to merge 2 commits into
openclaw:mainfrom
litang9:codex/gmail-oauth-terminal-ack
Open

[codex] acknowledge terminal Gmail OAuth watch failures#930
litang9 wants to merge 2 commits into
openclaw:mainfrom
litang9:codex/gmail-oauth-terminal-ack

Conversation

@litang9

@litang9 litang9 commented Jul 19, 2026

Copy link
Copy Markdown

Summary

  • classify expired or revoked Gmail OAuth credentials as terminal processing failures
  • persist a durable auth-recovery marker before acknowledging terminal Pub/Sub deliveries
  • preserve the last successfully processed Gmail history cursor across watch start and watch renew
  • use Gmail's immediate watch-registration notification to catch up after re-authentication
  • keep transient Gmail API, rate-limit, persistence, and downstream hook failures retryable

Root cause

The Gmail watch consumers treated every non-rate-limit processing failure as retryable. When the OAuth refresh token returned invalid_grant, push delivery responded with HTTP 500 and pull delivery nacked the message. Pub/Sub then redelivered a failure that cannot recover without operator re-authentication, producing an unbounded retry and log storm.

Recovery contract

Terminal OAuth failures are acknowledged only after authRecoveryPending and the failure timestamp/reason are saved without advancing historyId or lastPushMessageId. A state write failure remains a normal processing error, so Pub/Sub keeps the delivery retryable.

After re-authenticating, run gog gmail watch renew or rerun gog gmail watch start. Successful registration preserves the old successful cursor while recovery is pending. Gmail sends an immediate notification for a successful watch registration, so the consumer catches up from that cursor without waiting for another mailbox change. Advancing history successfully clears the recovery marker. gog gmail watch status exposes the pending recovery state.

Temporary API failures, rate limits, and hook delivery failures retain their existing retry behavior.

Validation

  • go test ./internal/gmailwatch
  • focused internal/cmd tests covering start, renew, status, push acknowledgement, pull acknowledgement, persistence failure, and catch-up
  • make fmt-check
  • make lint
  • make deadcode
  • make build

The broader internal/cmd Gmail-watch test selection still has pre-existing shared-state failures when run as one package on this machine. The same failures reproduce on unmodified upstream/main; the focused regression tests above pass.

Redacted live Gmail/Pub/Sub evidence

Run on 2026-07-19 with commit 7b32e33be8a5. The run used:

  • a real Gmail account and Gmail-generated history event
  • a temporary real Pub/Sub pull subscription on the configured topic
  • an isolated --home and encrypted file keyring
  • a temporary Gmail label added to and then removed from one existing INBOX message
  • the real watch start, pull acknowledgement, watch renew, immediate Gmail notification, history fetch, and hook delivery paths

Only the isolated refresh token was replaced with an invalid value. The production token was never revoked or modified.

runtime_binary=7b32e33be8a5
delivery=real_gmail_pubsub_pull
terminal_oauth=invalid_grant pull_acknowledged=yes
recovery_pending=true reason=reauthentication_required cursor_preserved=yes
watch_renewed=yes expiration_updated=yes cursor_preserved=yes
recovery_pending_after_renew=true
renew_notification=real_gmail_pubsub_pull
hook_received=true message_count=3 history_set=true
cursor_advanced=yes recovery_pending_after=false
cleanup=label_removed,label_deleted,subscription_deleted,isolated_credentials_deleted

The invalid-token pull logged Gmail authorization requires re-authentication; acknowledging pull without advancing history after Google OAuth returned invalid_grant. After restoring the isolated token, watch renew retained the old cursor and pending marker. Gmail's immediate registration notification arrived through the temporary Pub/Sub subscription; the consumer fetched the retained history, delivered three message records to a local 204 hook, advanced the cursor, cleared the marker, and acknowledged the notification.

Account addresses, project/topic/subscription names, history IDs, Pub/Sub message IDs, Gmail message IDs, label IDs, endpoints, and all credential material are omitted. Cleanup was verified: no temporary process, directory, subscription, label, or credential copy remains.

Related OpenClaw lifecycle fix: openclaw/openclaw#96202.

@litang9
litang9 marked this pull request as ready for review July 19, 2026 10:53
@clawsweeper

clawsweeper Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 19, 2026, 8:52 AM ET / 12:52 UTC.

Summary
The PR records terminal Gmail OAuth failures as durable recovery state, acknowledges affected Pub/Sub deliveries without advancing the Gmail history cursor, and preserves that cursor across watch registration for catch-up after reauthentication.

Reproducibility: yes. for the source-level path: an OAuth refresh error carrying invalid_grant reaches the Gmail watch source setup or history fetch and is classified before the delivery adapter decides whether to acknowledge. The PR also supplies redacted after-fix output from a real Gmail/Pub/Sub pull run, although reproducing it independently requires live OAuth and Pub/Sub setup.

Review metrics: 2 noteworthy metrics.

  • Patch surface: 18 files affected; 534 added, 20 removed. The change spans the shared processor, push and pull adapters, persisted state, CLI output, docs, and targeted regression tests.
  • Delivery paths: 2 acknowledgement adapters changed. Both HTTP push and Pub/Sub pull delivery paths now handle terminal OAuth errors, so the policy must remain consistent across both modes.

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:

  • [P2] Confirm the intended acknowledgement-versus-retry policy for terminal OAuth failures before merge.

Risk before merge

  • [P1] Merging changes the existing failure contract: an invalid or revoked OAuth credential stops Pub/Sub redelivery after the recovery marker is saved, so operators must notice the pending state and reauthenticate before further Gmail changes are delivered.
  • [P1] The persisted watch-state schema gains recovery fields and intentionally preserves an older history cursor across start or renew while recovery is pending; maintainers should accept this upgrade behavior as the supported catch-up policy.

Maintainer options:

  1. Accept the durable recovery contract (recommended)
    Approve the explicit policy that terminal OAuth failures acknowledge only after recovery state is persisted and retain the prior cursor for reauthentication catch-up.
  2. Add a stronger operator recovery signal
    Before merge, add or require a clearer operational alerting path if gmail watch status alone is not sufficient for users to discover pending recovery.
  3. Pause on delivery-policy direction
    Do not merge until maintainers decide whether avoiding retry storms outweighs retaining automatic Pub/Sub retry behavior for expired credentials.

Next step before merge

  • [P2] A maintainer must choose the intended terminal-OAuth delivery contract; no safe mechanical repair is indicated by the current diff.

Maintainer decision needed

  • Question: Should Gmail watch acknowledge a terminal OAuth failure after durably recording recovery state, rather than continue Pub/Sub retries until an operator reauthenticates?
  • Rationale: This is an intentional delivery-policy tradeoff: it prevents unbounded retry storms but requires an operator to act before subsequent mailbox changes are caught up.
  • Likely owner: salmonumbrella — Their recent Gmail watch history work is the closest available ownership signal for the delivery and cursor semantics.
  • Options:
    • Accept durable recovery acknowledgement (recommended): Merge the proposed policy: save recovery state, acknowledge terminal OAuth failures, and use retained-cursor catch-up after reauthentication.
    • Keep retry-until-reauthentication behavior: Reject acknowledgement for terminal OAuth failures and retain the existing Pub/Sub retry behavior despite repeated failures.
    • Require a different recovery surface: Pause the PR until maintainers define an alternative operator alerting or recovery mechanism before changing acknowledgement semantics.

Security
Cleared: The diff adds no dependency, workflow, permission, secret-handling, or third-party execution change; its OAuth error handling and persisted recovery marker are consistent with the stated Gmail watch purpose.

Review details

Best possible solution:

Adopt the durable-marker recovery contract only if maintainers want terminal OAuth failures to stop redelivery storms while preserving the last confirmed cursor, with gmail watch status and the documented reauthentication-plus-renew flow as the operator recovery path.

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

Yes for the source-level path: an OAuth refresh error carrying invalid_grant reaches the Gmail watch source setup or history fetch and is classified before the delivery adapter decides whether to acknowledge. The PR also supplies redacted after-fix output from a real Gmail/Pub/Sub pull run, although reproducing it independently requires live OAuth and Pub/Sub setup.

Is this the best way to solve the issue?

Unclear until maintainers choose the delivery policy. The implementation is a narrow and internally consistent way to prevent retry storms while retaining catch-up state, but acknowledging a terminal failure rather than retrying is an operator-experience decision rather than a purely mechanical bug fix.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P1: A terminal Gmail OAuth failure can repeatedly disrupt active watch delivery and create an unbounded Pub/Sub retry/logging loop.
  • merge-risk: 🚨 compatibility: The PR changes persisted watch-state behavior and preserves prior cursor state during start or renew after a terminal OAuth failure.
  • merge-risk: 🚨 message-delivery: The patch deliberately acknowledges terminal OAuth delivery failures instead of requesting Pub/Sub redelivery.
  • 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 body contains redacted after-fix live output from a real Gmail account and temporary Pub/Sub pull subscription, including invalid-token acknowledgement, recovery-state persistence, reauthentication renewal, immediate notification catch-up, hook delivery, and marker clearance.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body contains redacted after-fix live output from a real Gmail account and temporary Pub/Sub pull subscription, including invalid-token acknowledgement, recovery-state persistence, reauthentication renewal, immediate notification catch-up, hook delivery, and marker clearance.
Evidence reviewed

What I checked:

  • Durable recovery before acknowledgement: The branch classifies terminal Gmail authorization errors, persists an auth-recovery marker without advancing the history cursor, and maps that classified error to an accepted push or acknowledged pull delivery. (internal/gmailwatch/processor.go:176, 7b32e33be8a5)
  • Push and pull paths retain retry behavior for other failures: The proposed push and pull handlers only acknowledge TerminalAuthError; ordinary processing and rate-limit failures retain their existing retry-oriented paths. (internal/gmailwatch/http.go:148, 7b32e33be8a5)
  • Recovery cursor preservation is covered: ApplyWatchRegistration keeps the prior successful history cursor, last push message ID, and recovery marker while a recovery is pending; tests cover both pending-recovery preservation and normal registration replacement. (internal/gmailwatch/state.go:98, 7b32e33be8a5)
  • Real behavior proof supplied: The PR body includes redacted after-fix output from a real Gmail account and temporary Pub/Sub pull subscription, showing invalid_grant acknowledgement, persisted recovery state, watch renewal, immediate registration notification, retained-cursor catch-up, hook delivery, and marker clearance. (7b32e33be8a5)
  • Adjacent Gmail watch history: Release history attributes prior Gmail watch enhancements, including history-type handling and fetch-delay work, to salmonumbrella; this makes that contributor a relevant routing candidate for the recovery and delivery semantics. (CHANGELOG.md, 4747fb05a429)

Likely related people:

  • salmonumbrella: Release history credits Gmail watch behavior work including watch history-type handling and fetch-delay changes, which are adjacent to this delivery and cursor-recovery path. (role: recent Gmail watch contributor; confidence: medium; files: internal/gmailwatch/processor.go, internal/gmailwatch/http.go, docs/watch.md)
  • chrysb: Release history credits a Gmail watch push-handling fix that preserved the selected OAuth client, making this person relevant to OAuth behavior in the same watch path. (role: recent adjacent contributor; confidence: medium; files: internal/cmd/gmail_watch_processor.go, internal/cmd/gmail_watch_server.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.
Review history (4 earlier review cycles)
  • reviewed 2026-07-19T10:53:46.482Z sha 554028d :: needs real behavior proof before merge. :: [P1] Guarantee catch-up before acknowledging terminal OAuth failures
  • reviewed 2026-07-19T11:35:15.643Z sha 554028d :: needs real behavior proof before merge. :: [P1] Guarantee catch-up before acknowledging terminal OAuth failures
  • reviewed 2026-07-19T11:44:25.952Z sha 7b32e33 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-19T12:26:10.550Z sha 7b32e33 :: needs real behavior proof before merge. :: none

@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. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. labels Jul 19, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. proof: sufficient Contributor real behavior proof is sufficient. 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. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. 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.

1 participant