Skip to content

Commit f0ca4cb

Browse files
committed
fix: revert the eager security-header write and tighten the async-timeout branch
The eager HeaderWriterFilter write broke image and screenshot caching. Spring Security's CacheControlHeadersWriter normally runs last and skips when the header is already present; writing eagerly meant it set `no-cache, no-store` first and ImageStorageController, which uses addHeader, then appended a second value — plus `Pragma: no-cache` and `Expires: 0` that nothing overrides. That is a worse regression than the race it was fixing, so it is reverted and StreamingBodyDatabasePoolHealthTest keeps its ignoreTestOnSpringBug wrapper. spring-security#9175 stays an open pre-existing hazard that higher streaming concurrency makes more likely. The async-timeout handler no longer infers "the stream already started" from response.isCommitted: a servlet response only commits once ~8KB is buffered, so a slow import-progress or MT stream that timed out having written a few lines was being reclassified as capacity and hidden from Sentry. The streaming provider now records that the body actually began, and the handler branches on that. Also: the three tests that customise the Spring context are tagged @ContextRecreatingTest, which is what stops them accumulating contexts and running the standard suite out of heap; @primary is dropped from the background executor since nothing needed it; and the internal streaming endpoint stages the same headers the real export endpoints do, so the rejection path's header handling is actually exercised.
1 parent 8d71aba commit f0ca4cb

13 files changed

Lines changed: 69 additions & 75 deletions

File tree

backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/administration/ProjectExportImportController.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ class ProjectExportImportController(
5858
val export = projectExportImportExporter.exportToTempFile(projectId, versionProvider.version)
5959
val tempFile = export.path
6060
// A saturated streaming pool rejects the body after this method returns, so the delete below
61-
// never runs. Bound that rather than leaking the temp file for the life of the pod.
61+
// never runs; this is the backstop, at JVM exit.
6262
tempFile.toFile().deleteOnExit()
6363
try {
6464
val body =

backend/app/src/main/kotlin/io/tolgee/ExceptionHandlers.kt

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import io.tolgee.security.ratelimit.RateLimitBlockedException
1717
import io.tolgee.security.ratelimit.RateLimitResponseBody
1818
import io.tolgee.security.ratelimit.RateLimitedException
1919
import io.tolgee.util.Logging
20+
import io.tolgee.util.StreamingResponseBodyProvider
2021
import io.tolgee.util.logger
2122
import jakarta.persistence.EntityNotFoundException
2223
import jakarta.servlet.http.HttpServletRequest
@@ -273,33 +274,31 @@ class ExceptionHandlers : Logging {
273274
ex: RejectedExecutionException,
274275
response: HttpServletResponse,
275276
): ResponseEntity<ErrorResponseBody> {
276-
// Spring wraps the policy's exception in TaskRejectedException. Any other rejection — a shutting
277-
// down executor, some other pool — belongs on the generic path, Sentry included.
278-
if (generateSequence(ex as Throwable) { it.cause }.none { it is StreamingCapacityExceededException }) {
277+
if (!isStreamingRejection(ex)) {
279278
return handleOtherExceptions(ex)
280279
}
281280
logger.debug("Streaming pool saturated, rejecting request", ex)
282281
return serverBusy(response)
283282
}
284283

285-
/**
286-
* A request that aged out of the streaming queue never started writing, so it is the same capacity
287-
* condition as an outright rejection — and answering it generically would raise a Sentry event per
288-
* overloaded request. A stream that timed out mid-write is a different animal: its response is
289-
* already committed, nothing can be said to the client, and it is worth reporting.
290-
*/
291284
@ExceptionHandler(AsyncRequestTimeoutException::class)
292285
fun handleAsyncRequestTimeout(
293286
ex: AsyncRequestTimeoutException,
287+
request: HttpServletRequest,
294288
response: HttpServletResponse,
295289
): ResponseEntity<ErrorResponseBody> {
296-
if (response.isCommitted) {
290+
// A stream that ran and still timed out is a slowness regression, not capacity, and reporting it
291+
// is the point. Only one that never left the queue is the same condition as a rejection.
292+
if (request.getAttribute(StreamingResponseBodyProvider.STREAM_STARTED_ATTRIBUTE) != null) {
297293
return handleOtherExceptions(ex)
298294
}
299295
logger.debug("Request timed out waiting for a streaming thread", ex)
300296
return serverBusy(response)
301297
}
302298

299+
private fun isStreamingRejection(ex: Throwable): Boolean =
300+
generateSequence(ex) { it.cause }.any { it is StreamingCapacityExceededException }
301+
303302
private fun serverBusy(response: HttpServletResponse): ResponseEntity<ErrorResponseBody> {
304303
dropStagedStreamingHeaders(response)
305304
return ResponseEntity
@@ -308,13 +307,7 @@ class ExceptionHandlers : Logging {
308307
.body(ErrorResponseBody(Message.SERVER_BUSY.code, null))
309308
}
310309

311-
/**
312-
* The streaming return-value handler stages Content-Disposition and an ETag before the task is
313-
* submitted, and a caching proxy would happily key this 503 against that ETag. Only reset() clears
314-
* them, but it also clears everything the filter chain has already written — CORS, the version
315-
* header, and the security headers HeaderWriterFilter now writes eagerly. So everything unrelated
316-
* to streaming is put back.
317-
*/
310+
/** A caching proxy would key this 503 against the ETag the streaming handler already staged. */
318311
private fun dropStagedStreamingHeaders(response: HttpServletResponse) {
319312
if (response.isCommitted) return
320313
val preserved =

backend/app/src/main/kotlin/io/tolgee/configuration/AsyncMethodConfiguration.kt

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import io.tolgee.configuration.tolgee.TolgeeProperties
44
import org.springframework.beans.factory.ObjectProvider
55
import org.springframework.context.annotation.Bean
66
import org.springframework.context.annotation.Configuration
7-
import org.springframework.context.annotation.Primary
87
import org.springframework.scheduling.annotation.AsyncConfigurer
98
import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor
109
import java.util.concurrent.Executor
@@ -19,7 +18,6 @@ class AsyncMethodConfiguration(
1918
) : AsyncConfigurer {
2019
override fun getAsyncExecutor(): Executor = backgroundAsyncExecutor()
2120

22-
@Primary
2321
@Bean(BACKGROUND_EXECUTOR_BEAN_NAME)
2422
fun backgroundAsyncExecutor(): ThreadPoolTaskExecutor {
2523
val factory = asyncExecutorFactory.getObject()

backend/app/src/main/kotlin/io/tolgee/configuration/WebSecurityConfig.kt

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@ import org.springframework.security.config.http.SessionCreationPolicy
4545
import org.springframework.security.web.SecurityFilterChain
4646
import org.springframework.security.web.access.intercept.AuthorizationFilter
4747
import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter
48-
import org.springframework.security.web.header.HeaderWriterFilter
4948
import org.springframework.security.web.header.writers.ReferrerPolicyHeaderWriter.ReferrerPolicy
5049
import org.springframework.web.servlet.config.annotation.InterceptorRegistry
5150
import org.springframework.web.servlet.config.annotation.WebMvcConfigurer
@@ -105,18 +104,6 @@ class WebSecurityConfig(
105104
it.requestMatchers("/api/**", "/v2/**").authenticated()
106105
it.anyRequest().permitAll()
107106
}.headers { headers ->
108-
// Write the security headers before the chain proceeds. By default HeaderWriterFilter writes
109-
// them while unwinding, which races a StreamingResponseBody task already writing the same
110-
// response — spring-security#9175, the ConcurrentModificationException that
111-
// io.tolgee.fixtures.springBug works around.
112-
headers.withObjectPostProcessor(
113-
object : ObjectPostProcessor<HeaderWriterFilter> {
114-
override fun <O : HeaderWriterFilter?> postProcess(filter: O): O {
115-
filter?.setShouldWriteHeadersEagerly(true)
116-
return filter
117-
}
118-
},
119-
)
120107
headers.xssProtection(Customizer.withDefaults())
121108
headers.contentTypeOptions(Customizer.withDefaults())
122109
headers.frameOptions {

backend/app/src/test/kotlin/io/tolgee/ExceptionHandlersAsyncCapacityTest.kt

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@ import io.tolgee.component.VersionFilter
44
import io.tolgee.constants.Message
55
import io.tolgee.exceptions.StreamingCapacityExceededException
66
import io.tolgee.testing.assert
7+
import io.tolgee.util.StreamingResponseBodyProvider
78
import org.junit.jupiter.api.Test
89
import org.springframework.core.task.TaskRejectedException
910
import org.springframework.http.HttpHeaders
1011
import org.springframework.http.HttpStatus
12+
import org.springframework.mock.web.MockHttpServletRequest
1113
import org.springframework.mock.web.MockHttpServletResponse
1214
import org.springframework.web.context.request.async.AsyncRequestTimeoutException
1315
import java.util.concurrent.RejectedExecutionException
@@ -87,7 +89,12 @@ class ExceptionHandlersAsyncCapacityTest {
8789
val response = MockHttpServletResponse()
8890
response.setHeader(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=export.zip")
8991

90-
val result = exceptionHandlers.handleAsyncRequestTimeout(AsyncRequestTimeoutException(), response)
92+
val result =
93+
exceptionHandlers.handleAsyncRequestTimeout(
94+
AsyncRequestTimeoutException(),
95+
MockHttpServletRequest(),
96+
response,
97+
)
9198

9299
result.statusCode.assert.isEqualTo(HttpStatus.SERVICE_UNAVAILABLE)
93100
result.body
@@ -96,13 +103,17 @@ class ExceptionHandlersAsyncCapacityTest {
96103
response.getHeader(HttpHeaders.CONTENT_DISPOSITION).assert.isNull()
97104
}
98105

106+
/** A slow stream writes far less than one 8KB buffer, so commitment is not the signal. */
99107
@Test
100-
fun `reports a stream that timed out after it started writing`() {
108+
fun `reports a stream that ran and still timed out, even with nothing flushed`() {
109+
val request = MockHttpServletRequest()
110+
request.setAttribute(StreamingResponseBodyProvider.STREAM_STARTED_ATTRIBUTE, true)
101111
val response = MockHttpServletResponse()
102-
response.flushBuffer()
103112

104-
val result = exceptionHandlers.handleAsyncRequestTimeout(AsyncRequestTimeoutException(), response)
113+
val result =
114+
exceptionHandlers.handleAsyncRequestTimeout(AsyncRequestTimeoutException(), request, response)
105115

116+
response.isCommitted.assert.isFalse()
106117
result.statusCode.assert.isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR)
107118
}
108119

backend/app/src/test/kotlin/io/tolgee/StreamingBodyDatabasePoolHealthTest.kt

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package io.tolgee
1919
import com.zaxxer.hikari.HikariDataSource
2020
import io.tolgee.development.testDataBuilder.data.TranslationsTestData
2121
import io.tolgee.fixtures.andIsOk
22+
import io.tolgee.fixtures.ignoreTestOnSpringBug
2223
import io.tolgee.fixtures.waitForNotThrowing
2324
import io.tolgee.testing.annotations.ProjectJWTAuthTestMethod
2425
import io.tolgee.testing.assert
@@ -50,25 +51,22 @@ class StreamingBodyDatabasePoolHealthTest : ProjectAuthControllerTest("/v2/proje
5051
projectSupplier = { testData.project }
5152
}
5253

53-
/**
54-
* Deliberately not wrapped in ignoreTestOnSpringBug: WebSecurityConfig now makes HeaderWriterFilter
55-
* write eagerly, so the request thread no longer races the streaming task over the response
56-
* headers. If that regresses, this must fail rather than quietly skip.
57-
*/
5854
@Test
5955
@ProjectJWTAuthTestMethod
6056
fun `streaming responses do not cause a database connection pool exhaustion`() {
61-
val hikariDataSource = dataSource as HikariDataSource
62-
val pool = hikariDataSource.hikariPoolMXBean
57+
ignoreTestOnSpringBug {
58+
val hikariDataSource = dataSource as HikariDataSource
59+
val pool = hikariDataSource.hikariPoolMXBean
6360

64-
waitForNotThrowing(pollTime = 50, timeout = 5000) {
65-
pool.activeConnections.assert.isEqualTo(0)
66-
}
67-
repeat(50) {
68-
performProjectAuthGet("export").andIsOk
69-
}
70-
waitForNotThrowing(pollTime = 50, timeout = 5000) {
71-
pool.activeConnections.assert.isEqualTo(0)
61+
waitForNotThrowing(pollTime = 50, timeout = 5000) {
62+
pool.activeConnections.assert.isEqualTo(0)
63+
}
64+
repeat(50) {
65+
performProjectAuthGet("export").andIsOk
66+
}
67+
waitForNotThrowing(pollTime = 50, timeout = 5000) {
68+
pool.activeConnections.assert.isEqualTo(0)
69+
}
7270
}
7371
}
7472
}

backend/app/src/test/kotlin/io/tolgee/configuration/AsyncDispatchTargetTest.kt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package io.tolgee.configuration
22

3+
import io.tolgee.testing.ContextRecreatingTest
34
import io.tolgee.testing.assert
45
import org.junit.jupiter.api.Test
56
import org.springframework.beans.factory.annotation.Autowired
@@ -11,6 +12,7 @@ import org.springframework.scheduling.annotation.Async
1112
import java.util.concurrent.CompletableFuture
1213
import java.util.concurrent.TimeUnit
1314

15+
@ContextRecreatingTest
1416
@SpringBootTest
1517
@Import(AsyncDispatchTargetTest.AsyncProbeConfiguration::class)
1618
class AsyncDispatchTargetTest {

backend/app/src/test/kotlin/io/tolgee/configuration/AsyncExecutorConfigurationTest.kt

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@ import org.springframework.boot.test.context.SpringBootTest
1515
import org.springframework.scheduling.annotation.Async
1616
import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor
1717
import org.springframework.security.task.DelegatingSecurityContextAsyncTaskExecutor
18-
import org.springframework.security.web.SecurityFilterChain
19-
import org.springframework.security.web.header.HeaderWriterFilter
2018
import org.springframework.test.util.ReflectionTestUtils
2119
import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter
2220
import java.util.concurrent.CyclicBarrier
@@ -38,9 +36,6 @@ class AsyncExecutorConfigurationTest {
3836
@Autowired
3937
private lateinit var requestMappingHandlerAdapter: RequestMappingHandlerAdapter
4038

41-
@Autowired
42-
private lateinit var securityFilterChain: SecurityFilterChain
43-
4439
@Autowired
4540
private lateinit var tolgeeProperties: TolgeeProperties
4641

@@ -132,17 +127,6 @@ class AsyncExecutorConfigurationTest {
132127
delegate.assert.isSameAs(streamingAsyncExecutor)
133128
}
134129

135-
@Test
136-
fun `security headers are written before the chain, not while it unwinds`() {
137-
val headerWriterFilter =
138-
securityFilterChain.filters.filterIsInstance<HeaderWriterFilter>().single()
139-
140-
ReflectionTestUtils
141-
.getField(headerWriterFilter, "shouldWriteHeadersEagerly")
142-
.assert
143-
.isEqualTo(true)
144-
}
145-
146130
@Test
147131
fun `the task decorator survives construction`() {
148132
ReflectionTestUtils

backend/app/src/test/kotlin/io/tolgee/configuration/AsyncPropertyBindingTest.kt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
package io.tolgee.configuration
22

3+
import io.tolgee.testing.ContextRecreatingTest
34
import io.tolgee.testing.assert
45
import org.junit.jupiter.api.Test
56
import org.springframework.beans.factory.annotation.Autowired
67
import org.springframework.beans.factory.annotation.Qualifier
78
import org.springframework.boot.test.context.SpringBootTest
89
import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor
910

11+
@ContextRecreatingTest
1012
@SpringBootTest(
1113
properties = [
1214
"tolgee.async.streaming.max-threads = 7",

backend/app/src/test/kotlin/io/tolgee/configuration/StreamingBackpressureHttpTest.kt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package io.tolgee.configuration
22

33
import io.tolgee.Metrics
4+
import io.tolgee.testing.ContextRecreatingTest
45
import io.tolgee.testing.assert
56
import org.junit.jupiter.api.AfterEach
67
import org.junit.jupiter.api.Test
@@ -23,6 +24,7 @@ import java.util.concurrent.TimeUnit
2324
* an unset async result whether or not the 503 actually reaches a client. Only a real container
2425
* proves the rejection becomes a response.
2526
*/
27+
@ContextRecreatingTest
2628
@SpringBootTest(
2729
webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT,
2830
properties = [

0 commit comments

Comments
 (0)