Skip to content

Conversation

barapa
Copy link
Contributor

@barapa barapa commented Oct 15, 2025

  • Previously save/delete failures were only logged, so callers believed commits succeeded when storage ops had already failed.
  • Sync commit: processes saves sequentially, then deletes. Logs failures with tracebacks and re-raises. Ignores FileNotFoundError on delete. Stops on first save error. Clears internal state only on full success.
  • Async commit: runs saves and deletes concurrently via asyncio.gather. Logs each failure with exc_info. Raises the first real Exception, and lets BaseException (e.g., CancelledError) bubble. Attempts deletes even if a save fails. Clears state only on full success.
  • Rollback (sync/async): deletes only files saved during the current transaction. Ignores FileNotFoundError. Logs and re-raises other errors. Clears state after processing.
  • Tracking: records successful saves in _saved_in_transaction for targeted rollback cleanup; state is retained on error to allow inspection/retry.

Tests

  • Sync/async save and delete error paths, including FileNotFoundError handling and BaseException bubbling.
  • Override semantics: save cancels prior delete; delete cancels prior save.
  • State behavior: clear on success; retain on error.
  • Async specifics: delete still attempted even if a save fails; raises first failing Exception in input order; logs include exc_info for stack traces.

@barapa barapa requested review from a team as code owners October 15, 2025 19:51
@barapa barapa changed the title fix: improve session tracker error logging fix: improve session tracker error logging and propagation Oct 15, 2025
@barapa
Copy link
Contributor Author

barapa commented Oct 15, 2025

Paraphrasing @Harshal6927:
With this change, errors are now raised on failed uploads. So this should be considered a breaking change.

@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2025

Codecov Report

❌ Patch coverage is 72.60274% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.19%. Comparing base (453f695) to head (fc94289).

Files with missing lines Patch % Lines
...anced_alchemy/types/file_object/session_tracker.py 72.05% 10 Missing and 9 partials ⚠️
advanced_alchemy/_listeners.py 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #580      +/-   ##
==========================================
+ Coverage   79.48%   80.19%   +0.71%     
==========================================
  Files          87       87              
  Lines        6346     6383      +37     
  Branches      807      821      +14     
==========================================
+ Hits         5044     5119      +75     
+ Misses       1051     1002      -49     
- Partials      251      262      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@barapa barapa changed the title fix: improve session tracker error logging and propagation fix: log and propagate errors when saving files Oct 16, 2025
@barapa barapa changed the title fix: log and propagate errors when saving files fix: Make FileObject session tracker surface storage errors Oct 16, 2025
@barapa barapa changed the title fix: Make FileObject session tracker surface storage errors fix: surface FileObject session errors; align commit/rollback semantics Oct 16, 2025
@cofin cofin force-pushed the feature/session-tracker-error-handling branch from 67b71fd to 65914c1 Compare October 18, 2025 03:20
@cofin cofin force-pushed the feature/session-tracker-error-handling branch from d08a51f to 8379100 Compare October 18, 2025 20:05
@github-actions
Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/advanced-alchemy-docs-preview/580

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants