discovery: add panic recovery for gossip message processing

[Roasbeef]

In this commit, we add a centralized panic recovery mechanism for gossip
goroutines. This increases the robustness of message processing in the
gossiper, as now we are able to keep on trucking in the face of logic
errors that may lead to panics.

We ensure that any deps are freed and we log the panic trace to help
catch bugs in the future.

IMO this is a defensive pattern we should adopt in other sub-systems that
implement the p2p facing functionality of the daemon. A lil defensive
programming can go a long way.

[gemini-code-assist]

Summary of Changes

Hello @Roasbeef, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly improves the fault tolerance of the gossip message processing within the discovery subsystem. By implementing a robust panic recovery mechanism, the system can now gracefully handle unexpected runtime errors in critical goroutines, preventing service interruptions. The recovery process includes logging detailed stack traces and ensuring proper resource cleanup, which will be invaluable for diagnosing and fixing underlying issues. This change represents a proactive step towards a more stable and resilient network operation.

Highlights

• Panic Recovery for Gossip Goroutines: Introduced a centralized panic recovery mechanism specifically for gossip goroutines to enhance the robustness of message processing.
• Increased System Resilience: The new mechanism allows the system to continue operating even when logic errors lead to panics in gossip message processing, preventing crashes.
• Improved Debugging and Resource Management: Upon panic, the system ensures that any dependencies are freed and logs the full panic trace, aiding in future bug identification and resolution.
• Defensive Programming Pattern: The author suggests this pattern as a valuable defensive measure for other p2p-facing sub-systems within the daemon.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a valuable panic recovery mechanism for gossip message processing, significantly enhancing the daemon's robustness. The implementation is sound, and the accompanying tests are thorough. My main suggestion is to refactor the logging within the new recoverGossipPanic function to use structured logging, aligning with the repository's style guide. This will improve log observability and consistency. I've provided detailed comments and code suggestions to facilitate this change.

[gemini-code-assist]

To enable structured logging, the call to recoverGossipPanic needs to be updated to pass a background context.

Suggested change

	defer ctx.gossiper.recoverGossipPanic(
	"testing", nMsg, &jobID,
	)
	defer ctx.gossiper.recoverGossipPanic(
	context.Background(), "testing", nMsg, &jobID,
	)

References

1. The style guide (lines 235-253) mandates the use of structured logging (slog), which requires passing a context to logging functions like log.ErrorS. ^(link)

[gemini-code-assist]

To enable structured logging, the call to recoverGossipPanic needs to be updated to pass a background context.

Suggested change

	defer ctx.gossiper.recoverGossipPanic(
	"testing", nMsg, &parentJobID,
	)
	defer ctx.gossiper.recoverGossipPanic(
	context.Background(), "testing", nMsg, &parentJobID,
	)

References

1. The style guide (lines 235-253) mandates the use of structured logging (slog), which requires passing a context to logging functions like log.ErrorS. ^(link)

[lightninglabs-deploy]

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

[ziggie1984]

Approach looks fine to me, had minor comments,

however we need to be aware that this recovery design has its limits, it will for example not recover panics which happen in second level goroutines. Most of our DB insertions happen in the batch function which is a separate goroutine so we won't catch these errrors for example:

 handleNetworkMessages (Goroutine 1)
    ├─ defer recoverGossipPanic()  ← Can only catch panics in THIS goroutine
    │
    ├─ processNetworkAnnouncement()
    │   └─ handleChanAnnouncement()
    │       └─ d.cfg.Graph.AddEdge(ctx, edge, ops...)
    │           └─ b.addEdge(ctx, edge, op...)
    │               └─ b.cfg.Graph.AddChannelEdge(ctx, edge, op...)
    │                   └─ scheduler.Execute(ctx, request)
    │                       └─ go s.b.trigger(ctx)  ← NEW GOROUTINE! (batch/scheduler.go:85)
    │                           └─ b.run(ctx)       ← NO PANIC RECOVERY!
    │                               └─ req.Do(tx)   ← 💥 If panic here → DAEMON CRASHES!
    │
    └─ recoverGossipPanic CANNOT catch the panic in the batch goroutine!

[Roasbeef]

however we need to be aware that this recovery design has its limits, it will for example not recover panics which happen in second level goroutines

That's a good point. I think to cover deeper chains like that we would consider wrapping our existing wg wrapper with a recover call. Here I'm after just shallow items within the gossip processing logic, will take a look at if things need to be extended slightly more.

[ziggie1984]

LGTM