Merge branch 'master' into models-type-stubs

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,

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

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']
 

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