Skip to content

fix(cli): settle file events before comparing contents - #1905

Merged
ChiragAgg5k merged 1 commit into
mainfrom
cli-watcher-debounce
Sep 12, 2026
Merged

ChiragAgg5k merged 1 commit into
mainfrom
cli-watcher-debounce

Conversation

@ChiragAgg5k

@ChiragAgg5k ChiragAgg5k commented Sep 12, 2026

Copy link
Copy Markdown
Member

appwrite run reloads a function when a watched file's contents change. This fixes two ways that goes wrong, and the second only exists because of the first.

What breaks

An editor saving a file truncates it and writes the bytes back. Those are two filesystem operations, so between them the file is empty on disk. The watcher fingerprints on every notification, so it sees the empty file, decides the contents changed, and reloads — then sees the restored bytes, decides they changed again, and reloads a second time. Saving a file you did not edit rebuilds your function, twice.

Debouncing fixes that: wait for the notifications to stop, then fingerprint once. But a single timer reset by every event never fires while events keep arriving. A build tool, a test runner, or a dependency install writing into the same tree resets the timer every few milliseconds, and the edit the user is actually waiting on never reaches the reload queue at all — it sits in the pending map, along with everything else, for as long as the noise lasts. Trading spurious reloads for no reloads is not a fix.

So the wait is per path rather than shared. Each path settles on its own clock, and each carries its own cap of one second from its first event, so a tree that never falls quiet still reports within a bounded time. Sharing one deadline would have the same shape of bug in miniature: a file first touched late in a busy stretch gets only whatever milliseconds are left on it, and is fingerprinted mid-save — the spurious reload this exists to prevent.

Reproduction

Three watcher variants — main, the debounce without the cap, and this PR — driven through the public Start API by the same harness. Two scenarios: saving an unchanged file ten times (every reload is one the user did not ask for), and editing a source file while a background writer rewrites a log every 5ms (the edit has to arrive).

macOS 15 (kqueue), Go 1.25

variant 10 unchanged saves, 5ms truncate gap 10 unchanged saves, no gap edit reported while busy
main 20 reloads 18 reloads after 3ms
debounce, no cap 0 reloads 0 reloads never (>5s)
this PR 0 reloads 0 reloads after 1.001s

Linux (inotify), golang:1.25

variant 10 unchanged saves, 5ms truncate gap 10 unchanged saves, no gap edit reported while busy
main 20 reloads 10 reloads after 1ms
debounce, no cap 0 reloads 0 reloads never (>5s)
this PR 0 reloads 0 reloads after 1.002s

Ten unchanged saves cost ten to twenty rebuilds today, depending on whether the kernel coalesces the truncate and the write into one notification or two. Both platforms starve identically without the cap, and both land on the one-second bound with it.

What the cap costs

Almost nothing, because the clock is per path. An edit is reported once its own events stop, whatever the rest of the tree is doing. Over 25 edits made at random points during continuous background writing:

min median p90 max
101ms 101ms 102ms 104ms

That is the 100ms settle and nothing more — the same latency an edit in a completely quiet tree pays. The one-second cap only binds for a file that is itself being written continuously, which is a log, not a source file.

An earlier revision of this PR shared one deadline across all pending paths. It fixed the starvation but charged every edit for the tree's noise: the same measurement gave a median of 575ms and a p90 of 878ms, spread uniformly across the cap window. Per-path settling is what makes the cap cheap.

Changes

  • Debounce events for 100ms and fingerprint once the path settles, so a truncate-and-rewrite is one decision rather than two.
  • Settle each path on its own clock, with its own one-second cap, so continuous activity delays a reload instead of cancelling it and a save begun mid-stretch still gets a full quiet interval.
  • Close drains the watcher instead of only signalling it, so no callback runs against a queue the caller has torn down. It stays idempotent.

Tests

Three tests drive the watcher through Start rather than its internals, so a behaviour-preserving change to how the debounce is implemented cannot break them:

  • a truncate-and-rewrite is not reported, while a file the user genuinely emptied is — fails on main
  • an edit is reported while the rest of the tree stays busy
  • a file that is itself written continuously is still reported, since it never falls quiet — fails without the cap
  • repeated unchanged saves during background activity are never reported — fails on main, and fails with one shared deadline rather than per-path ones
  • Close does not return while a report is still in flight — fails without the drain

Each was run against the version lacking its fix to confirm it fails there, rather than assumed to cover it. The saves in the busy test are spaced so they drift against any batching instead of landing at the same point in it each time, because a save is only ignored if it is ignored whenever it happens.

Verified with gofmt and go vet, and the suite run repeatedly under -race on macOS and on Linux (golang:1.25).

This was previously carried on the renovate deps branch (#1904); it belongs in its own PR.

@greptile-apps

greptile-apps Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with the previously reported debounce, shutdown, and test-coverage concerns no longer outstanding.

Summary

  • Coalesces truncate-and-rewrite event sequences by path.
  • Applies independent quiet intervals and deadlines to prevent cross-path starvation.
  • Makes Close idempotent and waits for the watcher loop to finish.
  • Adds public-API behavioral coverage for unchanged saves, busy trees, continuously written files, and shutdown synchronization.

Reviews (4) · Last reviewed commit: "fix(cli): settle file events before comp..."

Comment thread templates/cli/internal/watch/watch.go Outdated
Comment thread templates/cli/internal/watch/watch.go
@ChiragAgg5k

Copy link
Copy Markdown
Member Author

@greptile review

@ChiragAgg5k

Copy link
Copy Markdown
Member Author

@greptile review

Comment thread templates/cli/internal/watch/watch_test.go Outdated
An editor saving a file truncates it and writes the bytes back, so a
notification can arrive while the file is briefly empty and the watcher reports
a reload the user never asked for. Events are debounced and fingerprinted once
the path settles.

Each path settles on its own clock: one shared deadline would hand a file first
touched late in a busy stretch only the milliseconds left on it. A per-path cap
of a second keeps a file that never falls quiet from being deferred forever.

Close drains the watcher rather than only signalling it, so no callback runs
against a queue the caller has already torn down.
@ChiragAgg5k

Copy link
Copy Markdown
Member Author

@greptile review

@ChiragAgg5k
ChiragAgg5k merged commit dc832d1 into main Sep 12, 2026
59 checks passed
@ChiragAgg5k
ChiragAgg5k deleted the cli-watcher-debounce branch September 12, 2026 18:58
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