fix: SBOM analysis request should be blocked when user limit exceeded by TamarW0 · Pull Request #302 · RHEcosystemAppEng/exploit-iq-client

TamarW0 · 2026-08-17T22:51:28Z

Add upfront queue capacity check to SPDX upload before creating product,
matching RPM/CycloneDX behavior. When user limit exceeded, return 429
immediately instead of accepting request then failing all components.

Changes:

SbomReportService: Add RequestQueueService injection and capacity check
Add SbomReportServiceQueueAdmissionTest to verify exception handling

Add upfront queue capacity check to SPDX upload before creating product, matching RPM/CycloneDX behavior. When user limit exceeded, return 429 immediately instead of accepting request then failing all components. Changes: - SbomReportService: Add RequestQueueService injection and capacity check - Add SbomReportServiceQueueAdmissionTest to verify exception handling

vbelouso · 2026-08-17T22:51:38Z

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

TamarW0 · 2026-08-17T22:52:06Z

/test exploit-iq-client-on-pr

tmihalac

Review Summary

The core change is correct — capacity check is placed before product creation, exceptions propagate properly, no queue slot leaks are possible (runIfHasCapacity is a pre-check only). The structural divergence from the RPM/CycloneDX saveAndSubmitNew pattern is justified since SPDX creates a Product then fans out to components.

No critical issues. Three important items and three suggestions below as inline comments.

tmihalac · 2026-08-18T05:33:31Z

+    return queueService.runIfHasCapacity(user, productId, () -> {
+      Map<String, String> metadata = new HashMap<>();
+      // Add CPE to metadata if present
+      if (productInfo.cpe() != null && !productInfo.cpe().trim().isEmpty()) {
+        metadata.put("cpe", productInfo.cpe());
+      }
+
+      if (Objects.nonNull(productInfo.spdxId())) {
+        metadata.put(RepositoryConstants.SPDX_ID_METADATA_KEY, productInfo.spdxId());
+      }
+
+      int totalComponentCount = finalParsed.components().size() + finalParsed.unsupportedComponents().size();
+      Product product = this.createProduct(finalCveId, productInfo.name(), productInfo.version(), totalComponentCount, metadata);
+
+      for (SpdxParsingService.UnsupportedComponentInfo unsupported : finalParsed.unsupportedComponents()) {
+        String errorMessage =
+            "Expects a container image purl with format pkg:oci/name@sha256:hash or pkg:oci/name@sha256%3Ahash?repository_url=...&tag=...";
+        String imageForDisplay = unsupported.purl() != null ? unsupported.purl() : "";
+        productRepository.addSubmissionFailure(product.id(), new FailedComponent(
+            unsupported.name(), unsupported.version(), imageForDisplay, errorMessage));
+      }
+
+      // Start component processing (chunks run in parallel on executor)
+      processSpdxComponents(product.id(), finalParsed, finalCveId, finalCredentialId);

-    // Start component processing (chunks run in parallel on executor)
-    processSpdxComponents(product.id(), parsed, cveId, credentialId);
+      LOGGER.infof("Created product %s, started component processing", product.id());

-    LOGGER.infof("Created product %s, started component processing", product.id());
-
-    return product.id();
+      return product.id();
+    });


Suggestion: Consider adding context logging for non-queue failures inside the lambda.

If createProduct or processSpdxComponents throws here, the exception propagates through runIfHasCapacity (which doesn't catch or log it) to the generic @ServerExceptionMapper — resulting in a 500 with no log of which CVE/product was being processed. The RPM/CycloneDX flow logs "Unable to submit request" with report IDs in ReportService.

A lightweight option:

return queueService.runIfHasCapacity(user, productId, () -> { try { // ... existing lambda body ... } catch (Exception e) { LOGGER.errorf(e, "Failed SPDX upload for CVE %s, product %s/%s", finalCveId, productInfo.name(), productInfo.version()); throw e; } });

Non-blocking — the current code works correctly, this just improves debuggability.

tmihalac · 2026-08-18T05:33:31Z

+ * matching the pattern used by RPM and CycloneDX flows.
+ */
+@QuarkusComponentTest
+class SbomReportServiceQueueAdmissionTest {


Important: Missing happy path test.

Both tests verify rejection (queue throws before the lambda runs). There's no test verifying the lambda actually executes when capacity is available. The RPM equivalent (ReportServiceQueueAdmissionTest) includes submitHappyPathWritesSubmittedAfterAdmission.

A happy path test would catch:

Lambda wiring bugs (variable capture, return value flowing through)

Regression if createProduct is accidentally moved outside the lambda

That processSpdxComponents is called on success

Suggested approach: mock runIfHasCapacity with thenAnswer that invokes the supplier (invocation.getArgument(2, Supplier.class).get()), mock productRepository.save() to succeed, verify save() is called and the product ID is returned.

tmihalac · 2026-08-18T05:33:31Z

+    assertThrows(UserQueueExceededException.class,
+        () -> sbomReportService.submitSpdx(spdxStream, "CVE-2024-1234", null));


Important: Arguments to runIfHasCapacity are not verified.

any() and nullable(String.class) match anything — the tests would still pass if the code passed null for user, hardcoded a string, or swapped the user/productId arguments.

Consider using eq() for at least the user argument:

doThrow(new UserQueueExceededException(5)) .when(queueService).runIfHasCapacity(eq("alice"), anyString(), any());

Same applies to the second test with eq("bob").

tmihalac · 2026-08-18T05:33:31Z

+    assertThrows(RequestQueueExceededException.class,
+        () -> sbomReportService.submitSpdx(spdxStream, "CVE-2024-5678", null));
+
+    // Verify: No product created


Suggestion: Add componentProcessingService never-verification here for consistency.

The first test verifies both productRepository.save and componentProcessingService.processComponents are never called, but this test only verifies save. Both exercise the same rejection path and should verify the same side effects.

verify(productRepository, never()).save(any(), any()); verify(componentProcessingService, never()).processComponents(any(), any(), any(), any(), any());

tmihalac · 2026-08-18T05:33:31Z

+        .when(queueService).runIfHasCapacity(any(), nullable(String.class), any());
+


Nit: nullable(String.class) suggests productId could be null, but generateProductId always returns a non-null String. anyString() would better express the contract.

zvigrinberg

Hi @TamarW0 , 2 important gaps:

As discussed, the API change ( product endpoints can return ) is not documented using annotations of SmallRye OpenAPI lib. it works because of 2 class level ServerExceptionMapper that accepts the thrown exceptions ( UserQueueExceededException and RequestQueueExceededException).
In addtion, see my comments about product id calculated twice, and is not correlated between the queue slot reservation and product object in DB, this is a bug that will cause the slot of the product to stay in the active queues even when the product analysis will complete.

zvigrinberg · 2026-08-18T06:20:50Z

+    // Generate productId before capacity check (needed for product slot optimization)
+    final String productId = generateProductId(productInfo.name(), productInfo.version());


@TamarW0 This is used for the slot reservation in the queue, but the product uses another generated productId, you need it to be the same.

zvigrinberg · 2026-08-18T06:23:49Z

+      }
+
+      int totalComponentCount = finalParsed.components().size() + finalParsed.unsupportedComponents().size();
+      Product product = this.createProduct(finalCveId, productInfo.name(), productInfo.version(), totalComponentCount, metadata);


@TamarW0 This is creating the product id ( alongside the product), and not reusing the already created product id before the capacity check, these 2 must be the same ( they're different because product id is also consist out of timestamp/instant, not only determinstic portions).

…rovements Critical fixes: - Fix product ID mismatch: pass productId to createProduct() instead of regenerating with different timestamp, ensuring queue slot and DB product have matching IDs - Add 429 response documentation to OpenAPI for both SPDX and CycloneDX upload endpoints Test improvements: - Add happy path test verifying lambda execution and product creation - Use eq() for user and anyString() for productId in test assertions - Add error logging in lambda to avoid silent 500 errors - Add componentProcessingService verification to second test Addresses review feedback from zvigrinberg and tmihalac Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

TamarW0 · 2026-08-18T07:55:19Z

/test exploit-iq-client-on-pr

zvigrinberg

LGTM Approved.

tmihalac · 2026-08-18T09:23:00Z

LGTM

TamarW0 marked this pull request as ready for review August 17, 2026 22:51

TamarW0 requested review from IlonaShishov, vbelouso and zvigrinberg as code owners August 17, 2026 22:51

tmihalac reviewed Aug 18, 2026

View reviewed changes

zvigrinberg requested changes Aug 18, 2026

View reviewed changes

zvigrinberg approved these changes Aug 18, 2026

View reviewed changes

		assertThrows(UserQueueExceededException.class,
		() -> sbomReportService.submitSpdx(spdxStream, "CVE-2024-1234", null));

		.when(queueService).runIfHasCapacity(any(), nullable(String.class), any());

		// Generate productId before capacity check (needed for product slot optimization)
		final String productId = generateProductId(productInfo.name(), productInfo.version());

Uh oh!

Conversation

TamarW0 commented Aug 17, 2026

Uh oh!

vbelouso commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Snyk checks have passed. No issues have been found so far.

Uh oh!

TamarW0 commented Aug 17, 2026

Uh oh!

tmihalac left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review Summary

Uh oh!

tmihalac Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

tmihalac Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

tmihalac Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

tmihalac Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

tmihalac Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

zvigrinberg left a comment

Choose a reason for hiding this comment

Uh oh!

zvigrinberg Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

zvigrinberg Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

TamarW0 commented Aug 18, 2026

Uh oh!

zvigrinberg left a comment

Choose a reason for hiding this comment

Uh oh!

tmihalac commented Aug 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vbelouso commented Aug 17, 2026 •

edited

Loading

tmihalac left a comment •

edited

Loading