Normalize local-filesystem path composition and enable extended-length test paths by TedHartMS · Pull Request #2147 · microsoft/garnet

Ted Hart (TedHartMS) · 2026-09-17T03:11:32Z

Problem

Garnet composed several local-filesystem paths by concatenating a forward slash onto a base directory. Windows normalizes / to \ in ordinary paths, but not inside a Win32 extended-length (\\?\) path, where CreateFileW then fails with ERROR_INVALID_NAME (123).

That blocked the natural fix for a second problem: RespAdminCommandsTests.SeSaveRecoverMultipleObjectsTest fails intermittently when the repo is checked out under a long path. LocalStorageDevice rejects non-extended paths longer than WIN32_MAX_PATH - 11 (249), and the deepest checkpoint file overflows it. It's intermittent because the generated test directory name embeds HashCode.ToHashCode(), which is randomized per process, so its length varies run to run.

Tsavorite's TestUtils already has EnsureExtendedLengthPathIfNeeded. Propagating it to Garnet alone is not enough — it makes test directories \\?\-prefixed, and the forward slashes above then become illegal. Hence two parts, in order.

Part 1 — path composition

Cluster config paths (ClusterManager, ReplicationManager) compose the cluster segment through FileDescriptor.directoryName rather than embedding it in the device factory's base name:

var deviceFactory = serverOptions.GetInitializedDeviceFactory(serverOptions.CheckpointDir ?? string.Empty);
clusterConfigDevice = deviceFactory.Get(new FileDescriptor(directoryName: "cluster", fileName: "nodes.conf"));

The base name is backend-neutral — a local directory, or an Azure container/prefix blob path that AzureStorageNamedDeviceFactory parses with Split('/'). Each factory joins descriptor components with its own separator (Path.Combine for local, GetSubDirectory for Azure), so this is correct on both, and resolves to the same local file as before.

Remaining sites use Path.Combine, with the leading slash dropped — Path.Combine treats a rooted second argument as absolute, so Path.Combine(dir, "/x") returns "\x":

File	Was
`libs/server/PubSub/SubscribeBroker.cs`	`logDir + "/pubsubkv"`
`libs/storage/.../Index/Common/KVSettings.cs`	`baseDir + "/hlog.log"`, `baseDir + "/checkpoints"`
`libs/storage/.../TsavoriteLog/TsavoriteLogSettings.cs`	`baseDir + "/tsavoritelog.log"`
`test/.../Garnet.test.collections/GarnetObjectTests.cs`	`MethodTestDir + "/hlog.log"`, `+ "/hlog.obj.log"`

Azure blob paths, runtimes/{rid}/native/... library paths, and Linux /sys/... paths correctly use forward slashes and are unchanged. On Linux Path.Combine yields /, so behavior there is byte-identical.

The KVSettings(baseDir) / TsavoriteLogSettings(baseDir) sites are the convenience constructors. No in-repo caller passes a non-null baseDir, so those are public-API hardening for external consumers.

Part 2 — extended-length test paths

TestUtils.UnitTestWorkingDir() now returns EnsureExtendedLengthPathIfNeeded(rootPath), mirroring the Tsavorite helper. A path is rewritten only when its fully-qualified length is within 100 chars of MAX_PATH, so ordinary checkouts and CI stay on ordinary paths.

The helper uses local constants rather than Tsavorite.core.Native32, whose members are internal: Garnet.test.cluster compiles this file via a linked <Compile Include=...> item and is not granted InternalsVisibleTo by Tsavorite.core.

Validation

This checkout's root is short enough (56 chars) that neither failure reproduces by default, so the extended-length route was exercised by temporarily lowering the helper's threshold — the same configuration that produces the failures on an affected machine. That override was reverted before commit.

Before the fix, with the prefix forced on, the cluster subset failed exactly as reported in the issue:

System.IO.IOException : Error creating log file for
  \\?\...\.tmp\<test>\7000/cluster\nodes.conf.0, error: 123

GarnetObjectTests was worse — it hung indefinitely rather than failing, because TestUtils.DeleteDirectory has a while (true) loop that swallows exceptions. After the fix it passes in about a second.

Suite	Extended paths forced	Normal threshold
`Garnet.test.cluster`	161 passed, 0 failed	161 passed, 0 failed
`Garnet.test.collections`	780 passed, 0 failed	780 passed, 0 failed
`Garnet.test`	1053 passed, 1 failed*	1054 passed, 0 failed
`SeSaveRecoverMultipleObjectsTest` x4	—	12 passed, 0 failed
`Garnet.test.acl` / `.complexstring` / `.extensions` / `.rangeindex`	—	466 / 403 / 534 / 62, all 0 failed
`Tsavorite.test`	—	342 passed, 0 failed
`Tsavorite.test.recovery`	—	207 passed, 1 failed**

dotnet format --verify-no-changes is clean on both Garnet.slnx and Tsavorite.slnx.

* MultipleClientsUnblockAndAddTest(20) — a pre-existing race between CLIENT UNBLOCK and concurrent pushes, no filesystem involvement. Passed in isolation and in the normal-threshold full run. Not reachable from this change: SubscribeBroker is always constructed with logDir: null, and no in-repo caller passes a non-null baseDir to KVSettings.

** RecoveryCheck2Tests.RecoveryRollback(FoldOver) — known flake on unmodified main, fixed separately in #2137; passed 3/3 in isolation here.

Notes for reviewers

This does not add a regression test for the flake itself: the fix is the test-infrastructure change, and asserting on it would mean asserting on the checkout root's length.
Do not report a failed checkpoint as a successful save #2145 (BGSAVE reporting a failed checkpoint as successful) originally used this path-length overflow as its fault injector. It has since moved to an independent failing-device injector, so there is no ordering dependency. Merging both branches locally is conflict-free, and with extended paths forced on, that PR's CheckpointFailureTests (4/4) and RespAdminCommandsTests (58/58) pass.

…h test paths Garnet composed several local-filesystem paths by concatenating a forward slash onto a base directory. Windows normalizes "/" to "\" in ordinary paths, but not inside a Win32 extended-length ("\\?\") path, where CreateFileW then fails with ERROR_INVALID_NAME. Replace that concatenation with Path.Combine at the six local-filesystem sites. The leading slash is dropped at each one because Path.Combine treats a rooted second argument as absolute and would discard the base directory. The cluster sites additionally guard a null CheckpointDir with string.Empty, matching the convention already used by AppendOnlyFileBaseDirectory and CheckpointBaseDirectory. Azure blob, native runtime, and Linux /sys paths keep their forward slashes. On Linux Path.Combine yields "/", so behavior there is unchanged. With the separators fixed, apply EnsureExtendedLengthPathIfNeeded to Garnet's TestUtils.UnitTestWorkingDir(), mirroring the helper already in Tsavorite's TestUtils. This fixes intermittent failures such as RespAdminCommandsTests.SeSaveRecoverMultipleObjectsTest when the repository is checked out under a long path: LocalStorageDevice rejects non-extended paths longer than WIN32_MAX_PATH - 11, and the deepest checkpoint files overflow that limit. The failures are intermittent because the generated test directory name embeds a per-process randomized HashCode, so its length varies run to run. The helper uses local constants rather than Tsavorite's Native32, whose members are internal; Garnet.test.cluster compiles this file via a linked Compile item and is not granted InternalsVisibleTo by Tsavorite.core.

Copilot

🟡 Changes recommended

Cluster paths regress Azure-backed storage on Windows, and one test still creates invalid mixed-separator extended paths.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Normalizes filesystem path composition and enables extended-length Windows test paths.

Changes:

Replaces manual separators with Path.Combine.
Adds extended-length path handling for test directories.
Updates cluster, replication, Pub/Sub, and Tsavorite paths.

File summaries

File	Description
`ClusterManager.cs`	Normalizes cluster configuration paths.
`ReplicationManager.cs`	Normalizes replication configuration paths.
`SubscribeBroker.cs`	Normalizes Pub/Sub log paths.
`KVSettings.cs`	Normalizes log and checkpoint paths.
`TsavoriteLogSettings.cs`	Normalizes Tsavorite log paths.
`TestUtils.cs`	Adds extended-length Windows test paths.

Review details

Files reviewed: 6/6 changed files
Comments generated: 3
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

… composition Compose the "cluster" segment through FileDescriptor.directoryName instead of embedding it in the device factory's base name. The base name is backend-neutral: for local storage it is a directory, but for Azure it is a "container/prefix" blob path that AzureStorageNamedDeviceFactory parses with Split('/'). Applying Path.Combine to it on Windows produced "container/prefix\cluster", which relocates nodes.conf and replication.conf away from the existing "prefix/cluster" blob hierarchy and prevents recovery of persisted cluster and replication state. Each factory already joins descriptor components with its own separator: LocalStorageNamedDeviceFactory uses Path.Combine, and AzureStorageNamedDeviceFactory uses GetSubDirectory, which joins with "/". Passing the segment as the descriptor directory is therefore correct on both backends, and resolves to the same local file as before, so existing on-disk clusters are unaffected. Also replace forward-slash concatenation in GarnetObjectTests.CreateStore. Now that the test working directory can be an extended-length path, that concatenation produced a mixed-separator "\\?\...\dir/hlog.log" name, which Windows does not normalize inside an extended-length path.

Ted Hart (TedHartMS) · 2026-09-20T19:40:21Z

Status analysis: the two open blockers

Short version: the red CI is a pre-existing break on main, not this PR, and both Copilot review concerns were already fixed in 1183b291c. Details and evidence below.

Blocker 1 — `Build Tsavorite` × 4 is `main`'s defect, not this PR's

Four jobs fail (ubuntu/windows × Debug/Release), all with the same error:

TestBase.cs(46,9): error CS0234: The type or namespace name 'TestUtils'
  does not exist in the namespace 'Garnet.test'  [Tsavorite.test.csproj]

Mechanism

libs/storage/Tsavorite/cs/test/Tsavorite.test.csproj:14 links exactly one file out of Garnet.test:

<Compile Include="..\..\..\..\..\test\standalone\Garnet.test\TestBase.cs" Link="TestBase.cs" />

TestUtils.cs is not linked. #2151 ("Give tests per-checkout port isolation via GARNET_TEST_PORT_SLOT") added a call to it inside TestBase.cs, in the global GlobalUnhandledExceptionHandling set-up fixture:

[OneTimeSetUp]
public void Install()
{
    // Resolve the port slot before any test runs. ...
    Garnet.test.TestUtils.EnsurePortSlotResolved();   // <-- TestBase.cs:46

So every project that links TestBase.cs now also needs TestUtils.cs. Tsavorite.test does not have it, and cannot compile.

Proof that it is `main`'s, independently reproduced

I built pristine origin/main @ 5c6e59536 in a detached scratch worktree — zero changes of mine present:

dotnet build libs\storage\Tsavorite\cs\test\Tsavorite.test.csproj -c Debug -f net10.0
  TestBase.cs(46,9): error CS0234: The type or namespace name 'TestUtils'
    does not exist in the namespace 'Garnet.test'
  1 Error(s)   BUILD_EXIT=1

Identical error, identical file, identical line. For completeness, this PR's head (d4e9a4635) fails the same way, and this PR touches neither of the two files involved:

	`TestBase.cs`	`Tsavorite.test.csproj`
`git diff origin/main...HEAD`	not touched	not touched

This PR's seven files are the cluster/pubsub/Tsavorite-settings path sites plus two test files. The break arrived purely through the base-branch update.

Minimal fix (belongs in a separate PR against `main`)

I tested the two candidates rather than guessing.

❌ Candidate A — link TestUtils.cs next to TestBase.cs. Does not work: 122 compile errors.

error CS0234: The type or namespace name 'client' does not exist in the namespace 'Garnet'
error CS0234: The type or namespace name 'common' does not exist in the namespace 'Garnet'
error CS0234: The type or namespace name 'server' does not exist in the namespace 'Garnet'
error CS0234: The type or namespace name 'CSharp' does not exist in the namespace 'Microsoft.CodeAnalysis'

TestUtils.cs pulls in Garnet.client, Garnet.common, Garnet.server, Garnet.server.Auth.Settings, Garnet.server.TLS, Roslyn and StackExchange.Redis, while Tsavorite.test references only Tsavorite.core and Tsavorite.devices.AzureStorageDevice. Making it compile would mean giving the storage engine's test project a dependency on the Garnet server stack — inverting the layering, since Garnet depends on Tsavorite and not the other way round.

✅ Candidate B — guard the one Garnet-only line. Validated end to end:

// TestBase.cs
#if !TSAVORITE_TEST
        Garnet.test.TestUtils.EnsurePortSlotResolved();
#endif

<!-- Tsavorite.test.csproj, in a PropertyGroup (it is a property, not an item) -->
<DefineConstants>$(DefineConstants);TSAVORITE_TEST</DefineConstants>

Result on pristine main + this patch:

project	build
`Tsavorite.test`	✅ exit 0
`Garnet.test`	✅ exit 0
`Garnet.test.cluster`	✅ exit 0

This is also semantically right, not just a compile workaround: Tsavorite.test references neither Garnet.server nor Garnet's TestUtils, so it never binds Garnet's per-sub-project test ports and has nothing to claim a port slot for. The symbol stays undefined in Garnet.test and Garnet.test.cluster, so #2151's isolation keeps working exactly as intended there.

A cleaner structural alternative, if maintainers prefer no #if: move GlobalUnhandledExceptionHandling out of TestBase.cs into its own file in Garnet.test. Tsavorite.test links only TestBase.cs, so it would stop seeing the Garnet-only call entirely — at the cost of losing the unhandled-exception diagnostics it currently inherits by accident.

Filed as #2161 with the full repro and both validated options. Either way the fix belongs on main, not on this branch. I have not applied it here. I'd also suggest not merging main into this branch again until it lands, and note that main itself is currently red for anyone who builds Tsavorite.

Blocker 2 — the Copilot review verdict

The 🟡 verdict was submitted against 7d9096852, which predates the current head. Its three inline threads are all resolved, and both headline concerns were fixed in 1183b291c. Proof for each:

2a. "Cluster paths regress Azure-backed storage on Windows" — fixed

The concern was correct against 7d9096852, which used Path.Combine(CheckpointDir, "cluster") as a device-factory base name. A base name is backend-neutral: AzureStorageNamedDeviceFactory splits it on / to derive container and prefix, so a Windows backslash corrupts the blob hierarchy. I reproduced that directly — mycontainer/myprefix\cluster parses to dir='myprefix\cluster' instead of myprefix/cluster.

The fix stopped composing into the base name at all. Path.Combine is gone from both sites; the segment moved into the file descriptor, which each factory joins with its own separator (Path.Combine for local, / for Azure):

libs/cluster/Server/ClusterManager.cs

var deviceFactory = serverOptions.GetInitializedDeviceFactory(serverOptions.CheckpointDir ?? string.Empty);
clusterConfigDevice = deviceFactory.Get(new FileDescriptor(directoryName: "cluster", fileName: "nodes.conf"));

libs/cluster/Server/Replication/ReplicationManager.cs

var deviceFactory = opts.GetInitializedDeviceFactory(opts.CheckpointDir ?? string.Empty);
replicationConfigDevice = deviceFactory.Get(new FileDescriptor(directoryName: "cluster", fileName: "replication.conf"));

Backward compatible on local: Path.Combine(dir + "/cluster", "", "nodes.conf") and Path.Combine(dir, "cluster", "nodes.conf") resolve to the same file. Both forms are now better than main, which concatenated "/cluster" and produced a mixed-separator path under an extended-length prefix.

Full re-audit of every converted site for Azure/blob reachability

Per the request, I re-traced each converted call site to its device rather than relying on the earlier round's conclusions:

#	Site	Composed value feeds	Azure reachable?
1	`ClusterManager.cs`	factory base name	n/a — no `Path.Combine` remains
2	`ReplicationManager.cs`	factory base name	n/a — no `Path.Combine` remains
3	`SubscribeBroker.cs`	`Devices.CreateLogDevice`	No
4	`KVSettings.cs` `LogDevice`	`Devices.CreateLogDevice`	No
5	`KVSettings.cs` `CheckpointDir`	`LocalStorageNamedDeviceFactoryCreator`	No
6	`TsavoriteLogSettings.cs`	`Devices.CreateLogDevice`	No

Sites 3, 4 and 6 — Devices.CreateLogDevice (libs/storage/Tsavorite/cs/src/core/Device/Devices.cs) switches over DeviceType and can only return NativeStorageDevice, LocalStorageDevice, RandomAccessLocalStorageDevice, ManagedLocalStorageDevice, NullDevice or LocalMemoryDevice; the default arm throws "Unsupported local device". There is no Azure arm — Azure devices live in the separate Tsavorite.devices.AzureStorageDevice assembly, which Tsavorite.core does not reference. These values are local paths by construction.

Site 5 — KVSettings.CheckpointDir is consumed at Tsavorite.cs:179, which hardcodes the local factory and canonicalizes through DirectoryInfo.FullName:

checkpointManager = checkpointSettings.CheckpointManager ??
    new DeviceLogCommitCheckpointManager
    (new LocalStorageNamedDeviceFactoryCreator(),
        new DefaultCheckpointNamingScheme(
            new DirectoryInfo(checkpointSettings.CheckpointDir ?? ".").FullName), ...);

Site 3 additionally has only one in-repo caller, GarnetServer.cs:277, which passes logDir: null — so it takes the NullDevice branch and the combine never executes in-repo.

So no converted site can feed a blob naming scheme. Azure blob paths, NativeStorageDevice's runtimes/{rid}/native/... and the useAzureStorage branches in TestUtils were deliberately left on forward slashes.

2b. "One test still creates invalid mixed-separator extended-length paths" — fixed

This one found a real gap: my original sweep covered libs/** and main/** but not test/**. test/standalone/Garnet.test.collections/GarnetObjectTests.cs:299-300 now reads:

logDevice ??= Devices.CreateLogDevice(Path.Combine(TestUtils.MethodTestDir, "hlog.log"));
objectLogDevice ??= Devices.CreateLogDevice(Path.Combine(TestUtils.MethodTestDir, "hlog.obj.log"));

Worth recording how bad this was: with the extended-length prefix forced on and the old concatenation in place, GarnetObjectTests hung indefinitely (killed at ~7 min) rather than failing, because TestUtils.DeleteDirectory retries in a while (true) that swallows exceptions. Fixed, it passes 5/5 in about a second. Garnet.test.collections is 780/780 both with and without the forced prefix.

I then re-swept test/** for the same pattern. The only remaining hits anywhere are in the YCSB benchmark data loader (TestLoader.cs:189-190), which reads user-supplied benchmark corpora and is not a Garnet test path — out of scope.

Related, and deliberately not fixed here

GetStoreCheckpointDirectory(dbId) and GetAppendOnlyFileDirectory(dbId) in GarnetServerOptions.cs still Path.Combine into device-factory base names, which is the same anti-pattern — but pre-existing on main and independent of this change. Filed separately as #2148. This PR has an empty diff against GarnetServerOptions.cs.

Bottom line

The red CI is main's break; this PR is not implicated, and the fix belongs in its own PR against main.
Both review concerns are already addressed, with no Path.Combine remaining on any backend-neutral base name.
Not merging.

Copilot AI balanced review requested due to automatic review settings September 17, 2026 03:11

Copilot started reviewing on behalf of Ted Hart (TedHartMS) September 17, 2026 03:12 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread libs/cluster/Server/ClusterManager.cs Outdated

Comment thread libs/cluster/Server/Replication/ReplicationManager.cs Outdated

Comment thread test/standalone/Garnet.test/TestUtils.cs

Badrish Chandramouli (badrishc) approved these changes Sep 18, 2026

View reviewed changes

Merge branch 'main' into tedhar-path-separator-normalization

d4e9a46

Ted Hart (TedHartMS) mentioned this pull request Sep 20, 2026

Tsavorite.test fails to compile on main: TestBase.cs references Garnet.test.TestUtils, which is not linked into Tsavorite.test.csproj #2161

Closed

Merge branch 'main' into tedhar-path-separator-normalization

48ffee3

Ted Hart (TedHartMS) merged commit 058e36f into main Sep 21, 2026
447 of 449 checks passed

Ted Hart (TedHartMS) deleted the tedhar-path-separator-normalization branch September 21, 2026 17:15

Navigation Menu

Normalize local-filesystem path composition and enable extended-length test paths - #2147

Normalize local-filesystem path composition and enable extended-length test paths#2147
Ted Hart (TedHartMS) merged 4 commits into
mainfrom
tedhar-path-separator-normalization

Ted Hart (TedHartMS) commented Sep 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ted Hart (TedHartMS) commented Sep 20, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Ted Hart (TedHartMS) commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Part 1 — path composition

Part 2 — extended-length test paths

Validation

Notes for reviewers

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ted Hart (TedHartMS) commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status analysis: the two open blockers

Blocker 1 — Build Tsavorite × 4 is main's defect, not this PR's

Mechanism

Proof that it is main's, independently reproduced

Minimal fix (belongs in a separate PR against main)

Blocker 2 — the Copilot review verdict

2a. "Cluster paths regress Azure-backed storage on Windows" — fixed

Full re-audit of every converted site for Azure/blob reachability

2b. "One test still creates invalid mixed-separator extended-length paths" — fixed

Related, and deliberately not fixed here

Bottom line

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ted Hart (TedHartMS) commented Sep 17, 2026 •

edited

Loading

Ted Hart (TedHartMS) commented Sep 20, 2026 •

edited

Loading

Blocker 1 — `Build Tsavorite` × 4 is `main`'s defect, not this PR's

Proof that it is `main`'s, independently reproduced

Minimal fix (belongs in a separate PR against `main`)