Fix state snapshot yellow color computation logic

[nebiyuelias1]

Fix discrepancy in logic found in the way color calculates yellow. Previously it checks whether or not recovery_started_at is truthy or not. But there is another function recovery_started? that has additional logic of checking whether or not recovery_started_at is in the future or not. Therefore, the color computation also uses this function to avoid drift further down the line.

Fixes #782

[github-actions]

Review price tag

🟢 6 effective lines — about 5 min of focused review (based on 200–500 lines/hour). This is within the range where reviewers find the most issues per line, and small changes usually receive feedback the fastest.

Why these numbers?

These minutes are what careful defect-finding costs at 200–500 lines/hour — the rate review studies report, not how long a skim takes. "Effective lines" already exclude generated files and lockfiles. Treat the rates and the 200/400 thresholds as guardrails, not laws.

[Lokideos]

Great first issue! 🚀 Keep on keeping on ^_^
Before merging:

1. We should investigate why specs are failing here and make sure that they are green.
2. Address incorrect Light state in the event of clock skew (we should take into account existance of both breached_at and recovery_started_at fields).

WDYT? 🙂

[Lokideos]

I think we should nest this spec context inside the when threshold breached context below like this:

#...
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

WDYT?

[nebiyuelias1]

Yeah much better this way thanks!

[Lokideos]

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):

• Memory Data Store
• Redis Data Store

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]

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]

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

[bolshakov]

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)