Skip to content

chainsource: escalate unrecoverable block epoch loss to the daemon - #762

Open
Roasbeef wants to merge 2 commits into
mainfrom
fail-fast-block-epoch-loss
Open

chainsource: escalate unrecoverable block epoch loss to the daemon#762
Roasbeef wants to merge 2 commits into
mainfrom
fail-fast-block-epoch-loss

Conversation

@Roasbeef

Copy link
Copy Markdown
Member

Motivation

A production signet incident left the darepod ark client wedged after its
backing LND bounced. The block epoch reconnect work (#698, "chainsource:
reconnect block epochs") correctly treats a closed stream as transient and
re-registers with bounded backoff — but it retries forever. When the
backend connection is genuinely stuck, dozens of block subscribers all
re-register against the dead notifier every 30s indefinitely (the log filled
with bursts of "Registering for block epoch notifications"), no subscription
ever heals, and the daemon serves RPCs as a zombie while its readiness probe
stays green.

The downstream effect: a Lightning → Ark in-swap never completed. The swap
server kept intercepting the HTLC but failed with get server pubkey: identity pubkey is empty, because the wedged client could not surface the
operator identity. HTLCs were canceled back; from the payer's side the swap
looked stuck.

What this does

Bounds the reconnect instead of retrying forever:

  • BlockEpochActor tracks how long a subscription has been continuously
    down
    — stamped on first loss, cleared only once a replacement stream
    actually delivers a block. A backend that hands back streams which
    immediately close therefore still counts as down (no false "healthy").
  • Once the down streak outlives FatalReconnectTimeout (default 5m), the
    actor stops retrying and escalates via a new OnFatal hook.
  • Because every block subscriber (vtxo, unroll, wallet, txconfirm)
    is served by this one actor, the escalation lives in a single shared place.
  • The hook is threaded from the daemon through ChainSourceConfig. The
    server wires it to a context.CancelCause on the run context, so run()
    returns the cause → non-zero exit → the orchestrator restarts darepod
    with a fresh backend connection. Normal signal-driven shutdown still
    returns nil.

No os.Exit in subsystem code; the failure bubbles up the normal error path.
A clock is injected so the timeout is exercised without real waits.

Testing

  • go test ./chainsource/... green, go vet ./chainsource/ ./darepod/ clean.
  • New test: a backend that can never sustain a stream escalates through
    OnFatal (clock-driven). Existing reconnect/iterator tests still pass.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a fatal timeout mechanism for block epoch subscriptions to prevent the daemon from silently spinning when a backend connection is stuck. If a subscription remains down past the fatal timeout, the actor escalates the failure via an OnFatal callback, which triggers a non-zero exit of the daemon so it can be restarted. A potential issue was identified where a normal shutdown or unsubscribe could trigger a spurious fatal escalation if the subscription had already exceeded the timeout; checking for context cancellation before escalating resolves this.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread chainsource/block_epoch_actor.go Outdated
Comment on lines +372 to +376
if registration == nil {
// If the subscription has stayed down past the fatal
// budget, stop retrying forever and escalate so the
// daemon can restart with a fresh backend connection.
if a.reconnectExhausted(downSince) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

If the actor is stopped normally (e.g., during a graceful shutdown or an unsubscribe request), a.ctx is cancelled. However, if the subscription was currently in a "down" state for longer than the fatal timeout, the loop will still evaluate reconnectExhausted(downSince) as true and trigger a spurious fatal escalation to the daemon before checking the context cancellation in waitForReconnect. Checking a.ctx.Err() != nil before escalating ensures we exit cleanly during normal shutdowns or unsubscribes.

		if registration == nil {
			if a.ctx.Err() != nil {
				return
			}

			// If the subscription has stayed down past the fatal
			// budget, stop retrying forever and escalate so the
			// daemon can restart with a fresh backend connection.
			if a.reconnectExhausted(downSince) {

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b19d79ce9b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread chainsource/block_epoch_actor.go Outdated
Comment on lines +376 to +379
if a.reconnectExhausted(downSince) {
a.escalateFatal(log, downSince, lastErr)

return

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Do not kill subscriptions when no fatal hook is installed

In configurations that don't install OnFatal (for example systest.NewChainSourceActor, and external users because the hook is optional), this branch still exits the monitor after the timeout; since escalateFatal is a no-op without a hook, a backend outage lasting longer than 5 minutes permanently closes the block subscription instead of either restarting the daemon or continuing to retry. Please only take the fatal return when a hook is installed, or make the hook required for bounded retries.

Useful? React with 👍 / 👎.

@levmi levmi added the P0 Priority 0 — blocker label Jun 22, 2026
@Roasbeef
Roasbeef force-pushed the fail-fast-block-epoch-loss branch from 176ace8 to d6fbb56 Compare June 23, 2026 21:21
@Roasbeef

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown

Claude finished @Roasbeef's task in 2m 44s —— View job


Review: chainsource: escalate unrecoverable block epoch loss to the daemon

I read the full diff across block_epoch_actor.go, chainsource.go, server.go, and the gateway/health changes. This is a well-motivated, carefully-built fix — the down-streak tracking (stamp on first loss, clear only on a delivered block) correctly defeats the "storming backend hands back streams that immediately close" false-healthy case, the once-sampled jitter is the right call for fleet de-correlation, and bubbling the failure through context.CancelCause instead of os.Exit keeps the shutdown path clean. The health/readiness backstop is a sensible companion. Comments are excellent.

Two correctness issues stand out, one of which I'd treat as blocking.


🔴 1. Without an OnFatal hook, the subscription is silently killed instead of retried (agree with Codex)

In monitorBlocks (chainsource/block_epoch_actor.go:430-438):

if a.reconnectExhausted(downSince, fatalTimeout) {
    a.escalateFatal(log, downSince, lastErr)
    return   // <-- exits the monitor unconditionally
}

escalateFatal is a no-op when OnFatal is None, but the return fires regardless. So for any caller that doesn't install the hook, a >5m backend outage now permanently closes the block subscription (and the iterator channel) rather than retrying forever as before #698. This is a behavioral regression, and it's reachable today:

  • systest/systest.go:209 builds ChainSourceConfig with no OnFatal — confirmed.
  • SDK/embedded and external BlockEpochConfig consumers, since the hook is documented as optional.

The daemon path is fine (server.go:1925 wires fn.Some(s.signalFatal)), but the actor shouldn't degrade into a silent dead-subscription for everyone else. Only take the fatal return when a hook is actually installed; otherwise keep retrying (preserving the old unbounded-retry contract):

if a.reconnectExhausted(downSince, fatalTimeout) &&
    a.cfg.OnFatal.IsSome() {

    a.escalateFatal(log, downSince, lastErr)
    return
}

Either that, or make OnFatal required for bounded retries and document it. Fix this →


🟠 2. Spurious fatal escalation during shutdown (agree with Gemini)

At block_epoch_actor.go:430, reconnectExhausted is evaluated before any check of a.ctx.Err() (that check only lives inside waitForReconnect, one step later). If the actor is stopped (Stop/OnStopa.cancel()) while the subscription is already down past the budget, the loop escalates on the way out.

Worth flagging that this is slightly worse than Gemini described: signalFatal unconditionally does s.fatalFlag.Store(true) (server.go:1254) before the fatalCancel call. Even though fatalCancel(err) is a no-op once the run ctx is already cancelled (first cause wins), the latched fatalFlag flips LivenessCheck/ReadinessCheck to failing — so a clean shutdown that happened to coincide with a long outage reports a fatal failure it didn't really have. Cheap guard:

if registration == nil {
    if a.ctx.Err() != nil {
        return
    }
    if a.reconnectExhausted(downSince, fatalTimeout) ... {

Fix this →


🟡 3. A stream that opens but never delivers (and never closes) won't escalate

reconnectExhausted is only consulted while registration == nil (the reconnect loop). If a reconnect succeeds but the new stream then stays open and silent forever, the goroutine blocks in the select on registration.Epochs (block_epoch_actor.go:464). downSince (stamped at the prior close) is never cleared — no block was delivered — but it's also never re-checked, so the fatal budget never fires. This is narrower than the symptom you fixed (closing streams), and a healthy backend delivers blocks well within the budget, so it may be acceptable — but it's a gap in the "continuously down" guarantee worth a sentence in the comment, or a bounded wait on the open-but-idle stream if you want full coverage.


Nits / observations (non-blocking)

  • server.go:1253 signalFatal concurrency: fatalCancel is written once in run() before any subsystem starts and read from the actor goroutine afterwards, so the happens-before holds and the no-mutex read is fine. The doc comment ("safe to call from any subsystem goroutine") is accurate. Just confirming it isn't a data race.
  • fatalReconnectTimeout jitter on the explicit-timeout path: the design (auto-jitter only on the default path, exact on an explicit timeout) is reasonable and well-documented, and production uses the default path (the daemon never sets FatalReconnectTimeout), so production gets jitter. 👍
  • Gateway health routes: in-process-only checks, %q-escaped reason, 503 on failure, warn-and-skip when checks are unavailable — all good. The unauthenticated-but-no-backend-I/O reasoning is sound.
  • Test coverage: TestBlockEpochActorEscalatesWhenBackendStaysDown and the jitter test are clean and clock-driven. Consider adding a case asserting the no-OnFatal behavior once lib: start adding lib helpers #1 is resolved (subscription keeps retrying rather than exiting), and one for the shutdown-during-down race in multI: add initial repo scaffolding #2.

Overall: solid, the architecture is right. I'd block on #1 (silent subscription death for hookless callers) and fold in #2 while you're there; #3 is a judgment call.

  • Read the PR diff and changed files
  • Analyze block_epoch_actor.go fatal reconnect logic
  • Analyze config wiring (chainsource.go, server.go, gateway)
  • Review tests
  • Evaluate prior reviewer feedback (Gemini, Codex)
  • Post review
    · fail-fast-block-epoch-loss

Roasbeef added 2 commits June 23, 2026 14:29
The block epoch reconnect work treats a closed backend stream as
transient and re-registers with bounded backoff, which heals a notifier
that briefly churns. But it retries forever. When the backend connection
is genuinely stuck — the production symptom was dozens of block
subscribers all re-registering against a dead notifier every 30s,
indefinitely — every subscriber spins in lockstep, no subscription ever
heals, and the daemon keeps serving RPCs as a zombie while its readiness
probe stays green. On that node a Lightning -> Ark in-swap never
completed: the ark client could not hand the swap server a server
pubkey, so intercepted HTLCs were canceled back.

In this commit, we bound the reconnect. The BlockEpochActor now tracks
how long a subscription has been continuously down — stamped when the
stream is first lost and cleared only once a replacement actually
delivers a block, so a backend that hands back streams which immediately
close still counts as down. Once the down streak outlives
FatalReconnectTimeout, the actor stops retrying and escalates through a
new OnFatal hook instead of spinning.

The fatal timeout carries a small one-shot jitter so a fleet that loses
a shared backend at the same instant does not all escalate and restart
in lockstep.

Because every block subscriber escalates through this one hook, the
escalation lives in a single place and covers them all. The hook is
threaded from the daemon through ChainSourceConfig: the server wires it
to a context.CancelCause on the run context, so run() returns the cause,
the process exits non-zero, and the orchestrator restarts darepod with a
fresh backend connection. Normal signal-driven shutdown still returns
nil. A clock is injected so the timeout is exercised in tests without
real waits.
The block epoch fail-fast makes the daemon exit on the wedge we know
about, and k8s restarts an exited container. But the chart only probes
the RPC port with tcpSocket, which stays green for a process that is
listening yet making no progress. This adds the progress-aware backstop,
mirroring tapd and nautilus.

In this commit, we mount unauthenticated /v1/health and /v1/ready routes
directly on the existing grpc-gateway mux via HandlePath. Both answer
from in-process state only and never touch the chain backend, so they
keep responding even when it is stuck, and a probe cannot be used to
amplify load onto it.

Liveness (/v1/health) fails when a subsystem has escalated a fatal
failure, or when the daemon stalls in startup past a generous deadline;
a failure restarts the pod. The wedged-backend class that motivated this
is already surfaced by the block epoch fatal escalation, which latches
that flag, so liveness needs no separate backend probe. Readiness
(/v1/ready) fails until the wallet subsystem finishes starting, draining
the pod from the Service endpoints without restarting it.

The chart change that points the probes at these routes lives in
lightning-infra.
@Roasbeef
Roasbeef force-pushed the fail-fast-block-epoch-loss branch from d6fbb56 to 54f7075 Compare June 23, 2026 21:29
@Roasbeef

Copy link
Copy Markdown
Member Author

Thanks — addressed the two correctness issues, folded into the existing commits (force-pushed 54f70758).

#1 (🔴 silent subscription death without OnFatal) — fixed. The fatal return is now gated on a.cfg.OnFatal.IsSome(): only callers that opted into bounded escalation take the exit path; hookless callers (systest, SDK/embedded) keep the original unbounded-retry contract instead of having the subscription silently killed after the budget. New test TestBlockEpochActorRetriesWithoutOnFatal asserts a hookless actor against a permanently-dead backend keeps reconnecting (attempt count climbs well past where an escalating actor would have stopped) rather than exiting.

#2 (🟠 spurious escalation on shutdown) — fixed. Added an a.ctx.Err() != nil early-return at the top of the registration == nil block, before reconnectExhausted, so a stop that coincides with a long outage exits cleanly and never latches fatalFlag (which would otherwise flip liveness/readiness to failing). I didn't add a dedicated unit test for this one: the trigger is an inherent goroutine-timing boundary (cancellation observed precisely between the loop top and the exhaustion check), so any test would be racy/flaky; the guard itself is a cheap, obviously-correct ordering check.

#3 (🟡 open-but-silent stream) — documented, not implemented. Agreed it's narrower than the closing-stream symptom this PR targets, and downSince is only ever stamped on a close, so a healthy backend (which delivers blocks well within the budget) is unaffected. Added a comment on the downSince/budget block noting that an open stream that goes silent without closing is out of scope. Happy to add a bounded idle-wait on the open stream in a follow-up if you'd prefer full coverage.

Nits confirmed (no change needed): the fatalCancel happens-before is sound, and the jitter-on-default-path design is intentional. go test -race ./chainsource/... ./darepod/... green; make lint-changed-local and commitmsg-lint clean.

@levmi levmi added daemon safety Fund-safety: stuck, lost, or mis-counted funds labels Jul 6, 2026
@litbot-9000

Copy link
Copy Markdown
Collaborator

@Roasbeef, remember to re-request review from reviewers when ready

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

Labels

daemon P0 Priority 0 — blocker safety Fund-safety: stuck, lost, or mis-counted funds

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants