fix(governor): address PR review feedback
Reject null governor policy values, remove the unused per-board cadence knob, and await governor shutdown cleanly. Also scope the agent query to governor-managed rows and drop temporary migration server defaults. Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -106,6 +106,15 @@ def _board_update_message(
|
||||
return "\n".join(lines)
|
||||
|
||||
|
||||
def _reject_null_governor_policy_fields(updates: dict[str, object]) -> None:
|
||||
null_fields = sorted(field_name for field_name, value in updates.items() if value is None)
|
||||
if null_fields:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_422_UNPROCESSABLE_CONTENT,
|
||||
detail=f"{', '.join(null_fields)} cannot be null",
|
||||
)
|
||||
|
||||
|
||||
async def _require_gateway_main_agent(session: AsyncSession, gateway: Gateway) -> None:
|
||||
main_agent = (
|
||||
await Agent.objects.filter_by(gateway_id=gateway.id)
|
||||
@@ -512,7 +521,6 @@ def get_auto_heartbeat_governor_policy(
|
||||
"""Get board-scoped auto heartbeat governor policy."""
|
||||
return AutoHeartbeatGovernorPolicyRead(
|
||||
enabled=bool(board.auto_heartbeat_governor_enabled),
|
||||
run_interval_seconds=int(board.auto_heartbeat_governor_run_interval_seconds),
|
||||
ladder=list(board.auto_heartbeat_governor_ladder or []),
|
||||
lead_cap_every=str(board.auto_heartbeat_governor_lead_cap_every),
|
||||
activity_trigger_type=str(board.auto_heartbeat_governor_activity_trigger_type),
|
||||
@@ -530,10 +538,9 @@ async def update_auto_heartbeat_governor_policy(
|
||||
) -> AutoHeartbeatGovernorPolicyRead:
|
||||
"""Patch board-scoped auto heartbeat governor policy."""
|
||||
updates = payload.model_dump(exclude_unset=True)
|
||||
_reject_null_governor_policy_fields(updates)
|
||||
if "enabled" in updates:
|
||||
board.auto_heartbeat_governor_enabled = bool(updates["enabled"])
|
||||
if "run_interval_seconds" in updates:
|
||||
board.auto_heartbeat_governor_run_interval_seconds = int(updates["run_interval_seconds"])
|
||||
if "ladder" in updates:
|
||||
board.auto_heartbeat_governor_ladder = list(updates["ladder"])
|
||||
if "lead_cap_every" in updates:
|
||||
|
||||
@@ -461,6 +461,10 @@ async def lifespan(_: FastAPI) -> AsyncIterator[None]:
|
||||
finally:
|
||||
if governor_task is not None:
|
||||
governor_task.cancel()
|
||||
try:
|
||||
await governor_task
|
||||
except asyncio.CancelledError:
|
||||
pass
|
||||
logger.info("app.lifecycle.stopped")
|
||||
|
||||
|
||||
|
||||
@@ -48,7 +48,6 @@ class Board(TenantScoped, table=True):
|
||||
|
||||
# Auto heartbeat governor policy (board-scoped).
|
||||
auto_heartbeat_governor_enabled: bool = Field(default=True)
|
||||
auto_heartbeat_governor_run_interval_seconds: int = Field(default=300)
|
||||
auto_heartbeat_governor_ladder: list[str] = Field(
|
||||
default_factory=lambda: ["10m", "30m", "1h", "3h", "6h"],
|
||||
sa_column=Column(JSON),
|
||||
|
||||
@@ -6,7 +6,7 @@ import re
|
||||
from enum import Enum
|
||||
from typing import Annotated
|
||||
|
||||
from pydantic import BaseModel, Field, field_validator
|
||||
from pydantic import BaseModel, ConfigDict, Field, field_validator
|
||||
|
||||
|
||||
class ActivityTriggerType(str, Enum):
|
||||
@@ -43,12 +43,6 @@ class AutoHeartbeatGovernorPolicyBase(BaseModel):
|
||||
default=True,
|
||||
description="If false, the governor will not manage heartbeats for this board.",
|
||||
)
|
||||
run_interval_seconds: int = Field(
|
||||
default=300,
|
||||
ge=30,
|
||||
le=24 * 60 * 60,
|
||||
description="Governor run cadence hint (seconds).",
|
||||
)
|
||||
ladder: list[DurationStr] = Field(
|
||||
default_factory=lambda: ["10m", "30m", "1h", "3h", "6h"],
|
||||
description="Backoff ladder values (non-leads).",
|
||||
@@ -94,8 +88,9 @@ class AutoHeartbeatGovernorPolicyRead(AutoHeartbeatGovernorPolicyBase):
|
||||
class AutoHeartbeatGovernorPolicyUpdate(BaseModel):
|
||||
"""Patch model for board-scoped governor policy."""
|
||||
|
||||
model_config = ConfigDict(extra="forbid")
|
||||
|
||||
enabled: bool | None = None
|
||||
run_interval_seconds: int | None = Field(default=None, ge=30, le=24 * 60 * 60)
|
||||
ladder: list[DurationStr] | str | None = None
|
||||
lead_cap_every: DurationStr | None = None
|
||||
activity_trigger_type: ActivityTriggerType | None = None
|
||||
|
||||
@@ -42,7 +42,7 @@ from app.services.openclaw.provisioning import (
|
||||
|
||||
logger = get_logger(__name__)
|
||||
|
||||
# Governor cadence + behaviour (defaults; may be overridden by board policy).
|
||||
# Governor defaults; board policy may override backoff behaviour.
|
||||
DEFAULT_ACTIVE_EVERY = "5m"
|
||||
DEFAULT_LADDER: list[str] = ["10m", "30m", "1h", "3h", "6h"]
|
||||
DEFAULT_LEAD_CAP_EVERY = "1h"
|
||||
@@ -200,7 +200,14 @@ async def run_governor_once() -> None:
|
||||
|
||||
try:
|
||||
now = utcnow()
|
||||
agents = (await session.exec(select(Agent))).all()
|
||||
agents = (
|
||||
await session.exec(
|
||||
select(Agent).where(
|
||||
col(Agent.auto_heartbeat_enabled).is_(True),
|
||||
col(Agent.gateway_id).is_not(None),
|
||||
),
|
||||
)
|
||||
).all()
|
||||
if not agents:
|
||||
return
|
||||
|
||||
|
||||
@@ -63,6 +63,9 @@ def upgrade() -> None:
|
||||
["auto_heartbeat_off"],
|
||||
unique=False,
|
||||
)
|
||||
op.alter_column("agents", "auto_heartbeat_enabled", server_default=None)
|
||||
op.alter_column("agents", "auto_heartbeat_step", server_default=None)
|
||||
op.alter_column("agents", "auto_heartbeat_off", server_default=None)
|
||||
|
||||
op.add_column(
|
||||
"boards",
|
||||
@@ -73,15 +76,6 @@ def upgrade() -> None:
|
||||
server_default=sa.text("true"),
|
||||
),
|
||||
)
|
||||
op.add_column(
|
||||
"boards",
|
||||
sa.Column(
|
||||
"auto_heartbeat_governor_run_interval_seconds",
|
||||
sa.Integer(),
|
||||
nullable=False,
|
||||
server_default="300",
|
||||
),
|
||||
)
|
||||
op.add_column(
|
||||
"boards",
|
||||
sa.Column(
|
||||
@@ -109,13 +103,16 @@ def upgrade() -> None:
|
||||
server_default="B",
|
||||
),
|
||||
)
|
||||
op.alter_column("boards", "auto_heartbeat_governor_enabled", server_default=None)
|
||||
op.alter_column("boards", "auto_heartbeat_governor_ladder", server_default=None)
|
||||
op.alter_column("boards", "auto_heartbeat_governor_lead_cap_every", server_default=None)
|
||||
op.alter_column("boards", "auto_heartbeat_governor_activity_trigger_type", server_default=None)
|
||||
|
||||
|
||||
def downgrade() -> None:
|
||||
op.drop_column("boards", "auto_heartbeat_governor_activity_trigger_type")
|
||||
op.drop_column("boards", "auto_heartbeat_governor_lead_cap_every")
|
||||
op.drop_column("boards", "auto_heartbeat_governor_ladder")
|
||||
op.drop_column("boards", "auto_heartbeat_governor_run_interval_seconds")
|
||||
op.drop_column("boards", "auto_heartbeat_governor_enabled")
|
||||
|
||||
op.drop_index(op.f("ix_agents_auto_heartbeat_off"), table_name="agents")
|
||||
|
||||
@@ -109,7 +109,6 @@ async def test_get_and_patch_policy_round_trip() -> None:
|
||||
"activity_trigger_type": "A",
|
||||
"ladder": ["15m", "45m"],
|
||||
"lead_cap_every": "2h",
|
||||
"run_interval_seconds": 600,
|
||||
}
|
||||
resp = await client.patch(
|
||||
f"/api/v1/boards/{board.id}/auto-heartbeat-governor-policy",
|
||||
@@ -121,7 +120,6 @@ async def test_get_and_patch_policy_round_trip() -> None:
|
||||
assert updated["activity_trigger_type"] == "A"
|
||||
assert updated["ladder"] == ["15m", "45m"]
|
||||
assert updated["lead_cap_every"] == "2h"
|
||||
assert updated["run_interval_seconds"] == 600
|
||||
|
||||
await engine.dispose()
|
||||
|
||||
@@ -149,6 +147,35 @@ async def test_policy_validation_rejects_disabled_duration() -> None:
|
||||
await engine.dispose()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_policy_validation_rejects_nulls_and_unknown_fields() -> None:
|
||||
engine = await _make_engine()
|
||||
session_maker = async_sessionmaker(engine, expire_on_commit=False, class_=AsyncSession)
|
||||
|
||||
async with session_maker() as session:
|
||||
board = await _seed_board(session)
|
||||
|
||||
app = _build_test_app(session_maker, board.id)
|
||||
|
||||
async with AsyncClient(
|
||||
transport=ASGITransport(app=app),
|
||||
base_url="http://test",
|
||||
) as client:
|
||||
null_resp = await client.patch(
|
||||
f"/api/v1/boards/{board.id}/auto-heartbeat-governor-policy",
|
||||
json={"lead_cap_every": None},
|
||||
)
|
||||
assert null_resp.status_code == 422
|
||||
|
||||
extra_resp = await client.patch(
|
||||
f"/api/v1/boards/{board.id}/auto-heartbeat-governor-policy",
|
||||
json={"run_interval_seconds": 600},
|
||||
)
|
||||
assert extra_resp.status_code == 422
|
||||
|
||||
await engine.dispose()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_policy_validation_rejects_empty_ladder() -> None:
|
||||
engine = await _make_engine()
|
||||
|
||||
@@ -73,7 +73,6 @@ type GovernorActivityTriggerType = "A" | "B";
|
||||
|
||||
type AutoHeartbeatGovernorPolicy = {
|
||||
enabled: boolean;
|
||||
run_interval_seconds: number;
|
||||
ladder: string[];
|
||||
lead_cap_every: string;
|
||||
activity_trigger_type: GovernorActivityTriggerType;
|
||||
@@ -1278,31 +1277,6 @@ export default function EditBoardPage() {
|
||||
</div>
|
||||
|
||||
<div className="grid gap-4 md:grid-cols-2">
|
||||
<div className="space-y-2">
|
||||
<label className="text-sm font-medium text-slate-900">
|
||||
Run interval (seconds)
|
||||
</label>
|
||||
<Input
|
||||
type="number"
|
||||
min={30}
|
||||
step={1}
|
||||
value={currentGovernorPolicy.run_interval_seconds}
|
||||
onChange={(event) => {
|
||||
const next = Number.parseInt(event.target.value, 10);
|
||||
setGovernorPolicyDraft({
|
||||
...currentGovernorPolicy,
|
||||
run_interval_seconds: Number.isNaN(next)
|
||||
? 300
|
||||
: Math.max(30, next),
|
||||
});
|
||||
}}
|
||||
disabled={isLoading || saveGovernorPolicyMutation.isPending}
|
||||
/>
|
||||
<p className="text-xs text-slate-500">
|
||||
Hint for cadence; backend enforces 30s minimum.
|
||||
</p>
|
||||
</div>
|
||||
|
||||
<div className="space-y-2">
|
||||
<label className="text-sm font-medium text-slate-900">
|
||||
Activity trigger type
|
||||
@@ -1382,8 +1356,6 @@ export default function EditBoardPage() {
|
||||
setGovernorPolicySaveSuccess(null);
|
||||
saveGovernorPolicyMutation.mutate({
|
||||
enabled: currentGovernorPolicy.enabled,
|
||||
run_interval_seconds:
|
||||
currentGovernorPolicy.run_interval_seconds,
|
||||
ladder: currentGovernorPolicy.ladder,
|
||||
lead_cap_every: currentGovernorPolicy.lead_cap_every,
|
||||
activity_trigger_type:
|
||||
|
||||
Reference in New Issue
Block a user