Skip to content

fix(goal): cancel pending reviews before session deletion - #42

Merged
alderpath merged 6 commits into
Reliary:mainfrom
jrzmurray:fix/delete-goal-review-race
Nov 14, 2026
Merged

alderpath merged 6 commits into
Reliary:mainfrom
jrzmurray:fix/delete-goal-review-race

Conversation

@jrzmurray

Copy link
Copy Markdown
Contributor

Summary

  • Track and cancel pending goal reviewer tasks during abort and session deletion.
  • Serialize reviewer and goal-driver registration with the session deletion fence.
  • Bound the pending reviewer registry at 128 entries and defer wakes at capacity.
  • Add deletion-race and capacity regression tests; update the specs and traceability matrix.

Dependency

This is stacked on PR #41, which adds the shared session delete fence and drain cancellation. Please review this after #41; the first commit in this branch is the #41 change.

Validation

  • cargo fmt --all -- --check — passed
  • cargo clippy --workspace -- -D warnings — passed
  • Goal deletion unit tests — 2 passed
  • goal_e2e — 13 passed
  • scripts/check-guards.sh — passed
  • scripts/check-matrix.sh — passed
  • cargo test --workspace — 88 passed, 8 failed, 1 ignored. The failures are existing host assumptions for Linux cgroups and /proc that are unavailable in this macOS environment (uid-unknown, RSS probe); focused goal tests pass.

jrzmurray and others added 4 commits November 13, 2026 02:02
Add two ignored red regressions for K-GOAL-DELETE-RACE. Running them with --ignored reproduced the bug: session deletion leaves a pending reviewer request open, and a goal driver registers behind an active delete fence. The fix follows in this branch.

Fmt, clippy, guards, and matrix passed. Workspace tests stop on the known macOS cgroup and /proc assumptions.
Track reviewer JoinHandles so abort and delete cancel pending model calls.
Serialize reviewer and driver registration with the session delete fence, and
cap pending reviews at 128 to keep the task registry bounded.

Covers K-GOAL-DELETE-RACE and K-GOAL-REVIEW-BOUND; names D-GOAL-REVIEW-CAP.
Focused goal tests, clippy, formatting, guards, and matrix pass. Workspace tests
remain blocked by pre-existing macOS cgroup and /proc assumptions (8 failures).
Amendment to the contributor's branch (credited @jrzmurray), from the review.

  * spawn_wake_public called check_v1_session_admission -> session_exists
    while holding the parking_lot v2_drain_tasks mutex: a synchronous SQLite
    query under a lock on an async worker (both halves are separately
    banned; F1 class). The check is now in-memory only. The durable goal row
    already implies the session, and a session deleted in the gap is caught
    by the turn itself, which 404s.
  * A review deferred at the 128-entry cap was dropped: its only retrigger
    was the next human turn, so an overnight goal stopped silently. One retry
    owner now backs off 2-60 s over 6 attempts, counts review_deferred, and
    PAUSES the goal with an operator-visible reason if capacity never frees.
    All three post-turn call sites route through it.
  * goal_is_active reads the goal row, so it moved to run_blocking.

Docs: D-GOAL-REVIEW-CAP rewritten to describe the retry; D-V2-DRAIN-CAP states
that it is a COUNT bound and gives the real worst case; TESTING/TRACEABILITY
rows added.

Verified: 932 workspace tests, clippy -D under CARGO_BUILD_WARNINGS=deny,
guards, matrix. Negative control: return after the first failed attempt and
the goal stays active at rounds=0 until the 90 s timeout.
@alderpath

Copy link
Copy Markdown
Collaborator

Thanks — the delete/review race is real and the fence is the right mechanism. Approved the workflows and pushed two commits onto your branch; I can split them into a separate PR if you prefer.

The two things I changed

  1. The admission check ran SQLite under the drain-registry mutex. spawn_wake_public held st.v2_drain_tasks.lock() and then called check_v1_session_admission → session_exists (an open_reader + SELECT count(*)). That is a synchronous store call on an async worker and blocking IO under a lock — both banned separately, and the exact class the repo fixed once before. It is now an in-memory session_is_deleting check only. The durable goal row already implies the session exists; a session deleted in the gap is caught by the turn, which 404s.

  2. A deferred review was dropped, not deferred. At 128 pending reviews you logged and returned false, and the only other trigger is the next session turn. For an overnight goal that is a silent stop until a human speaks — the failure mode the goal feature exists to avoid. There is now one retry owner: 2 s doubling to a 60 s cap, 6 attempts, review_deferred counted, and if capacity never frees the goal is paused with a visible reason instead of hanging. All three post-turn call sites route through it, so a deferral anywhere behaves the same.

  3. goal_is_active reads the goal row, so it also moved to run_blocking.

Evidence

goal_e2e::deferred_review_is_retried_without_another_user_turn saturates the 128-entry registry, arms a goal, waits for the review_deferred metric (so the test cannot pass without exercising the retry), frees exactly one slot, and asserts the goal reaches complete with no further turn. Negative control: return after the first failed attempt and the goal stays active at rounds: 0 until the 90 s timeout.

932 workspace tests, clippy -D under deny-warnings, guards and matrix green.

One note on scope: your MAX_PENDING_GOAL_REVIEWS is fine as a count; I made it pub only so the test can saturate it.

Kept both sides: the permission and drain/delete rows from main, the
goal-review cancellation row and the new deferred-retry row from this branch.
The Reliary#41 branch made check_v1_session_admission async; main's Reliary#42 branch added
a call site that was written against the sync signature. Caught by
check-matrix (workspace must build before the test-existence check runs),
which is exactly what that fail-closed behaviour is for.
@alderpath
alderpath merged commit 01722cd into Reliary:main Nov 14, 2026
15 checks passed
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.

2 participants