Repository navigation
Conversation
validate_bench locked the Bench row FOR UPDATE on every Site save, so all saves on a bench queued behind any long transaction touching that bench. During a slow window, SaaS signups on the standby pool benches hit lock wait timeouts (1205) and failed partway through the handover. The lock only guards against Bench.archive racing a site that lands on the bench, so take it on insert or on a bench change, and read without it otherwise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the standby handover failed, it only reset is_standby. If part of the handover had already been committed, the site kept the customer's team with no Product Trial Request pointing at it: a working site with no plan, no subscription and no billing. Retries then claimed another standby site. Restore team, account request, signup time and trial end date to what the pool site had before the claim. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Tick the box to add this pull request to the merge queue (same as
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| frappe.db.rollback() | ||
| frappe.db.set_value("Site", standby_site, "is_standby", 1) | ||
| # Part of the handover may already be committed, so undo it all, not just is_standby | ||
| frappe.db.set_value("Site", standby_site, {"is_standby": 1, **pool_state}) |
There was a problem hiding this comment.
The failure handler restores five fields with frappe.db.set_value. The repository guide requires doc.save() when several fields change together. Restore these fields through the Site document after rollback. This requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/saas/doctype/product_trial/product_trial.py
Line: 155
Comment:
**Recovery bypasses the required save**
The failure handler restores five fields with `frappe.db.set_value`. The repository guide requires `doc.save()` when several fields change together. Restore these fields through the `Site` document after rollback. This requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/frappe/press/blob/develop/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| self.assertEqual(standby_site.is_standby, 1) | ||
| self.assertEqual(standby_site.team, pool_team) | ||
| self.assertEqual(standby_site.signup_time, pool_signup_time) |
There was a problem hiding this comment.
The test checks team and signup_time, but not the newly restored account_request and trial_end_date. Pass a nonempty account_request and assert both fields return to their pool values. Otherwise, removing either field from the recovery write would still pass this test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/saas/doctype/product_trial/test_product_trial.py
Line: 126-128
Comment:
**Two restored fields go unchecked**
The test checks `team` and `signup_time`, but not the newly restored `account_request` and `trial_end_date`. Pass a nonempty `account_request` and assert both fields return to their pool values. Otherwise, removing either field from the recovery write would still pass this test.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.This branch has not been deployed
1 participant
During a window of heavy lock contention on the press database, SaaS signups failed partway through handing out a standby site. Each failure left a working site assigned to the customer's team with no Product Trial Request pointing at it, so the site never got a plan, a subscription or billing. Customers who retried claimed another standby site each time.
All the failures were the same lock wait (1205) on
SELECT status FROM tabBench WHERE name=... FOR UPDATE, fromSite.validate_bench. That lock is taken on every Site save, so the handover (which saves the site twice) queued behind any long transaction touching the same bench. Standby replenishment, TLS renewal and worker auto-scaling hit the same lock waits in the same window.Changes
fix(site): Lock the bench only when a site lands on it. The
FOR UPDATEon the Bench row exists so a site can't land on a bench thatBench.archiveis archiving (1ef6905, 87f14e0). That race only matters when a site is created or moves to another bench, so the lock is now taken only then. Other saves read the bench status without locking. Standby handovers and site update status changes no longer queue on the bench row.fix(product-trial): Return a failed standby handover fully to the pool. On failure,
setup_trial_siteonly resetis_standby. If part of the handover was already committed, the site kept the customer's team. It now also restores team, account request, signup time and trial end date to what the pool site had before the claim.Not in this PR
Tests
test_saving_a_site_that_stays_on_its_bench_does_not_lock_the_benchtest_moving_a_site_to_another_bench_locks_the_destination_benchtest_failed_standby_handover_returns_site_to_pool_and_undoes_the_customer_team🤖 Generated with Claude Code