Repository navigation
fix(coordinator): retain in-flight ownership and isolate metadata waits - #202
Conversation
Acquire the existing inspection mutex before reading the header compared by snapshot CAS, so independent reference writers do not queue with a header invalidated by earlier writers in the same Session. Preserve artifact lock order, epoch/source/record/catalog checks, global revision advancement, cache invalidation and the existing retry budget. Cover concurrent acquisition and release of512 distinct snapshots against real Redis, retaining all original references and record states. Signed-off-by: hky1999 <keyang.hu@qq.com>
|
head_sha: 变更摘要本 PR 修复 主要改动
|
|
head_sha: 代码审查审查结论逐文件审查结果
发现清单
总体风险判断锁位置调整方向正确、未引入内存/panic/死锁类回归,测试用例本身可编译且断言合理;主要风险是修复只覆盖了无卷分支,带卷快照的并发争用(本 PR 的核心症状)依然可达,且没有对应测试覆盖。整体风险中等,建议在合并前补齐带卷分支的锁内 header 一致性校验。
💬 仅评论 |
| let mut inventory = self.inspection.lock().await; | ||
| let header_value = match catalog { | ||
| Some(catalog) => Some(catalog.header_value.clone()), | ||
| None => self.store.fields([HEADER.into()]).await?[0].clone(), |
There was a problem hiding this comment.
head_sha: 9e25831a7c7b2128d727eac92af153d92adc0518
🟡 Medium Priority
变更行(snapshots.rs:46 新增 let mut inventory = self.inspection.lock().await;)把 header 读取移动到 inspection 互斥区之后,但只有 catalog == None 的分支真正受益:第 49 行在锁内重新 fields([HEADER]);而第 48 行 Some(catalog) => Some(catalog.header_value.clone()) 仍使用调用方在加锁之前取得的 catalog 快照(publish_snapshot_checked 第 188 行、mutate_snapshot 第 266 行调用 volume_catalog();volumes.rs:891 记录当次 header_value)。
契约与失败链:EVAL 脚本第 65 行用 ARGV[6](即 ARGV 来自第 93 行的 header_value)与 Redis 当前 header 比较;而带卷快照的提交在同一事务里会写入 catalog.advanced_header()(第 53、72 行),因此任意一个带卷提交都会推进共享 header。当 512 个带卷快照并发 acquire/release 时,某个等待者在排队期间,header 已被排在它前面的写者推进;它拿到的 ARGV[6] 已过期 → 脚本 return 0 → 本函数返回 Ok(false);mutate_snapshot 只能重试 ATTEMPTS = 32(storage.rs:102)次,全部失败后返回 Error::Unavailable("snapshot reference contention")。这正是本 PR 声称修复的同一症状,且此次改动让两条分支产生语义分叉(无卷分支读写锁内 header,带卷分支读写锁外 header),与本行注释「Read the compared header after joining that queue」的声明不一致。
建议:在取得 inspection 锁后,对 catalog 分支校验 catalog.header_value 是否仍等于当前 header;不一致则返回 Ok(false) 交给上层重试(避免用过期 token 发起必然失败的 EVAL),并补充带 volumes 模板的并发回归用例。若要保持语义统一,可让两个分支都在锁内使用同一份「锁内读取的 header」。
There was a problem hiding this comment.
这条旧 head 的带 Volume catalog 陈旧问题已在后续提交中修复,并在当前 e05b61b8 保留。snapshot_cas_with_source 先进入 inspection 提交队列,再在锁内读取完整 volume_catalog 并生成 protect/release 变更和对应 header。不是仅刷新旧 catalog 的 header,因此不会用过期的 Volume 目录覆盖前一个写者已提交的快照 hold;原有 32 次重试预算和 CAS/epoch/源身份检查保留。
已在最新 master(含 #195/#208)上重跑 219 项 Coordinator/快照/存储/Volume 测试,包括真实 Redis 的 64 并发 Volume 快照发布/删除、64 个共享 Volume 首次发布,以及 512 个工件结果发布和快照引用竞争回归。84 项真实 Redis/mTLS control-RPC 测试另行通过(其中 21 项 library 用例与前组重叠,不合并成唯一测试数)。完整 workspace all-target/all-feature Clippy、格式、文档和 diff 检查通过。原 1d57793 的准备阶段错误与未知写结果区分、#206/#209 和新 796bdc8 的按节点过滤后复制修复均保留。
Signed-off-by: 月可 <keyang.hu@qq.com>
Read compared metadata after acquiring the existing object publication and inspection locks. Preserve artifact/object/inspection lock ordering, epoch and record fences, receipts, and the existing retry budget. Native restore remains outside this metadata queue. Signed-off-by: 月可 <keyang.hu@qq.com>
Signed-off-by: 月可 <keyang.hu@qq.com>
Integrate the PR openJiuwen-ai#206 liveness fix on the latest PR openJiuwen-ai#202 head without reverting its preparation/write boundary or fresh Volume catalog updates. Snapshot storage waits no longer hold the global scheduler mutex. Keep source-record and node-session fences, per-artifact publication ownership, and exact-owner checks when releasing consumed snapshot references. Retain the initial Running publication and Volume concurrency regressions. Validate with isolated real Redis and the maintained mTLS control-RPC suite, including stalled writes, cancellation and replaced node sessions. Signed-off-by: 月可 <hukeyang.hky@antgroup.com>
|
Given the scope of these changes, has end-to-end regression testing been performed on a deployed cluster? If so, could you share the tested commit, scenarios, and results? In particular, it would be helpful to cover concurrent snapshot and Volume operations, node heartbeats under Redis delays, and recovery after Redis or node disruptions. |
Signed-off-by: 月可 <hukeyang.hky@antgroup.com>
|
Replying to the deployed-cluster regression question: the final current head has not yet completed the requested deployed-cluster fault matrix, so I am not claiming that qualification. Current head is e05b61b8, synced with master 5ff832e and preserving 1d57793, #206/#209 and 796bdc8. On this exact clean source:
There are earlier deployed integration results, but they must be kept separate: source Remaining qualification is the final combined deployed candidate under concurrent snapshots/Volumes, injected Redis delay/disruption and node restart/loss, including original-process/data continuity and cleanup. This review follow-up changed no cluster deployment and leaves that acceptance gap explicit rather than presenting local component tests or an older cluster run as final-head E2E proof. Review threads remain open for verification. |
Signed-off-by: 月可 <hukeyang.hky@antgroup.com>
Signed-off-by: 月可 <hukeyang.hky@antgroup.com>
Signed-off-by: 月可 <hukeyang.hky@antgroup.com>
81a74bc
into
openJiuwen-ai:master
|
🤖 This pull request has been merged, and the target branch is up to date. |
[mirror] #202: fix(coordinator): retain in-flight ownership and isolate metadata waits Created-by: openjiuwen-sync Commit-by: 月可;hky1999 Merged-by: robbluo Description: Paired: [GitHub #202](#202) ↔ [GitCode !156](https://gitcode.com/openJiuwen/agent-dx/merge_requests/156) <!-- bot1-mirror-meta --> github_pr: 202 head_repo: hky1999/agent-dx head_ref: hky1999/snapshot-reference-contention-20261104 head_sha: abd6cea base_repo: openJiuwen-ai/agent-dx base_ref: master github_repo: openJiuwen-ai/agent-dx base_branch: master base_sha: ec4d210 github_user: hky1999 <!-- /bot1-mirror-meta --> GitHub PR mirror for CodeArts CI. <!-- bot1-source-body --> Snapshot/Volume publication prepared metadata before entering its commit queue and waited for Redis under the scheduler mutex. Concurrent captures could therefore lose reference updates, exhaust CAS retries and delay heartbeats. This change retains in-flight ownership, moves ordinary metadata waits off that mutex, and reuses the committed snapshot inventory instead of transferring and decoding all historical tombstones on every publication/list/GC pass. ## Changes - Read the complete snapshot/Volume catalog and compared header after queue admission. Preserve the original epoch, source-record, node-session, immutable-identity, receipt and CAS checks and retry budget. - Serialize initial artifact/Volume publication preparation in consistent lock order. Keep runtime work outside the metadata queue. Preparation failures before EVAL do not dirty publication state; ambiguous submitted writes require authoritative repair. - Keep snapshot publication/get/list/delete, node inspection and collector I/O off the global scheduler mutex. Readiness checks do not wait for a cache writer. - Retain valid same-process dispatched reservations during catalog repair; persisted records win and replacement/retired sessions are excluded. Deadlines/resources and fresh-process recovery semantics remain unchanged. - Filter committed snapshot inventory before cloning. Ownership publication borrows records to test the same storage/location predicate; Deleting still owns its artifact. An unavailable inventory cannot mean “unowned.” Standalone storage sessions without an initialized view retain their durable reads. - Fence the cached view by Redis transport generation. Reconnection/header success does not revalidate cached ownership; in-flight writers cannot restore an earlier-generation view, and a recovery crossing transport replacement fails closed. - Include master `ec4d2108`, which already contains #181, #195 and #208. ## Validation Current PR head: `abd6ceadfc4260c07c90e722b0537e39a526b2c9`. [Current-head CodeCheck](#202 (comment)) passed all five configured checks. UT/ST/package build are N/A in that report; executable evidence is separate. Clean combined source `7a0614b43078c96ff9e80bcacace129374e49503` integrates this PR and [!153](https://gitcode.com/openJiuwen/agent-dx/merge_requests/153) `3432d411`. Coordinator, platform/shared crates and the snapshot contract are byte-identical to this PR head; ADXlet is byte-identical to [!153](https://gitcode.com/openJiuwen/agent-dx/merge_requests/153). Executed gates: - **224 Coordinator tests**, including isolated real Redis contention, reservation/session fencing, write ambiguity, snapshot/Volume protection and allocation budgets. - **457 ADXlet tests**; one existing dedicated empty-Sandboxd integration remains ignored. - **88 official real Redis/mTLS control-RPC tests**; 25 library cases overlap the Coordinator group. - Formatting, full workspace/all-target/all-feature Clippy with warnings denied, documentation and diff checks pass, with unchanged source hashes. - RED reproduced redundant HVALS reads and stale ownership after real Redis restart. The hot-path, reconnect and in-flight-writer fencing regressions pass. - Fresh complete maintained platform/static-musl Execd/EROFS archives were built and verified from the same clean source. ## Current deployed fault acceptance: passed The single HK1019 deployment uses immutable framework `sha256:fb78c1fea551230feeac0c9d3da3ec846a154c6fddddaddd96b7da65108112ad` on all 13 workers and Coordinator/API/Redis. Actual running executable hashes were checked. Framework binary-build source is AKernel `478aef4d`; chart source is separately `092f2f27`. Managed tools `sha256:fbed1c21…` are independently pinned. Redis PVC and the sole public API/Gateway entry were preserved. This is combined-candidate evidence, not a bare-PR build or formal release approval. Using public native SDK/HTTP APIs: - **16 live environments**, each with an exclusive directory Volume, 64MiB high-entropy touched heap, 32MiB private file, open FD offset and established framed TCP stream. - Two synchronized 16-way batches created **32 reusable snapshots**, with a 1.5s Redis write pause and 16 concurrent unrelated Volume creations in the first batch. All 32 calls returned 200. Replaying one exact request ID/body returned the same immutable snapshot. - All 16 referenced-Volume deletions were rejected. Recreating a Volume produced a new UID; the stale UID was rejected with **409 CONFLICT, outcome=not_started, retry=never**, without replacement occupancy or references. - Replaced the existing Redis Pod using the same PVC, restarted the existing Coordinator, then replaced one worker Pod. Sources remained usable after Redis/Coordinator recovery. After worker session loss, all **16 original-process restores** proved PID/start time, nonce, heap hash, FD offset, framed TCP, private-file hash and Volume UID/data. - Cleanup reached **0 test environments and 0 Volumes**. All 32 snapshot tombstones were Deleted with empty references, and all 32 physical checkpoint directories were absent. Temporary domain/data/configuration were removed; the exact **118 baseline snapshot IDs** remained and all 13 workers were healthy. The first current-package fault attempt stopped before injection because an operator projection exported unrelated historical tombstones and exceeded the exec-output frame. That failure was retained; the fresh passing attempt uses bounded explicit HGET projections. Product code and fault assertions did not change. Scope: same-node native rootfs/runtime snapshots and real directory Volumes. Redis replacement was requested during the second capture batch; exact overlap of an individual EVAL with disconnect was not established. Committed baseline had WAITAOF; AOF everysec stayed unchanged. This does not qualify forced power-loss/ambiguous-EVAL durability, storage-side split-brain fencing, managed image-collection daemon/host restart, cross-node portability or artifact GC. Forged stale-session RPC negatives remain component/RPC evidence; the cluster negative used the public stale-UID API. ## Current 1024 concurrent C/R qualification: passed The same immutable deployment completed two globally synchronized rounds over **1024 resident subjects**: **2048 captures, 2048 restore API successes and 2048 complete original-process proofs**. All eight clients exited 0. Cleanup debt is 0, all 2048 physical snapshot directories were absent, and the exact 118 baseline snapshots remained. | Public SDK boundary | p50 | p95 | Maximum | | --- | --- | --- | --- | | Checkpoint | 33.321s | 36.553s | 38.307s | | Restore API | 2.284s | 5.231s | 7.075s | | Complete payload proof | 15.114s | 22.629s | 24.879s | Complete proof includes bounded client verification after all restore APIs return. It is separate from runtime restore latency. Each subject has 64MiB random touched heap and a 32MiB private upper file; requests are 10mCPU/64MiB/512MiB, with unchanged 2CPU/8GiB/20GiB hard caps. Captures are whole-sandbox runtime calls, without guest Python or application signals. Fourteen node samples and 16 control-plane samples had no collection errors or new observed OOM kills. Sampled peaks: Coordinator 504.31MiB, Redis 124.58MiB, busiest worker sandbox-parent 21.266GiB. Service Pod UIDs did not change during the cohort. Original Python-free 0774 net-stats and original kv-live-surgery also passed mixed Harbor Oracle plus independent verifiers (reward 1 each), two captures and original-process restore per case. Compose main/loadgen identities and a one-launch count were retained. No real model inference or Claude usage-accounting claim is made for this Oracle run. ## Failure history and remaining gates Earlier 1024 requalification on framework f86b7d3b failed after 2047 captures and 1024 restore proofs: a 300s snapshot budget was truncated by the generic 15s catalog-publication RPC timeout. The remaining-budget repair is independently in [!153](https://gitcode.com/openJiuwen/agent-dx/merge_requests/153) `3432d411`, with real-gRPC RED/GREEN tests. The current matched-package two-round pass above qualifies that integrated fix; it does not erase the old failure. Older capacity/fault evidence retains its original artifact identity. An independent cross-version EROFS identity failure was corrected by optional runtime-artifact pinning in [AKernel #148](inclusionAI/AKernel#148). A snapshot captured under actual old services then restored under the new services with the original process proof. This preserves one qualified runtime/Execd tuple, not arbitrary runtime migration or all historical snapshot compatibility. Original-Dockerfile/context startup remains a separate pending gate: the latest 128-workflow cohort reached 127/128 before native export stalled. HK BuildKit has not been changed. The independently reproduced converter cancellation fix in [Nydus Draft #817](containerd/nydus-snapshotter#817) is not deployed and is not claimed to explain the entire stall. The 1024 C/R pass establishes a tested waterline, not an absolute maximum or a Dockerfile-startup result. <!-- /bot1-source-body --> See merge request: openJiuwen/agent-dx!156
Paired: GitHub #202 ↔ GitCode !156
Snapshot/Volume publication prepared metadata before entering its commit queue and waited for Redis under the scheduler mutex. Concurrent captures could therefore lose reference updates, exhaust CAS retries and delay heartbeats. This change retains in-flight ownership, moves ordinary metadata waits off that mutex, and reuses the committed snapshot inventory instead of transferring and decoding all historical tombstones on every publication/list/GC pass.
Changes
ec4d2108, which already contains fix(sdk): preserve direct transport failures and initial attempt errors #181, fix(snapshot): inherit captured process geometry and execution limits #195 and fix(adxlet): reconcile failed executions with unsealed captures #208.Validation
Current PR head:
abd6ceadfc4260c07c90e722b0537e39a526b2c9. Current-head CodeCheck passed all five configured checks. UT/ST/package build are N/A in that report; executable evidence is separate.Clean combined source
7a0614b43078c96ff9e80bcacace129374e49503integrates this PR and #1993432d411. Coordinator, platform/shared crates and the snapshot contract are byte-identical to this PR head; ADXlet is byte-identical to #199. Executed gates:Current deployed fault acceptance: passed
The single HK1019 deployment uses immutable framework
sha256:fb78c1fea551230feeac0c9d3da3ec846a154c6fddddaddd96b7da65108112adon all 13 workers and Coordinator/API/Redis. Actual running executable hashes were checked. Framework binary-build source is AKernel478aef4d; chart source is separately092f2f27. Managed toolssha256:fbed1c21…are independently pinned. Redis PVC and the sole public API/Gateway entry were preserved. This is combined-candidate evidence, not a bare-PR build or formal release approval.Using public native SDK/HTTP APIs:
The first current-package fault attempt stopped before injection because an operator projection exported unrelated historical tombstones and exceeded the exec-output frame. That failure was retained; the fresh passing attempt uses bounded explicit HGET projections. Product code and fault assertions did not change.
Scope: same-node native rootfs/runtime snapshots and real directory Volumes. Redis replacement was requested during the second capture batch; exact overlap of an individual EVAL with disconnect was not established. Committed baseline had WAITAOF; AOF everysec stayed unchanged. This does not qualify forced power-loss/ambiguous-EVAL durability, storage-side split-brain fencing, managed image-collection daemon/host restart, cross-node portability or artifact GC. Forged stale-session RPC negatives remain component/RPC evidence; the cluster negative used the public stale-UID API.
Current 1024 concurrent C/R qualification: passed
The same immutable deployment completed two globally synchronized rounds over 1024 resident subjects: 2048 captures, 2048 restore API successes and 2048 complete original-process proofs. All eight clients exited 0. Cleanup debt is 0, all 2048 physical snapshot directories were absent, and the exact 118 baseline snapshots remained.
Complete proof includes bounded client verification after all restore APIs return. It is separate from runtime restore latency. Each subject has 64MiB random touched heap and a 32MiB private upper file; requests are 10mCPU/64MiB/512MiB, with unchanged 2CPU/8GiB/20GiB hard caps. Captures are whole-sandbox runtime calls, without guest Python or application signals.
Fourteen node samples and 16 control-plane samples had no collection errors or new observed OOM kills. Sampled peaks: Coordinator 504.31MiB, Redis 124.58MiB, busiest worker sandbox-parent 21.266GiB. Service Pod UIDs did not change during the cohort.
Original Python-free 0774 net-stats and original kv-live-surgery also passed mixed Harbor Oracle plus independent verifiers (reward 1 each), two captures and original-process restore per case. Compose main/loadgen identities and a one-launch count were retained. No real model inference or Claude usage-accounting claim is made for this Oracle run.
Failure history and remaining gates
Earlier 1024 requalification on framework f86b7d3b failed after 2047 captures and 1024 restore proofs: a 300s snapshot budget was truncated by the generic 15s catalog-publication RPC timeout. The remaining-budget repair is independently in #199
3432d411, with real-gRPC RED/GREEN tests. The current matched-package two-round pass above qualifies that integrated fix; it does not erase the old failure. Older capacity/fault evidence retains its original artifact identity.An independent cross-version EROFS identity failure was corrected by optional runtime-artifact pinning in AKernel #148. A snapshot captured under actual old services then restored under the new services with the original process proof. This preserves one qualified runtime/Execd tuple, not arbitrary runtime migration or all historical snapshot compatibility.
Original-Dockerfile/context startup remains a separate pending gate: the latest 128-workflow cohort reached 127/128 before native export stalled. HK BuildKit has not been changed. The independently reproduced converter cancellation fix in Nydus Draft #817 is not deployed and is not claimed to explain the entire stall. The 1024 C/R pass establishes a tested waterline, not an absolute maximum or a Dockerfile-startup result.