You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
refactor(account): share the entry scope of FinishAsync and SetScrobblingAsync - #281
FinishAsync and SetScrobblingAsync in AccountService now enter an account call through one private helper. Before this change, both started with the same seven statements: the caller check, the host-access check, the current authorization, a token linked to that authorization, and the per-user account gate. The duplicate-code scan reported the two copies as one 9-line clone group.
Shared entry:EnterAccountAsync runs those steps and returns an AccountScope that carries the operation, the authorization and the linked token. Disposing the scope releases the gate first and then the linked token, the same order the two using declarations had. If the gate wait throws, the linked token is disposed, as before.
BeginAsync unchanged: it validates the callback between the host-access check and the authorization lookup. Moving that check into the helper would change which error a disabled user with a bad callback sees, so BeginAsync keeps its own steps.
Summaries: the helper, the scope type, FinishAsync and SetScrobblingAsync each have a one-line <summary>.
No account, configuration, plugin API or Plugin Repository behaviour changes.
Validation
Clone scan:praetorctl dedupe scan --json (jscpd 5.4.2). At 6c056ae it reports 111 groups, one of them in AccountService.cs (:175 and :324, 9 lines). With this change it reports 110 groups and none in the Account sources or tests. The first version of the new concurrency test repeated the tail of an existing test and added a 9-line group in AccountConcurrencyTests.cs; the second commit moves those checks into AssertGrantCancelledAndDisconnectedAsync, and the group is gone.
New tests:DisabledHostUserCannotContinueBackgroundOperations now also checks that a disabled host user is refused by FinishAsync and SetScrobblingAsync. ScrobblingChangeWaitsForAPendingGrantAndEndsWithDisconnect checks that a scrobbling change waits while a pending grant holds the account gate, and that a disconnect cancels it while the gate is still held.
Mutants: three mutants of the helper each turn the account tests red, and the restored code passes all 47 of them:
Dropping the host-access check fails DisabledHostUserCannotContinueBackgroundOperations.
Linking only the caller's token fails the three disconnect tests in AccountConcurrencyTests.
Releasing the gate before returning the scope fails ScrobblingChangeWaitsForAPendingGrantAndEndsWithDisconnect.
Tests from the Codacy review:CancelledScrobblingWaitLeavesTheAccountGateFree cancels eight scrobbling changes in a row while a pending grant holds the account gate, then shows a further change applies, so a failed gate wait gives back both the gate and its waiter slot. DisabledHostUserIsRefusedBeforeTheCallbackIsChecked shows BeginAsync refuses a disabled user with UnauthorizedAccessException before it rejects an invalid return address, and stores no attempt. On the full suite of 769 tests, each source change below fails at least one of them, and the restored code passes all 769:
Checking host access after the return address in BeginAsync fails DisabledHostUserIsRefusedBeforeTheCallbackIsChecked.
Releasing a gate the cancelled waiter never held fails 6 tests, CancelledScrobblingWaitLeavesTheAccountGateFree among them.
Keeping the waiter slot after a cancelled wait fails CancelledScrobblingWaitLeavesTheAccountGateFree; with only one cancelled round it passed all 769 tests, hence eight rounds.
Cancellation and user isolation: the existing account, concurrency and connection-approval tests pass, among them disconnect during a pending grant, cancelled gate waiters, and 300 sequential users without exhausting the lease capacity.
Gate:make verify-all on 5ed3ca6 (cd12211 merged with main) exits 0, with dotnet and npm in the pinned SDK container (tools/sdk.sh): 0 build warnings, 769 .NET tests and 450 Python tests pass. It also prints four pre-existing HISS-18 draft-trigger warnings for ci.yml and codeql.yml; that rule reports and does not enforce, and this change does not touch either file.
Summary by Sourcery
Centralize the account-operation entry scope and strengthen tests for authorization, cancellation, gating, and disabled-user behavior.
Enhancements:
Share account-entry validation, authorization cancellation, and per-user gating between FinishAsync and SetScrobblingAsync while preserving disposal and cancellation behavior.
Document the account operations and shared account scope with concise API summaries.
Tests:
Expand account concurrency coverage to verify cancelled scrobbling waiters release capacity and disconnect cancels changes blocked behind a pending grant.
Verify disabled host users are rejected consistently by account operations before callback validation.
Summary by CodeRabbit
Bug Fixes
Disconnecting an account now consistently cancels pending authorization and scrobbling changes, and a late authorization response can’t undo the disconnect.
Account operations are rejected when the associated host user is disabled, including attempts to begin or finish an account connection and changes to scrobbling settings.
Scrobbling changes wait for pending account operations and are cancelled if the account is disconnected.
Repeatedly cancelled scrobbling changes no longer prevent a later change from succeeding.
…lingAsync
FinishAsync and SetScrobblingAsync started with the same seven
statements: caller and host-access checks, the current authorization,
a token linked to it, and the per-user account gate. They now call one
private EnterAccountAsync helper that returns an AccountScope. Disposing
the scope releases the gate first and then the linked token, the same
order the two using declarations had. If the gate wait throws, the
linked token is disposed as before.
BeginAsync keeps its own steps: it validates the callback between the
host-access check and the authorization lookup, and moving that would
change which error a disabled user with a bad callback sees.
Tests pin the folded behaviour: a disabled host user is refused by
FinishAsync and SetScrobblingAsync, and a scrobbling change waits for
a pending grant's gate and is cancelled when the account disconnects.
Signed-off-by: lusoris <lusoris@proton.me>
The new scrobbling-gate test ended with the same three checks as
DisconnectImmediatelyCancelsPendingGrantAndCannotBeUndoneByItsResponse:
the released grant ends cancelled, the disconnect finishes, and the
account is gone. The clone scan reported the two tails as one 9-line
group. Both tests now call AssertGrantCancelledAndDisconnectedAsync.
Signed-off-by: lusoris <lusoris@proton.me>
Factor shared caller and host checks, authorization-linked cancellation, and account-gate acquisition into an AccountScope used by FinishAsync and SetScrobblingAsync, while preserving disposal order and gate-wait cleanup.
src/JellySin.Plugin.Lastfm/Configuration/AccountService.cs L175-182, L287-322, L356-363 (+1 small edits)
Tests
Cover the shared account-entry behavior with cancellation and disconnect concurrency checks, plus disabled-host rejection and BeginAsync callback-validation ordering.
Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment @sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment @sourcery-ai review to trigger a new review!
No actionable comments were generated in the recent review.
Walkthrough
FinishAsync and SetScrobblingAsync now use a shared account scope for access checks, cancellation, and connection-gate ownership. Tests cover pending-grant cancellation and disabled host-user access.
FinishAsync and SetScrobblingAsync use EnterAccountAsync to validate access and acquire linked cancellation and the connection gate. FinishAsync renews authorization through the scope’s operation.
Concurrency and access checks tests/JellySin.Plugin.Lastfm.Tests/Core/AccountConcurrencyTests.cs, tests/JellySin.Plugin.Lastfm.Tests/Core/AccountTests.cs
Concurrency tests check cancellation of a scrobbling change during a pending grant and verify grant cancellation and account removal after disconnect. The disabled-user test checks that both operations throw UnauthorizedAccessException.
No actionable merge-blocking issue is established; the account-operation refactor appears mergeable after normal checks.
Security architecture risk: Low · up to 5ed3c
The refactor preserves the reviewed caller identity, access checks, authorization revision, and cleanup order. No introduced security weakness was identified in these paths. Remaining coverage gaps limit confidence in a broader security conclusion.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The reviewed HTTP paths retain caller-account targeting rather than accepting a request-selected account identifier. Affected state includes that user's connection attempt, protected session, outbox, and capture binding. No expansion of cross-account authority was identified in the compared paths.
Trust Boundaries and Controls
observed — Browser approval remains bound to an attempt and authorization revision before its protected token can reach the session-exchange call. Finish still checks cancellation and host access after the remote response and before account replacement. Shared entry preserves host-access rejection for both refactored operations.
Resilience and Maintainability Implications
observed — The gate pool retains bounded admission, reserved cleanup capacity, cancelled-waiter reclamation, and idempotent lease release. Added test assertions exercise repeated cancelled scrobbling waits and cancellation during disconnect; existing assertions exercise a response delivered after cancellation and subsequent reconnection. These checks support lifecycle preservation but are not an exhaustive failure or crash-recovery proof.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly describes the main change: sharing account-operation scope between FinishAsync and SetScrobblingAsync.
Description check
The description includes both required sections. It explains the change and its behavior, and records validation methods, results, cancellation coverage, and known warnings.
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 3 files.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
✅ No blocking issues noticed
No blocking code findings were identified. The required scenarios for failed gate acquisition cleanup and preservation of BeginAsync validation ordering are still missing, so these behaviors should be covered before merging.
Test suggestions
A disabled host user is rejected by FinishAsync and SetScrobblingAsync.
SetScrobblingAsync waits for a pending grant and is cancelled by disconnect.
FinishAsync is cancelled and the account is removed when disconnect occurs during a pending grant.
AccountScope releases the gate before its linked cancellation source is disposed.
Failed gate acquisition disposes the linked cancellation source.
BeginAsync preserves its callback-validation ordering for disabled users.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Failed gate acquisition disposes the linked cancellation source.
2. BeginAsync preserves its callback-validation ordering for disabled users.
A scrobbling change whose wait for the account gate is cancelled now
has a test: the gate stays free, and a later change still applies.
BeginAsync now has a test showing that it refuses a disabled host user
before it checks the return address and that it stores no attempt.
Both tests answer the Codacy review of this pull request. Moving the
host check after the callback check fails the second test, and
releasing a gate the waiter never held fails the first.
Signed-off-by: lusoris <lusoris@proton.me>
The cancelled-wait test now cancels eight scrobbling changes in a row
while a pending grant holds the account gate. Eight is the gate pool's
per-user limit for ordinary waiters, so a cancelled wait that kept its
slot would make the last round fail with "Too many concurrent account
changes" instead of a cancellation.
Without this, a pool that never gave the slot back passed all 769
tests; with it, that change fails the test. Both new tests also gain a
one-line summary.
Signed-off-by: lusoris <lusoris@proton.me>
Failed gate acquisition:CancelledScrobblingWaitLeavesTheAccountGateFree holds the account gate with a pending grant, then cancels eight scrobbling changes in a row while each waits for it. After the grant finishes, a further change applies. Eight is the gate pool's per-user limit for ordinary waiters, so a cancelled wait that kept its slot would make the last round fail with "Too many concurrent account changes". The linked token source disposed in the catch leaves nothing visible through the public API, so the test checks what a failed acquisition must give back: the gate and the waiter slot.
BeginAsync check order:DisabledHostUserIsRefusedBeforeTheCallbackIsChecked passes a disabled Jellyfin user together with an invalid return address. The call must throw UnauthorizedAccessException rather than ArgumentException, and it must store no sign-in attempt.
Each test was checked against a source change it should catch, running the full suite of 769 tests each time:
Source change
Result
BeginAsync checks host access after the return address
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
Change
FinishAsyncandSetScrobblingAsyncinAccountServicenow enter an account call through one private helper. Before this change, both started with the same seven statements: the caller check, the host-access check, the current authorization, a token linked to that authorization, and the per-user account gate. The duplicate-code scan reported the two copies as one 9-line clone group.EnterAccountAsyncruns those steps and returns anAccountScopethat carries the operation, the authorization and the linked token. Disposing the scope releases the gate first and then the linked token, the same order the twousingdeclarations had. If the gate wait throws, the linked token is disposed, as before.BeginAsynckeeps its own steps.FinishAsyncandSetScrobblingAsynceach have a one-line<summary>.No account, configuration, plugin API or Plugin Repository behaviour changes.
Validation
praetorctl dedupe scan --json(jscpd 5.4.2). At6c056aeit reports 111 groups, one of them inAccountService.cs(:175 and :324, 9 lines). With this change it reports 110 groups and none in the Account sources or tests. The first version of the new concurrency test repeated the tail of an existing test and added a 9-line group inAccountConcurrencyTests.cs; the second commit moves those checks intoAssertGrantCancelledAndDisconnectedAsync, and the group is gone.DisabledHostUserCannotContinueBackgroundOperationsnow also checks that a disabled host user is refused byFinishAsyncandSetScrobblingAsync.ScrobblingChangeWaitsForAPendingGrantAndEndsWithDisconnectchecks that a scrobbling change waits while a pending grant holds the account gate, and that a disconnect cancels it while the gate is still held.DisabledHostUserCannotContinueBackgroundOperations.AccountConcurrencyTests.ScrobblingChangeWaitsForAPendingGrantAndEndsWithDisconnect.CancelledScrobblingWaitLeavesTheAccountGateFreecancels eight scrobbling changes in a row while a pending grant holds the account gate, then shows a further change applies, so a failed gate wait gives back both the gate and its waiter slot.DisabledHostUserIsRefusedBeforeTheCallbackIsCheckedshowsBeginAsyncrefuses a disabled user withUnauthorizedAccessExceptionbefore it rejects an invalid return address, and stores no attempt. On the full suite of 769 tests, each source change below fails at least one of them, and the restored code passes all 769:BeginAsyncfailsDisabledHostUserIsRefusedBeforeTheCallbackIsChecked.CancelledScrobblingWaitLeavesTheAccountGateFreeamong them.CancelledScrobblingWaitLeavesTheAccountGateFree; with only one cancelled round it passed all 769 tests, hence eight rounds.make verify-allon5ed3ca6(cd12211merged withmain) exits 0, with dotnet and npm in the pinned SDK container (tools/sdk.sh): 0 build warnings, 769 .NET tests and 450 Python tests pass. It also prints four pre-existing HISS-18 draft-trigger warnings forci.ymlandcodeql.yml; that rule reports and does not enforce, and this change does not touch either file.Summary by Sourcery
Centralize the account-operation entry scope and strengthen tests for authorization, cancellation, gating, and disabled-user behavior.
Enhancements:
Tests:
Summary by CodeRabbit