microsoft / microsoft/garnet

Azure storage on Windows: checkpoint and AOF commit base names embed backslashes, breaking container/blob naming

Open
#2,148 0 comments 0 reactions 1 assignee Claimed by @TedHartMS View on GitHub
Dominant language
C#
Stars
12k
Forks
703
Avg merge
2d 19h
Merged PRs (30d)
36

Description

### Describe the bug

When Garnet runs on **Windows** with `DeviceType.AzureStorage`, the base names handed to `AzureStorageNamedDeviceFactory` for checkpoints and AOF commits are built with `Path.Combine`, which inserts a backslash. Azure treats `/` as its blob hierarchy separator and parses the base name with `Split('/')`, so the backslash is not a separator there.

Two base names are affected:

| Consumer | Base name |
|---|---|
| `GarnetServer.cs:479` → `DefaultCheckpointNamingScheme` → `DeviceLogCommitCheckpointManager` | `GarnetServerOptions.GetStoreCheckpointDirectory(dbId)` |
| `GarnetServerOptions.cs:1125` → `DefaultCheckpointNamingScheme` | `GarnetServerOptions.GetAppendOnlyFileDirectory(dbId)` |

Both resolve through:

```csharp
public string StoreCheckpointBaseDirectory => Path.Combine(CheckpointBaseDirectory, "Store");
public string GetStoreCheckpointDirectory(int dbId) =>
Path.Combine(StoreCheckpointBaseDirectory, GetCheckpointDirectoryName(dbId));
public string GetAppendOnlyFileDirectory(int dbId) =>
Path.Combine(AppendOnlyFileBaseDirectory, GetAppendOnlyFileDirectoryName(dbId));
```

`Options.GetServerOptions()` only canonicalizes `checkpointDir` for non-Azure configs (`if (!useAzureStorage) checkpointDir = new DirectoryInfo(...).FullName;`), so with Azure the user-supplied value flows through as-is and the `Path.Combine` calls above append `\` on Windows.

`AzureStorageNamedDeviceFactory` then does:

```csharp
var path = baseName.Split('/');
var containerName = path[0];
var dirName = string.Join('/', path.Skip(1));
```

The impact depends on the form of `--checkpointdir`:

| `CheckpointDir` | Resulting base name | Container | Blob directory | Effect |
|---|---|---|---|---|
| `mycontainer` | `mycontainer\Store\checkpoints` | `mycontainer\Store\checkpoints` | *(empty)* | **Fails** — backslashes are illegal in an Azure container name |
| `mycontainer/myprefix` | `mycontainer/myprefix\Store\checkpoints` | `mycontainer` | `myprefix\Store\checkpoints` | **Silently wrong** — blob names are permissive, so data is written to a flat `myprefix\Store\checkpoints` name instead of the intended `myprefix/Store/checkpoints` hierarchy |

On Linux `Path.Combine` yields `/`, so the composition happens to be correct there and this is Windows-only.

### Steps to reproduce the bug

1. On Windows, start Garnet configured for Azure storage — `--storage-string ` with a device type of `AzureStorage` — and set `--checkpointdir mycontainer`.
2. Trigger a checkpoint (`SAVE` / `BGSAVE`).
3. The container name is rejected because it contains backslashes. With `--checkpointdir mycontainer/myprefix` instead, the operation succeeds but the blobs are created under a literal `myprefix\Store\checkpoints` name rather than the nested hierarchy.

### Expected behavior

Base names handed to a device factory should stay backend-neutral, and path segments below them should be expressed via `FileDescriptor.directoryName` so each factory composes them with its own separator — `Path.Combine` for `LocalStorageNamedDeviceFactory`, `GetSubDirectory` (which joins with `/`) for `AzureStorageNamedDeviceFactory`.

### Additional context

There is already a correct precedent in the same file — the AOF **log device** does exactly this:

```csharp
// GarnetServerOptions.cs:1223
return GetInitializedDeviceFactory(AppendOnlyFileBaseDirectory)
.Get(new FileDescriptor(GetAppendOnlyFileDirectoryName(dbId), "aof.log"));
```

`AppendOnlyFileBaseDirectory` is `CheckpointDir ?? string.Empty` with no `Path.Combine`, and the per-database subdirectory is supplied through the descriptor. Only the two `DefaultCheckpointNamingScheme` base names above still embed segments via `Path.Combine`.

The same pattern was applied to the cluster config paths in #2147 (review feedback on `ClusterManager` / `ReplicationManager`), which is what surfaced this. That PR deliberately does not change `GarnetServerOptions`, so these two sites are unaffected by it and remain as they are on `main`.

Note that a fix changes the on-disk/blob layout for existing Windows+Azure deployments in the `container/prefix` case, since data previously written under the backslash name would no longer be found — worth considering whether a migration or a release note is needed.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.