fix(notebooks): detect quota exhaustion on Discovery Engine code 7/8, not just code 3 (#1546)#1574
fix(notebooks): detect quota exhaustion on Discovery Engine code 7/8, not just code 3 (#1546)#1574teng-lin wants to merge 2 commits into
Conversation
|
@claude review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe single ChangesQuota RPC Code Expansion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @teng-lin's task in 1m 25s —— View job Code Review
This is a well-scoped, well-tested fix. The two-stage gate design (code set membership → count-vs-limit verification) is the right architecture — the broad code matching is safe precisely because the count check does the real disambiguating. A few observations below, ordered by impact. Logic (
|
| Test | Code | Count | Expected |
|---|---|---|---|
test_create_permission_denied_at_limit_raises_limit_error |
7 | 500/500 | NotebookLimitError ✓ |
test_create_resource_exhausted_at_limit_raises_limit_error |
8 | 500/500 | NotebookLimitError ✓ |
test_create_permission_denied_under_limit_preserves_rpc_error |
7 | 20/500 | raw RPCError ✓ |
Minor gap: There is a false-positive guard test for code 7 under limit but not for code 8. Since code 8 is explicitly described as "insurance" (not yet observed in the wild), this is understandable — but a symmetrical test would complete the picture:
@pytest.mark.asyncio
async def test_create_resource_exhausted_under_limit_preserves_rpc_error(self):
original = _create_invalid_argument_error(rpc_code=8)
api = _make_api(rpc_call=AsyncMock(side_effect=original))
_set_account_limit(api, 500)
api.list = AsyncMock(return_value=_owned_notebooks(20))
with pytest.raises(RPCError) as exc_info:
await api.create("Genuine Exhausted Error")
assert exc_info.value is original
assert not isinstance(exc_info.value, NotebookLimitError)Pre-existing nit (not introduced by this PR): _create_invalid_argument_error(rpc_code=7) still emits the hardcoded message "status code 3 (Invalid argument)" even when called with rpc_code=7 or rpc_code=8. The tests only assert on the rpc_code field and exception identity, so this doesn't affect correctness, but it does make the error messages confusing when a test fails.
CHANGELOG
Entry is accurate, well-structured, and appropriately explains both what changed and why broadening the code set is safe. The false-positive explanation is directly useful to future readers diagnosing a bug.
Hold note
The PR description flags code 7 as pending verification on #1546 — confirmed as the right call to mark as "Related to" rather than "Closes." If it turns out code 7 is not quota-specific, dropping it from the frozenset is a one-line edit and the count-vs-limit gate guarantees no false NotebookLimitError was ever raised in the interim.
Summary: The core change is correct and the coverage is strong. The one meaningful gap is the missing code-8 false-positive guard test (suggested above). Everything else is either clean or pre-existing. Ready to merge once the hold-note verification on #1546 is complete.
There was a problem hiding this comment.
Code Review
This pull request expands notebook-quota exhaustion detection to support the "Discovery Engine" backend by matching a set of gRPC status codes {3, 7, 8} instead of just the legacy code 3. The reviewer suggests including string representations of these status codes in the CREATE_NOTEBOOK_QUOTA_RPC_CODES set to ensure robust matching, as RPCError.rpc_code can sometimes be populated as a string.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
358ce64 to
9f2dadd
Compare
… not just code 3 (#1546) `notebooks.create()` converts a quota-shaped CREATE_NOTEBOOK failure into the actionable `NotebookLimitError`, but the trigger was gated on a single gRPC status code (3 / INVALID_ARGUMENT — the legacy backend's quota code from #328). Google's newer "Discovery Engine" backend reports the same notebook-limit exhaustion as code 7 (PERMISSION_DENIED), so affected accounts got a raw, confusing RPC error instead of the "delete old notebooks" guidance (#1546). Replace the scalar `CREATE_NOTEBOOK_QUOTA_RPC_CODE = 3` with `CREATE_NOTEBOOK_QUOTA_RPC_CODES = frozenset({3, 7, 8})` (3 = legacy INVALID_ARGUMENT, 7 = Discovery Engine PERMISSION_DENIED, 8 = canonical gRPC RESOURCE_EXHAUSTED as insurance) and switch the membership check to `not in`. This cannot cause false positives: a matched code only *gates* the quota path; the existing count-vs-limit verification (fetch the account's advertised notebook_limit, raise only when owned_count is at/near it) is the real disambiguator. A code-7 failure on an account well under its limit (a genuine permission error) still propagates unchanged — covered by a new regression test mirroring the existing code-3 under-limit case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9f2dadd to
c8d42d9
Compare
|
Thanks @claude — addressed both suggestions in c8d42d9:
Also addressed gemini-code-assist's Noted on the hold: keeping code 7 in the set and marked "Related to" #1546 pending the verification there. |
|
Claude finished @teng-lin's task in 2m 6s —— View job Code Review (follow-up on c8d42d9)
All three items from the previous review have been addressed correctly. Here's a brief verification of each.
The implementation is clean and covers every meaningful case:
Catching both Missing code-8 false-positive guard (
String-form test (
Helper message nit (
Summary: All feedback addressed correctly. The normalization approach is better than embedding string forms in the frozenset (fewer representations to keep in sync, handles any future coercion edge cases). The test coverage is now symmetric across all three quota codes in both the at-limit and under-limit directions. Ready to merge pending the #1546 verification on code 7. |
#1546) Add a notebook/source-limit reference to the "RPCError: [3]" / quota section: distinguishes the notebooks-per-account cap from the sources-per-notebook cap (a common confusion), notes the library auto-raises NotebookLimitError for the notebook case, and lists the documented per-tier limits with a link to Google's official limits page. Companion to the quota-detection change in this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
notebooks.create()converts a quota-shapedCREATE_NOTEBOOKRPC failure into the actionableNotebookLimitError("delete old notebooks and try again"). The trigger was gated on a single gRPC status code —CREATE_NOTEBOOK_QUOTA_RPC_CODE = 3(INVALID_ARGUMENT), established as the legacy backend's quota code in #328.#1546 surfaced that Google's newer "Discovery Engine" backend reports notebook-quota exhaustion as gRPC code 7 (PERMISSION_DENIED), not 3. The reporter (
caichengle666) confirmed: hit a notebook limit, theAzXHBdcreate returnedstatus=7, and freeing space fixed it. The code-3-only filter misses that, so affected accounts get a raw, confusing RPC error instead of the clear limit guidance.This widens the gate to a set of quota-suspect codes:
CREATE_NOTEBOOK_QUOTA_RPC_CODES = frozenset({3, 7, 8}).Related Issue
Related to #1546.
Changes
src/notebooklm/_notebooks.py: replace the scalarCREATE_NOTEBOOK_QUOTA_RPC_CODE = 3withCREATE_NOTEBOOK_QUOTA_RPC_CODES = frozenset({3, 7, 8})and switch the membership check in_raise_quota_error_if_detectedfrom!=tonot in. Codes: 3 = legacy INVALID_ARGUMENT (fix(settings): use account limits for create quota errors #328), 7 = Discovery Engine PERMISSION_DENIED (CREATE_NOTEBOOK RPC CCqFvf returns status=3 (Invalid argument) since Jun 10 #1546), 8 = canonical gRPC RESOURCE_EXHAUSTED (insurance for if Google ever adopts the "correct" code). Comments updated to explain the multi-cohort code situation and why the gate is safe.tests/unit/test_notebook_api.py: extend theTestCreateNotebookQuotaDetectioncluster — code 7 at/over limit raisesNotebookLimitError; code 8 at/over limit raisesNotebookLimitError; code 7 well under limit does NOT convert and the rawRPCErrorpropagates (the critical false-positive guard, mirroring the existing code-3 under-limit test). Existing code-3 behavior unchanged.CHANGELOG.md: entry under### Fixed(Unreleased).Why broadening the code set is safe (no false positives)
The code check only gates the quota path — it never decides it. After the code matches,
_raise_quota_error_if_detectedverifies against the account's real limit: it fetchesaccount_limits.notebook_limitviaGET_USER_SETTINGSand only raisesNotebookLimitErrorwhenowned_count >= max(notebook_limit - 1, 0). That count-vs-limit gate is the true disambiguator, so widening the code set cannot cause a falseNotebookLimitErrorunless the account is genuinely at/near its limit.Concretely, a code-7 failure on an account well under its limit (i.e. a genuine permission error, not quota) still propagates the raw
RPCErrorunchanged — proven by the newtest_create_permission_denied_under_limit_preserves_rpc_errorregression test. The same protection covers code 3 (which also fires on wire-format drift) and code 8.The renamed constant is private to the
_notebooksmodule (not in__all__, not re-exported, not imported by tests), so the scalar→frozenset rename is not a public-API or test-seam break — confirmed byscripts/audit_public_api_compat.py(no new allowlist entries).Test Plan
uv run pytest: 10206 passed, 69 skipped, 1 xfailed)uv run ruff check .+uv run ruff format --check ., whole tree)uv run mypy src/notebooklm --ignore-missing-imports)uv run python scripts/audit_public_api_compat.py)Notes
AzXHBdcreate still return 7 after quota is freed?). Codes 3 (fix(settings): use account limits for create quota errors #328-confirmed) and 8 (canonicalRESOURCE_EXHAUSTED) are not at risk. If 7 turns out not to be quota, it can be dropped from the set before merge; the change is a one-line edit to thefrozenset. Marked "Related to" rather than "Closes" CREATE_NOTEBOOK RPC CCqFvf returns status=3 (Invalid argument) since Jun 10 #1546 for that reason.🤖 Generated with Claude Code
Summary by CodeRabbit