wso2 / wso2/wso2-cli

Harden the session lock's tests: no non-vacuous cross-process exclusion test exists

Open
#49 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

ready-for-agent Type/Improvement
Dominant language
Go
Stars
0
Forks
3
Avg merge
8h 43m
Merged PRs (30d)
60

Description

This was generated by AI during triage.

Problem

The session lock's exclusion guarantee is sound after #39, but the tests that defend it are weaker than they look. Adversarial review of #39 demonstrated each of the following by mutation — every claim below is something that was observed, not inferred.

No non-vacuous cross-process exclusion test exists. TestWithLockSurvivesKilledHolder is the only cross-process lock test, and it passes with tryLock stubbed to a no-op that always succeeds. It proves the recovery direction only: it never establishes that the lock was exclusive while the helper process was alive, because held is written inside fn, which a no-op lock reaches just as happily. Cross-process exclusion currently rests entirely on TestWithLockSerializesWriters, which is in-process. The missing assertion is that the parent's acquire must fail while the helper holds, and it is awkward because lockDeadline is a package constant now at 45s.

TestWithLockSerializesWriters has a racy overlap detector that can mask a real overlap. It does now := atomic.AddInt32(&inside, 1); if now > atomic.LoadInt32(&maxInside) { atomic.StoreInt32(&maxInside, now) } — a non-atomic read-modify-write. A late writer with now == 1 that read maxInside == 0 before an earlier writer stored 2 clobbers it back to 1, hiding the overlap. #39 required this test to pass unchanged, so it is not that commit's defect, but the commit now leans on it as the in-process exclusion proof. It should use the mutex-guarded form that runAbandonedLockRound already uses.

TestWithLockNeverOverlapsWithAbandonedLockFile needs 60 rounds because each round is only ~4% effective, not because the assertion is weak. Measured against the pre-fix implementation: 1,1,2,2,2,3,3,3,3,3,4,6 overlaps out of 60 rounds. That still leaves roughly a 9% chance of missing a reintroduced regression, at a measured ~4.4s of CI wall time. The reason each round is weak is structural rather than statistical — under the old code tookOverStale was one-shot per WithLock call, so each round offered exactly one takeover window, at the instant the burst arrived. The other 31 writers and the remaining ~800ms contributed nothing. Reseeding the abandoned file per writer, or shrinking lockRetryInterval under test, would make each round near-certain and let the round count fall by an order of magnitude.

What to do

  • Give TestWithLockSurvivesKilledHolder an exclusion assertion, so the suite has one cross-process test that a no-op lock cannot pass. This likely needs lockDeadline to be injectable under test without becoming a caller-facing knob.
  • Convert TestWithLockSerializesWriters to the mutex-guarded detector.
  • Strengthen the per-round setup in TestWithLockNeverOverlapsWithAbandonedLockFile and drop the round count accordingly. Verify the stronger round still fails against 69299ab^ before reducing the count.

Smaller items from the same review

  • lock_windows.go folds ERROR_IO_PENDING into contention. It is unreachable, because the handle is not opened FILE_FLAG_OVERLAPPED and LOCKFILE_FAIL_IMMEDIATELY precludes pending, but it is not contention — it means the request was queued and will be granted asynchronously. Treating it as a real error, or dropping the case, is more honest than the current mapping.
  • release on Windows relies on handle close to drop the lock. MSDN documents that as working but notes the latency "depends upon available system resources" and recommends explicit unlocking. The retry loop absorbs it, so this is robustness rather than a bug; an explicit UnlockFileEx before Close would remove the dependence on undocumented timing.
  • A root go mod tidy reintroduces a github.com/wso2/wso2-cli/sdk requirement, because go.work composes the unpublished local module and committed replace directives are prohibited. This predates #39 — go mod tidy -diff on 992a47a produces the identical requirement — and CI only runs tidy -diff inside sdk/ with GOWORK=off. Worth either a note in the root go.mod or a root hygiene check.

Out of scope

Changing the locking mechanism. #39's guarantee was verified: the landed regression test fails 13/13 against 69299ab^, a mutant reintroducing the forbidden unlink on release is caught 9–17 times per run, and one fresh open file description per acquisition was confirmed by direct flock probe. This issue is about the tests, not the lock.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with TestWithLockSurvivesKilledHolder, TestWithLockSerializesWriters, and TestWithLockNeverOverlapsWithAbandonedLockFile to understand the existing lock assertions and helpers such as runAbandonedLockRound. Then inspect lock_windows.go and the root go.mod behavior described in the issue. Done means the cross-process exclusion is non-vacuous, overlap detection is race-free, abandoned-lock coverage is stronger with fewer rounds, and the Windows and module-hygiene concerns are addressed or explicitly documented.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
cli, operating-systems, testing-qa
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.