Skip to content

Commit 7fcedca

Browse files
committed
fix(goal): never delete temps during goal load
Load no longer scans or removes .goal.json.tmp.* files. A concurrent save can keep its in-flight temp, then rename it onto goal.json instead of failing with ENOENT. Co-authored-by: Mathis <echobt@users.noreply.github.com>
1 parent 3dfda21 commit 7fcedca

2 files changed

Lines changed: 50 additions & 85 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1212
## 0.1.11
1313

1414
### Changed
15-
- `/goal` production harden: persist is atomic + fsynced; corrupt `goal.json` is quarantined so session resume continues (and the status text says so only when the move succeeded); load never deletes another process's in-flight `.goal.json.tmp.{pid}`; `/goal status` is a reserved status token; continuation records a finished turn *then* wrap-up/continues (last remaining turn still runs); `UpdateGoal` complete accepts only `file` / `command` / `test` evidence (globally deduped); status, pause, resume, and resume-on-session print the chip (`Goal · 2/8` / paused / done / budget / blocked). Live smoke still SKIP without a key.
15+
- `/goal` production harden: persist is atomic + fsynced; corrupt `goal.json` is quarantined so session resume continues (and the status text says so only when the move succeeded); load never deletes `.goal.json.tmp.*` (a concurrent save's in-flight temp is left alone); `/goal status` is a reserved status token; continuation records a finished turn *then* wrap-up/continues (last remaining turn still runs); `UpdateGoal` complete accepts only `file` / `command` / `test` evidence (globally deduped); status, pause, resume, and resume-on-session print the chip (`Goal · 2/8` / paused / done / budget / blocked). Live smoke still SKIP without a key.
1616
- Hidden `cortex mcp-server --verify` stdio JSON-RPC server (`cortex-verify`) so CI and agents can audit TUI chrome, lock scenes, login product copy, and API error paths offline. Remains `hide = true` until Designer sign-off.
1717

1818
### Changed

src/cortex-engine/src/goal/persist.rs

Lines changed: 49 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,11 @@ pub fn load_goal(session_dir: impl AsRef<Path>) -> Result<Option<Goal>> {
3737
}
3838

3939
/// Load with an explicit missing / loaded / quarantined result for resume UX.
40+
///
41+
/// Load never deletes `.goal.json.tmp.*`. A concurrent `save_goal` may have
42+
/// that file open; removing it makes the writer's rename fail with ENOENT.
4043
pub fn load_goal_report(session_dir: impl AsRef<Path>) -> Result<GoalLoad> {
4144
let dir = session_dir.as_ref();
42-
cleanup_stale_tmps(dir);
4345
let path = goal_path(dir);
4446
if !path.exists() {
4547
return Ok(GoalLoad::Missing);
@@ -101,65 +103,6 @@ fn quarantine(path: &Path, why: &str) -> GoalLoad {
101103
GoalLoad::Quarantined { reason }
102104
}
103105

104-
fn is_goal_tmp_name(name: &str) -> bool {
105-
name.starts_with(".goal.json.tmp.") || name.starts_with(".goal.tmp.")
106-
}
107-
108-
fn tmp_owner_pid(name: &str) -> Option<u32> {
109-
name.rsplit_once('.')?.1.parse().ok()
110-
}
111-
112-
/// Probe whether `pid` still appears to be running.
113-
///
114-
/// A live owner may be mid-`save_goal`; deleting that temp makes the later
115-
/// rename fail with ENOENT and drops the write. Dead owners are leftovers.
116-
fn process_appears_alive(pid: u32) -> bool {
117-
if pid == 0 {
118-
return false;
119-
}
120-
if pid == std::process::id() {
121-
return true;
122-
}
123-
#[cfg(unix)]
124-
{
125-
// Safety: `kill(pid, 0)` delivers no signal; it only checks existence.
126-
let rc = unsafe { libc::kill(pid as libc::pid_t, 0) };
127-
if rc == 0 {
128-
return true;
129-
}
130-
std::io::Error::last_os_error().raw_os_error() != Some(libc::ESRCH)
131-
}
132-
#[cfg(not(unix))]
133-
{
134-
// Cannot probe; keep the file so a concurrent writer is never stolen.
135-
true
136-
}
137-
}
138-
139-
fn goal_tmp_owned_by_live_process(name: &str) -> bool {
140-
match tmp_owner_pid(name) {
141-
Some(pid) => process_appears_alive(pid),
142-
// Unknown suffix: do not delete a file we cannot attribute.
143-
None => true,
144-
}
145-
}
146-
147-
fn cleanup_stale_tmps(dir: &Path) {
148-
let Ok(entries) = std::fs::read_dir(dir) else {
149-
return;
150-
};
151-
for entry in entries.flatten() {
152-
let name = entry.file_name();
153-
let Some(name) = name.to_str() else {
154-
continue;
155-
};
156-
if !is_goal_tmp_name(name) || goal_tmp_owned_by_live_process(name) {
157-
continue;
158-
}
159-
let _ = std::fs::remove_file(entry.path());
160-
}
161-
}
162-
163106
fn durable_atomic_write(path: &Path, bytes: &[u8]) -> Result<()> {
164107
let parent = path.parent().ok_or_else(|| {
165108
CortexError::InvalidInput(format!("Cannot write goal to {}", path.display()))
@@ -275,51 +218,73 @@ mod tests {
275218
}
276219

277220
#[test]
278-
fn load_does_not_delete_tmp_owned_by_live_process() {
221+
fn load_never_deletes_goal_temps() {
279222
let dir = tempfile::tempdir().unwrap();
280223
let live = dir
281224
.path()
282225
.join(format!(".goal.json.tmp.{}", std::process::id()));
226+
let other = dir.path().join(".goal.json.tmp.1");
283227
std::fs::write(&live, b"in-flight").unwrap();
228+
std::fs::write(&other, b"other-writer").unwrap();
284229
assert!(matches!(
285230
load_goal_report(dir.path()).unwrap(),
286231
GoalLoad::Missing
287232
));
288-
assert!(
289-
live.exists(),
290-
"must not steal this process's in-flight goal write"
291-
);
233+
assert!(live.exists(), "load must not steal this process's temp");
234+
assert!(other.exists(), "load must not steal another writer's temp");
292235
}
293236

294-
#[cfg(unix)]
295237
#[test]
296-
fn load_does_not_delete_tmp_owned_by_init() {
238+
fn load_does_not_break_in_flight_writer_rename() {
297239
let dir = tempfile::tempdir().unwrap();
298-
let foreign = dir.path().join(".goal.json.tmp.1");
299-
std::fs::write(&foreign, b"other-process").unwrap();
240+
let goal = Goal::new("from writer");
241+
let tmp = dir
242+
.path()
243+
.join(format!(".goal.json.tmp.{}", std::process::id()));
244+
std::fs::write(&tmp, serde_json::to_string_pretty(&goal).unwrap()).unwrap();
300245
let _ = load_goal_report(dir.path()).unwrap();
301-
assert!(
302-
foreign.exists(),
303-
"must not delete another live process's goal temp"
304-
);
246+
std::fs::rename(&tmp, goal_path(dir.path())).expect("writer rename must not ENOENT");
247+
let loaded = load_goal(dir.path()).unwrap().unwrap();
248+
assert_eq!(loaded.objective, "from writer");
305249
}
306250

307251
#[cfg(unix)]
308252
#[test]
309-
fn load_removes_tmp_owned_by_dead_process() {
310-
let mut child = std::process::Command::new("true")
311-
.spawn()
312-
.expect("spawn short-lived helper");
313-
let pid = child.id();
314-
let _ = child.wait();
253+
fn parent_load_does_not_steal_child_writer_temp() {
315254
let dir = tempfile::tempdir().unwrap();
316-
let stale = dir.path().join(format!(".goal.json.tmp.{pid}"));
317-
std::fs::write(&stale, b"orphan").unwrap();
318-
let _ = load_goal_report(dir.path()).unwrap();
255+
let session = dir.path();
256+
let script = r#"
257+
set -e
258+
tmp="$1/.goal.json.tmp.$$"
259+
printf '%s\n' '{"schema_version":1,"id":"child","objective":"child-writer","state":"active","progress":null,"turns_used":0,"turn_budget":8,"tokens_used":0,"token_budget":null,"evidence":[],"last_reason":null,"created_at":0,"updated_at":0}' > "$tmp"
260+
echo ready > "$1/ready"
261+
while [ ! -f "$1/go" ]; do sleep 0.05; done
262+
mv "$tmp" "$1/goal.json"
263+
"#;
264+
let mut child = std::process::Command::new("sh")
265+
.arg("-c")
266+
.arg(script)
267+
.arg("goal-tmp-writer")
268+
.arg(session)
269+
.spawn()
270+
.expect("spawn child writer");
271+
let ready = session.join("ready");
272+
for _ in 0..200 {
273+
if ready.exists() {
274+
break;
275+
}
276+
std::thread::sleep(std::time::Duration::from_millis(10));
277+
}
278+
assert!(ready.exists(), "child should publish its in-flight temp");
279+
let _ = load_goal_report(session).unwrap();
280+
std::fs::write(session.join("go"), b"go").unwrap();
281+
let status = child.wait().expect("wait child writer");
319282
assert!(
320-
!stale.exists(),
321-
"dead-process leftover temp should be removed"
283+
status.success(),
284+
"child rename must succeed after parent load"
322285
);
286+
let loaded = load_goal(session).unwrap().unwrap();
287+
assert_eq!(loaded.objective, "child-writer");
323288
}
324289

325290
#[test]

0 commit comments

Comments
 (0)