Comfy-Org / Comfy-Org/ComfyUI_frontend

Agent CRDT doc lifecycle: persist-on-intent, stranded retry budget, unguarded subscribe ack, sticky refused doc id, unbounded probe

Open
#17,453 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

The agent CRDT doc lifecycle (`AgentCrdtDocLifecycle`, extracted verbatim in #16908) has four related defects in how it drives subscribe retry, recency probing, and confirmed-doc persistence. All four predate #16908 — that PR only moved the code into its own class — so they are split out here rather than folded into a refactor whose job was to change no behaviour.

`src/workbench/extensions/agent/crdt/agentCrdtDocLifecycle.ts`

### 1. Persist-on-intent contradicts the file's own persist-on-ok rule

`onDocumentUpdate()` re-arms the stale probe only when one is already armed, but calls `refreshPersistedDocId()` unconditionally. `lastPersistedAt` starts at `0`, so the first `doc_update` after mount always writes a record — including for a doc whose subscribe was never acked, and after `onDocReset`/`onSchemaError` cleared the probe.

`onSubscribeConfirmed` states the intended rule directly:

> FE-1902 (poc-3): only a CONFIRMED binding is worth rebinding to after a remount - persist on ok, not on intent.

The sibling `onDocumentResult()` already gates on `staleProbeTimer === null`. Applying the same guard in `onDocumentUpdate()` closes it.

### 2. Exhausting the subscribe-retry budget strands the follower

`subscribeRetryAttempt` is reset only by `clearSubscribeRetry()` — reachable from a confirmed subscribe or a retarget. After `SUBSCRIBE_RETRY_MAX_ATTEMPTS` (6, ~31.5 s) refusals, `scheduleSubscribeRetry()` returns immediately and `onSubscribeRefused` has already cleared the stale probe, so the lifecycle holds no timer at all. The follower sits unbound with `connected: false` and only a dev-only ring-buffer event. Recovery depends entirely on an unrelated `status` frame reaching `onSocketActivity` and driving `bridge.reconcile()`.

### 3. A late subscribe ack for a stale target is accepted

`onSubscribeConfirmed()` persists `this.workflowId()` — the *currently desired* doc — not the id the ack actually confirmed, and the caller's `onSubscribed` in `useAgentCrdtFollower.ts` never checks `detail.workflowId` the way `onUpdate`, `onOpsResult` and `onDocReset` do. A late `ok:true` for a previous target therefore marks the channel connected, arms the probe, and persists the *new* doc id as confirmed even though its own subscribe was never acked. A late `ok:false` schedules retries against a doc that was never refused.

### 4. A server-refused doc id stays persisted for the full TTL

`onSubscribeRefused()` leaves the persisted record intact, so a doc id the server just rejected stays readable via `readPersistedDocId()` for the 5-minute `DOC_ID_TTL_MS`. Every panel remount in that window rebinds to the known-dead doc and re-runs the refusal/retry cycle.

### 5. The recency probe re-arms forever

`armStaleProbe()` re-arms itself unconditionally with no attempt cap, so a bound panel that receives no doc-scoped frames resubscribes every `STALE_AFTER_MS` (30 s) for as long as it lives. Because an idle workflow and a stale channel are passively indistinguishable (as the doc comment says), this fires for every idle-but-connected panel, each probe costing the host a state-vector delta. That is a perpetual heartbeat rather than the "ONE active probe" budget the doc comment describes — either bound the chain (give up until the next inbound frame or `status`) or reword the comment.

Raised on #16908 by the multi-model ultrareview and CodeRabbit: r3931221149, r3931221155, r3931221159, r3931221162, r3931221167, r3984745541.

Contributor guide

Open the contributing guide

Research direction

Start with src/workbench/extensions/agent/crdt/agentCrdtDocLifecycle.ts, then trace the onSubscribed caller in useAgentCrdtFollower.ts. Review the existing guards, retry state, persisted-id handling, and stale-probe lifecycle before running the relevant agent CRDT tests. Done means the four listed lifecycle defects and the unbounded probe behavior are addressed without regressing confirmed subscriptions.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
distributed-systems, frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.