Repository navigation
test(sessions): run the session lock test on Windows and add a cross-process one - #1115
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe changes add a test that checks session-lock blocking across processes and remove the Windows skip from the existing cross-store lock test. ChangesSession Lock Tests
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the two main changes: enabling the session-lock test on Windows and adding a cross-process session-lock test. Linked Issues check Issue #1104requires coverage for the WindowsLockFileExsession-lock path. The PR removes the Windows skip fromTestSessionFileLockSerializesAcrossStoresand adds `TestSessionFileLockBlocksAnothe…Out of Scope Changes check The PR changes only session-lock tests. The removed skip and the helper-process test directly support issue #1104. The helper-process setup and synchronization code are required test infrastructure fo…
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The helper may release its lock early because the lock-retaining closure is not kept alive.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds Windows and cross-process coverage for session file locking.
Changes:
- Enables the existing cross-store lock test on Windows.
- Adds a helper-process test for lock release on process exit.
| File | Description |
|---|---|
internal/sessions/rewind_test.go |
Removes the Windows test skip. |
internal/sessions/filelock_process_test.go |
Adds cross-process lock coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
please address copilot comments |
…process one TestSessionFileLockSerializesAcrossStores was skipped on Windows with "flock semantics differ on windows", so the LockFileEx path in filelock_windows.go was never exercised by a test on the platform that uses it. LockFileEx locks are per handle, which is what the test relies on, so the skip was unnecessary: remove it. Also add a helper-process test (same pattern as lockutil's) proving the session lock blocks a different process and is released by the OS when the holder exits without unlocking. Both fail on Windows when the lock is replaced by a no-op. Fixes Twigpine#1104 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The helper discarded the unlock closure returned by lockSession, which is the only reference to the open lock file/handle. GC finalization could close it while the helper waits on stdin, releasing the OS lock early and making the cross-process test pass for the wrong reason. Retain the closure and runtime.KeepAlive it after io.Copy, as the lockutil helper does.
5709d13 to
921342e
Compare
|
@kevincodex1 The Copilot comment is addressed in 921342e: the helper now keeps the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
TestSessionFileLockSerializesAcrossStoreswas skipped on Windows (flock semantics differ on windows), so theLockFileEximplementation ininternal/sessions/filelock_windows.gowas never covered by a test on the only platform that uses it.The skip was not needed:
LockFileExlocks are per handle, and twoStoreinstances open separate handles, so the test's expectation (the second store'sAppendEventblocks until the first releases) holds on Windows. This PR removes the skip.It also adds
TestSessionFileLockBlocksAnotherProcessUntilItExits, a helper-process test in the same style aslockutil_test.go: a child process takes the session lock and exits without unlocking, and the parent checks that itsAppendEventblocks while the child holds the lock and proceeds once the child exits (the OS releases the lock). It runs on all platforms.Test-only change. The other skips the issue lists (
oauth/store_test.go,config/writer_test.go) and migratingsessionsontolockutilare not included.Linked issue
Fixes #1104
Note: #1104 does not currently carry the
issue-approvedlabel. Opening this anyway at the author's request; it can be held until the issue is approved.Verification
LockFileExcall with a no-op makes both fail (storeB AppendEvent completed while storeA held the OS lock, andAppendEvent completed while another process held the session lock); the original code was restored afterwards. So the tests now guard the Windows lock.go build ./...,go vet ./internal/sessions/,gofmt -l internal/sessions,git diff --cached --checkclean;go test ./internal/sessions/passes.flock) should behave the same for both tests, but that is left to CI.-race, fullgo test ./..., smoke andmaketargets were not run locally.Checklist
issue-approvedlabel. (not yet)go build ./...,go vet ./..., andgo test ./...pass locally. (build passes; vet and tests only forinternal/sessions)gofmtclean.-racewhere relevant). (tests added;-racenot run locally)🤖 Generated with Claude Code
Summary by CodeRabbit