Skip to content

fix(edge): make registry claim settlement idempotent - #663

Open
gouhongshen wants to merge 1 commit into
matrixorigin:mainfrom
gouhongshen:codex/main-fix-edge-registration-claim
Open

fix(edge): make registry claim settlement idempotent#663
gouhongshen wants to merge 1 commit into
matrixorigin:mainfrom
gouhongshen:codex/main-fix-edge-registration-claim

Conversation

@gouhongshen

@gouhongshen gouhongshen commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Ports #636 onto current main and 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.

DatabaseEdgeRegistryService remains 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 existing registration_state = 0 row as durable inactive ownership evidence; routing continues to select only registration_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 = 2 connection 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, and workspace_id are 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

  • Feature
  • Bug fix
  • Documentation
  • Refactor or performance improvement
  • Test
  • Build, CI, or maintenance

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

  • Canonical owner: DatabaseEdgeRegistryService remains the sole owner of durable Edge registration and exact-generation cleanup.
  • Existing state reused: registration_state = 0 remains the inactive/unpublished state; registration_previous_edge_id records whether a finalized successor may still restore its live predecessor.
  • Existing heartbeat extended: the WebSocket's durable claim identity fences and renews only the current finalized generation, in the same SQL statement.
  • Release reconciliation is a connection-scoped future, not a detached task or new state machine; connection teardown cancels it automatically.
  • Superseded behavior removed: physical deletion, absence-as-success classification, unconditional predecessor resurrection, claim-erasing predecessor cleanup, and synchronous release waits in the WebSocket data loop.
  • No compatibility path, fallback, schema object, or parallel durable owner was added.

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.
  • MatrixOne 4.1.2 live DB: the previous 12 edge_registry_ cases passed on head a68a863e. 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

  • I added or updated tests at the layer that owns the behavior, or explained why no test is needed.
  • I updated public or design documentation for contract changes, or the change needs no documentation update.
  • I checked the diff for credentials, private URLs, customer data, generated files, and other sensitive information.
  • The PR title follows the repository's Conventional Commit format.

@gouhongshen
gouhongshen marked this pull request as ready for review September 3, 2026 04:55

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.

  1. 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=2 row 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

  1. 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.

  2. The “different claim means superseded” proof depends on database semantics that are not encoded. claim_id is 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 that SELECT FOR UPDATE is 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.

  3. The public Result<bool, String> contract is too lossy for enterprise failure handling. The implementation internally has AlreadyApplied / 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.

@gouhongshen
gouhongshen force-pushed the codex/main-fix-edge-registration-claim branch 2 times, most recently from 11c2fb5 to 3fea452 Compare September 3, 2026 07:49
@gouhongshen

Copy link
Copy Markdown
Collaborator Author

Addressed the requested ownership review on the latest main and squashed the branch to one commit.

  • First-registration rollback now retries an empty pre-mutation observation and accepts absence only on the bounded final attempt; the deterministic None -> owned state=0 -> deleted policy sequence is covered by a unit test.
  • Heartbeat and unregister retain their single-SQL success path. A zero-row result now enters shared bounded settlement, uses a no-op UPDATE as MatrixOne's optimistic-transaction write barrier, and reports Superseded only after a different generation survives commit validation.
  • A finalized current generation remains heartbeatable when claim release has an outcome-unknown failure, so the local connection is not falsely closed.
  • No monotonic-generation column was added: the committed write barrier supplies the required authoritative observation without a schema/migration change.
  • I kept Result<bool, String> because it already preserves all caller-relevant states: true = applied/idempotent, false = verified superseded, Err = storage failure or outcome unknown. The WebSocket callers handle these states differently today. I tightened the trait documentation so this guarantee is explicitly limited to durable claiming backends. Replacing the public trait result with another enum would not change control flow and would unnecessarily widen this PR.
  • The post-AuthOk degraded-state protocol remains a separate protocol/UX concern; this PR prevents it from being misclassified as supersession but does not add a new wire message.

Verification: format and diff checks passed; cargo check -p astra-services -p astra-runtime passed; cargo clippy -p astra-services --lib -- -D warnings passed; focused edge-registry tests passed (10/10). The services full unit suite reached 1,805 passes; its 5 failures are unrelated macOS session-journal tests requiring rename-resistant session execution authority.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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 ADELETE/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.

  1. 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 --lib on 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.

@gouhongshen
gouhongshen force-pushed the codex/main-fix-edge-registration-claim branch from 3fea452 to b75b376 Compare September 3, 2026 15:47
@gouhongshen

Copy link
Copy Markdown
Collaborator Author

Addressed both P1 findings on rebased head b75b376a.

  1. Absence is no longer accepted as settlement evidence. Rollback and unregister now transition the existing registry row to the already-defined, non-routable registration_state = 0 instead of deleting it. That retained inactive owner row is the stable write-conflict/idempotence boundary; all routing queries continue to select only state 1, and later registrations reuse the row. If every bounded observation is still absent, settlement returns an outcome-unknown storage error rather than success. I also added a live-DB integration case covering first-registration rollback, repeated rollback, non-routability, and later row reuse.

  2. Release outcome-unknown now self-heals during the live WebSocket lifecycle. The handler retains the lease and retries release with exponential backoff from 1s to 30s. A later Ok(true) completes durable publication; a definitive Ok(false) sends Closing and terminates the locally published connection. The new WebSocket E2E covers Err -> Ok(true) and verifies that the healthy connection remains available while publication converges. The existing confirmed-claim-loss test now verifies the explicit Closing terminal.

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:

  • cargo check -p astra-services -p astra-runtime
  • cargo clippy -p astra-services -p astra-runtime --all-targets -- -D warnings
  • cargo test -p astra-services edge_registry --lib — 10 passed
  • cargo test -p astra-runtime --test edge_ws_e2e — 25 passed
  • full astra-services lib suite — 1,805 passed; the same 5 unrelated macOS session-journal platform failures remain

The PR is rebased onto current main; the description now documents the retained inactive-owner contract, reconciliation behavior, performance boundary, and current verification scope.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.

  1. 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.

@gouhongshen
gouhongshen force-pushed the codex/main-fix-edge-registration-claim branch from 7ec3f23 to a68a863 Compare September 4, 2026 02:47
@gouhongshen

Copy link
Copy Markdown
Collaborator Author

Both findings are valid and fixed on current-main head a68a863e.

  1. Predecessor cleanup no longer erases a successor claim or resurrects a disconnected predecessor. Generation cleanup is now one shared, generation-scoped SQL transition. For state=1, edge=A, claim=B, it deactivates A and scrubs A's metadata while preserving B's claim. For state=2, edge=B, previous=A, claim=B, it clears only the predecessor-liveness marker. Finalize records previous_edge_id only when the durable pre-state still proves A is live; rollback restores A only under that same proof, otherwise it settles to a skeletal state-0 owner. Normal unregister remains one SQL.

  2. Inactive owners no longer retain private operational metadata. Every deactivation/rollback-to-inactive path clears hostname, worktree_path, capabilities_json, and workspace_id. A first registration now persists only skeletal identity plus its claim until finalize, so even an unpublished state-0 owner does not store those fields. Reusing a state-0 owner keeps it skeletal until the successor finalizes.

Added current-head MatrixOne 4.1.2 coverage with independent predecessor/successor pools for both requested interleavings:

  • claim B -> unregister A -> finalize/release B publishes B and preserves B's claim.
  • finalize B -> unregister A -> rollback B leaves no routable owner and does not republish A.

The live DB suite also asserts metadata scrubbing and later owner-row reuse: all 12 edge_registry_ integration tests passed. Current-head verification additionally passed 11 registry unit tests, all 25 Edge WebSocket E2E tests, format/diff checks, and astra-services all-target clippy with warnings denied.

The branch is rebased and squashed onto latest main; the PR description is updated with the state-machine, retention, performance, and verification contracts.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

结论:Request changes

当前 head a68a863e 的方向是正确的:幂等操作基于 postcondition、明确区分 superseded/unknown、保留非路由 tombstone、清理私有元数据,都符合 Astra 的企业运行时设计。但从第一性原则和企业 unhappy path 看,仍有以下阻塞问题。

1. P1 — 不兼容滚动升级,旧实例会重新引入本 PR 修复的竞争条件

新版本获取 successor claim 时保留 predecessor 的 edge_id,让旧连接继续服务:

if let Some(previous) = previous {
// Acquire only the setup claim. Keep every active routing field
// unchanged until finalize_registration(), so the published
// predecessor remains heartbeatable and routable while setup is
// pending.
let updated = sqlx::query(
"UPDATE edge_agent_registry \
SET registration_claim_id = ?, \
registration_claim_expires_at = DATE_ADD(NOW(6), INTERVAL 120 SECOND), \
hostname = CASE WHEN registration_state = 1 THEN hostname ELSE NULL END, \
worktree_path = CASE WHEN registration_state = 1 THEN worktree_path ELSE NULL END, \
capabilities_json = CASE WHEN registration_state = 1 THEN capabilities_json ELSE NULL END, \
workspace_id = CASE WHEN registration_state = 1 THEN workspace_id ELSE NULL END \
WHERE user_id = ? AND registry_id = ? AND edge_id = ? \
AND (registration_claim_id IS NULL \
OR registration_claim_expires_at < NOW(6))",
)
.bind(&claim_id)
.bind(user_id)
.bind(&previous.registry_id)
.bind(&previous.edge_id)
.execute(&mut *transaction)
.await
.map_err(|e| format!("edge_registry lease update (attempt {attempt}): {e}"))?
.rows_affected();
if updated == 0 {
transaction.rollback().await.map_err(|e| {
format!("edge_registry lease rollback (attempt {attempt}): {e}")
})?;
continue;
}

但旧版本断连时会直接删除匹配 edge_id 的整行:

async fn unregister_generation(
&self,
user_id: &str,
edge_agent_id: &str,
edge_id_header: &str,
) -> Result<bool, String> {
let deleted = sqlx::query(
"DELETE FROM edge_agent_registry \
WHERE user_id = ? AND edge_agent_id = ? AND edge_id = ?",
)
.bind(user_id)
.bind(edge_agent_id)
.bind(edge_id_header)
.execute(&self.pool)
.await
.map_err(|e| format!("edge_registry unregister: {e}"))?;
Ok(deleted.rows_affected() > 0)
}

正常滚动升级中可能发生:

旧实例 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:

let updated = sqlx::query(
"UPDATE edge_agent_registry \
SET registration_claim_id = ?, \
registration_claim_expires_at = DATE_ADD(NOW(6), INTERVAL 120 SECOND), \
hostname = CASE WHEN registration_state = 1 THEN hostname ELSE NULL END, \
worktree_path = CASE WHEN registration_state = 1 THEN worktree_path ELSE NULL END, \
capabilities_json = CASE WHEN registration_state = 1 THEN capabilities_json ELSE NULL END, \
workspace_id = CASE WHEN registration_state = 1 THEN workspace_id ELSE NULL END \
WHERE user_id = ? AND registry_id = ? AND edge_id = ? \
AND (registration_claim_id IS NULL \
OR registration_claim_expires_at < NOW(6))",
)
.bind(&claim_id)
.bind(user_id)
.bind(&previous.registry_id)
.bind(&previous.edge_id)
.execute(&mut *transaction)
.await
.map_err(|e| format!("edge_registry lease update (attempt {attempt}): {e}"))?
.rows_affected();
if updated == 0 {
transaction.rollback().await.map_err(|e| {
format!("edge_registry lease rollback (attempt {attempt}): {e}")
})?;
continue;
}
let now = chrono::Utc::now()
.format("%Y-%m-%d %H:%M:%S%.6f")
.to_string();
// State 1 is the only published state. State 0 is an inactive
// owner (either never published or disconnected while its
// successor holds the claim), and state 2 is a finalized
// generation whose claim is not released; neither is safe to
// resurrect as a rollback target.
let published_previous = (registration_state == 1).then_some(previous.clone());
let current = EdgeAgentRecord {
registry_id: previous.registry_id.clone(),
user_id: user_id.to_string(),
edge_agent_id: edge_agent_id.to_string(),
edge_id: edge_id_header.to_string(),
hostname: hostname.map(ToString::to_string),
worktree_path: worktree_path.map(ToString::to_string),
capabilities: capabilities.clone(),
workspace_id: workspace_id.map(ToString::to_string).or_else(|| {
published_previous
.as_ref()
.and_then(|record| record.workspace_id.clone())
}),
registered_at: previous.registered_at.clone(),
last_heartbeat_at: now,
};
let lease = EdgeRegistrationLease {
current,
previous: published_previous,
claim_id: Some(claim_id),
};

例如:

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 后直接等待数据库:

let mut pending_registration_release = None;
match edge_registry
.release_registration(&registration_lease)
.await
{
Ok(false) if registration_lease.claim_id.is_some() => {
// A definite claim mismatch means another pod already owns the
// durable generation. Fail closed instead of publishing a local
// connection that cross-pod routing cannot consistently target.
state.edge_connection_pool.unregister_generation(
&user_id,
&edge_agent_id,
pool_generation,
);
forward_task.abort();
drop(reconnect_guard);
state
.edge_connection_pool
.gc_reconnect_lock(&user_id, &edge_agent_id);
let _ = send_edge_msg(
&ws_sink,
EdgeServerMessage::Closing {
reason: "edge registry registration claim lost".into(),
},
)
.await;
return;
}
Err(error) => {
tracing::error!(
target: "astra_runtime::edge_ws",
user_id = %user_id,
edge_agent_id = %edge_agent_id,
%error,
"edge WebSocket: failed to release durable registration claim"
);
pending_registration_release = Some(registration_lease.clone());
}
_ => {}
}

后续 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 场景。

@gouhongshen
gouhongshen force-pushed the codex/main-fix-edge-registration-claim branch from a68a863 to e1f7acd Compare September 4, 2026 05:22
@gouhongshen

Copy link
Copy Markdown
Collaborator Author

Addressed the current-head review on rebased commit e1f7acdb.

  1. Mixed-version DELETE race — explained and intentionally accepted. The sequence is valid only while old and new Server binaries overlap. Preventing it requires a staged compatibility release or feature gate, not a local state-machine correction. This rollout accepts one failed overlapping Edge reconnect (and at most the turn relying on it); retry after convergence recreates/claims the owner normally. It does not fail service startup, schema upgrade, or unrelated sessions. The PR compatibility section now states this boundary explicitly; no compatibility shim was added.

  2. Expired state=2 claim — fixed without another normal-path query. WebSocket heartbeat now carries the lease claim identity. The existing heartbeat UPDATE renews the 120-second claim only for state=2 AND edge_id=current AND registration_claim_id=expected. A stale owner cannot renew a successor claim and is verified as superseded after takeover. The new three-generation live-DB case forces expiry, verifies exact-claim renewal blocks C, then forces abandonment, lets C claim, and verifies B is fenced and C rollback cannot resurrect B.

  3. Release reconciliation blocking the data plane — fixed. Initial release and retries are now one connection-scoped, cancellable future polled by tokio::select!, with a five-second deadline per DB attempt and 1–30 second backoff. No release call is awaited inside a completed select arm. Ping, ToolResult, Close, and heartbeat remain pollable while release is permanently pending, and dropping the connection cancels the future. The WebSocket E2E keeps the release gate closed while proving Ping/Pong and disconnect cleanup complete.

Current-head verification passed:

  • format and diff checks
  • cargo test -p astra-services edge_registry --lib — 11/11
  • cargo test -p astra-runtime --test edge_ws_e2e — 25/25
  • cargo clippy -p astra-services -p astra-runtime --all-targets -- -D warnings

The prior 12 MatrixOne 4.1.2 registry cases passed on a68a863e. The new thirteenth DB case is committed; the local current-head rerun could not start because the Docker daemon was unresponsive, which is recorded in the PR verification section rather than claimed as passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants