feat(trusty-console): webhook ingress with a spool that is durable before the ack (#5089 step 3) - #5175
Merged
Merged
Conversation
…fore the ack (#5089 step 3) Both existing webhook handlers return 202 and *then* do the work (trusty-review/src/service/webhook.rs:305-309, trusty-analyze/src/service/handlers/review.rs:210-214). GitHub never retries an acknowledged delivery, so every post-ack failure is permanent silent loss with health still reporting green. This builds the console path that replaces them; the two handlers stay live until step 4. POST /api/webhooks/{source} multiplexes review and analyze. The order of operations is the fix: unknown source 404s before any secret handling; the HMAC is verified once over the exact received bytes, with an unset secret and a bad signature both 401 (ADR-0034 §2 unifies the policy to trusty-review's fail-closed answer); the delivery is written and fsync'd to a spool under the console data directory; and only then is a 202 sent. A spool write that fails returns 5xx and never acks, so GitHub keeps the delivery redeliverable. A relay outcome other than an explicit "ack": true leaves the entry pending with an incremented attempt count and a durable reason. Reaching the target is deliberately not enough — treating a successful connect as a successful delivery is the same silent loss one layer down. GET /api/console/metrics/webhooks reports oldest-pending age, pending count and failed-attempt total, red once the oldest entry passes the threshold. The scan runs on the request rather than from a cache, so the signal stays honest if the background retry sweep stops; a spool that cannot be read is red rather than empty. trusty-common gains the two shared entry points this needs rather than a fourth bespoke copy of each: uds::rpc::send_framed_request (the framed UDS transport ADR-0034 §4 names, dialling through connect_hardened) and webhook_hmac (one GitHub signature verifier, returning a three-state verdict so "no secret configured" cannot collapse into permission to proceed). The existing UDS clients and the two HMAC copies migrate in a follow-up. Spawn-on-demand is step 4. Until then no target binds a listener, every relay lands in Unreachable, and that is a durable pending state. Refs #5089 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
…pool green (#5089 step 3 review) Seven findings from the code-critic round on #5175. All three HIGHs were in the machinery the PR adds, not the ordering it fixes. HIGH-1 — the sweep and the request path relayed the same delivery twice. `retry_pending_once` listed every `.json` with no claim and no age filter, so a tick landing inside the <=5s relay window re-sent a delivery `ingest` was still sending. A `ClaimSet` now gives one relay per entry path at a time, released on drop so a panicking relay cannot wedge an entry. HIGH-2 — no backoff, contrary to ADR-0034 §2. Every pending entry was relayed every 60s and each non-ack rewrote the whole base64 body plus two fsyncs; until step 4 binds a listener that is every delivery, forever. `BackoffPolicy` spaces attempts 30s doubling to a 1h ceiling, holds a fresh entry for the relay timeout, and stops at 24 failures. An exhausted entry is never relayed again and never deleted — it holds the health signal red. A per-pass `SWEEP_BUDGET` keeps one serial pass from outlasting its own tick. HIGH-3 — an absent spool directory reported green. `list_pending` answered `ErrorKind::NotFound` with an empty listing, so a removed data dir or an unmounted volume made every POST 500 while the metrics route said `{"status":"ok","pending":0}` — ingress dead, alarm healthy. `Spool` now records whether it was opened; for an opened spool a missing directory is `ReadDir`, which the health scan renders red. A never-opened one is still legitimately empty. Also: - 25 MiB body limit on the webhook sub-router. axum's 2 MiB default 413s a real push/pull_request delivery before the handler runs — no spool entry, no metric, no log. - The relay wire contract moves to `trusty_common::webhook_relay`. Step 4's receivers are trusty-review and trusty-analyze, which cannot depend on the console, so "both halves read this constant" was not true as written. - `persist_new` commits with `hard_link`, not `rename`, so a colliding path fails atomically instead of clobbering an already-acknowledged delivery. `persist_update` keeps rename semantics for `record_attempt`. - The socket-dir comment claimed the paths avoid `$TMPDIR`; they do not. Corrected to say #5099's 0700 uid-keyed dir plus `connect_hardened`'s owner/mode re-check supersedes ADR §3's path rule. Adds the concurrency test class rung 5 requires and the first round lacked — which is where HIGH-1 lived. Verified by reverting each fix in place: the sweep-race, both vanished-directory cases, and the 3 MiB body case all fail against the pre-fix behaviour and pass with it. Refs #5089 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
…lth signal moving (#5089 step 3 review 2) Two HIGHs from the round-2 critic, both in the exhaustion semantics round 1 introduced, plus the runtime-blocking MEDIUM. HIGH-A — exhausted entries accumulated forever and every scan paid full decode for them. The sweep counted `exhausted` and `continue`d; nothing deleted, moved, or aged them out. Since no target binds a listener until step 4, that is EVERY delivery reaching 24 attempts in ~17h and staying in the live set permanently, with both `retry_pending_once` and `scan_health` reading and `serde_json`-decoding it on every pass. An exhausted entry now moves to `webhook-spool/exhausted/` — kept, because it is still an unacknowledged webhook, but off both hot paths. `Spool::scan_metadata` reads receipt times from filenames and opens nothing, which is what `entry_path`'s timestamp prefix was always for; `Spool::load` decodes the single entry the health scan actually needs. HIGH-B — once red, the signal stopped saying anything new. `SpoolHealth` had no exhausted field and `SweepReport.exhausted` reached exactly one consumer, a `tracing::info!` — the log line `health.rs`'s own module doc names as the trap. Worse, `scan_health` took `listing.pending.first()` and exhausted entries are by construction the oldest, so the delivery id, last error and age pinned to the first poisoned entry forever. Day 30's genuinely stuck delivery moved nothing an operator or alert rule reads. `pending` and `exhausted` are now counted separately with their own ids and ages, and `oldest_pending_*` describes the oldest LIVE entry. Any exhausted entry still holds `status: Error`, via its own field rather than by hijacking the oldest-pending ones. `total_failed_attempts` is replaced by `oldest_pending_attempts` — the sum cost one decode per entry per request, which is what HIGH-A is about. `sweep_stops_relaying_an_exhausted_entry` asserted the old behaviour as desired; it now asserts the entry leaves the live set, stays readable with its body intact, and shows up in the exhausted fields. MEDIUM — all spool I/O now runs through `spawn_blocking`. Ingest fsyncs a file and a directory twice per delivery; the metrics route scans two directories per request. Verified by reverting each fix in place: the sweep-quarantine case and the live-versus-exhausted diagnostics case both fail against 92c1ed3. Refs #5089 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
…p 3) `check_test_pointers.sh` was red on two citations that named a prefix of the real test rather than the test: - `schedule.rs:48` cited `claim_set_releases_on_drop`; the test is `claim_set_releases_on_drop_even_when_the_holder_panics`. - `spool.rs:498` cited `sweep_quarantines_an_exhausted_entry`; the test is `sweep_quarantines_an_exhausted_entry_and_stops_paying_for_it`. The pointers were wrong, not the names — both test names say more about what they prove, so they stand. Refs #5089 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
…e the sweep test (#5089 step 3) CI was red on `sweep_does_not_relay_an_entry_the_request_path_is_still_relaying` while it passed locally. Diagnosing it found both a racy test AND a real code defect; they are separate and both are fixed here. The test was racy. It slept 150 ms and assumed `ingest` had reached its claim by then. It does not control that window: printing the whole SweepReport at sleeps of 0/1/5 ms shows every field zero, meaning `list_pending` returned an EMPTY spool — the spawned task had not persisted yet. That is exactly CI's signature (`in_flight: 0` with nothing else set). Moving the write onto the blocking pool in aeba797 lengthened the pre-claim phase enough to lose the race under CI load. The test now rendezvouses with the target stub: the stub signals once it has read the frame and waits to be released, so the sweep runs when the relay is provably in flight. No wall clock, and it drops from 0.64 s to 0.04 s. The code was also wrong. The claim was taken AFTER `persist_new` returned, leaving the entry on disk and unclaimed for the gap between the write landing and `ingest` being re-polled. At sleep=20 ms the sweep reported `acked: 1` — two relays for one delivery, the exact defect the claim exists to prevent. The claim is now taken on `entry_path` before the write; the path is a pure function of the receipt time and delivery id, both already fixed, so the window does not exist. Backoff's first-attempt grace also covered it in production, but two guards that each fully close it is the point. No regression test pins the ordering. The window is one scheduler poll wide, and a rendezvous, a select! loop and a 4-worker parallel hammer all passed against a claim-after-write build as readily as against the correct one. A test that cannot tell the two apart is a false assurance, so none was kept and the code carries a comment saying so. Evidence: the deraced test is 30/30 green under 12-way CPU saturation, and 5/5 red against a build whose sweep ignores claims. Refs #5089 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defect
Both existing webhook handlers return
202and then do the work, so any failure after the ack loses the delivery permanently — GitHub never retries an acknowledged delivery, and every health signal still reports green.crates/trusty-review/src/service/webhook.rs—tokio::spawnat :240, ack at :322-329, failure written only to an in-memoryMutex<Option<String>>at :305-309.crates/trusty-analyze/src/service/handlers/review.rs—tokio::spawnat :210, ack at :216, failure is a baretracing::warn!at :211-213.Neither handler is modified here. They stay live until step 4 retires them; this builds the console path that replaces them.
Resolution
POST /api/webhooks/{source}multiplexesreviewandanalyze. The order of operations is the fix, per ADR-0034 §2:{source}→404, before any secret handling;401, unifying the policy to trusty-review's fail-closed answer;resolve_data_dir("trusty-console")/webhook-spool/— on failure500, and no202is ever sent, so GitHub keeps the delivery redeliverable;202, because step 3 succeeded — not because step 4 did.Per failure arm:
5xxbefore any202. The relay never runs — asserted, not assumed.pending, attempt count and reason written durably, sweep retries. Never deleted on failure.ackis#[serde(default)]false, so a result object without it isRefused. OnlyRelayOutcome::Ackedreaches the one deletion path.GET /api/console/metrics/webhooksgoes red. It scans the spool on the request, not from a cache, so the signal does not go quiet if the retry sweep stops. A spool that cannot be read is red, not empty.Durability is a temp write →
sync_allthe file → rename →sync_allthe directory, so the entry's name survives a crash too, not just its bytes.Shared entry points, not a fourth copy
trusty_common::uds::rpc::send_framed_request— the framed UDS transport ADR-0034 §4 names, dialling throughconnect_hardenedso the0700directory and0600socket are verified before a byte is written. Caps the response and bounds the exchange with a timeout. The four existing hand-rolled clients migrate onto it in a documented fast-follow, per the PM's scope call.trusty_common::webhook_hmac(featurewebhook-hmac) — one GitHub signature verifier, returning a three-stateSignatureVerdictso "no secret configured" cannot collapse into permission to proceed, which is the shape of trusty-analyze's live fail-open. The two existing copies are retired in step 4.Spawn-on-demand (calling step 2's
UdsServiceSupervisor) is deferred to step 4 — no target binds a listener yet, so it would be dead code. Until then every relay lands inUnreachable, which is a durable pending state.Testing — rung 5
Failure-path coverage is the deliverable. 43 new cases in
crates/trusty-console/src/webhook/tests.rs, 7 incrates/trusty-common/src/uds/rpc.rs, 7 inwebhook_hmac. Each arm above has at least one case that fails against the pre-fix shape (where all four are a202plus a log line):ingest_returns_spool_failed_and_never_accepts_when_the_write_fails/route_returns_500_and_no_ack_when_the_spool_write_failsrelay_failure_leaves_a_pending_entry_with_an_incremented_attempt_countconnected_without_ack_never_deletes_the_entry/relay_treats_a_result_without_ack_as_refusedmetrics_route_reports_red_for_an_aged_pending_entry/metrics_route_reports_red_when_the_spool_cannot_be_readThe spool-failure injection is a regular file where the directory belongs (ENOTDIR for every uid, root included), so it is deterministic in any CI container rather than depending on a non-root runner. The relay runs against a test-double
UnixListenerbound throughbind_hardened.--include-ignoredintegration coverage:integration_from_env_delivery_survives_a_console_restartdrives the full loop against the realfrom_envwiring — verified delivery → durable spool → relay fails → a fresh ingress over the same directory still finds it pending with its body intact → the target comes up and acks → the entry is gone and health returns toOk. These two are#[ignore]d because they mutateTRUSTY_DATA_DIR_OVERRIDEandGITHUB_WEBHOOK_SECRET, which every concurrent sibling in the binary can observe.Gates run
Rung 4's
cargo check --workspacepasses for every crate excepttrusty-code-guiandtrusty-mpm-gui, which both fail ontauri::generate_context!panicking over a missingui/dist— a frontend build artifact absent in a fresh worktree, unrelated to this diff (which touches zero files in either crate).cargo check --workspace --exclude trusty-code-gui --exclude trusty-mpm-guiexits 0.Refs #5089. ADR:
docs/adr/0034-webhook-ingress-console-relays-over-uds-to-a-supervised-on-demand-process.md.🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
Review round 1 — all three HIGHs plus four others (commit
92c1ed3fc)HIGH-1 — the sweep and the request path relayed the same delivery twice.
retry_pending_oncelisted every.jsonwith no claim and no age filter, so a tick landing inside the ≤5 s relay window re-sent a deliveryingestwas still sending.schedule::ClaimSetnow gives one relay per entry path at a time, released on drop so a panicking relay cannot wedge an entry.sweep_does_not_relay_an_entry_the_request_path_is_still_relayingreproduces the critic's case — 600 ms delayed acking stub, sweep 150 ms in — and asserts exactly one frame reaches the target.HIGH-2 — no backoff, contrary to ADR-0034 §2. Every pending entry was relayed every 60 s and each non-ack rewrote the whole base64 body plus two
fsyncs. Until step 4 binds a listener that is every delivery, forever.BackoffPolicyholds a fresh entry for the relay timeout, then spaces attempts 30 s doubling to a 1 h ceiling, and stops at 24 failures. An exhausted entry is never relayed again and never deleted — it holds the health signal red until an operator intervenes, which bounds the write cost absolutely rather than just reducing it. A per-passSWEEP_BUDGETof 32 keeps one serial pass from outlasting its own tick.HIGH-3 — an absent spool directory reported green.
list_pendingansweredErrorKind::NotFoundwith an empty listing, so a removed console data dir or an unmounted volume made everyPOST500 while/api/console/metrics/webhooksreported{"status":"ok","pending":0}. Ingress dead, alarm healthy — the exact contradiction between the PR body's claim and the code.Spoolnow records whether it wasopened; for an opened spool a missing directory isSpoolError::ReadDir, whichscan_healthrenders red.Spool::aton a never-created path is still legitimately empty, covered separately.Body limit. 25 MiB on the webhook sub-router only. axum's 2 MiB default 413s a real
push/pull_requestdelivery before the handler runs — no spool entry, no metric, no log. Same invisible drop, arriving through the framework instead of the code.Wire contract moved to
trusty-common.RELAY_METHOD,RelayFrame,RelayParams,Provenanceand the response types now live intrusty_common::webhook_relay, with an ownedRelayRequestfor the receiving side andRelayResponse::{ack, refuse, is_ack}.relay.rs's "both halves read this constant" was not true as written — step 4's receivers cannot depend on the console.persist_newcommits withhard_link, notrename, so a colliding path fails atomically withAlreadyExistsinstead of clobbering a delivery that may already have been acknowledged.persist_updatekeeps rename semantics forrecord_attempt's deliberate overwrite. Temp files usecreate_newwith a pid+nanosecond suffix.Socket-dir comment corrected. It claimed the paths avoid the
$TMPDIRconvention ADR-0034 §3 rejects;scratch_socket_dir()is$TMPDIR/trusty-<uid>with a/tmpfallback. It now says what is actually true: #5099's0700uid-keyed directory plusconnect_hardened's owner/mode re-check supersedes §3's path rule by satisfying the property §3 wanted.The concurrency test class
Rung 5 for criterion (c) specifies failure-path and concurrency tests, and the first round had none — which is where HIGH-1 lived. Added: the sweep-versus-request race, two overlapping sweeps over two entries, two concurrent deliveries, claim exclusion, and claim release under panic. Plus five
backoff_*cases and two sweep-schedule cases.Regression proof
Each fix was reverted in place and the corresponding test re-run:
All four pass with the fixes restored.
Gates re-run
The two excluded crates fail on
tauri::generate_context!over a missingui/distbuild artifact, unrelated to this diff — unchanged from the first round.Now non-optional for step 4
Target-side dedup on
delivery_id. Console keeps a spool entry until an explicit ack, so a receiver that acknowledges after doing the work can still be re-sent the same delivery if the ack is lost. Relay is at-least-once by construction: duplicate delivery is the normal case, not the edge.RelayParams::attemptsis a hint, never a guarantee that the first attempt did nothing — the doc comment on that field says so.Deliberately out of scope this round, per review: the
tctl doctorcheck (owned by in-flight #5018 / #5011), a console UI tab for the webhooks metric, and header denylist → allowlist.Review round 2 — two HIGHs plus the runtime-blocking MEDIUM (commit
aeba7970a)HIGH-A — exhausted entries accumulated forever and every scan paid full decode for them. The sweep counted
exhaustedandcontinued; nothing deleted, quarantined, or aged them out. Since no target binds a listener until step 4, that is every delivery reaching 24 attempts in ~17 h and staying in the live set permanently, with bothretry_pending_onceandscan_healthreading andserde_json-decoding it on every pass.An exhausted entry now moves to
webhook-spool/exhausted/. Kept, because it is still an unacknowledged webhook and still holds its body for a manual redelivery — but off both hot paths. Two new primitives make the scan cheap:Spool::scan_metadatareadsreceived_at_unix_msand the delivery id from filenames and opens nothing. That is whatentry_path's zero-padded timestamp prefix was always for; until now its stated rationale ("does not have to parse every file") was not realised anywhere, which was the MEDIUM atspool.rs:281.Spool::loaddecodes the single entry the health scan actually needs — the oldest live one, for its attempt count and last error.A filename that does not parse is reported through
unparsablerather than dating to 0, which would read as the oldest entry in the spool and hijack every diagnostic.HIGH-B — once red, the signal stopped saying anything new.
SpoolHealthhad no exhausted field, andSweepReport.exhaustedreached exactly one consumer: atracing::info!— the log linehealth.rs's own module doc names as the trap. Worse,scan_healthtooklisting.pending.first(), and exhausted entries are by construction the oldest, sooldest_pending_delivery_id,oldest_pending_last_errorandoldest_pending_age_secspinned to the first poisoned delivery forever.pendingandexhaustedare now counted separately, with their own ids and ages, andoldest_pending_*describes the oldest live entry. Any exhausted entry still forcesstatus: Error— via its own field, not by hijacking the oldest-pending ones.total_failed_attemptsis replaced byoldest_pending_attempts: the spool-wide sum cost one decode per entry per metrics request, which is what HIGH-A is about, and the oldest live entry's count is the actionable number.sweep_stops_relaying_an_exhausted_entryasserted the old behavior as desired. It now asserts the entry leaves the live set, stays readable with its body byte-exact, and appears in the exhausted fields.MEDIUM — spool I/O off the async runtime. All of it now goes through
spawn_blocking. Ingest fsyncs a file and a directory twice per delivery; the metrics route scans two directories per request. A join failure is surfaced as an error, andWebhookIngress::healthrenders an unrunnable scan red rather than empty — same rule as an unreadable spool.Regression proof
Each fix reverted in place and the corresponding test re-run:
Both pass with the fixes restored. The HIGH-B case is the one you asked for specifically: one exhausted entry from day 1 alongside a newer live entry failing on day 30, asserting the diagnostic fields describe the live one.
Gates re-run
Left for step 4, deliberately
The ingest concurrency semaphore (25 MiB buffered before HMAC at ~3× body size), the unreachable
Noneclaim arm, and thehard_linkfilesystem doc note. A>SWEEP_BUDGETtest did not fall out of the HIGH-A work for free, so it is not here.webhook-spool/exhausted/has no retention policy: it grows until an operator acts, and the red health state with the exhausted count and ids is what tells them to. Deleting an undelivered webhook automatically is the one thing this whole step exists to prevent.