mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-04 15:57:33 +01:00
fix(brain-sync): --publish-removals trap expands its temp paths at install, so cleanup and lock release run after the function returns (GSTA-87)
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
e66c43c572
commit
ee160980ff
3 files changed
+31
-3
No files matched your search
@@ -1,5 +1,15 @@
|
||||
# Changelog
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
**`gstack-brain-sync --publish-removals --yes` no longer prints `list: unbound variable` when it finishes, and it now removes its temp list and releases the sync lock.** The command's EXIT trap named the function's local `$list`. When a publish succeeded, the function returned before the script exited, so the trap ran after `list` had gone out of scope. Under `set -u` it stopped at the unbound variable and never reached the cleanup, which left `brain-sync-removals.*` in `$TMPDIR` and `.brain-sync.lock.d` in `~/.gstack` until a later run cleared the lock as stale (found by GSTA-44).
|
||||
|
||||
### Fixed
|
||||
- **`bin/gstack-brain-sync`**: `--publish-removals` writes the temp file paths into its EXIT/INT/TERM trap when it installs the trap (quoted with `printf %q`), so the cleanup and lock release work however the function ends. The drain's trap in `--once` also names locals, but every path in that function ends with `exit` while they are still in scope, so it's unchanged. The other trap sites (the drain's lock-only trap and `gstack-brain-restore`'s, which uses a global) don't have the bug.
|
||||
|
||||
### Tests
|
||||
- `test/brain-sync-removals.test.ts`: a new case runs `--publish-removals`, then `--publish-removals --yes` twice (publishing, then nothing to publish) with a private `TMPDIR`. After each run, stderr has no `unbound variable`, the exit status is 0, `TMPDIR` is empty and the lock directory is gone. It failed before the fix.
|
||||
|
||||
## [1.91.191.0] - 2026-10-25
|
||||
|
||||
**`gstack-version-bump write --changelog` moves the branch's `[Unreleased]` entry under the new version, and /ship Step 12 stops treating a provisional queue pick as a verified slot.** Stamping by hand meant writing VERSION with `gstack-version-bump write` and then renaming the `## [Unreleased]` heading yourself, because only `gstack-restamp` re-headed the CHANGELOG. Separately, when `gstack-next-version` could not read some open PRs' VERSION it returned `complete:false` and a provisional pick, and Step 12 printed the warning and then wrote that version as if the queue had been checked (left over from #3258, noted in GSTA-45).
|
||||
|
||||
@@ -1068,7 +1068,7 @@ subcmd_publish_removals() {
|
||||
sync_active || { echo "BRAIN_SYNC: artifacts sync is not active; nothing to publish" >&2; exit 1; }
|
||||
gstack_brain_lock_acquire "$GSTACK_HOME" try || { echo "BRAIN_SYNC: another sync is running; try again" >&2; exit 1; }
|
||||
list=$(mktemp "${TMPDIR:-/tmp}/brain-sync-removals.XXXXXX") || exit 1
|
||||
trap 'rm -f "$list" "$list.gone"; gstack_brain_lock_release' EXIT INT TERM
|
||||
trap "rm -f $(printf '%q ' "$list" "$list.gone"); gstack_brain_lock_release" EXIT INT TERM
|
||||
removal_candidates "$list" || { echo "BRAIN_SYNC: could not list removed files" >&2; exit 1; }
|
||||
n=$(grep -c . "$list" 2>/dev/null || true)
|
||||
if [ "$n" -eq 0 ]; then
|
||||
|
||||
@@ -16,9 +16,9 @@ const ROOT = path.resolve(import.meta.dir, '..');
|
||||
let home: string;
|
||||
let remote: string;
|
||||
|
||||
function run(argv: string[]) {
|
||||
function run(argv: string[], extraEnv: Record<string, string> = {}) {
|
||||
const r = spawnSync(path.join(ROOT, 'bin', argv[0]), argv.slice(1), {
|
||||
env: { ...process.env, HOME: home, GSTACK_HOME: home },
|
||||
env: { ...process.env, HOME: home, GSTACK_HOME: home, ...extraEnv },
|
||||
encoding: 'utf-8',
|
||||
timeout: 60_000,
|
||||
});
|
||||
@@ -145,6 +145,24 @@ describe('artifacts_sync_removals', () => {
|
||||
expect(run(['gstack-brain-sync', '--publish-removals', '--yes']).stdout).toContain('no removals to publish');
|
||||
});
|
||||
|
||||
test('--publish-removals cleans up its temp files and lock on every exit path', () => {
|
||||
syncAll(['projects/p/designs/a.md', 'projects/p/designs/b.md']);
|
||||
fs.rmSync(path.join(home, 'projects/p/designs/a.md'));
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'brain-removals-tmp-'));
|
||||
try {
|
||||
for (const argv of [['--publish-removals'], ['--publish-removals', '--yes'], ['--publish-removals', '--yes']]) {
|
||||
const r = run(['gstack-brain-sync', ...argv], { TMPDIR: tmp });
|
||||
expect(r.stderr).not.toContain('unbound variable');
|
||||
expect(r.status).toBe(0);
|
||||
expect(fs.readdirSync(tmp)).toEqual([]);
|
||||
expect(fs.existsSync(path.join(home, '.brain-sync.lock.d'))).toBe(false);
|
||||
}
|
||||
expect(remoteFiles()).toEqual(['projects/p/designs/b.md']);
|
||||
} finally {
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('config rejects values other than on and off', () => {
|
||||
const r = run(['gstack-config', 'set', 'artifacts_sync_removals', 'yes']);
|
||||
expect(r.stderr).toContain("artifacts_sync_removals 'yes' not recognized");
|
||||
|
||||
Reference in new issue
Block a user