Fix history API not returning data when recorder runs are lost #123449
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @tanelvakker
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
home-assistant
bot
added
bugfix
cla-needed
core
integration: history
small-pr
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
| @@ -107,8 +107,7 @@ async def get( | |||
| no_attributes = "no_attributes" in request.query | |||
|
|
|||
| if ( | |||
| (end_time and not has_recorder_run_after(hass, end_time)) | |||
There was a problem hiding this comment.
I was hoping has_recorder_run_after would get fixed instead of dropping the check.
There was a problem hiding this comment.
The root cause of this issue is probably not in this code. There is something happening in recorder that loses previous runs in the database. What could be causing this is beyond my comprehension.
There was a problem hiding this comment.
Is the recorder run missing from the table? Maybe the purge code deletes it too early, and that’s what we need to fix instead?
There was a problem hiding this comment.
Absolutely, but really no idea what causes this to happen. It happens less than once per month and makes it hard to debug for me
There was a problem hiding this comment.
Here is the logic to remove the recorder runs in case you see anything obviously wrong
There was a problem hiding this comment.
When the purge is finished we need to store:
The timestamp of the possible oldest row in the database (ie purge before) in the table
Update the memory cache of the same
I think we just need purge finished for the figuring out when to remove the old ones from the table and we don't care about when it started
There was a problem hiding this comment.
OK, so we add a new row only when a purge completes?
There was a problem hiding this comment.
PR #130378, which corrects the logic for purging recorder runs, has been merged and will be included in HA Core 2024.12.
I've started working on purge_runs table as suggested, but I'm not sure it'll be ready in time for HA Core 2024.12.
There was a problem hiding this comment.
After some more discussion, we don't think a purge runs table is needed. Here's a PR which replaces checking recorder run start time with the timestamp of the oldest state in the database: #131702
|
Putting to draft as the discussion above leads me to believe that the approach needs to be re-evaluated |
|
@tanelvakker @RudolfRendier I think we can close this PR now because #131702 has been merged which fixes the underlying issue. |
|
As this was fixed on recorder side, closing this PR |
Proposed change
Removes check for recorder run when querying history using API. Fixes #117793
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: