Fix compiling missing statistics losing rows #101616
Conversation
home-assistant
bot
added
cla-signed
core
Hacktoberfest
integration: recorder
small-pr
|
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
|
|
It looks like we don't have good coverage for compile_missing_statistics |
|
Either I've screwed up the test or I'm actually hitting the bug in #101613 I think I'm actually hitting the bug 👍 Anyways I'm about 9 hours into this today so I need to pick it back up tomorrow |
|
So the problem here is that we create sessions inside sessions when than get |
bdraco
changed the title
bdraco
changed the title
| # There is already an active session when this code is called since | ||
| # it is called from the recorder statistics. We need to make sure | ||
| # this session never gets committed since it would be out of sync | ||
| # with the recorder statistics session so we mark it as read only. | ||
| # | ||
| # If we ever need to write to the database from this function we | ||
| # will need to refactor the recorder statistics to use a single | ||
| # session. |
There was a problem hiding this comment.
I should have refactored this as soon as I noticed this was happening when I added this comment in f6f3565 This wasn't the source of the problem, but its when I noticed this pattern.
I added the comment because I was worried this was a bit brittle but I didn't realize it was actually a problem as well at the time because I didn't understand the full impact of the nested sessions.
The irony is, I was too concerned about refactoring risk that I under-estimated the impact here even though I thought it was a problem enough to add this comment 🤦
| last_stats = statistics.get_latest_short_term_statistics_with_session( | ||
| hass, session, to_query, {"last_reset", "state", "sum"}, metadata=old_metadatas | ||
| ) |
There was a problem hiding this comment.
This was the only call that created a new session here which is the source of the issue
|
Ran on production overnight. All good I'm much more confident in this code now that we have explicit coverage for it. |
|
Thanks |
Note this is easier to review without white space since there is a context manager removed
Proposed change
When we compiled missing statistics we would create a session inside another session. Since sqlalchemy sessions are thread local, when used with the
scoped_sessioncontext manager, when the inner session was finished, it would callsession.close()and delete all the pending inserts in the outer session (because its actually the same session since its the same thread). The end result would be all the stats would get lumped together.This changes compiling platform stats to pass the session to each platform so we avoid the session within a session pattern. Its possible a custom component may implement their own platform stats, which would make this a breaking change but that seems unlikely and fixing this is likely worth any fallout if that is the case.
This change refactors
compile_missing_statisticsto useget_latest_short_term_statistics_with_sessionwhich will use the same session for the whole process to avoid the session within a session pattern to ensuresession.close()only happens when we are actually finished.We had no explicit tests for
compile_missing_statistics(only indirect coverage). They have been added to ensure this does not regress in the future.fixes #101613
Type of change
Additional information
Checklist
black --fast 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..coveragerc.To help with the load of incoming pull requests: