Repository navigation
Let a failed US state economy run be retried after two minutes - #4064
Merged
Merged
Conversation
A failed economy job without a breakdown is stored as an error for the cache lifetime of 30 days (`REFORM_IMPACT_TTL_SECONDS`), and every later request gets that stored error back. The app runs each US state on its ACS-local dataset for a nationwide report's congressional districts and offers Retry for a state that failed, so that Retry could never start a new run. A failed US state job (`country_id` `us`, region `state/...`, which a bare state code such as `ca` is normalized to) now keeps its error for two minutes on its own record, as a failed local-area breakdown already does. Until then every request gets the same error, which ends "A new run for this state can be requested 2 minutes after this failure."; after it, the next request finds no record and submits a new job. `_record_breakdown_failure` becomes `_record_retryable_failure`, which takes the retry note, and `_failure_retry_note` chooses the note for a failed job: the breakdown note, the US state note, or none for every other job. `LOCAL_AREA_BREAKDOWN_FAILURE_SECONDS` becomes `RETRYABLE_FAILURE_SECONDS` (still 120), since both kinds of job use it. US national, congressional district and place failures, UK national failures and typed SPM failures are still kept for 30 days. The specification's 502 description, docs/canonical-spm.md and a changelog fragment describe the change. Tests written first. Before the change, 3 failed for the expected reason: - `test_a_failed_us_state_job_is_returned_for_two_minutes_then_resubmitted` for `state/ca` and for the bare `ca`: the error had no retry note, and 121 seconds after the failure the request still got the stored error, with the simulation API's `run` called once; - `TestHandleExecutionState`'s state test: the error had no retry note and was stored without `keep_seconds=120`. The tests that failed US national, district and place jobs are still replayed, or stored without `keep_seconds`, passed before and after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4064 +/- ##
===========================================
+ Coverage 51.71% 88.14% +36.42%
===========================================
Files 267 279 +12
Lines 15184 16389 +1205
Branches 2432 2671 +239
===========================================
+ Hits 7853 14446 +6593
+ Misses 6676 1282 -5394
- Partials 655 661 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
master split economy_service.py and its tests into modules (#4056), so this pull request's change moves with them: - RETRYABLE_FAILURE_SECONDS (renamed from LOCAL_AREA_BREAKDOWN_FAILURE_SECONDS) and US_STATE_RETRY_NOTE are defined in economy_impact_types.py, and economy_service.py re-exports them; - _failure_retry_note and _record_retryable_failure (renamed from _record_breakdown_failure) are in economy_impact_execution.py; - the two TestHandleExecutionState tests move to test_economy_service_impact_execution.py. Against master's code the US state tests failed and the national, district and place tests passed; with the change all of them pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t's tense A failed US state job's stored error now expires after two minutes, so its log record is what keeps the reason. EconomicImpactResult.error already logs the message at ERROR; the state test now asserts it, so the reason cannot stop being logged unnoticed. The constants' comment said the app offers Retry for each state it runs for a nationwide report. That app change has not shipped, so it now says the app will offer it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The test compared the result with EconomicImpactResult.error(...), which logs the same message through the mocked logger, before it checked the log, so the log check passed even when the code under test logged nothing. It now checks the log first; with _record_retryable_failure temporarily changed not to log, it fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
master removed the legacy OpenAPI document (#4017), so this pull request's sentence in its 502 description goes with it; the two-minute rule for failed US state jobs stays documented in docs/canonical-spm.md and the changelog fragment. #4017 also removed ReformImpactsService.get_recent_reform_impacts, and master's tests read the store with _cache.recent instead; this pull request's three request tests now do the same. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch was previously deployed
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
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.
Fixes #4063
Summary
A failed US state economy job now keeps its error for two minutes instead of 30 days, so a client's Retry can run the state again. The app will run every state on its ACS-local dataset for a nationwide report's congressional districts, and will offer Retry per state; until now a failed state returned its stored error for 30 days.
country_iduswith regionstate/...(a bare state code such ascais normalized tostate/cafirst), the failed job's own record becomes an error that expires after 120 seconds. Until then every request gets the same error, ending "A new run for this state can be requested 2 minutes after this failure."; after it, the next request finds no record and submits a new job._record_breakdown_failurebecomes_record_retryable_failure, which takes the retry note, and the new_failure_retry_notepicks the breakdown note, the US state note, or none.LOCAL_AREA_BREAKDOWN_FAILURE_SECONDSis renamedRETRYABLE_FAILURE_SECONDS(still 120), since both kinds of job now use it. Since Split the economy service and its tests under the file-length limit #4056 split the economy service into modules, the two methods are ineconomy_impact_execution.py, and the constants, with the newUS_STATE_RETRY_NOTE, are ineconomy_impact_types.py, whicheconomy_service.pyre-exports.docs/canonical-spm.mdandchangelog.d/us-state-failure-retry.changed.md. (The legacy OpenAPI document this also changed was removed frommasterby Remove the routes only the legacy site used, check health at /health, and stop serving Canada, Nigeria and Israel #4017.)A state error stored before this deploys keeps its 30-day lifetime, unless a package version change gives it a different cache version.
Testing
The work was done red-green: the tests were written first and seen failing for the expected reason, then the code was written, and both are in one commit. After
masterwas merged in, with the change moved into the split modules, the same tests were run again againstmaster's code (the US state tests failed, the rest passed) and then with the change (all passed).tests/unit/services/test_economy_service_request_plumbing.py, with a new_us_request(region)helper:test_a_failed_us_state_job_is_returned_for_two_minutes_then_resubmitted(forstate/caand the bareca): red before the change, at its first assertion, because the error had no retry note. (Before the change the stored error would also have been returned 121 seconds after the failure, without a new run; the test stops before it checks that.)test_a_failed_us_national_job_is_still_replayedandtest_a_failed_us_district_job_is_still_replayed: passed before and after.tests/unit/services/test_economy_service_impact_execution.py,TestHandleExecutionState:state/cafailure is stored withkeep_seconds=120and the retry note: red before the change;keep_seconds: passed before and after.ruff format --checkandruff checkon the changed files,uv run --frozen --extra dev mypy,python scripts/run_quality_guards.pyandBASE_SHA=origin/master .github/scripts/check_changelog_fragment.sh: all passed. The full suite runs in CI.EconomicImpactResult.error), since its stored error expires after two minutes. The check runs before the test builds its expected result, which logs too; with_record_retryable_failuretemporarily changed to build its result without logging, the test failed ("call not found"). The constants' comment now says the app will offer Retry per state.No inline ruff suppressions are added.
🤖 Generated with Claude Code