fix(web): plugin action params, refused on-demand starts, pending-operation 500, double-click 409, binary static files - #744
Merged
Conversation
POST /api/v3/plugins/action runs a plugin's script through a generated
Python wrapper, and the params went into that wrapper's source as
`params = <json.dumps(params)>`. JSON true, false and null are undefined
names in Python, so any params holding one made the wrapper die with a
NameError before the script ran, and the route answered "Action failed".
The plugin file manager's category toggle sends {"category_name": ...,
"enabled": true}, so of-the-day's category toggle failed every time.
The wrapper now reads the params from its own stdin (json.loads) and the
route passes them there; nothing taken from the request is written into
the generated source any more. The script's side is unchanged: the same
json.dumps(params) on its stdin, LEDMATRIX_ROOT set, stdout parsed.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
POST /api/v3/display/on-demand/start delivered the request (control socket, else the file mailbox) before it checked the display service. With the service stopped the socket is absent, so the request went to the mailbox; the route then answered 400 "Display service is not running" when start_service was off, or 500 "Failed to start display service" when the start failed. The display reads that mailbox with max_age=3600 and never checks a request's timestamp, so the next time it was started it ran the refused request, pinned if asked. The service is now checked before anything is delivered, and nothing is posted when start_service is off and the service is down. When the start itself fails, the request is withdrawn from the mailbox, but only while the mailbox still holds this request_id (the compare-before-delete the display's _consume_on_demand_request uses), so a newer request posted in the meantime is left for the display. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PluginOperationQueue.enqueue_operation stores the operation's callback in operation.parameters['_callback'], and the worker pops it only when it runs the operation. PluginOperation.to_dict() returned parameters as they were, so GET /api/v3/plugins/operation/<id> for an operation still waiting in the queue (an install queued behind another plugin's) handed jsonify a function and answered 500 "A system error occurred" on every poll until the worker reached it. to_dict() now leaves out parameters whose name starts with "_". The operation itself keeps its callback for the worker; every other field of the answer, and the operation-history records (a different class), are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PluginOperationQueue.enqueue_operation raises ValueError when the plugin already has an operation waiting or running. /plugins/install did not catch it, so a double-clicked Install (the button is never disabled) answered 500 "An error occurred; see logs for details" from the blueprint's catch-all while the first install carried on. /plugins/uninstall caught it in its own catch-all: a 500 "Failed to uninstall plugin", plus an "uninstall failed" operation-history record for an uninstall that never started. Both routes now enqueue through _enqueue_or_conflict, which turns the queue's refusal into a 409 PLUGIN_OPERATION_CONFLICT naming the plugin, and records nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GET /api/v3/plugins/<plugin_id>/static/<path> read every file with open(..., 'r', encoding='utf-8') and returned the decoded text, so any binary file -- a plugin icon or preview image, which is what the REST API reference says the route is for -- raised UnicodeDecodeError and answered 500. The file is now sent with send_file, as bytes. HTML, JavaScript, CSS and JSON keep the content types the route always set, and other text keeps text/plain; anything else gets the type mimetypes knows it by (image/png for a .png). The plugin id and path validation and the resolve_under containment check are untouched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cdaeb38 checked the systemd unit before delivering the on-demand request, so a display run by hand or in the emulator (no active unit) with start_service off now got nothing, where before the request went over the control socket and took effect behind a 400. A socket acknowledgement is the display itself saying it is running and has the request queued, so it is the better witness than systemd. The request is delivered first again. When the display acknowledged it over the socket, the route answers success without consulting systemd for the "not running" 400 and without starting the unit (with start_service on it tried to start a second display beside the one that answered); the service is still reported the way _ensure_display_service_running reports a running one. When it went to the mailbox, the 400 (service down, start_service off) and the failed-start 500 both withdraw this request_id from the mailbox, leaving a newer request alone, so neither refusal runs later. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 29 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 12 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Five web API bugs, one commit each, plus one refinement.
true,falseornull(plugins.py).json.dumps(params)into generated Python source (params = {...}), wheretrueis an undefined name. The wrapper died with a NameError.{"enabled": true}and always failed.request_id.start_serviceon, the route also stops trying to start a second display beside it.to_dict()serialized the queued_callback._-prefixed) parameters are left out. The operation keeps its callback.409 PLUGIN_OPERATION_CONFLICTwith no history record.send_file. The path containment checks are unchanged..svgis nowimage/svg+xml, and text is no longer newline-normalised.Tests
test_api_v3_plugin_action_params.py(8),test_api_v3_on_demand_restart.py::TestARefusedStartLeavesNoRequestBehind(6),test_api_v3_operation_status_pending.py(4),test_api_v3_plugin_operation_conflict.py(3),test_api_v3_plugin_static_files.py(10). Each fails on main.On ledpi: the of-the-day
toggle-categoryaction with"enabled": truereturnedsuccess; on main that call dies with a NameError. An on-demand start was acknowledged over the socket, and current-status showed it.Part of a bug sweep
This is one of 10 independent fix PRs from one sweep, all based on
mainef69201.main: the same 62 failures and 6 errors on both. These are the known Windows path and file-locking tests. 191 more tests pass.test_backup_manager.py::test_create_backup_contents(os.replace→WinError 5on a temp zip), was a Windows file-lock flake. It passes on rerun, and nothing here touchescreate_backup.main, with a clean start and no errors or render stalls in the journal. ledpi is back on plainmain.🤖 Generated with Claude Code