daemon: Bound concurrent credential discovery - #227
Open
Preovaleo wants to merge 2 commits into
Open
Conversation
added 2 commits
September 5, 2026 15:53
Repeated DiscoveryRequested signals could spawn independent streams for the same request, consuming resources and racing request completion. Hold one request-scoped permit for the lifetime of the forwarded stream. Concurrent signals are ignored, while releasing the permit allows a later discovery attempt.
This was referenced Sep 5, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Split and stacked review
Original PR: linux-credentials/credentialsd#220.
The replacement is split into four parts. Parts 1 → 2 → 3 form native GitHub stack #4 on the fork; part 4 is independent.
main)/1→main/2→/1/3→/2Review and integrate parts 1 → 2 → 3 in order. Part 4 can be reviewed and merged independently.
GitHub does not support native stacks across forks. Use Files changed on the fork PRs for incremental review of each layer. The upstream PRs target
main; #225 and #226 include earlier layers until their dependencies land and the branches are rebased. Integration into the main project happens through the upstream PRs; merging the fork stack only updates the fork.Hello 👋,
Summary
This PR extracts the trusted UI discovery concurrency bound from #220 and fixes the lifetime of its discovery permit.
Repeated
DiscoveryRequestedsignals could start multiple discovery tasks for the same credential request. Each task could initiate device discovery and forward background events independently.This PR:
Result
Each credential request can have one active discovery stream at a time. The slot remains occupied while the background stream is pending or forwarding events. When the stream ends, or forwarding stops because sending an update fails, the task releases its permit.
A later discovery attempt can claim the slot again while the request remains active.
The permit is explicitly moved into the spawned task. Passing an unused permit to the spawning function would release it when that function returns, making the concurrency bound ineffective.
Scope and dependencies
This is part 4 of the split. It is based directly on
mainand is independent of the stack starting at #224 and #225; it can be reviewed and merged separately.The bound applies after the existing D-Bus sender and session validation, protecting the daemon from repeated discovery signals from a faulty trusted UI.
Authenticator enumeration, dynamic source-list updates, and retry policy remain separate concerns tracked by #32 and #206.
Diff composition
main8054b8c— Bound concurrent discovery35415ed— Hold the permit in the taskCommit sequence and rationale
The commit order follows the dependencies between these changes:
daemon: Bound concurrent credential discovery
The lifecycle fixes make completion safer, but repeated UI signals can still launch redundant discovery tasks. The bound belongs at the
DiscoveryRequestedentry point, before starting that work. A request-scoped semaphore expresses one active discovery attempt, and immediate acquisition lets an additional signal be ignored while that attempt is running. Releasing the slot allows a subsequent attempt, so the bound does not impose a one-discovery-per-request policy. This is independent of the transport stream changes in parts 1–3.daemon: Hold discovery permit for the background task lifetime
The first commit passes the permit to the forwarding function but leaves it unused by the spawned future. It is consequently dropped when the function returns, before discovery ends. This commit fixes that ownership by moving the permit into the background task, making the bound last across the forwarding loop and its waits.
The original test covered acquiring and releasing a semaphore permit, so it could pass despite that integration error. Supplying the event sender as a callback lets the regression test run the actual forwarding function without a D-Bus connection. Holding its stream pending and then closing it demonstrates the lifetime the bound depends on. The changelog accompanies the completed behavior.
Testing
Validation performed at the branch tip during the split:
cargo test -p credentialsd --bin credentialsd— 27 tests passedcargo clippy -p credentialsd --bin credentialsd -- -Dwarnings— passedcargo fmt --all --check— passedgit diff --check— passedThe added tests cover concurrent slot acquisition, retention while the background stream is pending, and release after stream completion. The background-task regression test runs without a live D-Bus service or physical authenticator.