fix(settings-hook): retry a lock released between mkdir and the existence check instead of dropping the mutation

This commit is contained in:
garrytan committed 2026-10-04 08:21:37 +00:00
1 parent fcb4024fe2
commit 8895ed7b1f
3 files changed
+41 -6

No files matched your search

+1
View File
@@ -57,6 +57,7 @@ The first two rows are the ones to care about: a review that read nothing, or fo
- **Required team mode also blocks Copilot CLI's skill tool.** Re-run `gstack-team-init required` to upgrade the project hook.
- **`gstack-qa-evidence capture` prints a `revalidate` list after your inputs change**, naming the probes to rerun on current inputs before `materialize`; `materialize` says its verdict is final for that report root, so rerun the listed probes first.
- **/gstack-upgrade asks you to paste the absolute path Step 2 prints**; every later step checks it is inside gstack's checkout before running git.
- **Two setups writing settings.json at once both land.** A writer whose lock attempt raced a holder releasing it used to skip its hook change (exit 5); it now retries once and only reports `cannot create lock` when the lock directory truly cannot be created.
- **/plan-ceo-review prints its mode handoff** by running `gstack-ceo-mode-handoff` right after the mode is chosen (by you or by a saved preference), so the selected mode and approved decisions always appear before the review continues.
- **Browser setup names the platform when Aside is absent:** the probe prints `NEEDS_ASIDE: <OS>` (was `NEEDS_ASIDE`), and the macOS-only download pitch trusts that line instead of re-checking the OS. `GSTACK_PLATFORM` overrides it for tests and unusual hosts; set it in your shell, never a project `.env`.
+8 -6
View File
@@ -337,7 +337,7 @@ _release_lock() {
_acquire_lock() {
# GSTACK_SETTINGS_LOCK_TIMEOUT_MS: test-only override for the give-up
# (a contention test should not stall the suite for 10 real seconds).
local waited_ms=0 token stale mtime now
local waited_ms=0 token stale mtime now vanished=0
local give_up_ms="${GSTACK_SETTINGS_LOCK_TIMEOUT_MS:-10000}"
local stale_after_s=30 # lock older than this belongs to a crashed holder
local poll_ms=50 # retry cadence; sleep below derives from this
@@ -351,15 +351,17 @@ _acquire_lock() {
trap 'exit 143' TERM
return 0
fi
# mkdir failed but no lock dir exists: NOT contention (unwritable parent,
# read-only fs, missing directory) -- waiting cannot help, so give up
# loudly now instead of spinning out the full timeout. (Tiny race: a
# contender could acquire+release between our mkdir and this check; that
# transient reads as an environment failure and the next run converges.)
# mkdir failed but no lock dir exists: either a holder released between
# our mkdir and this check, or NOT contention (unwritable parent, read-only
# fs, missing directory). Retry mkdir once; a second failure with still no
# lock dir is the environment, and waiting cannot help, so give up loudly
# now instead of spinning out the full timeout.
if [ ! -e "$_LOCK_DIR" ]; then
if [ "$vanished" -eq 0 ]; then vanished=1; continue; fi
echo "gstack-settings-hook: cannot create lock $_LOCK_DIR (unwritable parent?) -- skipping this mutation, exit 5" >&2
return 1
fi
vanished=0
# Stale takeover: atomic rename means exactly one contender wins; the
# loser loops and re-contends against the winner's fresh mkdir.
# GNU stat (-c %Y) first: on Linux, BSD-style `stat -f %m` prints a
+32
View File
@@ -102,6 +102,38 @@ describe('settings hook preserves the resolved settings target', () => {
expect(fs.existsSync(settings + '.lock')).toBe(false);
});
// Ubicloud run 21: a contender's lock released between this writer's failed
// mkdir and its existence check, and the writer dropped its mutation (exit 5).
test.each([
['a lock released between mkdir and the check is retried', 'once', 0],
['a lock that can never be created still fails fast', 'always', 5],
] as const)('%s', (_name, failures, status) => {
const bin = path.join(root, 'bin-vanish');
fs.mkdirSync(bin);
const realMkdir = spawnSync('which', ['mkdir'], { encoding: 'utf8', timeout: 5_000 }).stdout.trim();
expect(realMkdir).not.toBe('');
fs.writeFileSync(path.join(bin, 'mkdir'), [
'#!/bin/bash',
'if [ "$1" = "$LOCK_EXPECTED" ]; then',
' if [ "$LOCK_FAILS" = always ] || [ ! -e "$LOCK_MARK" ]; then : > "$LOCK_MARK"; exit 1; fi',
'fi',
'exec "$REAL_MKDIR" "$@"',
].join('\n') + '\n', { mode: 0o755 });
const result = spawnSync('bash', [hook, ...addArgs()], {
env: { ...env(), PATH: `${bin}:${process.env.PATH}`, LOCK_EXPECTED: target + '.lock', LOCK_FAILS: failures,
LOCK_MARK: path.join(root, 'lock-failed-once'), REAL_MKDIR: realMkdir, GSTACK_SETTINGS_LOCK_TIMEOUT_MS: '2000' },
encoding: 'utf8', timeout: 10_000,
});
expect(result.status, result.stderr).toBe(status);
const stored = fs.readFileSync(target, 'utf8');
if (status === 0) expect(JSON.parse(stored).hooks.PostToolUse[0]._gstack_source).toBe('link-test');
else {
expect(result.stderr).toContain('cannot create lock');
expect(stored).not.toContain('link-test');
}
expect(fs.existsSync(target + '.lock')).toBe(false);
});
test('dangling and cyclic links are rejected without replacing them', () => {
fs.unlinkSync(target);
const dangling = run(addArgs());