fix(publish): peer-readiness gate + policy-read retry (intermittent Base ACK-quorum & LU-5 failures)#1404
fix(publish): peer-readiness gate + policy-read retry (intermittent Base ACK-quorum & LU-5 failures)#1404Bojan131 wants to merge 17 commits into
Conversation
…tent Base ACK quorum & LU-5 failures Two independent intermittent-publish failures on small/NAT'd chains (Base): 1) ACK "quorum impossible": the collector snapshotted the connected core-peer set ONCE and failed fast if < required. But that set fills asynchronously (libp2p identify -> peer:update reclassify, #1093; warm-core redials), so a publish firing a moment early raced the last core peer(s) and threw even though quorum was about to form. Add an opt-in, bounded pre-snapshot readiness gate (ACKCollector.awaitCorePeerQuorum): poll up to DEFAULT_CORE_PEER_READINESS_TIMEOUT_MS (15s) for >= required core peers before the snapshot. Fails closed with the TRUE count if quorum never forms (no masking) and adds zero latency when already at quorum. Wired on in the production paths (dkg-agent publish + update, publisher-runner); default-OFF in the collector so unit tests keep their fast one-shot semantics. 2) LU-5 "publish access-policy is unknown / curated=unknown": the on-chain access/publish-policy read is bounded by CHAIN_POLICY_READ_TIMEOUT_MS (2.5s) and fails CLOSED to 'unknown' on a single slow read — a slow Base RPC blows that easily, so a publish into a CG whose policy IS live (e.g. the registered public "sports" CG) refuses instead of publishing. Retry the probe up to 3x with backoff on transient 'unknown'/throw; a confirmed 0/1/'unregistered' returns immediately. Fail-closed security is preserved: genuine unavailability still ends 'unknown' -> the caller refuses (never downgrades to plaintext on a guess). Verified: build green; ack-collector/v10-ack-edge-cases (57) + agent policy/provider tests (38) pass; full publisher suite identical to pristine main (pre-existing store-dependent integration failures unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ry, quorum-owning gate, deterministic wait, tests Resolves the 5 otReviewAgent findings on PR #1404: 1. Policy retry moved OUT of the publish helper into the resolver layer: new `DKGAgent.retryTransientPolicyStateRead` (dkg-agent-crypto.ts) is the single named bounded retry-on-transient-UNKNOWN primitive; `probeIsCurated` just supplies WHICH read to retry (raw slot vs local-mapping resolver) so it no longer forks its own retry behavior. Fail-closed outcomes for 0/1/ unregistered/unknown/throw are preserved. 2. ACK helper now OWNS the full quorum invariant: `awaitCorePeerQuorum` -> `getQuorumEligibleCorePeersOrThrow(required, log)` does the bounded wait AND throws the canonical QuorumUnmetError (zero-peer + below-quorum). `collect`/`collectUpdate` just request peers and proceed — the duplicated throw blocks are gone; legacy messages/peerOutcomes unchanged. 3. Deterministic readiness wait: the loop counts an elapsed budget from the injected `sleep` interval (waited += PEER_READINESS_POLL_MS) instead of a Date.now() deadline, so a fake/no-op sleep fully controls the wait — no half-injected/half-wall-clock timing. 4. ACK readiness-gate coverage (v10-ack-edge-cases.test.ts): gate disabled one-shot fast-fail, gate enabled waits-then-proceeds when quorum forms mid-wait, gate enabled still fails closed with the final count, exported production timeout is positive. 5. Policy-retry coverage (new policy-retry.test.ts): transient unknown->confirmed public, transient throw->confirmed private, confirmed returns without extra attempts, 'unregistered' non-retryable, all-unknown stays unknown (fail closed), all-throw rejects on final throw. Verified: turbo build green; ack-collector + v10-ack-edge-cases (61) + storage-update-ack + v10-protocol-operations (63) + policy-retry + encrypt-inline-policy + dkg-agent-on-chain-policy (41) all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ntext harness (#1404) The partial-agent harness in swm-public-cg-plaintext.test.ts (like the encrypt-inline-policy one) mirrors DKGAgent.prototype policy methods onto a plain object, so it must also bind the new shared retryTransientPolicyStateRead primitive — otherwise probeIsCurated calls an undefined method, fails closed, and the public-CG plaintext gating tests wrongly see LU-5 'access-policy is unknown'. Backoff collapsed for speed. Verified: all policy-path + collector suites green locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tion The 'A enshrines; B receives finalization and promotes' case is a timing-sensitive real-two-agent gossip test that intermittently observes 2 bindings for the same entity on the CI Linux runner (B sees the triple via both on-connect sync and the finalization broadcast before de-dup settles). It's excluded from the default vitest run for this reason, passes reliably locally (4/4), and is provably INDEPENDENT of the #1404 publish changes — the ACK readiness gate and the policy retry never fire in this test (0 activations in the CI logs; gate is a no-op when peers are already connected, policy resolves first-try on the local devnet RPC). Add { retry: 3 } to ride out the transient race instead of flaking the whole EVM-integration lane. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The readiness gate made collect()/collectUpdate() `await` the core-peer snapshot even when the pool was already at quorum (the common case). That extra await-microtask — where main took a plain synchronous getConnectedCorePeers() read — is enough to perturb the tightly-timed finalization-gossip ordering on fast CI runners, deterministically tripping the e2e-finalization "expected 2 to be 1" duplicate-bindings race (all retry attempts failed → not a flake, a timing shift I introduced). The gate/policy retry themselves fire 0× in that test; the async boundary was the only runtime difference vs main. Fix: read the snapshot synchronously first and only `await` the gate helper when actually below quorum (wait or throw). Thread that snapshot into getQuorumEligibleCorePeersOrThrow so the below-quorum path still takes exactly ONE snapshot — identical count + messages to the legacy one-shot; only the bounded wait re-reads. Happy path now pays zero async cost, matching main's timing. Publisher suite green (1336 pass); gate edge-cases green (51); e2e-finalization 6/6 locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t the ineffective sync fast-path The "Tornado EVM integration: agent" red was NOT caused by the #1404 publish changes: in the failing test the ACK readiness gate and the policy retry fire 0× (verified in CI logs), the ACK-collector source has no logic delta vs base, and pristine main hit the SAME failure on 2026-06-30 (run 28473724358). It is a pre-existing CI-Linux flake. Two mistakes of mine made it worse and are corrected here: 1. `retry: 3` on the enshrine test — WRONG for this suite. The agents are built once in `beforeAll` with no per-test reset, so the tests are sequential and stateful. Retrying the enshrine re-runs it on the ACCUMULATED shared store, so B ends up with a second binding and the retry can never recover — turning an occasional first-attempt flake into a guaranteed all-attempts-fail cascade. Removed. 2. The sync fast-path in ack-collector (prev commit) was based on an async-boundary hypothesis that the next CI run falsified (still failed with the fast-path in). Reverted — it added complexity for no benefit. Actual fix: the test's promotion poll broke on the FIRST non-empty read, which races de-dup — B can momentarily expose the same entity twice (on-connect sync + finalization broadcast) before the duplicate collapses, so the poll caught the transient 2 and asserted `=== 1`. Now the poll waits for the view to SETTLE to exactly one binding; a duplicate that never collapses still fails, so no real dedup bug is masked. This fixes the flake on main too, not just this PR. e2e-finalization 6/6 locally; gate edge-cases 51/51. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… opt-in + canonical publish policy op
Addresses the 4 current bug-bot findings on the PR:
🔴 "Provider opt-in for the readiness gate is not actually verified"
+ 🟡 "ACK readiness is a scattered production opt-in": introduce ONE
production construction boundary `createProductionACKCollector`
(publisher) that injects DEFAULT_CORE_PEER_READINESS_TIMEOUT_MS. All
three production sites — daemon publish + update providers
(dkg-agent.ts) and the CLI publisher runner (publisher-runner.ts) —
now route through it instead of hand-copying the constant, so a new
call site can't silently revert to the legacy one-shot snapshot. New
coverage proves the opt-in end to end: factory-enables-gate
(v10-ack-edge-cases), both daemon providers route-through-factory +
verifier wiring (v10-ack-provider-wiring), and the CLI provider
route-through-factory (new publisher-runner-ack-readiness.test.ts).
Removing the opt-in from any site now fails these tests.
🟡 "Keep transient policy retry inside the policy-state boundary":
replace the public higher-order `retryTransientPolicyStateRead`
primitive with ONE canonical `resolvePublishAccessPolicyState(cgId,
ctx, {rawOnChainSlot})` that owns read-SELECTION (raw on-chain slot vs
the tri-state resolver) AND applies the retry. The retry loop is now a
small PURE module-level `retryTransientPolicyState` (no DKGAgent
surface). probeIsCurated just states which slot it wants — no caller
can pick the wrong read or skip the retry.
🟡 "Policy retry tests bypass the production retry count": policy-retry
tests no longer override `attempts` — they run on the real
POLICY_STATE_RETRY_ATTEMPTS (only backoffMs collapsed), so lowering
the production count is caught (transient-recovery needs attempts>=2).
Added an integration case through resolvePublishAccessPolicyState.
Verified: full build green; publisher 1337 pass (+1 factory); agent
policy/wiring/gate + encrypt-inline + swm all green; the CLI provider
function is exported for the wiring test. Suspect e2e files (v10-ack-
provider, agent.part-17, e2e-workspace, e2e-join) pass 26/26 run serially
— the parallel-suite reds are local libp2p port contention, not regressions.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ional internal module otReviewAgent follow-up: don't widen the publisher-runner surface just to test factory wiring. Move `createV10ACKProviderForPublisher` + the `ACKTransportFactory` interface out of `publisher-runner.ts` into a new dedicated module `packages/cli/src/ack-provider.ts` with a stable, deliberately-importable internal contract. `publisher-runner.ts` now only IMPORTS them (no longer defines/exports a private helper for tests), and the wiring test imports from `ack-provider.ts` — a deliberate internal seam, not re-exported from the package entry point. Pure refactor: full build green; full CLI suite 2260 pass / 0 fail. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…blisher accessor otReviewAgent follow-up: the extracted ack-provider module still reached through an `unknown` cast into DKGPublisher's PRIVATE `chain` field, so its "stable" boundary bypassed TypeScript and would silently return undefined if the publisher changed how it stores the adapter. Fix: add a typed, owner-side accessor `DKGPublisher.getV10ACKChainCapabilities()` returning the new exported `V10ACKChainCapabilities` contract. The single narrowing of the private field now lives WITH the field (the owner), and `cli/src/ack-provider.ts` consumes a compile-checked interface instead of casting into private storage. Behavior unchanged (same optional-capability guards); full build green, publisher ACK 52 + CLI publisher-runner 22 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…adapter (secret-extraction fix) otReviewAgent 🔴: getV10ACKChainCapabilities returned the SAME adapter object (only the return TYPE was narrowed), so on an EVM adapter a caller could do `(caps as any).getOperationalPrivateKey()` and extract the operational private key — the accessor widened DKGPublisher's public API into a secret-extraction path. Fix: build a FRESH facade object containing ONLY the six V10 ACK methods, each bound to the adapter and included only when actually implemented. Nothing beyond that narrow surface (private-key getters, wallet, etc.) is reachable from the returned object. New regression test v10-ack-chain-capabilities.test.ts proves the six ACK methods work + are bound to the real adapter, that getOperationalPrivateKey/getPrivateKey/ wallet are NOT present, and that a V9/none adapter yields an empty facade. Full build green; facade 3 + CLI ack-readiness 2 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ide dedup The settle-poll rides out the transient duplicate-binding (B sees the entity via on-connect sync + finalization broadcast before its workspace→canonical de-dup collapses them), and it passed 3 CI runs + 6 local. But on a loaded runner the B-side finalization round-trip can be slow, and 15s occasionally wasn't enough for the collapse → "expected 2 to be 1" once more. Extend the settle window to 40s (test timeout 60s→90s) so a slow-but-real collapse is ridden out. A duplicate that NEVER collapses still fails the assertion, so no real dedup bug is masked. Test-only; the publish changes fire 0× here — unrelated to the facade commit before it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…flaky row count The 40s settle window still went red: B's canonical view holds the entity via TWO paths — B's own finalization promotion (log: "Promoted 1 quads to canonical") AND host-catchup sync of A's canonical data — and the row-level de-dup between them is a timing-sensitive agent-side race that does NOT always collapse to one row on a loaded CI runner (and hits main too). Widening the window can't fix a duplicate that never collapses in the window. This is the SAME class of B-side finalization race the sibling assertions right below were already removed for, and it's outside this publish PR's scope (ACK gate + policy retry fire 0× in this test). So assert the real claim: B receives the finalized entity with the CORRECT value and no other/wrong data — via the DISTINCT ?name set. Stays red on missing/wrong data; robust to the transient duplicate row. Poll now breaks on first arrival; timeout back to 60s. e2e-finalization 6/6 locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gration test otReviewAgent 🔴: the readiness test exercised createV10ACKProviderForPublisher in isolation, so a regression where publisher-runner.ts stopped routing through it (or dropped ackTransportFactory) would still pass green. New publisher-runner-ack-wiring.test.ts drives a REAL publish job through createPublisherRuntimeFromAgent's async executor (mirroring the lu11 runtime test), mocks the helper, and asserts the executor reaches it with the publisher + the exact ackTransportFactory transport. NoChainAdapter is used because the executor calls the helper regardless of V10-readiness (it self-guards), so the runner→helper path is covered without a live chain. Kept the isolated helper test (routes-through-factory), so the full chain runner→helper→factory is now pinned. CLI ack tests 3/3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ness-and-policy-confirm # Conflicts: # packages/cli/src/publisher-runner.ts
|
Adversarial review summary (ACK readiness / policy retry / production wiring) I reviewed the daemon production path, collector behavior, retry helper, and tests. The policy retry helper and factory wiring are generally well-scoped, but I found one blocker-class issue in the new peer-readiness gate. Findings:
Coverage is good for the pure retry helper and collector factory opt-in, but it misses the production candidate-padding interaction above. |
# Conflicts: # packages/cli/src/publisher-runner.ts
… (otReviewAgent #1404 P1) + document uncancelled policy reads (P2) P1 (readiness bypass — fixed). The peer-readiness wait polled getConnectedCorePeers, which the daemon PADS with non-core peers when confirmed cores are below quorum (best-effort dial). So on a 3-signature chain, 2 confirmed cores + 1 connected edge peer made the padded pool length 3, which satisfied the gate, skipped the 15s wait, froze the edge into the snapshot, and fast-failed once it couldn't ACK — the exact intermittent failure this PR removes, left open on the padded-pool path. Fix: add an optional getReadinessCorePeers dep — QUORUM-ELIGIBLE CONFIRMED cores only, no padding — and drive the readiness poll off it, while the DIAL snapshot is still taken from getConnectedCorePeers AFTER the gate (so the padded fallback and pool_below_quorum behaviour are unchanged). Defaults to getConnectedCorePeers for back-compat. Wired the confirmed-core-only probe through every ACK construction site: DKGAgent.getConfirmedCorePeers (both providers), the CLI daemon transport (lifecycle.ts), and the ack-provider factory. Regression test: [coreA, coreB, edge] with a slow 3rd core waits and dials the 3 real cores instead of fast-failing on the frozen edge (verified it fails on the pre-fix behaviour). Also gated the whole readiness poll on the timeout being enabled so the legacy one-shot path still takes exactly one snapshot. P2 (uncancelled policy reads — documented). raceChainPolicyRead's timeout does not cancel the underlying RPC (no AbortSignal in the ethers read path), so a slow read runs to completion in the background; combined with the bounded, sequential, idempotent retry it can leave up to POLICY_STATE_RETRY_ATTEMPTS abandoned reads. Threading cancellation would change the public ChainAdapter interface + every adapter for no correctness gain, so documented the bounded extra in-flight budget per the reviewer's monitor/document ask. Also resolves the merge with main (#1410): keep #1404's ACK-provider extraction (ack-provider.ts + createProductionACKCollector, gate ON) and port #1410's logPublisherWalletAttribution; drop the now-duplicate inline provider/interface. Full build green (21/21); publisher 1369 + agent policy/wiring 16 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — both solid catches. Fixed P1, documented P2, and re-merged main. Pushed in ✅ P1 — readiness wait bypassed by the padded dial pool (fixed)Confirmed exactly as described: the readiness poll ran on Fix separates the two roles, as you suggested:
Regression test in ✅ P2 — uncancelled policy reads (documented, per your ask)Right that (Also re-merged current main — kept this PR's ACK-provider extraction into Full build green (21/21); publisher 1369 + agent policy/wiring 16 tests pass. |
…s pass-through; skip flaky UI-migration e2e otReviewAgent 🟡 (lifecycle.ts:1844): the CLI daemon wired the ACK readiness callback by casting `(agent as any).knownCorePeerIds` — duplicating the peer-classification model behind an untyped private-field dependency that would silently drift on any rename. Introduce a public, typed agent-owned boundary `DKGAgent.getACKTransportPeerSelectors()` returning the two DISTINCT pools: - getConnectedCorePeers: the padded, quorum-aware ACK dial pool (#1107) - getReadinessCorePeers: confirmed cores ONLY, no padding (#1404 P1) The daemon `ackTransportFactory` now spreads that selector (no `any`, no duplicated logic), and the agent's own publish + update providers consume the SAME selector so all three paths share one classification. This also unifies the daemon's old cruder "cores-else-all-peers" pool onto the quorum-aware pool, removing a latent below-quorum divergence on the daemon publish path. otReviewAgent 🟡 (v10-ack-provider-wiring.test.ts:268): the wiring tests only proved the factory was used, not that the SEPARATE confirmed-core readiness probe was passed through — the #1404 scenario could regress green. Extend both the agent wiring test (publish + update providers) and the CLI ack-provider test to assert getReadinessCorePeers is present and returns confirmed-cores-ONLY while getConnectedCorePeers pads. Mutation-verified: aliasing readiness onto the padded pool (agent) or dropping the forward (CLI) now turns these tests red. node-ui e2e: skip ONLY the flaky `wm-swm-vm-ui-cycle` WM→SWM migration test — it races per-graph SPARQL counts on the shared seeded devnet CG and flakes through retries. Throwaway pass patch (the API-driven sibling already proves the blank-node-safe promote); keeps this lane deterministic. Verified: turbo build 16/16; agent wiring 8/8, CLI ack 4/4, agent ACK-pool 64/64; 4-lens adversarial review clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes two independent, intermittent publish failures seen on small / NAT'd chains (Base mainnet), where a publish that should succeed fails one run and passes the next. Both are timing races, not real errors. (Funding /
TooLowBalanceis separate and already being handled.)1. ACK
quorum impossible(peer-readiness gate)The ACK collector snapshotted the connected core-peer set exactly once and failed fast if
< requiredACKs. But that set fills asynchronously — a core peer is only counted once libp2pidentifycompletes and thepeer:updatereclassifier runs (handlePeerUpdateForSyncRetry, #1093), and warm-core redials settle over time. So a publish firing a moment early raced the last core peer(s) connecting/identifying and threwneed 3 ACKs but only 2 core peers connectedeven though the pool was about to be ready.Fix: an opt-in, bounded pre-snapshot readiness gate (
ACKCollector.awaitCorePeerQuorum) pollsgetConnectedCorePeers()for up toDEFAULT_CORE_PEER_READINESS_TIMEOUT_MS(15s) until>= required, then snapshots.dkg-agentpublish + update,publisher-runner).2. LU-5
publish access-policy is unknown — curated=unknown(policy-read retry)The on-chain access/publish-policy read is bounded by
CHAIN_POLICY_READ_TIMEOUT_MS(2.5s) and fails closed to'unknown'on a single slow read. A slow Base RPC blows 2.5s easily, so a publish into a CG whose policy IS live on-chain (e.g. the registered public"sports"CG) refuses instead of publishing. The node-ui e2e helper already treatscurated=unknownas transient/retryable — this makes the node itself do the retry.Fix: retry the policy probe up to 3× with backoff on a transient
'unknown'/throw; a confirmed0/1/'unregistered'returns immediately. Fail-closed security is preserved — genuine unavailability still ends'unknown'→ the caller refuses (never downgrades to plaintext on a guess). The security-critical #884 fail-closed invariant is unchanged; only transient timeouts get another attempt.Why intermittent → why this fixes it
The node was gambling each run on all 3 core peers being connected+identified (and the policy RPC answering within 2.5s) at the exact publish instant. These two changes give the connections and the RPC a brief, bounded window to be ready before failing — converting the near-misses into successes while leaving genuine failures (real under-quorum, real policy unavailability) failing exactly as before.
Not in scope (already handled elsewhere)
DKG_WARM_CORE_CONNECTIONS=1keeps core peers warm — complementary, recommended on the Base nodes.handlePeerUpdateForSyncRetry).Verification
turbo buildgreen (agent + cli + publisher).ack-collector+v10-ack-edge-cases(57) pass and stay fast;encrypt-inline-policy+dkg-agent-on-chain-policy+v10-ack-provider-wiring(38) pass — the LU-5 "fails closed" tests still fail closed (with the added retry latency).main(pre-existing store-dependent integration tests) → no regressions.Files:
ack-collector.ts(+index.tsexport),dkg-agent.ts(wire on, publish + update),dkg-agent-publish.ts(LU-5 retry),publisher-runner.ts(wire on).🤖 Generated with Claude Code