#30361 doc: Drop description of LogError messages as fatal

ReviewabilityReviewsAgreementSize
#30361 ryanofsky doc: Drop description of LogError messages as fatalWaiting on author0 (+1) -2Blocked5
  • Align developer logging guidelines with actual codebase practice
  • Reduce confusion during development by not reserving error logs strictly for fatal shutdowns
  • Wait for the author to respond to multiple standing objections on whether to close this pull request
  • Opposed by reviewers who prefer fixing call sites over lowering doc standards (maflcko, ajtowns)
  • Unaddressed objection: unnecessary given progress toward original guidelines, recommends closing (sedited)
  • Support as a slight improvement to the documentation (l0rinc)
  • 5 lines added or modified outside tests
  • 0 lines added or modified in tests
  • 6 lines removed in total
  • 1 files, 1 commits
Documentation#7 of 9
P3 · cleanup
  • P3 after comparing with the other PRs here (assessed alone as P4): Raised to P4->P3: the current text is factually wrong against the codebase (58/129 LogError calls fatal, per #30348) and keeps re-triggering review debates, which is more than cleanup.
  • P4 because it only loosens internal developer guidance for logging conventions
  • It does not correct misleading user-facing documentation or affect node behavior
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Updates developer documentation to loosen the definition of `LogError`. Instead of describing `LogError` as reserved strictly for fatal errors requiring node shutdown, it documents it for severe errors that the node operator must address.

Problem: The developer documentation instructed developers to reserve `LogError` only for severe problems requiring a node or subsystem shutdown, but the codebase did not follow this convention (fewer than half of existing call sites were fatal), creating confusion during development and code review.

Evidence of importance:

  • PR description / issue #30348: 'Of 129 current uses only 58 cases are fatal according to #30348.'
  • PR description quoting PR #30347: '[T]here's not much benefit in a log that says "hey this error is about to cause the node to stop" -- you already get that information by seeing "Shutdown: in progress..." immediately following.'

Reviewability

Paused · Waiting on author

Open objections from ajtowns, maflcko, sedited with no author reply for 40 days; review would repeat what has been asked.

Waiting on (from the thread read):

  • author: response to multiple NACKs and pings regarding whether to close or continue 2026-09-22
  • author: response to standing NACKs and pings on whether to close 2026-09-22

Agreement

Blocked

objection that needs a change, nobody has replied (maflcko, ajtowns, sedited)

Thread notes: Reclassified c:2197781492 as a suggestion since it requested retaining examples and wording rather than identifying a concrete harm of merging. The PR currently has three open objections recommending closure or opposing the change, waiting on the author's response. / Brought record up to date with schema rules. PR has three standing objections and is awaiting the author's response on whether to close.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
ajtowns (member)objection, approachkeep examples in both entries and retain severe in the LogWarning descriptionfixed · fix pushed · author addressed feedback by re-adding examples and severeansweredyes2024-06-28: 'I think both entries should continue to include examples. I think removing "severe" from the LogWarning description doesn't make sense'
Status from: 2024-06-30: ryanofsky: 'Thanks, added back examples and "severe".'
maflcko (member)objection, approachincreases developer confusion and weakens long-term code standards based on temporary improper log callsopenchangedno2024-09-18: 'I am NACK -ish on increasing confusion in the docs, motivated by code that will be removed or changed long-term anyway.'
ajtowns (member)objection, approachweakens documentation standards to match poor call-site practice and distracts from fixing log levels or consolidating themopenchangedno2026-03-04: 'Concept NACK... logging not-very-important errors as LogError (or even LogWarning) is not a desirable thing for our codebase'
sedited (member)objection, approachunnecessary change given progress toward realizing original log level guidelinesopenchangedno2026-05-10: 'Retracting my A-C-K here. Seems like we were closer to realizing the log level guidelines than I thought. Given the two standing N-A-C-Ks, I think this should be closed.'

Support:

  • l0rinc (member) (ACK, 2024-09-18) [read]: While I understand why @maflcko doesn't want to change twice, this seems to me like a slight improvement.

Participants:

  • ajtowns (member, objection, 4 statements 2024-06-28 to 2026-03-04): Concept NACK; opposes weakening documentation to fit improper log usage rather than fixing call sites or unifying levels
  • maflcko (member, objection, 8 statements 2024-07-16 to 2026-03-04): NACK-ish; argues against increasing doc confusion based on code that should be fixed long-term
  • l0rinc (member, support, 4 statements 2024-09-18 to 2024-09-18): utACK; views the documentation update as a slight improvement
  • sedited (member, objection, 6 statements 2026-03-02 to 2026-09-22): retracted initial ACK, recommended closing in favor of existing guidelines, and pinged the author

Files

0 lines under test/bench/ci.

  • doc/developer-notes.md +5/-6

Card

The short text the ranking pass compares across PRs.

This PR updates developer documentation to remove the requirement that `LogError` be reserved strictly for fatal errors causing node shutdown, clarifying it instead for severe errors that a node administrator needs to address. The existing documentation conflicted with code reality, where fewer than half of `LogError` calls were actually fatal, causing confusion and debate in logging PRs. Evidence includes an audit in issue #30348 showing only 58 of 129 `LogError` usages were fatal.

Data

dossier JSON · extract JSON · model ledger, generated 2026-09-30T01:21, confidence high, input hash 5e2c54326c4e85ae
thread read through 2026-09-22 (24 statements), head 5e2eb5bde2
labels: Docs · draft

Categories

Documentation

P3 · cleanup (assessed alone as P4; Raised to P4->P3: the current text is factually wrong against the codebase (58/129 LogError calls fatal, per #30348) and keeps re-triggering review debates, which is more than cleanup.)

Clarifies internal developer notes regarding LogError conventions to match current call-site usage. The change merely loosens developer guidance on when to invoke LogError rather than correcting unsafe or misleading user-facing documentation.

Membership: Carries the Docs label and directly edits developer guidelines in doc/developer-notes.md.

Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0