feat: background job log size ceiling, hardened by an adversarial pass - #12
Merged
Merged
Conversation
The write side of a job log was unbounded: stdout+stderr went straight to the
log fd, so a runaway job (`yes`, a spew loop, a pathological build) could fill
the disk. The read side was already bounded, so the fix is a ceiling enforced
INSIDE the detached process tree — it has to hold after pi exits or crashes.
Wrapper (extension/index.ts): the command's output is tee'd into a byte counter
and piped through `head -c CAP` into the log, with `cat >/dev/null` draining the
rest, so the producer never gets SIGPIPE and the job still exits with its real
code (the code travels through a file — a pipeline's `$?` is the reader's). The
count comes from the tee'd copy, not from "what head left behind": head
over-reads into its buffer, so the remainder under-reports (a 1500-byte stream
capped at 1000 leaves 0, not 500), and the log fd cannot be re-opened for
sizing (it is O_WRONLY). If mkfifo fails the wrapper falls back to uncapped —
losing output is worse than losing the ceiling.
- `maxLogBytes` / `PI_BGRUN_MAX_LOG_BYTES`, default 64 MiB, 0 = unlimited (a
blank env var does NOT mean unlimited). Read per job at spawn time.
- The cap keeps the FIRST N bytes; there is no portable in-tree tail cap, and a
ring buffer would break the marker-at-tail contract every reader relies on.
- Truncation is never silent: the log gets `[pi-bgrun] output truncated at N
bytes (first N bytes kept)` on the line before the exit marker, and readers
filter it like the exit marker. The wake's Stats line says so, a configured
digest scorecard is SKIPPED rather than scored against a log that lost its end
(summaries and failure lists live there), and bgtail/bggrep append a note and
report truncatedAtBytes in their details.
- bgtail/bggrep take `bytes` (2 MiB default, max 64 MiB). Widening only changes
how much is SCANNED — the returned text stays capped by the condenser, so a
wider window costs latency and memory, not context. A changed window resets
bgtail's delta instead of reporting never-seen lines as new.
- countLogLines is bounded (64 MiB, else the stat is omitted) and takes its tail
offset from fstatSync; the sweep reclaims `.tmp-*.{log,ec,fifo,cnt}` staging
files; tailBookmarks is evicted when a log is removed and capped at 1000.
Tests: 18 new, covering the cap boundary (exact-cap is not "truncated"),
exit-code preservation through the pipeline, a multi-megabyte flood not dying of
SIGPIPE, no staging strays, digest/stat/last-line honesty, the `bytes` window,
and config normalization.
Project-scoped logs are the model: one shared `~/.pi-bgrun/jobs` means cross-project clutter, ambiguous `bgstatus` scope, cleanup reaching into other projects' runs, and logs outside the workspace that project-sandboxed analysis tools would have to be handed by absolute path. Staged retirement, nothing breaks today: - `PI_BGRUN_GLOBAL_DIR` and the `~/.pi-bgrun/jobs` destination are marked deprecated in the env table, the Roadmap and the run-bg skill, with a dedicated section covering why, what is lost (cross-project discovery) and how to migrate. Removal is reserved for a future major. - An existing absolute `jobsDir` / `PI_BGRUN_DIR` keeps working exactly as before, and a cwd with no project root still falls back to the global dir — the alternative is scattering logs into an arbitrary cwd. Also resolves home through a HOME-first `homeDir()` helper (`process.env.HOME || homedir()`) for the global jobs dir, `~` expansion, the project-root exclusion and the user-config path. Node's os.homedir() already behaves this way; Bun's ignores HOME entirely, which is why the suite had to lean on PI_BGRUN_GLOBAL_DIR (and why resolveConfig carries a userConfigPath test seam). Tests now pin HOME, so retiring the deprecated knob will not require rewriting them. Green under both runners: bun and `node --test` (183/183 each).
Bun already ran every step: `bunx tsc` covers `npx tsc`, `npm test` just shells `bun test`, and `bun pm pack --dry-run` prints the same packed file list plus `Total files: N` that the tarball-allowlist guard needs (the guard's parse is re-pointed at Bun's casing). The setup-node step was installing a toolchain nothing used. What this gives up: the V8/worker_threads side of bggrep's bounded matching is no longer exercised in CI. Bun implements node:worker_threads, so the worker MECHANISM is still covered — only V8's backtracking behaviour is not. Insurance is a comment in the workflow: `node --test extension/index.test.ts` passes 183/183 in ~16s (Node 24.15); re-run it by hand after touching the worker path.
…ical test `bggrep: a pathological regex returns within the budget instead of hanging` never produced a pathological match. The log line was 60 000 "a"s plus a "b", but bggrep pre-truncates every line to BGGREP_LINE_CAP = 10 000 BEFORE matching, so the "b" that forces backtracking was cut away and `^(a+)+$` matched the remaining all-"a" line in 0ms on both engines. The failing character now sits inside the cap window, which makes the test real: on Node/V8 the same input spends the full 2s budget and requires the worker to be terminated (2004ms measured), while on Bun/JSC it answers in ~250ms. That split is the point: V8 backtracks exponentially where JSC does not — `^(a+)+$` over 100 "a"s + "!" hangs Node past 10s and returns on Bun in ~250ms — so an input-driven catastrophic pattern cannot assert the terminate() behaviour portably. matchLinesWithBudget() therefore takes an injectable worker body (defaulting to BGGREP_WORKER_SOURCE) and is exported for tests: a worker that never returns proves the budget still ends it, on any engine, in ~300ms. No behaviour change: the parameter is optional and only tests pass it.
matchLinesSyncBounded runs only where node:worker_threads is unavailable — never on Node or Bun — so nothing exercised it: a regression there would ship silently and surface as "bggrep behaves differently in that environment", which is the worst way to find out. Exported for tests (like the other seams), and covered: - Parity with the worker path: same matchIdx for matches, misses, the per-line cap (a NEEDLE past cap 10 must be missed by both), and the same `invalid` outcome for a bad pattern. Parity is the contract — the fallback exists to degrade, not to disagree. - Both budget guards: fired before the first line when the budget is already spent, and re-checked mid-scan (a 0ms budget over 300 000 lines aborts instead of finishing the corpus). That guard is what keeps an unbounded scan off the main thread, which is the whole reason the worker exists. Suite: 186 pass / 0 fail under bun, and 186/186 under `node --test`.
bun publish has no OIDC trusted-publishing or provenance support — it authenticates with a long-lived NPM_CONFIG_TOKEN — while this workflow relies on id-token: write and stores no token at all. Recorded next to the Node setup so a future 'make CI bun-only' sweep does not quietly downgrade the release path.
An adversarial pass over the committed ceiling (e7f0669, the `tee | head -c | cat` shape) reproduced eight defects. Each is now fixed and pinned by a test; with this test file run against e7f0669, 14 tests fail. - A ceiling literal `sh` cannot parse emptied the log: `1e21` reached the wrapper as `head -c 1e+21` (errors, writes nothing), and a fractional cap truncated to 0, which means unlimited — so the cap silently vanished. Normalize to an integer in 1..MAX_SAFE_INTEGER before it is interpolated. - A failed `mkfifo` ran uncapped *silently*: the fallback now prints a notice and sets the marker's `nocap` flag, so "ceiling unavailable" is not indistinguishable from "output was that small". - Truncation was inferable from printable text (a command echoing a notice-shaped line could fake a capped log). The state now comes from the writer: a fifo drain reports the byte budget, and the flag rides the exit marker's own line. - A backgrounded child held the job open: as a pipeline stage the wrapper waited for pipe EOF, so `sleep 30 & echo done` produced no wake for 30s — never, for a daemon. Completion now follows the command's pid, with a bounded drain grace. - The widest `bytes` window could not cover a capped log: it was clamped to the ceiling itself, which a capped log always exceeds. `readWindowMax()` is the ceiling plus the wrapper's overhead. - A window could be materialized line-by-line without bound (>3 GB of RSS for 64 MiB of one-character lines, i.e. an OOM on the log class the ceiling exists for). Bound to the last 500k lines, with a labelled note so a trimmed window is never reported as a plain "none" — and only report that note when the line limit actually trimmed bytes. - `countLogLines` scoped its scan bound to the cap, which would drop the line count for every capped job; it takes the tail offset from `fstatSync` and bounds the scan by the window bound instead. - Staging files outlived the sweep, while sweeping them on mtime alone would delete a running job's staged fifo. Sweep by mtime, skipping any stem whose `.pid` is a live process.
The tarball that ships is built by `npm publish` (release.yml, OIDC trusted publishing), so the allowlist must be checked with the packer that actually produces it. Both packers emit the same 7 files today; keeping that true is the point of the step. npm is preinstalled on the runner, so ci.yml stays bun-only for every other step. Also fix the guard's silent-pass hole: a failing `bun pm pack` printed `? files` and the step still went green, because the leak greps below cannot fail on an empty listing. It now fails on a non-zero pack status or a missing file count, and the count is extracted with sed (npm lowercases "total files:", and GNU `grep -oP` was never portable).
…ne bounds) Reading a capped log is now honestly bounded and honestly described: the flag lives in the exit marker rather than in printable text (so a command echoing a notice-shaped line is content, not a signal), the widest `bytes` window is the ceiling plus 4 KiB so it can span the whole kept log, a window is additionally limited to its last 500k lines, and when that bites bgtail/bggrep say so.
The design record for the ceiling and the seven review passes behind it: why the cap keeps the first bytes, why the mechanism lives inside the detached tree, the fifo/copier shape and the alternatives it beat, and each defect an adversarial pass reproduced against the previous shape. Not part of the published tarball (package.json files[]).
The brief at the repo root was a process artifact — status line, commit list, time estimate, "re-verified against <sha>" — exactly the content that rots once it is tracked. Its substance now lives in docs/log-size-ceiling.md: the problem and the five constraints, the shipped fifo/copier shape with each load-bearing oddity explained, the measurement table that rules out the cheap truncation detectors, the visible-and-unforgeable contract, reader bounds, config normalization, the defects already paid for, and the rejected alternatives. docs/ is the home for design notes of this kind from here on. Also fix what the docs claimed the truncation notice looks like. README and the run-bg skill promised `[pi-bgrun] output truncated at <N> bytes (first <N> bytes kept)`; the wrapper has never printed that. The shipped lines are `__BGRUN_TRUNC__ output truncated: kept the first <N> bytes` and, when the ceiling could not be installed, `__BGRUN_NOCAP__ log ceiling unavailable`. Both are reserved `__BGRUN_*__` lines, identified by the marker's flag rather than by matching text — an agent following the skill was looking for a string that does not exist. And describe both wrapper paths in "How it works", which still showed only the uncapped one-liner.
`.pi/pi-bgrun.json` is per-contributor state, not repo policy: it is read only for a trusted project, it changes what every bgrun job in the checkout does (`showCompletedJobs`, plus a digest that shells out at wake time), and two of the three keys it usually carries are noise (`jobsDir` restates the default). So the repo now ignores its own copy and the maintainer setup lives in docs/dogfooding.md — the config to copy, what each key does, how to see the scorecard on a real run, and how config layering unwinds it. The doc is the live file's own content, so it stays honest by being exercised: both of this maintainer's worktrees run it. README's configuration section says plainly why such a file is not shared.
CI report
Ref: |
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.
What
Cap a background job's log so a runaway job (
yes, a spew loop, a pathological build) cannot fill the disk, and make every reader honest about a log that lost its end. The branch also carries the follow-up work it accumulated: deprecating the machine-global jobs dir, consolidating CI on Bun, and twobggreptest gaps.The ceiling
stdout+stderr went straight to the log fd with no write bound. The read side was already bounded; the write side is now capped too, inside the detached process tree so it still holds after pi exits or crashes:
maxLogBytes/PI_BGRUN_MAX_LOG_BYTES, default 64 MiB;0= unlimited (the previous behavior, verbatim wrapper path).141.perl, elsedd/head) that caps, flags, and drains; the exit code travels out-of-band, and completion follows the command's pid, not the stream — sosleep 30 & echo donewakes immediately instead of hanging.__BGRUN_TRUNC__ output truncated: kept the first <N> bytesbefore the marker, and the marker itself carriestruncated=<N>(ornocap=1whenmkfifowas unavailable and the job ran uncapped). Reserved__BGRUN_*__lines; readers classify by the marker flag, never by matching text, so a command echoing a notice-shaped line cannot fake a capped log.log truncated at <N>,bgtail/bggrepappend a note and reporttruncatedAtBytes, and a configured digest scorecard is skipped rather than scored against a log that lost its end.byteswindow is the ceiling plus the wrapper's overhead (a capped log always exceeds the cap), and a window is additionally limited to its last 500 000 lines — 64 MiB of one-character lines was measured at >3 GB of RSS. Both limits announce themselves.Behavior change: the cap is on by default.
maxLogBytes: 0restores uncapped logs.Ten defects an adversarial pass reproduced, all fixed and pinned
shcannot parse emptied the log (1e21→head -c 1e+21); a fractional cap truncated to0, which means unlimited. → integer normalization, floor of 1.mkfiforan uncapped silently →__BGRUN_NOCAP__notice +nocap=1.countLogLinesbounded its scan by the cap, dropping the line count for every capped job →fstatSynctail offset, window-max bound..pidis alive.? filesand the step stayed green).Measured, not asserted: with this branch's test file run against the pre-round source (
e7f0669, thetee | head -c | catshape), 14 tests fail — including the backgrounded-child test timing out, which is the old hang.Also in this branch
PI_BGRUN_GLOBAL_DIR/~/.pi-bgrun/jobsare documented as deprecated with a migration path. No behavior change today — an absolutejobsDir/PI_BGRUN_DIRstill works.bggrep's bounded matching is verified by hand withnode --test; two new tests close the worker-abort and sync-fallback gaps.npm pack, the packer that actually publishes (release.yml usesnpm publishwith OIDC); both packers emit the same 7 files today, and the step now fails loudly instead of reporting? files.docs/log-size-ceiling.md(design rationale, measurements, rejected alternatives, the defects above as why the wrapper is not simpler),docs/dogfooding.md(the maintainers' local config, gitignored here — per-contributor state, not repo policy), and README/SKILL updated to the real notice literals, which they had never matched.Verification
bun test extension/index.test.ts— 193/193 (what CI runs)node --test extension/index.test.ts— 193/193 (Node 24.15, run by hand; the V8 path)tsc --noEmitclean,actionlintcleannpm pack --dry-runguard exercised against both packers: 7 files, no test or docs leak