fix(time-profiler): serialize before clearing the source mapper in stop()

[mcollina]

Summary

The legacy time-profiler stop() serializes the profile after tearing down the profiler state, so source maps are silently dropped. CPU profiles taken via start() + stop() with a sourceMapper no longer map transpiled frames (e.g. TypeScript) back to their original sources — they point at the generated .js files instead. This is a regression introduced between 5.14.1 and 5.14.3/5.14.4.

Root cause

In ts/src/time-profiler.ts, stop() runs handleStopNoRestart() (which sets gSourceMapper = undefined and disposes the profiler) before calling serializeTimeProfile(profile, gIntervalMicros, gSourceMapper, …):

const profile = gProfiler.stop(restart);
if (restart) { handleStopRestart(); } else { handleStopNoRestart(); } // clears gSourceMapper
const serializedProfile = serializeTimeProfile(profile, gIntervalMicros, gSourceMapper, …); // undefined!

Because gSourceMapper is undefined at serialization time, serializeTimeProfile never calls sourceMapper.mappingInfo(...), and frames keep their generated-file locations.

stopV2() is unaffected: it serializes inside the stopAndCollect callback, which runs while gSourceMapper is still set. In 5.14.1, stop() itself used the stopAndCollect path, so the mapper was still set at serialization time — hence this is a regression.

Fix

Move serialization ahead of the teardown so the source mapper is still set, matching stopV2() (and the current main branch, which already serializes before clearing). The profile object returned by the native stop() does not depend on the profiler still being alive, so this reordering is safe.

Reproduction

With 5.14.4:

const { time, SourceMapper } = require('@datadog/pprof')
const mapper = await SourceMapper.create([appDir])     // appDir contains dist/foo.js + dist/foo.js.map (-> foo.ts)
time.start({ sourceMapper: mapper, lineNumbers: true, intervalMicros: 1000 })
// ... run transpiled code ...
const profile = time.stop()
// profile functions reference dist/foo.js  ❌  (5.14.1: foo.ts ✅)

sourceMapper.mappingInfo is called 0 times on 5.14.4 vs 26 times on 5.14.1 for the same workload. Using time.stopV2() instead of time.stop() produces the correctly mapped profile, confirming the diagnosis.

Test

Added a deterministic regression test in test-time-profiler.ts (under profile (w/ stubs)): it stubs serializeTimeProfile and asserts stop() passes the source mapper through. It fails on the current code (undefined !== {}) and passes with this fix.

Notes

Issues are disabled on this repository, so I'm opening this as a PR with the fix. Happy to adjust the approach or test as preferred.

[szegedi]

Thanks for finding this!

FWIW, we're switching back to "legacy" stop because with stopV2 we had a larger overlap between two profiling sessions being active which triggered crashes due to a race condition in V8 (see my report and also it was fixed since, but since it's been present for forever, we can't use stopV2 until all non-EOL Node.js versions have the fix.)

Anyhow, I have a nit about this – in the restart case, I still want the profiler to perform the restart early; aside from that, looks good!

[szegedi]

We definitely want to – at least for the restart case – to stop/start the profiler before we serialize, as then the serialization work itself is captured in the next profile; we like to be honest like that and let the users observe its cost :-) Arguably, for the plain stop case it doesn't matter. I guess maybe we'll need to blow up this nice encapsulation of handleStop[No]Restart() here and have an early stop-start for the restart case (which should not dispose of source maps, right?) and a late stop for for no-restart case.

[mcollina]

Should be fixed, lmk