Skip to content

daemon: Bound concurrent credential discovery - #227

Open
Preovaleo wants to merge 2 commits into
linux-credentials:mainfrom
Preovaleo:perso/generic_transport-split/4
Open

daemon: Bound concurrent credential discovery#227
Preovaleo wants to merge 2 commits into
linux-credentials:mainfrom
Preovaleo:perso/generic_transport-split/4

Conversation

@Preovaleo

@Preovaleo Preovaleo commented Sep 5, 2026

Copy link
Copy Markdown

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.

Part Upstream integration PR (targets main) Focused stacked review on the fork
1 — Shared transport lifecycle #224 Preovaleo/credentialsd#2/1main
2 — Cancellation and request ownership #225 Preovaleo/credentialsd#1/2/1
3 — Silent transport exhaustion #226 Preovaleo/credentialsd#3/3/2
4 — Discovery concurrency bound #227 Independent; review directly upstream

Review 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 DiscoveryRequested signals could start multiple discovery tasks for the same credential request. Each task could initiate device discovery and forward background events independently.

This PR:

  • adds a single discovery slot scoped to each credential request;
  • claims the slot before starting discovery;
  • ignores additional discovery requests while that slot is occupied;
  • keeps the permit inside the spawned background task;
  • releases the slot when the task exits, allowing a later discovery attempt;
  • adds regression coverage for the actual background stream lifetime;
  • updates the changelog.

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 main and 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

Scope Test code Daemon code Moved/reused text (included)
This PR, relative to main ≈57% ≈43% ≈0%
8054b8c — Bound concurrent discovery ≈46% ≈54% ≈0%
35415ed — Hold the permit in the task ≈61% ≈39% ≈0%

Commit sequence and rationale

The commit order follows the dependencies between these changes:

  1. 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 DiscoveryRequested entry 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.

  2. 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 passed
  • cargo clippy -p credentialsd --bin credentialsd -- -Dwarnings — passed
  • cargo fmt --all --check — passed
  • git diff --check — passed

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

Théo Bougé 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.
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