Skip to content

fix(storage): preserve exclusive save fallback safety - #643

Open
sb123sb123 wants to merge 3 commits into
pyrite-wiki:devfrom
sb123sb123:fix/477-exclusive-save-errors
Open

sb123sb123 wants to merge 3 commits into
pyrite-wiki:devfrom
sb123sb123:fix/477-exclusive-save-errors

Conversation

@sb123sb123

@sb123sb123 sb123sb123 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

What changed

  • Keep hard-link publication first and handle its successful unlink separately, so cleanup errors are not misreported as target collisions.
  • Restrict link/O_EXCL fallbacks to unsupported-operation errors. Propagate ENOSPC, EDQUOT, EIO, and other ordinary failures.
  • If replace fails after this call creates an O_EXCL placeholder, remove it only while the target still identifies that placeholder; preserve a concurrent replacement.

Closes #477

Validation

  • The new regressions failed on the issue's base for placeholder cleanup, post-link error classification, ENOSPC safety, and unrelated link errors; the concurrent-replacement guard passed.
  • Windows G: focused tests/test_entry_save_atomic.py -k 'not mode': 16 passed, 2 deselected. The two deselected checks assert POSIX mode bits on the unchanged non-exclusive save path; Windows reported 0666 where they expect 0644/0600.
  • tests/test_changelog_fragments.py: 17 passed. Ruff check/format and git diff --check passed.
  • GitHub Actions run 37043656752: gate, verify-red, test (3.12), and changes passed; e2e, coverage, frontend, smoke, and KB were skipped by path filters.
  • That run began before dev advanced to e5d2643; the intervening change only edits kb/roadmap.md, outside this PR.
  • Spike evidence and candidate comparison: Exclusive create: two failure paths on filesystems without hard links leave a stray file or misreport a success #477 (comment)

@sb123sb123
sb123sb123 marked this pull request as ready for review October 2, 2026 18:47
@markramm

markramm commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator

Covers all three points of #477 with tests. One possible regression: now only EPERM, ENOTSUP, EOPNOTSUPP and ENOSYS count as "hard links unsupported". Our reading of CPython's Windows error mapping is that a hard-link failure on a FAT/exFAT drive surfaces as EINVAL, which would turn a save that used to fall back and succeed into an error. (Reasoned from the mapping, not tested.)

Property: on a filesystem without hard links, including Windows FAT/exFAT, creating an entry still succeeds. A test that makes the link step fail with EINVAL would pin it.

Automated review by an agent for the maintainer; comment only.

markramm commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator

Automated review. I'm an AI agent that triages pull requests for this project's maintainer. A person reads these and makes the final call. If I've got something wrong, say so here.

Thank you for the careful work on #477, including the spike report. I read the head (02e0457); its checks pass. This adds one point to the earlier review on this PR and does not repeat it.

Matches the issue

  • The groom's three cases each have a test in tests/test_entry_save_atomic.py: the placeholder is removed only while samestat still matches (a concurrent replacement is kept), ENOSPC and an EDQUOT test (skipped where the errno is absent) propagate, and the fallback errnos are named in _publish_exclusive.

Differs from the groom

  • The groom's acceptance for case 2 reads: "the save returns the path and leaves no temp file it can remove". In the diff, a failed os.unlink(tmp) after a successful link now raises its OSError, and test_unlink_error_after_successful_link_is_not_a_collision asserts pytest.raises(OSError) with errno.EIO. The entry is published, but save does not return and self.file_path is not set. The spike report asks only that the error not be classified as a collision. Which is wanted is the maintainer's call.

Overlaps another open pull request

Tests

  • The tests call NoteEntry.save(exclusive=True) directly with injected errors, not through a CLI, REST or MCP create.

Housekeeping

  • The changelog fragment is present.

This is a first pass for the maintainer, who makes the review decision.


Generated by Claude Code

markramm commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Automated review. I'm an AI agent that triages pull requests for this project's maintainer. A person reads these and makes the final call. If I've got something wrong, say so here.

Thank you for the follow-up work on #477. I read the current head (6aba94f); its checks pass (gate, test 3.12, verify-red). This adds to the earlier reviews on this PR and does not repeat them.

Matches the issue

  • The earlier EINVAL question is now handled: _publish_exclusive adds errno.EINVAL to the hard-link fallback set when os.name == "nt", with test_einval_from_hard_link_falls_back_only_on_windows.
  • A failed os.unlink(tmp) after a successful link is now retried once and then ignored, so save returns. That matches the groom's "returns the path and leaves no temp file it can remove" better than the previous head, which raised.

Property coverage

  • The EINVAL fallback is gated on os.name == "nt"; the earlier review mentioned FAT/exFAT generally. Not checked: whether other platforms report EINVAL for a link on such a filesystem.

Overlaps another open pull request

Tests

  • They call NoteEntry.save(exclusive=True) with injected errors, not a CLI, REST or MCP create.

Housekeeping

  • The changelog fragment is present.

This is a first pass for the maintainer, who makes the review decision.


Generated by Claude Code

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.

Exclusive create: two failure paths on filesystems without hard links leave a stray file or misreport a success

2 participants