fix(shutdown): cancel chain events before retirement drain - #2552
Closed
branarakic wants to merge 47 commits into
Closed
branarakic wants to merge 47 commits into
branarakic wants to merge 47 commits into
Conversation
Contributor
Author
|
Fixed both failures from CI run 34495032247 in ae66ac4.
The edited fixtures are strict-type clean; the agent build, lint, and canary merge simulation pass. No production behavior changed in this CI repair. A fresh CI run will validate the full shard matrix. |
…wner Review finding on evm-hub-contract-bindings.ts: the owner kept a second source of truth (`resolved` beside the mutable ContractCache) and several compatibility setters, so readers had to know setter ordering and property-presence rules, and direct handle writes could disagree with the resolved set. One generation record now owns the handle store, the decided keys and the readiness flag, and only the owner writes into it. The mutable setters are replaced by explicit transitions: `install(contracts)` is the single typed seam for fixtures and subclasses (a complete caller-owned set, ready at once), `invalidate(dropped?)` retires a generation for rotation or the write-side self-heal while retaining handles for in-flight work, and `completeInitialization` publishes readiness only for a completely decided, still-current generation. `resolve` commits handles and decided keys together, and the store is exposed as `EvmHubContractStore`, whose Hub-bound keys are read-only, so the adapter's lazy Chronos read and the self-heal drop now go through the owner. The adapter's protected `contracts`/`initialized` accessors remain the fixture seam but each maps to one complete owner transition, order-independently. Tests cover install, invalidate with and without dropped handles, readiness refusal for incomplete or retired generations, an install racing a staged lookup, and a type-level fixture proving Hub-bound handles cannot be written around the owner. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tor table Review finding on evm-event-contracts.ts: the alias-to-binding map was a second registry beside the event branches in listenForEvents, so supporting or renaming an event needed coordinated string edits in two files, and a mismatch silently yielded nothing. Every supported EVM event is now defined once in EVM_EVENT_DESCRIPTORS with its aliases, the Hub binding it reads and the scan that parses it. Both capability selection (eventContractKeysFor, in declaration order, so the Hub read order of a multi-capability scan is unchanged) and dispatch (evmEventDescriptorFor) are derived from that table. listenForEvents keeps the physical read boundary: the wide-scan query with failover, the per-log cancellation checkpoint, generation-owned binding resolution and the rotation watcher start. Parsing, aliases, the supplemental KCCreated mint and Transfer scans and their cancellation behavior are moved unchanged. A table-driven test enumerates the descriptors: every alias resolves its declared binding, listenForEvents scans only that binding's contract, and each scan parses real encoded logs into the expected events, including the legacy-mint, greenfield-owner and attested-author publisher fallbacks. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review findings on packages/cli/src/daemon/teardown.ts and packages/agent/src/dkg-agent.ts: the early shutdown fence was scattered across a direct state write, a callback named for chain-event admission and a hidden VM-reconcile side effect, so the protocol was hard to discover and each new producer would have added another ad-hoc callback. The agent now exposes one synchronous, idempotent, agent-wide `closeWorkAdmission()` that fences chain event polling and the VM-reconcile lifecycle (periodic, manual and event-admitted recovery); the poller and VM-reconcile closures stay private implementation details, and `stop()` reuses the same fence before draining. The feature-named `closeChainEventAdmission()` introduced earlier in this PR is replaced, not aliased. On the daemon side `closeDaemonAdmissions()` is the single synchronous boundary that aggregates the catch-up flag and the agent fence; `beginGracefulShutdown` calls it before its first await and takes the agent instead of a per-feature callback. Teardown and drain ordering is unchanged. Tests: `beginGracefulShutdown` closes both admissions before `removeApiPort` suspends; the daemon entry-point wiring test proves the agent fence runs while api.port still exists and before `agent.stop()`; the agent test proves the fence aborts both the poll and VM-lifecycle signals, is idempotent, and is reused synchronously by `stop()`; the three real built-daemon shutdown scenarios record the renamed fence. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review finding on packages/agent/src/dkg-agent-lifecycle.ts: the poller callback split ChainEventDispatchContext into separate operation and signal parameters, so metadata and cancellation from different runs could be paired by accident and every caller had to rebuild the pair by hand. `handleKARegisteredNudge` now takes the ChainEventDispatchContext as one value and destructures it internally; the poller callback forwards the context unchanged, and composition with the VM lifecycle signal is preserved. The typecheck fixture rejects a missing context, a bare operation context, a hand-paired operation and signal, and partial contexts, instead of only a missing fourth parameter. Direct-call tests pass one context object per admitted run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e suites Review nit on packages/publisher/vitest.unit.config.ts: the manifest listed test/chain-event-lane-runner.unit.test.ts, which no longer exists since that suite was split by responsibility on the base branch, so a green unit run silently implied coverage that never executed. The stale entry is replaced by the five split suites (allocator-backfill, cursor-persistence, lifecycle, publish, scheduler). They run on an in-memory store and a fake chain, so they belong in the Hardhat-free unit lane; every manifest entry now matches an existing file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…-2361-chain-event-shutdown-20260910 Brings in the five canary merges since the last sync (#2545, #2562, #2561, #2553, #2557; base head 559257e). Three files conflicted; each resolution keeps both sides' behavior. packages/publisher/src/chain-event-poller.ts Base (#2553) added a poller-lifetime AbortController and ran laneRunner.poll() inside withRpcRequestContext({ requestClass: 'background', signal }) so every poller RPC is governed as background work and cancelled on stop. This branch replaced the running/timer/ inFlightPoll state with an admitted generation (controller, timer, active) plus a retirement drain, closeAdmission() and a required ChainEventDispatchContext threaded to the runner and callbacks. Kept the branch's generation structure (its controller already owns every chain request of the poller lifetime) and re-applied the base's behavior on top of it: poll(context) now wraps laneRunner.poll(context) in withRpcRequestContext({ requestClass: 'background', signal: context.signal }), and closeAdmission() aborts the generation with the base's descriptive AbortError reason. packages/publisher/src/chain-event-lane-runner.ts Base added an optional AbortSignal parameter to poll()/scanLane() with throwIfAborted checkpoints (including one before persistScanResults). This branch requires a ChainEventDispatchContext ({ operation, signal }) on poll()/restoreCurrentlyActive()/scanLane(), passes the signal into getBlockNumber({ signal }) and the EventFilter, and rethrows aborts before failure backoff. Kept the branch's required context (the test/_helpers typecheck fixture asserts a bare signal is rejected) and retained every checkpoint the base introduced, spelled against the non-optional signal. packages/chain/test/mock-adapter-parity.test.ts Both sides extended EVM_INTERNAL_METHODS; kept the union (branch's readHubAddress/resolveHubContractBindings/requireChronos/ loadHubContractBinding/cancellableLogs and base's requestBrowserWalletRpc). Auto-merged follow-ups needed to express both sides: - packages/publisher/test/chain-event-lane-scheduler.test.ts: the base's new "classifies every poller RPC as background work" test called the private poll() without the now-required context; routed it through the branch's pollOnce() helper. - packages/chain/src/evm-adapter-base.ts: requireChronos() (branch) now throws the base's typed HubContractNotFoundError instead of a hand-built Error with the same message. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ures stop() now closes work admission through closeWorkAdmission(), which asks the chain poller to close admission before draining it. The two synthetic shutdown fixtures that install a fake poller only stubbed stop(), so the fence threw a TypeError before the retirement timeout the tests assert. Give those fakes the closeAdmission method the production poller exposes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: retry_exhausted
…udit The generation-owned Hub binding registry added four TS-private helpers to EVMChainAdapterBase: isHubBindingKey, selectHubBindings, replaceAdapterContracts and createLegacyContractCache. The mock-parity audit walks the runtime prototype, so it saw them as public methods missing from MockChainAdapter and failed the chain shard. They are plumbing behind the registry seams already exempted above (installHubContractBindings and friends); MockChainAdapter has no Hub registry to classify keys for, install a binding subset into, or project a legacy cache from. Document them in EVM_INTERNAL_METHODS alongside those seams. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nt address Finding 1 asked for a rotation-during-scan test proving the stale page is not checkpointed AND that the retry queries the replacement address. The fence landed in 2ade974 with a test that invalidates the generation directly and asserts the page rejects; the replacement-address half was still unproven, as was the pre-scan window. Adds two cases over the real loopback JSON-RPC harness. The first resolves ContextGraphStorage to address A, rotates the Hub to address B from inside the page's in-flight eth_getLogs, and asserts the page rejects, that only A was queried, and that the replay of the same block range queries B — the property that makes the failed page safe rather than merely loud. The second proves the pre-scan check fires when a rotation-listener startup replays a rotation, with no descriptor read issued at all. Both complete successfully — yielding an empty, checkpointable page — if the fence is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Finding 2 asked to keep selectEvmEventPlan as the one projection of EVM_EVENT_DESCRIPTORS and to remove the test-only lookup map along with the wrapper selectors and redundant aliases. 2ade974 removed the wrappers (eventContractKeysFor, selectEvmEventDescriptors) and the widened EvmEventCapabilityKey / EvmEventContracts aliases, and narrowed a plan's bindings to EvmEventContractKey. DESCRIPTOR_BY_ALIAS and evmEventDescriptorFor were left behind; nothing outside the descriptor tests uses them. Both are removed, so the module exports exactly the table, its derived types and the plan. The descriptor tests do their alias lookup as what it always was — a one-line projection of the plan — and now also assert that an unsupported alias yields an empty plan on both halves, and that binding deduplication makes a full plan resolve fewer bindings than it runs descriptors. Adds evm-event-contracts.typecheck.ts, the focused proof the finding asked for that the derived binding type stays narrower than EvmHubContractKey: `token` is a Hub binding no descriptor declares, and assigning it to EvmEventContractKey must not compile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…shot Finding 3 asked that if a legacy surface must remain, it be isolated with coherent get/set/enumeration/deletion semantics — and that this be proven. 2ade974 removed the live Proxy and moved production consumers onto hubContracts / adapterContracts, leaving a frozen snapshot getter behind the whole-cache setter. Only the type-level half was covered: a @ts-expect-error that the snapshot is read-only. Adds the runtime half over a Probe subclass. For a Hub-owned binding, an adapter-owned lazy slot, the Hub handle and an undecided binding alike, `get`, `in`, `Object.keys` and the own-property descriptor all agree — the exact disagreement the review reported, where `contracts.token` returned a handle and `Object.keys` listed it while `'token' in contracts` was false. Writes and deletes are refused identically because the object is frozen, rather than a delete silently leaving the registry binding installed. It also pins the snapshot semantics: a rotation is invisible to a cache already handed out and visible to the next read, so a caller cannot mistake it for a live view. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The fence in 2ade974 added three protected methods to EVMChainAdapterBase — replaceHubContractBinding, isHubContractBindingSnapshotCurrent and the renamed replaceAdapterContractBindings — and removed createLegacyContractCache with the live Proxy. The parity audit walks the runtime prototype, so it saw the three as public methods missing from MockChainAdapter and failed the chain shard; two names in the exemption list no longer referred to anything. Documents the new seams alongside the registry entries they belong to (single-binding rotation, and the liveness check an event page uses to refuse a range it scanned with retired handles), renames the adapter-side replacement entry and drops the dead Proxy entry, so the list describes the code that exists. Same pattern as 80f9c95. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
Addressed the remaining changed-line coverage failure in Local validation:
|
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: retry_exhausted
Review asked that the canonical Hub accessor pin a generation and that the live `hubContracts` getter stop being the ordinary read path, and that the test binding helper stop fabricating capabilities from the Hub handle. Both landed, but `evm-adapter-pca-rpc.unit.test.ts` still built its missing-required-binding case by overriding `hubContracts` on a subclass. That getter no longer exists, so the override was inert: the adapter had no bindings installed at all, both parameterized cases returned null for the same trivial reason, and deleting the override entirely left all 33 tests green. The complete-installation fixture cannot express an absent required binding because it fills one with a fail-fast sentinel, so the shared fixture gains the focused unresolved-generation seam the review suggested: retire the installed generation and drop named handles the way a Hub rotation does. The test now installs profile and identity, drops exactly the parameterized one, and proves the guard returns null without probing identity storage. Mutating the production guard to `if (!identity)` fails the Profile case, which the previous version did not catch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The inline ACK-digest branch of updateKnowledgeCollectionV10 called computeV10UpdateAckDigest, which re-captured Hub bindings on its own, so a Hub rotation between admission and the digest could price and address the update from a different generation than the submission. The digest helper now takes the caller's captured bindings and the update path passes them; standalone callers still capture their own. publishToContextGraph rejects immediately with the canonical unsupported error — no init, binding capture, or contract guards in front of a path that can never succeed — and the interface member is deprecated. Tests: a rotation-during-update regression covering the inline digest, growth-cost pricing, approval, and submission; an equivalent case for createOnChainContextGraph through the deposit-recovery retry; the §F2 guard test now proves init() and binding capture are never entered. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Sep 20th, 2026 9:00 AM.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Shutdown fences chain scans and event-admitted VM recovery before the daemon's first teardown wait. Cooperative requests abort immediately; noncooperative physical work must retire before Oxigraph or DashboardDB closes. Interrupted scans keep their cursor for replay. Live nudges can checkpoint after handing off to the bounded VM scheduler, whose separately owned work is fenced at the same shutdown boundary.
Every admitted poll carries a required operation-and-signal context through the runner, restore path, lane scan and all six callbacks. Existing one-argument callback implementations remain assignable. Cancellable Hub reads stay independent of an unrelated shared-cache request.
Event scans resolve only their requested contract capabilities. Event subsets and full initialization now share one canonical Hub binding store, generation and readiness state. A subset installs atomically after all reads and cancellation checks succeed. Rotation invalidates that generation while preserving handles already held by in-flight operations; new admissions reload them. Full initialization retries if rotation occurs during staged reads or after binding commit while Random Sampling setup is pending. The boot binding specification also defines its invalidation entries, and the event module only maps event aliases to capability keys. Random Sampling retains its existing pair/TTL owner.
Token resolves after Random Sampling initialization and watcher startup, preserving the final generation check. This prevents a rotation during a long cold-start wait from leaving Token bound to an obsolete address after the event has aged out of the watcher's replay window. Real-HTTP cancellation scenarios share the canonical loopback RPC harness.
Validation on the current head, including canary b7c2357:
Fresh CI is running.
Fixes #2361.