09e9f3545f
Test / Code Quality (push) Has been cancelled
Test / Test (macos-latest, Python 3.10) (push) Has been cancelled
Test / Test (macos-latest, Python 3.11) (push) Has been cancelled
Test / Test (macos-latest, Python 3.12) (push) Has been cancelled
Test / Test (macos-latest, Python 3.13) (push) Has been cancelled
Test / Test (macos-latest, Python 3.14) (push) Has been cancelled
Test / Test (ubuntu-latest, Python 3.10) (push) Has been cancelled
Test / Test (ubuntu-latest, Python 3.11) (push) Has been cancelled
Test / Test (ubuntu-latest, Python 3.12) (push) Has been cancelled
Test / Test (ubuntu-latest, Python 3.13) (push) Has been cancelled
Test / Test (ubuntu-latest, Python 3.14) (push) Has been cancelled
Test / Test (windows-latest, Python 3.10) (push) Has been cancelled
Test / Test (windows-latest, Python 3.11) (push) Has been cancelled
Test / Test (windows-latest, Python 3.12) (push) Has been cancelled
Test / Test (windows-latest, Python 3.13) (push) Has been cancelled
Test / Test (windows-latest, Python 3.14) (push) Has been cancelled
CodeQL / Analyze (push) Has been cancelled
dependency-audit / pip-audit (push) Has been cancelled
737 lines
31 KiB
Python
737 lines
31 KiB
Python
"""Variant-keyed idempotency tests for ADD_SOURCE + ADD_SOURCE_FILE.
|
||
|
||
Tier 9 Wave 2 (P0-3-sources, P1-2-sources): the previous behavior in
|
||
``_source/add.py``/``_source/upload.py`` relied on the inner transport
|
||
retry loop to handle 5xx for mutating create RPCs, which could duplicate
|
||
sources when the server already committed the write before returning the
|
||
5xx. The fix is two-fold:
|
||
|
||
1. Variant-keyed registry entries — ``(ADD_SOURCE, "url"|"drive")`` and
|
||
``(ADD_SOURCE_FILE, None)`` flip to ``PROBE_THEN_CREATE``;
|
||
``(ADD_SOURCE, "text")`` flips to ``NON_IDEMPOTENT_NO_RETRY``. The
|
||
registry forces ``disable_internal_retries=True`` at the executor.
|
||
|
||
2. Probe-then-create wrappers — for the three PROBE_THEN_CREATE variants,
|
||
``idempotent_create`` issues a single create attempt, and on a
|
||
retryable transport error (5xx / 429 / network) runs a probe before
|
||
the second attempt. The probe is variant-specific:
|
||
|
||
- ``add_url`` probes by ``source.url == url``
|
||
- ``add_drive`` probes by ``/d/<file_id>`` URL-segment marker with a
|
||
trailing boundary (avoids interior-substring + prefix-collision
|
||
false-positives)
|
||
- ``register_file_source`` probes by baseline-diff + ``source.title ==
|
||
filename`` (filenames are not identity-bearing, so the wrapper
|
||
captures source-ids before the create and filters probe matches to
|
||
sources that appeared after the create started)
|
||
|
||
The "commit-lost-response" test sequence is: first call returns 200
|
||
(server commits + returns success), second call returns 503 (lost
|
||
response from a third-party retry); the probe short-circuits and the
|
||
wrapper returns the existing source without re-issuing the create.
|
||
|
||
3. Probe-failure propagation — a probe that raises ``NetworkError``
|
||
(transport-layer) must propagate so the caller sees the original
|
||
failure mode, not a silent retry on top of a broken probe.
|
||
|
||
These tests use mock ``httpx.MockTransport`` (NOT VCR cassettes) per the
|
||
Codex iter-1 critique: cassette-based replay can't model the "first call
|
||
returns 200, second call returns 503" sequence because each route is
|
||
flatly keyed by request shape, not call order.
|
||
"""
|
||
|
||
from __future__ import annotations
|
||
|
||
import json
|
||
from pathlib import Path
|
||
from unittest.mock import AsyncMock, patch
|
||
|
||
import httpx
|
||
import pytest
|
||
|
||
import notebooklm._runtime.helpers as _runtime_helpers
|
||
from notebooklm import NotebookLMClient
|
||
from notebooklm._idempotency import IDEMPOTENCY_REGISTRY, IdempotencyPolicy
|
||
from notebooklm.exceptions import NetworkError, NotebookLMError
|
||
from notebooklm.rpc import RPCMethod
|
||
from tests._fixtures.kernel_test_helpers import install_http_client_for_test
|
||
|
||
# Mock-transport idempotency tests; no HTTP, no cassette. Opt out of the
|
||
# tier-enforcement hook in tests/integration/conftest.py.
|
||
pytestmark = pytest.mark.allow_no_vcr
|
||
|
||
|
||
# ---------------------------------------------------------------------------
|
||
# Helpers (mirror tests/integration/concurrency/test_idempotency_create.py)
|
||
# ---------------------------------------------------------------------------
|
||
|
||
|
||
def _wrb_response(rpc_id: str, payload) -> str:
|
||
"""Build a single-RPC batchexecute response body."""
|
||
inner = json.dumps(payload)
|
||
chunk = json.dumps([["wrb.fr", rpc_id, inner, None, None]])
|
||
return f")]}}'\n{len(chunk)}\n{chunk}\n"
|
||
|
||
|
||
def _get_notebook_with_sources_response(
|
||
notebook_id: str,
|
||
sources: list[tuple[str, str, str | None]],
|
||
) -> str:
|
||
"""Build a GET_NOTEBOOK response that ``SourcesAPI.list`` parses.
|
||
|
||
``sources`` is ``[(source_id, title, url_or_None), ...]``. The metadata
|
||
layout matches the parsing path in ``Source.from_api_response``: ``url``
|
||
at index ``[7]`` when present (matches ``_extract_source_url`` precedence
|
||
with ``allow_bare_http=False``).
|
||
"""
|
||
src_rows: list = []
|
||
for src_id, title, url in sources:
|
||
metadata: list = [None] * 8
|
||
if url is not None:
|
||
metadata[7] = [url]
|
||
status_block = [None, 2] # READY
|
||
src_rows.append([[src_id], title, metadata, status_block])
|
||
nb_info = ["Test Notebook", src_rows]
|
||
return _wrb_response(RPCMethod.GET_NOTEBOOK.value, [nb_info])
|
||
|
||
|
||
def _make_client_with_transport(
|
||
transport: httpx.AsyncBaseTransport,
|
||
auth_tokens,
|
||
*,
|
||
server_error_max_retries: int = 3,
|
||
) -> NotebookLMClient:
|
||
"""Construct a NotebookLMClient backed by a mock transport.
|
||
|
||
Mirrors the helper used in tests/integration/concurrency/
|
||
test_idempotency_create.py: stub in a pre-built httpx.AsyncClient
|
||
wired to the supplied mock transport, bypassing the full
|
||
``ClientLifecycle.open()`` path that would otherwise build a real connection
|
||
pool.
|
||
"""
|
||
client = NotebookLMClient(
|
||
auth_tokens,
|
||
server_error_max_retries=server_error_max_retries,
|
||
)
|
||
install_http_client_for_test(
|
||
client._collaborators.kernel,
|
||
httpx.AsyncClient(
|
||
transport=transport,
|
||
headers={
|
||
"Content-Type": "application/x-www-form-urlencoded;charset=UTF-8",
|
||
},
|
||
),
|
||
)
|
||
return client
|
||
|
||
|
||
def _rpc_id_in_request(request: httpx.Request) -> str | None:
|
||
for key, value in request.url.params.multi_items():
|
||
if key == "rpcids":
|
||
return value
|
||
return None
|
||
|
||
|
||
# ---------------------------------------------------------------------------
|
||
# add_url — commit-lost-response (PROBE_THEN_CREATE, variant="url")
|
||
# ---------------------------------------------------------------------------
|
||
|
||
|
||
async def test_add_url_probe_short_circuits_when_first_response_lost(auth_tokens) -> None:
|
||
"""First ADD_SOURCE call commits server-side but client sees 503; probe wins.
|
||
|
||
Models the commit-lost-response failure mode: the server processed the
|
||
create successfully, but the response was lost (e.g. proxy timeout
|
||
returned 503 to the caller). The probe finds the new source already
|
||
landed and returns it; only ONE ADD_SOURCE actually fires.
|
||
"""
|
||
notebook_id = "nb_test"
|
||
url = "https://example.com/article"
|
||
src_id = "src_lost_response"
|
||
|
||
add_count = 0
|
||
get_count = 0
|
||
|
||
def handler(request: httpx.Request) -> httpx.Response:
|
||
nonlocal add_count, get_count
|
||
rpc_id = _rpc_id_in_request(request)
|
||
if rpc_id == RPCMethod.ADD_SOURCE.value:
|
||
add_count += 1
|
||
# First call (the server committed but the response was lost)
|
||
# — the client sees a 503.
|
||
return httpx.Response(503, text="service unavailable")
|
||
if rpc_id == RPCMethod.GET_NOTEBOOK.value:
|
||
get_count += 1
|
||
return httpx.Response(
|
||
200,
|
||
text=_get_notebook_with_sources_response(notebook_id, [(src_id, "Article", url)]),
|
||
)
|
||
return httpx.Response(404, text=f"unexpected rpc_id={rpc_id}")
|
||
|
||
transport = httpx.MockTransport(handler)
|
||
client = _make_client_with_transport(transport, auth_tokens)
|
||
try:
|
||
source = await client.sources.add_url(notebook_id, url)
|
||
finally:
|
||
await client._collaborators.kernel.get_http_client().aclose()
|
||
|
||
assert source.id == src_id
|
||
assert source.url == url
|
||
# Exactly ONE ADD_SOURCE (no naive re-POST after the 503)
|
||
assert add_count == 1, f"expected 1 ADD_SOURCE, got {add_count}"
|
||
# Exactly ONE GET_NOTEBOOK (the probe)
|
||
assert get_count == 1, f"expected 1 GET_NOTEBOOK probe, got {get_count}"
|
||
|
||
|
||
# ---------------------------------------------------------------------------
|
||
# add_drive — commit-lost-response (PROBE_THEN_CREATE, variant="drive")
|
||
# ---------------------------------------------------------------------------
|
||
|
||
|
||
async def test_add_drive_probe_short_circuits_when_first_response_lost(auth_tokens) -> None:
|
||
"""Drive sources: 503 + ``/d/<file_id>`` segment probe returns existing source.
|
||
|
||
Drive sources canonically embed the file_id as a path segment of
|
||
``source.url`` (typical shape: ``https://docs.google.com/document/d/<file_id>/edit``).
|
||
The probe matches by ``/d/<file_id>/`` segment marker (with trailing
|
||
boundary) so neither interior-substring nor prefix collisions can
|
||
spuriously match — see the dedicated false-positive tests below.
|
||
"""
|
||
notebook_id = "nb_test"
|
||
file_id = "drive_file_abc123xyz"
|
||
title = "My Drive Doc"
|
||
src_id = "src_drive_lost"
|
||
drive_url = f"https://docs.google.com/document/d/{file_id}/edit"
|
||
|
||
add_count = 0
|
||
get_count = 0
|
||
|
||
def handler(request: httpx.Request) -> httpx.Response:
|
||
nonlocal add_count, get_count
|
||
rpc_id = _rpc_id_in_request(request)
|
||
if rpc_id == RPCMethod.ADD_SOURCE.value:
|
||
add_count += 1
|
||
return httpx.Response(502, text="bad gateway")
|
||
if rpc_id == RPCMethod.GET_NOTEBOOK.value:
|
||
get_count += 1
|
||
return httpx.Response(
|
||
200,
|
||
text=_get_notebook_with_sources_response(notebook_id, [(src_id, title, drive_url)]),
|
||
)
|
||
return httpx.Response(404, text=f"unexpected rpc_id={rpc_id}")
|
||
|
||
transport = httpx.MockTransport(handler)
|
||
client = _make_client_with_transport(transport, auth_tokens)
|
||
try:
|
||
source = await client.sources.add_drive(notebook_id, file_id, title)
|
||
finally:
|
||
await client._collaborators.kernel.get_http_client().aclose()
|
||
|
||
assert source.id == src_id
|
||
assert file_id in (source.url or "")
|
||
assert add_count == 1, f"expected 1 ADD_SOURCE, got {add_count}"
|
||
assert get_count == 1, f"expected 1 GET_NOTEBOOK probe, got {get_count}"
|
||
|
||
|
||
async def test_add_drive_probe_matches_segment_at_end_of_url(auth_tokens) -> None:
|
||
"""Drive probe correctly handles ``/d/<file_id>`` at the very end of the URL.
|
||
|
||
Some Drive URLs are stored without a trailing ``/edit`` or other path
|
||
suffix (e.g. ``https://docs.google.com/document/d/<file_id>``). The
|
||
end-of-string branch of the probe ensures these still match.
|
||
"""
|
||
notebook_id = "nb_test"
|
||
file_id = "drive_file_xyz123"
|
||
title = "Untrailing Drive Doc"
|
||
src_id = "src_drive_no_trailing"
|
||
drive_url = f"https://docs.google.com/document/d/{file_id}" # no trailing /
|
||
|
||
add_count = 0
|
||
get_count = 0
|
||
|
||
def handler(request: httpx.Request) -> httpx.Response:
|
||
nonlocal add_count, get_count
|
||
rpc_id = _rpc_id_in_request(request)
|
||
if rpc_id == RPCMethod.ADD_SOURCE.value:
|
||
add_count += 1
|
||
return httpx.Response(502, text="bad gateway")
|
||
if rpc_id == RPCMethod.GET_NOTEBOOK.value:
|
||
get_count += 1
|
||
return httpx.Response(
|
||
200,
|
||
text=_get_notebook_with_sources_response(notebook_id, [(src_id, title, drive_url)]),
|
||
)
|
||
return httpx.Response(404, text=f"unexpected rpc_id={rpc_id}")
|
||
|
||
transport = httpx.MockTransport(handler)
|
||
client = _make_client_with_transport(transport, auth_tokens)
|
||
try:
|
||
source = await client.sources.add_drive(notebook_id, file_id, title)
|
||
finally:
|
||
await client._collaborators.kernel.get_http_client().aclose()
|
||
|
||
assert source.id == src_id
|
||
assert add_count == 1, f"expected 1 ADD_SOURCE, got {add_count}"
|
||
assert get_count == 1, f"expected 1 GET_NOTEBOOK probe, got {get_count}"
|
||
|
||
|
||
@pytest.mark.parametrize(
|
||
"other_drive_url",
|
||
[
|
||
# Interior substring: contains ``abc`` but not as a ``/d/abc/`` segment.
|
||
"https://docs.google.com/document/d/xabcy/edit",
|
||
# Prefix collision: another file_id begins with the target file_id.
|
||
# ``/d/abc`` IS a substring of ``/d/abcdef/edit`` but the trailing
|
||
# boundary check rejects it.
|
||
"https://docs.google.com/document/d/abcdef/edit",
|
||
],
|
||
ids=["interior_substring", "prefix_collision"],
|
||
)
|
||
async def test_add_drive_probe_does_not_substring_match_unrelated_file_id(
|
||
auth_tokens, other_drive_url: str
|
||
) -> None:
|
||
"""Drive probe uses ``/d/<file_id>/`` segment match with trailing boundary.
|
||
|
||
Regression guard against two false-positive shapes the probe must reject:
|
||
|
||
* **Interior substring**: ``/d/xabcy/`` contains ``abc`` as a naked
|
||
substring, so the original ``file_id in source.url`` check would
|
||
spuriously match.
|
||
* **Prefix collision**: ``/d/abc`` is also a substring of ``/d/abcdef/``,
|
||
so the simpler ``/d/<file_id>`` segment marker (without a trailing
|
||
boundary) would still false-positive. Real-world Drive IDs are 33–44
|
||
character Base64URL strings making prefix collisions astronomically
|
||
unlikely, but the boundary check is essentially free.
|
||
|
||
In both cases the probe must return ``None`` so ``idempotent_create``
|
||
retries the create rather than returning the unrelated source.
|
||
"""
|
||
from notebooklm.exceptions import ServerError
|
||
|
||
notebook_id = "nb_test"
|
||
target_file_id = "abc" # short file_id chosen to maximize collision chance
|
||
title = "My Drive Doc"
|
||
|
||
add_count = 0
|
||
get_count = 0
|
||
|
||
def handler(request: httpx.Request) -> httpx.Response:
|
||
nonlocal add_count, get_count
|
||
rpc_id = _rpc_id_in_request(request)
|
||
if rpc_id == RPCMethod.ADD_SOURCE.value:
|
||
add_count += 1
|
||
return httpx.Response(502, text="bad gateway")
|
||
if rpc_id == RPCMethod.GET_NOTEBOOK.value:
|
||
get_count += 1
|
||
return httpx.Response(
|
||
200,
|
||
text=_get_notebook_with_sources_response(
|
||
notebook_id, [("src_other", title, other_drive_url)]
|
||
),
|
||
)
|
||
return httpx.Response(404, text=f"unexpected rpc_id={rpc_id}")
|
||
|
||
transport = httpx.MockTransport(handler)
|
||
client = _make_client_with_transport(transport, auth_tokens)
|
||
try:
|
||
with pytest.raises(ServerError):
|
||
await client.sources.add_drive(notebook_id, target_file_id, title)
|
||
finally:
|
||
await client._collaborators.kernel.get_http_client().aclose()
|
||
|
||
# The probe must NOT have spuriously matched the unrelated Drive
|
||
# source — instead the wrapper retried, exhausted, and re-raised.
|
||
assert add_count >= 1
|
||
|
||
|
||
# ---------------------------------------------------------------------------
|
||
# register_file_source (ADD_SOURCE_FILE) — commit-lost-response
|
||
# (PROBE_THEN_CREATE, variant=None)
|
||
# ---------------------------------------------------------------------------
|
||
|
||
|
||
async def test_register_file_source_probe_short_circuits_when_first_response_lost(
|
||
auth_tokens, tmp_path: Path
|
||
) -> None:
|
||
"""File uploads: 503 on ADD_SOURCE_FILE + baseline-diff probe returns new source.
|
||
|
||
The full add_file flow is three steps (register → start_resumable → finalize).
|
||
Here we exercise only the register step's idempotency: the test patches
|
||
the upload stages to no-op so the probe-then-retry behavior of
|
||
``register_file_source`` is observable in isolation.
|
||
|
||
Because filenames are not identity-bearing (two uploads of ``report.pdf``
|
||
are legitimately two distinct sources), the probe uses a baseline-diff
|
||
pattern: it captures source IDs BEFORE the create attempt and only
|
||
counts sources that appear AFTER the create as "the upload landed."
|
||
This test exercises the typical case: baseline returns no matching
|
||
sources, the create gets 503, the probe finds the new source, and the
|
||
wrapper short-circuits with exactly 1 ADD_SOURCE_FILE call.
|
||
"""
|
||
notebook_id = "nb_test"
|
||
filename = "my_document.pdf"
|
||
src_id = "src_file_lost"
|
||
|
||
test_file = tmp_path / filename
|
||
test_file.write_bytes(b"%PDF-1.4 minimal pdf")
|
||
|
||
register_count = 0
|
||
get_count = 0
|
||
|
||
def handler(request: httpx.Request) -> httpx.Response:
|
||
nonlocal register_count, get_count
|
||
rpc_id = _rpc_id_in_request(request)
|
||
if rpc_id == RPCMethod.ADD_SOURCE_FILE.value:
|
||
register_count += 1
|
||
return httpx.Response(503, text="service unavailable")
|
||
if rpc_id == RPCMethod.GET_NOTEBOOK.value:
|
||
get_count += 1
|
||
# Call 1: baseline — empty notebook (no pre-existing file).
|
||
# Call 2: probe after the 503 — the new source has landed
|
||
# server-side; the wrapper should return its id without retrying.
|
||
if get_count == 1:
|
||
return httpx.Response(
|
||
200,
|
||
text=_get_notebook_with_sources_response(notebook_id, []),
|
||
)
|
||
return httpx.Response(
|
||
200,
|
||
text=_get_notebook_with_sources_response(notebook_id, [(src_id, filename, None)]),
|
||
)
|
||
return httpx.Response(404, text=f"unexpected rpc_id={rpc_id}")
|
||
|
||
transport = httpx.MockTransport(handler)
|
||
client = _make_client_with_transport(transport, auth_tokens)
|
||
try:
|
||
# Stub start_resumable_upload + upload_file_streaming so this test
|
||
# exercises only the ADD_SOURCE_FILE register step's idempotency.
|
||
with (
|
||
patch.object(
|
||
client.sources._uploader,
|
||
"start_resumable_upload",
|
||
AsyncMock(return_value="https://upload.example/scotty"),
|
||
),
|
||
patch.object(
|
||
client.sources._uploader,
|
||
"upload_file_streaming",
|
||
AsyncMock(return_value=None),
|
||
),
|
||
):
|
||
source = await client.sources.add_file(notebook_id, test_file)
|
||
finally:
|
||
await client._collaborators.kernel.get_http_client().aclose()
|
||
|
||
assert source.id == src_id
|
||
# Exactly ONE ADD_SOURCE_FILE register request (no naive re-POST)
|
||
assert register_count == 1, f"expected 1 ADD_SOURCE_FILE, got {register_count}"
|
||
# TWO GET_NOTEBOOK calls: baseline + probe-after-failure
|
||
assert get_count == 2, f"expected 2 GET_NOTEBOOK calls (baseline + probe), got {get_count}"
|
||
|
||
|
||
async def test_register_file_source_does_not_match_pre_existing_filename(
|
||
auth_tokens, tmp_path: Path
|
||
) -> None:
|
||
"""File uploads: baseline-diff prevents matching a pre-existing same-named source.
|
||
|
||
Regression guard for the original Codex critical finding: filenames are
|
||
NOT identity-bearing. If the notebook already has ``report.pdf`` from a
|
||
previous upload and the user calls ``add_file(notebook_id, report.pdf)``
|
||
again, a transport failure during the second register must NOT cause
|
||
the wrapper to return the OLD source's id — that would silently
|
||
redirect the new upload onto the existing source.
|
||
|
||
Scenario:
|
||
- baseline list returns the pre-existing ``report.pdf`` source
|
||
- create gets 503 (no second source landed server-side either)
|
||
- probe list returns the SAME pre-existing source
|
||
- filtered by baseline_ids, the probe finds zero "new" matches
|
||
- the wrapper retries the create, which 503s again, and exhausts
|
||
attempts → original ServerError surfaces
|
||
|
||
The load-bearing assertion is that the wrapper does NOT return the
|
||
pre-existing source's id under any failure mode.
|
||
"""
|
||
from notebooklm.exceptions import ServerError
|
||
|
||
notebook_id = "nb_test"
|
||
filename = "report.pdf"
|
||
pre_existing_src_id = "src_OLD_report"
|
||
|
||
test_file = tmp_path / filename
|
||
test_file.write_bytes(b"%PDF-1.4 minimal pdf")
|
||
|
||
register_count = 0
|
||
get_count = 0
|
||
|
||
def handler(request: httpx.Request) -> httpx.Response:
|
||
nonlocal register_count, get_count
|
||
rpc_id = _rpc_id_in_request(request)
|
||
if rpc_id == RPCMethod.ADD_SOURCE_FILE.value:
|
||
register_count += 1
|
||
return httpx.Response(503, text="service unavailable")
|
||
if rpc_id == RPCMethod.GET_NOTEBOOK.value:
|
||
get_count += 1
|
||
# Both baseline and probe return the SAME pre-existing source.
|
||
return httpx.Response(
|
||
200,
|
||
text=_get_notebook_with_sources_response(
|
||
notebook_id, [(pre_existing_src_id, filename, None)]
|
||
),
|
||
)
|
||
return httpx.Response(404, text=f"unexpected rpc_id={rpc_id}")
|
||
|
||
transport = httpx.MockTransport(handler)
|
||
client = _make_client_with_transport(transport, auth_tokens)
|
||
try:
|
||
with (
|
||
patch.object(
|
||
client.sources._uploader,
|
||
"start_resumable_upload",
|
||
AsyncMock(return_value="https://upload.example/scotty"),
|
||
),
|
||
patch.object(
|
||
client.sources._uploader,
|
||
"upload_file_streaming",
|
||
AsyncMock(return_value=None),
|
||
),
|
||
pytest.raises(ServerError),
|
||
):
|
||
await client.sources.add_file(notebook_id, test_file)
|
||
finally:
|
||
await client._collaborators.kernel.get_http_client().aclose()
|
||
|
||
# The pre-existing source's id was never returned — instead, the
|
||
# original transport error propagated after retries were exhausted.
|
||
# The exact register_count is implementation-defined (idempotent_create
|
||
# default is 2 attempts), but it must be at least 1.
|
||
assert register_count >= 1, f"expected ≥1 ADD_SOURCE_FILE, got {register_count}"
|
||
assert get_count >= 1, f"expected ≥1 GET_NOTEBOOK, got {get_count}"
|
||
|
||
|
||
async def test_register_file_source_baseline_unavailable_raises_on_ambiguity(
|
||
auth_tokens, tmp_path: Path
|
||
) -> None:
|
||
"""Baseline fetch failure + same-name match → raise ``SourceAddError``.
|
||
|
||
When the baseline GET_NOTEBOOK fails (e.g. transient 5xx) AND the probe
|
||
later finds a same-named source, the wrapper cannot safely distinguish
|
||
"this upload landed" from "a pre-existing source has the same filename."
|
||
Surfacing this as an ambiguity is the correct behavior — silently
|
||
returning the existing source would direct the subsequent upload stream
|
||
to the wrong source (the original CodeRabbit critical concern).
|
||
|
||
Scenario:
|
||
- baseline list raises (server 503)
|
||
- create gets 503
|
||
- probe lists notebook → finds same-named source
|
||
- baseline_ids is None (sentinel) → wrapper raises SourceAddError
|
||
rather than returning the existing source's id
|
||
"""
|
||
notebook_id = "nb_test"
|
||
filename = "report.pdf"
|
||
pre_existing_src_id = "src_OLD_report"
|
||
|
||
test_file = tmp_path / filename
|
||
test_file.write_bytes(b"%PDF-1.4 minimal pdf")
|
||
|
||
register_count = 0
|
||
get_count = 0
|
||
|
||
def handler(request: httpx.Request) -> httpx.Response:
|
||
nonlocal register_count, get_count
|
||
rpc_id = _rpc_id_in_request(request)
|
||
if rpc_id == RPCMethod.ADD_SOURCE_FILE.value:
|
||
register_count += 1
|
||
return httpx.Response(503, text="service unavailable")
|
||
if rpc_id == RPCMethod.GET_NOTEBOOK.value:
|
||
get_count += 1
|
||
# First call (baseline) — 503 to simulate transport failure.
|
||
# Subsequent calls (probe) — return the pre-existing source.
|
||
if get_count == 1:
|
||
return httpx.Response(503, text="service unavailable")
|
||
return httpx.Response(
|
||
200,
|
||
text=_get_notebook_with_sources_response(
|
||
notebook_id, [(pre_existing_src_id, filename, None)]
|
||
),
|
||
)
|
||
return httpx.Response(404, text=f"unexpected rpc_id={rpc_id}")
|
||
|
||
transport = httpx.MockTransport(handler)
|
||
client = _make_client_with_transport(transport, auth_tokens, server_error_max_retries=0)
|
||
try:
|
||
with (
|
||
patch.object(
|
||
client.sources._uploader,
|
||
"start_resumable_upload",
|
||
AsyncMock(return_value="https://upload.example/scotty"),
|
||
),
|
||
patch.object(
|
||
client.sources._uploader,
|
||
"upload_file_streaming",
|
||
AsyncMock(return_value=None),
|
||
),
|
||
pytest.raises(NotebookLMError, match="baseline snapshot was unavailable"),
|
||
):
|
||
await client.sources.add_file(notebook_id, test_file)
|
||
finally:
|
||
await client._collaborators.kernel.get_http_client().aclose()
|
||
|
||
# Pre-existing source's id was NOT silently returned — instead the
|
||
# baseline-unavailable ambiguity guard fired.
|
||
assert register_count >= 1
|
||
assert get_count >= 2 # baseline + at least one probe
|
||
|
||
|
||
# ---------------------------------------------------------------------------
|
||
# add_text — NON_IDEMPOTENT_NO_RETRY enforcement
|
||
# ---------------------------------------------------------------------------
|
||
|
||
|
||
async def test_add_text_no_probe_no_retry_under_5xx(
|
||
auth_tokens, monkeypatch: pytest.MonkeyPatch
|
||
) -> None:
|
||
"""add_text under 5xx must surface the failure immediately.
|
||
|
||
No probe (no reliable dedupe key) and no retry — the registry
|
||
classifies (ADD_SOURCE, "text") as NON_IDEMPOTENT_NO_RETRY, which
|
||
force-disables the inner transport retry loop. The caller sees the
|
||
503 on the first attempt and exactly one ADD_SOURCE request fires.
|
||
|
||
asyncio.sleep is patched to a no-op so a regression that re-enables
|
||
retries doesn't pay backoff wall time before the test catches it. The
|
||
assertion on the surfaced exception type tolerates either
|
||
``ServerError`` or ``SourceAddError`` because ``add_text`` historically
|
||
wraps ``RPCError`` subclasses (including ``ServerError``) in
|
||
``SourceAddError`` — the load-bearing assertion is the request count.
|
||
"""
|
||
notebook_id = "nb_test"
|
||
title = "Some Note"
|
||
content = "some content"
|
||
add_count = 0
|
||
get_count = 0
|
||
|
||
def handler(request: httpx.Request) -> httpx.Response:
|
||
nonlocal add_count, get_count
|
||
rpc_id = _rpc_id_in_request(request)
|
||
if rpc_id == RPCMethod.ADD_SOURCE.value:
|
||
add_count += 1
|
||
return httpx.Response(503, text="service unavailable")
|
||
if rpc_id == RPCMethod.GET_NOTEBOOK.value:
|
||
# Should NEVER be called for add_text.
|
||
get_count += 1
|
||
return httpx.Response(
|
||
200,
|
||
text=_get_notebook_with_sources_response(notebook_id, []),
|
||
)
|
||
return httpx.Response(404, text=f"unexpected rpc_id={rpc_id}")
|
||
|
||
async def _no_sleep(_seconds: float) -> None:
|
||
return None
|
||
|
||
# Object-form patch against the locally-imported seam alias (ADR-0007
|
||
# Form 2): mutate the ``asyncio`` module reference that
|
||
# ``_runtime.helpers`` reads, instead of a string-target patch. This is a
|
||
# *defensive* shim — under the correct NON_IDEMPOTENT_NO_RETRY behavior
|
||
# ``add_text`` never retries, so ``asyncio.sleep`` is never reached; the
|
||
# patch only bounds wall-time if a regression re-enables retries. Because
|
||
# the green path never sleeps, the seam-binding itself is asserted
|
||
# (``resolve_sleep`` is the production read path) rather than call count.
|
||
monkeypatch.setattr(_runtime_helpers.asyncio, "sleep", _no_sleep)
|
||
assert _runtime_helpers.resolve_sleep(None) is _no_sleep, (
|
||
"object-form patch must target the seam production reads via "
|
||
"resolve_sleep(None); a wrong-namespace alias would silently no-op"
|
||
)
|
||
|
||
transport = httpx.MockTransport(handler)
|
||
client = _make_client_with_transport(transport, auth_tokens)
|
||
try:
|
||
with pytest.raises(NotebookLMError):
|
||
await client.sources.add_text(notebook_id, title, content)
|
||
finally:
|
||
await client._collaborators.kernel.get_http_client().aclose()
|
||
|
||
# Exactly ONE ADD_SOURCE attempt: no retry loop, no probe.
|
||
assert add_count == 1, (
|
||
f"add_text must fire exactly 1 ADD_SOURCE under NON_IDEMPOTENT_NO_RETRY; got {add_count}"
|
||
)
|
||
assert get_count == 0, f"add_text must not probe; got {get_count} GET_NOTEBOOK"
|
||
|
||
|
||
# ---------------------------------------------------------------------------
|
||
# Probe-failure propagation (P1-2)
|
||
# ---------------------------------------------------------------------------
|
||
|
||
|
||
async def test_add_url_probe_network_error_propagates(auth_tokens) -> None:
|
||
"""When the probe itself fails with NetworkError, propagate the failure.
|
||
|
||
Previously the probe wrapper caught any Exception and returned ``None``,
|
||
which made ``idempotent_create`` re-issue the create on top of a broken
|
||
probe — duplicating the resource on the very next attempt. The fix
|
||
surfaces transport-layer probe failures directly so the caller can act
|
||
on them (refresh auth, back off, etc.) instead of silently retrying.
|
||
|
||
Triggered by:
|
||
- first ADD_SOURCE returns 502 → enters probe branch
|
||
- GET_NOTEBOOK raises a transport-level failure (httpx.ConnectError),
|
||
which ``rpc_call`` translates into NetworkError
|
||
- probe propagates NetworkError → idempotent_create surfaces it
|
||
to the caller
|
||
"""
|
||
notebook_id = "nb_test"
|
||
url = "https://example.com/article"
|
||
|
||
add_count = 0
|
||
get_count = 0
|
||
|
||
def handler(request: httpx.Request) -> httpx.Response:
|
||
nonlocal add_count, get_count
|
||
rpc_id = _rpc_id_in_request(request)
|
||
if rpc_id == RPCMethod.ADD_SOURCE.value:
|
||
add_count += 1
|
||
return httpx.Response(502, text="bad gateway")
|
||
if rpc_id == RPCMethod.GET_NOTEBOOK.value:
|
||
get_count += 1
|
||
# Synthesize a transport-layer connect failure for the probe.
|
||
raise httpx.ConnectError("probe synthetic connection error")
|
||
return httpx.Response(404, text=f"unexpected rpc_id={rpc_id}")
|
||
|
||
transport = httpx.MockTransport(handler)
|
||
client = _make_client_with_transport(transport, auth_tokens, server_error_max_retries=0)
|
||
try:
|
||
with pytest.raises(NetworkError, match="probe synthetic connection error"):
|
||
await client.sources.add_url(notebook_id, url)
|
||
finally:
|
||
await client._collaborators.kernel.get_http_client().aclose()
|
||
|
||
# Probe was attempted (the original create failed with 502, then the
|
||
# probe was issued and raised).
|
||
assert add_count == 1, f"expected 1 ADD_SOURCE before probe, got {add_count}"
|
||
assert get_count >= 1, f"probe must run; got {get_count}"
|
||
|
||
|
||
# ---------------------------------------------------------------------------
|
||
# Registry sanity check: variant-keyed entries are present and classified
|
||
# ---------------------------------------------------------------------------
|
||
|
||
|
||
def test_registry_has_variant_entries_for_add_source_and_add_source_file() -> None:
|
||
"""Smoke-test that the four required registry entries exist and have the
|
||
right policy classification.
|
||
|
||
This guards against accidental regressions where a refactor drops the
|
||
registry registration in ``_idempotency.py`` but leaves the per-variant
|
||
plumbing intact — the executor would silently fall back to UNCLASSIFIED
|
||
(today's retries) and the duplicate-source bug would resurrect.
|
||
"""
|
||
url_entry = IDEMPOTENCY_REGISTRY.get_entry(RPCMethod.ADD_SOURCE, operation_variant="url")
|
||
drive_entry = IDEMPOTENCY_REGISTRY.get_entry(RPCMethod.ADD_SOURCE, operation_variant="drive")
|
||
text_entry = IDEMPOTENCY_REGISTRY.get_entry(RPCMethod.ADD_SOURCE, operation_variant="text")
|
||
file_entry = IDEMPOTENCY_REGISTRY.get_entry(RPCMethod.ADD_SOURCE_FILE, operation_variant=None)
|
||
|
||
assert url_entry.policy is IdempotencyPolicy.PROBE_THEN_CREATE
|
||
assert drive_entry.policy is IdempotencyPolicy.PROBE_THEN_CREATE
|
||
assert text_entry.policy is IdempotencyPolicy.NON_IDEMPOTENT_NO_RETRY
|
||
assert file_entry.policy is IdempotencyPolicy.PROBE_THEN_CREATE
|