Skip to content

fix(platform)!: refuse audited requests on a stalled stdout audit stream - #2019

Open
jeremi wants to merge 3 commits into
mainfrom
jeremi/audit-stream-deadline
Open

jeremi wants to merge 3 commits into
mainfrom
jeremi/audit-stream-deadline

Conversation

@jeremi

@jeremi jeremi commented Oct 12, 2026

Copy link
Copy Markdown
Member

Summary

A stdout audit destination whose reader stops reading held every audited
request for as long as the reader stayed away. This change bounds that wait in
registry-platform-audit (owning area: crates/, with the platform changelog
and security principles under products/platform).

  • One thread owns the stream and writes the lines handed to it in order.
  • An append waits at most 5 seconds for its line. A destination holding one
    line past that deadline is stalled: the waiting request is refused with the
    added AuditUnavailableReason::Stalled, later requests are refused at once,
    and ready() is false until the destination takes the line. The writer then
    accepts entries again with no restart.
  • A failed write still stops the writer until restart.
  • The queue behind the line being written is bounded at 4,096 lines, and a full
    queue refuses instead of growing.
  • Dropping the writer waits one deadline for the thread, then leaves it.
  • The start and the end of a stall are each logged once, with no entry content.

The deadline is a constant, not a configuration key. A shared audit
configuration block across products is tracked in #2006.

Evidence

  • Unit tests, cargo test --locked -p registry-platform-audit: 145 passed.
    The tests that cover the change are
    a_stalled_stream_refuses_the_append_at_the_deadline_and_later_ones_at_once,
    a_write_error_still_stops_the_stream_after_its_sink_recovers,
    a_stalled_stream_leaves_the_runtime_to_other_tasks,
    a_full_stream_queue_refuses_a_line_instead_of_growing,
    a_runtime_without_a_timer_refuses_the_append_before_queueing_its_line, and
    dropping_a_request_never_waits_on_a_stalled_stream.
  • Consumer test, registry-scheduling unit tests (118 passed):
    a_stalled_destination_is_not_ready_until_it_drains proves a product's
    readiness follows the stall and its end, with the real 5-second deadline.
  • cargo fmt --check and
    cargo clippy --locked --workspace --all-targets -- -D warnings: clean.
  • cargo test --locked --workspace --no-fail-fast on macOS arm64: all targets
    pass except three, with 7 failing tests that do not exercise this change:
    registry-casework --test install_script (5 tests, the installer script
    cannot read its fixture release binary), registry-evidencectl --test access
    (1) and --test jwk_from_pem (1), both stopped by dyld failing to load
    libaws_lc_fips in a spawned evidencectl. The same 7 tests
    fail the same way on this machine with the crates at origin/main.
  • The workspace runs above were made before rebasing onto current main.
    After the rebase, cargo fmt --check, clippy with -D warnings, and
    cargo test --locked were repeated for registry-platform-audit,
    registry-scheduling and registry-schedulingctl: clean, 332 passed,
    0 failed.
  • Static reading: every Tokio runtime the workspace builds enables its time
    driver, except the one test here that proves the refusal without one.
  • Not run: cargo deny check, the PostgreSQL suites that need a disposable
    database, and the docs site checks (no docs site file changes).

Notes

Security-sensitive: audit integrity. Review notes are in
crates/registry-platform-audit/README.md.

  • Breaking: AuditUnavailableReason gains the Stalled variant, and a
    stream append waits on the Tokio timer, so the runtime that appends needs its
    time driver. Without one the append is refused before its line is queued.
  • Caveat recorded in the review notes: the line the destination is holding
    when its caller's deadline passes is still written when the destination
    takes it, so the audit stream can carry an entry for a request that was
    refused. A refused entry does not undo work a product already committed.
  • Known limits, not addressed here: the queue is bounded by line count,
    not bytes; a full queue refuses appends but does not turn ready() false;
    the start of a stall is logged when the next append observes it.
  • Operator documentation under docs/site does not describe the stall
    behaviour yet; that is a follow-up docs change.

Refs #1566

DCO

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

A stdout audit destination whose reader stops reading held every audited
request for as long as the reader stayed away: each append waited on the
stream lock behind the stuck write, nothing bounded the wait, readiness
stayed true, and dropping the writer joined the stuck thread.

One thread now owns the stream and writes the lines handed to it in order.
An append waits at most 5 seconds for its line. A destination holding one
line past that deadline is stalled: the waiting request is refused with the
added AuditUnavailableReason::Stalled, later requests are refused at once,
and ready() is false until the destination takes the line, after which the
writer accepts entries again with no restart. A failed write still stops
the writer until restart. The queue behind the line being written is
bounded, and a full queue refuses instead of growing. Dropping the writer
waits one deadline for the thread, then leaves it.

The start and the end of a stall are each logged once, with no entry
content. The line the destination is holding when its caller's deadline
passes is still written when the destination takes it; the crate README
records that caveat with the review notes.

BREAKING: AuditUnavailableReason gains the Stalled variant, and a stream
append waits on the Tokio timer, so the runtime that appends needs its time
driver. Without one the append is refused before its line is queued. The
schedulingctl test that appended on a runtime without one enables it.

Refs #1566

Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 12, 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-10-13T02:48:26.620239Z 6d11885 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.

Both sides added an entry under Unreleased in products/platform/CHANGELOG.md. Keep both.

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

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

@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: 6d1188590c

ℹ️ 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 +1501 to +1502
if let Some(took) = took.filter(|took| reported || *took >= self.deadline) {
self.report(StreamEvent::StallEnded(took));

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 Emit a stall start before reporting its end

When a detached entry is the only activity and its sink blocks longer than the deadline, no append or readiness check calls stalled(), so stall_reported remains false; this branch nevertheless emits StallEnded based solely on elapsed time. Operators then receive a recovery warning without the promised stall-start error, potentially missing the actual outage. Emit the start event before the end event when took >= deadline and the stall was not previously reported.

Useful? React with 👍 / 👎.

This branch has not been deployed

No deployments
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