Skip to content

livekit-api: thread and database clients - #930

Open
theomonnom wants to merge 10 commits into
mainfrom
theo/thread-database-clients
Open

theomonnom wants to merge 10 commits into
mainfrom
theo/thread-database-clients

Conversation

@theomonnom

@theomonnom theomonnom commented Nov 8, 2026 •

Copy link
Copy Markdown
Member

Adds LiveKitAPI.threads and LiveKitAPI.databases (agent threads and agent databases on the LiveKit Cloud Public API), mirroring livekit/rust-sdks#1379.

from livekit import api

async with api.LiveKitAPI() as lkapi:
    thread = await lkapi.threads.create_thread(api.CreateThreadRequest(scope="user_42"))
    items = await lkapi.threads.get_thread_items(api.GetThreadItemsRequest(thread_id=thread.thread_id))
    await lkapi.threads.delete_thread(thread_id=thread.thread_id)

    dbs = await lkapi.databases.list_databases(api.ListDatabasesRequest())

Errors are the same ServerError as the other services. Twirp and Public API services now share one BaseService, and the error types moved to livekit.api.errors.

livekit/api/_generated/ is generated by livekit-api/public-api/generate.sh from the vendored openapi.yaml (OpenAPI Generator, aiohttp). The spec is from public-api-server@e273831, since livekit/public-api-server#150 hid these operations.

…se clients

Generate agent/livekit_agent_thread.proto (AgentThread, AgentTask) at the
current protocol pin. Add LiveKitAPI.threads and LiveKitAPI.databases, served
by the LiveKit Cloud Public API. The HTTP layer is generated with OpenAPI
Generator (python, asyncio) from the vendored spec in livekit-api/public-api;
run generate.sh there to re-vendor and regenerate.
@theomonnom
theomonnom marked this pull request as ready for review November 8, 2026 22:23
devin-ai-integration[bot]

This comment was marked as resolved.

@devin-ai-integration devin-ai-integration Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.

Devin Review found 1 new potential issue.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review


async def _call(
self,
*,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Negative page sizes break list requests

For a negative list limit or page_size, _call sends that value instead of leaving the page size unset. The server receives an invalid page.pageSize, so the list request fails rather than using its default size.

Learn more

The four list methods pass protobuf integer page sizes through list, which forwards them as page_page_size to _call. The filter omits zero but preserves negative sizes, while the earlier page_size helper omitted every nonpositive size. The generated list operations serialize any present integer into page.pageSize, so a negative size reaches the server.

Example: With AgentThread.ListRequest(limit=-1), list_threads now sends page.pageSize=-1. Previously it omitted page.pageSize, letting the server apply its default page size.

Recommended fix: Normalize page_page_size in PublicApiClient.list with limit if limit > 0 else None, while preserving _call filtering for optional strings and zero-valued fields.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration[bot]

This comment was marked as resolved.

@xianshijing-lk xianshijing-lk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for the three reproduced correctness findings (comments 1–3): numeric RPC error mapping, timeout handling, and nested response validation. Comments 4–8 concern public API design/documentation to resolve before release; comments 9–16 are non-blocking improvements or questions.

Validation on head 3a99544: API suite 59 passed / 28 skipped on Python 3.9, 3.12 and 3.14; 16 supplemental checks passed, and 11 targeted regression cases reproduced the three correctness findings. Builds, mypy, Ruff, formatting, pdoc generation, and exact OpenAPI regeneration were checked. The skipped tests need the unavailable shared mock server; no live Cloud operations were exercised. The Python 3.9 minimum-dependency run required protobuf 5.29.3 because of an existing generated-code/runtime floor mismatch. The changeset release flow was separately verified to raise the livekit-protocol dependency floor correctly.

Two additional comments concern files outside this PR's diff, so they are included here:

11. [P3, non-blocking] Update service discoverability — README.md:118, livekit-api/livekit/api/__init__.py:19

Please add threads and databases to the README's service examples and the package description, with links to their documentation. These new public capabilities are currently absent from the existing service overview.

12. [Testing, non-blocking] Exercise Public API operations from installed wheels — .github/workflows/test-api.yml:107

The wheel tests import livekit.api, but the token/webhook tests do not invoke the lazily loaded generated Public API client.

Could we include the local-server Public API tests in the installed-wheel matrix, especially with minimum dependencies? This would cover generated-module packaging and runtime compatibility beyond the workspace environment.

except ValueError:
data = None
if isinstance(data, dict):
code = data["code"] if isinstance(data.get("code"), str) else None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. [P2] Preserve numeric RPC error codes

The spec defines google.rpc.Status.code as an integer, but this implementation only preserves string codes. Falling back to HTTP status loses distinctions: ABORTED becomes already_exists; FAILED_PRECONDITION and OUT_OF_RANGE become invalid_argument; DATA_LOSS and UNKNOWN become internal. Callers cannot reliably distinguish these failures using ServerErrorCode.

Please map recognized numeric RPC codes to the existing constants before using the HTTP fallback. Keep the fallback for missing/unrecognized codes and non-JSON gateway errors, and correct the comment claiming numeric codes convey no additional information. Please add regression tests for codes sharing the same HTTP status.

timeout: Optional[aiohttp.ClientTimeout] = None,
) -> None:
self.host = public_api_url(host)
self.timeout = timeout.total if timeout and timeout.total else DEFAULT_TIMEOUT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2. [P2] Preserve the complete timeout configuration

Reducing ClientTimeout to a float discards connection and socket-read deadlines, replaces disabled total deadlines (None/0) with 10 seconds, and overrides a caller-supplied session's timeout when no explicit override was provided.

Please preserve the full configuration and inherit the session's timeout when there is no override. This requires adapting the transport boundary: the generated operation currently accepts a number or tuple, not ClientTimeout. Add runtime tests for session defaults, socket-read deadlines, and disabled total deadlines; assertions on the stored .total value do not cover these cases.

# A limit of zero or below leaves the page size to the server.
params={"page_cursor": page_token, "page_page_size": max(limit, 0), **params},
)
next_page = (data.get("pageInfo") or {}).get("nextCursor", "")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3. [P2] Validate nested successful-response shapes

Malformed nested payloads bypass the intended error handling. {"thread":5} leaks TypeError, malformed pageInfo leaks AttributeError, and {"thread":"bad"} or {"thread":[]} silently returns an empty Thread. The resource conversion at line 194 has the same gap.

Please validate nested resource and pagination shapes and raise ServerError(MALFORMED) with the response's HTTP status. Preserve legitimate omitted/default protobuf fields and unknown-field compatibility, and add regression coverage for these cases.

```
"""

async def create_thread(self, req: CreateThreadRequest) -> Thread:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4. [API design] Settle the request-argument convention before release

Across ThreadService and DatabaseService, seven methods accept request protos while five accept keyword-only identifiers. Corresponding Get/Delete/Export request protos already exist but are unused and unexported.

Request objects would align with RoomService; direct arguments also have precedent in agent-dispatch and SIP. Please make the convention explicit and ensure the signatures, exports, and examples reflect that choice before consumers depend on this public API.

),
)

async def get_thread(self, *, thread_id: str) -> Thread:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5. [Docs] Complete the public method documentation

get_thread, get_database, and list_databases have no docstrings. The remaining methods mostly provide summaries without parameter or return guidance.

Please document all twelve methods using the SDK's Args/Returns style. The documentation workflow renders these with pdoc's Google docstring format. Include relevant error behavior without implying that transport failures and input-validation errors are always ServerError.

# Each request signs its own token, so a short lifetime is enough.
TOKEN_TTL = datetime.timedelta(seconds=60)

def __init__(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10. [Docs, non-blocking] Explain transport and constructor differences

These services use Public API routing and do not inherit Twirp failover behavior. Their direct constructors also omit the positional failover argument accepted by the existing services.

Please document these differences and clarify which services LiveKitAPI.failover affects. Prefer explicit documentation over accepting and silently ignoring the argument. Also avoid implying that every self-hosted LiveKit server implements these endpoints merely because non-Cloud URLs are forwarded unchanged (see public_api_url's docstring).

return STAGING_URL
if hostname.endswith(".livekit.cloud"):
return PRODUCTION_URL
return http.rstrip("/")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

13. [P3, non-blocking] Reject scheme-less URLs with a clear configuration error

public_api_url("x.livekit.cloud") returns the scheme-less value unchanged, leaving the failure to surface later during a request.

Could we validate supported URL schemes and report the configuration problem earlier? LiveKitAPI already rejects empty URLs; similarly clear handling would help callers diagnose malformed URLs.

"protobuf>=4",
"types-protobuf>=4",
"livekit-protocol>=1.1.31,<2.0.0",
# The generated Public API client (livekit/api/_generated, see public-api/generate.sh).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

14. [P3, non-blocking] Mention the new mandatory dependencies in the changeset

Lazy loading avoids importing the generated client at package startup, but pydantic, aiohttp-retry, python-dateutil, and typing-extensions still become installation requirements.

Could the changeset mention these additions so downstream users can anticipate the dependency changes?

"""The JSON object of a successful response and its status. Required parameters (the
operation's own, without a default) must be set; optional ones at their protobuf
default are left unset."""
required = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

15. [Simplification, non-blocking] Replace runtime signature inspection with explicit parameter handling

_call() inspects the generated operation's signature to discover required parameters, then infers which values to omit. This makes request behavior depend on both generated Python signatures and protobuf defaults.

Could we validate required path identifiers and normalize optional query fields explicitly at the service boundary—for example, scope=req.scope or None and a page size only when positive? The transport could then omit None query parameters, while path identifiers are validated at the call site and never omitted. This would remove reflection and make default/presence behavior easier to audit.

# The generated client (and pydantic) load on the first Public API call, not with
# `import livekit.api`.
@functools.cached_property
def _api(self) -> Any:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

16. [Question, non-blocking] Document the generated/handwritten boundary

Could we document the intended boundary between generated and handwritten code? This adds 47 generated Python files, approximately 8,848 lines, for twelve operations. Requests pass through protobuf → dictionary → Pydantic → dictionary → JSON, while responses bypass generated model deserialization and are parsed into protobufs by the adapter.

The adapter also supplies session management and error handling, and service methods depend on generated operation names and request-model conversion conventions. Generation provides useful spec-derived paths, query names, and encoding; explaining that division of responsibility would help future maintainers understand which layer owns each behavior. This is a request to clarify the boundary, not to remove generated code.

@xianshijing-lk xianshijing-lk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work!!! left a few comments, I think it is ready to go once some of them are addressed.

@@ -0,0 +1,5 @@
---
"livekit-api": patch

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, should it be major ? or at least minor ?


async def export_database(self, *, database_id: str) -> ExportDatabaseResponse:
"""A pre-authenticated download URL of a consistent snapshot of the database."""
return await self._client.request(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curiously, does ExportDatabase prepare the snapshot before returning, and can that exceed the 10s default for supported database sizes? Asking because the response carries a ready downloadUrl, which suggests the staging happens inline.

If so, two things would help beyond the timeout comment above: a per-call timeout override on export_database, following SipService.create_sip_participant's timeout: Optional[float] = None; and a docstring note that the snapshot scales with database size.

Separately, worth documenting that a client timeout doesn't imply the server cancelled or failed : these calls carry no X-Livekit-Request-Id, unlike the Twirp path, so a retry after a timeout isn't deduplicated. That matters most for create_database, where the docstring already notes a retry creates a second database, and for delete_database, where it'd be good to state whether deletion is complete when the call returns or eventual.

401: ServerErrorCode.UNAUTHENTICATED,
403: ServerErrorCode.PERMISSION_DENIED,
404: ServerErrorCode.NOT_FOUND,
409: ServerErrorCode.ALREADY_EXISTS,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curiously, 409 in the original code can mean both already_exists and aborted, are we now binding it to already_exists and dropped aborted ?

This branch has not been deployed

No deployments
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.

2 participants