Skip to content

fix: count a Gmail refusal we cannot read apart from a message that is gone - #862

Merged
yadava5 merged 4 commits into
mainfrom
fix/744-unrecognised-refusal
Sep 6, 2026
Merged

fix: count a Gmail refusal we cannot read apart from a message that is gone#862
yadava5 merged 4 commits into
mainfrom
fix/744-unrecognised-refusal

Conversation

@yadava5

@yadava5 yadava5 commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Refs #744 — deliberately not Closes; see Scope at the bottom.

_batch_fetch_metadata._send drops any failed sub-request that is neither recognisably rate-limited nor retryable into unreadable, and the client then advances next_page_token past those ids permanently. Right for a message that is genuinely gone; wrong for a quota refusal wearing an error envelope we do not recognise.

The code already records that Gmail's envelope changed shape once — _RATE_LIMIT_REASONS carries RESOURCE_EXHAUSTED because the newer google.rpc shape reports error.status instead of error.errors[].reason. A third shape would reproduce the original defect (a scan that silently returns a fraction of the mailbox and reports success) through a narrower door.

Which of the two options the issue offers

Not "retryable-once". It collides with a shipped, deliberately argued invariant: an unrecognised 403 is more likely a revoked grant than a rate limit, and retrying burns the budget a real rate limit needs. test_an_unrecognised_refusal_is_still_not_retried pins that this change did not smuggle it in.

The distinct count, which is additive and overturns no existing test.

The predicate is not the obvious one, and that is the substance of this PR

The issue thread proposed "incremented when a failure matched neither reason set". Measured against the suite's own fixtures, that is true for three cases:

case naive predicate should count?
403 authError — a revoked grant ✅ true ❌ recognised, understood, permanent
404 notFound — the ordinary case ✅ true ❌ every deleted message
403 google.rpc, no readable reason ✅ true the open door

A counter that increments on every deleted message is non-zero on essentially every real page, so an envelope change would arrive as one more unit in a number already large — the opposite of loud.

The test used instead is "it named no reason we could read, on a status that does not speak for itself": _SELF_EXPLANATORY_STATUSES = {400, 404}. A 404 means the message is gone whatever its body says; a 403 is exactly where a quota refusal and a revoked grant are indistinguishable without a reason.

Scope

unrecognised is a subset of unreadable, carried on MetadataBatch, MessagePage and HistoryPage, on the four API response models, and on InboxPage web-side — plus a warning that names _RATE_LIMIT_REASONS as the thing to extend.

No UI. The issue's bar is "loud instead of quiet", which the log line and the distinct count meet. A surface for it is a separate call, and inventing one here would be scope I was not asked for.

The issue's "cheap next step" — re-run a live 200-message mine and read the per-failure log — is not done and is not doable from here (it needs the owner's mailbox). It is also largely superseded: that step existed to attribute the observed unreadable: 49, and the count now answers the same question without a manual re-run.

Verification

22 tests, up from 18. Five mutations, __pycache__ cleared between each, source restored byte-identical after every one:

mutation result
the naive "neither reason set" predicate 2 red
drop the self-explanatory-status check 1 red
unrecognised collapses into dropped 1 red
unrecognised refusals get retried 1 red
never record — the fix becomes a no-op 2 red

The second one initially survived. _SELF_EXPLANATORY_STATUSES was a constant nothing graded: every fixture that reached it carried a reason and returned early. Added a 404 and a 400 with no parseable body, which is the case the constant exists for.

Zero new lint: 19 advisory ruff findings across the three touched files before and after. ruff format was run and then reverted — CI runs ruff check ., not the formatter, and formatting collapsed unrelated multi-line expressions across both modules, which is churn that mixes concerns into the diff.


Review round: three defects found, all confirmed by execution

The first commit on this branch was broken, not merely incomplete. Recording it here because two of the three share one cause.

1. Every server-side POST /gmail/sync returned 500

gmail_sync bound unrecognised only on the relay branch; the server-side branch copied unreadable off the outcome and never bound it. UnboundLocalError at the SyncResponse build, swallowed by the broad except and re-raised as a 500 with a sync failure recorded against the mailbox.

The existing suite always could see this: reverting just that one line reds 30 of the 109 test_gmail_oauth_cloud.py tests. A full-suite run on the broken commit found 31 failures from this single cause. The commit simply was never run against them before being pushed.

2. _ScanOutcome.unrecognised had no writer

Both constructions pass unreadable=incremental.unreadable / unreadable=read.unreadable. So even once bound, the sync surface would have published a permanent 0 — a field on a model that nothing populates.

Both misses share one cause worth naming. The edit was applied by exact whole-line matching against the four spellings I had already seen, and every missed site is one whose right-hand side differs. A mechanical pass that looks exhaustive because it is literal is only as wide as the examples that built it.

3. The counter missed its own target scenario

_record_unrecognised read failures, which is .clear()ed at the top of every retry round, and ran only at _send's two exits. An unrecognised refusal sharing a round with a retryable flake was wiped before anything looked at it — never deferred, never re-sent, never recorded.

That is the common presentation: an envelope change fails many sub-requests at once, and one 5xx among them is unremarkable. Every original test used a single failing id and none could see it. Now recorded once per round, which also makes the - set(results) subtraction live rather than the dead code it had been.

And the predicate was too narrow in the other direction

It treated "it named a reason" as "we recognise it" — which waves through exactly the change it watches for. The envelope already moved the quota signal once (that is why RESOURCE_EXHAUSTED is in _RATE_LIMIT_REASONS), and a renamed quota reason arrives unfamiliar rather than absent. Now three-sided: a status that speaks for itself, then a reason we have classified as permanent, then everything else. _PERMANENT_REASONS is deliberately small — erring small counts an unknown as unrecognised, which is loud, and loud is what this prefers.

New coverage, each proven able to fail

control proof
the co-occurrence round red before the fix, green after
five endpoint-level assertions, one per threading hop each red when its own kwarg is deleted
the three reason sets are disjoint red on an overlap
unrecognised <= unreadable across four page shapes structural invariant

136 passed across the two modules. readme_facts --check green. Zero new lint.

A separate defect, filed not fixed

#863_error_reasons returns before it reads error.status, so a google.rpc quota envelope carrying an ErrorInfo reason is not read as a rate limit at all and never aborts the page. Measured: rate_limited=False on that shape against True on the shape the existing test uses. This counter makes that loss loud; it does not prevent it, and the prose no longer claims otherwise.

Scope — why this says Refs and not Closes

The issue's durable fix is done. Its cheap next step — re-run a live 200-message mine and read the per-failure log to identify what the original unreadable: 49 actually were — is not, and cannot be from here: it needs the owner's mailbox and the deployed app.

That step is now cheaper than it was (the count narrows the question, and the logging it depends on already shipped), but it is a real deliverable and I am not closing the issue over it. Close it after that run, or split the run into its own issue.

Also not done, stated plainly: nothing renders unrecognised. InboxWorkbench shows unreadable ("N could not be read") and reads the new field nowhere, and no sync consumer reads it either. It is log-and-wire only. That meets the issue's "loud instead of quiet" bar for an operator; it would not be honest to describe it as a user-visible surface.

Ayush Yadav added 2 commits September 6, 2026 04:04
…s gone

`_batch_fetch_metadata._send` drops any failed sub-request that is neither
recognisably rate-limited nor retryable into `unreadable`, and the client
then advances `next_page_token` past those ids permanently. That is right
for a message that is genuinely gone and wrong for a quota refusal wearing
an error envelope we do not recognise.

The code already records that Gmail's envelope changed shape once:
`_RATE_LIMIT_REASONS` carries `RESOURCE_EXHAUSTED` because the newer
google.rpc shape reports `error.status` instead of `error.errors[].reason`.
A third shape would reproduce the original defect -- a scan that silently
returns a fraction of the mailbox and reports success -- through a
narrower door.

NOT "RETRYABLE-ONCE", which the issue floats first. It collides with a
shipped, deliberately argued invariant: an unrecognised 403 is more likely
a revoked grant than a rate limit, and retrying burns the budget a real
rate limit needs. A test pins that the new count did not smuggle it in.

THE PREDICATE IS NOT "MATCHED NEITHER REASON SET", which is the obvious
spelling and is the one the issue thread proposed. Measured against the
suite's own fixtures, that is true for THREE cases:

    403 authError    a revoked grant. Recognised, understood, permanent.
    404 notFound     the ordinary case. Every deleted message.
    403 google.rpc   no readable reason at all. The open door.

A counter that increments on every deleted message is non-zero on
essentially every real page, so an envelope change would arrive as one more
unit in a number already large -- the opposite of loud. The test is instead
"it named no reason we could read, on a status that does not speak for
itself": `_SELF_EXPLANATORY_STATUSES = {400, 404}`.

`unrecognised` is carried as a SUBSET of `unreadable` on MetadataBatch,
MessagePage and HistoryPage, on the four API response models, and on
`InboxPage` web-side, plus a warning naming `_RATE_LIMIT_REASONS` as the
thing to extend. No UI: the issue's bar is "loud rather than quiet", which
the log and the distinct count meet; a surface for it is a separate call.

Verified: 22 passed (18 before). Five mutations, __pycache__ cleared
between each, source restored byte-identical after every one:

  the naive "neither reason set" predicate        -> 2 red
  drop the self-explanatory-status check          -> 1 red
  unrecognised collapses into dropped             -> 1 red
  unrecognised refusals get retried               -> 1 red
  never record (the fix becomes a no-op)          -> 2 red

The second of those initially SURVIVED: `_SELF_EXPLANATORY_STATUSES` was a
constant nothing graded, because every fixture reaching it carried a reason
and returned early. Added a 404 and a 400 with no parseable body, which is
the case the constant exists for.

Zero new lint: 19 advisory ruff findings across the three touched files
before and after. `ruff format` was run and then REVERTED -- CI runs
`ruff check .` and not the formatter, and formatting collapsed unrelated
multi-line expressions across both modules, which is churn that mixes
concerns into the diff.

Refs #744.
@vercel

vercel Bot commented Sep 6, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
jobtracker-web Ready Ready Preview Sep 6, 2026 8:27am UTC

Request Review

…it per round

Review found three defects in the previous commit, all confirmed by
execution before being fixed. The first two mean the previous commit was
broken, not merely incomplete.

1. EVERY SERVER-SIDE /gmail/sync RETURNED 500. `gmail_sync` bound
   `unrecognised` only on the relay branch, while the server-side branch
   copied `unreadable` off the outcome and never bound it -- an
   UnboundLocalError at the `SyncResponse` build, swallowed by the broad
   except and re-raised as a 500 with a sync failure recorded against the
   mailbox. Reverting just that line reds 30 of the 109 existing
   `test_gmail_oauth_cloud.py` tests, so the suite always could see it;
   the previous commit simply was not run against them.

2. `_ScanOutcome.unrecognised` HAD NO WRITER, so the sync surface would
   have published a permanent 0 even once bound. Both constructions pass
   `unreadable=incremental.unreadable` / `unreadable=read.unreadable`.

   Both misses share one cause worth recording: the edit was applied by
   exact whole-line matching against the four spellings I had already
   seen, and every missed site is one whose right-hand side differs. A
   mechanical pass that looks exhaustive because it is literal is not
   exhaustive; it is only as wide as the examples that built it.

3. THE COUNTER MISSED ITS OWN TARGET SCENARIO. `_record_unrecognised`
   read `failures`, which is cleared at the top of every retry round, and
   was called only at `_send`'s two exits. So an unrecognised refusal
   sharing a round with a retryable flake was wiped before anything looked
   at it -- never deferred, never re-sent, never recorded. That is the
   COMMON presentation: an envelope change fails many sub-requests at once
   and one 5xx among them is unremarkable. Now recorded once per round,
   which also makes the `- set(results)` subtraction live rather than dead.

Also, from the same review:

THE PREDICATE WAS TOO NARROW IN THE OTHER DIRECTION. It treated "it named
a reason" as "we recognise it", which waves through exactly the change it
watches for: the envelope already moved the quota signal once (that is why
RESOURCE_EXHAUSTED is in _RATE_LIMIT_REASONS), and a RENAMED quota reason
arrives unfamiliar rather than absent. Now three-sided -- a status that
speaks for itself, then a reason we have CLASSIFIED as permanent, then
everything else. `_PERMANENT_REASONS` is deliberately small: erring small
counts an unknown as unrecognised, which is loud, and loud is the failure
mode this prefers.

New coverage, each proven able to fail:
  the co-occurrence round            red before the fix, green after
  five endpoint-level assertions     one per threading hop, each red when
                                     its own kwarg is deleted
  the three reason sets are disjoint red on an overlap
  unrecognised <= unreadable         across four page shapes

136 passed across the two modules (109 + 25 + 2 new endpoint tests).
readme_facts --check green. Zero new lint.

A separate defect found while checking the predicate is filed as #863 and
deliberately NOT fixed here: `_error_reasons` returns before it reads
`error.status`, so a google.rpc quota envelope carrying an ErrorInfo
reason is not read as a rate limit at all and never aborts the page.
Measured: `rate_limited=False` on that shape. This counter makes that loss
loud; it does not prevent it, and the prose no longer claims otherwise.

Refs #744, #863.
`apps/web/lib/api/schema.d.ts` is generated from the cloud OpenAPI document
by scripts/generate_api_schema.sh, and E2E CI's "Fail if the committed API
schema drifted" step compares the committed copy against a fresh build.
Adding `unrecognised` to the response models moved the document and not the
file, so the gate redded.

Purely additive: ten lines, two `unrecognised: number` entries, matching
`unreadable`'s two. tsc clean.

Worth recording that this was called before CI found it -- the review named
the ungenerated schema as a warning and I pushed without acting on it. The
gate did its job; I should not have needed it to.
@yadava5
yadava5 merged commit 7413af6 into main Sep 6, 2026
16 checks passed
@yadava5
yadava5 deleted the fix/744-unrecognised-refusal branch September 6, 2026 09:01
yadava5 added a commit that referenced this pull request Sep 7, 2026
…885)

The backend has counted `unrecognised` since #862 -- of the messages Gmail
listed and would not hand over, the share it refused with a reason this client
does not recognise. It reaches `lib/gmail/types.ts` and the generated schema
and stops there. Nothing on any screen has ever read it, and `types.ts` says so
in its own docstring.

That is the number that matters most of the three. An ordinary unreadable
message is a deletion or a revoked grant and there is nothing to be done about
it. An UNRECOGNISED refusal means Gmail answered in a shape this client cannot
parse, which is exactly how a scan silently shrinks when Google changes an
error envelope. The backend counts it so that change is loud; it was loud only
as far as the wire.

## A SUBSET, so it renders in a parenthetical

`MessagePage.unrecognised` is documented as "Of `unreadable`, the share Gmail
refused without a reason we recognise", and the batch measurements agree --
an unrecognised refusal is also a drop. So it is never added to anything.
Rendering it as a second count would double-report every message it names.

    9 could not be read (4 refused in a way this app doesn't recognise)

## Verified in a browser, both arms

`/demo/scan` mounts the REAL `InboxWorkbench`, so the render path is reachable
without a session. Driving it through the snapshot the component already
restores from:

    unreadable 9, unrecognised 4  ->  "... 9 could not be read (4 refused in a
                                       way this app doesn't recognise)"
    unreadable 9, unrecognised 0  ->  "... 9 could not be read"

The second is the control: the parenthetical is conditional, and its absence at
zero is observed rather than assumed.

## No snapshot version bump

`unrecognised` is added to the stored shape as OPTIONAL and read with `?? 0`,
which is precisely how `unreadable` was added on 2026-09-04 -- the field's own
comment records that v2 snapshots written before that date do not carry it. The
"bump the version" rule in `SNAPSHOT_KEY`'s note is about the VERDICT shape; an
older snapshot restores here with a count of zero and renders exactly as it
does today.

Gates: `tsc --noEmit` clean, `eslint --max-warnings 0` clean, unit suite
922/922.

## What this does not close

#744's remaining item is the live 200-message mine against the owner's real
mailbox, which needs production Gmail and is the owner's to run. The backend
half was already fixed and is already tested.

Refs #744.

Co-authored-by: Ayush Yadav <aesh_1055@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant