Skip to content

fix(api): keep provider refresh waiters single-flight#38226

Merged
WH-2099 merged 5 commits into
mainfrom
fix/api-plugin-provider-refresh-coordinator
Jul 2, 2026
Merged

fix(api): keep provider refresh waiters single-flight#38226
WH-2099 merged 5 commits into
mainfrom
fix/api-plugin-provider-refresh-coordinator

Conversation

@WH-2099

@WH-2099 WH-2099 commented Jun 30, 2026

Copy link
Copy Markdown
Member

Summary

This is a follow-up to #37388 for the provider-list refresh path tracked under #34264.

The previous single-flight lock prevented ordinary cache-miss stampedes, but waiters could still fall through to the plugin daemon after the lock TTL deadline.
That made the Redis lock TTL act as both a crashed-owner lease and a waiter deadline.

This PR makes the coordination contract explicit:

  • Redis lock TTL is only a lock lease.
  • Waiters keep reading the current provider generation and cache until they either read cache or acquire the refresh lock.
  • Redis cache or lock failures still fall back to direct daemon discovery to preserve availability.
  • Generation changes naturally restart the cache/lock path on the new generation.

CI Stability

This PR also fixes #38277.

test_inspect_skill_maps_invalid_archives_to_service_errors generated ZIP bytes during pytest collection.
Because zipfile.writestr(name, payload) stores the current timestamp in ZIP entry metadata, xdist workers could collect different parametrized node ids and fail before running tests.

The ZIP fixture now uses a fixed entry timestamp, and the parametrized cases have stable ids.

Tests

  • env DEBUG=false HTTP_PROXY= HTTPS_PROXY= ALL_PROXY= NO_PROXY= http_proxy= https_proxy= all_proxy= no_proxy= uv run --project api pytest -o addopts='' api/tests/unit_tests/services/plugin/test_plugin_service.py api/tests/unit_tests/core/plugin/test_model_runtime_adapter.py -q
  • uv run --project api ruff check api/core/plugin/plugin_service.py api/tests/unit_tests/services/plugin/test_plugin_service.py api/tests/unit_tests/core/plugin/test_model_runtime_adapter.py
  • env -u HTTP_PROXY -u HTTPS_PROXY -u ALL_PROXY -u NO_PROXY -u http_proxy -u https_proxy -u all_proxy -u no_proxy DEBUG=false uv run --project api pytest -p no:benchmark -n 2 api/tests/unit_tests/services/test_agent_config_service.py::test_inspect_skill_maps_invalid_archives_to_service_errors
  • git diff --check

Refs #34264
Fixes #38277

@WH-2099 WH-2099 self-assigned this Jun 30, 2026
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Pyrefly Type Coverage

Metric Base PR Delta
Type coverage 51.87% 51.87% +0.00%
Strict coverage 51.39% 51.39% +0.00%
Typed symbols 31,536 31,542 +6
Untyped symbols 29,539 29,539 0
Modules 2952 2952 0

@WH-2099 WH-2099 marked this pull request as ready for review June 30, 2026 11:25
@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jun 30, 2026
@WH-2099 WH-2099 enabled auto-merge June 30, 2026 11:36
@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.80328% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.27%. Comparing base (61650d3) to head (b57853c).

Files with missing lines Patch % Lines
api/core/plugin/plugin_service.py 91.80% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #38226      +/-   ##
==========================================
- Coverage   85.27%   85.27%   -0.01%     
==========================================
  Files        4982     4982              
  Lines      263256   263230      -26     
  Branches    50022    50014       -8     
==========================================
- Hits       224504   224480      -24     
- Misses      34366    34367       +1     
+ Partials     4386     4383       -3     
Flag Coverage Δ
api 85.50% <91.80%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cqjjjzr

cqjjjzr commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

I feel that we should not make this an infinite wait. A good idea would be introduce a hard timeout that once exceeded, fail the whole process.

Copilot AI 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.

Pull request overview

This PR refines the plugin provider-list refresh coordination in PluginService so concurrent cache-miss waiters remain single-flight across Redis lock TTL boundaries (treating lock TTL as a lease rather than a waiter deadline), while still preserving the “fallback to direct daemon discovery” behavior when Redis cache/lock coordination fails.

Changes:

  • Replaces the prior “wait with deadline” approach with a loop that keeps re-checking the current generation cache and retrying lock acquisition until cache is filled or the caller becomes refresh owner.
  • Tightens typing around Redis locks via a small internal Protocol and updates the lock acquisition/release helpers accordingly.
  • Updates and extends unit tests to cover (a) continuing to wait past lock TTL and (b) restarting the lock/cache path after generation changes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
api/core/plugin/plugin_service.py Reworks provider-cache coordination to keep waiters single-flight across lock TTL and generation changes.
api/tests/unit_tests/services/plugin/test_plugin_service.py Updates and adds tests validating the new wait/lock/generation behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread api/core/plugin/plugin_service.py
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jun 30, 2026
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Pyrefly Diff

base → PR
--- /tmp/pyrefly_base.txt	2026-07-02 04:15:00.682688556 +0000
+++ /tmp/pyrefly_pr.txt	2026-07-02 04:14:52.046710658 +0000
@@ -4619,8 +4619,6 @@
    --> tests/unit_tests/core/plugin/test_backwards_invocation_app.py:417:74
 ERROR Argument `SimpleNamespace` is not assignable to parameter `tenant` with type `Tenant` in function `core.plugin.backwards_invocation.model.PluginModelBackwardsInvocation.invoke_summary` [bad-argument-type]
   --> tests/unit_tests/core/plugin/test_backwards_invocation_model.py:54:72
-ERROR Generator function should return `Generator` [bad-return]
-  --> tests/unit_tests/core/plugin/test_model_runtime_adapter.py:72:51
 ERROR Class member `_BadString.__str__` overrides a member in a parent class but is missing an `@override` decorator [missing-override-decorator]
    --> tests/unit_tests/core/plugin/test_plugin_entities.py:238:17
 ERROR Argument `TestPluginDaemonEntities.test_credential_type_helpers._FakeCredential` is not assignable to parameter `self` with type `CredentialType` in function `core.plugin.entities.plugin_daemon.CredentialType.get_name` [bad-argument-type]
@@ -6827,8 +6825,6 @@
   --> tests/unit_tests/services/plugin/test_oauth_service.py:68:34
 ERROR Object of class `FunctionType` has no attribute `assert_called_once_with` [missing-attribute]
   --> tests/unit_tests/services/plugin/test_oauth_service.py:95:9
-ERROR Generator function should return `Generator` [bad-return]
-  --> tests/unit_tests/services/plugin/test_plugin_service.py:18:51
 ERROR Class member `DummyRetrieval.get_pipeline_templates` overrides parent class `PipelineTemplateRetrievalBase` in an inconsistent manner [bad-override]
  --> tests/unit_tests/services/rag_pipeline/pipeline_template/test_pipeline_template_base.py:5:9
 ERROR Class member `DummyRetrieval.get_pipeline_templates` overrides a member in a parent class but is missing an `@override` decorator [missing-override-decorator]

@WH-2099 WH-2099 force-pushed the fix/api-plugin-provider-refresh-coordinator branch from 067986f to ed0265f Compare June 30, 2026 18:00
@WH-2099 WH-2099 requested a review from crazywoola as a code owner June 30, 2026 18:00
@github-actions github-actions Bot added the web This relates to changes on the web. label Jun 30, 2026
crazywoola
crazywoola previously approved these changes Jul 1, 2026
@WH-2099 WH-2099 added this pull request to the merge queue Jul 1, 2026
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jul 1, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 1, 2026
@WH-2099 WH-2099 requested a review from Copilot July 1, 2026 04:05

Copilot AI 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.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

Comment thread api/core/plugin/plugin_service.py Outdated
@autofix-ci autofix-ci Bot requested a review from a team July 1, 2026 05:21
@WH-2099 WH-2099 force-pushed the fix/api-plugin-provider-refresh-coordinator branch from 08feeaa to a885793 Compare July 1, 2026 07:59
@github-actions github-actions Bot removed the web This relates to changes on the web. label Jul 1, 2026
@WH-2099 WH-2099 force-pushed the fix/api-plugin-provider-refresh-coordinator branch from a885793 to 3f2e02d Compare July 1, 2026 12:14
@cqjjjzr cqjjjzr force-pushed the fix/api-plugin-provider-refresh-coordinator branch from 404588f to b57853c Compare July 2, 2026 04:13

@cqjjjzr cqjjjzr 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.

LGTM

@WH-2099 WH-2099 added this pull request to the merge queue Jul 2, 2026
Merged via the queue into main with commit 7fb797c Jul 2, 2026
36 checks passed
@WH-2099 WH-2099 deleted the fix/api-plugin-provider-refresh-coordinator branch July 2, 2026 05:00
hjlarry pushed a commit that referenced this pull request Jul 3, 2026
@dosubot dosubot Bot mentioned this pull request Jul 7, 2026
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(api): stabilize ZIP archive fixture collection under xdist

4 participants