Skip to content

fix(sync): cancel chain event work during shutdown - #2044

Open
branarakic wants to merge 2 commits into
codex/rfc64-m1-cold-historical-bindingfrom
codex/rfc64-m1-clean-restart
Open

fix(sync): cancel chain event work during shutdown#2044
branarakic wants to merge 2 commits into
codex/rfc64-m1-cold-historical-bindingfrom
codex/rfc64-m1-clean-restart

Conversation

@branarakic

Copy link
Copy Markdown
Contributor

Outcome

Controlled Edge restarts now cancel active DKG and chain-event work before the daemon drains dependent workers. The process completes its normal dependency-safe teardown and exits 0 instead of reaching the forced-shutdown watchdog.

This closes the restart blocker in the RFC-64 M1 selective VM/SWM coverage gate. It does not change which context graphs an Edge selects or which public graphs a Core covers.

User impact

  • An Edge can restart while an always-on public CG is processing a chain registration or sync read without becoming a zombie.
  • Interrupted chain-event pages retain their previous durable cursor, so restart replays the partial page instead of skipping undispatched events.
  • A CG identity resolver receives the same abort signal and cannot bind or trigger reconciliation after shutdown begins.
  • Provider operations that settle after abort remain observed, avoiding unhandled rejections during adapter teardown.
  • Shutdown logs identify the exact completed drain boundary for operator diagnosis.

Before

sequenceDiagram
  participant O as Operator
  participant D as Daemon
  participant P as Chain event poller
  participant R as CG resolver
  participant N as DKG network
  O->>D: Restart Edge
  D->>P: Await poller stop
  P->>R: Await active CG resolution
  R->>N: Await chain or protocol work
  Note over D,N: Cancellation begins only in later node.stop
  D--xO: Watchdog forces exit 100
Loading

After

sequenceDiagram
  participant O as Operator
  participant D as Daemon
  participant N as DKG node
  participant P as Chain event poller
  participant R as CG resolver
  O->>D: Restart Edge
  D->>N: beginStop and close admission
  D->>P: Abort active scan
  P->>R: Propagate AbortSignal
  R-->>P: Stop without binding or reconciling
  P-->>D: Retain prior cursor for replay
  D->>N: Stop libp2p at dependency-safe boundary
  D-->>O: Clean exit 0
Loading

Verification

  • Recursive release build: all 17 CLI dependency projects passed, including package-root and type-contract checks.
  • Core DKG node lifecycle: 3/3 passed.
  • EVM chain event abort and endpoint carve-outs: 17/17 passed.
  • Publisher chain-event lane and cursor behavior: 23/23 passed.
  • Agent shutdown ordering: 5/5 passed.
  • Agent VM self-prime and abort behavior: 5/5 passed.
  • Live isolated Base Testnet Edge probe: shutdown issued while KnowledgeAssetRegisteredToContextGraph resolution was active; chain poller stopped, four peer connections closed, Oxigraph stopped, and the daemon exited 0 in under one second with no shutdown watchdog.

No production Core service was changed during validation.

Stack

contract,
label,
(c) => c.queryFilter(eventFilter, fromBlock, toBlock),
{ policy: 'wideLogScan', skipPreferred: true },

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Issue: Abort path abandons the poller wait without cancelling the underlying RPC scan

What's wrong
The new abort wrapper only rejects the promise returned to the event poller. It does not pass the signal into the chain adapter's existing cancellable RPC layer, so the eth_getLogs request can continue after stop() has returned. That weakens the shutdown contract and can leave sockets/RPC work alive while the agent proceeds with teardown.

Example
During shutdown, ChainEventPoller.stop() aborts the signal while an eth_getLogs scan is blocked. The new wrapper rejects the poller's await immediately, but because the signal was not passed into readContractWith, the underlying provider request keeps running until the RPC/server/provider times out or is destroyed. Expected behavior is that the same shutdown signal cancels the caller wait and the actual RPC transport.

Suggested direction
Pass the AbortSignal through the existing ReadOpts path, e.g. include signal in the readContractWith options, while keeping the wrapper that prevents late provider rejections from becoming unhandled.

For Agents
Look at queryFilterWithFailover in packages/chain/src/evm-adapter-events.ts. Preserve the immediate AbortError behavior and the pending.then rejection observation, but also thread signal into the readContractWith ReadOpts so rpc-request-transport can abort the HTTP request. Add a unit test that records the fourth readContractWith argument and proves the signal is passed, or an integration-style fake transport test that observes the abort.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Bug: EVM event scans can still yield buffered logs after abort

What's wrong
The new signal option does not fully uphold its cursor-safety contract for the EVM adapter. It aborts the RPC wait, but once ethers has returned a page of logs, an abort is not observed until the adapter finishes that event-type branch, so the generator can continue producing events from an already-buffered page.

Example
A caller reads one item from adapter.listenForEvents(filter, { signal }), aborts the controller, then calls next() again while the generator is still inside the same buffered logs array. The EVM adapter can yield the next log anyway, so a cursor-owning caller could process or advance past events after shutdown was requested.

Suggested direction
Centralize a small throwIfAborted/returnIfAborted guard and call it after each query resolves and inside the per-log loops before yielding or doing extra work.

Confidence note
The current internal runner checks the signal before advancing its own cursor, so this mainly affects the new public listenForEvents(..., { signal }) contract or any future caller that relies on the adapter to stop yielding after abort.

For Agents
In packages/chain/src/evm-adapter-events.ts, enforce the abort signal while draining buffered logs, not only around queryFilterWithFailover. Add signal checks before each per-log parse/yield path, including secondary arrays such as mint/transfer logs, and prove with a test that aborting after the first yielded event prevents a second event from being produced.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Issue: Use the existing RPC cancellation boundary instead of hand-rolling event-scan aborts

What's wrong
This adds a second cancellation mechanism around the chain read layer instead of using the one the package already owns. That makes the event adapter harder to reason about: future changes now have to know whether cancellation is handled by ReadOpts.signal, by this promise race, or by both. It also hides the stronger existing behavior, which can abort the underlying raw RPC request rather than only abandoning the caller-facing wait.

Example
For this scan, the same behavior can be expressed by passing the signal through the canonical read path: this.readContractWith(contract, label, fn, { policy: 'wideLogScan', skipPreferred: true, signal }). That removes the bespoke eventScanAborted helper and the manual listener/promise wrapper.

Suggested direction
Collapse this into the existing chain read abstraction. The event scan is already a readContractWith call with policy options, and cancellation is already part of that options model; threading the signal there keeps cancellation in one canonical layer and deletes a whole local promise/listener/error helper.

For Agents
In packages/chain/src/evm-adapter-events.ts, delete the local abort wrapper and pass signal through the existing ReadOpts object in queryFilterWithFailover. Preserve the caller-facing abort behavior and keep the unhandled-provider-rejection test, adjusting it to prove the canonical read layer observes the aborted signal.

liveSeedLookbackBlocks?: number;
cadenceMs: number;
dispatch(event: ChainEvent, ctx: OperationContext): Promise<void>;
dispatch(event: ChainEvent, ctx: OperationContext, signal?: AbortSignal): Promise<void>;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Issue: Collapse the shutdown signal instead of threading it through every event callback

What's wrong
The change spreads one shutdown concern across the generic lane interface, the public poller callback type, and the agent lifecycle callback. That makes cancellation ownership harder to reason about and creates an uneven callback contract where one domain event receives infrastructure state while the others do not. The cleaner model is to keep poller cancellation in the poller and agent cancellation in the agent.

Example
Six lane specs still ignore the third dispatch argument, while the VM-reconcile lane forwards it through two more callback layers. The agent already has a canonical shutdown signal via this.node.stopSignal after beginStop(), so the KA nudge can use that directly for its network-backed resolver without changing the lane/domain callback contracts.

Suggested direction
Use the lane runner's abort signal only inside the lane runner and chain scan boundary. Let the agent use its existing node stop signal for agent-owned network waits, which should delete the new dispatch(..., signal), OnKARegisteredToContextGraph(..., signal), and forwarding-only plumbing.

For Agents
In packages/publisher/src/chain-event-lane-runner.ts, keep the poller signal local to scan cancellation and cursor no-advance decisions. In packages/publisher/src/chain-event-poller.ts, restore domain callbacks to event+context shapes. In packages/agent/src/dkg-agent-lifecycle.ts, pass this.node.stopSignal into handleKARegisteredNudge or have the handler read it directly. Preserve the behavior that stopping the poller does not advance a partial lane cursor and agent shutdown aborts blocked network reads.

Comment thread packages/core/src/node.ts
/**
* Close network admission before a higher-level owner starts awaiting its
* own teardown dependencies.
*

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Issue: Keep agent shutdown policy out of the core node abstraction

What's wrong
The core node layer now describes and names its new API around a higher-level agent/poller deadlock. That leaks ownership across package boundaries and makes DKGNode less self-contained: the method's actual contract is "abort protocol reads now, stop libp2p later," but the public abstraction is framed as an agent lifecycle workaround.

Example
A maintainer reading packages/core/src/node.ts now has to understand DKGAgent cleanup ordering and the chain-event poller to know why a core node method exists. A different DKGNode consumer could also call beginStop() expecting a broader lifecycle transition even though it only aborts network reads and leaves libp2p running.

Suggested direction
Expose a node-level primitive with node-level semantics, then let the agent and daemon compose it into their shutdown ordering. This keeps the core package reusable and makes the partial-stop API harder to misuse.

For Agents
In packages/core/src/node.ts, rename/document the primitive around the core-layer effect, such as abortNetworkReadsForShutdown() or closeProtocolAdmission(), without mentioning DKGAgent. Keep the agent/daemon ordering rationale in packages/agent/src/dkg-agent.ts and packages/cli/src/daemon/lifecycle.ts. Preserve idempotency and the existing call from stop().

// Without this early boundary, the outer daemon and inner agent each wait
// for the other layer to initiate cancellation and the watchdog must force
// exit even though DKGAgent.stop() itself has the correct ordering.
agent.beginStop();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Issue: Daemon-level early cancellation is not covered by the new shutdown tests

What's wrong
The PR adds an early cancellation boundary in the daemon because daemon-owned workers are drained before agent.stop(). Current new tests verify that DKGAgent.stop() calls beginStop() before its chain poller drain, but they do not verify that runDaemonInner invokes agent.beginStop() before publisher/promote/catch-up drains. That leaves the main regression path unguarded.

Example
Regression sketch: remove agent.beginStop() from runDaemonInner.shutdown() but leave DKGAgent.stop() unchanged. The new agent test still passes, yet a daemon shutdown can still block in publisherState.runtime.stop() or daemonState.catchupRunner.close() before it ever reaches agent.stop().

Suggested direction
Cover the daemon shutdown path itself, not only DKGAgent.stop(), because the bug being prevented occurs before the daemon reaches agent.stop().

Confidence note
I found agent-level shutdown-order coverage, but no test exercising the daemon shutdown path that owns this new call. The exact daemon harness may already exist elsewhere outside the diff, so this should be confirmed against the full test suite.

For Agents
Add a CLI daemon lifecycle test around runDaemonInner shutdown. Mock an agent with beginStop, a publisher runtime or catch-up runner whose stop/close waits until beginStop has been observed, trigger graceful shutdown, and assert beginStop runs before those drains and before agent.stop(). Preserve the existing order where actual network teardown remains in agent.stop().

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Issue: Daemon-level early admission close has no regression test

What's wrong
This PR changes the daemon shutdown contract by closing agent network admission before draining daemon-owned workers, but the added tests only verify DKGAgent.stop() and DKGNode.beginStop() behavior. They do not prove the daemon calls beginStop() at the point that matters for the outer-layer deadlock described in the new comments.

Example
A regression that moves agent.beginStop() below publisherState.runtime?.stop() or removes it from runDaemonInner would still leave the new DKGAgent.stop() ordering test green, but the daemon-owned publisher/catch-up workers could again block on agent router reads before agent.stop() runs.

Suggested direction
Cover the outer shutdown sequence directly, because the agent-level stop test does not verify the daemon's new pre-drain cancellation boundary.

For Agents
Add a CLI daemon lifecycle test for the shutdown path in runDaemonInner: use mocked agent, publisher runtime, and catch-up runner, trigger shutdown, and assert agent.beginStop() happens before publisher/catch-up drains and before agent.stop().

expect(beginStop).toHaveBeenCalledOnce();
await pollerDrain;
});
const agent = Object.create(DKGAgent.prototype) as any;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Issue: Extract a shutdown-test fixture instead of adding another raw prototype stub

What's wrong
The new test repeats the full manual agent shape, and the surrounding diff shows why that structure is brittle: adding beginStop() and stopSampling() forced unrelated tests to grow more stub fields. This is maintainability debt in a lifecycle area that is already large and order-sensitive; the tests are becoming a mirror of private implementation details rather than a small harness for shutdown behavior.

Example
A narrow helper such as makeStartedStopAgent(overrides) could own the default started, timers, runtime, node, router, store, messenger, and logger stubs. Each test would override only the dependency it is asserting, such as chainPoller.stop or messenger.stopOutboxDrain.

Suggested direction
Create a local factory for the minimal started-agent shutdown harness and let individual tests pass overrides. That keeps these tests focused on the ordering they care about and prevents every new shutdown dependency from creating broad, repetitive fixture churn.

Confidence note
This is a maintainability finding about the new test structure, not about runtime behavior.

For Agents
In packages/agent/test/outbox-shutdown-lifecycle.test.ts, extract a local fixture/factory for the minimal started agent used by shutdown tests. Preserve each test's ordering assertions, but move default stop dependencies into the helper so future lifecycle fields do not require touching every case.

// GH #1098 — body extracted to `handleKARegisteredNudge` so the
// bind-only-the-matching-CG branch is directly testable.
await this.handleKARegisteredNudge(onChainId, kaId, ctx);
await this.handleKARegisteredNudge(onChainId, kaId, ctx, signal);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Issue: The KACG abort signal wiring is not covered end to end

What's wrong
The new cancellation behavior depends on the signal being passed through multiple call boundaries. The added abort test verifies the handler when called directly, but not the production wiring that supplies the signal to that handler.

Example
A regression that changes await this.handleKARegisteredNudge(onChainId, kaId, ctx, signal) back to await this.handleKARegisteredNudge(onChainId, kaId, ctx) would leave the direct nudge abort test green, while production shutdown could still wait on the unresolved CG resolver.

Suggested direction
Add a regression test that drives a KnowledgeAssetRegisteredToContextGraph event through the real poller callback boundary and asserts the signal reaches handleKARegisteredNudge and becomes aborted during stop().

For Agents
Add a wiring-level test around ChainEventPoller plus the agent lifecycle callback, or a focused lifecycle test that stubs handleKARegisteredNudge and proves the callback receives the poller's stop signal. Preserve the existing payload behavior and assert that stopping the poller aborts the same signal observed by the nudge handler.

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