Skip to content

fix(audit): pair every audit request entry with a response (WP35) - #1601

Merged
jeremi merged 32 commits into
jeremi/audit-simplificationfrom
fix/audit-pairing-wp35
Sep 26, 2026
Merged

jeremi merged 32 commits into
jeremi/audit-simplificationfrom
fix/audit-pairing-wp35

Conversation

@jeremi

@jeremi jeremi commented Sep 26, 2026 •

Copy link
Copy Markdown
Member

Pull Request

Summary

Closes WP35 of #1561 (audit request/response pairing). Every caller-requested operation now writes one audit request entry and at least one response entry under the same correlation (PR #1560 decision D2). BREG-V1-27 returns to enforced.

Base: jeremi/audit-simplification.

Closes #1576
Closes #1582
Closes #1592
Closes #1585
Closes #1588
Closes #1589
Closes #1597
Closes #1587
Closes #1590
Closes #1593
Closes #1575
Closes #1507

Mechanism: one shared request handle in registry-platform-audit

AuditWriter::begin(schema, correlation, request, unfinished) -> AuditRequest appends the request entry and returns a #[must_use] handle that owes the response:

  • AuditRequest::respond / finish append a response in the request's own schema and correlation, awaited and fail closed.
  • Any accepted response appended through AuditWriter::append with the same schema and correlation also answers the oldest open handle. This lets products whose terminal entry is built far from the attempt keep that code: they only hold the handle for the operation's lifetime.
  • A handle dropped unanswered writes the product's unfinished record as the response. That covers an early ? return, a panic, and a canceled future (a request timeout or a disconnected caller). Stream destinations hand it to a dedicated writer thread that the writer joins when it is dropped, so a stalled stream never blocks the dropping task. The file destination queues it into the group commit and spawns the flush. If the runtime shuts down first, as in a one-shot operator command, Drop for GroupCommitFile writes the queued line synchronously, unless a blocking write is still in flight.
  • begin writes and registers the request inside a spawned task, and append / respond settle their bookkeeping there too. A caller canceled while its request or response entry is being written therefore still gets exactly one answer, and a response accepted after the caller left is the only one. The unfinished record is validated before the request is written, so a request that could never be answered is refused up front.
  • A response in another schema does not answer, so a cross-schema close like BReg: a refused ingestion run closes its ingestion-schema request entry with a general-schema refusal entry #1597 stays visible as an unpaired request.

No config knobs, metrics or CLIs were added.

Why this shape. A closure or scoped API would need every BReg coordinator restructured around its commit. It would still need a drop guard for cancellation, which is the #1507 / #1563 case. Panicking in Drop is wrong: cancellation is legitimate, and a panic during unwind aborts. The drop write cannot be awaited, so it cannot fail closed. Known outcomes (refusals, failed transitions) are therefore written explicitly and awaited; the drop path is the backstop for exits no code path names. A stopped writer refuses the drop write too, which is the existing "writer refused; repair and restart" state that readiness reports.

#1507, settled consistently with #1563. A timed-out or canceled request's accepted attempt is answered by exactly one unfinished response when its handle drops. It is never lost unless the writer has already stopped, and never duplicated, because an answered handle writes nothing on drop. This is independent of the read path's connection cancel guard that #1563 tracks; that change needs nothing from this mechanism.

Call-site audit

"Before" is the base branch. "After" is this PR.

Product Request writers Gaps found Fix
Platform AuditWriter::append(request) No pairing primitive begin / AuditRequest
Casework CaseworkAudit::begin at 27 sites: store.rs (10), review.rs (6), assignment.rs (4), clocks.rs (2), task_grants.rs (4), source_retention.rs (1) Every ? or refusal exit before complete, plus the internal failures of commit() (invalidations, ensure_terminal, the database commit), left the request unpaired. add_review_note wrote no entry at all. AuditOperation holds AuditRequest, and the unfinished record is {event, outcome: "unfinished"}. add_review_note is audited as casework.review_note_added.
Scheduling audit_request at 6 commitment sites Store, Query and Hooks failures, FactsStale, KeyReused and KeyExpired wrote no response. record_refusal swallowed a failed append (fail open). Cancellation left requests unpaired. Explicit unfinished responses with closed reasons (commitment.failed, commitment.facts-stale, idempotency.key-reused, idempotency.expired). Refusals fail closed (service.unavailable). The handle covers everything else (commitment.unfinished). SEC-14 is revised.
Scheduling Hook delivery seam Terminal entries were written before the commit (shared worker) See the webhook row
BReg 12 Attempt sites: mutation.rs (2), mutation/action.rs (4), mutation/request.rs (2), postgres/read.rs (2), revision_read.rs, history_read.rs Any ? after the attempt (bindings, transactions, fault points, timeouts, the evidence preflight). Post-read processing failures (#1593). Discarded terminal errors. begin_pre_io_audit / begin_action_pre_io_audit return the handle. Reads write the Refused terminal and log refused terminals. record_pre_io_audit now refuses Attempt.
BReg Reviewed apply (request_reviewed_apply) The receipt preflight and review-authority reads ran before the attempt (#1587) The attempt is begun before the preflight and held across evidence apply and the action (attempt_recorded = true)
BReg migration_reconcile (Completable / Revertible) A transition error returned after the request with no response (#1592 case 1, #1585) Handle plus an explicit failed response
BReg request_retention::erase The transaction error path was unpaired. The terminal was appended before external deletions. A post-commit refusal was indistinguishable (#1592 case 3, #1589). Handle; a refused or failed response; external deletions before the terminal, with pending and tombstone counts; ErasureUnaudited → request_retention.erasure.unaudited
BReg request_retention::cleanup_attachments The deletion error path was unpaired Handle plus failed
BReg Ingestion append_run_request (create, cancel, chunk replay, chunk receipt) Refusals after the ingestion-schema request were closed by a breg-audit/v2 refusal (#1597) RunAttempt; the service answers in breg-ingestion-audit/v1 (refused) and returns IngestionRefusal { answered }, so the handler skips the general refusal. A fresh chunk's refusal belongs to the batch mutation's own v2 pair and is no longer recorded twice.
BReg evidence-retention erase-expired Wrote no entry (#1590) breg-evidence-retention-audit/v1: a request naming the cutoff and a response with the erased count or failed, through the bregctl companion destination
BReg Webhook worker (registry-platform-hooks) Entries were appended before commit, so a failed commit left an entry for a transition that never happened. Replay was response-only (#1592 case 2, #1588). Attempt before the lease commit, answered with worker_interrupted if that commit fails. Terminals and expiries after commit. Replay: a replay_requested request, then a replay_committed or replay_refused response.
BReg history rebaseline, standalone history erase, field-encryption erase-and-rebaseline lifecycle, attachment verification worker The request was appended, then refusals (such as CoverageComplete or NoPendingPlaintextHistory), database errors, and the worker's 180-second budget returned without a response begin_maintenance_request holds the request through the run, and the lifecycle's request is held through erase_field_encryption_history. An early end answers unfinished.
Render render_route The correlation was the caller's Idempotency-Key (#1575). A dropped call was unpaired. A rendered entry could precede a failed response build. A server-drawn correlation (the key is kept as correlationId); the handle writes unfinished; the response is built before the rendered entry
Relay, Evidence Access entries Relay is paired except on cancellation. Evidence has a few single-stage ? exits. Out of scope; follow-ups

Evidence

Environment: PostgreSQL 17.x with PostGIS in this container (SSL off, port 5433), one disposable database per suite. Every PostgreSQL suite below ran against that database; none skipped for a missing URL. Base jeremi/audit-simplification at 196affa.

Gate Result
cargo fmt --check pass
cargo check --locked --workspace --all-targets pass
cargo clippy --workspace --all-targets -- -D warnings pass
cargo test --locked for every workspace member pass, except root-only tests: the disk allowance cannot hold every test binary at once, so this ran package by package (-p <pkg> --no-fail-fast, binaries deleted between packages) rather than as one --workspace build. The failures are six registry-platform-audit permission tests, registry-caseworkctl dev::tests::config_change_keeps_the_owner_after_created_container_cannot_be_saved, and registry-evidencectl access::revoke_leaves_the_record_untouched_when_the_private_directory_cannot_be_removed. Each relies on a 0o500/0o200 path that root ignores. The platform-audit ones fail identically on the base, and the other two are in crates this PR does not change. The platform-audit and caseworkctl ones pass (91/91 and 88/88) when rerun as an unprivileged user.
BReg products/breg/scripts/test-postgres.sh --lane postgres targets pass: all 69 targets with the lane's own feature sets, run one target at a time for disk, including the three Evidence targets with the real evidence binary, except postgres_startup::prepared_server_wires_services_and_static_jwks_readiness_tracks_database. That test writes {}\n as a "completed" companion audit file, and the base's own current-format check (#1581) refuses it. The test and that check are unchanged here.
BReg --lane immediate-actions (postgres_immediate_actions) pass (18/18)
BReg suites rerun after the last BReg commit (postgres_history_rebaseline, postgres_history_erasure, postgres_history_migration, postgres_field_encryption, postgres_change_requests, cargo test -p registry-bregctl) pass
Casework PostgreSQL suites (all rows of the README table) pass, except review_postgres::cancellation_and_final_decision_commit_exactly_one_terminal_result, which fails deterministically on the base head without these changes
Scheduling postgres_commitments, schedulingctl intents_postgres and records_apply_postgres pass (89, 1, 2)
Render cargo test -p registry-render pass
python3 products/breg/scripts/validate_product.py and test_validate_product.py pass (38 tests)
products/breg/scripts/check-contracts.sh pass
products/casework/scripts/check-checkpoint.sh pass
products/scheduling/scripts/check-checkpoint.sh and check-contracts.sh pass
docs/site npm test; check:markdown, check:style, check:evidence-anchors, check:content pass (614 tests)
docs/site check:evidence-links inconclusive: it fails only on standards links to commits and tags missing from this shallow clone, in files not touched here. The full npm run check build was not run for lack of disk.
OpenAPI and CLI records not needed: no endpoint or clap definition changed; the new bregctl diagnostic is failure-report content only
Identifier catalog not needed: no runtime schema changed

Fault-injection proof. These tests were run against the unfixed code first and failed: the five new Scheduling tests, the Casework review-note test, and the Render serve pairing test. The Render concurrency test and the webhook terminal ordering were also mutation-checked: the fix was reverted locally, the test failed, and the fix was restored. The existing tests that pinned the old unpaired behaviour ("retains only its attempt", "a refused rebaseline records its request and no committed response") failed once each fix landed and were updated to the paired expectation. The remaining new BReg fault tests (reconcile, retention, ingestion, Evidence retention, strong-ETag read) were written after their fixes. Each asserts a response entry that the base code, on reading, never writes, but they were not executed against the base.

Notes

Shared-seam change

registry-platform-hooks::DeliverySeams::record_audit no longer receives the transaction, and the worker now calls it after commit for terminals, expiries and replay outcomes. This also changes Scheduling's hook seam (crates/registry-scheduling/src/hooks.rs). One behaviour changes: a terminal entry the audit destination refuses now leaves the committed disposition in place, and the writer stays stopped, instead of rolling it back and redelivering after lease expiry. That matches how Scheduling already treats a response refused after commit.

Security review notes (audit integrity is security-sensitive)

Path Threat Enforcement point Negative test
Any product request An unpaired request makes "orphaned request" useless as an alarm, and a crash looks the same as an ordinary error AuditRequest drop writes unfinished; explicit awaited responses for known outcomes writer::tests::a_request_dropped_unanswered_writes_its_unfinished_response, a_canceled_operation_pairs_its_request_in_the_file, a_panicking_operation_pairs_its_request, a_command_that_exits_after_an_early_return_pairs_its_request, a_response_in_another_schema_does_not_answer_the_request
Casework refusals A refused claim leaves a request with no outcome AuditOperation holds the handle postgres_transactions::a_refusal_after_the_request_entry_pairs_it_with_an_unfinished_response, audit::tests::a_requested_operation_without_a_response_record_is_refused
Casework review notes A note added with no audit trace add_review_note begin/commit; text and audience are never recorded review_postgres::review_notes_are_audited_without_their_text
Scheduling refusal fail-open A refusal answered while its audit entry was lost record_refusal returns service.unavailable a_refusal_whose_response_entry_is_refused_answers_service_unavailable, a_permission_refusal_the_destination_refuses_answers_service_unavailable
Scheduling undecided commitments Store failure, stale facts and key refusals leave the request unpaired Explicit unfinished responses a_failed_capacity_transaction_pairs_its_request_entry, a_records_swap_under_a_commitment_pairs_its_request_entry, an_idempotency_key_refusal_pairs_its_request_entry
BReg reads Post-processing failures and timeouts leave the attempt unpaired Refused terminal; handle postgres_read::real_postgres_read_is_authorized_bounded_minimized_and_audit_gated (StrongEtag and BeforeTerminalAudit faults)
BReg reviewed apply Protected reads with no audit trace Attempt before the preflight postgres_change_requests::cached_review_result_cannot_authorize_fresh_apply_but_committed_receipt_recovers_offline
BReg reconcile A failed transition leaves the request unpaired failed response postgres_migration::real_postgres_reconciliation_completes_reverts_or_refuses_a_pinned_target (a held registry_state row fails activation)
BReg retention erase A data-destroying erasure unaudited, or audited as finished while objects remain Response after external deletions; ErasureUnaudited postgres_request_read_retention::request_detail_erasure_pairs_its_request_entry_on_every_outcome, postgres_request_upgrade_retention::operator_retention_service_counts_pages_erases_under_forced_rls_and_audits, request_retention::tests::an_unaudited_erasure_stays_distinct_from_a_refused_one
BReg Evidence retention Bulk erasure of retained Evidence with no audit Request plus response with cutoff and count postgres_action_evidence_retention::expired_request_evidence_erases_only_retained_uses, retention_refuses_misbound_database_with_identical_roles_and_catalog_drift
BReg ingestion Refusal closed in another schema Ingestion-schema refused response; handler skips the v2 refusal postgres_ingestion_runs::a_refusal_after_the_ingestion_request_is_answered_in_the_ingestion_schema
BReg maintenance commands A refused or failed rebaseline or erasure leaves its request unpaired Held request answers unfinished postgres_history_rebaseline::rebaseline_refuses_while_maintenance_is_not_ready and rebaseline_restores_snapshot_coverage_from_current_state_after_an_erasure; postgres_history_erasure::field_encryption_erasure_uses_flip_provenance_for_structured_plaintext
Webhook worker A journal entry for a rolled-back transition; replay response-only Append after commit; interrupted answer on a lease commit failure; replay pair postgres_webhook_delivery::real_postgres_webhook_delivery_retry_dead_letter_replay_is_package_bound_audited_and_confined (deferred-trigger commit failures)
Render A caller-chosen key collides pairing across calls Server-drawn correlation server::tests::concurrent_calls_sharing_an_idempotency_key_pair_their_own_entries, serve::a_render_writes_a_request_entry_then_a_response_entry_sharing_correlation

Which of these tests were run failing first is stated under Evidence.

Review fixes

The Codex review raised five findings, and all five are fixed:

  • Cancellation during begin or append: a request is now paired even when its caller is canceled mid-write (a_request_canceled_while_its_entry_is_written_is_still_paired, a_response_accepted_after_its_caller_left_is_the_only_answer, a_response_appended_after_its_caller_left_still_answers_the_request).
  • An oversized unfinished record: now refused before the request is written (an_unfinished_record_too_large_to_write_refuses_the_request).
  • A stream drop write blocking the dropping task: now on the dedicated writer thread (dropping_a_request_never_waits_on_a_stalled_stream).
  • Webhook worker, commit acknowledgement lost: the worker reads the transition's committed state on a fresh connection before choosing a terminal or an interrupted answer. The rolled-back branch is covered by the deferred-trigger cases in postgres_webhook_delivery. The committed-but-unacknowledged branch cannot be reproduced against a local PostgreSQL, so it has no dedicated test.

A second Codex review of 5de578f raised four more findings, and all four are fixed:

  • An operator replay whose caller left: the replay runs to its response in a spawned task. The new cancellation case in postgres_webhook_delivery fails with that task removed.
  • A committed replay the worker has already claimed: the replacement generation, which only a replay writes, proves the reset committed (a_replay_whose_reset_committed_holds_after_the_worker_moves_it).
  • An erasure whose commit returned an error: the detail is read back on a fresh connection, and the request is answered with the erasure, failed, or unfinished. The refused-commit case is in request_detail_erasure_pairs_its_request_entry_on_every_outcome.
  • A replayed ingestion receipt: the answer is built inside the release transaction, before the disclosure entry, so nothing fallible follows an accepted disclosure.

After these fixes, postgres_webhook_delivery, postgres_request_read_retention and postgres_ingestion_runs pass, as do the registry-platform-hooks tests (149), fmt, and clippy on the changed crates.

A third Codex review of 09106c9 raised three more findings, and all three are fixed:

  • A handle dropped while an appended response is written: AuditWriter::append now claims the request before its write starts (a_request_dropped_while_an_appended_response_is_written_is_answered_once, which fails with the claim removed).
  • Evidence retention and migration reconcile after a commit error: each reads the durable state back and answers with the committed outcome, failed, or unfinished when that state cannot be read. expired_request_evidence_erases_only_retained_uses adds a refused commit.

The other request paths here already record an unknown outcome as unfinished. A BReg mutation's terminal entry is written before its commit and gates it, which is the base design.

After these fixes, the BReg targets postgres_action_evidence_retention, postgres_migration, postgres_read and postgres_mutation pass, as do the Casework and Scheduling PostgreSQL suites (apart from the known Casework failure below), the registry-platform-audit tests apart from the root-only ones, and the Casework and Scheduling checkpoint and contracts scripts.

A fourth Codex review of 8ee314f raised two more findings, and both are fixed:

  • A file response dropped outside a runtime while the file state is busy: it now waits for the lock instead of giving up after a bounded loop (a_request_dropped_outside_a_runtime_while_the_file_is_busy_still_pairs, which fails without the fix).
  • A retry or dead letter the worker has moved past: a later attempt of the same generation proves a retry committed, and a later generation proves a dead letter did (a_committed_retry_holds_after_its_next_attempt_is_claimed, a_committed_dead_letter_holds_after_it_is_replayed).

After these fixes, the registry-platform-audit tests (apart from the root-only ones), the registry-platform-hooks tests (151), postgres_webhook_delivery, Scheduling postgres_commitments and clippy pass.

Follow-ups (out of scope, not fixed here)

  • Casework review_postgres::cancellation_and_final_decision_commit_exactly_one_terminal_result fails deterministically on the base head without these changes (both the cancel and the decision succeed).
  • submit_ingestion_chunk performs run and binding reads before its ingestion request entry (the same class as BReg reviewed-apply preflight runs database reads before the request audit entry #1587).
  • The attachment verification worker still appends its terminal entry before its verdict commits. This is deliberate: a refused entry rolls the verdict back and the job retries. But a failed commit after an accepted terminal leaves an entry for a verdict that did not commit, the BReg: audit entries moved outside Postgres transactions can be orphaned or describe uncommitted transitions #1592 case-2 class, on a background path outside D2.
  • Response-only operator reads (list, dry runs, ingestion list and read) and webhook expiry terminal duplicates.
  • Evidence access entries: a few single-stage ? exits; Relay: cancellation.
  • Root-only permission tests: six registry-platform-audit writer tests and caseworkctl dev::tests::config_change_keeps_the_owner_after_created_container_cannot_be_saved fail when run as root and pass as an unprivileged user.
  • request_store::load_postgres_env still reads a hard-coded /private/tmp/... path. Tests here set BREG_TEST_DATABASE_URL and do not rely on it.

DCO

  • Every commit includes a Signed-off-by trailer.
  • I reviewed the submitted changes and am responsible for the contribution.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YcLEc9VkbA2p5WSEZXugQz


Generated by Claude Code

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-26T20:10:43.658982Z 3dd1be2 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 04ef524d41

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/registry-platform-audit/src/writer.rs Outdated
Comment thread crates/registry-platform-audit/src/writer.rs Outdated
Comment thread crates/registry-platform-audit/src/writer.rs Outdated
Comment thread crates/registry-platform-hooks/src/delivery/service.rs Outdated
Comment thread crates/registry-platform-audit/src/writer.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5de578f1b0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/registry-platform-hooks/src/delivery/service.rs
Comment thread crates/registry-platform-hooks/src/delivery/service.rs Outdated
Comment thread crates/registry-breg/src/request_retention.rs
Comment thread crates/registry-breg/src/postgres/mutation.rs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

transaction
.commit()
.await
.map_err(|_| MutationError::Unavailable)?;

P1 Badge Reconcile an ambiguous Evidence erasure commit

If PostgreSQL commits the expired-Evidence deletion but the connection disappears before acknowledging commit, this maps the result to Unavailable, after which erase_expired records a durable failed response even though the protected material was erased. Re-read the retained Evidence state on a fresh connection, as the request-retention path does, before deciding whether to record erased, failed, or an uncertain outcome.

AGENTS.md reference: products/breg/AGENTS.md:L175-L182

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/registry-platform-audit/src/writer.rs Outdated
Comment thread crates/registry-breg/src/migration_reconcile.rs Outdated

jeremi commented Sep 26, 2026

Copy link
Copy Markdown
Member Author

This answers the Codex review-body finding on action_evidence_maintenance.rs about reconciling an Evidence erasure commit that returned an error. It is fixed in 8ee314f.

erase_in_transaction fixes the deletion's cutoff at the start of its transaction. When the commit returns an error, erase_expired checks on a fresh connection whether retained Evidence at or before that cutoff remains, then answers:

  • None remains: erased, with the count.
  • Some remains: failed.
  • State can't be read: unfinished.

postgres_action_evidence_retention::expired_request_evidence_erases_only_retained_uses now starts with a commit refused by a deferred trigger. It checks for a failed response with the material still retained, before the ordinary erasures.


Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8ee314f7ac

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/registry-platform-audit/src/writer.rs
Comment thread crates/registry-platform-hooks/src/delivery/service.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8f329cac50

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +658 to +659
tokio::spawn(async move {
let result = writer.write(&entry).await;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Settle claimed requests when the spawned task is canceled

When the Tokio runtime shuts down while this task is awaiting the write, the task is dropped before lines 665–671 clear claimed/in_flight; the owning AuditRequest then observes an in-flight response and skips its unfinished entry, leaving a durable request permanently unpaired. AuditRequest::respond has the same non-RAII bookkeeping pattern. Fresh evidence after the earlier cancellation fix is that the spawned task survives cancellation of its caller but not cancellation of the runtime itself, so the claim needs a drop guard that settles or queues the unfinished response.

AGENTS.md reference: products/platform/AGENTS.md:L29-L31

Useful? React with 👍 / 👎.

Comment on lines +2723 to +2725
let _attempt = self
.begin_action_boundary_audit(claims, input.route_id, input.correlation)
.await?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Carry the Evidence action handle through finalization

For every Evidence-backed action that passes preflight without replaying a receipt, _attempt is dropped when this function returns PreparedEvidenceAction, before invoke_evidence_action performs Evidence evaluation and calls finalize_evidence_action. Its destructor therefore writes an unfinished response even though the operation is still running, and finalization later appends a second terminal response under the same correlation. Return the handle with the prepared state or hold it in the outer coordinator until evaluation and finalization finish.

AGENTS.md reference: products/breg/AGENTS.md:L10-L15

Useful? React with 👍 / 👎.

Comment on lines +397 to +407
if reset.is_err()
&& self
.transition_committed(
&PendingAudit {
outcome: DeliveryAuditOutcome::ReplayCommitted,
..replay.clone()
},
None,
)
.await
== Some(true)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Distinguish another replay from this reset

When this replay's reset rolls back or fails before commit, a concurrent replay waiting on the same row can subsequently commit the identical next_generation before transition_committed reads it; this branch then reports the first replay as committed too, even though the API normally rejects the second use of an expected generation. Fresh evidence beyond the generation-only reconciliation fix is that another replay—not only this request—can write that replacement generation, so reconciliation needs a request-specific durable marker rather than generation equality alone.

AGENTS.md reference: products/platform/AGENTS.md:L29-L31

Useful? React with 👍 / 👎.

@jeremi
jeremi force-pushed the fix/audit-pairing-wp35 branch 2 times, most recently from 3815eb4 to 795f5cc Compare September 26, 2026 19:32

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 795f5cc221

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +652 to +655
state.in_flight -= 1;
state.in_flight == 0 && state.dropped
});
if owes_unfinished {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Wait for the actual write before emitting unfinished

When runtime shutdown cancels the async response task after its stream or file write has started, that underlying blocking write can still finish successfully, but this guard decrements in_flight and allows the request handle to emit unfinished. The journal then contains both the real terminal response and an unfinished response for one request. Fresh evidence after the new drop-guard fix is that bookkeeping is still tied to the cancelable async task rather than the cancellation-surviving write operation.

AGENTS.md reference: products/platform/AGENTS.md:L29-L31

Useful? React with 👍 / 👎.

Comment on lines +142 to +143
match self.expired_evidence_remains(cutoff).await {
Some(false) => Ok(erased),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Tie erasure recovery to this transaction

When this erasure's commit rolls back after returning an ambiguous error, a concurrent erase-expired invocation can delete the same rows before expired_evidence_remains runs; Some(false) then attributes the second invocation's deletion to the first and records the first request as erased with its speculative count. Fresh evidence beyond the prior read-back fix is that the recovery predicate is not transaction-specific; use the original transaction status or a request-specific durable marker instead.

AGENTS.md reference: products/breg/AGENTS.md:L175-L182

Useful? React with 👍 / 👎.

outcome: DeliveryAuditOutcome::ReplayRequested,
disposition: DeliveryAuditDisposition::ReplayPending,
};
self.seams.record_audit(replay.record()).await?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Audit replay requests before protected lookup

When an operator replay names an absent, stale, forbidden, expired, or nonterminal delivery, replay_in has already opened a transaction and read protected delivery state but returns before this request entry is written, so there is neither a request nor the newly promised replay_refused response. This also makes the newly enforced BREG-V1-27 claim false for replay refusals; begin a minimized replay audit before the lookup and answer it on every refusal path.

AGENTS.md reference: products/platform/AGENTS.md:L29-L31

Useful? React with 👍 / 👎.

Comment on lines +324 to +326
`request-retention erase` deletes the external attachment objects before it records its response,
which states how many objects still wait for deletion; when that deletion pass itself fails, the
count is `null` and the command reports the failure. If the erasure committed but its response

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Document the actual erasure audit order

The implementation now records the committed erasure response before calling retry_external_deletions, and that response contains no pending-object count; if the deletion pass fails, the response is already durable and the command returns the retry error. These lines state the opposite ordering and promise a nullable count, which can mislead operators investigating a destructive partial cleanup. Update this procedure to match RequestRetentionOperatorService::erase (and the new test that explicitly pins commit recording before retries).

AGENTS.md reference: docs/site/AGENTS.md:L14-L16

Useful? React with 👍 / 👎.

Comment on lines +1010 to +1011
if let Some(unfinished) = unfinished {
writer.append_detached(&AuditEntry::response(key.0, key.1, unfinished));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Await unfinished responses before releasing failures

When an audited operation exits through an early ?, dropping its handle only calls append_detached; the caller future can therefore return its HTTP or CLI failure before the file response is fsynced or the stream response is written. If that detached write subsequently fails, the failure has already been released and only an error is logged, contradicting the new fail-closed, response-before-release guarantee for the paths that rely on this unfinished backstop. These paths need an awaited cancellation-surviving completion, or the product contract must not claim durable acceptance before release.

AGENTS.md reference: products/platform/AGENTS.md:L29-L31

Useful? React with 👍 / 👎.

Comment on lines +761 to +767
*lifecycle_attempt = Some(
begin_maintenance_request(
request.audit,
lifecycle_request_entry(request, &lifecycle_reference)?,
)
.await?,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep progress responses from closing the lifecycle handle

When this lifecycle scrubs any request snapshots, the later request_scrub_entry is a response with the same FIELD_ENCRYPTION_AUDIT_SCHEMA and lifecycle correlation, so AuditWriter::append immediately marks this new handle answered after the first scrub transaction. If a subsequent record erasure or rebaseline then fails or is canceled, dropping lifecycle_attempt writes no unfinished response, leaving only an intermediate phase: "request-scrub" entry rather than the terminal or unfinished lifecycle outcome promised by this change. Use a distinct correlation/schema for progress entries or keep them from settling the lifecycle request.

AGENTS.md reference: products/breg/AGENTS.md:L175-L182

Useful? React with 👍 / 👎.

Add AuditWriter::begin, which appends the request entry and returns an
AuditRequest handle that owes its response. The handle responds in the
request's own schema and correlation, and a response appended through
AuditWriter::append under the same schema and correlation answers it too.
A handle dropped unanswered writes the product's unfinished record as the
response, so an early return, a panic, or a canceled future still pairs
the request entry. For the file destination that line is queued into the
group commit and, if the runtime shuts down first, written when the last
reference to the file is dropped.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
Hold the shared AuditRequest for each caller-requested operation, so a
refusal, a failed commit, or a canceled request that returns after the
request entry still writes {event, outcome: "unfinished"} as its response
under the same correlation. Audit add_review_note, which wrote no entries:
it now records who added a note and its history event, never the note's
text or audience.

Refs #1576

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
An operation that has no response record to append still withholds its
result, and now writes its unfinished outcome as the response, so its
request entry is no longer left unpaired.

Refs #1576

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
A failed capacity transaction, records replaced under a commitment, and a
reused or expired idempotency key wrote a request entry and no response.
They now write a response with the outcome unfinished and a closed reason
(commitment.failed, commitment.facts-stale, idempotency.key-reused,
idempotency.expired), never an authorization verdict. The request is held
through the shared AuditRequest handle, so a commitment that returns or
is canceled before it answers writes commitment.unfinished.

A refusal, from the ledger or the permission check, is now answered only
once its response entry is accepted, and service.unavailable otherwise,
instead of logging the write failure and answering the refusal anyway.
SCHEDULING-SEC-14 and the runtime configuration reference say so.

Refs #1582

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
The audit correlation was the caller's Idempotency-Key, which two calls
may share, so their request and response entries could not be told
apart. Every call now draws its own correlation and the caller's key is
recorded only as correlationId. The request entry is held through the
shared AuditRequest handle, so a call dropped before its outcome, such
as a disconnected caller, writes an unfinished response. The answer is
built before the response entry is written, so a rendered entry is never
recorded for a document that could not be sent.

Closes #1575

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
Every attempt is held through the shared AuditRequest handle until its
terminal or refusal entry answers it; a request that ends first, through
an error, a timeout, or a dropped caller, writes an unfinished response
under the same correlation. Reads that fail after their rows were read
write the Refused terminal and log a terminal the destination refuses.
Plain record_pre_io_audit now accepts only refusals, so an attempt can
only be written through the handle.

- A reviewed apply records its attempt before the receipt preflight's
  reads and the review authority, and holds it across the action.
- Migration reconciliation answers a transition that fails after its
  request entry with a failed response.
- Request detail erasure answers a refused or failed erasure, deletes
  external attachment objects before its response and records how many
  remain, and reports an erasure that committed without its response as
  request_retention.erasure.unaudited. Attachment cleanup is held too.
- Evidence retention erasure is audited under
  breg-evidence-retention-audit/v1 with its cutoff and erased count.
- An ingestion call refused after its ingestion request entry is answered
  in the ingestion schema and not recorded again as a general refusal.

Refs #1592 #1587 #1597 #1590 #1593 #1507

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
The delivery worker appended terminal and expiry entries inside the
transaction it was about to commit, so a failed commit left an entry for
a transition that never happened. Terminal dispositions and payload
expiries are now recorded after their transaction commits. An attempt's
request stays ahead of its lease commit, so it is on record before
egress; if that commit fails the worker answers it with
worker_interrupted. An operator replay is a replay_requested request
before the reset and a replay_committed or replay_refused response after
it. The seam no longer receives the transaction.

This changes the shared seam, so Scheduling's hook audit follows the
same order.

Refs #1592 #1588

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
A mutation that stops after its attempt now answers it with an
unfinished response, so each fault leaves two audit entries.

Refs #1593

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
A read or tombstone that stops after its attempt now answers it with an
unfinished response, so each fault leaves the attempt and its answer.

Refs #1593 #1507

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
Every caller-requested operation now answers its audit request entry, so
the row returns to enforced with the fault-injection tests that prove the
paths its gap listed.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
History rebaseline, a standalone history erasure, and a field-encryption
erase-and-rebaseline run appended their request entry and then returned
through refusals and database errors without a response. They now hold
the shared request handle through the run, so an early end writes an
unfinished response under the same correlation. The attachment
verification worker holds its attempt the same way, which also pairs a
job its time budget cancels.

Refs #1592

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
Review findings on the AuditRequest handle:

- A caller canceled while its request entry was written could leave the
  request accepted with no handle to answer it. The write and the handle's
  registration now run in one task that outlives the caller, and a caller
  that stopped waiting drops the finished handle, which answers it.
- A response accepted after its caller was canceled was not marked as
  answering its request, so the drop wrote a second, false unfinished
  response. append and respond now record the answer inside the task that
  writes it; a drop while a response is in flight leaves that response to
  settle the request, and writes the unfinished record only if it is
  refused.
- The unfinished entry is validated before the request is written, so a
  request is never accepted with a response it could not write.
- A stream destination writes a dropped request's unfinished entry on a
  dedicated thread, joined when the writer is dropped, instead of blocking
  a runtime thread on a stalled stream. wait_for_detached_entries lets
  tests and shutdown paths read after a drop; the product test captures
  use it.

Refs #1561

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
A PostgreSQL commit error does not prove the transaction rolled back.
After a failed commit the worker now reads the delivery state on a fresh
connection: a terminal disposition or replay reset that did commit is
recorded, and one that rolled back is left to expiry recovery or recorded
as refused. A lease commit whose fate cannot be read is answered as
interrupted, since a second interrupted answer is harmless and a missing
one is not.

Refs #1592 #1588

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
An operator replay now runs to its response in a task of its own, so a
caller that times out or disconnects after the replay_requested entry is
accepted still leaves that request answered.

A reset whose commit acknowledgement was lost is recognized by its
replacement generation, which only a replay writes, rather than by the
pending state the worker may already have moved it out of.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
A request-detail erasure whose commit returned an error now reads the
detail back on a fresh connection and answers its request with the
erasure, failed, or unfinished, instead of assuming a rollback.

A replayed ingestion chunk builds its answer inside the release
transaction, before its disclosure entry, so an accepted disclosure is
never followed by a failure that returns no receipt.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
A response appended through AuditWriter::append now claims the oldest
open request under its schema and correlation before its write starts,
so a handle dropped while that response is written leaves the request
to it instead of also writing its unfinished record.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…rors

An Evidence retention erasure whose commit returned an error now checks
on a fresh connection whether expired material remains, and a
reconciliation transition that returned an error reads the maintenance
state back. Each answers its request with the durable outcome, failed,
or unfinished when that state cannot be read, instead of assuming a
rollback.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
A request dropped outside a Tokio runtime while the file state is busy
now waits for the lock to queue its unfinished response, instead of
discarding that response after a bounded number of attempts. No holder
keeps the lock across an await, and the file's drop or the next group
commit writes the queued line.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
A scheduled retry whose commit acknowledgement was lost is recognized by
a later attempt of the same generation, and a dead letter by a later
replay generation, rather than only by the state the worker may already
have moved on from.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…down

A response claimed between a handle's in-flight check and its close no longer lets the drop write unfinished as a second answer, and a response task dropped by a runtime shutdown releases its in-flight count so the handle still pairs the request. A poisoned open-request map and a detached line lost at shutdown are logged.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…rs it

Preflight dropped the attempt handle when it returned, so an admitted Evidence action was answered unfinished while evaluation still ran and answered again by its terminal or refusal. The admission now carries the held attempt to finalize, and the tests assert every audit request has exactly one response.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
The response of a committed request-detail erasure waited on up to thirty seconds of external-deletion retries, so a slow backend or an interrupted process left the committed erasure answered unfinished. The response is now recorded once the commit is confirmed, and the operation result alone reports the external deletions still pending.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…fails

A run creation, cancellation, blocking, or receipt release whose commit returned an error answered its request entry refused, although the transition may have committed. Those commit errors now answer the request unfinished.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…nown

A replay whose reset changed no row is refused without a read-back, so
a generation another replay wrote is never taken for its own. A claim
whose lease commit failed records each durable recovered or expired
delivery whatever the lease's fate, and a finalize whose terminal
commit cannot be read back answers its attempt as interrupted. The
read-backs wait a short bounded backoff between attempts, and a replay
read-back accepts any later generation.

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
… caller is canceled

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…e recording it unfinished

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…concerns by its pseudonym

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
… lost before recording it

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
… a request ends early

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
@jeremi
jeremi force-pushed the fix/audit-pairing-wp35 branch from 795f5cc to 3dd1be2 Compare September 26, 2026 20:01
@jeremi
jeremi merged commit 3dd1be2 into jeremi/audit-simplification Sep 26, 2026
3 of 4 checks passed
@jeremi
jeremi deleted the fix/audit-pairing-wp35 branch September 26, 2026 20:01

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

(DeliveryAuditPhase::Replay, _) => observed.generation >= event.generation,

P2 Badge Use a replay-specific marker when reconciling reset commits

When replay A's reset rolls back after an ambiguous commit and concurrent replay B commits the same or a later generation before A's read-back, this >= returns true for A, so A emits replay_committed and succeeds despite not performing the reset; both callers can therefore report success. Fresh evidence in the current version is that reconciliation now explicitly accepts a later replay's generation, so the recovery check needs a request-specific durable marker rather than generation monotonicity alone.

AGENTS.md reference: products/platform/AGENTS.md:L29-L31


if self.seams.record_audit(started.record()).await.is_err() {

P1 Badge Guard the lease attempt across cancellation

When deliver_once is canceled while this audit append is waiting for fsync, AuditWriter::append can still durably accept the AttemptStarted request in its spawned task, while dropping claim rolls back the lease transaction. Because no lease then exists for expiry recovery and this direct request entry has no AuditRequest guard, no worker_interrupted response is ever emitted, leaving the request permanently unpaired; retain a cancellation-safe handle or run the request, commit, and response bookkeeping in a task that survives its caller.

AGENTS.md reference: products/platform/AGENTS.md:L29-L31

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant