Skip to content

fix(test): make immutable deadline check more conservative#13112

Merged
rvagg merged 1 commit intomasterfrom
rvagg/immutable-deadline-race
May 21, 2025
Merged

fix(test): make immutable deadline check more conservative#13112
rvagg merged 1 commit intomasterfrom
rvagg/immutable-deadline-race

Conversation

@rvagg
Copy link
Member

@rvagg rvagg commented May 8, 2025

Fixes: #13098

@rvagg rvagg requested review from ZenGround0 and aarshkshah1992 May 8, 2025 18:15
@github-project-automation github-project-automation bot moved this to 📌 Triage in FilOz May 8, 2025
@rvagg rvagg added the skip/changelog This change does not require CHANGELOG.md update label May 8, 2025
@BigLep BigLep moved this from 📌 Triage to 🔎 Awaiting Review in FilOz May 13, 2025
@BigLep
Copy link
Member

BigLep commented May 20, 2025

@aarshkshah1992 @ZenGround0 : please take a look so we can get this merged.

@BigLep BigLep requested a review from Copilot May 20, 2025 05:13
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the immutable deadline check to a more conservative approach, adjusting both the function's logic and its naming.

  • Changed the immutable deadline check from an equality comparison to a range-based comparison (current, next, and the one after next)
  • Renamed IsImmutableDeadline to MaybeImmutableDeadline to better reflect the updated check
Comments suppressed due to low confidence (2)

itests/kit/node_unmanaged.go:1708

  • [nitpick] The function documentation mentions that immutable deadlines are defined as the current or the next deadline, yet the logic now covers three deadlines. Consider updating the comment to clearly reflect that the check now includes the deadline after the next.
// Deadline after the next deadline included to be conservative to avoid race conditions before messages land.

itests/kit/node_unmanaged.go:806

  • [nitpick] Renaming the function from IsImmutableDeadline to MaybeImmutableDeadline modifies how the intent is conveyed. Please verify that all call sites and tests are updated accordingly to maintain consistency.
if tm.MaybeImmutableDeadline(provingDeadline) {

Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

Ok this actually makes sense given how the test works. The fact that this method is unused anywhere else and the behavior is so specific to this test makes me suspect its going to be cleaner to just put MaybeImmutableDeadline inline.

@github-project-automation github-project-automation bot moved this from 🔎 Awaiting Review to ✔️ Approved by reviewer in FilOz May 20, 2025
@rvagg rvagg merged commit 7c355be into master May 21, 2025
99 of 100 checks passed
@rvagg rvagg deleted the rvagg/immutable-deadline-race branch May 21, 2025 03:15
@github-project-automation github-project-automation bot moved this from ✔️ Approved by reviewer to 🎉 Done in FilOz May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip/changelog This change does not require CHANGELOG.md update

Projects

Status: 🎉 Done

Development

Successfully merging this pull request may close these issues.

Very expensive test run failed

4 participants