Compare commits

...
Author SHA1 Message Date
Miguel Palhas bbafff5a98 fix(maestro): add settle-finished observable for natural-exit test
The no-retained-store test asserts a negative — the tree survives — so
waiting on that negative proves nothing: settle could still remove the
tree one instant after the assertion passes. mark_session_dead now flips
a per-record settled flag once the exit rule has been applied or refused,
exposed as SessionManager::is_settled; the test waits on it before
asserting.

Refs #115
2026-08-24 23:12:46 +01:00
Miguel Palhas 17e1dc4049 merge: sidebar-cards/114-hook-error 2026-08-24 23:01:14 +01:00
Miguel Palhas 436cb194c3 merge: sidebar-cards/113-more-flakes 2026-08-24 23:00:54 +01:00
Miguel Palhas f689c916bf fix(maestro): poll awaiting-input until leaderless state holds
The pipeline leader exits as soon as both stages are forked, which can
beat the surviving stage opening the tty and parking on read — the one-
shot assertion then ran against a group with no parked member yet.
Poll for the answer, accepting it only while the foreground group is
still leaderless so a shell that regained the tty cannot satisfy it.

Fixes #113
2026-08-24 22:50:02 +01:00
Miguel Palhas 88233cb96d fix(maestro): sync watcher re-emit test to real event flow
Two races. First, the watcher updates the snapshot and emits
SessionUpdated as two steps, so the drain could run between them: the
legitimate breakdown emission then arrived inside the quiet window.
Subscribe before the watcher starts and consume through the breakdown
event so the window begins after the last real change.

Second, the assertion treated any SessionUpdated as a failed
suppression, but the activity pass may legitimately announce an
Active→Idle transition in that window (ACTIVE_WINDOW is 3s), carrying
today's blocked_work unchanged. Only a changed indicator counts as a
re-emit now.

Fixes #113
2026-08-24 22:50:02 +01:00
Miguel Palhas efbccb4e11 fix(maestro): wait for settle in pristine natural-exit test
Dead status is observable a moment before the settle step removes the
worktree, so the assertions raced the removal under parallel load. Wait
for the tree to be gone and the store to stay empty, same as the
retained-store wait cfebfd4 added to the guarded sibling.

Fixes #113
2026-08-24 22:41:54 +01:00
3 changed files with 100 additions and 10 deletions
+26
View File
@@ -62,6 +62,11 @@ pub struct SessionRecord {
/// resumes and [`Session::activity_detail`] is cleared with it. This
/// field is daemon-local bookkeeping only — it never rides the wire.
pub reported_activity: Option<crate::activity::ReportedActivity>,
/// Flipped once the natural-exit settle step (#82) has been applied or
/// refused for this session. `Dead` is observable before settling ends
/// (#115), so this is the observable that says its outcomes are safe to
/// assert on. Daemon-local bookkeeping; never rides the wire.
pub settled: bool,
}
#[derive(Debug, thiserror::Error)]
@@ -294,6 +299,17 @@ impl SessionManager {
sessions.get(&id).map(|r| r.session.status)
}
/// Whether the natural-exit settle step (#82) has finished for this
/// session: `Some(true)` once applied or refused, `Some(false)` while
/// still pending, `None` if the session is unknown. `Dead` is observable
/// before settling ends (#115), so anything asserting on a dead
/// session's worktree waits on this rather than inferring settle
/// completion from its side effects.
pub async fn is_settled(&self, id: Uuid) -> Option<bool> {
let sessions = self.inner.sessions.lock().await;
sessions.get(&id).map(|r| r.settled)
}
/// Spawn a new session. On success returns the freshly-created `Session`.
pub async fn create(&self, req: CreateSessionRequest) -> Result<Session, SessionError> {
let (effective_cwd, worktree_path, worktree_base_repo) = match &req.worktree {
@@ -429,6 +445,7 @@ impl SessionManager {
worktree_branch: req.worktree.as_ref().map(|w| w.branch.clone()),
bridge,
reported_activity: None,
settled: false,
},
);
}
@@ -657,6 +674,7 @@ impl SessionManager {
worktree_branch: None,
bridge,
reported_activity: None,
settled: false,
},
);
let _ = self
@@ -1069,6 +1087,14 @@ async fn mark_session_dead(inner: &Arc<Inner>, id: Uuid) {
}
}
}
// Settle has now been applied or refused either way (#115): from here on
// its outcomes are safe to observe.
{
let mut sessions = inner.sessions.lock().await;
if let Some(record) = sessions.get_mut(&id) {
record.settled = true;
}
}
let _ = inner.sidecar.write(&session);
let _ = inner.events.send(CatalogEvent::SessionUpdated { session });
+19 -4
View File
@@ -189,8 +189,23 @@ fn a_foreground_pipeline_whose_leader_exited_is_still_awaiting_input() {
"never observed a foreground group whose leader had exited"
);
assert!(
awaiting_input(pane.pane_pid, &pane.tty, &ProcGroups::snapshot()),
"a surviving pipeline member still holds the tty"
);
// The leader can be gone while the surviving stage is still setting up:
// `true` exits as soon as both stages are forked, before the reader has
// opened the tty and parked. Poll for the answer, but only accept it
// while the state this test exists for still holds — otherwise a shell
// that regained the terminal would satisfy it vacuously.
let deadline = std::time::Instant::now() + Duration::from_secs(5);
let mut holds = false;
while std::time::Instant::now() < deadline {
let groups = ProcGroups::snapshot();
let still_leaderless = pgrp_and_tpgid(pane.pane_pid).is_some_and(|(pgrp, tpgid)| {
tpgid > 0 && tpgid != pgrp && pgrp_and_tpgid(tpgid).is_none()
});
if still_leaderless && awaiting_input(pane.pane_pid, &pane.tty, &groups) {
holds = true;
break;
}
std::thread::sleep(Duration::from_millis(20));
}
assert!(holds, "a surviving pipeline member still holds the tty");
}
+55 -6
View File
@@ -397,6 +397,12 @@ async fn watcher_refreshes_the_blocked_work_indicator() {
init_bare_repo(origin.path());
push_all_to_origin(repo.path(), origin.path());
// Subscribe before the watcher starts so every pass's emission is
// captured — otherwise the breakdown event can land between the
// snapshot observation below and the drain, and the quiet window
// would see it as an illegitimate re-emit.
let mut events = fx.manager.events();
fx.manager.start_liveness_watcher_with_blocked_work(
Duration::from_millis(50),
Duration::from_millis(100),
@@ -418,14 +424,38 @@ async fn watcher_refreshes_the_blocked_work_indicator() {
})
.await;
// A steady state must not re-emit every tick (#83 change-detection).
let mut events = fx.manager.events();
// Consume through the emission that carried the breakdown (#83
// change-detection): the quiet window below must start after the last
// legitimate change, not race it into the channel.
loop {
match tokio::time::timeout(Duration::from_secs(5), events.recv()).await {
Ok(Ok(CatalogEvent::SessionUpdated { session }))
if matches!(
&session.blocked_work,
Some(b) if b.uncommitted_files >= 1 && b.untracked_files >= 1
) =>
{
break;
}
Ok(Ok(_)) => continue,
_ => panic!("never saw the SessionUpdated carrying the breakdown"),
}
}
while events.try_recv().is_ok() {}
tokio::time::sleep(Duration::from_millis(400)).await;
assert!(
!matches!(events.try_recv(), Ok(CatalogEvent::SessionUpdated { .. })),
"an unchanged worktree must not re-emit SessionUpdated"
);
// Only a *changed* indicator counts as a re-emit (#83 change-detection).
// The activity pass may still legitimately announce e.g. Active→Idle
// (its debounce is time-based, not worktree-based), and its payload
// carries today's blocked_work unchanged.
while let Ok(CatalogEvent::SessionUpdated { session }) = events.try_recv() {
assert!(
matches!(
&session.blocked_work,
Some(b) if b.uncommitted_files >= 1 && b.untracked_files >= 1
),
"an unchanged worktree must not re-emit a changed indicator"
);
}
}
/// Cleanup on kill is unconditional once the guard passes (#79): even a spec
@@ -848,6 +878,17 @@ async fn natural_exit_of_a_pristine_worktree_removes_it() {
})
.await;
// (`Dead` is observable a moment before the settle step removes the tree.)
wait_for_async(Duration::from_secs(3), || async {
!wt_path.exists()
&& fx
.retained_store()
.load_all()
.map(|all| all.is_empty())
.unwrap_or(false)
})
.await;
assert!(
!wt_path.exists(),
"a pristine worktree leaves nothing behind"
@@ -905,6 +946,14 @@ async fn natural_exit_without_a_retained_store_destroys_nothing() {
})
.await;
// The settle step has been applied or refused either way — here refused,
// since there is no store to record the tree into. Only from this point
// is "the tree survived" a settled fact rather than a race (#115).
wait_for_async(Duration::from_secs(3), || async {
manager.is_settled(s.id).await == Some(true)
})
.await;
assert!(
wt_path.exists(),
"nothing may be destroyed when it cannot be recorded"