lightninglabs / lightninglabs/lndmon

Collectors treat nearly all lnd RPC errors as fatal, exiting on benign transient conditions

Open
#136 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
160
Forks
53
PR merge metrics
No merged PRs in 30d

Description

## Summary

lndmon's collectors escalate almost every error from lnd to a process exit. A
single failed RPC or a single broken subscription anywhere in a scrape cycle
terminates the whole exporter, taking all metrics down until something restarts
it. Only one error class (`DeadlineExceeded`) is tolerated; common, benign,
self-clearing lnd conditions — a brief `Unavailable` during an lnd restart, a
`Canceled` while a connection is recycled, transient `Unknown`-coded messages —
all cause a fatal exit. The streaming collectors have no transient handling and
no reconnect at all. This issue documents the full set of error-catching sites
and classes, and proposes centralizing the fatal/non-fatal decision.

## How errors flow today

There are two independent paths to process exit:

1. **`errChan` → exit.** Collectors send errors into a shared `errChan`
(`collectors/prometheus.go`). The main loop selects on it
(`lndmon.go:99`) and, on any receive, prints `Lndmon exiting with error`
and shuts down. There is no severity classification and no retry — one send
equals one exit.
2. **`return err` → `os.Exit`.** Setup/startup failures propagate up through
`PrometheusExporter.Start()` to `main`, which exits.

The only mechanism for *not* exiting on an RPC error is a guard of the form:

```go
if !IsDeadlineExceeded(err) {
errChan <- err
}
```

…present at the unary scrape sites. Everything outside that narrow guard is
fatal.

## Observed in practice

Representative log lines from a single run against a node under load. The
`DeadlineExceeded` scrape errors are tolerated (logged, lndmon keeps running),
while a transient `Unknown` from `PendingChannels` terminates the process:

```
[ERR] LNDMON: WalletCollector WalletBalance failed with: rpc error: code = DeadlineExceeded desc = context deadline exceeded
[ERR] LNDMON: ChannelsCollector PendingChannels failed with: rpc error: code = Unknown desc = unable to find arbitrator
Lndmon exiting with error: ChannelsCollector PendingChannels failed with: rpc error: code = Unknown desc = unable to find arbitrator
[ERR] LNDMON: InfoCollector GetInfo failed with: rpc error: code = Canceled desc = grpc: the client connection is closing
```

Mapping to the classes below:

- **`DeadlineExceeded` (tolerated):** the `WalletBalance` line — logged, scrape
skipped, no exit. This is the one class currently handled.
- **`Unknown` (fatal):** the `PendingChannels` "unable to find arbitrator" line
is a transient contract-court race, yet it exits the process. The node had
self-healed by the next scrape, so a restart accomplished nothing a skipped
scrape would not have.
- **`Canceled` (fatal class):** the `GetInfo` "the client connection is closing"
line. In this capture it appeared *during* the shutdown triggered by the line
above, so here it was a teardown artifact — but the same class landing on a
live scrape would itself force an exit.

## Secondary symptom: broken-pipe log flood on exit

When a fatal error fires mid-scrape, the exit tears down the exporter while a
Prometheus scrape is still in flight, producing a burst of dozens of identical
`promhttp ... write: broken pipe` lines. That flood is tracked as its own issue
(#135); the crash-triggered variant is a consequence of the over-eager
exits described here, so fixing this removes it.

## Table 1 — error classes and current disposition

For a scrape-cycle unary RPC, whether lndmon exits depends entirely on the error:

| gRPC code / condition | Disposition | Notes |
|---|---|---|
| `DeadlineExceeded` | **non-fatal** | The only tolerated class (logged, scrape skipped). |
| `"watchtower client not active"` (string) | **non-fatal** | Special-cased skip at the watchtower collector only. |
| `Unavailable` | **fatal** | lnd restart / connection drop / not-ready. Most common real transient. |
| `Canceled` | **fatal** | Connection closing/recycling. |
| `Unknown` (incl. "unable to find arbitrator") | **fatal** | Contract-court races, subsystem startup messages. |
| `ResourceExhausted`, `Internal`, `Aborted`, `FailedPrecondition`, `NotFound`, `OutOfRange`, `DataLoss` | **fatal** | No handling; transient variants also exit. |
| `Unauthenticated`, `PermissionDenied` | **fatal** | Bad/expired macaroon. Fatal is appropriate. |
| `Unimplemented` | **fatal** | Version/feature mismatch. Fatal is appropriate. |
| `InvalidArgument` | **fatal** | Programming error. Fatal is appropriate. |

Streaming/subscription sites: **every error of any class is fatal** (no guard,
no reconnect). Startup/setup paths: **every error is fatal** (`return err` →
`os.Exit`).

## Table 2 — call sites

### Class A — unary scrape RPCs: non-fatal on `DeadlineExceeded`, fatal on all else

| Site | RPC |
|---|---|
| `chain_collector.go:84` | GetInfo |
| `info_collector.go:61` | GetInfo |
| `wt_client_collector.go:83` | ListTowers |
| `wallet_collector.go:123` | ListUnspent |
| `wallet_collector.go:185` | WalletBalance |
| `wallet_collector.go:207` | ListAccounts |
| `peer_collector.go:100` | ListPeers |
| `channels_collector.go:212` | ClosedChannels (cache-refresh goroutine) |
| `channels_collector.go:309` | ChannelBalance |
| `channels_collector.go:339` | GetInfo |
| `channels_collector.go:363` | ListChannels |
| `channels_collector.go:480` | PendingChannels |
| `channels_collector.go:573` | getRemotePolicies |
| `graph_collector.go:330` | DescribeGraph |
| `graph_collector.go:354` | NetworkInfo |

### Class B — streaming/subscription: fatal on any error, no reconnect

| Site | Context |
|---|---|
| `state_collector.go:75` | SubscribeState setup |
| `state_collector.go:94` | state-update stream error |
| `payments_collector.go:125` | payment stream `Recv()` |
| `htlcs_collector.go:159` | htlc stream closed (`!ok`) |
| `htlcs_collector.go:166` | processHtlcEvent error |
| `htlcs_collector.go:171` | htlc stream error |
| `htlcs_collector.go:176` | htlc collector quit — **fires on a normal shutdown** |

### Startup/setup — fatal via `return err` → `os.Exit`

| Site | Context |
|---|---|
| `prometheus.go:108` | exporter construction |
| `prometheus.go:168` | nil lnd backend |
| `prometheus.go:175` | registerMetrics |
| `prometheus.go:182` | htlcMonitor.start → `htlcs_collector.go:143` SubscribeHtlcEvents |
| `prometheus.go:191` | paymentsMonitor.start → `payments_collector.go:99` TrackPayments |

### Existing non-fatal handlers (handle and continue)

| Site | Condition | Behavior |
|---|---|---|
| `wt_client_collector.go:73` | `"watchtower client not active"` | Debug log + skip collector (precedent for tolerate-by-match) |
| `channels_collector.go:552` | unrecognized close type | Warn + continue |
| `prometheus.go:214` | `http.ListenAndServe` returns | logged at `Info`, process runs on — **under-handled** |
| `channels_collector.go:~685` | missing channel policy in `getInboundFee` | returns nil, continues (by design) |

## Proposed direction

Rather than adding one string/code exception at a time, centralize the
classification:

1. **A single `IsTransient(err)` classifier in `collectors/errors.go`**, keyed on
the gRPC status code: `DeadlineExceeded`, `Unavailable`, and
context-driven `Canceled` are transient, plus a small, documented allowlist
of known-transient `Unknown` messages. Use it at all Class A unary sites in
place of the current single-class guard.
2. **Reserve fatal exit for structural errors** — `Unauthenticated` /
`PermissionDenied` (credentials), `Unimplemented` (version mismatch),
configuration/connection errors. These are the cases where exiting so a
supervisor restarts lndmon with fresh config actually helps.
3. **Add reconnect/backoff loops to the streaming collectors** (state, payments,
htlcs); escalate to fatal only after repeated failures, not on the first
break. Stop `htlcs_collector.go:176` from reporting a normal quit as an error.
4. **Make `prometheus.go:214` loud** — the scrape endpoint dying is a real
failure and should surface as an error (or a fatal), not an `Info` log.

Transient handling should still log every occurrence at `Error` level so
failures remain visible in logs and alertable via absent/stale metrics; the goal
is only to stop a benign, self-clearing lnd condition from taking the whole
exporter down.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with collectors/prometheus.go and lndmon.go:99 to trace errChan into process exit, then inspect the unary call sites listed in the collector files. Read the streaming paths in state_collector.go, payments_collector.go, and htlcs_collector.go alongside the proposed errors.go classifier. Done means transient unary failures no longer terminate lndmon, stream breaks reconnect with backoff, and normal htlc shutdown is not reported as an error.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, prometheus
Domain
backend, observability-sre
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.