fix(execution): follow-ups from the review of #309, relative artifact paths, chat attachment lease

[dovvnloading]

Problem

An independent review of #309 found six small defects in the execution store, and one backlog item was only partly done.

• A job id could be a Windows device name (nul, con, aux, com1, nul.txt, ...) or end in a dot. create_job accepted it and the first publish_artifact then failed with a raw OSError carrying the absolute path; a. shared a directory with a.
• finish_unsuccessful_job says nothing raises, but the job read and the cancel probe sat outside its try.
• A failing artifact-root sweep (for example connect() on a locked store) recorded a retention pass that had already finished as failed and dropped its counters.
• The sweep's removal limit was soft: with limit=1, one directory of 64 stale files lost 65 things in one pass. The entry-budget clause had no test that would fail without it.
• Chat attachment staging had the same create, publish, complete crash window that execution staging already closed: a stager killed part-way left a queued job that recovery cannot see, in the task tray for good.
• Artifact rows and cleanup tombstones recorded absolute paths, so moving the data directory left every row pointing at nothing: live attachments stopped reading and expired ones were never reclaimed.

Root cause

• The job id was held to the artifact-name shape, which allows nul and a trailing dot; those are not usable directory names on Windows.
• get_job and the probe were simply written outside the guard.
• run_once called the sweep inside the same try as the retention pass.
• The limit was checked between top-level entries only, so one job directory could exceed it.
• ChatAttachmentService.stage created the job and published without holding a lease, and a retry with the same request id treated any non-succeeded job as a conflict without checking whether anyone was working on it.
• publish_artifact and _record_artifact_cleanup stored str(path).

Change

One commit per concern.

1. fix(execution) job ids: a stricter shape (no trailing dot, no device name, with or without an extension) at create_job and publish_artifact. OSError from mkdir, resolve and the temporary file's open is reported as ExecutionRepositoryError without the path, and a failing unlink in the failure handler no longer replaces the error being raised.
2. fix(execution) finish_unsuccessful_job: the job read and the probe are inside the guard; failures are logged by exception type only.
3. fix(execution) supervisor: the sweep has its own guard. The pass still counts as a success with its counters, the type is logged and kept in last_error, and the sweep is retried next pass.
4. fix(execution) sweep bound: each entry is handed the removals left. A job directory stops at the limit and is resumed at the same directory by the next pass. The docstring states exactly what is bounded (removals exactly; the entry budget between top-level entries, so it can be overshot by one directory's children).
5. test(execution) mutants: recording-read tests for owner= in attachment_staging._existing and recipe_coordinator._load_input, and a real-junction test for the sweep's reparse-point check. The fourth survivor (the containment clause before mkdir) is pinned in the job-id commit's test file.
6. fix(chat): a short lease is held while a chat attachment is staged and released on every exit that runs code; a retry of the same request retires a job nobody is working on (no live lease, no write for ABANDONED_STAGE_SECONDS). The answer to the retry is unchanged.
7. feat(execution) relative paths: new artifact rows and new tombstones record their path relative to the artifact root. Both forms are read; get_artifact still reports an absolute path. This commit's own message says "schema version 4"; that bump is removed again by item 10, so the version stays 3.
8. test(execution): the junction test is skipped where Path.is_junction does not exist (Python before 3.12).
9. fix(chat) (review fix): ChatAttachmentService._stage_created now checks the job that the final transition returns. A stager whose job a retry had already retired as abandoned used to return a descriptor and leave its artifact row behind; it now removes the artifact (best effort) and raises attachment_persist_failed.
10. fix(execution) (review fix): the schema version is left at 3 and the ladder is back to its original form, so the previous release can open a store this build wrote. The three tests that pin the version are back to the literal 3.
11. test(execution) (review fix): tests that kill three surviving mutants (the sweep's complete flag, the ladder's step condition, owner= on get_artifact in attachment_staging._existing).

Main was merged into the branch several times (no conflicts); nothing was rebased or force-pushed after review.

Compatibility and rollback

• No schema change. Recording relative paths adds no table and no column, so SCHEMA_VERSION stays 3 and the migration ladder is unchanged. Rows written by an earlier build keep their absolute paths and keep working; new rows are relative; the read path accepts both.
• Verified against the real previous build (origin/main's code, version 3) opening a store this branch wrote (two jobs, one live artifact, one expired):
  • nothing is set aside: no new file, the version stays 3, the same installation principal is read, and both jobs are kept;
  • the live artifact's row is kept, but reading it fails closed with "Artifact path is unavailable", because that build cannot resolve a relative path. It reads again once this build is back;
  • only the expired row is discarded (its cleanup pass reports skipped=1). That build's artifact-root sweep matches files to rows by artifact id, so it removes only the expired row's file and the directory that emptied, and keeps the live file.
• So rolling back keeps every job, fails closed on reading new artifacts, and loses only expired rows. An earlier revision of this branch bumped the version to 4; that made the previous build set the whole store aside (jobs and principal lost) and its sweep later delete every artifact file, which is why the bump was removed.
• A store stamped 4 by a build of that earlier revision would be set aside by this build, because it looks newer. Nothing with that stamp was released.
• Rows written before this change are not rewritten, so they still do not survive a data-directory move (they are contained and dropped exactly as before). Only rows written from now on do.
• Job ids from earlier builds are unaffected in practice: every production id is a uuid4 hex. A hand-made id that ends in a dot or is a device name is now refused at publish with "Execution job does not exist."

Checks

At the current head (after the review fixes and the merges of main):

• python -m ruff check backend tests tools main.py app_factory.py scripts: all checks passed.
• python -m mypy: no issues in 97 source files.
• python -m pytest -q on Python 3.14.0 (after merging test(execution): stop the cleanup tests depending on a one-second window #312, ci: run the dependency refresh on Node 24 #317 and feat(frontend): pick GGUF files from a repository, cancel and resume model downloads #314): 2834 passed, 25 subtests passed. After merging fix(api): small API defects - drop the raw message write, kind-checked /generations, can_cancel, client route check, one-query task list, 1 MiB body ceiling #316, the store, execution API, chat attachment, relative-path, installation-principal, reclamation, attachment-staging and allow-once modules: 189 passed.
• Python 3.12.6, the changed and related modules (chat attachments, relative paths, installation principal, reclamation, attachment staging, store, allow-once approval, cleanup, finish, path safety): 260 passed.
• python tools/generate_contracts.py --check: exit 0. python tools/artifact_boundary_review.py --json --strict: 12 of 12 passed.
• The pre-push hook on the pushed head: all 11 checks passed in 376 s (backend suite 229 s, vitest 835 passed). The first attempt failed only in the frontend unit tests with a Node/V8 crash (exit 134) on a loaded machine; the identical retry passed.
• Mutants, each applied by hand and reverted, and each killed by a named test: dropping owner= in attachment_staging._existing (both get_artifact and read_artifact), dropping it in recipe_coordinator._load_input, removing _is_reparse_point from _sweep_entry, removing planned.parent != root before mkdir, removing the entry-budget clause, removing the removed >= limit clause, replacing if not complete: in the sweep, widening the ladder's step condition, setting SCHEMA_VERSION = 4, removing the returned-job check in _stage_created, and narrowing the except around the artifact removal. The test names for the review fixes are in the review-fix comment below.

From before the review fixes (earlier head, not re-run as a whole):

• Python 3.12.6, whole suite: 2479 passed, 1 skipped, 83 failed. All 83 were ModuleNotFoundError: No module named 'gguf' on that interpreter (82 in test_gguf_download.py, 1 in test_repository_conventions.py); none is in a module this change touches.
• Failing before, passing after: the tests for each repaired behaviour were run against the unfixed code first and failed there (24, 2, 2, 5, 4 and 5 failing tests for the job-id, finish, supervisor, sweep-bound, chat-lease and relative-path changes, in that order). The containment and failure-path tests for relative rows also pass on the old code, by design: they pin behaviour that must not change.
• Not run by me: Python 3.10 and 3.13 (not installed here). The frontend is not touched by this change; its checks ran only as part of the push hook.

Security / data-loss / concurrency

• Relative rows are confined by the same validators as absolute ones. Tests cover a row that climbs out with .., a rooted path, a path on another drive, one that names the quarantine directory, one that names a directory, and a quarantine path that points at another job's live file: each is refused or discarded and the file it points at is untouched.
• No path or message from an OSError reaches an error message or a log; exception types only.
• The sweep never removes more than its limit and never advances past an entry it has not finished, so a partly swept directory is resumed, not skipped.
• The chat lease is short (60 seconds), released in finally, and expires by itself if the process is killed. A retry can only retire a job that matches its own request and has neither a live lease nor a recent write.

Limits

• Chat attachment staging is only partly closed. The startup recovery in local_runtime does not yet fail a chat.attachment.v1 job whose lease it reclaims, so a stager killed and then never retried is still left queued. That file is being changed by another pull request, so it is left for a follow-up.
• The OSError wrap covers mkdir, resolve and open. A failure in the write, fsync or os.replace still propagates as a raw OSError (existing behaviour, with tests written for it). ArtifactBoundary.publish_outputs catches only ExecutionRepositoryError, so such a failure escapes it after earlier outputs were published; not changed here.
• The junction test runs on Windows with Python 3.12 or later. On 3.10 and 3.11 _is_reparse_point cannot see a junction; what the sweep does with one there was not verified.
• The entry budget can still be overshot by one job directory's children (at most 65 entries); only removals are exact.
• The chat stager's lease is 60 seconds and is never renewed. A stage that outlives it can still be retired by a matching retry; it now fails closed (an error, no orphaned row) instead of reporting a success that resolve later refuses. If publish_artifact succeeds and the completing transition then raises, the artifact row waits for retention, as it did before.
• ChatAttachmentService.resolve and _existing still read the artifact without owner= (the owner-scoped get_artifact runs first); create_job in stage is not wrapped in a repository-error translation. Neither is changed.

Plan item: EX-05

🤖 Generated with Claude Code

[dovvnloading]

Review fixes (head a75ca87)

Three commits on top of the branch, plus merges of main (#312, #317, #314, #316). The merges had no conflicts, and git diff origin/main --stat lists only this pull request's 13 files.

1. Major: a retired chat stager reported success (fixed, c09dd8f5)

ChatAttachmentService._stage_created ignored the job that transition(..., "succeeded") returns. A finished job is immutable, so when a retry had retired the job as abandoned (lease lapsed, 120 s idle) the transition wrote nothing and returned the failed job, and the original call still returned a descriptor and left the artifact row behind. It now captures the returned job; when the status is not succeeded it removes the published artifact (best effort: a failing removal is swallowed, the row waits for retention) and raises ChatAttachmentError("attachment_persist_failed"), the error the caller already maps. No cause is chained, so no store path can reach the message.

• tests/test_chat_attachments.py::test_a_stager_retired_between_publish_and_completion_fails_and_leaves_no_artifact: retires the job between publish and completion (clock advanced, matching retry through the service), asserts the error, the job ends failed / attachment_interrupted with result None, no completed event, no lease, and no artifact row or file.
• tests/test_chat_attachments.py::test_a_failed_removal_of_the_orphaned_artifact_never_replaces_the_staging_error: a removal that raises a path-bearing error still yields attachment_persist_failed, with no __cause__ and no location in the message.
• Mutants, applied by hand and reverted: if completed.status != "succeeded": replaced by if False: fails both tests (DID NOT RAISE); narrowing except Exception in _discard_artifact fails the second.

2. Rollback: the schema version stays 3 (fixed, 09ac69f2)

The previous release build treats a store stamped 4 as newer than itself and sets the whole file aside, so the bump is removed. SCHEMA_VERSION is 3 again and the ladder is back to if current_version < SCHEMA_VERSION:, with no version-4 step. New rows are still written relative to the artifact root and the read path still accepts both forms. Comments and docstrings that described a version-4 step are rewritten.

Rollback behaviour, checked against the real previous build (origin/main's code, version 3) opening a store this head wrote (two jobs, one live artifact, one expired):

• Nothing is set aside: no new file appears, the version stays 3, the same installation principal is read and both jobs are kept.
• The live artifact's row is kept but reading it fails closed ("Artifact path is unavailable"), because that build resolves a relative path against the working directory. It reads again once this build is back.
• Only the expired row is discarded (its cleanup pass reports skipped=1). That build's artifact-root sweep matches files to rows by artifact id, so it removes only the expired row's file (and the directory that emptied) and keeps the live file.

Tests:

• The three tests that pin the version are back to the literal == 3, so they no longer follow the constant: tests/test_execution_store.py::test_durable_idempotency_event_replay_and_restart_recovery, tests/test_installation_principal.py::test_installation_principal_is_atomic_persistent_and_migrates_additively, tests/test_allow_once_approval.py::test_the_use_record_is_additive_and_is_removed_with_its_job. (== SCHEMA_VERSION pins nothing, so I read "pin the version again" as the literal.)
• tests/test_execution_relative_paths.py::test_a_store_holding_absolute_and_relative_rows_opens_and_serves_and_reclaims_both: one store with live and expired rows of both forms and pending tombstones of both forms; opens at version 3 unchanged, both forms read, both expired rows reclaimed with nothing skipped.
• tests/test_execution_relative_paths.py::test_new_rows_leave_the_schema_version_where_the_previous_build_reads_it and ::test_a_store_the_previous_build_wrote_is_read_as_it_was_and_is_not_rewritten (renamed from the "before version 4" test) pin the version against a constant spelled out in the test file.
• Mutant: SCHEMA_VERSION = 4 fails 7 tests (the three above and four in the relative-path and owner-migration tests).

3. Surviving mutants (fixed, 09ac69f2 and 464eb137)

• (a) tests/test_artifact_reclamation.py::test_an_entry_cut_short_by_the_limit_is_resumed_not_skipped now runs the passes one by one and asserts that job-a is finished (gone) before job-b loses anything. Replacing if not complete: with if False: in sweep_artifact_root now fails it ("job-a was not finished before the sweep went on to job-b"); the per-pass totals alone were identical with and without the flag.
• (b) tests/test_installation_principal.py::test_a_current_store_is_not_put_through_the_owner_migration_again: a version-3 store in which two owners share one request id must reopen without "ambiguous", with owners and principal unchanged. Widening the step condition to <= fails it. This adapts the test to the ladder as it now stands, since the version-3 step is the only one left.
• (c) tests/test_attachment_staging.py::test_a_repeated_request_looks_up_and_reads_the_staged_artifact_as_its_owner_never_unscoped (renamed) records the owner= passed to both get_artifact and read_artifact. Dropping owner= from the get_artifact call in _existing now fails it.

Documented, not fixed

• A store stamped 4 by a build of this branch before this fix would be set aside by this build (it looks newer). Nothing with that stamp was released; it can only exist in a development copy.
• The stager's lease is still 60 s and never renewed. A stage that outlives it can still be retired by a matching retry; the difference now is that it fails closed (an error, no orphaned row) instead of reporting a success that resolve later refuses.
• If publish_artifact succeeds and transition then raises, the artifact row is left until retention (unchanged from main; only the returned-failed-job path is handled here).

Checks, from the worktree root

• python -m ruff check backend tests tools main.py app_factory.py scripts: all checks passed.
• python -m mypy: no issues in 97 source files.
• python -m pytest -q on Python 3.14.0 after merging test(execution): stop the cleanup tests depending on a one-second window #312, ci: run the dependency refresh on Node 24 #317 and feat(frontend): pick GGUF files from a repository, cancel and resume model downloads #314: 2834 passed, 25 subtests passed. After merging fix(api): small API defects - drop the raw message write, kind-checked /generations, can_cancel, client route check, one-query task list, 1 MiB body ceiling #316 the narrow set (store, execution API, chat attachments, relative paths, installation principal, reclamation, attachment staging, allow-once approval): 189 passed.
• Python 3.12.6, the changed and related test files (chat attachments, relative paths, installation principal, reclamation, attachment staging, store, allow-once approval, cleanup, finish, path safety): 260 passed.
• python tools/generate_contracts.py --check: exit 0. python tools/artifact_boundary_review.py --json --strict: 12 of 12 passed.
• Pre-push hook on the pushed head: all 11 checks passed in 376 s (backend suite 229 s, vitest 835 passed). The first attempt failed only in the frontend unit tests, with a Node/V8 crash (exit 134) while the machine was loaded; the identical retry passed. Python 3.10 and 3.13 are not installed here and were not run.