Repository navigation
fix(host-admission): one wake per replay kick; signal-driven daemon tests - #4167
Conversation
The profile replay worker kept two wake signals for one kick (its own Notify permit and the broker's replay request) and cleared neither when a pass began. A pass that served both left them pending, so the idle worker ran one or two redundant passes over the same spool. Refs #4149
retained_begin_and_join_report_partial_effect_and_restart_recovers_same_operation bounded the restarted scheduler's recovery pass with a 2 s wake wait. Under CPU starvation the pass was still running (backlog 1, no blocker) at 2 s and settled at 4.4-5.4 s, so the test failed 27 of 40 in the loaded recipe. The test now waits on the worker's idle notification through a new wait_project_idle test helper, which mirrors wait_profile_idle. The receipt read still proves the restart settled the persisted operation.
The attempt harness polled attempts on a native runtime thread, which production never does. Their synchronous storage writes raised the process-wide native writer-wait counter that off-Tokio tests in the same binary assert stays flat. Poll them on the blocking pool, as spawn_for_worktree does, and drop the serializing probe mutex.
A 5 s shutdown deadline is shorter than the reserves behind the client drain, so that phase was floored at now and ended at the first timer tick. Under load the tick fired before the test released its admitted commit, and the terminal owner ran early. Use the daemon deadline.
The wall budget is armed after the provider spawns. Under CPU starvation the spawn took up to 2.6 s of the 2 s budget, production armed nothing, and the payload contract refused the zero-budget terminal. The double now records when the attempt ran, and the test asserts the branch production took instead of assuming the spawn is fast.
Bootstrap tests bounded the worker's completion with 1-10 s timers. Under load the 60 ms retry-budget test missed its 5 s bound. Tests that expect completion now wait on the completion signal; only the deadline test still passes a timeout.
The test bounded wait_idle with a 250 ms timer, which a loaded runner missed. wait_idle already returns false at its grace, so the result alone proves the notification arrived; the grace is now long enough that a lost notification hangs to the harness timeout.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| } | ||
| self.busy.store(true, Ordering::Release); | ||
| let _ = self.dirty.swap(false, Ordering::AcqRel); | ||
| self.broker.clear_replay_request(); |
There was a problem hiding this comment.
🔴 Concurrent replay kick strands pending records
When a kick lands after dirty.swap but before clear_replay_request, the clear erases its broker wake. If the pass stops, dirty remains set while the worker sleeps, leaving pending records unreplayed until another kick.
Learn more
A replay kick marks dirty and sets the broker's replay request. The worker consumes dirty immediately before clearing that request. A kick between those operations sets both again, but the clear removes its request. If classify_replay_pass returns Stop or TerminalNoop, the worker exits the drain loop and waits solely on that erased request, even though dirty remains true.
Example: An old pending record yields NotApplicable. A new admission kicks replay after dirty.swap(false) but before clear_replay_request(). The pass leaves the old record unchanged and stops; the new record waits until another external kick.
Recommended fix: Consume the broker request before clearing dirty, or combine both signals so a kick concurrent with pass startup cannot be erased without either leaving dirty visible to the next loop or leaving a broker wake pending. Add a deterministic interleaving test that kicks between the two operations and ends the current pass with TerminalNoop.
Was this helpful? React with 👍 or 👎 to provide feedback.
Refs #4149
//crates/tracedecay-daemon-service:unit_testfailed under the #4149 load recipe in eleven tests. One cause was a production bug. The rest were tests that raced a wall clock or shared a process-wide counter. After this branch, the whole binary passes 30 of 30 under the recipe:Production fix
Host-admission replay ran extra passes. One kick raised two wake signals: the worker's own
Notifypermit and the broker's replay request. Neither was cleared when a pass started, so an idle worker ran one or two redundant passes over the same spool. The worker now wakes only through the broker request and clears it when a pass starts (clear_replay_requestincrates/tracedecay-host-admission/src/lib.rs).not_applicable_pending_record_does_not_back_offfailed 1 to 2 in 40 withleft: 3, right: 2(the issue report).Test fixes
Each item lists the cause, the failing-before output, and the defect the test still catches.
profile_host_admission_replayreplay testsbootstrap_gives_up_terminally_once_its_retry_budget_is_spentand 9 other bootstrap waitswait_bootstrap_completedraced a 1 to 10 s timerleft: TimedOut, right: Completed(1/30)idle_notification_during_probe_is_not_lostwait_idleidle notification must avoid the full grace: Elapsed(())(1/30)mark_idledrops its notify: TIMEOUTa_worker_ensured_off_tokio_replays_and_joins,shutting_down_off_tokio_empties_the_registrynative_thread_writer_waitscounter these tests readleft: 5, right: 0terminal_owner_waits_for_admitted_commit_to_drainDaemonShutdownBudget::phasefloored that phase atnowassertion failed: !terminal_cancelled.load(Ordering::Acquire)(1/30)wait_for_idleignores the admitted activity: fails at line 918a_wall_exhausted_provider_seals_timed_out_and_emits_the_no_progress_terminalexactly one no-progress terminal, left: 0, right: 1(2/30, 3/30)left: 0, right: 1contended_prepare_lock_times_out_with_retryable_receipta_handshake_finished_by_a_flush_wakes_the_parked_reader,a_late_listener_poll_does_not_time_out_a_prompt_handshakethe reader parks on the handshakeretained_begin_and_join_report_partial_effect_and_restart_recovers_same_operationThe wall-exhausted test now records when the attempt was marked running. When that is at or past the deadline, it asserts the branch production took:
TimedOutand no terminal. Unloaded runs take the positive branch, which the break check above exercises.Not changed
Two
shutdown::orchestrationtests failed once each in one later round:a_stuck_background_owner_cannot_starve_project_servers_or_store_closeanda_project_server_drain_that_ignores_its_deadline_hits_the_backstop. Both test real-time phase deadlines (50 to 300 ms) under a 2 to 4 s coordinator deadline. The coordinator deadline fired first.native_runtimehas no virtual clock, so a fix needs either one or a wider coordinator deadline. Neither is in this PR; they are noted on #4149.Verification
bazel test //crates/tracedecay-host-admission:unit_test //crates/tracedecay-session-runtime:unit_test: passed.bazel test //crates/tracedecay:unit_test --test_arg=branch_admin: passed.bazel build --config=clippyon daemon-service, host-admission, session-runtime and//crates/tracedecay:unit_test: clean.