fix(vehicle): keep config state per vehicle and surface ignored fields - #10
Conversation
![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: 93c30a9052
ℹ️ 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".
| # Take ownership of the batch before sending: a change the API rejects | ||
| # must not remain pending and fail every later update along with it. | ||
| pending = self._config | ||
| self._config = {} |
There was a problem hiding this comment.
If token retrieval, the PATCH request, or response JSON parsing raises (for example during a transient network failure), clearing _config before the await permanently discards every requested field. This is especially problematic for _enable_field, which launches add_field only once, so a later unrelated update no longer retries the lost field. Clear the batch after receiving an API response, or restore it on exceptions while continuing to drop explicit API rejections.
AGENTS.md reference: AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 640eb64f78
ℹ️ 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".
| LOGGER.error( | ||
| "Error updating streaming config for %s: %s", self.vin, error | ||
| ) | ||
| return |
There was a problem hiding this comment.
When a supported field is enabled in the same one-second debounce window as a rejected field such as RouteLastUpdated, pending contains both changes, but this return discards the entire batch after _config was cleared. The supported field is therefore never configured, and _enable_field schedules its update only once, so that listener will not trigger a retry; isolate and retry batch entries individually after a rejection rather than dropping every co-batched change.
AGENTS.md reference: AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e61cf8fa3
ℹ️ 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 error := data.get("error"): | ||
| LOGGER.error("Error updating streaming config for %s: %s", self.vin, error) | ||
| return REJECTED |
There was a problem hiding this comment.
When the endpoint returns a JSON error for an expired credential, rate limit, or temporary server failure, this classifies the response as a field rejection even though patch_config deliberately accepts every HTTP status. A single-field request then reaches _quarantine, and all future add_field calls for that valid field are silently skipped for the lifetime of the vehicle object. Preserve non-validation failures for retry by retaining the response status/error classification rather than treating every error body as proof that the field itself was rejected.
AGENTS.md reference: AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08e1350c9f
ℹ️ 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".
| return APPLIED | ||
| if 400 <= status < 500 and status not in RETRYABLE_STATUSES: | ||
| return PERMANENT | ||
| return TRANSIENT |
There was a problem hiding this comment.
When an isolated RouteLastUpdated or LifetimeEnergyUsedDrive update receives the documented opaque 500, this fallback labels it transient. _settle therefore puts the field back into _config, so every later config change resends the known-dead field and the per-field retry preserves it again, recreating the repeated-500/request-amplification problem this commit is intended to remove. Make the classification/isolation flow recognize a repeatable isolated rejection rather than retaining it indefinitely.
AGENTS.md reference: AGENTS.md:L10-L12
Useful? React with 👍 / 👎.
|
|
||
| async def patch_config(self, config: dict[str, Any]) -> dict[str, Any]: | ||
| """Modify the configuration for the vehicle.""" | ||
| entries = split_config(pending) |
There was a problem hiding this comment.
When a debounced batch has multiple entries and the initial response is 429, 401/403, or an outage-related 5xx, the outcome is TRANSIENT, but this path still immediately PATCHes every entry individually. A startup batch can therefore turn one rate-limited request into dozens under the same rate limit or expired credential, worsening throttling and load; retain the whole transient batch for a later update unless the response is specifically eligible for field isolation.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
| if 400 <= status < 500 and status not in RETRYABLE_STATUSES: | ||
| return PERMANENT |
There was a problem hiding this comment.
If the API returns an unrecognized 4xx such as 404 or 409, this broad status check declares it permanent even though only the field/schema rejection cases are known to be permanent. For a single-field update, _settle then adds the valid signal to _rejected, so a temporary missing configuration or conflict prevents every future add_field call for that signal for the lifetime of the vehicle object. Match explicit permanent statuses or error signatures and leave other 4xx responses transient.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e3897093e
ℹ️ 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".
| # An opaque failure names nothing, so only a change that fails on its | ||
| # own twice has shown the change itself to be the cause. | ||
| condemned = outcome == PERMANENT or ( | ||
| outcome == OPAQUE and fields and all(f in self._suspect for f in fields) |
There was a problem hiding this comment.
If a field's requested value changes between opaque failures—for example, add_field("BatteryLevel", interval=10) gets a 500 and a later call replaces it with interval 20—this check treats the second payload as a repeat failure because _suspect stores only the field name. The interval-20 change is consequently quarantined after its first failure, and all later attempts for that field are suppressed for the vehicle object's lifetime. Key suspicion by the complete field entry, or clear it when the pending value changes, so each distinct change must fail in isolation twice.
AGENTS.md reference: AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68da1bf68a
ℹ️ 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".
| field, | ||
| self.vin, | ||
| ) | ||
| self._rejected.add(field) |
There was a problem hiding this comment.
When a permanent 400/422 is caused by one particular field value—for example, an invalid interval passed to add_field—this stores only the field name in _rejected. A later call with a corrected interval is then silently skipped at the start of add_field, so the valid configuration cannot be applied for the lifetime of this vehicle object. Track the rejected entry's complete value, as _suspect now does, so a changed configuration is judged independently.
AGENTS.md reference: AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 170a8aaf99
ℹ️ 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 _send_config(self, batch: dict[str, Any]) -> str: | ||
| """Send one config batch and record it if the API confirms it.""" | ||
| status, data = await self._patch_config(batch) |
There was a problem hiding this comment.
For subclasses or test harnesses that override or monkey-patch patch_config, which update_config previously invoked, this direct call to _patch_config silently bypasses the override and may perform a real session request instead. Retaining the public wrapper's signature does not preserve that delegation behavior; the status-aware path should still provide a compatible overridable hook.
Useful? React with 👍 / 👎.
Bre77
changed the title
170a8aa to
ffdc03a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffdc03a6ca
ℹ️ 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".
| LOGGER.info("Updated vehicle streaming config for %s", self.vin) | ||
| if fields := self._config.get("fields"): | ||
| LOGGER.debug("Configured streaming fields %s", ", ".join(fields.keys())) | ||
| self.fields = {**self.fields, **fields} |
There was a problem hiding this comment.
When the API returns ignoredFields, this still copies every requested field into self.fields, including fields the server explicitly dropped. A later add_field for one of those fields then treats it as already enabled and returns without sending another request, so the library permanently reports unavailable telemetry as configured for this vehicle instance. Filter the ignored names before updating self.fields.
AGENTS.md reference: AGENTS.md:L9-L11
Useful? React with 👍 / 👎.
| LOGGER.info("Updated vehicle streaming config for %s", self.vin) | ||
| if fields := self._config.get("fields"): | ||
| LOGGER.debug("Configured streaming fields %s", ", ".join(fields.keys())) | ||
| self.fields = {**self.fields, **fields} |
There was a problem hiding this comment.
After a successful default add_field(field) call, the submitted field value is None, and this assignment stores that None in self.fields. Calling add_field for the same field again—such as when a second listener for the signal is registered—then executes self.fields[field].get(...) and raises AttributeError. Record an empty mapping (or otherwise make the duplicate-field check handle None) for interval-free fields.
AGENTS.md reference: AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
Pending config, known fields and prefer_typed were class attributes, so every vehicle shared one dict and one vehicle's pending change was sent for another. The success branch looked for updated_vehicles under a response key, but the config API reports success flat, so it never ran: fields were never recorded and the pending batch was never cleared. Read the real shape, and warn naming anything the API reports in ignoredFields. Also record prefer_typed by value - it was assigned the result of an 'in' test, so setting it False set it True.
ffdc03a to
6c88f3c
Compare
1 participant
Intent
Three defects in
TeslemetryStreamVehicle's config handling, all visible against the live API.fields,preferTypedand_configwere class attributes, never assigned in__init__, so every vehicle mutated one dict — one vehicle's pending change was sent for another. Now per-instance.updated_vehiclesunder aresponsekey, but the config API reports success flat as{"updated_vehicles": 1}and only wraps errors as{"response": null, "error": ...}. That lookup never matched, so configured fields were never recorded and the pending batch was never cleared — it grew and was resent on every later update.prefer_typedwas inverted. It was assigned the result of... in [True, False], i.e. whether the value is a bool rather than the value, soprefer_typed(False)setpreferTypedtoTrue.Alongside this, the API now accepts requests containing fields it has retired and names them in a top-level
ignoredFieldslist instead of failing. Those are logged as a warning naming the fields, and excluded from the recorded config — recording them would report unavailable telemetry as enabled and stopadd_fieldfrom ever asking again. A field requested at its default is recorded as an empty options mapping rather thanNone, so a second listener on the same signal does not raise.tests/test_config_update.pycovers this against the response bodies the API actually returns, and fails onmain.