fix(updates): seed Pending state on register so /updates/{id}/status returns 200#379
fix(updates): seed Pending state on register so /updates/{id}/status returns 200#379
Conversation
…returns 200
Previously UpdateManager::register_update delegated to the backend and
returned without touching states_, so the very next GET /updates/{id}/status
returned 404 with vendor_code x-medkit-update-not-found until the caller
called /prepare. Any UI that gates actions on a non-null status response
(e.g. UpdatesDashboard, which hides Prepare/Execute/Delete buttons when
the status fetch fails) saw newly-registered updates as "Status
unavailable" with no way forward.
Fix: after a successful backend register, seed states_[id] with a
PackageState in phase None and status Pending (idempotent - preserves
any already-seeded state, for symmetry with the defensive pattern used
in the prepare/execute paths).
Unit test StatusPendingRightAfterRegister verifies the new contract:
register returns OK, the immediately following get_status returns a
Pending status with no progress/error. StatusNotFoundForUnknown still
covers the NotFound path for truly unregistered IDs.
There was a problem hiding this comment.
Pull request overview
Seeds a default in-memory update status immediately after successful update registration so /updates/{id}/status returns 200 with pending right away (avoiding an initial 404 that blocks some clients’ UX flows).
Changes:
- Seed
states_[id]withphase=Noneandstatus=Pendingafterbackend_->register_update(...)succeeds. - Add a unit test asserting
Pendingstatus is available immediately afterregister_update.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ros2_medkit_gateway/src/updates/update_manager.cpp | Seeds an initial Pending status for newly registered updates to make status queries succeed immediately. |
| src/ros2_medkit_gateway/test/test_update_manager.cpp | Adds a unit test covering the “pending right after register” behavior. |
| // Seed an initial Pending state so GET /updates/<id>/status returns | ||
| // {"status":"pending"} immediately after register, rather than 404. UI | ||
| // consumers (UpdatesDashboard) gate action buttons on the status field | ||
| // and won't render "Prepare" when the status query fails, so a newly- | ||
| // registered update would otherwise be stuck from the frontend's view. | ||
| // Preserve an already-seeded state if a concurrent caller raced us | ||
| // through the backend (shouldn't happen with current backends, but the | ||
| // defensive check keeps the method idempotent). | ||
| if (metadata.contains("id") && metadata["id"].is_string()) { | ||
| const auto id = metadata["id"].get<std::string>(); | ||
| std::lock_guard<std::mutex> lock(mutex_); | ||
| auto & state_ptr = states_[id]; | ||
| if (!state_ptr) { | ||
| state_ptr = std::make_unique<PackageState>(); | ||
| state_ptr->phase = UpdatePhase::None; | ||
| state_ptr->status = UpdateStatusInfo{UpdateStatus::Pending, std::nullopt, std::nullopt, std::nullopt}; | ||
| } | ||
| } |
There was a problem hiding this comment.
The REST API docs currently state that GET /api/v1/updates/{id}/status returns 404 when no operation has started. With this change, a newly registered update will have a seeded Pending state, so the docs should be updated to reflect that 200 pending is returned immediately after a successful POST /updates (and reserve 404 for unknown/unregistered IDs).
| // Seed an initial Pending state so GET /updates/<id>/status returns | ||
| // {"status":"pending"} immediately after register, rather than 404. UI | ||
| // consumers (UpdatesDashboard) gate action buttons on the status field | ||
| // and won't render "Prepare" when the status query fails, so a newly- | ||
| // registered update would otherwise be stuck from the frontend's view. | ||
| // Preserve an already-seeded state if a concurrent caller raced us | ||
| // through the backend (shouldn't happen with current backends, but the | ||
| // defensive check keeps the method idempotent). | ||
| if (metadata.contains("id") && metadata["id"].is_string()) { | ||
| const auto id = metadata["id"].get<std::string>(); | ||
| std::lock_guard<std::mutex> lock(mutex_); | ||
| auto & state_ptr = states_[id]; | ||
| if (!state_ptr) { | ||
| state_ptr = std::make_unique<PackageState>(); | ||
| state_ptr->phase = UpdatePhase::None; | ||
| state_ptr->status = UpdateStatusInfo{UpdateStatus::Pending, std::nullopt, std::nullopt, std::nullopt}; | ||
| } |
There was a problem hiding this comment.
This change affects observable REST API behavior (GET /updates/{id}/status will now return 200 pending immediately after POST /updates). There are existing integration tests for /updates endpoints, but none currently assert this specific behavior. Please add/adjust a launch_testing integration test (e.g., in src/ros2_medkit_integration_tests/test/features/test_updates.test.py) that registers a package and immediately fetches /updates/{id}/status, asserting 200 with status==pending, while keeping the unknown-ID 404 case.
Summary
Closes #378. After a successful
backend_->register_update(...), seedstates_[id]withphase=None/status=Pendingso the firstGET /updates/{id}/statusreturns200 {"status":"pending"}instead of404 vendor-error. Clients that gate UI on a non-null status (web UIUpdatesDashboard) can now render Prepare / Execute / Delete buttons immediately after registering a new update.Idempotent - preserves an already-seeded state if a concurrent caller got there first (matches the defensive pattern already used in
start_prepare/run_prepare).Test plan
UpdateManagerTest.StatusPendingRightAfterRegistercovers the happy path.StatusNotFoundForUnknownstill covers the NotFound path for truly unregistered IDs.test_update_managersuite passes locally (20/20).