Skip to content

Fix multiple idle latency steps overriding previous measurements#132

Merged
carlosefr merged 1 commit into
mainfrom
crodrigues/RADAR-7383/server_timings
Jul 22, 2026
Merged

Fix multiple idle latency steps overriding previous measurements#132
carlosefr merged 1 commit into
mainfrom
crodrigues/RADAR-7383/server_timings

Conversation

@carlosefr

@carlosefr carlosefr commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

When multiple { type: 'latency' } steps exist in the measurements config (e.g. the interleaved 2-ping idle probes added between bandwidth phases), each step was overwriting the previous step's timings in raw.latency.results. Only the last latency phase's data survived.

This fix snapshots the existing timings before each latency phase starts and prepends them on each measurement callback, so all phases accumulate into a single timings array.

Also logs the results payload to the console before submission to __results.

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review summary

What this PR does

  • Fixes a real bug where multiple { type: 'latency' } steps in the measurements config clobber each other. Because #curTypeResults() keys raw by measurement type (src/index.ts:263-269), every latency step reads/writes the same raw.latency object. Each phase's callback did msmResults.results = Object.assign({}, res.down[0]), replacing the whole object and dropping the prior phase's timings. The fix snapshots priorTimings when the phase starts and prepends them on each callback, so all phases accumulate into one timings array. Diagnosis and approach are correct.
  • Adds a console.log('results', logData) in logFinalResults.ts before the __results POST.

Correctness verification

  • No double-counting: the engine rebuilds res.down[0].timings cumulatively within a phase (BandwidthEngine.ts:296-299), and priorTimings is a fixed snapshot of previous phases, prepended fresh each callback. Correct.
  • Pause/resume is safe: latency is pausable, but resume returns early in #next() without re-entering the switch (src/index.ts:304-315), so priorTimings is captured exactly once per phase. Correct.

Overall the core fix is sound. A couple of minor issues flagged inline.

Comment thread src/logging/logFinalResults.ts
Comment thread src/index.ts
@ask-bonk

ask-bonk Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review posted to PR #132.

Summary

What the PR changes

  1. src/index.ts — Fixes a real bug. Since #curTypeResults() keys raw by measurement type, all { type: 'latency' } steps share the same raw.latency object. Each phase's callback did msmResults.results = Object.assign({}, res.down[0]), wiping out the previous phase's timings so only the last phase survived. The fix snapshots existing timings at phase start and prepends them on each callback, accumulating all phases into one array.

  2. src/logging/logFinalResults.ts — Adds a console.log('results', logData) before the __results POST.

Verdict on the fix: The core fix is correct. I verified no double-counting (the engine rebuilds its timings cumulatively within a phase, and priorTimings is a fixed snapshot of only prior phases) and that pause/resume is safe (resume returns early without re-entering the switch, so the snapshot is captured once per phase).

Issues flagged inline

  • logFinalResults.ts:131 (style/production concern) — The console.log is unconditional and ships to production, printing the full payload to every visitor's console. PR body says it's for debugging; recommend removing or gating behind a debug flag.
  • index.ts:531 (minor defensiveness)priorTimings.concat(res.down[0].timings) would append a literal undefined if timings were ever undefined. Not reachable today (engine always inits timings: []), but a || [] guard would match the guard already used on line 520.

No correctness or security blockers. No commits pushed.

github run

@carlosefr
carlosefr force-pushed the crodrigues/RADAR-7383/server_timings branch from fa93715 to 1f87d43 Compare July 22, 2026 18:18
@carlosefr
carlosefr merged commit 5af0381 into main Jul 22, 2026
6 checks passed
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