Repository navigation
Conversation
…ob requests Add cloudscheduler_deduplicate_job_requests, a Cloud Run function that does a Cloud Scheduler job's work once per scheduled run, even when Cloud Scheduler delivers the same run more than once. The function identifies a run by the job name and the X-CloudScheduler-ScheduleTime header, which stays the same across retries. It claims the run in a Firestore transaction with a lease: - A completed run is skipped. - An active claim returns 409. - An expired claim from a crashed attempt can be claimed again. If the work fails, the function releases the claim so that Cloud Scheduler's retry can process the run. This rewrites the sample proposed in #14702 and keeps its region tag. Unit tests use an in-memory fake Firestore. Integration tests run against a real Firestore database and are skipped unless RUN_INTEGRATION_TESTS is set. Co-authored-by: Camie Kim <12733285+camiekim@users.noreply.github.com>
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a Cloud Function to deduplicate Cloud Scheduler job requests using Firestore for distributed locking and lease management. The review feedback highlights a potential race condition where a slow worker whose lease has expired could inadvertently overwrite or delete a lease claimed by a subsequent retry. To address this, the reviewer suggests using the lease expiration timestamp as a fencing token and performing both the completion and release operations transactionally, providing concrete code suggestions for the implementation and corresponding test updates.
| @firestore.transactional | ||
| def claim_run( | ||
| transaction: firestore.Transaction, | ||
| run_ref: firestore.DocumentReference, | ||
| now: datetime.datetime, | ||
| ) -> str: | ||
| """Claims the run unless it's done or another attempt holds the lease.""" | ||
| snapshot = run_ref.get(transaction=transaction) | ||
| if snapshot.exists: | ||
| run = snapshot.to_dict() | ||
| if run["status"] == "completed": | ||
| return "completed" | ||
| if run["lease_expires"] > now: | ||
| return "in_progress" | ||
| transaction.set( | ||
| run_ref, | ||
| { | ||
| "status": "in_progress", | ||
| "lease_expires": now + LEASE, | ||
| "expire_at": now + RETENTION, | ||
| }, | ||
| ) | ||
| return "claimed" |
There was a problem hiding this comment.
To prevent race conditions where a slow worker's attempt completes or fails after its lease has expired and been claimed by another worker, we should perform both the completion and release operations transactionally. By using the lease_expires timestamp as a fencing token, we can ensure that we only update or delete the document if our specific attempt still holds the active lease.
Let's update claim_run to accept lease_expires and add transactional helper functions complete_run and release_run.
@firestore.transactional
def claim_run(
transaction: firestore.Transaction,
run_ref: firestore.DocumentReference,
now: datetime.datetime,
lease_expires: datetime.datetime,
) -> str:
"""Claims the run unless it's done or another attempt holds the lease."""
snapshot = run_ref.get(transaction=transaction)
if snapshot.exists:
run = snapshot.to_dict()
if run["status"] == "completed":
return "completed"
if run["lease_expires"] > now:
return "in_progress"
transaction.set(
run_ref,
{
"status": "in_progress",
"lease_expires": lease_expires,
"expire_at": now + RETENTION,
},
)
return "claimed"
@firestore.transactional
def complete_run(
transaction: firestore.Transaction,
run_ref: firestore.DocumentReference,
lease_expires: datetime.datetime,
) -> None:
"""Marks the run as completed if the lease is still held by this attempt."""
snapshot = run_ref.get(transaction=transaction)
if snapshot.exists:
run = snapshot.to_dict()
if run.get("lease_expires") == lease_expires and run.get("status") == "in_progress":
transaction.update(run_ref, {"status": "completed"})
@firestore.transactional
def release_run(
transaction: firestore.Transaction,
run_ref: firestore.DocumentReference,
lease_expires: datetime.datetime,
) -> None:
"""Releases the claim if the lease is still held by this attempt."""
snapshot = run_ref.get(transaction=transaction)
if snapshot.exists:
run = snapshot.to_dict()
if run.get("lease_expires") == lease_expires and run.get("status") == "in_progress":
transaction.delete(run_ref)References
- When handling scheduled runs, check and claim the run atomically under a lock before performing the work. If the work raises an exception, ensure the claim is released so that subsequent retries can process the run.
| now = datetime.datetime.now(datetime.timezone.utc) | ||
|
|
||
| status = claim_run(db.transaction(), run_ref, now) | ||
| if status == "completed": | ||
| return "Duplicate run skipped", 200 | ||
| if status == "in_progress": | ||
| # A non-2xx response keeps retries going in case that attempt fails. | ||
| return "Run in progress", 409 | ||
|
|
||
| try: | ||
| run_job_logic() | ||
| except Exception: | ||
| run_ref.delete() # Lets a retry claim the run. | ||
| raise | ||
| run_ref.update({"status": "completed"}) | ||
| return "Run processed", 200 |
There was a problem hiding this comment.
Update handle_scheduled_job to calculate lease_expires and use the new transactional complete_run and release_run helpers. This ensures that if this invocation runs slowly and its lease expires, it won't accidentally overwrite or delete a lease claimed by a subsequent retry.
| now = datetime.datetime.now(datetime.timezone.utc) | |
| status = claim_run(db.transaction(), run_ref, now) | |
| if status == "completed": | |
| return "Duplicate run skipped", 200 | |
| if status == "in_progress": | |
| # A non-2xx response keeps retries going in case that attempt fails. | |
| return "Run in progress", 409 | |
| try: | |
| run_job_logic() | |
| except Exception: | |
| run_ref.delete() # Lets a retry claim the run. | |
| raise | |
| run_ref.update({"status": "completed"}) | |
| return "Run processed", 200 | |
| now = datetime.datetime.now(datetime.timezone.utc) | |
| lease_expires = now + LEASE | |
| status = claim_run(db.transaction(), run_ref, now, lease_expires) | |
| if status == "completed": | |
| return "Duplicate run skipped", 200 | |
| if status == "in_progress": | |
| # A non-2xx response keeps retries going in case that attempt fails. | |
| return "Run in progress", 409 | |
| try: | |
| run_job_logic() | |
| except Exception: | |
| release_run(db.transaction(), run_ref, lease_expires) # Lets a retry claim the run. | |
| raise | |
| complete_run(db.transaction(), run_ref, lease_expires) | |
| return "Run processed", 200 |
References
- When handling scheduled runs, check and claim the run atomically under a lock before performing the work. If the work raises an exception, ensure the claim is released so that subsequent retries can process the run.
| def transaction(self) -> mock.MagicMock: | ||
| # firestore.transactional begins, commits, and retries the transaction. | ||
| transaction = mock.MagicMock(_max_attempts=1, _id=b"test", _read_only=False) | ||
| transaction.set.side_effect = lambda ref, data: self.store.__setitem__( | ||
| ref.id, dict(data) | ||
| ) | ||
| return transaction |
There was a problem hiding this comment.
To support the transactional update and delete operations in our unit tests, we need to mock these methods on the FakeDatabase transaction object.
| def transaction(self) -> mock.MagicMock: | |
| # firestore.transactional begins, commits, and retries the transaction. | |
| transaction = mock.MagicMock(_max_attempts=1, _id=b"test", _read_only=False) | |
| transaction.set.side_effect = lambda ref, data: self.store.__setitem__( | |
| ref.id, dict(data) | |
| ) | |
| return transaction | |
| def transaction(self) -> mock.MagicMock: | |
| # firestore.transactional begins, commits, and retries the transaction. | |
| transaction = mock.MagicMock(_max_attempts=1, _id=b"test", _read_only=False) | |
| transaction.set.side_effect = lambda ref, data: self.store.__setitem__( | |
| ref.id, dict(data) | |
| ) | |
| transaction.update.side_effect = lambda ref, data: self.store[ref.id].update(data) | |
| transaction.delete.side_effect = lambda ref: self.store.pop(ref.id, None) | |
| return transaction |
Description
Adds
cloudscheduler_deduplicate_job_requests, a Cloud Run function that does a Cloud Scheduler job's work once per scheduled run, even when Cloud Scheduler delivers the same run more than once. This rewrites #14702 by @camiekim, who is credited as co-author, and keeps that PR's region tag so the in-progress "Deduplicate job requests" docs page doesn't need to change.How it works
X-CloudScheduler-ScheduleTime, hashed into a Firestore document ID. The schedule time stays the same across retries, so the header is used exactly as received.expire_atfield for a Firestore TTL policy.Changes from #14702
cloud_scheduler/deduplicate_job_requests/, as a functions-framework function with its own requirements (google-cloud-firestore2.34.1), instead of adding Flask and Firestore tosnippets/.Scope. In testing, HTTP targets received
X-CloudScheduler-JobName(the job ID) andX-CloudScheduler-ScheduleTime. The API reference documents the same headers for App Engine targets, so this sample covers both. Pub/Sub messages from Cloud Scheduler carry no run identifier, and Google API targets such as Workflows have no user handler, so those targets are better covered on the docs page. A forced run sends the next scheduled time, so header-based deduplication then skips that scheduled run. A comment in the sample notes this.Tests (in
tests/)RUN_INTEGRATION_TESTS=true; setFIRESTORE_DATABASEto use a named database.Checklist
Testing
Compliance & Style
cloud_scheduler, which the default owners cover.Post-Approval Actions