Skip to content

Add deterministic coverage for concurrent index_router cache insertion #847

Open

Description

@coderabbitai

Summary

Codecov coverage of the if (not inserted) return path in index_router::multilayer_slots_for is nondeterministic. This is a valid concurrent-cache path, not evidence of a correctness bug, but it produces small, unrelated coverage changes in pull requests.

Add a focused regression test for concurrent publication and reuse of the cached message-slot and end-token-entry results. Stabilizing coverage is a secondary benefit.

Required change

Deterministically make at least two callers observe a cache miss for the same initially absent layer_hash before either attempts insertion. Then allow insertion to proceed and verify that the losing inserter receives the fully initialized cached results.

A barrier before calling multilayer_slots_for is insufficient: one caller can complete insertion before another performs its initial lookup, causing the latter to take the ordinary cache-hit path instead. More threads, sleeps, or repeated attempts increase the probability of contention but do not guarantee the target branch.

Synchronization must therefore occur after the failed shared-accessor lookup and before the exclusive-accessor insertion. A minimal internal test seam may be needed because multilayer_slots_for is private. Exercise the actual router implementation, not a copied implementation of its cache logic, and do not make the method public solely for testing.

Preserve the shared-read fast path and existing runtime locking behavior. Do not synchronize participating callers while holding the exclusive cache accessor, as this can deadlock. If deterministic testing would require substantial production scaffolding, reconsider the cost before proceeding.

Do not change the cursor or GSL work in PR #839 for this task.

Rationale

The branch handles another caller inserting the entry between the initial cache lookup and insertion. The exclusive accessor protects initialization, so the losing inserter should receive fully initialized message-slot and end-token-entry vectors.

A deterministic test increases confidence in this publication behavior while reducing Codecov noise. This is low-priority test hardening, not a correctness fix; runtime locking should not be changed solely to stabilize coverage.

Affected area

  • phlex/core/index_router.cpp: index_router::multilayer_slots_for, specifically the if (not inserted) return path (originally line 366; line numbers may change).
  • phlex/core/index_router.hpp: a minimal internal testability change, if needed.
  • Tests for concurrent index-router cache behavior.

Acceptance criteria

  • At least two callers are guaranteed to observe a miss for the same initially absent layer hash before either attempts insertion.
  • The test guarantees that one or more callers take the not inserted branch; it does not rely on scheduling luck, sleeps, or retries.
  • The fixture produces meaningful, nonempty message-slot and end-token-entry results.
  • Concurrent callers receive the expected contents and the same cached shared_ptrs for both result vectors.
  • A subsequent ordinary cache-hit call returns the same cached results.
  • The test exercises the actual router implementation and preserves the shared-read fast path.
  • The test is stable across repeated runs, including constrained execution environments.
  • Coverage consistently includes the target return path. Repeated coverage runs confirm determinism rather than provide it.

References

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

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions