ECHO-465 Non-blocking uploads with presigned URLs + Kubernetes worker optimization#318
ECHO-465 Non-blocking uploads with presigned URLs + Kubernetes worker optimization#318
Conversation
- Added .env and notes.md to .gitignore to prevent sensitive files from being tracked. - Changed API proxy target in vite.config.ts from localhost:8000 to localhost:8001. - Enhanced DIRECTUS_PUBLIC_URL logic in config.ts to support both absolute and relative URLs. - Added trankit and torch as dependencies in pyproject.toml and updated requirements files accordingly, ensuring compatibility with existing packages.
- API: Switch from uvicorn to gunicorn (2 workers/pod) - Network workers: 3×50 → 2×20 (per pod) - CPU workers: 4×6 → 2×4 (per pod) - Add gunicorn==21.2.* dependency - All configurations via environment variables - Designed for K8s horizontal pod scaling Configuration: - API_WORKERS=2 (default) - NETWORK_WORKER_PROCESSES=2, THREADS=20 (default) - CPU_WORKER_PROCESSES=2, THREADS=4 (default) All values configurable via env vars without code changes. Risk: LOW - No breaking changes, easy rollback Tested: Shell syntax validated, dependencies verified
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an S3 presigned‑post upload flow (frontend + backend) with confirm endpoint and rate limiting, hardens S3 key validation, adjusts frontend config and Vite proxy, makes runtime scripts Kubernetes‑friendly, adds Gunicorn dependency, and disables the NER/anonymization pipeline. Also updates .gitignore. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
LGTM. Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-10-03T13:45:53.929ZApplied to files:
📚 Learning: 2025-10-06T16:19:09.311ZApplied to files:
🧬 Code graph analysis (1)echo/server/dembrane/api/participant.py (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @dtrn2048! Thank you for contributing to Dembrane ECHO! Before we consider your Pull Request, we ask that you sign our Contributor License Agreement (CLA). This is only required for your first Pull Request. Please review the CLA, and sign it by adding your GitHub username to the contributors.yml file. Thanks! |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
echo/frontend/src/lib/api.ts (1)
671-777: Finish hook only fires for the first conversationLine [671] seeds
conversationIdonce, while Line [689] spins up a fresh conversation per file. When you drop in multiple files, only that first conversation ever reaches the/finishcall on Line [769], so every other conversation stays stuck without ETL/summarization. Multi-file uploads silently die after item one. Let's track every created conversation id and finish each one before we return.- let conversationId: string | null = null; + const conversationIds = new Set<string>(); ... - if (!conversationId) { - conversationId = conversation.id; - } + conversationIds.add(conversation.id); ... - if (conversationId && failures.length === 0) { - console.log(`[Upload] Triggering finish hook for conversation ${conversationId}`); - try { - await finishConversation(conversationId); - console.log(`[Upload] Finish hook triggered successfully`); - } catch (error) { - console.error('[Upload] Failed to trigger finish hook:', error); - // Don't throw - uploads succeeded, this is just post-processing - } - } + if (conversationIds.size > 0 && failures.length === 0) { + for (const id of conversationIds) { + console.log(`[Upload] Triggering finish hook for conversation ${id}`); + try { + await finishConversation(id); + console.log(`[Upload] Finish hook triggered successfully for ${id}`); + } catch (error) { + console.error(`[Upload] Failed to trigger finish hook for ${id}:`, error); + // Don't throw - uploads succeeded, this is just post-processing + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
echo/server/requirements-dev.lockis excluded by!**/*.lockecho/server/requirements.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.gitignore(0 hunks)echo/frontend/src/config.ts(1 hunks)echo/frontend/src/lib/api.ts(3 hunks)echo/frontend/vite.config.ts(1 hunks)echo/server/dembrane/api/participant.py(4 hunks)echo/server/dembrane/s3.py(2 hunks)echo/server/dembrane/service/conversation.py(4 hunks)echo/server/prod-worker-cpu.sh(1 hunks)echo/server/prod-worker.sh(1 hunks)echo/server/prod.sh(1 hunks)echo/server/pyproject.toml(2 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (2)
echo/server/dembrane/service/conversation.py (1)
echo/server/dembrane/directus.py (2)
DirectusBadRequest(27-28)directus_client_context(32-40)
echo/server/dembrane/api/participant.py (3)
echo/server/dembrane/utils.py (2)
generate_uuid(13-14)get(67-79)echo/server/dembrane/service/conversation.py (4)
get_by_id_or_raise(46-83)ConversationNotFoundException(25-26)ConversationNotOpenForParticipationException(29-30)create_chunk(206-283)echo/server/dembrane/s3.py (3)
get_sanitized_s3_key(200-236)generate_presigned_post(154-197)get_file_size_bytes_from_s3(251-254)
🪛 Shellcheck (0.11.0)
echo/server/prod.sh
[info] 16-16: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 19-19: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 22-22: Double quote to prevent globbing and word splitting.
(SC2086)
echo/server/prod-worker.sh
[info] 14-14: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 15-15: Double quote to prevent globbing and word splitting.
(SC2086)
echo/server/prod-worker-cpu.sh
[info] 14-14: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 15-15: Double quote to prevent globbing and word splitting.
(SC2086)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: ci-check-server
🔇 Additional comments (2)
echo/frontend/vite.config.ts (1)
38-38: LGTM! Port shift tracks the backend reconfig.Clean change, aligns with the API server updates mentioned in the PR objectives. No issues here.
echo/frontend/src/config.ts (1)
9-19: LGTM! Solid URL resolution pattern.The IIFE handles absolute URLs, relative paths, and fallback cases cleanly. Good defensive logic for the Vite proxy setup and dynamic port scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.gitignore(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
echo/server/dembrane/api/participant.py(3 hunks)echo/server/dembrane/service/conversation.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
echo/server/dembrane/service/conversation.py (2)
echo/server/dembrane/directus.py (2)
DirectusBadRequest(27-28)directus_client_context(32-40)echo/server/dembrane/utils.py (1)
generate_uuid(13-14)
echo/server/dembrane/api/participant.py (3)
echo/server/dembrane/s3.py (3)
get_file_size_bytes_from_s3(251-254)get_sanitized_s3_key(200-236)generate_presigned_post(154-197)echo/server/dembrane/service/conversation.py (4)
ConversationNotFoundException(25-26)ConversationNotOpenForParticipationException(29-30)get_by_id_or_raise(46-83)create_chunk(206-283)echo/server/dembrane/utils.py (2)
generate_uuid(13-14)get(67-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci-check-server
🔇 Additional comments (2)
echo/server/dembrane/service/conversation.py (2)
3-11: LGTM—Logger setup is clean.Standard Python logging pattern with qualified module name. No issues here.
279-281: Clean background task trigger.Comment and logging are clear. No issues.
- Introduced a new function `sanitize_url_for_logging` to remove sensitive query parameters and fragments from URLs before logging. - Updated logging statements in the `ConversationService` to use the new sanitization function for file URLs.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
echo/server/dembrane/service/conversation.py (1)
262-264: Problem solved. Logging is now safe.Both branches sanitize
file_urlbefore logging—credentials no longer leak into log streams. The messages clearly distinguish API uploads from presigned URL uploads, which is clutch for debugging the new flow. This closes out the past review concern.echo/server/dembrane/api/participant.py (1)
436-450: Return 404 when the conversation is gone.If
conversation_service.create_chunkraisesConversationNotFoundException, we fall through to the catch-all and ship a 500, which is the wrong contract. Catch it explicitly and surface the expected 404 before the generic handler.except ConversationNotOpenForParticipationException as e: logger.error(f"Conversation not open for participation: {conversation_id}") raise HTTPException( status_code=403, detail="Conversation not open for participation" ) from e + except ConversationNotFoundException as e: + logger.error(f"Conversation not found while confirming upload: {conversation_id}") + raise HTTPException(status_code=404, detail="Conversation not found") from e except HTTPException: # Re-raise HTTP exceptions as-is raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.gitignore(1 hunks)echo/server/dembrane/api/participant.py(3 hunks)echo/server/dembrane/service/conversation.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
echo/server/dembrane/service/conversation.py (1)
echo/server/dembrane/directus.py (2)
DirectusBadRequest(27-28)directus_client_context(32-40)
echo/server/dembrane/api/participant.py (3)
echo/server/dembrane/utils.py (2)
generate_uuid(13-14)get(67-79)echo/server/dembrane/s3.py (3)
get_file_size_bytes_from_s3(251-254)get_sanitized_s3_key(200-236)generate_presigned_post(154-197)echo/server/dembrane/service/conversation.py (4)
get_by_id_or_raise(53-90)ConversationNotFoundException(32-33)ConversationNotOpenForParticipationException(36-37)create_chunk(213-290)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: ci-check-server
🔇 Additional comments (6)
.gitignore (1)
14-15: Ship it. Tight addition—keeping env blobs and scratch pads out of git is exactly the move.echo/server/dembrane/service/conversation.py (5)
2-5: Ship it. Imports are clean.Standard library imports for logging and URL parsing—exactly what you need for the sanitization helper. No bloat, no drama.
12-13: LGTM. Logger setup is textbook.Module-scoped logger with a qualified name—perfect for tracing in production when you're hunting down that one rogue chunk upload at 3AM.
22-26: Nailed it. URL sanitization is solid.This helper strips sensitive query params and fragments—exactly what the past review asked for. Presigned URL signatures stay out of your log aggregators. Clean implementation, zero complaints.
273-273: Better, but still needs upstream validation.Always using
file_urlforpathis the right move—it fixes the presigned URL case. However, if bothfile_objandfile_urlareNone,pathwill still beNone, and downstreamsplit_audio_chunkwill blow up withFileNotFoundError. The past review requested API-level validation to reject requests where neither is provided. This change improves the situation but doesn't close the loop.Add a guard in
create_chunkbefore chunk creation:+ if file_url is None and transcript is None: + raise ConversationServiceException("Either file_obj/file_url or transcript must be provided") + with directus_client_context() as client: chunk = client.create_item(Or enforce at the API boundary in
dembrane/api/participant.pybefore callingcreate_chunk.
286-287: Good call. Observability FTW.Logging before the async task dispatch gives you a clear trail when you're debugging chunk processing in prod. Helps correlate API calls with background job execution. Clean and useful.
- Updated `prod-worker-cpu.sh` and `prod-worker.sh` to quote the `$PROCESSES` and `$THREADS` variables for improved handling of values with spaces or special characters.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
echo/server/dembrane/api/participant.py (5)
95-123: Rate limiter still process-local (known issue).The past review comment about multi-worker/pod rate limit bypass remains unaddressed. This is expected if it's a known limitation being tracked separately.
323-324: Redundant local import (known issue).Past review comment about the duplicate
get_sanitized_s3_keyimport applies here.
332-339: Redundant local import (known issue).Past review comment about the duplicate
generate_presigned_postimport applies here.
342-342: Double-slash risk in file_url (known issue).Past review comment about normalizing
STORAGE_S3_ENDPOINTto prevent double slashes applies here.
388-417: Catch ValueError from get_sanitized_s3_key (known issue).Past review comment about handling
ValueErrorfromget_sanitized_s3_keyat line 390 applies here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
echo/server/dembrane/api/participant.py(3 hunks)echo/server/prod-worker-cpu.sh(1 hunks)echo/server/prod-worker.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
echo/server/dembrane/api/participant.py (3)
echo/server/dembrane/utils.py (2)
generate_uuid(13-14)get(67-79)echo/server/dembrane/s3.py (3)
get_file_size_bytes_from_s3(251-254)get_sanitized_s3_key(200-236)generate_presigned_post(154-197)echo/server/dembrane/service/conversation.py (4)
get_by_id_or_raise(53-90)ConversationNotFoundException(32-33)ConversationNotOpenForParticipationException(36-37)create_chunk(213-290)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: ci-check-server
🔇 Additional comments (2)
echo/server/dembrane/api/participant.py (2)
442-444: ConversationNotFoundException now handled correctly.The past review comment about catching
ConversationNotFoundExceptionexplicitly has been addressed. 404 contract is now correct.
395-417: LGTM: Retry logic handles S3 eventual consistency.The 3-attempt backoff (100ms → 500ms → 2s) with clear logging is solid. Ships 400 after exhausting retries, which is the right contract for "upload failed or S3 unavailable."
- Updated `prod.sh` to quote the `$WORKERS`, `$TIMEOUT`, and `$MAX_REQUESTS` variables to ensure proper handling of values with spaces or special characters.
- Added validation to ensure a chunk has either an audio file or a transcript before creation, raising a `ConversationServiceException` if both are missing. - Updated error handling in the API to catch `ConversationServiceException` and return a 400 HTTP error with a detailed message. - Improved logging for chunk creation failures and clarified processing behavior based on the presence of audio files.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
echo/server/dembrane/ner.py(1 hunks)echo/server/dembrane/quote_utils.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: ci-check-server
🔇 Additional comments (1)
echo/server/dembrane/quote_utils.py (1)
160-160: Using raw transcripts without anonymization.LGTM from a code perspective—you're just pulling
chunk.transcriptdirectly. But the privacy implications are massive. Raw transcripts might contain names, emails, phone numbers, etc. Make sure the legal team signed off on this before it ships.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… and ensure proper function definition fix: Remove unused import of get_sanitized_s3_key in get_chunk_upload_url fix: Add missing newline at end of run-worker scripts for better compatibility
dtrn2048
changed the title
…ies (excl trankit + torch)
spashii
left a comment
spashii
left a comment
There was a problem hiding this comment.
All changes are safe except vite.config.ts - 8001 port change. I think you should try out the dev containers. The networking / containers setup is managed through it.
… optimization (#318) ## Summary Implements non-blocking file uploads using S3 presigned URLs and reconfigures workers for Kubernetes horizontal pod autoscaling. ## Changes ### Non-blocking Upload System - Frontend: New `uploadConversationChunkWithPresignedUrl` function - Backend: Endpoints for presigned URL generation and upload confirmation - API server no longer handles file transfers - Direct client to S3 uploads - Retry logic: 3 attempts with exponential backoff - Rate limiting: 10 requests/min per conversation - Progress tracking through upload stages ### Worker Configuration - API: Switched from uvicorn to gunicorn (2 workers/pod) - Network workers: 3×50 → 2×20 threads per pod - CPU workers: 4×6 → 2×4 threads per pod - All settings configurable via environment variables: - `API_WORKERS=2` - `NETWORK_WORKER_PROCESSES=2`, `THREADS=20` - `CPU_WORKER_PROCESSES=2`, `THREADS=4` ### Other - Fixed DIRECTUS_PUBLIC_URL handling for absolute/relative URLs - Added dependencies: gunicorn, trankit, torch - Updated vite proxy to localhost:8001 ## Files Changed **Backend:** - `dembrane/api/participant.py`: Presigned URL endpoints - `dembrane/s3.py`: URL generation logic - `dembrane/service/conversation.py`: Chunk handling updates - `prod*.sh`: Worker configuration scripts - `pyproject.toml`: Dependencies **Frontend:** - `lib/api.ts`: Upload implementation - `config.ts`: URL handling - `vite.config.ts`: Proxy config ## Notes - No breaking changes to existing upload functionality - Worker settings now environment-configurable - Risk: LOW, easy rollback --- **Commits:** - feat: optimize workers for Kubernetes horizontal scaling - Update configuration and dependencies <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Direct-to-storage uploads via presigned POSTs with per-file progress, retries, finish hook, and retained legacy upload path; server endpoints to request and confirm uploads with per-conversation rate limiting. * **Bug Fixes** * Chunk creation now records file URLs consistently; safer URL logging and stricter filename/key validation to prevent path traversal; clearer upload/confirmation error handling. * **Chores** * Kubernetes-friendly startup scripts, switched API runtime to Gunicorn and added dependency, improved public-URL resolution and dev proxy target, expanded .gitignore, and NER anonymization pipeline disabled. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary
Implements non-blocking file uploads using S3 presigned URLs and reconfigures workers for Kubernetes horizontal pod autoscaling.
Changes
Non-blocking Upload System
uploadConversationChunkWithPresignedUrlfunctionWorker Configuration
API_WORKERS=2NETWORK_WORKER_PROCESSES=2,THREADS=20CPU_WORKER_PROCESSES=2,THREADS=4Other
Files Changed
Backend:
dembrane/api/participant.py: Presigned URL endpointsdembrane/s3.py: URL generation logicdembrane/service/conversation.py: Chunk handling updatesprod*.sh: Worker configuration scriptspyproject.toml: DependenciesFrontend:
lib/api.ts: Upload implementationconfig.ts: URL handlingvite.config.ts: Proxy configNotes
Commits:
Summary by CodeRabbit
New Features
Bug Fixes
Chores