Skip to content

feat: report server-accepted upload bytes in results#121

Draft
devandrepascoa wants to merge 2 commits into
mainfrom
apascoa/report-accepted-upload-bytes
Draft

feat: report server-accepted upload bytes in results#121
devandrepascoa wants to merge 2 commits into
mainfrom
apascoa/report-accepted-upload-bytes

Conversation

@devandrepascoa
Copy link
Copy Markdown
Contributor

Captures the cf-meta-upload-bytes response header on upload requests and surfaces the server-accepted upload size through the measurement results.

  • LoggingBandwidthEngine reads cf-meta-upload-bytes on every response (independent of per-measurement logging) and attaches it to the upload measurement result.
  • The accepted size is threaded through Results / getBandwidthPoints.
  • The final results payload now reports the server-accepted upload size per point, falling back to the client-requested size when the header is absent. Downloads are unaffected.

Depends conceptually on the server returning cf-meta-upload-bytes; works regardless of whether the client sends bytes= on uploads (see #120).

Capture the cf-meta-upload-bytes response header on upload requests and
surface it through the bandwidth measurement results. The final results
payload now reports the server-accepted upload size per point instead of
the client-requested size, falling back to the requested size when the
header is absent. Downloads are unaffected.
Copy link
Copy Markdown

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR captures the cf-meta-upload-bytes response header on upload requests and surfaces the server-accepted upload size through the measurement results.

Changes:

  • LoggingBandwidthEngine gains two exported helpers, parseUploadBytesHeader (validates and parses the header into a safe integer) and getLoggedBytes (chooses accepted vs. requested bytes for logging). The response hook now records #uploadBytes on every response, independent of per-measurement logging, and #applyUploadBytes attaches it to upload measurement results.
  • BandwidthMeasurementTiming / BandwidthTiming / BandwidthPoint gain an optional uploadBytes field, threaded through getBandwidthPoints.
  • The final AIM results payload (bpsPointsParser) reports the server-accepted upload size per point, falling back to the requested size when the header is absent. Downloads are unaffected.
  • Adds unit tests for parseUploadBytesHeader / getLoggedBytes and a getBandwidthPoints test.

Verification: pnpm test:unit (99 passed) and pnpm lint both pass locally on the PR head.

The implementation is clean and well-tested. The shared-mutable-state approach (#uploadBytes on the instance, read in a deferred setTimeout) is safe in practice here because bandwidth requests in this engine are strictly sequential and the setTimeout(0) result callback fires long before the next request's network response. I left a few inline notes — the main one worth discussing is the bytes/bps consistency in the final payload.

const bpsPointsParser: ParserFn = pnts =>
(pnts as BandwidthPoint[]).map(d => ({
bytes: +d.bytes,
bytes: d.uploadBytes ?? +d.bytes,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness / semantics: bytes and bps become internally inconsistent.

Here bytes is reported as the server-accepted size (uploadBytes), but bps (from d.bps) was computed by calcUploadSpeed in BandwidthEngine.ts using the client-requested numBytes, not the accepted size. When the server caps the upload (the whole motivation for cf-meta-upload-bytes), the emitted pair no longer reconciles: a consumer estimating bps ≈ bytes * 8 / duration would get a different number than the reported bps.

Worth confirming this is intended. If the goal is accurate accepted-size throughput, bps should probably be recomputed from uploadBytes; otherwise consider keeping bps derived from the same bytes value you report.

#loggingResponseHook(r: ResponseHookPayload): void {
// Capture server-accepted upload bytes regardless of per-measurement
// logging, so the final results payload can report actual uploaded sizes.
this.#uploadBytes = parseUploadBytesHeader(r.headers);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shared mutable state via #uploadBytes — fragile, please add a guarding comment or assertion.

#uploadBytes is set here synchronously in the response hook, but it is read later in #applyUploadBytes / #logMeasurement, which run inside the setTimeout(...)-deferred onMeasurementResult callback (see BandwidthEngine.#saveMeasurementResults). This is correct today only because bandwidth requests in this engine are strictly sequential and the setTimeout(0) result callback fires well before the next request's network response can overwrite #uploadBytes.

This is an implicit invariant. If request pipelining/concurrency is ever added to BandwidthEngine, the value captured here could be clobbered by a later request before the deferred callback reads it, silently attaching the wrong uploadBytes to a measurement. Consider attaching the parsed value to the per-request flow (e.g. carry it through the timing object) instead of an instance field, or at minimum document this ordering dependency.

headers: Headers
): number | undefined => {
const value = headers.get('cf-meta-upload-bytes');
if (!value || !/^\d+$/.test(value)) return undefined;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: parseUploadBytesHeader rejects 0 is fine, but note that Number.isSafeInteger already returns false for values exceeding 2^53-1, so the /^\d+$/ regex is the only guard against arbitrarily large strings — good. One edge case to confirm: a value of '0' parses to 0, which is falsy, so getLoggedBytes/#applyUploadBytes (uploadBytes !== undefined) would still treat an accepted size of 0 as valid and report bytes: 0. If the server can legitimately send 0, that's correct; if 0 should mean "no cap", this would mis-report. Worth a quick confirmation.

serverTime,
transferSize
transferSize,
uploadBytes
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor/style: uploadBytes is now spread onto every bandwidth point, including download points where it is always undefined. That's harmless (optional field), but it does add an explicit uploadBytes: undefined key to download point objects. If you want download points to stay shape-identical to before, you could conditionally include it. Non-blocking.

@ask-bonk
Copy link
Copy Markdown

ask-bonk Bot commented Jun 5, 2026

Review posted to PR #121. Here's a summary of my review.

What the PR changes

It captures the cf-meta-upload-bytes response header on upload requests and surfaces the server-accepted upload size in the results:

  • LoggingBandwidthEngine — adds two exported helpers (parseUploadBytesHeader, getLoggedBytes), records #uploadBytes from the response hook on every response, and attaches it to upload measurement results via #applyUploadBytes.
  • types.ts / BandwidthEngine.ts — adds optional uploadBytes to the timing/point interfaces.
  • MeasurementCalculations.getBandwidthPoints — threads uploadBytes through each point.
  • logFinalResults.ts — the final AIM payload now reports the accepted size, falling back to requested size when the header is absent. Downloads unaffected.
  • Adds unit tests; pnpm test:unit (99 passed) and pnpm lint pass on the head.

Issues flagged (inline)

  1. Correctness/semantics (main one)logFinalResults.ts:60: the reported bytes becomes the accepted size, but bps is still computed from the client-requested bytes. When the server caps uploads, the bytes/bps pair no longer reconciles for consumers.
  2. Fragile shared mutable stateLoggingBandwidthEngine.ts:119: #uploadBytes is set synchronously in the response hook but read in a setTimeout-deferred callback. Correct today only because requests are strictly sequential; would break under any future request concurrency/pipelining. Suggested carrying the value through the per-request timing object or documenting the invariant.
  3. Edge caseparseUploadBytesHeader: a header value of '0' is treated as a valid accepted size (0 !== undefined), so it would report bytes: 0. Fine if the server can legitimately send 0, but worth confirming it doesn't mean "no cap".
  4. Minor stylegetBandwidthPoints now spreads uploadBytes: undefined onto download points too; harmless but changes their shape slightly.

No security issues found. The change is well-tested and otherwise clean. No commits pushed.

github run

Adds a browser e2e (real SpeedTest engine + local mock) proving the
/__log and /__results payloads report cf-meta-upload-bytes, not the
upload Content-Length. The mock forces the accepted size below the body
size so the assertion can only pass if the client uses the header; a
second case covers the fallback when the header is absent.

Also disables the browser-compat lint rule for tests/, which run in Node
and headless Chromium rather than the library's target browsers.
...CORS,
'content-type': 'application/octet-stream'
});
res.end(Buffer.alloc(size, 0x30));
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants