optimize mount readiness and align local Antares integration tests#1959
Conversation
|
Claude finished @jerry609's task in 4m 24s —— View job Code Review: optimize mount readiness and align local Antares integration tests
OverviewThis is a substantial and well-motivated PR addressing two real problems: Buck2 cold-start metadata storms and FUSE operational stability. The two-phase readiness model (Phase 1: Dicfuse in-memory warmup → Ready, Phase 2: background kernel cache warm) is architecturally clean and the performance improvements claimed are compelling. The code quality is generally high with good documentation and logging. Below are specific issues found during review. 🔴 Critical Issues1.
|
| Category | Count |
|---|---|
| 🔴 Critical | 1 |
1 (missing libfuse-fs/ directory) |
|
| 🟡 Medium | 4 |
| 🟢 Low/Style | 4 |
The most important issue to address before merging is the missing libfuse-fs/ directory (build-breaking) and the .await while holding write locks (correctness + potential stuck state). The wait_for_mount_ready silent-success-on-timeout should also be addressed to improve debuggability.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.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: 90a26559e5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| [patch.crates-io] | ||
| libfuse-fs = { path = "libfuse-fs" } |
There was a problem hiding this comment.
This change adds a workspace-level [patch.crates-io] override to libfuse-fs via a local path, but the repository does not include a libfuse-fs/ directory, so Cargo cannot resolve dependencies and cargo check for scorpio fails immediately with failed to load source for dependency 'libfuse-fs'. Because this override is global, it breaks normal builds for targets that pull in rfuse3 until the patched crate is committed or the override is removed.
Useful? React with 👍 / 👎.
| Ok(resp) => { | ||
| tracing::debug!( | ||
| mount_id = mount_id, | ||
| status = %resp.status(), | ||
| "Unexpected response from mount ready check" |
There was a problem hiding this comment.
wait_for_mount_ready() treats all non-success HTTP statuses as retryable and keeps polling until the full 600s deadline, which means a stable 404/4xx response (e.g., when talking to an older Scorpio that lacks /mounts/{id}/ready) blocks for the entire timeout instead of degrading quickly. Since build() calls this readiness wait for both new and old mounts, one attempt can be delayed by roughly 20 minutes in mixed-version deployments before work proceeds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR optimizes FUSE mount readiness for the Mega monorepo system by introducing a two-phase warmup strategy and lifecycle state management. The changes address Buck2 daemon startup performance issues caused by cold FUSE caches triggering metadata storms.
Changes:
- Introduces new mount lifecycle states (
Ready,Quiescing) to decouple initial mount availability from background kernel cache warmup - Implements bounded-parallelism deep preload with configurable strategies (scan_only, hotset, dirs_only, full) and time budgets
- Fixes FUSE path encoding for special characters (
+,#) in API requests to handle crate versions like2.0.16+zstd.1.5.7 - Adds negative entry caching (5s TTL) to reduce repeated ENOENT lookups for probe paths
- Improves unmount logic with graceful→lazy fallback strategy
- Integrates Orion to wait for mount readiness before starting Buck2 builds
- Removes api-model dependency from scorpio by defining lightweight local structs
- Adds file-lock feature to rfuse3 and patches libfuse-fs for POSIX advisory lock support
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scorpio/src/manager/fetch.rs | Removes api-model dependency; defines local LatestCommitInfo struct |
| scorpio/src/dicfuse/store.rs | Adds encode_api_path() for URL encoding special characters in paths |
| scorpio/src/dicfuse/mod.rs | Improves error handling (map_err instead of unwrap); adds symlink skip logging |
| scorpio/src/dicfuse/async_io.rs | Implements negative entry cache for ENOENT; adds POSIX advisory lock stubs |
| scorpio/src/daemon/antares.rs | Adds Ready/Quiescing states; implements deep_preload_walk; adds /ready endpoint |
| scorpio/src/antares/fuse.rs | Implements graceful→lazy unmount fallback with timeout |
| scorpio/Cargo.toml | Removes api-model dependency; enables file-lock feature for rfuse3 |
| orion/src/buck_controller.rs | Adds wait_for_mount_ready() polling; replaces ls-based preheat with Rust walk |
| Cargo.toml | Adds local libfuse-fs patch (directory not included in PR) |
| fn encode_api_path(path: &str) -> String { | ||
| let clean = path.trim_start_matches('/'); | ||
| let normalized = if clean.is_empty() { | ||
| "/".to_string() | ||
| } else { | ||
| format!("/{clean}") | ||
| }; | ||
| // Percent-encode characters that are problematic in URL query strings. | ||
| // '+' → '%2B' is the critical fix; we also encode other reserved chars. | ||
| normalized.replace('+', "%2B").replace('#', "%23") | ||
| } |
There was a problem hiding this comment.
The manual URL encoding using replace() only handles + and # characters. This is incomplete and fragile—many other special characters in URLs (e.g., ?, &, =, %, space, etc.) are not handled.
Consider using url::form_urlencoded::byte_serialize() or percent_encoding::utf8_percent_encode() from the percent-encoding crate with the appropriate encoding set (e.g., NON_ALPHANUMERIC or CONTROLS) to properly encode all special characters in path components. This would be more robust and maintainable.
Example:
use percent_encoding::{utf8_percent_encode, NON_ALPHANUMERIC};
let encoded = utf8_percent_encode(clean, NON_ALPHANUMERIC).to_string();| /// * `Ok(())` - The mount is ready for heavy I/O. | ||
| /// * `Err(...)` - Timed out or failed to contact the scorpio daemon. |
There was a problem hiding this comment.
The documentation states that this function returns Err(...) on timeout, but the actual implementation returns Ok(()) when the deadline is exceeded (line 326). This is a discrepancy between the documented and actual behavior.
Update the documentation to reflect the actual behavior: timeouts are treated as non-fatal and the function returns Ok(()) to allow the build to proceed with best-effort caching.
| /// * `Ok(())` - The mount is ready for heavy I/O. | |
| /// * `Err(...)` - Timed out or failed to contact the scorpio daemon. | |
| /// * `Ok(())` - The mount is ready for heavy I/O, or the wait timed out and we | |
| /// decided to proceed anyway with best-effort caching (a warning is logged). | |
| /// * `Err(...)` - Failed to construct or use the HTTP client (e.g. invalid | |
| /// configuration) before we can complete the readiness checks. |
| [patch.crates-io] | ||
| libfuse-fs = { path = "libfuse-fs" } | ||
|
|
There was a problem hiding this comment.
The Cargo.toml adds a patch for libfuse-fs pointing to a local libfuse-fs directory, but this directory is not present in the pull request. This will cause build failures when trying to compile this code.
According to the PR description, this patch implements POSIX advisory lock support (getlk/setlk) in the libfuse-fs library's passthrough and overlayfs layers. However, the patched code must be included in the repository for the build to succeed.
Either:
- Add the
libfuse-fsdirectory with the necessary patches to this PR, or - Remove the
[patch.crates-io]section if the patch is not ready yet, and handle advisory locks differently (e.g., accept the ENOSYS behavior temporarily)
| [patch.crates-io] | |
| libfuse-fs = { path = "libfuse-fs" } |
| async fn getlk( | ||
| &self, | ||
| _req: Request, | ||
| _inode: Inode, | ||
| _fh: u64, | ||
| _lock_owner: u64, | ||
| _start: u64, | ||
| _end: u64, | ||
| start: u64, | ||
| end: u64, | ||
| _type: u32, | ||
| _pid: u32, | ||
| ) -> Result<ReplyLock> { | ||
| Err(libc::ENOSYS.into()) | ||
| // Read-only filesystem: no locks are ever held, report F_UNLCK. | ||
| Ok(ReplyLock { | ||
| start, | ||
| end, | ||
| r#type: libc::F_UNLCK as u32, | ||
| pid: 0, | ||
| }) | ||
| } |
There was a problem hiding this comment.
The getlk implementation returns F_UNLCK unconditionally, indicating no locks are held. While this is appropriate for a read-only filesystem at the Dicfuse layer, there's a conceptual mismatch: the PR description states that POSIX advisory lock support is implemented in the libfuse-fs passthrough/overlayfs layers (via the local patch), not at the Dicfuse level.
Since Dicfuse is the bottom read-only layer and the libfuse-fs overlayfs sits on top, the advisory lock operations should ideally be handled by the overlayfs layer (which the patch supposedly implements), not by Dicfuse itself. The current implementation might work but creates ambiguity about which layer is responsible for lock semantics.
Consider documenting why both layers implement lock operations, or clarifying the layering model for advisory locks in the code comments.
Signed-off-by: jerry609 <1772030600@qq.com>
jerry609
force-pushed
the
fix/buck2-daemon-timeout-v2
branch
from
90a2655 to
8f9bba3
Compare
|
Claude finished @jerry609's task in 4m 52s —— View job Code Review: optimize mount readiness and align local Antares integration tests
OverviewThis PR re-review is for the updated commit. The build-breaking ✅ Fixed Since Last ReviewMissing 🔴 Critical — Still Present1.
|
| Category | Count | Change from Previous Review |
|---|---|---|
| ✅ Fixed | 1 | Missing libfuse-fs/ directory resolved |
| 🔴 Critical | 1 | Still present (now confirmed in 3 call sites) |
| 🟡 Medium | 4 | Still present (issue #3 is newly confirmed via Codex) |
| 🟢 Low/Style | 5 | Still present |
The most important remaining issue is the .await-while-holding-write-locks pattern in delete_mount, build_cl, and clear_cl — specifically the stuck-state-on-failure risk. Addressing this and the Failed-state early exit in wait_for_mount_ready would significantly improve the operational reliability of this feature.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f9bba32c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let entry = mounts | ||
| .get(&mount_id) | ||
| .ok_or(ServiceError::NotFound(mount_id))?; | ||
| let ready = entry.state == MountLifecycle::Ready; |
There was a problem hiding this comment.
check_mount_ready() only reports ready=true for MountLifecycle::Ready, but this commit still leaves mounts in Mounted on successful rollback paths in build_cl/clear_cl (for example when remounting the old fuse after a CL switch failure). In that state the filesystem is usable, yet /mounts/{id}/ready will stay false forever, so clients that poll readiness (including Orion’s wait_for_mount_ready) can block until the full timeout after a recoverable CL-switch error. Consider either mapping Mounted to ready here or transitioning those rollback states to Ready.
Useful? React with 👍 / 👎.
|
Claude finished @Ivanbeethoven's task in 3m 39s —— View job Code Review: optimize mount readiness and align local Antares integration tests
OverviewThis is a re-review of the latest commit. Two previous review cycles have been completed. Below is the current status of all flagged issues, with a newly confirmed bug added from the Codex review. ✅ Fixed Since Previous ReviewsMissing 🔴 Critical — Still Present (3rd Review)1.
|
| Category | Count | Change from Previous Review |
|---|---|---|
| ✅ Fixed | 1 | libfuse-fs/ directory resolved (1st review); total fixed = 1 |
| 🔴 Critical | 1 | Still present (3rd review, root cause now confirmed: only blocking create_dir_all) |
| 🟡 Medium | 4 | Issue #2 (Mounted rollback state) newly confirmed from Codex note |
| 🟢 Low/Style | 4 | Still present |
The highest-priority items to address before merging:
.await-while-holding-write-locks indelete_mount/build_cl/clear_cl— stuck-state risk oncreate_dir_allfailureMountedrollback state not treated as ready — causes 600s timeout after any recoverable CL-switch errorwait_for_mount_readysilent proceed onFailedstate — obscures mount failures as build errors
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ea281f0a2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "scan" | "scan_only" | "readdirplus" => DeepPreloadMode::ScanOnly, | ||
| "full" => DeepPreloadMode::Full, | ||
| "dirs" | "dirs_only" => DeepPreloadMode::DirsOnly, | ||
| "hotset" | "" => DeepPreloadMode::Hotset, |
There was a problem hiding this comment.
deep_preload_mode() treats an explicitly empty ANTARES_DEEP_PRELOAD_MODE value as Hotset ("hotset" | ""), even though the documented/default behavior is scan_only. In environments where templating injects empty env vars, this silently enables extra metadata touching and can materially increase background mount warmup cost compared to the intended default.
Useful? React with 👍 / 👎.
| }; | ||
| local_entries += 1; | ||
|
|
||
| if file_type.is_dir() && depth < max_depth { |
There was a problem hiding this comment.
The depth gate currently enqueues subdirectories when depth < max_depth while traversal starts with root at depth 0, so the walker still scans one level deeper than configured (and ANTARES_DEEP_PRELOAD_MAX_DEPTH=0 still scans all root entries). This weakens the operator-facing depth limit and can cause unexpectedly heavy background traversal on large trees.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
scorpio/src/manager/fetch.rs:817
fetch_parent_commit()interpolatespathdirectly into thepath=query parameter without percent-encoding. If repo paths can contain reserved characters (notably+), the server may parse the path incorrectly (e.g.+as space). Construct the URL with proper query encoding.
let url = format!(
"{}/api/v1/latest-commit?path=/{}",
config::base_url(),
path.trim_start_matches('/')
);
| // Percent-encode characters that are problematic in URL query strings. | ||
| // '+' → '%2B' is the critical fix; we also encode other reserved chars. | ||
| normalized.replace('+', "%2B").replace('#', "%23") |
There was a problem hiding this comment.
encode_api_path() only replaces + and #, but the value is used directly in a query string. Paths containing other reserved characters (e.g. &, =, %, whitespace) will produce malformed URLs or change query semantics. Use a real query-component encoder (e.g. url::Url + query_pairs_mut() / form_urlencoded / percent_encoding) instead of manual replace().
| let (lock, cv) = &*state; | ||
| let mut guard = lock.lock().expect("deep_preload_walk lock poisoned"); | ||
| guard.done = true; |
There was a problem hiding this comment.
deep_preload_walk() uses lock.lock().expect("... lock poisoned") (and similar expect()s elsewhere in this function). Since this warmup is best-effort, a poisoned mutex should not panic/crash the worker threads; recover via PoisonError::into_inner() (and ideally mark the walk as done/cancelled) instead of expect().
| /// Lightweight mirror of the server's latest-commit response so that scorpio | ||
| /// does not depend on the `api-model` crate for a single deserialization site. | ||
| #[derive(serde::Deserialize)] | ||
| struct LatestCommitInfo { |
There was a problem hiding this comment.
While touching this area: fetch_tree() builds a ?path= query URL by concatenating the raw repo path (via tree_file_endpoint() + clean_path). This will misbehave for paths containing reserved query characters (e.g. +, #, &). Consider switching both fetch_tree() and fetch_parent_commit() to construct URLs via proper query percent-encoding (shared helper).
背景与目标
Buck2 存在两类问题：
目标：降低启动路径时延、提升挂载切换安全性、补齐兼容性能力。
关键改动
Scorpio
挂载状态解耦
Ready与Quiescing生命周期。Ready只依赖 Phase 1（目录缓存可用），不再被深度预热阻塞。GET /mounts/{id}/ready供 Orion 轮询。预热机制收敛
deep_preload_walk改为有界并行、可取消任务。CL 切换安全性
build_cl/clear_cl进入Quiescing，短暂 grace 后再执行切换。FUSE 行为修复
+/#路径异常。libfuse-fs本地 patch 中实现getlk/setlk（passthrough 层直接调用底层fcntl()，overlayfs/unionfs 层路由到底层），解决 Buck2 SQLite advisory lock 的ENOSYS问题。fusermount -u，失败再回退-uz，降低 lazy unmount 带来的句柄风险。Orion
wait_for_mount_ready()，在build()中对 old/new 两个挂载等待 ready。preheat()改为 Rust 原生浅层预热（替代ls -lR），降低不可控开销。libfuse-fs 本地 Patch
通过
[patch.crates-io]指向本地libfuse-fs，修复上游缺失的 POSIX advisory lock 支持：getlk/setlk，调用底层文件描述符的fcntl(F_GETLK/F_SETLK/F_SETLKW)，将锁透明转发到宿主文件系统。新增参数说明
ANTARES_DEEP_PRELOAD_WORKERSclamp(cpu_count, 2, 8)ANTARES_DEEP_PRELOAD_MODEscan_onlyscan_only/hotset/dirs_only/fullANTARES_DEEP_PRELOAD_MAX_DEPTH4ANTARES_DEEP_PRELOAD_MAX_MS80000=不限制ANTARES_CL_QUIESCE_GRACE_MS150性能优化
预热阶段说明
挂载可用分为两个阶段：
load_dir_depth()（网络→内存）statx穿透到远端 HTTPdeep_preload_walk()（FUSE 挂载点遍历）优化策略：
Ready状态仅依赖 Phase 1 完成，Phase 2 改为后台 best-effort 任务，不再阻塞主流程。当前效率（实测数据，同机同仓约 1615 entries）
mount -> Ready(Phase 1)结论：主链路
Ready稳定在毫秒级，不再阻塞构建启动；后台预热从分钟级降至约 9 秒，且不阻塞主流程。版本替换友好性
向后兼容：
/ready为可选轮询）wait_for_mount_ready()时，行为降级为“不等 Ready 直接构建”（与优化前一致，会报错）可配置降级：
ANTARES_DEEP_PRELOAD_WORKERS=1可回退到单线程ANTARES_DEEP_PRELOAD_MAX_MS=0可取消时间预算限制ANTARES_DEEP_PRELOAD_MODE=full可恢复全量 metadata 预热渐进式部署：
Mounted/Unmounted状态流转测试与验证
cargo check/test（scorpio、orion）通过buck2 targets //...→ 2772 targets 验证通过风险与回滚
workers=1、mode=full）wait_for_mount_ready()调用，行为与旧版一致[patch.crates-io]配置即可回退到上游版本（会重新出现ENOSYS问题）