fix: close reconnect-window gap in add_field/prefer_typed no-op skip - #31
fix: close reconnect-window gap in add_field/prefer_typed no-op skip#31Bre77 wants to merge 1 commit into
Conversation
connected flips true as soon as connect() installs the response, before listen() has dispatched the connection's config snapshot. A connection listener calling add_field()/prefer_typed() in that window could match against the stale pre-disconnect record and skip its PATCH, with nothing retrying it once the snapshot later revealed the mismatch. _record_is_live() now also requires the vehicle's last-synced connection id to match the stream's current one, closing the gap until this connection's own snapshot has actually been applied.
![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: 68c1aeab3b
ℹ️ 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 | ||
|
|
||
| self._synced_connection_id = getattr(self.stream, "_connection_id", 0) |
There was a problem hiding this comment.
When the first config event after a reconnect omits or malforms either fields or prefer_typed, this assignment still advances the vehicle to the current connection ID even though _on_config_event deliberately retains that piece from the previous connection. _record_is_live() can then trust the mixed stale/current record and incorrectly skip a required add_field() or prefer_typed() PATCH; track synchronization per piece or only mark the complete record synced after every required snapshot component validates.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
|
Superseded by #32 - a simpler fix (populated flag + lazy fetch, no connection-id/snapshot tracking) per captain direction. Closing. |
… fetch (#32) add_field/prefer_typed no longer gate their no-op skip on connection/topic state (_record_is_live). Instead, TeslemetryStreamVehicle tracks whether its record has ever been populated (by a push event or a REST fetch); an unpopulated vehicle awaits a single-flight get_config() before deciding, a populated one trusts the optimistically-updated record outright. A disconnect notification unpopulates the record so a call landing in the reconnect window (before the next connection's config snapshot arrives) refetches instead of trusting stale pre-disconnect data. This replaces the mechanisms proposed in #30 (a second dispatch pass for listeners added mid-event) and #31 (per-connection snapshot tracking) with a smaller, self-contained fix scoped to the vehicle itself.
1 participant
Intent
TeslemetryStream.connectedflips true as soon asconnect()installs the response, beforelisten()has read the connection's post-connect config snapshot off the wire.add_field/prefer_typed's no-op skip is gated on_record_is_live(), which only checked "connected and not topic-filtered" - not "has this connection's snapshot actually arrived yet".add_field()/prefer_typed()in that window (e.g. an integration re-asserting its desired fields on reconnect) could match against the stale pre-disconnect record and skip its PATCH. Nothing retries it once the snapshot later reveals the mismatch.TeslemetryStream._connection_idis bumped once per successfulconnect().TeslemetryStreamVehicle._synced_connection_idis set to it inside_on_config_event;_record_is_live()now also requires the two to match.getattr(..., 0), so test doubles that never call the realconnect()(i.e. don't define_connection_id) keep the pre-existing "live" behavior unchanged.tests/test_reconnect_config_window.pyreproduces the race against unfixed code (connect -> reconnect-listener schedulesadd_field-> snapshot arrives with a differing value -> PATCH wrongly skipped) and is now the regression test.