Skip to content

fix(fs): fsPromises.watch() delivers post-pull writes (#5099 regression)#5117

Merged
proggeramlug merged 1 commit into
mainfrom
fix/fspromises-watch-postpull
Jun 14, 2026
Merged

fix(fs): fsPromises.watch() delivers post-pull writes (#5099 regression)#5117
proggeramlug merged 1 commit into
mainfrom
fix/fspromises-watch-postpull

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Problem

PR #5099 changed fsPromises.watch() to seed its snapshot baseline at creation time and to keep that creation-time baseline at the first .next() pull (removing the re-snapshot in start_promise_watcher). The stated intent was to deliver events emitted between watch() and the first .next().

That broke post-pull delivery. With the baseline frozen at the empty creation-time state, the first poll after the first .next() diffs that empty baseline against the post-early-write directory and resolves the first pending pull with the pre-iteration write. fs/watch/promises-lazy-start.ts reuses that same pending promise for the next race, so the later write (late.txt) is never delivered to it — and the pre-iteration write is wrongly surfaced. Both pre-pull ignored and post-pull delivered assertions then fail against Node.

Root cause

start_promise_watcher (in crates/perry-runtime/src/fs/dir_glob_watch.rs) no longer refreshed state.snapshot at the first pull, so the per-poll diff started from stale creation-time state. The per-poll advance (state.snapshot = current) was fine on its own, but the initial baseline was wrong.

Fix

Re-take the baseline snapshot in start_promise_watcher at the first .next() (folding the current directory state into the baseline so pre-iteration writes are ignored — matching Node, whose async iterator only starts collecting once you iterate), then let promise_watcher_poll_impl keep advancing the baseline after every poll (so a write after a pull begins is detected as a fresh change and delivered).

The creation-time snapshot_watch_target call is kept solely as the synchronous path-validation / error path (Node throws ENOENT etc. at watch() call time). #5099 is not reverted wholesale.

Validation (node v26)

  • fs/watch/promises-lazy-start.ts: deterministically prints pre-pull ignored: true / post-pull delivered: true (3/3).
  • fs/watch/promises-watch.ts: still PASS.
  • Isolated repro: a write after a pull begins is delivered to that pull.
  • cargo test -p perry-runtime -p perry-stdlib: no failures attributable to this change (touches only fs/dir_glob_watch.rs).

Summary by CodeRabbit

  • Bug Fixes

    • Fixed file system watch behavior to refresh snapshots at the correct time, ensuring accurate change detection for files modified during watch operations.
  • Documentation

    • Updated documentation describing file system watch delivery semantics and Node.js-compatible behavior.

… post-pull writes are delivered (#5099 regression)

#5099 seeded the promises-watcher snapshot at watch() creation time AND
stopped re-snapshotting at the first .next() pull (in
start_promise_watcher), intending to deliver writes emitted between
watch() and the first iteration.

That broke post-pull delivery. With the baseline frozen at the empty
creation-time state, the first poll after the first .next() diffs that
empty baseline against the post-early-write directory and resolves the
*first* pending pull with the pre-iteration write. The promises-lazy-start
test reuses that same pending promise for the next race, so the later
write (late.txt) is never delivered to it — and the pre-iteration write
is wrongly surfaced. Result: both "pre-pull ignored" and "post-pull
delivered" assertions fail against Node.

Fix: re-take the baseline snapshot in start_promise_watcher at the first
.next() (folding the current directory state into the baseline, so
pre-iteration writes are ignored — matching Node, whose async iterator
only starts collecting once you iterate), then let promise_watcher_poll_impl
keep advancing the baseline after every poll (so a write *after* a pull
is detected as a fresh change and delivered). The creation-time
snapshot_watch_target call is kept solely as the synchronous path
validation / error path (Node throws ENOENT etc. at watch() call time).

Validation (node v26):
- fs/watch/promises-lazy-start.ts: deterministically prints
  "pre-pull ignored: true" / "post-pull delivered: true" (3/3).
- fs/watch/promises-watch.ts: still PASS.
- Isolated repro: a write after a pull begins is delivered to that pull.
@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d1b77c24-0fef-4001-8ec2-2ea693650280

📥 Commits

Reviewing files that changed from the base of the PR and between c441293 and dea48e7.

📒 Files selected for processing (1)
  • crates/perry-runtime/src/fs/dir_glob_watch.rs

📝 Walkthrough

Walkthrough

start_promise_watcher now refreshes state.snapshot by re-calling snapshot_watch_target at the moment the first .next() pull begins, replacing the creation-time snapshot. Comments in both start_promise_watcher and the js_fs_promises_watch initializer are updated to describe this re-baselining contract and its event-delivery semantics.

Changes

Snapshot re-baselining on first async iterator pull

Layer / File(s) Summary
Re-baseline snapshot at first pull + aligned docs
crates/perry-runtime/src/fs/dir_glob_watch.rs
start_promise_watcher overwrites state.snapshot via snapshot_watch_target at the first .next() call; js_fs_promises_watch initialization comment updated to describe the creation-time snapshot as a sync-validation placeholder that the first pull refreshes, with subsequent polls advancing from the refreshed baseline.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 Hop, hop, I take a fresh look,
Not the old snapshot from when the watch was booked!
First pull refreshes the baseline anew,
Writes before .next() get their events too.
The warren is tidy, the semantics are clear —
Re-baseline at pull time, nothing to fear! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main fix: addressing a regression in fsPromises.watch() that broke post-pull write delivery, with reference to the related regression issue #5099.
Description check ✅ Passed The PR description comprehensively covers problem statement, root cause analysis, the implemented fix, and validation results. However, it does not include a test plan checklist or confirmation of adherence to contribution guidelines.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/fspromises-watch-postpull

Comment @coderabbitai help to get the list of available commands and usage tips.

@proggeramlug proggeramlug merged commit 15da197 into main Jun 14, 2026
15 checks passed
@proggeramlug proggeramlug deleted the fix/fspromises-watch-postpull branch June 14, 2026 08:02
proggeramlug pushed a commit that referenced this pull request Jun 14, 2026
Rolls up the issue-fix batch merged on top of 0.5.1165 (#5102, #5103,
#5105, #5106, #5107, #5108, #5109, #5110, #5112, #5117). See CHANGELOG
for the per-PR breakdown.
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.

1 participant