Repository navigation
fix(data-modeling): stop scheduling v2 saved queries without a node - #78259
Merged
Merged
Conversation
get_v2_saved_query_ids resolved a saved query to its DAG through its node, so an absent node read as "not on v2" and schedule_materialization minted a v1 per-query schedule beside the team's live tier. The node can be absent because sync_saved_query_to_dag deletes it when dependency resolution raises. Resolve a node-less saved query against its team's DAGs instead, and map each saved query to every DAG it belongs to so a query in several DAGs no longer resolves to an arbitrary one.
|
😎 Merged successfully - details. |
On v2 the DAG run materializes nodes, so a saved query with no node is not reachable by any schedule. Every caller syncs the query to the DAG before scheduling and every one swallows that sync failing, so the query was left claiming to be materialized while nothing refreshed it. Raise MissingDagNodeError instead, which lands in the existing failure contract: the error is captured and is_materialized is cleared, so the state is visible rather than silently stale.
sakce
changed the title
Covers the caller composition the unit tests do not reach: the managed viewset swallows a dag sync failure and schedules the query anyway. Fails on master by calling sync_saved_query_workflow(create=True).
The deferred import was justified by a circular import that does not exist: the saved query model imports schedule.py only inside methods, so a module-level import is fine and the comment claiming otherwise was wrong. Also document the node-less fallback on get_v2_saved_query_ids itself, so the guard in schedule_materialization reads as reachable from the call site, drop two mocks the raise makes unreachable, and name the helper for what it returns.
The endpoints path swallows a dag sync failure, and schedule_materialization applies a disable-on-failure contract rather than raising, so a failed schedule surfaced only as is_materialized flipping back. The enable returned success and the endpoint served inline forever with nothing refreshing it. Check the flag after scheduling and fail, which unwinds the enclosing atomic block so no half-enabled saved query or version link is left behind.
Deriving the team from the candidate ids meant an unscoped lookup on a team-scoped model, which semgrep blocks as an IDOR risk, and the batch callers pass ids spanning teams so no single filter fitted. Take team_id from the caller instead, which removes the saved query lookup entirely and leaves one team-filtered DAG query. It also keeps the fallback opt-in, so only schedule_materialization, the one caller that can mint a v1 schedule, changes behaviour; the v1 remediation commands and the run action keep resolving through nodes as before.
sakce
force-pushed
the
worktree-fix-v2-detection-fails-open
branch
from
August 5, 2026 14:27
df590d1 to
49454a7
Compare
sakce
marked this pull request as ready for review
August 5, 2026 14:31
sakce
added
stamphog
Contributor
|
Reviews (1): Last reviewed commit: "fix(data-modeling): take the node-less f..." | Re-trigger Greptile |
Contributor
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 0 consider. Published 1 finding (view the review). |
Contributor
|
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}
Contributor
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 1 issue
Files (4)
products/data_modeling/backend/logic/saved_query_dag_sync.pyproducts/data_modeling/backend/models/datawarehouse_saved_query.pyproducts/data_modeling/backend/schedule.pyproducts/endpoints/backend/logic/materialization.py
Comment on lines
+257
to
+263
| # schedule_materialization applies a disable-on-failure contract rather than raising, | ||
| # so a failure to schedule shows up only as is_materialized flipping back. Checking it | ||
| # is what stops the enable reporting success on an endpoint nothing will ever refresh. | ||
| saved_query.refresh_from_db(fields=["is_materialized"]) | ||
| if not saved_query.is_materialized: | ||
| raise APIException(f"Failed to schedule materialization for endpoint {endpoint.name}.") | ||
|
|
Contributor
There was a problem hiding this comment.
Schedule-failure check always surfaces as a 500, even for client-caused query errors
Why we think it's a valid issue
- Checked: the new post-schedule check against the full enable flow —
can_materialize_query(products/endpoints/backend/models.py:76-126),sync_saved_query_to_dag(products/data_modeling/backend/logic/saved_query_dag_sync.py:161-248),resolve_dependency_to_node(same file:80-133),schedule_materialization's failure contract (products/data_modeling/backend/models/datawarehouse_saved_query.py:220-290), andenable_materialization's exception handlers (products/endpoints/backend/logic/materialization.py:145-200). - Found (reachability):
can_materialize_queryvalidates only query shape (kind, compare/cohort filters, variables, non-empty HogQL string) and never resolves DAG parents, so a query referencing a since-deleted/renamed upstream passes pre-flight.resolve_dependency_to_nodethen raisesUnknownParentError/DoesNotExist(saved_query_dag_sync.py:88-133), andsync_saved_query_to_dag'sexcept Exception: target.delete(); raise(lines 230-232) deletes the node. The endpoints caller swallows that at materialization.py:230. - Found (flip → 500): with no node on a v2 team,
schedule_materializationraisesMissingDagNodeError, caught by the disable-on-failure contract (datawarehouse_saved_query.py:278-290) which setsis_materialized=False. The new check then raisesAPIException, which is not aValidationError, so it falls through toenable_materialization's genericexcept Exception(materialization.py:181-200) → incrementsstatus="error"(the alerting metric, per the in-file comment at 183-184) and re-raises a 500. - Found (contract break): the sibling handler at materialization.py:176-180 already routes bad-user-query causes to a 400
ValidationError; the new bare-boolean check has no cause information, so a client-fixable query error and a genuine Temporal/infra fault both collapse to 500. - Impact: a user whose endpoint query references an unresolvable upstream gets a 500 (reads as 'retry later', not 'fix your query') on every attempt, fires the on-call paging alert, and double-captures to Sentry — a real, reachable client-error-as-server-error and false-alerting defect, not a speculative edge case.
should_fixis appropriate: the enable does correctly fail, but the misclassification carries concrete operational cost.
Issue description
After schedule_materialization() returns, the new code treats any is_materialized == False outcome as an APIException (HTTP 500). But is_materialized can flip to False for a client-fixable cause too: the pre-existing try/except a few lines above (around the sync_saved_query_to_dag(saved_query) call) already swallows resolution failures such as UnknownParentError/QueryError/cycle errors when the endpoint's underlying query references a table/view/dependency that can't be resolved, logging and capturing them but continuing. That missing node is exactly what makes schedule_materialization() raise MissingDagNodeError (products/data_modeling/backend/logic/saved_query_dag_sync.py) internally and clear is_materialized. The sibling branch in enable_materialization (except (ExposedHogQLError, MaterializationNotSupportedError)) treats this same class of 'bad user query' as a 400 ValidationError, but the new check here has no way to distinguish 'the user's query doesn't resolve' from 'Temporal/infra failed to schedule' -- both collapse into an unconditional APIException. A client whose endpoint query references a since-renamed or since-deleted upstream view will now get a 500 on every retry, which reads as 'try again later' instead of 'fix your query', misleading both automated retriers and on-call alerting (500s are what pages, per the status="error" metric label used a few lines up in enable_materialization).
Suggested fix
Propagate the underlying cause instead of a bare boolean: e.g. have the swallowed sync_saved_query_to_dag failure (or the resulting MissingDagNodeError) record why scheduling failed on the saved query, then in this check branch on that reason -- raise ValidationError when the root cause is an unresolvable/invalid query (a request problem), and reserve APIException for genuine Temporal/scheduling infrastructure failures.
Prompt to fix with AI (copy-paste)
## Context
@products/endpoints/backend/logic/materialization.py#L257-263
<issue_description>
After `schedule_materialization()` returns, the new code treats any `is_materialized == False` outcome as an `APIException` (HTTP 500). But `is_materialized` can flip to False for a client-fixable cause too: the pre-existing try/except a few lines above (around the `sync_saved_query_to_dag(saved_query)` call) already swallows resolution failures such as `UnknownParentError`/`QueryError`/cycle errors when the endpoint's underlying query references a table/view/dependency that can't be resolved, logging and capturing them but continuing. That missing node is exactly what makes `schedule_materialization()` raise `MissingDagNodeError` (products/data_modeling/backend/logic/saved_query_dag_sync.py) internally and clear `is_materialized`. The sibling branch in `enable_materialization` (`except (ExposedHogQLError, MaterializationNotSupportedError)`) treats this same class of 'bad user query' as a 400 `ValidationError`, but the new check here has no way to distinguish 'the user's query doesn't resolve' from 'Temporal/infra failed to schedule' -- both collapse into an unconditional `APIException`. A client whose endpoint query references a since-renamed or since-deleted upstream view will now get a 500 on every retry, which reads as 'try again later' instead of 'fix your query', misleading both automated retriers and on-call alerting (500s are what pages, per the `status="error"` metric label used a few lines up in `enable_materialization`).
</issue_description>
<issue_validation>
- **Checked:** the new post-schedule check against the full enable flow — `can_materialize_query` (products/endpoints/backend/models.py:76-126), `sync_saved_query_to_dag` (products/data_modeling/backend/logic/saved_query_dag_sync.py:161-248), `resolve_dependency_to_node` (same file:80-133), `schedule_materialization`'s failure contract (products/data_modeling/backend/models/datawarehouse_saved_query.py:220-290), and `enable_materialization`'s exception handlers (products/endpoints/backend/logic/materialization.py:145-200).
- **Found (reachability):** `can_materialize_query` validates only query shape (kind, compare/cohort filters, variables, non-empty HogQL string) and never resolves DAG parents, so a query referencing a since-deleted/renamed upstream passes pre-flight. `resolve_dependency_to_node` then raises `UnknownParentError`/`DoesNotExist` (saved_query_dag_sync.py:88-133), and `sync_saved_query_to_dag`'s `except Exception: target.delete(); raise` (lines 230-232) deletes the node. The endpoints caller swallows that at materialization.py:230.
- **Found (flip → 500):** with no node on a v2 team, `schedule_materialization` raises `MissingDagNodeError`, caught by the disable-on-failure contract (datawarehouse_saved_query.py:278-290) which sets `is_materialized=False`. The new check then raises `APIException`, which is not a `ValidationError`, so it falls through to `enable_materialization`'s generic `except Exception` (materialization.py:181-200) → increments `status="error"` (the alerting metric, per the in-file comment at 183-184) and re-raises a 500.
- **Found (contract break):** the sibling handler at materialization.py:176-180 already routes bad-user-query causes to a 400 `ValidationError`; the new bare-boolean check has no cause information, so a client-fixable query error and a genuine Temporal/infra fault both collapse to 500.
- **Impact:** a user whose endpoint query references an unresolvable upstream gets a 500 (reads as 'retry later', not 'fix your query') on every attempt, fires the on-call paging alert, and double-captures to Sentry — a real, reachable client-error-as-server-error and false-alerting defect, not a speculative edge case. `should_fix` is appropriate: the enable does correctly fail, but the misclassification carries concrete operational cost.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Propagate the underlying cause instead of a bare boolean: e.g. have the swallowed `sync_saved_query_to_dag` failure (or the resulting `MissingDagNodeError`) record why scheduling failed on the saved query, then in this check branch on that reason -- raise `ValidationError` when the root cause is an unresolvable/invalid query (a request problem), and reserve `APIException` for genuine Temporal/scheduling infrastructure failures.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
Fixed in f3df56b. The swallowed sync_saved_query_to_dag error is now kept and used to classify: an unresolved dependency raises ValidationError (400, status="validation_error", no page) matching the sibling handler, and APIException is reserved for a scheduling failure with no sync error.
…a fault The enable swallows a dag sync failure, so the is_materialized check could not tell a query whose dependencies do not resolve from a Temporal fault, and reported both as a server error. That tells the author to retry a problem only they can fix, and pages on-call through the status=error metric. Keep the swallowed error and use it to classify: an unresolved dependency is a 400 like its sibling handler, and a genuine scheduling failure stays a 500.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Scheduling/materialization bug fix with thorough targeted tests; the one bot review concern (500 vs 400 on sync failure) is already fixed in this head via the sync_error classification, and the author has STRONG familiarity with the affected code (last-touched 72% of these lines, 8/8 files, 0 days ago).
- Author wrote 72% of the modified lines and has 137 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 100L, 4F substantive, 221L/8F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (221L, 8F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 18e5e44 · reviewed head f3df56b |
Contributor
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/endpoints/backend/logic/materialization.py |
85.7% | 269 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 31021401097 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
demo |
███████████░░░░░░░░░ 56.3% |
1,497 / 2,661 |
tasks |
██████████████░░░░░░ 70.6% |
34,541 / 48,945 |
signals |
████████████████░░░░ 82.2% |
26,649 / 32,430 |
cdp |
█████████████████░░░ 84.2% |
3,914 / 4,646 |
data_modeling |
█████████████████░░░ 85.9% |
7,959 / 9,263 |
notebooks |
█████████████████░░░ 86.4% |
8,026 / 9,293 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
managed_warehouse |
█████████████████░░░ 87.1% |
6,219 / 7,141 |
cohorts |
█████████████████░░░ 87.4% |
6,459 / 7,386 |
data_warehouse |
██████████████████░░ 87.8% |
11,261 / 12,823 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
engineering_analytics |
██████████████████░░ 89.2% |
6,666 / 7,477 |
exports |
██████████████████░░ 89.4% |
7,277 / 8,143 |
dashboards |
██████████████████░░ 89.5% |
5,989 / 6,693 |
conversations |
██████████████████░░ 90.0% |
18,520 / 20,575 |
alerts |
██████████████████░░ 90.3% |
4,482 / 4,966 |
streamlit_apps |
██████████████████░░ 90.7% |
2,630 / 2,901 |
error_tracking |
██████████████████░░ 91.1% |
11,106 / 12,197 |
canvas |
██████████████████░░ 91.1% |
2,033 / 2,231 |
stamphog |
██████████████████░░ 91.3% |
4,505 / 4,936 |
slack_app |
██████████████████░░ 91.6% |
10,497 / 11,454 |
marketing_analytics |
██████████████████░░ 92.1% |
13,388 / 14,539 |
product_analytics |
███████████████████░ 92.5% |
5,849 / 6,321 |
ai_observability |
███████████████████░ 92.6% |
16,173 / 17,472 |
mcp_analytics |
███████████████████░ 92.7% |
3,810 / 4,112 |
early_access_features |
███████████████████░ 92.9% |
1,340 / 1,443 |
web_analytics |
███████████████████░ 93.1% |
15,963 / 17,154 |
surveys |
███████████████████░ 93.2% |
5,853 / 6,281 |
posthog_ai |
███████████████████░ 93.3% |
1,327 / 1,423 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
approvals |
███████████████████░ 93.5% |
3,491 / 3,734 |
workflows |
███████████████████░ 94.1% |
7,663 / 8,142 |
endpoints |
███████████████████░ 94.3% |
8,771 / 9,306 |
review_hog |
███████████████████░ 94.6% |
8,246 / 8,715 |
skills |
███████████████████░ 94.6% |
3,158 / 3,337 |
logs |
███████████████████░ 95.6% |
10,759 / 11,259 |
experiments |
███████████████████░ 95.6% |
27,390 / 28,637 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
feature_flags |
███████████████████░ 96.4% |
17,539 / 18,195 |
replay_vision |
███████████████████░ 96.5% |
16,697 / 17,311 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
customer_analytics |
███████████████████░ 97.1% |
10,748 / 11,070 |
warehouse_sources |
███████████████████░ 97.4% |
379,406 / 389,548 |
data_catalog |
████████████████████ 97.9% |
2,651 / 2,708 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
Asheesh7298
pushed a commit
to Asheesh7298/posthog
that referenced
this pull request
Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
get_v2_saved_query_idsdecides whether a saved query already runs on a v2 DAG schedule.schedule_materializationmints a v1 per-query schedule when the answer is no. The lookup returned no for two reasons that were not true.sync_saved_query_to_dagdeletes the node when dependency resolution raises.The second defect contradicts the contract stated on
partition_saved_queries_by_v2_schedulein the same module: a saved query is on v2 when any DAG it belongs to has a schedule.Changes
Three parts. The first stops the wrong answer, the second and third stop a wrong answer from being silent.
1. The lookup no longer fails open.
get_v2_saved_query_idsmaps each saved query to every DAG it belongs to.team_id, which the caller supplies. Onlyschedule_materializationpasses it, because it is the only caller that can mint a v1 schedule.runaction, and the three v1 remediation commands) resolves through nodes exactly as before, so their behavior is unchanged.2.
schedule_materializationrefuses to schedule a v2 saved query with no node.On v2 the DAG run materializes nodes, so a query with no node is not reachable by any schedule. Part 1 alone would have replaced a double materialization with no materialization at all, and nothing would have reported it:
apply_saved_query_frequency_targetdocuments that it writes the target nowhere, andmaterialize_saved_querylogs a warning and returns.MissingDagNodeErrorwhen the saved query has no node.is_materialized.3. Enabling endpoint materialization fails when scheduling failed.
schedule_materializationapplies its disable-on-failure contract instead of raising, so the endpoints caller could not tell success from failure. It reported success whileis_materializedwas false and nothing would ever refresh the endpoint._enable_materialization_innerchecks the flag after scheduling and fails.materializeaction already did this; the managed viewset self-heals on its next sync.Note
The endpoints caller already documents the invariant part 2 enforces: "The DAG node must exist before scheduling: the v2 detection and the freshness-target write-through both resolve this saved query through its Node row."
How did you test this code?
Automated tests only. I did not run this against a real Temporal namespace.
Each new test fails without its fix:
test_saved_query_whose_node_vanished_is_refused_instead_of_scheduledfails by callingsync_saved_query_workflow(create=True), the mint this PR prevents. It also assertsis_materializedends up false.test_failed_dag_sync_does_not_mint_a_v1_scheduledrives the real managed-viewset path: the sync raises,sync_viewsswallows it and schedules anyway. Fails on master the same way. This is the production shape.test_enable_fails_when_the_dag_sync_leaves_no_nodefails on the branch without part 3, by returning 200 on an endpoint that will never materialize.test_saved_query_without_a_node_is_not_reported_as_v1_eligiblefails by returning an empty set.test_saved_query_in_two_dags_is_on_v2_when_either_dag_isis parameterized over node creation order. Only the order that writes the v2 node first fails on master; the other passes by luck, which is the defect.test_saved_query_without_a_node_stays_v1_eligible_when_no_dag_is_v2_scheduledholds the fallback to its intended scope.No existing test covered a saved query without a node, or a saved query in two DAGs.
Suites run
products/data_modeling/backendproducts/endpoints/backend/testsproducts/data_warehouse/backend/tests/api/test_saved_query.pytest_managed_viewset.pyruff checkandruff formatuv run mypy --cache-fine-grained .Known boundaries
One case this deliberately does not close: a brand-new team whose first managed view fails to sync has no node to bootstrap from, so it still mints v1. Closing it would mean refusing to schedule node-less queries on genuinely v1 teams, which changes behavior for teams still on v1 for no benefit.
Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
I (actually Claude, via Claude Code) wrote this after an audit of live v1 schedules found saved queries running a v1 per-query schedule and a v2 DAG tier at the same time. Every confirmed case came through one caller, which pointed at this lookup.
Three directions were rejected. Tracking failed syncs in each caller loop was rejected because there are three callers with the same shape. Shipping part 1 alone was rejected once Jovan asked what actually runs a saved query with no node: it converts a visible double materialization into an invisible absence of one. Stopping
sync_saved_query_to_dagdeleting the node is the deeper fix but changes what the DAG materializes against a wrong parent set, so it stays a separate piece of work.Two Claude subagents reviewed the branch independently, one for correctness and regressions and one for design and test value. Part 3 and the "Sync now" note come from that review, as does removing a deferred import whose stated justification (a circular import) did not exist. I verified both claims myself before acting on them.
Skills invoked:
/writing-tests,/writing-code-comments,/writing-pr-descriptions.