Conversation
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
products/experiments/backend/temporal/test_scheduled_recalculation_schedule.py |
4 | 39 |
This check does not block merging. It updates on every push and clears when the share drops.
✅ Django migration risk — no migrations to analyze
No Django migrations need risk analysis.
|
[High risk] Adds hourly scheduled experiment recalculation workflows. The PR appears safe to merge, though the scheduled-recalculation documentation should be corrected for teams with two configured hours. Reviews (2) · Last reviewed commit: "fix(experiments): expose the scheduled r..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
69564ce to
c1ac571
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: PostHog/posthog/.coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe change adds 24 daily Temporal schedules that start experiment recalculation workflows at minute 30. It registers schedule creation with general schedule initialization. The schedule function updates existing schedules and attempts to delete each hourly schedule. The README documents eligibility rules, skipped experiments, and coordinator events for started and skipped recalculations. Priority: ➖ Normal Merge Risk: 🔵 Low · up to A failed schedule deletion during rollback can leave hourly coordinator runs in place without reporting the cleanup failure. The organization flag limits recalculation work while disabled, so this is a bounded operational risk rather than a broad release blocker. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Automatic recalculation remains limited by organization eligibility and existing tenant-scoped execution safeguards. The main concern is rollback verification: schedule removal can silently fail, and disabling the feature does not cancel work already selected. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
products/experiments/backend/temporal/schedule.py-118-122 (1)
118-122: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLog per-schedule deletion failures instead of swallowing them silently.
except Exception: passhides real failures such as permission errors or connection loss. It treats them like a missing schedule. A persistent failure leaves schedules running with no signal. Catch the not-found case explicitly and log anything else. The loop keeps going, so other hours still get deleted.Proposed fix
+ from temporalio.service import RPCError, RPCStatusCode # noqa: PLC0415 + for hour in range(24): + schedule_id = f"{SCHEDULED_RECALCULATION_SCHEDULE_ID_PREFIX}-{hour:02d}" try: - await a_delete_schedule(client, f"{SCHEDULED_RECALCULATION_SCHEDULE_ID_PREFIX}-{hour:02d}") - except Exception: - pass # Schedule might not exist + await a_delete_schedule(client, schedule_id) + except RPCError as e: + if e.status != RPCStatusCode.NOT_FOUND: + logger.exception("experiment_scheduled_recalculation_schedule_delete_failed", schedule_id=schedule_id)
loggermust be defined in this module if it is not already.Source: Learnings
products/experiments/backend/temporal/schedule.py-94-111 (1)
94-111: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winContinue hourly setup, then re-raise the failure.
A failed
a_schedule_exists,a_create_schedule, ora_update_schedulecall stops later hours. The exception also propagates through the general scheduler’sTaskGroup, which can cancel other schedule initializers.Catch and log each hourly failure, continue through all 24 hours, then re-raise an aggregated failure. This lets the management command report failure instead of silently succeeding with missing schedules. The repository does not show an in-code retry, so re-raising preserves the failure signal for any external deployment retry.
Suggested fix
+import structlog + ... +logger = structlog.get_logger(__name__) + ... async def create_experiment_scheduled_recalculation_schedules(client: Client) -> None: + failures: list[Exception] = [] + for hour in range(24): - schedule_id = f"{SCHEDULED_RECALCULATION_SCHEDULE_ID_PREFIX}-{hour:02d}" - - schedule = Schedule( - action=ScheduleActionStartWorkflow( - SCHEDULED_RECALCULATION_WORKFLOW_NAME, - ScheduledRecalculationWorkflowInputs(hour=hour), - id=f'{SCHEDULED_RECALCULATION_SCHEDULE_ID_PREFIX}-{hour:02d}-{{{{.ScheduledTime.Format "2006-01-02"}}}}', - task_queue=settings.GENERAL_PURPOSE_TASK_QUEUE, - ), - spec=ScheduleSpec(cron_expressions=[f"30 {hour} * * *"]), - policy=SchedulePolicy(overlap=ScheduleOverlapPolicy.SKIP), - ) - - if await a_schedule_exists(client, schedule_id): - await a_update_schedule(client, schedule_id, schedule) - else: - await a_create_schedule(client, schedule_id, schedule, trigger_immediately=False) + try: + schedule_id = f"{SCHEDULED_RECALCULATION_SCHEDULE_ID_PREFIX}-{hour:02d}" + ... + except Exception as exc: + logger.exception("Failed to initialize hourly experiment schedule", hour=hour) + failures.append(exc) + + if failures: + raise ExceptionGroup("Failed to initialize hourly experiment schedules", failures)The sequential-latency concern is not independently actionable. Do not parallelize these RPCs without a latency requirement and a plan for partial failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: c3555bf3-ec01-4dd6-bd8a-38d5f549963e
📒 Files selected for processing (4)
posthog/temporal/experiments/README.mdposthog/temporal/schedule.pyproducts/experiments/backend/temporal/schedule.pyproducts/experiments/backend/temporal/test_scheduled_recalculation_schedule.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
c1ac571 to
563bf70
Compare
6e24cd6 to
e169684
Compare
05c6e0e to
6eb0ef0
Compare
👀 Auto-assigned reviewersThese soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:
Soft owners come from each directory's |
6eb0ef0 to
3039c19
Compare
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/experiments/backend/temporal/schedule.py-119-120 (1)
119-120: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPropagate non-missing schedule deletion failures.
If
handle.delete()raises a non-NOT_FOUNDTemporal error, the broad handler returns success while the schedule remains. The feature flag can stop recalculation candidates independently, so this is a cleanup and observability issue rather than a major recalculation failure. Catch only aNOT_FOUNDRPCErrorand propagate other failures.Suggested fix
+from temporalio.service import RPCError, RPCStatusCode + ... - except Exception: - pass # Schedule might not exist + except RPCError as error: + if error.status != RPCStatusCode.NOT_FOUND: + raise
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 12c040a2-679c-4924-8010-cc88eb4cc0af
📒 Files selected for processing (5)
posthog/temporal/experiments/README.mdposthog/temporal/schedule.pyproducts/experiments/backend/temporal/schedule.pyproducts/experiments/backend/temporal/test_scheduled_recalculation_schedule.pytach.toml
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
3ec85ed to
ffafbf4
Compare
ffafbf4 to
9d52c88
Compare
|
Risk: No findings This increment adds only Sentinel reviewed |
bbb3653 to
00d736b
Compare
00d736b to
b42cf82
Compare
4998846 to
ed2fd71
Compare
ed2fd71 to
59815f5
Compare
|
✨ Stack submitted to Merge by Rodrigo Iloro (a GitHub user). It will be added to the merge queue once all branch protection rules pass. See more details here. |
|
Heads up, this stack is failing in the merge queue ( #110127 merged this morning and removed To fix it, rebase on master, switch those tests to |
7626970 to
832ca2c
Compare
832ca2c to
8b18027
Compare
8b18027 to
9c2c13c
Compare
This branch has not been deployed
3 participants
Problem
The coordinator workflow from #108100 is registered but nothing fires it.
Changes
This PR creates the 24 schedules and turns the feature on for teams whose organization has the flag enabled.
One hourly schedule
A single schedule,
experiment-scheduled-recalculation, with cron30 * * * *and no workflow input. Discovery resolves the hour from the clock and selects the teams configured for it, so one schedule serves all 24 hours.The
:30offset is the coexistence design. The daily timeseries workflow fires at:00for the same teams, reading the sameexperiment_recalculation_timeconfig, so the offset gives its sync publish a head start before a real run supersedes it.The timeseries workflows keep 24 schedules because each runs its own metric queries and can outlive its hour. This coordinator starts other workflows and returns, so a
SKIPoverlap policy on one schedule covers the same ground.Creating the schedule also deletes the 24 per-hour schedules this replaces. Temporal keeps a schedule until it is deleted, so leaving them would start the workflow 25 times an hour.
products/experiments/backend/temporal/schedule.pyproducts/experiments/backend/temporal/test_scheduled_recalculation_schedule.pyposthog/temporal/schedule.pyDocumentation
The experiments temporal README documents the daily timeseries workflow and the approximations in its sync row. A new section explains how scheduled recalculations relate to it, including why the freshness check ignores sync rows.
posthog/temporal/experiments/README.mdRollback plan
Turn the organization feature flag off. Discovery returns no candidates, so the schedules keep firing and do nothing. This is the fast lever and it needs no deploy.
Reverting this PR stops the schedules being created or updated, but the 24 already registered in Temporal keep firing, so a revert alone is not enough. To remove them, run
delete_experiment_scheduled_recalculation_schedulesagainst the namespace, the same way the sibling timeseries deleter is run.How did you test this code?
uv run mypy --cache-fine-grained .Beyond the suite, the creator was run against a local Temporal: it registers one schedule firing at minute 30 of every hour, with no input args, and deletes all 24 of the per-hour schedules it replaces. A wrong cron or a mistyped workflow name fails only in production, since nothing local fires these schedules.
Release status
Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Model: Opus 5 (1M context)
Manually refactored: no
Skills used:
Relevant decisions:
scheduledtrigger, which feat(experiments): add missing metric recalculation triggers #107119 added for this workflow and which already advances the recalculation window so every metric recomputes on fresh data.:30rather than:00, so the daily timeseries sync publish lands before a real recalculation supersedes it, rather than the two racing for the same teams.TeamExperimentsConfig.experiment_recalculation_timesfield is deliberately not read. Both systems stay on the singular field so a team has one hour for both.CodeRabbit CLI pass skipped: the CLI is installed but signed out, and authorizing it for the workspace was not available in this session.