fix: TimedOutboxArchiver falls back to sync when no async outbox (#3670)

[iancooper]

Summary

• TimedOutboxArchiver.Archive() now branches on outbox capability instead of unconditionally calling ArchiveAsync, which threw when only a sync outbox was registered
• Added HasAsyncOutbox() / HasOutbox() to OutboxArchiver, matching the existing OutboxProducerMediator convention
• Removed duplicate OutboxSweeperSleeping log call (was logged twice on the success path)

Closes #3670

Design

Three-way branch in TimedOutboxArchiver.Archive():

1. HasAsyncOutbox() → await ArchiveAsync(...) (existing behavior, preferred)
2. HasOutbox() → await Task.Run(() => Archive(...)) (sync fallback)
3. Neither → Log.NoOutboxConfigured(...) (guard, no throw)

See ADR 0056 for full rationale.

Test plan

• When_archiving_with_sync_only_outbox_should_call_sync_archive — core fix, sync-only outbox archives messages
• When_archiving_with_async_only_outbox_should_call_async_archive — async-only still works (FR1/AC2)
• When_archiving_with_both_sync_and_async_outbox_should_prefer_async — async preferred (AC3)
• When_archiving_with_no_outbox_configured_should_not_throw — guard path (FR3)
• All 18 archiving tests pass on net9.0 and net10.0
• No regressions in existing tests

🤖 Generated with Claude Code

[codescene-delta-analysis]

Gates Passed
4 Quality Gates Passed

See analysis details in CodeScene

Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

[claude]

Code Review — PR #4066: TimedOutboxArchiver sync fallback

Overview

This PR fixes a real bug (#3670): TimedOutboxArchiver unconditionally called ArchiveAsync, throwing ArgumentException when only a sync outbox was registered. The fix adds a three-way branch (async → sync fallback → no-outbox guard), surfaced via two new query methods on OutboxArchiver, following the established OutboxProducerMediator pattern exactly. The approach is minimal, well-documented, and includes an ADR.

---

Positives

• Minimal, focused fix — the core production change is ~10 lines; no unnecessary refactoring.
• Follows existing conventions — HasAsyncOutbox() / HasOutbox() match OutboxProducerMediator names exactly (the design review finding about HasSyncOutbox() was correctly resolved to HasOutbox()).
• Bonus cleanup — removes the pre-existing duplicate OutboxSweeperSleeping log call that fired twice on the success path.
• Good test doubles — SyncOnlyOutboxWrapper, AsyncOnlyOutboxWrapper, and NullOutbox are clearly scoped and their purpose is well-documented.
• Four test scenarios cover all branches: sync-only, async-only, both (prefers async), and neither. This is complete coverage for the three-way branch.
• ADR 0056 documents the decision clearly, including alternatives considered and consequences.

---

Issues

Medium — Test name/assertion mismatch in the "no outbox" test

File: tests/.../When_archiving_with_no_outbox_configured_should_log_warning.cs

The file is named should_log_warning, but the test method is named should_not_throw, and no logging assertion is present — only Assert.Empty(archiveProvider.ArchivedMessages). This means the Log.NoOutboxConfigured call added in TimedOutboxArchiver is untested. If someone accidentally removes the log call, or changes it to a throw, the test still passes.

Suggestion: Either rename the file to should_not_throw to match what is actually asserted, or inject a ILogger spy / use FakeLogger (from Microsoft.Extensions.Logging.Testing) to assert the warning was emitted.

Minor — Task.Delay-based test assertions are timing-dependent

All four new tests use this pattern:

await timedArchiver.StartAsync(cts.Token);
await Task.Delay(TimeSpan.FromSeconds(2));
await timedArchiver.StopAsync(cts.Token);

The TimerInterval is set to 5 seconds, but the tests wait only 2 seconds before asserting. This works in practice because TimedOutboxArchiver fires immediately on start, but it's not obvious from reading the test that this is intentional — the 2-second delay is there to let the async timer callback complete, not to wait for the interval. A comment explaining this, or a longer delay (e.g. 5s) to make the intent explicit, would help future readers.

More broadly, wall-clock Task.Delay in tests can be flaky under CI load. Given that InMemoryOutbox accepts a FakeTimeProvider, it might be worth exploring whether the timer in TimedOutboxArchiver itself can be driven by TimeProvider to allow deterministic firing in tests (out of scope for this PR, but worth noting).

Nit — specs/README.md checkboxes are all unchecked

specs/0026-timed-outbox-archiver-sync-fallback/README.md was generated from a template and all checkboxes remain unchecked, even though the spec is fully implemented. This is a cosmetic issue but could confuse someone scanning the spec status.

Nit — review-design.md is an internal iteration artifact

specs/0026-timed-outbox-archiver-sync-fallback/review-design.md documents an in-progress design review with a "NEEDS WORK" verdict. The findings were addressed in the final implementation, which is good, but including the intermediate review document in the merged PR creates confusion about the current state. Consider removing it or noting "resolved" on each finding.

---

Summary

Area	Assessment
Core fix correctness	✅ Correct three-way branch, no regressions
API design	✅ Follows existing OutboxProducerMediator convention
Test coverage (branches)	✅ All three paths covered
Logging assertion (no-outbox path)	⚠️ Missing — file name promises should_log_warning but doesn't assert it
Test reliability	⚠️ Timing-dependent delays; low risk but worth a comment
Documentation	✅ ADR is thorough; specs housekeeping is minor nit

The fix is sound and the approach is the right one. The logging assertion gap is the only thing worth addressing before merge — everything else is minor.