Skip to content

fix(lifecycle): make progress reporting non-authoritative and failure-isolated #285

Description

@morluto

Severity

Critical lifecycle and correctness — a transport/progress callback exception can abort durable scientific work, leave a preregistered experiment matrix incomplete, or mark a healthy capture failed even though only progress delivery failed

Validated against main at 0a60b288342cd96e2608302fa9fd2917e59d7e58 with application-level callbacks that raise deterministically.

Audit only; no implementation change is included.

Summary

Long-running Flameox application services accept an optional async progress callback. MCP tools pass ctx.report_progress directly into those services.

The callback is awaited inline as if it were part of the domain operation. Exceptions are not isolated into a best-effort notification channel.

Consequences differ by workflow:

  • ExperimentService.run() can publish the experiment protocol and some trials, then propagate a progress exception without publishing the remaining cells as unattempted/cancelled or a terminal experiment result.
  • CaptureService.execute() interprets a progress exception as a capture/startup/internal failure and terminalizes the run accordingly, even when the workload/provider state is healthy.
  • analysis/comparison materialization can compute an expensive result, release its snapshot, then discard publication because progress delivery failed.
  • any MCP session/transport failure can therefore change persisted scientific state rather than only the caller's visibility of it.

Progress reporting must be observational. It cannot be an authority that decides whether work runs, succeeds, fails, or becomes durable.

Direct code evidence

MCP passes transport reporting directly into domain services

execute_capture_plan defines:

async def report(completed, total, message):
    await ctx.report_progress(completed, total, message)

result = await capture_service.execute(plan_id, progress=report)

run_experiment does the same:

async def report(...):
    await ctx.report_progress(...)

result = await experiment_service.run(plan_id, progress=report)

https://github.com/morluto/flameox/blob/0a60b288342cd96e2608302fa9fd2917e59d7e58/src/flameox/mcp/server.py

No adapter catches transport/session errors before they cross into application control flow.

Capture report awaits the callback directly

async def report(self, completed, message):
    self.logger.emit(...)
    if self.progress is not None:
        await self.progress(completed, 8, message)

https://github.com/morluto/flameox/blob/0a60b288342cd96e2608302fa9fd2917e59d7e58/src/flameox/application/capture.py

During startup, calls such as:

await capture.report(1, "Capture plan validated")
await capture.report(2, "Run lifecycle initialized")
await capture.report(3, "Source and environment identity collected")

occur inside exception handlers that translate any generic exception into a capture startup failure.

Later report calls occur inside the main capture lifecycle and can similarly enter failure finalization after process/artifact side effects.

Experiment report has no generic protection

ExperimentService.run() defines:

async def report(completed, message):
    if progress is not None:
        await progress(completed, total_steps, message)

It publishes the experiment protocol, then immediately calls:

await report(1, "Experiment protocol recorded")

Within the trial loop it calls report before planning and after persisting each trial.

The loop catches asyncio.CancelledError and DomainError around capture planning/execution, but a RuntimeError or other callback exception from report() is outside those domain transitions. It propagates out of run().

https://github.com/morluto/flameox/blob/0a60b288342cd96e2608302fa9fd2917e59d7e58/src/flameox/application/experiments.py

Deterministic experiment orphan proof

Use any two-cell experiment plan and this callback:

calls = 0

async def failing_progress(completed, total, message):
    global calls
    calls += 1
    if calls == 3:
        raise RuntimeError("progress transport closed")

A representative execution sequence is:

  1. protocol row is published;
  2. report Experiment protocol recorded succeeds;
  3. report Starting trial 1/... succeeds;
  4. trial 1 is planned, executed, and its row is published;
  5. report Completed trial 1/... raises.

Current result:

ExperimentService.run raises RuntimeError
single-use plan is consumed
protocol row exists
trial 1 row exists
trial 2 has no trial row at all
no unattempted/cancelled completion row is published
no variants/run sets/outcome/comparison/terminal result is published

The code's explicit cancellation/fatal-planning matrix completion is bypassed because the exception came from presentation, not the inner capture try/except.

A retry cannot consume the same plan again. The durable protocol no longer has a complete terminal cell matrix despite the documentation promising remaining cells become unattempted on fatal interruption.

The strongest early witness is even simpler: fail on the first report after protocol publication. The protocol exists with zero trial rows and no recovery identity.

Deterministic capture misclassification proof

Use a valid capture plan and:

async def failing_progress(*_):
    raise RuntimeError("progress sink unavailable")

CaptureService.execute():

  1. consumes the single-use plan;
  2. creates staging;
  3. creates the initial run manifest;
  4. observes environment;
  5. calls capture.report(1, "Capture plan validated");
  6. callback raises;
  7. generic startup exception handler calls terminate_startup_failure();
  8. run is persisted as failed/internal error and staging is cleaned.

The workload/provider did not fail. Only the optional observer did.

If a later report fails after workload execution or artifact preservation, the same class can convert successful expensive work into a failed result and trigger cleanup/finalization paths unrelated to the actual process outcome.

Analysis/comparison consequence

AnalysisMaterializationService.record_async() and ComparisonService.record_async() call progress after the query result has been computed under a pinned snapshot and before durable publication. A callback failure can discard that result and release its retention pin without publishing, amplifying #261's handoff surface and wasting expensive analysis.

Why callback failure is expected to be non-authoritative

The callback type is optional:

Callable[[float, float, str], Awaitable[None]] | None

It carries no domain result and is not included in plan/protocol identity. MCP progress is a notification to the current requester; the durable operation must remain correct after the requester disappears.

A presentation sink may fail because of:

  • client cancellation/disconnection;
  • closed stdio/session;
  • unsupported/invalid progress token;
  • notification serialization error;
  • test/UI callback bug;
  • server shutdown race.

None proves the workload, evidence, or scientific protocol failed.

Violated invariant

For fixed authorized operation inputs:

durable domain outcome(progress=None)
== durable domain outcome(progress=healthy_observer)
== durable domain outcome(progress=failing_observer)

except that the last case may record a bounded notification-delivery limitation/counter.

Progress delivery must not alter side effects, terminal states, trial coverage, or evidence publication.

Proposed direction

1. Isolate progress at one boundary

Use a non-throwing reporter abstraction that:

  • catches notification exceptions;
  • preserves cancellation semantics deliberately rather than swallowing operation cancellation;
  • rate-limits one safe warning/dropped-progress counter;
  • never exposes arbitrary callback exceptions to domain code.

2. Distinguish request cancellation from notification failure

If MCP request cancellation is intended to cancel synchronous work, propagate it through the explicit operation cancellation contract/cancel scope—not by allowing ctx.report_progress to throw at arbitrary phases.

Durable/detached work should continue or reconcile according to its lifecycle policy after the requester disappears.

3. Keep progress outside atomic transitions

Persist domain state first or after according to the operation protocol; progress should observe completed transitions. It must not sit between a source write and the required terminal/recovery write.

4. Test every long-running service

Inject callbacks that fail:

before first side effect
after initial durable record
after process launch
after one trial
after artifact preservation
before/after publication
during terminalization

Acceptance criteria

  • Progress callback exceptions never change capture/experiment/analysis/comparison domain outcomes.
  • The experiment witness still publishes every remaining cell with the correct terminal/unattempted status and returns/reconnects to one durable result.
  • A healthy capture cannot be marked failed solely because progress delivery failed.
  • Expensive computed analysis results are not discarded because the requester cannot receive progress.
  • Notification failure is visible only through a bounded diagnostic/metric/limitation, not arbitrary error text.
  • Explicit operation/request cancellation remains distinguishable and tested.
  • MCP disconnection tests cover synchronous capture, experiments, materialization, and durable detached/setup operations.
  • Application services remain transport-independent; they do not rely on MCP callback behavior for correctness.
  • Architecture checks route new progress reporting through the shared non-authoritative reporter.

Relationship to existing issues

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions