Skip to content

fix(hooks): remove id(config) from get_global_client cache key to stop unbounded growth#3179

Open
nankingjing wants to merge 1 commit into
volcengine:mainfrom
nankingjing:fix/get-global-client-cache-leak
Open

fix(hooks): remove id(config) from get_global_client cache key to stop unbounded growth#3179
nankingjing wants to merge 1 commit into
volcengine:mainfrom
nankingjing:fix/get-global-client-cache-leak

Conversation

@nankingjing

Copy link
Copy Markdown
Contributor

Bug

The shared VikingClient cache in openviking_hooks.get_global_client keyed on (workspace_id, running_loop_id, id(config)). Because id() returns the in-memory address and config objects are rebuilt on every fresh request, each new config created a new cache entry — unbounded growth, leak.

The two genuine reasons to create a new client are different workspace (admin/scoping) and different running event loop (loop-bound asyncio primitives). The cache key was over-specified by including a non-stable handle.

Fix

cache_key = (
    str(workspace_id or "__default__"),
    id(asyncio.get_running_loop()),
)

config is forwarded to the cached client only on cache miss (the first call for a given workspace + loop), matching the original intent — one client per (workspace, loop).

Regression tests

5 tests in tests/unit/test_global_client_cache.py:

  • reuse across rotating config objects (the regression)
  • cache size does not grow with repeated calls
  • different workspaces still get distinct clients
  • workspace_id=None normalized to stable default key
  • cache key shape is exactly 2-tuple (guards against re-introducing id(config))

All 5 pass locally.

The shared VikingClient cache in openviking_hooks.get_global_client keyed
on (workspace_id, running_loop_id, id(config)). Because id() returns the
in-memory address and config objects are rebuilt on every fresh request,
each new config created a new cache entry — unbounded growth, leak.

Key on (workspace_id, running_loop_id) only. The two genuine reasons to
create a new client are:
- different workspace (admin/scoping)
- different running event loop (loop-bound asyncio primitives)

config is forwarded to the cached client only on cache miss (i.e. the
first call for a given workspace + loop), which matches the original
intent — a single client per (workspace, loop).

5 regression tests in tests/unit/test_global_client_cache.py cover:
- reuse across rotating config objects (regression)
- cache size does not grow with repeated calls
- different workspaces still get distinct clients
- None workspace normalized to '__default__'
- cache key shape is exactly 2-tuple
@nankingjing

Copy link
Copy Markdown
Contributor Author
Self-reviewed: diff is correct, minimal, and well-tested. No issues found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

1 participant