fix(daemon): eliminate flakiness in daemon and wrapper-daemon test suites

[svarlamov]

Summary

Three root causes of daemon/wrapper-daemon test flakiness fixed:

• Snapshot timestamp bug: payload_timestamp_ns in trace_normalizer.rs was calling Value::as_u64 on git trace2's time field, which is an RFC3339 string. It fell through to t_abs (elapsed seconds since program start, ~2–3ms), making finished_at_ns nearly zero. The mtime-cutoff filter then treated every file as "modified after cutoff", producing empty carryover snapshots and silently dropping amend-authorship working-log data. Fix: parse time as RFC3339 via chrono.

• Missing pending-root slot for current_dir invocations: Commands run via current_dir (not -C) have no -C in argv, so the git trace2 start event carries cwd=None → no worktree hint → no PendingRoot slot → command was processed via the no-op "applied" path with no deferred snapshot capture. Subdirectory amend tests failed because the carryover snapshot was never captured. Fix: when replace_pending_root_entry finds no slot but the command has a resolved family_key, insert it directly as a ReadyCommand (late registration).

• Async drain races captured-checkpoint tests: The async per-family drain workers made captured checkpoints (delegated with wait=false) truly fire-and-forget. Tests that called git_ai checkpoint mock_ai <files> and immediately read current_working_logs() raced against the daemon. The production code path is correct. Fix in the test harness only: git_ai_with_env and git_ai_with_stdin now capture the daemon's total completion-log baseline before running a checkpoint, then wait for it to advance if the output contains "Checkpoint queued" (the existing production marker for async-delegated checkpoints).

Test plan

• All 2937 integration tests pass at 4, 8, 12, and 16 threads with 0 failures
• Verified status_ignore and subdirs amend tests pass in daemon and wrapper-daemon modes
• No production code paths changed by the test harness fix

🤖 Generated with Claude Code

---

[devin-ai-integration]

Devin Review found 1 potential issue.

View 6 additional findings in Devin Review.

[devin-ai-integration]

🔴 get_or_create_drain_notifier returns stale notifier for a dead drain worker, silently losing notifications

If a per-family drain worker exits unexpectedly (e.g., side_effect_exec_lock returns Err at src/daemon.rs:5450-5452 due to a poisoned std::sync::Mutex), its Arc<Notify> clone is dropped, bringing the strong_count of the map entry down to 1. However, get_or_create_drain_notifier only checks whether an entry exists in the map — it does not verify the worker is still alive. Subsequent callers receive the stale Arc<Notify> and call notify_one(), but no worker is running to consume the permit. All sequencer entries for that family (commands and checkpoints) are silently stuck until gc_stale_family_state runs (every 500 trace payloads) and removes the dead entry, after which the next call spawns a fresh worker.

For wait=true checkpoint requests, this means the oneshot::Receiver returned to the control-socket handler will block indefinitely until GC happens to run and a subsequent event for the same family triggers a new worker spawn — potentially causing long hangs.

Suggested change

	if let Some(existing) = map.get(family) {
	return Ok(existing.clone());
	}
	if let Some(existing) = map.get(family) {
	if Arc::strong_count(existing) > 1 {
	return Ok(existing.clone());
	}
	// The drain worker for this family has exited (only the map holds
	// a reference). Remove the stale entry and fall through to spawn
	// a new worker.
	map.remove(family);
	}

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 9 additional findings in Devin Review.

[devin-ai-integration]

🟡 Drain worker holds exec_lock during notified.await, blocking the lock indefinitely between drain passes

The drain worker loop acquires _guard = exec_lock.lock().await at line 5454 and then waits on notified.await at line 5470 — but _guard lives until the end of the loop iteration (line 5471), so the side_effect_exec_lock for this family is held for the entire duration the worker sleeps waiting for a notification. Currently the drain worker appears to be the sole acquirer of this lock (the two previous callers in replace_pending_root_entry and append_checkpoint_to_family_sequencer were changed to no longer acquire it), so this doesn't cause a deadlock today. However, side_effect_exec_lock() is still a public method and any future code that acquires it for the same family will block indefinitely until the drain worker happens to receive a notification. The fix is to drop the guard before awaiting: drop(_guard); before notified.await;.

Suggested change

	let _guard = exec_lock.lock().await;
	if coordinator.is_shutting_down() {
	break;
	}
	if let Err(e) = coordinator
	.drain_ready_family_sequencer_entries_locked(&family_clone)
	.await
	{
	debug_log(&format!(
	"drain worker error for family {}: {}",
	family_clone, e
	));
	}
	// Wait for the next notification now that the drain pass is complete.
	// Any `notify_one()` that fired during the drain will have stored a
	// permit, so this await will return immediately in that case.
	notified.await;
	let _guard = exec_lock.lock().await;
	if coordinator.is_shutting_down() {
	break;
	}
	if let Err(e) = coordinator
	.drain_ready_family_sequencer_entries_locked(&family_clone)
	.await
	{
	debug_log(&format!(
	"drain worker error for family {}: {}",
	family_clone, e
	));
	}
	drop(_guard);
	// Wait for the next notification now that the drain pass is complete.
	// Any `notify_one()` that fired during the drain will have stored a
	// permit, so this await will return immediately in that case.
	notified.await;

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 11 additional findings in Devin Review.

[devin-ai-integration]

🟡 Error-detection skip() in wait_for_daemon_checkpoint_completion_count uses checkpoint count to skip over all-type entries

In wait_for_daemon_checkpoint_completion_count, baseline_count is the number of checkpoint-specific entries (from daemon_checkpoint_completion_count()), but entries returned by daemon_completion_entries_for_family() contains entries of all types (commands, checkpoints, etc.). Using skip(baseline_count as usize) on the full entry list skips fewer entries than intended when non-checkpoint entries precede the baseline checkpoints. This can expose pre-existing error entries to the find(|entry| entry.status == "error") check, potentially causing a false panic on an old unrelated error.

For example, if at baseline there are 5 checkpoint entries and 5 non-checkpoint entries (10 total), skip(5) leaves 5 entries visible — some of which may be pre-baseline non-checkpoint errors. The analogous wait_for_daemon_total_completion_count (test_repo.rs:1605) correctly uses a total-entry baseline for its skip().

Prompt for agents

In wait_for_daemon_checkpoint_completion_count (test_repo.rs around line 1676), the error-scanning skip(baseline_count as usize) uses the checkpoint-specific baseline count to skip entries in the full (all-type) entry list. This is inconsistent with wait_for_daemon_total_completion_count which correctly uses a total count for skipping.

To fix: either (a) capture the total entry count at the start and use that for the skip, or (b) filter the entries for checkpoints before applying the skip and error check, or (c) simply remove the skip and scan all entries for errors (since pre-existing errors would have already panicked in prior waits).

Approach (a) is most aligned with the existing wait_for_daemon_total_completion_count pattern. You would need to pass a separate total_baseline alongside the checkpoint baseline, or compute the total count internally.

---

Was this helpful? React with 👍 or 👎 to provide feedback.