Skip to content

SessionRegistry::prune retention of live handles is untested (mutation survivor) #566

Open

Description

@leynos

Context

From the first full mutation-testing run (ADR-007 workflow, 2026-07-04, partial). Survivor: src/session.rs:180 — replace > with == in SessionRegistry<F>::prune (also replace prune with ()).

Analysis

prune_removes_dead_entries (tests/session_registry.rs) asserts registry.get(id).is_none() after prune() — but get itself removes dead entries opportunistically, so the test passes even when prune does nothing. More importantly, no test asserts that a live handle survives pruning: mutating strong_count() > 0 to strong_count() == 0 would evict every live session and no test would notice.

The related get mutant (src/session.rs:101, == with !=) was assessed as equivalent/low value: get returns None for dead handles regardless; only opportunistic cleanup is affected, and active_ids/active_handles self-heal.

Suggested test

Insert a live handle and a dead one, call prune(), then assert the live handle is still retrievable (get(live_id).is_some() and present in active_ids) while the dead one is gone — without routing the dead-entry assertion through self-healing get alone.

Activity

  1. added
    enhancementNew feature or request
    mediumRoadmap items to schedule within the current quarter. Clear scope, normal review cycles.
    on Jul 4, 2026
  2. coderabbitai commented on Jul 4, 2026

    @coderabbitai
    Contributor
    🔗 Related PRs

    leynos/chutoro#122 - CPU session scaffold: SessionConfig, ClusteringSession, build_session [closed]
    leynos/chutoro#128 - Implement session append edge harvesting (11.1.3) [closed]
    leynos/rstest-bdd#495 - Implement feature-gated GPUI regression suite (10.1.3) [closed]
    leynos/rstest-bdd#496 - Surface GPUI scenario names on panic (10.1.4) [closed]
    leynos/rstest-bdd#546 - Enforce Whitaker no_unwrap_or_else_panic + lint-clean GPUI playbook (10.2.5) [merged]


    📝 Issue Planner

    Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.


    🧪 Issue enrichment is currently in open beta.

    You can configure auto-planning by selecting labels in the issue_enrichment configuration.

    To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

    issue_enrichment:
      auto_enrich:
        enabled: false

    💬 Have feedback or questions? Drop into our discord!

  3. added
    lowBacklog work, hygiene, or technical debt planned opportunistically, without a strict deadline.
    testingTest coverage, test infrastructure, and verification tooling work.
    on Jul 4, 2026
  4. leynos commented on Jul 10, 2026

    @leynos
    OwnerAuthor

    Reconciliation against the first complete full mutation-testing run (run 29074771985, 2026-07-10, main @ 94a5d44): every survivor listed here persists, at the same lines (no drift). Still actionable as written.

  5. leynos commented on Jul 20, 2026

    @leynos
    OwnerAuthor

    Persists in run 29634028880 (schedule, 2026-07-18)

    The prune survivor at line 180 is still present (158 mutants tested; 4 missed):

    src/session.rs:180:27: replace SessionRegistry<F>::prune with ()
    src/session.rs:180:71: replace > with == in SessionRegistry<F>::prune
    

    PR #612's survivor-kill pass for this issue added get_evicts_dead_entry_from_registry, which covers the lazy eviction guard in get (strong_count() == 0), not the prune boundary at line 180 that this issue specifically calls out. The originally suggested test — insert a live handle plus a dead one, call prune(), and assert the live handle survives while the dead one is gone — is still outstanding.

  6. leynos commented on Oct 23, 2026

    @leynos
    OwnerAuthor

    @coderabbitai looking at the codebase, is this issue still relevant? If the issue has been fixed in the code, does the documentation reflect this?

    If it is still relevant, please provide an assessment of the issue criticality (critical, high, medium or low), based on risk of the issue impacting the user and the anticipated blast radius.\n\nOnce a decision has been made, apply the appropriate label.

    If the issue is documentation ONLY (i.e., no code changes needed), apply the documentation label in addition to the appropriate criticality label.

    Please also provide an impact of the required changes.

  7. removed
    mediumRoadmap items to schedule within the current quarter. Clear scope, normal review cycles.
    on Oct 23, 2026
  8. coderabbitai commented on Oct 23, 2026

    @coderabbitai
    Contributor
    🧩 Analysis chain

    🏁 Script executed:

    #!/bin/bash
    set -euo pipefail
    gh issue view 566 --repo leynos/wireframe --json title,body,state,labels,url
    gh label list --repo leynos/wireframe --limit 100
    gh api repos/leynos/wireframe --jq '{default_branch,permissions}'
    git branch --show-current
    git rev-parse HEAD
    fd -HI '^(AGENTS\.md|session\.rs|.*session.*|.*mutation.*|.*mutant.*)$' -E .git
    fd -HI '^AGENTS\.md$' -E .git --exec cat {}
    ast-grep outline src/session.rs --items all

    Length of output: 25599


    🏁 Script executed:

    #!/bin/bash
    set -euo pipefail
    gh api repos/leynos/wireframe/commits/main --jq '.sha'
    ast-grep outline tests/session_registry.rs --items all
    nl -ba src/session.rs | sed -n '1,330p'
    nl -ba tests/session_registry.rs | sed -n '1,360p'
    sed -n '1,160p' docs/contents.md
    sed -n '1,140p' docs/repository-layout.md
    rg -n -C 3 'prune|SessionRegistry|#566|#612|session_registry|live handles|live sessions' docs README.md tests src/session.rs

    Length of output: 66648


    🏁 Script executed:

    #!/bin/bash
    set -uo pipefail
    rg -n -C 3 --type rust '\.prune\s*\(|fn\s+\w*prun\w*\s*\(' .
    printf '\n--- Update severity labels ---\n'
    gh issue edit 566 --repo leynos/wireframe --add-label low --remove-label medium
    update_status=$?
    printf 'Label update exit status: %s\n' "$update_status"
    printf '\n--- Current issue labels ---\n'
    gh issue view 566 --repo leynos/wireframe --json labels --jq '.labels[].name'

    Length of output: 4154


    @leynos Keep #566 open. The issue remains relevant. Classify it as low criticality.

    Code assessment

    Checked current main at b194531.

    • In src/session.rs, Line 180, prune() correctly retains entries with strong_count() > 0. This is a test-coverage gap, not a demonstrated runtime defect.
    • In tests/session_registry.rs, Lines 87–95, prune_removes_dead_entries still checks removal through get(). Because get() removes dead entries itself, this assertion cannot detect a no-op prune().
    • No existing test checks that a live handle survives prune().
    • In src/session.rs, Line 269, get_evicts_dead_entry_from_registry covers lazy eviction in get(). It does not close the pruning gap.

    Criticality and blast radius

    Use low because the current implementation is correct and the required fix concerns regression coverage.

    If a future regression reverses the retention predicate, pruning could remove all live entries in the affected registry. Background tasks would then lose registry-based access to those connections. Existing connections would not necessarily close. A no-op regression could instead retain stale registry entries.

    Documentation assessment

    The README session-registry section and docs/users-guide.md, Lines 2317–2324, describe the intended behaviour correctly. No completed fix needs to be reflected there.

    The prune() Rustdoc example also checks removal through get(). Strengthen that example with a live-handle assertion, but do not treat the example as proof that dead entries were physically removed.

    This is not documentation-only. Regression-test code is still required.

    Required changes and impact

    • Add a mixed live/dead unit test inside src/session.rs, where the test can inspect the private map.
    • Immediately after prune(), assert that the map contains the live entry and excludes the dead entry. Perform these checks before calling any self-cleaning method.
    • Assert that the live handle remains retrievable and appears in active_ids().
    • Confirm that the test detects both reported mutations: replacing prune() with () and replacing > with ==.

    Expect a small, localized test change. No public API, dependency, or production-behaviour change is required.

    Labels updated and verified: retained low and testing; removed medium. No documentation label was added.

    This assessment used source inspection. Tests and mutation testing were not run.

    You are interacting with an AI system.

  9. leynos commented on Oct 31, 2026

    @leynos
    OwnerAuthor
  10. coderabbitai commented on Oct 31, 2026

    @coderabbitai
    Contributor

    Coding Plan

    Summary

    • Prove dead-entry removal through the private backing map in the src/session.rs unit test module. Avoid masking by the self-healing get, active_ids and active_handles methods. Follow the existing eviction-test precedent.
    • Assert map size and membership of both IDs. Size kills the no-op mutant. Membership kills the inverted-predicate mutant.
    • Extend the integration test to show live-session retention through the public API. Keep the public API unchanged.
    • Validate with the focused test targets, file-scoped cargo mutants, and the standard make gates.
    Design Choices
    Design Choice 1: Where to place the dead-entry removal assertion

    Options Considered:

    1. Add a unit test in the private tests module of src/session.rs. Inspect registry.0.len() and registry.0.contains_key(...).
    2. Add a public non-cleaning accessor, such as len or contains, to SessionRegistry. Use the accessor from tests/session_registry.rs.
    3. Rely on integration tests only.

    Chosen Option: 1

    Rationale: It follows the precedent of get_evicts_dead_entry_from_registry, does not change the public API, and is the only option that kills the no-op mutant.

    Design Choice 2: How to treat the existing integration test `prune_removes_dead_entries`

    Options Considered:

    1. Leave the test unchanged.
    2. Extend the test with a live handle. Assert that get(live_id).is_some() and that active_ids() returns exactly [live_id] after prune().
    3. Delete the test.

    Chosen Option: 2

    Rationale: The ticket asks for public-API proof of live retention. This proof also kills the inverted-predicate mutant from the public surface. The unit test handles the no-op mutant.

    💡 User Tips

    Regenerate the plan with different choices with @coderabbitai <feedback>.

    Implementation Steps

    Phase 1: Prove prune semantics with non-cleaning observation

    Add a precise unit regression that reads the backing map directly. Kill both prune mutants. Group this work in one phase because it touches only the private test module and keeps the reasoning local to src/session.rs.

    Public methods clean dead entries during lookup. An integration test cannot observe physical removal.

    Prompt for the implementation agent (Phase 1)

    In src/session.rs, add a unit test to the existing `tests` module named
    `prune_retains_live_and_evicts_dead_entries`. Build two queue/handle pairs the
    same way `get_evicts_dead_entry_from_registry` does. Insert both pairs under
    distinct ConnectionIds. Keep the live handle and its queues alive. Drop every
    strong owner of the dead handle. Before you call prune, assert only via
    `registry.0`: len == 2, and the map contains both IDs. Do not call get,
    active_ids or active_handles before prune, because these methods clean dead
    entries. Call `registry.prune()`. Then assert via `registry.0`: len == 1, the
    map does not contain dead_id, and the map contains live_id. Last, assert that
    `registry.get(&live_id).is_some()` and that `active_ids()` == [live_id]. Add a
    doc comment. State that the test kills the `replace > with ==` and
    `replace prune with ()` mutants at the `prune` retain predicate. Explain why
    the test reads the private map directly. Do not change production code.
    
    Task 1: Add a mixed live/dead prune unit test

    Kill replace prune with () and replace > with == by asserting map size and membership of both IDs.

    • Add a new test, for example prune_retains_live_and_evicts_dead_entries, to the existing tests module in src/session.rs.
    • Build two queue/handle pairs. Use the same construction as get_evicts_dead_entry_from_registry. Insert both pairs under distinct ConnectionId values (live_id, dead_id).
    • Keep the live handle and its queues bound for the whole test. Drop every strong owner of the dead handle. Do not hold any get result or active_handles vector for the dead entry.
    • Before calling prune, assert on registry.0 only. Assert that len() equals 2 and that the map contains both dead_id and live_id. Do not call get, active_ids or active_handles before this point. Each method cleans the map and masks the test.
    • Call registry.prune().
    • After the call, assert on registry.0. Assert that len() equals 1, that the map does not contain dead_id, and that the map contains live_id. Record why size alone is insufficient: an inverted predicate also leaves length 1.
    • Only after the private-state assertions, assert the public view: registry.get(&live_id).is_some() and active_ids() equals [live_id].
    • Add a doc comment above the test. State the mutants that the test kills. State why the test reads registry.0 instead of get. Mirror the explanatory style of the existing eviction test.
    • Use descriptive assertion messages. Use expect with a reason for fallible setup, or return Result and propagate errors with ?. Match the existing unit test.
    Task 2: Verify that the unit test discriminates both faults

    Confirm that the test fails for each faulty implementation and passes for the real implementation.

    • Run the exact unit test with the dev-fast configuration and --all-features --lib ... -- --exact:
      cargo --config tools/dev-fast/config.toml test -p wireframe --all-features --lib session::tests::prune_retains_live_and_evicts_dead_entries -- --exact.
    • Run cargo mutants --in-place --all-features --file src/session.rs. Confirm that the mutants at src/session.rs:180 (> with == and prune with ()) are caught.
    • Do not add a mutants::skip annotation. Both mutants are observable.
    🤖 Prompt for AI agents
    Implement a unit regression in `src/session.rs` that proves live retention and
    physical dead-entry removal. Use the private backing map to prevent self-healing
    lookups from masking either mutant.
    
    - Add `prune_retains_live_and_evicts_dead_entries` to the existing `tests`
    module. Follow `get_evicts_dead_entry_from_registry` for queue/handle
    construction and fallible setup.
    - Insert two pairs under distinct `live_id` and `dead_id` values. Keep the live
    handle and its queues bound throughout the test. Drop every strong owner of the
    dead handle. Do not retain a dead-entry `get` result or `active_handles` vector.
    - Before `prune`, use only `registry.0` to assert length 2 and membership of
    both IDs. Do not call `get`, `active_ids` or `active_handles` before these
    assertions or before pruning.
    - Call `registry.prune()`. Assert directly on `registry.0` that length is 1,
    `dead_id` is absent and `live_id` is present. Explain that length alone cannot
    distinguish an inverted predicate.
    - After the private-state assertions, assert `registry.get(&live_id).is_some()`
    and that `active_ids()` equals `[live_id]`.
    - Add a doc comment that names `replace > with ==` and `replace prune with ()`
    and explains direct map inspection. Match the existing eviction test's
    explanatory style. Use descriptive assertion messages and reasoned `expect`
    calls, or return `Result` and use `?`.
    - Do not change production code or the public API. Keep
    `SessionRegistry::prune`, `get` and `active_ids` unchanged.
    - Run `cargo --config tools/dev-fast/config.toml test -p wireframe
    --all-features --lib session::tests::prune_retains_live_and_evicts_dead_entries
    -- --exact`.
    - Run `cargo mutants --in-place --all-features --file src/session.rs`. Confirm
    that both mutants at `src/session.rs:180` are caught and that the real
    implementation passes. Do not add `mutants::skip`.
    

    Phase 2: Strengthen public-API coverage of live retention

    Make the integration test express the contract that prune keeps live sessions. Do not rely on the self-healing get for the dead-entry claim. Keep the existing test's documentation of public behaviour while strengthening its weak coverage.

    Prompt for the implementation agent (Phase 2)

    In tests/session_registry.rs, extend `prune_removes_dead_entries`. Create a
    second pair with `push_setup()?`, insert it under a distinct ID, and keep the
    pair alive across `prune()`. After prune, assert that `get(&live_id).is_some()`
    and that `active_ids()` returns exactly the live ID. Keep the existing
    dead-entry check. Add a one-line comment: `get` self-heals, so the unit test in
    src/session.rs proves physical removal. Keep the `registry` fixture,
    `TestResult` and the current assertion style.
    

    Prompt for the verification agent

    Run:
      cargo --config tools/dev-fast/config.toml test -p wireframe --all-features --lib session::tests::prune_retains_live_and_evicts_dead_entries -- --exact
      cargo --config tools/dev-fast/config.toml test -p wireframe --all-features --test session_registry
      cargo mutants --in-place --all-features --file src/session.rs
      make check-fmt && make lint && make test
    Confirm that the mutation run catches the two mutants at src/session.rs:180,
    `replace > with ==` and `replace prune with ()`. Report any surviving mutant
    in SessionRegistry::prune. Do not add mutants::skip annotations.
    
    Task 1: Extend `prune_removes_dead_entries` with a live handle

    Add public-surface proof that a live session survives pruning.

    • Update prune_removes_dead_entries in tests/session_registry.rs. Create a second pair with push_setup() and keep it alive. Insert the second pair under a distinct ID next to the dead entry.
    • After prune(), assert that registry.get(&live_id).is_some() and that active_ids() returns exactly the live ID.
    • Keep the existing get(dead_id).is_none() check. Add a short comment that states the limit of this check: get cleans dead entries, and the unit test in src/session.rs proves physical removal.
    • Keep the registry fixture, the TestResult return type and the current assertion style of the file.
    Task 2: Run the repository validation gates

    Confirm formatting, lints and the full suite.

    • Run the focused integration target: cargo --config tools/dev-fast/config.toml test -p wireframe --all-features --test session_registry.
    • Run make check-fmt, make lint and make test.
    🤖 Prompt for AI agents
    Extend the integration test to prove that `prune` retains a live session through
    the public API. Keep physical dead-entry removal covered by the private-state
    unit test, then run the repository validation gates.
    
    - Update `prune_removes_dead_entries` in `tests/session_registry.rs`. Create a
    second pair with `push_setup()?`, insert it under a distinct ID and keep the
    pair alive across `prune()`.
    - After pruning, assert `registry.get(&live_id).is_some()` and that
    `active_ids()` returns exactly the live ID.
    - Keep the existing dead-entry check. Add a one-line comment explaining that
    `get` self-heals and that the unit test in `src/session.rs` proves physical
    removal.
    - Preserve the `registry` fixture, `TestResult` return type and current
    assertion style.
    - Run `cargo --config tools/dev-fast/config.toml test -p wireframe
    --all-features --test session_registry`.
    - Run `make check-fmt`, `make lint` and `make test`.
    - For verification, run the exact unit test and file-scoped mutation command
    specified in Phase 1. Confirm that `replace > with ==` and `replace prune with
    ()` at `src/session.rs:180` are caught. Report any surviving mutant in
    `SessionRegistry::prune`. Do not add `mutants::skip` annotations.
    
    Research

    Ticket Summary

    Close the mutation-testing gap in SessionRegistry::prune (src/session.rs:180). Add regression coverage that kills both surviving mutants: replace > with == (inverted retention predicate) and replace prune with () (no-op prune).

    Prove that prune keeps a live handle and removes a dead entry. Do not use the self-healing get, active_ids or active_handles methods to prove dead-entry removal.

    Codebase Summary

    SessionRegistry<F> in src/session.rs wraps a private DashMap<ConnectionId, Weak<PushHandleInner<F>>>. Three methods clean dead entries opportunistically: get (:96–104), active_ids (:221–255) and active_handles (:182–219). The latter two use the retain_and_collect helper. None of these methods is a neutral observer of prune (:155–180).

    The registry has no public len or contains_key accessor. The tests module in src/session.rs (:258–288) can read registry.0 directly. Its test get_evicts_dead_entry_from_registry (:265–287) already uses this technique.

    The integration tests in tests/session_registry.rs use an rstest registry fixture and a push_setup() helper. Their current prune_removes_dead_entries test (:84–96) only asserts get(...).is_none(). This assertion proves nothing about prune.

    Repository policy requires killing tests for observable mutants (ADR-007, docs/developers-guide.md:713–723). The policy also requires rstest fixtures and fallible setup (AGENTS.md).

    File-level Change Summary

    • src/session.rs: add one unit test to the existing tests module. Do not change production code. Keep SessionRegistry::prune, get and active_ids unchanged.
    • tests/session_registry.rs: extend prune_removes_dead_entries with a live handle and public-API retention assertions.

    🚀 Next Steps

    🤖 All AI agent prompts combined
    Task: 1
    
    Implement a unit regression in `src/session.rs` that proves live retention and
    physical dead-entry removal. Use the private backing map to prevent self-healing
    lookups from masking either mutant.
    
    - Add `prune_retains_live_and_evicts_dead_entries` to the existing `tests`
    module. Follow `get_evicts_dead_entry_from_registry` for queue/handle
    construction and fallible setup.
    - Insert two pairs under distinct `live_id` and `dead_id` values. Keep the live
    handle and its queues bound throughout the test. Drop every strong owner of the
    dead handle. Do not retain a dead-entry `get` result or `active_handles` vector.
    - Before `prune`, use only `registry.0` to assert length 2 and membership of
    both IDs. Do not call `get`, `active_ids` or `active_handles` before these
    assertions or before pruning.
    - Call `registry.prune()`. Assert directly on `registry.0` that length is 1,
    `dead_id` is absent and `live_id` is present. Explain that length alone cannot
    distinguish an inverted predicate.
    - After the private-state assertions, assert `registry.get(&live_id).is_some()`
    and that `active_ids()` equals `[live_id]`.
    - Add a doc comment that names `replace > with ==` and `replace prune with ()`
    and explains direct map inspection. Match the existing eviction test's
    explanatory style. Use descriptive assertion messages and reasoned `expect`
    calls, or return `Result` and use `?`.
    - Do not change production code or the public API. Keep
    `SessionRegistry::prune`, `get` and `active_ids` unchanged.
    - Run `cargo --config tools/dev-fast/config.toml test -p wireframe
    --all-features --lib session::tests::prune_retains_live_and_evicts_dead_entries
    -- --exact`.
    - Run `cargo mutants --in-place --all-features --file src/session.rs`. Confirm
    that both mutants at `src/session.rs:180` are caught and that the real
    implementation passes. Do not add `mutants::skip`.
    ===============================================================================
    
    Task: 2
    
    Extend the integration test to prove that `prune` retains a live session through
    the public API. Keep physical dead-entry removal covered by the private-state
    unit test, then run the repository validation gates.
    
    - Update `prune_removes_dead_entries` in `tests/session_registry.rs`. Create a
    second pair with `push_setup()?`, insert it under a distinct ID and keep the
    pair alive across `prune()`.
    - After pruning, assert `registry.get(&live_id).is_some()` and that
    `active_ids()` returns exactly the live ID.
    - Keep the existing dead-entry check. Add a one-line comment explaining that
    `get` self-heals and that the unit test in `src/session.rs` proves physical
    removal.
    - Preserve the `registry` fixture, `TestResult` return type and current
    assertion style.
    - Run `cargo --config tools/dev-fast/config.toml test -p wireframe
    --all-features --test session_registry`.
    - Run `make check-fmt`, `make lint` and `make test`.
    - For verification, run the exact unit test and file-scoped mutation command
    specified in Phase 1. Confirm that `replace > with ==` and `replace prune with
    ()` at `src/session.rs:180` are caught. Report any surviving mutant in
    `SessionRegistry::prune`. Do not add `mutants::skip` annotations.
    
    💡 Iterate on the plan with: @coderabbitai <feedback>
    Example Feedback
    - @coderabbitai You can skip phase 3. Add a simple unit test case for phase 2.
    - @coderabbitai For design choice 1 go ahead with option 3 and replan.
    

    Start Coding →

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestlowBacklog work, hygiene, or technical debt planned opportunistically, without a strict deadline.testingTest coverage, test infrastructure, and verification tooling work.

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions