Skip to content

fix(api): close TOCTOU race in RateLimit.enter()#39179

Open
amogh-nagri-11 wants to merge 3 commits into
langgenius:mainfrom
amogh-nagri-11:fix/39177
Open

fix(api): close TOCTOU race in RateLimit.enter()#39179
amogh-nagri-11 wants to merge 3 commits into
langgenius:mainfrom
amogh-nagri-11:fix/39177

Conversation

@amogh-nagri-11

Copy link
Copy Markdown

Fixes #39177

RateLimit.enter() (api/core/app/features/rate_limiting/rate_limit.py) used two separate, unsynchronized Redis round-trips — hlen() to check the active-request count against max_active_requests, then hset() to register the request — to enforce a per-app concurrency cap. This is a classic check-then-act race: under concurrent requests, multiple callers could all read a count below the cap before any of them wrote, so all of them get admitted, letting the active count exceed max_active_requests.

Replaced the two-step check with a single atomic Redis Lua script (_ENTER_SCRIPT) that checks HLEN against the cap and conditionally does the HSET in one round trip, closing the race entirely (Redis executes scripts single-threaded, so no other client can interleave between the check and the write).

-        active_requests_count = redis_client.hlen(self.active_requests_key)
-        if active_requests_count >= self.max_active_requests:
-            raise AppInvokeQuotaExceededError(...)
-        redis_client.hset(self.active_requests_key, request_id, str(time.time()))
+        prefixed_key = serialize_redis_name_arg(self.active_requests_key, get_redis_key_prefix())
+        admitted = redis_client.eval(
+            _ENTER_SCRIPT, 1, prefixed_key, self.max_active_requests, request_id, str(time.time())
+        )
+        if not admitted:
+            raise AppInvokeQuotaExceededError(...)

Note: eval isn't wrapped by RedisClientWrapper (unlike hlen/hset, which auto-prefix keys), so the key is prefixed manually via the same serialize_redis_name_arg/get_redis_key_prefix helpers the wrapper uses internally, to keep behavior consistent with REDIS_KEY_PREFIX config.

No dependencies added.

Screenshots

N/A — backend-only logic fix, no UI change.

Checklist

  • This change requires a documentation update, included: Dify Document
  • I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.
  • I ran make lint && make type-check (backend) and cd web && pnpm exec vp staged (frontend) to appease the lint gods

session.add/session.commit for a new segment ran outside the
redis_client.lock covering the max-position read, so concurrent
create_segment calls on the same document could read the same
max_position and commit duplicate segment positions.
hlen() and hset() were two separate, unsynchronized Redis round-trips,
letting concurrent requests all read a count under the cap and all get
admitted, exceeding max_active_requests. Replaced with a single atomic
Lua script that checks-and-sets in one round trip.
@github-actions

Copy link
Copy Markdown
Contributor

Pyrefly Diff

base → PR
--- /tmp/pyrefly_base.txt	2026-07-17 05:20:35.968975799 +0000
+++ /tmp/pyrefly_pr.txt	2026-07-17 05:20:25.581880003 +0000
@@ -4118,15 +4118,15 @@
 ERROR `None` is not subscriptable [unsupported-operation]
   --> tests/unit_tests/core/app/entities/test_task_entities.py:58:16
 ERROR Object of class `Mapping` has no attribute `close` [missing-attribute]
-   --> tests/unit_tests/core/app/features/rate_limiting/test_rate_limit.py:396:9
+   --> tests/unit_tests/core/app/features/rate_limiting/test_rate_limit.py:392:9
 ERROR Argument `TestRateLimitGenerator.test_should_handle_generator_without_close_method.SimpleGenerator` is not assignable to parameter `generator` with type `Generator[str] | Mapping[str, Any]` in function `core.app.features.rate_limiting.rate_limit.RateLimit.generate` [bad-argument-type]
-   --> tests/unit_tests/core/app/features/rate_limiting/test_rate_limit.py:429:43
+   --> tests/unit_tests/core/app/features/rate_limiting/test_rate_limit.py:425:43
 ERROR Object of class `Mapping` has no attribute `close` [missing-attribute]
-   --> tests/unit_tests/core/app/features/rate_limiting/test_rate_limit.py:430:9
+   --> tests/unit_tests/core/app/features/rate_limiting/test_rate_limit.py:426:9
 ERROR Object of class `Mapping` has no attribute `close` [missing-attribute]
-   --> tests/unit_tests/core/app/features/rate_limiting/test_rate_limit.py:448:9
+   --> tests/unit_tests/core/app/features/rate_limiting/test_rate_limit.py:444:9
 ERROR Argument `Mapping[str, Any] | RateLimitGenerator` is not assignable to parameter `i` with type `SupportsNext[@_]` in function `next` [bad-argument-type]
-   --> tests/unit_tests/core/app/features/rate_limiting/test_rate_limit.py:451:18
+   --> tests/unit_tests/core/app/features/rate_limiting/test_rate_limit.py:447:18
 ERROR Argument `SimpleNamespace` is not assignable to parameter `app_record` with type `App` in function `core.app.features.annotation_reply.annotation_reply.AnnotationReplyFeature.query` [bad-argument-type]
   --> tests/unit_tests/core/app/features/test_annotation_reply.py:68:24
 ERROR Argument `SimpleNamespace` is not assignable to parameter `message` with type `Message` in function `core.app.features.annotation_reply.annotation_reply.AnnotationReplyFeature.query` [bad-argument-type]
@@ -7869,13 +7869,13 @@
 ERROR Argument `SimpleNamespace` is not assignable to parameter `dataset` with type `Dataset` in function `services.dataset_service.SegmentService.delete_child_chunk` [bad-argument-type]
    --> tests/unit_tests/services/test_dataset_service_segment.py:280:64
 ERROR Argument `SimpleNamespace` is not assignable to parameter `segment` with type `DocumentSegment` in function `services.dataset_service.SegmentService.update_child_chunk` [bad-argument-type]
-   --> tests/unit_tests/services/test_dataset_service_segment.py:946:49
+    --> tests/unit_tests/services/test_dataset_service_segment.py:1003:49
 ERROR Argument `SimpleNamespace` is not assignable to parameter `document` with type `Document` in function `services.dataset_service.SegmentService.update_child_chunk` [bad-argument-type]
-   --> tests/unit_tests/services/test_dataset_service_segment.py:946:68
+    --> tests/unit_tests/services/test_dataset_service_segment.py:1003:68
 ERROR Argument `SimpleNamespace` is not assignable to parameter `dataset` with type `Dataset` in function `services.dataset_service.SegmentService.update_child_chunk` [bad-argument-type]
-   --> tests/unit_tests/services/test_dataset_service_segment.py:946:87
+    --> tests/unit_tests/services/test_dataset_service_segment.py:1003:87
 ERROR Argument `SimpleNamespace` is not assignable to parameter `dataset` with type `Dataset` in function `services.dataset_service.SegmentService.delete_child_chunk` [bad-argument-type]
-   --> tests/unit_tests/services/test_dataset_service_segment.py:960:60
+    --> tests/unit_tests/services/test_dataset_service_segment.py:1017:60
 ERROR Argument `str` is not assignable to parameter `type` with type `Literal['basic', 'bearer', 'custom'] | None` in function `services.entities.external_knowledge_entities.external_knowledge_entities.AuthorizationConfig.__init__` [bad-argument-type]
    --> tests/unit_tests/services/test_external_dataset_service.py:118:60
 ERROR Argument `str` is not assignable to parameter `type` with type `Literal['api-key', 'no-auth']` in function `services.entities.external_knowledge_entities.external_knowledge_entities.Authorization.__init__` [bad-argument-type]

@github-actions

Copy link
Copy Markdown
Contributor

Pyrefly Type Coverage

Metric Base PR Delta
Type coverage 53.99% 53.99% -0.00%
Strict coverage 53.48% 53.48% -0.00%
Typed symbols 34,364 34,364 0
Untyped symbols 29,561 29,563 +2
Modules 3029 3029 0

…test

create_segment takes session as an explicit parameter and never touches
the module-level db global, so patching it was unnecessary and appears
fragile under CI's parallel (-n auto) test execution, where the patch
target intermittently reported "module has no attribute db". Build the
mock session directly instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition in RateLimit.enter() allows max_active_requests cap to be exceeded under concurrent requests

1 participant