Skip to content

implement cooperative heartbeat-based locking for rolling deployments - #203

Open
dembrane-sam-bot wants to merge 1 commit into
mainfrom
sam/update-heartbeat-locking
Open

implement cooperative heartbeat-based locking for rolling deployments#203
dembrane-sam-bot wants to merge 1 commit into
mainfrom
sam/update-heartbeat-locking

Conversation

@dembrane-sam-bot

Copy link
Copy Markdown
Contributor
  • What is this change?: Implements heartbeat-based file locking and cooperative standby for Cloud Run rolling deployments to prevent split-brain duplicates during startup/deployment.
  • What did Sam notice that led to this?: Rolling updates spin up a new container while the old container is still draining. The new container's _recover_from_reactions immediately re-queued in-flight tasks (left at :eyes: or :hourglass: by the draining container), creating duplicate sessions and GCS write collisions.
  • Tier?: Tier 3 (runtime)
  • Confidence?: 100% (passes all 565 runtime tests + 6 brand new heartbeat-lock tests + 45 structural eval tests).

@spashii spashii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mechanism is a no-op in production: cooperative standby never actually waits.

Failure scenario, step by step:

  1. CLOUD_RUN_INSTANCE_ID is not set anywhere in this repo (dockerfile, terraform, or code), and Cloud Run does not provide it as an env var — the instance id lives in the metadata server. In production instance_id is always "".
  2. The container runs CMD ["python3", "-m", "src.runtime.daemon"], so the daemon has the same PID in the old and new containers.
  3. The self-lock test is pid == os.getpid() and (not instance_id or instance_id == current). With equal PIDs and empty instance ids, every instance concludes any lock is its own_wait_for_previous_daemon_shutdown breaks on its first iteration.
  4. Ordering makes it worse: acquire_lock() runs at boot and unconditionally calls write_heartbeat(), overwriting the draining instance's lock with the new instance's identity before recovery's wait ever reads it. The wait then sees a lock that genuinely is its own.
  5. Net effect: recovery re-queues in-flight tasks while the old container drains — the exact duplicate-session/GCS-collision split-brain this PR targets — plus both instances now ping-pong the lock file every 5s.

The tests pass because they monkeypatch CLOUD_RUN_INSTANCE_ID and simulate differing PIDs — conditions production never has.

Suggested fix:

  • Derive identity per process at import time: _INSTANCE_ID = uuid4().hex module-level (or fetch the real instance id from http://metadata.google.internal/computeMetadata/v1/instance/id with the Metadata-Flavor: Google header). Compare on that alone — drop the PID comparison entirely; PIDs cannot distinguish containers.
  • Do the standby wait before claiming the lock, not after: wait-then-write inside the boot path, so the draining instance's heartbeat survives until it actually stops.
  • Have the draining instance stop its heartbeat loop at the top of shutdown (before waiting out workers), so the successor's 30s staleness window starts counting from drain-start rather than process-exit.

Also note: this will conflict with #222 (daemon.py, test_recovery.py) — rebase once that lands.

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.

2 participants