Repository navigation
Stop the log from inventing paths when the tree moves - #2
Merged
Merged
Conversation
These five scenarios all fail today. inotify is not recursive and its watches are bound to the inode rather than the path, so every directory move is a place where the watcher silently stops matching reality: - a directory moved into the tree is never walked, so it and everything written inside it stay invisible for the life of the process - a renamed directory keeps its cached path, so writes are attributed to a path that no longer exists - a directory moved out of the tree keeps its watch, so writes outside the mandate are reported under the vacated in-tree path That last one is the reason these are worth a commit of their own: the log does not go quiet, it starts inventing paths, and a wrong path is never noticed the way a missing line is. The remaining two cover a write whose path is longer than the log line buffer (dropped with no diagnostic) and a watcher that cannot see its root (blocks forever on an empty inotify fd, so systemd reports it as active and Restart=on-failure never fires). Every watcher runs under `timeout` — without it the rootless case would hang CI instead of failing it, which is the same failure this is about. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two failures that both presented as silence. The event line was formatted into a 1200-byte buffer while paths were accepted up to MAX_PATH (4096). Past roughly 1.15 KB of path the bufPrint failed and the `catch return` swallowed the event whole — no line, no diagnostic, nothing to count. Both line buffers are now derived from MAX_PATH, with a unit test guarding the arithmetic so raising MAX_PATH cannot quietly reintroduce the gap. The unformattable case keeps a LINE-OVERFLOW line instead of returning, because an event we cannot render in full is still an event worth admitting to. The second is worse in production: a watcher that could not watch its root logged `START ... (0 dirs)` and then blocked forever on an empty inotify fd. It never exits, so Restart=on-failure never fires and systemd keeps reporting active (running) while nothing is observed. The same state is reachable at runtime when the root is deleted and its IGNORED drops the last watch. Both now log the reason and exit non-zero so the supervisor can act. Turns three integration checks green. The directory-move scenarios still fail; they need the watch rebuild and are left for the next commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
inotify watches are bound to the inode, not the path, so a directory move leaves the wd→path map describing a tree that no longer exists. The descriptors that need correcting are precisely the ones whose new location cannot be derived, so there is nothing to patch in place. Every invalidating event now sets a flag that triggers one full re-scan from the root: a directory moved in or out, a directory renamed, a MOVE_SELF on a watch, or a queue overflow that may have swallowed a directory's creation. MOVE_SELF has to join WATCH_MASK for that, so the drift guard and eventStr grew with it. The re-scan runs once per read batch rather than inline. That is not just economy — rebuilding mid-batch would invalidate the descriptors the remaining events in the same buffer still refer to. It also collapses the three events a single rename produces into one re-scan. A fresh inotify instance replaces the old one instead of removing watches individually, which would deliver a burst of IGNORED events into the middle of our own stream. The replacement is opened before the old one is closed, so a failure leaves us degraded rather than blind. When the re-scan comes back empty — the root itself moved away — the existing zero-watch check turns it into a clean exit. Directory creation stays incremental: a new directory is empty by definition, so one watch is enough. Two things followed from the above rather than being goals of their own. Directory events are now named apart from file ones (CREATE-DIR, RENAME-TO-DIR, ...), because a reader has to see which events forced a rebuild; that made RENAME-FROM-DIR the longest tag, so the tag column widened from 12 to 15 to keep the path column aligned. And the size suffix is no longer attached to directory events, where statx was reporting the 4096-byte size of the directory entry itself. Turns the remaining seven integration checks green; all 19 pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Four things that all read as the log being trustworthy when it was not. write(2) may accept fewer bytes than offered and reports ENOSPC through a return value nobody was reading, so a full disk discarded the entire product of this process in silence. Writes now loop to completion, and a failure on the log file is fatal: it is recorded on the context and turned into a non-zero exit by the event loop, since writeLine is called from too deep to unwind. stdout stays best-effort — a closed pipe there is not a reason to bring the watcher down. The hidden-directory filter lived only in the startup scan, which made it a rule about *when* a directory appeared rather than what it is: a .git already present was skipped, while the identical directory created a second later was watched and logged. It now sits in one predicate used by both the scan and the runtime path. inotify_add_watch gains IN_ONLYDIR, closing the race where the entry we had decided was a directory is a file by the time we watch it. It is passed at the call site rather than folded into WATCH_MASK, which is asserted to be exactly the set of bits eventStr can name. Timestamps gain a trailing Z. The value was always UTC — the conversion applies no timezone — but unlabelled it reads as local time, silently two hours off from journalctl on this host and from every other log you would correlate it against. Two smaller silences on the way past: getdents64 now retries EINTR instead of truncating the scan, and a directory we hold a watch on but cannot open is reported as NO-DESCEND rather than returning quietly with its subdirectories left uncovered. The ENOSPC path is the one behaviour here with no automated test — it needs a filesystem of its own — and was verified by hand against a 64K tmpfs: partial write detected mid-line, diagnostic on stderr, exit 1. 22/22 integration checks, 8/8 unit tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ocwatch is deployed on aarch64 and CI only ever built for x86-64, so the architecture it actually runs on was never tested. The repository is public, which makes GitHub's arm64 runners available, so both architectures now run the full suite rather than cross-compiling for the target and calling that coverage. Nearly everything that can break here — the wd map, the rebuild after a directory move, the exit paths — only appears against a live kernel, which a cross-build never touches. fail-fast is off so one architecture failing does not hide the other. Log rotation is documented rather than implemented: ocwatch has no business rotating its own log, but it does constrain how, and that constraint is easy to get wrong. It holds a single O_APPEND descriptor for the life of the process with no way to be told to reopen it, so rotating by rename would leave it writing into an unlinked inode — the new log stays empty and the trail stops with nothing to indicate it. The config therefore requires copytruncate, which O_APPEND in turn makes safe: writes re-seek to the end, so the file resumes at offset 0 rather than returning as a sparse hole. Sizing comes from 120 days of production data — about 77 KB/day, compressing to roughly a tenth — not from a guess. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The workflow only triggered on pushes to main and on pull requests, so a branch had no CI at all until somebody opened a PR — which is exactly when the feedback is least useful. Pushes to any branch now build. The pull_request trigger stays, since pushes from forks do not run here. A branch with an open PR consequently builds twice; at under a minute a run that is worth paying. Superseded runs are cancelled, except on main where every commit's result is worth keeping. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A review of this repo turned up four P0 defects. Each was reproduced against a
running binary before anything was changed, and each now has an integration
test that failed first.
The one that matters
inotify watches are bound to the inode, not the path. A directory moved out of
the watch tree therefore keeps its watch, and ocwatch went on reporting writes
that happened outside its mandate — under the in-tree path it used to have:
A renamed directory produced the same class of failure from the other side:
writes attributed to the pre-rename path forever. For a tool whose entire job
is a trustworthy audit trail, this is worse than going quiet. A gap shows up
when you count lines; a wrong path never does.
Directories moved into the tree were simply never walked — they and
everything written inside them stayed invisible for the life of the process.
The rest
formatted into 1200 bytes while paths were accepted up to
MAX_PATH(4096).Past ~1.15 KB the
bufPrintfailed and a barecatch returnswallowed theevent whole.
START ... (0 dirs)and then waited on an empty inotify fd — never exiting,so
Restart=on-failurenever fired while systemd reportedactive (running).Reachable at runtime too, by deleting the root.
write(2)errors and short writes were ignored, soENOSPCdiscarded theentire product of the process without a word.
rule about when a directory appeared rather than what it is: a
.gitpresent at startup was skipped, the identical one created a second later was
watched.
Approach
Anything that invalidates the wd→path map — a directory moved in, out or
renamed,
MOVE_SELF, or a queue overflow that may have hidden a directory'screation — triggers one full re-scan from the root, logged as
REBUILD.The re-scan runs once per read batch rather than inline. That is not economy:
rebuilding mid-batch would invalidate the descriptors the remaining events in
the same buffer still refer to. It also collapses the three events a single
rename raises into one re-scan. A fresh inotify instance replaces the old one
rather than removing watches individually, which would deliver a burst of
IGNOREDinto the middle of our own stream; the replacement is opened beforethe old one is closed, so a failure leaves the watcher degraded rather than
blind.
Directory creation stays incremental — a new directory is empty by definition.
Where ocwatch would otherwise continue in a state where it observes nothing, it
now exits non-zero and lets the supervisor act: no watchable root, the last
watch gone, or the log file refusing writes.
Also here
Z. The value was always UTC, but unlabelled itread as local time — silently hours off from
journalctlon any host that isnot UTC.
IN_ONLYDIR,EINTRretried ongetdents64, and a directory we watch butcannot open reported as
NO-DESCENDinstead of returning quietly.aarch64 and was never tested there; a cross-build would not have exercised
any of the kernel-dependent behaviour above. Every branch builds now, not
only
main.copytruncateis mandatory ratherthan a preference: ocwatch holds one
O_APPENDdescriptor for its lifetimeand cannot be told to reopen it.
Tests
22 integration checks and 8 unit tests, stable across repeated runs. The five
scenarios covering tree mutations were committed first, red, in d1c2bd5.
The
ENOSPCpath is the one behaviour with no automated test — it needs afilesystem of its own — and was verified by hand against a 64K tmpfs: partial
write detected mid-line, diagnostic on stderr, exit 1.
🤖 Generated with Claude Code