Skip to content

fix(save): LRU prune survives an early break; failed saves are loud (#3) - #4

Merged
emooreatx merged 3 commits into
mainfrom
fix-3-save-prune-sigpipe
Oct 8, 2026
Merged

emooreatx merged 3 commits into
mainfrom
fix-3-save-prune-sigpipe

Conversation

@emooreatx

@emooreatx emooreatx commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Closes #3

Cause (measured)

save/action.yml piped find | sort -n into a while loop that breaks once target/ is under max-size-mb. On a large target/ the break leaves sort still writing, so sort gets SIGPIPE. Under pipefail the pipeline fails, set -e kills the step before tar/oras push, and the caller's continue-on-error: true reported success. Measured on CIRISPersist (details on CIRISAI/CIRISPersist#1022): no Linux blob has been published since at least v53.1.3.

Reproduced locally with a 3000-file fixture (12356 KB, 8192 KB budget), running the original snippet under bash -e -o pipefail:

SIGPIPE before (pipe into while) after (this PR)
ignored, as on the runner sort: write failed: 'standard output': Broken pipe, exit 2, tar never reached exit 0, pruned target/ 12356KB→8192KB, reaches tar
default exit 141, tar never reached exit 0, same

Fix

  • scripts/prune.sh (cc_prune_lru): the sorted list goes to a temp file and the loop reads from it, so the break has no writer to kill. LRU semantics are unchanged: oldest mtime goes first, and pruning stops at the budget. The log line now shows real before→after numbers (the old one printed a value from the pre-subshell).
  • A failed save is loud. The inputs now reach the push script through env (no ${{ }} spliced into the script). Each stage sets STAGE. An EXIT trap turns any failure into:
    • default (fail-open): ::warning::CIRISCache: save NOT published for <ref> — <stage> failed (exit N), then exit 0
    • strict: "true" (new input): ::error::, and the step fails
  • New outputs published, reason and key. A failure after the push (the alias tail) keeps published=true. nothing to cache is now a warning too.
  • _cc_timeout: killing a watchdog that has already exited no longer trips set -e and hides the 124 behind a plain 1.
  • selftest saves with strict: "true" and asserts published. The README documents the outputs, strict, and why callers should check published and not the step outcome.

Tests

  • tests/prune_test.sh runs the prune on a fixture over budget, under bash -e -o pipefail, with SIGPIPE both default and ignored. It asserts exit 0 and that the step reaches tar with no broken pipe. It also asserts that the kept set is exactly the newest files within budget with minimal deletion, and that the loop broke early with more than 64 KiB of the list unread. It also checks that the action sources the function and calls it.
  • tests/save_step_test.sh runs the push step's script verbatim with a stub oras. Cases: a good save through the prune, login fails, prune fails, push fails, push fails with strict, nothing to cache, retag fails, and a failure after the push. It asserts the exit code, the annotation, and published/reason.
  • .github/workflows/test.yml runs both on every PR.

Mutation results (all run locally):

  • Putting back the pipe-into-while form makes prune_test fail in both SIGPIPE modes (141, and 2 with the runner's Broken pipe line). KILLED.
  • Deleting newest-first, never breaking, and over-subtracting the size: all KILLED.
  • On the step: removing the trap, ignoring strict, dropping the prune stage, exiting rc under fail-open, removing the post-publish guard, and silencing nothing-to-cache: all KILLED. The post-publish guard was only killed after I added the post-publish case.
  • Writing published=true before the push: SURVIVED. This mutant is equivalent if GITHUB_OUTPUT is last-write-wins, which is BELIEVED.

Claim status

claim status
the original snippet fails with the runner's exact line (exit 2) when SIGPIPE is ignored, and with 141 by default TESTED (local repro)
the new prune exits 0 in both modes and keeps the newest files within budget TESTED
each failure path emits its annotation and sets published/reason as described TESTED (stub oras, script extracted from action.yml)
bash -n and shellcheck are clean on the scripts and tests; actionlint is clean on the workflows TESTED. The extracted step has one pre-existing info-level SC2185 (find --version, intended).
composite outputs: still carry published=false when the inner step fails under strict BELIEVED
GITHUB_OUTPUT is last-write-wins, so the trap's published=false overrides nothing written earlier BELIEVED (the step only writes published=true after the push)
$GITHUB_ACTION_PATH/../scripts/prune.sh resolves on all three runner OSes (cygpath on Windows) Exercised by selftest on this PR, which now sets max-size-mb: "1024" (within budget, so the script is sourced but nothing is pruned). macOS has no GNU find, so the prune is skipped there by design.
the fix unblocks CIRISPersist's Linux publish BELIEVED until a persist save runs on a ref that includes this change (v1 has not moved)

Not done here: moving v1, or tagging.

🤖 Generated with Claude Code

https://claude.ai/code/session_016kQb87339uTdFWivk3j2hF

emooreatx and others added 2 commits October 7, 2026 21:06
…ud (#3)

The prune piped `find | sort -n` into a `while` loop that breaks once the
budget is met. On a large target/ that leaves sort still writing: it takes
SIGPIPE (or, on the runner, EPIPE: "sort: write failed: 'standard output':
Broken pipe", exit 2), pipefail fails the pipeline, and set -e kills the
step before tar/push. Callers' continue-on-error hid it, so every save that
took the prune path published nothing.

- scripts/prune.sh: cc_prune_lru writes the sorted list to a temp file and
  reads it, so there is no writer for the break to kill. Same LRU
  semantics; the report now shows before→after instead of a subshell value.
- save/action.yml: inputs reach the push script through env (no ${{ }}
  splices). Each stage sets STAGE; an EXIT trap turns any failure into a
  ::warning:: (fail-open, exit 0) or, with the new `strict: "true"`
  input, an ::error:: that fails the step. New outputs `published`,
  `reason`, `key`. A failure after the push keeps published=true.
  `nothing to cache` is a warning too. _cc_timeout's kill of an exited
  watchdog no longer trips set -e and masks the 124.
- selftest: strict save, and assert `published`.
- README: the outputs, strict, and why to gate on `published`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016kQb87339uTdFWivk3j2hF
- tests/prune_test.sh: a 3000-file target/ over budget with controlled
  mtimes, pruned under `bash -e -o pipefail` with SIGPIPE default and
  ignored. Asserts exit 0, the step reaches tar, no broken pipe, the kept
  set is exactly the newest files within budget with minimal deletion,
  and the loop broke early with >64 KiB of list unread. Also checks
  save/action.yml sources and calls cc_prune_lru.
- tests/save_step_test.sh: runs the push step's script verbatim with a
  stub oras: good save through the prune, login/prune/push failures,
  strict mode, nothing to cache, retag failure, and a failure after the
  push. Asserts exit code, annotation, and the published/reason outputs.
- .github/workflows/test.yml runs both.

Mutation: restoring the pipe-into-while form fails prune_test in both
SIGPIPE modes (exit 141 and exit 2 with the runner's "Broken pipe" line).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016kQb87339uTdFWivk3j2hF
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@emooreatx
emooreatx merged commit 2ea692b into main Oct 8, 2026
4 checks passed
@emooreatx
emooreatx deleted the fix-3-save-prune-sigpipe branch October 8, 2026 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant