Skip to content

fix(atomic_io): quarantine falls back to exclusive-create copy when os.link is unavailable - #966

Open
kvandre12-commits wants to merge 1 commit into
mpfaffenberger:mainfrom
kvandre12-commits:fix/atomic-quarantine-oslink-fallback
Open

kvandre12-commits wants to merge 1 commit into
mpfaffenberger:mainfrom
kvandre12-commits:fix/atomic-quarantine-oslink-fallback

Conversation

@kvandre12-commits

Copy link
Copy Markdown
Contributor

Problem

On Android/Termux (bionic), os.link is not available (hasattr(os, 'link') == False). atomic_io.quarantine_file() uses os.link to set a confirmed-corrupt config aside before recovery, so on those platforms a corrupt config crashes with AttributeError: module 'os' has no attribute 'link' instead of being quarantined and recovered — load_config/mutate_config raise instead of self-healing.

Fix

quarantine_file() now creates the backup via a small _hardlink_or_copy() helper:

  • Prefers os.link where available (atomic, shares the inode, refuses to overwrite) — unchanged on Linux/macOS/Windows.
  • Falls back to an exclusive-create copy (os.open(..., O_CREAT | O_EXCL)) only when os.link is missing (AttributeError) or unsupported by the filesystem (ENOSYS/EPERM/EOPNOTSUPP/EMLINK/EXDEV). EACCES is surfaced, not treated as 'unsupported'.

The fallback preserves the guarantees that matter (but not hard-link identity):

  • no-overwrite: O_EXCL raises FileExistsError exactly like os.link, so the existing retry loop still picks a fresh collision-resistant name and never clobbers a prior backup;
  • byte integrity: streamed via fd-level os.read in bounded chunks (memory never balloons); every os.write is looped until the whole buffer lands;
  • permissions: source permission bits are copied;
  • cleanup / rollback / no-data-loss: dst is fsynced before return; on any failure the partial destination is removed, fds are closed, and the error re-raised with the source untouched.

Inode identity, link count, ownership and timestamps are NOT reproduced (only data + mode). The retry/unlink/rollback loop is otherwise unchanged.

Tests

New tests/test_atomic_io_quarantine_fallback.py (forces os.link absent so it runs everywhere): name/content preservation + original vacated; never overwrites an existing backup; partial-failure rollback for injected read/write/fsync/close and source-unlink failures (source always intact, partial dest removed); os.write short-write looping; EACCES surfaced (not downgraded); symlink source; and os.link still preferred where available. Existing collision tests were made backend-agnostic.

Verification

…s.link is unavailable

Android/Termux (bionic) does not expose os.link, so the hard-link-based
quarantine_file() raised AttributeError instead of quarantining a corrupt
config -- breaking config-corruption recovery on that platform (and masking
nine test failures behind a pytest WindowsPath INTERNALERROR).

quarantine_file() now creates the backup via _hardlink_or_copy(): it prefers
os.link where available (atomic, shares the inode) and otherwise falls back to
an exclusive-create (O_CREAT|O_EXCL) copy. The fallback preserves the
guarantees that matter -- but not perfect hard-link identity:
  - no-overwrite: O_EXCL raises FileExistsError like os.link (retry loop);
  - byte integrity: bounded os.read chunks; every os.write looped to completion;
  - permissions: source permission bits are copied;
  - rollback / no-data-loss: fsync before return; on ANY failure the partial
    destination is removed, fds are closed, and the error re-raised with the
    source untouched.
It does NOT reproduce inode identity, link count, ownership, or timestamps.
EACCES is surfaced (a real restriction), never treated as "unsupported"; only
ENOSYS/EPERM/EOPNOTSUPP/EMLINK/EXDEV trigger the fallback. The existing
retry/unlink/rollback loop is unchanged.

Adds tests/test_atomic_io_quarantine_fallback.py (name/content preservation,
no-overwrite, partial-failure rollback for read/write/fsync/close, short
writes, EACCES surfacing, symlink handling, os.link preferred where supported)
and makes the two existing os.link-hardcoded collision tests backend-agnostic.
@kvandre12-commits

Copy link
Copy Markdown
Contributor Author

CI passed Ruff and Windows encoding. The macOS test job had one failure in tests/mcp/test_tool_failure_not_fatal.py::test_failing_tool_does_not_kill_the_run, ending in CancelledError / a server-session initialization deadline. That test is outside this PR's four-file atomic-I/O scope.

The identical head and base passed the full macOS suite in fork qualification PR #10: https://github.com/kvandre12-commits/code_puppy/actions/runs/36147090556

A rerun may clear the timeout when convenient.

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.

1 participant