lightningd: don't force-close when fulfilled HTLC removal is in progress

[vincenzopalazzo]

Summary

Fixes #8899: CLN force-closes a channel with Fulfilled HTLC SENT_REMOVE_HTLC cltv hit deadline even though the node has the preimage and the fulfill is already in progress.

The bug

In htlcs_notify_new_block(), when a fulfilled incoming HTLC hits the cltv deadline, CLN force-closes the channel without checking whether the HTLC removal is already in progress. The HTLC state machine shows the fulfill was queued to channeld (SENT_REMOVE_HTLC), but the force-close fires before the upstream peer receives update_fulfill_htlc.

This was reported in the wild: the upstream peer was connected and exchanging pings, but CLN force-closed instead of sending the fulfill message.

The fix

When the HTLC is in SENT_REMOVE_HTLC or later state (>= SENT_REMOVE_HTLC), skip the force-close and log a warning instead. This is safe because:

1. The preimage is persisted to DB — onchaind can claim on-chain if needed
2. On reconnect, channeld will resend update_fulfill_htlc
3. If the peer goes on-chain themselves, onchaind handles it with the known preimage
4. The cooperative path (reconnect + fulfill) is cheaper and preserves the channel

Commits

1. pytest: reproduce issue FC due to Fulfilled HTLC $ID SENT_REMOVE_HTLC cltv $CLTV hit deadline without attempt to claim #8899 — Test that triggers the buggy force-close (l2 disconnects from l1 before sending update_fulfill_htlc, then mining blocks to the deadline)
2. lightningd: fix — Skip force-close when hin->hstate >= SENT_REMOVE_HTLC, log log_unusual instead

Changelog-Fixed: Don't force-close channel when fulfilled HTLC hits deadline but removal is already in progress.

Test plan

• test_fulfilled_htlc_deadline_no_force_close reproduces the bug (first commit)
• With the fix, l2 logs "but removal already in progress" instead of force-closing
• CI passes (Valgrind, ASan, integration tests)

🤖 Generated with Claude Code

[Andezion]

Isnt the new guard hin->hstate >= SENT_REMOVE_HTLC is unconditionally true at the point it's checked, so channel_fail_permanent() for a fulfilled HTLC becomes dead code - not just for the #8899 race, but for every case?

I mean, from what i see on master:

1. htlcs_notify_new_block() already requires hin->preimage != NULL to reach this code (lightningd/peer_htlcs.c at line 2998 - if (!hin->preimage) continue;)
2. hin->preimage is set in exactly one place, fulfill_htlc() (lightningd/peer_htlcs.c at line 397), and the very next line unconditionally advances state - htlc_in_update_state(channel, hin, SENT_REMOVE_HTLC) (line 400), with the comment "We update state now to signal it's in progress, for persistence." No other call site assigns hin->preimage (the only other match, htlc_end.c at line 158, sets it to NULL).
   So hin->preimage != NULL implies hin->hstate >= SENT_REMOVE_HTLC always? The new if block will always be taken, and the original channel_fail_permanent("Fulfilled HTLC /// hit deadline") can never fire again

[Andezion]

And after the forst comment, test_fulfilled_htlc_deadline_no_force_close and test_fulfilled_htlc_deadline_reconnect don't actually test "removal in progress -> skip" vs. "removal stuck -> still force-close". Once comment is fixed with a real distinguishing condition, these tests should be extended to also cover the case where the peer never reconnects and the channel should still eventually force-close