fix(runtime): resolve committed write audit outcomes by ohdearquant · Pull Request #2331 · ohdearquant/khive

ohdearquant · 2026-08-30T06:27:49Z

Authored with OpenAI Codex.

Summary

keep the same enqueued audit receiver alive after the admission wait threshold for successful non-degrade-safe operations
preserve bounded admission behavior for allowlisted reads, failed/denied outcomes, pure observability, and pre-enqueue queue exhaustion
make git digest receipts use the same resolved-outcome contract
record the distinction in ADR-133 and the audit batch API documentation

The write handler is never rerun and its audit row is never re-enqueued. A committed domain effect now reports success only after that generation commits, while genuine terminal audit failures remain failures.

Test plan

focused TDD regression: committed non-idempotent write remains pending past the audit deadline, then succeeds once its single audit row commits
cargo test -p khive-runtime --all-features
cargo test --workspace including doctests
cargo check --workspace --all-targets
cargo clippy --workspace --all-targets -- -D warnings
cargo fmt --all -- --check
git diff --check

Closes #2256

ohdearquant · 2026-08-30T06:39:51Z

The problem this fixes is real and the diagnosis is right: reporting a committed non-idempotent write
as failed, on a deadline that only describes the audit lane's own admission, is a genuine defect and
the retry it invites is genuinely unsafe. The refactor is also clean — submit now delegates to
submit_with_wait_policy(row, false), and that path is byte-for-byte the old behaviour including the
comment explaining why AdmissionDeadlineExpired is distinct from a queue-full refusal.

What I want to push back on is the specific remedy, because I think it treats the wrong half of the
problem as the defect.

The bound was not the bug; the reason was

After the deadline elapses, submit_with_wait_policy falls through to a bare rx.await with no
timeout. With admission_deadline defaulting to 5s, this converts a bounded 5-second wait into an
unbounded one for DispatchSucceeded && !degrade_allowlisted — which is the ordinary write path, not
an edge case.

The stated justification is that returning AdmissionDeadlineExpired "would report a false operation
failure and invite an unsafe retry." That is correct, but the harmful property there is that
AdmissionDeadlineExpired is indistinguishable from a failure for which retry is safe. Waiting
forever does not remove that ambiguity — it relocates it to the client, and makes it worse. A caller
that eventually gives up on its own transport timeout gets a cancellation carrying no
AuditTerminalReason at all, which is strictly less information than the reason it replaced, and it
invites exactly the same unsafe retry of the same committed write. The one place a caller could have
been told "your effect committed, do not retry" is the return value that this change removes.

A distinct terminal reason meaning "domain effect committed, audit outcome unresolved, retry is not
safe", returned on a longer but finite bound, would preserve the entire safety property this PR is
after while keeping the caller bounded. That seems strictly better than an unbounded await, and it is
a smaller change than the one already made.

Worth being precise about how narrow the hang actually is, because the surrounding code is careful:
every join outcome in supervisor_loop resolves its waiters — DriverPanicked / DriverCancelled
on a join error, Err(reason) on GenerationResult::Failed, per-row dispositions on Committed, and
fail_driver on a length mismatch. So a driver that dies still unblocks the caller. The unbounded
case is specifically a driver that stays alive and never completes its generation — child.await
inside the supervisor has no timeout either — which is the wedged-store case. That is the one where a
5s answer was doing real work.

(I looked for an outer per-request bound on the serve path and did not find one; that is
"not found by this grep" rather than a claim it does not exist, so please correct me if a transport
timeout bounds this elsewhere.)

Nothing covers the arm that actually changed

write_verb_waits_past_audit_deadline_until_row_commits is a good test — it holds the generation,
asserts the dispatch is still pending 80ms past the deadline, then releases and asserts it resolves
with the committed {"created": true} and the expected counts. It would redden on a revert to
submit, and the pending assertion fails in the safe direction on a slow machine rather than flaking.

But it only covers the generation that eventually commits. The behaviour that changed for the worse
is the generation that never resolves, and there is no arm for it. A test that wedges the driver and
asserts the caller still receives some bounded, correctly-labelled terminal outcome is the one that
would have surfaced the concern above — and under the current design it would have to assert the
caller hangs, which is itself a useful thing to have to write down.

Checked and fine

The predicate producer == AuditProducer::DispatchSucceeded && !admission_degrade_eligible reduces
to DispatchSucceeded && !degrade_allowlisted, which matches the summary exactly. Failed and denied
outcomes keep the bounded wait, correctly — their caller-visible result is already fixed.
persist_git_digest_receipt switching unconditionally looks asymmetric next to that predicate, but
it early-returns PersistenceUnavailable unless result is Ok, so it is success-only by
construction, and its doc comment already declares it "intentionally strict while every other
dispatch audit remains best-effort." The ADR-133 comment above the call site stays true — arguably
more true than before.
Pre-enqueue refusals (PreflightRejected, AdmissionClosed, QueueAdmissionExhausted,
Lifecycle::Failed) all still return before the wait, so the "pre-enqueue queue exhaustion"
carve-out in the summary holds.

No builds or tests were run for this pass; every finding is from source reading, and the checked
boxes in the test plan were not relied on.

ohdearquant · 2026-08-30T17:59:26Z

The strict path drops the caller's bound entirely after the deadline

Following up on the earlier note — there is a sharper version of it, and it is blocking.

At this head, wait_until_resolved waits twice. The first wait is bounded. The second is not:

// crates/khive-runtime/src/audit_batch.rs:558-570
if wait_until_resolved {
    match tokio::time::timeout(self.config.admission_deadline, &mut rx).await {
        Ok(Ok(result)) => return result,
        Ok(Err(_recv_error)) => return Err(AuditTerminalReason::DriverJoinLost),
        Err(_elapsed) => tracing::warn!(
            ?producer,
            "strict audit obligation remains enqueued after the admission wait deadline; \
             waiting for its real terminal outcome"
        ),
    }
    return match rx.await {          // <- no timeout, no cancellation branch
        Ok(result) => result,
        Err(_recv_error) => Err(AuditTerminalReason::DriverJoinLost),
    };
}

The elapsed arm no longer ends the wait — it logs and then falls through to an unbounded rx.await.
On origin/main the same site is a single bounded wait whose elapsed arm terminates:

// origin/main:crates/khive-runtime/src/audit_batch.rs:693,702
match tokio::time::timeout(self.config.admission_deadline, rx).await {
    ...
    Err(_elapsed) => Err(AuditTerminalReason::AdmissionDeadlineExpired),

So the deadline changed meaning: it used to end the caller's wait, and now it only decides when to
emit a warning. For a DispatchSucceeded row outside the degrade allowlist this is the selected
path, which makes a live-but-stalled writer or a generation that never completes hold the caller
indefinitely, and take close_and_drain() / quiesce() with it.

Nothing upstream restores the bound. The request wrapper installs a task-local context but supplies
no independent abort, and this write is not inside the read-phase deadline, so the request deadline
is not an effective ceiling here.

The direction matters: waiting too long is silent and unbounded and consumes request capacity
exactly when the system is already degraded, whereas ending the wait early is loud and recoverable.
The change trades the recoverable failure for the silent one.

The added regression covers a generation that is delayed and then commits. It has no
never-completing driver case, so it passes either way — it cannot distinguish the bounded
implementation from this one.

Suggested direction: keep a finite, cancellation-aware bound after the ordinary admission
threshold, and give the caller a distinct outcome meaning the domain effect committed, the audit
outcome is unresolved, with explicit no-retry semantics — a generic failure here would invite a
retry of an operation that already committed. Then add a case with a driver that never completes;
that is the test that separates the two implementations.

On provenance: an automated pass over this diff ran on a model from the same family as the one that
wrote it, so its output was treated as input evidence only. The three facts above — the unbounded
second await at this head, the base's terminating elapsed arm, and the selection of this path for
ordinary successful dispatch — were each re-derived from the two refs before posting.

ohdearquant

Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.

Verdict on head ac96077: REQUEST-CHANGES, 2 blocking findings. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.

A committed write or git.digest receipt whose audit row was still enqueued past admission_deadline kept awaiting its generation's outcome with no upper bound. A stalled append_events_idempotent call therefore retained the completed write's caller, its request slot, and its audit-lane waiter forever, stopping the driver from draining anything queued behind it and exhausting both request and audit capacity together. Add a second, larger AuditBatchConfig bound, resolution_deadline (defaults to 6x admission_deadline, validated in debug builds to be no shorter than it). Once it also elapses, the caller now gets a dedicated terminal reason, AuditTerminalReason::ResolutionDeadlineExpired, distinct from AdmissionDeadlineExpired: the domain effect has already committed, so it is never retried and the row is never re-enqueued, but the audit outcome itself is now reported as unresolved instead of the wait continuing indefinitely. The row is left exactly where the driver holds it for the driver to resolve independently, same as the existing admission-deadline arm.

rustdoc with -D warnings rejects a public doc link to a crate-private item; the two references to the batch resolver become plain code spans.

ohdearquant

Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.

Verdict on head 08f1e6d: REQUEST-CHANGES, 1 blocking finding. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.

The resolution-deadline fix only bounded how long a caller waits for a stalled audit append; the driver holding that append had no bound of its own, so a permanently stalled EventStore::append_events_idempotent() call kept the driver from ever draining new rows out of the pending queue. Enough submissions piled up to exhaust max_pending_rows, and every later audited write failed with QueueAdmissionExhausted, not just the one that started the stall; a caller giving up on its own deadline freed nothing. supervisor_loop now wraps its await on the generation's child task in a timeout derived from resolution_deadline (3x it, chosen so the driver can never race ahead of a still-waiting caller's own, more specific ResolutionDeadlineExpired reason for the same row). On expiry the generation is resolved with a new AuditTerminalReason::DriverAppendAbandoned for every waiter, removed from the driver's in-flight state, and the loop immediately continues to drain whatever has queued up since — restoring admission capacity for later callers. The underlying append is not cancelled (it may be a blocking call that cannot be safely aborted mid-write); it is handed to a detached task that runs it to completion and discards the eventual result. The caller's already-committed domain effect is never retried and the row is never re-enqueued, matching the existing resolution-deadline contract. Also makes crates/khive-runtime/tests/read_verb_admission_exhaustion.rs run under the default `cargo test -p khive-runtime` invocation instead of only under --features fault-injection,test-internals, via a self dev-dependency that enables those features for the crate's own test builds without affecting downstream consumers. Documents the new bound in ADR-133 Amendment 5 and in audit_batch.md.

Amendment 4 promised a ResolutionDeadlineExpired row is left where the driver holds it for the driver to resolve; Amendment 5 added a second ending for that row, so Amendment 4 now points at it. The Invariants section names Amendment 5 as the second bounded exception to INV-1 beside Amendment 1: the trigger (a generation append that does not return within driver_append_deadline), what happens to the rows in each outcome of the detached append, and how the loss surfaces (the generation snapshot's terminal reason and zero committed rows, flush_failures, pure-producer degradation). Amendment 5 also states the detached-task rate under a store that never returns: one task per driver_append_deadline, bounded in rate and not in count.

ohdearquant

Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.

Verdict on head 79f4d3f: REQUEST-CHANGES, 1 blocking finding. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.

… the cap Once the audit-batch driver abandons a stalled generation (DriverAppendAbandoned), its append is handed to a detached task that keeps running against a store that may never return. Nothing capped how many of those detached tasks could accumulate: a store whose append never returns minted one every driver_append_deadline, each retaining up to max_rows_per_generation events, growing without bound until the process exits. AuditBatchConfig gains max_abandoned_appends (default 4). Before spawning a generation's child, the driver checks how many detached appends from prior abandonments are still outstanding; at or above the cap it treats the store as wedged and sheds the generation instead of attempting another append: no child task, no store call, every waiter resolves immediately with a new terminal reason, AuditTerminalReason::StoreWedged, and flush_failures increments. A detached task that finally returns decrements the outstanding count and records the outcome (commit or failure) on two new counters, AuditBatchHealthMetrics::late_append_commits/late_append_failures, so an operator can see that a store recorded as wedged later drained. Once the count drops back below the cap, the next generation attempts a real append again — recovery needs no timer. Combined with the existing driver_append_deadline bound, this caps the retained-buffer growth a wedged store can cause at max_abandoned_appends times max_rows_per_generation rows. Also corrects stale test-setup documentation: khive-runtime's tests/ integration binaries already get fault-injection and test-internals from the crate's own dev-dependency self-reference, so plain cargo test -p khive-runtime already runs them without an extra --features flag; two doc comments claiming otherwise are fixed, along with a stale "every test in this file arms SUPERVISOR_SLEEP_BEFORE_SPAWN" comment that predated several tests that never arm it. ADR-133 Amendment 5 and its INV-1 "named exceptions" paragraph are updated to describe the cap and shed path instead of the "bounded in rate, unbounded in count" text that is no longer accurate; the audit-batch API doc gains a section describing the driver's own append bound and the abandoned-append cap together.

ohdearquant

Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge.

Verdict on current head: APPROVE, zero blocking findings. This is a comment, not an approval — a human reviewer decides whether to approve and merge.

ohdearquant · 2026-09-07T02:03:43Z

Reviewed at 0c92b8bf. The admission-deadline half of #2256 looks right to me, and the reasoning in the doc comments on AuditTerminalReason and the two counters is unusually easy to follow. Two things from running the current main build tonight.

The defect is live, and it does not only surface as AdmissionDeadlineExpired. Under write contention on 0.8.0, two verbs dispatched in one batch both returned:

internal: audit obligation commit failed for verb "gtd.complete": StoreFailure
internal: audit obligation commit failed for verb "comm.inbox": StoreFailure

status was partial with summary.succeeded: 0. The write had committed: the next call on the same row returned invalid input: task <id> is in terminal state "done"; no further transitions allowed. A comm.send in the same period returned AdmissionDeadlineExpired and was delivered. So the same caller-visible sentence carries at least two terminal reasons, and at least one of them can follow a committed effect.

The gap that survives this change. append_audit_event_best_effort now routes a successful non-degrade-safe operation through submit_until_resolved, which is what removes the false failure at the admission boundary. But when the generation then resolves terminally — StoreFailure, IdentityConflict, DriverPanicked, DriverCancelled — the caller still gets the unchanged message:

"audit obligation commit failed for verb {verb:?}: {reason:?}"

That sentence does not say the domain effect may already be durable, so a caller reading ok: false as "did not happen" retries a committed non-idempotent write. That is the same hazard #2256 describes, moved from the deadline path to the terminal path rather than closed.

The fix is already in this diff for the neighbouring case. GIT_DIGEST_RECEIPT_FAILURE says exactly the right thing:

git.digest writes may have committed, but no durable success receipt was confirmed;
inspect ingest state before retrying

Suggest the general obligation failure carry the same shape whenever the operation's own result was a success, since by construction that is precisely when the effect is durable and the audit is not. Two callers cannot be expected to key retry logic on a {reason:?} enum they have no stable contract for.

Test-population note. The listed regression covers the deadline arm ("remains pending past the audit deadline, then succeeds once its single audit row commits"). An arm where the generation resolves terminally after a committed write would pin the sentence above, and would fail today.

Three conflicts: - `docs/adr/ADR-133`: both sides added an Amendment 3. Main's (2026-09-08, the obligation error carrying the domain disposition) keeps the number, because ADR-174 and ADR-179 already cite "ADR-133 Amendment 3" and mean that one. This branch's three amendments are renumbered 4, 5 and 6 with their dates unchanged, every cross-reference inside them rewritten to match, and the one reference in the INV-1 named-exceptions block that merged cleanly outside the conflict updated with them. A comment above the block records that numbers are allocated in merge order, so the non-monotonic dates read as deliberate. - `crates/khive-runtime/Cargo.toml`: both dev-dependency additions are kept; they are different entries in the same table. - `crates/Cargo.lock`: resolved to main's and regenerated against the merged manifests. The result is purely additive against main, no existing pin changes, and `cargo metadata --locked` resolves. ADR reference lint 427 files, status lint 185 records, both clean; the ADR is `deno fmt` clean.

`AuditObligationFailure::wire_code` is a closed mapping that landed on main after this branch's base, while this branch adds three `AuditTerminalReason` variants. Neither side touched the other's file, so the merge was clean and the merged tree did not compile: E0004 at `khive-runtime/src/error.rs:143`, with `ResolutionDeadlineExpired`, `DriverAppendAbandoned` and `StoreWedged` uncovered. Each variant gets the snake_case arm the surrounding mapping already uses, placed in enum order. No behaviour outside the mapping changes. cargo check --workspace --all-targets passes on the merge result.

One conflict: both sides add the same self dev-dependency on `khive-runtime`. Kept the superset, `fault-injection` plus `test-internals`, with both rationales folded into one comment. The feature table already declares both.

fix(runtime): resolve committed write audit outcomes

cc0ad49

Merge branch 'main' into codex/audit-deadline-committed-result

52fb133

ohdearquant marked this pull request as ready for review September 1, 2026 16:38

Merge branch 'main' into codex/audit-deadline-committed-result

ac96077

ohdearquant commented Sep 3, 2026

View reviewed changes

ohdearquant added 3 commits September 3, 2026 17:16

Merge branch main

1f81d05

docs(runtime): reference the private resolver as a code span

08f1e6d

rustdoc with -D warnings rejects a public doc link to a crate-private item; the two references to the batch resolver become plain code spans.

ohdearquant commented Sep 4, 2026

View reviewed changes

ohdearquant added 3 commits September 4, 2026 07:33

Merge branch main

4e9b5b4

ohdearquant commented Sep 4, 2026

View reviewed changes

ohdearquant added 3 commits September 10, 2026 22:25

Merge main into the audit resolved-wait branch

5de3a95

One conflict: both sides add the same self dev-dependency on `khive-runtime`. Kept the superset, `fault-injection` plus `test-internals`, with both rationales folded into one comment. The feature table already declares both.

Navigation Menu

fix(runtime): resolve committed write audit outcomes - #2331

fix(runtime): resolve committed write audit outcomes#2331
ohdearquant wants to merge 13 commits into
mainfrom
codex/audit-deadline-committed-result

ohdearquant commented Aug 30, 2026

Uh oh!

ohdearquant commented Aug 30, 2026

Uh oh!

ohdearquant commented Aug 30, 2026

Uh oh!

ohdearquant left a comment

Uh oh!

ohdearquant left a comment

Uh oh!

ohdearquant left a comment

Uh oh!

ohdearquant left a comment

Uh oh!

ohdearquant commented Sep 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ohdearquant commented Aug 30, 2026

Summary

Test plan

Uh oh!

ohdearquant commented Aug 30, 2026

The bound was not the bug; the reason was

Nothing covers the arm that actually changed

Checked and fine

Uh oh!

ohdearquant commented Aug 30, 2026

The strict path drops the caller's bound entirely after the deadline

Uh oh!

ohdearquant left a comment

Choose a reason for hiding this comment

Uh oh!

ohdearquant left a comment

Choose a reason for hiding this comment

Uh oh!

ohdearquant left a comment

Choose a reason for hiding this comment

Uh oh!

ohdearquant left a comment

Choose a reason for hiding this comment

Uh oh!

ohdearquant commented Sep 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant