Skip to content

fix(mock): do not persist snapshots on close in playback mode#5359

Merged
metcoder95 merged 1 commit into
nodejs:mainfrom
GeoffreyBooth:fix/snapshot-no-save-in-playback
Jun 5, 2026
Merged

fix(mock): do not persist snapshots on close in playback mode#5359
metcoder95 merged 1 commit into
nodejs:mainfrom
GeoffreyBooth:fix/snapshot-no-save-in-playback

Conversation

@GeoffreyBooth

Copy link
Copy Markdown
Member

Problem

SnapshotAgent.close() unconditionally delegates to the recorder's close(), which saves snapshots to disk whenever any are held — with no regard for the current mode:

// lib/mock/snapshot-recorder.js
async close () {
  if (this.#snapshotPath && this.#snapshots.size !== 0) {
    await this.saveSnapshots()
  }
  this.destroy()
}

In playback mode this is a problem because findSnapshot() mutates each matched snapshot on every call:

// lib/mock/snapshot-recorder.js — findSnapshot()
const currentCallCount = snapshot.callCount || 0
const responseIndex = Math.min(currentCallCount, snapshot.responses.length - 1)
snapshot.callCount = currentCallCount + 1

And callCount is serialized by saveSnapshots(). So the chain is:

playback run → every matched request bumps callCount in memory → close() flushes unconditionally → the snapshot file is rewritten on disk.

The result is spurious diffs in committed snapshot fixtures after a plain playback run (no recording involved). For projects that store fixtures in git (e.g. via Git LFS), every test run produces a new file hash and noisy, meaningless diffs. The persisted callCount is also stale state — loadSnapshots() does not reset it — so it has no business being written back during playback.

Fix

Gate persistence at SnapshotAgent.close(), which is the only layer that knows the mode. Record/update modes still save via recorder.close(); playback only cleans up timers via recorder.destroy():

async close () {
  if (this[kSnapshotMode] === 'playback') {
    this[kSnapshotRecorder].destroy()
  } else {
    await this[kSnapshotRecorder].close()
  }
  await this[kRealAgent]?.close()
  await super.close()
}

The public saveSnapshots() is intentionally left untouched, so an explicit save still works in any mode — only the automatic save-on-close is suppressed during playback.

Test

Added a regression test in the existing Close Method block (test/snapshot-testing.js): record a fixture, capture its exact on-disk bytes, run a playback session with several matched requests (mutating callCount in memory), close(), and assert the file is byte-for-byte unchanged. It fails on main and passes with this change.

Note / possible follow-up

The opt-in auto-flush timer (autoFlush: true) also calls saveSnapshots() and is likewise not mode-gated, so it could still rewrite a fixture during a long-running playback session. That's a separate, opt-in path (default off) and out of scope here, but worth flagging — happy to address it in a follow-up if maintainers prefer.

SnapshotAgent.close() unconditionally delegated to the recorder's
close(), which saves to disk whenever any snapshots are held,
regardless of mode. In playback mode, findSnapshot() mutates each
matched snapshot's callCount on every match, so closing a playback
agent rewrote the snapshot file even though nothing new was recorded.
This produced spurious diffs in committed fixtures (and persisted
callCount values, which loadSnapshots() does not reset).

Gate persistence at close() — the only mode-aware layer: record and
update modes still save via recorder.close(), while playback only
cleans up timers via recorder.destroy(). The public saveSnapshots()
is left untouched, so explicit saves continue to work in any mode.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Geoffrey Booth <webadmin@geoffreybooth.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.23%. Comparing base (9b1d58f) to head (7ba095c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5359   +/-   ##
=======================================
  Coverage   93.23%   93.23%           
=======================================
  Files         110      110           
  Lines       36668    36676    +8     
=======================================
+ Hits        34187    34196    +9     
+ Misses       2481     2480    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@metcoder95 metcoder95 merged commit 2f66db7 into nodejs:main Jun 5, 2026
38 checks passed
@GeoffreyBooth GeoffreyBooth deleted the fix/snapshot-no-save-in-playback branch June 6, 2026 01:17
@github-actions github-actions Bot mentioned this pull request Jun 6, 2026
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.

4 participants