Fix: Handle missing auth token gracefully in logfile upload - Resolves #1269
๐ฅ 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:
- Added auth token check in
upload_logfile()before attempting upload - Skip upload gracefully with debug message if auth not complete
- Clear buffer in both success and skip scenarios
- Added unit tests for both authenticated and unauthenticated scenarios
๐งช 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 None -
test_upload_logfile_uploads_when_auth_token_is_set()- Verifies upload proceeds normally when auth token is set - Both tests pass โ
-
-
Lint Checks: All ruff checks pass โ
๐ Review Focus Areas
โ ๏ธ Important: Please review the following:
-
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:
- Link to Devin run: https://app.agentops.ai/sessions/85ded112b5314bb49ef3bb8b99d26f12
- Requested by: Alex ([email protected]) / @areibman
Devin Session: https://app.devin.ai/sessions/85ded112b5314bb49ef3bb8b99d26f12
๐ค Devin AI Engineer
I'll be helping with this pull request! Here's what you should know:
โ I will automatically:
- Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
- Look at CI failures and help fix them
Note: I can only respond to comments from users who have write access to this repository.
โ๏ธ Control Options:
- [ ] Disable automatic comment and CI monitoring