fix: consume config-update SSE events into the vehicle config record - #27
fix: consume config-update SSE events into the vehicle config record#27Bre77 wants to merge 9 commits into
Conversation
The internal fields/preferTyped record only reflected what this client had itself requested or observed at connect, so a config change applied elsewhere (another client, the console, a server-side migration) left it stale - causing add_field/prefer_typed's no-op check to either send redundant requests or wrongly skip real ones. TeslemetryStreamVehicle now listens for the config SSE topic and merges well-typed pieces of the pushed record in, leaving a missing piece untouched and logging (without corrupting the record) a malformed one.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6f09dde73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.stream.async_add_listener( | ||
| self._on_config_event, {Key.VIN: self.vin, Key.CONFIG: None} | ||
| ) |
There was a problem hiding this comment.
When a vehicle is constructed with the default manual=False outside a running event loop, this becomes the first listener and TeslemetryStream.async_add_listener() immediately calls asyncio.create_task(self.listen()). That raises RuntimeError: no running event loop, so the previously synchronous TeslemetryStream(..., vin=...) and direct vehicle constructors can no longer be used before an event loop starts.
Useful? React with 👍 / 👎.
| # config change outside this client (another client, the console, | ||
| # a Teslemetry-side migration), so add_field/prefer_typed's no-op | ||
| # checks don't act on stale history. | ||
| self.stream.async_add_listener( |
There was a problem hiding this comment.
For every non-manual vehicle stream, this listener remains permanently in stream._listeners. Consequently, removing the last public or typed listener can never satisfy the empty-listener check in TeslemetryStream.async_add_listener()'s remover, so close() is not called and the SSE connection and owned listen task continue running indefinitely after consumers unsubscribe.
Useful? React with 👍 / 👎.
Registering the config-sync listener eagerly in the vehicle constructor called async_add_listener -> asyncio.create_task for the stream's first listener, which needs a running loop - breaking synchronous TeslemetryStream(vin=...)/vehicle construction. Registration is now lazy, on the first add_field/prefer_typed/update_config call. The listener also permanently occupied stream._listeners, so removing every public listener could never satisfy the "no more listeners" auto-close check and the SSE connection leaked open. async_add_listener gained an internal flag excluding bookkeeping-only listeners from that check.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d07e96ea7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """ | ||
| self._listeners.pop(remove_listener) | ||
| if not self._listeners: | ||
| if not any(not is_internal for _, _, is_internal in self._listeners.values()): |
There was a problem hiding this comment.
When a typed vehicle listener has registered the permanent internal config listener, removing the last public listener leaves that internal entry here and closes the stream. With the default manual=False, subsequently adding a public listener computes schedule_refresh = not self._listeners as false, so no new _listen_task is created and the re-added callback receives no SSE events. Determine startup from the transition in non-internal listeners rather than whether the entire registry is empty.
AGENTS.md reference: AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
| if "fields" in config: | ||
| fields = config["fields"] | ||
| if isinstance(fields, dict): | ||
| self.fields = fields |
There was a problem hiding this comment.
When a malformed config event has a dictionary-valued fields member but a non-dictionary field entry, such as {"BatteryLevel": null}, this accepts it as the new last-known-good record. A later add_field("BatteryLevel", ...) then calls .get() on that value and raises AttributeError instead of sending or skipping the update. Validate each field configuration before replacing self.fields, preserving the previous record when the nested shape is malformed.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
async_add_listener's start check still used whole-registry emptiness, so an internal listener surviving auto-close made a later public listener's zero-to-one transition invisible and the owned task never restarted. Both the start and stop checks now count only public (non-internal) listeners. _on_config_event also accepted a "fields" dict whose entries weren't themselves dicts (e.g. a null), which add_field's no-op check then dereferenced with .get() and crashed on. Each entry is now validated; a bad one rejects the whole fields piece and keeps the prior record.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08080ff000
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| async def add_field(self, field: Signal | str, interval: int | None = None) -> None: | ||
| """Handle vehicle data from the stream.""" | ||
| self._ensure_config_listener() |
There was a problem hiding this comment.
When the stream has been closed after its last public listener was removed, external config changes cannot reach this lazily registered internal listener. On the next add_field call, _ensure_config_listener() only registers the callback and does not start or await the stream, so the immediately following check can still use stale self.fields and incorrectly return without re-enabling a field; the same race affects prefer_typed, including typed-listener setup where its add_field task runs before the new connection can deliver config. Refresh authoritative config before these no-op checks or defer them until the initial config event has been consumed.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
The config-sync listener can only observe server-side changes while connected, so a record that went stale during a disconnect (e.g. auto-closed after the last public listener was removed) could pass add_field/prefer_typed's no-op check and wrongly skip a change the server still needs. Both now call get_config() first whenever stream.connected is false, reusing the existing REST fetch rather than adding new state tracking.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 790d9be8a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async def add_field(self, field: Signal | str, interval: int | None = None) -> None: | ||
| """Handle vehicle data from the stream.""" | ||
| self._ensure_config_listener() | ||
| await self._refresh_if_disconnected() |
There was a problem hiding this comment.
When a field listener is added to the initially disconnected stream, _enable_field() runs add_field() as an untracked background task and this new mandatory get_config() executes before the bounded-retry PATCH path. A transient aiohttp.ClientError, timeout, or HTTP error from that GET therefore terminates the task without ever enabling the requested field, leaving the registered callback permanently silent unless the consumer happens to register it again. Treat a failed freshness refresh as retryable or continue with the configuration update instead of aborting it.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
| if not self.stream.connected: | ||
| await self.get_config() |
There was a problem hiding this comment.
During the documented batch-listener setup, every concurrently scheduled add_field() observes the fresh stream as disconnected and independently calls get_config() before any connection can be established. An 80-field setup therefore produces roughly 80 simultaneous REST GETs even though the write side deliberately coalesces the batch into one PATCH, creating a new request storm that can trigger rate limits or setup failures. Share one in-flight per-vehicle refresh before performing the individual no-op checks.
AGENTS.md reference: AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
| if not self.stream.connected: | ||
| await self.get_config() |
There was a problem hiding this comment.
When an active stream uses an explicit topic allowlist that omits config, such as topics=[SseTopic.DATA], stream.connected is true even though the server can never deliver the internal listener's config events. After another client changes a field interval or prefer_typed, this condition skips the REST refresh and the subsequent no-op check can incorrectly decide that no PATCH is needed. Include availability of the config topic in the freshness decision rather than treating every connected stream as synchronized.
AGENTS.md reference: AGENTS.md:L18-L19
Useful? React with 👍 / 👎.
The mandatory get_config() refresh added a failure path and could storm the API with one GET per caller in a batch. The no-op skip is purely an optimization - the server handles a redundant PATCH fine - so add_field/prefer_typed now gate the skip on _record_is_live() (stream connected AND the config topic not filtered out via TeslemetryStream(topics=...)) instead of trying to force the record fresh. When not live, they send unconditionally: one redundant request, same as the pre-feature status quo.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bcee2282d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not self.stream.connected: | ||
| return False | ||
| topics = self.stream.topics | ||
| return topics is None or SseTopic.CONFIG in topics |
There was a problem hiding this comment.
When the stream is already connected before the first config operation, _ensure_config_listener() has only just installed a local callback and cannot replay config events dispatched earlier on that connection, but this method immediately reports the record as live. For example, after get_config() records BatteryLevel, a state listener can connect, another client can remove that field and emit a config event before the lazy listener exists, and the first matching add_field() then skips its PATCH, leaving the callback silent. Track whether the current connection has actually delivered config state before permitting the no-op check. Fresh evidence beyond the earlier disconnected-stream report is that this stale skip remains possible while connected specifically because registration is lazy.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
Lazy registration left a gap: a stream already connected before the listener existed could have dispatched a config event that was simply never seen. The loop hazard that motivated deferring it was never about timing - async_add_listener's schedule_refresh is unconditionally false for internal=True, so it never reaches the asyncio.create_task() call that needs a running loop, regardless of when it's registered. Register it unconditionally in __init__ instead. No connection can now predate the listener, and construction stays loop-free (still asserted by a dedicated regression test). Removes _ensure_config_listener() and its call sites; _record_is_live() is unaffected.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b431af981c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(fields, dict) and all( | ||
| isinstance(value, dict) for value in fields.values() | ||
| ): | ||
| self.fields = fields |
There was a problem hiding this comment.
When a public generic listener retains the same config event and later mutates or normalizes event["config"]["fields"] in place, this assignment aliases that caller-visible mapping, so the mutation silently corrupts the vehicle's last-known-good record. A fabricated matching entry or changed interval can then make add_field() incorrectly skip its PATCH; copy the mapping and each nested configuration before storing it.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
_on_config_event stored the same dict object the event exposed to public listeners, so a consumer mutating event["config"]["fields"] in place after delivery silently corrupted the last-known-good record. Store a copy of the fields mapping and each nested field config.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 744fd2b2ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async for event in self: | ||
| if event: | ||
| for listener, filters in self._listeners.values(): | ||
| for listener, filters, _internal in self._listeners.values(): |
There was a problem hiding this comment.
When a callback on an active multi-vehicle stream calls get_vehicle() for a VIN that is not cached yet, the vehicle constructor now unconditionally inserts its internal listener into _listeners while this loop is iterating the live dict_values view. On the next iterator step Python raises RuntimeError: dictionary changed size during iteration outside the per-callback exception handler, causing listen() to disconnect and finish without restarting even though public listeners remain. Iterate over a snapshot such as list(self._listeners.values()) so callbacks can safely create vehicles or add listeners.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
… loop Eager config-listener registration means a callback that calls get_vehicle() for an uncached VIN mid-dispatch now inserts into _listeners while listen() is iterating it live, raising "dictionary changed size during iteration" and killing the listen task outside the per-callback error handler. listen() now iterates list(self._listeners.values()), a snapshot, so a callback can safely create vehicles or add listeners mid-dispatch.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28e9c61bde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # mid-dispatch must not mutate _listeners while this is | ||
| # iterating it, which would raise RuntimeError and kill | ||
| # the loop. | ||
| for listener, filters, _internal in list(self._listeners.values()): |
There was a problem hiding this comment.
When a public listener is registered before get_vehicle()—for example, a generic callback later creates the vehicle—this insertion-ordered loop invokes that public callback before the vehicle's internal config listener. If the callback removes or normalizes event["config"] in place, the internal listener can miss the match or cache the already-mutated value, corrupting the record used by later no-op checks. Unlike the earlier post-delivery alias case, copying inside _on_config_event does not help because the mutation occurs before that callback runs; use the _internal flag to dispatch bookkeeping first or otherwise isolate its event.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
A public callback could run before the internal config-sync listener purely by registration order, mutate event["config"] in place, and have the internal listener cache the already-mutated value - the defensive copy in _on_config_event happened too late to help. listen() now dispatches over a snapshot sorted internal-first, so bookkeeping listeners always see the pristine event regardless of registration order.
1 participant
Intent
fields/preferTypedrecord onTeslemetryStreamVehicleonly reflected what this client itself had requested or observed at connect, so a config change applied elsewhere (another client, the console, a Teslemetry-side migration) left it stale.add_field/prefer_typed's no-op check, causing either redundant PATCHes or wrongly-skipped ones.configSSE topic and merge server-pushed changes into the record.TeslemetryStreamVehicle.__init__registers an internal listener (_on_config_event) filtered on{Key.VIN, Key.CONFIG: None}, mirroring the existinglisten_State-style per-vehicle filter pattern.get_configresponse shape ({fields, prefer_typed}); a well-typed piece replaces the corresponding record field, a missing piece is left untouched, and a malformed piece is logged and skipped without touching the other piece.TeslemetryStreamVehicle(test doubles included) now needsasync_add_listener- updated the three existing test doubles accordingly.tests/test_config_events.pycovers the record update, the resulting skip/send behavior inadd_field, and malformed/partial events.