Skip to content

session-update: lock records the parent $$ from a backgrounded subshell, so the stale-lock branch lets every concurrent session in #2613

Description

@exGeni

Summary

bin/gstack-session-update records its lock holder with echo $$ from inside the backgrounded subshell. In bash, $$ does not change in a subshell — it stays the parent shell's PID. The parent here is the hook script itself, which exits immediately by design, so the PID written to the lock belongs to a process that is gone milliseconds later. Every subsequent session then classifies the lock as stale, removes it, and re-acquires. The guard admits exactly the callers it exists to exclude.

The code

(
  # ... entire update body runs here ...
  if ! mkdir "$LOCK_DIR" 2>/dev/null; then
    if [ -f "$LOCK_DIR/pid" ]; then
      LOCK_PID=$(cat "$LOCK_DIR/pid" 2>/dev/null || echo 0)
      if [ "$LOCK_PID" -gt 0 ] 2>/dev/null && ! kill -0 "$LOCK_PID" 2>/dev/null; then
        rm -rf "$LOCK_DIR" 2>/dev/null          # <- treated as stale, re-acquired
        mkdir "$LOCK_DIR" 2>/dev/null || { log_entry "SKIP lock_contested"; exit 0; }
      else
        log_entry "SKIP locked_by=$LOCK_PID"; exit 0
      fi
    else
      log_entry "SKIP locked_no_pid"; exit 0
    fi
  fi
  echo $$ > "$LOCK_DIR/pid" 2>/dev/null          # bin/gstack-session-update:76
  trap 'rm -rf "$LOCK_DIR" 2>/dev/null' EXIT
  # ... git pull --ff-only --autostash, then ./setup -q ...
) >/dev/null 2>&1 &

exit 0

The script's own header states the intent that makes the recorded PID short-lived:

The entire update runs in background (forked). The hook itself exits immediately so session startup is never delayed.

Reproduction

The bash behaviour on its own:

$ echo "parent $$"; ( echo "subshell $$"; echo "BASHPID $BASHPID" ) & wait
parent 167666
subshell 167666
BASHPID 167678

And on a live install, immediately after a real run, the PID recorded in $LOCK_DIR/pid is already dead:

$ kill -0 "$(cat "$STATE_DIR/.setup-lock/pid")" || echo "already dead"
already dead

Observed consequence

Two Claude Code sessions starting close together both ran the update body concurrently. Neither logged the SKIP locked_by= that a holding lock produces, and both failed:

PULL_FAILED exit=128 reason=fatal: Cannot fast-forward to multiple branches.
PULL_FAILED exit=128 reason=fatal: Cannot fast-forward to multiple branches.

Two concurrent git pulls in one checkout race on FETCH_HEAD: git truncates it, then reopens it to append merge candidates, so a second fetch interleaving can leave more than one merge-marked line, and --ff-only aborts with exactly that message.

I want to be precise about how far this is proven. Repository configuration was ruled out as the cause: a single git pull --ff-only --autostash in the same tree exits 0, branch.<name>.merge is a single ref, fetch.all and extra merge targets are absent, and a second remote with a wildcard refspec is not consulted by an argument-less git pull. But the log alone, without a FETCH_HEAD snapshot and timing, does not prove those two hook processes were the only competitors — any other concurrent fetch in the same checkout would produce the same state. So: the lock defect is directly demonstrated; the FETCH_HEAD race is its expected consequence and the most likely explanation of these two lines, not a conclusively isolated one.

The practical effect either way is that auto-upgrade silently skips whenever sessions start together, and the install drifts behind until a session happens to start alone. It is silent because the subshell sends all stdio to /dev/null — deliberately, to avoid SIGPIPE once the hook exits — so the only trace is the analytics log.

On the fix

$BASHPID is the semantically correct value (the subshell's own PID), but I do not think it is safe as a portable one-line patch: BASHPID was introduced in Bash 4.0, and the system bash on older macOS is 3.2, where it expands to nothing and the lock file would be written empty. That lands in the SKIP locked_no_pid branch rather than anything dangerous, but it would disable auto-upgrade outright on those hosts.

Two options that avoid that, whichever fits your intent:

  • Re-invoke the script in an explicit worker mode (e.g. an internal flag) so the update body runs as its own process and its $$ is genuinely its own PID. No version dependency.
  • Use $BASHPID and declare Bash 4+ a requirement for this path, with a fallback that keeps the current behaviour rather than writing an empty PID file.

Observed on 1.67.0.0. Happy to send a PR for whichever shape you prefer.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions