fix(web): answer capacity exhaustion with 409, not 500 - #1293
Open
boddumanohar wants to merge 1 commit into
Open
fix(web): answer capacity exhaustion with 409, not 500#1293boddumanohar wants to merge 1 commit into
boddumanohar wants to merge 1 commit into
Conversation
A cluster with no room for another logical volume answered create and restore with a 500. Two call sites turn add_lvol_ha's (id, error) tuple into an exception, and neither could tell a refusal from a fault: backup_controller raised RuntimeError, the v2 volume route raised ValueError, and both reached the client as a server error. A full cluster has not failed. It declined a well-formed request, and it will go on declining until capacity is freed or a node is added, so a 5xx is wrong twice over: it blames the control plane for a healthy refusal, and it puts the answer in the status class that every generic retry policy retries. The simplyblock-operator hit exactly that, retrying a restore every 10 seconds indefinitely because a 500 is, by convention, worth retrying. InsufficientCapacityError now carries the condition, and a handler maps it to 409 with a machine-readable "code": "insufficient_capacity", so clients need never match on prose. 409 rather than 507: 507 names the condition more precisely but is a 5xx, which would leave naive clients retrying a permanent refusal. Both refusals are recognized through the constants they are built from (ERR_NO_NODE_WITH_CAPACITY, ERR_OBJECT_LIMIT_PREFIX), so a reworded message cannot silently stop being classified. The endpoint unit tests built a bare FastAPI() carrying none of the real app's exception handlers, so no test in that suite could observe a status code a handler produced. Handler registration moved into register_exception_handlers() and the test app now uses it, which is what lets the new tests assert 409 at all. Two typos in existing handler log messages are fixed in passing. Tests, all four red first: the classification in restore_backup for both refusals, and the 409 at the restore and volume-create endpoints.
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.
A cluster with no room for another logical volume answers create and restore with a 500. It should be a 409.
Companion to simplyblock/simplyblock-operator#478, which worked around this operator-side. This is the contract fix.
The bug
Two call sites turn
add_lvol_ha's(id, error)tuple into an exception, and neither could tell a refusal from a fault:simplyblock_core/controllers/backup_controller.py:490RuntimeErrorapp.py)simplyblock_web/api/v2/.../volume/__init__.py:124ValueErrorA full cluster has not failed. It declined a well-formed request, and it will go on declining until capacity is freed or a node is added. A 5xx is wrong twice over: it blames the control plane for a healthy refusal, and it puts the answer in the status class every generic retry policy retries.
That is not theoretical. The operator retried a restore every 10 seconds indefinitely on exactly this 500 — 124 stuck restores and 2,301 retry events in 29 minutes on a cluster at its object limit — because a 500 is by convention worth retrying. The
PreconditionErrorcases on the same endpoint terminated correctly, because those already return 400.The fix
InsufficientCapacityErrorcarries the condition, and a handler maps it to 409 with a machine-readable discriminator so clients never match on prose:{"error": "Insufficient capacity", "code": "insufficient_capacity", "detail": "..."}Why 409 and not 507. 507 Insufficient Storage names the condition more precisely, but it is a 5xx — choosing it would leave every client that has not special-cased it retrying a permanent refusal, which is the bug. 409 ("conflicts with the current state of the target resource") is accurate, is non-retryable in every client by default, and is already this codebase's idiom for a state conflict (
volume/__init__.py:80).Why 409 and not 422. The request is well-formed and perfectly processable — resubmit it after someone frees space and it succeeds. 422 would tell the client its payload is bad and send it down the wrong remediation path.
Both refusals are recognized through the constants they are built from (
ERR_NO_NODE_WITH_CAPACITY,ERR_OBJECT_LIMIT_PREFIX) rather than duplicated string literals, so a reworded message cannot silently stop being classified.Also fixes two typos in existing handler log messages (
Preciondition,Unexcpected), in code being restructured anyway.Tests
Four, all red before the fix:
test_capacity_exhaustion_is_not_a_server_error— the classification inrestore_backup, parametrized over both refusals. Red withRuntimeError: Failed to create restore volume: ...test_capacity_exhaustion_is_a_conflict_not_a_server_error(restore endpoint) — red with the exception propagating unhandledValueError: No nodes found with enough resources...tox run-parallel -e lint,typesclean.tox run -e unitandtox run -e py314t-unitgreen — 3.14t is fully green (2088 passed); the 3.9 run has one failure,test_snapshot_monitor_phase2.py::test_leaderless_warning_is_rate_limited, which fails identically onmainand is unrelated.Systemic causes
1. A tuple-and-string error return stood in for an exception.
add_lvol_hareturns(False, "message"), whichCONTRIBUTING.mdexplicitly forbids ("never returnNone/booleans for errors"). Because the failure is a bare string, every caller must re-derive what it meant, and the two that did derived it differently —RuntimeErrorin one,ValueErrorin the other, neither saying "the cluster is full." The string matching this PR adds should not need to exist.Durable fix:
add_lvol_haraisesInsufficientCapacityErroritself. Five call sites:simplyblock_web/api/v1/lvol.py:154,simplyblock_web/api/v2/.../volume/__init__.py:85,simplyblock_cli/clibase.py:754,simplyblock_core/controllers/backup_controller.py:469,simplyblock_core/services/snapshot_replication.py:301. Roughly half a day including the v1 and CLI paths. Not done here to keep the blast radius off a hot function; happy to follow up.2. The endpoint unit tests ran against an app that was not the real app.
tests/unit/web/api/v2/conftest.pybuilt a bareFastAPI()with the router and no exception handlers, so no test in that suite could ever observe a status code produced by a handler. That is precisely why "capacity returns 500" survived: the only tests positioned to catch it were structurally incapable of it. Fixed here — handler registration is nowregister_exception_handlers(), used by both apps — and it is what makes the new assertions possible at all.3. Two competing conventions for mapping core exceptions to status codes. Some flow through the global handlers in
app.py; others are caught per-route, as atsimplyblock_web/api/v2/cluster/subsystem/migration.py:99,152, which flattensValueError,MigrationConflictError,PreconditionError, andRuntimeErrorinto a singleHTTPException(400, ...). So a genuine conflict reports 400 instead of 409, and a real server fault reports 400 instead of 500 — the same misclassification as this bug, in the other direction. Durable fix: one typed exception per condition, mapped once globally, and delete the per-routeexceptblocks. Small per site, worth doing as a sweep.4. Nothing tested the status codes these endpoints promise. Follow-on from (2): the suite asserted handler-produced codes nowhere, so any of them could regress unnoticed. The new tests cover 409 on two routes; the other codes are still unasserted.
5. Not documented in OpenAPI. These global handler codes (400, 409, 500) appear in no route's
responses=, sosimplyblock_web/static/openapi.jsondoes not advertise them. Left alone for consistency with the existing pattern —PreconditionError's 400 is undeclared too — but a client generating from the spec cannot see 409 exists. Worth one pass across the routes.🤖 Generated with Claude Code