Skip to content

Drop stuck lookups#5824

Merged
mergify[bot] merged 1 commit intosigp:unstablefrom
dapplion:drop-stuck-lookups
May 23, 2024
Merged

Drop stuck lookups#5824
mergify[bot] merged 1 commit intosigp:unstablefrom
dapplion:drop-stuck-lookups

Conversation

@dapplion
Copy link
Collaborator

@dapplion dapplion commented May 22, 2024

Issue Addressed

PR

Makes us aware of the extent of the problem of sync lookups randomly getting stuck.

While useful I believe we should:

  • Take it one step further and actively drop lookups that get stuck
  • Increase the log level of "lookup stuck alarm"

While

  • Fix underlying bugs that cause lookups to get stuck, both find by us and reported by users

The trade-off is quite good:

  • A healthy lookup needing to exist for > 16 minutes is highly unlikely. Even if it's the case, range sync can rescue the node eventually
  • So far, bugs that cause lookups to get stuck are sporadic, usually dependent of a specific sequence of events that may not repeat in a while.

Therefore dropping stuck lookups should be a net positive

Proposed Changes

  • If a lookup lasts more than LOOKUP_MAX_DURATION_SECS this function will find its oldest ancestor and then drop it and all its children.
  • Increase the "lookup stuck" log from debug to warn, as now it will log only once per lookup id

@dapplion dapplion mentioned this pull request May 22, 2024
Open
@AgeManning
Copy link
Member

AgeManning commented May 22, 2024

This seems reasonable to me.

@michaelsproul michaelsproul added ready-for-review The code is ready for review v5.2.0 Q2 2024 labels May 23, 2024
michaelsproul added a commit that referenced this pull request May 23, 2024
@michaelsproul
Drop stuck lookups (#5824)
acc5eef 
Squashed commit of the following:

commit f15131f
Author: dapplion <35266934+dapplion@users.noreply.github.com>
Date:   Wed May 22 16:01:22 2024 +0200

    Drop stuck lookups
@michaelsproul michaelsproul mentioned this pull request May 23, 2024
Merged
jimmygchen
jimmygchen approved these changes May 23, 2024
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels May 23, 2024
@jimmygchen
Copy link
Member

jimmygchen commented May 23, 2024

@mergify queue

@mergify
Copy link

mergify bot commented May 23, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at 17d9086

mergify bot added a commit that referenced this pull request May 23, 2024
@mergify
Merge of #5824
8cec4e1
mergify bot added a commit that referenced this pull request May 23, 2024
@mergify
Merge of #5824
0b0dbf8
This was referenced May 23, 2024
Closed
Merged
@mergify mergify bot merged commit 17d9086 into sigp:unstable May 23, 2024
@dapplion dapplion deleted the drop-stuck-lookups branch May 23, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimmygchen jimmygchen jimmygchen approved these changes

@AgeManning AgeManning AgeManning approved these changes

@realbigsean realbigsean Awaiting requested review from realbigsean

Assignees

No one assigned

Labels

ready-for-merge This PR is ready to merge. v5.2.0 Q2 2024

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dapplion @AgeManning @jimmygchen @michaelsproul