fix(edge): make registry claim settlement idempotent - #663
Conversation
XuPeng-SH
left a comment
There was a problem hiding this comment.
Request changes. The direction is good—durable ownership, generation fencing, postcondition-based idempotence, and bounded retries—but the fix is not yet systemic enough to merge.
Blocking findings
- First-registration rollback still treats a possibly stale empty read as success (P1).
The rollback classifier maps (previous=None, row=None) to AlreadyApplied, and the retry path explicitly excludes this case.
A concrete failure sequence is:
INSERT(state=0, claim=A) commits → rollback opens another session → visibility lag returns None → rollback returns Ok(true) → the row later becomes visible still as state=0, claim=A.
That row is not routable, and its live claim blocks a new registration for up to 120 seconds. This is the same cross-session visibility problem the PR is intended to fix. Retry the empty read for first-registration rollback as well; only classify it as idempotent success after the bounded final attempt. Add a regression test for None → owned state=0 → deleted.
- The original zero-row ambiguity remains in heartbeat and unregister (P1).
Heartbeat still treats rows_affected() == 0 as definitive supersession. Unregister still returns rows_affected() > 0 directly, and the runtime helper stops on Ok(false).
Therefore the same MatrixOne visibility behavior can still:
- close a healthy current Edge on a heartbeat;
- leave a stale routable registry row after cleanup;
- contradict the release error policy: release failure keeps the local connection, but a finalized
state=2row causes the next heartbeat to classify it as superseded.
Please apply the same authoritative read/postcondition classification to every fenced ownership operation, not only finalize/release/rollback. Superseded should mean a current, verified different generation—not merely zero affected rows.
Important design/test gaps
-
The tests do not deterministically reproduce the reported failure. The new unit tests cover the classifier and the DB tests cover ordinary sequential repeats, but not stale/empty reads, zero-row mutations, first-registration rollback, release outcome-unknown followed by heartbeat, or unregister visibility races. A small injectable storage/settlement seam or scripted observation sequence would make this regression-proof.
-
The “different claim means superseded” proof depends on database semantics that are not encoded.
claim_idis a random UUID, so it has no ordering. If an old snapshot exposes a predecessor claim, the code cannot distinguish predecessor from successor. MatrixOne documents thatSELECT FOR UPDATEis not a universal serialization barrier in optimistic transactions: https://github.com/matrixorigin/matrixone/blob/1092ab739c120052fff0b8d1ff27854663cd098f/pkg/frontend/databranchutils/lineage_publication_lock.go#L17-L27. Either make the required pessimistic/current-read deployment contract explicit and enforce it, or use a monotonic generation/version for fencing. -
The public
Result<bool, String>contract is too lossy for enterprise failure handling. The implementation internally hasAlreadyApplied / Apply / Superseded, but callers also need an outcome-unknown/storage-failure distinction. The trait docs also overpromise rollback semantics for non-durable backends. A typed settlement outcome would prevent callers from turning uncertainty into user-visible disconnects.
The current WS path also sends AuthOk before durable release and the Edge client ignores a later AuthError; a release failure is therefore experienced as an unexplained reconnect rather than a precise degraded-state signal. This can be a follow-up, but should be tracked.
The current head CI is green, but the PR is behind main; after addressing the above, please rebase/squash and rerun the focused edge/database tests.
11c2fb5 to
3fea452
Compare
|
Addressed the requested ownership review on the latest
Verification: format and diff checks passed; |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Request changes on the latest head (3fea452). The new settlement loop correctly stops treating every zero-row result as immediate supersession, and the focused tests pass, but two ownership gaps remain.
Blocking findings
- P1 — A row-local no-op UPDATE cannot prove absence, so rollback/unregister can still report success while the durable row survives.
establish_registry_current_read and establish_generation_current_read issue the barrier against the same registry row whose visibility is uncertain. When that row is absent from the transaction snapshot, the UPDATE matches nothing and contributes no stable-row write to validate at commit. The MatrixOne pattern this is based on deliberately updates a bootstrap-created row that is guaranteed to exist before any protected object: https://github.com/matrixorigin/matrixone/blob/1092ab739c120052fff0b8d1ff27854663cd098f/pkg/frontend/databranchutils/lineage_publication_lock.go#L17-L35
Consequently this sequence is still possible:
row/claim commits in session A → DELETE/UPDATE reports 0 in session B → all six row-local barriers and reads see None → the final transaction commits without a conflicting row write → cleanup returns success → the committed row later becomes visible.
The code then turns final absence into GenerationMutationOutcome::Absent and unregister_generation maps that to Ok(true). First-registration rollback has the same issue by mapping the last None to AlreadyApplied. Bounded waiting reduces probability; it is not proof of the postcondition.
Please serialize through a guaranteed-existing owner/sentinel row (or a durable monotonic/tombstone generation), or preserve final absence as OutcomeUnknown rather than successful cleanup. Add a test that exercises the actual async settlement/storage seam; the current first_registration_rollback... unit test only calls the pure classifier with hand-supplied values and cannot validate the barrier or mutation behavior.
- P1 — Release outcome-unknown leaves an authenticated healthy Edge indefinitely invisible to cross-pod routing.
After pool commit, release_registration errors are only logged and the connection remains active. This patch makes heartbeat accept the owned state=2 row, but find_by_agent_id_and_workspace and list_by_user expose only state=1. Claim expiry does not transition state=2 to state=1, and no reconciliation path retries release. If the failed release truly did not apply, the user receives AuthOk, same-pod routing may work, and cross-pod routing silently fails for the lifetime of the socket.
This is not precise degradation and breaks the single durable provider view. Keep/reconcile the lease until durable publication is known, or expose an explicit degraded/not-ready state and a self-healing path. Add an unhappy-path test for finalize succeeds → pool commit → release does not apply/returns unknown → cross-pod lookup eventually converges or the connection is explicitly rejected/degraded.
Verification
cargo test -p astra-services edge_registry --libon the PR head: 10 passed.- Current CI is green, but the PR is 6 commits behind
main; rebase and rerun the focused database/runtime tests after fixing the above.
3fea452 to
b75b376
Compare
|
Addressed both P1 findings on rebased head
The normal paths are unchanged in database cost: heartbeat/unregister still use one SQL, and release still uses one transition. The added settlement reads and release retries execute only after an ambiguous miss/storage error. Verification on this head:
The PR is rebased onto current |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Request changes on the latest head (7ec3f236). The previous absence-as-success and release-reconciliation findings are materially improved, but the current state machine still has one availability race and one enterprise data-retention regression.
Blocking findings
- P1 — predecessor cleanup can erase a live successor claim and reject the healthy reconnect.
claim_registration deliberately leaves the published predecessor edge_id and state unchanged while it installs the successor claim. But unregister_generation matches only that predecessor edge_id, then clears registration_claim_id and changes the row to state 0. It returns on the affected-row fast path, so none of the new settlement checks run.
A normal cross-pod sequence is therefore:
A published (state=1, edge=A) → B claims reconnect (state=1, edge=A, claim=B) → A disconnects and unregisters → the UPDATE clears claim B and writes state 0 → B finalizes → claim mismatch is classified as superseded → both connections are unavailable.
The symmetric race after B finalizes also needs treatment: A cleanup is classified as superseded, but if B then rolls back, Rollback restores A to state 1 even though A has already disconnected.
Please model predecessor liveness through the whole claim lifecycle. Merely adding registration_claim_id IS NULL to unregister avoids erasing B but still allows rollback to resurrect disconnected A. Add deterministic coverage for at least claim B → unregister A → finalize/release B and finalize B → unregister A → rollback B; the first must publish B, and the second must not republish A.
- P2 — the new durable tombstone retains private Edge metadata indefinitely.
The old disconnect path deleted the row. The new unregister only clears claim/state fields, leaving hostname, worktree_path, capabilities_json, and workspace_id intact; first-registration rollback explicitly persists the same metadata while writing state 0. There is no production tombstone GC or explicit deletion path. For enterprise runners this silently changes a transient private path/capability record into indefinite retention.
Keeping a skeletal state-0 owner row for fencing is reasonable, but deactivate it by scrubbing fields not required for identity/idempotence (or define and implement an explicit bounded retention/deletion contract that preserves the fencing proof). Add a DB assertion that inactive rows do not retain private operational metadata and that later registration still reuses the owner safely.
Verification gap
The current CI is green, but the added DB cases are ignored and the PR description says the live MatrixOne suite was not run on this head. The pure classifier tests cannot exercise either the cross-session visibility contract or the predecessor-disconnect races above. Please run the updated current head against MatrixOne and include an actual two-session/concurrent regression at the storage boundary.
The branch is also two commits behind main; rebase after the state-machine changes and rerun the focused registry and WebSocket suites.
7ec3f23 to
a68a863
Compare
|
Both findings are valid and fixed on current-main head
Added current-head MatrixOne 4.1.2 coverage with independent predecessor/successor pools for both requested interleavings:
The live DB suite also asserts metadata scrubbing and later owner-row reuse: all 12 The branch is rebased and squashed onto latest |
XuPeng-SH
left a comment
There was a problem hiding this comment.
结论:Request changes
当前 head a68a863e 的方向是正确的:幂等操作基于 postcondition、明确区分 superseded/unknown、保留非路由 tombstone、清理私有元数据,都符合 Astra 的企业运行时设计。但从第一性原则和企业 unhappy path 看,仍有以下阻塞问题。
1. P1 — 不兼容滚动升级,旧实例会重新引入本 PR 修复的竞争条件
新版本获取 successor claim 时保留 predecessor 的 edge_id,让旧连接继续服务:
Astra/crates/services/src/multi_agent/edge_registry.rs
Lines 1056 to 1086 in a68a863
但旧版本断连时会直接删除匹配 edge_id 的整行:
Astra/crates/services/src/multi_agent/edge_registry.rs
Lines 762 to 779 in 5d3275d
正常滚动升级中可能发生:
旧实例 A 已发布 → 新实例 B 获取 claim,DB 仍显示 edge=A → A 断连执行旧 DELETE → B finalize 找不到 owner row → A/B 都不可用
Astra Helm 默认两副本,Kubernetes 默认 RollingUpdate,因此必须有兼容升级方案。请采用分阶段协议/功能门控,或明确要求先完成兼容 cleanup 版本的全量升级;并加入模拟旧版 DELETE 与新版 claim 并存的数据库回归测试。
2. P1 — claim 过期不等于 owner 已死亡,state=2 接管可能淘汰健康连接
过期 claim 可以被新连接接管,但只有 state=1 才会被保存为 rollback predecessor:
Astra/crates/services/src/multi_agent/edge_registry.rs
Lines 1061 to 1117 in a68a863
例如:
A 正常运行 → B finalize,release 结果未知 → claim 超时 → C 接管 → C 在 finalize 前失败 → C 因 previous=None 回滚为自己的 state=0 tombstone
此时 A 或 B 仍可能健康,但数据库已经无法恢复它们;后续 heartbeat 会把它们视为 superseded。根因是单个可变 row 无法同时表达 current、predecessor 和 candidate 三代状态,而 TTL 不是进程死亡证明。
请为 state=2 定义专门的超时恢复协议,或把 generation 建模为独立持久记录并维护 active pointer。至少增加强制 claim 过期的三连接回归测试。
3. P1 — durable reconciliation 会同步阻塞 WebSocket 数据面
首次 release 在 pool commit 后直接等待数据库:
Astra/crates/runtime/src/server/edge/edge_ws_handler.rs
Lines 633 to 672 in a68a863
后续 retry 又在处理消息、结果 ACK、heartbeat 的同一个循环内直接 await:
https://github.com/matrixorigin/Astra/blob/a68a863eb38fbbf0b3c57524fce6f55db0/crates/runtime/src/server/edge/edge_ws_handler.rs#L1049-L1095
如果数据库调用半开或长时间阻塞,Edge 已收到 AuthOk,但 Ping、ToolResult、Close 都无法处理;pool 仍可能派发任务而结果无法 ACK。现有测试只覆盖立即 Err → Ok(true),没有覆盖第二次 release 永不返回:
https://github.com/matrixorigin/Astra/blob/a68a863eb38fbbf0b3c57524fce6f55db0/crates/runtime/tests/edge_ws_e2e.rs#L1622-L1683
请将 reconciliation 作为独立、可取消且有明确 deadline 的任务/future,保证控制面数据库故障不会冻结 Runner 数据面,并补充永久 pending、客户端断连和 shutdown 测试。
CI 全绿不能覆盖上述协议和版本交错问题;修复后请重新跑 focused registry/WebSocket 测试,并补充混合版本和 claim-expiry 场景。
a68a863 to
e1f7acd
Compare
|
Addressed the current-head review on rebased commit
Current-head verification passed:
The prior 12 MatrixOne 4.1.2 registry cases passed on |
Summary
Ports #636 onto current
mainand fixes Edge registry ownership settlement under MatrixOne cross-session visibility and optimistic transactions.Finalize, release, rollback, heartbeat, and unregister previously treated zero affected rows or a potentially stale observation as proof that an operation had completed or a newer generation had taken ownership. That could discard a healthy Edge, leave a pending claim for up to 120 seconds, or leave stale routing state after disconnect.
DatabaseEdgeRegistryServiceremains the sole durable owner. Registration transitions use a MatrixOne-compatible no-op UPDATE write boundary before accepting terminal observations and retry ambiguous reads within a bounded policy. An absent row is never classified as success. Rollback and unregister retain the existingregistration_state = 0row as durable inactive ownership evidence; routing continues to select onlyregistration_state = 1, and later registrations reuse the same row.Reconnect cleanup models predecessor liveness across the full claim lifecycle. If predecessor A disconnects while successor B owns the setup claim, cleanup deactivates A without clearing B's claim. If B later rolls back, A is restored only when the durable row still proves A is live; otherwise rollback leaves a skeletal inactive owner. This covers disconnects both before and after B finalizes.
A finalized
state = 2connection now supplies its exact claim identity on heartbeat. The existing one-statement heartbeat renews that claim's 120-second lease only when both generation and claim still match. A third generation therefore cannot take over a healthy connection merely because publication reconciliation lasts longer than the original claim TTL; after a real takeover, the displaced heartbeat is verified as superseded instead of extending the successor's claim.Inactive/unpublished owners do not retain operational metadata:
hostname,worktree_path,capabilities_json, andworkspace_idare scrubbed. First-registration claims persist only skeletal identity until finalize, and later registrations safely reuse that identity row.Post-pool-commit claim release is reconciled as a cancellable future polled alongside the WebSocket. Each database attempt has a five-second deadline and failures retry with 1–30 second exponential backoff. A pending or half-open release no longer blocks Ping, ToolResult, Close, or heartbeat handling; disconnect drops the future and cancels its database attempt. A definitive claim loss still closes the connection.
Related issue
Main port of #636. The original failure was observed in QA trace
trace_39c691bb97457a4e139c0235451da391.Change type
User and compatibility impact
No API, configuration, schema, migration, or new status value. Valid Edge connections no longer get discarded because a durable ownership operation temporarily reports zero affected rows. Inactive owner rows are non-routable, contain only identity/fencing data, and are reused by later registrations.
Normal registration, heartbeat, release, and unregister retain their existing database operation counts. Claim renewal is part of the existing heartbeat UPDATE. Additional reads/write boundaries occur only after an ambiguous zero-row result, and release retries occur only after an outcome-unknown storage failure or timeout.
One bounded rolling-upgrade risk is intentionally accepted. An old Server instance still uses generation-scoped physical DELETE while a new instance can hold a successor claim over the predecessor
edge_id. If that exact predecessor disconnects during the mixed-version window, one overlapping Edge reconnect—and therefore at most the turn relying on it—may fail. The user can retry after the rollout converges; this does not fail service startup, schema upgrade, or unrelated sessions. Avoiding that window would require a staged compatibility release or feature gate, which is deliberately not added to this focused bug fix.Architecture and complexity delta
DatabaseEdgeRegistryServiceremains the sole owner of durable Edge registration and exact-generation cleanup.registration_state = 0remains the inactive/unpublished state;registration_previous_edge_idrecords whether a finalized successor may still restore its live predecessor.Net delta: one squashed bug-fix commit in nine files; zero schema/table/status additions.
Verification
cargo fmt --all -- --check— passed on current head.git diff --check origin/main...HEAD— passed on current head.cargo clippy -p astra-services -p astra-runtime --all-targets -- -D warnings— passed on current head.cargo test -p astra-services edge_registry --lib— 11 passed on current head.cargo test -p astra-runtime --test edge_ws_e2e— 25 passed on current head, including a permanently pending release attempt while Ping and disconnect cleanup remain responsive.edge_registry_cases passed on heada68a863e. The current head adds a thirteenth three-generation claim-expiry/renewal case; a local rerun was attempted but the Docker daemon was unresponsive before MatrixOne could start.Final checklist