diff --git a/routemaster/state_machine/api.py b/routemaster/state_machine/api.py index 0379a508..258439b2 100644 --- a/routemaster/state_machine/api.py +++ b/routemaster/state_machine/api.py @@ -1,6 +1,6 @@ """The core of the state machine logic.""" -from typing import Callable, Iterable +from typing import Callable, Iterable, Optional from routemaster.db import Label, History from routemaster.app import App @@ -39,7 +39,7 @@ def list_labels(app: App, state_machine: StateMachine) -> Iterable[LabelRef]: yield LabelRef(name=name, state_machine=state_machine.name) -def get_label_state(app: App, label: LabelRef) -> State: +def get_label_state(app: App, label: LabelRef) -> Optional[State]: """Finds the current state of a label.""" state_machine = get_state_machine(app, label) return get_current_state(app, label, state_machine) @@ -204,6 +204,10 @@ def delete_label(app: App, label: LabelRef) -> None: # Add a history entry for the deletion current_state = get_current_state(app, label, state_machine) + + if current_state is None: + raise AssertionError(f"Active label {label} has no current state!") + row.history.append(History( old_state=current_state.name, new_state=None, diff --git a/routemaster/state_machine/tests/test_state_machine.py b/routemaster/state_machine/tests/test_state_machine.py index 44444366..94c14468 100644 --- a/routemaster/state_machine/tests/test_state_machine.py +++ b/routemaster/state_machine/tests/test_state_machine.py @@ -369,3 +369,31 @@ def test_delete_label_only_deletes_target_label(app, assert_history, mock_test_f app, label_bar, ) + + +def test_handles_label_state_change_race_condition(app, create_deleted_label): + test_machine = app.config.state_machines['test_machine'] + state = test_machine.states[1] + + # Create a label which is not in the expected state. Doing this and then + # returning the affected label from the `get_labels` call is easier and + # equivalent to having the state of the label change between the return of + # that call and when the label is used. + label = create_deleted_label('foo', 'test_machine') + + mock_processor = mock.Mock() + mock_get_labels = mock.Mock(return_value=[label.name]) + + with mock.patch( + 'routemaster.state_machine.api.suppress_exceptions', + ): + state_machine.process_cron( + mock_processor, + mock_get_labels, + app, + test_machine, + state, + ) + + # Assert no attempt to process the label + mock_processor.assert_not_called() diff --git a/routemaster/state_machine/tests/test_state_machine_utils.py b/routemaster/state_machine/tests/test_state_machine_utils.py index 72bcac36..fabe43e8 100644 --- a/routemaster/state_machine/tests/test_state_machine_utils.py +++ b/routemaster/state_machine/tests/test_state_machine_utils.py @@ -8,12 +8,70 @@ from routemaster import state_machine from routemaster.feeds import Feed +from routemaster.config import ( + Gate, + NoNextStates, + StateMachine, + ConstantNextState, +) from routemaster.webhooks import WebhookResult from routemaster.state_machine import utils +from routemaster.exit_conditions import ExitConditionProgram from routemaster.state_machine.types import LabelRef from routemaster.state_machine.exceptions import UnknownStateMachine +def test_get_current_state(app, create_label): + label = create_label('foo', 'test_machine', {}) + state_machine = app.config.state_machines['test_machine'] + state = state_machine.states[0] + with app.new_session(): + assert utils.get_current_state(app, label, state_machine) == state + + +def test_get_current_state_for_deleted_label(app, create_deleted_label): + label = create_deleted_label('foo', 'test_machine') + state_machine = app.config.state_machines['test_machine'] + with app.new_session(): + assert utils.get_current_state(app, label, state_machine) is None + + +def test_get_current_state_for_label_in_invalid_state(custom_app, create_label): + state_to_be_removed = Gate( + name='start', + triggers=[], + next_states=ConstantNextState('end'), + exit_condition=ExitConditionProgram('false'), + ) + end_state = Gate( + name='end', + triggers=[], + next_states=NoNextStates(), + exit_condition=ExitConditionProgram('false'), + ) + + app = custom_app(state_machines={ + 'test_machine': StateMachine( + name='test_machine', + states=[state_to_be_removed, end_state], + feeds=[], + webhooks=[], + ), + }) + + label = create_label('foo', 'test_machine', {}) + state_machine = app.config.state_machines['test_machine'] + + # Remove the state which we expect the label to be in from the state + # machine; this is logically equivalent to loading a new config which does + # not have the state + del state_machine.states[0] + + with app.new_session(): + with pytest.raises(Exception): + utils.get_current_state(app, label, state_machine) + + def test_get_state_machine(app): label = LabelRef(name='foo', state_machine='test_machine') state_machine = app.config.state_machines['test_machine'] @@ -52,6 +110,22 @@ def test_needs_gate_evaluation_for_metadata_change(app, create_label): ) == (True, current_state) +def test_needs_gate_evaluation_for_metadata_change_errors_on_deleted_label(app, create_deleted_label): + label = create_deleted_label('foo', 'test_machine') + state_machine = app.config.state_machines['test_machine'] + + with app.new_session(): + with pytest.raises(ValueError) as excinfo: + utils.needs_gate_evaluation_for_metadata_change( + app, + state_machine, + label, + {}, + ) + + assert 'deleted' in str(excinfo.value) + + def test_does_not_need_gate_evaluation_for_metadata_change_with_action(app, create_label, mock_webhook): state_machine = app.config.state_machines['test_machine'] diff --git a/routemaster/state_machine/utils.py b/routemaster/state_machine/utils.py index c0f8235e..2294d546 100644 --- a/routemaster/state_machine/utils.py +++ b/routemaster/state_machine/utils.py @@ -3,7 +3,7 @@ import datetime import functools import contextlib -from typing import Any, Dict, List, Tuple +from typing import Any, Dict, List, Tuple, Optional import dateutil.tz from sqlalchemy import func @@ -55,9 +55,12 @@ def get_current_state( app: App, label: LabelRef, state_machine: StateMachine, -) -> State: +) -> Optional[State]: """Get the current state of a label, based on its last history entry.""" history_entry = get_current_history(app, label) + if history_entry.new_state is None: + # label has been deleted + return None return state_machine.get_state(history_entry.new_state) @@ -92,6 +95,12 @@ def needs_gate_evaluation_for_metadata_change( current_state = get_current_state(app, label, state_machine) + if current_state is None: + raise ValueError( + f"Cannot determine gate evaluation for deleted label {label} " + "(deleted labels have no current state)", + ) + if not isinstance(current_state, Gate): # Label is not a gate state so there's no trigger to resolve. return False, current_state