fix: AE-2316: field validator for serverless idleTimeout#287
Conversation
|
📝 Documentation updates detected! New suggestion: Document valid range for idle_timeout parameter Tip: Filter the Dashboard by labels or assignees to focus on what matters to you 🔎 |
There was a problem hiding this comment.
Pull request overview
This PR adds construction-time validation for the idleTimeout field on serverless endpoints to match backend constraints (1–3600 seconds), preventing invalid configs from failing later at deploy time.
Changes:
- Add a Pydantic
field_validatorforidleTimeoutinServerlessResource. - Add unit tests covering invalid and boundary-valid
idleTimeoutvalues forServerlessResource. - Add a unit test confirming
LiveServerlessrejects an invalididleTimeout.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/runpod_flash/core/resources/serverless.py |
Enforces idleTimeout range (1–3600) during model construction via field_validator. |
tests/unit/resources/test_serverless.py |
Adds parametrized tests for invalid values and valid boundary values. |
tests/unit/resources/test_live_serverless.py |
Adds a regression test ensuring LiveServerless inherits/enforces the idleTimeout validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
runpod-Henrik
left a comment
There was a problem hiding this comment.
-
Validator correctness and placement
The validator is placed on ServerlessResource, which is the common base for all concrete resource types. A single validator here covers the entire hierarchy. The bounds 1 <= value <= 3600 match the stated backend restriction. The default of 60 falls within the valid range. The None pass-through is correct. -
Error timing for Endpoint users
The PR description says "errors happen at construction time rather than deploy time." This is true for direct ServerlessResource use. For Endpoint users (@endpoint(idle_timeout=...)), the error fires at decoration time — still much better than deploy time, and acceptable behavior. -
Test coverage
test_serverless.py has good parametric coverage: [0, -1, 3601] for rejection and [1, 3600] for boundary acceptance.
test_live_serverless.py adds only a single test (idleTimeout=0). Missing: upper bound rejection (3601) and valid boundary acceptance (1, 3600). Since LiveServerless inherits the validator with no override, the gap is low risk, but inconsistent with the base-class test pattern established in this same PR.
- Missing Endpoint-level unit test
The new unit tests exercise ServerlessResource and LiveServerless directly. There is no unit-level test for Endpoint(idle_timeout=0) or Endpoint(idle_timeout=3601) raising ValueError. A companion test in test_endpoint.py would complete the coverage.
Nits
Error message says "idleTimeout must be between 1 and 3600 seconds" — camelCase matches other validators in the file. Consistent.
Verdict
PASS WITH NITS — validator is correct, placement covers the full class hierarchy, and bounds match the stated backend constraint. The test_live_serverless.py addition only tests one of the three rejection cases covered in test_serverless.py; rounding it out would be a small improvement.
🤖 Reviewed by Henrik's AI-Powered Bug Finder
adds a field validator to
idleTimeoutso errors happen at construction time rather than deploy time; backend currently restrictsidleTimeoutto between 1 and 3600s.