Skip to content

451efd1c - Reap what a killed worker leaves behind - #59

Draft
davidleomay wants to merge 2 commits into
developfrom
451efd1c-runner-reap
Draft

451efd1c - Reap what a killed worker leaves behind#59
davidleomay wants to merge 2 commits into
developfrom
451efd1c-runner-reap

Conversation

@davidleomay

Copy link
Copy Markdown
Member

EN:
Sixth phase of moving the runner here (#45): the artefacts a killed worker leaves behind are now reaped. Orphaned tmux sessions are killed and orphaned worktrees removed through git, while a job still being prepared for dispatch is protected by its marker — until that marker is old enough to count as abandoned. Nothing here deletes a path outside the work root, and a listing this instance cannot read is never taken to mean nothing is running. The raw removal of directories no job row knows about is deliberately not part of this change.

DE:
Sechste Phase der Verlagerung des Runners (#45): Was ein beendeter Worker zurücklässt, wird jetzt eingesammelt. Verwaiste tmux-Sessions werden beendet und verwaiste Arbeitskopien über git entfernt, während ein Auftrag in Vorbereitung durch seinen Marker geschützt bleibt — bis dieser alt genug ist, um als abgebrochen zu gelten. Nichts hier löscht einen Pfad ausserhalb der Arbeitswurzel, und eine nicht lesbare Session-Liste gilt nie als „nichts läuft". Das direkte Entfernen von Verzeichnissen ohne zugehörige Auftragszeile gehört bewusst nicht dazu.

Details

One pure module, one wiring. reap.py decides what is orphaned — it holds no clock, no filesystem and no store, and deletes nothing. reap_orphans collects the inputs, applies those decisions, and issues the git and tmux commands.

Four guards, each pinned by a test that fails when the guard is removed:

  • A failed session listing kills nothing. Reading "cannot list" as "nothing is running" would kill every session on the machine. This rule is now enforced by exactly one check — see the note on double protection below.
  • A computed path that escapes the work root is refused. A job id carrying a traversal segment would otherwise let a path leave the work root on its way to a command that deletes it.
  • An orphan with no job row is skipped, never deleted. Removing a directory nothing knows about needs a raw delete, which this change does not do.
  • A session whose job holds a fresh preparing marker survives. Killing it would abandon a dispatch mid-flight.

Markers expire. A job being prepared writes a marker so its half-built worktree is not reaped mid-setup. If the marker cannot be read, the job counts as active — failing the other way would reap a worktree being built right now. But past the maximum age it stops protecting: an abandoned dispatch must not shield a real orphan forever.

Two defects were found by the tests and fixed:

  • The preparing set was built only from work directories, then used to protect sessions as well. A session whose job held a fresh marker but had no directory yet would have been killed. The original consults the marker for the session's own job id, independent of any directory; the candidate set now covers both sources.
  • The "failed listing kills nothing" rule was guarded twice — by the flag and by an empty name list — so neither guard could be exercised and removing either left the suite green. It is now a single check, and the test supplies a name that would be killed if that check were dropped. Double protection reads as safety but makes a rule untestable, and an untestable safety rule quietly stops being one.

One guard is deliberately not pinned. In within_root, the explicit candidate == root rejection cannot be mutated red, because the prefix test below it already excludes equality. It stays because the rule is worth stating where a reader looks for it rather than leaving it implied by a string comparison, and the comment there says so — the same treatment ingest.py already gives its own unmutatable guard.

A limit that must survive into the next change. within_root is a lexical check. The original resolved symlinks before comparing; a pure module cannot, so the caller must pass already-resolved paths. A symlink inside the root pointing outside it would not be caught. That matters most for the raw removal below, which is where an unbounded delete would live.

Verification. 850 passed under Python 3.13. Two failures in tests/test_dashboard_control.py are pre-existing and unrelated — they reproduce identically on an unmodified base on the same machine, where the dashboard's local HTTP server does not bind in time, and CI is green.

Deliberately not here. Removing a work directory that no job row knows about requires a raw recursive delete — the only unbounded deletion in this port. It is left out so it can be reviewed on its own terms, with the symlink-resolution obligation above met at the call site. Such directories are counted as skipped for now.

@TaprootFreakAI

Copy link
Copy Markdown
Collaborator

EN:
Recommendation: keep this in draft until command failures are handled and the required reviews are completed. In reap_orphans(), the return codes from worktree_remove_argv, worktree_prune_argv, and kill_session_argv are ignored; job/session IDs are appended to the removed/killed result lists even when the commands fail.

I reproduced this against the PR's function using an in-memory store and a stub runner: session listing succeeded, every remove/prune/kill command returned exit code 1, yet the function returned (['dead-job'], ['agent-orphan'], 0). No real deletion or process termination was performed. Please report failed operations explicitly and only count removals/terminations confirmed successful, with regression coverage for nonzero exit codes.

The cleanup capability is useful and GitHub currently reports no merge conflicts. Before wiring it into the runner, also verify compatibility with the native tmux naming changes recently merged in #60 and enforce the documented resolved-path requirement at the deletion boundary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants