Skip to content
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.

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

[BUG] Fix recursion in auth_and_get_tenant_and_database_for_request #3514

Conversation

tazarov
Copy link
Contributor

@tazarov tazarov commented Jan 20, 2025

Description of changes

Closes #3498

Summarize the changes made by this PR.

  • Improvements & Bug fixes
    • Fixes a recursion in auth_and_get_tenant_and_database_for_request to correctly call sync_auth_and_get_tenant_and_database_for_request in run_sync context

Test plan

How are these changes tested?

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

N/A

Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

Copy link
Contributor Author

tazarov commented Jan 20, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@tazarov tazarov requested a review from rescrv January 23, 2025 13:05
@tazarov
Copy link
Contributor Author

tazarov commented Jan 23, 2025

@rescrv, PTAL, I might be wrong but the current impl has recursion that might not be intended.

@HammadB HammadB merged commit 4d8178d into main Jan 29, 2025
80 checks passed
@tazarov tazarov deleted the trayan-01-20-feat_fix_recursion_in_auth_and_get_tenant_and_database_for_request branch February 2, 2025 16:27
@PhemeloDev
Copy link

@tazarov, @HammadB this issue: #3798 is not resolved

I have added more details to the ticket, please reopen.

chromadb

Hi this bug is not resolved.

I ran

docker run -d -p 8000:8000 --name chroma -e CHROMA_SERVER_CORS_ALLOW_ORIGINS='["*"]' chromadb/chroma:latest

the container starts successfully but I get a 400: [17-02-2025 06:07:51] 172.17.0.1:39662 - "GET /api/v1/collections HTTP/1.1" 400

logs

2025-02-17 08:07:37 Starting 'uvicorn chromadb.app:app' with args: --workers 1 --host 0.0.0.0 --port 8000 --proxy-headers --log-config chromadb/log_config.yml --timeout-keep-alive 30 2025-02-17 08:07:39 WARNING: [17-02-2025 06:07:39] chroma_server_nofile is set to 65536, but this is less than current soft limit of 1048576. chroma_server_nofile will not be set. 2025-02-17 08:07:40 INFO: [17-02-2025 06:07:40] Anonymized telemetry enabled. See https://docs.trychroma.com/telemetry for more information. 2025-02-17 08:07:40 DEBUG: [17-02-2025 06:07:40] Starting component System 2025-02-17 08:07:40 DEBUG: [17-02-2025 06:07:40] Starting component OpenTelemetryClient 2025-02-17 08:07:40 DEBUG: [17-02-2025 06:07:40] Starting component SqliteDB 2025-02-17 08:07:40 DEBUG: [17-02-2025 06:07:40] Starting component SimpleQuotaEnforcer 2025-02-17 08:07:40 DEBUG: [17-02-2025 06:07:40] Starting component Posthog 2025-02-17 08:07:40 DEBUG: [17-02-2025 06:07:40] Starting component SimpleRateLimitEnforcer 2025-02-17 08:07:40 DEBUG: [17-02-2025 06:07:40] Starting component LocalSegmentManager 2025-02-17 08:07:40 DEBUG: [17-02-2025 06:07:40] Starting component LocalExecutor 2025-02-17 08:07:40 DEBUG: [17-02-2025 06:07:40] Starting component SegmentAPI 2025-02-17 08:07:40 DEBUG: [17-02-2025 06:07:40] Starting component SimpleAsyncRateLimitEnforcer 2025-02-17 08:07:40 INFO: [17-02-2025 06:07:40] Started server process [1] 2025-02-17 08:07:40 INFO: [17-02-2025 06:07:40] Waiting for application startup. 2025-02-17 08:07:40 INFO: [17-02-2025 06:07:40] Application startup complete. 2025-02-17 08:07:40 INFO: [17-02-2025 06:07:40] Uvicorn running on http://0.0.0.0:8000 (Press CTRL+C to quit) 2025-02-17 08:07:51 INFO: [17-02-2025 06:07:51] 172.17.0.1:39662 - "GET /api/v1/collections HTTP/1.1" 400

@tazarov
Copy link
Contributor Author

tazarov commented Feb 17, 2025

@PhemeloDev, no new release containing the fix has been made. Use the latest dev release docker pull chromadb/chroma:0.6.4.dev226

@dhqanh-rcc
Copy link

Use the latest dev release docker pull chromadb/chroma:0.6.4.dev226. This issue is resolved

@oncelyunus
Copy link

oncelyunus commented Mar 20, 2025

i tested and
it's resolved finally. thanks team :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Issue with Docker Image - InvalidArgumentError in /api/v1/collections
6 participants