Skip to content

Fix state snapshot yellow color computation logic #877

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nebiyuelias1 wants to merge 2 commits into
base: develop
Choose a base branch
from
+8 −1
Open

Fix state snapshot yellow color computation logic #877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
20c5ed2
Fix state snapshot yellow color computation logic
nebiyuelias1 Aug 16, 2026
d0c997c
Nest spec context
nebiyuelias1 Aug 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/stoplight/domain/state_snapshot.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def color
Color::GREEN
elsif locked_state == State::LOCKED_RED
Color::RED
elsif (recovery_scheduled_after && recovery_scheduled_after! < time) || recovery_started_at
elsif (recovery_scheduled_after && recovery_scheduled_after! < time) || recovery_started?

@Lokideos Lokideos Aug 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we cannot realistically reach state in which we have set breached_at and recovery_started_at since we clear metrics across data stores at the moment when we transition to yellow (when we set recovery_started_at):

Which leads to an interesting case in which in the event of clocks skew we will return GREEN color since brached_at is nil, but we should return RED. 🤔

@bolshakov, I think that from the CB logic percpective it means that if we either have breached_at set or recovery_started_at set in the future it means that the Color is RED (meaning we should slightly adjust logic for Red color below).
WDYT?

@bolshakov bolshakov Aug 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

it means that if we either have breached_at set or recovery_started_at set in the future it means that the Color is RED (meaning we should slightly adjust logic for Red color below).

if breached_at is in the future , yes this indicates started recovery with clock skew probably. But if recovery_started_at is in the future, it means that:

  • another process started recovery
  • two process has clock skew related to each other

I don't think we should treat such case as red. This would lead to two process reaching different decisions - one already declared recovery and another one thinks it's red.

@Lokideos Lokideos Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

But what if breached_at is nil, since we clear the metrics during recovery attempt and recovery_started_at is in the future? 🤔

@bolshakov bolshakov Aug 19, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Also indicates clock skew. We have two separate fields - "recovery scheduled after" which is set in the future when we enter red state and "recovery started at" which us set when we enter yellow state.

So the first field allows all instances align when recovery should start and the second one helps them get o learn that recovery has already started (most likely the first prove is about to fire or already fired)

Color::YELLOW
elsif breached_at
Color::RED
Expand Down
7 changes: 7 additions & 0 deletions spec/unit/stoplight/domain/state_snapshot_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,17 @@
it { is_expected.to be(Stoplight::Color::YELLOW) }
end


context "when threshold breached" do
let(:breached_at) { time - 3 }

it { is_expected.to be(Stoplight::Color::RED) }

context "when recovery is in the future" do
let(:recovery_started_at) { time + 3 }

it { is_expected.to be(Stoplight::Color::RED) }
end
end
end
end