Skip to content

fix: make task markers inert outside docket workers - #11

Open
BrennanTM wants to merge 2 commits into
lanl:codex/fastmcp-v4-betafrom
BrennanTM:fastmcp-v4-direct-call-fix
Open

fix: make task markers inert outside docket workers#11
BrennanTM wants to merge 2 commits into
lanl:codex/fastmcp-v4-betafrom
BrennanTM:fastmcp-v4-direct-call-fix

Conversation

@BrennanTM

Copy link
Copy Markdown

Summary

Fixes the Demo Image smoke failure on codex/fastmcp-v4-beta, the branch behind #7. FastMCP's dependency resolver resolves any parameter default that is a Dependency instance on plain tools/call too, and both task markers placed by build_torch_module_fastmcp_tool are worker-only. On the direct path there is no docket worker, so resolving _task_concurrency raises LookupError on current_execution, which surfaces as the RuntimeError: Failed to resolve dependency '_task_concurrency' for fn in the CI log. _task_admission carries the same exposure in its queue-full branch, where AdmissionBlocked(current_execution.get(), ...) would replace the manager's graceful Server busy error with the same internal RuntimeError.

The change

_WorkerScopedConcurrencyLimit subclasses docket's ConcurrencyLimit and no-ops on enter and exit unless current_execution is set, delegating to the real enforcement inside a worker. DocketQueueAdmission.__aenter__ returns before reserving when no worker owns the execution. Direct calls therefore fall through to _enqueue_request's own backpressure, the same manager-side limit that governs direct calls on main, and worker-side task enforcement is unchanged. No dependency changes: pydocket stays at 0.20.1 (its 0.21 to 0.24 releases rewrite the concurrency engine and still require worker context, so a bump neither fixes this nor is needed by it).

Verification

  • Three regression tests, committed red first and flipped by the fix with no test edits: a direct run() of a built tool outside any worker (the smoke failure's exact shape), the admission marker reserving nothing outside a worker, and the concurrency marker delegating only inside worker context.
  • End to end on this branch's frozen lockfiles: the exact smoke tools/call returns real MIST inference, and a six-way concurrent burst at max_pending_per_tool: 1 yields two successes, four graceful Server busy errors, zero marker errors, and a follow-up success, so no reservation leaks.
  • Full suite: 362 passed; the one failure (test_uses_workspace_virtualenv_for_runtime) fails on my machine with or without this change.
  • CI expectation: this fixes the image smoke. The observability smoke has never executed on this branch because the job died before reaching it, so its first result will be information about the migration rather than about this change.

This is your migration, so treat this as a ready-made option; happy to adjust or step back if you prefer a different shape.

A plain tools/call resolves the task markers with no docket worker
context. Pre registered red: dependency resolution raises for
_task_concurrency, the admission marker cannot reach its cleanup
stack, and the concurrency marker delegates unconditionally.
FastMCP resolves Dependency typed defaults on plain tools/call, where
no docket worker context exists. The concurrency marker now delegates
to docket only inside a worker and reads as inert otherwise, and the
admission marker returns before reserving queue capacity, letting
_enqueue_request apply its own backpressure with the client facing
Server busy error. Worker side task enforcement is unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant