Skip to content

Commit e5026d1

Browse files
chore(sdks/python): fix pyright errors from CI on 8d97a64
CI's Python SDK Quality (sandbox) job failed at the pyright step with 10 errors after I ignored that lint in earlier rounds. All on this PR's new code; tests and ruff are unaffected. ### Three categories **(1) ``timedelta | None`` not assignable to ``timedelta`` (4 errors)** The auto-default change in 4e4f12c made ``PoolConfig.acquire_min_remaining_ttl`` declared as ``timedelta | None`` (``None`` means "auto-derive from idle_timeout" at ``__post_init__``). Even though ``__post_init__`` always resolves it to a concrete ``timedelta`` via ``object.__setattr__``, pyright can't see through that, so passing it to the helper functions tripped. Fix: widen the helper signatures to accept ``timedelta | None`` and treat ``None`` as zero. This matches what ``__post_init__`` would do anyway and removes the need for ``cast`` at every call site. **(2) ``_kill_sandbox_best_effort`` returns ``bool`` but reconciler wants ``Callable[[str], None]`` (2 errors)** In 33ee6f3 ``_kill_sandbox_best_effort`` was changed to return ``bool`` so ``_kill_discarded_alive`` could fire the success debug log only on a confirmed kill. But the reconciler's ``on_discard_sandbox`` parameter is typed ``Callable[[str], None]``, and ``bool`` is not a subtype of ``None``. Fix: introduce a thin ``_discard_sandbox_callback`` adapter on both pool facades that calls ``_kill_sandbox_best_effort`` and drops the return. The reconciler now receives the adapter; the bool is still available to ``_kill_discarded_alive`` for the success-only log. **(3) ``getattr(store, ...)`` returns ``object`` (4 errors)** The Python ``hasattr``-based store-compat shim in ``pool_types.py`` uses ``getattr(store, "try_take_idle_min_ttl", None)``, which pyright types as ``object``. Calling that or awaiting it tripped four errors in the helpers. Fix: ``cast`` each ``getattr`` result to the actual signature (``Awaitable[TakeIdleResult]`` / ``Iterable[str] | None`` etc.). Behavior unchanged at runtime — ``callable(method)`` still gates the call. ### Verification - ``uv run pyright`` — 0 errors - ``uv run ruff check`` — clean - ``uv run pytest`` — 207 passed - Kotlin ``:sandbox:test`` + ``:sandbox-pool-redis:test`` (real Redis 7) + ``spotlessCheck`` — all green Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 8d97a64 commit e5026d1

3 files changed

Lines changed: 40 additions & 20 deletions

File tree

sdks/sandbox/python/src/opensandbox/pool_async.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,7 @@ async def _run_reconcile_tick(self) -> None:
370370
config=self._config.with_max_idle(await self._resolve_max_idle()),
371371
state_store=self._state_store,
372372
create_one=self._create_one_sandbox,
373-
on_discard_sandbox=self._kill_sandbox_best_effort,
373+
on_discard_sandbox=self._discard_sandbox_callback,
374374
reconcile_state=self._reconcile_state,
375375
)
376376
except Exception as exc:
@@ -484,6 +484,13 @@ def _connection_for_pool_resource(self) -> ConnectionConfig:
484484
config._owns_transport = True
485485
return config
486486

487+
async def _discard_sandbox_callback(self, sandbox_id: str) -> None:
488+
"""``Callable[[str], Awaitable[None]]`` adapter for the reconciler's
489+
``on_discard_sandbox`` hook. Drops the bool return value of
490+
:meth:`_kill_sandbox_best_effort`.
491+
"""
492+
await self._kill_sandbox_best_effort(sandbox_id)
493+
487494
async def _kill_sandbox_best_effort(self, sandbox_id: str) -> bool:
488495
"""Best-effort kill a sandbox via the pool's manager.
489496

sdks/sandbox/python/src/opensandbox/pool_types.py

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
from datetime import datetime, timedelta
2323
from enum import Enum
2424
from math import ceil
25-
from typing import TYPE_CHECKING, Protocol
25+
from typing import TYPE_CHECKING, Protocol, cast
2626
from uuid import uuid4
2727

2828
from opensandbox.config.connection_sync import ConnectionConfigSync
@@ -34,7 +34,7 @@
3434
)
3535

3636
if TYPE_CHECKING:
37-
from collections.abc import Awaitable
37+
from collections.abc import Awaitable, Iterable
3838

3939
from opensandbox.config.connection import ConnectionConfig
4040
from opensandbox.sandbox import Sandbox
@@ -348,53 +348,55 @@ def _require_positive(value: timedelta, message: str) -> None:
348348

349349

350350
def try_take_idle_with_min_ttl(
351-
store: PoolStateStore, pool_name: str, min_remaining_ttl: timedelta
351+
store: PoolStateStore, pool_name: str, min_remaining_ttl: timedelta | None
352352
) -> TakeIdleResult:
353353
"""Call ``store.try_take_idle_min_ttl`` if available, else fall back to ``try_take_idle``.
354354
355355
Pool stores added before #983 only implement :meth:`PoolStateStore.try_take_idle`.
356356
This helper preserves source compatibility for those stores: when the threshold is
357-
zero/negative or the store does not implement the variant, the binary-expiry path
358-
is used and the returned result has an empty ``discarded_alive_sandbox_ids``.
357+
``None``, zero, or negative — or the store does not implement the variant — the
358+
binary-expiry path is used and the returned result has an empty
359+
``discarded_alive_sandbox_ids``.
359360
"""
360-
if min_remaining_ttl.total_seconds() <= 0:
361+
if min_remaining_ttl is None or min_remaining_ttl.total_seconds() <= 0:
361362
return TakeIdleResult(sandbox_id=store.try_take_idle(pool_name))
362363
method = getattr(store, "try_take_idle_min_ttl", None)
363364
if callable(method):
364-
return method(pool_name, min_remaining_ttl)
365+
return cast(TakeIdleResult, method(pool_name, min_remaining_ttl))
365366
return TakeIdleResult(sandbox_id=store.try_take_idle(pool_name))
366367

367368

368369
async def try_take_idle_with_min_ttl_async(
369-
store: AsyncPoolStateStore, pool_name: str, min_remaining_ttl: timedelta
370+
store: AsyncPoolStateStore, pool_name: str, min_remaining_ttl: timedelta | None
370371
) -> TakeIdleResult:
371372
"""Async counterpart of :func:`try_take_idle_with_min_ttl`."""
372-
if min_remaining_ttl.total_seconds() <= 0:
373+
if min_remaining_ttl is None or min_remaining_ttl.total_seconds() <= 0:
373374
return TakeIdleResult(sandbox_id=await store.try_take_idle(pool_name))
374375
method = getattr(store, "try_take_idle_min_ttl", None)
375376
if callable(method):
376-
return await method(pool_name, min_remaining_ttl)
377+
coro = cast("Awaitable[TakeIdleResult]", method(pool_name, min_remaining_ttl))
378+
return await coro
377379
return TakeIdleResult(sandbox_id=await store.try_take_idle(pool_name))
378380

379381

380382
def reap_expired_idle_with_min_ttl(
381383
store: PoolStateStore,
382384
pool_name: str,
383385
now: datetime,
384-
min_remaining_ttl: timedelta,
386+
min_remaining_ttl: timedelta | None,
385387
) -> tuple[str, ...]:
386388
"""Call ``store.reap_expired_idle_min_ttl`` if available, else fall back.
387389
388390
Returns the IDs of alive sandboxes the store dropped because their remaining TTL fell
389-
below the threshold, so callers can kill them. Stores predating this method return an
390-
empty tuple.
391+
below the threshold, so callers can kill them. Stores predating this method, or callers
392+
passing ``None`` / zero / negative, get an empty tuple.
391393
"""
392-
if min_remaining_ttl.total_seconds() <= 0:
394+
if min_remaining_ttl is None or min_remaining_ttl.total_seconds() <= 0:
393395
store.reap_expired_idle(pool_name, now)
394396
return ()
395397
method = getattr(store, "reap_expired_idle_min_ttl", None)
396398
if callable(method):
397-
result = method(pool_name, now, min_remaining_ttl)
399+
result = cast("Iterable[str] | None", method(pool_name, now, min_remaining_ttl))
398400
return tuple(result) if result else ()
399401
store.reap_expired_idle(pool_name, now)
400402
return ()
@@ -404,15 +406,18 @@ async def reap_expired_idle_with_min_ttl_async(
404406
store: AsyncPoolStateStore,
405407
pool_name: str,
406408
now: datetime,
407-
min_remaining_ttl: timedelta,
409+
min_remaining_ttl: timedelta | None,
408410
) -> tuple[str, ...]:
409411
"""Async counterpart of :func:`reap_expired_idle_with_min_ttl`."""
410-
if min_remaining_ttl.total_seconds() <= 0:
412+
if min_remaining_ttl is None or min_remaining_ttl.total_seconds() <= 0:
411413
await store.reap_expired_idle(pool_name, now)
412414
return ()
413415
method = getattr(store, "reap_expired_idle_min_ttl", None)
414416
if callable(method):
415-
result = await method(pool_name, now, min_remaining_ttl)
417+
coro = cast(
418+
"Awaitable[Iterable[str] | None]", method(pool_name, now, min_remaining_ttl)
419+
)
420+
result = await coro
416421
return tuple(result) if result else ()
417422
await store.reap_expired_idle(pool_name, now)
418423
return ()

sdks/sandbox/python/src/opensandbox/sync/pool.py

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -357,7 +357,7 @@ def _run_reconcile_tick_locked(self, executor: ThreadPoolExecutor) -> None:
357357
config=self._config.with_max_idle(self._resolve_max_idle()),
358358
state_store=self._state_store,
359359
create_one=self._create_one_sandbox,
360-
on_discard_sandbox=self._kill_sandbox_best_effort,
360+
on_discard_sandbox=self._discard_sandbox_callback,
361361
reconcile_state=self._reconcile_state,
362362
warmup_executor=executor,
363363
)
@@ -467,6 +467,14 @@ def _connection_for_pool_resource(self) -> ConnectionConfigSync:
467467
config._owns_transport = True
468468
return config
469469

470+
def _discard_sandbox_callback(self, sandbox_id: str) -> None:
471+
"""``Callable[[str], None]`` adapter for the reconciler's ``on_discard_sandbox``
472+
hook. The reconciler does not care whether the kill succeeded — it only needs the
473+
sandbox to be removed from the pool's bookkeeping — so we drop the bool return
474+
value here.
475+
"""
476+
self._kill_sandbox_best_effort(sandbox_id)
477+
470478
def _kill_sandbox_best_effort(self, sandbox_id: str) -> bool:
471479
"""Best-effort kill a sandbox via the pool's manager.
472480

0 commit comments

Comments
 (0)