implement cooperative heartbeat-based locking for rolling deployments - #203
Open
dembrane-sam-bot wants to merge 1 commit into
Open
implement cooperative heartbeat-based locking for rolling deployments#203dembrane-sam-bot wants to merge 1 commit into
dembrane-sam-bot wants to merge 1 commit into
Conversation
dembrane-sam-bot
enabled auto-merge
July 30, 2026 05:56
spashii
requested changes
Aug 9, 2026
spashii
left a comment
Member
There was a problem hiding this comment.
The mechanism is a no-op in production: cooperative standby never actually waits.
Failure scenario, step by step:
CLOUD_RUN_INSTANCE_IDis 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 productioninstance_idis always"".- The container runs
CMD ["python3", "-m", "src.runtime.daemon"], so the daemon has the same PID in the old and new containers. - 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_shutdownbreaks on its first iteration. - Ordering makes it worse:
acquire_lock()runs at boot and unconditionally callswrite_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. - 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().hexmodule-level (or fetch the real instance id fromhttp://metadata.google.internal/computeMetadata/v1/instance/idwith theMetadata-Flavor: Googleheader). 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
_recover_from_reactionsimmediately re-queued in-flight tasks (left at:eyes:or:hourglass:by the draining container), creating duplicate sessions and GCS write collisions.