Fix: Handle missing auth token gracefully in logfile upload - Resolves #1269#1270
Open
devin-ai-integration[bot] wants to merge 2 commits intomainfrom
Open
Fix: Handle missing auth token gracefully in logfile upload - Resolves #1269#1270devin-ai-integration[bot] wants to merge 2 commits intomainfrom
devin-ai-integration[bot] wants to merge 2 commits intomainfrom
Conversation
…#1269 The issue was that upload_logfile() was being called when a trace ended, but the async authentication task may not have completed yet, causing the V4 client to not have the auth token set. This resulted in a 401 error. Changes: - Check if auth token is available before attempting to upload logfile - Skip upload gracefully with debug message if auth hasn't completed yet - Clear buffer even when skipping upload to prevent memory leaks - Add tests to verify the fix handles both scenarios correctly This fix ensures that short-running traces that complete before authentication finishes no longer throw 401 errors. Co-Authored-By: Alex <meta.alex.r@gmail.com>
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
The tests were calling setup_print_logger() which monkey-patches builtins.print, but weren't restoring it afterward. This leaked state was causing widespread CI failures across unit tests and example tests. Added reset_print fixture to properly restore builtins.print after each test, mirroring the pattern used in test_instrument_logging.py. Co-Authored-By: Alex <meta.alex.r@gmail.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📥 Pull Request
📘 Description
Fixes #1269 - Resolves 401 error when uploading logfiles for short-running traces.
Root Cause: The
upload_logfile()function was being called when traces ended, but the async authentication task running in the background may not have completed yet. This resulted in the V4 client not having an auth token set, causing a 401 Unauthorized error.Solution: Added a check to verify the auth token is available before attempting to upload the logfile. If authentication hasn't completed yet, the upload is skipped gracefully with a debug message instead of throwing a 401 error. The log buffer is still cleared to prevent memory leaks.
This is particularly common for short-running traces that complete before the async authentication finishes (typically takes a few hundred milliseconds).
Changes:
upload_logfile()before attempting upload🧪 Testing
Unit Tests: Added
test_upload_logfile_no_auth.pywith two test cases:test_upload_logfile_skips_when_no_auth_token()- Verifies upload is skipped when auth token is Nonetest_upload_logfile_uploads_when_auth_token_is_set()- Verifies upload proceeds normally when auth token is setLint Checks: All ruff checks pass ✅
🔍 Review Focus Areas
Behavior Change: The fix changes behavior from throwing a 401 error to silently skipping with a debug log. Is this the desired behavior, or should we retry/queue the upload instead?
Logging Level: Currently using
logger.debug(). Should this belogger.warning()orlogger.info()instead to make it more visible?Buffer Clearing: The fix clears the buffer even when skipping upload (to prevent memory leaks). This means logs from unauthenticated sessions are lost. Confirm this is acceptable.
Thread Safety: The auth token check
if not client.api.v4.auth_tokenassumes this is thread-safe with the async auth task. Review the Client class implementation to confirm.Session Info:
Devin Session: https://app.devin.ai/sessions/85ded112b5314bb49ef3bb8b99d26f12