Repository navigation
Conversation
#152's first criterion — "a slow or failing agent workload cannot starve approval, scheduling or reconciliation" — was structurally in place and never demonstrated. Production could not demonstrate it: across the 44 h baseline to 2026-09-03 the control pool never fell below 96 free slots of 100, so the isolation was never exercised in either direction. A criterion that has never been put under the load it describes is untested, not met. So the load is staged deterministically instead, with no model quota spent: CONTROL_MAX_CONCURRENT_ACTIVITIES + 20 slow activities holding execution-queue slots the way a two-hour submit_and_wait poll does, and short control activities racing beside them. Two tests, because the first alone proves nothing: - a saturated execution queue lets all ten control probes through; - the identical load on ONE queue (the pre-#282 world) lets ZERO through. The second is the mutation, kept as a permanent control rather than run once by hand — without it the first test is satisfied by any topology whatsoever, since ten short activities can always run somewhere. Measured: 10/10 with the split, 0/10 without. The assertion is on probe activities COMPLETING, not on wall-clock latency: latency in a time-skipping environment means nothing, and saturation itself is awaited and asserted before the probe starts, so a run that staged nothing fails instead of passing. Criterion 3, "scaling worker replicas does not create duplicate schedules or duplicate runs", was covered only on a single process. TestConcurrentReplicas adds a shared fake cluster so two replicas can actually race in _ensure_schedule: the loser converges rather than crashing (drop the ScheduleAlreadyRunningError branch and it fails), and every schedule comes out of the race with overlap=SKIP, which is what stops two replicas' ticks running side by side (flip it to ALLOW_ALL and it fails). A third test asserting "one schedule per id" was written and deleted: that is guaranteed by the id being a dict key and passes on any implementation, and three tests where one cannot fail are worse than two that can. ADR-008's Non-goals listed HPA alongside quotas and stopped there, leaving Scope's "provide an HPA/replica strategy" undelivered. Replaced with an actual replica strategy: fixed replicas raised by hand, why a CPU-target HPA measures the one signal guaranteed not to move on two I/O-bound waiters, the two signals that together justify a replica, what N>1 is safe against and which test knows it, and execution-worker-first ordering. Refs #152 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AMpSrP38wdw9eskEwUhurQ
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @mashkovd's task in 4m 27s —— View job Review complete — changes requested
Findings: 1 P2, 1 P3
Everything else checked out:
Note: I was unable to execute Verdict posted: CHANGES_REQUESTED (bound to commit |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # Wait for the queue to be genuinely backed up before probing. | ||
| # Without this the probe could run before a single slow activity | ||
| # had taken a slot, and the test would pass having staged nothing. | ||
| await asyncio.wait_for(gate.saturated.wait(), timeout=60) |
There was a problem hiding this comment.
P2: the saturation wait (await asyncio.wait_for(gate.saturated.wait(), timeout=60)) sits outside the try/finally at lines 175-192 that releases gate.released. If saturation never happens in time (CI slowness, or a genuine regression where the execution worker never picks up activities), this raises a bare TimeoutError while the SaturateExecutionQueue workflow's slow activities are still blocked on gate.released.wait(). The gate is never released before async with control, execution: tears down, so the failure surfaces as an opaque timeout/cancellation rather than the clear, purpose-built assertion this file is designed to give. Consider wrapping the saturation wait in the same try/finally (or moving the whole body under one try/finally that always calls gate.released.set()) so any failure path releases the blocked activities before worker teardown.
| async def _slow_activity(gate_key: str) -> str: | ||
| gate = _GATES[gate_key] | ||
| gate.started += 1 | ||
| if gate.started >= EXECUTION_MAX_CONCURRENT_ACTIVITIES: |
There was a problem hiding this comment.
P3: the saturation signal is hardcoded to EXECUTION_MAX_CONCURRENT_ACTIVITIES (40) regardless of which queue actually runs the slow activities. test_the_same_load_on_one_queue_does_starve_control only reaches this threshold because CONTROL_MAX_CONCURRENT_ACTIVITIES (100) happens to exceed it today. If the two constants' relative ordering is ever flipped (e.g. control capacity tuned down, or execution capacity tuned up — plausible given ADR-008's new replica-sizing guidance), that test's saturation wait would time out instead of demonstrating starvation, hitting the P2 issue noted at line 173. Worth a comment noting the dependency, or computing the threshold from the worker actually receiving the load.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P2 (missing try/finally around the saturation wait in test_queue_isolation.py can turn a saturation-timeout failure into an opaque TimeoutError instead of the intended assertion, without releasing blocked activities before worker teardown), 1 P3 (implicit ordering assumption between EXECUTION_MAX_CONCURRENT_ACTIVITIES and CONTROL_MAX_CONCURRENT_ACTIVITIES that the same P2 path would hit if it ever flips). Everything else — the concurrent-replica schedule race test, the ADR-008 replica-strategy writeup — reads correctly and matches the described behaviour in orchestrator/temporal/worker.py.
Antigravity (agy) reviewModel: gemini-3.1-pro-high. Here is my review of the provided diff. Finding 1Severity: P2 Finding 2Severity: P3 Finding 3Severity: P3 |
Closes the two open acceptance criteria of #152.
Refs, notCloses— theissue is closed by comment once the gitops replica change has soaked.
Criterion 1 — "a slow or failing agent workload cannot starve approval, scheduling or reconciliation"
Structurally in place since 1.39.0, never demonstrated. Production could
not demonstrate it: over the 44 h baseline to 2026-09-03 the control pool never
fell below 96 free slots of 100, so the isolation was never exercised in
either direction. Waiting longer would not have helped — the criterion waits on
load, and two more quiet days add no information.
So the load is staged here, deterministically and without spending model quota:
CONTROL_MAX_CONCURRENT_ACTIVITIES + 20slow activities holding execution-queueslots the way a two-hour
submit_and_waitpoll does, with short controlactivities racing beside them.
Two tests, because the first alone proves nothing:
The second test is the mutation, kept as a permanent control rather than run
once by hand. Without it the first is satisfied by any topology whatsoever —
ten short activities can always run somewhere.
Two things the test does on purpose:
time-skipping environment is meaningless.
that staged nothing would pass — the exact "check that skips when its data
is missing" shape.
Criterion 3 — "scaling worker replicas does not create duplicate schedules or duplicate runs"
The schedule half was covered, but only ever on one process:
_FakeClientmodels a single replica, so two of them would each see
existing=Noneand each"create" into their own list.
TestConcurrentReplicasadds a shared fakecluster with a yield between the check and the write, so the race is real:
ScheduleAlreadyRunningErrorescaping would crash-loop every replica after the first, i.e. a scale-out
that scales to one;
overlap=SKIP, which is whatactually prevents two replicas' ticks running side by side.
What is NOT in this PR
replicaCount: 1 → 2onmctl-agents-worker-exec— a mctl-gitops change,landing separately so the code that describes the behaviour is merged and
reviewed before the cluster does it.
ADR-008
## Non-goalslisted HPA next to quotas and stopped, which left Scope's"provide an HPA/replica strategy" undelivered rather than declined. Replaced
with a real strategy: fixed replicas raised by hand; why a CPU-target HPA
measures the one signal guaranteed not to move on two I/O-bound waiters; the
two signals that together justify a replica (a queue's
temporal_worker_task_slots_availableat zero and p95schedule_to_startrising — either alone is not it); what N>1 is safe againstand which test knows it; execution-worker-first ordering.
Verification
ScheduleAlreadyRunningErrorbranchtest_the_loser_of_the_race_converges_instead_of_failingfailsoverlap=SKIP→ALLOW_ALLtest_every_schedule_survives_a_concurrent_boot_with_skipfails793 passed, 2 skipped;
ruff checkclean.One test written and deleted
An earlier
test_two_replicas_booting_together_create_one_scheduleassertedone schedule per id. That is guaranteed by the id being a dict key and passes
on any implementation — it survived the
ALLOW_ALLmutation untouched. Threetests where one cannot fail are worse than two that can, so it is gone and its
one real assertion (both replicas actually raced) moved into the test that can.
Honest remainder
Control still registers
submit_and_wait, becausepatched()memoizes perexecution: pre-1.39.0 executions stay on the control queue for the rest of
their lives, bounded by the 14-day merge watch. Isolation in production is
complete only once those drain. That is a correct partial, not a gap in this
PR — the topology is what is under test here.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AMpSrP38wdw9eskEwUhurQ