Repository navigation
fix(flags): wind down hypercache verification before the hard time limit - #87131
Conversation
github-actions
Bot
added
feature/feature-flags
🤖 CI report
|
| Project | Preview | Updated (UTC) |
|---|---|---|
| posthog.com | Open preview | Aug 24, 2026, 9:31 AM |
The preview should be ready in about 10 minutes. Open the preview at /handbook/engineering/.
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 2 consider. Published 3 findings (view the review). Resolved comments: 1 fixed |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
8a312ca to
41b33c6
Compare
The team_metadata and flag_definitions verification sweeps ran right at their
time budget in prod. When a run tipped past the hard limit, Celery SIGKILLed
the worker mid-batch; because the task is ignore_result and the child died
before reporting, the run vanished with no failure, metric, or log. Prod
metrics (observed 2026-08-21) showed team_metadata pinned at its 20-minute soft
limit and flag_definitions pinned at its 30-minute hard limit, while flags
peaked near 8 minutes.
Give each sweep a monotonic deadline so it winds down at a batch boundary and
records the deferral instead of being killed. Set the deadline
DEADLINE_HEADROOM_SECONDS before the soft time limit so the batch-boundary
check trips before Celery's SoftTimeLimitExceeded can fire mid-batch; without
the headroom a healthy over-budget run recorded "soft_time_limit" (the
budget-too-loose signal) instead of the graceful "deadline" reason.
Size each task's budget to its own load: flags to soft 25 / hard 28 min so the
hard limit stays under its 30-minute schedule, team_metadata to 35 / 40, and
flag_definitions up to 35 / 40 (was pinned at its old 30-minute hard limit).
Each task now locks for its own hard time limit via self.time_limit, replacing
the two shared lock-timeout constants, so every lock expires before the next
scheduled run.
Deadline wind-downs record incomplete_runs{reason="deadline"}, distinct from
"soft_time_limit". Also corrects the team_metadata docstring's ~3-5 min
estimate to the observed ~15-20 min.
Generated-By: PostHog Desktop
Task-Id: b806c584-37aa-4ae7-a0c2-ac587ff30087
The team_metadata verification docstring cited an exact team count as production scale. This repository is public and CLAUDE.md names "exact affected team counts" as prohibited private operational scale, so remove the figure while keeping the expected-duration note. Generated-By: PostHog Desktop Task-Id: dc9cc8f9-f4e2-417b-a075-248864275020
The prefer-frozen-dataclasses semgrep rule blocks the bare @DataClass on VerificationResult once this branch touches it. The result object is a mutable accumulator (the sweep sets counts, appends team ids, and flags the wind-down in place), so state the choice explicitly with @DataClass(frozen=False) rather than freezing it. Generated-By: PostHog Desktop Task-Id: b806c584-37aa-4ae7-a0c2-ac587ff30087
The deadline check ran after each batch and set wound_down_early purely on
elapsed time. When the deadline tripped on the final batch — every team already
covered — the sweep recorded a false incomplete_runs{reason="deadline"} and
emitted a spurious wind-down warning although no team was deferred.
Gate the wind-down on a cheap LIMIT-1 peek for a remaining team, so the flag is
set only when there is genuinely work to defer to the next cycle. The peek runs
only on the rare batch where the deadline trips and reuses the existing retrying
fetch, so a database drop there is still classified as db_unreachable.
Add a parameterized deadline case (both teams in one chunk, deadline passed)
that fails on the old code with wound_down_early=True at full coverage.
Generated-By: PostHog Desktop
Task-Id: ce8cda4f-549c-4cef-ab46-08dc3ac00f5c
The wind-down comment claimed the run "defers its remaining teams to the next cycle," but the cursor is never persisted, so the next cycle restarts from id 0. State that plainly and note the tail of high-id teams stays unverified until a run finishes within the deadline. Generated-By: PostHog Desktop Task-Id: b806c584-37aa-4ae7-a0c2-ac587ff30087
Prompt To Fix All With AI### Issue 1
posthog/storage/test/test_hypercache_verifier.py:1151
**Redundant deadline test docstring**
This class docstring only restates the behavior already conveyed by `TestVerifyAndFixAllTeamsDeadline`, adding maintenance surface without documenting any additional rationale.
```suggestion
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "docs(flags): correct hypercache wind-dow..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
9bb1efa to
f7948ce
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the HyperCache verification Celery sweeps to stop cleanly before Celery’s soft/hard time limits, so over-budget runs are observable (metrics/logs) and incomplete work is deferred to the next scheduled cycle instead of being silently SIGKILLed.
Changes:
- Adds a monotonic “wind-down deadline” (soft limit minus
DEADLINE_HEADROOM_SECONDS) and records a distinctincomplete_runs{reason="deadline"}when the sweep returns partial results. - Resizes per-task soft/hard time limits and aligns distributed lock timeouts with each task’s hard limit (
self.time_limit), ensuring locks expire before the next scheduled run. - Extends tests to cover deadline threading and deadline-driven incomplete-run metrics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| posthog/tasks/hypercache_verification.py | Introduces deadline headroom, new deadline incomplete-run reason, passes a monotonic deadline to the sweep, and tunes task budgets/lock timeouts. |
| posthog/tasks/test/test_hypercache_verification.py | Updates task tests to use VerificationResult, adds assertions for deadline propagation and incomplete-run metrics. |
| posthog/storage/hypercache_verifier.py | Adds stop_time support to wind down at batch boundaries and marks partial sweeps via VerificationResult.wound_down_early. |
| posthog/storage/test/test_hypercache_verifier.py | Adds coverage for stopping at a batch boundary after the monotonic deadline passes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Generated-By: PostHog Desktop Task-Id: b806c584-37aa-4ae7-a0c2-ac587ff30087
The hypercache doc still described the old 25/30 minute limits and did not mention the deadline wind-down. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JamesPatrickGill
left a comment
JamesPatrickGill
left a comment
There was a problem hiding this comment.
Pushed 85ad105 updating docs/internal/feature-flags/hypercache-system.md — it still described the flag definitions verifier's old 25/30 min limits. Drop or reword if you'd rather handle it yourself.
| # the hard time limit (which reports nothing). The cursor is not persisted, so | ||
| # the next cycle restarts from id 0 rather than resuming here: a run that winds | ||
| # down repeatedly leaves the same tail of high-id teams unverified until it can | ||
| # finish within the deadline. Only wind down when teams actually remain: a |
There was a problem hiding this comment.
Non-blocking: since the cursor restarts from id 0 and reason="deadline" doesn't alert, a sweep that consistently outruns its ~33 min deadline leaves the same high-id tail unverified indefinitely — and the self-heal drain covers misses, not mismatches. Maybe the follow-up is an alert on sustained deadline rate, or persisting the cursor?
There was a problem hiding this comment.
Good catch, this is a real gap: the current alerting only covers db_unreachable and error, not a sweep that repeatedly hits its own deadline. I'm leaving it as a follow-up since fixing it means either alerting on sustained deadline wind-downs or persisting the cursor, both bigger than this PR. Thanks for flagging it.
| # finish within the deadline. Only wind down when teams actually remain: a | ||
| # deadline that trips on the final batch has already covered every team, so it | ||
| # completed rather than winding down early. | ||
| if stop_time is not None and time.monotonic() > stop_time and _fetch_team_batch(base_qs, teams[-1].id, 1): |
There was a problem hiding this comment.
Minor: a DB blip at this probe records the run as db_unreachable rather than deadline, so it feeds the RepeatedGiveUps alert. Probably fine, noting for metric-reading.
There was a problem hiding this comment.
Right, that's a real edge case. The one-team lookahead probe uses the same retry-and-raise path as the main fetch, so a DB blip exactly at the deadline boundary gets recorded as db_unreachable instead of deadline. Agreed it's minor. Leaving it as-is, but worth keeping in mind when reading the db_unreachable metric.
|
/trunk merge |
Problem
When a hypercache verification sweep ran over its time budget, an engineer watching cache health saw nothing. Celery SIGKILLed the worker mid-batch at the hard time limit, and because the task is
ignore_result, the run vanished with no failure, no metric, and no log.Prod metrics (observed 2026-08-21) showed this was already happening: the
flag_definitionssweep sat right at its 30-minute hard limit andteam_metadataat its 20-minute soft limit, whileflagspeaked near 8 minutes.Changes
A sweep that runs over budget now defers its remaining teams to the next cycle and records why, instead of being killed silently.
The sweep takes a monotonic deadline and stops at the next batch boundary once it passes, returning a partial result flagged
wound_down_early.The deadline sits
DEADLINE_HEADROOM_SECONDS(2 min) before the soft time limit, so the batch-boundary check trips before Celery'sSoftTimeLimitExceededcan fire mid-batch. Without the headroom a healthy over-budget run recordedsoft_time_limit(the "budget too loose" signal) instead of the gracefuldeadlinereason.Each task's budget is sized to its own load, and every hard limit stays under its schedule interval:
Each task locks for its own hard time limit via
self.time_limit, replacing two shared lock-timeout constants. Every lock now expires before the next scheduled run.A deadline wind-down records
incomplete_runs{reason="deadline"}, distinct fromsoft_time_limit. Mechanical: theteam_metadatadocstring estimate is corrected to the observed range.How did you test this code?
hogli test posthog/tasks/test/test_hypercache_verification.py posthog/storage/test/test_hypercache_verifier.py::TestVerifyAndFixAllTeamsDeadline— 28 passed locally against a fresh test database.New and changed tests and the regression each guards:
test_passes_monotonic_deadline_to_sweep(flags) now freezes the clock and asserts the deadline equalssoft_time_limit - DEADLINE_HEADROOM_SECONDS, so dropping the headroom or the soft-limit link fails. The old assertion only checked the deadline was non-null.test_deadline_winddown_records_incomplete_run_without_raising(flag_definitions) guards its separate_run_flag_definitions_verificationpath, which threads the deadline independently of the flags and team_metadata path.TestVerifyAndFixAllTeamsDeadlinecovers both sides of the batch-boundary check: a passed deadline stops after one batch, headroom processes all teams.Not run: the full backend suite and mypy repo-wide. Preflight (ruff lint, ruff format) passed; the type-check step is advisory and the changes only add
floatparameters to existing signatures.Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored by Claude (PostHog Desktop) under Phil's direction. The branch started with a first pass that bumped both large sweeps to 35/40 and set the deadline equal to the soft limit. Review surfaced two problems: the deadline could never win the race against Celery's soft-limit signal, and the flags task's 40-minute hard limit exceeded its 30-minute schedule.
Prod Grafana metrics drove the redesign. The task durations showed flags never approaches its limits (so it was tightened to 25/28, resolving the schedule overlap), while flag_definitions was the one actually being killed (so it was raised to 35/40). The deadline gained headroom, and the two shared lock-timeout constants were replaced with each task's own
self.time_limit.Skills invoked:
/review-code,/explain-open,/writing-tests,/writing-pr-descriptions.