From f05137d8a1cb145cb7e85152641339e643d5c7cc Mon Sep 17 00:00:00 2001 From: Keith Voels Date: Fri, 14 Aug 2026 15:04:01 -0500 Subject: [PATCH 01/10] feat: add DispatchPhase to factory event handlers (PHASE-001) Handlers registered with [FactoryEventHandler] can now declare when they run relative to the factory operation that raised the event: - Immediate (default, unchanged): dispatched at Raise time, in the caller's transaction, observing staged state. - AfterFlush / AfterCommit: deferred into a per-scope queue and run when that phase drains, so read-only projections stop inheriting the in-transaction contract built for atomic write handlers. This plan lands the model, the registry phase, the queueing, and the drain primitive (IFactoryEventPhaseScheduler in the Internal namespace). The drain POINTS come next: entry-call tracking in PHASE-003, the consumer-facing coordinator in PHASE-004. Failure semantics key off the drain point rather than the phase: an in-transaction drain propagates handler exceptions so the caller can roll back; a post-completion drain logs (9003) and swallows, since a throw there can no longer roll anything back. A drain covers the requested phase and every earlier one, so nothing a handler enqueues mid-drain is silently dropped. FactoryEventHandlerAttribute moved to its own file: FactoryAttributes.cs is linked into the netstandard2.0 generator, and compiling DispatchPhase there would duplicate a public runtime type. The generator matches the attribute by metadata name, so it never needed the type. Backward compatible: no phase argument means Immediate, the generator's existing two-argument RegisterHandler call is untouched, and no existing test was modified. Co-Authored-By: Claude Fable 5 --- .../plans/001-phase-model-and-queueing.md | 74 +++- .../plans/003-aftercommit-entry-call-drain.md | 21 +- .../plans/004-afterflush-coordinator.md | 15 +- .../reviews/001-code-review.md | 66 +++ .../reviews/001-test-review.md | 73 ++++ .../todos/PHASE-phased-event-dispatch/todo.md | 22 +- src/Design/CLAUDE-DESIGN.md | 4 + src/RemoteFactory/AddRemoteFactoryServices.cs | 7 + src/RemoteFactory/DispatchPhase.cs | 65 +++ src/RemoteFactory/FactoryAttributes.cs | 37 -- .../FactoryEventHandlerAttribute.cs | 61 +++ .../FactoryEventHandlerRegistry.cs | 29 +- src/RemoteFactory/FactoryEventsDispatcher.cs | 36 +- src/RemoteFactory/IFactoryEvents.cs | 21 +- .../Internal/FactoryEventPhaseScheduler.cs | 156 +++++++ src/RemoteFactory/Internal/Log.cs | 39 ++ src/RemoteFactory/RaiseOptions.cs | 10 +- .../FactoryEventPhaseRegistrationTests.cs | 151 +++++++ .../FactoryEventPhaseSchedulerTests.cs | 409 ++++++++++++++++++ .../FactoryEventsDispatcherPhaseTests.cs | 252 +++++++++++ 20 files changed, 1485 insertions(+), 63 deletions(-) create mode 100644 docs/todos/PHASE-phased-event-dispatch/reviews/001-code-review.md create mode 100644 docs/todos/PHASE-phased-event-dispatch/reviews/001-test-review.md create mode 100644 src/RemoteFactory/DispatchPhase.cs create mode 100644 src/RemoteFactory/FactoryEventHandlerAttribute.cs create mode 100644 src/RemoteFactory/Internal/FactoryEventPhaseScheduler.cs create mode 100644 src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventPhaseRegistrationTests.cs create mode 100644 src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventPhaseSchedulerTests.cs create mode 100644 src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs diff --git a/docs/todos/PHASE-phased-event-dispatch/plans/001-phase-model-and-queueing.md b/docs/todos/PHASE-phased-event-dispatch/plans/001-phase-model-and-queueing.md index b63d0efa..e053c2bb 100644 --- a/docs/todos/PHASE-phased-event-dispatch/plans/001-phase-model-and-queueing.md +++ b/docs/todos/PHASE-phased-event-dispatch/plans/001-phase-model-and-queueing.md @@ -3,7 +3,7 @@ **Plan #:** 001 **Date:** 2026-08-14 **Related Todo:** [../todo.md](../todo.md) -**Status:** Draft +**Status:** Done **Last Updated:** 2026-08-14 **Plan-review opt-in:** Yes (public API: new enum, attribute constructor, registry signature — hard to change after release) **Code-review opt-in:** Yes (changes the core dispatch path every event flows through) @@ -128,6 +128,11 @@ the consumer-facing coordinator is PHASE-004), and does not change relay or seri - [ ] An event raised by a handler *during* a drain, whose handlers land in the draining or an already-passed phase, is processed in the same drain (drain-until-empty — nothing is silently dropped). `[unit]` +- [ ] A drain also sweeps up any earlier phase the consumer never drained, earliest phase + first, under the drain point's failure semantics — the fail-open behavior PHASE-004 + builds its warning on. Later phases than the one requested are left alone. `[unit]` +- [ ] A queued dispatch reaches its handler with the event instance, `RaiseOptions`, and + originating scope provider it was queued with. `[unit]` - [ ] Two scopes' queues are independent; a scope disposed without draining runs nothing (rollback-discard). *(Same interim annotation as bullet 2.)* `[unit]` - [ ] A phase-registered event raised without `ServerOnly` is still collected for the @@ -175,13 +180,76 @@ Walked 2026-08-14 against v1.7.0 (`main` @ 94a8a12): ## Test Evidence -*(Filled after implementation, before the Step 5 gate.)* +All cited tests live in `src/Tests/RemoteFactory.UnitTests/Internal/`; namespace prefix +`RemoteFactory.UnitTests.Internal` omitted below for width. + +| Acceptance bullet (short) | Tier declared | Test method | Tier confirmed | +|---|---|---|---| +| Phase-less handler keeps today's contract | `[explicit-skip]` | No existing test modified (`git status src/Tests` shows only new files). Suite: 653 unit × net9.0/net10.0 (0 failures) against a 614 baseline on `main` — +39 new cases, all added by this plan; integration 561 passed / 5 skipped / 566 total × 2 (`reviews/001-test.log`); Design 86 × 2, 0 failures (`reviews/001-test-design.log`) | ✓ | +| AfterCommit handler not invoked at raise time | `[unit]` | `FactoryEventsDispatcherPhaseTests.Raise_DeferredHandler_DoesNotDispatchAtRaiseTime`; primitive-level: `FactoryEventPhaseSchedulerTests.Enqueue_DoesNotInvokeHandler` | ✓ | +| Mixed phases: Immediate runs, other queues | `[unit]` | `FactoryEventsDispatcherPhaseTests.Raise_MixedPhases_ImmediateRunsAndDeferredWaits` (also pins cross-phase ordering) | ✓ | +| Post-completion drain: FIFO, logged+swallowed, rest still run, OCE propagates | `[unit]` | `FactoryEventPhaseSchedulerTests.DrainAsync_RunsDeferredDispatchesInFifoOrder`, `.DrainAsync_PostCompletion_SwallowsHandlerExceptionAndRunsTheRest`, `.DrainAsync_PostCompletion_StillPropagatesCancellation`, and for the log half `.DrainAsync_PostCompletionSwallow_LogsTheDedicatedEventIdWithTheException` (asserts event id 9003 + attached exception) | ✓ | +| Drain sweeps earlier phases the consumer never drained | `[unit]` | `FactoryEventPhaseSchedulerTests.DrainAsync_SweepsAnEarlierPhaseTheConsumerNeverDrained` (also asserts 9003 attributes the failure to the *queued* phase), `.DrainAsync_MidDrainEarlierPhaseWork_PreemptsRemainingLaterPhaseWork` (pins the ordering choice), `.DrainAsync_DoesNotRunLaterPhasesThanRequested` — **all three verified red** against the pre-fix drain (`reviews/001-redproof.log`) | ✓ | +| Queued dispatch carries its event, options, and provider intact | `[unit]` | `FactoryEventPhaseSchedulerTests.DrainAsync_HandlerReceivesTheEventAndOptionsItWasQueuedWith` (asserts `Assert.Same` on the originating scope provider, not merely non-null) | ✓ | +| In-transaction drain propagates; same handlers swallow at post-completion point | `[unit]` | `FactoryEventPhaseSchedulerTests.DrainAsync_InTransaction_PropagatesHandlerExceptionAndAbortsRemaining`, `.DrainAsync_SamePhaseHandlersDrainPoint_KeysSemanticsNotThePhase` | ✓ | +| Re-entrant enqueue during drain runs in same drain (same phase AND already-passed phase) | `[unit]` | `FactoryEventPhaseSchedulerTests.DrainAsync_ReentrantEnqueueDuringDrain_RunsInTheSameDrain` (same phase), `.DrainAsync_ReentrantEnqueueIntoAnAlreadyPassedPhase_StillRunsInThisDrain` (already-passed — **verified red against the pre-fix single-queue drain**), `.DrainAsync_DoesNotRunLaterPhasesThanRequested` (bounds it); real raise-path re-entrancy: `FactoryEventsDispatcherPhaseTests.DrainedHandlerRaisingAnEvent_GoesThroughTheRealRaisePath` | ✓ | +| Scope independence; never-drained runs nothing | `[unit]` | `FactoryEventPhaseRegistrationTests.PhaseDispatcher_IsScoped_NotSharedAcrossScopes`, `FactoryEventPhaseSchedulerTests.ScopeDisposedWithoutDraining_RunsNothing` (real scope, disposed) | ✓ | +| Phase-registered event still collected for relay | `[unit]` | `FactoryEventsDispatcherPhaseTests.Raise_DeferredHandler_StillCollectsForRelayAtRaiseTime` (Server-mode container, per review B-C3); `ServerOnly` counterpart: `.Raise_DeferredHandlerWithServerOnly_IsNotCollectedForRelay` | ✓ | +| Registry dedupe holds; first-registration-wins documented | `[unit]` | `FactoryEventPhaseRegistrationTests.RegisterHandler_RepeatedContainerBuilds_DedupesByEventAndHandlerClass`, `.RegisterHandler_SameHandlerClassTwoPhases_KeepsTheFirstRegistration` | ✓ | +| Build/test green | `[explicit-skip]` | `reviews/001-build.log` (0 errors, net9.0+net10.0), `reviews/001-test.log` (0 failures) | ✓ | + +Additional coverage not tied to a single bullet: `RegisterHandler_WithoutPhase_DefaultsToImmediate`, +`RegisterHandler_WithPhase_RoundTripsThePhase`, `PhaseDispatcher_RegisteredInModesThatDispatchHandlers` +(Theory: Server + Logical), `PhaseDispatcher_NotRegisteredInRemoteMode`, +`DrainAsync_OnlyDrainsTheRequestedPhase`, `DrainAsync_NothingDeferred_IsANoOp`, +`DrainAsync_DeferredDispatchesRunOnce_NotOnASecondDrain`, +`Attribute_NoArgument_DefaultsToImmediate`, `Attribute_ExplicitPhase_RoundTrips` (Theory: all +three phases), `RaiseUntyped_DeferredHandler_DefersJustLikeRaise`, +`Raise_PhasedHandlerWithNoQueueInScope_DispatchesImmediatelyRatherThanVanishing`, +`DrainAsync_HandlerReceivesTheDrainTimeCancellationToken` (pins the drain-time token choice +PHASE-003 will reason from), `Enqueue_NullEvent_Throws`. + +**Deliberately not covered here** (drain *points* are PHASE-003/004's deliverable, so no +integration-tier coverage of real factory calls belongs to this plan): entry-call tracking, +the consumer coordinator, and the same-response relay-batch guarantee. --- ## Plan Amendments -*(none yet)* +### 2026-08-14 — Handler attribute moved out of the generator-linked source file + +- **Section affected:** Step 1 +- **Original said:** add the optional phase argument to `[FactoryEventHandler]` in place. +- **What changed:** the attribute moved to its own `FactoryEventHandlerAttribute.cs`. +- **Why:** `FactoryAttributes.cs` is ``-linked into the netstandard2.0 + Generator project, so referencing `DispatchPhase` from it compiled the enum into + `Neatoo.Generator.dll`, duplicating a public runtime type (CS0436/CS0433 in every project + referencing both). The generator matches the attribute by metadata-name string and never + needed the type. +- **Discovery Log link:** 2026-08-14 — PHASE-001 (shared-source build constraint) + +### 2026-08-14 — Drain sweeps earlier phases, not just the requested one + +- **Section affected:** Step 5, Constraints (re-entrant enqueue) +- **Original said:** drain-until-empty within the phase being drained. +- **What changed:** `DrainAsync` drains the requested phase *and every earlier phase*, + earliest first, until none remain. +- **Why:** the test-review gate found the original shape silently dropped work enqueued + into an already-passed phase — and, separately, would have lost `AfterFlush` handlers + entirely for any consumer who never called the coordinator. That fail-open behavior is + PHASE-004's AC-5, now structurally satisfied here. +- **Discovery Log link:** 2026-08-14 — PHASE-001 (gate found a real defect) + +### 2026-08-14 — Scheduler naming + +- **Section affected:** Step 5 +- **Original said:** the drain primitive as `IFactoryEventPhaseDispatcher`. +- **What changed:** renamed to `IFactoryEventPhaseScheduler` / `FactoryEventPhaseScheduler`. +- **Why:** code review C1 — two "…Dispatcher" types in one assembly doing different jobs, + settled before PHASE-003 emits generated call sites against the name. `…Queue` was + unavailable: CA1711 forbids the suffix. +- **Discovery Log link:** see [reviews/001-code-review.md](../reviews/001-code-review.md) --- diff --git a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md index ca6277ef..97cbee06 100644 --- a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md +++ b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md @@ -26,4 +26,23 @@ directly, bypassing public wrappers. This plan does NOT own the consumer-facing --- -*(Stub — Intent, Alignment, Constraints, Steps, Acceptance filled at Step 2.)* +## Constraints inherited from PHASE-001 (recorded at its Step 5 gate) + +- **The drain call sits on the success path only** — never in a `finally`, never in a + scope-disposal hook or middleware that runs on failure. Rollback-discard is emergent + ("a scope that fails simply never drains"), so a drain on the failure path breaks the + todo's AC-2 silently and no primitive-level test can catch it (code review C4). +- The scheduler API to call is `IFactoryEventPhaseScheduler.DrainAsync(phase, + inTransaction, ct)` in `Neatoo.RemoteFactory.Internal` — public so generated code can + reach it. Pass `inTransaction: false` at the entry-call drain point. +- The cancellation-token *policy* question is open here: queued dispatches currently + receive the drain-time token (pinned by + `FactoryEventPhaseSchedulerTests.DrainAsync_HandlerReceivesTheDrainTimeCancellationToken`). + Decide whether a post-completion drain should pass the request token at all — an + `OperationCanceledException` from it fails a call that already succeeded (plan review + B-C5). +- `IFactoryEvents.RaiseUntyped` has no general test coverage repo-wide; it is the + server-side landing point for client-raised events, so this plan's remote-entry work is + the natural place to add it (tech debt raised at PHASE-001's gate). + +*(Stub — Intent, Alignment, remaining Constraints, Steps, Acceptance filled at Step 2.)* diff --git a/docs/todos/PHASE-phased-event-dispatch/plans/004-afterflush-coordinator.md b/docs/todos/PHASE-phased-event-dispatch/plans/004-afterflush-coordinator.md index 032dffb7..1b1e5f3c 100644 --- a/docs/todos/PHASE-phased-event-dispatch/plans/004-afterflush-coordinator.md +++ b/docs/todos/PHASE-phased-event-dispatch/plans/004-afterflush-coordinator.md @@ -22,4 +22,17 @@ coordinator is a drain trigger, nothing more. --- -*(Stub — Intent, Alignment, Constraints, Steps, Acceptance filled at Step 2.)* +## Inherited from PHASE-001 (recorded at its Step 5 gate) + +- **The fail-open sweep is already implemented and test-pinned.** `DrainAsync(phase, …)` + drains the requested phase *and every earlier one*, earliest first, so `AfterFlush` + handlers a consumer never drained are swept up at the `AfterCommit` point under that + drain point's swallow semantics (`FactoryEventPhaseSchedulerTests.DrainAsync_SweepsAnEarlierPhaseTheConsumerNeverDrained`). + What is missing is only the **logged warning** the todo's AC-5 requires — and the + discriminator is already plumbed: `TryDequeueThrough` returns the phase each dispatch was + queued at (code review C3). Wire the warning; do not re-plumb the sweep. +- `IFactoryEventPhaseCoordinator.DrainAsync(AfterFlush)` should call through to the + scheduler with `inTransaction: true` so handler exceptions propagate and the consumer's + transaction can still roll back. + +*(Stub — Intent, Alignment, remaining Constraints, Steps, Acceptance filled at Step 2.)* diff --git a/docs/todos/PHASE-phased-event-dispatch/reviews/001-code-review.md b/docs/todos/PHASE-phased-event-dispatch/reviews/001-code-review.md new file mode 100644 index 00000000..9e528b36 --- /dev/null +++ b/docs/todos/PHASE-phased-event-dispatch/reviews/001-code-review.md @@ -0,0 +1,66 @@ +# PHASE-001 Code Review — 2026-08-14 + +**Reviewer:** code-reviewer agent (findings-only, no grade) +**Disposition:** all four veto-tier findings fixed; callouts fixed, traced forward, or +accepted with reason below. + +## Verified clean by the reviewer + +- **Trimming invariant holds.** `[DynamicallyAccessedMembers(All)]` is present on `TEvent` + for *both* `RegisterHandler` overloads, and the 2-arg forwards into the annotated 3-arg + parameter so the annotation flows rather than being dropped at the hop. +- **No reflection introduced.** The only `GetType()` calls are `.Name` for log messages. +- **Backward compatibility is structural:** zero modified test files; the generator's + emitted 2-arg `RegisterHandler` call and the checked-in generated output still compile. +- **The `TryDequeueThrough` LINQ-over-dictionary pattern is safe:** `OrderBy` buffers its + source on first `MoveNext`, and the loop returns synchronously before any `await`, so the + only mutation that can add a key (`Enqueue`, reachable only from inside a handler) happens + after enumeration has finished. +- **Scoping split is right:** process-static registry keeps its locking; the per-scope + scheduler is unsynchronized, matching the established `FactoryEventCollector` stance. +- **The build-config move is correct:** the generator matches the handler attribute by + metadata-name string in both places it looks, so it never needed the type. + +## Veto-tier findings — all fixed + +- **V1 — `CLAUDE-DESIGN.md` log-id table not updated.** Added rows for 9001/9002/9003 (and + 9004, added in this same pass) with propagation semantics reflecting the drain-point + keying. +- **V2 — no test-review record.** The gate had in fact run; the record is now written to + [`001-test-review.md`](./001-test-review.md), including a second round. +- **V3 — Test Evidence cited numbers the log contradicted.** Corrected to the actual + figures: 653 unit × 2 TFMs against a 614 baseline on `main`, integration 561 passed / + 5 skipped / 566 total, Design 86 × 2 with its own retained log. +- **V4 — `DispatchPhase` XML doc asserted an ordering guarantee the drain-until-empty + stance breaks.** Added the carve-out sentence naming the re-entrant case, and rewrote the + `DrainAsync` doc (which the test reviewer independently flagged as stale). + +## Callout dispositions + +- **C1 (naming collision)** — fixed by renaming to `IFactoryEventPhaseScheduler` / + `FactoryEventPhaseScheduler`, settled *before* PHASE-003 emits generated call sites. + (`…Queue` was not available: CA1711 forbids the suffix, which is what produced the + original `Dispatcher` name.) +- **C2 (silent phase→Immediate fallback)** — fixed: new debug event id 9004 fires when a + phased handler is raised in a scope with no scheduler, matching the todo's house rule of + "dispatch immediately with a debug log, no silent drop." +- **C3 (fail-open discriminator already plumbed)** — recorded as a constraint on the + PHASE-004 draft; the sweep is now also test-pinned, so 004 wires a warning rather than + re-plumbing. +- **C4 (no discard affordance)** — recorded as a hard constraint on the PHASE-003 draft: + the drain call must sit on the success path only, never in a `finally`. +- **C5 (near-tautological provider assertion)** — fixed: `Assert.Same` against the + originating scope provider. +- **C6 (evidence claims without artifacts)** — fixed: `001-redproof.log` and + `001-test-design.log` retained in `reviews/`. +- **C9 (handler not null-guarded)** — fixed. +- **C11 (LoggerMessage parameter order)** — fixed for 9002/9003 to match the repo's + template-order convention. +- **C7 (per-item LINQ re-derivation)** — accepted. ≤3 phases makes the cost nil, and the + safety argument is documented in the method's comment. Pre-seeding the dictionary is a + reasonable future hardening but changes `HasPending`'s shape. +- **C8 (single-logical-flow assumption undocumented)** — accepted with a note: the + assumption matches the pre-existing `FactoryEventCollector` stance. Worth a sentence on + the interface if PHASE-003 finds a concurrent-raise scenario. +- **C10 (`NeatooLoggerCategories.Server` in Logical mode)** — accepted; the existing + category set has no better fit and inventing one is out of this plan's scope. diff --git a/docs/todos/PHASE-phased-event-dispatch/reviews/001-test-review.md b/docs/todos/PHASE-phased-event-dispatch/reviews/001-test-review.md new file mode 100644 index 00000000..db0560c3 --- /dev/null +++ b/docs/todos/PHASE-phased-event-dispatch/reviews/001-test-review.md @@ -0,0 +1,73 @@ +# PHASE-001 Test Review — 2026-08-14 + +**Reviewer:** test-reviewer agent (two rounds) +**Closing tier:** must-cover closed; all round-1 should-cover closed; round-2 should-cover +closed. Remaining nice-to-have accepted or queued as tech debt. + +## Round 1 — findings and dispositions + +**must-cover (all closed):** + +1. **Re-entrant enqueue into an already-passed phase was untested — and was a real + production defect.** `DrainAsync` resolved exactly one queue, so a handler enqueueing + into a phase whose drain point had passed was silently dropped. Fixed by + `TryDequeueThrough`, which takes the next dispatch from the earliest non-empty phase at + or before the requested one, looping until all are empty. Pinned by + `DrainAsync_ReentrantEnqueueIntoAnAlreadyPassedPhase_StillRunsInThisDrain`. +2. **The 9xxx logging path never executed in any test** (the test helper left the optional + `ILoggerFactory` null). `NewDispatcher` now wires a capturing provider; + `DrainAsync_PostCompletionSwallow_LogsTheDedicatedEventIdWithTheException` asserts id + 9003, `LogLevel.Error`, and the exact exception instance. +3. **Queued-dispatch payload fidelity unasserted.** + `DrainAsync_HandlerReceivesTheEventAndOptionsItWasQueuedWith` asserts the exact + (event value, `RaiseOptions`) pairs and — after code-review C5 — `Assert.Same` on the + originating scope provider. + +**should-cover (all closed):** real scope-disposal rollback-discard test replacing the +vacuous `NeverDrained_RunsNothing`; `RaiseUntyped` parity; drain-time cancellation-token +plumbing; attribute default/explicit phase round-trip; the no-queue fallback tested rather +than deleted; re-entrancy exercised through the real `handler → IFactoryEvents.Raise → +registry → defer` path. + +## Round 2 — findings and dispositions + +**must-cover (closed):** the multi-phase drain fix introduced a *new* untested behavior — +a later-phase drain now also sweeps an earlier phase the consumer never drained (PHASE-004's +fail-open path, implemented here as a side effect). Closed by +`DrainAsync_SweepsAnEarlierPhaseTheConsumerNeverDrained`, which also asserts 9003 +attributes the failure to the phase the dispatch was *queued* at rather than the phase +requested. + +**should-cover (closed):** mid-drain earlier-phase work preempts remaining later-phase work +— pinned deliberately by `DrainAsync_MidDrainEarlierPhaseWork_PreemptsRemainingLaterPhaseWork` +so the ordering is a decision, not an accident; and the stale `DrainAsync` XML doc, rewritten +to state that the drain covers the requested phase and every earlier one. + +**nice-to-have (accepted, not actioned):** `Raise_DeferredHandler_StillCollectsForRelayAtRaiseTime` +does not also assert `HasPending` (reviewer confirmed not must-fix — the premise is +independently pinned); 9002's count spans phases while its `{Phase}` names only the +requested one; `DrainAsync_OnlyDrainsTheRequestedPhase` is now a mild misnomer; +`Enqueue(DispatchPhase.Immediate, ...)` remains unspecified on the interface. + +## Red-verification + +Per the project memory *"a check that could never go red is not evidence"*, the three +multi-phase drain tests were run against the pre-fix implementation (`p <= through` +reverted to `p == through`). All three failed; log retained at +[`001-redproof.log`](./001-redproof.log). The fix was then restored and the full suite +re-run green. + +## Tech debt queued (not absorbed into this plan) + +- `IFactoryEvents.RaiseUntyped` has no general coverage anywhere in the repo — pairs + naturally with PHASE-003's remote-entry work. +- `FactoryEventHandlerRegistry` is process-global mutable static with no test-isolation + hook (`Clear()` is internal and uncalled), forcing every test to invent unique event + types. + +Both are recorded as Plan Index entries in the parent todo. + +## Final state + +Build 0 errors; unit 653 × net9.0/net10.0, integration 561 passed / 5 skipped × 2, Design +86 × 2 — all 0 failures. No existing test modified. diff --git a/docs/todos/PHASE-phased-event-dispatch/todo.md b/docs/todos/PHASE-phased-event-dispatch/todo.md index 0a9248db..1e5467de 100644 --- a/docs/todos/PHASE-phased-event-dispatch/todo.md +++ b/docs/todos/PHASE-phased-event-dispatch/todo.md @@ -70,17 +70,37 @@ exposes drain points. | # | File | Title | Status | |---|------|-------|--------| -| 001 | [001-phase-model-and-queueing](./plans/001-phase-model-and-queueing.md) | DispatchPhase enum, registry phase, dispatcher queueing | Draft | +| 001 | [001-phase-model-and-queueing](./plans/001-phase-model-and-queueing.md) | DispatchPhase enum, registry phase, dispatcher queueing | Done | | 002 | [002-generator-phase-passthrough](./plans/002-generator-phase-passthrough.md) | Generator reads phase from attribute, threads to registration | Draft | | 003 | [003-aftercommit-entry-call-drain](./plans/003-aftercommit-entry-call-drain.md) | Entry-call tracking in generated factories; AfterCommit drain | Draft | | 004 | [004-afterflush-coordinator](./plans/004-afterflush-coordinator.md) | IFactoryEventPhaseCoordinator public API + fallback drain | Draft | | 005 | [005-design-docs-skill](./plans/005-design-docs-skill.md) | Design projects, published docs, skill reference | Draft | | 006 | [006-coalescing](./plans/006-coalescing.md) | Opt-in same-event coalescing (v2, queued per user) | Draft | +| 007 | *(not yet drafted)* | Tech debt: registry test-isolation hook (`Clear()` is internal and uncalled; every test invents unique event types) | Draft | --- ## Discovery Log +### 2026-08-14 — PHASE-001 (gate found a real defect) +- **Finding:** The test-review gate caught that the drain resolved only the requested + phase's queue, so work a handler enqueued into an already-passed phase was silently + dropped — the exact silent-loss class this todo exists to remove. +- **Decision:** Amend — replaced with a drain that sweeps the requested phase and every + earlier one, earliest first; three tests verified red against the pre-fix code. +- **Follow-up:** PHASE-004 inherits the sweep (it implements that plan's fail-open path); + constraint recorded in its draft. See [reviews/001-test-review.md](./reviews/001-test-review.md). + +### 2026-08-14 — PHASE-001 (shared-source build constraint) +- **Finding:** `FactoryAttributes.cs` is linked into the netstandard2.0 Generator project, + so putting `DispatchPhase` on the handler attribute compiled the enum into + `Neatoo.Generator.dll` too — duplicating a public runtime type and breaking every + project referencing both (CS0436 in RemoteFactory, CS0433 in UnitTests). +- **Decision:** Amend — moved `FactoryEventHandlerAttribute` to its own unlinked file; + the generator matches it by metadata-name string and never needed the type. +- **Follow-up:** n/a (PHASE-002 must not re-link `DispatchPhase` into the generator; the + new file's XML doc carries the warning). + ### 2026-08-14 — PHASE-001 (plan review) - **Finding:** Plan review returned CONCERNS — 4 veto findings, the sharpest being that failure semantics belong to the drain point rather than the phase, and that three diff --git a/src/Design/CLAUDE-DESIGN.md b/src/Design/CLAUDE-DESIGN.md index 353ca451..3cbafb41 100644 --- a/src/Design/CLAUDE-DESIGN.md +++ b/src/Design/CLAUDE-DESIGN.md @@ -1014,6 +1014,10 @@ These are known limitations or open questions. They are documented here to preve | 3009 | `FactoryEventDeserializationFailed` | Error | Wire-format event deserialization fails (e.g. `UnknownFactoryEventTypeException`) | Swallowed; `Relay` is NOT invoked for that call (the one legitimate case of zero `Relay` invocations for a [Remote] call) | | 3011 | `NoOpFactoryEventRelayFirstEvent` | Warning | `NoOpFactoryEventRelay` receives its first non-empty batch (consumer forgot to register a relay) | Informational; fires once per process | | 3012 | `FactoryEventTypeRegistryCollision` | Warning | `FactoryEventTypeRegistry` assembly scan finds two distinct `Type`s sharing the same `FullName` | Documents kept/dropped assembly; wire messages resolve to the kept type | +| 9001 | `FactoryEventPhaseQueued` | Debug | A handler registered at a non-`Immediate` `DispatchPhase` is deferred instead of dispatched at `Raise` time | Informational | +| 9002 | `FactoryEventPhaseDrained` | Debug | A phase drain completes, reporting how many dispatches ran through the requested phase (earlier phases included) | Informational | +| 9003 | `FactoryEventPhaseHandlerFailed` | Error | A deferred handler throws during a **post-completion** drain (no ambient transaction) | Swallowed — the exception can no longer roll anything back; remaining queued handlers still run. `OperationCanceledException` still propagates. In-transaction drains propagate instead, so this never fires for them. | +| 9004 | `FactoryEventPhaseNoQueueInScope` | Debug | An event with a phased handler is raised in a scope with no `IFactoryEventPhaseScheduler` registered | Dispatched immediately rather than dropped | ### Public Exception diff --git a/src/RemoteFactory/AddRemoteFactoryServices.cs b/src/RemoteFactory/AddRemoteFactoryServices.cs index a7d9843c..eadb7416 100644 --- a/src/RemoteFactory/AddRemoteFactoryServices.cs +++ b/src/RemoteFactory/AddRemoteFactoryServices.cs @@ -77,6 +77,13 @@ public static IServiceCollection AddNeatooRemoteFactory(this IServiceCollection { services.TryAddScoped(); + // Per-scope queue for handlers registered at a non-Immediate DispatchPhase. + // Registered for Server AND Logical — unlike IFactoryEventCollector, which is + // about relaying to a client and is therefore Server-only. Handler dispatch + // happens in both modes, so both need somewhere to queue. + services.TryAddScoped(sp => + new FactoryEventPhaseScheduler(sp, sp.GetService())); + // Register the delegate handler for remote IFactoryEvents.Raise requests. // When a Remote client sends a RaiseFactoryEventRemote request, the server // resolves this delegate, dispatches to local handlers in the request scope, diff --git a/src/RemoteFactory/DispatchPhase.cs b/src/RemoteFactory/DispatchPhase.cs new file mode 100644 index 00000000..a01f17b4 --- /dev/null +++ b/src/RemoteFactory/DispatchPhase.cs @@ -0,0 +1,65 @@ +namespace Neatoo.RemoteFactory; + +/// +/// Declares when a [FactoryEventHandler<T>] handler runs relative to the +/// factory operation that raised the event. +/// +/// +/// +/// handlers dispatch at time, +/// inside whatever transaction the caller has open, observing the caller's staged +/// (unflushed) state. and handlers are +/// queued per DI scope at raise time and drained later; if the factory operation fails, +/// the queues are discarded and the handlers never run. +/// +/// +/// Cross-phase ordering is guaranteed: for one factory operation, all +/// handlers complete before any handler +/// runs, and all handlers complete before any +/// handler runs. Order within a phase is unspecified. +/// +/// +/// One carve-out: a handler running in a drain can itself raise an event whose handlers +/// belong to a phase whose drain point has already passed. That work runs in the current +/// drain — before any dispatch still queued for later phases — rather than being dropped. +/// So a handler for an earlier phase can observe a later phase's handler having already +/// run, when the earlier-phase work was created by that later-phase handler. +/// +/// +/// RemoteFactory owns no persistence concepts — it never flushes or commits. The phase +/// names describe consumer intent at the drain points the framework exposes. +/// +/// +public enum DispatchPhase +{ + /// + /// Today's contract and the default: dispatched synchronously at + /// time in the caller's DI scope, sequential and + /// awaited, mid-transaction with the caller's writes still staged. A handler exception + /// aborts the remaining handlers and propagates so the caller's transaction can roll + /// back. The right phase for handlers that must be atomic with the save. + /// + Immediate = 0, + + /// + /// Queued at raise time; drained when the consumer signals — typically between the + /// outermost flush and the commit, via + /// IFactoryEventPhaseCoordinator.DrainAsync. Handlers run in-transaction but + /// see flushed state; an exception propagates to the drain caller so the transaction + /// can still roll back. Handlers the consumer never drains run at the + /// point instead, with a logged warning (fail-open). + /// + AfterFlush = 1, + + /// + /// Queued at raise time; drained by the framework when the entry factory call + /// completes successfully. Strictly, this means "after the entry factory method body + /// returns" — it is "after commit" because consumers commit inside their factory + /// bodies, which is the universal RemoteFactory pattern. Handlers run with no ambient + /// transaction; a handler exception cannot roll anything back, so the framework logs + /// it and continues ( still propagates). + /// Events raised by these handlers still join the same response's relay batch. + /// The right phase for read-only projections. + /// + AfterCommit = 2, +} diff --git a/src/RemoteFactory/FactoryAttributes.cs b/src/RemoteFactory/FactoryAttributes.cs index a07ef5bb..cc5b3ca3 100644 --- a/src/RemoteFactory/FactoryAttributes.cs +++ b/src/RemoteFactory/FactoryAttributes.cs @@ -113,43 +113,6 @@ public sealed class ExecuteAttribute : FactoryOperationAttribute public ExecuteAttribute() : base(FactoryOperation.Execute) { } } -/// -/// Marks a class as a handler for factory events of type . -/// The handler method must be static, return , and have a first -/// non-[Service]/non-CancellationToken parameter of type . -/// -/// Handlers are registered in and dispatched -/// via on the server, in the caller's DI scope, -/// sequentially, awaited. -/// -/// -/// Instance-method handlers are silently ignored by the generator — they were the -/// former client-side relay pattern, now replaced by . -/// Client-side event consumers implement to bridge -/// relayed events to their own event aggregator. -/// -/// -/// Trimming: every generated handler registration is wrapped in -/// NeatooRuntime.IsServerRuntime, so on a client published with the feature switch -/// set to false the handler bodies and their [Service] dependencies are -/// removed from the output. This works because the generator points the assembly's -/// at a generated forwarding holder rather -/// than at the handler class itself — the attribute preserves every method on whatever it -/// names, bodies included, so naming the handler class would ship the handler bodies to the -/// browser. Fixed in v1.7.0; before that, it did. -/// -/// -/// One consequence for consumers: because the registrations are server-guarded, there is -/// nothing on a trimmed client to resolve, and handler registration cannot be verified from -/// a client-side test. Coverage for it lives in server-side/untrimmed tests. -/// -/// -/// The event type (must inherit from ). -[System.AttributeUsage(AttributeTargets.Class, Inherited = false, AllowMultiple = true)] -#pragma warning disable CA1813 // Sealed generic attribute must be unsealed for generator discovery -public sealed class FactoryEventHandlerAttribute : Attribute { } -#pragma warning restore CA1813 - [System.AttributeUsage(AttributeTargets.Class | AttributeTargets.Interface, Inherited = false, AllowMultiple = false)] public sealed class AuthorizeFactoryAttribute : Attribute { diff --git a/src/RemoteFactory/FactoryEventHandlerAttribute.cs b/src/RemoteFactory/FactoryEventHandlerAttribute.cs new file mode 100644 index 00000000..742d1fcb --- /dev/null +++ b/src/RemoteFactory/FactoryEventHandlerAttribute.cs @@ -0,0 +1,61 @@ +namespace Neatoo.RemoteFactory; + +/// +/// Marks a class as a handler for factory events of type . +/// The handler method must be static, return , and have a first +/// non-[Service]/non-CancellationToken parameter of type . +/// +/// Handlers are registered in and dispatched +/// on the server in the caller's DI scope, sequentially, awaited. +/// controls when: the default dispatches at +/// time, mid-transaction, observing the caller's +/// staged (unflushed) state; and +/// queue the dispatch and drain it later. +/// +/// +/// Instance-method handlers are silently ignored by the generator — they were the +/// former client-side relay pattern, now replaced by . +/// Client-side event consumers implement to bridge +/// relayed events to their own event aggregator. +/// +/// +/// Trimming: every generated handler registration is wrapped in +/// NeatooRuntime.IsServerRuntime, so on a client published with the feature switch +/// set to false the handler bodies and their [Service] dependencies are +/// removed from the output. This works because the generator points the assembly's +/// at a generated forwarding holder rather +/// than at the handler class itself — the attribute preserves every method on whatever it +/// names, bodies included, so naming the handler class would ship the handler bodies to the +/// browser. Fixed in v1.7.0; before that, it did. +/// +/// +/// One consequence for consumers: because the registrations are server-guarded, there is +/// nothing on a trimmed client to resolve, and handler registration cannot be verified from +/// a client-side test. Coverage for it lives in server-side/untrimmed tests. +/// +/// +/// This attribute lives outside FactoryAttributes.cs deliberately: that file is +/// linked into the netstandard2.0 Generator project, and this type references +/// . Compiling the enum into the generator assembly would make +/// it a duplicate of the runtime's own copy in every project referencing both. The +/// generator matches this attribute by metadata-name string, so it never needs the type. +/// +/// +/// The event type (must inherit from ). +[AttributeUsage(AttributeTargets.Class, Inherited = false, AllowMultiple = true)] +#pragma warning disable CA1813 // Sealed generic attribute must be unsealed for generator discovery +public sealed class FactoryEventHandlerAttribute : Attribute +{ + /// Registers the handler at . + public FactoryEventHandlerAttribute() : this(DispatchPhase.Immediate) { } + + /// Registers the handler at . + public FactoryEventHandlerAttribute(DispatchPhase phase) + { + this.Phase = phase; + } + + /// When the handler runs relative to the factory operation that raised the event. + public DispatchPhase Phase { get; } +} +#pragma warning restore CA1813 diff --git a/src/RemoteFactory/FactoryEventHandlerRegistry.cs b/src/RemoteFactory/FactoryEventHandlerRegistry.cs index de1c8867..5b26f5af 100644 --- a/src/RemoteFactory/FactoryEventHandlerRegistry.cs +++ b/src/RemoteFactory/FactoryEventHandlerRegistry.cs @@ -14,18 +14,29 @@ public static class FactoryEventHandlerRegistry private readonly struct HandlerEntry { - public HandlerEntry(Type handlerClassType, Func invoke) + public HandlerEntry(Type handlerClassType, DispatchPhase phase, Func invoke) { HandlerClassType = handlerClassType; + Phase = phase; Invoke = invoke; } public Type HandlerClassType { get; } + public DispatchPhase Phase { get; } public Func Invoke { get; } } /// - /// Registers a handler factory for the given event type. + /// Registers a handler factory for the given event type at . + /// + public static void RegisterHandler<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] TEvent>( + Type handlerClassType, + Func handlerFactory) + where TEvent : FactoryEventBase + => RegisterHandler(handlerClassType, DispatchPhase.Immediate, handlerFactory); + + /// + /// Registers a handler factory for the given event type at . /// Called by generated FactoryServiceRegistrar methods during DI setup. /// /// @@ -37,11 +48,14 @@ public HandlerEntry(Type handlerClassType, Func /// /// Registrations are deduplicated by the (event type, handler class type) pair - /// so multiple DI container builds in a test run do not multiply registrations. + /// so multiple DI container builds in a test run do not multiply registrations. One + /// consequence: a handler class declaring the same event type twice at different + /// phases keeps the phase registered first, for the life of the process. /// /// public static void RegisterHandler<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] TEvent>( Type handlerClassType, + DispatchPhase phase, Func handlerFactory) where TEvent : FactoryEventBase { @@ -51,21 +65,22 @@ public HandlerEntry(Type handlerClassType, Func e.HandlerClassType == handlerClassType)) { - list.Add(new HandlerEntry(handlerClassType, handlerFactory)); + list.Add(new HandlerEntry(handlerClassType, phase, handlerFactory)); } } } /// - /// Gets all registered handler factories for the given event type. + /// Gets all registered handler factories for the given event type, paired with the + /// phase each was registered at. /// - internal static IReadOnlyList>? GetHandlers(Type eventType) + internal static IReadOnlyList<(DispatchPhase Phase, Func Invoke)>? GetHandlers(Type eventType) { if (!_handlers.TryGetValue(eventType, out var handlers)) return null; lock (handlers) { - return handlers.Select(h => h.Invoke).ToArray(); + return handlers.Select(h => (h.Phase, h.Invoke)).ToArray(); } } diff --git a/src/RemoteFactory/FactoryEventsDispatcher.cs b/src/RemoteFactory/FactoryEventsDispatcher.cs index 7f9a445e..2532fcb2 100644 --- a/src/RemoteFactory/FactoryEventsDispatcher.cs +++ b/src/RemoteFactory/FactoryEventsDispatcher.cs @@ -1,5 +1,6 @@ using System.Diagnostics.CodeAnalysis; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using Neatoo.RemoteFactory.Internal; namespace Neatoo.RemoteFactory; @@ -10,20 +11,29 @@ namespace Neatoo.RemoteFactory; /// Each handler was registered by a generated FactoryServiceRegistrar during DI setup. /// /// -/// Handlers run sequentially in the caller's so that -/// a DbContext (or any other scoped service) is shared between the factory method -/// and its handlers. A handler exception aborts the remaining handlers and propagates to -/// the caller, so the caller can let the transaction roll back. +/// +/// handlers — the default — run sequentially +/// in the caller's so that a DbContext (or any other +/// scoped service) is shared between the factory method and its handlers. A handler +/// exception aborts the remaining handlers and propagates to the caller, so the caller can +/// let the transaction roll back. +/// +/// +/// Handlers registered at another phase are queued in the scope's +/// instead, and run when that phase drains. +/// /// internal sealed class FactoryEventsDispatcher : IFactoryEvents { private readonly IServiceProvider _sp; private readonly IFactoryEventCollector? _collector; + private readonly IFactoryEventPhaseScheduler? _phaseQueue; public FactoryEventsDispatcher(IServiceProvider sp) { _sp = sp; _collector = sp.GetService(); + _phaseQueue = sp.GetService(); } public Task Raise<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>(T factoryEvent, RaiseOptions options = RaiseOptions.None, CancellationToken cancellationToken = default) where T : FactoryEventBase @@ -51,8 +61,24 @@ private async Task DispatchToHandlers(Type eventType, object factoryEvent, Raise // Sequential dispatch in the caller's scope. Handler order is unspecified — // callers must not depend on it. Exceptions propagate immediately and abort // the remaining handlers so the caller's transaction can roll back. - foreach (var handler in handlers) + // + // Phased handlers are queued instead, and run when their phase drains. Without a + // queue in the scope they fall back to immediate dispatch rather than vanishing. + foreach (var (phase, handler) in handlers) { + if (phase != DispatchPhase.Immediate) + { + if (_phaseQueue != null) + { + _phaseQueue.Enqueue(phase, (FactoryEventBase)factoryEvent, options, handler); + continue; + } + + _sp.GetService()? + .CreateLogger(NeatooLoggerCategories.Server) + .FactoryEventPhaseNoQueueInScope(eventType.Name, phase); + } + await handler(_sp, factoryEvent, options, cancellationToken).ConfigureAwait(false); } } diff --git a/src/RemoteFactory/IFactoryEvents.cs b/src/RemoteFactory/IFactoryEvents.cs index 81c1b089..a735ef45 100644 --- a/src/RemoteFactory/IFactoryEvents.cs +++ b/src/RemoteFactory/IFactoryEvents.cs @@ -8,17 +8,26 @@ namespace Neatoo.RemoteFactory; /// /// /// -/// Execution model. Raise is always shared-scope, sequential, and awaited: -/// handlers share the caller's DI scope (so a DbContext injected into the factory -/// method and a DbContext injected into a handler are the same instance), run one -/// after another in unspecified order, and any handler exception aborts the remaining -/// handlers and propagates to the caller. Across the client/server boundary the HTTP call -/// stays open until every server-side handler has completed. +/// Execution model. Raise is shared-scope, sequential, and awaited: handlers +/// share the caller's DI scope (so a DbContext injected into the factory method and +/// a DbContext injected into a handler are the same instance), run one after another +/// in unspecified order, and any handler exception aborts the remaining handlers and +/// propagates to the caller. Across the client/server boundary the HTTP call stays open +/// until every server-side handler has completed. /// /// /// This makes FactoryEvent the right tool for domain events that must participate /// in the caller's transaction. /// +/// +/// Phases. The above is the contract — the +/// default, and the only behavior before phases existed. A handler registered at another +/// is queued when the event is raised and dispatched when that +/// phase drains, so Raise returning no longer means every handler has run. Handlers +/// that must be atomic with the caller's transaction stay ; +/// read-only projections that need to see the completed operation use +/// . +/// /// public interface IFactoryEvents { diff --git a/src/RemoteFactory/Internal/FactoryEventPhaseScheduler.cs b/src/RemoteFactory/Internal/FactoryEventPhaseScheduler.cs new file mode 100644 index 00000000..a09b7527 --- /dev/null +++ b/src/RemoteFactory/Internal/FactoryEventPhaseScheduler.cs @@ -0,0 +1,156 @@ +using Microsoft.Extensions.Logging; + +namespace Neatoo.RemoteFactory.Internal; + +/// +/// Scope-scoped store of factory-event dispatches deferred by their +/// , plus the drain primitive that runs them. +/// +/// +/// +/// Public because generated factory code calls at the entry-call +/// boundary; it lives in the Internal namespace alongside +/// and , which +/// are public for the same reason. +/// +/// +/// State is per DI scope and holds no persistence concepts. A scope whose factory +/// operation fails is simply never drained — that is what makes rollback-discard +/// structural rather than a rule anyone has to remember. +/// +/// +public interface IFactoryEventPhaseScheduler +{ + /// True when any phase has deferred dispatches waiting. + bool HasPending { get; } + + /// Defers a handler dispatch until drains. + void Enqueue(DispatchPhase phase, FactoryEventBase factoryEvent, RaiseOptions options, Func handler); + + /// + /// Runs the deferred dispatches for and every earlier + /// phase, earliest first, until none are left. + /// + /// + /// Draining earlier phases too is what makes the drain total: a handler running here + /// can raise an event whose handlers sit in this phase or in one whose drain point has + /// already passed, and a consumer may never drain + /// at all. Either way the work joins this drain rather than being silently dropped. + /// Later phases than are left alone. + /// + /// The latest phase to drain; earlier phases drain first. + /// + /// Declares the drain point, which is what failure semantics key off — not the + /// phase. when the caller still has a transaction open, so a + /// handler exception propagates and the caller can roll back. + /// for a post-completion drain, where a throw can no longer roll anything back and is + /// therefore logged and swallowed per handler; + /// still propagates. + /// + /// Token passed to the drained handlers. + Task DrainAsync(DispatchPhase phase, bool inTransaction, CancellationToken cancellationToken = default); +} + +internal sealed class FactoryEventPhaseScheduler : IFactoryEventPhaseScheduler +{ + private readonly IServiceProvider _sp; + private readonly ILogger? _logger; + private readonly Dictionary> _deferred = new(); + + public FactoryEventPhaseScheduler(IServiceProvider sp, ILoggerFactory? loggerFactory = null) + { + _sp = sp; + _logger = loggerFactory?.CreateLogger(NeatooLoggerCategories.Server); + } + + private readonly record struct QueuedDispatch( + FactoryEventBase Event, + RaiseOptions Options, + Func Handler); + + public bool HasPending => _deferred.Any(q => q.Value.Count > 0); + + public void Enqueue(DispatchPhase phase, FactoryEventBase factoryEvent, RaiseOptions options, Func handler) + { + ArgumentNullException.ThrowIfNull(factoryEvent); + ArgumentNullException.ThrowIfNull(handler); + + if (!_deferred.TryGetValue(phase, out var queue)) + { + queue = new Queue(); + _deferred[phase] = queue; + } + + queue.Enqueue(new QueuedDispatch(factoryEvent, options, handler)); + + if (_logger?.IsEnabled(LogLevel.Debug) == true) + { + _logger.FactoryEventPhaseQueued(factoryEvent.GetType().Name, phase); + } + } + + public async Task DrainAsync(DispatchPhase phase, bool inTransaction, CancellationToken cancellationToken = default) + { + var drained = 0; + + // Dequeue one at a time rather than snapshotting: a handler running here may raise + // an event whose handlers are deferred, and those dispatches belong to this drain. + // TryDequeueThrough also picks up earlier phases, which a handler can still enqueue + // into after that phase's own drain point has passed — without this they would sit + // in a scope nobody drains again. An unterminated raise loop is the consumer's bug, + // exactly as it is for today's synchronous chained raises. + while (TryDequeueThrough(phase, out var dispatch, out var dispatchPhase)) + { + drained++; + + if (inTransaction) + { + await dispatch.Handler(_sp, dispatch.Event, dispatch.Options, cancellationToken).ConfigureAwait(false); + continue; + } + +#pragma warning disable CA1031 // Post-completion handler exceptions cannot roll anything back; swallowing is the contract. + try + { + await dispatch.Handler(_sp, dispatch.Event, dispatch.Options, cancellationToken).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + throw; + } + catch (Exception ex) + { + _logger?.FactoryEventPhaseHandlerFailed(dispatchPhase, dispatch.Event.GetType().Name, ex); + } +#pragma warning restore CA1031 + } + + if (drained > 0 && _logger?.IsEnabled(LogLevel.Debug) == true) + { + _logger.FactoryEventPhaseDrained(drained, phase); + } + } + + /// + /// Takes the next dispatch from the earliest non-empty phase at or before + /// , so cross-phase ordering holds even for work a handler + /// enqueues mid-drain. + /// + private bool TryDequeueThrough(DispatchPhase through, out QueuedDispatch dispatch, out DispatchPhase phase) + { + foreach (var candidate in _deferred.Keys.Where(p => p <= through).OrderBy(p => p)) + { + var queue = _deferred[candidate]; + if (queue.Count > 0) + { + dispatch = queue.Dequeue(); + phase = candidate; + return true; + } + } + + dispatch = default; + phase = through; + return false; + } +} diff --git a/src/RemoteFactory/Internal/Log.cs b/src/RemoteFactory/Internal/Log.cs index 2166b6d2..662d6160 100644 --- a/src/RemoteFactory/Internal/Log.cs +++ b/src/RemoteFactory/Internal/Log.cs @@ -470,4 +470,43 @@ public static partial void TraceSerializeCompleted( string typeName, long elapsedMs, int jsonLength); + + // ===== Phased Event Dispatch (9xxx) ===== + + [LoggerMessage( + EventId = 9001, + Level = LogLevel.Debug, + Message = "Factory event {EventType} queued for {Phase} dispatch")] + public static partial void FactoryEventPhaseQueued( + this ILogger logger, + string eventType, + DispatchPhase phase); + + [LoggerMessage( + EventId = 9002, + Level = LogLevel.Debug, + Message = "Drained {HandlerCount} queued handler dispatch(es) through phase {Phase}")] + public static partial void FactoryEventPhaseDrained( + this ILogger logger, + int handlerCount, + DispatchPhase phase); + + [LoggerMessage( + EventId = 9003, + Level = LogLevel.Error, + Message = "A {Phase} handler for factory event {EventType} threw after the factory operation completed; the exception was logged and swallowed because it can no longer roll anything back. Remaining queued handlers still run.")] + public static partial void FactoryEventPhaseHandlerFailed( + this ILogger logger, + DispatchPhase phase, + string eventType, + Exception? exception); + + [LoggerMessage( + EventId = 9004, + Level = LogLevel.Debug, + Message = "Factory event {EventType} has a {Phase} handler but no phase queue exists in this scope; dispatching it immediately instead.")] + public static partial void FactoryEventPhaseNoQueueInScope( + this ILogger logger, + string eventType, + DispatchPhase phase); } diff --git a/src/RemoteFactory/RaiseOptions.cs b/src/RemoteFactory/RaiseOptions.cs index 751e5f6e..fc5e2ef7 100644 --- a/src/RemoteFactory/RaiseOptions.cs +++ b/src/RemoteFactory/RaiseOptions.cs @@ -5,12 +5,18 @@ namespace Neatoo.RemoteFactory; /// /// /// -/// FactoryEvent raises are always shared-scope, sequential, and awaited: -/// handlers share the caller's DI scope (and therefore the caller's DbContext and +/// FactoryEvent raises are shared-scope, sequential, and awaited: handlers +/// share the caller's DI scope (and therefore the caller's DbContext and /// transaction), run one after another in unspecified order, and any handler exception /// aborts the remaining handlers and propagates to the caller. Across the client/server /// boundary the HTTP call stays open until every server-side handler completes. /// +/// +/// That describes handlers, which is the default and +/// was the only behavior before phases existed. Handlers registered at another +/// are queued at raise time and run when their phase drains — +/// still in the same scope, but no longer before Raise returns. +/// /// [Flags] public enum RaiseOptions diff --git a/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventPhaseRegistrationTests.cs b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventPhaseRegistrationTests.cs new file mode 100644 index 00000000..0fcd5e61 --- /dev/null +++ b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventPhaseRegistrationTests.cs @@ -0,0 +1,151 @@ +using Microsoft.Extensions.DependencyInjection; +using Neatoo.RemoteFactory; +using Neatoo.RemoteFactory.Internal; + +namespace RemoteFactory.UnitTests.Internal; + +/// +/// Covers phase-aware handler registration and the DI registration of the phase queue +/// across NeatooFactory modes. +/// +public class FactoryEventPhaseRegistrationTests +{ + private sealed record RegistrationEventA(string Value) : FactoryEventBase; + private sealed record RegistrationEventB(string Value) : FactoryEventBase; + private sealed record RegistrationEventC(string Value) : FactoryEventBase; + private sealed record RegistrationEventD(string Value) : FactoryEventBase; + + private sealed class HandlerOne { } + private sealed class HandlerTwo { } + + private static Func NoOp + => (_, _, _, _) => Task.CompletedTask; + + [Fact] + public void RegisterHandler_WithoutPhase_DefaultsToImmediate() + { + FactoryEventHandlerRegistry.RegisterHandler(typeof(HandlerOne), NoOp); + + var handlers = FactoryEventHandlerRegistry.GetHandlers(typeof(RegistrationEventA)); + + Assert.NotNull(handlers); + var entry = Assert.Single(handlers); + Assert.Equal(DispatchPhase.Immediate, entry.Phase); + } + + [Fact] + public void RegisterHandler_WithPhase_RoundTripsThePhase() + { + FactoryEventHandlerRegistry.RegisterHandler(typeof(HandlerOne), DispatchPhase.AfterCommit, NoOp); + FactoryEventHandlerRegistry.RegisterHandler(typeof(HandlerTwo), DispatchPhase.AfterFlush, NoOp); + + var handlers = FactoryEventHandlerRegistry.GetHandlers(typeof(RegistrationEventB)); + + Assert.NotNull(handlers); + Assert.Equal(2, handlers.Count); + Assert.Contains(handlers, h => h.Phase == DispatchPhase.AfterCommit); + Assert.Contains(handlers, h => h.Phase == DispatchPhase.AfterFlush); + } + + [Fact] + public void RegisterHandler_RepeatedContainerBuilds_DedupesByEventAndHandlerClass() + { + // Simulates multiple DI container builds in one test run. + FactoryEventHandlerRegistry.RegisterHandler(typeof(HandlerOne), DispatchPhase.AfterCommit, NoOp); + FactoryEventHandlerRegistry.RegisterHandler(typeof(HandlerOne), DispatchPhase.AfterCommit, NoOp); + FactoryEventHandlerRegistry.RegisterHandler(typeof(HandlerOne), DispatchPhase.AfterCommit, NoOp); + + var handlers = FactoryEventHandlerRegistry.GetHandlers(typeof(RegistrationEventC)); + + Assert.NotNull(handlers); + Assert.Single(handlers); + } + + [Fact] + public void RegisterHandler_SameHandlerClassTwoPhases_KeepsTheFirstRegistration() + { + // Documents the interim semantics of the (eventType, handlerClassType) dedupe key: + // a handler class declaring one event at two phases keeps the phase registered + // first, for the life of the process. PHASE-002 decides whether the generator + // should diagnose this instead. + FactoryEventHandlerRegistry.RegisterHandler(typeof(HandlerOne), DispatchPhase.AfterCommit, NoOp); + FactoryEventHandlerRegistry.RegisterHandler(typeof(HandlerOne), DispatchPhase.Immediate, NoOp); + + var handlers = FactoryEventHandlerRegistry.GetHandlers(typeof(RegistrationEventD)); + + Assert.NotNull(handlers); + var entry = Assert.Single(handlers); + Assert.Equal(DispatchPhase.AfterCommit, entry.Phase); + } + + [Fact] + public void Attribute_NoArgument_DefaultsToImmediate() + { + var attribute = new FactoryEventHandlerAttribute(); + + Assert.Equal(DispatchPhase.Immediate, attribute.Phase); + } + + [Theory] + [InlineData(DispatchPhase.Immediate)] + [InlineData(DispatchPhase.AfterFlush)] + [InlineData(DispatchPhase.AfterCommit)] + public void Attribute_ExplicitPhase_RoundTrips(DispatchPhase phase) + { + var attribute = new FactoryEventHandlerAttribute(phase); + + Assert.Equal(phase, attribute.Phase); + } + + [Theory] + [InlineData(NeatooFactory.Server)] + [InlineData(NeatooFactory.Logical)] + public void PhaseDispatcher_RegisteredInModesThatDispatchHandlers(NeatooFactory mode) + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddNeatooRemoteFactory(mode, typeof(FactoryEventPhaseRegistrationTests).Assembly); + + using var provider = services.BuildServiceProvider(); + using var scope = provider.CreateScope(); + + Assert.NotNull(scope.ServiceProvider.GetService()); + } + + [Fact] + public void PhaseDispatcher_NotRegisteredInRemoteMode() + { + // Remote mode sends raises to the server; no handlers dispatch client-side, so + // there is nothing to defer. + var services = new ServiceCollection(); + services.AddLogging(); + services.AddNeatooRemoteFactory(NeatooFactory.Remote, typeof(FactoryEventPhaseRegistrationTests).Assembly); + + using var provider = services.BuildServiceProvider(); + using var scope = provider.CreateScope(); + + Assert.Null(scope.ServiceProvider.GetService()); + } + + [Fact] + public void PhaseDispatcher_IsScoped_NotSharedAcrossScopes() + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddNeatooRemoteFactory(NeatooFactory.Server, typeof(FactoryEventPhaseRegistrationTests).Assembly); + + using var provider = services.BuildServiceProvider(); + using var scopeA = provider.CreateScope(); + using var scopeB = provider.CreateScope(); + + var a = scopeA.ServiceProvider.GetRequiredService(); + var b = scopeB.ServiceProvider.GetRequiredService(); + + Assert.NotSame(a, b); + + a.Enqueue(DispatchPhase.AfterCommit, new RegistrationEventA("x"), RaiseOptions.None, NoOp); + + Assert.True(a.HasPending); + Assert.False(b.HasPending); + } +} diff --git a/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventPhaseSchedulerTests.cs b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventPhaseSchedulerTests.cs new file mode 100644 index 00000000..4b7f5c79 --- /dev/null +++ b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventPhaseSchedulerTests.cs @@ -0,0 +1,409 @@ +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Neatoo.RemoteFactory; +using Neatoo.RemoteFactory.Internal; + +namespace RemoteFactory.UnitTests.Internal; + +/// +/// Covers the phase queue and its drain primitive: what gets deferred, what order it +/// drains in, and which drain points propagate handler exceptions versus swallow them. +/// +public class FactoryEventPhaseSchedulerTests +{ + private sealed record PhaseTestEvent(string Value) : FactoryEventBase; + + private static IFactoryEventPhaseScheduler NewDispatcher() + => NewDispatcher(out _); + + private static IFactoryEventPhaseScheduler NewDispatcher(out CapturingLoggerProvider logs) + { + var services = new ServiceCollection(); + services.AddLogging(); + var captured = new CapturingLoggerProvider(); + // Not disposed: the dispatcher holds a logger created from this factory for the + // lifetime of the test. + var loggerFactory = LoggerFactory.Create(b => + { + b.SetMinimumLevel(LogLevel.Debug); + b.AddProvider(captured); + }); + logs = captured; + return new FactoryEventPhaseScheduler(services.BuildServiceProvider(), loggerFactory); + } + + private sealed record LogEntry(int EventId, LogLevel Level, Exception? Exception, DispatchPhase? Phase); + + private sealed class CapturingLoggerProvider : ILoggerProvider + { + public List Entries { get; } = []; + + public ILogger CreateLogger(string categoryName) => new CapturingLogger(this); + + public void Dispose() { } + + private sealed class CapturingLogger(CapturingLoggerProvider owner) : ILogger + { + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) + { + DispatchPhase? phase = null; + if (state is IReadOnlyList> values) + { + foreach (var pair in values) + { + if (pair.Key == "Phase" && pair.Value is DispatchPhase p) + { + phase = p; + } + } + } + + lock (owner.Entries) + { + owner.Entries.Add(new LogEntry(eventId.Id, logLevel, exception, phase)); + } + } + } + } + + private static Func Recording(List log, string name) + => (_, _, _, _) => + { + log.Add(name); + return Task.CompletedTask; + }; + + private static Func Throwing(Exception ex) + => (_, _, _, _) => throw ex; + + [Fact] + public void Enqueue_DoesNotInvokeHandler() + { + var dispatcher = NewDispatcher(); + var log = new List(); + + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("a"), RaiseOptions.None, Recording(log, "handler")); + + Assert.Empty(log); + Assert.True(dispatcher.HasPending); + } + + [Fact] + public async Task DrainAsync_RunsDeferredDispatchesInFifoOrder() + { + var dispatcher = NewDispatcher(); + var log = new List(); + + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("a"), RaiseOptions.None, Recording(log, "first")); + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("b"), RaiseOptions.None, Recording(log, "second")); + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("c"), RaiseOptions.None, Recording(log, "third")); + + await dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + Assert.Equal(["first", "second", "third"], log); + Assert.False(dispatcher.HasPending); + } + + [Fact] + public async Task DrainAsync_OnlyDrainsTheRequestedPhase() + { + var dispatcher = NewDispatcher(); + var log = new List(); + + dispatcher.Enqueue(DispatchPhase.AfterFlush, new PhaseTestEvent("a"), RaiseOptions.None, Recording(log, "flush")); + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("b"), RaiseOptions.None, Recording(log, "commit")); + + await dispatcher.DrainAsync(DispatchPhase.AfterFlush, inTransaction: true); + + Assert.Equal(["flush"], log); + Assert.True(dispatcher.HasPending); + + await dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + Assert.Equal(["flush", "commit"], log); + Assert.False(dispatcher.HasPending); + } + + [Fact] + public async Task DrainAsync_PostCompletion_SwallowsHandlerExceptionAndRunsTheRest() + { + var dispatcher = NewDispatcher(); + var log = new List(); + + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("a"), RaiseOptions.None, Recording(log, "before")); + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("b"), RaiseOptions.None, Throwing(new InvalidOperationException("read-side blew up"))); + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("c"), RaiseOptions.None, Recording(log, "after")); + + await dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + Assert.Equal(["before", "after"], log); + } + + [Fact] + public async Task DrainAsync_PostCompletion_StillPropagatesCancellation() + { + var dispatcher = NewDispatcher(); + + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("a"), RaiseOptions.None, Throwing(new OperationCanceledException())); + + await Assert.ThrowsAsync( + () => dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false)); + } + + [Fact] + public async Task DrainAsync_InTransaction_PropagatesHandlerExceptionAndAbortsRemaining() + { + var dispatcher = NewDispatcher(); + var log = new List(); + + dispatcher.Enqueue(DispatchPhase.AfterFlush, new PhaseTestEvent("a"), RaiseOptions.None, Throwing(new InvalidOperationException("write-side blew up"))); + dispatcher.Enqueue(DispatchPhase.AfterFlush, new PhaseTestEvent("b"), RaiseOptions.None, Recording(log, "never runs")); + + await Assert.ThrowsAsync( + () => dispatcher.DrainAsync(DispatchPhase.AfterFlush, inTransaction: true)); + + Assert.Empty(log); + } + + [Fact] + public async Task DrainAsync_SamePhaseHandlersDrainPoint_KeysSemanticsNotThePhase() + { + // The same AfterFlush handlers get swallow semantics when drained at a + // post-completion point (the fail-open path a consumer who never drains hits). + var dispatcher = NewDispatcher(); + var log = new List(); + + dispatcher.Enqueue(DispatchPhase.AfterFlush, new PhaseTestEvent("a"), RaiseOptions.None, Throwing(new InvalidOperationException("boom"))); + dispatcher.Enqueue(DispatchPhase.AfterFlush, new PhaseTestEvent("b"), RaiseOptions.None, Recording(log, "still runs")); + + await dispatcher.DrainAsync(DispatchPhase.AfterFlush, inTransaction: false); + + Assert.Equal(["still runs"], log); + } + + [Fact] + public async Task DrainAsync_ReentrantEnqueueDuringDrain_RunsInTheSameDrain() + { + var dispatcher = NewDispatcher(); + var log = new List(); + + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("a"), RaiseOptions.None, (_, _, _, _) => + { + log.Add("outer"); + // A handler raising an event whose handlers land in the phase being drained. + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("nested"), RaiseOptions.None, Recording(log, "nested")); + return Task.CompletedTask; + }); + + await dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + Assert.Equal(["outer", "nested"], log); + Assert.False(dispatcher.HasPending); + } + + [Fact] + public async Task DrainAsync_NothingDeferred_IsANoOp() + { + var dispatcher = NewDispatcher(); + + await dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + Assert.False(dispatcher.HasPending); + } + + [Fact] + public async Task DrainAsync_DeferredDispatchesRunOnce_NotOnASecondDrain() + { + var dispatcher = NewDispatcher(); + var log = new List(); + + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("a"), RaiseOptions.None, Recording(log, "handler")); + + await dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + await dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + Assert.Equal(["handler"], log); + } + + [Fact] + public async Task DrainAsync_ReentrantEnqueueIntoAnAlreadyPassedPhase_StillRunsInThisDrain() + { + // The AfterCommit drain is the last one a scope gets. A handler running there that + // raises an event with an AfterFlush handler would otherwise leave that dispatch in + // a scope nobody drains again. + var dispatcher = NewDispatcher(); + var log = new List(); + + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("a"), RaiseOptions.None, (_, _, _, _) => + { + log.Add("commit"); + dispatcher.Enqueue(DispatchPhase.AfterFlush, new PhaseTestEvent("late"), RaiseOptions.None, Recording(log, "late-flush")); + return Task.CompletedTask; + }); + + await dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + Assert.Equal(["commit", "late-flush"], log); + Assert.False(dispatcher.HasPending); + } + + [Fact] + public async Task DrainAsync_SweepsAnEarlierPhaseTheConsumerNeverDrained() + { + // Fail-open: a consumer who never calls the AfterFlush drain point must not lose + // those handlers. The AfterCommit drain sweeps them up, earliest phase first, and + // they take the post-completion drain point's swallow semantics. + var dispatcher = NewDispatcher(out var logs); + var log = new List(); + + dispatcher.Enqueue(DispatchPhase.AfterFlush, new PhaseTestEvent("a"), RaiseOptions.None, Throwing(new InvalidOperationException("flush-side blew up"))); + dispatcher.Enqueue(DispatchPhase.AfterFlush, new PhaseTestEvent("b"), RaiseOptions.None, Recording(log, "flush")); + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("c"), RaiseOptions.None, Recording(log, "commit")); + + await dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + Assert.Equal(["flush", "commit"], log); + Assert.False(dispatcher.HasPending); + + // The failure is attributed to the phase the dispatch was queued at, not the phase + // that was requested. + var failure = Assert.Single(logs.Entries, e => e.EventId == 9003); + Assert.Equal(DispatchPhase.AfterFlush, failure.Phase); + } + + [Fact] + public async Task DrainAsync_MidDrainEarlierPhaseWork_PreemptsRemainingLaterPhaseWork() + { + // Pins the ordering choice rather than leaving it accidental: earlier-phase work + // created mid-drain runs before later-phase dispatches that were already queued. + var dispatcher = NewDispatcher(); + var log = new List(); + + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("a"), RaiseOptions.None, (_, _, _, _) => + { + log.Add("commit1"); + dispatcher.Enqueue(DispatchPhase.AfterFlush, new PhaseTestEvent("late"), RaiseOptions.None, Recording(log, "late-flush")); + return Task.CompletedTask; + }); + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("b"), RaiseOptions.None, Recording(log, "commit2")); + + await dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + Assert.Equal(["commit1", "late-flush", "commit2"], log); + } + + [Fact] + public async Task DrainAsync_DoesNotRunLaterPhasesThanRequested() + { + var dispatcher = NewDispatcher(); + var log = new List(); + + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("a"), RaiseOptions.None, Recording(log, "commit")); + + await dispatcher.DrainAsync(DispatchPhase.AfterFlush, inTransaction: true); + + Assert.Empty(log); + Assert.True(dispatcher.HasPending); + } + + [Fact] + public async Task DrainAsync_PostCompletionSwallow_LogsTheDedicatedEventIdWithTheException() + { + var dispatcher = NewDispatcher(out var logs); + var boom = new InvalidOperationException("read-side blew up"); + + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("a"), RaiseOptions.None, Throwing(boom)); + + await dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + var failure = Assert.Single(logs.Entries, e => e.EventId == 9003); + Assert.Equal(LogLevel.Error, failure.Level); + Assert.Same(boom, failure.Exception); + } + + [Fact] + public async Task DrainAsync_HandlerReceivesTheEventAndOptionsItWasQueuedWith() + { + var services = new ServiceCollection(); + services.AddLogging(); + var scopeProvider = services.BuildServiceProvider(); + var dispatcher = new FactoryEventPhaseScheduler(scopeProvider); + + var seen = new List<(string Value, RaiseOptions Options)>(); + var providers = new List(); + + Func capture = + (sp, evt, options, _) => + { + seen.Add((((PhaseTestEvent)evt).Value, options)); + providers.Add(sp); + return Task.CompletedTask; + }; + + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("first"), RaiseOptions.None, capture); + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("second"), RaiseOptions.ServerOnly, capture); + + await dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + Assert.Equal([("first", RaiseOptions.None), ("second", RaiseOptions.ServerOnly)], seen); + // Handlers resolve their [Service] dependencies from the originating scope. + Assert.All(providers, sp => Assert.Same(scopeProvider, sp)); + Assert.Equal(2, providers.Count); + } + + [Fact] + public async Task DrainAsync_HandlerReceivesTheDrainTimeCancellationToken() + { + // The token is supplied at drain time, not captured at raise time. PHASE-003 owns + // the policy question of which token a post-completion drain should pass; this + // pins the plumbing it will reason from. + var dispatcher = NewDispatcher(); + using var drainCts = new CancellationTokenSource(); + CancellationToken received = default; + + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("a"), RaiseOptions.None, (_, _, _, ct) => + { + received = ct; + return Task.CompletedTask; + }); + + await dispatcher.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false, drainCts.Token); + + Assert.Equal(drainCts.Token, received); + } + + [Fact] + public void Enqueue_NullEvent_Throws() + { + var dispatcher = NewDispatcher(); + + Assert.Throws( + () => dispatcher.Enqueue(DispatchPhase.AfterCommit, null!, RaiseOptions.None, (_, _, _, _) => Task.CompletedTask)); + } + + [Fact] + public void ScopeDisposedWithoutDraining_RunsNothing() + { + // Rollback-discard: a scope whose factory operation failed is never drained. The + // dispatcher must not run deferred work on the way out. + var services = new ServiceCollection(); + services.AddLogging(); + services.AddNeatooRemoteFactory(NeatooFactory.Server, typeof(FactoryEventPhaseSchedulerTests).Assembly); + + using var provider = services.BuildServiceProvider(); + var log = new List(); + + var scope = provider.CreateScope(); + var dispatcher = scope.ServiceProvider.GetRequiredService(); + dispatcher.Enqueue(DispatchPhase.AfterCommit, new PhaseTestEvent("a"), RaiseOptions.None, Recording(log, "handler")); + Assert.True(dispatcher.HasPending); + + scope.Dispose(); + + Assert.Empty(log); + } +} diff --git a/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs new file mode 100644 index 00000000..88a01fab --- /dev/null +++ b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs @@ -0,0 +1,252 @@ +using Microsoft.Extensions.DependencyInjection; +using Neatoo.RemoteFactory; +using Neatoo.RemoteFactory.Internal; + +namespace RemoteFactory.UnitTests.Internal; + +/// +/// Covers what does with phase-registered handlers: +/// Immediate dispatches as it always has, other phases defer to the scope's queue. +/// +public class FactoryEventsDispatcherPhaseTests +{ + private sealed record ImmediateOnlyEvent(string Value) : FactoryEventBase; + private sealed record DeferredOnlyEvent(string Value) : FactoryEventBase; + private sealed record MixedPhaseEvent(string Value) : FactoryEventBase; + private sealed record RelayCollectionEvent(string Value) : FactoryEventBase; + private sealed record UntypedRaiseEvent(string Value) : FactoryEventBase; + private sealed record ChainedSourceEvent(string Value) : FactoryEventBase; + private sealed record ChainedFollowUpEvent(string Value) : FactoryEventBase; + private sealed record NoQueueEvent(string Value) : FactoryEventBase; + + private sealed class ImmediateHandler { } + private sealed class DeferredHandler { } + + private static readonly List Dispatched = []; + + private static Func Recording(string name) + => (_, _, _, _) => + { + lock (Dispatched) + { + Dispatched.Add(name); + } + return Task.CompletedTask; + }; + + private static (ServiceProvider Provider, IServiceScope Scope) ServerScope() + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddNeatooRemoteFactory(NeatooFactory.Server, typeof(FactoryEventsDispatcherPhaseTests).Assembly); + var provider = services.BuildServiceProvider(); + return (provider, provider.CreateScope()); + } + + [Fact] + public async Task Raise_ImmediateHandler_DispatchesAtRaiseTime() + { + lock (Dispatched) { Dispatched.Clear(); } + FactoryEventHandlerRegistry.RegisterHandler(typeof(ImmediateHandler), DispatchPhase.Immediate, Recording("immediate")); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var events = scope.ServiceProvider.GetRequiredService(); + + await events.Raise(new ImmediateOnlyEvent("x")); + + lock (Dispatched) + { + Assert.Equal(["immediate"], Dispatched); + } + } + } + + [Fact] + public async Task Raise_DeferredHandler_DoesNotDispatchAtRaiseTime() + { + lock (Dispatched) { Dispatched.Clear(); } + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, Recording("deferred")); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var events = scope.ServiceProvider.GetRequiredService(); + var queue = scope.ServiceProvider.GetRequiredService(); + + await events.Raise(new DeferredOnlyEvent("x")); + + lock (Dispatched) + { + Assert.Empty(Dispatched); + } + Assert.True(queue.HasPending); + + await queue.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + lock (Dispatched) + { + Assert.Equal(["deferred"], Dispatched); + } + } + } + + [Fact] + public async Task Raise_MixedPhases_ImmediateRunsAndDeferredWaits() + { + lock (Dispatched) { Dispatched.Clear(); } + FactoryEventHandlerRegistry.RegisterHandler(typeof(ImmediateHandler), DispatchPhase.Immediate, Recording("immediate")); + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, Recording("deferred")); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var events = scope.ServiceProvider.GetRequiredService(); + var queue = scope.ServiceProvider.GetRequiredService(); + + await events.Raise(new MixedPhaseEvent("x")); + + lock (Dispatched) + { + Assert.Equal(["immediate"], Dispatched); + } + + await queue.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + // Cross-phase ordering: the Immediate handler completed before the deferred one ran. + lock (Dispatched) + { + Assert.Equal(["immediate", "deferred"], Dispatched); + } + } + } + + [Fact] + public async Task RaiseUntyped_DeferredHandler_DefersJustLikeRaise() + { + // RaiseUntyped is the path client-raised events take server-side, so it is the + // entry point for the most interesting phase case. + lock (Dispatched) { Dispatched.Clear(); } + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, Recording("deferred")); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var events = scope.ServiceProvider.GetRequiredService(); + var queue = scope.ServiceProvider.GetRequiredService(); + + await events.RaiseUntyped(new UntypedRaiseEvent("x")); + + lock (Dispatched) + { + Assert.Empty(Dispatched); + } + Assert.True(queue.HasPending); + + await queue.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + lock (Dispatched) + { + Assert.Equal(["deferred"], Dispatched); + } + } + } + + [Fact] + public async Task DrainedHandlerRaisingAnEvent_GoesThroughTheRealRaisePath() + { + // Re-entrancy as production hits it: handler -> IFactoryEvents.Raise -> registry + // lookup -> defer, rather than calling Enqueue directly. + lock (Dispatched) { Dispatched.Clear(); } + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var events = scope.ServiceProvider.GetRequiredService(); + var queue = scope.ServiceProvider.GetRequiredService(); + + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, Recording("follow-up")); + FactoryEventHandlerRegistry.RegisterHandler(typeof(ImmediateHandler), DispatchPhase.AfterCommit, async (sp, _, _, ct) => + { + lock (Dispatched) + { + Dispatched.Add("source"); + } + await sp.GetRequiredService().Raise(new ChainedFollowUpEvent("chained"), RaiseOptions.None, ct); + }); + + await events.Raise(new ChainedSourceEvent("x")); + await queue.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + + lock (Dispatched) + { + Assert.Equal(["source", "follow-up"], Dispatched); + } + Assert.False(queue.HasPending); + } + } + + [Fact] + public async Task Raise_PhasedHandlerWithNoQueueInScope_DispatchesImmediatelyRatherThanVanishing() + { + lock (Dispatched) { Dispatched.Clear(); } + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, Recording("deferred")); + + // A container without the phase queue registered — the fallback path in + // FactoryEventsDispatcher. + var services = new ServiceCollection(); + services.AddLogging(); + using var provider = services.BuildServiceProvider(); + var dispatcher = new FactoryEventsDispatcher(provider); + + await dispatcher.Raise(new NoQueueEvent("x")); + + lock (Dispatched) + { + Assert.Equal(["deferred"], Dispatched); + } + } + + [Fact] + public async Task Raise_DeferredHandler_StillCollectsForRelayAtRaiseTime() + { + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, Recording("deferred")); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var events = scope.ServiceProvider.GetRequiredService(); + var collector = scope.ServiceProvider.GetRequiredService(); + + await events.Raise(new RelayCollectionEvent("x")); + + var collected = Assert.Single(collector.GetCollectedEvents()); + Assert.IsType(collected); + } + } + + [Fact] + public async Task Raise_DeferredHandlerWithServerOnly_IsNotCollectedForRelay() + { + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, Recording("deferred")); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var events = scope.ServiceProvider.GetRequiredService(); + var collector = scope.ServiceProvider.GetRequiredService(); + + await events.Raise(new RelayCollectionEvent("x"), RaiseOptions.ServerOnly); + + Assert.Empty(collector.GetCollectedEvents()); + } + } +} From 18035aadd1443d07c7e61af52304c721331a3a7c Mon Sep 17 00:00:00 2001 From: Keith Voels Date: Fri, 14 Aug 2026 19:57:40 -0500 Subject: [PATCH 02/10] docs(todo): record RFEF sibling in PHASE Co-Authored-By: Claude Fable 5 --- docs/todos/PHASE-phased-event-dispatch/todo.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/todos/PHASE-phased-event-dispatch/todo.md b/docs/todos/PHASE-phased-event-dispatch/todo.md index 1e5467de..7de47c9f 100644 --- a/docs/todos/PHASE-phased-event-dispatch/todo.md +++ b/docs/todos/PHASE-phased-event-dispatch/todo.md @@ -127,7 +127,13 @@ exposes drain points. ## Sibling Todos -*(none)* +- [ ] [RFEF — RemoteFactory.EntityFrameworkCore, declarative factory transactions](../RFEF-factory-transactions/todo.md) + — surfaced 2026-08-14 while discussing this todo's target consumer code (per-method + begin/commit boilerplate); doesn't advance PHASE's goal (persistence stays out of this + framework arc) but builds directly on PHASE-003's entry-call tracking and PHASE-004's + drain semantics, and would generate the `AfterFlush` drain call PHASE-004 otherwise + leaves to consumer code. Blocked until those plans land. (Link resolves once the RFEF + branch merges to main.) --- From d25bef793e22e9d3132e23c16b937b757e8fd5ad Mon Sep 17 00:00:00 2001 From: Keith Voels Date: Fri, 14 Aug 2026 20:11:00 -0500 Subject: [PATCH 03/10] =?UTF-8?q?docs(todo):=20draft=20PHASE-003=20?= =?UTF-8?q?=E2=80=94=20entry-call=20tracking=20and=20AfterCommit=20drain?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ordering re-split recorded: PHASE-003 worked ahead of PHASE-002 (independent; riskiest first; RFEF blocked on it). Pre-flight Current State walked against the choke point, all three renderer seams, and the sync-method shape. Co-Authored-By: Claude Fable 5 --- .../plans/003-aftercommit-entry-call-drain.md | 267 +++++++++++++++++- .../todos/PHASE-phased-event-dispatch/todo.md | 13 + 2 files changed, 271 insertions(+), 9 deletions(-) diff --git a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md index 97cbee06..aee0594d 100644 --- a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md +++ b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md @@ -22,11 +22,71 @@ the keyboard: the three renderers share no pipeline helper; static factories hav `Local*` methods (DI delegate lambdas instead); public wrappers are mostly non-async; `LocalSave` nests into `LocalInsert`/`LocalUpdate`/`LocalDelete`; HTTP calls enter `Local*` directly, bypassing public wrappers. This plan does NOT own the consumer-facing drain API -(PHASE-004). +(PHASE-004), and does NOT thread the attribute's phase argument through the generator +(PHASE-002) — its tests register phased handlers through the registry's 3-arg overload. --- -## Constraints inherited from PHASE-001 (recorded at its Step 5 gate) +## Intent + +Today the framework has no notion of "a factory call is in flight" — phased dispatches are +queued (PHASE-001) but nothing ever drains them, and the dispatcher queues whenever a +scheduler exists in the scope regardless of whether any factory call is active. After this +plan, the framework knows when an entry call begins and ends: + +- **Entry-call tracking** is a per-scope, depth-aware notion. Nested factory work — the + `LocalSave` → `LocalInsert` nesting, one factory method invoking another factory, the + HTTP handler wrapping a `Local*` method — increments depth rather than creating a second + entry. Only the outermost completion is "the entry call completing." +- **Two entry families, one contract.** Remote entries (every `[Remote]` factory call and + every client-raised event) all pass through the single runtime choke point that resolves + and invokes the DI-registered delegate; that choke point marks the entry and drains on + success, before relay collection, so events raised by `AfterCommit` handlers join the + same HTTP response's relay batch. Local/direct entries (Logical mode, server-side code + calling a factory) are marked by generated code at the local execution seam in all three + factory patterns. Both families drain by calling the PHASE-001 scheduler + (`DrainAsync(AfterCommit, inTransaction: false, …)` — which sweeps `AfterFlush` first, + giving PHASE-004's fail-open its drain point for free). +- **Failure discards structurally.** The drain call exists only on the success path. An + entry call that throws — or is forbidden by authorization — simply never drains; the + scope dies with its queues. No discard code, nothing to get wrong on the failure path. +- **Raise outside any factory call stops queueing.** When no entry call is active, phased + handlers dispatch immediately with a debug-level log — the designed replacement for + PHASE-001's interim "queue and hope someone drains" behavior. The existing no-scheduler + fallback (9004) remains for scopes that have no scheduler at all. +- **Cancellation policy (closes plan-review B-C5):** entry-call drains pass no + cancellation token (`CancellationToken.None`). By the time an `AfterCommit` drain runs, + the entry call has already succeeded; honoring the request token would let a client + disconnect — or a token cancelled between success and drain — fail a call that already + succeeded, which is the exact failure mode B-C5 flagged. The scheduler's API and its + drain-time-token contract are unchanged; the *entry caller* chooses `None`. + +--- + +## Framework & Architectural Alignment + +- **Persistence-agnostic:** "entry call completes" is the framework's only observable + signal — no flush, no commit, no transaction awareness. (RFEF later builds transaction + scoping on exactly this tracking; keep the seam clean.) +- **Forwarding-holder / trimming invariants (v1.7.0):** nothing in this plan ships handler + bodies to trimmed clients; entry tracking in generated code must be inert in Remote-mode + containers (no scheduler registered there) and sit behind the existing + `NeatooRuntime.IsServerRuntime` guard structure where applicable. +- **Generator/runtime boundary:** generated code reaches the scheduler via the public + `Neatoo.RemoteFactory.Internal` surface, matched by name. The Generator project must + not link any new runtime source (the PHASE-001 CS0436 lesson; warning recorded in + `FactoryEventHandlerAttribute`'s XML doc). +- **Wrapper-splitting precedent:** where a non-async generated method needs post-completion + work, the established shape is the guard + `*Core` split (`RenderLocalMethodOpening`, + TRIM-009); prefer extending that shape over inventing a new one. +- **Log events:** new ids continue the 9xxx phased-dispatch block in `Internal/Log.cs` + with matching rows in `CLAUDE-DESIGN.md`'s Runtime Log Events table. + +--- + +## Constraints & Invariants + +Inherited from PHASE-001 (recorded at its Step 5 gate): - **The drain call sits on the success path only** — never in a `finally`, never in a scope-disposal hook or middleware that runs on failure. Rollback-discard is emergent @@ -35,14 +95,203 @@ directly, bypassing public wrappers. This plan does NOT own the consumer-facing - The scheduler API to call is `IFactoryEventPhaseScheduler.DrainAsync(phase, inTransaction, ct)` in `Neatoo.RemoteFactory.Internal` — public so generated code can reach it. Pass `inTransaction: false` at the entry-call drain point. -- The cancellation-token *policy* question is open here: queued dispatches currently - receive the drain-time token (pinned by - `FactoryEventPhaseSchedulerTests.DrainAsync_HandlerReceivesTheDrainTimeCancellationToken`). - Decide whether a post-completion drain should pass the request token at all — an - `OperationCanceledException` from it fails a call that already succeeded (plan review - B-C5). +- The cancellation-token *policy* question (plan review B-C5) is resolved in this plan: + entry drains pass `CancellationToken.None` — see Intent. The scheduler-level pin + (`DrainAsync_HandlerReceivesTheDrainTimeCancellationToken`) stays valid: the drain-time + token at an entry drain *is* `None`. - `IFactoryEvents.RaiseUntyped` has no general test coverage repo-wide; it is the server-side landing point for client-raised events, so this plan's remote-entry work is the natural place to add it (tech debt raised at PHASE-001's gate). -*(Stub — Intent, Alignment, remaining Constraints, Steps, Acceptance filled at Step 2.)* +New in this plan: + +- **Depth correctness is the invariant everything hangs on:** a drain that fires at a + nested completion (e.g., inside `LocalInsert` while `LocalSave` is still the entry) + runs handlers mid-operation — in-transaction once RFEF exists. One entry, one drain, + at the outermost successful completion only. +- **The authorization-forbidden path is a failure path.** The remote choke point returns + a success-shaped empty response for `AspForbidException`; it must not drain. +- **No silent loss from sync entries.** Some factory methods generate synchronous, + non-`Task` signatures (value-object `Create`). A `Raise` inside one can enqueue phased + work before any await. Whatever shape the keyboard picks (block-drain when pending, + immediate-dispatch semantics for sync entries, or a generator diagnostic), queued work + must not evaporate — and the chosen shape gets a Plan Amendment recording it. +- **Planned restatement of PHASE-001 interim pins — not test-gutting.** PHASE-001's gate + annotated specific tests as pinning interim behavior this plan is chartered to invert + (dispatcher queues whenever a scheduler exists; nothing drains at entry). Those tests + are amended here with intent preserved and each amendment listed in this plan's Test + Evidence; every other existing test passes unmodified. +- **Remote-mode containers are untouched:** no scheduler, no tracker, no behavior change + client-side; generated entry-tracking code must resolve services null-tolerantly. + +--- + +## Steps + +1. Add entry-call tracking to the runtime: per-scope, depth-aware begin/end with + drain-on-outermost-success. Whether it lives on the scheduler or as a small sibling + service in `Internal` is a keyboard decision; it must be reachable from both runtime + and generated code. +2. Wire the remote choke point: mark entry around delegate invocation in the portal + request handler; on success, drain before relay collection so drained-handler events + join the same response. Forbidden and thrown paths never drain. +3. Change the dispatcher's queue-or-dispatch decision: queue phased handlers only while + an entry call is active; otherwise dispatch immediately and log a new debug event id + ("raise outside factory call"). Keep the existing no-scheduler fallback (9004). +4. Emit entry begin/drain in the class-factory renderer at the local execution seam + (`Local*` methods), following the guard + `*Core` split precedent; verify the + `LocalSave` → `LocalInsert`/`Update`/`Delete` nesting drains exactly once. +5. Do the same for the interface-factory renderer (its `Local*` seam) and the + static-factory renderer (its server-side DI lambda seam, including `[Execute]` + delegates). +6. Resolve the sync (non-`Task`) factory-method shape at the keyboard under the + no-silent-loss invariant; record the chosen shape as a Plan Amendment. +7. Amend the PHASE-001 interim-behavior tests flagged for restatement (dispatcher + queue-when-scheduler-present pins), preserving each test's original intent; list every + amended test in Test Evidence. +8. Add `RaiseUntyped` remote-entry coverage: a client-raised event with phased handlers + gets entry semantics (queued during dispatch, drained after the raise delegate + completes, relayed in the same response). +9. End-to-end integration coverage via `ClientServerContainers` with handlers registered + through the registry's 3-arg overload (PHASE-002 not yet landed): success drain, relay + batch inclusion, rollback-discard, handler-failure swallow not failing the response. +10. Add the new log event ids to `Internal/Log.cs` and the `CLAUDE-DESIGN.md` Runtime Log + Events table. + +--- + +## Acceptance + +- [ ] An `AfterCommit` handler runs after the entry factory call completes for an + HTTP-dispatched `[Remote]` call, and events it raises reach the client in the same + response's relay batch. `[integration]` +- [ ] An `AfterCommit` handler runs after the entry factory call completes for a direct + Logical/server-side invocation through the public factory wrapper. `[integration]` +- [ ] A `Save` on an entity (the `LocalSave` → `LocalInsert` nesting) drains exactly once, + after the outermost save completes — never at the nested completion. `[integration]` +- [ ] Queued `AfterFlush` dispatches run before queued `AfterCommit` dispatches at the + entry drain (sweep order observable at the entry level, not just the scheduler + level). `[integration]` +- [ ] If the entry factory call throws, queued phased handlers never run — for both the + remote choke point and the direct/local path. `[integration]` +- [ ] An authorization-forbidden remote call does not drain. `[integration]` +- [ ] An `AfterCommit` handler exception at the entry drain is logged (9003) and swallowed + and does not fail the entry call's response; remaining queued handlers still run. + `[integration]` +- [ ] A client-raised event (`RaiseUntyped` remote path) with phased handlers gets entry + semantics: drained after the raise completes, relayed in the same response. + `[integration]` +- [ ] A server-side `Raise` outside any factory call with phase-registered handlers + dispatches immediately with a debug-level log (no queue growth, no silent drop). + `[unit]` +- [ ] The entry drain passes no cancellation token: cancelling the request token after the + entry call succeeds does not abort the drain. `[unit]` +- [ ] A synchronous (non-`Task`) factory method that enqueued phased work does not lose it + silently. `[unit]` +- [ ] Nested factory calls (a factory method invoking another factory) do not drain at the + inner completion. `[unit]` +- [ ] Backward compatibility: the full existing suite passes with only the pre-declared + PHASE-001 interim-pin amendments (each listed in Test Evidence with intent + preserved). `[integration]` + +--- + +## Current State (Pre-Flight) + +*Walked 2026-08-14, on `PHASE-003-aftercommit-entry-call-drain` @ `18035aa` (stacked on +the PHASE-001 branch — its scheduler/registry/dispatcher work is not yet on `main`).* + +**The remote choke point** — `src/RemoteFactory/HandleRemoteDelegateRequest.cs`, +`LocalServer.HandlePortalRequest` returns the `HandleRemoteDelegateRequest` delegate +(line 63). Success path: `method.DynamicInvoke(invokeParams)` (103), `await task` (115), +post-invoke cancellation check (137), relay collection from `IFactoryEventCollector` +(159–177), response serialization (181). Failure exits: `OperationCanceledException` +rethrow (190), `AspForbidException` → **returns a success-shaped +`RemoteResponseDto(string.Empty)`** (196–202) — a failure path despite the return, must +not drain — and general rethrow (203–208). The drain point goes between delegate +completion and relay collection. Both the AspNetCore endpoint +(`WebApplicationExtensions.cs:56`) and the integration-test containers +(`ClientServerContainers.cs:65,120`) invoke this same delegate — one choke point covers +every remote entry. + +**Remote event raises land here too** — `RemoteFactoryEvents` (client) sends +`RaiseFactoryEventRemote`; the server registers that delegate as a scoped lambda +forwarding to `IFactoryEvents.RaiseUntyped` (`AddRemoteFactoryServices.cs:91–96`), so a +client `Raise` is just another delegate invocation through the choke point. + +**DI wiring** — scheduler registered scoped for Server AND Logical +(`AddRemoteFactoryServices.cs:84–85`, `TryAddScoped`); `IFactoryEventCollector` is +Server-only (138); `HandleRemoteDelegateRequest` registered transient at 140–144 (the +AspNetCore package registers its own scoped copy, `ServiceCollectionExtensions.cs:33`). +Remote mode registers neither scheduler nor collector — client containers have no phased +machinery at all. + +**The dispatcher's queue decision** — `FactoryEventsDispatcher.DispatchToHandlers` +(`FactoryEventsDispatcher.cs:49–84`): non-Immediate + scheduler present → `Enqueue` and +continue (69–75); scheduler absent → 9004 debug log then immediate dispatch (77–82). +There is no "is an entry call active" input to this decision today — that's the Step 3 +change site. + +**Generated class factory** (walked `Design.Domain…OrderFactory.g.cs`): public wrapper +`Create` forwards to `CreateProperty` (54–57), a delegate property the constructors point +at `LocalCreate` (server/logical ctor, 37–43) or `RemoteCreate` (remote ctor, 45–52). +`LocalCreate` is sync-returning-`Task` — ends in `Task.FromResult` (92). `LocalInsert` is +the guard + `LocalInsertCore` async split (150–157 → 157–218) — the TRIM-009 precedent +shape. `LocalSave` is a sync forwarder that returns `LocalInsert`/`LocalUpdate`/ +`LocalDelete`'s task directly (370–391). The registrar's DI delegate lambdas route +`CreateDelegate`/`FetchDelegate`/`SaveDelegate` to the `Local*` methods (419–433) — that +is what `DynamicInvoke` executes on the HTTP path, bypassing the public wrappers. +**Depth note:** on the HTTP path the choke point holds depth 1 while `Local*` runs at +depth 2 — generated-code drains must be depth-gated or the HTTP path double-drains. + +**Generated static factory** (walked `…ExampleCommandsFactory.g.cs`): no `Local*` +methods; the server-side registration is a DI lambda that resolves `[Service]` parameters +and invokes the hidden `_Method` (30–37); the Remote registration is a lambda forwarding +to `IMakeRemoteDelegateRequest` (20–23). The server lambda body is the entry seam. The +lambda is non-async, returning the user method's task directly. + +**Generated interface factory** (walked `…IExampleRepositoryFactory.g.cs`): shaped like +the class factory — `Local*` methods (58, 76, 94) with delegate registrations — same seam +as the class renderer. + +**Sync factory methods are real** — `…MoneyFactory.g.cs` generates +`public virtual Money Create(...)` (37) and a sync `LocalCreate` (42): non-`Task` +signatures with no place to await a drain. Source of the no-silent-loss constraint. + +**Renderer seams** — `ClassFactoryRenderer.RenderLocalMethodOpening` +(`ClassFactoryRenderer.cs:370`, called from 5 sites: 422, 846, 899, 1117, 1384) is the +wrapper-splitting helper; `InterfaceFactoryRenderer.RenderLocalMethod` +(`InterfaceFactoryRenderer.cs:250`); `StaticFactoryRenderer` emits the registrar lambdas. +No shared pipeline helper across the three — each gets its own emission change. + +**Scheduler surface** (PHASE-001) — `IFactoryEventPhaseScheduler` in +`Internal/FactoryEventPhaseScheduler.cs`: `bool HasPending`, `Enqueue(phase, evt, +options, invoke)`, `DrainAsync(phase, inTransaction, ct = default)`; drain sweeps the +requested phase and all earlier phases, earliest first, drain-until-empty; `inTransaction: +false` → swallow + 9003, OCE rethrows. No entry-call/depth state exists anywhere yet. + +--- + +## Test Evidence + +*(filled before the Step 5 gate)* + +--- + +## Plan Amendments + +*(none yet)* + +--- + +## Notes + +- **Branching:** this plan's branch is stacked on `PHASE-001-phase-model-and-queueing` + rather than the `PHASE` todo branch — PHASE-001's implementation (scheduler, registry + phase, dispatcher queueing) hasn't merged to `main` yet and this plan builds directly + on it. When PHASE-001 merges, this branch rebases/merges forward per the conventions' + "pull main back" step. +- **Ordering:** worked ahead of PHASE-002 (see the todo's Discovery Log entry of + 2026-08-14). Tests here register phased handlers via the registry's 3-arg overload; + PHASE-002 later makes the attribute's phase argument flow end-to-end and owns the + duplicate-registration diagnostic decision. diff --git a/docs/todos/PHASE-phased-event-dispatch/todo.md b/docs/todos/PHASE-phased-event-dispatch/todo.md index 7de47c9f..ca622b42 100644 --- a/docs/todos/PHASE-phased-event-dispatch/todo.md +++ b/docs/todos/PHASE-phased-event-dispatch/todo.md @@ -82,6 +82,19 @@ exposes drain points. ## Discovery Log +### 2026-08-14 — Ordering: PHASE-003 worked ahead of PHASE-002 +- **Finding:** PHASE-002 (generator threads the attribute's phase argument to + registration) and PHASE-003 (entry-call tracking + AfterCommit drain) are independent: + PHASE-003's tests can register phased handlers through the registry's 3-arg overload + directly, so nothing in it waits on the generator pass-through. +- **Decision:** Re-split (ordering only — no index rows change). Work PHASE-003 next: + it is the plan the review flagged as riskiest, it restates PHASE-001's interim + acceptance pins, and the RFEF sibling todo is blocked on it. PHASE-002 follows and + makes the attribute phase flow end-to-end. +- **Follow-up:** PHASE-003's branch is stacked on the PHASE-001 plan branch (its + scheduler work isn't on `main` yet) — a recorded deviation from the "plan branches off + the todo branch" convention; see the plan's Notes. + ### 2026-08-14 — PHASE-001 (gate found a real defect) - **Finding:** The test-review gate caught that the drain resolved only the requested phase's queue, so work a handler enqueued into an already-passed phase was silently From 29250350f1d172f762e2eb9f7c6acc413ae27545 Mon Sep 17 00:00:00 2001 From: Keith Voels Date: Fri, 14 Aug 2026 20:40:33 -0500 Subject: [PATCH 04/10] =?UTF-8?q?docs(todo):=20PHASE-003=20plan=20review?= =?UTF-8?q?=20(CONCERNS)=20=E2=80=94=206=20vetoes=20addressed=20by=20draft?= =?UTF-8?q?=20amendment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Failure path now clears explicitly at outermost exit (never drains); entry stays active through the drain (B-V3); interface-renderer walk corrected (inline guard, no split, TRIM item 20); drain pinned before the choke point's post-invoke cancellation check; pin-amendment set widened to six named tests; client-raise relay gap logged as deferred discovery (A-V1). Co-Authored-By: Claude Fable 5 --- .../plans/003-aftercommit-entry-call-drain.md | 195 +++++++++++++----- .../reviews/003-plan-review.md | 157 ++++++++++++++ .../todos/PHASE-phased-event-dispatch/todo.md | 26 +++ 3 files changed, 330 insertions(+), 48 deletions(-) create mode 100644 docs/todos/PHASE-phased-event-dispatch/reviews/003-plan-review.md diff --git a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md index aee0594d..b4fba43e 100644 --- a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md +++ b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md @@ -20,8 +20,9 @@ the "Raise outside any factory call" semantics (dispatch immediately, debug log) that is the absence-of-entry-tracking case. Known recon risks this plan must resolve at the keyboard: the three renderers share no pipeline helper; static factories have no `Local*` methods (DI delegate lambdas instead); public wrappers are mostly non-async; -`LocalSave` nests into `LocalInsert`/`LocalUpdate`/`LocalDelete`; HTTP calls enter `Local*` -directly, bypassing public wrappers. This plan does NOT own the consumer-facing drain API +`LocalSave` nests into `LocalInsert`/`LocalUpdate`/`LocalDelete`; HTTP calls enter +`Local*` directly on the class leg but through the public wrapper on the interface leg. +This plan does NOT own the consumer-facing drain API (PHASE-004), and does NOT thread the attribute's phase argument through the generator (PHASE-002) — its tests register phased handlers through the registry's 3-arg overload. @@ -42,14 +43,31 @@ plan, the framework knows when an entry call begins and ends: every client-raised event) all pass through the single runtime choke point that resolves and invokes the DI-registered delegate; that choke point marks the entry and drains on success, before relay collection, so events raised by `AfterCommit` handlers join the - same HTTP response's relay batch. Local/direct entries (Logical mode, server-side code - calling a factory) are marked by generated code at the local execution seam in all three - factory patterns. Both families drain by calling the PHASE-001 scheduler - (`DrainAsync(AfterCommit, inTransaction: false, …)` — which sweeps `AfterFlush` first, - giving PHASE-004's fail-open its drain point for free). -- **Failure discards structurally.** The drain call exists only on the success path. An - entry call that throws — or is forbidden by authorization — simply never drains; the - scope dies with its queues. No discard code, nothing to get wrong on the failure path. + same HTTP response's relay batch — and *before* the choke point's post-invoke + cancellation check, so a token cancelled between success and drain cannot skip the + drain (plan review B-V2: the same failure mode B-C5 flagged, relocated). Local/direct + entries (Logical mode, server-side code calling a factory) are marked by generated code + at the local execution seam in all three factory patterns. Both families drain by + calling the PHASE-001 scheduler (`DrainAsync(AfterCommit, inTransaction: false, …)` — + which sweeps `AfterFlush` first, giving PHASE-004's fail-open its drain point for + free). +- **Failure discards explicitly — a clear, never a drain.** The drain call exists only on + the success path. At the *outermost* exit of a failed entry call, the queues are + **cleared** (plan review A-V2: "the scope dies with its queues" is false for long-lived + scopes — Logical mode, Blazor Server circuits, and the integration harness's single + reused server scope — where surviving queues would drain into the *next* successful + call). Clearing is not draining: PHASE-001's C4 constraint forbids running handlers on + the failure path; it says nothing against discarding them there. The resulting + invariant: **between entry calls, the scheduler is always empty.** (The + `AspForbidException` denial shape throws and therefore clears; the `Authorized` + denial shape returns normally — a successful call whose body never ran — and drains an + empty queue harmlessly.) +- **The entry stays active for the duration of the entry drain** (plan review B-V3): + depth pops only after the drain completes. An event raised *by* a drained handler + therefore still queues through the dispatcher and joins the current drain via the + scheduler's drain-until-empty contract — preserving the sweep behavior installed by + PHASE-001's gate-defect fix. "Raise outside a factory call" can never trigger during a + drain. - **Raise outside any factory call stops queueing.** When no entry call is active, phased handlers dispatch immediately with a debug-level log — the designed replacement for PHASE-001's interim "queue and hope someone drains" behavior. The existing no-scheduler @@ -108,54 +126,96 @@ New in this plan: - **Depth correctness is the invariant everything hangs on:** a drain that fires at a nested completion (e.g., inside `LocalInsert` while `LocalSave` is still the entry) runs handlers mid-operation — in-transaction once RFEF exists. One entry, one drain, - at the outermost successful completion only. + at the outermost successful completion only. Two known traps (plan review B-C6, B-C2): + depth must release on *task completion*, not method return — `LocalSave` is a sync + forwarder with four return paths that returns inner tasks directly, so a naive + `try/finally` fires the drain mid-operation and `LocalSave` needs a split it doesn't + have; and the scheduler's state is unsynchronized while long-lived scopes (Blazor + Server circuits, Logical mode, the shared-scope harness) make concurrent flows in one + scope realistic — the keyboard decides the synchronization posture and records it. - **The authorization-forbidden path is a failure path.** The remote choke point returns - a success-shaped empty response for `AspForbidException`; it must not drain. + a success-shaped empty response for `AspForbidException`; it must not drain, and it + clears like any other failure. (The `Authorized` denial shape is a *successful* + call — see Intent.) - **No silent loss from sync entries.** Some factory methods generate synchronous, non-`Task` signatures (value-object `Create`). A `Raise` inside one can enqueue phased work before any await. Whatever shape the keyboard picks (block-drain when pending, immediate-dispatch semantics for sync entries, or a generator diagnostic), queued work - must not evaporate — and the chosen shape gets a Plan Amendment recording it. -- **Planned restatement of PHASE-001 interim pins — not test-gutting.** PHASE-001's gate - annotated specific tests as pinning interim behavior this plan is chartered to invert - (dispatcher queues whenever a scheduler exists; nothing drains at entry). Those tests - are amended here with intent preserved and each amendment listed in this plan's Test - Evidence; every other existing test passes unmodified. + must not evaporate — and the chosen shape gets a Plan Amendment recording it. Note the + sync shape is also **client-reachable**: `LocalCreate` on a value-object factory has no + `IsServerRuntime` guard and ships into trimmed client assemblies, so whatever is + emitted there must resolve services null-tolerantly and no-op cleanly (plan review + B-C3). +- **Planned restatement of PHASE-001 pins — pre-declared, not test-gutting.** The + following six tests (all in `FactoryEventsDispatcherPhaseTests` / + `FactoryEventPhaseRegistrationTests` / `FactoryEventPhaseSchedulerTests`) pin interim + behavior this plan is chartered to invert — the dispatcher queueing on a bare scope + with no entry call active, and drained-handler re-raise semantics with no entry + tracking. Each is amended with its original intent preserved and restated under entry + semantics, and each amendment is listed in this plan's Test Evidence (plan review + B-V4 widened this set beyond PHASE-001's three annotated bullets): + - `Raise_DeferredHandler_DoesNotDispatchAtRaiseTime` → defers *during an entry call*. + - `Raise_MixedPhases_ImmediateRunsAndDeferredWaits` → same restatement. + - `RaiseUntyped_DeferredHandler_DefersJustLikeRaise` → RaiseUntyped parity, restated + under an active entry (was never annotated interim — flagged by review as the gap). + - `PhaseDispatcher_IsScoped_NotSharedAcrossScopes` → scope isolation, restated with + entries active in each scope. + - `ScopeDisposedWithoutDraining_RunsNothing` → intent survives; the equivalent designed + behavior is now failure-clear plus never-queued-outside-entry. + - `DrainedHandlerRaisingAnEvent_GoesThroughTheRealRaisePath` → re-pointed to run under + entry-active-during-drain semantics so it can go red (review showed the current form + stays green under either B-V3 answer). + Every other existing test passes unmodified, including the Design solution's suite. - **Remote-mode containers are untouched:** no scheduler, no tracker, no behavior change client-side; generated entry-tracking code must resolve services null-tolerantly. +- **The client-raise relay gap is not this plan's to fix.** `ForDelegateEvent` discards + the response today, so nothing raised during a client-initiated `Raise` is ever relayed + back — a pre-existing gap affecting Immediate handlers equally, with an echo-to-self + design question attached (plan review A-V1). Recorded in the todo Discovery Log; + this plan's remote-raise Acceptance claims the drain only. --- ## Steps 1. Add entry-call tracking to the runtime: per-scope, depth-aware begin/end with - drain-on-outermost-success. Whether it lives on the scheduler or as a small sibling + drain-on-outermost-success and clear-on-outermost-failure; the entry stays active + until the drain completes. Whether it lives on the scheduler or as a small sibling service in `Internal` is a keyboard decision; it must be reachable from both runtime and generated code. 2. Wire the remote choke point: mark entry around delegate invocation in the portal - request handler; on success, drain before relay collection so drained-handler events - join the same response. Forbidden and thrown paths never drain. + request handler; on success, drain *before* the post-invoke cancellation check and + before relay collection so drained-handler events join the same response. Forbidden + and thrown paths never drain (they clear via the entry-exit failure path). 3. Change the dispatcher's queue-or-dispatch decision: queue phased handlers only while an entry call is active; otherwise dispatch immediately and log a new debug event id ("raise outside factory call"). Keep the existing no-scheduler fallback (9004). 4. Emit entry begin/drain in the class-factory renderer at the local execution seam (`Local*` methods), following the guard + `*Core` split precedent; verify the - `LocalSave` → `LocalInsert`/`Update`/`Delete` nesting drains exactly once. -5. Do the same for the interface-factory renderer (its `Local*` seam) and the - static-factory renderer (its server-side DI lambda seam, including `[Execute]` - delegates). -6. Resolve the sync (non-`Task`) factory-method shape at the keyboard under the + `LocalSave` → `LocalInsert`/`Update`/`Delete` nesting drains exactly once, with depth + released on task completion, not method return. +5. Interface-factory renderer: **introduce** the guard + `*Core` split on its `Local*` + seam — this leg has the guard inline today, no split, and its trimming behavior is + explicitly unverified (TRIM Deferred Work item 20), so this is a trimming-invariant + change, not a mechanical repeat of Step 4. Its delegate lambdas also route through + the public wrapper, adding one nesting level the depth gating must absorb. +6. Static-factory renderer: mark entry in the server-side DI lambda seam (including + `[Execute]` delegates) — the cheap leg: every delegate is `Task`-returning and the + `IsServerRuntime` guard wraps the registration, not the lambda body, so an async + lambda moves no guard into a state machine. +7. Resolve the sync (non-`Task`) factory-method shape at the keyboard under the no-silent-loss invariant; record the chosen shape as a Plan Amendment. -7. Amend the PHASE-001 interim-behavior tests flagged for restatement (dispatcher - queue-when-scheduler-present pins), preserving each test's original intent; list every - amended test in Test Evidence. -8. Add `RaiseUntyped` remote-entry coverage: a client-raised event with phased handlers +8. Amend the six pre-declared PHASE-001 pin tests (named in Constraints), preserving + each test's original intent restated under entry semantics; list every amendment in + Test Evidence. +9. Add `RaiseUntyped` remote-entry coverage: a client-raised event with phased handlers gets entry semantics (queued during dispatch, drained after the raise delegate - completes, relayed in the same response). -9. End-to-end integration coverage via `ClientServerContainers` with handlers registered - through the registry's 3-arg overload (PHASE-002 not yet landed): success drain, relay - batch inclusion, rollback-discard, handler-failure swallow not failing the response. -10. Add the new log event ids to `Internal/Log.cs` and the `CLAUDE-DESIGN.md` Runtime Log + completes). The relay half of that path is the deferred A-V1 gap — out of scope. +10. End-to-end integration coverage via `ClientServerContainers` with handlers registered + through the registry's 3-arg overload (PHASE-002 not yet landed): success drain, + relay batch inclusion, failure-clear (including a subsequent success in the same + scope), handler-failure swallow not failing the response. +11. Add the new log event ids to `Internal/Log.cs` and the `CLAUDE-DESIGN.md` Runtime Log Events table. --- @@ -166,7 +226,8 @@ New in this plan: HTTP-dispatched `[Remote]` call, and events it raises reach the client in the same response's relay batch. `[integration]` - [ ] An `AfterCommit` handler runs after the entry factory call completes for a direct - Logical/server-side invocation through the public factory wrapper. `[integration]` + Logical/server-side invocation through the public factory wrapper (a *class* + factory — interface factories register nothing in Logical mode). `[integration]` - [ ] A `Save` on an entity (the `LocalSave` → `LocalInsert` nesting) drains exactly once, after the outermost save completes — never at the nested completion. `[integration]` - [ ] Queued `AfterFlush` dispatches run before queued `AfterCommit` dispatches at the @@ -174,25 +235,38 @@ New in this plan: level). `[integration]` - [ ] If the entry factory call throws, queued phased handlers never run — for both the remote choke point and the direct/local path. `[integration]` -- [ ] An authorization-forbidden remote call does not drain. `[integration]` +- [ ] After a failed entry call, a *subsequent successful* call in the same scope runs + only its own queued handlers — the failure's queued work was cleared, not left to + ride the next drain (the long-lived-scope case: the harness's single reused server + scope is the natural fixture). `[integration]` +- [ ] An authorization-forbidden remote call does not drain: an entry that enqueues + phased work and then hits a forbidden call fails without running it (the falsifiable + form — a bare forbidden call has an empty queue and proves nothing). + `[integration]` - [ ] An `AfterCommit` handler exception at the entry drain is logged (9003) and swallowed and does not fail the entry call's response; remaining queued handlers still run. `[integration]` - [ ] A client-raised event (`RaiseUntyped` remote path) with phased handlers gets entry - semantics: drained after the raise completes, relayed in the same response. - `[integration]` + semantics: drained after the raise delegate completes. (Relay of that path is the + deferred A-V1 gap — not claimed here.) `[integration]` +- [ ] An event raised *by* an `AfterCommit` handler during the entry drain, itself having + phased handlers, joins the current drain — it is not dispatched inline as + "outside a factory call" (entry-active-during-drain, B-V3). `[unit]` - [ ] A server-side `Raise` outside any factory call with phase-registered handlers dispatches immediately with a debug-level log (no queue growth, no silent drop). `[unit]` -- [ ] The entry drain passes no cancellation token: cancelling the request token after the - entry call succeeds does not abort the drain. `[unit]` +- [ ] The entry drain passes no cancellation token, and it runs *before* the choke + point's post-invoke cancellation check: cancelling the request token after the + delegate succeeds neither aborts nor skips the drain (exercised at the choke point + — a scheduler-level assertion cannot fail on the placement risk). `[integration]` - [ ] A synchronous (non-`Task`) factory method that enqueued phased work does not lose it silently. `[unit]` - [ ] Nested factory calls (a factory method invoking another factory) do not drain at the inner completion. `[unit]` -- [ ] Backward compatibility: the full existing suite passes with only the pre-declared - PHASE-001 interim-pin amendments (each listed in Test Evidence with intent - preserved). `[integration]` +- [ ] Backward compatibility: the full existing suite — unit, integration, AND the + Design solution's suite (renderer changes regenerate every Design factory) — + passes with only the six pre-declared pin amendments named in Constraints (each + listed in Test Evidence with intent preserved). `[integration]` --- @@ -250,9 +324,19 @@ and invokes the hidden `_Method` (30–37); the Remote registration is a lambda to `IMakeRemoteDelegateRequest` (20–23). The server lambda body is the entry seam. The lambda is non-async, returning the user method's task directly. -**Generated interface factory** (walked `…IExampleRepositoryFactory.g.cs`): shaped like -the class factory — `Local*` methods (58, 76, 94) with delegate registrations — same seam -as the class renderer. +**Generated interface factory** (walked `…IExampleRepositoryFactory.g.cs`) — **corrected +per plan review B-V1; the original walk got this wrong.** It has `Local*` methods (58, +76, 94) but is NOT the class renderer's shape: `InterfaceFactoryRenderer.RenderLocalMethod` +(`InterfaceFactoryRenderer.cs:250–309`) emits the `IsServerRuntime` guard **inline** +(279–281) with no guard + `*Core` split and a conditional `async` keyword (252); its own +comment (272–278) marks body elimination on this leg UNVERIFIED — TRIM Deferred Work +item 20. Making its sync `Local*` forwarders `async` to await a drain would lower the +guard into `MoveNext`, the measured-bad shape `ClassFactoryRenderer.cs:350–368` +documents — so this leg gets the split *introduced*, not repeated. Its registrar lambdas +also route through the **public wrapper** (`IExampleRepositoryFactory.g.cs:122, 127, +132` call `factory.GetAllAsync()` → delegate property → `Local*`), one extra nesting +level vs. the class leg's direct `Local*` routing. And it registers for `Remote` (106) +and `Server` (115) only — **Logical mode registers nothing** for interface factories. **Sync factory methods are real** — `…MoneyFactory.g.cs` generates `public virtual Money Create(...)` (37) and a sync `LocalCreate` (42): non-`Task` @@ -268,7 +352,22 @@ No shared pipeline helper across the three — each gets its own emission change `Internal/FactoryEventPhaseScheduler.cs`: `bool HasPending`, `Enqueue(phase, evt, options, invoke)`, `DrainAsync(phase, inTransaction, ct = default)`; drain sweeps the requested phase and all earlier phases, earliest first, drain-until-empty; `inTransaction: -false` → swallow + 9003, OCE rethrows. No entry-call/depth state exists anywhere yet. +false` → swallow + 9003, OCE rethrows. No entry-call/depth state exists anywhere yet, no +`Clear` on the interface, and the internal `Dictionary>` is +unsynchronized. + +**Two registrations of the choke point** — the core package registers +`HandleRemoteDelegateRequest` transient (`AddRemoteFactoryServices.cs:140–144`); the +AspNetCore package registers a scoped copy that wins in a real server +(`ServiceCollectionExtensions.cs:33`). Both capture the resolving provider, so tracker +resolution sees the request scope either way — but "one choke point" means one code +path, not one registration. + +**Harness scope lifetime** — `ClientServerContainers` creates **one** server scope per +`Scopes()` call and reuses it for every remote call in a test (146–158, 62–65). Queues, +collector contents, and depth state persist across calls within a test — the fixture +that makes the failure-then-success Acceptance bullet natural, and a fidelity caveat for +every "drains exactly once" assertion. --- diff --git a/docs/todos/PHASE-phased-event-dispatch/reviews/003-plan-review.md b/docs/todos/PHASE-phased-event-dispatch/reviews/003-plan-review.md new file mode 100644 index 00000000..9a23c068 --- /dev/null +++ b/docs/todos/PHASE-phased-event-dispatch/reviews/003-plan-review.md @@ -0,0 +1,157 @@ +# PHASE-003 Plan Review — 2026-08-14 + +**Reviewer:** plan-reviewer agent (two-pass: A = documented requirements, B = codebase) +**Plan reviewed:** [../plans/003-aftercommit-entry-call-drain.md](../plans/003-aftercommit-entry-call-drain.md) (draft as of commit `d25bef7`) +**Verdict: CONCERNS** — 6 veto-tier findings, 10 callouts. All vetoes addressed by +draft amendment before implementation; disposition recorded at the end of this file. + +--- + +## Pass A — Plan vs. Documented Requirements + +### Veto-tier + +**A-V1 — Acceptance bullet 8's "relayed in the same response" is unreachable, and +satisfying it naively breaks a documented contract.** +The client-raise path has no relay today: `MakeRemoteDelegateRequest.ForDelegateEvent` +(`src/RemoteFactory/Internal/MakeRemoteDelegateRequest.cs:167-201`) awaits the round-trip +and discards the response — never reads `RelayedEvents`, never touches +`IFactoryEventRelay` (compare `ForDelegateNullable` at `:114-148`, which does). The +integration stand-in mirrors this (`ClientServerContainers.cs:111-121` vs `:75-106`). +The server *does* collect the client-raised event (`FactoryEventsDispatcher.cs:52-55` +runs for `RaiseUntyped` too), so naively wiring relay into `ForDelegateEvent` would echo +the client's own just-raised event back into its own `IFactoryEventRelay` — a behavior +change against CLAUDE-DESIGN.md's "One `[Remote]` call = exactly one `Relay` invocation" +semantics, decided in a plan that doesn't own the relay path. + +**A-V2 — "Rollback-discard is structural" is false for long-lived scopes, violating +todo AC-2.** +"The scope dies with its queues" holds only for per-operation scopes. In Logical mode, +Blazor Server (scope per circuit), and this repo's own integration harness +(`ClientServerContainers.cs:146-158` creates one server scope per `Scopes()` call, reused +for every remote call at `:62-65`), a failed entry call's queued work survives in +`FactoryEventPhaseScheduler._deferred`, and the sweep + drain-until-empty design +guarantees the *next successful entry call in that scope* drains it — handlers AC-2 says +must never run then run later, attached to an unrelated operation. The plan's +single-failing-call Acceptance bullet cannot catch this. Distinction the plan conflated: +PHASE-001 code review C4 forbids *draining* on the failure path — it says nothing about +*clearing*. + +### Callout-tier + +- **A-C1** — todo AC-6 traces cleanly; "fresh-scope execution out of scope" honored. No + conflict. +- **A-C2** — Renderer changes regenerate every Design factory; the backward-compat + Acceptance bullet should name the Design solution's test run explicitly (PHASE-001 + recorded it as a separate 86×2 run). +- **A-C3** — CLAUDE-DESIGN.md log-table location confirmed; new ids continue cleanly + after 9004. + +--- + +## Pass B — Plan vs. Codebase + +Current State verified largely accurate (choke-point line map, DI wiring, dispatcher +decision sites, class/static shapes, `MoneyFactory` sync shape, renderer seam +locations). `[Execute]`-on-class, ctor-injected, and `[Execute]`+ctor shapes all funnel +through the class renderer's `Local*` seam (covered by Step 4). `ILazyLoadFactory` +re-enters through normal factory seams — not an uncovered entry point. The dispatcher +*can* learn "entry active" from its position (scoped, same scope provider). + +### Veto-tier + +**B-V1 — The interface renderer is NOT "the same seam as the class renderer."** +`InterfaceFactoryRenderer.RenderLocalMethod` (`InterfaceFactoryRenderer.cs:250-309`) +emits the `IsServerRuntime` guard **inline** with **no** guard + `*Core` split; its own +comment records "THIS LEG HAS RECEIVED NEITHER FIX … Treat body elimination on this leg +as UNVERIFIED. Tracked as Deferred Work item 20 on the TRIM todo." Adding an awaited +drain forces `async` onto currently-sync `Local*` methods, lowering the guard into +`MoveNext` — the measured-bad shape `ClassFactoryRenderer.cs:350-368` documents. +Also: the interface leg's registrar lambdas route to the **public wrapper** +(`IExampleRepositoryFactory.g.cs:122,127,132`), not `Local*` — one extra nesting level +vs. the class leg. Step 5 understates this work by an order of magnitude. + +**B-V2 — The drain's position vs `HandleRemoteDelegateRequest.cs:137` decides whether +the cancellation Acceptance can hold.** +`ThrowIfCancellationRequested()` at `:137` sits inside the exact window the plan claims +for the drain. If the drain lands after `:137`, a token cancelled between success and +drain throws first and the drain never runs — the identical B-C5 failure mode, +relocated. The `[unit]` tier makes it worse: a scheduler-level assertion that +`CancellationToken.None` was received passes green regardless of drain placement. + +**B-V3 — "Queue only while an entry call is active" collides with drain-until-empty, +and the plan doesn't say which wins.** +If depth pops *before* `DrainAsync`, an event raised by a drained handler sees no entry +active → dispatches immediately and inline, silently voiding the sweep/drain-until-empty +behavior installed by PHASE-001's gate-defect fix. If depth pops *after*, the fix holds +but "raise outside a factory call" needs an explicit carve-out. Evidence it won't be +caught: `FactoryEventsDispatcherPhaseTests.DrainedHandlerRaisingAnEvent_GoesThroughTheRealRaisePath` +(`:161-193`) stays green under either answer while testing nothing. + +**B-V4 — The pre-declared amendment set is smaller than the actual breakage.** +PHASE-001 annotated three interim bullets (four tests). Step 3 additionally inverts +`RaiseUntyped_DeferredHandler_DefersJustLikeRaise` (`:129-158`, asserts deferral on a +bare-scope raise) — listed in PHASE-001's evidence as "additional coverage," never +annotated interim. With the B-V3 false-green re-point, that is two out-of-scope tests; +the global "Existing Tests Are Sacred — REPORT and ASK" rule applies. + +### Callout-tier + +- **B-C1 (observability)** — "Forbidden does not drain" is unfalsifiable as written: + the `Authorized` denial shape **returns successfully** (and would drain — an empty + queue, harmlessly, since the body never ran), and the `AspForbidException` shape + throws before anything can be queued. Falsifiable form: an outer entry enqueues, then + a forbidden inner call throws. Intent's "forbidden by authorization — simply never + drains" is only true for the throwing shape. +- **B-C2 (concurrency)** — `FactoryEventPhaseScheduler` uses an unsynchronized + `Dictionary>`; a depth counter inherits the hazard. Blazor + Server / Logical / the shared-scope harness make concurrent flows in one scope + realistic. PHASE-001's code review parked this for PHASE-003 — it lands here. +- **B-C3 (client-reachable emission)** — `MoneyFactory.LocalCreate` has **no** + `IsServerRuntime` guard; it runs client-side. Whatever Step 6 emits there ships into + trimmed client assemblies and must be null-tolerant at runtime. +- **B-C4 (static leg is the easy one)** — `StaticFactoryRenderer.cs:99` emits + `Task<…>`-returning delegates unconditionally (no sync shape), and its guard wraps the + *registration*, not the lambda body — making the lambda `async` moves no guard into a + state machine. Don't lump it with the interface leg. +- **B-C5 (Logical mode + interface factories)** — interface factories register for + `Remote` and `Server` only; **`Logical` registers nothing**. The Logical-mode + Acceptance bullet is only writable against a class or static factory. +- **B-C6 (`LocalSave` depth release)** — `LocalSave` is a `virtual` sync forwarder with + four return paths (one returning `Task.FromResult(default)` directly); a naive + `try/finally` decrements depth at *return*, before the inner task completes — firing + the drain mid-operation. Depth must release on task completion; `LocalSave` needs a + split it doesn't have. +- **B-C7 (harness fidelity)** — one server scope serves every remote call in a test; + queues, collector contents, and depth state persist across calls within a test. +- **B-C8 (two registrations of the choke point)** — core registers + `HandleRemoteDelegateRequest` transient; AspNetCore registers a scoped copy that wins + in a real server. Fine for tracker resolution, but "one choke point" ≠ "one + registration." +- **B-C9** — no Skills section; naming the trimming/wrapper-split rules would help. + +**Code-density check:** design-focused; zero code fences; file:line confined to Current +State (its sanctioned home). 13 Acceptance bullets is large but the charter is coherent — +no split recommended. + +--- + +## Orchestrator Disposition (2026-08-14) + +Calibration applied: diagnoses adopted; remedies picked at the keyboard. + +| Finding | Disposition | +|---|---| +| A-V1 | **Amended.** Acceptance bullet 8 split: drain half stays; relay half removed. The client-raise relay gap (pre-existing, affects Immediate handlers equally) recorded in the todo Discovery Log as a deferred question — not owned by this plan. | +| A-V2 | **Amended.** Failure discard is now explicit: outermost exit clears (never drains) on failure. New invariant: between entry calls the scheduler is empty. New Acceptance bullet: a failure followed by a success in the same scope runs only the success's handlers. | +| B-V1 | **Amended.** Current State corrected (inline guard, no split, TRIM item 20, wrapper-routed lambdas); Step 5 re-scoped: interface leg = introduce the split on trimming-unverified ground; static leg separated as the cheap case (B-C4). | +| B-V2 | **Amended.** Plan now pins the drain *before* the post-invoke cancellation check; cancellation Acceptance re-tiered to `[integration]` at the choke point. | +| B-V3 | **Amended.** Decision: the entry remains active for the duration of the entry drain (depth pops after the drain completes), preserving sweep/drain-until-empty and the V4 carve-out; "outside factory call" therefore cannot trigger during a drain. Re-entrancy Acceptance bullet added; `DrainedHandlerRaisingAnEvent…` re-pointed (listed in the amendment set). | +| B-V4 | **Amended.** Pre-declared amendment set widened to name all six tests, each with its preserved intent. Reported to the user in the session summary per the sacred-tests rule. | +| A-C2 | Folded in: backward-compat bullet names the Design solution run. | +| B-C1 | Folded in: Intent wording fixed; forbidden bullet made falsifiable (outer enqueues, inner forbid throws). | +| B-C2, B-C6 | Folded into Constraints (concurrency hazard; depth release on task completion). | +| B-C3 | Folded into Constraints (client-reachable unguarded sync shape must be null-tolerant). | +| B-C4, B-C5 | Folded into Steps/Acceptance (static leg separated; Logical bullet targets a class factory). | +| B-C7, B-C8 | Folded into Current State notes. | +| B-C9 | Skipped — no compact skill reference exists for the wrapper-split rules; the TRIM todo's docs are linked from Current State instead. | diff --git a/docs/todos/PHASE-phased-event-dispatch/todo.md b/docs/todos/PHASE-phased-event-dispatch/todo.md index ca622b42..d6429a38 100644 --- a/docs/todos/PHASE-phased-event-dispatch/todo.md +++ b/docs/todos/PHASE-phased-event-dispatch/todo.md @@ -82,6 +82,32 @@ exposes drain points. ## Discovery Log +### 2026-08-14 — PHASE-003 (plan review) +- **Finding:** Plan review returned CONCERNS — 6 veto findings. The sharpest: the + "structural" rollback-discard story is false for long-lived scopes (a failed call's + queues would drain into the next successful call in the same scope — Logical mode, + Blazor Server, the integration harness); the interface renderer is not the class + renderer's shape (inline guard, no `*Core` split, trimming UNVERIFIED — TRIM item 20); + and the drain-vs-cancellation-check ordering inside the choke point could silently + recreate the B-C5 failure mode. +- **Decision:** Amend (draft edited before implementation; all vetoes addressed — + failure now *clears* explicitly at outermost exit, never drains; entry stays active + through the drain; pre-declared pin-amendment set widened to six named tests). +- **Follow-up:** [reviews/003-plan-review.md](./reviews/003-plan-review.md) — includes + the full disposition table. + +### 2026-08-14 — Client-raise relay gap (pre-existing, deferred) +- **Finding:** Review A-V1: `MakeRemoteDelegateRequest.ForDelegateEvent` discards the + server response, so events raised during a client-initiated `Raise` — by handlers of + any phase, including Immediate, today — are collected server-side but never relayed + back. Naively wiring relay in would echo the client's own event back to its own + relay, against the "one `[Remote]` call = exactly one `Relay` invocation" contract. +- **Decision:** Defer — pre-existing gap, not phase-related; PHASE-003's remote-raise + acceptance claims the drain only. Needs a user decision on whether it warrants a plan + row (echo-to-self semantics are a real design question) or is working-as-intended. +- **Follow-up:** revisit at PHASE-004 (which owns consumer-facing drain/relay surface) + or at todo close. + ### 2026-08-14 — Ordering: PHASE-003 worked ahead of PHASE-002 - **Finding:** PHASE-002 (generator threads the attribute's phase argument to registration) and PHASE-003 (entry-call tracking + AfterCommit drain) are independent: From 9d6716a802ddf2a95e569ebc1c990aac1fc27ddc Mon Sep 17 00:00:00 2001 From: Keith Voels Date: Fri, 14 Aug 2026 20:55:43 -0500 Subject: [PATCH 05/10] feat: entry-call tracking and AfterCommit drain at factory completion (PHASE-003) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Runtime: IFactoryEventPhaseScheduler gains depth-aware BeginEntryCall/ EndEntryCallAsync — drain (AfterCommit sweep, no token) at outermost success, explicit clear on failure; scheduler state now lock-guarded. Dispatcher queues phased handlers only while an entry call is active (9005 outside-entry log, 9006 failure-clear log). HandleRemoteDelegateRequest wraps delegate invocation as the remote entry — drain before the post-invoke cancellation check and relay collection. Generator: every Local* method splits into a non-async guarded wrapper routing through FactoryEntryCall (new) to a private Core — class, interface (split introduced; TRIM item 20 note updated), and static (DI lambda) legs. Sync non-Task factories block-drain only when pending (no-silent-loss). Tests: six pre-declared PHASE-001 pin amendments (intent restated under entry semantics; re-entrancy test re-pointed to discriminate on drain-join order), two TRIM-009 emission-shape pins amended to the new wrapper shape, new FactoryEntryCallTests + outside-entry/failure-clear dispatcher tests. 662x2. Co-Authored-By: Claude Fable 5 --- .../plans/003-aftercommit-entry-call-drain.md | 2 +- .../todos/PHASE-phased-event-dispatch/todo.md | 2 +- .../Renderer/ClassFactoryRenderer.cs | 62 +++--- .../Renderer/InterfaceFactoryRenderer.cs | 28 ++- .../Renderer/StaticFactoryRenderer.cs | 10 +- src/RemoteFactory/FactoryEventsDispatcher.cs | 23 +- .../HandleRemoteDelegateRequest.cs | 75 +++++-- .../Internal/FactoryEntryCall.cs | 112 ++++++++++ .../Internal/FactoryEventPhaseScheduler.cs | 190 ++++++++++++++-- src/RemoteFactory/Internal/Log.cs | 17 ++ .../AssemblyAttributeEmissionTests.cs | 30 ++- .../Internal/FactoryEntryCallTests.cs | 203 ++++++++++++++++++ .../FactoryEventsDispatcherPhaseTests.cs | 105 ++++++++- 13 files changed, 748 insertions(+), 111 deletions(-) create mode 100644 src/RemoteFactory/Internal/FactoryEntryCall.cs create mode 100644 src/Tests/RemoteFactory.UnitTests/Internal/FactoryEntryCallTests.cs diff --git a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md index b4fba43e..60a00d9c 100644 --- a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md +++ b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md @@ -3,7 +3,7 @@ **Plan #:** 003 **Date:** 2026-08-14 **Related Todo:** [../todo.md](../todo.md) -**Status:** Draft +**Status:** In Progress **Last Updated:** 2026-08-14 **Plan-review opt-in:** Yes (touches all three factory renderers; entry-shape subtleties found at recon make this the riskiest plan) **Code-review opt-in:** Yes (behavior-changing across generated code and runtime) diff --git a/docs/todos/PHASE-phased-event-dispatch/todo.md b/docs/todos/PHASE-phased-event-dispatch/todo.md index d6429a38..e81b66e5 100644 --- a/docs/todos/PHASE-phased-event-dispatch/todo.md +++ b/docs/todos/PHASE-phased-event-dispatch/todo.md @@ -72,7 +72,7 @@ exposes drain points. |---|------|-------|--------| | 001 | [001-phase-model-and-queueing](./plans/001-phase-model-and-queueing.md) | DispatchPhase enum, registry phase, dispatcher queueing | Done | | 002 | [002-generator-phase-passthrough](./plans/002-generator-phase-passthrough.md) | Generator reads phase from attribute, threads to registration | Draft | -| 003 | [003-aftercommit-entry-call-drain](./plans/003-aftercommit-entry-call-drain.md) | Entry-call tracking in generated factories; AfterCommit drain | Draft | +| 003 | [003-aftercommit-entry-call-drain](./plans/003-aftercommit-entry-call-drain.md) | Entry-call tracking in generated factories; AfterCommit drain | In Progress | | 004 | [004-afterflush-coordinator](./plans/004-afterflush-coordinator.md) | IFactoryEventPhaseCoordinator public API + fallback drain | Draft | | 005 | [005-design-docs-skill](./plans/005-design-docs-skill.md) | Design projects, published docs, skill reference | Draft | | 006 | [006-coalescing](./plans/006-coalescing.md) | Opt-in same-event coalescing (v2, queued per user) | Draft | diff --git a/src/Generator/Renderer/ClassFactoryRenderer.cs b/src/Generator/Renderer/ClassFactoryRenderer.cs index a2e34fb1..adae25f0 100644 --- a/src/Generator/Renderer/ClassFactoryRenderer.cs +++ b/src/Generator/Renderer/ClassFactoryRenderer.cs @@ -344,19 +344,26 @@ private static void RenderRemoteMethod(StringBuilder sb, FactoryMethodModel meth } /// - /// Emits the opening of a Local* factory method — signature, brace, and the - /// guard when the method is server-only. + /// Emits a Local* factory method as a NON-async wrapper — signature, the + /// guard when the method is server-only, + /// and an entry-call forward to a private Local*Core whose opening is emitted + /// last; the caller's body lands in the core. /// /// - /// For guarded async methods the guard is emitted in a NON-async wrapper that - /// forwards to a private async core, and the caller's body lands in the core. + /// The wrapper routes through FactoryEntryCall (PHASE-003): entry-call + /// tracking is depth-aware, drains AfterCommit at the outermost successful + /// completion, and discards deferred work on failure. Nested calls — LocalSave + /// into LocalInsert, the remote request handler around any Local* — + /// only increment depth. The helper resolves the scheduler null-tolerantly, so + /// client-reachable unguarded methods (public non-[Remote]) reduce to the body. /// - /// The guard must sit outside the async state machine. When it is inside, the compiler - /// lowers the whole body — guard included — into MoveNext, inside the builder's - /// own protected region. ILLink folds the feature switch there but does not eliminate - /// the unreachable remainder, so [Remote] bodies, their [Service] - /// interfaces, and their string literals ship to publish-trimmed clients. A sync method - /// puts the guard ahead of any protected region, so the whole remainder goes. + /// The guard must sit outside the async state machine, which is why the wrapper is + /// never async. When the guard is inside, the compiler lowers the whole body — + /// guard included — into MoveNext, inside the builder's own protected region. + /// ILLink folds the feature switch there but does not eliminate the unreachable + /// remainder, so [Remote] bodies, their [Service] interfaces, and their + /// string literals ship to publish-trimmed clients. A sync method puts the guard + /// ahead of any protected region, so the whole remainder goes. /// /// /// Measured, not assumed (TRIM-009): stripping the async lifecycle probes and the @@ -375,25 +382,12 @@ private static void RenderLocalMethodOpening( string parameters, string forwardArgs, bool needsAsync, - bool isServerOnly, - bool blankLineAfterGuard = true) + bool isServerOnly) { - if (needsAsync && isServerOnly) - { - sb.AppendLine($" {modifiers} {returnType} Local{uniqueName}({parameters})"); - sb.AppendLine(" {"); - sb.AppendLine(" if (!NeatooRuntime.IsServerRuntime)"); - sb.AppendLine(" throw new InvalidOperationException(\"Server-only method called in non-server runtime.\");"); - sb.AppendLine($" return Local{uniqueName}Core({forwardArgs});"); - sb.AppendLine(" }"); - sb.AppendLine(); - sb.AppendLine($" private async {returnType} Local{uniqueName}Core({parameters})"); - sb.AppendLine(" {"); - return; - } + var isTaskReturn = returnType == "Task" || returnType.StartsWith("Task<", StringComparison.Ordinal); + var entryRun = isTaskReturn ? "RunAsync" : "Run"; - var asyncKeyword = needsAsync ? "async " : ""; - sb.AppendLine($" {modifiers} {asyncKeyword}{returnType} Local{uniqueName}({parameters})"); + sb.AppendLine($" {modifiers} {returnType} Local{uniqueName}({parameters})"); sb.AppendLine(" {"); // Feature switch guard -- only emit for internal or [Remote] methods. @@ -402,11 +396,14 @@ private static void RenderLocalMethodOpening( { sb.AppendLine(" if (!NeatooRuntime.IsServerRuntime)"); sb.AppendLine(" throw new InvalidOperationException(\"Server-only method called in non-server runtime.\");"); - if (blankLineAfterGuard) - { - sb.AppendLine(); - } } + + sb.AppendLine($" return FactoryEntryCall.{entryRun}(ServiceProvider, () => Local{uniqueName}Core({forwardArgs}));"); + sb.AppendLine(" }"); + sb.AppendLine(); + var asyncKeyword = needsAsync ? "async " : ""; + sb.AppendLine($" private {asyncKeyword}{returnType} Local{uniqueName}Core({parameters})"); + sb.AppendLine(" {"); } private static void RenderReadLocalMethod(StringBuilder sb, ReadMethodModel method, ClassFactoryModel model) @@ -1115,8 +1112,7 @@ private static void RenderSaveLocalMethod(StringBuilder sb, SaveMethodModel meth var paramIdentifiers = GetParameterIdentifiersWithCancellationToken(method.Parameters, includeServices: false); RenderLocalMethodOpening(sb, "public virtual", returnType, method.UniqueName, parameters, paramIdentifiers, - method.IsAsync, method.IsInternal || method.IsRemote, blankLineAfterGuard: false); - sb.AppendLine(); + method.IsAsync, method.IsInternal || method.IsRemote); // Default return value var defaultReturn = method.HasAuth diff --git a/src/Generator/Renderer/InterfaceFactoryRenderer.cs b/src/Generator/Renderer/InterfaceFactoryRenderer.cs index bafc9716..0d4c54c3 100644 --- a/src/Generator/Renderer/InterfaceFactoryRenderer.cs +++ b/src/Generator/Renderer/InterfaceFactoryRenderer.cs @@ -249,13 +249,14 @@ private static void RenderRemoteMethod(StringBuilder sb, InterfaceMethodModel me private static void RenderLocalMethod(StringBuilder sb, InterfaceMethodModel method, InterfaceFactoryModel model) { - var asyncKeyword = method.IsAsync ? "async" : ""; + var asyncKeyword = method.IsAsync ? "async " : ""; var returnType = GetReturnType(method); var parameters = GetParameterDeclarations(method.Parameters); + var forwardArgs = GetParameterIdentifiers(method.Parameters); var paramIdentifiers = GetParameterIdentifiers(method.Parameters, includeServices: false); - sb.AppendLine($" public {asyncKeyword} {returnType} Local{method.UniqueName}({parameters})"); - sb.AppendLine(" {"); + var isTaskReturn = returnType == "Task" || returnType.StartsWith("Task<", StringComparison.Ordinal); + var entryRun = isTaskReturn ? "RunAsync" : "Run"; // Feature switch guard -- when IsServerRuntime=false the switch folds to a constant and // the body becomes unreachable. @@ -269,16 +270,23 @@ private static void RenderLocalMethod(StringBuilder sb, InterfaceMethodModel met // holder, because [DynamicallyAccessedMembers] covers NonPublicMethods and roots the // private core on its own. // - // THIS LEG HAS RECEIVED NEITHER FIX. It still emits the guard inline (no wrapper split) - // and line 48 still points the assembly attribute at {ImplName}Factory, which hosts every - // Local* method. No leak has been observed here, but the leg reaches its implementation - // through interfaces, so a client-side trimmed harness reads "absent" either way and - // cannot prove elimination -- and Deferred Work item 19 blocks the fixture change that - // would give it a reachable marker. Treat body elimination on this leg as UNVERIFIED. - // Tracked as Deferred Work item 20 on the TRIM todo. + // PHASE-003 introduced the wrapper split on this leg: the guard now sits in a NON-async + // wrapper that forwards through FactoryEntryCall (entry-call tracking + AfterCommit + // drain), and the body lands in a private Local*Core. That aligns the guard topology + // with the class leg, but the second TRIM-009 fix -- a single-method registrar holder -- + // is still absent here (the assembly attribute still points at {ImplName}Factory, which + // hosts every Local* method), and Deferred Work item 19 still blocks the fixture change + // that would give this leg a reachable marker. Treat body elimination on this leg as + // UNVERIFIED. Tracked as Deferred Work item 20 on the TRIM todo. + sb.AppendLine($" public {returnType} Local{method.UniqueName}({parameters})"); + sb.AppendLine(" {"); sb.AppendLine(" if (!NeatooRuntime.IsServerRuntime)"); sb.AppendLine(" throw new InvalidOperationException(\"Server-only method called in non-server runtime.\");"); + sb.AppendLine($" return FactoryEntryCall.{entryRun}(ServiceProvider, () => Local{method.UniqueName}Core({forwardArgs}));"); + sb.AppendLine(" }"); sb.AppendLine(); + sb.AppendLine($" private {asyncKeyword}{returnType} Local{method.UniqueName}Core({parameters})"); + sb.AppendLine(" {"); // CanXxx methods only perform authorization checks and return the result if (method.Name.StartsWith("Can")) diff --git a/src/Generator/Renderer/StaticFactoryRenderer.cs b/src/Generator/Renderer/StaticFactoryRenderer.cs index 4be6cc19..37b8e957 100644 --- a/src/Generator/Renderer/StaticFactoryRenderer.cs +++ b/src/Generator/Renderer/StaticFactoryRenderer.cs @@ -227,11 +227,17 @@ private static void RenderLocalDelegateRegistration(StringBuilder sb, ExecuteDel var allParamIdentifiers = BuildDomainMethodInvocationParams(del); // Feature switch guard -- when IsServerRuntime=false, the trimmer removes the entire registration + // The delegate body routes through FactoryEntryCall (PHASE-003): the delegate IS the + // static pattern's local execution seam — its entry marks the factory call, drains + // AfterCommit at the outermost successful completion, and discards deferred work on + // failure. Service resolution sits inside the entry, so a missing server-only service + // counts as entry failure (clear, no drain). The guard wraps the registration itself, + // so no async state machine ever hosts it. sb.AppendLine(" if (NeatooRuntime.IsServerRuntime)"); sb.AppendLine(" {"); sb.AppendLine($" services.AddTransient<{typeName}.{del.DelegateName}>(cc =>"); sb.AppendLine(" {"); - sb.AppendLine($" return ({paramDecl}) => {{"); + sb.AppendLine($" return ({paramDecl}) => FactoryEntryCall.RunAsync(cc, () => {{"); if (!string.IsNullOrEmpty(serviceAssignments)) { @@ -239,7 +245,7 @@ private static void RenderLocalDelegateRegistration(StringBuilder sb, ExecuteDel } sb.AppendLine($" return {typeName}.{del.Name}({allParamIdentifiers});"); - sb.AppendLine(" };"); + sb.AppendLine(" });"); sb.AppendLine(" });"); sb.AppendLine(" }"); } diff --git a/src/RemoteFactory/FactoryEventsDispatcher.cs b/src/RemoteFactory/FactoryEventsDispatcher.cs index 2532fcb2..0c511547 100644 --- a/src/RemoteFactory/FactoryEventsDispatcher.cs +++ b/src/RemoteFactory/FactoryEventsDispatcher.cs @@ -62,21 +62,32 @@ private async Task DispatchToHandlers(Type eventType, object factoryEvent, Raise // callers must not depend on it. Exceptions propagate immediately and abort // the remaining handlers so the caller's transaction can roll back. // - // Phased handlers are queued instead, and run when their phase drains. Without a - // queue in the scope they fall back to immediate dispatch rather than vanishing. + // Phased handlers are queued only while an entry factory call is active — that + // is the only time a drain point is coming. The entry stays active through the + // entry-call drain itself, so an event a drained handler raises still queues + // here and joins the current drain. Raised outside any factory call (or in a + // scope with no scheduler at all), phased handlers dispatch immediately rather + // than vanishing, each case with its own debug log. foreach (var (phase, handler) in handlers) { if (phase != DispatchPhase.Immediate) { - if (_phaseQueue != null) + if (_phaseQueue != null && _phaseQueue.IsEntryCallActive) { _phaseQueue.Enqueue(phase, (FactoryEventBase)factoryEvent, options, handler); continue; } - _sp.GetService()? - .CreateLogger(NeatooLoggerCategories.Server) - .FactoryEventPhaseNoQueueInScope(eventType.Name, phase); + var logger = _sp.GetService()? + .CreateLogger(NeatooLoggerCategories.Server); + if (_phaseQueue == null) + { + logger?.FactoryEventPhaseNoQueueInScope(eventType.Name, phase); + } + else + { + logger?.FactoryEventPhaseRaisedOutsideEntryCall(eventType.Name, phase); + } } await handler(_sp, factoryEvent, options, cancellationToken).ConfigureAwait(false); diff --git a/src/RemoteFactory/HandleRemoteDelegateRequest.cs b/src/RemoteFactory/HandleRemoteDelegateRequest.cs index 42acd314..70e5f07a 100644 --- a/src/RemoteFactory/HandleRemoteDelegateRequest.cs +++ b/src/RemoteFactory/HandleRemoteDelegateRequest.cs @@ -98,39 +98,68 @@ public static HandleRemoteDelegateRequest HandlePortalRequest(IServiceProvider s trace.TraceInvokingDelegate(correlationId); var invokeSw = Stopwatch.StartNew(); object? result; + + // Entry-call tracking: this delegate invocation is the entry factory call for + // every remote request — [Remote] factory methods and client-raised events + // alike. Generated Local* methods nest inside it (depth > 1), so the drain + // happens here, once, at the outermost completion. The drain sits BEFORE the + // post-invoke cancellation check and before relay collection: a token + // cancelled after the delegate succeeded must neither skip the drain nor + // abort it, and events raised by drained handlers must join this response's + // relay batch. Failure paths (throw, forbid, cancellation during the + // delegate) discard the deferred work — a clear, never a drain. + var phaseScheduler = serviceProvider.GetService(); + phaseScheduler?.BeginEntryCall(); try { - result = method.DynamicInvoke(invokeParams); - } - catch (TargetInvocationException tie) when (tie.InnerException != null) - { - // Unwrap the TargetInvocationException from DynamicInvoke - System.Runtime.ExceptionServices.ExceptionDispatchInfo.Capture(tie.InnerException).Throw(); - throw; // Unreachable, but required by compiler - } + try + { + result = method.DynamicInvoke(invokeParams); + } + catch (TargetInvocationException tie) when (tie.InnerException != null) + { + // Unwrap the TargetInvocationException from DynamicInvoke + System.Runtime.ExceptionServices.ExceptionDispatchInfo.Capture(tie.InnerException).Throw(); + throw; // Unreachable, but required by compiler + } - if (result is Task task) - { - trace.TraceAwaitingResult(correlationId); - await task; - invokeSw.Stop(); - trace.TraceDelegateCompleted(correlationId, invokeSw.ElapsedMilliseconds, true); - // Only get result for Task, not for non-generic Task (events, void methods) - var resultProperty = task.GetType().GetProperty(Result); - if (resultProperty != null && resultProperty.PropertyType != typeof(void) && - resultProperty.PropertyType.Name != "VoidTaskResult") + if (result is Task task) { - result = resultProperty.GetValue(task); + trace.TraceAwaitingResult(correlationId); + await task; + invokeSw.Stop(); + trace.TraceDelegateCompleted(correlationId, invokeSw.ElapsedMilliseconds, true); + // Only get result for Task, not for non-generic Task (events, void methods) + var resultProperty = task.GetType().GetProperty(Result); + if (resultProperty != null && resultProperty.PropertyType != typeof(void) && + resultProperty.PropertyType.Name != "VoidTaskResult") + { + result = resultProperty.GetValue(task); + } + else + { + result = null; + } } else { - result = null; + invokeSw.Stop(); + trace.TraceDelegateCompleted(correlationId, invokeSw.ElapsedMilliseconds, false); + } + + if (phaseScheduler != null) + { + await phaseScheduler.EndEntryCallAsync(success: true); } } - else + catch { - invokeSw.Stop(); - trace.TraceDelegateCompleted(correlationId, invokeSw.ElapsedMilliseconds, false); + if (phaseScheduler != null) + { + await phaseScheduler.EndEntryCallAsync(success: false); + } + + throw; } // Check for cancellation before serializing response diff --git a/src/RemoteFactory/Internal/FactoryEntryCall.cs b/src/RemoteFactory/Internal/FactoryEntryCall.cs new file mode 100644 index 00000000..215a8589 --- /dev/null +++ b/src/RemoteFactory/Internal/FactoryEntryCall.cs @@ -0,0 +1,112 @@ +using Microsoft.Extensions.DependencyInjection; + +namespace Neatoo.RemoteFactory.Internal; + +/// +/// Entry-call wrapper invoked by generated factory code around the local execution of a +/// factory method. Marks the call on the scope's +/// so phased event dispatches queue while the call is in flight, drains +/// at the outermost successful completion, and +/// discards deferred work when the call fails. +/// +/// +/// +/// Public because generated factory code calls it; it lives in the Internal +/// namespace alongside for the same reason. +/// +/// +/// Resolution is null-tolerant by design: Remote-mode (client) containers register no +/// scheduler, and some generated callers (value-object factories) run client-side with +/// no IsServerRuntime guard — there the wrapper reduces to invoking the body. +/// Keeping these methods non- at the generated call site also +/// preserves the IL-trimming wrapper shape: the caller's runtime guard stays in the +/// outer method rather than being lowered into an async state machine. +/// +/// +public static class FactoryEntryCall +{ + /// Runs a -returning factory body as an entry call. + public static async Task RunAsync(IServiceProvider serviceProvider, Func> body) + { + ArgumentNullException.ThrowIfNull(serviceProvider); + ArgumentNullException.ThrowIfNull(body); + + var scheduler = serviceProvider.GetService(); + if (scheduler == null) + { + return await body().ConfigureAwait(false); + } + + scheduler.BeginEntryCall(); + try + { + var result = await body().ConfigureAwait(false); + await scheduler.EndEntryCallAsync(success: true).ConfigureAwait(false); + return result; + } + catch + { + await scheduler.EndEntryCallAsync(success: false).ConfigureAwait(false); + throw; + } + } + + /// Runs a -returning factory body as an entry call. + public static async Task RunAsync(IServiceProvider serviceProvider, Func body) + { + ArgumentNullException.ThrowIfNull(serviceProvider); + ArgumentNullException.ThrowIfNull(body); + + var scheduler = serviceProvider.GetService(); + if (scheduler == null) + { + await body().ConfigureAwait(false); + return; + } + + scheduler.BeginEntryCall(); + try + { + await body().ConfigureAwait(false); + await scheduler.EndEntryCallAsync(success: true).ConfigureAwait(false); + } + catch + { + await scheduler.EndEntryCallAsync(success: false).ConfigureAwait(false); + throw; + } + } + + /// + /// Runs a synchronous factory body as an entry call. If the body deferred phased + /// work (a fire-and-forget Raise inside a synchronous factory method), the + /// completion drain blocks rather than losing that work — the no-silent-loss + /// invariant outranks staying non-blocking on this edge. With nothing deferred, the + /// completion is fully synchronous. + /// + public static T Run(IServiceProvider serviceProvider, Func body) + { + ArgumentNullException.ThrowIfNull(serviceProvider); + ArgumentNullException.ThrowIfNull(body); + + var scheduler = serviceProvider.GetService(); + if (scheduler == null) + { + return body(); + } + + scheduler.BeginEntryCall(); + try + { + var result = body(); + scheduler.EndEntryCallAsync(success: true).GetAwaiter().GetResult(); + return result; + } + catch + { + // Completes synchronously: the failure path clears without running handlers. + scheduler.EndEntryCallAsync(success: false).GetAwaiter().GetResult(); + throw; + } + } +} diff --git a/src/RemoteFactory/Internal/FactoryEventPhaseScheduler.cs b/src/RemoteFactory/Internal/FactoryEventPhaseScheduler.cs index a09b7527..46b120e3 100644 --- a/src/RemoteFactory/Internal/FactoryEventPhaseScheduler.cs +++ b/src/RemoteFactory/Internal/FactoryEventPhaseScheduler.cs @@ -4,19 +4,25 @@ namespace Neatoo.RemoteFactory.Internal; /// /// Scope-scoped store of factory-event dispatches deferred by their -/// , plus the drain primitive that runs them. +/// , plus the drain primitive that runs them and the +/// entry-call tracking that decides when the framework drains on its own. /// /// /// -/// Public because generated factory code calls at the entry-call +/// Public because generated factory code calls the entry-call members at the entry-call /// boundary; it lives in the Internal namespace alongside /// and , which /// are public for the same reason. /// /// -/// State is per DI scope and holds no persistence concepts. A scope whose factory -/// operation fails is simply never drained — that is what makes rollback-discard -/// structural rather than a rule anyone has to remember. +/// State is per DI scope and holds no persistence concepts. Entry-call tracking is +/// depth-aware: nested factory work (a save cascading into an insert, one factory +/// invoking another, the remote request handler wrapping a local method) increments +/// depth rather than starting a second entry, and only the outermost completion is +/// "the entry call completing." A failed entry call clears its deferred work at +/// the outermost exit — never drains it — so between entry calls the scheduler is +/// always empty. Scopes can be long-lived (Blazor Server circuits, Logical mode); the +/// clear is what keeps a failed call's work from riding into the next call's drain. /// /// public interface IFactoryEventPhaseScheduler @@ -24,6 +30,30 @@ public interface IFactoryEventPhaseScheduler /// True when any phase has deferred dispatches waiting. bool HasPending { get; } + /// + /// True while an entry factory call is in flight in this scope — including for the + /// duration of the entry-call drain itself, so work a drained handler raises still + /// queues and joins the current drain. + /// + bool IsEntryCallActive { get; } + + /// Marks the start of a factory call. Nested calls increment depth. + void BeginEntryCall(); + + /// + /// Marks the end of a factory call. At the outermost exit: a successful entry drains + /// (which sweeps earlier phases first) with + /// no cancellation token — the entry call already succeeded, so nothing may abort its + /// post-completion work — while a failed entry discards all deferred dispatches + /// without running any. Nested exits only decrement depth. + /// + /// + /// Whether the factory call completed successfully. Callers pass + /// from failure paths only; this method never throws when + /// is . + /// + Task EndEntryCallAsync(bool success); + /// Defers a handler dispatch until drains. void Enqueue(DispatchPhase phase, FactoryEventBase factoryEvent, RaiseOptions options, Func handler); @@ -57,6 +87,12 @@ internal sealed class FactoryEventPhaseScheduler : IFactoryEventPhaseScheduler private readonly ILogger? _logger; private readonly Dictionary> _deferred = new(); + // Guards _deferred and _entryDepth. Scopes can be shared by concurrent flows + // (Blazor Server circuits, Logical mode, a reused test scope); handlers are + // invoked outside the lock. + private readonly object _gate = new(); + private int _entryDepth; + public FactoryEventPhaseScheduler(IServiceProvider sp, ILoggerFactory? loggerFactory = null) { _sp = sp; @@ -68,20 +104,98 @@ private readonly record struct QueuedDispatch( RaiseOptions Options, Func Handler); - public bool HasPending => _deferred.Any(q => q.Value.Count > 0); + public bool HasPending + { + get + { + lock (_gate) + { + return _deferred.Any(q => q.Value.Count > 0); + } + } + } + + public bool IsEntryCallActive + { + get + { + lock (_gate) + { + return _entryDepth > 0; + } + } + } + + public void BeginEntryCall() + { + lock (_gate) + { + _entryDepth++; + } + } + + public async Task EndEntryCallAsync(bool success) + { + if (!success) + { + ClearAtExit(); + return; + } + + bool outermost; + lock (_gate) + { + if (_entryDepth == 0) + { + throw new InvalidOperationException( + $"{nameof(EndEntryCallAsync)} called without a matching {nameof(BeginEntryCall)}."); + } + + outermost = _entryDepth == 1; + } + + if (!outermost) + { + lock (_gate) + { + _entryDepth--; + } + + return; + } + + // The entry stays active (depth 1) for the duration of the drain, so an event a + // drained handler raises still queues through the dispatcher and joins this drain + // via drain-until-empty. No token: the entry call already succeeded, so nothing + // may abort its post-completion work. + try + { + await DrainAsync(DispatchPhase.AfterCommit, inTransaction: false, CancellationToken.None).ConfigureAwait(false); + } + finally + { + // Depth release and a discard of anything a thrown drain (handler OCE) left + // behind — a clear, never a drain, preserving "between entry calls the + // scheduler is empty." + ClearAtExit(); + } + } public void Enqueue(DispatchPhase phase, FactoryEventBase factoryEvent, RaiseOptions options, Func handler) { ArgumentNullException.ThrowIfNull(factoryEvent); ArgumentNullException.ThrowIfNull(handler); - if (!_deferred.TryGetValue(phase, out var queue)) + lock (_gate) { - queue = new Queue(); - _deferred[phase] = queue; - } + if (!_deferred.TryGetValue(phase, out var queue)) + { + queue = new Queue(); + _deferred[phase] = queue; + } - queue.Enqueue(new QueuedDispatch(factoryEvent, options, handler)); + queue.Enqueue(new QueuedDispatch(factoryEvent, options, handler)); + } if (_logger?.IsEnabled(LogLevel.Debug) == true) { @@ -131,6 +245,37 @@ public async Task DrainAsync(DispatchPhase phase, bool inTransaction, Cancellati } } + /// + /// Outermost-exit cleanup shared by the failure path and the post-drain release: + /// decrements depth (tolerantly — failure paths run inside catch blocks and must + /// never throw) and, at depth zero, discards whatever is still deferred. + /// + private void ClearAtExit() + { + int discarded = 0; + lock (_gate) + { + if (_entryDepth > 0) + { + _entryDepth--; + } + + if (_entryDepth == 0) + { + foreach (var queue in _deferred.Values) + { + discarded += queue.Count; + queue.Clear(); + } + } + } + + if (discarded > 0) + { + _logger?.FactoryEventPhaseClearedOnFailure(discarded); + } + } + /// /// Takes the next dispatch from the earliest non-empty phase at or before /// , so cross-phase ordering holds even for work a handler @@ -138,19 +283,22 @@ public async Task DrainAsync(DispatchPhase phase, bool inTransaction, Cancellati /// private bool TryDequeueThrough(DispatchPhase through, out QueuedDispatch dispatch, out DispatchPhase phase) { - foreach (var candidate in _deferred.Keys.Where(p => p <= through).OrderBy(p => p)) + lock (_gate) { - var queue = _deferred[candidate]; - if (queue.Count > 0) + foreach (var candidate in _deferred.Keys.Where(p => p <= through).OrderBy(p => p)) { - dispatch = queue.Dequeue(); - phase = candidate; - return true; + var queue = _deferred[candidate]; + if (queue.Count > 0) + { + dispatch = queue.Dequeue(); + phase = candidate; + return true; + } } - } - dispatch = default; - phase = through; - return false; + dispatch = default; + phase = through; + return false; + } } } diff --git a/src/RemoteFactory/Internal/Log.cs b/src/RemoteFactory/Internal/Log.cs index 662d6160..a41f8951 100644 --- a/src/RemoteFactory/Internal/Log.cs +++ b/src/RemoteFactory/Internal/Log.cs @@ -509,4 +509,21 @@ public static partial void FactoryEventPhaseNoQueueInScope( this ILogger logger, string eventType, DispatchPhase phase); + + [LoggerMessage( + EventId = 9005, + Level = LogLevel.Debug, + Message = "Factory event {EventType} has a {Phase} handler but was raised outside any factory call; dispatching it immediately instead.")] + public static partial void FactoryEventPhaseRaisedOutsideEntryCall( + this ILogger logger, + string eventType, + DispatchPhase phase); + + [LoggerMessage( + EventId = 9006, + Level = LogLevel.Debug, + Message = "Discarded {DiscardedCount} deferred handler dispatch(es) because the entry factory call did not complete successfully.")] + public static partial void FactoryEventPhaseClearedOnFailure( + this ILogger logger, + int discardedCount); } diff --git a/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs b/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs index df038195..2f670687 100644 --- a/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs +++ b/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs @@ -157,11 +157,13 @@ public partial class MyEntity Assert.NotNull(generatedSource); - // Wrapper: NOT async, carries the guard, forwards to the core. + // Wrapper: NOT async, carries the guard, forwards to the core through the + // entry-call helper (PHASE-003). The trimming-relevant property is unchanged: + // the guard sits in a non-async method, never inside a state machine. Assert.Matches( @"public Task LocalFetchIt\(string name, CancellationToken cancellationToken = default\)\s*\{\s*if \(!NeatooRuntime\.IsServerRuntime\)", generatedSource); - Assert.Contains("return LocalFetchItCore(name, cancellationToken);", generatedSource); + Assert.Contains("return FactoryEntryCall.RunAsync(ServiceProvider, () => LocalFetchItCore(name, cancellationToken));", generatedSource); // Core: async, private, and carries NO guard — the guard already ran in the wrapper. Assert.Contains("private async Task LocalFetchItCore(", generatedSource); @@ -189,15 +191,19 @@ public partial class MyEntity // emission assertion, which needs an ASP-auth fixture in this harness. /// - /// A SYNCHRONOUS guarded Local* method keeps the guard inline and is not split. + /// A SYNCHRONOUS guarded Local* method splits into a non-async guarded wrapper + /// and a NON-async private core. /// /// - /// The sync shape already trims correctly — unreachability begins before any protected - /// region, so the whole remainder goes. Splitting it would be churn. This test is the - /// control that keeps the wrapper narrowly scoped to the shape that needed it. + /// Amended by PHASE-003 (was ClassFactory_GuardedSyncLocalMethod_IsNotSplit): + /// every Local* method now splits so the wrapper can route through the + /// entry-call helper. The trimming intent this test pins is unchanged from TRIM-009 — + /// the guard must sit ahead of any protected region — and the sync shape still + /// satisfies it: the wrapper is not async, and the core keeps the body's original + /// synchronous form (no async keyword, no state machine hosting the guard). /// [Fact] - public void ClassFactory_GuardedSyncLocalMethod_IsNotSplit() + public void ClassFactory_GuardedSyncLocalMethod_SplitsIntoSyncWrapperAndSyncCore() { var source = @" using Neatoo.RemoteFactory; @@ -221,10 +227,18 @@ internal void Create(string name) { } ?.ToString(); Assert.NotNull(generatedSource); - Assert.DoesNotContain("LocalCreateCore", generatedSource); + + // Wrapper: NOT async, carries the guard, forwards through the entry-call helper. Assert.Matches( @"public Task LocalCreate\(string name, CancellationToken cancellationToken = default\)\s*\{\s*if \(!NeatooRuntime\.IsServerRuntime\)", generatedSource); + Assert.Contains("return FactoryEntryCall.RunAsync(ServiceProvider, () => LocalCreateCore(name, cancellationToken));", generatedSource); + + // Core: private, NOT async (the sync body keeps its shape), and carries no guard. + Assert.Matches( + @"private Task LocalCreateCore\([^)]*\)\s*\{\s*(?!\s*if \(!NeatooRuntime\.IsServerRuntime\))", + generatedSource); + Assert.DoesNotContain("private async Task LocalCreateCore(", generatedSource); } /// diff --git a/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEntryCallTests.cs b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEntryCallTests.cs new file mode 100644 index 00000000..12583f9b --- /dev/null +++ b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEntryCallTests.cs @@ -0,0 +1,203 @@ +using Microsoft.Extensions.DependencyInjection; +using Neatoo.RemoteFactory; +using Neatoo.RemoteFactory.Internal; + +namespace RemoteFactory.UnitTests.Internal; + +/// +/// Covers the entry-call wrapper generated factory code routes through (PHASE-003): +/// depth-aware begin/end, drain at the outermost successful completion only, clear on +/// failure, the no-token drain policy, and null-tolerance when no scheduler exists. +/// +public class FactoryEntryCallTests +{ + // One event type per test: FactoryEventHandlerRegistry is process-global with + // (eventType, handlerClass) first-registration-wins dedupe, so sharing an event + // type across tests silently drops the later registration (PHASE-007 tech debt). + private sealed record EntryEvent(string Value) : FactoryEventBase; + private sealed record FailingEntryEvent(string Value) : FactoryEventBase; + private sealed record NestedEntryEvent(string Value) : FactoryEventBase; + private sealed record SyncEntryEvent(string Value) : FactoryEventBase; + private sealed record TokenCaptureEvent(string Value) : FactoryEventBase; + + private sealed class DeferredHandler { } + + private static (ServiceProvider Provider, IServiceScope Scope) ServerScope() + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddNeatooRemoteFactory(NeatooFactory.Server, typeof(FactoryEntryCallTests).Assembly); + var provider = services.BuildServiceProvider(); + return (provider, provider.CreateScope()); + } + + [Fact] + public async Task RunAsync_DeferredWorkDrainsAtCompletion_AndResultFlowsThrough() + { + var dispatched = new List(); + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, + (_, _, _, _) => { dispatched.Add("deferred"); return Task.CompletedTask; }); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var sp = scope.ServiceProvider; + var events = sp.GetRequiredService(); + + var result = await FactoryEntryCall.RunAsync(sp, async () => + { + await events.Raise(new EntryEvent("x")); + Assert.Empty(dispatched); + return 42; + }); + + Assert.Equal(42, result); + Assert.Equal(["deferred"], dispatched); + } + } + + [Fact] + public async Task RunAsync_NestedEntry_DoesNotDrainAtTheInnerCompletion() + { + var dispatched = new List(); + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, + (_, _, _, _) => { dispatched.Add("deferred"); return Task.CompletedTask; }); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var sp = scope.ServiceProvider; + var events = sp.GetRequiredService(); + + await FactoryEntryCall.RunAsync(sp, async () => + { + await FactoryEntryCall.RunAsync(sp, async () => + { + await events.Raise(new NestedEntryEvent("x")); + return 0; + }); + + // The inner entry completed successfully — but it is nested, so the + // deferred work must still be waiting for the OUTERMOST completion. + Assert.Empty(dispatched); + return 0; + }); + + Assert.Equal(["deferred"], dispatched); + } + } + + [Fact] + public async Task RunAsync_BodyThrows_DeferredWorkIsClearedNotRun() + { + var dispatched = new List(); + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, + (_, _, _, _) => { dispatched.Add("deferred"); return Task.CompletedTask; }); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var sp = scope.ServiceProvider; + var events = sp.GetRequiredService(); + var scheduler = sp.GetRequiredService(); + + await Assert.ThrowsAsync(() => + FactoryEntryCall.RunAsync(sp, async () => + { + await events.Raise(new FailingEntryEvent("x")); + throw new InvalidOperationException("entry failed"); + })); + + Assert.Empty(dispatched); + Assert.False(scheduler.HasPending); + } + } + + [Fact] + public void Run_SyncEntryWithDeferredWork_DoesNotLoseIt() + { + // A synchronous (non-Task) factory method can still enqueue phased work via a + // fire-and-forget Raise. The sync wrapper block-drains at completion rather than + // silently dropping it — the no-silent-loss invariant. + var dispatched = new List(); + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, + (_, _, _, _) => { dispatched.Add("deferred"); return Task.CompletedTask; }); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var sp = scope.ServiceProvider; + var events = sp.GetRequiredService(); + + var result = FactoryEntryCall.Run(sp, () => + { + events.Raise(new SyncEntryEvent("x")).GetAwaiter().GetResult(); + return "done"; + }); + + Assert.Equal("done", result); + Assert.Equal(["deferred"], dispatched); + } + } + + [Fact] + public async Task RunAsync_EntryDrainPassesNoCancellationToken() + { + // B-C5 policy: the entry call already succeeded, so nothing may abort its + // post-completion work — drained handlers receive CancellationToken.None even + // when the factory call itself carried a live token. + CancellationToken? received = null; + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, + (_, _, _, ct) => { received = ct; return Task.CompletedTask; }); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var sp = scope.ServiceProvider; + var events = sp.GetRequiredService(); + using var cts = new CancellationTokenSource(); + + await FactoryEntryCall.RunAsync(sp, async () => + { + await events.Raise(new TokenCaptureEvent("x"), RaiseOptions.None, cts.Token); + return 0; + }); + + Assert.Equal(CancellationToken.None, received); + } + } + + [Fact] + public async Task RunAsync_NoSchedulerInScope_JustRunsTheBody() + { + // Remote-mode (client) containers register no scheduler; client-reachable + // generated wrappers must reduce to the body. + var services = new ServiceCollection(); + using var provider = services.BuildServiceProvider(); + + var result = await FactoryEntryCall.RunAsync(provider, () => Task.FromResult(7)); + + Assert.Equal(7, result); + } + + [Fact] + public async Task EndEntryCall_WithoutBegin_ThrowsOnTheSuccessPath() + { + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var scheduler = scope.ServiceProvider.GetRequiredService(); + + await Assert.ThrowsAsync(() => scheduler.EndEntryCallAsync(success: true)); + + // The failure path runs inside catch blocks and must never throw. + await scheduler.EndEntryCallAsync(success: false); + } + } +} diff --git a/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs index 88a01fab..f84374d1 100644 --- a/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs +++ b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs @@ -6,8 +6,17 @@ namespace RemoteFactory.UnitTests.Internal; /// /// Covers what does with phase-registered handlers: -/// Immediate dispatches as it always has, other phases defer to the scope's queue. +/// Immediate dispatches as it always has; other phases defer to the scope's scheduler +/// while an entry factory call is active (PHASE-003) and dispatch immediately otherwise. /// +/// +/// PHASE-003 amended the deferral tests here to raise inside an active entry call +/// (): PHASE-001's interim +/// behavior — queue whenever a scheduler exists — was chartered to be inverted by the +/// entry-call work, and each test's original intent (defer at raise time, dispatch at +/// the drain, cross-phase ordering, RaiseUntyped parity) is restated under entry +/// semantics rather than removed. +/// public class FactoryEventsDispatcherPhaseTests { private sealed record ImmediateOnlyEvent(string Value) : FactoryEventBase; @@ -18,6 +27,9 @@ private sealed record UntypedRaiseEvent(string Value) : FactoryEventBase; private sealed record ChainedSourceEvent(string Value) : FactoryEventBase; private sealed record ChainedFollowUpEvent(string Value) : FactoryEventBase; private sealed record NoQueueEvent(string Value) : FactoryEventBase; + private sealed record OutsideEntryEvent(string Value) : FactoryEventBase; + private sealed record FailedEntryEvent(string Value) : FactoryEventBase; + private sealed record SecondEntryEvent(string Value) : FactoryEventBase; private sealed class ImmediateHandler { } private sealed class DeferredHandler { } @@ -77,6 +89,7 @@ public async Task Raise_DeferredHandler_DoesNotDispatchAtRaiseTime() var events = scope.ServiceProvider.GetRequiredService(); var queue = scope.ServiceProvider.GetRequiredService(); + queue.BeginEntryCall(); await events.Raise(new DeferredOnlyEvent("x")); lock (Dispatched) @@ -85,7 +98,7 @@ public async Task Raise_DeferredHandler_DoesNotDispatchAtRaiseTime() } Assert.True(queue.HasPending); - await queue.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + await queue.EndEntryCallAsync(success: true); lock (Dispatched) { @@ -108,6 +121,7 @@ public async Task Raise_MixedPhases_ImmediateRunsAndDeferredWaits() var events = scope.ServiceProvider.GetRequiredService(); var queue = scope.ServiceProvider.GetRequiredService(); + queue.BeginEntryCall(); await events.Raise(new MixedPhaseEvent("x")); lock (Dispatched) @@ -115,7 +129,7 @@ public async Task Raise_MixedPhases_ImmediateRunsAndDeferredWaits() Assert.Equal(["immediate"], Dispatched); } - await queue.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + await queue.EndEntryCallAsync(success: true); // Cross-phase ordering: the Immediate handler completed before the deferred one ran. lock (Dispatched) @@ -140,6 +154,7 @@ public async Task RaiseUntyped_DeferredHandler_DefersJustLikeRaise() var events = scope.ServiceProvider.GetRequiredService(); var queue = scope.ServiceProvider.GetRequiredService(); + queue.BeginEntryCall(); await events.RaiseUntyped(new UntypedRaiseEvent("x")); lock (Dispatched) @@ -148,7 +163,7 @@ public async Task RaiseUntyped_DeferredHandler_DefersJustLikeRaise() } Assert.True(queue.HasPending); - await queue.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + await queue.EndEntryCallAsync(success: true); lock (Dispatched) { @@ -162,6 +177,13 @@ public async Task DrainedHandlerRaisingAnEvent_GoesThroughTheRealRaisePath() { // Re-entrancy as production hits it: handler -> IFactoryEvents.Raise -> registry // lookup -> defer, rather than calling Enqueue directly. + // + // PHASE-003 re-pointed this test to discriminate on WHERE the follow-up runs. + // The entry stays active for the duration of the entry drain, so the follow-up + // raised by the draining source handler must QUEUE and run after the source + // handler completes ("source-after-raise" before "follow-up"). If entry depth + // popped before the drain, the follow-up would dispatch inline inside the + // source handler's Raise call and the order would invert. lock (Dispatched) { Dispatched.Clear(); } var (provider, scope) = ServerScope(); @@ -179,19 +201,90 @@ public async Task DrainedHandlerRaisingAnEvent_GoesThroughTheRealRaisePath() Dispatched.Add("source"); } await sp.GetRequiredService().Raise(new ChainedFollowUpEvent("chained"), RaiseOptions.None, ct); + lock (Dispatched) + { + Dispatched.Add("source-after-raise"); + } }); + queue.BeginEntryCall(); await events.Raise(new ChainedSourceEvent("x")); - await queue.DrainAsync(DispatchPhase.AfterCommit, inTransaction: false); + await queue.EndEntryCallAsync(success: true); lock (Dispatched) { - Assert.Equal(["source", "follow-up"], Dispatched); + Assert.Equal(["source", "source-after-raise", "follow-up"], Dispatched); } Assert.False(queue.HasPending); } } + [Fact] + public async Task Raise_PhasedHandlerOutsideAnyFactoryCall_DispatchesImmediately() + { + // A scheduler exists in the scope, but no entry factory call is active — the + // "Raise outside any factory call" case. The phased handler dispatches + // immediately (with a debug log) instead of queueing into a drain nobody owns. + lock (Dispatched) { Dispatched.Clear(); } + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, Recording("deferred")); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var events = scope.ServiceProvider.GetRequiredService(); + var queue = scope.ServiceProvider.GetRequiredService(); + + await events.Raise(new OutsideEntryEvent("x")); + + lock (Dispatched) + { + Assert.Equal(["deferred"], Dispatched); + } + Assert.False(queue.HasPending); + } + } + + [Fact] + public async Task FailedEntryCall_ClearsDeferredWork_AndTheNextSuccessRunsOnlyItsOwn() + { + // The long-lived-scope case (plan review A-V2): a failed entry call's deferred + // work must not ride into the next successful call's drain in the same scope. + lock (Dispatched) { Dispatched.Clear(); } + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, Recording("from-failed-call")); + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, Recording("from-second-call")); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var events = scope.ServiceProvider.GetRequiredService(); + var queue = scope.ServiceProvider.GetRequiredService(); + + // First entry call defers work, then fails. + queue.BeginEntryCall(); + await events.Raise(new FailedEntryEvent("x")); + Assert.True(queue.HasPending); + await queue.EndEntryCallAsync(success: false); + + Assert.False(queue.HasPending); + lock (Dispatched) + { + Assert.Empty(Dispatched); + } + + // Second entry call in the SAME scope succeeds and drains only its own work. + queue.BeginEntryCall(); + await events.Raise(new SecondEntryEvent("y")); + await queue.EndEntryCallAsync(success: true); + + lock (Dispatched) + { + Assert.Equal(["from-second-call"], Dispatched); + } + } + } + [Fact] public async Task Raise_PhasedHandlerWithNoQueueInScope_DispatchesImmediatelyRatherThanVanishing() { From eed3b786e9d4a9838e217c7eaea1bb9932cc879b Mon Sep 17 00:00:00 2001 From: Keith Voels Date: Fri, 14 Aug 2026 21:09:55 -0500 Subject: [PATCH 06/10] test: end-to-end integration coverage for the PHASE-003 entry-call drain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 14 new tests in FactoryEventPhaseEntryTests: HTTP + Logical entry drains with the method-done ordering discriminator, LocalSave nesting (local and under the choke point), AfterFlush-before-AfterCommit sweep at the entry, failure paths never running queued handlers, failure-then-success same-scope clear (A-V2 fixture), falsifiable forbidden-inner-call via the throwing interface-factory auth shape, handler-throw swallow with survivor, drained-handler events joining the same response's relay batch, client-raise entry semantics proven by 9001 logs, and post-success token cancellation not skipping the drain. Unit 662x2, integration 575x2, Design 86x2 — all green. Co-Authored-By: Claude Fable 5 --- .../Phases/FactoryEventPhaseEntryTests.cs | 264 +++++++++++++ .../Events/FactoryEventPhaseEntryTargets.cs | 355 ++++++++++++++++++ 2 files changed, 619 insertions(+) create mode 100644 src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs create mode 100644 src/Tests/RemoteFactory.IntegrationTests/TestTargets/Events/FactoryEventPhaseEntryTargets.cs diff --git a/src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs b/src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs new file mode 100644 index 00000000..0a4db17f --- /dev/null +++ b/src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs @@ -0,0 +1,264 @@ +using Microsoft.Extensions.DependencyInjection; +using Neatoo.RemoteFactory; +using RemoteFactory.IntegrationTests.TestContainers; +using RemoteFactory.IntegrationTests.TestTargets.Events; +using System.Collections.Concurrent; + +namespace RemoteFactory.IntegrationTests.Events.Phases; + +/// +/// End-to-end coverage for PHASE-003: the entry-call drain of AfterCommit handlers, +/// uniformly for HTTP-dispatched [Remote] calls (through HandleRemoteDelegateRequest) +/// and direct Logical invocation (through the generated wrapper seam), with +/// rollback-discard, swallow semantics, sweep ordering, and same-response relay. +/// +/// +/// The ordering discriminator used throughout: the factory method records +/// "*-method-done" as its last statement, so an AfterCommit handler recorded AFTER +/// that marker provably ran at the entry drain, not inline at raise time. +/// +public class FactoryEventPhaseEntryTests +{ + public FactoryEventPhaseEntryTests() + { + PhaseHandlerRegistrations.EnsureRegistered(); + } + + private static List RecordedFor(IServiceScope server, Guid id) + { + return server.GetRequiredService() + .GetRecordedEvents() + .Where(e => e.EntityId == id) + .Select(e => e.EventName) + .ToList(); + } + + [Fact] + public async Task RemoteCreate_AfterCommitHandlerRunsAfterTheEntryCallCompletes() + { + var (server, client, _) = ClientServerContainers.Scopes(); + var factory = client.GetRequiredService(); + var id = Guid.NewGuid(); + + await factory.Create(id); + + Assert.Equal(["immediate", "create-method-done", "after-commit"], RecordedFor(server, id)); + } + + [Fact] + public async Task LogicalCreate_AfterCommitHandlerRunsAfterTheWrapperCompletes() + { + var (_, _, local) = ClientServerContainers.Scopes(); + var factory = local.GetRequiredService(); + var id = Guid.NewGuid(); + + await factory.Create(id); + + Assert.Equal(["immediate", "create-method-done", "after-commit"], RecordedFor(local, id)); + } + + [Fact] + public async Task LogicalSave_NestedInsert_DrainsExactlyOnceAfterTheOutermostCompletion() + { + var (_, _, local) = ClientServerContainers.Scopes(); + var factory = local.GetRequiredService(); + var id = Guid.NewGuid(); + var target = new PhaseEntryTarget { Id = id }; + + await factory.Save(target); + + var recorded = RecordedFor(local, id); + Assert.Equal(["insert-method-done", "save-after-commit"], recorded); + } + + [Fact] + public async Task RemoteSave_NestedUnderTheChokePoint_StillDrainsExactlyOnce() + { + var (server, client, _) = ClientServerContainers.Scopes(); + var factory = client.GetRequiredService(); + var id = Guid.NewGuid(); + var target = new PhaseEntryTarget { Id = id }; + + await factory.Save(target); + + var recorded = RecordedFor(server, id); + Assert.Equal(["insert-method-done", "save-after-commit"], recorded); + } + + [Fact] + public async Task EntryDrain_SweepsAfterFlushBeforeAfterCommit() + { + var (server, client, _) = ClientServerContainers.Scopes(); + var run = client.ServiceProvider.GetRequiredService(); + var id = Guid.NewGuid(); + + await run(id); + + Assert.Equal(["sweep-method-done", "sweep-flush", "sweep-commit"], RecordedFor(server, id)); + } + + [Fact] + public async Task StaticCommand_DrainsAtTheDelegateSeam_LocalAndRemote() + { + var (server, client, local) = ClientServerContainers.Scopes(); + + var remoteId = Guid.NewGuid(); + await client.ServiceProvider.GetRequiredService()(remoteId); + Assert.Equal(["static-method-done", "static-after-commit"], RecordedFor(server, remoteId)); + + var localId = Guid.NewGuid(); + await local.ServiceProvider.GetRequiredService()(localId); + Assert.Equal(["static-method-done", "static-after-commit"], RecordedFor(local, localId)); + } + + [Fact] + public async Task RemoteEntryFails_QueuedHandlersNeverRun() + { + var (server, client, _) = ClientServerContainers.Scopes(); + var factory = client.GetRequiredService(); + var id = Guid.NewGuid(); + + await Assert.ThrowsAsync(() => factory.FetchAndFail(id)); + + Assert.Empty(RecordedFor(server, id)); + } + + [Fact] + public async Task LogicalEntryFails_QueuedHandlersNeverRun() + { + var (_, _, local) = ClientServerContainers.Scopes(); + var factory = local.GetRequiredService(); + var id = Guid.NewGuid(); + + await Assert.ThrowsAsync(() => factory.FetchAndFail(id)); + + Assert.Empty(RecordedFor(local, id)); + } + + [Fact] + public async Task FailedCall_ThenSuccessfulCall_InTheSameServerScope_RunsOnlyTheSecond() + { + // The harness reuses ONE server scope for every remote call in a test — the + // long-lived-scope fixture (plan review A-V2). The failed call's deferred + // work must be cleared, not left to ride the next call's drain. + var (server, client, _) = ClientServerContainers.Scopes(); + var factory = client.GetRequiredService(); + var failedId = Guid.NewGuid(); + var successId = Guid.NewGuid(); + + await Assert.ThrowsAsync(() => factory.FetchAndFail(failedId)); + await factory.Create(successId); + + Assert.Empty(RecordedFor(server, failedId)); + Assert.Equal(["immediate", "create-method-done", "after-commit"], RecordedFor(server, successId)); + } + + [Fact] + public async Task ForbiddenInnerCall_AfterEnqueueingPhasedWork_NothingRuns() + { + var (server, client, _) = ClientServerContainers.Scopes(); + var run = client.ServiceProvider.GetRequiredService(); + var id = Guid.NewGuid(); + + await Assert.ThrowsAsync(() => run(id)); + + Assert.Empty(RecordedFor(server, id)); + } + + [Fact] + public async Task ThrowingAfterCommitHandler_IsSwallowed_TheCallSucceeds_AndTheSurvivorStillRuns() + { + var (server, client, _) = ClientServerContainers.Scopes(); + var run = client.ServiceProvider.GetRequiredService(); + var id = Guid.NewGuid(); + + // The handler throw must NOT fail the entry call's response. + var returned = await run(id); + + Assert.Equal(id, returned); + Assert.Equal(["thrower", "survivor"], RecordedFor(server, id)); + } + + [Fact] + public async Task EventsRaisedByAfterCommitHandlers_JoinTheSameResponsesRelayBatch() + { + var relayed = new ConcurrentBag(); + var (client, server, _) = ClientServerContainers.Scopes( + configureClient: services => services.AddSingleton(new CapturingRelay(relayed))); + + PhaseHandlerRegistrations.EnsureRegistered(); + var run = client.ServiceProvider.GetRequiredService(); + var id = Guid.NewGuid(); + + await run(id); + + // The relay fires on a background task (production parity) — poll briefly. + var deadline = DateTime.UtcNow.AddSeconds(5); + while (DateTime.UtcNow < deadline && !relayed.OfType().Any(e => e.Id == id)) + { + await Task.Delay(25); + } + + Assert.Contains(relayed.OfType(), e => e.Id == id); + // The chain event itself was also raised (by the factory method) and relays too. + Assert.Contains(relayed.OfType(), e => e.Id == id); + } + + [Fact] + public async Task ClientRaisedEvent_PhasedHandlerGetsEntrySemantics_NotTheOutsideEntryFallback() + { + var (server, client, _) = ClientServerContainers.ScopesWithLogging(out var logger); + + PhaseHandlerRegistrations.EnsureRegistered(); + var events = client.GetRequiredService(); + var id = Guid.NewGuid(); + + await events.Raise(new PhasedUntypedRemoteEvent(id)); + + Assert.Equal(["untyped-after-commit"], RecordedFor(server, id)); + + // Entry semantics, provable in the logs: the dispatch was QUEUED (9001) during + // the raise delegate and drained afterward — never the outside-entry immediate + // fallback (9005) and never the no-scheduler fallback (9004). + Assert.Contains(logger.Messages, m => + m.EventId.Id == 9001 && m.Message.Contains(nameof(PhasedUntypedRemoteEvent))); + Assert.DoesNotContain(logger.Messages, m => + (m.EventId.Id == 9005 || m.EventId.Id == 9004) && m.Message.Contains(nameof(PhasedUntypedRemoteEvent))); + } + + [Fact] + public async Task TokenCancelledAfterTheEntryCallSucceeds_DrainStillRuns() + { + var trigger = new PhaseCancellationTrigger(); + var (client, server, _) = ClientServerContainers.Scopes( + configureServer: services => services.AddSingleton(trigger)); + + PhaseHandlerRegistrations.EnsureRegistered(); + using var cts = new CancellationTokenSource(); + trigger.OnCancel = cts.Cancel; + + var run = client.ServiceProvider.GetRequiredService(); + var id = Guid.NewGuid(); + + // The factory method succeeds, then the token goes cancelled. The choke point's + // post-invoke cancellation check still throws to the caller (pre-existing + // behavior), but the drain sits BEFORE that check and passes no token — the + // AfterCommit handler must have run. + await Assert.ThrowsAnyAsync(() => run(id, cts.Token)); + + Assert.Equal(["cancel-after-commit"], RecordedFor(server, id)); + } + + private sealed class CapturingRelay(ConcurrentBag sink) : IFactoryEventRelay + { + public Task Relay(IReadOnlyList factoryEvents) + { + foreach (var evt in factoryEvents) + { + sink.Add(evt); + } + + return Task.CompletedTask; + } + } +} diff --git a/src/Tests/RemoteFactory.IntegrationTests/TestTargets/Events/FactoryEventPhaseEntryTargets.cs b/src/Tests/RemoteFactory.IntegrationTests/TestTargets/Events/FactoryEventPhaseEntryTargets.cs new file mode 100644 index 00000000..27bf6390 --- /dev/null +++ b/src/Tests/RemoteFactory.IntegrationTests/TestTargets/Events/FactoryEventPhaseEntryTargets.cs @@ -0,0 +1,355 @@ +using Microsoft.Extensions.DependencyInjection; +using Neatoo.RemoteFactory; +using Neatoo.RemoteFactory.Internal; + +namespace RemoteFactory.IntegrationTests.TestTargets.Events; + +// ============================================================================= +// PHASE-003 ENTRY-CALL TARGETS +// ============================================================================= +// +// End-to-end targets for entry-call tracking and the framework-owned AfterCommit +// drain. Handlers are registered through FactoryEventHandlerRegistry's 3-arg +// overload (see PhaseHandlerRegistrations) rather than a [FactoryEventHandler] +// phase argument — the generator does not thread the attribute's phase until +// PHASE-002 lands. +// +// One event type per scenario: the registry is process-global with +// (eventType, handlerClass) first-registration-wins dedupe, so sharing an event +// type across scenarios silently drops registrations. + +// ----------------------------------------------------------------------------- +// EVENTS +// ----------------------------------------------------------------------------- + +public record PhasedCreateEvent(Guid Id) : FactoryEventBase; +public record PhasedStaticCommandEvent(Guid Id) : FactoryEventBase; +public record PhasedSaveEvent(Guid Id) : FactoryEventBase; +public record PhasedSweepEvent(Guid Id) : FactoryEventBase; +public record PhasedFailureEvent(Guid Id) : FactoryEventBase; +public record PhasedForbiddenEvent(Guid Id) : FactoryEventBase; +public record PhasedThrowingHandlerEvent(Guid Id) : FactoryEventBase; +public record PhasedRelayChainEvent(Guid Id) : FactoryEventBase; +public record PhasedRelayOutEvent(Guid Id) : FactoryEventBase; +public record PhasedUntypedRemoteEvent(Guid Id) : FactoryEventBase; +public record PhasedCancelAfterSuccessEvent(Guid Id) : FactoryEventBase; + +// ----------------------------------------------------------------------------- +// HANDLER MARKER CLASSES — registration keys only; the invokers are lambdas in +// PhaseHandlerRegistrations +// ----------------------------------------------------------------------------- + +public sealed class PhasedImmediateMarker { } +public sealed class PhasedAfterCommitMarker { } +public sealed class PhasedAfterFlushMarker { } +public sealed class PhasedThrowerMarker { } +public sealed class PhasedSurvivorMarker { } +public sealed class PhasedChainRaiserMarker { } + +/// +/// Registers the phased handlers exactly once per process (the registry's +/// first-registration-wins dedupe makes repeated calls no-ops). +/// +public static class PhaseHandlerRegistrations +{ + private static readonly Lazy Registered = new(RegisterAll); + + public static void EnsureRegistered() => _ = Registered.Value; + + private static bool RegisterAll() + { + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedImmediateMarker), DispatchPhase.Immediate, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("immediate", ((PhasedCreateEvent)evt).Id); + return Task.CompletedTask; + }); + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedAfterCommitMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("after-commit", ((PhasedCreateEvent)evt).Id); + return Task.CompletedTask; + }); + + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedAfterCommitMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("static-after-commit", ((PhasedStaticCommandEvent)evt).Id); + return Task.CompletedTask; + }); + + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedAfterCommitMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("save-after-commit", ((PhasedSaveEvent)evt).Id); + return Task.CompletedTask; + }); + + // Registration order deliberately inverts drain order: AfterCommit first, + // AfterFlush second. The sweep drains AfterFlush first regardless. + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedAfterCommitMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("sweep-commit", ((PhasedSweepEvent)evt).Id); + return Task.CompletedTask; + }); + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedAfterFlushMarker), DispatchPhase.AfterFlush, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("sweep-flush", ((PhasedSweepEvent)evt).Id); + return Task.CompletedTask; + }); + + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedAfterCommitMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("failure-after-commit", ((PhasedFailureEvent)evt).Id); + return Task.CompletedTask; + }); + + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedAfterCommitMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("forbidden-after-commit", ((PhasedForbiddenEvent)evt).Id); + return Task.CompletedTask; + }); + + // Thrower first, survivor second: the post-completion drain must swallow the + // throw and still run the survivor. + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedThrowerMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("thrower", ((PhasedThrowingHandlerEvent)evt).Id); + throw new InvalidOperationException("AfterCommit handler throws (swallow expected)"); + }); + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedSurvivorMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("survivor", ((PhasedThrowingHandlerEvent)evt).Id); + return Task.CompletedTask; + }); + + // Raises a second event from inside the AfterCommit drain. The raise happens + // while the entry is still active and before relay collection, so the new + // event must join the same response's relay batch. + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedChainRaiserMarker), DispatchPhase.AfterCommit, + (sp, evt, _, ct) => + sp.GetRequiredService().Raise(new PhasedRelayOutEvent(((PhasedRelayChainEvent)evt).Id), RaiseOptions.None, ct)); + + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedAfterCommitMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("untyped-after-commit", ((PhasedUntypedRemoteEvent)evt).Id); + return Task.CompletedTask; + }); + + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedAfterCommitMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("cancel-after-commit", ((PhasedCancelAfterSuccessEvent)evt).Id); + return Task.CompletedTask; + }); + + return true; + } +} + +// ----------------------------------------------------------------------------- +// FACTORY TARGETS +// ----------------------------------------------------------------------------- + +/// +/// Entity whose factory methods raise phased events. Create covers the class +/// renderer's read seam; Insert (reached through Save's LocalSave nesting) covers +/// the write seam and the drains-exactly-once-at-the-outermost invariant. +/// +[Factory] +public partial class PhaseEntryTarget : IFactorySaveMeta +{ + public Guid Id { get; set; } + public bool IsDeleted { get; set; } + public bool IsNew { get; set; } = true; + + [Create] + [Remote] + internal async Task Create( + Guid id, + [Service] IFactoryEvents events, + [Service] IEventTestService testService, + CancellationToken ct) + { + Id = id; + await events.Raise(new PhasedCreateEvent(id), RaiseOptions.None, ct); + testService.RecordEventFired("create-method-done", id); + } + + [Fetch] + [Remote] + internal async Task FetchAndFail( + Guid id, + [Service] IFactoryEvents events, + CancellationToken ct) + { + await events.Raise(new PhasedFailureEvent(id), RaiseOptions.None, ct); + throw new InvalidOperationException($"entry call fails for {id}"); + } + + [Insert] + [Remote] + internal async Task Insert( + [Service] IFactoryEvents events, + [Service] IEventTestService testService, + CancellationToken ct) + { + await events.Raise(new PhasedSaveEvent(Id), RaiseOptions.None, ct); + testService.RecordEventFired("insert-method-done", Id); + IsNew = false; + } + + [Update] + [Remote] + internal Task Update() + { + return Task.CompletedTask; + } +} + +/// +/// Static factory commands — the static renderer's DI-lambda entry seam. +/// +[Factory] +public static partial class PhaseStaticCommands +{ + [Execute] + [Remote] + internal static async Task _RunPhased( + Guid id, + [Service] IFactoryEvents events, + [Service] IEventTestService testService, + CancellationToken ct) + { + await events.Raise(new PhasedStaticCommandEvent(id), RaiseOptions.None, ct); + testService.RecordEventFired("static-method-done", id); + return id; + } + + [Execute] + [Remote] + internal static async Task _RunSweep( + Guid id, + [Service] IFactoryEvents events, + [Service] IEventTestService testService, + CancellationToken ct) + { + await events.Raise(new PhasedSweepEvent(id), RaiseOptions.None, ct); + testService.RecordEventFired("sweep-method-done", id); + return id; + } + + [Execute] + [Remote] + internal static async Task _RunWithThrowingHandler( + Guid id, + [Service] IFactoryEvents events, + CancellationToken ct) + { + await events.Raise(new PhasedThrowingHandlerEvent(id), RaiseOptions.None, ct); + return id; + } + + [Execute] + [Remote] + internal static async Task _RunRelayChain( + Guid id, + [Service] IFactoryEvents events, + CancellationToken ct) + { + await events.Raise(new PhasedRelayChainEvent(id), RaiseOptions.None, ct); + return id; + } + + /// + /// Enqueues phased work, then calls an authorization-denied interface factory — + /// the falsifiable forbidden-does-not-drain shape (a bare forbidden call has an + /// empty queue and proves nothing). The interface-factory leg is the harness's + /// auth shape that THROWS on denial (class-factory reads return + /// Authorized<T> instead — a successful call; see the plan's Intent). + /// + [Execute] + [Remote] + internal static async Task _RunForbiddenInner( + Guid id, + [Service] IFactoryEvents events, + [Service] IPhaseDeniedServiceFactory deniedFactory, + CancellationToken ct) + { + await events.Raise(new PhasedForbiddenEvent(id), RaiseOptions.None, ct); + await deniedFactory.GetSecret(); + return id; + } + + /// + /// Enqueues phased work, then cancels the request token — the call itself has + /// already succeeded, so the entry drain must still run in full. + /// + [Execute] + [Remote] + internal static async Task _RunAndCancel( + Guid id, + [Service] IFactoryEvents events, + [Service] PhaseCancellationTrigger trigger, + CancellationToken ct) + { + await events.Raise(new PhasedCancelAfterSuccessEvent(id), RaiseOptions.None, ct); + trigger.Cancel(); + return id; + } +} + +/// +/// Test hook that lets a server-side factory method cancel the request token the +/// client passed in — simulating a token that goes cancelled between the entry +/// call succeeding and the response being produced. +/// +public sealed class PhaseCancellationTrigger +{ + public Action? OnCancel { get; set; } + public void Cancel() => OnCancel?.Invoke(); +} + +/// Authorization that always denies reads. +public class PhaseDenyAuth +{ + [AuthorizeFactory(AuthorizeFactoryOperation.Read)] + public bool CanRead() => false; +} + +/// +/// Interface factory whose methods are always authorization-denied — the harness's +/// throwing denial shape (NotAuthorizedException from the generated auth check). +/// +[Factory] +[AuthorizeFactory] +public interface IPhaseDeniedService +{ + Task GetSecret(); +} + +/// Named to match IPhaseDeniedService for RegisterMatchingName. +public class PhaseDeniedService : IPhaseDeniedService +{ + public Task GetSecret() => Task.FromResult("secret"); +} From 6150faffaeb0c329cc3d592acbf254a585d97769 Mon Sep 17 00:00:00 2001 From: Keith Voels Date: Fri, 14 Aug 2026 21:14:45 -0500 Subject: [PATCH 07/10] docs(todo): PHASE-003 Test Evidence map, plan amendments, red-proof log; 9005/9006 log-table rows Co-Authored-By: Claude Fable 5 --- .../plans/003-aftercommit-entry-call-drain.md | 87 ++++++++++++++++++- src/Design/CLAUDE-DESIGN.md | 2 + 2 files changed, 87 insertions(+), 2 deletions(-) diff --git a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md index 60a00d9c..3f8470e3 100644 --- a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md +++ b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md @@ -373,13 +373,96 @@ every "drains exactly once" assertion. ## Test Evidence -*(filled before the Step 5 gate)* +Suites (Release, both TFMs, logs in `reviews/003-build.log` / `reviews/003-test.log`): +unit **662×2** (baseline 653; +8 new, +1 renamed), integration **575×2 +5 skipped** +(baseline 561+5; +14 new), Design **86×2** (unchanged). 0 failures. + +Unit tests live in `FactoryEntryCallTests` (new), `FactoryEventsDispatcherPhaseTests`; +integration tests in `Events/Phases/FactoryEventPhaseEntryTests` (new) with targets in +`TestTargets/Events/FactoryEventPhaseEntryTargets.cs`. + +| Acceptance bullet | Test(s) | Tier | +|---|---|---| +| HTTP entry: AfterCommit after entry completes + same-response relay | `RemoteCreate_AfterCommitHandlerRunsAfterTheEntryCallCompletes` (ordering discriminator: handler after `create-method-done`); relay half: `EventsRaisedByAfterCommitHandlers_JoinTheSameResponsesRelayBatch` | integration ✓ | +| Logical entry through the public wrapper (class factory) | `LogicalCreate_AfterCommitHandlerRunsAfterTheWrapperCompletes` | integration ✓ | +| `LocalSave` nesting drains exactly once at the outermost | `LogicalSave_NestedInsert_DrainsExactlyOnceAfterTheOutermostCompletion`, `RemoteSave_NestedUnderTheChokePoint_StillDrainsExactlyOnce` (depth-3: choke → LocalSave → LocalInsert; a depth mismatch throws on the success path, so green = depth-correct) | integration ✓ | +| AfterFlush before AfterCommit at the entry drain | `EntryDrain_SweepsAfterFlushBeforeAfterCommit` (registration order deliberately inverted) | integration ✓ | +| Entry throws → queued handlers never run (both entry families) | `RemoteEntryFails_QueuedHandlersNeverRun`, `LogicalEntryFails_QueuedHandlersNeverRun` | integration ✓ | +| Failure then success in the same scope runs only the success's work | `FailedCall_ThenSuccessfulCall_InTheSameServerScope_RunsOnlyTheSecond` (harness's single reused server scope); unit-level: `FailedEntryCall_ClearsDeferredWork_AndTheNextSuccessRunsOnlyItsOwn` | integration ✓ | +| Forbidden call does not drain (falsifiable form) | `ForbiddenInnerCall_AfterEnqueueingPhasedWork_NothingRuns` — outer enqueues, then a `NotAuthorizedException`-throwing interface-factory denial. **Note:** the harness has no ASP.NET pipeline, so the `AspForbidException` shape specifically is unexercised; it rides the same choke-point catch as every throw (see Current State), and the class-factory `Authorized` denial is a *successful* call by design (Intent). | integration ✓ (noted gap) | +| Handler exception swallowed, response succeeds, survivors run | `ThrowingAfterCommitHandler_IsSwallowed_TheCallSucceeds_AndTheSurvivorStillRuns`; scheduler-level 9003/OCE pins carried from PHASE-001 | integration ✓ | +| Client-raised event (`RaiseUntyped` remote path) gets entry semantics | `ClientRaisedEvent_PhasedHandlerGetsEntrySemantics_NotTheOutsideEntryFallback` (9001 queued in logs; 9004/9005 absent) — also closes the RaiseUntyped tech debt together with `RaiseUntyped_DeferredHandler_DefersJustLikeRaise` | integration ✓ | +| Event raised by a draining handler joins the current drain (B-V3) | `DrainedHandlerRaisingAnEvent_GoesThroughTheRealRaisePath` (re-pointed; mid-drain marker discriminates) — **red-proofed** | unit ✓ | +| Raise outside any factory call dispatches immediately + debug log | `Raise_PhasedHandlerOutsideAnyFactoryCall_DispatchesImmediately` (9005 emission pinned indirectly via the integration log test's DoesNotContain) | unit ✓ | +| Entry drain passes no token and sits before the cancellation check | `TokenCancelledAfterTheEntryCallSucceeds_DrainStillRuns` `[integration]` — **red-proofed**; token identity: `RunAsync_EntryDrainPassesNoCancellationToken` `[unit]` | integration ✓ | +| Sync (non-`Task`) factory entry loses nothing | `Run_SyncEntryWithDeferredWork_DoesNotLoseIt` | unit ✓ | +| Nested factory calls don't drain at the inner completion | `RunAsync_NestedEntry_DoesNotDrainAtTheInnerCompletion` | unit ✓ | +| Backward compatibility (full suites + Design, only pre-declared amendments) | Suite totals above; zero failures outside the amended set | integration ✓ | + +**Red-proofing** (`reviews/003-redproof.log`): three deliberate wrong-implementations, +each turning exactly the predicted tests red on both TFMs — (1) depth popped before the +drain → re-entrancy order inverts; (2) drain moved after the choke point's cancellation +check → cancel-after-success handler never runs; (3) failure decrements without clearing +→ the failed call's work rides the next drain at both tiers. + +**Pre-declared amendment set — actual outcomes:** + +| Test | Outcome | +|---|---| +| `Raise_DeferredHandler_DoesNotDispatchAtRaiseTime` | Amended: raise now inside `BeginEntryCall`; drain via `EndEntryCallAsync(true)`. Intent (defer at raise, dispatch at drain) preserved. | +| `Raise_MixedPhases_ImmediateRunsAndDeferredWaits` | Amended likewise; cross-phase ordering assertion unchanged. | +| `RaiseUntyped_DeferredHandler_DefersJustLikeRaise` | Amended likewise; RaiseUntyped parity intent unchanged. | +| `PhaseDispatcher_IsScoped_NotSharedAcrossScopes` | **No change needed** — registers/enqueues at the scheduler level, unaffected by the dispatcher's entry gate; still green, still pins scope isolation. | +| `ScopeDisposedWithoutDraining_RunsNothing` | **No change needed** — scheduler-level pin (disposal runs nothing) still meaningful and green. | +| `DrainedHandlerRaisingAnEvent_GoesThroughTheRealRaisePath` | Re-pointed with the mid-drain marker so it discriminates (red-proof 1). | + +Also amended (in-charter, generator emission shape): two TRIM-009 pins in +`AssemblyAttributeEmissionTests` — the async-split test's forward line updated to the +`FactoryEntryCall` route, and `ClassFactory_GuardedSyncLocalMethod_IsNotSplit` renamed to +`…_SplitsIntoSyncWrapperAndSyncCore` (every `Local*` now splits; the pinned trimming +property — guard never inside a state machine — is asserted in its new form). --- ## Plan Amendments -*(none yet)* +### 2026-08-14 — Entry tracking lives on the scheduler; generated code routes through a helper + +Step 1's keyboard decision: `BeginEntryCall`/`EndEntryCallAsync(success)`/`IsEntryCallActive` +went onto `IFactoryEventPhaseScheduler` itself (already scoped, already registered in +Server+Logical, already the drain surface) rather than a sibling service. Generated code +does not emit begin/try/catch scaffolding — it routes the local execution through a new +public helper, `Neatoo.RemoteFactory.Internal.FactoryEntryCall.Run/RunAsync(sp, body)`, +which concentrates the entry semantics in one unit-testable place and keeps every +generated wrapper **non-async** (guard never enters a state machine — the B-V1 trimming +concern never materializes). The scheduler's queues and depth are lock-guarded (B-C2). + +### 2026-08-14 — Every `Local*` method now splits into wrapper + Core + +The TRIM-009 guard-split, previously emitted only for async server-only methods, is now +the uniform shape: non-async wrapper (guard when server-only + `FactoryEntryCall` +forward) → private `Local*Core` keeping the body's original asyncness. The interface +renderer gained the split for the first time (its inline-guard comment updated; TRIM +item 20's elimination-UNVERIFIED status stands — the single-method-holder half of the +TRIM-009 fix is still absent on that leg). The static renderer wraps its server DI-lambda +body in `FactoryEntryCall.RunAsync` (all static delegates are `Task`-returning; its +guard wraps the registration, so nothing moved into a state machine). `LocalSave`'s +depth releases on task completion because the helper awaits the returned task (B-C6). + +### 2026-08-14 — Sync (non-`Task`) entry shape: block-drain only when pending (Step 7) + +`FactoryEntryCall.Run` resolves the scheduler null-tolerantly (client-reachable unguarded +shapes no-op, B-C3); on outermost success with pending deferred work it drains via +`GetAwaiter().GetResult()` — blocking, accepted deliberately: the only way work is +pending in a sync entry is a fire-and-forget `Raise` inside a synchronous factory +method, and no-silent-loss outranks non-blocking there. With nothing pending the +completion is fully synchronous. + +### 2026-08-14 — Post-OCE clear at the entry drain + +If the entry drain itself throws (a handler's own `OperationCanceledException`), the +outermost exit still releases depth and discards whatever remained — a clear, never a +drain — so "between entry calls the scheduler is empty" holds on every exit path. --- diff --git a/src/Design/CLAUDE-DESIGN.md b/src/Design/CLAUDE-DESIGN.md index 3cbafb41..f202d984 100644 --- a/src/Design/CLAUDE-DESIGN.md +++ b/src/Design/CLAUDE-DESIGN.md @@ -1018,6 +1018,8 @@ These are known limitations or open questions. They are documented here to preve | 9002 | `FactoryEventPhaseDrained` | Debug | A phase drain completes, reporting how many dispatches ran through the requested phase (earlier phases included) | Informational | | 9003 | `FactoryEventPhaseHandlerFailed` | Error | A deferred handler throws during a **post-completion** drain (no ambient transaction) | Swallowed — the exception can no longer roll anything back; remaining queued handlers still run. `OperationCanceledException` still propagates. In-transaction drains propagate instead, so this never fires for them. | | 9004 | `FactoryEventPhaseNoQueueInScope` | Debug | An event with a phased handler is raised in a scope with no `IFactoryEventPhaseScheduler` registered | Dispatched immediately rather than dropped | +| 9005 | `FactoryEventPhaseRaisedOutsideEntryCall` | Debug | An event with a phased handler is raised while no entry factory call is active in the scope | Dispatched immediately rather than queued into a drain nobody owns | +| 9006 | `FactoryEventPhaseClearedOnFailure` | Debug | An entry factory call exits without completing successfully and discards its deferred dispatches | The clear (never a drain) that keeps a failed call's work from riding a later call's drain in long-lived scopes | ### Public Exception From 595d195b4dd209c2c653a1e4dc3b8e8014e09a0a Mon Sep 17 00:00:00 2001 From: Keith Voels Date: Fri, 14 Aug 2026 21:40:54 -0500 Subject: [PATCH 08/10] test: close PHASE-003 test-review round 1 findings (3 must, 6 should) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AspForbidException success-shaped denial exercised end-to-end; concurrent- flows-share-entry-state semantics pinned as a documented limitation; interface renderer emission-shape pin added; nested-save inner-vs-outer discriminator; post-OCE entry-exit clear + double-End tolerance pin; relay-collection tests' entry-active premise restored; positive 9005 emission pin; caught-nested- failure and handler-invokes-factory re-entrancy pins; interface success-path and generated-sync-shape integration coverage. Unit 668x2, integration 579x2, Design 86x2 — 0 failures. Co-Authored-By: Claude Fable 5 --- .../plans/003-aftercommit-entry-call-drain.md | 46 +++- .../reviews/003-test-review.md | 79 +++++++ .../todos/PHASE-phased-event-dispatch/todo.md | 30 ++- .../Phases/FactoryEventPhaseEntryTests.cs | 67 ++++++ .../Events/FactoryEventPhaseEntryTargets.cs | 151 +++++++++++++ .../AssemblyAttributeEmissionTests.cs | 52 +++++ .../Internal/FactoryEntryCallTests.cs | 213 +++++++++++++++++- .../FactoryEventsDispatcherPhaseTests.cs | 61 ++++- 8 files changed, 678 insertions(+), 21 deletions(-) create mode 100644 docs/todos/PHASE-phased-event-dispatch/reviews/003-test-review.md diff --git a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md index 3f8470e3..2054ba92 100644 --- a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md +++ b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md @@ -265,8 +265,10 @@ New in this plan: inner completion. `[unit]` - [ ] Backward compatibility: the full existing suite — unit, integration, AND the Design solution's suite (renderer changes regenerate every Design factory) — - passes with only the six pre-declared pin amendments named in Constraints (each - listed in Test Evidence with intent preserved). `[integration]` + passes with only the pre-declared amendment set: the six pin tests named in + Constraints plus the two TRIM-009 emission-shape pins and the two relay-collection + premise restorations disclosed in Test Evidence (each with intent preserved). + `[integration]` --- @@ -373,9 +375,13 @@ every "drains exactly once" assertion. ## Test Evidence +*(Updated after the test-review round 1 loop — see `reviews/003-test-review.md` for the +findings and their disposition.)* + Suites (Release, both TFMs, logs in `reviews/003-build.log` / `reviews/003-test.log`): -unit **662×2** (baseline 653; +8 new, +1 renamed), integration **575×2 +5 skipped** -(baseline 561+5; +14 new), Design **86×2** (unchanged). 0 failures. +unit **668×2** (baseline 653; +9 in the first pass, +6 closing the gate's findings; one +rename is count-neutral), integration **579×2 +5 skipped** (baseline 561+5; +14 first +pass, +4 closing findings), Design **86×2** (unchanged). 0 failures. Unit tests live in `FactoryEntryCallTests` (new), `FactoryEventsDispatcherPhaseTests`; integration tests in `Events/Phases/FactoryEventPhaseEntryTests` (new) with targets in @@ -385,15 +391,15 @@ integration tests in `Events/Phases/FactoryEventPhaseEntryTests` (new) with targ |---|---|---| | HTTP entry: AfterCommit after entry completes + same-response relay | `RemoteCreate_AfterCommitHandlerRunsAfterTheEntryCallCompletes` (ordering discriminator: handler after `create-method-done`); relay half: `EventsRaisedByAfterCommitHandlers_JoinTheSameResponsesRelayBatch` | integration ✓ | | Logical entry through the public wrapper (class factory) | `LogicalCreate_AfterCommitHandlerRunsAfterTheWrapperCompletes` | integration ✓ | -| `LocalSave` nesting drains exactly once at the outermost | `LogicalSave_NestedInsert_DrainsExactlyOnceAfterTheOutermostCompletion`, `RemoteSave_NestedUnderTheChokePoint_StillDrainsExactlyOnce` (depth-3: choke → LocalSave → LocalInsert; a depth mismatch throws on the success path, so green = depth-correct) | integration ✓ | +| `LocalSave` nesting drains exactly once at the outermost | `LogicalSave_NestedInsert_DrainsExactlyOnceAfterTheOutermostCompletion`, `RemoteSave_NestedUnderTheChokePoint_StillDrainsExactlyOnce` (once-only + depth-3 shape), and — the actual inner-vs-outer discriminator, added at the gate after review round 1 showed the first two cannot separate the drains — `NestedChildSave_DoesNotDrainAtTheChildsCompletion` (parent Insert saves a child then records a marker; an inner drain lands between the markers) | integration ✓ | | AfterFlush before AfterCommit at the entry drain | `EntryDrain_SweepsAfterFlushBeforeAfterCommit` (registration order deliberately inverted) | integration ✓ | | Entry throws → queued handlers never run (both entry families) | `RemoteEntryFails_QueuedHandlersNeverRun`, `LogicalEntryFails_QueuedHandlersNeverRun` | integration ✓ | | Failure then success in the same scope runs only the success's work | `FailedCall_ThenSuccessfulCall_InTheSameServerScope_RunsOnlyTheSecond` (harness's single reused server scope); unit-level: `FailedEntryCall_ClearsDeferredWork_AndTheNextSuccessRunsOnlyItsOwn` | integration ✓ | -| Forbidden call does not drain (falsifiable form) | `ForbiddenInnerCall_AfterEnqueueingPhasedWork_NothingRuns` — outer enqueues, then a `NotAuthorizedException`-throwing interface-factory denial. **Note:** the harness has no ASP.NET pipeline, so the `AspForbidException` shape specifically is unexercised; it rides the same choke-point catch as every throw (see Current State), and the class-factory `Authorized` denial is a *successful* call by design (Intent). | integration ✓ (noted gap) | +| Forbidden call does not drain (falsifiable form) | `ForbiddenInnerCall_AfterEnqueueingPhasedWork_NothingRuns` (outer enqueues, then a `NotAuthorizedException`-throwing interface-factory denial) and `AspForbidException_AfterEnqueueingPhasedWork_ClearsWithoutDraining` (the success-shaped-return denial: the exception type is public in the core package — review round 1 corrected the earlier claim that it needed an ASP.NET pipeline; the test also pins the pre-existing empty-shape → `default` client observable). The class-factory `Authorized` denial remains a *successful* call by design (Intent). | integration ✓ | | Handler exception swallowed, response succeeds, survivors run | `ThrowingAfterCommitHandler_IsSwallowed_TheCallSucceeds_AndTheSurvivorStillRuns`; scheduler-level 9003/OCE pins carried from PHASE-001 | integration ✓ | | Client-raised event (`RaiseUntyped` remote path) gets entry semantics | `ClientRaisedEvent_PhasedHandlerGetsEntrySemantics_NotTheOutsideEntryFallback` (9001 queued in logs; 9004/9005 absent) — also closes the RaiseUntyped tech debt together with `RaiseUntyped_DeferredHandler_DefersJustLikeRaise` | integration ✓ | | Event raised by a draining handler joins the current drain (B-V3) | `DrainedHandlerRaisingAnEvent_GoesThroughTheRealRaisePath` (re-pointed; mid-drain marker discriminates) — **red-proofed** | unit ✓ | -| Raise outside any factory call dispatches immediately + debug log | `Raise_PhasedHandlerOutsideAnyFactoryCall_DispatchesImmediately` (9005 emission pinned indirectly via the integration log test's DoesNotContain) | unit ✓ | +| Raise outside any factory call dispatches immediately + debug log | `Raise_PhasedHandlerOutsideAnyFactoryCall_DispatchesImmediately` — now with a **positive** 9005 Debug emission assertion via a capturing logger (review round 1: an absence assertion cannot pin an emission) | unit ✓ | | Entry drain passes no token and sits before the cancellation check | `TokenCancelledAfterTheEntryCallSucceeds_DrainStillRuns` `[integration]` — **red-proofed**; token identity: `RunAsync_EntryDrainPassesNoCancellationToken` `[unit]` | integration ✓ | | Sync (non-`Task`) factory entry loses nothing | `Run_SyncEntryWithDeferredWork_DoesNotLoseIt` | unit ✓ | | Nested factory calls don't drain at the inner completion | `RunAsync_NestedEntry_DoesNotDrainAtTheInnerCompletion` | unit ✓ | @@ -420,7 +426,31 @@ Also amended (in-charter, generator emission shape): two TRIM-009 pins in `AssemblyAttributeEmissionTests` — the async-split test's forward line updated to the `FactoryEntryCall` route, and `ClassFactory_GuardedSyncLocalMethod_IsNotSplit` renamed to `…_SplitsIntoSyncWrapperAndSyncCore` (every `Local*` now splits; the pinned trimming -property — guard never inside a state machine — is asserted in its new form). +property — guard never inside a state machine — is asserted in its new form). Review +round 1 also caught two tests **weakened by omission** rather than edit — +`Raise_DeferredHandler_StillCollectsForRelayAtRaiseTime` and its ServerOnly sibling ran +with no entry active after the dispatcher change, so their deferral premise was vacuous; +both restored with `BeginEntryCall` (the failure mode pre-declaration cannot catch). + +**Gate-closure additions (after test-review round 1):** + +| Round-1 finding | Closing test | Tier | +|---|---|---| +| Must 1: `AspForbidException` success-shaped denial unexercised | `AspForbidException_AfterEnqueueingPhasedWork_ClearsWithoutDraining` | integration | +| Must 2: concurrent flows in one scope — semantics unrecorded | `ConcurrentFlowsInOneScope_ShareEntryState_FailedFlowsWorkRidesTheSurvivingDrain` (pins the documented limitation; Discovery Log entry records the posture) | unit | +| Must 3: interface-renderer emission unpinned | `InterfaceFactory_GuardedLocalMethod_SplitsIntoSyncWrapperAndCore` | unit | +| Should 4: nesting tests didn't discriminate | `NestedChildSave_DoesNotDrainAtTheChildsCompletion` | integration | +| Should 5: post-OCE clear + double-End tolerance unpinned | `HandlerThrowsOperationCanceled_MidDrain_EntryExitStillClearsAndDepthSurvives` | unit | +| Should 6: relay-collection premise vacuous | Both tests restored with `BeginEntryCall` + `HasPending` assertion | unit | +| Should 7: 9005 emission unpinned | Positive Debug 9005 assertion added to the outside-entry test | unit | +| Should 8: caught-nested-failure + handler-invokes-factory | `NestedEntryFails_OuterCatchesAndSucceeds_TheEntryStillDrains`, `DrainedHandlerInvokingAFactory_NestsWithoutDrainingOrClearingTheDrainInProgress` | unit | +| Should 9: interface success-path + generated sync shape | `InterfaceFactory_AsTheOutermostEntry_DrainsOnSuccess`, `SyncFactoryMethod_WithPendingPhasedWork_BlockDrainsAtCompletion` | integration | +| (nice) sync no-scheduler mirror; strengthened misuse pin | `Run_NoSchedulerInScope_JustRunsTheBody`; `EndEntryCall_WithoutBegin_ThrowsOnTheSuccessPath` now asserts depth survival + a working follow-on cycle | unit | + +Not closed this round (recorded, not hidden): 9002/9004/9006 positive emission pins and +the `ClientServerContainers` tuple-order/duplication hazard → routed to the PHASE-007 +tech-debt plan; the relay-batch integration test was not red-proofed (structurally sound; +noted). --- diff --git a/docs/todos/PHASE-phased-event-dispatch/reviews/003-test-review.md b/docs/todos/PHASE-phased-event-dispatch/reviews/003-test-review.md new file mode 100644 index 00000000..6234824a --- /dev/null +++ b/docs/todos/PHASE-phased-event-dispatch/reviews/003-test-review.md @@ -0,0 +1,79 @@ +# PHASE-003 Test Review — Step 5 Gate + +**Plan:** [../plans/003-aftercommit-entry-call-drain.md](../plans/003-aftercommit-entry-call-drain.md) +**Logs:** `003-build.log`, `003-test.log`, `003-redproof.log` (regenerated after each round) + +## Round 1 — 2026-08-14 + +Reviewer verdict shape: 3 must-cover, 6 should-cover, 5 nice-to-have (plan-related); +1 must / 2 should / 2 nice (pre-existing tech debt). Baseline cross-check and red-proof +log verified genuine. Three Test Evidence rows found overstating their citations. + +### Must-cover (all closed) + +1. **`AspForbidException` never exercised — and the noted mitigation was wrong.** The + reviewer showed the exception type is public in the core package (only its producers + need ASP.NET), so a direct-throw target needs no pipeline. Closed with + `AspForbidException_AfterEnqueueingPhasedWork_ClearsWithoutDraining`, which also pins + the pre-existing empty-shape → `default` client observable. Evidence row corrected. +2. **Concurrent flows in one scope: lock ≠ flow isolation, semantics unrecorded.** A + failed flow's exit is a nested exit (no clear); the surviving flow's drain runs both + flows' work. Closed by pinning exactly those semantics + (`ConcurrentFlowsInOneScope_ShareEntryState_FailedFlowsWorkRidesTheSurvivingDrain`) + and recording the per-scope-granularity posture as a Discovery Log entry — scopes are + the framework's isolation unit; concurrent flows sharing one scope already share + every scoped service. +3. **Interface-renderer emission shape unpinned** (the leg where the split is new and + trimming is UNVERIFIED). Closed with + `InterfaceFactory_GuardedLocalMethod_SplitsIntoSyncWrapperAndCore` (wrapper non-async + + guard + helper forward; core private and unguarded; an `async` wrapper goes red). + +### Should-cover (all closed) + +4. Nesting tests couldn't discriminate inner-vs-outer drain (and the evidence row's + "depth mismatch throws" justification was wrong — a balanced always-drain stays + green). Closed with `NestedChildSave_DoesNotDrainAtTheChildsCompletion` (parent saves + a child, then records a marker; an inner drain lands between the markers). Row + corrected. +5. Post-OCE entry-exit clear and the double-`EndEntryCallAsync` tolerance it relies on: + `HandlerThrowsOperationCanceled_MidDrain_EntryExitStillClearsAndDepthSurvives`. +6. Two sacred relay-collection tests were silently weakened by the production change + (ran with no entry active → deferral premise vacuous). Restored with + `BeginEntryCall` + `HasPending`; disclosed in the evidence as the failure mode + pre-declaration cannot catch. +7. 9005 had no positive emission pin ("absence elsewhere" ≠ pin). Closed with a + capturing logger in the outside-entry unit test. +8. Caught-nested-failure and drained-handler-invokes-a-factory: + `NestedEntryFails_OuterCatchesAndSucceeds_TheEntryStillDrains` (pins that only the + outermost exit decides drain-vs-clear) and + `DrainedHandlerInvokingAFactory_NestsWithoutDrainingOrClearingTheDrainInProgress`. +9. Interface-leg success-path drain as the outermost entry + (`InterfaceFactory_AsTheOutermostEntry_DrainsOnSuccess`) and the generated sync + non-`Task` shape with pending work + (`SyncFactoryMethod_WithPendingPhasedWork_BlockDrainsAtCompletion`). + +### Nice-to-have + +Closed: sync no-scheduler mirror; strengthened End-without-Begin pin (depth survival + +follow-on cycle). Open by choice: 9006 emission pin (routed to PHASE-007 with 9002/9004), +relay-batch red-proof (structurally sound — the drain sits before relay collection in the +same method; noted). + +### Tech debt routed to PHASE-007 + +9002/9004 (and now 9006) positive emission pins; `ClientServerContainers` tuple-order +divergence + `ScopesWithLogging` duplication. The pre-existing AspForbid response-shape +gap is now partially covered by the new integration test. + +### Bookkeeping corrections applied + +Unit-count breakdown (+9, not +8+1); backward-compat Acceptance bullet re-worded to name +the full disclosed amendment set (six pins + two TRIM-009 emission pins + two +relay-collection restorations). + +**Post-closure totals:** unit 668×2, integration 579×2 (+5 skipped), Design 86×2 — 0 +failures. Logs regenerated. + +## Round 2 — 2026-08-14 + +*(appended after re-review)* diff --git a/docs/todos/PHASE-phased-event-dispatch/todo.md b/docs/todos/PHASE-phased-event-dispatch/todo.md index e81b66e5..0634be40 100644 --- a/docs/todos/PHASE-phased-event-dispatch/todo.md +++ b/docs/todos/PHASE-phased-event-dispatch/todo.md @@ -76,12 +76,40 @@ exposes drain points. | 004 | [004-afterflush-coordinator](./plans/004-afterflush-coordinator.md) | IFactoryEventPhaseCoordinator public API + fallback drain | Draft | | 005 | [005-design-docs-skill](./plans/005-design-docs-skill.md) | Design projects, published docs, skill reference | Draft | | 006 | [006-coalescing](./plans/006-coalescing.md) | Opt-in same-event coalescing (v2, queued per user) | Draft | -| 007 | *(not yet drafted)* | Tech debt: registry test-isolation hook (`Clear()` is internal and uncalled; every test invents unique event types) | Draft | +| 007 | *(not yet drafted)* | Tech debt: registry test-isolation hook (`Clear()` is internal and uncalled; every test invents unique event types); 9002/9004/9006 positive emission pins; `ClientServerContainers` tuple-order divergence + `ScopesWithLogging` duplication | Draft | --- ## Discovery Log +### 2026-08-14 — PHASE-003 (concurrent flows share entry state — documented limitation) +- **Finding:** The test-review gate pressed on plan-review B-C2: the scheduler's lock + gives data-race safety, but entry tracking is per-scope, not per-flow. Two concurrent + flows in one scope interleave on one depth counter — a failed flow's exit is a nested + exit (no clear), so its queued work rides the surviving flow's drain. +- **Decision:** Document, don't redesign. Scopes are the framework's isolation unit; + concurrent flows sharing a scope already share DbContexts and every scoped service. + The actual semantics are pinned + (`ConcurrentFlowsInOneScope_ShareEntryState_FailedFlowsWorkRidesTheSurvivingDrain`) + so any change to them is a conscious one. Per-flow tracking (AsyncLocal) would be a + design change with its own hazards — revisit only if a real consumer hits this. +- **Follow-up:** PHASE-005 documents "one factory call per scope at a time" as the + concurrency guidance. + +### 2026-08-14 — PHASE-003 (test-review round 1: 3 must-cover, all closed) +- **Finding:** The gate's sharpest catches: the `AspForbidException` mitigation note was + factually wrong (the type is public in core — no ASP.NET pipeline needed to exercise + it); the nested-save tests could not discriminate inner-vs-outer drain and the + evidence row's justification was incorrect; and two sacred relay-collection tests had + been silently weakened by the dispatcher change without being edited — the failure + mode pre-declaration cannot catch. +- **Decision:** Amend — all 3 must-cover and all 6 should-cover findings closed with + tests (+6 unit, +4 integration); evidence rows corrected; two nice-to-haves left open + by choice and recorded. +- **Follow-up:** [reviews/003-test-review.md](./reviews/003-test-review.md); 9002/9004/ + 9006 emission pins and the `ClientServerContainers` tuple-order/duplication hazard + routed to PHASE-007 (its scope grows accordingly). + ### 2026-08-14 — PHASE-003 (plan review) - **Finding:** Plan review returned CONCERNS — 6 veto findings. The sharpest: the "structural" rollback-discard story is false for long-lived scopes (a failed call's diff --git a/src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs b/src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs index 0a4db17f..c63311e0 100644 --- a/src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs +++ b/src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs @@ -249,6 +249,73 @@ public async Task TokenCancelledAfterTheEntryCallSucceeds_DrainStillRuns() Assert.Equal(["cancel-after-commit"], RecordedFor(server, id)); } + [Fact] + public async Task AspForbidException_AfterEnqueueingPhasedWork_ClearsWithoutDraining() + { + // The one failure path with a success-shaped return: the choke point converts + // AspForbidException to an empty RemoteResponseDto. The drain must not ride + // that shape — the entry failed, the queued work clears. + var (server, client, _) = ClientServerContainers.Scopes(); + var run = client.ServiceProvider.GetRequiredService(); + var id = Guid.NewGuid(); + + // The empty-shape response deserializes to default client-side — the + // pre-existing forbid semantics for value returns. Pinned here so a change to + // that shape is a conscious one; the load-bearing assertion is the no-drain. + var returned = await run(id); + Assert.Equal(Guid.Empty, returned); + + Assert.Empty(RecordedFor(server, id)); + } + + [Fact] + public async Task NestedChildSave_DoesNotDrainAtTheChildsCompletion() + { + // The inner-vs-outer discriminator: the parent's Insert raises phased work, + // saves a child through the child's factory (a nested entry), then records + // "parent-after-child". A drain firing at ANY inner completion would land + // "nested-after-commit" between the two markers. + var (_, _, local) = ClientServerContainers.Scopes(); + var factory = local.GetRequiredService(); + var id = Guid.NewGuid(); + + await factory.Save(new PhaseParentTarget { Id = id }); + + Assert.Equal( + ["child-insert-done", "parent-after-child", "nested-after-commit"], + RecordedFor(local, id)); + } + + [Fact] + public async Task InterfaceFactory_AsTheOutermostEntry_DrainsOnSuccess() + { + // Success-path drain on the interface leg, resolved directly server-side so the + // interface factory itself is the depth-1 entry (behind the choke point it is + // always nested). + var (server, _, _) = ClientServerContainers.Scopes(); + var factory = server.GetRequiredService(); + var id = Guid.NewGuid(); + + await factory.AuditPhased(id); + + Assert.Equal(["interface-method-done", "interface-after-commit"], RecordedFor(server, id)); + } + + [Fact] + public void SyncFactoryMethod_WithPendingPhasedWork_BlockDrainsAtCompletion() + { + // The generated Run route (sync non-Task factory shape) with deferred work + // actually pending — no-silent-loss at the generated level, not just the + // FactoryEntryCall.Run unit level. + var (server, _, _) = ClientServerContainers.Scopes(); + var factory = server.GetRequiredService(); + var id = Guid.NewGuid(); + + factory.Create(id); + + Assert.Equal(["sync-method-done", "sync-after-commit"], RecordedFor(server, id)); + } + private sealed class CapturingRelay(ConcurrentBag sink) : IFactoryEventRelay { public Task Relay(IReadOnlyList factoryEvents) diff --git a/src/Tests/RemoteFactory.IntegrationTests/TestTargets/Events/FactoryEventPhaseEntryTargets.cs b/src/Tests/RemoteFactory.IntegrationTests/TestTargets/Events/FactoryEventPhaseEntryTargets.cs index 27bf6390..7725d67f 100644 --- a/src/Tests/RemoteFactory.IntegrationTests/TestTargets/Events/FactoryEventPhaseEntryTargets.cs +++ b/src/Tests/RemoteFactory.IntegrationTests/TestTargets/Events/FactoryEventPhaseEntryTargets.cs @@ -33,6 +33,10 @@ public record PhasedRelayChainEvent(Guid Id) : FactoryEventBase; public record PhasedRelayOutEvent(Guid Id) : FactoryEventBase; public record PhasedUntypedRemoteEvent(Guid Id) : FactoryEventBase; public record PhasedCancelAfterSuccessEvent(Guid Id) : FactoryEventBase; +public record PhasedAspForbidEvent(Guid Id) : FactoryEventBase; +public record PhasedNestedSaveEvent(Guid Id) : FactoryEventBase; +public record PhasedInterfaceEvent(Guid Id) : FactoryEventBase; +public record PhasedSyncEntryEvent(Guid Id) : FactoryEventBase; // ----------------------------------------------------------------------------- // HANDLER MARKER CLASSES — registration keys only; the invokers are lambdas in @@ -163,6 +167,38 @@ private static bool RegisterAll() return Task.CompletedTask; }); + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedAfterCommitMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("aspforbid-after-commit", ((PhasedAspForbidEvent)evt).Id); + return Task.CompletedTask; + }); + + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedAfterCommitMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("nested-after-commit", ((PhasedNestedSaveEvent)evt).Id); + return Task.CompletedTask; + }); + + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedAfterCommitMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("interface-after-commit", ((PhasedInterfaceEvent)evt).Id); + return Task.CompletedTask; + }); + + FactoryEventHandlerRegistry.RegisterHandler( + typeof(PhasedAfterCommitMarker), DispatchPhase.AfterCommit, + (sp, evt, _, _) => + { + sp.GetRequiredService().RecordEventFired("sync-after-commit", ((PhasedSyncEntryEvent)evt).Id); + return Task.CompletedTask; + }); + return true; } } @@ -301,6 +337,23 @@ internal static async Task _RunForbiddenInner( return id; } + /// + /// Enqueues phased work, then throws — the denial + /// shape the choke point converts to a success-shaped empty response. It must clear, + /// never drain, despite that return shape. (The exception type is public in the core + /// package; only its producers live in the AspNetCore package.) + /// + [Execute] + [Remote] + internal static async Task _RunAspForbid( + Guid id, + [Service] IFactoryEvents events, + CancellationToken ct) + { + await events.Raise(new PhasedAspForbidEvent(id), RaiseOptions.None, ct); + throw new AspForbidException($"phased entry forbidden for {id}"); + } + /// /// Enqueues phased work, then cancels the request token — the call itself has /// already succeeded, so the entry drain must still run in full. @@ -330,6 +383,104 @@ public sealed class PhaseCancellationTrigger public void Cancel() => OnCancel?.Invoke(); } +/// +/// Child entity for the nested-save discriminator: the parent's Insert saves this child +/// through its factory (a nested entry), then records a marker. A drain firing at the +/// child's completion instead of the parent's lands between the two markers. +/// +[Factory] +public partial class PhaseChildTarget : IFactorySaveMeta +{ + public Guid Id { get; set; } + public bool IsDeleted { get; set; } + public bool IsNew { get; set; } = true; + + [Insert] + internal Task Insert([Service] IEventTestService testService) + { + testService.RecordEventFired("child-insert-done", Id); + IsNew = false; + return Task.CompletedTask; + } +} + +/// +/// Parent whose Insert raises a phased event, saves a child through the child's factory, +/// and then records "parent-after-child" — the inner-vs-outer drain discriminator. +/// +[Factory] +public partial class PhaseParentTarget : IFactorySaveMeta +{ + public Guid Id { get; set; } + public bool IsDeleted { get; set; } + public bool IsNew { get; set; } = true; + + [Insert] + internal async Task Insert( + [Service] IFactoryEvents events, + [Service] IPhaseChildTargetFactory childFactory, + [Service] IEventTestService testService, + CancellationToken ct) + { + await events.Raise(new PhasedNestedSaveEvent(Id), RaiseOptions.None, ct); + await childFactory.Save(new PhaseChildTarget { Id = Id }, ct); + testService.RecordEventFired("parent-after-child", Id); + IsNew = false; + } +} + +/// +/// Interface factory whose implementation raises a phased event — success-path drain +/// coverage for the interface leg, as the OUTERMOST entry (resolved directly server-side, +/// depth 1, unlike the always-nested position behind the choke point). +/// +[Factory] +public interface IPhaseAuditService +{ + Task AuditPhased(Guid id); +} + +/// Named to match IPhaseAuditService for RegisterMatchingName. +public class PhaseAuditService : IPhaseAuditService +{ + private readonly IFactoryEvents _events; + private readonly IEventTestService _testService; + + public PhaseAuditService(IFactoryEvents events, IEventTestService testService) + { + _events = events; + _testService = testService; + } + + public async Task AuditPhased(Guid id) + { + await _events.Raise(new PhasedInterfaceEvent(id)); + _testService.RecordEventFired("interface-method-done", id); + return id; + } +} + +/// +/// Sync (non-Task) factory method that enqueues phased work via a blocking Raise — +/// the generated Run route (value-object shape) with pending deferred work. +/// +[Factory] +public partial class PhaseSyncTarget +{ + public Guid Id { get; set; } + + [Create] + internal void Create( + Guid id, + [Service] IFactoryEvents events, + [Service] IEventTestService testService) + { + Id = id; + events.Raise(new PhasedSyncEntryEvent(id)).GetAwaiter().GetResult(); + testService.RecordEventFired("sync-method-done", id); + } +} + /// Authorization that always denies reads. public class PhaseDenyAuth { diff --git a/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs b/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs index 2f670687..2f876a92 100644 --- a/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs +++ b/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs @@ -456,6 +456,58 @@ public interface IMyService Assert.Contains("[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::TestNamespace.MyServiceFactory))]", generatedSource); } + /// + /// The interface leg's Local* methods split into a NON-async guarded wrapper + /// forwarding through the entry-call helper to a private, unguarded core. + /// + /// + /// Introduced by PHASE-003 — this leg previously emitted the guard inline on the + /// method itself with a conditional async keyword, the shape TRIM-009 + /// measured as guard-inside-MoveNext on the class leg whenever the method + /// went async. Body elimination on the interface leg is still UNVERIFIED (TRIM + /// Deferred Work item 20: the single-method registrar holder half of the fix is + /// absent here), so this emission pin is the only obtainable evidence that the + /// guard sits in a non-async wrapper. An async keyword appearing on the + /// wrapper must go red here. + /// + [Fact] + public void InterfaceFactory_GuardedLocalMethod_SplitsIntoSyncWrapperAndCore() + { + var source = @" +using Neatoo.RemoteFactory; + +namespace TestNamespace +{ + [Factory] + public interface IMyService + { + Task DoWork(string input); + } +} +"; + var (_, _, runResult) = DiagnosticTestHelper.RunGenerator(source); + + var generatedSource = runResult.GeneratedTrees + .FirstOrDefault(t => t.FilePath.Contains("MyServiceFactory")) + ?.GetText() + ?.ToString(); + + Assert.NotNull(generatedSource); + + // Wrapper: NOT async, carries the guard, forwards through the entry-call helper. + Assert.Matches( + @"public Task LocalDoWork\([^)]*\)\s*\{\s*if \(!NeatooRuntime\.IsServerRuntime\)", + generatedSource); + Assert.DoesNotContain("public async Task LocalDoWork(", generatedSource); + Assert.Contains("return FactoryEntryCall.RunAsync(ServiceProvider, () => LocalDoWorkCore(", generatedSource); + + // Core: private and unguarded (this fixture's body forwards the target's task + // without awaiting, so no async keyword; the guard already ran in the wrapper). + Assert.Matches( + @"private (async )?Task LocalDoWorkCore\([^)]*\)\s*\{\s*(?!\s*if \(!NeatooRuntime\.IsServerRuntime\))", + generatedSource); + } + #endregion #region Relay Handler diff --git a/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEntryCallTests.cs b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEntryCallTests.cs index 12583f9b..cb8f58b0 100644 --- a/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEntryCallTests.cs +++ b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEntryCallTests.cs @@ -188,16 +188,225 @@ public async Task RunAsync_NoSchedulerInScope_JustRunsTheBody() [Fact] public async Task EndEntryCall_WithoutBegin_ThrowsOnTheSuccessPath() { + var dispatched = new List(); + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, + (_, _, _, _) => { dispatched.Add("deferred"); return Task.CompletedTask; }); + var (provider, scope) = ServerScope(); using (provider) using (scope) { - var scheduler = scope.ServiceProvider.GetRequiredService(); + var sp = scope.ServiceProvider; + var scheduler = sp.GetRequiredService(); await Assert.ThrowsAsync(() => scheduler.EndEntryCallAsync(success: true)); - // The failure path runs inside catch blocks and must never throw. + // The failure path runs inside catch blocks and must never throw — and it + // must not corrupt depth (no negative depth, no phantom entry). await scheduler.EndEntryCallAsync(success: false); + Assert.False(scheduler.IsEntryCallActive); + + // A subsequent normal entry cycle still works. + var events = sp.GetRequiredService(); + await FactoryEntryCall.RunAsync(sp, async () => + { + await events.Raise(new MisuseRecoveryEvent("x")); + return 0; + }); + Assert.Equal(["deferred"], dispatched); } } + + [Fact] + public async Task HandlerThrowsOperationCanceled_MidDrain_EntryExitStillClearsAndDepthSurvives() + { + // The one exit path where EndEntryCallAsync(true) itself throws: a handler's own + // OperationCanceledException aborts the drain (scheduler contract), the helper's + // catch then calls EndEntryCallAsync(false) — a second End for one Begin, safe by + // the depth tolerance — and "between entry calls the scheduler is empty" must + // still hold on the way out. + var dispatched = new List(); + FactoryEventHandlerRegistry.RegisterHandler(typeof(PhaseOceThrower), DispatchPhase.AfterCommit, + (_, _, _, _) => throw new OperationCanceledException()); + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, + (_, _, _, _) => { dispatched.Add("behind-the-oce"); return Task.CompletedTask; }); + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, + (_, _, _, _) => { dispatched.Add("recovery"); return Task.CompletedTask; }); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var sp = scope.ServiceProvider; + var events = sp.GetRequiredService(); + var scheduler = sp.GetRequiredService(); + + await Assert.ThrowsAsync(() => + FactoryEntryCall.RunAsync(sp, async () => + { + await events.Raise(new OceThrowingEvent("x")); + return 0; + })); + + // The aborted drain's leftover was cleared (a clear, never a drain) and the + // entry state fully released. + Assert.Empty(dispatched); + Assert.False(scheduler.HasPending); + Assert.False(scheduler.IsEntryCallActive); + + // The scope is reusable: a fresh entry drains only its own work. + await FactoryEntryCall.RunAsync(sp, async () => + { + await events.Raise(new OceRecoveryEvent("y")); + return 0; + }); + Assert.Equal(["recovery"], dispatched); + } + } + + [Fact] + public async Task NestedEntryFails_OuterCatchesAndSucceeds_TheEntryStillDrains() + { + // A nested factory call failing is not the entry failing: only the OUTERMOST + // exit decides drain-vs-clear. When the outer body catches the inner failure and + // completes, the entry succeeded — everything still queued (including what the + // failed inner section enqueued before throwing) drains with it. Pinned so an + // "always clear on any failure" refactor goes red here. + var dispatched = new List(); + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, + (_, _, _, _) => { dispatched.Add("from-inner"); return Task.CompletedTask; }); + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, + (_, _, _, _) => { dispatched.Add("from-outer"); return Task.CompletedTask; }); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var sp = scope.ServiceProvider; + var events = sp.GetRequiredService(); + + await FactoryEntryCall.RunAsync(sp, async () => + { + try + { + await FactoryEntryCall.RunAsync(sp, async () => + { + await events.Raise(new CaughtInnerEvent("x")); + throw new InvalidOperationException("inner fails"); + }); + } + catch (InvalidOperationException) + { + // The outer entry chooses to continue. + } + + await events.Raise(new CaughtOuterEvent("y")); + return 0; + }); + + Assert.Equal(["from-inner", "from-outer"], dispatched); + } + } + + [Fact] + public async Task DrainedHandlerInvokingAFactory_NestsWithoutDrainingOrClearingTheDrainInProgress() + { + // Realistic projection-handler shape: an AfterCommit handler writes through a + // factory. That factory call is a NESTED entry (the entry is still active during + // the drain), so it must neither drain mid-drain nor throw — and phased work it + // raises joins the drain in progress. + var dispatched = new List(); + FactoryEventHandlerRegistry.RegisterHandler(typeof(PhaseFactoryCallingHandler), DispatchPhase.AfterCommit, + async (sp, _, _, _) => + { + dispatched.Add("handler-start"); + await FactoryEntryCall.RunAsync(sp, async () => + { + await sp.GetRequiredService().Raise(new HandlerFactoryFollowUpEvent("chained")); + return 0; + }); + dispatched.Add("handler-after-factory"); + }); + FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, + (_, _, _, _) => { dispatched.Add("follow-up"); return Task.CompletedTask; }); + + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var sp = scope.ServiceProvider; + var events = sp.GetRequiredService(); + + await FactoryEntryCall.RunAsync(sp, async () => + { + await events.Raise(new HandlerFactoryCallEvent("x")); + return 0; + }); + + // The nested factory call completed inside the handler without draining + // (follow-up runs after the handler, from the drain in progress). + Assert.Equal(["handler-start", "handler-after-factory", "follow-up"], dispatched); + } + } + + [Fact] + public void Run_NoSchedulerInScope_JustRunsTheBody() + { + // The sync mirror of RunAsync_NoSchedulerInScope — B-C3's client-reachable + // unguarded shape (value-object factories) reduced to the body. + var services = new ServiceCollection(); + using var provider = services.BuildServiceProvider(); + + var result = FactoryEntryCall.Run(provider, () => 7); + + Assert.Equal(7, result); + } + + [Fact] + public async Task ConcurrentFlowsInOneScope_ShareEntryState_FailedFlowsWorkRidesTheSurvivingDrain() + { + // DOCUMENTED LIMITATION (plan review B-C2 follow-through): entry tracking is + // per-SCOPE, not per-flow. Two concurrent flows in one scope interleave on the + // same depth counter, so a failed flow's exit is a nested exit (no clear) and + // the surviving flow's outermost drain runs BOTH flows' queued work. Scopes are + // the framework's isolation unit — concurrent flows sharing a scope already + // share DbContexts and every other scoped service. This test pins the actual + // semantics so a change to them is a conscious one. + var dispatched = new List(); + var (provider, scope) = ServerScope(); + using (provider) + using (scope) + { + var scheduler = scope.ServiceProvider.GetRequiredService(); + + scheduler.BeginEntryCall(); // flow A + scheduler.BeginEntryCall(); // flow B (interleaved) + scheduler.Enqueue(DispatchPhase.AfterCommit, new FlowAEvent("a"), RaiseOptions.None, + (_, _, _, _) => { dispatched.Add("flow-a"); return Task.CompletedTask; }); + scheduler.Enqueue(DispatchPhase.AfterCommit, new FlowBEvent("b"), RaiseOptions.None, + (_, _, _, _) => { dispatched.Add("flow-b"); return Task.CompletedTask; }); + + await scheduler.EndEntryCallAsync(success: false); // flow A fails — nested exit, no clear + Assert.True(scheduler.HasPending); + + await scheduler.EndEntryCallAsync(success: true); // flow B succeeds — drains both + + Assert.Equal(["flow-a", "flow-b"], dispatched); + Assert.False(scheduler.HasPending); + Assert.False(scheduler.IsEntryCallActive); + } + } + + private sealed record MisuseRecoveryEvent(string Value) : FactoryEventBase; + private sealed record OceThrowingEvent(string Value) : FactoryEventBase; + private sealed record OceRecoveryEvent(string Value) : FactoryEventBase; + private sealed record CaughtInnerEvent(string Value) : FactoryEventBase; + private sealed record CaughtOuterEvent(string Value) : FactoryEventBase; + private sealed record HandlerFactoryCallEvent(string Value) : FactoryEventBase; + private sealed record HandlerFactoryFollowUpEvent(string Value) : FactoryEventBase; + private sealed record FlowAEvent(string Value) : FactoryEventBase; + private sealed record FlowBEvent(string Value) : FactoryEventBase; + + private sealed class PhaseOceThrower { } + private sealed class PhaseFactoryCallingHandler { } } diff --git a/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs index f84374d1..04d83fb8 100644 --- a/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs +++ b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs @@ -1,4 +1,5 @@ using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using Neatoo.RemoteFactory; using Neatoo.RemoteFactory.Internal; @@ -224,24 +225,56 @@ public async Task Raise_PhasedHandlerOutsideAnyFactoryCall_DispatchesImmediately { // A scheduler exists in the scope, but no entry factory call is active — the // "Raise outside any factory call" case. The phased handler dispatches - // immediately (with a debug log) instead of queueing into a drain nobody owns. + // immediately, with the 9005 debug log positively pinned here (an absence + // assertion elsewhere cannot pin an emission). lock (Dispatched) { Dispatched.Clear(); } FactoryEventHandlerRegistry.RegisterHandler(typeof(DeferredHandler), DispatchPhase.AfterCommit, Recording("deferred")); - var (provider, scope) = ServerScope(); - using (provider) - using (scope) + var capture = new CapturingProvider(); + var services = new ServiceCollection(); + services.AddLogging(b => b.AddProvider(capture).SetMinimumLevel(Microsoft.Extensions.Logging.LogLevel.Trace)); + services.AddNeatooRemoteFactory(NeatooFactory.Server, typeof(FactoryEventsDispatcherPhaseTests).Assembly); + using var provider = services.BuildServiceProvider(); + using var scope = provider.CreateScope(); + + var events = scope.ServiceProvider.GetRequiredService(); + var queue = scope.ServiceProvider.GetRequiredService(); + + await events.Raise(new OutsideEntryEvent("x")); + + lock (Dispatched) { - var events = scope.ServiceProvider.GetRequiredService(); - var queue = scope.ServiceProvider.GetRequiredService(); + Assert.Equal(["deferred"], Dispatched); + } + Assert.False(queue.HasPending); + lock (capture.Entries) + { + Assert.Contains(capture.Entries, e => + e.EventId == 9005 && e.Level == Microsoft.Extensions.Logging.LogLevel.Debug); + } + } - await events.Raise(new OutsideEntryEvent("x")); + private sealed class CapturingProvider : Microsoft.Extensions.Logging.ILoggerProvider + { + public List<(int EventId, Microsoft.Extensions.Logging.LogLevel Level)> Entries { get; } = []; - lock (Dispatched) + public Microsoft.Extensions.Logging.ILogger CreateLogger(string categoryName) => new CapturingLogger(this); + + public void Dispose() { } + + private sealed class CapturingLogger(CapturingProvider owner) : Microsoft.Extensions.Logging.ILogger + { + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(Microsoft.Extensions.Logging.LogLevel logLevel) => true; + + public void Log(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, TState state, Exception? exception, Func formatter) { - Assert.Equal(["deferred"], Dispatched); + lock (owner.Entries) + { + owner.Entries.Add((eventId.Id, logLevel)); + } } - Assert.False(queue.HasPending); } } @@ -317,11 +350,16 @@ public async Task Raise_DeferredHandler_StillCollectsForRelayAtRaiseTime() { var events = scope.ServiceProvider.GetRequiredService(); var collector = scope.ServiceProvider.GetRequiredService(); + var queue = scope.ServiceProvider.GetRequiredService(); + // Entry active so the handler genuinely DEFERS — collection at raise time + // is only meaningful while the dispatch hasn't happened yet. + queue.BeginEntryCall(); await events.Raise(new RelayCollectionEvent("x")); var collected = Assert.Single(collector.GetCollectedEvents()); Assert.IsType(collected); + Assert.True(queue.HasPending); } } @@ -336,7 +374,10 @@ public async Task Raise_DeferredHandlerWithServerOnly_IsNotCollectedForRelay() { var events = scope.ServiceProvider.GetRequiredService(); var collector = scope.ServiceProvider.GetRequiredService(); + var queue = scope.ServiceProvider.GetRequiredService(); + // Entry active — see Raise_DeferredHandler_StillCollectsForRelayAtRaiseTime. + queue.BeginEntryCall(); await events.Raise(new RelayCollectionEvent("x"), RaiseOptions.ServerOnly); Assert.Empty(collector.GetCollectedEvents()); From aa3a0ac66168b5ba8fd74533c6b86a232be28933 Mon Sep 17 00:00:00 2001 From: Keith Voels Date: Fri, 14 Aug 2026 21:52:33 -0500 Subject: [PATCH 09/10] test: close PHASE-003 test-review round 2 (S1 forbid clear-half, S2 premise pin) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gate closed: unit 668x2, integration 579x2, Design 86x2 — 0 failures. Co-Authored-By: Claude Fable 5 --- .../plans/003-aftercommit-entry-call-drain.md | 2 +- .../reviews/003-test-review.md | 48 +++++++++++++++++-- .../todos/PHASE-phased-event-dispatch/todo.md | 8 +++- .../Phases/FactoryEventPhaseEntryTests.cs | 8 ++++ .../FactoryEventsDispatcherPhaseTests.cs | 2 + 5 files changed, 62 insertions(+), 6 deletions(-) diff --git a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md index 2054ba92..62b3ab53 100644 --- a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md +++ b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md @@ -441,7 +441,7 @@ both restored with `BeginEntryCall` (the failure mode pre-declaration cannot cat | Must 3: interface-renderer emission unpinned | `InterfaceFactory_GuardedLocalMethod_SplitsIntoSyncWrapperAndCore` | unit | | Should 4: nesting tests didn't discriminate | `NestedChildSave_DoesNotDrainAtTheChildsCompletion` | integration | | Should 5: post-OCE clear + double-End tolerance unpinned | `HandlerThrowsOperationCanceled_MidDrain_EntryExitStillClearsAndDepthSurvives` | unit | -| Should 6: relay-collection premise vacuous | Both tests restored with `BeginEntryCall` + `HasPending` assertion | unit | +| Should 6: relay-collection premise vacuous | Both tests restored with `BeginEntryCall`; the `HasPending` premise assertion initially landed in only one — round 2 caught the omission and both now carry it | unit | | Should 7: 9005 emission unpinned | Positive Debug 9005 assertion added to the outside-entry test | unit | | Should 8: caught-nested-failure + handler-invokes-factory | `NestedEntryFails_OuterCatchesAndSucceeds_TheEntryStillDrains`, `DrainedHandlerInvokingAFactory_NestsWithoutDrainingOrClearingTheDrainInProgress` | unit | | Should 9: interface success-path + generated sync shape | `InterfaceFactory_AsTheOutermostEntry_DrainsOnSuccess`, `SyncFactoryMethod_WithPendingPhasedWork_BlockDrainsAtCompletion` | integration | diff --git a/docs/todos/PHASE-phased-event-dispatch/reviews/003-test-review.md b/docs/todos/PHASE-phased-event-dispatch/reviews/003-test-review.md index 6234824a..10c0d9e7 100644 --- a/docs/todos/PHASE-phased-event-dispatch/reviews/003-test-review.md +++ b/docs/todos/PHASE-phased-event-dispatch/reviews/003-test-review.md @@ -74,6 +74,48 @@ relay-collection restorations). **Post-closure totals:** unit 668×2, integration 579×2 (+5 skipped), Design 86×2 — 0 failures. Logs regenerated. -## Round 2 — 2026-08-14 - -*(appended after re-review)* +## Round 2 — 2026-08-14 (re-review after the add-tests loop) + +All 3 must-cover and all 6 should-cover round-1 findings **verified closed** by reading +the closing tests against production: each pins what the disposition claims, at the +right tier, and would go red on the regression it names. Both nice-to-haves closed. The +closure commit (`595d195`) touched only tests and docs — no production code was reshaped +to fit a test — and the red-proofed tests are byte-identical to their state when +`003-redproof.log` was captured, so reusing that log is valid. Suite arithmetic checks +out (+6 unit, +4 integration → 668×2 / 579×2 +5 / 86×2, 0 failures; build warnings all +pre-existing and unrelated). + +Two new **should-cover** findings, both one-liners, neither invalidating a closed must: + +1. `AspForbidException_AfterEnqueueingPhasedWork_ClearsWithoutDraining` asserted only the + "without draining" half — a forbid route that skipped `EndEntryCallAsync(false)` + would leave a long-lived scope at depth ≥ 1 and silently kill every subsequent drain + while staying green. +2. `Raise_DeferredHandlerWithServerOnly_IsNotCollectedForRelay` received + `BeginEntryCall()` but not the `HasPending` premise assertion its sibling got — the + exact round-1 vacating failure mode, and the Gate-closure row overstated by one test. + +Nice-to-have: the concurrent-flows pin covers only the enqueue-before-either-exits +interleaving; the enqueue-during-the-survivor's-drain window is timing-dependent +(joins the drain or is cleared) and was unrecorded. Also noted: the OCE mid-drain test +depends on registry handler order (fails loudly, not falsely), and the forbidden-path +tests asserted consequence without premise. + +Open-by-choice items and tech-debt routing re-checked and found honestly recorded. + +### Round 2 disposition (orchestrator) + +- **S1 closed:** the AspForbid test now follows the forbidden call with a successful + `Create` in the same server scope and asserts that call's full drain — a stuck depth + fails it. +- **S2 closed:** `HasPending` premise assertion added to the ServerOnly sibling; + Gate-closure row corrected to record the two-round history honestly. +- **N1 recorded:** the Discovery Log's concurrent-flows entry now names the + enqueue-during-drain window as inherent to per-scope granularity (not pinned — same + documented-limitation posture). +- N2/N3 noted, no action: the OCE order dependence fails loudly; the forbidden-path + premise is now indirectly asserted by S1's follow-on drain. + +**Gate closed.** Final totals: unit 668×2, integration 579×2 +5 skipped, Design 86×2 — +0 failures (both round-2 closures strengthened existing tests rather than adding new +ones; logs regenerated). diff --git a/docs/todos/PHASE-phased-event-dispatch/todo.md b/docs/todos/PHASE-phased-event-dispatch/todo.md index 0634be40..197175d2 100644 --- a/docs/todos/PHASE-phased-event-dispatch/todo.md +++ b/docs/todos/PHASE-phased-event-dispatch/todo.md @@ -91,8 +91,12 @@ exposes drain points. concurrent flows sharing a scope already share DbContexts and every scoped service. The actual semantics are pinned (`ConcurrentFlowsInOneScope_ShareEntryState_FailedFlowsWorkRidesTheSurvivingDrain`) - so any change to them is a conscious one. Per-flow tracking (AsyncLocal) would be a - design change with its own hazards — revisit only if a real consumer hits this. + so any change to them is a conscious one. A second window exists and is recorded but + not pinned (round-2 N1): work a concurrent flow enqueues *while* the survivor's + outermost drain is running either joins that drain or is discarded by the post-drain + clear, depending on timing — inherent to the same per-scope granularity. Per-flow + tracking (AsyncLocal) would be a design change with its own hazards — revisit only if + a real consumer hits this. - **Follow-up:** PHASE-005 documents "one factory call per scope at a time" as the concurrency guidance. diff --git a/src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs b/src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs index c63311e0..59cf39d3 100644 --- a/src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs +++ b/src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs @@ -266,6 +266,14 @@ public async Task AspForbidException_AfterEnqueueingPhasedWork_ClearsWithoutDrai Assert.Equal(Guid.Empty, returned); Assert.Empty(RecordedFor(server, id)); + + // The clear half (round-2 S1): the forbid exit must release entry state, not + // just skip the drain. A forbid route that skipped EndEntryCallAsync(false) + // would leave this long-lived server scope at depth >= 1 and silently kill + // every subsequent drain — so a follow-on success in the same scope must drain. + var successId = Guid.NewGuid(); + await client.GetRequiredService().Create(successId); + Assert.Equal(["immediate", "create-method-done", "after-commit"], RecordedFor(server, successId)); } [Fact] diff --git a/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs index 04d83fb8..e7d8bb2a 100644 --- a/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs +++ b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs @@ -381,6 +381,8 @@ public async Task Raise_DeferredHandlerWithServerOnly_IsNotCollectedForRelay() await events.Raise(new RelayCollectionEvent("x"), RaiseOptions.ServerOnly); Assert.Empty(collector.GetCollectedEvents()); + // The deferral premise, asserted so it cannot vacate silently again. + Assert.True(queue.HasPending); } } } From f6866dd8cb2cde5964998ce1659be5ee23dd173f Mon Sep 17 00:00:00 2001 From: Keith Voels Date: Fri, 14 Aug 2026 22:18:32 -0500 Subject: [PATCH 10/10] fix: interface-leg registrar holder + entry-call review fixes (PHASE-003 code review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit V1: interface factories now emit a single-method registrar holder (NeatooInterfaceFactoryRegistrar_ prefix) and point the assembly attribute at it — the Local*Core split no longer sits in the DAM-roots-everything shape TRIM-009 measured as insufficient. C1: EndEntryCallAsync collapsed to one lock acquisition. C4: 9006 renamed FactoryEventPhaseDiscardedAtExit with a cause- neutral message. C9: FactoryEntryCall emitted global::-qualified. C3: sync block-drain deadlock caveat documented. C10: stale guard comments fixed; RFEF substrate + TRIM direction recorded in the Discovery Log. PHASE-003 Done. Unit 668x2, integration 579x2, Design 86x2 — 0 failures. Co-Authored-By: Claude Fable 5 --- .../plans/003-aftercommit-entry-call-drain.md | 26 +++++++++- .../plans/004-afterflush-coordinator.md | 14 ++++++ .../reviews/003-code-review.md | 47 +++++++++++++++++++ .../todos/PHASE-phased-event-dispatch/todo.md | 28 ++++++++++- src/Design/CLAUDE-DESIGN.md | 2 +- .../Renderer/ClassFactoryRenderer.cs | 10 ++-- .../Renderer/InterfaceFactoryRenderer.cs | 38 +++++++++++++-- .../Renderer/StaticFactoryRenderer.cs | 2 +- .../Internal/FactoryEntryCall.cs | 8 ++++ .../Internal/FactoryEventPhaseScheduler.cs | 16 +++---- src/RemoteFactory/Internal/Log.cs | 4 +- .../AssemblyAttributeEmissionTests.cs | 29 +++++++++--- 12 files changed, 194 insertions(+), 30 deletions(-) create mode 100644 docs/todos/PHASE-phased-event-dispatch/reviews/003-code-review.md diff --git a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md index 62b3ab53..b28a2cb6 100644 --- a/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md +++ b/docs/todos/PHASE-phased-event-dispatch/plans/003-aftercommit-entry-call-drain.md @@ -3,7 +3,7 @@ **Plan #:** 003 **Date:** 2026-08-14 **Related Todo:** [../todo.md](../todo.md) -**Status:** In Progress +**Status:** Done **Last Updated:** 2026-08-14 **Plan-review opt-in:** Yes (touches all three factory renderers; entry-shape subtleties found at recon make this the riskiest plan) **Code-review opt-in:** Yes (behavior-changing across generated code and runtime) @@ -488,6 +488,19 @@ pending in a sync entry is a fire-and-forget `Raise` inside a synchronous factor method, and no-silent-loss outranks non-blocking there. With nothing pending the completion is fully synchronous. +### 2026-08-14 — Code-review fixes (V1, C1, C4, C9) + +The interface leg gained the single-method registrar holder +(`NeatooInterfaceFactoryRegistrar_` prefix; assembly attribute repointed) so the new +`Local*Core` split does not sit in the DAM-roots-everything configuration TRIM-009 +measured as insufficient — see the Discovery Log entry. `EndEntryCallAsync`'s +check-then-decrement collapsed into one lock acquisition (C1: the two-acquisition window +could reach depth 0 having neither drained nor cleared). Log 9006 renamed +`FactoryEventPhaseDiscardedAtExit` with a cause-neutral message (C4: the shared clear +also runs on the post-OCE success-path cleanup). `FactoryEntryCall` is emitted +`global::`-qualified (C9). Full findings + dispositions: +[../reviews/003-code-review.md](../reviews/003-code-review.md). + ### 2026-08-14 — Post-OCE clear at the entry drain If the entry drain itself throws (a handler's own `OperationCanceledException`), the @@ -507,3 +520,14 @@ drain — so "between entry calls the scheduler is empty" holds on every exit pa 2026-08-14). Tests here register phased handlers via the registry's 3-arg overload; PHASE-002 later makes the attribute's phase argument flow end-to-end and owns the duplicate-registration diagnostic decision. +- **Recorded limitations (code review C5–C8, accepted):** every factory call now pays a + closure + delegate + (on await) a state machine, and the client-reachable value-object + `Run` route pays one null `GetService` lookup per call — unmeasured (the only perf + suite is skipped); a `ref`/`in`/`out`/ref-struct factory parameter would fail to + compile in generated code under the lambda capture (no diagnostic yet — if the shape + compiles today on `main`, a generator diagnostic warrants its own Draft row); the + `Local{X}Core` name-collision surface widened to all factory methods; static-leg + delegates resolved from the root provider now touch a scoped service and throw under + `ValidateScopes` (delegates are meant to be scope-resolved). OCE-from-a-drained- + handler still fails an already-succeeded call (chartered by the todo AC); whether a + post-completion drain should swallow OCE too is handed to PHASE-004. diff --git a/docs/todos/PHASE-phased-event-dispatch/plans/004-afterflush-coordinator.md b/docs/todos/PHASE-phased-event-dispatch/plans/004-afterflush-coordinator.md index 1b1e5f3c..2dbd0db0 100644 --- a/docs/todos/PHASE-phased-event-dispatch/plans/004-afterflush-coordinator.md +++ b/docs/todos/PHASE-phased-event-dispatch/plans/004-afterflush-coordinator.md @@ -35,4 +35,18 @@ coordinator is a drain trigger, nothing more. scheduler with `inTransaction: true` so handler exceptions propagate and the consumer's transaction can still roll back. +--- + +## Inherited from PHASE-003 (recorded at its Step 5 gates) + +- **Open question this plan owns (code review C2):** a drained handler's + `OperationCanceledException` at the *AfterCommit entry drain* propagates and fails a + call that already succeeded — chartered by the todo's AC ("OCE still propagates") but + in tension with the no-token entry-drain policy. Decide here, as the owner of the + consumer-facing drain surface, whether post-completion drains should swallow OCE too + (and if so, restate the AC as a planned amendment). +- The entry drain passes `CancellationToken.None`; the coordinator's `AfterFlush` drain + is in-transaction and consumer-invoked, so it takes the consumer's token — the two + drain points deliberately differ. + *(Stub — Intent, Alignment, remaining Constraints, Steps, Acceptance filled at Step 2.)* diff --git a/docs/todos/PHASE-phased-event-dispatch/reviews/003-code-review.md b/docs/todos/PHASE-phased-event-dispatch/reviews/003-code-review.md new file mode 100644 index 00000000..a9b8f3bb --- /dev/null +++ b/docs/todos/PHASE-phased-event-dispatch/reviews/003-code-review.md @@ -0,0 +1,47 @@ +# PHASE-003 Code Review (per-plan, findings-only) — 2026-08-14 + +**Plan:** [../plans/003-aftercommit-entry-call-drain.md](../plans/003-aftercommit-entry-call-drain.md) +**Verdict shape:** 1 veto (V1), 11 callouts (C1–C11). Build/test/red-proof logs verified +by the reviewer (2666 passing, red-proofs genuine, backward-compat claim confirmed). + +## Veto + +**V1 — Interface leg's wrapper split moved previously-eliminable bodies into a +DAM-rooted, unguarded core.** The assembly attribute pointed at `{Impl}Factory` (hosting +every `Local*`), whose `[DynamicallyAccessedMembers(PublicMethods | NonPublicMethods)]` +roots the new private cores with their bodies — the configuration TRIM-009 *measured* as +insufficient on the class leg. The plan's "TRIM item 20 status unchanged" was honest +about ignorance but silent about direction. +**Disposition: FIXED** — the interface leg now emits a single-method registrar holder +(`NeatooInterfaceFactoryRegistrar_` prefix, distinct per the item-15 convention) and the +assembly attribute points at it, aligning all three legs on the measured-good shape. +`InterfaceFactory_EmitsAssemblyAttribute` amended to pin the holder + the +does-not-name-the-factory regression assertion (intent preserved: the attribute names +the correct type; the correct type changed). Elimination on this leg remains UNVERIFIED +(TRIM item 20 — the fixture is still blocked by item 19), but the shape is no longer the +measured-bad one. Recorded in the Discovery Log. + +## Callouts and dispositions + +| # | Finding | Disposition | +|---|---|---| +| C1 | `EndEntryCallAsync` check-then-decrement spanned two lock acquisitions — an interleaving could reach depth 0 having neither drained nor cleared | **Fixed** — single lock block | +| C2 | A handler's OCE at the entry drain fails an already-succeeded call (chartered by the todo's AC, but in tension with the no-token rationale) | **Recorded**; the "swallow OCE at a post-completion drain?" question handed to PHASE-004 (its stub carries it) | +| C3 | Sync block-drain can deadlock under a captured SynchronizationContext | **Fixed (docs)** — caveat added to `FactoryEntryCall.Run` XML; PHASE-005 carries the guidance | +| C4 | 9006 asserted a cause (`did not complete successfully`) the shared `ClearAtExit` cannot guarantee (also fires on post-OCE success-path cleanup) | **Fixed** — renamed `FactoryEventPhaseDiscardedAtExit`, message + CLAUDE-DESIGN row reworded | +| C5 | Every factory call now pays closure + delegate + state machine; value-object `Create` pays a null `GetService` per call client-side; the only perf suite is skipped | **Recorded** (plan Notes); revisit on consumer report | +| C6 | `ref`/`in`/`out`/ref-struct factory parameters would now fail to compile in generated code (lambda capture); no diagnostic | **Recorded** (plan Notes) — if the shape turns out to compile today, a generator diagnostic gets its own Draft row | +| C7 | `Local{X}Core` name-collision surface widened (a user `FetchCore` factory method beside `Fetch`) | **Recorded** (plan Notes) | +| C8 | Static-leg delegates resolved from the root provider now touch a scoped service (`ValidateScopes` throws) | **Recorded** (plan Notes) — delegates are meant to be scope-resolved | +| C9 | `FactoryEntryCall` emitted unqualified (CS0104 risk against consumer types) | **Fixed** — `global::`-qualified in all three renderers; emission pins updated | +| C10 | (a) Cross-plan outcomes unrecorded for RFEF and TRIM; (b) four stale "inside guard" comments | **Fixed** — two Discovery Log entries added; comments corrected | +| C11 | `Can*` authorization probes are now entry calls (harmless empty drain today; under RFEF a read-only probe would open a transaction) | **Recorded** in the RFEF-substrate Discovery Log entry | + +Checked-and-clear list from the reviewer retained in full in the agent transcript; +highlights: AspForbid reaches `End(false)` before the success-shaped return; handlers +invoked outside the lock (no re-entrant deadlock); both choke-point registrations pass +the scoped provider; every Test Evidence citation exists at its declared tier (19/19 +spot-checked). + +**Post-fix suites:** unit 668×2, integration 579×2 +5 skipped, Design 86×2 — 0 failures +(logs regenerated). diff --git a/docs/todos/PHASE-phased-event-dispatch/todo.md b/docs/todos/PHASE-phased-event-dispatch/todo.md index 197175d2..591c92af 100644 --- a/docs/todos/PHASE-phased-event-dispatch/todo.md +++ b/docs/todos/PHASE-phased-event-dispatch/todo.md @@ -72,7 +72,7 @@ exposes drain points. |---|------|-------|--------| | 001 | [001-phase-model-and-queueing](./plans/001-phase-model-and-queueing.md) | DispatchPhase enum, registry phase, dispatcher queueing | Done | | 002 | [002-generator-phase-passthrough](./plans/002-generator-phase-passthrough.md) | Generator reads phase from attribute, threads to registration | Draft | -| 003 | [003-aftercommit-entry-call-drain](./plans/003-aftercommit-entry-call-drain.md) | Entry-call tracking in generated factories; AfterCommit drain | In Progress | +| 003 | [003-aftercommit-entry-call-drain](./plans/003-aftercommit-entry-call-drain.md) | Entry-call tracking in generated factories; AfterCommit drain | Done | | 004 | [004-afterflush-coordinator](./plans/004-afterflush-coordinator.md) | IFactoryEventPhaseCoordinator public API + fallback drain | Draft | | 005 | [005-design-docs-skill](./plans/005-design-docs-skill.md) | Design projects, published docs, skill reference | Draft | | 006 | [006-coalescing](./plans/006-coalescing.md) | Opt-in same-event coalescing (v2, queued per user) | Draft | @@ -82,6 +82,32 @@ exposes drain points. ## Discovery Log +### 2026-08-14 — PHASE-003 (code review: interface leg aligned on the registrar-holder shape) +- **Finding:** Code review V1: introducing the wrapper/core split on the interface leg + moved its bodies into private `Local*Core` methods while the assembly attribute still + pointed at `{Impl}Factory` — whose `[DynamicallyAccessedMembers]` roots every method — + i.e. the configuration TRIM-009 *measured* as insufficient on the class leg. "TRIM + item 20 status unchanged" understated a direction-of-change. +- **Decision:** Amend — the interface renderer now emits a single-method registrar + holder (`NeatooInterfaceFactoryRegistrar_` prefix) and points the attribute at it, + aligning all three legs on the measured-good shape. Elimination on this leg is still + UNVERIFIED (TRIM Deferred Work item 20; fixture blocked by item 19) — but the shape is + no longer the measured-bad one. +- **Follow-up:** TRIM item 20's eventual verification now tests the holder shape. + [reviews/003-code-review.md](./reviews/003-code-review.md). + +### 2026-08-14 — PHASE-003 (the RFEF substrate, as actually built) +- **Finding:** RFEF plans to build declarative transactions on this plan's entry-call + tracking. What landed, for its Current State: tracking lives on + `IFactoryEventPhaseScheduler` (events-named, scoped, Server+Logical) with **no + observer hook** — RFEF needs a seam that does not exist yet; granularity is per-scope + (concurrent flows share depth; see the limitation entry below); and generated + `Can*`/`LocalCan*` authorization probes are now full entry calls — under RFEF a + read-only auth probe would open and commit a transaction unless excluded. +- **Decision:** Record here; no code change in PHASE. RFEF-001's draft inherits these + three facts as Current State constraints. +- **Follow-up:** RFEF todo (sibling; blocked on PHASE-003/004 — 003 is now landing). + ### 2026-08-14 — PHASE-003 (concurrent flows share entry state — documented limitation) - **Finding:** The test-review gate pressed on plan-review B-C2: the scheduler's lock gives data-race safety, but entry tracking is per-scope, not per-flow. Two concurrent diff --git a/src/Design/CLAUDE-DESIGN.md b/src/Design/CLAUDE-DESIGN.md index f202d984..ee3ee83e 100644 --- a/src/Design/CLAUDE-DESIGN.md +++ b/src/Design/CLAUDE-DESIGN.md @@ -1019,7 +1019,7 @@ These are known limitations or open questions. They are documented here to preve | 9003 | `FactoryEventPhaseHandlerFailed` | Error | A deferred handler throws during a **post-completion** drain (no ambient transaction) | Swallowed — the exception can no longer roll anything back; remaining queued handlers still run. `OperationCanceledException` still propagates. In-transaction drains propagate instead, so this never fires for them. | | 9004 | `FactoryEventPhaseNoQueueInScope` | Debug | An event with a phased handler is raised in a scope with no `IFactoryEventPhaseScheduler` registered | Dispatched immediately rather than dropped | | 9005 | `FactoryEventPhaseRaisedOutsideEntryCall` | Debug | An event with a phased handler is raised while no entry factory call is active in the scope | Dispatched immediately rather than queued into a drain nobody owns | -| 9006 | `FactoryEventPhaseClearedOnFailure` | Debug | An entry factory call exits without completing successfully and discards its deferred dispatches | The clear (never a drain) that keeps a failed call's work from riding a later call's drain in long-lived scopes | +| 9006 | `FactoryEventPhaseDiscardedAtExit` | Debug | An entry-call exit discards deferred dispatches without running them — a failed call's clear, or the leftovers of a drain a handler's `OperationCanceledException` aborted | The clear (never a drain) that keeps discarded work from riding a later call's drain in long-lived scopes | ### Public Exception diff --git a/src/Generator/Renderer/ClassFactoryRenderer.cs b/src/Generator/Renderer/ClassFactoryRenderer.cs index adae25f0..123cbfa0 100644 --- a/src/Generator/Renderer/ClassFactoryRenderer.cs +++ b/src/Generator/Renderer/ClassFactoryRenderer.cs @@ -398,7 +398,7 @@ private static void RenderLocalMethodOpening( sb.AppendLine(" throw new InvalidOperationException(\"Server-only method called in non-server runtime.\");"); } - sb.AppendLine($" return FactoryEntryCall.{entryRun}(ServiceProvider, () => Local{uniqueName}Core({forwardArgs}));"); + sb.AppendLine($" return global::Neatoo.RemoteFactory.Internal.FactoryEntryCall.{entryRun}(ServiceProvider, () => Local{uniqueName}Core({forwardArgs}));"); sb.AppendLine(" }"); sb.AppendLine(); var asyncKeyword = needsAsync ? "async " : ""; @@ -419,7 +419,7 @@ private static void RenderReadLocalMethod(StringBuilder sb, ReadMethodModel meth RenderLocalMethodOpening(sb, "public", returnType, method.UniqueName, parameters, forwardArgs, needsAsync, method.IsInternal || method.IsRemote); - // Authorization checks (inside guard -- auth types are server-only) + // Authorization checks — in the unguarded Core, reached only through the guarded wrapper RenderAuthorizationChecks(sb, method); // Determine if this is a "read-style" (constructor/static factory) or "write-style" (instance target) invocation. @@ -843,7 +843,7 @@ private static void RenderClassExecuteLocalMethod( RenderLocalMethodOpening(sb, "public", returnType, method.UniqueName, parameters, forwardArgs, needsAsync: true, isServerOnly: method.IsInternal || method.IsRemote); - // Authorization checks (inside guard -- auth types are server-only) + // Authorization checks — in the unguarded Core, reached only through the guarded wrapper RenderAuthorizationChecks(sb, method); // Service assignments @@ -896,7 +896,7 @@ private static void RenderLocalMethod(StringBuilder sb, WriteMethodModel method, RenderLocalMethodOpening(sb, "public", returnType, method.UniqueName, parameters, forwardArgs, needsAsync, method.IsInternal || method.IsRemote); - // Authorization checks (inside guard -- auth types are server-only) + // Authorization checks — in the unguarded Core, reached only through the guarded wrapper RenderAuthorizationChecks(sb, method); // Cast target to implementation type @@ -1380,7 +1380,7 @@ private static void RenderCanLocalMethod(StringBuilder sb, CanMethodModel method RenderLocalMethodOpening(sb, "public", returnType, method.UniqueName, parameters, forwardArgs, method.IsAsync, method.IsInternal || method.IsRemote); - // Authorization checks (inside guard -- auth types are server-only) + // Authorization checks — in the unguarded Core, reached only through the guarded wrapper RenderAuthorizationChecks(sb, method); // Return success diff --git a/src/Generator/Renderer/InterfaceFactoryRenderer.cs b/src/Generator/Renderer/InterfaceFactoryRenderer.cs index 0d4c54c3..5626b429 100644 --- a/src/Generator/Renderer/InterfaceFactoryRenderer.cs +++ b/src/Generator/Renderer/InterfaceFactoryRenderer.cs @@ -44,8 +44,13 @@ public static string Render(FactoryGenerationUnit unit) sb.AppendLine(); - // Assembly-level attribute for trimming-safe factory discovery - sb.AppendLine($"[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::{unit.Namespace}.{model.ImplementationTypeName}Factory))]"); + // Assembly-level attribute for trimming-safe factory discovery. + // Targets the generated registrar holder, NEVER the factory class: the + // attribute's [DynamicallyAccessedMembers] preserves every method on whatever + // type it names, bodies included. Naming {Impl}Factory rooted every Local*Core + // body — the configuration TRIM-009 measured as insufficient on the class leg + // (PHASE-003 code review V1 aligned this leg with the class/static holders). + sb.AppendLine($"[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::{unit.Namespace}.{RegistrarHolderPrefix}{model.ImplementationTypeName}))]"); sb.AppendLine(); sb.AppendLine("/*"); @@ -62,11 +67,38 @@ public static string Render(FactoryGenerationUnit unit) // Factory implementation class RenderFactoryClass(sb, model); + // Registrar holder — the assembly attribute's DAM target, so the DAM blast + // radius is one forwarding method instead of every method on the factory class. + sb.AppendLine(); + RenderRegistrarHolder(sb, unit, model); + sb.AppendLine("}"); return sb.ToString(); } + /// + /// Prefix for the generated registrar holder type. Distinct from the class leg's + /// (NeatooClassFactoryRegistrar_), the static leg's + /// (NeatooFactoryRegistrar_), and the relay leg's prefixes so no two ever + /// collide on one consumer type (Deferred Work item 15 convention). A PREFIX, not a + /// suffix, so the namespace-qualified factory name is not a substring of the holder + /// name and "attribute must not name the factory type" assertions mean what they + /// say. + /// + internal const string RegistrarHolderPrefix = "NeatooInterfaceFactoryRegistrar_"; + + private static void RenderRegistrarHolder(StringBuilder sb, FactoryGenerationUnit unit, InterfaceFactoryModel model) + { + sb.AppendLine($" internal static class {RegistrarHolderPrefix}{model.ImplementationTypeName}"); + sb.AppendLine(" {"); + sb.AppendLine(" internal static void FactoryServiceRegistrar(IServiceCollection services, NeatooFactory remoteLocal)"); + sb.AppendLine(" {"); + sb.AppendLine($" global::{unit.Namespace}.{model.ImplementationTypeName}Factory.FactoryServiceRegistrar(services, remoteLocal);"); + sb.AppendLine(" }"); + sb.AppendLine(" }"); + } + private static void RenderFactoryInterface(StringBuilder sb, InterfaceFactoryModel model) { sb.AppendLine($" public interface {model.ServiceTypeName}Factory : {model.ServiceTypeName}"); @@ -282,7 +314,7 @@ private static void RenderLocalMethod(StringBuilder sb, InterfaceMethodModel met sb.AppendLine(" {"); sb.AppendLine(" if (!NeatooRuntime.IsServerRuntime)"); sb.AppendLine(" throw new InvalidOperationException(\"Server-only method called in non-server runtime.\");"); - sb.AppendLine($" return FactoryEntryCall.{entryRun}(ServiceProvider, () => Local{method.UniqueName}Core({forwardArgs}));"); + sb.AppendLine($" return global::Neatoo.RemoteFactory.Internal.FactoryEntryCall.{entryRun}(ServiceProvider, () => Local{method.UniqueName}Core({forwardArgs}));"); sb.AppendLine(" }"); sb.AppendLine(); sb.AppendLine($" private {asyncKeyword}{returnType} Local{method.UniqueName}Core({parameters})"); diff --git a/src/Generator/Renderer/StaticFactoryRenderer.cs b/src/Generator/Renderer/StaticFactoryRenderer.cs index 37b8e957..c18e1040 100644 --- a/src/Generator/Renderer/StaticFactoryRenderer.cs +++ b/src/Generator/Renderer/StaticFactoryRenderer.cs @@ -237,7 +237,7 @@ private static void RenderLocalDelegateRegistration(StringBuilder sb, ExecuteDel sb.AppendLine(" {"); sb.AppendLine($" services.AddTransient<{typeName}.{del.DelegateName}>(cc =>"); sb.AppendLine(" {"); - sb.AppendLine($" return ({paramDecl}) => FactoryEntryCall.RunAsync(cc, () => {{"); + sb.AppendLine($" return ({paramDecl}) => global::Neatoo.RemoteFactory.Internal.FactoryEntryCall.RunAsync(cc, () => {{"); if (!string.IsNullOrEmpty(serviceAssignments)) { diff --git a/src/RemoteFactory/Internal/FactoryEntryCall.cs b/src/RemoteFactory/Internal/FactoryEntryCall.cs index 215a8589..28c75556 100644 --- a/src/RemoteFactory/Internal/FactoryEntryCall.cs +++ b/src/RemoteFactory/Internal/FactoryEntryCall.cs @@ -84,6 +84,14 @@ public static async Task RunAsync(IServiceProvider serviceProvider, Func b /// invariant outranks staying non-blocking on this edge. With nothing deferred, the /// completion is fully synchronous. /// + /// + /// The blocking drain can deadlock under a captured + /// (e.g. Logical mode inside a Blazor Server circuit) if a drained handler awaits + /// without ConfigureAwait(false) — the continuation posts to the context this + /// call is blocking. Handlers registered at a non-Immediate phase should not assume + /// a context, and synchronous factory methods should not raise phased events on + /// context-bound scopes. + /// public static T Run(IServiceProvider serviceProvider, Func body) { ArgumentNullException.ThrowIfNull(serviceProvider); diff --git a/src/RemoteFactory/Internal/FactoryEventPhaseScheduler.cs b/src/RemoteFactory/Internal/FactoryEventPhaseScheduler.cs index 46b120e3..78cb4e1c 100644 --- a/src/RemoteFactory/Internal/FactoryEventPhaseScheduler.cs +++ b/src/RemoteFactory/Internal/FactoryEventPhaseScheduler.cs @@ -142,7 +142,9 @@ public async Task EndEntryCallAsync(bool success) return; } - bool outermost; + // Single lock acquisition: a check-then-decrement across two acquisitions opens + // a window (interleaved with a concurrent flow's failure exit) where depth hits + // zero having neither drained nor cleared (code review C1). lock (_gate) { if (_entryDepth == 0) @@ -151,17 +153,11 @@ public async Task EndEntryCallAsync(bool success) $"{nameof(EndEntryCallAsync)} called without a matching {nameof(BeginEntryCall)}."); } - outermost = _entryDepth == 1; - } - - if (!outermost) - { - lock (_gate) + if (_entryDepth > 1) { _entryDepth--; + return; } - - return; } // The entry stays active (depth 1) for the duration of the drain, so an event a @@ -272,7 +268,7 @@ private void ClearAtExit() if (discarded > 0) { - _logger?.FactoryEventPhaseClearedOnFailure(discarded); + _logger?.FactoryEventPhaseDiscardedAtExit(discarded); } } diff --git a/src/RemoteFactory/Internal/Log.cs b/src/RemoteFactory/Internal/Log.cs index a41f8951..5014b884 100644 --- a/src/RemoteFactory/Internal/Log.cs +++ b/src/RemoteFactory/Internal/Log.cs @@ -522,8 +522,8 @@ public static partial void FactoryEventPhaseRaisedOutsideEntryCall( [LoggerMessage( EventId = 9006, Level = LogLevel.Debug, - Message = "Discarded {DiscardedCount} deferred handler dispatch(es) because the entry factory call did not complete successfully.")] - public static partial void FactoryEventPhaseClearedOnFailure( + Message = "Discarded {DiscardedCount} deferred handler dispatch(es) at entry-call exit without running them.")] + public static partial void FactoryEventPhaseDiscardedAtExit( this ILogger logger, int discardedCount); } diff --git a/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs b/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs index 2f876a92..eb009f7d 100644 --- a/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs +++ b/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs @@ -163,7 +163,7 @@ public partial class MyEntity Assert.Matches( @"public Task LocalFetchIt\(string name, CancellationToken cancellationToken = default\)\s*\{\s*if \(!NeatooRuntime\.IsServerRuntime\)", generatedSource); - Assert.Contains("return FactoryEntryCall.RunAsync(ServiceProvider, () => LocalFetchItCore(name, cancellationToken));", generatedSource); + Assert.Contains("return global::Neatoo.RemoteFactory.Internal.FactoryEntryCall.RunAsync(ServiceProvider, () => LocalFetchItCore(name, cancellationToken));", generatedSource); // Core: async, private, and carries NO guard — the guard already ran in the wrapper. Assert.Contains("private async Task LocalFetchItCore(", generatedSource); @@ -232,7 +232,7 @@ internal void Create(string name) { } Assert.Matches( @"public Task LocalCreate\(string name, CancellationToken cancellationToken = default\)\s*\{\s*if \(!NeatooRuntime\.IsServerRuntime\)", generatedSource); - Assert.Contains("return FactoryEntryCall.RunAsync(ServiceProvider, () => LocalCreateCore(name, cancellationToken));", generatedSource); + Assert.Contains("return global::Neatoo.RemoteFactory.Internal.FactoryEntryCall.RunAsync(ServiceProvider, () => LocalCreateCore(name, cancellationToken));", generatedSource); // Core: private, NOT async (the sync body keeps its shape), and carries no guard. Assert.Matches( @@ -427,9 +427,16 @@ public void StaticFactory_RegistrarHolder_ForwardsToUserClass() #region Interface Factory /// - /// Interface factory generated source contains the assembly-level NeatooFactoryRegistrar - /// attribute with the fully-qualified implementation factory type name. + /// Interface factory generated source points the assembly-level NeatooFactoryRegistrar + /// attribute at a single-method registrar holder — never at the factory class, whose + /// methods (including the private Local*Core bodies) the attribute's + /// [DynamicallyAccessedMembers] would otherwise root into trimmed clients. /// + /// + /// Amended by PHASE-003 (code review V1): previously the attribute named + /// MyServiceFactory itself, which became the TRIM-009 measured-insufficient + /// configuration once the Local* wrapper/core split landed on this leg. + /// [Fact] public void InterfaceFactory_EmitsAssemblyAttribute() { @@ -453,7 +460,17 @@ public interface IMyService ?.ToString(); Assert.NotNull(generatedSource); - Assert.Contains("[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::TestNamespace.MyServiceFactory))]", generatedSource); + Assert.Contains("[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::TestNamespace.NeatooInterfaceFactoryRegistrar_MyService))]", generatedSource); + + // The attribute must not name the factory type (the prefix convention keeps the + // factory's qualified name from being a substring of the holder's). + Assert.DoesNotContain("NeatooFactoryRegistrar(typeof(global::TestNamespace.MyServiceFactory)", generatedSource); + + // The holder exists and forwards to the factory's registrar. + Assert.Matches( + @"internal static class NeatooInterfaceFactoryRegistrar_MyService\s*\{\s*internal static void FactoryServiceRegistrar\(IServiceCollection services, NeatooFactory remoteLocal\)", + generatedSource); + Assert.Contains("global::TestNamespace.MyServiceFactory.FactoryServiceRegistrar(services, remoteLocal);", generatedSource); } /// @@ -499,7 +516,7 @@ public interface IMyService @"public Task LocalDoWork\([^)]*\)\s*\{\s*if \(!NeatooRuntime\.IsServerRuntime\)", generatedSource); Assert.DoesNotContain("public async Task LocalDoWork(", generatedSource); - Assert.Contains("return FactoryEntryCall.RunAsync(ServiceProvider, () => LocalDoWorkCore(", generatedSource); + Assert.Contains("return global::Neatoo.RemoteFactory.Internal.FactoryEntryCall.RunAsync(ServiceProvider, () => LocalDoWorkCore(", generatedSource); // Core: private and unguarded (this fixture's body forwards the target's task // without awaiting, so no async keyword; the guard already ran in the wrapper).