From 46b455e3c2ab742b7842658e0606a111ce691a30 Mon Sep 17 00:00:00 2001 From: edaywalid Date: Sun, 9 Aug 2026 16:21:20 +0100 Subject: [PATCH 01/10] fix(shim): stop recording before the disk is gone The size budget lived in `undo gc`, which only runs from the shell hook's precmd, which only fires when the command returns. A command that runs for a day never gives it a turn, and gc skips live sessions anyway, so the one session that is actively growing is the one nothing was ever going to prune. A single session reached 147G and took the filesystem to zero. Both ceilings move into the shim, where the writes happen: UNDO_MIN_FREE free-space floor, default 2G, checked by statvfs amortised over every 64M of backups UNDO_MAX_SESSION per-session cap, default 1G Either one trips and the session stops recording and writes a `degraded` file saying which and why. The counter is a shared mapping rather than a static, so every process a build forks bills to the same session. UNDO_MAX_BYTES, the per-file cap, now applies to hardlinked backups too. It only ever covered copies, on the reasoning that a link costs nothing. A link is not free: it is exactly what stops the original blocks being returned when the file is unlinked, so deleting a 4G file cost 4G that the per-file cap was supposed to have refused. Stopping early loses undo history. Filling the filesystem takes down everything else on the machine, and a tool that exists to save you from mistakes should not be capable of making that one. --- shim/undo_shim.c | 206 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 201 insertions(+), 5 deletions(-) diff --git a/shim/undo_shim.c b/shim/undo_shim.c index ddaf2a1..1a5fffb 100644 --- a/shim/undo_shim.c +++ b/shim/undo_shim.c @@ -20,7 +20,9 @@ #include #include #include +#include #include +#include #include #include @@ -29,6 +31,11 @@ #endif #define DEFAULT_MAX_BYTES (256UL * 1024 * 1024) +#define DEFAULT_MAX_SESSION (1UL << 30) +#define DEFAULT_MIN_FREE (2UL << 30) + +/* how many bytes of backups may go by between two statvfs calls */ +#define FREE_CHECK_INTERVAL (64UL << 20) static __thread int in_shim; @@ -93,9 +100,17 @@ static void enc_append(char *dst, size_t cap, size_t *len, const char *s) } } +static int recording_stopped(void); + /* jwrite("op", field1, field2, NULL) */ static void jwrite(const char *op, ...) { + /* Once a ceiling has been hit the journal stops growing too. It is + * small next to the backups, but the point of the free-space floor is + * that undo stops touching a filesystem in trouble, and a record of + * changes whose backups were never taken is not worth a byte of it. */ + if (recording_stopped()) + return; int fd = journal_fd(); if (fd < 0) return; @@ -206,6 +221,165 @@ static unsigned long max_bytes(void) return v; } +/* A limit read from the environment, where an explicit 0 means no limit. + * Distinct from max_bytes() above, whose 0 has always meant "unset". */ +static unsigned long limit_env(const char *name, unsigned long def) +{ + const char *s = getenv(name); + if (!s || !*s) + return def; + return parse_ulong(s); +} + +/* ---------- space guards ---------- */ + +/* Two ceilings, both enforced here rather than by the shell hook. The hook + * only gets a turn when the command returns, and the command that fills a + * disk is the one that runs for a day: an editor, a dev server, an agent. + * By the time precmd could call `undo gc` the damage is done, and gc would + * skip the session anyway because it is still live. + * + * Whichever ceiling trips first, the session stops recording and drops a + * `degraded` file saying why. Stopping early loses undo history, which is + * bad. Filling the filesystem takes down everything else on the machine, + * which is worse, and a tool that exists to save you from mistakes has no + * business making that one. + * + * The counter lives in a shared mapping, not a static, so the thousand + * compilers a build forks all bill to the same session budget. */ +struct budget { + uint64_t saved; /* bytes of backups written by every process */ + uint32_t stopped; /* set once, by whoever trips a ceiling first */ +}; + +static struct budget *budget_map(void) +{ + static __thread char cached_dir[PATH_MAX]; + static __thread struct budget *map; + const char *dir = session_dir(); + if (!dir) + return NULL; + if (map && strcmp(cached_dir, dir) == 0) + return map; + if (map) { + munmap(map, sizeof *map); + map = NULL; + } + char path[PATH_MAX]; + if ((size_t)snprintf(path, sizeof path, "%s/budget", dir) >= sizeof path) + return NULL; + REAL(open, int, const char *, int, ...); + int fd = real_open(path, O_RDWR | O_CREAT | O_CLOEXEC, 0600); + if (fd < 0) + return NULL; + /* only grow it: a racing process may already have mapped this page, + * and truncating back to zero would reset a budget mid-session */ + struct stat st; + REAL(ftruncate, int, int, off_t); + if (fstat(fd, &st) != 0 || + (st.st_size < (off_t)sizeof *map && + real_ftruncate(fd, (off_t)sizeof *map) != 0)) { + close(fd); + return NULL; + } + void *p = mmap(NULL, sizeof *map, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); + close(fd); + if (p == MAP_FAILED) + return NULL; + map = p; + snprintf(cached_dir, sizeof cached_dir, "%s", dir); + return map; +} + +/* Records why recording stopped, once per session. Called before the + * filesystem is actually full, so this small write still has room. */ +static void budget_stop(const char *why) +{ + struct budget *b = budget_map(); + if (b && __atomic_exchange_n(&b->stopped, 1, __ATOMIC_RELAXED)) + return; /* someone else already wrote the marker */ + const char *dir = session_dir(); + if (!dir) + return; + char path[PATH_MAX]; + if ((size_t)snprintf(path, sizeof path, "%s/degraded", dir) >= sizeof path) + return; + REAL(open, int, const char *, int, ...); + int fd = real_open(path, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0600); + if (fd < 0) + return; + char msg[512]; + int n = snprintf(msg, sizeof msg, "%s\n", why); + if (n > 0) { + ssize_t w = write(fd, msg, (size_t)n); + (void)w; + } + close(fd); +} + +static int recording_stopped(void) +{ + struct budget *b = budget_map(); + return b && __atomic_load_n(&b->stopped, __ATOMIC_RELAXED); +} + +/* true if `want` more bytes of backup are affordable */ +static int budget_ok(unsigned long want) +{ + struct budget *b = budget_map(); + if (b && __atomic_load_n(&b->stopped, __ATOMIC_RELAXED)) + return 0; + + char why[512]; + unsigned long cap = limit_env("UNDO_MAX_SESSION", DEFAULT_MAX_SESSION); + if (b && cap) { + unsigned long used = __atomic_load_n(&b->saved, __ATOMIC_RELAXED); + if (used + want > cap) { + snprintf(why, sizeof why, + "stopped recording: session reached the %lu MB budget " + "(UNDO_MAX_SESSION)", + (cap + (1UL << 19)) >> 20); + budget_stop(why); + return 0; + } + } + + unsigned long floor = limit_env("UNDO_MIN_FREE", DEFAULT_MIN_FREE); + if (!floor) + return 1; + /* statvfs on every backup would be wasteful and on none would be + * useless; amortise it over the bytes actually written. Starting at + * the interval forces a check before the first backup of a session, + * so a session opened on an already-full disk never gets going. */ + static __thread unsigned long since = FREE_CHECK_INTERVAL; + since += want; + if (since < FREE_CHECK_INTERVAL) + return 1; + since = 0; + const char *dir = session_dir(); + struct statvfs vfs; + if (!dir || statvfs(dir, &vfs) != 0) + return 1; /* cannot tell: let the session budget do the limiting */ + unsigned long avail = (unsigned long)vfs.f_bavail * (unsigned long)vfs.f_frsize; + if (avail < floor + want) { + snprintf(why, sizeof why, + "stopped recording: %lu MB free on the store's filesystem, " + "floor is %lu MB (UNDO_MIN_FREE)", + (avail + (1UL << 19)) >> 20, + (floor + (1UL << 19)) >> 20); + budget_stop(why); + return 0; + } + return 1; +} + +static void budget_add(unsigned long n) +{ + struct budget *b = budget_map(); + if (b) + __atomic_add_fetch(&b->saved, (uint64_t)n, __ATOMIC_RELAXED); +} + static int backup_name(char *out) { static unsigned long counter; @@ -267,11 +441,29 @@ static int copy_file(const char *src, const char *dst) } /* Save `abs` before it is destroyed. When the original inode survives the - * operation untouched (unlink, rename target), a hardlink is enough and - * costs nothing; when data is rewritten in place (O_TRUNC, plain write - * opens), we need a full copy. */ + * operation untouched (unlink, rename target), a hardlink is enough; when + * data is rewritten in place (O_TRUNC, plain write opens), we need a full + * copy. + * + * A hardlink costs no extra inode data, which used to read as "costs + * nothing", so the per-file cap was only ever applied to copies. It is not + * free: the link is what stops the original blocks being returned when the + * file is unlinked, so a deleted 4 GB file is 4 GB the store is holding. + * Both paths are charged the same now. */ static int save_file(const char *abs, int need_copy, char *bak) { + struct stat st; + if (lstat(abs, &st) != 0 || !S_ISREG(st.st_mode)) + return -1; + if ((unsigned long)st.st_size > max_bytes()) { + errno = EFBIG; + return -1; + } + if (!budget_ok((unsigned long)st.st_size)) { + errno = ENOSPC; + return -1; + } + /* Names can collide when a shell execs its last command without * forking (same pid, counter reset); retry with the next counter. */ for (int tries = 0; tries < 1000; tries++) { @@ -279,14 +471,18 @@ static int save_file(const char *abs, int need_copy, char *bak) return -1; if (!need_copy) { REAL(link, int, const char *, const char *); - if (real_link(abs, bak) == 0) + if (real_link(abs, bak) == 0) { + budget_add((unsigned long)st.st_size); return 0; + } if (errno == EEXIST) continue; /* cross-device etc: fall through to a copy under this name */ } - if (copy_file(abs, bak) == 0) + if (copy_file(abs, bak) == 0) { + budget_add((unsigned long)st.st_size); return 0; + } if (errno != EEXIST) break; } From b9fd0d4b4a000d80938793f363fb11c250489f7d Mon Sep 17 00:00:00 2001 From: edaywalid Date: Sun, 9 Aug 2026 16:21:20 +0100 Subject: [PATCH 02/10] fix(shim): ignore the caches that actually churn The default list was node_modules, .cache, __pycache__ and .git. The 147G incident was mostly .turbo/cache and .next, neither of which it covers: seg_match compares whole components, so `.turbo/cache` does not match the `.cache` pattern. These are the highest-churn and lowest-value files in a repo and a dev loop rewrites them hundreds of times an hour. Adds the tool-owned cache directories. Names a person might have picked for their own source stay out of the defaults, even though dist and target and friends hold generated output just as often, because a default that makes `rm -rf dist` unrecoverable is a worse bug than the one being fixed here. They are in examples/ignore instead. The dot-prefixed names go in a second list guarded by one strstr for "/.", so a path with no dot-directory in it still costs four component scans rather than twenty. ignored() runs on every intercepted open. --- examples/ignore | 33 ++++++++++++++++++++++++++------- shim/undo_shim.c | 31 ++++++++++++++++++++++++++++--- 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/examples/ignore b/examples/ignore index 24bdb49..08dbd7c 100644 --- a/examples/ignore +++ b/examples/ignore @@ -6,22 +6,41 @@ # anything under a directory named target, at any depth) # /abs/path matches that absolute path and everything under it # -# The shim ALWAYS ignores these on top of your list, unless you set -# UNDO_DEFAULT_IGNORE=0: node_modules .cache __pycache__ .git +# On top of your list the shim always ignores the tool-owned caches +# below, unless you set UNDO_DEFAULT_IGNORE=0: +# +# node_modules __pycache__ test-results playwright-report +# .git .cache .turbo .next .nuxt .vite .svelte-kit +# .parcel-cache .angular .nx .tox .pytest_cache .mypy_cache +# .ruff_cache .gradle .terraform .dart_tool +# +# Everything below is opt-in. These names hold generated output most of +# the time, but people also name their own source directories this, and +# undo will not make a directory unrecoverable by default on a guess. -# Rust / Go / Java build output +# Rust / Go / Java / C build output target build +out # Python .venv -.mypy_cache -.pytest_cache +venv # JS/TS dist -.next -.turbo +.output + +# Test and coverage output +coverage +.nyc_output + +# Vendored dependencies +vendor + +# A build tool with a non-standard output directory: add it by name. +# Next.js with a custom distDir, for example. +# .next-dev # a specific directory tree, by absolute path # /home/you/big-scratch-area diff --git a/shim/undo_shim.c b/shim/undo_shim.c index 1a5fffb..2ebc8af 100644 --- a/shim/undo_shim.c +++ b/shim/undo_shim.c @@ -510,9 +510,28 @@ static int seg_match(const char *abs, const char *seg, size_t seglen) /* High-churn, always-regenerable trees. Skipped unless the user sets * UNDO_DEFAULT_IGNORE=0. Keeps `undo list` and the store free of build - * noise (a compiler rewriting node_modules should not fill the store). */ + * noise (a compiler rewriting node_modules should not fill the store). + * + * Everything here is a name a tool owns and will happily recreate. Names + * a person might have chosen for their own source -- dist, build, out, + * target, vendor, coverage -- are deliberately absent even though they + * hold generated output just as often: an accidental `rm -rf dist` is a + * thing people genuinely want back, and a default that silently made it + * unrecoverable would be a worse bug than the one this list fixes. They + * are in examples/ignore for anyone who wants them. + * + * Split in two because ignored() runs on every intercepted open, and a + * path with no dot-directory in it can skip the whole second list for the + * price of one strstr. */ static const char *const default_ignores[] = { - "node_modules", ".cache", "__pycache__", ".git", NULL, + "node_modules", "__pycache__", "test-results", "playwright-report", NULL, +}; + +static const char *const default_dot_ignores[] = { + ".git", ".cache", ".turbo", ".next", ".nuxt", + ".vite", ".svelte-kit", ".parcel-cache", ".angular", ".nx", + ".tox", ".pytest_cache", ".mypy_cache", ".ruff_cache", + ".gradle", ".terraform", ".dart_tool", NULL, }; /* true if `abs` should not be journaled. Patterns come from @@ -531,10 +550,16 @@ static int ignored(const char *abs) use_default = 0; } - if (use_default) + if (use_default) { for (int i = 0; default_ignores[i]; i++) if (seg_match(abs, default_ignores[i], strlen(default_ignores[i]))) return 1; + if (strstr(abs, "/.") != NULL) + for (int i = 0; default_dot_ignores[i]; i++) + if (seg_match(abs, default_dot_ignores[i], + strlen(default_dot_ignores[i]))) + return 1; + } for (const char *s = patterns; *s;) { const char *end = strchr(s, ':'); From 19d0bb61911beaaa1e3bbb1226ab2403666aa24d Mon Sep 17 00:00:00 2001 From: edaywalid Date: Sun, 9 Aug 2026 16:21:20 +0100 Subject: [PATCH 03/10] fix(hooks): do not error at the prompt when the store is gone precmd wrote its done marker unconditionally. If the session directory had been removed underneath it, by gc or by someone cleaning up by hand, every subsequent prompt printed _undo_precmd:9: no such file or directory: .../sessions/17862.../done A hook has no business erroring at the prompt because its own store changed. Guard the write in all three shells. The same spot now reports a session the shim gave up on. precmd runs once per session, so the warning prints exactly once, and it is the only chance the user has to learn that the last command was not fully recorded. The hook smoke test grew a case for it. Getting it to fail without the fix took a detour worth writing down: the obvious version removed the store with a plain `rm -rf` and the store survived, because the shim was armed for that rm and backed each file up into the directory being deleted, recreating it as fast as rm unlinked it. The test disarms the shim for that one command. The underlying behaviour is real and is why gc skips live sessions. --- shell/undo.bash | 11 ++++++++++- shell/undo.fish | 13 ++++++++++++- shell/undo.zsh | 11 ++++++++++- test/hook.sh | 14 +++++++++++++- 4 files changed, 45 insertions(+), 4 deletions(-) diff --git a/shell/undo.bash b/shell/undo.bash index 8a25b4c..bb138a9 100644 --- a/shell/undo.bash +++ b/shell/undo.bash @@ -70,7 +70,16 @@ _undo_precmd() { elif [[ -n ${_undo_saved_preload-} ]]; then export LD_PRELOAD=$_undo_saved_preload fi - : >| "$_undo_session/done" + # The shim gives up when it would otherwise fill the disk. Say so: + # precmd runs once per session, so this warns exactly once, and it is + # the only chance the user gets to hear about it. + if [[ -s $_undo_session/degraded ]]; then + printf 'undo: %s\n' "$(<"$_undo_session/degraded")" >&2 + fi + + # the store can be removed underneath a running command, by gc or by + # hand. A hook has no business erroring at the prompt when it is. + [[ -d $_undo_session ]] && : >| "$_undo_session/done" 2>/dev/null unset _undo_saved_preload _undo_session if command -v undo >/dev/null 2>&1; then diff --git a/shell/undo.fish b/shell/undo.fish index a37d151..06d93b3 100644 --- a/shell/undo.fish +++ b/shell/undo.fish @@ -85,7 +85,18 @@ function _undo_postexec --on-event fish_postexec end set -e _undo_saved_preload end - true >$_undo_session/done + # The shim gives up when it would otherwise fill the disk. Say so: + # precmd runs once per session, so this warns exactly once, and it is + # the only chance the user gets to hear about it. + if test -s $_undo_session/degraded + printf 'undo: %s\n' (cat $_undo_session/degraded) >&2 + end + + # the store can be removed underneath a running command, by gc or by + # hand. A hook has no business erroring at the prompt when it is. + if test -d $_undo_session + true >$_undo_session/done 2>/dev/null + end set -e _undo_session if command -q undo diff --git a/shell/undo.zsh b/shell/undo.zsh index e3b6e9d..0a63856 100644 --- a/shell/undo.zsh +++ b/shell/undo.zsh @@ -75,7 +75,16 @@ _undo_precmd() { export LD_PRELOAD=$_undo_saved_preload fi unset _undo_saved_preload - : >| $_undo_session/done + + # The shim gives up when it would otherwise fill the disk. Say so: + # precmd runs once per session, so this warns exactly once, and it is + # the only chance the user gets to hear about it. + [[ -s $_undo_session/degraded ]] && + print -ru2 -- "undo: $(<$_undo_session/degraded)" + + # the store can be removed underneath a running command, by gc or by + # hand. A hook has no business erroring at the prompt when it is. + [[ -d $_undo_session ]] && : >| $_undo_session/done 2>/dev/null unset _undo_session # prune: the CLI enforces count and size budgets; fall back to a diff --git a/test/hook.sh b/test/hook.sh index 7dd9010..86134e0 100755 --- a/test/hook.sh +++ b/test/hook.sh @@ -52,7 +52,16 @@ EOF ;; esac -cmds=$(printf 'rm %s/play/file.txt\nundo -y\ncat %s/play/file.txt\nexit\n' \ +# The last two lines pull the store out from under a live session: precmd +# used to write its done marker unconditionally and spent the rest of the +# session shouting at the prompt about a directory that was gone. +# +# It removes its own session directory and nothing else, so the earlier +# sessions this test checks below survive. The shell expands +# $UNDO_SESSION, then env clears it so the shim is disarmed for the rm: +# armed, it backs each deleted file up into the very directory being +# deleted and recreates it as fast as rm unlinks it. +cmds=$(printf 'rm %s/play/file.txt\nundo -y\ncat %s/play/file.txt\nenv -u UNDO_SESSION rm -rf "$UNDO_SESSION"\ntrue\nexit\n' \ "$WORK" "$WORK") # The exit status is the last command's, not a verdict on the hook, and @@ -86,6 +95,9 @@ fail() { grep -q "precious data" <<<"$out" || fail "file not restored" +grep -qi "no such file or directory" <<<"$out" && + fail "hook complained at the prompt after the store was removed" + # The store proves which half of the hook ran. A session directory means # preexec fired; the done marker means postexec did, which is what puts # LD_PRELOAD back. Checking $LD_PRELOAD from a command cannot see this: From a7ec97d97a41ba33118131acdb5a9c58c45ec059 Mon Sep 17 00:00:00 2001 From: edaywalid Date: Sun, 9 Aug 2026 16:21:20 +0100 Subject: [PATCH 04/10] feat(cli): flag sessions the shim stopped recording A session the shim gave up on restores fine, it just is not the whole command. Finding that out afterwards, from files that did not come back, is the wrong way to learn it. undo list marks it with !, undo show prints the reason and that changes past that point were not recorded, and undo warns before reverting one. e2e covers both ceilings. Neither test fills a disk: the budget case uses a 1MB cap, and the floor case sets UNDO_MIN_FREE above any plausible free space so the first backup trips it. --- cmd/undo/main.go | 13 +++++++++++++ internal/session/session.go | 7 +++++++ test/e2e.sh | 30 ++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/cmd/undo/main.go b/cmd/undo/main.go index de5a246..d4e697c 100644 --- a/cmd/undo/main.go +++ b/cmd/undo/main.go @@ -129,6 +129,11 @@ func cmdList() { if s.Undone { mark = "u" } + // a partial session restores fine, it just is not the whole + // command, and that is not something to find out afterwards + if s.Degraded != "" { + mark = "!" + } cmd := s.Cmd if len(cmd) > 60 { cmd = cmd[:57] + "..." @@ -153,6 +158,9 @@ func cmdShow(args []string) { fatal(fmt.Errorf("no such session")) } fmt.Printf("session %s (%s)\n$ %s\n\n", shortID(s.ID), when(s.ID), s.Cmd) + if s.Degraded != "" { + fmt.Printf(" ! %s\n ! changes after that point were not recorded\n\n", s.Degraded) + } for i, e := range s.Entries { fmt.Printf(" %2d %s\n", i+1, e.Describe()) } @@ -224,6 +232,11 @@ func cmdApply(s *session.Session, dir restore.Direction, opts restore.Options, y } fatal(fmt.Errorf("the command may still be running (pid %d); --force to override", s.Pid)) } + if s.Degraded != "" && dir == restore.Undo { + fmt.Fprintf(os.Stderr, "warning: %s\n"+ + "warning: this session is incomplete; changes after that point"+ + " cannot be reverted\n", s.Degraded) + } if dir == restore.Undo && s.Undone { fatal(fmt.Errorf("session was already undone (undo redo %s to re-apply)", shortID(s.ID))) } diff --git a/internal/session/session.go b/internal/session/session.go index 561fd8a..3111645 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -32,6 +32,7 @@ type Session struct { UndoneAt time.Time // when the undo happened, zero if not undone Done bool // the command finished (done marker present) Pid int // shell or runner pid, 0 for pre-lock sessions + Degraded string // why the shim stopped recording, empty if it did not Entries []journal.Entry } @@ -80,6 +81,12 @@ func load(dir string) (*Session, error) { if b, err := os.ReadFile(filepath.Join(dir, "pid")); err == nil { s.Pid, _ = strconv.Atoi(strings.TrimSpace(string(b))) } + // The shim writes this when it hits the free-space floor or the + // session budget. The session is still restorable, it is just not the + // whole command, so anything that shows a session has to say so. + if b, err := os.ReadFile(filepath.Join(dir, "degraded")); err == nil { + s.Degraded = strings.TrimSpace(string(b)) + } entries, err := journal.Read(filepath.Join(dir, "journal")) if err != nil { if os.IsNotExist(err) { diff --git a/test/e2e.sh b/test/e2e.sh index f2d92bb..c3ff7c5 100755 --- a/test/e2e.sh +++ b/test/e2e.sh @@ -308,5 +308,35 @@ else echo " (no cc, skipped)" fi +echo "== case 26: the session budget stops recording and says so" +mkdir -p "$PLAY/big" +for i in 1 2 3 4 5; do head -c 400000 /dev/zero >"$PLAY/big/f$i"; done +UNDO_MAX_SESSION=1000000 run_armed "rm $PLAY/big/f1 $PLAY/big/f2 $PLAY/big/f3 $PLAY/big/f4 $PLAY/big/f5" +last=$(ls "$UNDO_DATA_DIR/sessions" | sort | tail -1) +sess=$UNDO_DATA_DIR/sessions/$last +[[ -s $sess/degraded ]] || fail "no degraded marker after blowing the budget" +grep -q UNDO_MAX_SESSION "$sess/degraded" || fail "degraded marker blames the wrong limit" +# the rm itself must still have happened: the shim records, it never vetoes +[[ ! -e $PLAY/big/f5 ]] || fail "shim blocked the command it could not record" +n=$(grep -c . "$sess/journal") +((n > 0 && n < 5)) || fail "expected a partial journal, got $n entries" +du_kb=$(du -sk "$sess" | cut -f1) +((du_kb < 2000)) || fail "session grew to ${du_kb}K past a 1MB budget" +"$UNDO" list | grep -q "^! " || fail "undo list does not flag the degraded session" +"$UNDO" show | grep -qi "not recorded" || fail "undo show does not explain the gap" +"$UNDO" -y >/dev/null 2>&1 +[[ -e $PLAY/big/f1 ]] || fail "the part that was recorded did not restore" + +echo "== case 27: the free-space floor stops recording" +echo keepme >"$PLAY/floor.txt" +# no real disk gets filled here: the floor is set above any plausible +# free space, so the very first backup trips it +UNDO_MIN_FREE=999999999999999 run_armed "rm $PLAY/floor.txt" +last=$(ls "$UNDO_DATA_DIR/sessions" | sort | tail -1) +sess=$UNDO_DATA_DIR/sessions/$last +[[ ! -e $PLAY/floor.txt ]] || fail "rm did not run" +grep -q UNDO_MIN_FREE "$sess/degraded" || fail "floor did not report itself" +[[ ! -s $sess/journal ]] || fail "journal grew after the floor was hit" + echo echo "all cases passed" From 4a854e9cf26b46df057a1e668a4c1bb8bb42cdee Mon Sep 17 00:00:00 2001 From: edaywalid Date: Sun, 9 Aug 2026 16:21:20 +0100 Subject: [PATCH 05/10] docs: document the space guards and the wider ignore list --- CHANGELOG.md | 31 +++++++++++++++++++++++++++++++ README.md | 49 +++++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 72 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e7a2a4f..b68d78b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,36 @@ # Changelog +## Unreleased + +- undo could fill a disk. One session grew to 147G over 23 hours and took + a 320G filesystem to zero bytes free, which broke every other program on + the machine. The size budget was enforced by `undo gc`, which the shell + hook only calls when a command returns; the command here was an agent + that ran all day and never did. Nothing was ever going to prune the one + session that was growing, and `gc` skips live sessions anyway. Two + ceilings now live in the shim, checked as it writes: `UNDO_MIN_FREE` + (2 GiB, a floor on free space) and `UNDO_MAX_SESSION` (1 GiB per + command). Whichever trips first, that session stops recording and says + so at the next prompt; `undo list` marks it `!` and `undo show` + explains the gap. The command itself is never blocked. +- `UNDO_MAX_BYTES` did not apply to deletions. A hardlink copies no data, + so the per-file cap skipped it, but the link is exactly what stops the + original blocks being freed when the file is unlinked. Deleting a 4G + file cost 4G that the cap was supposed to have refused. +- The built-in ignore list missed the caches that churn hardest. + `.turbo/cache` never matched the `.cache` pattern, because patterns + match whole path components. Added `.turbo`, `.next`, `.nuxt`, `.vite`, + `.svelte-kit`, `.parcel-cache`, `.angular`, `.nx`, `.tox`, + `.pytest_cache`, `.mypy_cache`, `.ruff_cache`, `.gradle`, `.terraform`, + `.dart_tool`, `test-results` and `playwright-report`. `dist`, `build`, + `target` and `vendor` stay opt-in in `examples/ignore`: they hold + generated output most of the time, but an accidental `rm -rf dist` is + something people want back. +- The shell hook wrote its done marker without checking the session + directory still existed, so a store removed underneath it made every + later prompt print `no such file or directory`. Fixed in all three + shells. + ## v0.2.9 - 2026-08-02 - The shim could stop journaling partway through a command with no sign diff --git a/README.md b/README.md index e9905e8..9ec1d36 100644 --- a/README.md +++ b/README.md @@ -75,6 +75,7 @@ whole storage format is plain files you can inspect: ├── cmd rm -rf thesis/ the command line, for `undo list` ├── journal one line per change replayed in reverse ├── pid, done liveness markers so undo won't touch a running command + ├── budget bytes stored so far shared by every process in the session └── data/ ├── 48211-1 = thesis/draft.md (hardlink, no data copied) └── 48211-2 = thesis/refs.bib @@ -219,10 +220,28 @@ If you deleted something specifically to free disk, run `undo purge`. **Overwriting a file in place does cost disk.** A hardlink cannot preserve content that is about to be rewritten, so for `>` truncation, editors, and `shred`, undo copies the previous version. That copy is real -new space, capped per file by `UNDO_MAX_BYTES` (256 MiB default). +new space, capped per file by `UNDO_MAX_BYTES` (256 MiB default). The +same cap applies to deletions: the hardlink costs no new blocks, but it +is exactly what keeps the original ones from being freed. -**Nothing grows without bound.** After every command undo prunes the -store: +**Two ceilings apply while a command runs**, enforced by the shim itself: + +| Limit | Default | Meaning | +| --- | --- | --- | +| `UNDO_MIN_FREE` | 2 GiB | free space undo refuses to eat into. Checked against the store's filesystem as backups are written. | +| `UNDO_MAX_SESSION` | 1 GiB | how much one command may record. | + +Hit either and the session stops recording, writes down why, and says so +at your next prompt. `undo list` marks it `!` and `undo show` explains +the gap. The command itself is never blocked or slowed down; undo only +ever stops recording it. + +This matters most for commands that do not return for hours: an editor, +a dev server, an agent. The store-wide limits below only get a turn +between commands, so before this they simply never ran for the session +that was actually growing. + +**Two more apply between commands**, when the prompt comes back: | Limit | Default | Meaning | | --- | --- | --- | @@ -260,9 +279,21 @@ and SSDs, which its own man page explains. A command that rewrites thousands of files (a package install, a build) would otherwise flood `undo list` and the store with churn you will never -revert. The shim always skips `node_modules`, `.cache`, `__pycache__`, -and `.git`, and collapses repeated writes to the same file within one -command down to a single backup. Add your own patterns in +revert. The shim always skips the caches that build tools own and +recreate: + +``` +node_modules __pycache__ test-results playwright-report +.git .cache .turbo .next .nuxt .vite .svelte-kit .parcel-cache +.angular .nx .tox .pytest_cache .mypy_cache .ruff_cache +.gradle .terraform .dart_tool +``` + +It also collapses repeated writes to the same file within one command +down to a single backup. Names you might have picked yourself, like +`dist`, `build`, `target` or `vendor`, are deliberately not built in: +they hold generated output most of the time, but an accidental +`rm -rf dist` is a thing people want back. Add your own patterns in `~/.config/undo/ignore` (see [`examples/ignore`](examples/ignore)): ``` @@ -282,11 +313,13 @@ Environment variables, set before sourcing the hook: | --- | --- | --- | | `UNDO_KEEP` | `30` | how many commands are kept (see [storage](#storage-and-disk-space)) | | `UNDO_MAX_STORE` | 1 GiB | total store size budget in bytes; oldest pruned first | -| `UNDO_MAX_BYTES` | 256 MiB | largest file the shim will copy for an in-place overwrite; deletions are hardlinked with no size limit | +| `UNDO_MAX_BYTES` | 256 MiB | largest file the shim will back up, for deletions as well as overwrites | +| `UNDO_MIN_FREE` | 2 GiB | stop recording when the store's filesystem has less than this free; `0` disables | +| `UNDO_MAX_SESSION` | 1 GiB | stop recording when one command has stored this much; `0` disables | | `UNDO_DATA_DIR` | `~/.local/share/undo` | where sessions live | | `UNDO_IGNORE` | from config file | colon-separated ignore patterns, overrides `~/.config/undo/ignore` | | `UNDO_IGNORE_FILE` | `~/.config/undo/ignore` | where the ignore list is read from | -| `UNDO_DEFAULT_IGNORE` | on | set to `0` to stop skipping `node_modules`, `.cache`, `__pycache__`, `.git` | +| `UNDO_DEFAULT_IGNORE` | on | set to `0` to stop skipping the built-in cache directories | | `UNDO_CAPTURE_SHELL` | off | zsh only: set to `1` to re-exec once at startup with the shim preloaded, so the shell's own redirections (`echo x > file`) are captured too | | `UNDO_LIB` | auto-detected | explicit path to `libundo.so` | From 421051f6b2cc529336f6832957332e31797c4ca8 Mon Sep 17 00:00:00 2001 From: edaywalid Date: Sun, 9 Aug 2026 16:21:20 +0100 Subject: [PATCH 06/10] fix(session): rename a session out of the way before deleting it Deleting a session whose writer is still running fails partway: rm: cannot remove '.../sessions/1786.../data': Directory not empty The shim saves each backup into the directory being deleted, so it recreates entries as fast as RemoveAll unlinks them. RemoveAll gives up having already destroyed most of the session, which is the worst of both outcomes. Reproduces reliably against a tight `rm -f` over 20k files. Rename first. That takes the whole tree out from under the shim in one step: the writer goes on appending into a directory nobody will read, and the delete that follows races with nothing. List skips a leftover .removing, which means a removal that died partway rather than a session. gc already declines to touch live sessions, so this is `undo purge --force` and anything that gets the timing wrong. --- internal/session/session.go | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/internal/session/session.go b/internal/session/session.go index 3111645..29d781e 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -110,7 +110,9 @@ func List() ([]*Session, error) { } names := make([]string, 0, len(dirs)) for _, d := range dirs { - if d.IsDir() { + // Remove renames before it deletes; a leftover means a removal + // died partway and the tree is rubbish, not a session. + if d.IsDir() && !strings.HasSuffix(d.Name(), removingSuffix) { names = append(names, d.Name()) } } @@ -293,9 +295,28 @@ func GC(keep int, maxBytes int64) (int, error) { return removed, nil } +// the name a session is parked under while it is being deleted +const removingSuffix = ".removing" + // Remove deletes a session and its backups entirely. +// +// The rename comes first because a session whose writer is still running +// recreates files as fast as RemoveAll unlinks them: the shim saves each +// backup into the directory being deleted, and RemoveAll gives up with +// "directory not empty" having already destroyed most of it. Renaming +// takes the whole tree out from under the shim in one step. The writer +// carries on appending to a path nobody will ever read, and the delete +// that follows races with nothing. func (s *Session) Remove() error { - return os.RemoveAll(s.Dir) + doomed := s.Dir + removingSuffix + if err := os.RemoveAll(doomed); err != nil { + return os.RemoveAll(s.Dir) + } + if err := os.Rename(s.Dir, doomed); err != nil { + // already gone, or a filesystem that will not have it + return os.RemoveAll(s.Dir) + } + return os.RemoveAll(doomed) } // MarkUndone records that a session was reverted, and when. From 197814ff63aa71d711d7b11c867acfbeacb1c756 Mon Sep 17 00:00:00 2001 From: edaywalid Date: Sun, 9 Aug 2026 16:21:20 +0100 Subject: [PATCH 07/10] fix(shim): release per-thread state when a thread exits The journal descriptor and the budget mapping are __thread. A thread that exits takes the variables with it but not the descriptor and the page they point at, so a program doing its file work on short-lived threads leaked one of each per thread until it ran out of descriptors. Measured: 205 threads, 205 leaked descriptors. This is the "descriptor leak" from the incident report, though not the mechanism it proposed. The shim was not reopening the journal per intercepted call, and the suggested O_APPEND | O_CLOEXEC was already there. Eight descriptors on one journal was eight threads. A key destructor is the only thread-exit hook C offers, so the shim now links pthread. Nothing here is shared between threads, so this releases state rather than synchronising it, and the write path is untouched. The released .so still builds against glibc 2.31 on debian:11, where pthread_key_create is GLIBC_2.2.5, so the advertised 2.6 floor holds. --- .github/workflows/ci.yml | 2 +- .github/workflows/release.yml | 4 +- Makefile | 2 +- shim/undo_shim.c | 96 ++++++++++++++++++++++++++--------- test/e2e.sh | 63 +++++++++++++++++++++++ 5 files changed, 139 insertions(+), 28 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1afe776..0fea28a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -53,7 +53,7 @@ jobs: - name: build shim and check its glibc floor run: | mkdir -p build - gcc -shared -fPIC -O2 -Wall -o build/libundo.so shim/undo_shim.c -ldl + gcc -shared -fPIC -O2 -Wall -o build/libundo.so shim/undo_shim.c -ldl -lpthread max=$(objdump -T build/libundo.so | grep -o 'GLIBC_[0-9.]*' | sort -u -V | tail -1) echo "highest glibc symbol required: $max" # even built on a modern runner, nothing newer than 2.34 (dlsym) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 6eed2ff..92d8eac 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -26,8 +26,8 @@ jobs: mkdir -p build docker run --rm -v "$PWD:/w" -w /w debian:11 bash -c ' apt-get update -qq && apt-get install -y -qq gcc gcc-aarch64-linux-gnu binutils - gcc -shared -fPIC -O2 -Wall -o build/libundo_amd64.so shim/undo_shim.c -ldl - aarch64-linux-gnu-gcc -shared -fPIC -O2 -Wall -o build/libundo_arm64.so shim/undo_shim.c -ldl + gcc -shared -fPIC -O2 -Wall -o build/libundo_amd64.so shim/undo_shim.c -ldl -lpthread + aarch64-linux-gnu-gcc -shared -fPIC -O2 -Wall -o build/libundo_arm64.so shim/undo_shim.c -ldl -lpthread ' - name: assert the shims load on old distros run: | diff --git a/Makefile b/Makefile index 2bd8b39..fba28d0 100644 --- a/Makefile +++ b/Makefile @@ -8,7 +8,7 @@ all: bin/undo build/libundo.so build/libundo.so: shim/undo_shim.c @mkdir -p build - $(CC) -shared -fPIC -O2 -Wall -Wextra -o $@ $< -ldl + $(CC) -shared -fPIC -O2 -Wall -Wextra -o $@ $< -ldl -lpthread bin/undo: $(GO_SRC) go.mod @mkdir -p bin diff --git a/shim/undo_shim.c b/shim/undo_shim.c index 2ebc8af..0740ce1 100644 --- a/shim/undo_shim.c +++ b/shim/undo_shim.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -39,6 +40,9 @@ static __thread int in_shim; +/* asks for a thread-exit callback; defined with the cleanup itself below */ +static void tls_arm(void); + #define REAL(name, ret, ...) \ static ret (*real_##name)(__VA_ARGS__); \ if (!real_##name) \ @@ -54,25 +58,29 @@ static const char *session_dir(void) return s; } +/* File scope, not function scope, so thread_cleanup() can reach them. */ +static __thread char jrn_dir[PATH_MAX]; +static __thread int jrn_fd = -1; + static int journal_fd(void) { - static __thread char cached_dir[PATH_MAX]; - static __thread int fd = -1; const char *dir = session_dir(); if (!dir) return -1; - if (fd >= 0 && strcmp(cached_dir, dir) == 0) - return fd; - if (fd >= 0) - close(fd); + if (jrn_fd >= 0 && strcmp(jrn_dir, dir) == 0) + return jrn_fd; + if (jrn_fd >= 0) + close(jrn_fd); char path[PATH_MAX]; if ((size_t)snprintf(path, sizeof path, "%s/journal", dir) >= sizeof path) - return fd = -1; + return jrn_fd = -1; REAL(open, int, const char *, int, ...); - fd = real_open(path, O_WRONLY | O_APPEND | O_CREAT | O_CLOEXEC, 0600); - if (fd >= 0) - snprintf(cached_dir, sizeof cached_dir, "%s", dir); - return fd; + jrn_fd = real_open(path, O_WRONLY | O_APPEND | O_CREAT | O_CLOEXEC, 0600); + if (jrn_fd >= 0) { + snprintf(jrn_dir, sizeof jrn_dir, "%s", dir); + tls_arm(); + } + return jrn_fd; } static int armed(void) @@ -252,18 +260,20 @@ struct budget { uint32_t stopped; /* set once, by whoever trips a ceiling first */ }; +/* file scope for the same reason as the journal descriptor above */ +static __thread char bgt_dir[PATH_MAX]; +static __thread struct budget *bgt_map; + static struct budget *budget_map(void) { - static __thread char cached_dir[PATH_MAX]; - static __thread struct budget *map; const char *dir = session_dir(); if (!dir) return NULL; - if (map && strcmp(cached_dir, dir) == 0) - return map; - if (map) { - munmap(map, sizeof *map); - map = NULL; + if (bgt_map && strcmp(bgt_dir, dir) == 0) + return bgt_map; + if (bgt_map) { + munmap(bgt_map, sizeof *bgt_map); + bgt_map = NULL; } char path[PATH_MAX]; if ((size_t)snprintf(path, sizeof path, "%s/budget", dir) >= sizeof path) @@ -277,18 +287,56 @@ static struct budget *budget_map(void) struct stat st; REAL(ftruncate, int, int, off_t); if (fstat(fd, &st) != 0 || - (st.st_size < (off_t)sizeof *map && - real_ftruncate(fd, (off_t)sizeof *map) != 0)) { + (st.st_size < (off_t)sizeof *bgt_map && + real_ftruncate(fd, (off_t)sizeof *bgt_map) != 0)) { close(fd); return NULL; } - void *p = mmap(NULL, sizeof *map, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); + void *p = mmap(NULL, sizeof *bgt_map, PROT_READ | PROT_WRITE, MAP_SHARED, + fd, 0); close(fd); if (p == MAP_FAILED) return NULL; - map = p; - snprintf(cached_dir, sizeof cached_dir, "%s", dir); - return map; + bgt_map = p; + snprintf(bgt_dir, sizeof bgt_dir, "%s", dir); + tls_arm(); + return bgt_map; +} + +/* A thread that exits takes its TLS variables with it, but not the + * descriptor and the mapping they point at: those belong to the process + * and stay until it dies. A program that does its file work on + * short-lived threads therefore leaked a journal descriptor and a page + * per thread, until it ran out of descriptors. Nothing here is shared + * between threads, so this is a release, not a synchronisation problem. + * + * A key destructor is the only thread-exit hook C gives us. */ +static pthread_key_t tls_key; +static pthread_once_t tls_once = PTHREAD_ONCE_INIT; + +static void thread_cleanup(void *unused) +{ + (void)unused; + if (jrn_fd >= 0) { + close(jrn_fd); + jrn_fd = -1; + } + if (bgt_map) { + munmap(bgt_map, sizeof *bgt_map); + bgt_map = NULL; + } +} + +static void tls_key_init(void) +{ + pthread_key_create(&tls_key, thread_cleanup); +} + +static void tls_arm(void) +{ + pthread_once(&tls_once, tls_key_init); + /* the value is only a flag: glibc skips the destructor for a NULL one */ + pthread_setspecific(tls_key, (void *)1); } /* Records why recording stopped, once per session. Called before the diff --git a/test/e2e.sh b/test/e2e.sh index c3ff7c5..0ff63fd 100755 --- a/test/e2e.sh +++ b/test/e2e.sh @@ -338,5 +338,68 @@ sess=$UNDO_DATA_DIR/sessions/$last grep -q UNDO_MIN_FREE "$sess/degraded" || fail "floor did not report itself" [[ ! -s $sess/journal ]] || fail "journal grew after the floor was hit" +echo "== case 28: threads do not leak a journal descriptor each" +# The journal descriptor is per-thread. A thread that exits takes the +# variable holding it, not the descriptor, so a program doing its file +# work on short-lived threads used to leak one per thread until it hit +# EMFILE. 205 threads leaked 205 descriptors before this was fixed. +cat >"$WORK/thr.c" <<'CEOF' +#include +#include +#include + +static char *dir; + +static void *work(void *arg) +{ + char p[512]; + snprintf(p, sizeof p, "%s/t%ld", dir, (long)arg); + FILE *f = fopen(p, "w"); + if (f) { + fputs("x\n", f); + fclose(f); + } + return NULL; +} + +static int openfds(void) +{ + DIR *d = opendir("/proc/self/fd"); + struct dirent *e; + int n = 0; + if (!d) + return -1; + while ((e = readdir(d)) != NULL) + if (e->d_name[0] != '.') + n++; + closedir(d); + return n; +} + +int main(int c, char **v) +{ + (void)c; + dir = v[1]; + pthread_t t; + for (long i = 0; i < 5; i++) { + pthread_create(&t, 0, work, (void *)i); + pthread_join(t, 0); + } + int before = openfds(); + for (long i = 5; i < 205; i++) { + pthread_create(&t, 0, work, (void *)i); + pthread_join(t, 0); + } + /* a couple of descriptors of slack, none of it proportional to 200 */ + return openfds() > before + 2; +} +CEOF +if cc -O2 -o "$WORK/thr" "$WORK/thr.c" -lpthread 2>/dev/null; then + mkdir -p "$PLAY/threads" + run_armed "$WORK/thr $PLAY/threads" || fail "threads leaked journal descriptors" +else + echo " (no cc, skipped)" +fi + echo echo "all cases passed" From fd52dd712e3cffb07842840eeb3cfac283d6c9de Mon Sep 17 00:00:00 2001 From: edaywalid Date: Sun, 9 Aug 2026 16:21:20 +0100 Subject: [PATCH 08/10] docs: changelog the descriptor leak and live-session removal --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b68d78b..6b98322 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,16 @@ `target` and `vendor` stay opt-in in `examples/ignore`: they hold generated output most of the time, but an accidental `rm -rf dist` is something people want back. +- The shim leaked a journal descriptor and a mapped page per thread. Both + are thread-local, and a thread that exits takes the variable holding + them but not the descriptor or the page itself, so a program doing its + file work on short-lived threads climbed towards `EMFILE`. Released + from a TLS destructor now; the shim links pthread for it, and the + released binary keeps its glibc 2.6 floor. +- Deleting a session whose command was still running could fail partway + with `Directory not empty`, having already destroyed most of it: the + shim recreated backups as fast as the delete unlinked them. Sessions + are renamed out of the way before removal. - The shell hook wrote its done marker without checking the session directory still existed, so a store removed underneath it made every later prompt print `no such file or directory`. Fixed in all three From b651c9209110ae212c2b466549dc731f389943be Mon Sep 17 00:00:00 2001 From: edaywalid Date: Sun, 9 Aug 2026 16:21:20 +0100 Subject: [PATCH 09/10] fix(doctor): report the real built-in ignore list doctor had its own hardcoded copy of the shim's defaults, so after the list grew it told users the shim skips four things when it skips twenty-one. The list is too long to spell out now, so it reports the count and the first few. The copy is still a copy, because the list lives in C and the CLI is Go. A test parses the arrays out of shim/undo_shim.c and fails if the two drift, which is the only reason this was caught. --- cmd/undo/doctor.go | 14 +++++++++++++- cmd/undo/doctor_test.go | 42 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 cmd/undo/doctor_test.go diff --git a/cmd/undo/doctor.go b/cmd/undo/doctor.go index 571ec08..6aa777a 100644 --- a/cmd/undo/doctor.go +++ b/cmd/undo/doctor.go @@ -133,8 +133,20 @@ func reportStore(report func(checkState, string, string), root string) { report(pass, "store", root) } +// builtinIgnores mirrors default_ignores and default_dot_ignores in +// shim/undo_shim.c. TestBuiltinIgnoresMatchShim parses the C source and +// fails if the two drift, which is how this list went stale last time. +var builtinIgnores = []string{ + "node_modules", "__pycache__", "test-results", "playwright-report", + ".git", ".cache", ".turbo", ".next", ".nuxt", ".vite", ".svelte-kit", + ".parcel-cache", ".angular", ".nx", ".tox", ".pytest_cache", + ".mypy_cache", ".ruff_cache", ".gradle", ".terraform", ".dart_tool", +} + func reportIgnore(report func(checkState, string, string)) { - defaults := "node_modules, .cache, __pycache__, .git (built in)" + // too many to spell out on one line; the README has the full list + defaults := fmt.Sprintf("%d tool caches built in (%s, ...)", + len(builtinIgnores), strings.Join(builtinIgnores[:4], ", ")) if extra := os.Getenv("UNDO_IGNORE"); extra != "" { n := len(strings.Split(extra, ":")) report(pass, "ignore", fmt.Sprintf("%d extra pattern(s) from config; %s", n, defaults)) diff --git a/cmd/undo/doctor_test.go b/cmd/undo/doctor_test.go new file mode 100644 index 0000000..c063e7b --- /dev/null +++ b/cmd/undo/doctor_test.go @@ -0,0 +1,42 @@ +package main + +import ( + "os" + "regexp" + "sort" + "testing" +) + +// The built-in ignore list lives in C, and doctor keeps a copy so it can +// report it. A copy that drifts tells users the shim skips things it does +// not, so read the real list out of the shim source and compare. +func TestBuiltinIgnoresMatchShim(t *testing.T) { + src, err := os.ReadFile("../../shim/undo_shim.c") + if err != nil { + t.Fatalf("cannot read shim source: %v", err) + } + arrays := regexp.MustCompile( + `(?s)default_(?:dot_)?ignores\[\] = \{(.*?)\};`).FindAllSubmatch(src, -1) + if len(arrays) != 2 { + t.Fatalf("expected 2 default ignore arrays in the shim, found %d", len(arrays)) + } + quoted := regexp.MustCompile(`"([^"]*)"`) + var fromShim []string + for _, a := range arrays { + for _, m := range quoted.FindAllSubmatch(a[1], -1) { + fromShim = append(fromShim, string(m[1])) + } + } + got := append([]string(nil), builtinIgnores...) + sort.Strings(got) + sort.Strings(fromShim) + if len(got) != len(fromShim) { + t.Fatalf("doctor lists %d built-in ignores, the shim has %d\ndoctor: %v\nshim: %v", + len(got), len(fromShim), got, fromShim) + } + for i := range got { + if got[i] != fromShim[i] { + t.Errorf("built-in ignore mismatch: doctor has %q, shim has %q", got[i], fromShim[i]) + } + } +} From 92dd00bc24dc6644a6c759c64fc390d55ce622cd Mon Sep 17 00:00:00 2001 From: edaywalid Date: Sun, 9 Aug 2026 16:47:27 +0100 Subject: [PATCH 10/10] fix(shim): derive the session cap and share the statvfs interval Review of the space guards turned up four things. The per-session cap defaulted to 1G, the same as UNDO_MAX_STORE, so one max-size session filled the entire store budget and gc dropped every other session behind it. Two numbers matching by coincidence, not a decision. It is half the store budget now, so the store holds more than one session, and raising UNDO_MAX_STORE raises it too. The statvfs interval was a __thread counter, so "one check per 64M" was per thread: a busy process checked far more often than intended and a session spread over several processes far less. It moves into the shared mapping with the byte count. UNDO_MIN_FREE and UNDO_MAX_SESSION were read with getenv on every backup, where max_bytes() beside them has always cached. They cache now, and jwrite reads the session directory once for the pair of lookups it does per journal line instead of once each. budget_stop() claimed to write the degraded marker once per session but only the shared flag enforced that, so a failed mmap meant every process wrote it. O_EXCL makes the claim true on its own. --- CHANGELOG.md | 4 +- README.md | 4 +- shim/undo_shim.c | 139 ++++++++++++++++++++++++++++++++--------------- test/e2e.sh | 28 ++++++++++ 4 files changed, 127 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b98322..a528f3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,8 +9,8 @@ that ran all day and never did. Nothing was ever going to prune the one session that was growing, and `gc` skips live sessions anyway. Two ceilings now live in the shim, checked as it writes: `UNDO_MIN_FREE` - (2 GiB, a floor on free space) and `UNDO_MAX_SESSION` (1 GiB per - command). Whichever trips first, that session stops recording and says + (2 GiB, a floor on free space) and `UNDO_MAX_SESSION` (half of + `UNDO_MAX_STORE`, so the store has room for more than one session). Whichever trips first, that session stops recording and says so at the next prompt; `undo list` marks it `!` and `undo show` explains the gap. The command itself is never blocked. - `UNDO_MAX_BYTES` did not apply to deletions. A hardlink copies no data, diff --git a/README.md b/README.md index 9ec1d36..c0a614c 100644 --- a/README.md +++ b/README.md @@ -229,7 +229,7 @@ is exactly what keeps the original ones from being freed. | Limit | Default | Meaning | | --- | --- | --- | | `UNDO_MIN_FREE` | 2 GiB | free space undo refuses to eat into. Checked against the store's filesystem as backups are written. | -| `UNDO_MAX_SESSION` | 1 GiB | how much one command may record. | +| `UNDO_MAX_SESSION` | half of `UNDO_MAX_STORE` (512 MiB) | how much one command may record. Derived so the store has room for more than one session. | Hit either and the session stops recording, writes down why, and says so at your next prompt. `undo list` marks it `!` and `undo show` explains @@ -315,7 +315,7 @@ Environment variables, set before sourcing the hook: | `UNDO_MAX_STORE` | 1 GiB | total store size budget in bytes; oldest pruned first | | `UNDO_MAX_BYTES` | 256 MiB | largest file the shim will back up, for deletions as well as overwrites | | `UNDO_MIN_FREE` | 2 GiB | stop recording when the store's filesystem has less than this free; `0` disables | -| `UNDO_MAX_SESSION` | 1 GiB | stop recording when one command has stored this much; `0` disables | +| `UNDO_MAX_SESSION` | half of `UNDO_MAX_STORE` | stop recording when one command has stored this much; `0` disables | | `UNDO_DATA_DIR` | `~/.local/share/undo` | where sessions live | | `UNDO_IGNORE` | from config file | colon-separated ignore patterns, overrides `~/.config/undo/ignore` | | `UNDO_IGNORE_FILE` | `~/.config/undo/ignore` | where the ignore list is read from | diff --git a/shim/undo_shim.c b/shim/undo_shim.c index 0740ce1..d90a872 100644 --- a/shim/undo_shim.c +++ b/shim/undo_shim.c @@ -32,9 +32,12 @@ #endif #define DEFAULT_MAX_BYTES (256UL * 1024 * 1024) -#define DEFAULT_MAX_SESSION (1UL << 30) #define DEFAULT_MIN_FREE (2UL << 30) +/* Mirrors the UNDO_MAX_STORE default in cmd/undo/main.go. The shim only + * reads it to size the per-session cap; gc is what enforces it. */ +#define DEFAULT_MAX_STORE (1UL << 30) + /* how many bytes of backups may go by between two statvfs calls */ #define FREE_CHECK_INTERVAL (64UL << 20) @@ -62,9 +65,8 @@ static const char *session_dir(void) static __thread char jrn_dir[PATH_MAX]; static __thread int jrn_fd = -1; -static int journal_fd(void) +static int journal_fd(const char *dir) { - const char *dir = session_dir(); if (!dir) return -1; if (jrn_fd >= 0 && strcmp(jrn_dir, dir) == 0) @@ -108,18 +110,23 @@ static void enc_append(char *dst, size_t cap, size_t *len, const char *s) } } -static int recording_stopped(void); +static int recording_stopped(const char *dir); /* jwrite("op", field1, field2, NULL) */ static void jwrite(const char *op, ...) { + /* One getenv for the pair below, not one each: this runs for every + * journal line and getenv walks environ. */ + const char *dir = session_dir(); + if (!dir) + return; /* Once a ceiling has been hit the journal stops growing too. It is * small next to the backups, but the point of the free-space floor is * that undo stops touching a filesystem in trouble, and a record of * changes whose backups were never taken is not worth a byte of it. */ - if (recording_stopped()) + if (recording_stopped(dir)) return; - int fd = journal_fd(); + int fd = journal_fd(dir); if (fd < 0) return; char line[4 * PATH_MAX]; @@ -229,14 +236,47 @@ static unsigned long max_bytes(void) return v; } -/* A limit read from the environment, where an explicit 0 means no limit. - * Distinct from max_bytes() above, whose 0 has always meant "unset". */ -static unsigned long limit_env(const char *name, unsigned long def) +/* A limit read from the environment once and remembered, where an + * explicit 0 means no limit. Distinct from max_bytes() above, whose 0 has + * always meant "unset", which is why that one cannot express "unlimited". + * + * Cached because these are read on every backup, and getenv walks environ + * each time. Nothing changes UNDO_* mid-process except the session, which + * is read separately. */ +static unsigned long min_free(void) { - const char *s = getenv(name); - if (!s || !*s) - return def; - return parse_ulong(s); + static unsigned long v; + static int loaded; + if (!loaded) { + loaded = 1; + const char *s = getenv("UNDO_MIN_FREE"); + v = (!s || !*s) ? DEFAULT_MIN_FREE : parse_ulong(s); + } + return v; +} + +/* Default: half the store budget, so the store can hold more than one + * session. A per-session cap equal to the whole store budget means one + * large command evicts every other session the moment gc runs, which is + * a coincidence of two numbers rather than a decision. Following + * UNDO_MAX_STORE also means raising the store budget raises this. */ +static unsigned long max_session(void) +{ + static unsigned long v; + static int loaded; + if (!loaded) { + loaded = 1; + const char *s = getenv("UNDO_MAX_SESSION"); + if (s && *s) { + v = parse_ulong(s); + return v; + } + const char *st = getenv("UNDO_MAX_STORE"); + unsigned long store = + (st && *st) ? parse_ulong(st) : DEFAULT_MAX_STORE; + v = store / 2; + } + return v; } /* ---------- space guards ---------- */ @@ -256,17 +296,17 @@ static unsigned long limit_env(const char *name, unsigned long def) * The counter lives in a shared mapping, not a static, so the thousand * compilers a build forks all bill to the same session budget. */ struct budget { - uint64_t saved; /* bytes of backups written by every process */ - uint32_t stopped; /* set once, by whoever trips a ceiling first */ + uint64_t saved; /* bytes of backups written by every process */ + uint64_t since_check; /* bytes since the last statvfs, shared */ + uint32_t stopped; /* set once, by whoever trips a ceiling first */ }; /* file scope for the same reason as the journal descriptor above */ static __thread char bgt_dir[PATH_MAX]; static __thread struct budget *bgt_map; -static struct budget *budget_map(void) +static struct budget *budget_map(const char *dir) { - const char *dir = session_dir(); if (!dir) return NULL; if (bgt_map && strcmp(bgt_dir, dir) == 0) @@ -341,19 +381,20 @@ static void tls_arm(void) /* Records why recording stopped, once per session. Called before the * filesystem is actually full, so this small write still has room. */ -static void budget_stop(const char *why) +static void budget_stop(const char *dir, const char *why) { - struct budget *b = budget_map(); + struct budget *b = budget_map(dir); if (b && __atomic_exchange_n(&b->stopped, 1, __ATOMIC_RELAXED)) return; /* someone else already wrote the marker */ - const char *dir = session_dir(); if (!dir) return; char path[PATH_MAX]; if ((size_t)snprintf(path, sizeof path, "%s/degraded", dir) >= sizeof path) return; + /* O_EXCL so this really is once per session even when the mapping + * above could not be made and every process reaches this point */ REAL(open, int, const char *, int, ...); - int fd = real_open(path, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0600); + int fd = real_open(path, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, 0600); if (fd < 0) return; char msg[512]; @@ -365,21 +406,21 @@ static void budget_stop(const char *why) close(fd); } -static int recording_stopped(void) +static int recording_stopped(const char *dir) { - struct budget *b = budget_map(); + struct budget *b = budget_map(dir); return b && __atomic_load_n(&b->stopped, __ATOMIC_RELAXED); } /* true if `want` more bytes of backup are affordable */ -static int budget_ok(unsigned long want) +static int budget_ok(const char *dir, unsigned long want) { - struct budget *b = budget_map(); + struct budget *b = budget_map(dir); if (b && __atomic_load_n(&b->stopped, __ATOMIC_RELAXED)) return 0; char why[512]; - unsigned long cap = limit_env("UNDO_MAX_SESSION", DEFAULT_MAX_SESSION); + unsigned long cap = max_session(); if (b && cap) { unsigned long used = __atomic_load_n(&b->saved, __ATOMIC_RELAXED); if (used + want > cap) { @@ -387,43 +428,52 @@ static int budget_ok(unsigned long want) "stopped recording: session reached the %lu MB budget " "(UNDO_MAX_SESSION)", (cap + (1UL << 19)) >> 20); - budget_stop(why); + budget_stop(dir, why); return 0; } } - unsigned long floor = limit_env("UNDO_MIN_FREE", DEFAULT_MIN_FREE); + unsigned long floor = min_free(); if (!floor) return 1; + /* statvfs on every backup would be wasteful and on none would be - * useless; amortise it over the bytes actually written. Starting at - * the interval forces a check before the first backup of a session, - * so a session opened on an already-full disk never gets going. */ - static __thread unsigned long since = FREE_CHECK_INTERVAL; - since += want; - if (since < FREE_CHECK_INTERVAL) - return 1; - since = 0; - const char *dir = session_dir(); + * useless, so amortise it over the bytes actually written. The + * counter lives in the shared mapping: per-thread, every thread got + * its own 64M of slack and a busy process checked far more often + * than intended while a session spread over many processes checked + * far less. The first backup of a session always checks, so a + * session opened on an already-full disk never gets going. */ + if (b) { + uint64_t used = __atomic_load_n(&b->saved, __ATOMIC_RELAXED); + uint64_t since = + __atomic_add_fetch(&b->since_check, want, __ATOMIC_RELAXED); + if (used != 0 && since < FREE_CHECK_INTERVAL) + return 1; + __atomic_store_n(&b->since_check, 0, __ATOMIC_RELAXED); + } + /* no mapping: cannot amortise, so pay for the check every time */ + struct statvfs vfs; if (!dir || statvfs(dir, &vfs) != 0) return 1; /* cannot tell: let the session budget do the limiting */ - unsigned long avail = (unsigned long)vfs.f_bavail * (unsigned long)vfs.f_frsize; + unsigned long avail = + (unsigned long)vfs.f_bavail * (unsigned long)vfs.f_frsize; if (avail < floor + want) { snprintf(why, sizeof why, "stopped recording: %lu MB free on the store's filesystem, " "floor is %lu MB (UNDO_MIN_FREE)", (avail + (1UL << 19)) >> 20, (floor + (1UL << 19)) >> 20); - budget_stop(why); + budget_stop(dir, why); return 0; } return 1; } -static void budget_add(unsigned long n) +static void budget_add(const char *dir, unsigned long n) { - struct budget *b = budget_map(); + struct budget *b = budget_map(dir); if (b) __atomic_add_fetch(&b->saved, (uint64_t)n, __ATOMIC_RELAXED); } @@ -507,7 +557,8 @@ static int save_file(const char *abs, int need_copy, char *bak) errno = EFBIG; return -1; } - if (!budget_ok((unsigned long)st.st_size)) { + const char *dir = session_dir(); + if (!budget_ok(dir, (unsigned long)st.st_size)) { errno = ENOSPC; return -1; } @@ -520,7 +571,7 @@ static int save_file(const char *abs, int need_copy, char *bak) if (!need_copy) { REAL(link, int, const char *, const char *); if (real_link(abs, bak) == 0) { - budget_add((unsigned long)st.st_size); + budget_add(dir, (unsigned long)st.st_size); return 0; } if (errno == EEXIST) @@ -528,7 +579,7 @@ static int save_file(const char *abs, int need_copy, char *bak) /* cross-device etc: fall through to a copy under this name */ } if (copy_file(abs, bak) == 0) { - budget_add((unsigned long)st.st_size); + budget_add(dir, (unsigned long)st.st_size); return 0; } if (errno != EEXIST) diff --git a/test/e2e.sh b/test/e2e.sh index 0ff63fd..01f0508 100755 --- a/test/e2e.sh +++ b/test/e2e.sh @@ -401,5 +401,33 @@ else echo " (no cc, skipped)" fi +echo "== case 29: the session budget is shared across processes" +# The whole point of keeping the counter in a shared mapping rather than a +# static: a build forks a thousand compilers and they all bill to one +# session. Five separate rm processes here, one file each. Per-process +# counters would let every one of them through. +mkdir -p "$PLAY/procs" +for i in 1 2 3 4 5; do head -c 400000 /dev/zero >"$PLAY/procs/f$i"; done +UNDO_MAX_SESSION=1000000 run_armed \ + "rm $PLAY/procs/f1; rm $PLAY/procs/f2; rm $PLAY/procs/f3; rm $PLAY/procs/f4; rm $PLAY/procs/f5" +last=$(ls "$UNDO_DATA_DIR/sessions" | sort | tail -1) +sess=$UNDO_DATA_DIR/sessions/$last +n=$(grep -c . "$sess/journal" 2>/dev/null || echo 0) +((n > 0 && n < 5)) || fail "expected the budget to bind across processes, got $n entries" +[[ -s $sess/degraded ]] || fail "no degraded marker from the multi-process run" +du_kb=$(du -sk "$sess" | cut -f1) +((du_kb < 2000)) || fail "session grew to ${du_kb}K past a 1MB budget" + +echo "== case 30: the per-session default follows the store budget" +# UNDO_MAX_SESSION unset: the cap is half of UNDO_MAX_STORE, so the store +# has room for more than one session. A tiny store here makes it visible. +mkdir -p "$PLAY/derived" +for i in 1 2 3; do head -c 300000 /dev/zero >"$PLAY/derived/f$i"; done +UNDO_MAX_STORE=1000000 run_armed "rm $PLAY/derived/f1 $PLAY/derived/f2 $PLAY/derived/f3" +last=$(ls "$UNDO_DATA_DIR/sessions" | sort | tail -1) +sess=$UNDO_DATA_DIR/sessions/$last +grep -q UNDO_MAX_SESSION "$sess/degraded" 2>/dev/null || + fail "a 500K derived cap should have stopped a 900K delete" + echo echo "all cases passed"