Skip to content

Issue 3840 test failure #3841

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 22, 2025
Merged

Issue 3840 test failure #3841

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f17543
Move comment
finestructure Jul 22, 2025
0cc1772
Fix logical race in TriggerBuilds_triggerBuilds_error
finestructure Jul 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions Tests/AppTests/BuildTriggerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import Testing

@testable import App

import AsyncHTTPClient
import Dependencies
import Fluent
import NIOConcurrencyHelpers
Expand Down Expand Up @@ -677,6 +678,7 @@ extension AllTests.BuildTriggerTests {
}

@Test func TriggerBuilds_triggerBuilds_trimming() async throws {
// Ensure we trim builds as part of triggering
try await withDependencies {
$0.buildSystem.getStatusCount = { @Sendable _ in 100 }
$0.environment.allowBuildTriggers = { true }
Expand All @@ -689,7 +691,6 @@ extension AllTests.BuildTriggerTests {
$0.environment.random = { @Sendable _ in 0 }
$0.environment.siteURL = { "http://example.com" }
} operation: {
// Ensure we trim builds as part of triggering
try await withSPIApp { app in
// setup
let p = Package(id: .id0, url: "2")
Expand All @@ -714,7 +715,6 @@ extension AllTests.BuildTriggerTests {
}

@Test func TriggerBuilds_triggerBuilds_error() async throws {
// Ensure we trim builds as part of triggering
let triggerCount = QueueIsolated(0)
try await withDependencies {
$0.buildSystem.getStatusCount = { @Sendable _ in 100 }
Expand All @@ -730,13 +730,15 @@ extension AllTests.BuildTriggerTests {
$0.environment.siteURL = { "http://example.com" }
$0.buildSystem.triggerBuild = BuildSystemClient.liveValue.triggerBuild
$0.httpClient.post = { @Sendable _, _, body in
defer { triggerCount.increment() }
// let the 5th trigger succeed to ensure we don't early out on errors
if triggerCount.value == 5 {
return .created(webUrl: "http://web_url")
} else {
struct Response: Content { var message: String }
return try .tooManyRequests(jsonEncode: Response(message: "Too many pipelines created in the last minute. Try again later."))
return try triggerCount.withValue { triggerCount -> HTTPClient.Response in
defer { triggerCount += 1 }
// Let one trigger not fail
if triggerCount == 5 {
return .created(webUrl: "http://web_url")
} else {
struct Response: Content { var message: String }
return try .tooManyRequests(jsonEncode: Response(message: "Too many pipelines created in the last minute. Try again later."))
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a logical race here where I'd properly locked the triggerCount value but the lock didn't span the whole of the operation that needed to be guarded. It could happen that all 32 tasks were run and saw a value < 5, because the increment happened on a later, separate lock than the check.

}
}
} operation: {
Expand All @@ -750,6 +752,9 @@ extension AllTests.BuildTriggerTests {
// MUT
try await triggerBuilds(on: app.db, mode: .packageId(.id0, force: false))

// Ensure all triggers were attempted
#expect(triggerCount.value == 32)

// validate that one build record is saved, for the successful trigger
let count = try await Build.query(on: app.db).count()
#expect(count == 1)
Expand Down
Loading