test(sessions): run the session lock test on Windows and add a cross-process one - #1115
PierrunoYT wants to merge 1 commit into
Conversation
…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>
|
Warning Review limit reachedOnly developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing. Next included review available in 30 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: Twigpine/zero/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Comment |
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.
| return | ||
| } | ||
| store := NewStore(StoreOptions{RootDir: os.Getenv("ZERO_SESSIONS_LOCK_ROOT")}) | ||
| if _, err := store.lockSession("s"); err != nil { |
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