Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
ebcece8
feat(tasks): schedule one-off runs through API and MCP
oliverb123 Sep 18, 2026
41c1b5f
chore: update OpenAPI generated types
tests-posthog[bot] Sep 18, 2026
7ea6249
Merge branch 'master' into posthog/scheduled-task-api-mcp
oliverb123 Sep 18, 2026
00a6f35
fix(tasks): align scheduled run schemas and validation
oliverb123 Sep 18, 2026
f3d2b87
fix(tasks): preserve scheduled cancellation lifecycle
oliverb123 Sep 18, 2026
d0318fe
fix(tasks): accept UTC-default timestamps in MCP schemas
oliverb123 Sep 18, 2026
69c6e9a
refactor(tasks): simplify scheduled run request handling
oliverb123 Sep 18, 2026
ec4cbf3
fix(tasks): constrain MCP reasoning effort to the API's choices
posthog[bot] Sep 21, 2026
90d16cc
Merge branch 'master' into posthog/scheduled-task-api-mcp
oliverb123 Sep 21, 2026
5b551f8
fix(tasks): recheck scheduled run usage before dispatch
oliverb123 Sep 21, 2026
9e17f7b
chore(tasks): shorten scheduling documentation
oliverb123 Sep 21, 2026
e04cbea
Merge branch 'master' into posthog/scheduled-task-api-mcp
oliverb123 Sep 21, 2026
dcbf96b
fix(tasks): validate scheduled runs before dispatch
oliverb123 Sep 21, 2026
bac64e3
Merge branch 'master' into posthog/scheduled-task-api-mcp
oliverb123 Sep 21, 2026
30282ea
fix(tasks): allow attachments within a 30-day schedule
oliverb123 Sep 21, 2026
7f9e3b8
test(mcp): update unit test snapshots
tests-posthog[bot] Sep 21, 2026
9f321ac
fix(tasks): finalize scheduled attachments and cancellation safely
oliverb123 Sep 24, 2026
6381e3c
Merge branch 'master' into posthog/scheduled-task-api-mcp
oliverb123 Sep 24, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 78 additions & 12 deletions products/tasks/backend/facade/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,7 @@
updated_at=run.updated_at,
completed_at=run.completed_at,
preview_available=task_run_preview_ready(run.state),
scheduled_at=run.scheduled_at,
)


Expand Down Expand Up @@ -2803,7 +2804,7 @@
)

run = TaskRun.objects.filter(pk=run_id).first()
if run is None:
if run is None or (run.scheduled_at is not None and run.queued_at is None):
return
try:
client = sync_connect()
Expand Down Expand Up @@ -3015,13 +3016,14 @@
)


def update_task_run(

Check warning on line 3019 in products/tasks/backend/facade/api.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

lint:complexity

`update_task_run` has cyclomatic complexity 36 (warn >10)

Check warning on line 3019 in products/tasks/backend/facade/api.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

`update_task_run` has cyclomatic complexity 36 (warn >10)
run_id: str | UUID,
task_id: str | UUID,
team_id: int,
*,
validated_data: dict,
only_if_non_terminal: bool = False,
only_if_not_started: bool = False,
caller_is_agent: bool = False,
) -> contracts.TaskRunDetailDTO | None:
"""Apply a PATCH to a run: merge output/state, set completion, then dispatch side effects.
Expand Down Expand Up @@ -3083,8 +3085,16 @@
update_fields: set[str] = set()

with transaction.atomic():
if has_output_merge or has_state_mutation or only_if_non_terminal or "status" in validated_data:
if (
has_output_merge
or has_state_mutation
or only_if_non_terminal
or only_if_not_started
or "status" in validated_data
):
run = TaskRun.objects.select_for_update().get(pk=run.pk)
if only_if_not_started and run.status != TaskRun.Status.NOT_STARTED:
return None
if only_if_non_terminal and run.is_terminal:
if validated_data.get("status") == run.status:
transaction.on_commit(lambda: resume_workflow_step_for_run(run))
Expand Down Expand Up @@ -3140,6 +3150,10 @@
update_fields.add("state")

new_status = validated_data.get("status")
if only_if_not_started and new_status == TaskRun.Status.CANCELLED:
# A dormant run has no workflow to complete its stream after cancellation.
run.state = {**(run.state or {}), "cancel_fallback_cleanup_complete": True}
update_fields.add("state")
if (
caller_is_agent
and new_status == TaskRun.Status.COMPLETED
Expand Down Expand Up @@ -3947,7 +3961,7 @@
return prepared, True


def finalize_task_run_artifact_uploads(

Check warning on line 3964 in products/tasks/backend/facade/api.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

lint:complexity

`finalize_task_run_artifact_uploads` has cyclomatic complexity 13 (warn >10)

Check warning on line 3964 in products/tasks/backend/facade/api.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

`finalize_task_run_artifact_uploads` has cyclomatic complexity 13 (warn >10)
run_id: str | UUID,
task_id: str | UUID,
team_id: int,
Expand Down Expand Up @@ -4507,7 +4521,7 @@
return missing_artifact_ids, True


def signal_task_run_user_message(

Check warning on line 4524 in products/tasks/backend/facade/api.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

lint:complexity

`signal_task_run_user_message` has cyclomatic complexity 14 (warn >10)

Check warning on line 4524 in products/tasks/backend/facade/api.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

`signal_task_run_user_message` has cyclomatic complexity 14 (warn >10)
run_id: str | UUID,
task_id: str | UUID,
team_id: int,
Expand Down Expand Up @@ -4915,7 +4929,7 @@
return bool(sandbox_id) and preview.get("sandbox_id") == sandbox_id


def resolve_task_run_preview_redirect(

Check warning on line 4932 in products/tasks/backend/facade/api.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

lint:complexity

`resolve_task_run_preview_redirect` has cyclomatic complexity 12 (warn >10)

Check warning on line 4932 in products/tasks/backend/facade/api.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

`resolve_task_run_preview_redirect` has cyclomatic complexity 12 (warn >10)
run_id: str | UUID, task_id: str | UUID, team_id: int, *, user_id: int
) -> TaskRunPreviewRedirect | None:
from products.tasks.backend.exceptions import (
Expand Down Expand Up @@ -5235,7 +5249,7 @@
}


def bootstrap_task_run(

Check warning on line 5252 in products/tasks/backend/facade/api.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

lint:complexity

`bootstrap_task_run` has cyclomatic complexity 23 (warn >10)

Check warning on line 5252 in products/tasks/backend/facade/api.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

`bootstrap_task_run` has cyclomatic complexity 23 (warn >10)
task_id: str | UUID, team_id: int, user_id: int | None, *, validated_data: dict
) -> contracts.TaskRunCreateResult | None:
"""Create a task run (without starting execution) from validated bootstrap data.
Expand Down Expand Up @@ -5417,16 +5431,15 @@
enqueue_or_start_workflow,
)
from products.tasks.backend.temporal.process_task.utils import ( # noqa: PLC0415 — keep temporalio off the api import path
RunSource,
mcp_scopes_for_run_source,
parse_run_state,
)
from products.tasks.backend.temporal.process_task.workflow import PendingFollowup # noqa: PLC0415

# SIGNAL_REPORT: implementation runs log their work on the report (notes, code references)
# via the task:write artefact tools.
full_mcp_run_sources = frozenset({None, RunSource.MANUAL, RunSource.SIGNAL_REPORT})
run_source = parse_run_state(run.state).run_source
posthog_mcp_scopes: Literal["read_only", "full"] = "full" if run_source in full_mcp_run_sources else "read_only"
posthog_mcp_scopes = mcp_scopes_for_run_source(run_source)
try:
logger.info("Attempting to trigger task processing workflow for task %s, run %s", task.id, run.id)
message = None
Expand Down Expand Up @@ -6518,6 +6531,7 @@
pending_user_message = (validated_data.pop("pending_user_message", None) or "").strip() or None
pending_user_artifact_ids = validated_data.pop("pending_user_artifact_ids", None) or []
warm_auto_publish = validated_data.pop("auto_publish", None)
validated_data.pop("scheduled_at", None)
# Names the task from the pasted content while `description` stays the bare prompt. Write-only,
# never persisted, so it must be popped before `Task.objects.create(**validated_data)`.
naming_source = (validated_data.pop("naming_source", None) or "").strip() or None
Expand Down Expand Up @@ -7394,12 +7408,16 @@
storage_path = str(staged_artifact["storage_path"])
if _find_artifact_manifest_entry(manifest, str(staged_artifact.get("id")), storage_path):
continue
tag_task_artifact(storage_path, ttl_days=RUN_ARTIFACT_TTL_DAYS, team_id=task.team_id)
# Scheduled attachments are tagged before the run-creation transaction takes its locks.
if run.scheduled_at is None:
tag_task_artifact(storage_path, ttl_days=RUN_ARTIFACT_TTL_DAYS, team_id=task.team_id)
manifest.append(dict(staged_artifact))
_save_artifact_manifest(run, manifest)
get_tasks_cache().delete_many(
[build_task_staged_artifact_cache_key(str(task.id), artifact_id) for artifact_id in artifact_ids]
)
cache_keys = [build_task_staged_artifact_cache_key(str(task.id), artifact_id) for artifact_id in artifact_ids]
if run.scheduled_at is not None:
transaction.on_commit(lambda: get_tasks_cache().delete_many(cache_keys), robust=True)
else:
get_tasks_cache().delete_many(cache_keys)


REPORT_WARM_RUN_NOT_ACTIVATED = "This sandbox is waiting for the report's Ask AI question. Send it from the report."
Expand Down Expand Up @@ -7998,15 +8016,19 @@
is_report_implementation_task,
)
from products.tasks.backend.logic.services.staged_artifacts import ( # noqa: PLC0415
RUN_ARTIFACT_TTL_DAYS,
get_task_run_artifacts_by_id,
get_task_staged_artifacts,
staged_artifacts_expire_by,
tag_task_artifact,
)
from products.tasks.backend.temporal.process_task.utils import ( # noqa: PLC0415 — keep temporalio off the api import path
PrAuthorshipMode,
RunSource,
cache_github_user_token,
get_provider_for_runtime_adapter,
get_reasoning_effort_error,
mcp_scopes_for_run_source,
parse_run_state,
)

Expand Down Expand Up @@ -8049,6 +8071,7 @@
)
mode = validated_data.get("mode", "background")
run_source = validated_data.get("run_source")
scheduled_at = validated_data.get("scheduled_at")
branch = validated_data.get("branch")
resume_from_run_id = validated_data.get("resume_from_run_id")
pending_user_message = validated_data.get("pending_user_message")
Expand Down Expand Up @@ -8113,7 +8136,16 @@
if claude_model_access is None and previous_state is not None:
claude_model_access = previous_state.claude_model_access

warm_run = None if run_source == RunSource.AGENT else _idling_warm_run_for_task(task)
if scheduled_at is not None and claude_model_access == "own-subscription":
return contracts.TaskRunResult(
error=contracts.TaskValidationError(
kind="validation_error",
code="invalid_input",
detail="Scheduled runs must use the PostHog gateway.",
attr="claude_model_access",
)
)
warm_run = None if scheduled_at is not None or run_source == RunSource.AGENT else _idling_warm_run_for_task(task)
if warm_run is not None and claude_model_access == "own-subscription":
warm_run = None
if warm_run is not None:
Expand Down Expand Up @@ -8433,14 +8465,43 @@
)
)

if scheduled_at is not None and staged_artifacts_expire_by(staged_artifacts, scheduled_at):
return contracts.TaskRunResult(
error=contracts.TaskValidationError(
kind="validation_error",
code="invalid_input",
detail="The attached files expire before this run can start. Choose an earlier time or upload new files.",
attr="scheduled_at",
)
)

logger.info("Creating task run for task %s with mode=%s, branch=%s", task.id, mode, branch)
if scheduled_at is not None:
for staged_artifact in staged_artifacts:
tag_task_artifact(
str(staged_artifact["storage_path"]),
ttl_days=RUN_ARTIFACT_TTL_DAYS,
team_id=task.team_id,
raise_on_error=True,
)
extra_state["pending_dispatch"] = {
Comment thread
oliverb123 marked this conversation as resolved.
Resolved
"user_id": user_id,
"create_pr": True,
"posthog_mcp_scopes": mcp_scopes_for_run_source(run_source),
}
Comment on lines +8479 to +8491

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recheck cloud-run eligibility before scheduled dispatch

should_fix bug

Issue description

These lines defer execution for up to 90 days. The dispatcher later checks only active project membership. It does not recheck Desktop entitlement, organization deactivation, or the PostHog Code usage limit. A run can start after the organization loses entitlement or reaches its cost cap. The existing review thread verifies membership only, so it does not cover these gates.

Why we think it's a valid issue
  • Checked: The new scheduled-run path (products/tasks/backend/facade/api.py:8150-8158, :8199-8203), the materializer and dispatcher that pick those runs up, the request-path gates the run endpoint applies, the gates the pre-existing deferred paths (loops, workflow-triggered tasks) reapply at fire time, and every post-dispatch checkpoint under products/tasks/backend/temporal/.
  • Found: The dispatcher's only eligibility gate is membership. _user_can_dispatch checks User.objects.filter(id=user_id, is_active=True) and UserPermissions(...).current_team.effective_membership_level is not None (products/tasks/backend/management/commands/run_task_workflow_dispatcher.py:57-66), and its failure marks the dispatch dead with reason permission (:207-212). materialize_due_scheduled_task_runs filters only on status, environment, scheduled_at and task__deleted (products/tasks/backend/logic/services/workflow_dispatch.py:212-224), so it adds no gate.
  • Found: The request path applies two further gates the deferred path does not. code_access_required_response(request, self.organization, task_id=pk) and usage_limit_response(request.user, self.team_id) both run before run_task (products/tasks/backend/presentation/views/api.py:1321-1324), and the create path repeats them with the signal-report carve-out (:748-758).
  • Found: The product already treats this recheck as required for deferred execution. A loop fire calls _usage_gate_blocked(loop) at fire time (products/tasks/backend/logic/services/loop_runs.py:351, :524-527), and workflow-triggered task creation calls usage_limit_response(gate_owner, team.id) at trigger time (products/tasks/backend/logic/services/workflow_tasks.py:246). The new path is the one deferred path with no equivalent, and its window is the longest at up to 90 days.
  • Found: get_desktop_access_decision(user, organization) takes a user and an organization, not a request (products/tasks/backend/access.py:56), so a dispatch-time recheck is feasible. Note it raises DesktopAccessResolutionError on a billing or flag-resolution failure, so a dispatch-time caller has to choose a fail-open or retry posture rather than reusing the request path's 503.
  • Impact: Confirmed gap, narrower than stated. The missing dispatch-time usage_limit_response equivalent is real and precedented, and the dead-dispatch machinery to express the denial already exists.
  • Priority: Lowered from must_fix. Two of the three named gates already have post-dispatch enforcement at the point compute is allocated: _create_sandbox_for_repository calls get_compute_quota_denial_reason(task) and raises ComputeBillingLimitError (products/tasks/backend/temporal/process_task/activities/provision_sandbox.py:777-780), and that function blocks a deactivated organization through organization_deactivated(task.team_id) and an exhausted PostHog Code credits quota through is_team_limited(..., QuotaResource.POSTHOG_CODE_CREDITS, ...) (products/tasks/backend/logic/services/compute_quota.py:18-19, :51-72, :82-88). The gateway buckets that usage_limit_response pre-reads from /v1/usage/posthog_code are independently enforced on inference by the gateway's own cost throttles (services/llm-gateway/src/llm_gateway/rate_limiting/cost_throttles.py), so an over-cap run cannot spend past the cap — it fails at its first model call.
  • Priority: The Desktop-entitlement half is a pre-existing product-wide posture, not something this PR introduces: a loop fire does not recheck get_desktop_access_decision either, so requiring it here widens scope beyond the change. The owner-credential check that the loops code calls the critical one — the run executes as its owner and can mint team-scoped tokens as them (products/tasks/backend/logic/services/loop_runs.py:333-339) — is performed by _user_can_dispatch, so there is no tenant-isolation or auth hole here.
Suggested fix

Add a domain-level eligibility check before start_workflow. Recheck Desktop entitlement and the usage gate for scheduled API runs. Preserve the request path's signal-report exemptions. If access is denied, make the run terminal and mark the dispatch dead. Add tests that change entitlement, organization state, and usage after scheduling.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/facade/api.py#L8153-8160

<issue_description>
These lines defer execution for up to 90 days. The dispatcher later checks only active project membership. It does not recheck Desktop entitlement, organization deactivation, or the PostHog Code usage limit. A run can start after the organization loses entitlement or reaches its cost cap. The existing review thread verifies membership only, so it does not cover these gates.
</issue_description>

<issue_validation>
- **Checked:** The new scheduled-run path (`products/tasks/backend/facade/api.py:8150-8158`, `:8199-8203`), the materializer and dispatcher that pick those runs up, the request-path gates the run endpoint applies, the gates the pre-existing deferred paths (loops, workflow-triggered tasks) reapply at fire time, and every post-dispatch checkpoint under `products/tasks/backend/temporal/`.
- **Found:** The dispatcher's only eligibility gate is membership. `_user_can_dispatch` checks `User.objects.filter(id=user_id, is_active=True)` and `UserPermissions(...).current_team.effective_membership_level is not None` (`products/tasks/backend/management/commands/run_task_workflow_dispatcher.py:57-66`), and its failure marks the dispatch dead with reason `permission` (`:207-212`). `materialize_due_scheduled_task_runs` filters only on status, environment, `scheduled_at` and `task__deleted` (`products/tasks/backend/logic/services/workflow_dispatch.py:212-224`), so it adds no gate.
- **Found:** The request path applies two further gates the deferred path does not. `code_access_required_response(request, self.organization, task_id=pk)` and `usage_limit_response(request.user, self.team_id)` both run before `run_task` (`products/tasks/backend/presentation/views/api.py:1321-1324`), and the create path repeats them with the signal-report carve-out (`:748-758`).
- **Found:** The product already treats this recheck as required for deferred execution. A loop fire calls `_usage_gate_blocked(loop)` at fire time (`products/tasks/backend/logic/services/loop_runs.py:351`, `:524-527`), and workflow-triggered task creation calls `usage_limit_response(gate_owner, team.id)` at trigger time (`products/tasks/backend/logic/services/workflow_tasks.py:246`). The new path is the one deferred path with no equivalent, and its window is the longest at up to 90 days.
- **Found:** `get_desktop_access_decision(user, organization)` takes a user and an organization, not a request (`products/tasks/backend/access.py:56`), so a dispatch-time recheck is feasible. Note it raises `DesktopAccessResolutionError` on a billing or flag-resolution failure, so a dispatch-time caller has to choose a fail-open or retry posture rather than reusing the request path's 503.
- **Impact:** Confirmed gap, narrower than stated. The missing dispatch-time `usage_limit_response` equivalent is real and precedented, and the dead-dispatch machinery to express the denial already exists.
- **Priority:** Lowered from `must_fix`. Two of the three named gates already have post-dispatch enforcement at the point compute is allocated: `_create_sandbox_for_repository` calls `get_compute_quota_denial_reason(task)` and raises `ComputeBillingLimitError` (`products/tasks/backend/temporal/process_task/activities/provision_sandbox.py:777-780`), and that function blocks a deactivated organization through `organization_deactivated(task.team_id)` and an exhausted PostHog Code credits quota through `is_team_limited(..., QuotaResource.POSTHOG_CODE_CREDITS, ...)` (`products/tasks/backend/logic/services/compute_quota.py:18-19`, `:51-72`, `:82-88`). The gateway buckets that `usage_limit_response` pre-reads from `/v1/usage/posthog_code` are independently enforced on inference by the gateway's own cost throttles (`services/llm-gateway/src/llm_gateway/rate_limiting/cost_throttles.py`), so an over-cap run cannot spend past the cap — it fails at its first model call.
- **Priority:** The Desktop-entitlement half is a pre-existing product-wide posture, not something this PR introduces: a loop fire does not recheck `get_desktop_access_decision` either, so requiring it here widens scope beyond the change. The owner-credential check that the loops code calls the critical one — the run executes as its owner and can mint team-scoped tokens as them (`products/tasks/backend/logic/services/loop_runs.py:333-339`) — is performed by `_user_can_dispatch`, so there is no tenant-isolation or auth hole here.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Add a domain-level eligibility check before `start_workflow`. Recheck Desktop entitlement and the usage gate for scheduled API runs. Preserve the request path's signal-report exemptions. If access is denied, make the run terminal and mark the dispatch dead. Add tests that change entitlement, organization state, and usage after scheduling.
</potential_solution>

@oliverb123 oliverb123 Sep 21, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am the coding agent assisting with this PR.

Fixed the missing usage precheck in 5b551f8. The dispatcher calls the existing usage_limit_response before a scheduled CREATE dispatch starts Temporal. A denial marks the dispatch dead and fails the run. Immediate starts and restarts keep their current behavior. The existing gate also blocks organization deactivation and retains its current gateway-outage behavior.

The denial is not silent: mark_dead calls _terminalize_unstarted_task_run, which stores the gate error in TaskRun.error_message and publishes the failure. The regression tests confirm both the failed status and the saved error.

Checks pass on the updated base: 35 dispatch tests, 13 focused MCP and schema tests, MCP type checking, full Python type checking, and CI preflight. API generation produced no further changes.

I did not add a new Desktop entitlement policy. As the review notes, other deferred paths do not apply that check. Project access remains checked, and signal-report entitlement exemptions remain unchanged.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escalating. The gap is real, but closing it needs product decisions I should not make unattended.


  • Confirmed: the dispatcher rechecks team membership only, so neither the Desktop entitlement gate nor the cost backstop runs again after scheduling.
  • Partly covered already: a deactivated organization and an exhausted credits quota are blocked later, when the sandbox is provisioned.
  • Someone must choose which gates re-run at dispatch, and whether they apply to every run or only to scheduled ones.
  • Someone must choose what a blocked scheduled run does. Marking the dispatch dead ends the run with nothing that tells the user why.
  • Someone must choose the posture when the entitlement check cannot resolve. It raises on a billing or flag failure, and the dispatcher has no caller to hand a retry to.

try:
with transaction.atomic():
task_run = task.create_run(mode=mode, branch=branch, extra_state=extra_state, acting_user_id=user_id)
task_run = task.create_run(
mode=mode, branch=branch, extra_state=extra_state, acting_user_id=user_id, scheduled_at=scheduled_at

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pending_dispatch commits with the run before _attach_staged_artifacts_to_run runs. if that attachment fails, the request errors but the materializer can still start a partially initialized run later, and a client retry can create duplicate work. can we keep it ineligible until attachment is complete, or roll the run back on failure?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to inside the same transaction

)
if report_id_for_slot_check is not None:
enforce_report_implementation_rerun_cap(
team_id=team_id, report_id=report_id_for_slot_check, task_id=str(task.id)
)
if scheduled_at is not None and pending_user_artifact_ids:
_attach_staged_artifacts_to_run(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id keep the object-storage calls outside this transaction. _attach_staged_artifacts_to_run makes one S3 tag request per attachment after task.create_run locks the task row, so a slow batch holds the database lock and successful tags survive a later rollback anyway. can we tag first, then atomically create the run and save its manifest?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

task_run, task, staged_artifacts=staged_artifacts, artifact_ids=pending_user_artifact_ids
)
except InvalidTaskOriginError as error:
return contracts.TaskRunResult(
error=contracts.TaskValidationError(
Expand All @@ -8465,14 +8526,19 @@
update_fields.append("relayed_mcp_servers")
task_run.save(update_fields=update_fields)

if pending_user_artifact_ids:
if pending_user_artifact_ids and scheduled_at is None:
_attach_staged_artifacts_to_run(
task_run, task, staged_artifacts=staged_artifacts, artifact_ids=pending_user_artifact_ids
)

if github_user_token and pr_authorship_mode == PrAuthorshipMode.USER:
cache_github_user_token(str(task_run.id), github_user_token)

if scheduled_at is not None:
return contracts.TaskRunResult(
task=_task_detail_to_dto(task, latest_run=task_run, include_latest_run_log_url=False)
)

logger.info("Triggering workflow for task %s, run %s", task.id, task_run.id)
if is_pi_task:
initial_message = (
Expand Down
47 changes: 36 additions & 11 deletions products/tasks/backend/facade/cancellation.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ def _publish_cancel_fallback_completion(run: TaskRun) -> bool:
publish_run_stream_completion(str(run.id))
TaskRun.update_state_atomic(run.id, updates={"cancel_fallback_cleanup_complete": False})
except Exception:
logger.warning("Failed to complete stream for workflow-gone run %s", run.id, exc_info=True)
logger.warning("Failed to complete stream for cancelled run %s", run.id, exc_info=True)
return False
return True

Expand Down Expand Up @@ -127,6 +127,18 @@ def cancel_task_run(
status_at_request = run.status
error_message = (reason or "").strip()[:500] or "Stopped by user"

def capture_cancel_request(task_run: TaskRun, signal_outcome: str) -> None:
task_run.capture_event(
"task_run_cancel_requested",
{
"cancel_source": source,
"cancel_reason": error_message,
"requested_by_user_id": requested_by_user_id,
"workflow_signal_outcome": signal_outcome,
"status_at_request": status_at_request,
},
)

marker: dict[str, Any] = {
"cancel_requested_at": django_timezone.now().isoformat(),
"cancel_source": source,
Expand Down Expand Up @@ -154,6 +166,28 @@ def cancel_task_run(
if only_if_awaiting_first_message:
return "unavailable", tasks_api._task_run_detail_to_dto(run)

if run.scheduled_at is not None and run.status == TaskRun.Status.NOT_STARTED:
dto = tasks_api.update_task_run(
run.id,
task_id,
team_id,
validated_data={"status": TaskRun.Status.CANCELLED, "error_message": error_message},
only_if_not_started=True,
)
if dto is not None:
capture_cancel_request(run, "not_needed")
run.refresh_from_db(fields=["state"])
if not _publish_cancel_fallback_completion(run):
return "unavailable", dto
return "accepted", dto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id publish the stream completion sentinel before returning here. this run has no workflow cleanup to publish it, so an existing /stream connection keeps rotating after the run is cancelled; can we make completion failure retriable like the workflow_gone path?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

run = tasks_api._get_visible_run(run_id, task_id, team_id)
if run is None:
return "not_found", None
if run.is_terminal:
if not _publish_cancel_fallback_completion(run):
return "unavailable", tasks_api._task_run_detail_to_dto(run)
return "already_terminal", tasks_api._task_run_detail_to_dto(run)

Comment on lines +169 to +190

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cancellation can race with a claimed dispatch

should_fix bug

Issue description

This path falls through when the materializer changes the run to QUEUED. _signal_complete_task can return workflow_gone before a claimed dispatcher calls Temporal. The fallback then marks the run CANCELLED and returns 202. The dispatcher does not recheck the status before client.start_workflow, so it can still provision a sandbox.

Why we think it's a valid issue
  • Checked: The full cancel path in products/tasks/backend/facade/cancellation.py, the dispatcher's per-row execution in products/tasks/backend/management/commands/run_task_workflow_dispatcher.py, resolve_ineligible, the workflow's run entry and its first activities, every reader of cancel_requested_at, and the terminal-status guard in update_task_run_status.
  • Found: The dominant cancel window is already fenced. _process re-reads the run and calls resolve_ineligible when run.status != TaskRun.Status.QUEUED (run_task_workflow_dispatcher.py:181-185), which marks the dispatch accepted with last_error="resolved: run left QUEUED" (products/tasks/backend/logic/services/workflow_dispatch.py:459-463). A cancel landing any time before the dispatcher reaches that read never starts a workflow.
  • Found: The unfenced span is real and nothing re-reads the run inside it. Between that status read and client.start_workflow (run_task_workflow_dispatcher.py:246-259) the dispatcher does Team.objects.aget, parse_create_payload, _user_can_dispatch (a user query plus UserPermissions) and _capture_run_feature_flags — so the gap is a flag evaluation and several queries wide, not instantaneous.
  • Found: No entry guard exists on the workflow side. ProcessTaskWorkflow.run goes from _get_task_processing_context(input) straight into _provision_and_start_agent (products/tasks/backend/temporal/process_task/workflow.py:1170-1190), with no terminal-status or cancel-marker check. Nothing in products/tasks/backend/temporal/process_task/activities/provision_sandbox.py reads cancel_requested_at; the only guards on that marker sit on follow-up message paths (activities/send_followup_to_sandbox.py:302, products/tasks/backend/facade/api.py:4371).
  • Found: The marker the cancel writes first would make a fence cheap. cancel_task_run stamps cancel_requested_at into run state through TaskRun.update_state_atomic before it signals (products/tasks/backend/facade/cancellation.py:142-160), so both the dispatcher and the workflow entry have something to read.
  • Found: The status itself does not flap. update_task_run_status re-reads under select_for_update and returns early when old_status in _TERMINAL_STATUSES (activities/update_task_run_status.py:82-92), so the CANCELLED row survives the workflow finishing.
  • Impact: Confirmed: a cancel landing in the dispatcher's in-flight span returns 202 with a CANCELLED run, and the workflow still starts, provisions a sandbox and runs the agent, which can open a PR. The user sees a cancelled run while the work proceeds.
  • Priority: Lowered from must_fix. The mechanism is inherited, not introduced: enqueue_or_start_workflow already returns early and leaves the start to the same dispatcher whenever flags.async_enabled (products/tasks/backend/logic/services/workflow_dispatch.py:523-524), and that flag comes from is_workflow_dispatch_async_enabled(organization_id, distinct_id) via evaluate_workflow_dispatch_flags (:475-490), so immediate runs carry the identical race today. The workflow_gone fallback this path falls through into (cancellation.py:186-208) is unchanged by this PR, and the new NOT_STARTED fast path closes the long pre-due window rather than opening it. With the dominant window fenced, the terminal-overwrite guard holding, and only a sub-second timing window left, this is worth hardening but does not block.
Suggested fix

Add a cancellation fence before dispatch starts Temporal. Recheck the run status after dispatch preparation. Also reject terminal runs at workflow entry. Add a race test that pauses a claimed dispatch, cancels the run, resumes dispatch, and confirms that no sandbox starts.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/facade/cancellation.py#L169-185

<issue_description>
This path falls through when the materializer changes the run to `QUEUED`. `_signal_complete_task` can return `workflow_gone` before a claimed dispatcher calls Temporal. The fallback then marks the run `CANCELLED` and returns 202. The dispatcher does not recheck the status before `client.start_workflow`, so it can still provision a sandbox.
</issue_description>

<issue_validation>
- **Checked:** The full cancel path in `products/tasks/backend/facade/cancellation.py`, the dispatcher's per-row execution in `products/tasks/backend/management/commands/run_task_workflow_dispatcher.py`, `resolve_ineligible`, the workflow's `run` entry and its first activities, every reader of `cancel_requested_at`, and the terminal-status guard in `update_task_run_status`.
- **Found:** The dominant cancel window is already fenced. `_process` re-reads the run and calls `resolve_ineligible` when `run.status != TaskRun.Status.QUEUED` (`run_task_workflow_dispatcher.py:181-185`), which marks the dispatch accepted with `last_error="resolved: run left QUEUED"` (`products/tasks/backend/logic/services/workflow_dispatch.py:459-463`). A cancel landing any time before the dispatcher reaches that read never starts a workflow.
- **Found:** The unfenced span is real and nothing re-reads the run inside it. Between that status read and `client.start_workflow` (`run_task_workflow_dispatcher.py:246-259`) the dispatcher does `Team.objects.aget`, `parse_create_payload`, `_user_can_dispatch` (a user query plus `UserPermissions`) and `_capture_run_feature_flags` — so the gap is a flag evaluation and several queries wide, not instantaneous.
- **Found:** No entry guard exists on the workflow side. `ProcessTaskWorkflow.run` goes from `_get_task_processing_context(input)` straight into `_provision_and_start_agent` (`products/tasks/backend/temporal/process_task/workflow.py:1170-1190`), with no terminal-status or cancel-marker check. Nothing in `products/tasks/backend/temporal/process_task/activities/provision_sandbox.py` reads `cancel_requested_at`; the only guards on that marker sit on follow-up message paths (`activities/send_followup_to_sandbox.py:302`, `products/tasks/backend/facade/api.py:4371`).
- **Found:** The marker the cancel writes first would make a fence cheap. `cancel_task_run` stamps `cancel_requested_at` into run state through `TaskRun.update_state_atomic` before it signals (`products/tasks/backend/facade/cancellation.py:142-160`), so both the dispatcher and the workflow entry have something to read.
- **Found:** The status itself does not flap. `update_task_run_status` re-reads under `select_for_update` and returns early when `old_status in _TERMINAL_STATUSES` (`activities/update_task_run_status.py:82-92`), so the CANCELLED row survives the workflow finishing.
- **Impact:** Confirmed: a cancel landing in the dispatcher's in-flight span returns 202 with a CANCELLED run, and the workflow still starts, provisions a sandbox and runs the agent, which can open a PR. The user sees a cancelled run while the work proceeds.
- **Priority:** Lowered from `must_fix`. The mechanism is inherited, not introduced: `enqueue_or_start_workflow` already returns early and leaves the start to the same dispatcher whenever `flags.async_enabled` (`products/tasks/backend/logic/services/workflow_dispatch.py:523-524`), and that flag comes from `is_workflow_dispatch_async_enabled(organization_id, distinct_id)` via `evaluate_workflow_dispatch_flags` (`:475-490`), so immediate runs carry the identical race today. The `workflow_gone` fallback this path falls through into (`cancellation.py:186-208`) is unchanged by this PR, and the new `NOT_STARTED` fast path closes the long pre-due window rather than opening it. With the dominant window fenced, the terminal-overwrite guard holding, and only a sub-second timing window left, this is worth hardening but does not block.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Add a cancellation fence before dispatch starts Temporal. Recheck the run status after dispatch preparation. Also reject terminal runs at workflow entry. Add a race test that pauses a claimed dispatch, cancels the run, resumes dispatch, and confirms that no sandbox starts.
</potential_solution>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am the coding agent assisting with this PR.

I confirmed this race. The dispatcher checks QUEUED before its asynchronous preparation. Cancellation can then mark the run CANCELLED before start_workflow. The workflow has no entry check that prevents sandbox creation in that case.

This also affects immediate runs through the existing asynchronous dispatcher. This PR leaves that dispatcher start protocol unchanged. Its new conditional NOT_STARTED cancellation path closes the pre-dispatch window, but does not fix the later race.

I am keeping the wider fix outside this focused scheduling PR. Another status read alone would only shorten the race. A complete fix needs coordinated dispatch and cancellation behavior, plus a workflow-entry check and a controlled race test. I am leaving this thread open because the issue remains.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escalating. The race is real, but it lives in the dispatcher and the workflow entry, which this change does not touch.


  • Confirmed: nothing re-reads the run between the dispatcher's status check and the call to Temporal, so a cancel in that span returns 202 while the workflow still starts.
  • Pre-existing: every immediate run that dispatches asynchronously carries the same race today, so this change does not create it.
  • The new fast path applies only to a scheduled run that is still waiting, so it removes a window rather than adding one.
  • A re-read in the dispatcher narrows the window but cannot close it. Closing it needs a guard where the workflow starts.
  • That guard needs a Temporal patch marker, like the local-environment guard already there, so it reaches every run and is a human's call.

_interrupt_agent_turn(run, requested_by_user_id, requested_by_distinct_id)

signal_outcome = _signal_complete_task(run, TaskRun.Status.CANCELLED, error_message)
Expand Down Expand Up @@ -185,14 +219,5 @@ def cancel_task_run(
push_dispatcher.notify_task_run_cancelled(run)
dto = tasks_api._task_run_detail_to_dto(run)

run.capture_event(
"task_run_cancel_requested",
{
"cancel_source": source,
"cancel_reason": error_message,
"requested_by_user_id": requested_by_user_id,
"workflow_signal_outcome": signal_outcome,
"status_at_request": status_at_request,
},
)
capture_cancel_request(run, signal_outcome)
return "accepted", dto
1 change: 1 addition & 0 deletions products/tasks/backend/facade/contracts.py
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,7 @@ class TaskRunDetailDTO:
updated_at: datetime | None = None
completed_at: datetime | None = None
preview_available: bool = False
scheduled_at: datetime | None = None


@dataclass(frozen=True)
Expand Down
18 changes: 17 additions & 1 deletion products/tasks/backend/logic/services/staged_artifacts.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from __future__ import annotations

import os
from datetime import datetime, timedelta
from typing import Any

from django.conf import settings
Expand Down Expand Up @@ -122,7 +123,20 @@ def get_task_run_artifacts_by_id(task_run: TaskRun, artifact_ids: list[str]) ->
return resolved_artifacts, missing_ids


def tag_task_artifact(storage_path: str, *, ttl_days: str, team_id: int) -> None:
def staged_artifacts_expire_by(artifacts: list[dict], scheduled_at: datetime) -> bool:
for artifact in artifacts:
stored_object = object_storage.head_object_strict(str(artifact["storage_path"]))
uploaded_at = stored_object.get("LastModified") if stored_object else None
if (
not isinstance(uploaded_at, datetime)
or timezone.is_naive(uploaded_at)
or scheduled_at >= uploaded_at + timedelta(days=int(RUN_ARTIFACT_TTL_DAYS))
):
return True
return False


def tag_task_artifact(storage_path: str, *, ttl_days: str, team_id: int, raise_on_error: bool = False) -> None:
try:
object_storage.tag(
storage_path,
Expand All @@ -139,3 +153,5 @@ def tag_task_artifact(storage_path: str, *, ttl_days: str, team_id: int) -> None
team_id=team_id,
error=str(exc),
)
if raise_on_error:
raise
Loading
Loading