feat: send bytes= query param on upload measurements#120
Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
This PR adds the bytes=<size> query param to upload requests (previously only downloads sent it), and teaches LoggingBandwidthEngine to log the server-reported cf-meta-upload-bytes value when present, so capped uploads log the accepted byte count rather than the client-requested size.
What changed:
BandwidthEngine.ts:bytes=is now set unconditionally on the query string (both up and down) instead of download-only.LoggingBandwidthEngine.ts: two new exported helpers (parseUploadBytesHeader,getLoggedBytes) plus#uploadBytesstate captured in the response hook and consumed in#logMeasurement.- New unit test file covering both helpers (good — validates the safe-integer / regex guards).
tests/e2e/speedtest.test.ts: ~264 lines changed, but almost entirely reindentation from re-wrapping theit(...)call plus aneslint-disable compat/compatheader — no substantive test-logic change.Results.test.ts: unrelated type/formatting cleanup.
Overall: the core change is small, correct, and well-tested. The #uploadBytes state follows the existing #token/#requestTime convention and getLoggedBytes correctly guards on type === 'up', so download logging is unaffected. No security concerns. A few minor points inline.
| ): number => | ||
| measData.type === 'up' && uploadBytes !== undefined | ||
| ? uploadBytes | ||
| : measData.bytes; |
There was a problem hiding this comment.
After this change the logged bytes field is semantically asymmetric: for up it can be the server-accepted byte count, while for down it stays the requested count. That's intentional per the PR, but it's a non-obvious contract for log consumers. Consider a short doc comment on getLoggedBytes (or the logData.bytes line) noting that upload bytes may reflect the server cap, so future readers don't assume bytes is always the requested size.
|
|
||
| // get request time | ||
| this.#requestTime = +r.headers.get(`cf-meta-request-time`)!; | ||
| this.#uploadBytes = parseUploadBytesHeader(r.headers); |
There was a problem hiding this comment.
#uploadBytes is set here in the response hook and cleared in #logMeasurement — same lifecycle as #token/#requestTime, so this is consistent with the existing pattern. One thing to confirm: if a response hook fires for a request that never produces a measurement result (e.g. an error/abort path), #uploadBytes would persist until the next #logMeasurement. This is exactly the pre-existing behavior for #token/#requestTime, and getLoggedBytes guards on type === 'up', so a stale value can't leak into a download log. No change required — flagging for awareness.
| import { describe, it, expect } from 'vitest'; | ||
| import SpeedTest from '../../src/index.ts'; | ||
|
|
||
| /* eslint-disable compat/compat */ |
There was a problem hiding this comment.
The bulk of this file's diff (~264 lines) is pure reindentation from re-wrapping the it(...) call, plus this eslint-disable compat/compat. That makes the upload-bytes change harder to review and risks merge-conflict noise. If the reindent isn't strictly required for the feature, consider splitting it into a separate formatting-only commit/PR. Also worth a brief comment explaining why compat/compat is disabled here (which API triggered it), since blanket file-level disables tend to mask future real violations.
|
Review posted successfully to PR #120 with 4 inline comments. Review summaryI reviewed PR #120, verified the build/lint pass, and ran the unit tests (98 passing). Posted a review with a summary and 4 inline comments. What the PR changes:
Findings (no blockers):
|
|
/bonk |
There was a problem hiding this comment.
Review summary
This PR sends the bytes=<size> query param on upload requests (previously download-only) and teaches LoggingBandwidthEngine to log the server-reported cf-meta-upload-bytes value when present, so capped uploads log the server-accepted byte count instead of the client-requested size.
What changed (verified against main):
BandwidthEngine.ts:348—qsParams.bytes = ${numBytes}is now set unconditionally instead ofisDown && (...). SincenumBytesequals the body produced bygenContent(numBytes), this is a no-op signal for uncapped servers and only matters when the server caps.LoggingBandwidthEngine.ts— two new exported pure helpers (parseUploadBytesHeader,getLoggedBytes), plus#uploadBytesstate captured in#loggingResponseHookand consumed/cleared in#logMeasurementfollowing the existing#token/#requestTimelifecycle.- New unit test file covering both helpers, including invalid-header and download-guard cases.
tests/e2e/speedtest.test.ts— 2 lines added (aneslint-disable compat/compat+ explanatory comment for thePromiseusage).Results.test.ts— replacesas anywith a typedResultsConfigalias; unrelated cleanup.
Verification: pnpm build, pnpm lint, and pnpm test:unit (98 passing) all pass on the head commit.
Correctness: Requests are issued strictly sequentially (#nextMeasurement only re-fires after the current request resolves), so the single-field #uploadBytes state cannot race across concurrent requests, and getLoggedBytes guards on type === 'up' so a stale upload value can never leak into a download log. The change is small, correct, and well-tested.
Security: None.
A few minor, non-blocking points inline.
Note: I could not reproduce a prior characterization of the e2e diff as ~264 lines of reindentation — the actual diff there is a 2-line addition.
|
|
||
| const bytes = Number(value); | ||
| return Number.isSafeInteger(bytes) ? bytes : undefined; | ||
| }; |
There was a problem hiding this comment.
parseUploadBytesHeader accepts '0' (it passes /^\d+$/ and Number.isSafeInteger(0)), so a cf-meta-upload-bytes: 0 response would make getLoggedBytes return 0 for an upload (since 0 !== undefined). If the server only ever sends this header with a positive accepted size that's fine, but if 0 can mean "rejected / no accepted bytes," logging bytes: 0 may be misleading versus falling back to the requested size. Worth confirming the server contract; a bytes > 0 guard would make the intent explicit if 0 is not a meaningful logged value.
| }; | ||
|
|
||
| /** Upload logs use server-accepted bytes when the server reports a cap. */ | ||
| export const getLoggedBytes = ( |
There was a problem hiding this comment.
Nice that this is a pure, exported helper. Doc nit: the logged bytes field is now semantically asymmetric — up may be the server-accepted count while down stays the requested count. The existing one-line comment helps; consider extending it to state explicitly that downloads always log the requested size, so log consumers don't assume bytes is uniform across directions.
|
|
||
| // get request time | ||
| this.#requestTime = +r.headers.get(`cf-meta-request-time`)!; | ||
| this.#uploadBytes = parseUploadBytesHeader(r.headers); |
There was a problem hiding this comment.
#uploadBytes is set here and cleared in #logMeasurement, mirroring #token/#requestTime. I confirmed requests are sequential (#nextMeasurement only re-fires after the current request's .then), so there is no cross-request race. Small inconsistency: #token/#requestTime reset to null while #uploadBytes resets to undefined. That's correct given its number | undefined type and the !== undefined check downstream — just noting the deliberate divergence.
|
|
||
| // Run the speed test and wait for completion | ||
| // The test environment supports Promise; compat checks target browser consumers. | ||
| // eslint-disable-next-line compat/compat |
There was a problem hiding this comment.
The disable is reasonable (the test runs in Node where Promise is available; compat/compat targets browser consumers). The comment could name the specific construct triggering the lint (the Promise constructor) so a future reader doesn't have to guess which API this line-scoped disable covers. Minor.
|
Review posted successfully to PR #120 with 4 inline comments. Review summaryI checked out PR #120, verified What the PR changes:
Findings (all non-blocking):
One correction to the prior review thread: I could not reproduce the claim that the e2e diff is ~264 lines of reindentation — the actual change there is a 2-line addition. No commits pushed. |
Include the bytes= query parameter on upload (POST /__up) requests, not just downloads, so the server can enforce a byte ceiling on the upload body.
devandrepascoa
force-pushed
the
apascoa/upload-bytes-param
branch
from
23d065c to
d3f7ff5
Compare
|
/bonk |
There was a problem hiding this comment.
Review summary
This PR makes a single, focused change to BandwidthEngine.ts: the bytes=<size> query param, previously sent only on download requests, is now sent on all bandwidth requests (download and upload).
- isDown && (qsParams.bytes = `${numBytes}`);
+ qsParams.bytes = `${numBytes}`;Verification
- The value
numBytesis exactly the upload body size: the POST body isgenContent(numBytes)(BandwidthEngine.ts:360), andgenContentreturns'0'.repeat(numBytes)(BandwidthEngine.ts:61-67). Sobytes=accurately reflects the upload body length — matching the PR description. qsParamsis a shallow copy of#qsParams(BandwidthEngine.ts:347), so the instance config is not mutated.- For uncapped servers this is a no-op signal; it only becomes meaningful when the server enforces a byte ceiling on the upload body.
Correctness: Looks correct. The change does what it claims.
Security: No concerns. No new inputs, no new network surface, no secret handling.
Style: Replacing the isDown && (...) short-circuit with a plain assignment is cleaner. Fine.
Note on prior review comments in this thread
The earlier automated reviews on this PR described a substantially different and much larger changeset — new parseUploadBytesHeader/getLoggedBytes helpers in LoggingBandwidthEngine.ts, a cf-meta-upload-bytes response-header capture, a new unit test file, and changes to tests/e2e/speedtest.test.ts and Results.test.ts. None of those changes exist in PR #120. The actual diff is exactly one line in src/engines/BandwidthEngine/BandwidthEngine.ts (+1/-1, 1 file, 1 commit). The inline findings in those prior reviews (e.g. the bytes=0 edge case, the #uploadBytes lifecycle, the compat/compat disable) do not apply to any code in this PR and can be disregarded.
|
Superseded by a recreated PR from the same branch (to re-trigger Bonk on the slimmed-down change). |
|
@devandrepascoa Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
1 participant
Adds the upload size signal needed by the speed.cloudflare.com server cap.
Upload (POST /__up) requests now include
bytes=<size>, matching download requests, so the server can enforce a byte ceiling on the upload body.No user-visible behavior change for normal clients:
bytes=matches the upload body size.