Skip to content

fix: use slash-separated lock paths on Windows - #344

Merged
hacdias merged 1 commit into
hacdias:mainfrom
snowyukitty:fix/windows-lock-paths
Jul 25, 2026
Merged

hacdias merged 1 commit into
hacdias:mainfrom
snowyukitty:fix/windows-lock-paths

Conversation

@snowyukitty

Copy link
Copy Markdown
Contributor

Summary

  • build shared lock names with slash separators on every platform
  • add regression coverage for root locks and nested user directories
  • run the test workflow on Windows and make the existing path/filename assertions portable

Problem

lockSystem adapts filesystem-backed users into one shared webdav.LockSystem namespace. It currently uses filepath.Join for that namespace.

The golang.org/x/net/webdav.LockSystem contract requires resource names to use / separators regardless of the host OS. On Windows, an infinite-depth lock is stored under a name such as /D:\data, while a descendant is resolved as /D:\data\file.txt. The in-memory lock system therefore does not recognize the latter as a descendant of the former.

This is observable in TestServerPartialUpdateHonorsLocks/root: a tokenless partial PATCH to a descendant returned 204 No Content instead of 423 Locked.

Fix

Normalize the filesystem directory with filepath.ToSlash, then join it to the WebDAV resource name with path.Join. This preserves the backing-directory namespace while satisfying the platform-independent LockSystem path contract.

The new tests cover both direct root-lock inheritance and the shared namespace used when one user's directory is nested under another user's directory.

Testing

Run on Windows:

  • go test ./...
  • go build ./...
  • go vet ./...

The test workflow now covers both ubuntu-latest (with the race detector) and windows-latest.

Coordination

#335 also extends locksystem.go for multi-directory support. This PR only fixes the current single-directory path adapter. If #335 lands first, I can rebase this focused change and carry the same slash-separated invariant into its added resolver.

@snowyukitty
snowyukitty force-pushed the fix/windows-lock-paths branch from 90ab625 to c6db9cf Compare July 13, 2026 00:23
@snowyukitty

Copy link
Copy Markdown
Contributor Author

Rebased onto current main and re-implemented after the multi-directory lock-system refactor (#335). The old lockSystem.directory field is gone, so the slash-normalization now lives in both resolve closures (newLockSystem and newMultiDirLockSystem) via filepath.ToSlash. DirectoryMount.filePath is intentionally left untouched, since it still returns OS-native paths for real filesystem access — only the lock namespace is normalized.

I also added windows-latest to the test matrix (running without the cgo-dependent race detector). The bug reproduces there through the existing TestServerPartialUpdateHonorsLocks, which returned 204 instead of 423 Locked before the fix. Three unrelated tests were made platform-aware so the Windows job is green: absolute-path config expectations, and an * filename that is invalid on Windows.

Validated locally on Windows — full suite passes, and gofmt, go vet, and golangci-lint are clean.

@hacdias

hacdias commented Jul 19, 2026

Copy link
Copy Markdown
Owner

Thanks for this PR. It seems the tests are failing though

Normalize lock namespace keys in both single- and multi-directory resolvers so descendant locks remain effective on Windows. Add platform-aware coverage and a Windows CI job, quoting the coverage output flag so PowerShell passes it as one argument.
@snowyukitty
snowyukitty force-pushed the fix/windows-lock-paths branch from c6db9cf to 7002b99 Compare July 19, 2026 09:02
@snowyukitty

Copy link
Copy Markdown
Contributor Author

Thanks for flagging this. The Windows job was failing before it reached the tests: PowerShell split the unquoted -coverprofile=coverage.txt argument and treated .txt as a package.

I rebased onto the current main, kept the updated setup-go@v7, and quoted that argument in the Windows step. The exact coverage command now passes locally with Go 1.26.5 on Windows, along with the full test suite, go vet, and golangci-lint. The updated workflows are awaiting approval to run on commit 7002b99.

@hacdias
hacdias merged commit 390fe21 into hacdias:main Jul 25, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants