refactor: split chunk storage from chunk index#56
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR extracts chunk blob protocol logic into a new shared IChunkStorageService/ChunkStorageService, moves repository-local path helpers into RepositoryPaths, centralizes ChunkHydrationStatus in shared scope, and refactors archive/restore/hydration-query handlers and tests to use the new service and path helpers instead of touching blob-level abstractions or deriving repo paths themselves. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #56 +/- ##
==========================================
+ Coverage 72.38% 72.76% +0.37%
==========================================
Files 63 66 +3
Lines 4817 4807 -10
Branches 640 651 +11
==========================================
+ Hits 3487 3498 +11
+ Misses 1194 1167 -27
- Partials 136 142 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Arius.E2E.Tests/E2ETests.cs (1)
274-280:⚠️ Potential issue | 🔴 Critical
RestoreCommandHandlerconstruction is missing the new chunk-storage dependency.This fixture still instantiates
RestoreCommandHandlerwith the old argument list. With the refactored constructor, this is a build-break for the E2E test project.🐛 Minimal fix
+using Arius.Core.Shared.ChunkStorage; @@ private RestoreCommandHandler CreateRestoreHandler() => new(BlobContainer, Encryption, Index, + new ChunkStorageService(BlobContainer, Encryption), FileTreeService, Snapshot, _mediator, NullLogger<RestoreCommandHandler>.Instance, _account, _container);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/E2ETests.cs` around lines 274 - 280, The test helper CreateRestoreHandler instantiates RestoreCommandHandler with the old parameter list; update the constructor call in CreateRestoreHandler to pass the new chunk-storage dependency required by RestoreCommandHandler (add the test fixture's chunk storage instance—e.g., ChunkStorage, IChunkStorage, or the fixture field like _chunkStorage—immediately after Index or in the correct position per the refactored constructor) while keeping the existing parameters (BlobContainer, Encryption, Index, FileTreeService, Snapshot, _mediator, NullLogger<RestoreCommandHandler>.Instance, _account, _container).src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs (1)
405-423:⚠️ Potential issue | 🟠 Major
RehydrationStartedEventstill undercounts tar bundle bytes.
totalRehydrateBytesis updated from the first file entry's proportionalCompressedSize. For tar-backed chunks that only captures one file's share, not the full tar blob, so the emitted rehydration size can be far too small. ReuseSumCompressedBytes(chunkHash)here instead.💡 Suggested fix
- if (indexEntries.TryGetValue(filesByChunkHash[chunkHash][0].ContentHash, out var ie)) - totalRehydrateBytes += ie.CompressedSize; + totalRehydrateBytes += SumCompressedBytes(chunkHash);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs` around lines 405 - 423, The RehydrationStartedEvent size metric is undercounting because totalRehydrateBytes is using the first file's proportional CompressedSize; replace that logic by calling SumCompressedBytes(chunkHash) for each chunk to accumulate the full tar blob size; inside the loop where _chunkStorage.StartRehydrationAsync(chunkHash, ...) is called, on success add totalRehydrateBytes += SumCompressedBytes(chunkHash) (using the existing SumCompressedBytes method) instead of reading indexEntries/filesByChunkHash, then publish the RehydrationStartedEvent as before.
🧹 Nitpick comments (4)
src/Arius.Core.Tests/List/ResolveFileHydrationStatusesHandlerTests.cs (1)
49-53: Prefer mockingIChunkStorageServicein handler-level tests.These tests now exercise the real
ChunkStorageServicepath, which can blur whether failures come from handler orchestration or storage internals. Consider substitutingIChunkStorageServicehere and keeping concrete storage behavior coverage inChunkStorageServicetests.As per coding guidelines
**/*.Tests/**: “Test projects. Focus on test coverage gaps and assertion quality rather than style.”Also applies to: 94-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/List/ResolveFileHydrationStatusesHandlerTests.cs` around lines 49 - 53, The handler-level test instantiates a real ChunkStorageService which leaks storage behavior into orchestration tests; replace the concrete new ChunkStorageService(...) with a mocked IChunkStorageService (e.g., a test double for IChunkStorageService) when creating the ChunkHydrationStatusQueryHandler so the test only verifies handler logic; update both instantiations that create ChunkHydrationStatusQueryHandler (the one assigning handler at line ~49 and the one at ~94) to inject the mocked IChunkStorageService, configure the mock’s methods to return the expected results for the test cases, and keep concrete ChunkStorageService coverage in its own ChunkStorageService tests.openspec/changes/split-chunk-storage-from-index/proposal.md (1)
18-19: Remove or populate the empty “Modified Capabilities” section.Leaving an empty heading makes the spec delta harder to scan.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/split-chunk-storage-from-index/proposal.md` around lines 18 - 19, The "Modified Capabilities" heading in openspec/changes/split-chunk-storage-from-index/proposal.md is empty; either remove this heading or add a brief bullet list describing which capabilities changed (or confirm "none") so the spec delta is self-contained—edit the "Modified Capabilities" section header to be removed if not needed or populate it with concise entries referencing the specific capability names affected by the split-chunk-storage-from-index proposal.src/Arius.Cli.Tests/DependencyInjectionTests.cs (1)
40-41: Strengthen DI assertion to guard against duplicateChunkStorageServiceinstances.These checks confirm resolvability, but they don’t verify that interface + concrete resolve to the same object. That invariant helps prevent split service graphs.
♻️ Suggested assertion tightening
- serviceProvider.GetRequiredService<IChunkStorageService>().ShouldBeOfType<ChunkStorageService>(); - serviceProvider.GetRequiredService<ChunkStorageService>().ShouldBeOfType<ChunkStorageService>(); + var chunkStorageViaInterface = serviceProvider.GetRequiredService<IChunkStorageService>(); + var chunkStorageViaConcrete = serviceProvider.GetRequiredService<ChunkStorageService>(); + chunkStorageViaInterface.ShouldBeOfType<ChunkStorageService>(); + ReferenceEquals(chunkStorageViaInterface, chunkStorageViaConcrete).ShouldBeTrue();As per coding guidelines "Register shared services once per repository/session in DI and feature handlers should consume those shared instances through constructor injection" and "Avoid duplicate service graphs for the same repository because that can split cache state and validation state".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/DependencyInjectionTests.cs` around lines 40 - 41, The test currently only verifies that IChunkStorageService and ChunkStorageService are resolvable but not that they refer to the same instance; update the assertion in DependencyInjectionTests to resolve both using serviceProvider.GetRequiredService<IChunkStorageService>() and serviceProvider.GetRequiredService<ChunkStorageService>() and assert they are the same object (e.g., use ReferenceEquals or your test framework's ShouldBeSameAs/BeSameAs) to guarantee the DI container returns a single shared ChunkStorageService instance for both the interface and concrete type.src/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceSurfaceTests.cs (1)
14-36: Assert the public contract onIChunkStorageService, not just the concrete class.Most of this test reflects over
ChunkStorageService, but the feature-facing boundary introduced by this PR isIChunkStorageService. An explicit-interface implementation or an extra public helper would make this suite fail or pass for the wrong reason. I'd keep theIsAssignableFromcheck, but move the signature assertions totypeof(IChunkStorageService).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceSurfaceTests.cs` around lines 14 - 36, Keep the IsAssignableFrom assertion for typeof(IChunkStorageService) vs typeof(ChunkStorageService), but change the reflection-based signature checks to inspect the interface type: replace typeof(ChunkStorageService).GetMethods(...) with typeof(IChunkStorageService).GetMethods(...) (or simply typeof(IChunkStorageService).GetMethods()) and then use the same BindingFlags/ToDictionary and the existing method-name checks (UploadLargeAsync, UploadTarAsync, UploadThinAsync, DownloadAsync, GetHydrationStatusAsync, StartRehydrationAsync, PlanRehydratedCleanupAsync) so the test asserts the public contract defined by IChunkStorageService rather than the concrete class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openspec/changes/split-chunk-storage-from-index/specs/chunk-storage-service/spec.md`:
- Around line 55-56: The implementation of GetHydrationStatusAsync in
ChunkStorageService returns ChunkHydrationStatus.Unknown when the primary chunk
blob is absent, but the spec and Shared/ChunkIndex/ChunkHydrationStatus.cs
define this state as Missing; update GetHydrationStatusAsync to return
ChunkHydrationStatus.Missing when the primary blob does not exist and update any
local checks in ChunkStorageService (and related handlers/tests that rely on
Unknown) to use Missing so the service and spec are consistent about the
missing-state name.
In `@src/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceReadTests.cs`:
- Around line 13-35: Add a test that seeds both the primary archived blob and
the rehydrated copy and asserts GetHydrationStatusAsync returns Available:
create a new test method (e.g.,
GetHydrationStatusAsync_ReturnsAvailable_WhenArchiveChunkRehydratedCopyExists)
that uses FakeMetadataOnlyBlobContainerService, set
blobs.Metadata[BlobPaths.Chunk("abc")] = new BlobMetadata { Exists = true, Tier
= BlobTier.Archive } and blobs.Metadata[BlobPaths.ChunkRehydrated("abc")] = new
BlobMetadata { Exists = true }, instantiate ChunkStorageService and call
GetHydrationStatusAsync("abc", ...), then assert the result is
ChunkHydrationStatus.Available to cover the case where the rehydrated copy
exists.
In
`@src/Arius.Core/Features/ChunkHydrationStatusQuery/ChunkHydrationStatusQuery.cs`:
- Around line 18-33: The handler currently injects and uses
ChunkHydrationStatusQueryHandler._chunkStorage.GetHydrationStatusAsync to
resolve hydration, but per architecture it should drop the IChunkStorageService
dependency and use IBlobContainerService (_blobs) directly; modify the
constructor to remove the IChunkStorageService parameter and field, remove all
calls to _chunkStorage.GetHydrationStatusAsync in
ChunkHydrationStatusQueryHandler, instead use ChunkIndexService to map file
content hashes to chunk hashes (reuse _index) and call the raw blob metadata
methods on _blobs to determine hydration state; update any method signatures and
tests that relied on _chunkStorage accordingly and ensure _blobs is actually
referenced where hydration state is computed.
In `@src/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cs`:
- Around line 262-272: The loop in ExecuteAsync increments deleted and adds
meta.ContentLength unconditionally even when the blob no longer exists; modify
ExecuteAsync to only count removals and add freed bytes when the blob actually
existed and was deleted: call _blobs.GetMetadataAsync(blobName) and if the
returned meta is null (or indicates non-existence) skip counting; otherwise call
_blobs.DeleteAsync(blobName) and only increment deleted and add
meta.ContentLength to freed if the delete call indicates success (or if
DeleteAsync has no result, rely on a non-null meta to signal that an actual
deletion occurred). Ensure the returned RehydratedChunkCleanupResult uses these
corrected deleted and freed totals.
- Around line 130-139: The BlobAlreadyExistsException handler in
ChunkStorageService (the catch block using blobName, existing,
BlobMetadataKeys.AriusType, ChunkUploadResult,
_blobs.GetMetadataAsync/_blobs.DeleteAsync and the retry/goto logic) must only
treat an existing blob as a successful recoverable upload when the blob is fully
committed and matches the expected state: verify the AriusType value equals the
expected type for this upload AND validate other required indicators of a
completed upload (e.g., existing.ContentLength == expected length and any other
required metadata keys/values that indicate finalization); if any check fails,
remove or repair the blob (call _blobs.DeleteAsync or perform metadata fixup)
reset content.Position when content.CanSeek and retry (preserve the existing
goto retry logic), and only return a ChunkUploadResult with AlreadyExisted: true
when all checks pass.
- Around line 136-139: The retry-after-delete path in ChunkStorageService.cs
uses goto retry after calling _blobs.DeleteAsync but does not handle
non-seekable source streams, so a non-seekable content can be reused
already-consumed and cause truncated uploads; fix by ensuring the stream is
rewindable before retrying: either (a) if content.CanSeek is false, create a
buffered rewindable copy (e.g., copy into a MemoryStream or temp file) and use
that rewindable stream for the upload attempts, or (b) if buffering is not
acceptable, avoid deleting/ retrying and instead surface an error — implement
this logic around the retry label and replace the current content.Position reset
with code that swaps in the bufferedStream when needed, referencing the content
variable, content.CanSeek, content.Position, the retry label and the call to
_blobs.DeleteAsync.
- Around line 281-314: ChunkDownloadStream only implements DisposeAsync, so
synchronous disposal leaks resources; add a protected override void Dispose(bool
disposing) in ChunkDownloadStream that checks disposing, disposes _inner,
_decryptStream, _progressStream, and _downloadStream (safely handling nulls and
avoiding double-dispose), and then calls base.Dispose(disposing); keep
DisposeCoreAsync as-is (or have it call the same disposal logic) so both async
and sync paths use the same cleanup code and optionally use a private disposed
flag to prevent double disposal.
In `@src/Arius.Integration.Tests/Pipeline/CrashRecoveryTests.cs`:
- Around line 94-95: The tests stopped injecting faults into the chunk-upload
path because ChunkStorageService writes via OpenWriteAsync while
FaultingBlobService only throws from UploadAsync; update the fault injection so
OpenWriteAsync (and the returned stream's Write/Flush/Dispose as appropriate)
can fault as well: modify or extend FaultingBlobService (or wrap blobService
passed into ChunkStorageService in CrashRecoveryTests) to throw during
OpenWriteAsync and/or when the returned stream is written to, ensuring the
crash-recovery tests still exercise “crash during chunk upload” paths used by
ChunkStorageService.OpenWriteAsync.
---
Outside diff comments:
In `@src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs`:
- Around line 405-423: The RehydrationStartedEvent size metric is undercounting
because totalRehydrateBytes is using the first file's proportional
CompressedSize; replace that logic by calling SumCompressedBytes(chunkHash) for
each chunk to accumulate the full tar blob size; inside the loop where
_chunkStorage.StartRehydrationAsync(chunkHash, ...) is called, on success add
totalRehydrateBytes += SumCompressedBytes(chunkHash) (using the existing
SumCompressedBytes method) instead of reading indexEntries/filesByChunkHash,
then publish the RehydrationStartedEvent as before.
In `@src/Arius.E2E.Tests/E2ETests.cs`:
- Around line 274-280: The test helper CreateRestoreHandler instantiates
RestoreCommandHandler with the old parameter list; update the constructor call
in CreateRestoreHandler to pass the new chunk-storage dependency required by
RestoreCommandHandler (add the test fixture's chunk storage instance—e.g.,
ChunkStorage, IChunkStorage, or the fixture field like _chunkStorage—immediately
after Index or in the correct position per the refactored constructor) while
keeping the existing parameters (BlobContainer, Encryption, Index,
FileTreeService, Snapshot, _mediator,
NullLogger<RestoreCommandHandler>.Instance, _account, _container).
---
Nitpick comments:
In `@openspec/changes/split-chunk-storage-from-index/proposal.md`:
- Around line 18-19: The "Modified Capabilities" heading in
openspec/changes/split-chunk-storage-from-index/proposal.md is empty; either
remove this heading or add a brief bullet list describing which capabilities
changed (or confirm "none") so the spec delta is self-contained—edit the
"Modified Capabilities" section header to be removed if not needed or populate
it with concise entries referencing the specific capability names affected by
the split-chunk-storage-from-index proposal.
In `@src/Arius.Cli.Tests/DependencyInjectionTests.cs`:
- Around line 40-41: The test currently only verifies that IChunkStorageService
and ChunkStorageService are resolvable but not that they refer to the same
instance; update the assertion in DependencyInjectionTests to resolve both using
serviceProvider.GetRequiredService<IChunkStorageService>() and
serviceProvider.GetRequiredService<ChunkStorageService>() and assert they are
the same object (e.g., use ReferenceEquals or your test framework's
ShouldBeSameAs/BeSameAs) to guarantee the DI container returns a single shared
ChunkStorageService instance for both the interface and concrete type.
In `@src/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceSurfaceTests.cs`:
- Around line 14-36: Keep the IsAssignableFrom assertion for
typeof(IChunkStorageService) vs typeof(ChunkStorageService), but change the
reflection-based signature checks to inspect the interface type: replace
typeof(ChunkStorageService).GetMethods(...) with
typeof(IChunkStorageService).GetMethods(...) (or simply
typeof(IChunkStorageService).GetMethods()) and then use the same
BindingFlags/ToDictionary and the existing method-name checks (UploadLargeAsync,
UploadTarAsync, UploadThinAsync, DownloadAsync, GetHydrationStatusAsync,
StartRehydrationAsync, PlanRehydratedCleanupAsync) so the test asserts the
public contract defined by IChunkStorageService rather than the concrete class.
In `@src/Arius.Core.Tests/List/ResolveFileHydrationStatusesHandlerTests.cs`:
- Around line 49-53: The handler-level test instantiates a real
ChunkStorageService which leaks storage behavior into orchestration tests;
replace the concrete new ChunkStorageService(...) with a mocked
IChunkStorageService (e.g., a test double for IChunkStorageService) when
creating the ChunkHydrationStatusQueryHandler so the test only verifies handler
logic; update both instantiations that create ChunkHydrationStatusQueryHandler
(the one assigning handler at line ~49 and the one at ~94) to inject the mocked
IChunkStorageService, configure the mock’s methods to return the expected
results for the test cases, and keep concrete ChunkStorageService coverage in
its own ChunkStorageService tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fc79423-b59f-4e43-9870-969325f94df5
📒 Files selected for processing (43)
README.mdopenspec/changes/split-chunk-storage-from-index/.openspec.yamlopenspec/changes/split-chunk-storage-from-index/design.mdopenspec/changes/split-chunk-storage-from-index/proposal.mdopenspec/changes/split-chunk-storage-from-index/specs/chunk-storage-service/spec.mdopenspec/changes/split-chunk-storage-from-index/tasks.mdsrc/Arius.Cli.Tests/CliTests.cssrc/Arius.Cli.Tests/DependencyInjectionTests.cssrc/Arius.Cli/CliBuilder.cssrc/Arius.Core.Tests/Archive/ArchiveRecoveryTests.cssrc/Arius.Core.Tests/ChunkIndex/ShardTests.cssrc/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceReadTests.cssrc/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceSurfaceTests.cssrc/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceUploadTests.cssrc/Arius.Core.Tests/FileTree/FileTreeServiceTests.cssrc/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cssrc/Arius.Core.Tests/List/ResolveFileHydrationStatusesHandlerTests.cssrc/Arius.Core.Tests/Repository/RepositoryPathsTests.cssrc/Arius.Core.Tests/Restore/RestoreCommandHandlerTests.cssrc/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cssrc/Arius.Core/Features/ChunkHydrationStatusQuery/ChunkHydrationStatusQuery.cssrc/Arius.Core/Features/ChunkHydrationStatusQuery/ChunkHydrationStatusResolver.cssrc/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cssrc/Arius.Core/ServiceCollectionExtensions.cssrc/Arius.Core/Shared/ChunkIndex/ChunkHydrationStatus.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cssrc/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cssrc/Arius.Core/Shared/ChunkStorage/ChunkUploadResult.cssrc/Arius.Core/Shared/ChunkStorage/IChunkStorageService.cssrc/Arius.Core/Shared/ChunkStorage/IRehydratedChunkCleanupPlan.cssrc/Arius.Core/Shared/ChunkStorage/RehydratedChunkCleanupResult.cssrc/Arius.Core/Shared/FileTree/FileTreeService.cssrc/Arius.Core/Shared/RepositoryPaths.cssrc/Arius.Core/Shared/Snapshot/SnapshotService.cssrc/Arius.E2E.Tests/E2ETests.cssrc/Arius.E2E.Tests/RehydrationE2ETests.cssrc/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cssrc/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cssrc/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/ContainerCreationTests.cssrc/Arius.Integration.Tests/Pipeline/CrashRecoveryTests.cssrc/Arius.Integration.Tests/Pipeline/PipelineFixture.cssrc/Arius.Integration.Tests/Pipeline/RehydrationStateTests.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cs`:
- Around line 39-75: Add a unit test to verify that GetHydrationStatusAsync
prefers a ready rehydrated copy over the primary's IsRehydrating flag: create a
FakeMetadataOnlyBlobContainerService where "chunks/abc" has Exists = true, Tier
= BlobTier.Archive, IsRehydrating = true and "chunks-rehydrated/abc" has Exists
= true and Tier = BlobTier.Hot (or any non-Archive), instantiate
ChunkStorageService and assert that GetHydrationStatusAsync("abc", ...) returns
ChunkHydrationStatus.Available; name the test clearly (e.g.,
GetHydrationStatusAsync_RehydratedCopyReady_TakesPrecedence).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f30f6b4-61b5-4beb-9a77-423261e77a48
📒 Files selected for processing (3)
src/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cssrc/Arius.Core/Features/ChunkHydrationStatusQuery/ChunkHydrationStatusResolver.cssrc/Arius.E2E.Tests/E2ETests.cs
💤 Files with no reviewable changes (1)
- src/Arius.Core/Features/ChunkHydrationStatusQuery/ChunkHydrationStatusResolver.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs (1)
37-60: 🛠️ Refactor suggestion | 🟠 MajorRestore now hides repository blob operations behind the shared chunk service.
This handler now routes hydration status, download, rehydration copy, and cleanup through
_chunkStorage, but the current restore guideline keeps those raw blob operations inRestoreCommandHandlerso the feature remains the read-only repository orchestrator. Please either keep these calls on_blobshere or update the architectural guidance alongside this refactor. As per coding guidelines,src/Arius.Core/Features/**/*RestoreCommand*.cs: RestoreCommandHandler should use SnapshotService to resolve the target snapshot, FileTreeService to traverse filetrees, ChunkIndexService to resolve content hashes to chunk metadata, and raw IBlobContainerService for chunk download, rehydration status, copy, and cleanup.Also applies to: 253-275, 412-440, 585-635
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs` around lines 37 - 60, The refactor moved blob operations (hydration status, download, rehydration copy, cleanup) to _chunkStorage but the restore handler must remain the repository read-only orchestrator and use IBlobContainerService (_blobs) per guidelines; update RestoreCommandHandler to call _blobs for those raw blob operations (hydration status, Download, StartRehydrate/Copy, Delete) while keeping SnapshotService, FileTreeService and ChunkIndexService usage for snapshot resolution, tree traversal and chunk metadata resolution; alternatively, if you intend to centralize these ops in _chunkStorage, update the architecture/guideline comments and adjust all referenced sections (around lines noted) to reflect the new responsibility so code and docs align.
♻️ Duplicate comments (4)
src/Arius.Core/Features/ChunkHydrationStatusQuery/ChunkHydrationStatusQuery.cs (1)
17-27: 🛠️ Refactor suggestion | 🟠 MajorThis still crosses the current feature/shared boundary.
ChunkHydrationStatusQueryHandlernow delegates hydration lookup toIChunkStorageService, but the repo rule for this handler keeps raw blob metadata calls in the feature afterChunkIndexServicemaps content hashes to chunk hashes. IfChunkStorageServiceis the new intended boundary, that architectural rule should be updated in the same PR; otherwise hydration-state resolution should stay on_blobshere. As per coding guidelines,src/Arius.Core/Features/**/*ChunkHydrationStatusQuery*.cs: ChunkHydrationStatusQueryHandler should use ChunkIndexService to map file content hashes to chunk hashes and raw IBlobContainerService metadata calls for hydration state.Also applies to: 61-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Features/ChunkHydrationStatusQuery/ChunkHydrationStatusQuery.cs` around lines 17 - 27, ChunkHydrationStatusQueryHandler currently delegates hydration lookup to IChunkStorageService (_chunkStorage) which violates the feature/shared boundary rule; either update the architectural boundary in the PR or revert this handler to resolve hydration state locally: use ChunkIndexService (_index) to map content hashes to chunk hashes and then call the raw blob metadata methods on IBlobContainerService (_blobs) to determine hydration state (instead of using IChunkStorageService), ensuring ChunkHydrationStatusQueryHandler follows the src/Arius.Core/Features/**/*ChunkHydrationStatusQuery*.cs guideline.src/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cs (1)
24-49:⚠️ Potential issue | 🟡 MinorStill missing the rehydrated-copy-precedence case.
The suite does not cover the combination where the primary chunk is
ArchivewithIsRehydrating = true, butchunks-rehydrated/<hash>is already in a readable tier. That is the branch-order case most likely to regress ifGetHydrationStatusAsync()gets reordered later. As per coding guidelines,**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cs` around lines 24 - 49, Add a test covering the precedence case where the primary blob is Archive with IsRehydrating=true but the rehydrated copy already exists in a readable tier; specifically, in List/FileHydrationStatusResolverTests create a scenario using FakeMetadataOnlyBlobContainerService where Metadata["chunks/abc"] = { Exists=true, Tier=BlobTier.Archive, IsRehydrating=true } and Metadata["chunks-rehydrated/abc"] = { Exists=true, Tier=BlobTier.Cool } then call ChunkStorageService.GetHydrationStatusAsync("abc", CancellationToken.None) and assert the result is ChunkHydrationStatus.Available (and optionally assert the requested blob checks include both "chunks/abc" and "chunks-rehydrated/abc") so the rehydrated-copy-precedence branch is exercised.src/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cs (2)
284-289:⚠️ Potential issue | 🟡 MinorCleanup totals still overcount after concurrent deletion.
deleted++andfreed += ...run even when the rehydrated blob is already gone by the timeExecuteAsync()reaches it. A concurrent cleanup, or a secondExecuteAsync()call, will therefore return inflated totals. Skip counting whenmeta.Existsis false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cs` around lines 284 - 289, The loop in ExecuteAsync currently increments deleted and adds to freed regardless of whether the blob still exists; after calling _blobs.GetMetadataAsync(blobName, cancellationToken) check meta.Exists and only call _blobs.DeleteAsync(blobName, cancellationToken) and increment deleted and freed (using meta.ContentLength) when meta.Exists is true; leave the rest unchanged so concurrent deletions don't inflate the totals (refer to _blobNames, _blobs.GetMetadataAsync, _blobs.DeleteAsync, meta.Exists, deleted, and freed).
131-136:⚠️ Potential issue | 🟠 MajorTighten the “already exists” recovery checks in both upload paths.
Both catch blocks still treat “metadata contains
AriusType” as sufficient proof that the existing blob is safe to reuse. That can falsely accept the wrong chunk kind or a half-finalized blob and skip the repair/delete path. Please require the expectedAriusTypevalue and the rest of the completion metadata before returning success here.Also applies to: 198-202
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Arius.Integration.Tests/Pipeline/CrashRecoveryTests.cs`:
- Line 27: The test helper ShouldFaultUpload currently uses
Interlocked.Increment(ref _uploadCount) > throwAfterN + 1 which shifts the fault
boundary by one; update the failure logic so throwAfterN accurately represents
the number of faultable uploads — for example change ShouldFaultUpload to use
Interlocked.Increment(ref _uploadCount) > throwAfterN (remove the + 1), or
refactor to count only the specific operation types the tests target (e.g.,
separate counters or a predicate for uploads vs tar operations) so tests like
Archive_CrashAfterFirstUpload_Rerun_ProducesCorrectSnapshot and
Archive_CrashAfterTarBeforeThinChunks_Rerun_CorrectSnapshot fault exactly at the
intended operation.
---
Outside diff comments:
In `@src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs`:
- Around line 37-60: The refactor moved blob operations (hydration status,
download, rehydration copy, cleanup) to _chunkStorage but the restore handler
must remain the repository read-only orchestrator and use IBlobContainerService
(_blobs) per guidelines; update RestoreCommandHandler to call _blobs for those
raw blob operations (hydration status, Download, StartRehydrate/Copy, Delete)
while keeping SnapshotService, FileTreeService and ChunkIndexService usage for
snapshot resolution, tree traversal and chunk metadata resolution;
alternatively, if you intend to centralize these ops in _chunkStorage, update
the architecture/guideline comments and adjust all referenced sections (around
lines noted) to reflect the new responsibility so code and docs align.
---
Duplicate comments:
In `@src/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cs`:
- Around line 24-49: Add a test covering the precedence case where the primary
blob is Archive with IsRehydrating=true but the rehydrated copy already exists
in a readable tier; specifically, in List/FileHydrationStatusResolverTests
create a scenario using FakeMetadataOnlyBlobContainerService where
Metadata["chunks/abc"] = { Exists=true, Tier=BlobTier.Archive,
IsRehydrating=true } and Metadata["chunks-rehydrated/abc"] = { Exists=true,
Tier=BlobTier.Cool } then call
ChunkStorageService.GetHydrationStatusAsync("abc", CancellationToken.None) and
assert the result is ChunkHydrationStatus.Available (and optionally assert the
requested blob checks include both "chunks/abc" and "chunks-rehydrated/abc") so
the rehydrated-copy-precedence branch is exercised.
In
`@src/Arius.Core/Features/ChunkHydrationStatusQuery/ChunkHydrationStatusQuery.cs`:
- Around line 17-27: ChunkHydrationStatusQueryHandler currently delegates
hydration lookup to IChunkStorageService (_chunkStorage) which violates the
feature/shared boundary rule; either update the architectural boundary in the PR
or revert this handler to resolve hydration state locally: use ChunkIndexService
(_index) to map content hashes to chunk hashes and then call the raw blob
metadata methods on IBlobContainerService (_blobs) to determine hydration state
(instead of using IChunkStorageService), ensuring
ChunkHydrationStatusQueryHandler follows the
src/Arius.Core/Features/**/*ChunkHydrationStatusQuery*.cs guideline.
In `@src/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cs`:
- Around line 284-289: The loop in ExecuteAsync currently increments deleted and
adds to freed regardless of whether the blob still exists; after calling
_blobs.GetMetadataAsync(blobName, cancellationToken) check meta.Exists and only
call _blobs.DeleteAsync(blobName, cancellationToken) and increment deleted and
freed (using meta.ContentLength) when meta.Exists is true; leave the rest
unchanged so concurrent deletions don't inflate the totals (refer to _blobNames,
_blobs.GetMetadataAsync, _blobs.DeleteAsync, meta.Exists, deleted, and freed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 878b3a98-4b3f-4e1a-b57a-208cb286e338
📒 Files selected for processing (12)
src/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceReadTests.cssrc/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceUploadTests.cssrc/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cssrc/Arius.Core.Tests/List/ResolveFileHydrationStatusesHandlerTests.cssrc/Arius.Core/Features/ChunkHydrationStatusQuery/ChunkHydrationStatusQuery.cssrc/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cssrc/Arius.Core/ServiceCollectionExtensions.cssrc/Arius.Core/Shared/ChunkIndex/ChunkHydrationStatus.cssrc/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cssrc/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cssrc/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cssrc/Arius.Integration.Tests/Pipeline/CrashRecoveryTests.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Arius.Core/Shared/ChunkIndex/ChunkHydrationStatus.cs
- src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs
- src/Arius.Core.Tests/List/ResolveFileHydrationStatusesHandlerTests.cs
- src/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cs
- src/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceReadTests.cs
- src/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceUploadTests.cs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cs (2)
14-15: Consider usingBlobPathshelper for consistency.The tests use hardcoded path strings like
"chunks/abc"whileChunkStorageServiceReadTests.csusesBlobPaths.Chunk("abc"). Using the helper would improve consistency across test files and make tests more resilient to path format changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cs` around lines 14 - 15, Replace the hardcoded blob key "chunks/abc" with the BlobPaths helper to keep path formatting consistent; locate where FakeMetadataOnlyBlobContainerService is instantiated and its Metadata dictionary is populated (blobs.Metadata) and change the key to BlobPaths.Chunk("abc") so the test uses the same path helper as ChunkStorageServiceReadTests.
9-89: Missing precedence test for rehydrated copy ready over primary IsRehydrating flag.The test suite covers the main hydration status scenarios well. However, a test is still needed where the primary chunk is
ArchivewithIsRehydrating = true, butchunks-rehydrated/<hash>exists in a non-Archive tier. The expected status should beAvailableto confirm the implementation's branch order is correct.✅ Suggested test addition
+ [Test] + public async Task GetHydrationStatusAsync_ReturnsAvailable_WhenPrimaryIsRehydratingButRehydratedCopyIsReady() + { + var blobs = new FakeMetadataOnlyBlobContainerService(); + blobs.Metadata["chunks/abc"] = new BlobMetadata { Exists = true, Tier = BlobTier.Archive, IsRehydrating = true }; + blobs.Metadata["chunks-rehydrated/abc"] = new BlobMetadata { Exists = true, Tier = BlobTier.Cool }; + var service = new ChunkStorageService(blobs, new Arius.Core.Shared.Encryption.PlaintextPassthroughService()); + + var status = await service.GetHydrationStatusAsync("abc", CancellationToken.None); + + status.ShouldBe(ChunkHydrationStatus.Available); + blobs.RequestedBlobNames.ShouldBe(["chunks/abc", "chunks-rehydrated/abc"]); + }As per coding guidelines, "
**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cs` around lines 9 - 89, Add a test in ChunkStorageHydrationStatusTests that verifies a rehydrated copy takes precedence over the primary IsRehydrating flag: create blobs.Metadata["chunks/abc"] = new BlobMetadata { Exists = true, Tier = BlobTier.Archive, IsRehydrating = true } and blobs.Metadata["chunks-rehydrated/abc"] = new BlobMetadata { Exists = true, Tier = BlobTier.Cool } (or Hot), call ChunkStorageService.GetHydrationStatusAsync("abc", ...), and assert the result is ChunkHydrationStatus.Available and that blobs.RequestedBlobNames includes "chunks/abc" and "chunks-rehydrated/abc" to confirm the branch ordering.src/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceReadTests.cs (1)
10-123: Good test coverage for the new ChunkStorageService read operations.The test suite covers the key hydration status scenarios, download behavior, rehydration kickoff, and cleanup planning/execution. The test at lines 37-48 addresses the previously requested "rehydrated copy already exists" case.
However, a precedence test is still missing: when the primary chunk is
ArchivewithIsRehydrating = truebut the rehydrated copy already exists in a non-Archive tier, the expected result should beAvailable(rehydrated copy takes precedence). This protects the branch-order contract inGetHydrationStatusCoreAsync.✅ Suggested test addition
+ [Test] + public async Task GetHydrationStatusAsync_ReturnsAvailable_WhenPrimaryIsRehydratingButRehydratedCopyIsReady() + { + var blobs = new FakeMetadataOnlyBlobContainerService(); + blobs.Metadata[BlobPaths.Chunk("abc")] = new BlobMetadata { Exists = true, Tier = BlobTier.Archive, IsRehydrating = true }; + blobs.Metadata[BlobPaths.ChunkRehydrated("abc")] = new BlobMetadata { Exists = true, Tier = BlobTier.Cool }; + var service = new ChunkStorageService(blobs, new PlaintextPassthroughService()); + + var status = await service.GetHydrationStatusAsync("abc", CancellationToken.None); + + status.ShouldBe(ChunkHydrationStatus.Available); + }As per coding guidelines, "
**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceReadTests.cs` around lines 10 - 123, Add a new unit test to assert that when the primary blob (BlobPaths.Chunk("abc")) is Archive with IsRehydrating = true but a rehydrated copy (BlobPaths.ChunkRehydrated("abc")) already exists in a non-Archive tier, ChunkStorageService.GetHydrationStatusAsync returns ChunkHydrationStatus.Available; use FakeMetadataOnlyBlobContainerService, set blobs.Metadata for the primary to BlobMetadata { Exists = true, Tier = BlobTier.Archive, IsRehydrating = true } and for the rehydrated path to BlobMetadata { Exists = true, Tier = BlobTier.Cold }, call service.GetHydrationStatusAsync("abc", ...), and assert the result equals ChunkHydrationStatus.Available to protect the branch ordering in GetHydrationStatusCoreAsync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceReadTests.cs`:
- Around line 10-123: Add a new unit test to assert that when the primary blob
(BlobPaths.Chunk("abc")) is Archive with IsRehydrating = true but a rehydrated
copy (BlobPaths.ChunkRehydrated("abc")) already exists in a non-Archive tier,
ChunkStorageService.GetHydrationStatusAsync returns
ChunkHydrationStatus.Available; use FakeMetadataOnlyBlobContainerService, set
blobs.Metadata for the primary to BlobMetadata { Exists = true, Tier =
BlobTier.Archive, IsRehydrating = true } and for the rehydrated path to
BlobMetadata { Exists = true, Tier = BlobTier.Cold }, call
service.GetHydrationStatusAsync("abc", ...), and assert the result equals
ChunkHydrationStatus.Available to protect the branch ordering in
GetHydrationStatusCoreAsync.
In `@src/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cs`:
- Around line 14-15: Replace the hardcoded blob key "chunks/abc" with the
BlobPaths helper to keep path formatting consistent; locate where
FakeMetadataOnlyBlobContainerService is instantiated and its Metadata dictionary
is populated (blobs.Metadata) and change the key to BlobPaths.Chunk("abc") so
the test uses the same path helper as ChunkStorageServiceReadTests.
- Around line 9-89: Add a test in ChunkStorageHydrationStatusTests that verifies
a rehydrated copy takes precedence over the primary IsRehydrating flag: create
blobs.Metadata["chunks/abc"] = new BlobMetadata { Exists = true, Tier =
BlobTier.Archive, IsRehydrating = true } and
blobs.Metadata["chunks-rehydrated/abc"] = new BlobMetadata { Exists = true, Tier
= BlobTier.Cool } (or Hot), call
ChunkStorageService.GetHydrationStatusAsync("abc", ...), and assert the result
is ChunkHydrationStatus.Available and that blobs.RequestedBlobNames includes
"chunks/abc" and "chunks-rehydrated/abc" to confirm the branch ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46ceed47-3927-4aa4-8ccf-1312aa68c130
📒 Files selected for processing (9)
openspec/changes/split-chunk-storage-from-index/specs/chunk-storage-service/spec.mdsrc/Arius.Core.Tests/ChunkStorage/ChunkStorageServiceReadTests.cssrc/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cssrc/Arius.Core/Features/ChunkHydrationStatusQuery/ChunkHydrationStatusQuery.cssrc/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cssrc/Arius.Core/Shared/ChunkIndex/ChunkHydrationStatus.cssrc/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cssrc/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cssrc/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs
✅ Files skipped from review due to trivial changes (2)
- src/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cs
- openspec/changes/split-chunk-storage-from-index/specs/chunk-storage-service/spec.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Arius.Core/Shared/ChunkIndex/ChunkHydrationStatus.cs
- src/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cs
Owner
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
This was referenced Apr 19, 2026
This was referenced Apr 27, 2026
This was referenced May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
IChunkStorageService/ChunkStorageServiceas the chunk-blob boundaryChunkStorageService, and simplify feature handlers to rely on higher-level shared services instead of raw blob plumbingTesting
dotnet test --project src/Arius.Core.Tests/Arius.Core.Tests.csprojdotnet test --project src/Arius.Architecture.Tests/Arius.Architecture.Tests.csprojdotnet test --project src/Arius.Integration.Tests/Arius.Integration.Tests.csproj --treenode-filter "/*/*/CrashRecoveryTests/*"