Skip to content

cache_lock_is_exclusive_then_released flaked once and cannot be triaged — its two assertions mean opposite things #263

Description

@MichaelTaylor3d

Task

tests::cache_lock_is_exclusive_then_released failed once and has not reproduced. The next occurrence
must record WHICH assertion failed
— that single detail decides whether this is a real defect or an
artifact, and it is unrecoverable once the run is gone.

What happened

Observed during dig-node PR #258. Failed once, then passed in isolation, in a full 864-test re-run, and
in CI. Neither the implementing lane nor the security gate would call it harmless, and both declined
to root-cause it — recorded here rather than dismissed.

What has been ruled out — measured, not assumed

The PR #258 security gate checked the full base..head diff against every symbol involved:

  • it touches none of acquire_cache_lock, lockfile_path, config_path, DIG_NODE_CACHE, or
    ENV_GUARD;
  • the new 498-line test module touches no env var and no cache lock — only two tempdir() calls;
  • the code is pre-existing on main.

So it was not introduced by that PR and it is not env contention from it. The obvious race was also
checked: the other DIG_NODE_CACHE mutator does hold ENV_GUARD.

The one instruction that makes this actionable

The test has two assertions and they mean opposite things:

  1. "a held lock must block a concurrent try_lock" — if this is the one that failed, it is a real
    mutual-exclusion failure
    and deserves its own audit. An advisory lock that does not exclude is not a
    lock, and the cache it guards is shared mutable state.
  2. the release half — if this is the one that failed, it is a lingering-handle artifact: the lock
    was released but a handle had not yet dropped. Annoying, not dangerous.

Right now nobody knows which. Make the failure record it: distinct assertion messages, or a
--nocapture-visible marker per phase. A flake report that says only "the test failed" cannot be
triaged, which is why this one has already cost two lanes' attention and produced no verdict.

Scope

  1. Give each assertion a message that identifies it unambiguously in CI output.
  2. Leave the test otherwise alone — do not add a retry, a sleep, or #[ignore]. Each of those
    converts an unexplained failure into an invisible one, which is strictly worse than a flake.
  3. If it recurs and it is assertion 1, open a mutual-exclusion audit as a separate ticket.

Why not just delete or ignore it

The failure mode it guards — two processes writing one cache — is exactly the class this ecosystem has
measured before. A test that fails once a month and cannot be triaged is a bad test; a test removed
because it was hard to triage is a missing guard.

Effort

Low. Two assertion messages.

Metadata

Metadata

Assignees

No one assigned

    Labels

    kind:maintenancereal work that changes nothing a user can perceive

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions