-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat(signals): auto-pause scouts that keep failing, and name the wall #74458
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.
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
Changes from all commits
1e5091b
e20dc9a
4dce239
19786b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [("signals", "0078_backfill_scout_status")] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name="signalscoutconfig", | ||
| name="consecutive_failure_count", | ||
| field=models.PositiveIntegerField(db_default=0, default=0), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| from django.db import migrations | ||
|
|
||
|
|
||
| def reconcile_enabled_wins(apps, schema_editor): | ||
| # Re-runs 0078's enabled-wins reconciliation over rows old instances drifted during the | ||
| # 0077 rollout window (enabled-only writes from pods that predated the dual-write land | ||
| # after the backfill). `enabled` carries the operator's intent for any mismatch: no | ||
| # system writer existed before the constraints in 0081, so a disagreeing row can only be | ||
| # a human toggle that missed the `status` side. Small table, plain UPDATEs. | ||
| SignalScoutConfig = apps.get_model("signals", "SignalScoutConfig") | ||
| manager = SignalScoutConfig._default_manager | ||
| manager.filter(enabled=False, status__in=["active", "pending_pause"]).update( | ||
| status="paused_by_user", pause_reason=None | ||
| ) | ||
| manager.filter(enabled=True, status__in=["paused_by_system", "paused_by_user"]).update( | ||
| status="active", pause_reason=None | ||
| ) | ||
| # Reason coherence for 0081's second constraint: a reason on a runnable/user-paused row is | ||
| # stray; a reasonless warning is meaningless (runnable anyway); a reasonless system pause | ||
| # can only predate system writers, so it reads as a human's off-switch. | ||
| manager.filter(status__in=["active", "paused_by_user"], pause_reason__isnull=False).update(pause_reason=None) | ||
| manager.filter(status="pending_pause", pause_reason__isnull=True).update(status="active") | ||
| manager.filter(status="paused_by_system", pause_reason__isnull=True).update(status="paused_by_user") | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("signals", "0079_signalscoutconfig_consecutive_failure_count"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RunPython(reconcile_enabled_wins, reverse_code=migrations.RunPython.noop), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| from django.db import migrations, models | ||
|
|
||
| from posthog.migration_helpers import AddConstraintNotValid | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("signals", "0080_reconcile_scout_status_enabled"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| AddConstraintNotValid( | ||
| model_name="signalscoutconfig", | ||
| constraint=models.CheckConstraint( | ||
| name="scout_config_enabled_matches_status", | ||
| condition=models.Q(enabled=True, status__in=["active", "pending_pause"]) | ||
| | models.Q(enabled=False, status__in=["paused_by_system", "paused_by_user"]), | ||
| ), | ||
| ), | ||
| AddConstraintNotValid( | ||
| model_name="signalscoutconfig", | ||
| constraint=models.CheckConstraint( | ||
| name="scout_config_pause_reason_matches_status", | ||
| condition=models.Q(status__in=["pending_pause", "paused_by_system"], pause_reason__isnull=False) | ||
| | models.Q(status__in=["active", "paused_by_user"], pause_reason__isnull=True), | ||
| ), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| from django.db import migrations | ||
|
|
||
| from posthog.migration_helpers import ValidateConstraint | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("signals", "0081_scout_status_constraints"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| ValidateConstraint(model_name="signalscoutconfig", name="scout_config_enabled_matches_status"), | ||
| ValidateConstraint(model_name="signalscoutconfig", name="scout_config_pause_reason_matches_status"), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 0078_backfill_scout_status | ||
| 0082_validate_scout_status_constraints |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1237,6 +1237,14 @@ class PauseReason(models.TextChoices): | |
| # Stamped by the coordinator after each dispatch; drives the due-check. Written every | ||
| # run, so it is excluded from activity logging (see field_exclusions below). | ||
| last_run_at = models.DateTimeField(null=True, blank=True) | ||
| # Failure-streak circuit breaker over this lane's run outcomes, maintained by the runner: | ||
| # bumped on a failed run, zeroed on a successful one. At `FAILURE_STREAK_PAUSE_THRESHOLD` | ||
| # the runner pauses the lane (`transition_status_by_system`, `repeated_failures`) and the | ||
| # coordinator holds it to one probe per `AUTO_PAUSE_PROBE_INTERVAL_S`. Without it a lane | ||
| # that can never succeed re-dispatches forever, taking a full-length sandbox lease per | ||
| # interval to produce nothing. Written on every run, so it is excluded from activity | ||
| # logging like `last_run_at`; the pause itself logs through `status` like any other. | ||
| consecutive_failure_count = models.PositiveIntegerField(default=0, db_default=0) | ||
|
andrewm4894 marked this conversation as resolved.
|
||
| created_at = models.DateTimeField(auto_now_add=True) | ||
| updated_at = models.DateTimeField(auto_now=True) | ||
| created_by = models.ForeignKey( | ||
|
|
@@ -1261,13 +1269,20 @@ class Meta: | |
| default_manager_name = "all_teams" | ||
| constraints = [ | ||
| models.UniqueConstraint(fields=["team", "skill_name"], name="unique_scout_config_per_team_skill"), | ||
| # A CHECK constraint tying `enabled` to `status` is deliberately deferred to a | ||
| # follow-up migration: enforcing it in the same deploy that introduces the | ||
| # dual-write breaks rolling deploys, because not-yet-replaced instances still | ||
| # write `enabled` alone and a NOT VALID constraint already checks new writes. | ||
| # That follow-up must first re-run the enabled-wins reconciliation over drifted | ||
| # rows (enabled-only writes from old instances during the rollout window land | ||
| # after the 0075 backfill), then add and validate the constraint. | ||
| # Backstop for the dual-write in `save`: added NOT VALID + validated (0080–0082) | ||
| # only after the 0077 deploy fully rolled, because enforcing it in the same deploy | ||
| # that introduced the dual-write would break not-yet-replaced instances that still | ||
| # write `enabled` alone. | ||
| models.CheckConstraint( | ||
| name="scout_config_enabled_matches_status", | ||
| condition=models.Q(enabled=True, status__in=["active", "pending_pause"]) | ||
| | models.Q(enabled=False, status__in=["paused_by_system", "paused_by_user"]), | ||
| ), | ||
| models.CheckConstraint( | ||
| name="scout_config_pause_reason_matches_status", | ||
| condition=models.Q(status__in=["pending_pause", "paused_by_system"], pause_reason__isnull=False) | ||
| | models.Q(status__in=["active", "paused_by_user"], pause_reason__isnull=True), | ||
| ), | ||
| ] | ||
|
|
||
| def save(self, *args: Any, **kwargs: Any) -> None: | ||
|
|
@@ -1329,7 +1344,18 @@ def transition_status_by_system( | |
| if new_status == self.Status.PAUSED_BY_USER: | ||
| raise ValueError("Only a user write may set paused_by_user.") | ||
| with transaction.atomic(): | ||
| locked = type(self).all_teams.select_for_update().get(pk=self.pk) | ||
| # One ordered query locks the whole team's rows, not just ours: the cap check below | ||
| # counts sibling rows, so two concurrent resumes locking only their own rows would | ||
| # each read the other as still paused and both slip under the cap. A single ordered | ||
| # lock set also keeps concurrent transitions on one team deadlock-free. Team config | ||
| # counts are small (capped) and transitions rare, so the wider lock is cheap. | ||
| team_rows = { | ||
| row.pk: row | ||
| for row in type(self).all_teams.select_for_update().filter(team_id=self.team_id).order_by("pk") | ||
| } | ||
| locked = team_rows.get(self.pk) | ||
| if locked is None: | ||
| return False | ||
| if locked.status == self.Status.PAUSED_BY_USER: | ||
| return False | ||
| if locked.status != self.Status.ACTIVE and locked.pause_reason != pause_reason: | ||
|
|
@@ -1351,7 +1377,7 @@ def transition_status_by_system( | |
| ) | ||
|
|
||
| if new_status in self.RUNNABLE_STATUSES and locked.status not in self.RUNNABLE_STATUSES: | ||
| peers = type(self).all_teams.filter(team_id=locked.team_id, enabled=True).exclude(pk=locked.pk).count() | ||
| peers = sum(1 for row in team_rows.values() if row.enabled and row.pk != locked.pk) | ||
| if peers >= MAX_ENABLED_SCOUTS_PER_TEAM: | ||
| return False | ||
| recorded_reason = None if new_status == self.Status.ACTIVE else pause_reason | ||
|
|
@@ -1362,17 +1388,28 @@ def transition_status_by_system( | |
| locked.status_changed_at = timezone.now() | ||
| locked.status_changed_by = None | ||
| locked.enabled = new_status in self.RUNNABLE_STATUSES | ||
| locked.save( | ||
| update_fields=[ | ||
| "status", | ||
| "pause_reason", | ||
| "status_changed_at", | ||
| "status_changed_by", | ||
| "enabled", | ||
| "updated_at", | ||
| ] | ||
| ) | ||
| for field in ("status", "pause_reason", "status_changed_at", "status_changed_by", "enabled"): | ||
| update_fields = [ | ||
| "status", | ||
| "pause_reason", | ||
| "status_changed_at", | ||
| "status_changed_by", | ||
| "enabled", | ||
| "updated_at", | ||
| ] | ||
| if new_status == self.Status.ACTIVE: | ||
| # A resume always starts with a clean failure streak — otherwise the very next | ||
| # failed run would re-trip the breaker off stale evidence. | ||
| locked.consecutive_failure_count = 0 | ||
| update_fields.append("consecutive_failure_count") | ||
|
Comment on lines
+1399
to
+1403
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an operator resumes a breaker-paused scout through Django admin, the writable Useful? React with 👍 / 👎. |
||
| locked.save(update_fields=update_fields) | ||
| for field in ( | ||
| "status", | ||
| "pause_reason", | ||
| "status_changed_at", | ||
| "status_changed_by", | ||
| "enabled", | ||
| "consecutive_failure_count", | ||
| ): | ||
| setattr(self, field, getattr(locked, field)) | ||
| return True | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.