Skip to content

Stop severing plugin artifact pulls at fixed timeouts - #6228

Merged
samuv merged 1 commit into
mainfrom
fix/ai-plugin-timeouts
Aug 6, 2026
Merged

Stop severing plugin artifact pulls at fixed timeouts#6228
samuv merged 1 commit into
mainfrom
fix/ai-plugin-timeouts

Conversation

@samuv

@samuv samuv commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

#6224 fixed two independent timeout ceilings for thv skill. I noted there that PluginsRouter looked like it had the same problem. It has both of them, for a structural reason: the plugins client is a near-copy of the skills client, and the plugins router sat in the same standardRouters map.

1. The client gave up at 30s and reported the server as unreachable, when the server was healthy and pulling. Abandoning the request cancels its context server-side, so the work is discarded rather than continuing — retrying restarts from nothing.

2. The plugins router inherited the flat 60s cap. WorkloadRouter and now SkillsRouter are both mounted outside standardRouters precisely because artifact pulls take minutes. install, build, and push move OCI artifacts the same way; the router just never got the same treatment. The route docs already anticipate this — several carry 504 Gateway Timeout (upstream pull timed out).

Changes, mirroring #6224 exactly:

  • ErrRequestTimeout, distinct from ErrServerUnreachable; caller cancellation (Ctrl-C) is classified as neither and surfaces as context.Canceled.
  • Client default raised, with TOOLHIVE_API_TIMEOUT as an override. An explicit WithTimeout still outranks the env var; an unparsable or non-positive value is ignored with a warning rather than disabling the timeout.
  • PluginsRouter gains per-route timeouts — long on install, build, push; short on the read routes — and is mounted outside standardRouters.
  • formatAIPluginError gives the hint that matches the failure and names the override.

Follows #6212 / #6224.

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)
  • Manual testing (describe below)

Same coverage as the skills PR: TestTimeoutFromEnv (table over unset / durations / whitespace / bare number / garbage / zero / negative), TestNewDefaultClientTimeoutPrecedence, and three classification tests over real sockets — slow server yields ErrRequestTimeout and not ErrServerUnreachable, closed port yields the reverse, cancelled caller context yields neither.

Because remounting a router is the kind of change that silently breaks routing, I checked the live surface rather than trusting the diff:

GET  /plugins        -> 200
GET  /plugins/builds -> 200
GET  /plugins/nope   -> 404
GET  /skills         -> 200

$ thv ai-plugin list
No plugins installed

/plugins/builds resolving to the builds route and /plugins/nope to 404 confirms the sub-route ordering survived the move.

Does this introduce a user-facing change?

Yes. thv ai-plugin operations that pull OCI artifacts no longer fail on slow or large pulls. Timeout failures say the request timed out instead of claiming the server is unreachable, and TOOLHIVE_API_TIMEOUT overrides the client limit — the same variable that already works for thv skill.

Special notes for reviewers

pkg/plugins/client and pkg/skills/client are now carrying a third and fourth copy of the same logic (timeoutFromEnv, classifyTransportError, the two sentinels, and the resolution ladder). I mirrored rather than extracted, because the two packages deliberately parallel each other and extracting mid-fix would have made this diff hard to review against #6224. But the duplication is real and getting harder to keep in sync — pkg/plugins/project_root.go already re-exports from skills, so there is precedent for consolidating. Worth a follow-up if you agree.

Generated with Claude Code

thv ai-plugin had both ceilings that #6212 fixed for skills, for the same
reason: its client is a near-copy of the skills client and its router sat
among the standard routers.

The client gave up after 30s and blamed server availability, when the
server was healthy and mid-pull. Past that, the plugins router inherited
the flat 60s cap that the workload and skills routers are both exempt
from, on the grounds that artifact pulls take minutes.

- classify timeouts as ErrRequestTimeout, separate from unreachability,
  and leave caller cancellation as neither
- raise the client default and allow TOOLHIVE_API_TIMEOUT to override it
- give the plugins router per-route timeouts, long ones on install,
  build, and push

Follows #6212.
@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Aug 6, 2026
@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.12195% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.47%. Comparing base (e91586a) to head (ddc1cef).

Files with missing lines Patch % Lines
pkg/plugins/client/client.go 92.85% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6228      +/-   ##
==========================================
- Coverage   72.47%   72.47%   -0.01%     
==========================================
  Files         739      739              
  Lines       76767    76790      +23     
==========================================
+ Hits        55639    55650      +11     
- Misses      17163    17171       +8     
- Partials     3965     3969       +4     

☔ 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.

@samuv
samuv merged commit 2e83cd1 into main Aug 6, 2026
48 checks passed
@samuv
samuv deleted the fix/ai-plugin-timeouts branch August 6, 2026 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants