Azure storage on Windows: checkpoint and AOF commit base names embed backslashes, breaking container/blob naming
- 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.