feat(docs): add S3 sync to Modal pipeline for K8s MCP server - #1430
Conversation
Add sync_to_s3() function that uploads generated docs and code databases from Modal volumes to S3 (trycua-docs-mcp-data bucket), bridging the gap so the K8s-hosted MCP server can pull fresh data. Changes: - Add boto3 dependency to Modal image - Declare docs-mcp-s3-readwrite Modal secret - Add sync_to_s3() function uploading SQLite + LanceDB files - Call sync_to_s3 at end of scheduled_crawl (docs, 6 AM UTC) - Call sync_to_s3 at end of scheduled_code_index (code, 5 AM UTC) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds S3 synchronization capability to a Modal documentation and code indexing application. It introduces AWS credentials configuration, creates a ChangesS3 Synchronization Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/scripts/modal_app.py (2)
330-337: Potential concurrent S3 writes from two overlapping scheduled jobs
scheduled_code_indexruns at 05:00 UTC with a 2-hour timeout, andscheduled_crawlruns at 06:00 UTC. Both write to the same S3 keys (e.g.,code_db/code_index.sqlite). If the code index job is still running at 06:00 when the crawl job starts and callssync_to_s3, two Modal containers may upload to the same S3 paths concurrently. S3PUToperations are atomic per-object, but a reader (K8s MCP pod) that pulls multiple files during the overlap window could observe a mix of old and new objects.The suggested 07:00 K8s cron time from the PR description mitigates the K8s reader side, but the two writers can still overlap. Consider adding a per-bucket lock (e.g., an S3 object as a lock file, or a simple naming convention such as writing to a staging prefix and swapping atomically) if data consistency for the K8s consumer matters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts/modal_app.py` around lines 330 - 337, scheduled_code_index and scheduled_crawl can concurrently call sync_to_s3 and overwrite the same S3 keys (e.g., code_db/code_index.sqlite); add a simple per-bucket locking or atomic-swap scheme around sync_to_s3 to prevent overlapping writers: implement a lock object (an S3 lock key or a short-lived lock file) that scheduled_code_index and scheduled_crawl acquire before uploading and release after, or change sync_to_s3 to write to a staging prefix (e.g., staging/<filename>) and then atomically rename/swap to the final prefix once all files are uploaded; ensure the lock acquisition/release or the staging+swap is used by the functions named scheduled_code_index, scheduled_crawl and by sync_to_s3 to guarantee single-writer behavior for code_db/code_index.sqlite.
362-401: ⚡ Quick winNo per-file error handling; a single S3 failure aborts the entire sync
s3.upload_file()can raisebotocore.exceptions.ClientErrororboto3.exceptions.S3UploadFailedError(e.g., transient network error, permissions issue on one key). Currently, the first such failure raises uncaught, leaving the remaining files un-uploaded and theuploadedcounter misleadingly low. Consider wrapping each upload in a try/except to log and continue, then surface a summary of failures at the end.♻️ Proposed pattern
+ failed = [] ... if sqlite_path.exists(): key = "docs_db/docs.sqlite" print(f" Uploading {sqlite_path} -> {key}") - s3.upload_file(str(sqlite_path), bucket, key) - uploaded += 1 + try: + s3.upload_file(str(sqlite_path), bucket, key) + uploaded += 1 + except Exception as e: + print(f" WARNING: failed to upload {key}: {e}") + failed.append(key) ... # At the end: + if failed: + print(f" {len(failed)} file(s) failed to upload: {failed}") - return {"bucket": bucket, "files_uploaded": uploaded} + return {"bucket": bucket, "files_uploaded": uploaded, "files_failed": failed}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts/modal_app.py` around lines 362 - 401, Wrap every call to s3.upload_file in a try/except so a single S3 failure doesn't abort the whole sync: catch botocore.exceptions.ClientError and boto3.exceptions.S3UploadFailedError around the uploads for the sqlite uploads (keys "docs_db/docs.sqlite" and "code_db/code_index.sqlite") and inside both rglob loops for docs.lance and code_index.lancedb; only increment the uploaded counter on success, append failures (file path and exception) to a list, log each failure (use print or processLogger) and after all uploads print a concise summary of total uploaded vs failed entries and the failed paths. Ensure you import the exception classes and update handling in the blocks around docs_db_dir, code_db_dir, docs.lance loop, and code_index.lancedb loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/scripts/modal_app.py`:
- Around line 423-426: Replace the blocking synchronous call to
sync_to_s3.remote() with the async Modal pattern inside the async functions: in
scheduled_crawl (and likewise in scheduled_code_index) use await
sync_to_s3.remote.aio() so the S3 sync does not block the event loop; update the
subsequent print/log to use the awaited result variable instead of the sync
return value.
- Around line 1555-1559: The S3 sync call sync_to_s3.remote() is currently
inside the same try/except that wraps generate_code_index_parallel and
aggregate_code_databases, so any S3 error causes the outer except to return a
zeroed error dict and discard the successful indexing result; move the S3 sync
out of that critical try/except (or wrap only the sync in its own try/except) so
indexing success is preserved: after aggregate_code_databases completes, call
sync_to_s3.remote() in a separate try block, on failure catch the exception, log
the error and record the sync failure in result (e.g., result["s3_sync"] =
{"ok": False, "error": str(err)}), but do not raise from there or overwrite the
successful indexing outputs from
generate_code_index_parallel/aggregate_code_databases so the function still
returns the valid result.
---
Nitpick comments:
In `@docs/scripts/modal_app.py`:
- Around line 330-337: scheduled_code_index and scheduled_crawl can concurrently
call sync_to_s3 and overwrite the same S3 keys (e.g.,
code_db/code_index.sqlite); add a simple per-bucket locking or atomic-swap
scheme around sync_to_s3 to prevent overlapping writers: implement a lock object
(an S3 lock key or a short-lived lock file) that scheduled_code_index and
scheduled_crawl acquire before uploading and release after, or change sync_to_s3
to write to a staging prefix (e.g., staging/<filename>) and then atomically
rename/swap to the final prefix once all files are uploaded; ensure the lock
acquisition/release or the staging+swap is used by the functions named
scheduled_code_index, scheduled_crawl and by sync_to_s3 to guarantee
single-writer behavior for code_db/code_index.sqlite.
- Around line 362-401: Wrap every call to s3.upload_file in a try/except so a
single S3 failure doesn't abort the whole sync: catch
botocore.exceptions.ClientError and boto3.exceptions.S3UploadFailedError around
the uploads for the sqlite uploads (keys "docs_db/docs.sqlite" and
"code_db/code_index.sqlite") and inside both rglob loops for docs.lance and
code_index.lancedb; only increment the uploaded counter on success, append
failures (file path and exception) to a list, log each failure (use print or
processLogger) and after all uploads print a concise summary of total uploaded
vs failed entries and the failed paths. Ensure you import the exception classes
and update handling in the blocks around docs_db_dir, code_db_dir, docs.lance
loop, and code_index.lancedb loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c68b981-69a6-4b3f-9375-28736a26bc51
📒 Files selected for processing (1)
docs/scripts/modal_app.py
| # Sync docs databases to S3 | ||
| print("Syncing docs databases to S3...") | ||
| sync_result = sync_to_s3.remote() | ||
| print(f"S3 sync result: {sync_result}") |
There was a problem hiding this comment.
Use await sync_to_s3.remote.aio() inside the async scheduler
scheduled_crawl is async def and every other remote call it makes uses the async pattern (await ...remote.aio()). Calling sync_to_s3.remote() here is a blocking, synchronous call that stalls the event loop thread for the full duration of the S3 sync. If the sync is slow, this degrades Modal's async scheduler internals.
🔧 Proposed fix
- sync_result = sync_to_s3.remote()
+ sync_result = await sync_to_s3.remote.aio()The same applies at Line 1557 inside scheduled_code_index, which is also async def.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/scripts/modal_app.py` around lines 423 - 426, Replace the blocking
synchronous call to sync_to_s3.remote() with the async Modal pattern inside the
async functions: in scheduled_crawl (and likewise in scheduled_code_index) use
await sync_to_s3.remote.aio() so the S3 sync does not block the event loop;
update the subsequent print/log to use the awaited result variable instead of
the sync return value.
| # Sync code databases to S3 | ||
| print("Syncing code databases to S3...") | ||
| sync_result = sync_to_s3.remote() | ||
| print(f"S3 sync result: {sync_result}") | ||
| result["s3_sync"] = sync_result |
There was a problem hiding this comment.
S3 sync failure silently discards the successful indexing result
sync_to_s3.remote() is inside the same try block (line 1536) as generate_code_index_parallel and aggregate_code_databases. If the S3 sync raises for any reason (bad credentials, network timeout, permission error, etc.), the outer except Exception handler at line 1575 fires and returns a zeroed-out error dict—throwing away the fully valid result from the completed indexing run. The actual databases on the Modal volume are intact; only the K8s download is affected. A S3 sync failure should not mask a successful index.
🛡️ Proposed fix
agg_result = aggregate_code_databases.remote()
print(f"Aggregation complete: {agg_result}")
result["aggregation"] = agg_result
- # Sync code databases to S3
- print("Syncing code databases to S3...")
- sync_result = sync_to_s3.remote()
- print(f"S3 sync result: {sync_result}")
- result["s3_sync"] = sync_result
+ # Sync code databases to S3 (best-effort; don't fail the index job on sync errors)
+ print("Syncing code databases to S3...")
+ try:
+ sync_result = await sync_to_s3.remote.aio()
+ print(f"S3 sync result: {sync_result}")
+ result["s3_sync"] = sync_result
+ except Exception as sync_err:
+ print(f"Warning: S3 sync failed (indexing still succeeded): {sync_err}")
+ result["s3_sync"] = {"error": str(sync_err)}
return result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/scripts/modal_app.py` around lines 1555 - 1559, The S3 sync call
sync_to_s3.remote() is currently inside the same try/except that wraps
generate_code_index_parallel and aggregate_code_databases, so any S3 error
causes the outer except to return a zeroed error dict and discard the successful
indexing result; move the S3 sync out of that critical try/except (or wrap only
the sync in its own try/except) so indexing success is preserved: after
aggregate_code_databases completes, call sync_to_s3.remote() in a separate try
block, on failure catch the exception, log the error and record the sync failure
in result (e.g., result["s3_sync"] = {"ok": False, "error": str(err)}), but do
not raise from there or overwrite the successful indexing outputs from
generate_code_index_parallel/aggregate_code_databases so the function still
returns the valid result.
Replace the docs-mcp-s3-readwrite Modal secret (static IAM keys) with OIDC federation using the existing modal-docs-mcp-write-role IAM role from cloud repo's terraform/aws/docs-mcp-storage/main.tf. Modal auto-injects MODAL_IDENTITY_TOKEN, which we use via STS AssumeRoleWithWebIdentity to get temporary credentials. No Modal secrets or key rotation needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Agent-friendly handoff doc covering full context, deploy steps, test procedure, and troubleshooting for the sync_to_s3 function. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
sync_to_s3()Modal function that uploads generated docs and code databases (SQLite + LanceDB) from Modal volumes to S3 (trycua-docs-mcp-databucket)modal-docs-mcp-write-roleIAM role — no static AWS keys or Modal secrets neededscheduled_crawl(6 AM UTC) andscheduled_code_index(5 AM UTC) so S3 is updated after every regenerationboto3to the Modal image for S3/STS operationsThis bridges the missing link: Modal (generates DBs) → S3 → K8s CronJob → PVC → MCP Server
How OIDC auth works
MODAL_IDENTITY_TOKENenv var into running functionssync_to_s3()calls STSAssumeRoleWithWebIdentitywith that tokencloudrepoterraform/aws/docs-mcp-storage/main.tf) is scoped toworkspace_id:ac-3LfmQEOVnLXl4ns0YBxNuA:app_name:cua-docs-mcpNo Modal secrets to create or rotate.
Deploy & test
Timeline note
K8s CronJob currently syncs at 2 AM UTC, but Modal generates data at 5 AM (code) and 6 AM (docs). Consider updating the K8s CronJob schedule from
0 2 * * *to0 7 * * *so it runs after Modal completes.Test plan
modal deploy docs/scripts/modal_app.pysucceedsmodal run docs/scripts/modal_app.py::sync_to_s3uploads files to S3aws s3 ls s3://trycua-docs-mcp-data/ --recursiveshows bothdocs_db/andcode_db/🤖 Generated with Claude Code