Harden the session lock's tests: no non-vacuous cross-process exclusion test exists
Nobody has claimed this yet.
- 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
TestWithLockSurvivesKilledHolderan exclusion assertion, so the suite has one cross-process test that a no-op lock cannot pass. This likely needslockDeadlineto be injectable under test without becoming a caller-facing knob. - Convert
TestWithLockSerializesWritersto the mutex-guarded detector. - Strengthen the per-round setup in
TestWithLockNeverOverlapsWithAbandonedLockFileand drop the round count accordingly. Verify the stronger round still fails against69299ab^before reducing the count.
Smaller items from the same review
lock_windows.gofoldsERROR_IO_PENDINGinto contention. It is unreachable, because the handle is not openedFILE_FLAG_OVERLAPPEDandLOCKFILE_FAIL_IMMEDIATELYprecludes 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.releaseon 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 explicitUnlockFileExbeforeClosewould remove the dependence on undocumented timing.- A root
go mod tidyreintroduces agithub.com/wso2/wso2-cli/sdkrequirement, becausego.workcomposes the unpublished local module and committedreplacedirectives are prohibited. This predates #39 —go mod tidy -diffon992a47aproduces the identical requirement — and CI only runstidy -diffinsidesdk/withGOWORK=off. Worth either a note in the rootgo.modor 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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