feat(source-gmail): add messages_details incremental sync, optional start_date filtering, configurable concurrency, and Gmail rate-limit handling#76431
Conversation
…d Gmail rate-limit handling Co-Authored-By: bot_apk <apk@cognition.ai>
This comment was marked as outdated.
This comment was marked as outdated.
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksPR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful Resources
|
Co-Authored-By: bot_apk <apk@cognition.ai>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Co-Authored-By: bot_apk <apk@cognition.ai>
Per PR review: CompositeErrorHandler is unnecessary when the wrapped error handlers are a specialized DefaultErrorHandler + a catch-all DefaultErrorHandler. A single DefaultErrorHandler with the Retry-After backoff and 429 response filter is equivalent and cleaner. Co-Authored-By: bot_apk <apk@cognition.ai>
…manifest.py Per PR review: - The api_budget MovingWindowCallRatePolicy used a flat req/sec limit, but Gmail's quota is enforced in quota-units per method (5-100 units). The reactive Retry-After error handler already correctly backs off on 429s, so the proactive budget was redundant and unnecessarily coarse. - test_manifest.py structural assertions duplicated what test_streams.py already verifies end-to-end through the CDK. Co-Authored-By: bot_apk <apk@cognition.ai>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ds for q=after:
Gmail's users.messages.list returns only {id, threadId} stubs; internalDate is
only in users.messages.get. The cursor therefore has to live on
messages_details, not messages. Keep incremental_dependency: true on the parent
config so the parent's q=after: can be driven by the child cursor on repeat
syncs.
Also fixes the drafts/threads q=after: filters to use unix seconds via
format_datetime — Gmail interprets after:YYYY/MM/DD as midnight PST, which
drifts ~8h for UTC users.
Changelog: link to the correct PR (#76431).
Co-Authored-By: bot_apk <apk@cognition.ai>
This comment was marked as outdated.
This comment was marked as outdated.
…te-limit handling - Give messages stream its own DatetimeBasedCursor on internalDate so incremental_dependency from messages_details is meaningful. Parent's cursor_datetime_formats is %s (unix seconds) to match what the child emits, since list responses carry no internalDate and the parent only reads state via the child's migrated value. - Reclassify HTTP 403 with rateLimitExceeded in the body as RATE_LIMITED so Gmail quota-saturation errors retry with Retry-After instead of terminating as auth failures. - Expose num_workers in the user spec (2..10, default 5) so users can throttle concurrency on lower-tier Gmail API quotas. - Add unit tests covering parent q=after advancement on resume and 403+rateLimitExceeded retry behavior. Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…dd ExponentialBackoff fallback Co-Authored-By: bot_apk <apk@cognition.ai>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
… parent for messages_details
Addresses pnilan's follow-up review. Exposing 'messages' as incremental on
internalDate is unsafe because users.messages.list returns {id, threadId}
stubs with no cursor field; DatetimeBasedCursor silently drops those records
when the stream is selected standalone (no child feeds state back).
Solution: keep public 'messages' as full-refresh (restores pre-0.1.0
semantics, adds optional start_date gate), and introduce an internal
'_messages_for_details' stream whose sole role is to serve as the parent of
'messages_details' so incremental_dependency can migrate child state back
and bound repeat-sync list calls. The internal stream is not added to the
top-level streams list, so it cannot be selected directly by users.
Tests: 2 new regression guards — one asserts public 'messages' emits
list-endpoint stubs standalone with no default q, one asserts it still
injects q=after:<unix> when start_date is configured. 11/11 pass.
Co-Authored-By: bot_apk <apk@cognition.ai>
devin-ai-integration
Bot
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
|
/ai-prove-fix
|
|
|
|
↪️ Triggering Reason: Ready-for-review PR with CI green and no prior |
Reviewing PR for connector safety and quality.
|
|
/ai-prove-fix
|
Fix Validation Evidence — Proven ✅Outcome: No regression detected between PR Evidence Summary
Per-command results: The DISCOVER/READ "both failed" outcome is rooted in a pre-existing connection-config state on the test actor (not in the code changed by this PR). Since target and control fail identically, there is no regression introduced by this PR. Rate-limit-handling paths are additionally exercised by new unit tests in
Next Steps
Connector & PR Details
Evidence PlanProving criteria
Disproving criteria
Testing strategy Regression tests only (comparison mode). Pre-flight Checks
Detailed Evidence LogRun 1 (initial attempt): https://github.com/airbytehq/airbyte-ops-mcp/actions/runs/24685261758 — failed in SPEC step with Run 2 (authoritative, with Artifacts (SPEC / CHECK / DISCOVER / READ) uploaded to the run above (retention 7 days). Customer-scoped connection / workspace identifiers are logged in the private oncall issue (not exposed here). |
…og conflicts Co-Authored-By: bot_apk <apk@cognition.ai>
|
…og conflicts Co-Authored-By: bot_apk <apk@cognition.ai>
…concurrency-ratelimit
I see three must-fix items before merging:
The internal Why this matters: the main Suggested remediation:
The manifest comment says both Suggested remediation: match the documented reasons explicitly and add coverage for both: predicate: >-
{{ ((response.get('error', {}).get('errors', [{}]) | first).get('reason', '')) in ['rateLimitExceeded', 'userRateLimitExceeded'] }}Then parameterize the 403 retry test over both reasons: @pytest.mark.parametrize("reason", ["rateLimitExceeded", "userRateLimitExceeded"])Keep the existing negative 403 test so auth/scope errors still fail fast.
The linked issue asks for incremental handling across messages, threads, and drafts. This PR documents deferring Suggested remediation: either:
|
…arametrize 403 retry for both reasons
- Remove _messages_for_details internal parent — it could not advance its
own cursor (parent records lack internalDate), so incremental_dependency
was a no-op. Re-parent messages_details to public messages, no
incremental_dependency. Cursor on messages_details still advances from
child observations and provides state continuity.
- Honestly document in manifest that list-call bounding by last-seen
cursor is NOT achieved declaratively (would require custom Python
state migration). The gain is state correctness, not API quota use.
- Fix 403 predicate: switch from substring containment ('rateLimitExceeded'
in reason) to exact membership against ['rateLimitExceeded',
'userRateLimitExceeded']. Substring would silently miss
userRateLimitExceeded because Python is case-sensitive.
- Add user-facing error_message on the 403 RATE_LIMITED filter.
- Parametrize 403 retry test for both documented reason strings.
- Replace list-call-bound test with state-continuity regression test
matching what we actually claim.
Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
|
Patrick Nilan (@pnilan) — addressing all three must-fix items from your latest review (#76431 (comment)) in 9e6b955 (+ formatter follow-up 2dd54be): 1. Parent lower bound never advanced — confirmed and fixed by removing the broken design. You were right. I reproduced your two-sync probe locally: emitted state had The internal
The fictional 2. 403 predicate now matches both documented reasons. Confirmed your case-sensitivity analysis: predicate: >-
{{ ((response.get('error', {}).get('errors', [{}]) | first).get('reason', '')) in ['rateLimitExceeded', 'userRateLimitExceeded'] }}Added a user-facing 3. PR scope — PR title and body updated.
CI is rerunning after the formatter follow-up; twelve manifest-driven tests pass locally. |
…/num_workers to config table Co-Authored-By: bot_apk <apk@cognition.ai>
devin-ai-integration
Bot
changed the title
messages_details incremental sync, optional start_date filtering, configurable concurrency, and Gmail rate-limit handling
|
/ai-prove-fix
|
|
| Command | Target | Control | Regression? |
|---|---|---|---|
| SPEC | success | success | No |
| CHECK | success (CONNECTION_STATUS:1) | success (CONNECTION_STATUS:1) | No |
| DISCOVER | success (CATALOG:1) | success (CATALOG:1) | No |
| READ | failed (TRACE:1) | failed (TRACE:1) | No (both-failed-identically) |
- Target image:
airbyte/source-gmail:dev(built from PR at255a2fe6) - Control image:
airbyte/source-gmail:0.0.52 - READ failure root cause: Expired OAuth credentials on the test connection. Both versions produce the same TRACE error — not introduced by this PR.
Pre-flight Checks
- Viability: Changes implement audit recommendations (incremental cursor on
messages_details, Gmail rate-limit handling, concurrency, optionalstart_date) - Safety: Manifest-only YAML, no suspicious patterns
- Breaking Change: NOT breaking — additive spec fields only, no schema/PK/cursor changes on existing streams, no stream removals
- Reversibility: Safe to revert — additive spec, state from
messages_detailswon't deserialize on 0.0.52 but that was full-refresh before
Next Steps
Recommendations
- Safe to merge and publish
0.1.0to registry. - Optional: run
/ai-canary-prereleaseto roll0.1.0-preview.255a2feto a small canary of connections for real-world sync validation before full publish. - Rate-limit retry logic is verified by unit tests (
test_retry_after_on_429_is_honoured,test_retry_after_on_403_rate_limit_exceeded_is_honoured,test_non_rate_limit_403_is_not_retried). Empirical rate-limit exercising would require a high-volume Gmail account.
|
|
/ai-canary-prerelease
|
🐤 Canary Prerelease Testing — StartingConnector: Beginning canary prerelease evaluation for
|
Canary Prerelease — Phase 2-3 UpdateBreaking Change Gate: PASSED — non-breaking, reversible change
Canary Connection Selection: 7 connections across 7 workspaces selected
Requesting approval to pin these 7 connections to |
Patrick Nilan (pnilan)
deleted the
devin/1776396681-source-gmail-incremental-concurrency-ratelimit
branch
Canary Prerelease — Phase 4 Deployment CompleteApproval: Received from Patrick Nilan (@pnilan) via Slack HITL All 7 connections successfully pinned:
Monitoring begins now. Will check back in approximately 1-2 hours for initial results. For full details, see the linked private issue. |
Canary Prerelease: Final ReportConnector: SummaryAll 7 canary connections are either Detailed Results
Canary VerdictOverall Status: BLOCKED Canary testing could not collect any data because there are no active Cloud connections using Recommendations:
All 7 canary pins will be removed (cleanup in progress). For full customer details, see the linked private issue. |
3 participants
What
Related to https://github.com/airbytehq/airbyte-internal-issues/issues/16209:
Partially addresses the audit recommendations from airbytehq/airbyte-internal-issues#16209 for
source-gmail(v0.0.52 → v0.1.0, manifest-only, community/alpha). Several recommendations are intentionally deferred to follow-ups — see Out of scope below.What ships in this PR:
Incremental sync on
messages_details—DatetimeBasedCursoroninternalDate(populated only byusers.messages.get). The cursor advances from this stream's own observed records, so repeat syncs preserve the prior cursor as a floor and advance tomax(observed internalDate)(state continuity).Public
messagesstream stays full-refresh, with an optionalstart_dategate on theq=parameter (only injected whenstart_dateis configured). Exposingmessagesas incremental oninternalDatewould silently drop every record:users.messages.listreturns{id, threadId}stubs withoutinternalDate, soDatetimeBasedCursor._is_within_daterange_boundarieswould returnFalse+WARNfor every list-stub record on standalone selection.messages_detailsparent is the publicmessagesstream, noincremental_dependency. Bounding the parent list call by the child's last-seen cursor is not achievable in pure declarative — see Out of scope below.Server-side
q=after:filtering onmessages,drafts, andthreads— only injected whenstart_dateis configured, preserving pre-0.1.0 request shape on upgrade. Uses unix seconds viaformat_datetime(..., '%s')rather thanYYYY/MM/DD, which Gmail interprets as midnight PST and drifts by approximately 8h for non-PST users.Top-level
concurrency_level(max_concurrency: 10,default_concurrency: {{ config.get('num_workers', 5) }}) with matching user-facingnum_workersspec field (default 5, min 2, max 10) — parallelizes detail-substream fan-out and lets users tune it down for low-quota Gmail tiers.Reactive rate-limit handling on
base_requester'sDefaultErrorHandler:backoff_strategies:WaitTimeFromHeader(Retry-After)withExponentialBackoffStrategy(factor=5)as fallback when the header is absent.response_filters: 429 →RATE_LIMITEDviahttp_codes, and 403 →RATE_LIMITEDonly whenerror.errors[0].reasonis exactlyrateLimitExceededoruserRateLimitExceeded. The predicate uses exact membership against the documented reasons rather than substring containment because'rateLimitExceeded' in 'userRateLimitExceeded'evaluates toFalse(case-sensitiveRvsr). The predicate cannot be combined withhttp_codes: [403]—HttpResponseFilter's matchers are OR'd, so addinghttp_codes: [403]would reclassify auth/scope 403s as retryable and mask real config errors. Gmail's quota errors are documented as 403 with thisreasonfield, not 429: https://developers.google.com/gmail/api/guides/handle-errors.api_budget: Gmail charges per-method quota units (5–100), whichMovingWindowCallRatePolicycannot model.New optional
start_datespec field — non-required; omitting it preserves full-history semantics on every stream.Out of scope — deferred to follow-ups
Issue airbytehq/airbyte-internal-issues#16209's full set of recommendations is broader than what this PR delivers. The deferred items are intentionally scoped out of this change so the rate-limit and concurrency improvements can land first:
messageslist call by the last-seen cursor on repeat syncs. This requires copying the child's cursor (messages_details.internalDate) into a parent's state at sync start. Pure declarative cannot express this — built-in state migrations likeLegacyToPerPartitionStateMigrationdon't cover cross-stream state copy, and no built-in component advances a parent cursor from a child's observed records. A custom PythonCustomStateMigrationwould deliver the optimisation but takes the connector out of manifest-only territory. On this PR, the gain onmessages_detailsis state correctness and reduced destination writes, not reduced API quota use.drafts. This PR only adds aq=after:start_dategate on the list call whenstart_dateis configured;draftsis still full-refresh. A true incremental cursor on drafts would require a similar*_detailssubstream pattern asmessages_details.threads_details.users.threads.getreturnsmessages[*].internalDate, so a cursor driven offmax(messages[*].internalDate)is technically possible, but it requires either aCustomRecordExtractor(non-declarative) or a manifest transformation to hoist the nested max into a top-level field beforeDatetimeBasedCursorcan observe it.How
Declarative-First Evaluation
This PR is entirely declarative (manifest-only) — no custom Python components introduced. Each behaviour evaluates to a built-in CDK feature:
messages_details→DatetimeBasedCursor(built-in).messages_details←messages→SubstreamPartitionRouter+ParentStreamConfig(built-in).start_datefiltering → Jinja interpolation inrequest_parameters(built-in), withformat_datetime(..., '%s')for the unix-seconds conversion required by Gmail'safter:operator.concurrency_level+ spec-drivennum_workers(built-in).DefaultErrorHandlerwithWaitTimeFromHeader+ExponentialBackoffStrategyand twoHttpResponseFilterrules (built-in). The 403 predicate uses Jinja membership ({{ reason in [...] }}) rather thanhttp_codes/error_message_containsbecause Gmail's quota signal is on the nestederror.errors[0].reasonfield, not the top-levelerror.message.A custom Python
CustomStateMigrationwas evaluated to bound the parent list call by the child's cursor and explicitly deferred to keep this PR manifest-only.Test Coverage
Twelve manifest-driven tests in
unit_tests/test_streams.py, all green locally:test_public_messages_standalone_is_full_refresh_and_emits_records— regression guard against re-declaringmessagesas incremental oninternalDate(would silently drop every list-stub record).test_public_messages_injects_after_unix_seconds_when_start_date_set—q=after:1704067200(unix s, notYYYY/MM/DD).test_messages_details_checkpoints_on_internal_date—messages_detailsemits a state cursor atmax(internalDate)across detail responses, in unix seconds (per the cursor'sdatetime_format: "%s").test_messages_details_parent_omits_q_when_no_start_date— when nostart_dateis configured, the parentmessageslist call must not inject anyq=after:filter (we deliberately do not claim list-call bounding).test_messages_details_parent_injects_after_from_start_date— whenstart_dateis configured, the parent list call usesq=after:1704067200.test_messages_details_repeat_sync_state_is_continuous— second sync preserves prior cursor as a floor and advances state to the latest observedinternalDate. (This is what we claim; we deliberately do NOT assert list-call bounding here.)test_drafts_injects_after_unix_seconds_when_start_date_setandtest_threads_injects_after_unix_seconds_when_start_date_set— same unix-seconds behaviour on the other two streams that gainedq=after:.test_retry_after_on_429_is_honoured—WaitTimeFromHeaderactually sleeps forRetry-Afterseconds on a 429 then succeeds.test_retry_after_on_403_rate_limit_exceeded_is_honoured[rateLimitExceeded]and[userRateLimitExceeded]— parametrized: 403 with each documented quota reason is reclassifiedRATE_LIMITEDand retried (not surfaced asconfig_error).test_non_rate_limit_403_is_not_retried— negative guard for the predicate: a 403 whosereasonisinsufficientPermissions(auth/scope failure) must stay classified asconfig_errorand fail fast — not be reclassified asRATE_LIMITEDand retried indefinitely.Manual / Cloud validation
airbyte/source-gmail:0.1.0-preview.27d9859./ai-prove-fixregression run against a customer connection: target vs. control diff confirmedregression_detected=falseon all four operations (SPEC, CHECK, DISCOVER, READ).Review guide
airbyte-integrations/connectors/source-gmail/manifest.yaml— all functional changes (cursor, parents, error filters, spec, concurrency).airbyte-integrations/connectors/source-gmail/unit_tests/test_streams.py— twelve manifest-driven tests covering each change.airbyte-integrations/connectors/source-gmail/metadata.yamlanddocs/integrations/sources/gmail.md— version bump (0.0.52 → 0.1.0), updated config and streams tables, and the changelog entry.User Impact
Existing users on v0.0.52 keep current behaviour exactly when they upgrade and don't change config:
messages,drafts,threads,threads_details: full-refresh, same request shape (noq=injected without a configuredstart_date).messages_details: switches from full-refresh to incremental oninternalDate. First post-upgrade sync emits state for the last-seeninternalDate; subsequent syncs preserve that cursor as a floor and advance with new mail. No data is dropped on the first incremental sync.Users who set the new optional
start_dateget server-side filtering onmessages,drafts,threads, andmessages_detailsfrom that date onward.Users who set the new optional
num_workers(default 5) can throttle parallelism for low-quota Gmail tiers.Can this PR be safely reverted and rolled back?
Spec changes are additive only (
start_dateandnum_workersare both optional, with safe defaults); no fields renamed/removed. State written bymessages_detailswill not deserialise on a roll back to v0.0.52, but since v0.0.52 hasmessages_detailsas full-refresh, the only effect is one full re-sync ofmessages_detailsafter the rollback — drafts/threads/messages full-refresh behaviour is unchanged.Link to Devin session: https://app.devin.ai/sessions/c20c8aa034984b66ba7be1de599b1543