Skip to content

Commit fcabff3

Browse files
committed
feat(caching): ServiceRef.ObservationOnly variant so ILogger doesn't uncacheable steps
The smart-caching planner's prior invariant — "any service dep makes a step uncacheable, cascade to downstream consumers" — was correct for domain services whose state can change a step's output (IRemoteTimeService, ICustomerRepository) but wrong for pure observation surfaces. Once we landed Create(ILogger logger) on ~half the example step library, every Flow that touched a logger-declaring step became uncacheable end-to-end via the cascade rule — a regression of the smart-caching system, not the intended outcome of plumbing a logger through. This change adds a third sealed variant to the ServiceRef closed sum — ObservationOnly(Type ServiceType) — that DI-resolves identically to CSharp but is skipped by CachePlanBuilder when counting cache-affecting dependencies. The [FlowthruStep] source generator recognises Microsoft.Extensions.Logging.ILogger by FQN and emits the new variant for it; everything else still emits CSharp. The "1 service dependency(ies)" message in the host log now reflects only cache-affecting refs, so a step that declares both a logger and a real service is still uncacheable but the count is honest (e.g., SimpleEffectsExample's 2 → 1). Three new CachePlanBuilder tests pin the contract: - ObservationOnly-only step stays fresh-eligible - ObservationOnly + regular dep still uncacheable, count excludes the observation ref - ObservationOnly parent doesn't cascade uncacheability to children The existing StepLoggerInjectionTests.SourceGenerator_ExtractsILogger test was renamed and the assertion updated to look for the ObservationOnly variant. ADR-0010 documents the design; src/core/CONTRIBUTING.md gets one paragraph cross-linking it from the Engine Logging section. User-side opt-in (an [ObservationOnly] attribute on Create() params for arbitrary observability services) is the planned follow-up when a second observation-only service surfaces — premature to design without a real use case.
1 parent 005ca67 commit fcabff3

7 files changed

Lines changed: 185 additions & 11 deletions

File tree

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
# Observation-only service refs are a distinct case in the `ServiceRef` closed sum
2+
3+
`ServiceRef` now has a third sealed nested variant — `ObservationOnly(Type ServiceType)` — alongside `CSharp` and `External`. The cache planner ([CachePlanBuilder.cs:119](/src/core/Flowthru.Core/Caching/CachePlanBuilder.cs#L119)) skips refs of this variant when counting cache-affecting dependencies, so steps that declare only observation-only services (the canonical case is `ILogger` per [ADR-0005](./0005-step-logging-via-shared-ilogger.md)) remain cache-eligible and don't cascade uncacheability through their downstream consumers. DI resolution is identical to `CSharp` — the `ServiceType` is the lookup key against the host's `IServiceProvider` — but the planner treats the dep as cache-neutral because observation surfaces can't change the step's output values, only the IO timeline. Recognition happens at source-generation time: the `[FlowthruStep]` generator emits `ServiceRef.ObservationOnly(...)` when a `Create()` parameter's fully-qualified type matches a hardcoded set (currently just `Microsoft.Extensions.Logging.ILogger`); a future per-parameter `[ObservationOnly]` attribute is the planned opt-in for user-defined observability services. The variant exists because the smart-caching planner's prior invariant — "any service dep makes a step uncacheable" — is correct for domain services whose state can change the step's output (e.g., `IRemoteTimeService`) but wrong for pure observation surfaces, and without the carve-out declaring a logger uncacheabilises every Flow in the workspace via the cascade rule.

src/core/CONTRIBUTING.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,8 @@ Engine components (`FlowthruService`, `ParallelFlowScheduler`, future schedulers
5353

5454
`FlowthruActivitySource` still emits trace spans (`flowthru.run`, `flowthru.preflight`, `flowthru.step`) for OpenTelemetry and other distributed-tracing consumers — that was always activities' real job. The CLI-side `FlowthruActivityLogger` bridge that translated those activities into log lines has been retired; see [.claude/docs/adr/0006-engine-logs-directly-retire-activity-bridge.md](/.claude/docs/adr/0006-engine-logs-directly-retire-activity-bridge.md). The step-side convention (`Create(ILogger)`) lives in [/examples/CONTRIBUTING.md](/examples/CONTRIBUTING.md) and [.claude/docs/adr/0005-step-logging-via-shared-ilogger.md](/.claude/docs/adr/0005-step-logging-via-shared-ilogger.md).
5555

56+
The smart-caching planner treats `ILogger` as **cache-neutral** — the source generator emits it as `ServiceRef.ObservationOnly` rather than `ServiceRef.CSharp`, so a step that declares only a logger remains cache-eligible (and its downstream consumers don't inherit uncacheability via the cascade rule). See [.claude/docs/adr/0010-observation-only-service-refs.md](/.claude/docs/adr/0010-observation-only-service-refs.md). Steps that declare both a regular service and a logger still cascade through the regular dep — `ObservationOnly` doesn't change *what* gets resolved, only *whether* the planner counts the dep when deciding cacheability.
57+
5658
## Prelude
5759

5860
`Flowthru.Prelude` houses the FP primitives the rest of Core builds on — `FlowIO<T>`, `EffResult<A>`, `Validated<TError, TValue>`, `FlowUnit`, `FlowResource`, plus the `IFlowResource` interface.
@@ -90,3 +92,12 @@ _Avoid_: helper method, fluent API
9092

9193
**Applicative vs monadic composition**: Two ways to chain operations on a closed sum. *Applicative* (`Zip`, `ZipAll`) accumulates errors from all branches — for pre-flight validation where every problem should surface at once. *Monadic* (`Bind`, LINQ `from`/`in`/`select`) short-circuits on the first error — for operations that genuinely depend on earlier success. Choose by asking: are these checks independent (applicative) or dependent (monadic)?
9294
_Avoid_: parallel vs sequential
95+
96+
**Anchor**: A structured property in a Flowthru analyzer's emitted `Diagnostic` that ties the diagnostic to a specific [[DAG]] element — a Step, Item, Edge, Flow, or Schema. Read by Tool renderers (notably the planned VSCode Editor Frontend's F2 surface) to project the diagnostic onto a DAG canvas in addition to the source-positional squiggly. Composite by design: a diagnostic may carry multiple Anchors of mixed kinds (Step + Item + Edge for a "no producer" diagnostic), and the renderer prioritizes label-keyed Anchors over type-symbol fallbacks. See [ADR-0009](/.claude/docs/adr/0009-diagnostic-anchor-contract.md) for the contract.
97+
_Avoid_: "tag" (Roslyn already uses `DiagnosticTag` for orthogonal concerns), "location" (Roslyn's `Location` is source-positional; an Anchor is semantic — it identifies a DAG element, not a span).
98+
99+
**Anchor block**: The complete set of `Flowthru.Anchor.*` property keys carried by a single diagnostic. Always uses numbered keys (`.0`, `.1`, …) even for single-anchor diagnostics — symmetry over special cases. A diagnostic's Anchor block is built via `FlowthruAnchor.Builder()` and attached at `CreateFlowthruDiagnostic` time; the extension-method pattern follows the Roslyn idiom (see `Roslyn.Diagnostics.Analyzers/Core/DiagnosticExtensions.cs`) rather than introducing a `FlowthruAnalyzer` base class.
100+
_Avoid_: "anchor set" (suggests unordered; the numbered indices are ordered for renderer stability), "property bag" (too generic — every Roslyn diagnostic has properties; the Anchor block is the specifically-keyed subset).
101+
102+
**`Anchor.None` sentinel**: Explicit "this diagnostic has no DAG meaning" marker, carried as `Flowthru.Anchor.None = "true"` in lieu of any kind-keyed Anchor. Used by diagnostics about authoring shape that don't correspond to any node or edge in a Flow Dev's DAG — extension-capability mismatches, analyzer-test scaffolding, source-generator preconditions. The sentinel disambiguates *intentional* unanchorability from *forgotten* anchoring; the test convention treats a missing Anchor block (no kind-keyed entries and no sentinel) as a failure.
103+
_Avoid_: omitting the block entirely (silent omission is ambiguous between "unanchorable" and "the analyzer author forgot"; the sentinel makes intent explicit).

src/core/Flowthru.Core.SourceGenerators/Step/StepMetadataGenerator.cs

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,22 @@ public sealed class StepMetadataGenerator : IIncrementalGenerator
5757
/// </summary>
5858
private const int CodeVersionHexLength = 16;
5959

60+
/// <summary>
61+
/// Fully-qualified type names of framework-recognised observation-
62+
/// only services. When a <c>Create()</c> parameter's FQN matches an
63+
/// entry here, the generator emits a <c>ServiceRef.ObservationOnly</c>
64+
/// instead of the default <c>ServiceRef.CSharp</c> — the cache
65+
/// planner skips observation-only refs when deciding step
66+
/// cacheability (ADR-0010). Keep this set tiny; an
67+
/// <c>[ObservationOnly]</c> per-parameter attribute is the planned
68+
/// opt-in mechanism for user-defined observability services.
69+
/// </summary>
70+
private static readonly System.Collections.Generic.HashSet<string> _observationOnlyFqns =
71+
new(System.StringComparer.Ordinal)
72+
{
73+
"global::Microsoft.Extensions.Logging.ILogger",
74+
};
75+
6076
/// <inheritdoc/>
6177
public void Initialize(IncrementalGeneratorInitializationContext context)
6278
{
@@ -240,7 +256,8 @@ private static void Emit(SourceProductionContext ctx, StepInfo info)
240256
sb.AppendLine(" {");
241257
foreach (var fqn in info.ServiceTypeFqns)
242258
{
243-
sb.AppendLine($" new global::Flowthru.Validation.Runtime.ServiceRef.CSharp(typeof({fqn})),");
259+
var variant = _observationOnlyFqns.Contains(fqn) ? "ObservationOnly" : "CSharp";
260+
sb.AppendLine($" new global::Flowthru.Validation.Runtime.ServiceRef.{variant}(typeof({fqn})),");
244261
}
245262
sb.AppendLine(" };");
246263
sb.AppendLine("}");

src/core/Flowthru.Core/Caching/CachePlanBuilder.cs

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,11 +116,20 @@ public static async Task<CachePlan> BuildAsync(
116116
uncacheableReasons[step.Label] = new StepUncacheableReason.NoCodeVersion();
117117
continue;
118118
}
119-
if (step.ServiceDependencies.Count > 0)
119+
// Cache-affecting deps only. ServiceRef.ObservationOnly variants
120+
// (e.g., ILogger) are skipped — per ADR-0010, observation surfaces
121+
// can't change a step's output values, so their presence or
122+
// identity doesn't invalidate a cached result. Without this
123+
// filter, declaring a logger on a Create() factory would
124+
// uncacheabilise the step and cascade through every downstream
125+
// consumer.
126+
var cacheAffectingDeps = step.ServiceDependencies
127+
.Count(r => r is not ServiceRef.ObservationOnly);
128+
if (cacheAffectingDeps > 0)
120129
{
121130
uncacheable.Add(step.Label);
122131
uncacheableReasons[step.Label] = new StepUncacheableReason.HasServiceDependencies(
123-
step.ServiceDependencies.Count);
132+
cacheAffectingDeps);
124133
continue;
125134
}
126135

src/core/Flowthru.Core/Validation/Runtime/ServiceRef.cs

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,40 @@ public sealed record External(IExtensionServiceRef Cause) : ServiceRef
5959
public override string DisplayName => Cause.DisplayName;
6060
}
6161

62+
/// <summary>
63+
/// A C# service identified by an interface or class type, classified
64+
/// as <em>observation-only</em>: its calls don't affect the step's
65+
/// output values, only the IO timeline. DI resolution is identical
66+
/// to <see cref="CSharp"/> — the <see cref="ServiceType"/> is the
67+
/// lookup key used against the host's
68+
/// <see cref="System.IServiceProvider"/> at runtime — but the cache
69+
/// planner skips refs of this variant when deciding step
70+
/// cacheability. The canonical (and currently only) recognised case
71+
/// is <c>Microsoft.Extensions.Logging.ILogger</c>; the
72+
/// <c>[FlowthruStep]</c> source generator emits this variant when a
73+
/// <c>Create()</c> parameter's fully-qualified type matches a
74+
/// hardcoded set.
75+
/// </summary>
76+
/// <remarks>
77+
/// <para>
78+
/// Per ADR-0010, this variant exists because the smart-caching
79+
/// planner's invariant is "any service dep makes a step
80+
/// uncacheable" (and cascades downstream) — true for domain
81+
/// services whose state can change the step's output (e.g.,
82+
/// <c>IRemoteTimeService</c>) but wrong for pure observation
83+
/// surfaces. Without this carve-out, declaring a logger
84+
/// uncacheabilises every Flow in the workspace.
85+
/// </para>
86+
/// </remarks>
87+
public sealed record ObservationOnly(Type ServiceType) : ServiceRef
88+
{
89+
/// <inheritdoc/>
90+
public override string DagId => ServiceType.FullName ?? ServiceType.Name;
91+
92+
/// <inheritdoc/>
93+
public override string DisplayName => ServiceType.Name;
94+
}
95+
6296
/// <summary>Build a <see cref="CSharp"/> ref from a generic type parameter.</summary>
6397
public static ServiceRef Of<TService>() where TService : class =>
6498
new CSharp(typeof(TService));

tests/core/Flowthru.Core.Tests/Caching/CachePlanBuilderTests.cs

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,102 @@ public async Task UncacheableReason_ServiceDependencies_CarriesCount()
350350
Assert.That(((StepUncacheableReason.HasServiceDependencies)reason).Count, Is.EqualTo(2));
351351
}
352352

353+
// ── ObservationOnly carve-out (ADR-0010) ────────────────────────────
354+
355+
[Test]
356+
public async Task ObservationOnlyDeps_DoNotMarkStepUncacheable()
357+
{
358+
// ADR-0010: ServiceRef.ObservationOnly variants (e.g., ILogger)
359+
// are skipped when computing cache eligibility — observation
360+
// surfaces don't affect step output values, so their presence
361+
// can't invalidate a cached result.
362+
var input = new FakeFingerprintItem<int>("in", fingerprint: "fp-in", exists: true);
363+
var output = new FakeFingerprintItem<int>("out", fingerprint: "fp-out", exists: true);
364+
var step = MakeStep(
365+
label: "transform", codeVersion: "code-v1",
366+
input: input, output: output,
367+
serviceDependencies: new ServiceRef[]
368+
{
369+
new ServiceRef.ObservationOnly(typeof(Microsoft.Extensions.Logging.ILogger)),
370+
});
371+
372+
var composite = CachePlanBuilder.ComposeStepFingerprint(
373+
"code-v1", new[] { ("in", "fp-in") });
374+
var manifest = Manifest(
375+
steps: new[] { ("transform", composite) },
376+
items: new[] { ("in", "fp-in") });
377+
378+
var plan = await CachePlanBuilder.BuildAsync(BuildFlow(step), manifest);
379+
380+
Assert.That(plan.FreshStepLabels, Is.EquivalentTo(new[] { "transform" }),
381+
"A step with only observation-only deps must remain eligible for caching when "
382+
+ "its inputs and code version match the manifest.");
383+
Assert.That(plan.UncacheableStepLabels, Is.Empty,
384+
"Observation-only deps must not push the step into the uncacheable bucket.");
385+
}
386+
387+
[Test]
388+
public async Task ObservationOnlyPlusRegularDep_StillUncacheable_CountExcludesObservation()
389+
{
390+
// The carve-out is for observation-only refs specifically; a step
391+
// that also declares a regular service dep is still uncacheable.
392+
// The reason's Count surfaces only the cache-affecting deps so
393+
// the developer-facing message stays meaningful.
394+
var input = new FakeFingerprintItem<int>("in", fingerprint: "fp-in", exists: true);
395+
var output = new FakeFingerprintItem<int>("out", fingerprint: "fp-out", exists: true);
396+
var step = MakeStep(
397+
label: "transform", codeVersion: "code-v1",
398+
input: input, output: output,
399+
serviceDependencies: new ServiceRef[]
400+
{
401+
new ServiceRef.ObservationOnly(typeof(Microsoft.Extensions.Logging.ILogger)),
402+
ServiceRef.Of<object>(),
403+
});
404+
405+
var plan = await CachePlanBuilder.BuildAsync(BuildFlow(step), CacheManifest.Empty);
406+
407+
Assert.That(plan.UncacheableStepLabels, Is.EquivalentTo(new[] { "transform" }));
408+
var reason = plan.UncacheableReasons["transform"];
409+
Assert.That(reason, Is.TypeOf<StepUncacheableReason.HasServiceDependencies>());
410+
Assert.That(((StepUncacheableReason.HasServiceDependencies)reason).Count, Is.EqualTo(1),
411+
"Count surfaces cache-affecting deps only — the ObservationOnly logger is excluded.");
412+
}
413+
414+
[Test]
415+
public async Task ObservationOnlyParent_DoesNotCascadeUncacheabilityToChildren()
416+
{
417+
// The original motivation for ADR-0010 was the cascade: an
418+
// ILogger-declaring parent step uncacheabilised every downstream
419+
// consumer too. With the carve-out the parent is cache-eligible,
420+
// so the cascade rule has nothing to propagate.
421+
var seedInput = new FakeFingerprintItem<int>("seed", fingerprint: "fp-seed", exists: true);
422+
var mid = new FakeFingerprintItem<int>("mid", fingerprint: "fp-mid", exists: true);
423+
var output = new FakeFingerprintItem<int>("out", fingerprint: "fp-out", exists: true);
424+
var stepA = MakeStep(
425+
label: "A", codeVersion: "code-A-v1",
426+
input: seedInput, output: mid,
427+
serviceDependencies: new ServiceRef[]
428+
{
429+
new ServiceRef.ObservationOnly(typeof(Microsoft.Extensions.Logging.ILogger)),
430+
});
431+
var stepB = MakeStep("B", "code-B-v1", mid, output);
432+
433+
var compositeA = CachePlanBuilder.ComposeStepFingerprint(
434+
"code-A-v1", new[] { ("seed", "fp-seed") });
435+
var compositeB = CachePlanBuilder.ComposeStepFingerprint(
436+
"code-B-v1", new[] { ("mid", "fp-mid") });
437+
var manifest = Manifest(
438+
steps: new[] { ("A", compositeA), ("B", compositeB) },
439+
items: new[] { ("seed", "fp-seed"), ("mid", "fp-mid") });
440+
441+
var plan = await CachePlanBuilder.BuildAsync(BuildFlow(stepA, stepB), manifest);
442+
443+
Assert.That(plan.UncacheableStepLabels, Is.Empty,
444+
"Observation-only deps on the parent must not cascade into the child.");
445+
Assert.That(plan.FreshStepLabels, Is.EquivalentTo(new[] { "A", "B" }),
446+
"Both steps eligible and matching the manifest → both fresh.");
447+
}
448+
353449
[Test]
354450
public async Task UncacheableReason_UnfingerprintableInput_NamesItemLabel()
355451
{

tests/core/Flowthru.Core.Tests/Step/StepLoggerInjectionTests.cs

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,9 @@ public static Func<IEnumerable<int>, IEnumerable<int>> Create(ILogger logger)
4646
/// (ADR-0005): a <c>[FlowthruStep]</c> class declares
4747
/// <see cref="ILogger"/> as a parameter on its <c>Create()</c>
4848
/// factory; the source generator extracts it as a
49-
/// <see cref="ServiceRef.CSharp"/>; the host resolves it through DI;
50-
/// the step's logged lines hit the captured provider.
49+
/// <see cref="ServiceRef.ObservationOnly"/> (ADR-0010); the host
50+
/// resolves it through DI; the step's logged lines hit the captured
51+
/// provider.
5152
/// </summary>
5253
[TestFixture]
5354
public class StepLoggerInjectionTests
@@ -62,22 +63,25 @@ public sealed class TestCatalog : CatalogAbstract
6263
}
6364

6465
[Test]
65-
public void SourceGenerator_ExtractsILoggerParameter_AsServiceRef()
66+
public void SourceGenerator_ExtractsILoggerParameter_AsObservationOnlyServiceRef()
6667
{
6768
// The [FlowthruStep] generator inspects Create() parameter types
68-
// and registers interface-typed params as ServiceRef.CSharp on the
69+
// and registers interface-typed params as ServiceRefs on the
6970
// step's metadata. ILogger (non-generic) must round-trip through
7071
// this path so the engine's shared "Flowthru"-category logger is
71-
// resolvable at flow-construction time.
72+
// resolvable at flow-construction time — AND it must be emitted
73+
// as the ObservationOnly variant (ADR-0010) so the cache planner
74+
// doesn't cascade uncacheability from steps that only declare a
75+
// logger.
7276
var entry = StepMetadataRegistry.TryGetEntry(typeof(FixtureLoggingStep));
7377
Assert.That(entry, Is.Not.Null,
7478
"ModuleInitializer should have registered the fixture step before any test runs.");
7579
Assert.That(
76-
entry!.Services.OfType<ServiceRef.CSharp>()
80+
entry!.Services.OfType<ServiceRef.ObservationOnly>()
7781
.Any(r => r.ServiceType == typeof(ILogger)),
7882
Is.True,
79-
"Step metadata must record ILogger as a CSharp ServiceRef. Found: "
80-
+ string.Join(", ", entry.Services.Select(s => s.DisplayName))
83+
"Step metadata must record ILogger as an ObservationOnly ServiceRef. Found: "
84+
+ string.Join(", ", entry.Services.Select(s => $"{s.GetType().Name}({s.DisplayName})"))
8185
);
8286
}
8387

0 commit comments

Comments
 (0)