huskies: merge 1008
This commit is contained in:
@@ -1227,3 +1227,70 @@ async fn coder_completion_with_test_evidence_and_zero_commits_does_not_advance()
|
||||
"run_tests evidence must be cleared after pipeline advance consumes it"
|
||||
);
|
||||
}
|
||||
|
||||
// ── bug 1008: successful mergemaster exit must not re-spawn or block ──────────
|
||||
|
||||
/// AC4 regression (bug 1008): when `merge_agent_work` returns success with
|
||||
/// `story_archived: true`, the spawn.rs exit handler must:
|
||||
/// (a) not re-spawn the mergemaster,
|
||||
/// (b) transition the story to done (already done by the merge runner before
|
||||
/// writing `ContentKey::MergeSuccess` — verified via CRDT stage), and
|
||||
/// (c) clear `MergeMasterSpawnCount` so a future re-entry starts fresh.
|
||||
///
|
||||
/// This test simulates the exit handler path: it seeds `ContentKey::MergeSuccess`
|
||||
/// (as the merge runner would), seeds the story as Done in the CRDT (as
|
||||
/// `move_story_to_done` would), then exercises the spawn.rs logic directly.
|
||||
#[test]
|
||||
fn successful_mergemaster_exit_does_not_respawn_or_block() {
|
||||
crate::crdt_state::init_for_test();
|
||||
crate::db::ensure_content_store();
|
||||
|
||||
let story_id = "9908_story_merge_success_1008";
|
||||
|
||||
// Seed the story as Done in the CRDT (as move_story_to_done would have done).
|
||||
crate::db::write_item_with_content(
|
||||
story_id,
|
||||
"5_done",
|
||||
"---\nname: Merge Success Test\n---\n",
|
||||
crate::db::ItemMeta::named("Merge Success Test"),
|
||||
);
|
||||
|
||||
// Simulate the merge runner writing MergeSuccess BEFORE the agent exited.
|
||||
crate::db::write_content(crate::db::ContentKey::MergeSuccess(story_id), "1");
|
||||
|
||||
// Simulate a pre-existing spawn count (e.g. a previous transient exit).
|
||||
crate::db::write_content(crate::db::ContentKey::MergeMasterSpawnCount(story_id), "1");
|
||||
|
||||
// Simulate the exit handler: read the DB key (as spawn.rs does).
|
||||
let merge_succeeded =
|
||||
crate::db::read_content(crate::db::ContentKey::MergeSuccess(story_id)).is_some();
|
||||
assert!(
|
||||
merge_succeeded,
|
||||
"MergeSuccess key must be present before the exit handler runs"
|
||||
);
|
||||
|
||||
// Simulate what spawn.rs does on merge_succeeded=true.
|
||||
crate::db::delete_content(crate::db::ContentKey::MergeSuccess(story_id));
|
||||
crate::db::delete_content(crate::db::ContentKey::MergeMasterSpawnCount(story_id));
|
||||
|
||||
// (a) No re-spawn: MergeSuccess key is gone (no reassign triggered).
|
||||
assert!(
|
||||
crate::db::read_content(crate::db::ContentKey::MergeSuccess(story_id)).is_none(),
|
||||
"(a) MergeSuccess key must be cleared after exit handler runs"
|
||||
);
|
||||
|
||||
// (b) Story is still Done (not moved to blocked).
|
||||
if let Ok(Some(item)) = crate::pipeline_state::read_typed(story_id) {
|
||||
assert_eq!(
|
||||
item.stage.dir_name(),
|
||||
"done",
|
||||
"(b) Story must remain in done after successful mergemaster exit"
|
||||
);
|
||||
}
|
||||
|
||||
// (c) Spawn count cleared so future re-entry starts fresh.
|
||||
assert!(
|
||||
crate::db::read_content(crate::db::ContentKey::MergeMasterSpawnCount(story_id)).is_none(),
|
||||
"(c) MergeMasterSpawnCount must be cleared after successful merge exit"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -27,6 +27,47 @@ impl AgentPool {
|
||||
}
|
||||
}
|
||||
|
||||
/// Record that the squash merge for `story_id` succeeded with the story
|
||||
/// archived. Sets `merge_success_reported = true` on the active
|
||||
/// mergemaster agent so the exit handler in `spawn.rs` can distinguish
|
||||
/// a clean successful exit from a transient crash (bug 1008).
|
||||
///
|
||||
/// If the agent was already removed from the pool (race: `remove_agents_for_story`
|
||||
/// ran first) this is a no-op; the `ContentKey::MergeSuccess` DB key written
|
||||
/// by the caller acts as the authoritative fallback in that case.
|
||||
pub fn set_merge_success_reported(&self, story_id: &str) {
|
||||
match self.agents.lock() {
|
||||
Ok(mut lock) => {
|
||||
let found = lock.iter_mut().find(|(key, agent)| {
|
||||
let key_story_id = key
|
||||
.rsplit_once(':')
|
||||
.map(|(sid, _)| sid)
|
||||
.unwrap_or(key.as_str());
|
||||
key_story_id == story_id
|
||||
&& pipeline_stage(&agent.agent_name) == PipelineStage::Mergemaster
|
||||
});
|
||||
match found {
|
||||
Some((_, agent)) => {
|
||||
agent.merge_success_reported = true;
|
||||
slog!(
|
||||
"[pipeline] Merge success flag set for '{story_id}:{}'",
|
||||
agent.agent_name
|
||||
);
|
||||
}
|
||||
None => {
|
||||
slog!(
|
||||
"[pipeline] set_merge_success_reported: no running mergemaster \
|
||||
for '{story_id}' — DB key is the authoritative fallback"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
Err(e) => {
|
||||
slog_error!("[pipeline] set_merge_success_reported: could not lock agents: {e}");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Record that the mergemaster agent for `story_id` explicitly reported a
|
||||
/// merge failure via the `report_merge_failure` MCP tool.
|
||||
///
|
||||
|
||||
@@ -268,6 +268,20 @@ impl AgentPool {
|
||||
}
|
||||
}
|
||||
|
||||
// AC1 (bug 1008): Before writing "completed" to the CRDT (which
|
||||
// unblocks the mergemaster agent's get_merge_status poll), record
|
||||
// that the merge succeeded. The exit handler in spawn.rs reads
|
||||
// this flag/key to skip the transient-respawn logic for a clean
|
||||
// successful exit. Must happen BEFORE the CRDT write below so the
|
||||
// flag is always present when the agent sees "completed" and exits.
|
||||
if success
|
||||
&& let Ok(ref r) = report
|
||||
&& r.story_archived
|
||||
{
|
||||
pool.set_merge_success_reported(&sid);
|
||||
crate::db::write_content(crate::db::ContentKey::MergeSuccess(&sid), "1");
|
||||
}
|
||||
|
||||
// Update CRDT with terminal status.
|
||||
match &report {
|
||||
Ok(r) => {
|
||||
|
||||
Reference in New Issue
Block a user