Skip to content

Clear the in-flight placeholder on any error in openplantbook.get - #119

Merged
Olen merged 3 commits into
mainfrom
fix/get-placeholder
Nov 7, 2026
Merged

Olen merged 3 commits into
mainfrom
fix/get-placeholder

Conversation

@Olen

@Olen Olen commented Nov 7, 2026

Copy link
Copy Markdown
Owner
  • openplantbook.get removes its in-flight placeholder in a finally whenever the fetch did not finish: on any error or cancellation, not only the three it handles.

Before, an unexpected error (a network error, for example) left the placeholder behind, so every later request for that species waited 10 seconds and failed until the integration was reloaded.

🤖 Generated with Claude Code

An error the service did not expect left the placeholder in the cache, so
every later request for that species waited on it and failed until the
integration was reloaded.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Olen Olen added the bug Something isn't working label Nov 7, 2026
@Olen
Olen requested a balanced review from Copilot November 7, 2026 21:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Pending API request cleanup can raise KeyError during integration unload or delete a newer request’s cache entry after reload.

1 open issue
What changed in this PR

This PR updates openplantbook.get to clear in-flight placeholders after unexpected errors or cancellation, allowing later requests for the same species to retry.

Changes:

  • Move failed-fetch cleanup into a finally block.
  • Add a regression test confirming a request succeeds after an unexpected network error.
File Description
tests/​test_init.py Tests recovery after an unexpected API error.
custom_components/​openplantbook/​__init__.py Centralizes placeholder cleanup for unsuccessful fetches.

Review effort: Balanced

An unload during the API call replaces the species cache; popping from the
original one neither raises nor touches a newer request's entry.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Olen
Olen requested a balanced review from Copilot November 7, 2026 21:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

A cancellation regression test is still missing to verify that cancellation propagates, the placeholder is removed, and a subsequent request succeeds.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Low severity Add a regression test for cancellation cleanup and retry

custom_components/​openplantbook/​__init__.py:271

Please add a cancellation regression test alongside the new ConnectionError test: cancel a request while the mocked API call is pending, verify that cancellation propagates and the placeholder is removed, then verify that a subsequent request succeeds. The current test does not protect the cancellation behavior explicitly promised by this change; it would also pass with cleanup that catches only Exception, which excludes asyncio.CancelledError.

1 resolved since last review

Review effort: Balanced

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Olen

Olen commented Nov 7, 2026

Copy link
Copy Markdown
Owner Author

The test from the last review summary is added in a5bacb6: test_get_plant_works_again_after_a_cancelled_request cancels a request while the API call is pending, checks that the cancellation propagates, and that the next request for the same species succeeds. It fails when the cleanup catches only Exception (checked), while the network-error test still passes then.

@Olen
Olen merged commit 6a01783 into main Nov 7, 2026
6 checks passed
@Olen
Olen deleted the fix/get-placeholder branch November 7, 2026 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants