Skip to content

fix(trusty-review): release the in_flight gauge on every exit path - #5026

Closed
bobmatnyc wants to merge 2 commits into
mainfrom
fix/review-in-flight-raii
Closed

bobmatnyc wants to merge 2 commits into
mainfrom
fix/review-in-flight-raii

Conversation

@bobmatnyc

Copy link
Copy Markdown
Owner

The defect

crates/trusty-review/src/service/handlers.rs and
crates/trusty-review/src/service/webhook.rs both bracketed
run_review(...).await with a bare fetch_add / fetch_sub pair on
AppState::in_flight — the gauge GET /status reports. The decrement is a
plain statement after the .await, so it is skipped on two real exit paths:

Path What happens Where
Client disconnects mid-review axum drops the handler future; the fetch_sub line is never reached handle_review
run_review panics the unwind passes over the fetch_sub; on the webhook path the panic is swallowed into a JoinError nobody reads and the daemon keeps running both

Either leaks a permanent +1 for the life of the process. Reviews take a ~36.7s
median, so the disconnect window is wide.

Verified against origin/main before fixing — the shape was as reported and no
guard existed.

The fix

InFlightCountGuard in crates/trusty-review/src/store/in_flight.rs:
acquire increments, Drop decrements. That module already owns this exact
idiom — InFlightGuard releases the dedup slot on drop, documented there as
"so the slot is always released even if the review task panics." This extends
the existing pattern rather than adding a second one; there is no shared
counter-guard in trusty-common to route through.

Both call sites now hold the guard. The increment has no spelling that does not
also arm the decrement — acquire is the only constructor.

Tests

Five new tests, all confirmed to go red with the guard removed:

  • review_handler_in_flight_returns_to_zero_when_future_dropped — parks
    run_review inside preflight_context on a never-resolving search probe,
    asserts the gauge reached 1, drops the future, asserts it is back to 0.
  • review_handler_in_flight_returns_to_zero_when_pipeline_panics — panics
    inside the pipeline, runs the handler under tokio::spawn so the unwind is
    caught, asserts the task panicked and the gauge is 0.
  • count_guard_increments_then_decrements_on_drop,
    count_guard_decrements_on_panic_unwind,
    count_guard_nests_and_unwinds_in_order — guard-level unit tests.

Break-and-watch, as required. Reverting handlers.rs to the bare
fetch_add/fetch_sub shape and re-running:

test service::handlers::tests::review_handler_in_flight_returns_to_zero_when_pipeline_panics ... FAILED
test service::handlers::tests::review_handler_in_flight_returns_to_zero_when_future_dropped ... FAILED

assertion `left == right` failed: dropping the handler future must release the in-flight slot
  left: 1
 right: 0
assertion `left == right` failed: a panicking pipeline must still release the in-flight slot
  left: 1
 right: 0

test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured; 1574 filtered out

The guard was then restored and the suite re-run green.

The webhook site has no end-to-end drop/panic test of its own — driving that
spawned task through run_review needs a GitHub PR-metadata fetch stub the
crate does not have. It is covered by the guard's own unit tests plus the
end-to-end handler tests, since it uses the same guard.

Also checked, deliberately not changed

  • in_flight_registry — already correct. Its PR- and SHA-level slots are
    RAII guards released on drop; that is the precedent this fix follows.
  • last_error — a Mutex<Option<String>> written after run_review
    returns, not an acquire/release pair. A panic means the error is not
    recorded, which is a missed log line, not a leak.
  • InferenceProbe::consecutive_unknown — a streak counter with an explicit
    store(0) reset, not a paired increment/decrement.
  • The durable dedup claim — the same drop-mid-flight shape does strand a
    claim, but DEDUP_STALE_SECS already reclaims abandoned claims, so it
    self-heals. Nothing to do.

Out of scope — NOT addressed by this PR

The scoping pass that found this counter leak also flagged two other
daemon-shape bugs in trusty-review:

  • resolve_index() running once at boot rather than per-invocation
  • /health blocking on a live Bedrock call

Neither is touched here. They are entangled with an undecided question
about whether trusty-review should remain a daemon at all. Do not read this
PR as having addressed them.

Gates — rung 3, concurrency-shaped

Gate Result
cargo fmt --check exit 0
cargo check -p trusty-review --all-targets exit 0
cargo clippy -p trusty-review --all-targets -- -D warnings exit 0
cargo test -p trusty-review --all-features 1571 passed; 0 failed; 5 ignored + 8 further suites all green, 0 failures
bash scripts/check_line_cap.sh 3742 tracked .rs file(s); 0 violations — OK
bash scripts/check_sld.sh 56 spec doc(s) + 3115 code file(s); 0 error(s), 0 warning(s)
bash scripts/check_test_pointers.sh 22079 Test: citation(s) — 0 dangling pointers — OK

Collision check

No other open PR touches crates/trusty-review/ — the open set is in
trusty-installer (#5011,
#5018),
trusty-common/memory_core (#5013),
and trusty-mpm assets (#5019).
This PR stays entirely inside trusty-review.

Note on CI

GitHub Actions is in a major outage — jobs die at "Set up job" with
Failed to resolve action download info. Red or absent checks here are that
outage, not this change. The local gates above are the evidence.

🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools

`handle_review` and the webhook's spawned review task both bracketed
`run_review(...).await` with a bare `fetch_add` / `fetch_sub` pair. The
decrement is skipped on two real exit paths:

- the handler future is dropped when an HTTP client disconnects mid-review
  (axum drops the future; the `fetch_sub` line is never reached)
- `run_review` panics, unwinding past the `fetch_sub`

Either leaks a permanent +1 into `AppState::in_flight`, the gauge `GET
/status` reports, for the life of the process. Reviews take ~37s median,
so the disconnect window is wide.

Both sites now take an `InFlightCountGuard` whose `Drop` decrements,
matching the RAII discipline `InFlightGuard` already applies to the dedup
slot in the same module.

🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
@bobmatnyc bobmatnyc added the trusty-mpm trusty-mpm platform and related work label Aug 6, 2026
@bobmatnyc bobmatnyc self-assigned this Aug 6, 2026
@bobmatnyc

Copy link
Copy Markdown
Owner Author

Closed as obsoleted. The fix is correct, but trusty-review's daemon removal (#5028) eliminates the code path this patches.

@bobmatnyc bobmatnyc closed this Aug 7, 2026
@bobmatnyc
bobmatnyc deleted the fix/review-in-flight-raii branch August 15, 2026 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

trusty-mpm trusty-mpm platform and related work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants