Harden the broker API and add server-side pagination - #25
Merged
Conversation
Server-side counterpart to the portal modernization. Fixes bugs found while reviewing api/app.py against the stored procedures, and adds the two endpoints the new UI needed. Bugs fixed: - The "No Limit" option never worked. The portal sends limit as the string "null"; the procedures declare @limit INT, so SQL Server failed converting it and all three history endpoints returned 500. Limits are now coerced, and the portal no longer sends the sentinel. - Database connections leaked on every exception path: 27 get_db_connection() calls had only 7 finally blocks. All handlers now use a db_connection() context manager that always closes. - 18 handlers returned the raw str(e) to the caller, exposing driver errors, server names and schema detail. Failures now return {"error": ...} and the detail goes to logger.exception. - 14 print() calls became logger calls, including inside get_db_connection() and Key Vault retrieval, so database and secret failures actually reach Application Insights instead of being swallowed. - /api/scaling/rules returned 404 when no rules existed, which made the portal flash an error rather than render its empty state. /api/scaling/log and /api/scaling/rules/history returned a dict when empty and a list otherwise. All three now return a JSON array with 200. - TriggerScalingLogic ran without committing. pymssql does not autocommit, so the power-state updates and the activity-log insert were rolled back while the Azure power operations still went ahead, leaving Azure and the broker out of step and the scaling activity log permanently empty. - is_member_of_group_cached was defined but never called; token_required used the uncached path, so every authenticated request from the AVD and Linux hosts hit Microsoft Graph. Now wired up, and a Graph failure raises rather than returning False so a throttled call is never cached as a denial. - The scaling procedures relied on implicit MM/DD/YYYY date conversion, which depends on the session DATEFORMAT. They now convert explicitly with style 101, matching GetVmHistory, via TRY_CONVERT. Added: - GET /api/vms/summary, so the dashboard no longer fetches every VM row to compute eight counters. - Opt-in page/per_page pagination on the three history endpoints, backed by new paged procedures that return TotalCount via COUNT(*) OVER (). With neither parameter the response stays a bare array, because the scheduled task and older portal builds consume these as plain lists. - api/tests/ (44 tests) with pymssql and the Azure SDKs mocked, plus CI wiring. Verified by mutation testing: each fix was reverted in turn and the suite failed every time. - api/README.md covering the endpoint surface, auth model, consumer map, error envelope and pagination contract. Front end: - The dashboard uses the summary endpoint, and the history pages use server-side pagination, so whole result sets are no longer cached in the Flask session. That caching grew without bound and let two browser tabs clobber each other. - Both have fallbacks for an API deployed behind the portal. Removed: the unused pyodbc dependency and the /api/vms/available endpoint, which had no callers in the repo. External callers of that endpoint, if any, would need checking before deploying. Not addressed: the 54 Dependabot alerts on the default branch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
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
Server-side counterpart to the portal modernization (#21). Reviewing
api/app.pyagainst the stored procedures turned up a bug that has never worked in production, several systemic reliability and disclosure problems, and two places where the API forced the new UI into the wrong shape.No changes to the AVD host, Linux host agents, or the scheduled task — backward compatibility for those was the hard constraint throughout.
Bugs fixed
limitas the string"null"; the procedures declare@Limit INT, so SQL Server failed the conversionget_db_connection()calls, only 7finally:blocksstr(e)print()calls instead oflogger, including insideget_db_connection()and Key Vault retrievallinuxbroker.apilogger, so database and secret failures never reached telemetry/api/scaling/rulesreturned 404 when empty/api/scaling/logand/api/scaling/rules/historyreturned a dict when empty, a list otherwiseisinstanceworkaroundTriggerScalingLogicnever committed.pymssqldoes not autocommitis_member_of_group_cachedwas defined but never called —token_requiredused the uncached pathMM/DD/YYYYconversion, which depends on sessionDATEFORMATGetVmHistoryalready did it correctlyAdded
GET /api/vms/summary— the dashboard no longer fetches every VM row to compute eight counters.Readyuses the same condition the checkout path uses to select a host.page/per_pageon the three history endpoints, backed by new paged procedures returningTotalCountviaCOUNT(*) OVER (). With neither parameter the response stays a bare array —task/function_app.pyiterates these as plain lists, so that default is load-bearing.api/tests/— 44 tests withpymssqland the Azure SDKs mocked, so they run with no database and no network. Plus CI wiring.api/README.md— endpoint surface, auth model, consumer map, error envelope, pagination contract.Front end
The dashboard uses the summary endpoint and the history pages use server-side pagination, so whole result sets are no longer cached in the Flask session — that was the deliberate deferral from #21, and it grew without bound while letting two browser tabs clobber each other. Both paths have fallbacks for an API deployed behind the portal.
Three bugs found by review of this change itself
Worth calling out, since two were introduced here:
is_member_of_groupreturnsFalseon Graph failure as well as on genuine non-membership. Memoizing that meant one throttled Graph call would deny a principal for the full 5-minute window — and for/vms/checkoutand/vms/<hostname>/releasethe group check is the only authorization path, so it would have blocked every checkout and session release. It now raisesGroupCheckUnavailableand surfaces as 503.total: 0on an out-of-range page, making "page 40 of 4" indistinguishable from "no matches" and collapsing the pager with no way back. Now re-probes for the count./api/vms/summary— Werkzeug matches it against/api/vms/<vmid>, which fails converting'summary'to an int and returns 500. Keying the fallback on 404 guarded a status the old build cannot produce.Verification
is_member_of_grouprather than exercising it), which is exactly the kind of false confidence mutation testing exists to catch — fixed by adding tests against the real function.TriggerScalingLogicfix).Caveat — please read before merging
The stored procedures in
sql_queries/034–039were only statically checked. There is no SQL Server in this environment, so they have never been executed. Column names were verified against theCREATE TABLEscripts and the laterALTERscripts, and the syntax was reviewed, but they need a real deployment run before this is trusted in production.Removed
pyodbcdependency.GET /api/vms/available— no callers anywhere in the repo. It is a published endpoint, so worth checking for external automation before deploying.Not addressed
The 43 Dependabot alerts on the default branch — that deserves its own focused PR.