Repository navigation
Conversation
The resource fields accepted any string and were read with parseInt, so "1792MB" became 1792 bytes and "1.5" CPU became 1 nanoCPU, which Swarm then rejected at deploy time. Require whole numbers no lower than Docker's minimums (empty or 0 still means no limit) and fix the reservation tooltips.
Comment on lines
+223
to
+225
| .refine((value) => /^0*$/.test(value) || Number(value) >= minimum, { | ||
| message: `Must be at least ${minimum} ${unit}`, | ||
| }) |
Contributor
There was a problem hiding this comment.
Large resource numbers lose precision
resourceValueSchema accepts digit strings that JavaScript cannot preserve as integers. For example, 9007199254740993 passes, but calculateResources converts it to 9007199254740992. Sufficiently long strings become Infinity.
Check that nonzero values convert to safe integers, apply the same check in show-resources.tsx, and add boundary tests. This would keep accepted values from changing when the server prepares them for deployment.
Digit strings past Number.MAX_SAFE_INTEGER lose precision or become Infinity when converted, so the value sent to Docker would differ from the one saved.
This branch has not been deployed
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
The Resources form and the API accepted any string for Memory Limit, Memory Reservation, CPU Limit and CPU Reservation.
calculateResourcesreads them withNumber.parseInt, so1792MBbecame 1792 bytes and1.5CPU became 1 nanoCPU. Swarm rejected the service spec at deploy time withMust be at least 4MiB/Must be at least 0.001, and values the manager accepts but the engine refuses (memory below 6 MiB) fail on the node instead.The two reservation tooltips were also wrong: reservations are used for scheduling placement only and never limit the container.
Changes
db/schema/shared.ts:resourceBytesSchemaandresourceNanoCpusSchema. Empty or0still means no limit; otherwise the value must be a whole number no lower than Docker's minimum (6 MiB of memory, 0.001 CPU), with a message that names the unit.updateendpoints reject a bad value on save instead of at deploy.show-resources.tsx: the same rule in the form schema, so the user sees an inline message before submitting; reservation tooltips corrected.Notes: the 6 MiB minimum is applied to reservations as well as limits for simplicity (Swarm's own floor for reservations is 4 MiB). Values already stored are not migrated; an invalid one shows the inline error the next time the form is saved.
openapi.jsonis not regenerated here since CI does that on canary.Tests
__test__/server/resource-schema.test.ts(38 tests): both helpers accept null, empty,0and valid digit strings and reject1792MB,1.5,2e9, negative and under-minimum values with one message each; everyapiUpdate*schema rejects a bad value and acceptsnulland a valid value;calculateResourcesbehaviour.libsql/create-schema,mechanizeDockerContainer,application.command,preview-build-server,provider-status: pass. Full suite: 1532 passed.tsc --noEmitforserveranddokploy: pass. Biome: clean.The PR appears safe to merge, with a non-blocking improvement needed for numbers outside JavaScript’s safe integer range.
Summary
Adds resource checks before saving application and database settings.
Acknowledged choices:
mjafartpexplicitly chose the 6 MiB minimum for memory reservations as well as limits, for simplicity.mjafartpexplicitly deferred migration of stored values and regeneration ofopenapi.json; CI regenerates the latter on canary.Reviews (1) · Last reviewed commit: "fix(resources): validate memory and CPU ..." · Reviewed by Greptile