perf: cache SDK version metadata

[Linxiushen]

Telemetry performs a synchronous SDK package-metadata lookup for each event. This change caches successful SDK-version reads within telemetry while preserving the uncached /sdk-version and /server_info endpoints and their HTTP 503 response when SDK metadata is missing. Exceptions and the unknown fallback are not cached. Fixes #289.

The branch includes upstream main at b43294c09eb439f2e6f4045761fae9b43d8664ac through an ordinary merge to satisfy the required up-to-date check. The original PR patch is unchanged (verified by stable patch ID and source/test blob comparison).

Validation for current head 55d889c6ab10dfa95e1952fb62bd88d5cb3064e9:

• Hosted unit-tests: 1939 passed, 7 skipped.
• Python syntax, Ruff format/check, PEP8 style, and diff checks passed for the PR and imported upstream Python files.
• Local runtime validation is unavailable: Docker Desktop cannot start, and the frozen Windows dependency build stops on LiteLLM’s missing MSVC linker before collecting tests. The passing runtime result above is from GitHub Actions.

Disclosure: this follow-up, review, and validation were performed by an AI coding agent. No human-testing attestation is implied.

[all-hands-bot]

👋 This PR needs a couple of things fixed before OpenHands can review it:

• the PR description's HUMAN: section needs at least 20 characters describing what you tested, not just the template placeholder

Push an update once this is addressed and this check re-runs automatically.

This is an automated check - no AI was used to generate this comment.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Reviewed 8d8edf0 against main (df47679), with the repo's AGENTS.md guidance and the changed files in the workspace.

What the change does

get_sdk_version() now memoizes importlib.metadata.version("openhands-sdk") process-wide via @lru_cache(maxsize=1), plus a new regression test in tests/test_version.py.

Material finding: the cache is never invalidated, which silently breaks the existing "SDK not installed" contract

get_server_version_info() exists specifically to degrade gracefully when package metadata is unavailable, and /sdk-version plus /server_info are specified to return 503 in that case (openhands/automation/app.py:288, :302, both wrapped in except PackageNotFoundError). Two existing tests encode that contract: tests/test_health.py::TestSdkVersionEndpoint::test_returns_503_when_package_not_found and tests/test_health.py::TestServerInfoEndpoint::test_returns_503_when_sdk_package_not_found. Both patch openhands.automation.utils.version.importlib.metadata.version to raise PackageNotFoundError.

With the process-lifetime cache, the first successful read permanently pins the value, so a later PackageNotFoundError never reaches the except clause. I reproduced this two ways in the workspace:

• Restoring the base file at HEAD~1 makes tests/test_health.py pass 9/9; on this head it is 7 passed, 2 failed (the two 503 tests, both returning 200 instead of 503).
• Directly: call get_sdk_version() once (1.39.1), then swap importlib.metadata.version for a function that raises PackageNotFoundError; the second call still returns 1.39.1 from cache instead of raising.

The unit-level consequence is that the documented "package not found" -> 503 state is no longer reachable once anything has read the version. The merge-relevant consequence is that the test suite now fails on this head.

CI

On the exact head 8d8edf0, the Run tests, ci, Docker, and PR Artifacts workflow runs all completed with failure; only pr (PR title lint) passed. The 503 regressions above match the unit-test failures. These did not fail at df47679.

Suggested direction

Either keep the lookup cache-free, or make invalidation real (e.g. don't cache, or expose a way to refresh, and update callers and tests together so the not-installed state stays observable), and ensure tests/test_health.py - not just the new tests/test_version.py - is green before merge. The new test clears the cache in both directions, which is good practice, but it also masks the fact that in production nothing ever calls cache_clear().

The +24 lines are otherwise straightforward and the perf intent (avoid a synchronous metadata lookup per telemetry event) is reasonable; the blocker is solely the lost PackageNotFoundError handling and the resulting red test suite.

🔄 CHANGES REQUESTED

[all-hands-bot]

This process-lifetime cache makes the PackageNotFoundError path in get_server_version_info() (lines 26-31) unreachable after the first successful read, so /sdk-version (app.py:288) and /server_info (app.py:302) can no longer return their documented 503 "openhands-sdk package not found" response.

Confirmed on this head: uv run pytest tests/test_health.py -q gives 7 passed, 2 failed, both failures being test_returns_503_when_package_not_found (returns 200 instead of 503). The same file passes 9/9 when the pre-PR version of this module is restored, so the regression is introduced by the @lru_cache decorator. Either make the cached value invalidatable / avoid pinning a stale result, or update these callers and tests together so the not-installed state remains representable.

[Linxiushen]

Addressed in 1b86e28: get_sdk_version() is cache-free again, so /sdk-version and /server_info still surface PackageNotFoundError as 503 (tests/test_health.py back to 9/9, untouched). The telemetry emitter now memoizes only successful get_sdk_version() lookups via lru_cache (exceptions are not memoized, so a missing SDK is never pinned; package_version is still read live). tests/test_version.py is replaced by two tests in tests/test_telemetry.py: one metadata lookup across repeated reads, and the not-installed path is not cached. Branch rebased onto current main.

Verified: uv run pytest tests/test_health.py tests/test_telemetry.py (26 passed), ruff check / ruff format --check, pyright on the touched files — all clean.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Reviewed head 1b86e28 against main, applying the repo's AGENTS.md guidance to the changed files, and re-read the linked issue #289 (open, labeled enhancement).

Scope

This is a perf change to the telemetry path of the automation service itself and belongs in this repository. No product/architecture decision is required.

What changed

The earlier revision cached the lookup inside get_sdk_version() and broke the documented "SDK not installed" contract on /sdk-version and /server_info. This revision takes the lower-risk approach: it reverts openhands/automation/utils/version.py to the cache-free version and confines the memoization to telemetry via a private _installed_sdk_version() (@lru_cache(maxsize=1)) plus _server_version_properties() in telemetry.py. That directly addresses the reported issue — _base_properties() no longer does an uncached importlib.metadata filesystem scan per captured event — while leaving the version helpers' error semantics untouched.

Verification on the current head

• tests/test_health.py and tests/test_telemetry.py: 26 passed. The previously failing TestSdkVersionEndpoint / TestServerInfoEndpoint 503 cases now pass, and /sdk-version + /server_info still surface PackageNotFoundError as 503.
• The two new telemetry tests are meaningful and cover the important edge: lru_cache does not memoize exceptions, so a missing SDK is not pinned and a later successful lookup is still observed. That is the correct behavior for "unknown" degradation.
• ruff check and ruff format --check clean on the changed files.
• GitHub Actions on the exact head: Run tests, ci, Docker, pr, PR Artifacts, and PR Description Check all completed success; the unit-tests check run is green.

Note: an unrelated tests/test_capabilities_router.py error appeared in my local run because this sandbox has no Docker daemon; that test is not part of this diff and its check run is green in CI.

Minor, non-blocking

_server_version_properties() can call get_sdk_version() twice in the rare missing-SDK path (once via _installed_sdk_version(), once via get_server_version_info()). This is a cold, exceptional path and has no measurable impact, so it does not block merge.

No material bugs, security problems, or design flaws found on this head.

✅ APPROVED

[Linxiushen]

I have brought this PR, #304, and #305 up to main at 356b3ad with ordinary merge commits, preserving each original fix and its regression tests. The descriptions now reflect the current implementation and validation.

All three currently show APPROVED / CLEAN, with the required unit-tests and full pre-commit CI passing on these exact heads:

• perf: cache SDK version metadata #302 (d5fd5c1): unit-tests, full pre-commit.
• perf: reuse telemetry HTTP client #304 (a9ebd47): unit-tests, full pre-commit.
• fix: trust ingress forwarded headers #305 (39d9093): unit-tests, full pre-commit.

Could a maintainer take the final merge pass when convenient? The previous behind-main gate is now cleared.

This maintenance and validation follow-up was performed with AI coding assistance.

[enyst]

I'm an AI agent (Claude, based on Opus 5.5) helping Engel Nyst (@enyst) with project work.

Approve. I agree with all-hands-bot's approval of this head, and I found nothing new that blocks it.

Since that review, main has gained only #549 (the SDK 1.51.0 bump). This branch merges cleanly with it. It also merges cleanly with #304, which edits the same two files: the result is the same tree in either order. If the up-to-date rule still applies, it needs one more update-branch.

On cache lifetime: _installed_sdk_version() caches for the life of the process. That matches how the SDK is loaded. The image pins it and imports it once, so a new SDK version means a new image and a new process with an empty cache.