Skip to content

Log a capture attachment that fails to download - #834

Merged
JSdotNet merged 3 commits into
mainfrom
claude/log-failed-attachment-downloads-f4f736
Sep 29, 2026
Merged

JSdotNet merged 3 commits into
mainfrom
claude/log-failed-attachment-downloads-f4f736

Conversation

@JSdotNet

@JSdotNet JSdotNet commented Sep 29, 2026 •

Copy link
Copy Markdown
Owner

Summary

When a desktop Inbox attachment download fails, the reason shows on the item's row but nothing was logged, so the failure never reached the Aspire structured logs or Application Insights. This logs each failure once, at Warning, with the attachment id, the item id and the reason — never the file's name or bytes. Follow-up listed in #698.

Changes

  • InboxAttachmentDownloads.Fail — the one place every failure path ends — writes Could not download attachment {AttachmentId} of inbox item {ItemId}: {Reason} via [LoggerMessage], with the exception attached when the failure was a throw. The class stays static; DownloadAsync now takes the caller's ILogger.
  • ReceiveCaptureCommandHandler (intake) and RetryAttachmentCommandHandler (Retry) take an optional ILogger<T> (falling back to NullLogger), matching how their attachment ports are already optional — no host or test composition had to change.
  • Backlog.Modules.Inbox references Microsoft.Extensions.Logging.Abstractions, as Backlog.Modules.Sync already does.
  • InboxAttachmentSyncSource deliberately stays silent: its failure is the reason logged here, so logging there too would log each failure twice.
  • 5 tests in AttachmentIntakeTests (fields are exactly AttachmentId/ItemId/Reason, the exception is carried, a replayed page does not log again, a failed Retry logs, a success logs nothing), with a RecordingLogger<T> double.

Breaks on main. This branch first mended two (stale IRepositoryDirectory.Register doubles after #793; missing README project rows). main has since fixed both itself, and the merge takes main's side, so neither remains in the diff. The merge commit does fix a third: PaneInitialLoadTests.HeldTaskItems lacked the sourceInboxIds parameter the batch route added to ITaskItems.ImportPlanAsync (#799 and the batch route crossed), so Backlog.Desktop.UI.UnitTests did not compile on main.

Testing

  • dotnet build Backlog.sln: 0 errors. dotnet test Backlog.sln: 9912/9912; after merging main, 9950 passed and 1 skipped.
  • Targeted QA against this worktree's Aspire AppHost:
    • Checked-in E2E ConferenceDayTests: 1/1 pass; no download warnings on the happy path.
    • Phone uploads a picture, blob storage taken down, desktop pulls the item: the row shows the reason and exactly one Warning appears in desktop-web-harness structured logs from ReceiveCaptureCommandHandler, with AttachmentId, ItemId (matching the row) and Reason; the file name appears nowhere in the logs.
    • Retry with storage still down: exactly one more Warning, from RetryAttachmentCommandHandler.
    • Retry after storage returns: the file downloads and nothing is logged.
  • Found in passing, outside this change: with storage unreachable a download holds the desktop's pull ~30 s, and sync itself logs nothing when a blob read times out (traces only).

Related

Refs #698 (listed this as a follow-up). Backlog plan item e245049f-2270-4e9e-b788-f44f12ce5043 (+mobile-inbox-slice-follow-up). The Capture attachments requirement in .devbook/domain/inbox/requirements.md gets its logging clause through the devbook flow as a follow-up.

Checklist

  • Scope is focused and excludes unrelated changes (the two main fixes are isolated in their own commit)
  • Documentation or instruction files were updated when governance or behavior changed (README rows; the devbook requirement follows)
  • Running Aspire instances for this checkout were stopped before PR creation
  • Follow-up work is captured when the implementation is intentionally partial
  • Issues this PR closes are linked with a closing keyword, or there are none

🤖 Generated with Claude Code

JSdotNet and others added 2 commits September 30, 2026 00:29
…EADME rows

PR #793 changed IRepositoryDirectory.Register to return Result<TasksRepositoryRef>; two doubles in Backlog.Infrastructure.FileSystem.UnitTests still returned TasksRepositoryRef, so the project did not compile. ReadmeTests failed because the project table had no row for Backlog.Infrastructure.Sessions, its unit tests, or Backlog.Infrastructure.BlobStorage.UnitTests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A failed desktop Inbox download recorded its reason on the row but logged nothing, so it never reached the Aspire structured logs or Application Insights. InboxAttachmentDownloads now writes one Warning per failure with the attachment id, the item id and the reason (the exception attached when there was one, never the file's name or bytes), through the logger of the handler that asked: ReceiveCaptureCommandHandler for the intake, RetryAttachmentCommandHandler for Retry. Both take it as an optional parameter, like their attachment ports. InboxAttachmentSyncSource stays silent: its failure is the reason logged here.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Both conflicts were main fixing the same two breaks this branch had mended: took main's README Sessions row and its Result<TasksRepositoryRef> double. Also adds the sourceInboxIds parameter to PaneInitialLoadTests.HeldTaskItems.ImportPlanAsync: #799 added the double while the batch route added the parameter to ITaskItems, so main's Backlog.Desktop.UI.UnitTests does not compile.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@JSdotNet
JSdotNet merged commit 9c851aa into main Sep 29, 2026
7 checks passed
@JSdotNet
JSdotNet deleted the claude/log-failed-attachment-downloads-f4f736 branch September 29, 2026 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant