fix(kernel): record scoped-admission effect failures once, not twice (#244) - #255
Conversation
…244) The mediation error handler in mediate_and_dispatch_action re-appended action.dispatch_failed and re-called batch_failure for scoped effects whose executor already self-audited and self-batched its typed disposition. This double-counted a single failed scoped draft (two digest rows, two audit rows) and mis-filed pre-effect NotAttempted refusals (target/payload mutation) under the Connector-class vocabulary. Thread a typed executor_recorded flag on DispatchedEffect: the scoped executor owns the record for every disposition it returns, the generic connector path and the un-recorded scoped arms (registry miss, executor error) leave it false. The mediation handler returns early when the executor already owns the record, so each failure is recorded exactly once with parity between the scoped and interactive-approval paths. Closes #244
…est write fails (#244) The scoped executor commits its typed failure record (settle_effect / append_audit draft.creation_failed) BEFORE the secondary, fail-closed owner-digest batch_failure. A batch-store error after that point returned a bare Err, which dispatch_scoped_effect mapped to executor_recorded=false -> the mediation handler re-audited action.dispatch_failed: a double-record on a genuine failure path. Add RecordedEffectError { disposition, source }: the four post-record batch_failure arms route through batch_then_record, returning the settled disposition on success or a RecordedEffectError on a fail-closed digest error (never a bare Err, never a swallowed success that would drop the digest with no redrive). Extract the executor-result -> DispatchedEffect mapping into a pure dispatched_from_executor that downcasts it to executor_recorded=true with the settled disposition while still surfacing the error for redrive. Prove it with a pure mapping test and a fail-closed digest test using the artifact store's existing set_fault_put_for_test seam.
…iteral (refs #251/#252) Cross-merge fixup: #251 added the required BriefcaseSection.origin field and updated existing literals; #252 landed a private_section() test literal without it. Their combination on main broke cargo test compilation. Add the missing 'origin: None,' matching the sibling literal in the same file. Coordinator-approved cross-merge fixup unblocking the #244 gate.
|
Warning Review limit reached
Next review available in: 32 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
What
Record a failed scoped-admission effect exactly once instead of twice, and make
the record-once guarantee hold even when the fail-closed owner-digest write
fails after the primary record is committed.
Why
From the Effect Truth architecture-review checkpoint (#244). With the settlement
seams merged (EffectDisposition + settle_reservations #238, begin_effect/
settle_effect #248), the scoped executor owns the durable record for every
disposition it returns. The mediation error handler in
mediate_and_dispatch_action(api/actions.rs) was still re-appendingaction.dispatch_failedand re-callingbatch_failureon top of that record,so one failed scoped draft produced two owner-digest rows and two audit rows,
and pre-effect
NotAttemptedrefusals (target/payload mutation) were mis-filedunder the Connector-class vocabulary.
How
Record-once at the mediation seam. Thread a typed
executor_recorded: boolonDispatchedEffect. ScopedOk(..)arms set it true (the executoralready audited/batched); the generic connector path, the scoped registry
miss (
NoExecutor), and pre-record executor errors set it false. Themediation error handler returns early when the executor owns the record, so
each failure yields exactly one digest row + one audit row, with parity
between the scoped and interactive-approval paths.
Fail-closed provenance. In
create_approved_draftthe primary failurerecord (
settle_effect(..ConfirmedFailure..)/append_audit) commitsbefore the secondary, deliberately fail-closed
batch_failure. A batch-storeerror after that point previously returned a bare
Err, which mapped toexecutor_recorded: falseand made the handler re-audit — a double-record ona genuine failure path. Added
RecordedEffectError { disposition, source }:the four post-record
batch_failurearms now route throughbatch_then_record, returning the settled disposition on success or aRecordedEffectErroron a fail-closed digest error — never a bareErr(which double-records) and never a swallowed success (which would drop the
digest with no redrive). The executor-result ->
DispatchedEffectmapping isextracted into a pure
dispatched_from_executorthat downcasts it toexecutor_recorded: truewith the settled disposition while still surfacingthe error for redrive.
Tests
scoped_admission_outcome_tests:failure_after_attempt_cancels_reservationand
pre_effect_refusal_cancels_reservation_without_writingassertexactly-once counts (1 executor audit, 0
action.dispatch_failed, correctdigest-row count).
scoped_admission_parity_tests:footprint;
recorded_effect_error_maps_to_executor_owned_surfaced_failureandplain_executor_error_stays_unrecorded_and_fails_closedprove the pureprovenance mapping;
digest_write_failure_after_settle_surfaces_recorded_effect_errorprovesthe fail-closed digest path (1
draft.creation_failedaudit, 0 digest rows,error surfaced) using the artifact store's existing
set_fault_put_for_testseam.
./scripts/check.shpasses (45 spec checks + full cargo suite); rebased onorigin/main.
Cross-merge fixup (refs #251/#252)
Rebasing onto origin/main surfaced a semantic merge conflict independent of
#244: #251 added the required
BriefcaseSection.originfield and updatedexisting literals, but #252 landed a
private_section()test literal withoutit, breaking
cargo testcompilation. Added the missingorigin: None,(matching the sibling literal in the same file) as a separate commit. Coordinator
approved.
Implementation notes (five-line summary)
action.dispatch_failedand re-calledbatch_failurefor scoped effects the executor had already recorded, double-counting failures and mis-classing pre-effect refusals as Connector-class.executor_recordedflag onDispatchedEffect; the handler returns early when the executor owns the record.Ok(..)arms set it true; generic path, registry-miss, and pre-record errors set it false.RecordedEffectError(keeps the failure visible/redrivable) while preserving record-once provenance, instead of a bareErrthat double-records or a swallowed success that drops the digest.set_fault_put_for_testseam)../scripts/check.shpasses (45 spec checks + full cargo suite); rebased on origin/main.Closes #244
Summary by cubic
Record scoped-admission failures exactly once and preserve provenance when the post-record owner-digest write fails. Previously, the mediator re-appended action.dispatch_failed and re-batched after executors already recorded, causing duplicate digest/audit rows and misclassifying pre-effect refusals.
Written for commit 29f09fa. Summary will update on new commits.