diff --git a/plugins/warranty/api/routes.py b/plugins/warranty/api/routes.py index 53db4ec4..8276cc66 100644 --- a/plugins/warranty/api/routes.py +++ b/plugins/warranty/api/routes.py @@ -281,14 +281,27 @@ def refresh_warranty(warrantyid): warranty = db.session.get(Warranty, warrantyid) if not warranty: return error_response(ErrorCodes.NOT_FOUND, 'Warranty not found', http_code=404) - provider = get_provider(warranty.provider) + # Copy to locals and end the transaction before the vendor call: the lookup + # can block for the full HTTP timeout, and holding a pooled connection + # across it starves the pool under load. Reading ORM attributes after the + # commit would check a connection straight back out. + providername = warranty.provider + servicetag = warranty.servicetag + vendor = warranty.vendor + db.session.commit() + + provider = get_provider(providername) try: - result = provider.lookup(warranty.servicetag, warranty.vendor) + result = provider.lookup(servicetag, vendor) except (ProviderNotConfigured, WarrantyLookupError) as exc: return error_response(ErrorCodes.VALIDATION_ERROR, str(exc), http_code=400) if not result: return error_response(ErrorCodes.VALIDATION_ERROR, 'This warranty is manual - nothing to refresh.', http_code=400) + + warranty = db.session.get(Warranty, warrantyid) + if not warranty: + return error_response(ErrorCodes.NOT_FOUND, 'Warranty not found', http_code=404) if result.get('servicelevel'): warranty.servicelevel = result['servicelevel'] if result.get('startdate'): @@ -342,11 +355,31 @@ def sync_dell(): 'candidates': 0, 'matched': 0, 'created': 0, 'updated': 0, }, message='No PCs need a warranty lookup.') + # by_tag is plain strings and ints, so it survives the commit. Release the + # connection before bulk_lookup: Dell is called 100 tags at a time and a + # fleet-sized sync would otherwise pin a pooled connection for minutes. + db.session.commit() + try: results = provider.bulk_lookup(list(by_tag.keys())) except (ProviderNotConfigured, WarrantyLookupError) as exc: return error_response(ErrorCodes.VALIDATION_ERROR, str(exc), http_code=400) + # Preload every warranty already linked to a candidate asset in one query. + # Done per asset this was a query per asset plus one per link, which would + # just trade the long network hold for a long write transaction. + matched_assetids = [assetid + for tag, assetids in by_tag.items() if results.get(tag) + for assetid in assetids] + existing_by_assetid = {} + if matched_assetids: + rows = (db.session.query(WarrantyAsset.assetid, Warranty) + .join(Warranty, Warranty.warrantyid == WarrantyAsset.warrantyid) + .filter(WarrantyAsset.assetid.in_(matched_assetids)) + .all()) + for assetid, warranty in rows: + existing_by_assetid.setdefault(assetid, []).append(warranty) + created = updated = matched = 0 now = datetime.now(timezone.utc).replace(tzinfo=None) for tag, assetids in by_tag.items(): @@ -357,15 +390,13 @@ def sync_dell(): for assetid in assetids: # Reuse an existing Dell warranty for this asset if there is one. existing = None - for link in WarrantyAsset.query.filter_by(assetid=assetid).all(): - candidate = db.session.get(Warranty, link.warrantyid) + for candidate in existing_by_assetid.get(assetid, []): # Reuse a warranty that is Dell by ANY signal, not just # provider: a manually-added or imported Dell warranty carries # provider 'manual' (create default) but the same service tag or # a "Dell" vendor. Matching only provider=='dell' made re-check # duplicate every one of those instead of updating it. - if candidate and ( - candidate.provider == 'dell' + if (candidate.provider == 'dell' or (candidate.servicetag or '').strip().upper() == tag or (candidate.vendor or '').strip().lower() == 'dell'): existing = candidate diff --git a/tests/test_plugins/test_warranty_connection_release.py b/tests/test_plugins/test_warranty_connection_release.py new file mode 100644 index 00000000..69239640 --- /dev/null +++ b/tests/test_plugins/test_warranty_connection_release.py @@ -0,0 +1,163 @@ +"""The Dell routes must not hold a database connection across the vendor call. + +A bulk sync asks Dell about 100 service tags per request and a fleet-sized run +is dozens of those, so a route that keeps its transaction open for the duration +pins a pooled connection for minutes. Under gevent workers that exhausts the +pool and takes down every other request, not just warranty. + +These pin the boundary itself rather than the result: a fake provider inspects +the session at the moment it is called and records whether a transaction was +still open. Behaviour is covered elsewhere - what would silently regress here is +someone reading an ORM attribute after the commit, which re-checks a connection +straight back out and restores the original defect while every other test +continues to pass. +""" + +from plugins.warranty.api import routes as warranty_routes +from plugins.warranty.models import Warranty, WarrantyAsset + + +def _asset(db, assetnumber='PC-0001', serialnumber='ABC1234'): + from shopdb.core.models import AssetType, AssetStatus + from shopdb.core.models.asset import Asset + + assettype = AssetType(assettype='computer') + status = AssetStatus(status='In Use') + db.session.add_all([assettype, status]) + db.session.flush() + + asset = Asset( + assetnumber=assetnumber, + name='Bench PC', + assettypeid=assettype.assettypeid, + statusid=status.statusid, + serialnumber=serialnumber, + ) + db.session.add(asset) + db.session.commit() + return asset + + +class _ProbeProvider: + """Stands in for DellProvider and records the session state when called. + + The real provider blocks on HTTP here; what matters is only whether the + caller was still inside a transaction at that instant. + """ + + def __init__(self, db, result): + self._db = db + self._result = result + self.in_transaction_at_call = None + self.called = False + + def _observe(self): + self.called = True + # db.session is a scoped_session proxy and does not forward + # in_transaction; calling it returns the real Session for this scope. + self.in_transaction_at_call = self._db.session().in_transaction() + + def lookup(self, servicetag, vendor=None): + self._observe() + return self._result + + def bulk_lookup(self, servicetags): + self._observe() + return {tag: self._result for tag in servicetags} + + +def test_refresh_releases_the_connection_before_the_vendor_call( + client, db, auth_headers, monkeypatch): + warranty = Warranty(vendor='Dell', servicetag='ABC1234', provider='dell') + db.session.add(warranty) + db.session.commit() + warrantyid = warranty.warrantyid + + probe = _ProbeProvider(db, { + 'servicelevel': 'ProSupport', + 'startdate': '2024-01-01', + 'enddate': '2027-01-01', + }) + monkeypatch.setattr(warranty_routes, 'get_provider', lambda name: probe) + + response = client.post(f'/api/warranty/{warrantyid}/refresh', headers=auth_headers) + + assert response.status_code == 200, response.get_json() + assert probe.called + assert probe.in_transaction_at_call is False, ( + 'refresh_warranty held a database connection across the vendor lookup' + ) + # The write half still has to land, or releasing early would be a + # regression dressed up as a fix. + refreshed = db.session.get(Warranty, warrantyid) + assert refreshed.servicelevel == 'ProSupport' + assert refreshed.lastcheckeddate is not None + + +def test_sync_dell_releases_the_connection_before_the_bulk_call( + client, db, auth_headers, monkeypatch): + asset = _asset(db) + assetid = asset.assetid + + probe = _ProbeProvider(db, { + 'servicelevel': 'Basic Onsite', + 'startdate': '2023-05-02', + 'enddate': '2026-05-02', + }) + monkeypatch.setattr(warranty_routes, 'get_provider', lambda name: probe) + + response = client.post('/api/warranty/sync/dell', headers=auth_headers) + + assert response.status_code == 200, response.get_json() + assert probe.called + assert probe.in_transaction_at_call is False, ( + 'sync_dell held a database connection across the Dell bulk lookup' + ) + body = response.get_json()['data'] + assert body['created'] == 1 + assert body['matched'] == 1 + + links = WarrantyAsset.query.filter_by(assetid=assetid).all() + assert len(links) == 1 + created = db.session.get(Warranty, links[0].warrantyid) + assert created.provider == 'dell' + assert created.servicetag == 'ABC1234' + + +def test_sync_dell_updates_instead_of_duplicating_on_a_recheck( + client, db, auth_headers, monkeypatch): + """Preloading the existing links must not lose the Dell-by-any-signal match. + + The write loop now reads from one preloaded query instead of a query per + asset. A manually-added Dell warranty carries provider 'manual', so if the + preload missed it a re-check would create a second warranty for the asset + rather than updating the one already there. + """ + asset = _asset(db) + assetid = asset.assetid + + existing = Warranty(vendor='Dell', servicetag='ABC1234', provider='manual') + existing.links.append(WarrantyAsset(assetid=assetid)) + db.session.add(existing) + db.session.commit() + existingid = existing.warrantyid + + probe = _ProbeProvider(db, { + 'servicelevel': 'ProSupport Plus', + 'startdate': '2024-02-02', + 'enddate': '2028-02-02', + }) + monkeypatch.setattr(warranty_routes, 'get_provider', lambda name: probe) + + response = client.post('/api/warranty/sync/dell?all=true', headers=auth_headers) + + assert response.status_code == 200, response.get_json() + body = response.get_json()['data'] + assert body['created'] == 0 + assert body['updated'] == 1 + + links = WarrantyAsset.query.filter_by(assetid=assetid).all() + assert len(links) == 1, 'a re-check duplicated the warranty instead of updating it' + updated = db.session.get(Warranty, existingid) + assert updated.provider == 'dell' + assert updated.servicelevel == 'ProSupport Plus'