Complete filesystem mount, migration, and recovery - #996
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughファイルシステム用キー、MVCCサービス、FUSEアダプタ、チャンクルーティング、分割境界観測を追加し、DistributionServer、監視レジストリ、関連テスト、設計書を更新した。 Changesファイルシステムキーとシャードルーティング
MVCCファイルシステムサービス
FUSEアダプタ境界
分割境界ルーティングと観測
補助検証と設計記録
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant FUSEClient
participant fuseadapterAdapter
participant filesystemService
participant Dispatcher
FUSEClient->>fuseadapterAdapter: Read or Write request
fuseadapterAdapter->>filesystemService: RefreshOpenHandleLease
fuseadapterAdapter->>filesystemService: Read or Write
filesystemService->>Dispatcher: Dispatch transaction
Dispatcher-->>filesystemService: CoordinateResponse
filesystemService-->>fuseadapterAdapter: Data or filesystem error
fuseadapterAdapter-->>FUSEClient: Data and syscall.Errno
sequenceDiagram
participant DistributionServer
participant fskeys
participant FileSystemMetrics
DistributionServer->>fskeys: NormalizeSplitBoundary
fskeys-->>DistributionServer: normalized split key
DistributionServer->>FileSystemMetrics: ObserveFilePinnedHotspot(split_boundary)
FileSystemMetrics-->>DistributionServer: increment Prometheus counter
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![gemini-code-assist[bot]](https://cdn.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a filesystem layer on top of Elastickv, implementing a core filesystem service, a FUSE session adapter, key formatting utilities, and manual split boundary snapping with associated metrics. The review feedback highlights several critical issues: a bug in extracting gRPC status codes from wrapped errors, atomicity violations in SetAttr, Release, and lease reaping due to split transactions, a performance bottleneck in StatFS caused by scanning the entire keyspace, and an optimization opportunity in the Write path to avoid redundant reads on full chunk overwrites.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Addressed the latest-head filesystem review blockers. Changes:
Validation run on this head:
|
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f140605b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the latest-head filesystem review findings on Changes:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7528bdd5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if replacedEntry.Type == TypeDirectory { | ||
| return nil, ErrIsDir | ||
| } |
There was a problem hiding this comment.
When the source entry is a directory and newName already names a regular file, this helper treats the target as replaceable because it only rejects directory targets. The caller then overwrites the file's dirent with the directory and unlinks/GCs the file, so rename(dir, file) succeeds instead of returning a type error; please compare the source and target types before allowing replacement.
Useful? React with 👍 / 👎.
| err := s.scanExistingChunks(ctx, meta, ts, start, func(pair *store.KVPair) error { | ||
| delta = delta.merge(usageDelta{bytesSub: uint64(len(pair.Value))}) | ||
| elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Del, Key: pair.Key}) | ||
| readKeys = append(readKeys, pair.Key) |
There was a problem hiding this comment.
For truncating a file with more than 10,000 stored chunks (about 40 GiB at the default 4 MiB chunk size), this loop appends every chunk key to ReadKeys; the coordinator rejects transactions once len(ReadKeys) > 10_000, so large-file truncation can fail instead of deleting the tail. The same unbounded read set is used by file GC below, so unlink/release/rename-over-target can hit the same limit; please page these deletes/usage updates or avoid a single read set that grows with the whole file.
Useful? React with 👍 / 👎.
| } | ||
| end := prefixEnd(prefix) | ||
| for { | ||
| page, err := s.store.ScanAt(ctx, start, end, statFSScanPageSize, ts) |
There was a problem hiding this comment.
When this service is backed by kv.ShardStore, chunk writes are stored on the group selected by routeKey(fskeys.ChunkKey(...)) (the virtual !fs|route|chk|... key), but this raw !fs|chk|... range scan is routed by ShardStore.ScanAt using the raw scan bounds. In a multi-range deployment where the raw chunk prefix and the virtual chunk-route prefix resolve to different groups, truncate/GC scans return no chunks, leaving deleted file data behind and usage deltas wrong; route these chunk prefix scans through the same filesystem route domain or scan the owning group directly.
Useful? React with 👍 / 👎.
|
Addressed the latest filesystem foundation blockers and merged current main. Changes:
Validation:
|
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6915d4680a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var delta usageDelta | ||
| var chunkDeletes *chunkDeletePlan | ||
| if replacedMeta != nil { | ||
| replacedElems, replacedReads, replacedDelta, plan, err := s.unlinkFileTxnParts(ctx, ts, replacedMeta, now) |
There was a problem hiding this comment.
When renaming a directory over an existing directory, replacedMeta is a directory but this path always invokes unlinkFileTxnParts. That helper never checks that the target directory is empty and only decrements Nlink, so rename(dirA, dirB) can overwrite a non-empty target dentry while leaving dirB's inode/children orphaned; even empty directory targets remain counted instead of having their inode/home/dir-version rows removed. Directory replacements need the rmdir semantics here.
Useful? React with 👍 / 👎.
| ts, err := reader.LinearizableRead(ctx) | ||
| return ts, errors.Wrap(err, "filesystem linearizable read") |
There was a problem hiding this comment.
When the service is wired to the production kv.ShardedCoordinator, this type assertion calls LinearizableRead and uses the returned Raft applied index as the MVCC read timestamp for GetAt/ScanAt. Filesystem writes commit at HLC/MVCC timestamps, not Raft indexes, so after InitializeRoot or any write with an HLC commit timestamp greater than the applied index, the next filesystem read can miss the committed version and report ErrNotFound or stale metadata. Use the store's MVCC watermark/snapshot timestamp after fencing instead of the linearizable-read index.
Useful? React with 👍 / 👎.
| if routeStart, routeEnd, ok := fskeys.ChunkScanRouteBounds(start, end); ok { | ||
| return s.engine.GetIntersectingRoutes(routeStart, routeEnd), false |
There was a problem hiding this comment.
This filesystem chunk scan path returns all intersecting virtual routes with clampToRoutes=false, but the forward scan loop scans each returned route's GroupID without de-duplicating. After SplitRange, child routes normally keep the same backing group, so a broad chunk scan such as the ChunkAllPrefix scan used by the StatFS fallback will scan the same store once per child route and return duplicate chunks, double-counting bytes and potentially duplicating API results.
Useful? React with 👍 / 👎.
|
Addressed the latest filesystem foundation review findings on Changes:
Validation:
Note: a full @codex review |
|
Current HEAD The remaining merge blocker is GitHub Actions status: several jobs are still reported in progress even though the Jepsen log reached successful cleanup. I am continuing with the CI gate separately. @codex review |
|
✅ Action performedReview finished.
|
|
Pushed signed CI refresh commit No file content changed; latest-head unresolved non-outdated review threads remain at 0. New CI is running on this head. @codex review |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
monitoring/filesystem.go (1)
1-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
fsPinnedHotspotReasonSplitBoundaryとadapter.DistributionFilePinnedHotspotSplitBoundaryが同じ値"split_boundary"を重複定義している。一方が変更された場合、メトリクスは
normalizeFilePinnedHotspotReasonによって"unknown"にフォールバックされるため実害はないが、意図しないデグレードの原因となる。定数の同期を検出するテストを追加するか、共通の定数を参照することを推奨する。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monitoring/filesystem.go` around lines 1 - 46, Duplicate the "split_boundary" reason definition shared by fsPinnedHotspotReasonSplitBoundary and adapter.DistributionFilePinnedHotspotSplitBoundary. Prefer referencing a single exported shared constant from normalizeFilePinnedHotspotReason and its callers; alternatively add a test asserting both constants remain synchronized.adapter/distribution_server.go (1)
211-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
isSplitBoundaryErrorのエラー判定が文字列マッチに依存している。
strings.Contains(err.Error(), errDistributionSplitKeyAtBoundary.Error())は、grpcStatusErrorがセンチネルエラーのメッセージ文字列のみを保持する設計上、現状では妥当な選択だが、エラーメッセージの変更に対して脆弱である。将来的にgrpcStatusErrorが元のエラーを wrap するように改修された場合、errors.Isへの移行を推奨する。また
status.Code(errors.Cause(err))とerr.Error()で unwrap の有無が不整合だが、現状ではvalidateSplitKeyがエラーを wrap せず返すため問題ない。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/distribution_server.go` around lines 211 - 222, isSplitBoundaryError の判定がエラーメッセージ文字列に依存している。grpcStatusError が元エラーを保持・wrapする実装へ移行し、errDistributionSplitKeyAtBoundary を errors.Is で判定できるようにする。あわせて status.Code とセンチネル判定で同じ unwrap 済みエラーを利用し、エラーの扱いを一貫させる。internal/filesystem/service.go (2)
375-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffコーディングガイドライン:
//nolintディレクティブの使用について。
SetAttr、Read、Write、Rename、Readdir、createNode、unlinkに//nolint:cyclopが設定されています。各関数のコメントで正当な理由が説明されていますが、コーディングガイドラインでは「//nolintの使用を避け、リファクタリングを推奨する」と明記されています。各関数の分岐をヘルパー関数へ抽出することで cyclop 複雑度を下げることを検討してください。ただし、現状のコメントによる正当化も妥当と見なせるため、必須ではありません。As per coding guidelines:
**/*.go: Avoid using//nolintdirectives; refactor instead in Go codeAlso applies to: 523-523, 589-589, 759-759, 923-923, 1361-1361, 1451-1451
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/filesystem/service.go` at line 375, Refactor SetAttr, Read, Write, Rename, Readdir, createNode, and unlink to reduce cyclop complexity by extracting their conditional branches into focused helper functions, then remove the corresponding //nolint:cyclop directives.Source: Coding guidelines
613-613: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
directoryInitialNlinkを容量計算の定数として再利用している点が分かりにくい。
last - first + uint64(directoryInitialNlink)はlast - first + 2と等価で、チャンク要素 + inode メタデータ + usage 更新の容量ヒントを意図していますが、ディレクトリのリンク数定数を流用する意味的接続が不明確です。専用の定数を定義するか、インラインで+ 2とコメントを添えることを推奨します。♻️ 提案する修正
const ( // ... directoryInitialNlink uint32 = 2 + writeTxnMetaElemCount uint64 = 2 // inode metadata + usage update )- elemCap, err := checkedInt(last - first + uint64(directoryInitialNlink)) + elemCap, err := checkedInt(last - first + writeTxnMetaElemCount)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/filesystem/service.go` at line 613, Replace the reuse of directoryInitialNlink in the capacity calculation near checkedInt with a semantically named dedicated constant representing the two additional capacity entries, or use an explicit +2 with a concise comment documenting the chunk, inode metadata, and usage update capacity hint; preserve the existing checkedInt behavior.internal/filesystem/fuseadapter/adapter.go (1)
208-222: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value未対応操作の errno が一部一貫していない点を確認してください。
SymlinkはEOPNOTSUPP、ReadlinkはENOSYSを返しています。両者とも「未実装」ですが、errno が異なります。FUSE クライアントの実装によっては挙動が変わる可能性があります。意図的な選択であれば問題ありませんが、ReadlinkもEOPNOTSUPPに統一することを検討してください。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/filesystem/fuseadapter/adapter.go` around lines 208 - 222, 未実装操作の errno が不統一です。Adapter の Symlink と同様に Readlink も syscall.EOPNOTSUPP を返すよう変更し、未対応操作のエラーコードを統一してください。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/filesystem/service.go`:
- Around line 1733-1748: deleteChunkPages currently retries indefinitely when
deleteChunkPage reports a write conflict. Add a bounded retry counter within
deleteChunkPages, define an appropriate maximum, and return an error once the
limit is exceeded while preserving existing handling for successful completion
and other errors.
---
Nitpick comments:
In `@adapter/distribution_server.go`:
- Around line 211-222: isSplitBoundaryError
の判定がエラーメッセージ文字列に依存している。grpcStatusError
が元エラーを保持・wrapする実装へ移行し、errDistributionSplitKeyAtBoundary を errors.Is
で判定できるようにする。あわせて status.Code とセンチネル判定で同じ unwrap 済みエラーを利用し、エラーの扱いを一貫させる。
In `@internal/filesystem/fuseadapter/adapter.go`:
- Around line 208-222: 未実装操作の errno が不統一です。Adapter の Symlink と同様に Readlink も
syscall.EOPNOTSUPP を返すよう変更し、未対応操作のエラーコードを統一してください。
In `@internal/filesystem/service.go`:
- Line 375: Refactor SetAttr, Read, Write, Rename, Readdir, createNode, and
unlink to reduce cyclop complexity by extracting their conditional branches into
focused helper functions, then remove the corresponding //nolint:cyclop
directives.
- Line 613: Replace the reuse of directoryInitialNlink in the capacity
calculation near checkedInt with a semantically named dedicated constant
representing the two additional capacity entries, or use an explicit +2 with a
concise comment documenting the chunk, inode metadata, and usage update capacity
hint; preserve the existing checkedInt behavior.
In `@monitoring/filesystem.go`:
- Around line 1-46: Duplicate the "split_boundary" reason definition shared by
fsPinnedHotspotReasonSplitBoundary and
adapter.DistributionFilePinnedHotspotSplitBoundary. Prefer referencing a single
exported shared constant from normalizeFilePinnedHotspotReason and its callers;
alternatively add a test asserting both constants remain synchronized.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56561c29-c6e8-49ee-b0f9-89f2d556deef
📒 Files selected for processing (18)
adapter/distribution_server.goadapter/distribution_server_test.goadapter/redis_delta_compactor_test.godocs/design/2026_02_24_partial_filesystem_on_elastickv.mdinternal/filesystem/fuseadapter/adapter.gointernal/filesystem/fuseadapter/adapter_test.gointernal/filesystem/service.gointernal/filesystem/service_test.gointernal/fskeys/keys.gointernal/fskeys/keys_test.gokv/shard_key.gokv/shard_key_test.gokv/shard_store.gokv/shard_store_test.gomain.gomonitoring/filesystem.gomonitoring/filesystem_test.gomonitoring/registry.go
|
Addressed the latest non-outdated filesystem review thread on Changes:
Caller audit:
Validation:
|
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6f76f6ddb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a08f9069be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| func (s *ShardStore) routesForFilesystemChunkScan(start []byte, end []byte) ([]distribution.Route, bool) { | ||
| if routeStart, routeEnd, ok := fskeys.ChunkScanRouteBounds(start, end); ok { | ||
| return dedupeRoutesByGroup(s.engine.GetIntersectingRoutes(routeStart, routeEnd)), true |
There was a problem hiding this comment.
This helper feeds NewBackupScanner, so deduplicating the chunk route list here drops catalog ranges before the scanner captures its snapshot. When a chunk-prefix backup spans multiple virtual file routes that share a group, the group scan can still return chunks from the later routes, but after a catalog change materializeRouteForKey no longer has the captured route containing kv.RouteKey(item.key) and can skip those chunks instead of reading them from the old group. Keep the full route list for the scanner and dedupe only in the per-group scan dispatch path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bce58eb. routesForFilesystemChunkScan now preserves every captured route range for backup materialization. Normal scan dispatch still deduplicates by group through seenGroups, so this changes snapshot metadata retention without issuing duplicate group scans. Added TestBackupScannerRetainsAllCapturedChunkRanges.
| chunkKey := fskeys.ChunkKey(meta.HomeSlot, inode, idx) | ||
| chunkStart := idx * chunkSize | ||
| writeStart := max(offset, chunkStart) | ||
| writeEnd := min(end, chunkStart+chunkSize) |
There was a problem hiding this comment.
When offset lands in the last address-space chunk (for example after SetAttr grows a sparse file near math.MaxUint64), chunkStart+chunkSize wraps before min is applied. That makes writeEnd less than writeStart, so the subsequent offset arithmetic returns ErrInvalid even though offset+len(data) was valid; use checked or saturating addition for the chunk boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bce58eb. Both Read and Write now use checked saturating chunk-end arithmetic, including the full-chunk predicate. The caller/path audit found both arithmetic sites and no remaining chunkStart + chunkSize expression. Added a MaxUint64 final-chunk read/write regression test.
| route, ok := s.store.engine.GetRoute(key) | ||
| if ok && route.GroupID == item.route.GroupID { | ||
| return route, true |
There was a problem hiding this comment.
BackupScanner is supposed to materialize from the route snapshot captured at creation time, but this fallback consults the live engine after those routes fail to contain the key. If a backup starts while x belongs to group 2, then the catalog later moves x to group 1, a stale/off-route version of x found on group 1 can now pass this same-group check and be included even though the captured route says it should be read from group 2 or skipped. Keep materialization decisions limited to item.route/s.routes so catalog changes after scanner creation cannot change the backup contents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bce58eb. materializeRouteForKey no longer consults the live routing engine and resolves keys only against the scanner-captured route snapshot. Added TestBackupScannerDoesNotUseLiveCatalogForMaterialization to cover a catalog change after scanner creation.
|
Updated PR #996 at Resolved the latest-head findings:
Audit:
Verification:
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Lines 211 to 213 in bce58eb
Keep the live route when coalescing backup keys
When a stale old-owner copy of the same key is still present on a route that sorts after the current owner (for example after a range move from the later group back to an earlier one), this duplicate-key branch overwrites the earlier/live routedScanKey with the later stale route. materializeRouteForKey then rejects that stale route because the key no longer belongs to that group, so the backup page drops the live key entirely. Prefer the item whose captured route contains routeKey(item.key) instead of blindly replacing duplicates.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| end, overflow := addUint64(offset, size) | ||
| if overflow { | ||
| return 0, ErrInvalid |
There was a problem hiding this comment.
When a file has been sparsely extended near math.MaxUint64 (for example via Truncate) and a caller reads from a valid offset close to EOF with a normal request size larger than the remaining bytes, offset + size can overflow here before the code gets a chance to clamp to fileSize. That makes a valid tail read return ErrInvalid/EINVAL instead of the remaining bytes; use saturating end arithmetic for reads before applying the EOF clamp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4685081. Read end calculation now uses saturating uint64 addition before clamping to EOF, so a valid tail read near MaxUint64 no longer fails on offset+size overflow. TestServiceWriteReadAtSaturatedFinalChunkBoundary now covers an overflowing request that returns only the remaining bytes. The boundedEnd caller audit found only Service.Read.
| // Keep every captured range so backup materialization remains pinned to the | ||
| // catalog snapshot. Unclamped scan dispatch de-duplicates these by group. | ||
| return s.engine.GetIntersectingRoutes(nil, nil), true |
There was a problem hiding this comment.
For StatFS, routedUsageCounterAt scans !fs|usage|route| and this branch fans the scan out to every group, but normal ScanAt does not materialize keys through the captured route or deduplicate equal keys. If a range move leaves a stale usage-counter copy on the old owner while the migrated copy exists on the new owner, both values are returned and merged, so used bytes/files are double-counted. Usage scans need the same owner filtering used by backup materialization, or stale off-route copies must be ignored before merging.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4685081. Filesystem usage scans now decode each routed counter embedded route key and retain it only when the scanned group is the current catalog owner. The filter covers forward value scans, key-only scans, and reverse scans. The regression test places both a duplicate stale value and a stale-only counter on the old group and verifies only the owner copy is returned.
|
Latest head: 4685081 Resolved the current-head findings:
Verification:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46850812fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var clampToRoutes bool | ||
| if st != nil { | ||
| routes, clampToRoutes = st.routesForScan(start, end) | ||
| routes, clampToRoutes = st.routesForForwardScan(start, end) |
There was a problem hiding this comment.
When the backup range overlaps fskeys.UsageRouteAllPrefix(), routesForForwardScan now fans out to every route; if a split/ownership move leaves a stale copy of the same routed usage counter in a later non-owner group, scanKeyRoutesWithSourceAt de-duplicates equal keys to that later route and materializeRouteForKey then rejects it, so Next advances past the key without emitting the live counter from the owner group. This can make backups omit filesystem usage counters even though normal forward/key-only/reverse scans filter stale copies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e9900d1. Duplicate routed usage keys now retain the copy from the live owner group before backup materialization. Added TestBackupScannerPrefersFilesystemUsageOwnerCopy; kv and full repository tests pass.
| if filterUsageOwners { | ||
| kvs = s.filterFilesystemUsageKVsForGroup(kvs, route.GroupID) |
There was a problem hiding this comment.
When a usage-counter scan hits a group that has at least limit stale !fs|usage|route|... rows before an owned counter, this post-scan filter can shrink the page below the requested limit without scanning further in the same group. Callers such as StatFS page until len(page) < statFSScanPageSize, so they can stop early and miss later owned counters after route moves/splits leave many stale copies behind.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e9900d1. Forward value, forward key-only, and reverse usage scans now continue paging after owner filtering until the requested limit is refilled or the group is exhausted. Added TestShardStoreScanAt_RefillsAfterStaleFilesystemUsageCounters; kv and full repository tests pass.
| if req.GetReverse() { | ||
| groupScanner, ok := r.store.(rawGroupReverseScanner) | ||
| if !ok { | ||
| return nil, errors.WithStack(status.Error(codes.FailedPrecondition, "reverse raw scan with explicit group requires a group-aware store")) | ||
| } | ||
| res, err := groupScanner.ReverseScanGroupAt(ctx, groupID, req.StartKey, req.EndKey, limit, readTS) | ||
| return res, errors.WithStack(err) |
There was a problem hiding this comment.
When a client sends RawScanAt with GroupId, Reverse, and KeysOnly, the request is routed to rawScanKeysAt before this new value-scan helper is reached, and that path still rejects explicit-group reverse scans. This leaves the newly supported explicit-group reverse scan path working only when values are requested, while key-only callers continue to get InvalidArgument for the same group/range.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e9900d1. GroupId + Reverse + KeysOnly now uses ReverseScanGroupAt and strips values without falling back to the global scan. Added TestGRPCServer_RawScanAt_ReverseKeysOnlyUsesExplicitGroup; adapter and full repository tests pass.
bootjp
changed the title
|
Implemented the remaining filesystem design requirements at e9900d1. Evidence:
Kernel mount smoke remains environment-dependent and is documented for a FUSE-capable deployment host; the raw protocol and Linux compile paths are covered here. @codex review |
|
All required checks are green on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9900d1b82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if refreshedMeta.Type == TypeDirectory { | ||
| return 0, nil, false, ErrIsDir | ||
| } | ||
| return refreshedTS, refreshedMeta, true, nil |
There was a problem hiding this comment.
When a size-changing Write/Truncate passes ensureActiveHomeAt and then MoveFile flips the file to MIGRATING while deleteChunksPastCurrentEOF runs, this refresh can load the migrating inode and return it as usable. The later transaction only includes HomeKey as an OCC read, so because the migration state already existed at refreshedTS it will not conflict; a grow/truncate can then commit during the move and the target copy can miss or retain stale chunks. Revalidate the refreshed home at this timestamp before returning it to callers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9be920ff. cleanupStaleChunksBeforeSizeChange now revalidates ensureActiveHomeAt at the refreshed timestamp after stale-chunk cleanup, so Write/Truncate/SetAttr fail with ErrStaleHome if migration started during maintenance. Added TestServiceTruncateRejectsHomeThatStartsMigratingDuringStaleCleanup and covered it under targeted race tests.
| if index+1 < len(result.Entries) { | ||
| handle.positions[offset] = dirPosition{cookie: position.cookie, skip: index + 1} | ||
| } else { | ||
| handle.positions[offset] = dirPosition{cookie: result.NextCookie} |
There was a problem hiding this comment.
When the backend returns the final directory page (NextCookie == ""), this records the empty start cookie for the last offset. The kernel will use that offset for the next READDIR call, so ReadDir calls the backend with an empty cookie and starts listing from the beginning again, producing duplicate entries and potentially an endless ls on ordinary directories whose final page fits in the buffer. Store an EOF state for that offset instead of the empty cookie.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9be920ff. Directory offsets now record a terminal EOF state when NextCookie is empty, and a subsequent READDIR at that offset returns an empty page without calling the backend again. Added TestRawFileSystemReadDirKeepsEOFOffsetTerminal.
| elems, reads, delta, err := s.rmdirTargetTxnParts(ctx, ts, parentMeta, replacedMeta) | ||
| return elems, reads, delta, nil, err | ||
| } | ||
| return s.unlinkFileTxnParts(ctx, ts, replacedMeta, now) |
There was a problem hiding this comment.
When a same-directory rename overwrites a file whose home is already MIGRATING, this path calls unlinkFileTxnParts without the active-home guard that direct Unlink uses. Because the rename reads the already-migrating inode at its ts, the transaction can commit, mark/delete the target while the move job is in progress, and then the mover hits ErrMoveFenceLost with durable job/intent state and leftover chunks; check ensureActiveHomeAt for the replacement before unlinking it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9be920ff. Replacement-file rename now requires an ACTIVE matching home before building unlink parts, and unlink transaction paths retain HomeKey as an OCC read so a move starting before commit also fences the namespace mutation. Added TestServiceRenameRejectsMigratingReplacement.
| } | ||
| for _, pair := range page { | ||
| if pair != nil && bytes.HasPrefix(pair.Key, prefix) { | ||
| out = append(out, pair) |
There was a problem hiding this comment.
When startup or an operator calls RecoverIntents with the default/explicit limit, scanRecoveryPrefix still keeps fetching limit-sized pages until the prefix is exhausted because every page is appended here without a total cap. On nodes with many stale intents or move jobs this defeats the intended bounded recovery batch and can make FUSE mount startup spend the full recovery timeout processing all records; stop once len(out) == limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9be920ff. scanRecoveryPrefix now returns as soon as the accumulated visible record count reaches the requested limit. TestServiceRecoverIntentsHonorsScanLimit verifies a three-record backlog is processed as bounded 2+1 batches.
| result, errno := r.adapter.Create(ctx, input.NodeId, []byte(name), filesystem.CreateOptions{ | ||
| Mode: input.Mode & permissionModeMask, | ||
| UID: input.Uid, | ||
| GID: input.Gid, | ||
| }) |
There was a problem hiding this comment.
For kernel MKNOD, this goes through Adapter.Create, which always injects the mount client ID and creates a durable open-handle lease, then performs a separate Release. If that release is canceled or hits a transient conflict after the create transaction committed, the syscall returns an error even though the file now exists, and a retry sees EEXIST while the leaked lease remains until the reaper runs. Use a create path that does not allocate an open handle for MKNOD.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9be920ff. Kernel MKNOD now uses a dedicated adapter path that clears ClientID before the core Create call, so no durable open handle or follow-up Release is created. Added TestRawFileSystemMknodDoesNotAllocateOpenHandle.
| if !ok { | ||
| return nil | ||
| } | ||
| groupID, found := resolver.FilesystemGroupForHome(home, inode) |
There was a problem hiding this comment.
When a stale physical copy leaves chunks for the same (home,inode) on another shard group that is still scanned by the chunk-prefix placement pass, resolving each chunk through the current catalog here attributes both copies to the same logical owner. That keeps len(groups) at 1, so MultiShardFiles and the critical alert miss the exact physical split they are meant to detect; carry the source group from the scan instead of recomputing it from the key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9be920ff. Placement collection now enumerates every physical ShardStore group and uses explicit-group scans, attributing each chunk to the scanned source group instead of the current route owner. Added same-key cross-group stale-copy coverage and sorted physical-group enumeration coverage.
|
Addressed all six latest-head findings on Changes:
Caller audit:
Validation:
Local full-suite note: @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9be920fff5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| meta, err := s.inodeAt(ctx, job.Inode, ts) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
When a move has reached MovePhaseSourceCleanup, switchMoveHome has already made the target home ACTIVE, so a concurrent Unlink can legally remove the file before ResumeMoveFile deletes the old source chunks. In that case this inodeAt/homeAt validation returns ErrNotFound and the move job/intent remains incomplete, leaving job.SourceHome chunks orphaned and still visible to physical scans/backup. Source cleanup needs to continue from the durable job state, or otherwise finish the job, when the inode/home disappeared after the switch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3b207a5. Source cleanup now permits the precise post-switch unlink state where both inode and home are absent, while every other missing or mismatched fence still fails. The durable job source prefix is then paged to completion. Added TestServiceResumeMoveSourceCleanupAfterConcurrentUnlink and targeted race coverage.
| opts := defaultMountOptions(options) | ||
| if recoverer, ok := adapter.core.(intentRecoverer); ok { | ||
| recoveryCtx, cancel := context.WithTimeout(context.Background(), startupRecoveryTimeout) | ||
| _, recoveryErr := recoverer.RecoverIntents(recoveryCtx, 0) |
There was a problem hiding this comment.
When more than defaultIntentRecoveryLimit unfinished move jobs survive a restart, this single RecoverIntents(..., 0) call processes only the first bounded batch and then exposes the mount; the remaining jobs stay in their migrating phases, and there is no later production caller here to resume them, so affected files can keep returning EAGAIN for writes/unlinks indefinitely. Startup should keep draining bounded batches until no recovery work remains, or schedule recovery before serving FUSE requests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3b207a5. FUSE startup now drains bounded 256-record recovery batches until a batch reports no resumed jobs, cleared completed jobs, or cleared intents. Recovery also removes completed move-job records so an earlier page cannot hide later unfinished work. Added service pagination and mount-drain regressions.
| return err | ||
| } | ||
| installFilesystemServerLifecycle(ctx, eg, cleanup, server, serveDone, config.mountPoint) | ||
| startFilesystemPlacementCollector(ctx, eg, service, *filesystemPlacementScanInterval) |
There was a problem hiding this comment.
With the FUSE mount enabled, this starts only the placement collector; no production lifecycle path schedules ReapExpiredOpenHandleLeases. If a client dies or a mount is lost after unlinking an open file, the durable !fs|ref lease eventually expires but is never removed, so the orphaned inode/chunks and their StatFS usage remain indefinitely. Start a periodic lease reaper alongside the placement collector so expired handles actually get reclaimed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3b207a5. The mounted filesystem lifecycle now starts a periodic expired open-handle lease reaper, enabled by default at 30 seconds and stopped through the shared context. It logs reclaimed refs/orphans and has a periodic lifecycle regression test.
|
Addressed all three latest-head findings on 3b207a5. Changes:
Caller audit:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b207a5c2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if completed { | ||
| if s.observer != nil { | ||
| s.observer.ObserveMoveJob(jobID, false) | ||
| } | ||
| return job, nil |
There was a problem hiding this comment.
When MoveFile/ResumeMoveFile completes during normal operation, this path returns while leaving the completed !fs|job|move|... record that was just written by the source-cleanup step. Since completed jobs are only cleared by RecoverIntents during startup recovery, a long-lived mounted server that performs successful migrations accumulates these rows indefinitely, and placement/recovery scans keep paying to decode them even though no consumer needs retained completed records. Clear the move job on this completion path before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63b15cd. ResumeMoveFile now deletes the completed durable job before returning, so normal migrations no longer accumulate completed rows. Recovery still clears legacy completed records. TestServiceMoveFileCopiesSwitchesAndCleansSource now asserts the job is absent.
| if s.observer != nil { | ||
| s.observer.ObserveMoveJob(jobID, true) | ||
| } |
There was a problem hiding this comment.
If ResumeMoveFile returns before completion — for example a stale/missing job ID from an internal retry path, context cancellation, or a terminal fence error — this active marker is never removed from FileSystemMetrics.activeMoveJobs. Because updateMoveInflightLocked takes the max of durable scan results and the active map, one failed resume can keep elastickv_fs_file_move_inflight elevated until process restart even after no durable job remains. Use a cleanup path that emits ObserveMoveJob(jobID, false) on all exits where the resume loop is no longer running.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63b15cd. Move observation cleanup is deferred immediately after the active marker is set, covering missing jobs, cancellation, fence errors, clear failures, and successful completion. Added TestServiceResumeMoveFileClearsActiveObservationOnFailure.
| if job.Phase != MovePhaseCompleted { | ||
| if _, resumeErr := s.ResumeMoveFile(ctx, job.ID); resumeErr != nil { | ||
| return stats, resumeErr |
There was a problem hiding this comment.
When two filesystem-enabled nodes run the pre-mount recovery drain at the same time, both can scan the same non-completed move job; if one node finishes the resume and clears the job before the other reaches this call, ResumeMoveFile returns ErrMoveJobNotFound and this path aborts that node's mount even though recovery already succeeded. Treat a missing job during startup recovery as already handled so concurrent drainers are idempotent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63b15cd. Startup recovery treats ErrMoveJobNotFound from ResumeMoveFile as already completed by another drainer, and recovery-record deletion retries OCC conflicts. Added a barrier-backed concurrent two-drainer race regression.
|
Addressed all three latest-head findings on 63b15cd. Changes:
Caller audit:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/bootjp/elastickv/blob/63b15cddce2d5951bd318c6b4aedb7793d101ea4//tmp/elastickv-review/internal/filesystem/service.go#L375-L381
When two filesystem-enabled servers bootstrap an empty store at the same time, both can pass the missing-root check using the same snapshot; one transaction creates the root and the other returns the wrapped store.ErrWriteConflict here, causing startFilesystemIfEnabled to abort even though the desired root now exists. Please handle this conflict by re-reading or retrying so concurrent startup remains idempotent.
https://github.com/bootjp/elastickv/blob/63b15cddce2d5951bd318c6b4aedb7793d101ea4//tmp/elastickv-review/internal/filesystem/migration.go#L131-L133
After a move switches the home but before its source-cleanup phase finishes, this active-home check allows a second MoveFile for the same inode to start. That second move bumps the inode/home epoch, so the first job later fails switchedMoveFenceMatches in recovery and leaves its old source chunks and durable job stuck; please also fence on unfinished move jobs for the inode or make source cleanup tolerate newer active homes.
https://github.com/bootjp/elastickv/blob/63b15cddce2d5951bd318c6b4aedb7793d101ea4//tmp/elastickv-review/internal/filesystem/service.go#L840
When a shrink touches more than the first chunk-delete page, the transaction above has already committed the smaller file size and partial usage delta before this paged cleanup runs with the caller's context. If the FUSE/client request is canceled or hits its deadline after the commit, the remaining beyond-EOF chunks and their usage accounting are left behind, and no background reaper scans live files for this state; please run the committed cleanup with an uncanceled/best-effort context or make it recoverable.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the three latest-head P2 findings on 99380f9. Changes:
Caller audit:
Validation:
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
## Summary - rename the completed filesystem design document from `partial` to `implemented` - align the filename with its existing implemented header and the design-document status convention ## Verification - confirmed PR #996 is merged with all required checks passing - confirmed no references remain to the old filename - pre-commit golangci-lint completed successfully Author: bootjp
1 participant
Summary
Behavior and risk
--filesystemMountis configuredMIGRATINGValidation
go test ./...go test ./adapter ./kv ./internal/fskeys ./internal/filesystem/... ./monitoring -count=1go test -racefor filesystem, routing, gRPC scan, and metrics pathsgolangci-lint --config=.golangci.yaml run --fixpromtool check configwith 5 filesystem rulesDesign doc
docs/design/2026_02_24_partial_filesystem_on_elastickv.mdAuthor: bootjp