Skip to content

fix(demo): report a failed cleanup and stop the writer that most likely raced it - #1166

Merged
MongLong0214 merged 1 commit into
mainfrom
fix-1163-demo-cleanup
Oct 3, 2026
Merged

MongLong0214 merged 1 commit into
mainfrom
fix-1163-demo-cleanup

Conversation

@MongLong0214

Copy link
Copy Markdown
Owner

Closes #1163

What the evidence was

One CI occurrence: check (24) on the canonical pull request for #1154,
test/demo.test.ts > temporary directory is gone after a simulated crash,
naming a leftover commitlore-demo-* directory. check (22.23.2) passed in the
same run, re-running the failed job on the same commit passed, and the full
suite passed locally on that commit.

And nothing said why. cleanup caught the rmSync error and discarded it, so
that occurrence carried no errno — a race, a permission, and a full disk were
indistinguishable from each other and from a removal that never ran.

The change

  1. The failure is reported. The directory and the reason, on stderr, never
    rethrown: cleanup also runs from a signal handler, where a throw has
    nowhere to go, and on the crash path it must not mask the error it is
    unwinding. Error.message from fs already carries the errno and the
    syscall, which is the part that says what kind of failure it was.
  2. The most plausible writer is removed. The demo's repository sets
    gc.auto=0 and maintenance.auto=false, so git commit cannot leave
    background maintenance writing inside a directory about to be removed. A
    throwaway repository should not start one regardless.
  3. rmSync gets maxRetries, which covers the errno set a concurrent
    writer produces (EBUSY, ENOTEMPTY, EPERM, …).

What this does not claim

It does not prove the cause, and the commit record says so — Unverified:
whether background maintenance was the writer, Certainty: tentative, and
Ruled-out: calling the retry a root fix | a retry makes a race survivable without showing that a race is what happened.

Verification

  • New test reports a cleanup failure instead of discarding it (bug-issue-1163). The failure is injected through a mocked rmSync, because
    a real race is not reliably reproducible and a test that waited for one would
    be the flake it is meant to explain. It asserts the stderr line names the
    directory and EACCES, that the simulated crash error still reaches the
    caller, and — as arrival — that the injection really did stop the removal.
  • The same test then reads gc.auto and maintenance.auto off the directory
    the failed cleanup left behind, so the config claim is checked against the
    repository the demo actually built.
  • Negative control per half: without the stderr line the test fails on
    expected '' to contain 'could not remove'; without the config the
    git config --get gc.auto lookup fails. Both halves are load-bearing.
  • Full suite: 212 files, 4615 passed, 4 skipped. tsc --noEmit clean.

dist/ is absent on purpose: canonical-merge.yml rebuilds it and regenerates
the manifest.

…kely raced it

The crash-cleanup test went red once in CI naming a leftover
`commitlore-demo-*` directory, passed on a re-run of the same commit, and
passed on the other node leg of the same run. Nothing said why: `cleanup`
caught the `rmSync` error and discarded it, so the one occurrence carried no
errno, and a race, a permission and a full disk were indistinguishable from
each other and from a removal that never ran.

Three changes, in order of what they are worth:

- The failure is reported — the directory and the reason, on stderr, never
  rethrown. `cleanup` also runs from a signal handler, where a throw has
  nowhere to go, and on the crash path it must not mask the error it is
  unwinding.
- The demo's repository sets `gc.auto=0` and `maintenance.auto=false`, so
  `git commit` cannot leave background maintenance writing inside a directory
  that is about to be removed. That is the mechanism the occurrence is most
  consistent with, and a throwaway repository should not start one regardless.
- `rmSync` gets `maxRetries`, which covers the errno set a concurrent writer
  produces.

What this does not do is prove the cause. The new test injects the failure
through a mocked `rmSync` because a real race is not reliably reproducible, so
what it pins is the reporting: a cleanup that fails names the path and the
errno. The next occurrence will say what this one could not.

Limit: the evidence is one CI occurrence that passed on re-run and on the other node leg of the same run, so the cause was never observed
Ruled-out: waiting for a reproduction before changing anything | a real removal race is not reliably reproducible, and the discarded errno is the reason the one occurrence could not be read at all
Ruled-out: calling the retry a root fix | a retry makes a race survivable without showing that a race is what happened, and the cause is still unobserved
Ruled-out: failing the command when cleanup fails | a leftover temporary directory is a leak, and a non-zero exit would make `commitlore demo` fail for something the user cannot act on
Warn: the new test injects the failure through a mocked `rmSync`, so it pins the reporting and not the cause; if this goes red again the stderr line carries the errno -- read it before changing anything else
Blast: local
Undo: easy
Certainty: tentative
Unverified: whether background maintenance was the writer; the repository config change removes that mechanism and nothing observed it happening
Record-Id: r-cleanupreport1163
Provenance: drafted
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown

CommitLore — record lint

Trailers: clean — 1 commit in origin/main..99e2d52786cd55d069b74bd5f20b099fab1c22ba
Active constraints: 5 limits · 10 ruled-out · 2 warnings — from 5 records over 2 changed paths

Active constraints for the paths this PR touches

Limits (5)

  • r-cleanupreport1163 99e2d52 — the evidence is one CI occurrence that passed on re-run and on the other node leg of the same run, so the cause was never observed
  • r-demotruthandassets f6424c7 — this is PR 1 of the README SSOT and touches no README. The hero is still 840x340 with 18px labels, the mark is not referenced anywhere yet, and the GIF sits unused until the English README lands -- deliberately, because a README referencing an asset that does not exist is what the SSOT forbids
  • r-demostory505 8016424 — the demo is one scenario, so it shows supersession and not expiry, path scope, or trust grading; a reader who wants those still has to read past the image
  • r-owntmproot 6543870 — the demo still defaults to the shared tmpdir, so concurrent commitlore demo runs still create sibling directories there -- that is deliberate, and it is safe only because nothing now asserts over that namespace
  • r-t1011demo 1c0fc0c — the scene is one fixed pair of decisions, so it demonstrates the mechanism rather than measuring how often it matters

Ruled out (10)

  • r-cleanupreport1163 99e2d52 — waiting for a reproduction before changing anything | a real removal race is not reliably reproducible, and the discarded errno is the reason the one occurrence could not be read at all
  • r-cleanupreport1163 99e2d52 — calling the retry a root fix | a retry makes a race survivable without showing that a race is what happened, and the cause is still unobserved
  • r-cleanupreport1163 99e2d52 — failing the command when cleanup fails | a leftover temporary directory is a leak, and a non-zero exit would make commitlore demo fail for something the user cannot act on
  • r-demostory505 8016424 — keeping the cache scenario and rewriting the README paragraph to match it | the pricing example is the one that names a cost a reader has paid, and the image should follow the argument rather than the argument follow the image
  • r-demostory505 8016424 — hand-editing the SVG to say pricing | the recording would then be a drawing of output the command does not produce
  • r-owntmproot 6543870 — An env override such as COMMITLORE_DEMO_TMPDIR | it moves the production default off the call site, where an ambient variable can redirect a real run and nothing in the code reads as changed
  • r-owntmproot 6543870 — Keeping the before/after delta and widening it | the delta narrows the window rather than closing it, and the directory that turned this red was created inside the window it leaves open
  • r-owntmproot 6543870 — Deleting the two tests or dropping the prefix filter to make them pass | the property is real and cheap to hold, so that trades a flaky true signal for a permanent blind spot over cleanup after a crash
  • r-t1011demo 1c0fc0c — seeding the demo into the user's repository behind a confirmation | a demo that can modify the thing it is explaining is not a demo, and a confirmation prompt is not a substitute for being unable to
  • r-t1011demo 1c0fc0c — recomputing lifecycle inside the demo to keep it self-contained | a second implementation of the rule would drift from the one under test, and the demo would stop being evidence

Warnings (2)

  • r-cleanupreport1163 99e2d52 (claim) — the new test injects the failure through a mocked rmSync, so it pins the reporting and not the cause; if this goes red again the stderr line carries the errno -- read it before changing anything else
  • r-demostory505 8016424 (claim) — the fixture's record ids are now r-price01/r-price02, which read as if they came from the repository's own history rather than from a temporary demo repo -- they do not, and nothing else in the tree uses them

git log --follow accepts exactly one pathspec, so renames are not followed for 2 paths; query one path at a time to follow its rename chain

Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR.

MongLong0214 added a commit that referenced this pull request Oct 3, 2026
@MongLong0214
MongLong0214 merged commit 3b49d81 into main Oct 3, 2026
10 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

demo crash-cleanup test is flaky, and the cleanup error that would explain it is swallowed

1 participant