Repository navigation
fix(sdl): report every sdl the manifest cannot be built from as invalid input - #4054
Conversation
…id input Commit 6903b3b turned chain-sdk's "Invalid size string" throw into a 400, but that is only one of the ways the manifest generator throws on a document its own validator accepts. GPU units that are not a whole number fail the BigInt conversion. A deployment entry for a service the SDL never declares fails only when the manifest groups are first read, and that read happened outside the guard, while the manifest version was being hashed. Both still answered 500 on POST and PUT /v1/deployments, which pages the 5xx alert. The generator reads nothing but the document, so the guard now reports anything it throws as a validation error with the generator's own message. It also reads the lazily built groups and reclamation before leaving the guard. A size without a unit keeps the message that commit gave it. A thrown value that is not an Error still propagates, as does every failure outside the generator (reference resolvers, the key service, the database, the signer), so those still answer 500. The auditor requirement is written into the placements before chain-sdk validates the document, and it threw on a placement, signedBy or anyOf that was not the mapping or list it writes to. It now skips those shapes and leaves them to the validator, whose 400 names the field. Create, update and patch all build the manifest before they reclaim trial orphans, seal secrets, record the definition, create or update the deployment, or push a manifest, so a refused document is never recorded or sent anywhere.
|
Warning Review limit reached
This review includes 8 billable files and costs up to $2.00.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 52 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: akash-network/console/.coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (8)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4054 +/- ##
==========================================
- Coverage 87.77% 87.62% -0.15%
==========================================
Files 1282 1177 -105
Lines 36138 33422 -2716
Branches 8603 8060 -543
==========================================
- Hits 31720 29286 -2434
+ Misses 3909 3646 -263
+ Partials 509 490 -19
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes error-handling behavior for every SDL-driven deployment create/update (POST and PUT /v1/deployments) and the author themselves flags a real tradeoff to weigh (a genuine chain-sdk bug on a valid SDL would now surface as a 400 instead of paging), a human look would still be worthwhile.
What was reviewed: the new generateManifestReportingBuildFailures (catches any Error while building/reading groups, groupSpecs, reclamation, maps size errors to the existing size ValidationError and everything else to a generic manifest ValidationError, rethrows non-Error throws); the canTakeAuditors guard replacing the old truthy check in #appendAuditorRequirement; and the unit/functional tests confirming a size-without-unit SDL and an SDL deploying an undeclared service both cleanly 400 with no reclamation/seal/db-write/broadcast side effects, while a non-SDL failure (e.g. secret store unreachable) still propagates as-is.
Extended reasoning...
The diff replaces sdl-sizes.ts with sdl-manifest.ts, broadening a narrow catch (only "Invalid size string") into a catch-all that maps any Error thrown while building or eagerly reading chain-sdk's manifest (groups/groupSpecs/reclamation) into a 400 ValidationError, and adds a canTakeAuditors guard so malformed placement/signedBy/anyOf shapes are deferred to chain-sdk's own validator instead of crashing. It touches no auth/crypto/secrets logic itself but does change how all deployment create/update requests handle manifest-build failures, a central production path; new unit, service and functional tests (including negative-path assertions that no reclamation/seal/db-write/broadcast happens) cover the new behavior well and mirror the CLAUDE.md setup/mock conventions. No findings were raised by the bug hunt, but the PR author explicitly calls out a design tradeoff (masking a real chain-sdk bug as a 400 instead of paging) that is worth a human sign-off given the criticality of the path.
1 participant
Why
On 2026-09-24 at 10:56 and 11:03 UTC,
POST /v1/deploymentsanswered 500 for an SDL that gave a storage size as a bare byte count, and it paged the 5xx alert. #4033 turned that case into a 400 later that day. It's only one of the ways chain-sdk's manifest generator throws on a document its own validator accepts, and the others still answer 500 on POST and PUT:unitsthat aren't a whole number (1.5,"abc") fail the BigInt conversion with a RangeError or SyntaxError.signedByoranyOfgiven as a plain value crashed the step that adds the allowed auditors, before chain-sdk ever validated the document.What
"anyOf" at "/profiles/placement/dcloud/signedBy" should be array.).One tradeoff to weigh in review: a real chain-sdk bug that breaks a valid SDL would now surface as a 400 to the caller instead of paging. The error handler still logs it at error level.
Stryker kills 53 of 54 mutants on the changed lines. The survivor is equivalent: replacing
[]inArray.isArray(anyOf ?? [])with another array gives the same result.