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..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:** Draft +**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) @@ -20,10 +20,514 @@ 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 -(PHASE-004). +`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. --- -*(Stub — Intent, Alignment, Constraints, Steps, Acceptance filled at Step 2.)* +## 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 — 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 + 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 + ("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 (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). + +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. 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, 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. 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 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* 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, 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. +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). 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. + +--- + +## 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 (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 + 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]` +- [ ] 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 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, 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 — unit, integration, AND the + Design solution's suite (renderer changes regenerate every Design factory) — + 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]` + +--- + +## 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`) — **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` +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, 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. + +--- + +## 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 **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 +`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` (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) 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` — 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 ✓ | +| 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). 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`; 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 | +| (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). + +--- + +## Plan Amendments + +### 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 — 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 +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. + +--- + +## 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. +- **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 032dffb7..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 @@ -22,4 +22,31 @@ 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. + +--- + +## 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/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/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/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/reviews/003-test-review.md b/docs/todos/PHASE-phased-event-dispatch/reviews/003-test-review.md new file mode 100644 index 00000000..10c0d9e7 --- /dev/null +++ b/docs/todos/PHASE-phased-event-dispatch/reviews/003-test-review.md @@ -0,0 +1,121 @@ +# 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 (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 0a9248db..591c92af 100644 --- a/docs/todos/PHASE-phased-event-dispatch/todo.md +++ b/docs/todos/PHASE-phased-event-dispatch/todo.md @@ -70,17 +70,134 @@ 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 | +| 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 | +| 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 (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 + 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. 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. + +### 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 + 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: + 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 + 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 @@ -107,7 +224,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.) --- diff --git a/src/Design/CLAUDE-DESIGN.md b/src/Design/CLAUDE-DESIGN.md index 353ca451..ee3ee83e 100644 --- a/src/Design/CLAUDE-DESIGN.md +++ b/src/Design/CLAUDE-DESIGN.md @@ -1014,6 +1014,12 @@ 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 | +| 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 | `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 a2e34fb1..123cbfa0 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 global::Neatoo.RemoteFactory.Internal.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) @@ -422,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. @@ -846,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 @@ -899,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 @@ -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 @@ -1384,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 bafc9716..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}"); @@ -249,13 +281,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 +302,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 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})"); + 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..c18e1040 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}) => global::Neatoo.RemoteFactory.Internal.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/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..0c511547 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,35 @@ 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 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 && _phaseQueue.IsEntryCallActive) + { + _phaseQueue.Enqueue(phase, (FactoryEventBase)factoryEvent, options, handler); + continue; + } + + 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/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/FactoryEntryCall.cs b/src/RemoteFactory/Internal/FactoryEntryCall.cs new file mode 100644 index 00000000..28c75556 --- /dev/null +++ b/src/RemoteFactory/Internal/FactoryEntryCall.cs @@ -0,0 +1,120 @@ +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. + /// + /// + /// 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); + 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 new file mode 100644 index 00000000..78cb4e1c --- /dev/null +++ b/src/RemoteFactory/Internal/FactoryEventPhaseScheduler.cs @@ -0,0 +1,300 @@ +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 and the +/// entry-call tracking that decides when the framework drains on its own. +/// +/// +/// +/// 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. 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 +{ + /// 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); + + /// + /// 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(); + + // 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; + _logger = loggerFactory?.CreateLogger(NeatooLoggerCategories.Server); + } + + private readonly record struct QueuedDispatch( + FactoryEventBase Event, + RaiseOptions Options, + Func Handler); + + 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; + } + + // 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) + { + throw new InvalidOperationException( + $"{nameof(EndEntryCallAsync)} called without a matching {nameof(BeginEntryCall)}."); + } + + if (_entryDepth > 1) + { + _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); + + lock (_gate) + { + 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); + } + } + + /// + /// 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?.FactoryEventPhaseDiscardedAtExit(discarded); + } + } + + /// + /// 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) + { + lock (_gate) + { + 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..5014b884 100644 --- a/src/RemoteFactory/Internal/Log.cs +++ b/src/RemoteFactory/Internal/Log.cs @@ -470,4 +470,60 @@ 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); + + [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) at entry-call exit without running them.")] + public static partial void FactoryEventPhaseDiscardedAtExit( + this ILogger logger, + int discardedCount); } 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.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs b/src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs new file mode 100644 index 00000000..59cf39d3 --- /dev/null +++ b/src/Tests/RemoteFactory.IntegrationTests/Events/Phases/FactoryEventPhaseEntryTests.cs @@ -0,0 +1,339 @@ +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)); + } + + [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)); + + // 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] + 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) + { + 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..7725d67f --- /dev/null +++ b/src/Tests/RemoteFactory.IntegrationTests/TestTargets/Events/FactoryEventPhaseEntryTargets.cs @@ -0,0 +1,506 @@ +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; +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 +// 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; + }); + + 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; + } +} + +// ----------------------------------------------------------------------------- +// 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 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. + /// + [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(); +} + +/// +/// 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 +{ + [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"); +} diff --git a/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs b/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs index df038195..eb009f7d 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 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); @@ -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 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( + @"private Task LocalCreateCore\([^)]*\)\s*\{\s*(?!\s*if \(!NeatooRuntime\.IsServerRuntime\))", + generatedSource); + Assert.DoesNotContain("private async Task LocalCreateCore(", generatedSource); } /// @@ -413,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() { @@ -439,7 +460,69 @@ 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); + } + + /// + /// 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 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). + Assert.Matches( + @"private (async )?Task LocalDoWorkCore\([^)]*\)\s*\{\s*(?!\s*if \(!NeatooRuntime\.IsServerRuntime\))", + generatedSource); } #endregion diff --git a/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEntryCallTests.cs b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEntryCallTests.cs new file mode 100644 index 00000000..cb8f58b0 --- /dev/null +++ b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEntryCallTests.cs @@ -0,0 +1,412 @@ +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 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 scheduler = sp.GetRequiredService(); + + await Assert.ThrowsAsync(() => scheduler.EndEntryCallAsync(success: true)); + + // 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/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..e7d8bb2a --- /dev/null +++ b/src/Tests/RemoteFactory.UnitTests/Internal/FactoryEventsDispatcherPhaseTests.cs @@ -0,0 +1,388 @@ +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +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 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; + 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 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 { } + + 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(); + + queue.BeginEntryCall(); + await events.Raise(new DeferredOnlyEvent("x")); + + lock (Dispatched) + { + Assert.Empty(Dispatched); + } + Assert.True(queue.HasPending); + + await queue.EndEntryCallAsync(success: true); + + 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(); + + queue.BeginEntryCall(); + await events.Raise(new MixedPhaseEvent("x")); + + lock (Dispatched) + { + Assert.Equal(["immediate"], Dispatched); + } + + await queue.EndEntryCallAsync(success: true); + + // 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(); + + queue.BeginEntryCall(); + await events.RaiseUntyped(new UntypedRaiseEvent("x")); + + lock (Dispatched) + { + Assert.Empty(Dispatched); + } + Assert.True(queue.HasPending); + + await queue.EndEntryCallAsync(success: true); + + 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. + // + // 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(); + 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); + lock (Dispatched) + { + Dispatched.Add("source-after-raise"); + } + }); + + queue.BeginEntryCall(); + await events.Raise(new ChainedSourceEvent("x")); + await queue.EndEntryCallAsync(success: true); + + lock (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 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 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) + { + 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); + } + } + + private sealed class CapturingProvider : Microsoft.Extensions.Logging.ILoggerProvider + { + public List<(int EventId, Microsoft.Extensions.Logging.LogLevel Level)> Entries { get; } = []; + + 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) + { + lock (owner.Entries) + { + owner.Entries.Add((eventId.Id, logLevel)); + } + } + } + } + + [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() + { + 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(); + 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); + } + } + + [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(); + 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()); + // The deferral premise, asserted so it cannot vacate silently again. + Assert.True(queue.HasPending); + } + } +}