From 45abcd8952a97496b52ef615d81ecddd24f62cc0 Mon Sep 17 00:00:00 2001 From: Caleb Syring Date: Tue, 10 Feb 2026 10:30:54 -0500 Subject: [PATCH 1/4] fix CRITIC_NAMESPACE getting cleared fixes #68 --- env-config.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/env-config.yaml b/env-config.yaml index b19459e..b8d3124 100644 --- a/env-config.yaml +++ b/env-config.yaml @@ -1,14 +1,19 @@ profile: test: AWS_PROFILE: 'critic-test' + # Devs need to set their own namespace via mise.local.toml. This prevents it from being cleared. + CRITIC_NAMESPACE: '{env.CRITIC_NAMESPACE}' MU_DEFAULT_ENV: '{env.CRITIC_NAMESPACE}' MU_CONFIG_PATH: '{env.CRITIC_SRC}/mu-test.toml' ci: AWS_PROFILE: 'critic-test' + CRITIC_NAMESPACE: 'ci' MU_DEFAULT_ENV: 'ci' MU_CONFIG_PATH: '{env.CRITIC_SRC}/mu-ci.toml' dev: AWS_PROFILE: 'critic-dev' + # Devs need to set their own namespace via mise.local.toml. This prevents it from being cleared. + CRITIC_NAMESPACE: '{env.CRITIC_NAMESPACE}' MU_DEFAULT_ENV: '{env.CRITIC_NAMESPACE}' MU_CONFIG_PATH: '{env.CRITIC_SRC}/mu-dev.toml' qa: @@ -18,5 +23,6 @@ profile: MU_CONFIG_PATH: '{env.CRITIC_SRC}/mu-qa.toml' prod: AWS_PROFILE: 'critic-prod' + CRITIC_NAMESPACE: 'prod' MU_DEFAULT_ENV: 'prod' MU_CONFIG_PATH: '{env.CRITIC_SRC}/mu-prod.toml' From cfb06586baf0631c9f4d8bd0f048b6106192a4d3 Mon Sep 17 00:00:00 2001 From: Caleb Syring Date: Sat, 7 Feb 2026 18:22:53 -0500 Subject: [PATCH 2/4] timing resiliency for uptime checks --- src/critic/libs/ddb.py | 92 +++++++++++++----- src/critic/libs/dt.py | 11 ++- src/critic/libs/testing.py | 24 +++++ src/critic/libs/uptime.py | 150 ++++++++++++++++++++++++++++++ src/critic/models.py | 6 +- src/critic/tasks.py | 99 ++------------------ tests/critic_tests/test_models.py | 8 ++ tests/critic_tests/test_tasks.py | 48 ++++++++-- 8 files changed, 317 insertions(+), 121 deletions(-) create mode 100644 src/critic/libs/uptime.py diff --git a/src/critic/libs/ddb.py b/src/critic/libs/ddb.py index 1c89b06..6cfbf31 100644 --- a/src/critic/libs/ddb.py +++ b/src/critic/libs/ddb.py @@ -6,6 +6,7 @@ from boto3 import client from boto3.dynamodb.types import TypeDeserializer, TypeSerializer +from botocore.exceptions import ClientError from pydantic import AwareDatetime, BaseModel, TypeAdapter from critic.libs.dt import to_utc @@ -133,45 +134,90 @@ def get(cls, partition_value: Any, sort_value: Any | None = None) -> BaseModel | @classmethod def query(cls, partition_value: Any) -> list[BaseModel]: """Query for all items with the given partition key.""" - # We use aliases for the partition key to avoid reserved word conflicts - pk_alias = f'#{cls.partition_key}' + names, values, clauses = cls.alias({cls.partition_key: partition_value}) response = get_client().query( TableName=cls.name(), - KeyConditionExpression=f'{pk_alias} = :pk', - ExpressionAttributeNames={pk_alias: cls.partition_key}, - ExpressionAttributeValues=serialize({':pk': partition_value}), + KeyConditionExpression=clauses[0], + ExpressionAttributeNames=names, + ExpressionAttributeValues=values, ) items = response.get('Items', []) return [cls.model(**deserialize(item)) for item in items] + @staticmethod + def alias(data: dict, val_suffix: str = '') -> tuple[dict, dict, list]: + """ + Serializes a dict of key-value pairs and returns: + 1. A dict mapping aliased keys to actual keys (often used as ExpressionAttributeNames) + Ex. {'#key1': 'key1', '#key2': 'key2'} + 2. A dict mapping aliased values to actual values (often used as ExpressionAttributeValues) + Ex. {':key1': 'value1', ':key2': 'value2'} + 3. A list of aliased key-value pairs in DDB expression format (often used in *Expression) + Ex. ['#key1 = :key1', '#key2 = :key2'] + + Sometimes you may want to use the same key with different values in different expressions. + For example, the condition expression may require a key to have one value while the update + expression will set it to another. In that case, you can pass a suffix to differentiate + them. + + You can then safely combine the results of multiple calls to this function into + ExpressionAttributeValues. You'll have something that looks like this: + {':key-suffix1': 'value1', ':key-suffix2': 'value2'} + """ + data = serialize(data) + names = {f'#{k}': k for k in data} + values = {f':{k}{val_suffix}': v for k, v in data.items()} + clauses = [f'#{k} = :{k}{val_suffix}' for k in data] + return names, values, clauses + + @staticmethod + def alias_all(*data: dict) -> tuple[dict, dict, list[list]]: + """ + Calls alias() for each dict and returns the combined results. See alias() for more info. + """ + names, values, clauses = {}, {}, [] + for i, d in enumerate(data): + n, v, c = Table.alias(d, f'_{i}') + names |= n + values |= v + clauses.append(c) + return names, values, clauses + @classmethod def update( cls, partition_value: Any, sort_value: Any | None = None, - **updates, - ): + updates: dict | None = None, + condition: dict | None = None, + ) -> bool: if not updates: raise ValueError('No updates provided') - updates = serialize(updates) - - # Alias every field - expr_attr_names = {f'#{k}': k for k in updates} - expr_attr_values = {f':{k}': v for k, v in updates.items()} - # Build SET expression - set_clauses = [f'#{k} = :{k}' for k in updates] - update_expr = 'SET ' + ', '.join(set_clauses) - - get_client().update_item( - TableName=cls.name(), - Key=cls.key(partition_value, sort_value), - UpdateExpression=update_expr, - ExpressionAttributeNames=expr_attr_names, - ExpressionAttributeValues=expr_attr_values, - ) + if condition: + names, values, clauses = cls.alias_all(updates, condition) + update_clauses, cond_clauses = clauses + else: + names, values, update_clauses = cls.alias(updates) + + kwargs = { + 'TableName': cls.name(), + 'Key': cls.key(partition_value, sort_value), + 'UpdateExpression': 'SET ' + ', '.join(update_clauses), + 'ExpressionAttributeNames': names, + 'ExpressionAttributeValues': values, + } + if condition: + kwargs['ConditionExpression'] = ' AND '.join(cond_clauses) + + try: + get_client().update_item(**kwargs) + except ClientError as e: + if e.response['Error']['Code'] == 'ConditionalCheckFailedException': + return False + return True @classmethod def delete(cls, partition_value: Any, sort_value: Any | None = None): diff --git a/src/critic/libs/dt.py b/src/critic/libs/dt.py index 27ed508..1425475 100644 --- a/src/critic/libs/dt.py +++ b/src/critic/libs/dt.py @@ -1,4 +1,4 @@ -from datetime import UTC, datetime +from datetime import UTC, datetime, timedelta def is_aware(dt: datetime) -> bool: @@ -9,3 +9,12 @@ def to_utc(dt: datetime) -> datetime: if not is_aware(dt): raise ValueError(f'datetime must be timezone aware, got {dt}') return dt.astimezone(UTC) + + +def round_minute(dt: datetime) -> datetime: + # Most of the time we want to round down (that usually means we're erring on the side of + # running a monitor too often vs. not enough). However, if we're very late in the minute, it's + # more likely that the task ran just a bit too early and we should round up. + if dt.second >= 55: + return dt.replace(second=0, microsecond=0) + timedelta(minutes=1) + return dt.replace(second=0, microsecond=0) diff --git a/src/critic/libs/testing.py b/src/critic/libs/testing.py index 17a1b76..e2948f3 100644 --- a/src/critic/libs/testing.py +++ b/src/critic/libs/testing.py @@ -1,3 +1,6 @@ +from collections.abc import Callable +import threading + import boto3 from polyfactory.factories.pydantic_factory import ModelFactory from pydantic import BaseModel @@ -102,6 +105,27 @@ def clear_tables(): _clear_table(table_name) +def race(func: Callable, *args, **kwargs): + """ + Runs the given function twice at the same time. Useful for testing race conditions. + """ + # Barrier forces threads to start together + barrier = threading.Barrier(2) + + def worker(): + barrier.wait() + func(*args, **kwargs) + + t1 = threading.Thread(target=worker) + t2 = threading.Thread(target=worker) + + t1.start() + t2.start() + + t1.join() + t2.join() + + class PutMixin: __table__: type[Table] diff --git a/src/critic/libs/uptime.py b/src/critic/libs/uptime.py new file mode 100644 index 0000000..ed3a239 --- /dev/null +++ b/src/critic/libs/uptime.py @@ -0,0 +1,150 @@ +from datetime import UTC, datetime, timedelta +from functools import cached_property +import logging +import time + +import httpx + +from critic.libs.dt import round_minute +from critic.models import MonitorState, UptimeLog, UptimeMonitorModel +from critic.tables import UptimeLogTable, UptimeMonitorTable + + +log = logging.getLogger(__name__) + + +class MonitorNotFoundError(ValueError): + pass + + +class UptimeCheck: + """ + This class is responsible for running a single uptime check for a given monitor. It handles + making the request, checking the response, updating the monitor, and saving a log. + """ + + def __init__(self, project_id: str, monitor_slug: str): + self.now = datetime.now(UTC) + self.project_id = project_id + self.monitor_slug = monitor_slug + + self.monitor: UptimeMonitorModel = UptimeMonitorTable.get( + self.project_id, self.monitor_slug + ) + if not self.monitor: + raise MonitorNotFoundError(f'Monitor {project_id}/{monitor_slug} not found') + + # Used internally to make sure we don't duplicate DB operations + self._updated_monitor = False + self._put_log = False + + @cached_property + def new_next_due_at(self): + """Returns what the monitor's next due time should be updated to.""" + # Normally we just add the frequency to the last due time. (Not using now helps prevent + # drift from imprecise scheduling.) + next_due_at = self.monitor.next_due_at + timedelta(minutes=self.monitor.frequency_mins) + + # If we do that and the next due time is still less than right now, that indicates we're + # way behind on checks on this monitor, and we need to "reset" relative to now. + if next_due_at < self.now: + next_due_at = round_minute(self.now) + timedelta(minutes=self.monitor.frequency_mins) + + return next_due_at + + def update_monitor(self, updates: dict | None = None) -> bool: + """ + Updates the monitor with the given updates and also updates the next due time. This method + should only be called once per monitor check. + + Returns True if the update was successful, False if it failed (due to a race condition). + """ + if self._updated_monitor: + raise Exception( + 'Monitor already updated! Do not call this method more than once in one run.' + ) + if updates is None: + updates = {} + updated = UptimeMonitorTable.update( + self.project_id, + self.monitor_slug, + updates={**updates, 'next_due_at': self.new_next_due_at}, + condition={'next_due_at': self.monitor.next_due_at}, + ) + self._updated_monitor = True + return updated + + def make_req(self) -> tuple[httpx.Response | None, float]: + """ + Makes the request and returns the response and the time it took to make the request. + """ + start = time.perf_counter() + with httpx.Client() as client: + try: + response = client.head( + str(self.monitor.url), timeout=float(self.monitor.timeout_secs) + ) + except httpx.TimeoutException: + response = None + finished = time.perf_counter() + latency = finished - start + return response, latency + + def alert(self): + """TODO: alert self.monitor.alert_slack_channels and self.monitor.alert_emails.""" + + def check_resp(self, response: httpx.Response | None) -> tuple[MonitorState, int]: + """Checks the response and returns the new state and consecutive fails. Also alerts if + needed. + """ + state = MonitorState.down + if response: + state = MonitorState.up + # TODO: check assertions + consecutive_fails = 0 if state == MonitorState.up else self.monitor.consecutive_fails + 1 + if consecutive_fails >= self.monitor.failures_before_alerting: + self.alert() + return state, consecutive_fails + + def put_log(self, state: MonitorState, status_code: int, latency: float): + """ + Puts a log for the check. This method should only be called once per monitor check. + """ + if self._put_log: + raise Exception('Log already put! Do not call this method more than once in one run.') + uptime_log = UptimeLog( + monitor_id=f'{self.monitor.project_id}/{self.monitor.slug}', + timestamp=self.now, + status=state, + resp_code=status_code, + latency_secs=latency, + ) + UptimeLogTable.put(uptime_log) + self._put_log = True + + def run(self): + """ + 1. Make the request + 2. Check the response + 3. Update the monitor + 4. Save a log + """ + log.info(f'Starting check for monitor: {self.monitor}') + + # Handle paused by just updating the next due time + if self.monitor.state == MonitorState.paused: + self.update_monitor() + return + + # Make the request + resp, latency = self.make_req() + + # Check the response (also kicks off alerts if needed) + state, consecutive_fails = self.check_resp(resp) + + # Update the monitor + updated = self.update_monitor({'state': state, 'consecutive_fails': consecutive_fails}) + + # Save a log + if updated: + self.put_log(state, resp.status_code if resp else 0, latency) diff --git a/src/critic/models.py b/src/critic/models.py index ec0a46e..935bb6d 100644 --- a/src/critic/models.py +++ b/src/critic/models.py @@ -31,7 +31,9 @@ class UptimeMonitorModel(BaseModel): state: MonitorState = Field(default=MonitorState.new) frequency_mins: int = Field(ge=1, default=1) consecutive_fails: int = Field(ge=0, default=0) - next_due_at: AwareDatetime = Field(default_factory=lambda: datetime.now(UTC)) + next_due_at: AwareDatetime = Field( + default_factory=lambda: datetime.now(UTC).replace(second=0, microsecond=0) + ) timeout_secs: float = Field(ge=0, default=5) assertions: dict[str, Any] = Field(default_factory=dict) failures_before_alerting: int = Field(ge=1, default=1) @@ -44,6 +46,8 @@ class UptimeMonitorModel(BaseModel): @classmethod def validate_next_due_at(cls, v: datetime) -> datetime: """Normalize to UTC""" + if v.second or v.microsecond: + raise ValueError('next_due_at must be no more precise than minutes') return to_utc(v) diff --git a/src/critic/tasks.py b/src/critic/tasks.py index d3a06c7..cc0fe15 100644 --- a/src/critic/tasks.py +++ b/src/critic/tasks.py @@ -1,99 +1,22 @@ -from datetime import UTC, datetime, timedelta +from datetime import UTC, datetime import logging -import time -import httpx import mu -from critic.models import MonitorState, UptimeLog, UptimeMonitorModel -from critic.tables import UptimeLogTable, UptimeMonitorTable +from critic.libs.dt import round_minute +from critic.libs.uptime import MonitorNotFoundError, UptimeCheck +from critic.tables import UptimeMonitorTable log = logging.getLogger(__name__) -# TODO -def send_slack_alerts(monitor: UptimeMonitorModel): - pass - - -# TODO -def send_email_alerts(monitor: UptimeMonitorModel): - pass - - -# TODO -def assertions_pass(monitor: UptimeMonitorModel, repsonse: httpx.Response): - return ( - repsonse is not None - ) # this will handle exceptions from http, but not 404 or other errors - - @mu.task def run_checks(monitor_id: str, monitor_slug: str): - monitor: UptimeMonitorModel = UptimeMonitorTable.get(monitor_id, monitor_slug) - log.info(f'Starting check for monitor: {monitor}') - - # if paused update the time and return - if monitor.state == MonitorState.paused: - UptimeMonitorTable.update( - monitor.project_id, - monitor.slug, - next_due_at=monitor.next_due_at + timedelta(minutes=monitor.frequency_mins), - ) - return - - start = time.perf_counter() - with httpx.Client() as client: - try: - response: httpx.Response = client.head( - str(monitor.url), timeout=float(monitor.timeout_secs) - ) - finished = time.perf_counter() - time_to_ping = finished - start - except httpx.TimeoutException: - response = None - # if we get some error, like a 404 that can be handled in assertions - # if there is a timeout, that should be handled here - time_to_ping = None - - # check response and update state, this will need to work with assertions later on - state = MonitorState.up - consecutive_fails = 0 - if not assertions_pass(monitor, response): - state = MonitorState.down - consecutive_fails = monitor.consecutive_fails + 1 - if consecutive_fails >= monitor.failures_before_alerting: - if monitor.alert_slack_channels: - send_slack_alerts(monitor) - if monitor.alert_emails: - send_email_alerts(monitor) - - # update ddb, should only need to send keys, state and nextdue - UptimeMonitorTable.update( - monitor.project_id, - monitor.slug, - state=state, - consecutive_fails=consecutive_fails, - next_due_at=monitor.next_due_at + timedelta(minutes=monitor.frequency_mins), - ) - - response_code = response.status_code if response else None - # update logs - uptime_log = UptimeLog( - monitor_id=f'{monitor.project_id}/{monitor.slug}', - timestamp=datetime.now(UTC), - status=monitor.state, - resp_code=response_code, - latency_secs=time_to_ping, - ) - - if uptime_log.resp_code is None: - uptime_log.resp_code = 0 - if uptime_log.latency_secs is None: - uptime_log.latency_secs = -1 - - UptimeLogTable.put(uptime_log) + try: + UptimeCheck(monitor_id, monitor_slug).run() + except MonitorNotFoundError: + log.info(f'Monitor {monitor_id}/{monitor_slug} not found, skipping') @mu.task @@ -105,12 +28,8 @@ def run_due_checks(): now = datetime.now(UTC) log.info(f'Triggering due checks at {now.isoformat()}') - # Round `now` to the nearest minute in case there is a slight inaccuracy in scheduling - rounded_now = now.replace(second=0, microsecond=0) - if now.second >= 30: - rounded_now = rounded_now + timedelta(minutes=1) - # Trigger `run_checks` for each due monitor. + rounded_now = round_minute(now) due_monitors = UptimeMonitorTable.get_due_since(rounded_now) for monitor in due_monitors: run_checks.invoke(str(monitor.project_id), monitor.slug) diff --git a/tests/critic_tests/test_models.py b/tests/critic_tests/test_models.py index 9548935..efcbb2a 100644 --- a/tests/critic_tests/test_models.py +++ b/tests/critic_tests/test_models.py @@ -13,3 +13,11 @@ def test_next_due_at_utc(self): def test_next_due_at_unaware(self): with pytest.raises(ValueError, match='Input should have timezone info'): UptimeMonitorFactory.build(next_due_at='2026-01-01 12:00:00') + + def test_next_due_at_second(self): + with pytest.raises(ValueError, match='next_due_at must be no more precise than minutes'): + UptimeMonitorFactory.build(next_due_at='2026-01-01 12:00:01Z') + + def test_next_due_at_microsecond(self): + with pytest.raises(ValueError, match='next_due_at must be no more precise than minutes'): + UptimeMonitorFactory.build(next_due_at='2026-01-01 12:00:00.000001Z') diff --git a/tests/critic_tests/test_tasks.py b/tests/critic_tests/test_tasks.py index 605f3c0..ed30723 100644 --- a/tests/critic_tests/test_tasks.py +++ b/tests/critic_tests/test_tasks.py @@ -1,3 +1,4 @@ +from datetime import UTC, datetime import logging from unittest import mock @@ -5,7 +6,7 @@ import httpx import pytest -from critic.libs.testing import UptimeMonitorFactory +from critic.libs.testing import UptimeMonitorFactory, race from critic.models import MonitorState, UptimeLog, UptimeMonitorModel from critic.tables import UptimeLogTable, UptimeMonitorTable from critic.tasks import run_checks, run_due_checks @@ -72,10 +73,9 @@ def test_down_with_consec_fails_above_threshold(self, httpx_mock): monitor_id = f'{monitor.project_id}/{monitor.slug}' response: UptimeLog = UptimeLogTable.query(monitor_id)[-1] - # log should have resp of 0 since there was a timeout, and a latency of -1 + # log should have resp of 0 since there was a timeout assert response.status == MonitorState.down assert response.resp_code == 0 - assert response.latency_secs == -1 def test_down_with_consec_fails_below_threshold(self, httpx_mock): monitor: UptimeMonitorModel = UptimeMonitorFactory.put( @@ -94,10 +94,9 @@ def test_down_with_consec_fails_below_threshold(self, httpx_mock): monitor_id = f'{monitor.project_id}/{monitor.slug}' response: UptimeLog = UptimeLogTable.query(monitor_id)[-1] - # log should have resp of 0 since there was a timeout, and a latency of -1 - assert response.status == MonitorState.up + # log should have resp of 0 since there was a timeout + assert response.status == MonitorState.down assert response.resp_code == 0 - assert response.latency_secs == -1 # what are we doing for next due time when paused? def test_paused(self): @@ -116,3 +115,40 @@ def test_paused(self): response: UptimeLog = UptimeLogTable.query(monitor_id) # does not have item because no log is created since the monitor is paused assert response == [] + + @freeze_time('2026-02-01 12:00:13', tz_offset=0) + def test_old_next_due_at(self, httpx_mock): + httpx_mock.add_response() + + # Pretend critic went down for a month (from Jan 1st to Feb 1st). Next due at is much older + # than we would expect. + monitor: UptimeMonitorModel = UptimeMonitorFactory.put( + next_due_at='2026-01-01 12:00:00Z', + frequency_mins=5, + ) + + run_checks(str(monitor.project_id), monitor.slug) + + # Next due at should be calculated from the current time + monitor: UptimeMonitorModel = UptimeMonitorTable.get(monitor.project_id, monitor.slug) + assert monitor.next_due_at == datetime(2026, 2, 1, 12, 5, 0, tzinfo=UTC) + + @freeze_time('2026-02-10 10:45:00', tz_offset=0) + def test_race(self, httpx_mock): + # Add a successful response for each call + httpx_mock.add_response() + httpx_mock.add_response() + + monitor: UptimeMonitorModel = UptimeMonitorFactory.put( + next_due_at='2026-02-10 10:45:00Z', + frequency_mins=5, + ) + + race(run_checks, str(monitor.project_id), monitor.slug) + + monitor = UptimeMonitorTable.get(monitor.project_id, monitor.slug) + assert monitor.next_due_at == datetime(2026, 2, 10, 10, 50, 0, tzinfo=UTC) + + monitor_id = f'{monitor.project_id}/{monitor.slug}' + logs = UptimeLogTable.query(monitor_id) + assert len(logs) == 1 From 8ce64ffb869d2b82f3a9b0c968055ee948f5fbf3 Mon Sep 17 00:00:00 2001 From: Caleb Syring Date: Tue, 10 Feb 2026 11:12:42 -0500 Subject: [PATCH 3/4] bump pip --- uv.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/uv.lock b/uv.lock index 3cc092d..79c8971 100644 --- a/uv.lock +++ b/uv.lock @@ -1330,11 +1330,11 @@ wheels = [ [[package]] name = "pip" -version = "25.3" +version = "26.0.1" source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/fe/6e/74a3f0179a4a73a53d66ce57fdb4de0080a8baa1de0063de206d6167acc2/pip-25.3.tar.gz", hash = "sha256:8d0538dbbd7babbd207f261ed969c65de439f6bc9e5dbd3b3b9a77f25d95f343", size = 1803014, upload-time = "2025-10-25T00:55:41.394Z" } +sdist = { url = "https://files.pythonhosted.org/packages/48/83/0d7d4e9efe3344b8e2fe25d93be44f64b65364d3c8d7bc6dc90198d5422e/pip-26.0.1.tar.gz", hash = "sha256:c4037d8a277c89b320abe636d59f91e6d0922d08a05b60e85e53b296613346d8", size = 1812747, upload-time = "2026-02-05T02:20:18.702Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/44/3c/d717024885424591d5376220b5e836c2d5293ce2011523c9de23ff7bf068/pip-25.3-py3-none-any.whl", hash = "sha256:9655943313a94722b7774661c21049070f6bbb0a1516bf02f7c8d5d9201514cd", size = 1778622, upload-time = "2025-10-25T00:55:39.247Z" }, + { url = "https://files.pythonhosted.org/packages/de/f0/c81e05b613866b76d2d1066490adf1a3dbc4ee9d9c839961c3fc8a6997af/pip-26.0.1-py3-none-any.whl", hash = "sha256:bdb1b08f4274833d62c1aa29e20907365a2ceb950410df15fc9521bad440122b", size = 1787723, upload-time = "2026-02-05T02:20:16.416Z" }, ] [[package]] From a617bbc3e327d0a57a3192da8588993e91855f7a Mon Sep 17 00:00:00 2001 From: Caleb Syring Date: Tue, 10 Feb 2026 12:06:25 -0500 Subject: [PATCH 4/4] improve coverage --- src/critic/libs/ddb.py | 1 + src/critic/libs/testing.py | 24 --- tests/critic_tests/test_libs/test_ddb.py | 68 +++++---- tests/critic_tests/test_libs/test_uptime.py | 160 ++++++++++++++++++++ tests/critic_tests/test_tasks.py | 142 ++--------------- 5 files changed, 216 insertions(+), 179 deletions(-) create mode 100644 tests/critic_tests/test_libs/test_uptime.py diff --git a/src/critic/libs/ddb.py b/src/critic/libs/ddb.py index 6cfbf31..768cba4 100644 --- a/src/critic/libs/ddb.py +++ b/src/critic/libs/ddb.py @@ -217,6 +217,7 @@ def update( except ClientError as e: if e.response['Error']['Code'] == 'ConditionalCheckFailedException': return False + raise return True @classmethod diff --git a/src/critic/libs/testing.py b/src/critic/libs/testing.py index e2948f3..17a1b76 100644 --- a/src/critic/libs/testing.py +++ b/src/critic/libs/testing.py @@ -1,6 +1,3 @@ -from collections.abc import Callable -import threading - import boto3 from polyfactory.factories.pydantic_factory import ModelFactory from pydantic import BaseModel @@ -105,27 +102,6 @@ def clear_tables(): _clear_table(table_name) -def race(func: Callable, *args, **kwargs): - """ - Runs the given function twice at the same time. Useful for testing race conditions. - """ - # Barrier forces threads to start together - barrier = threading.Barrier(2) - - def worker(): - barrier.wait() - func(*args, **kwargs) - - t1 = threading.Thread(target=worker) - t2 = threading.Thread(target=worker) - - t1.start() - t2.start() - - t1.join() - t2.join() - - class PutMixin: __table__: type[Table] diff --git a/tests/critic_tests/test_libs/test_ddb.py b/tests/critic_tests/test_libs/test_ddb.py index a6ff7f8..5f280c2 100644 --- a/tests/critic_tests/test_libs/test_ddb.py +++ b/tests/critic_tests/test_libs/test_ddb.py @@ -1,5 +1,7 @@ from datetime import datetime +from unittest import mock +from botocore.exceptions import ClientError import pytest from critic.models import UptimeMonitorModel @@ -67,29 +69,43 @@ def test_missing_sort_key(self): with pytest.raises(ValueError): UptimeMonitorTable.get('6033aa47-a9f7-4d7f-b7ff-a11ba9b34474') - -def test_query_from_monitor_table(): - in_data = { - 'project_id': '6033aa47-a9f7-4d7f-b7ff-a11ba9b34474', - 'slug': 'my-monitor', - 'url': 'https://example.com/health', - 'frequency_mins': 5, - 'consecutive_fails': 0, - 'next_due_at': '2025-11-10T20:35:00Z', - 'timeout_secs': 30, - 'assertions': {'status_code': 200, 'body_contains': 'OK'}, - 'failures_before_alerting': 2, - 'alert_slack_channels': ['#ops'], - 'alert_emails': ['alerts@example.com'], - 'realert_interval_mins': 60, - } - in_data = UptimeMonitorModel(**in_data) - UptimeMonitorTable.put(in_data) - out_data = UptimeMonitorTable.query('6033aa47-a9f7-4d7f-b7ff-a11ba9b34474') - assert len(out_data) == 1 - assert str(out_data[0].url) == 'https://example.com/health' - - -def test_serialize_unaware_dt(): - with pytest.raises(ValueError, match='must be timezone aware'): - UptimeMonitorTable.get_due_since(datetime.now()) + def test_query_from_monitor_table(self): + in_data = { + 'project_id': '6033aa47-a9f7-4d7f-b7ff-a11ba9b34474', + 'slug': 'my-monitor', + 'url': 'https://example.com/health', + 'frequency_mins': 5, + 'consecutive_fails': 0, + 'next_due_at': '2025-11-10T20:35:00Z', + 'timeout_secs': 30, + 'assertions': {'status_code': 200, 'body_contains': 'OK'}, + 'failures_before_alerting': 2, + 'alert_slack_channels': ['#ops'], + 'alert_emails': ['alerts@example.com'], + 'realert_interval_mins': 60, + } + in_data = UptimeMonitorModel(**in_data) + UptimeMonitorTable.put(in_data) + out_data = UptimeMonitorTable.query('6033aa47-a9f7-4d7f-b7ff-a11ba9b34474') + assert len(out_data) == 1 + assert str(out_data[0].url) == 'https://example.com/health' + + def test_serialize_unaware_dt(self): + with pytest.raises(ValueError, match='must be timezone aware'): + UptimeMonitorTable.get_due_since(datetime.now()) + + def test_update_no_updates(self): + with pytest.raises(ValueError, match='No updates provided'): + UptimeMonitorTable.update('6033aa47-a9f7-4d7f-b7ff-a11ba9b34474', 'my-monitor', {}) + + @mock.patch('critic.libs.ddb.get_client') + def test_update_error_not_conditional(self, m_get_client): + # We supress conditional check errors. Make sure that doesn't inadvertently suppress other + # errors. + error = ClientError({'Error': {'Code': 'SomeOtherError'}}, 'update_item') + m_get_client.return_value.update_item.side_effect = error + with pytest.raises(ClientError) as excinfo: + UptimeMonitorTable.update( + '6033aa47-a9f7-4d7f-b7ff-a11ba9b34474', 'my-monitor', {'a': 1} + ) + assert excinfo.value == error diff --git a/tests/critic_tests/test_libs/test_uptime.py b/tests/critic_tests/test_libs/test_uptime.py new file mode 100644 index 0000000..6639ef6 --- /dev/null +++ b/tests/critic_tests/test_libs/test_uptime.py @@ -0,0 +1,160 @@ +from datetime import UTC, datetime +import logging + +from freezegun import freeze_time +import httpx +import pytest + +from critic.libs.testing import UptimeMonitorFactory +from critic.libs.uptime import MonitorNotFoundError, UptimeCheck +from critic.models import MonitorState, UptimeLog, UptimeMonitorModel +from critic.tables import UptimeLogTable, UptimeMonitorTable + + +class TestUptimeCheck: + def test_not_found(self): + with pytest.raises(MonitorNotFoundError): + UptimeCheck('6033aa47-a9f7-4d7f-b7ff-a11ba9b34474', 'my-monitor') + + def test_duplicate_update(self): + monitor = UptimeMonitorFactory.put() + check = UptimeCheck(str(monitor.project_id), monitor.slug) + check.update_monitor() + with pytest.raises(Exception, match='Monitor already updated'): + check.update_monitor() + + def test_duplicate_log(self): + monitor = UptimeMonitorFactory.put() + check = UptimeCheck(str(monitor.project_id), monitor.slug) + check.put_log(MonitorState.up, 200, 0.1) + with pytest.raises(Exception, match='Log already put'): + check.put_log(MonitorState.up, 200, 0.1) + + def test_race_condition(self, httpx_mock): + monitor = UptimeMonitorFactory.put(next_due_at='2026-02-10 11:50:00Z') + + # Initialize the check + check = UptimeCheck(str(monitor.project_id), monitor.slug) + + # Simulate a check running after the first one initialized + UptimeMonitorTable.update( + monitor.project_id, monitor.slug, updates={'next_due_at': '2026-02-10 12:00:00Z'} + ) + + # Run the check + httpx_mock.add_response() + check.run() + + # Next due at remains the same + monitor = UptimeMonitorTable.get(monitor.project_id, monitor.slug) + assert monitor.next_due_at == datetime(2026, 2, 10, 12, 0, 0, tzinfo=UTC) + + # No log is created + logs = UptimeLogTable.query(f'{monitor.project_id}/{monitor.slug}') + assert len(logs) == 0 + + def test_run_up(self, caplog, httpx_mock): + monitor: UptimeMonitorModel = UptimeMonitorFactory.put( + consecutive_fails=1, failures_before_alerting=2, state=MonitorState.up + ) + + caplog.set_level(logging.INFO) + + time_to_check = monitor.next_due_at + + httpx_mock.add_response() + UptimeCheck(str(monitor.project_id), monitor.slug).run() + + # check ddb entries + assert 'Starting check' in caplog.text # make sure method is sending log + response: UptimeMonitorModel = UptimeMonitorTable.get(monitor.project_id, monitor.slug) + + # check that monitor is up, next due at is later, and consecutive fails is 0 because passing + assert response.state == MonitorState.up + assert response.next_due_at > time_to_check + assert response.consecutive_fails == 0 + + monitor_id = f'{monitor.project_id}/{monitor.slug}' + response: UptimeLog = UptimeLogTable.query(monitor_id)[-1] + + # check logging stuff + assert response.status == MonitorState.up + assert response.resp_code > 0 + assert response.latency_secs > 0 + + def test_down_with_consec_fails_above_threshold(self, httpx_mock): + monitor: UptimeMonitorModel = UptimeMonitorFactory.put( + consecutive_fails=1, + failures_before_alerting=2, + state=MonitorState.down, + ) + + httpx_mock.add_exception(httpx.TimeoutException('Connection timed out')) + UptimeCheck(str(monitor.project_id), monitor.slug).run() + + response: UptimeMonitorModel = UptimeMonitorTable.get(monitor.project_id, monitor.slug) + # Monitor should be down with 2 consec fails + assert response.state == MonitorState.down + assert response.consecutive_fails == 2 + + monitor_id = f'{monitor.project_id}/{monitor.slug}' + response: UptimeLog = UptimeLogTable.query(monitor_id)[-1] + # log should have resp of 0 since there was a timeout + assert response.status == MonitorState.down + assert response.resp_code == 0 + + def test_down_with_consec_fails_below_threshold(self, httpx_mock): + monitor: UptimeMonitorModel = UptimeMonitorFactory.put( + consecutive_fails=0, + failures_before_alerting=2, + state=MonitorState.up, + ) + + httpx_mock.add_exception(httpx.TimeoutException('Connection timed out')) + + UptimeCheck(str(monitor.project_id), monitor.slug).run() + + response: UptimeMonitorModel = UptimeMonitorTable.get(monitor.project_id, monitor.slug) + assert response.state == MonitorState.down + assert response.consecutive_fails == 1 + + monitor_id = f'{monitor.project_id}/{monitor.slug}' + response: UptimeLog = UptimeLogTable.query(monitor_id)[-1] + # log should have resp of 0 since there was a timeout + assert response.status == MonitorState.down + assert response.resp_code == 0 + + # what are we doing for next due time when paused? + def test_paused(self): + monitor: UptimeMonitorModel = UptimeMonitorFactory.put( + consecutive_fails=0, + failures_before_alerting=2, + state=MonitorState.paused, + ) + time_to_check = monitor.next_due_at + + UptimeCheck(str(monitor.project_id), monitor.slug).run() + + response: UptimeMonitorModel = UptimeMonitorTable.get(monitor.project_id, monitor.slug) + assert response.next_due_at > time_to_check + monitor_id = f'{monitor.project_id}/{monitor.slug}' + response: UptimeLog = UptimeLogTable.query(monitor_id) + # does not have item because no log is created since the monitor is paused + assert response == [] + + @freeze_time('2026-02-01 12:00:13', tz_offset=0) + def test_old_next_due_at(self, httpx_mock): + httpx_mock.add_response() + + # Pretend critic went down for a month (from Jan 1st to Feb 1st). Next due at is much older + # than we would expect. + monitor: UptimeMonitorModel = UptimeMonitorFactory.put( + next_due_at='2026-01-01 12:00:00Z', + frequency_mins=5, + ) + + UptimeCheck(str(monitor.project_id), monitor.slug).run() + + # Next due at should be calculated from the current time + monitor: UptimeMonitorModel = UptimeMonitorTable.get(monitor.project_id, monitor.slug) + assert monitor.next_due_at == datetime(2026, 2, 1, 12, 5, 0, tzinfo=UTC) diff --git a/tests/critic_tests/test_tasks.py b/tests/critic_tests/test_tasks.py index ed30723..b2f1d4b 100644 --- a/tests/critic_tests/test_tasks.py +++ b/tests/critic_tests/test_tasks.py @@ -1,14 +1,11 @@ -from datetime import UTC, datetime import logging from unittest import mock from freezegun import freeze_time -import httpx import pytest -from critic.libs.testing import UptimeMonitorFactory, race -from critic.models import MonitorState, UptimeLog, UptimeMonitorModel -from critic.tables import UptimeLogTable, UptimeMonitorTable +from critic.libs.testing import UptimeMonitorFactory +from critic.libs.uptime import MonitorNotFoundError from critic.tasks import run_checks, run_due_checks @@ -27,128 +24,15 @@ def test_run_due_checks(self, m_run_check, kickoff_time): class TestRunChecks: - def test_up(self, caplog, httpx_mock): - monitor: UptimeMonitorModel = UptimeMonitorFactory.put( - consecutive_fails=1, failures_before_alerting=2, state=MonitorState.up - ) - + @mock.patch('critic.tasks.UptimeCheck') + def test_runs_check(self, m_uptime_check): + run_checks('6033aa47-a9f7-4d7f-b7ff-a11ba9b34474', 'my-monitor') + m_uptime_check.assert_called_once_with('6033aa47-a9f7-4d7f-b7ff-a11ba9b34474', 'my-monitor') + m_uptime_check.return_value.run.assert_called_once_with() + + @mock.patch('critic.tasks.UptimeCheck') + def test_handles_not_found(self, m_uptime_check, caplog): caplog.set_level(logging.INFO) - - time_to_check = monitor.next_due_at - - httpx_mock.add_response() - run_checks(monitor.project_id, monitor.slug) - - # check ddb entries - assert 'Starting check' in caplog.text # make sure method is sending log - response: UptimeMonitorModel = UptimeMonitorTable.get(monitor.project_id, monitor.slug) - - # check that monitor is up, next due at is later, and consecutive fails is 0 because passing - assert response.state == MonitorState.up - assert response.next_due_at > time_to_check - assert response.consecutive_fails == 0 - - monitor_id = f'{monitor.project_id}/{monitor.slug}' - response: UptimeLog = UptimeLogTable.query(monitor_id)[-1] - - # check logging stuff - assert response.status == MonitorState.up - assert response.resp_code > 0 - assert response.latency_secs > 0 - - def test_down_with_consec_fails_above_threshold(self, httpx_mock): - monitor: UptimeMonitorModel = UptimeMonitorFactory.put( - consecutive_fails=1, - failures_before_alerting=2, - state=MonitorState.down, - ) - - httpx_mock.add_exception(httpx.TimeoutException('Connection timed out')) - run_checks(monitor.project_id, monitor.slug) - - response: UptimeMonitorModel = UptimeMonitorTable.get(monitor.project_id, monitor.slug) - # Monitor should be down with 2 consec fails - assert response.state == MonitorState.down - assert response.consecutive_fails == 2 - - monitor_id = f'{monitor.project_id}/{monitor.slug}' - response: UptimeLog = UptimeLogTable.query(monitor_id)[-1] - # log should have resp of 0 since there was a timeout - assert response.status == MonitorState.down - assert response.resp_code == 0 - - def test_down_with_consec_fails_below_threshold(self, httpx_mock): - monitor: UptimeMonitorModel = UptimeMonitorFactory.put( - consecutive_fails=0, - failures_before_alerting=2, - state=MonitorState.up, - ) - - httpx_mock.add_exception(httpx.TimeoutException('Connection timed out')) - - run_checks(monitor.project_id, monitor.slug) - - response: UptimeMonitorModel = UptimeMonitorTable.get(monitor.project_id, monitor.slug) - assert response.state == MonitorState.down - assert response.consecutive_fails == 1 - - monitor_id = f'{monitor.project_id}/{monitor.slug}' - response: UptimeLog = UptimeLogTable.query(monitor_id)[-1] - # log should have resp of 0 since there was a timeout - assert response.status == MonitorState.down - assert response.resp_code == 0 - - # what are we doing for next due time when paused? - def test_paused(self): - monitor: UptimeMonitorModel = UptimeMonitorFactory.put( - consecutive_fails=0, - failures_before_alerting=2, - state=MonitorState.paused, - ) - time_to_check = monitor.next_due_at - - run_checks(monitor.project_id, monitor.slug) - - response: UptimeMonitorModel = UptimeMonitorTable.get(monitor.project_id, monitor.slug) - assert response.next_due_at > time_to_check - monitor_id = f'{monitor.project_id}/{monitor.slug}' - response: UptimeLog = UptimeLogTable.query(monitor_id) - # does not have item because no log is created since the monitor is paused - assert response == [] - - @freeze_time('2026-02-01 12:00:13', tz_offset=0) - def test_old_next_due_at(self, httpx_mock): - httpx_mock.add_response() - - # Pretend critic went down for a month (from Jan 1st to Feb 1st). Next due at is much older - # than we would expect. - monitor: UptimeMonitorModel = UptimeMonitorFactory.put( - next_due_at='2026-01-01 12:00:00Z', - frequency_mins=5, - ) - - run_checks(str(monitor.project_id), monitor.slug) - - # Next due at should be calculated from the current time - monitor: UptimeMonitorModel = UptimeMonitorTable.get(monitor.project_id, monitor.slug) - assert monitor.next_due_at == datetime(2026, 2, 1, 12, 5, 0, tzinfo=UTC) - - @freeze_time('2026-02-10 10:45:00', tz_offset=0) - def test_race(self, httpx_mock): - # Add a successful response for each call - httpx_mock.add_response() - httpx_mock.add_response() - - monitor: UptimeMonitorModel = UptimeMonitorFactory.put( - next_due_at='2026-02-10 10:45:00Z', - frequency_mins=5, - ) - - race(run_checks, str(monitor.project_id), monitor.slug) - - monitor = UptimeMonitorTable.get(monitor.project_id, monitor.slug) - assert monitor.next_due_at == datetime(2026, 2, 10, 10, 50, 0, tzinfo=UTC) - - monitor_id = f'{monitor.project_id}/{monitor.slug}' - logs = UptimeLogTable.query(monitor_id) - assert len(logs) == 1 + m_uptime_check.side_effect = MonitorNotFoundError + run_checks('6033aa47-a9f7-4d7f-b7ff-a11ba9b34474', 'my-monitor') + assert 'not found, skipping' in caplog.text