Skip to content

Name the tasks created by mass.create_task - #6334

Draft
balloob wants to merge 4 commits into
devfrom
claude/friendly-rubin-xzj5qb
Draft

balloob wants to merge 4 commits into
devfrom
claude/friendly-rubin-xzj5qb

Conversation

@balloob

@balloob balloob commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

What does this implement/fix?

Name tasks so we can find the culprit that's slow and causing warnings.

Related issue (if applicable):

  • related issue

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix
  • New feature (non-breaking change which adds functionality) — new-feature
  • Enhancement to an existing feature — enhancement
  • New music/player/metadata/plugin provider — new-provider
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — breaking-change
  • Refactor (no behaviour change) — refactor
  • Documentation only — documentation
  • Maintenance / chore — maintenance
  • CI / workflow change — ci
  • Dependencies bump — dependencies

Checklist

  • The code change is tested and works locally.
  • pre-commit run --all-files passes.
  • pytest passes, and tests have been added/updated under tests/ where applicable.
  • For changes to shared models, the companion PR in music-assistant/models is linked.
  • For changes affecting the UI, the companion PR in music-assistant/frontend is linked.
  • I have read and complied with the project's AI Policy for any AI-assisted contributions.
  • I have raised a PR against the documentation repository targeting the main or beta branch as appropriate.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GMb5jsTKqjUmyELMK8HTn2

An asyncio task carries its name in its repr, which is what the slow-callback
warning and the task exception log print. The tasks were unnamed, so both
reported "Task-1052" and could not be traced back to a caller.

Set the name at construction, because an eagerly started task runs its first
step inside the constructor. The name defaults to task_id, with a separate
name parameter for callers that want to identify a task without deduplicating
it. The provider post-load task uses it to name its provider.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GMb5jsTKqjUmyELMK8HTn2
Copilot AI balanced review requested due to automatic review settings September 13, 2026 22:19
@musicassistant-bot

musicassistant-bot Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

✅ The title and description are good to go. Thanks!

@codspeed

codspeed Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 11 untouched benchmarks
⏩ 7 skipped benchmarks1


Comparing claude/friendly-rubin-xzj5qb (7fe34e9) with dev (62e26a5)2

Open in CodSpeed

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on dev (f839c25) during the generation of this report, so 62e26a5 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

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.

🟡 Changes recommended

Guarded methods receiving a name keyword now fail due to duplicate keyword forwarding, and task-name behavior lacks regression coverage.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds meaningful names to tasks created through MusicAssistant.create_task, improving asyncio diagnostics and exception logs.

Changes:

  • Adds explicit task names with task_id fallback.
  • Names provider post-load tasks.
  • Preserves guarded-request task naming.
File summaries
File Description
music_assistant/mass.py Adds task naming and provider-specific names.
music_assistant/helpers/util.py Supplies names for guarded request tasks.
Review details

Suppressed comments (1)

music_assistant/mass.py:798

  • [PROBLEM] The new task-naming contract is the entire behavior of this change, but the existing create_task test suite does not verify either branch. Add assertions that an explicit name is preserved and that task_id becomes the default name, so future constructor changes cannot silently return anonymous task diagnostics.
        task: asyncio.Task[_R] = asyncio.Task(
            coro, loop=self.loop, eager_start=eager_start, name=name or task_id
        )
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread music_assistant/helpers/util.py Outdated
Comment thread music_assistant/mass.py
@MarvinSchenkel
MarvinSchenkel marked this pull request as draft September 15, 2026 13:19
A guarded method is free to name a parameter after one of the create_task
options that guard_single_request passes. Forwarding its kwargs alongside
those options made such a call raise TypeError, which the new name option
turned from latent into reachable.

Build the coroutine in the wrapper instead, so the wrapped function consumes
its own keywords and only task options reach create_task.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GMb5jsTKqjUmyELMK8HTn2
Copilot AI review requested due to automatic review settings September 15, 2026 13:28

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.

🟡 Changes recommended

Task IDs may expose sensitive data in logs, the new keyword breaks callable argument forwarding, and naming lacks direct tests.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

music_assistant/mass.py:742

  • [PROBLEM] Adding name as a task option breaks the documented callable-plus-**kwargs form: create_task(func, name="x") previously forwarded name to func, but now consumes it as task metadata and invokes func without its required argument. The decorator workaround only protects guard_single_request; separate task metadata from target kwargs (for example via an options object), or otherwise preserve name forwarding for coroutine-function targets.
    music_assistant/mass.py:1490
  • [PROBLEM] The new task-naming behavior has no direct regression coverage: the added helper test only checks wrapped-argument forwarding and would pass even if create_task ignored name. Add focused coverage alongside the existing create_task tests in tests/core/test_server_base.py that asserts the explicit task name and the chosen safe default behavior.
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread music_assistant/mass.py Outdated
Task ids key on the arguments that identify a call, among them complete image
urls and search terms. Defaulting the asyncio task name to the task id put
those into the slow-callback warnings and the task exception log.

Name a task only when the caller passes one, and leave the rest to asyncio's
own numbering as before.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GMb5jsTKqjUmyELMK8HTn2
Copilot AI review requested due to automatic review settings September 15, 2026 13:38

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.

🟢 Approval recommended

The implementation is focused, backward-compatible for existing callers, and adequately tested.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings September 15, 2026 13:44

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.

🔵 Needs a closer look

The new name control keyword breaks the existing target-keyword forwarding contract.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

music_assistant/mass.py:742

  • [PROBLEM] Adding name as a control keyword breaks the existing **kwargs forwarding contract: create_task(func, name=value) now omits a required argument from func, and the decorator workaround only protects guard_single_request while other callers (including call_later) remain exposed. Use a non-colliding option such as task_name and update the new caller/tests accordingly.
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants