feat: stop PictoPy's own file writes from triggering a folder resync - #1482
feat: stop PictoPy's own file writes from triggering a folder resync#1482rohan-pandeyy wants to merge 3 commits into
Conversation
The watcher treats any change under a watched folder as a user edit, so a file PictoPy writes comes straight back as a full resync. Records each write before the bytes land and lets the watcher claim it instead.
|
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR adds SQLite-backed self-write tracking, atomic file replacement, application initialization, and watcher filtering. Matching events are claimed and skipped based on normalized paths, file size, and modification time. ChangesSelf-write suppression flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PictoPy
participant SelfWriteUtility
participant SQLiteLedger
participant Watcher
participant FolderSynchronization
PictoPy->>SelfWriteUtility: replace file data
SelfWriteUtility->>SQLiteLedger: record expected path, size, and mtime
SelfWriteUtility-->>PictoPy: return replacement result
Watcher->>SQLiteLedger: match observed file events
SQLiteLedger-->>Watcher: return and claim self-write paths
Watcher->>FolderSynchronization: synchronize remaining events
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
sync-microservice/app/utils/watcher.py (1)
68-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
watcher_util_drop_self_writesto match what it returns.The function drops nothing from its input. It returns the subset of paths that match a recorded self-write, and the caller decides what to skip. A name such as
watcher_util_find_self_writesorwatcher_util_claim_self_writesstates that, and "claim" matches the wording used indb_take_matching_self_writes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sync-microservice/app/utils/watcher.py` around lines 68 - 84, Rename watcher_util_drop_self_writes to watcher_util_find_self_writes (or watcher_util_claim_self_writes) and update all call sites to match its behavior of returning matching self-write paths without modifying the input.backend/app/utils/self_write.py (1)
50-63: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider preserving the target file mode.
tempfile.mkstempcreates the temp file with mode 0600, andos.replacekeeps that mode. After a replacement, the photo is readable only by the owner. If the library sits on a shared or group-readable location, other users lose access.♻️ Optional fix
stats = os.stat(temp_path) + try: + os.chmod(temp_path, os.stat(target).st_mode & 0o7777) + except OSError: + # New target, or a filesystem without modes: keep the default. + pass db_record_self_write(target, stats.st_size, int(stats.st_mtime))Call
os.stat(temp_path)afteros.chmodso the recorded mtime stays accurate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/utils/self_write.py` around lines 50 - 63, Preserve the existing target file permissions in the self-write replacement flow by reading the target mode and applying it to temp_path with os.chmod before the os.stat(temp_path) call and os.replace operation. Keep db_record_self_write using the post-chmod temp-file metadata so the recorded mtime remains accurate.backend/app/database/self_writes.py (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeprecated
typingaliases in the new modules. Ruff reports UP035 on each new file because the code usestyping.List,typing.Set,typing.Tuple, andtyping.Iterator. Builtin generics work at runtime from Python 3.9 onward, which matches the declared floor for this repository, so the replacements are safe.
backend/app/database/self_writes.py#L16-L16: replace the import withfrom typing import Setremoval and uselist[ObservedFile],set[str], andObservedFile = tuple[str, int, int].sync-microservice/app/database/self_writes.py#L11-L11: apply the same replacement forList,Set, andTuple.backend/tests/test_self_write.py#L11-L11: importIteratorfromcollections.abc.Run
ruff check --fixon the three files; do not runruff format.As per coding guidelines: "Ruff may be used as a linter and with
--fix."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/database/self_writes.py` at line 16, Replace deprecated typing aliases with Python 3.9+ builtin generics in backend/app/database/self_writes.py (line 16) and sync-microservice/app/database/self_writes.py (line 11), including list, set, and tuple annotations and removing unused typing imports. In backend/tests/test_self_write.py (line 11), import Iterator from collections.abc. Run ruff check --fix on all three files, without running ruff format.Sources: Coding guidelines, Linters/SAST tools
backend/tests/test_self_write.py (1)
44-92: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd one case for duplicate observed paths.
The suite does not cover a batch where two observed entries normalize to the same key. That is the input that breaks
db_take_matching_self_writestoday (see the comment onbackend/app/database/self_writes.pylines 118-127). One assertion pins the fix.💚 Proposed test
def test_nothing_observed_queries_nothing(self, test_db): assert db_take_matching_self_writes([]) == set() + + def test_repeated_observations_of_one_path_still_match(self, test_db, tmp_path): + """One batch can carry an add and a modify for the same file.""" + photo = tmp_path / "a.jpg" + photo.write_bytes(b"x" * 100) + observed = _observe(str(photo)) + db_record_self_write(*observed) + + assert db_take_matching_self_writes([observed, observed]) == {observed[0]}Also applies to: 102-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_self_write.py` around lines 44 - 92, Add a test method in TestLedger covering a batch with duplicate observed entries for the same path and metadata, verifying db_take_matching_self_writes handles the normalized duplicate key correctly with one expected claim. Use the existing _observe and db_record_self_write helpers and assert the precise returned path set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/database/self_writes.py`:
- Around line 118-127: Update the placeholder construction in the self-write
query to derive its count from the unique keys in by_key, matching the list of
bound parameters. Keep the existing deduplication and query behavior unchanged
so duplicate observed paths do not cause a binding-count error.
- Around line 37-38: Update the _connect() helper to enable SQLite foreign-key
enforcement with PRAGMA foreign_keys = ON immediately after creating the
connection, matching the behavior of other _connect() helpers while still
returning the configured connection.
In `@backend/tests/test_self_write.py`:
- Around line 93-100: Update test_lookup_survives_a_differently_spelled_path to
assert the platform-independent abspath normalization on all systems, and guard
the path.upper() case-folding scenario with a Windows-only condition. Keep the
db_take_matching_self_writes expectation for the normalized path, while avoiding
the uppercase comparison on POSIX.
---
Nitpick comments:
In `@backend/app/database/self_writes.py`:
- Line 16: Replace deprecated typing aliases with Python 3.9+ builtin generics
in backend/app/database/self_writes.py (line 16) and
sync-microservice/app/database/self_writes.py (line 11), including list, set,
and tuple annotations and removing unused typing imports. In
backend/tests/test_self_write.py (line 11), import Iterator from
collections.abc. Run ruff check --fix on all three files, without running ruff
format.
In `@backend/app/utils/self_write.py`:
- Around line 50-63: Preserve the existing target file permissions in the
self-write replacement flow by reading the target mode and applying it to
temp_path with os.chmod before the os.stat(temp_path) call and os.replace
operation. Keep db_record_self_write using the post-chmod temp-file metadata so
the recorded mtime remains accurate.
In `@backend/tests/test_self_write.py`:
- Around line 44-92: Add a test method in TestLedger covering a batch with
duplicate observed entries for the same path and metadata, verifying
db_take_matching_self_writes handles the normalized duplicate key correctly with
one expected claim. Use the existing _observe and db_record_self_write helpers
and assert the precise returned path set.
In `@sync-microservice/app/utils/watcher.py`:
- Around line 68-84: Rename watcher_util_drop_self_writes to
watcher_util_find_self_writes (or watcher_util_claim_self_writes) and update all
call sites to match its behavior of returning matching self-write paths without
modifying the input.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d929ef08-5f25-4b0e-b41a-cfe045c597dc
📒 Files selected for processing (7)
backend/app/database/self_writes.pybackend/app/utils/self_write.pybackend/main.pybackend/tests/conftest.pybackend/tests/test_self_write.pysync-microservice/app/database/self_writes.pysync-microservice/app/utils/watcher.py
| def test_lookup_survives_a_differently_spelled_path(self, test_db, tmp_path): | ||
| """The watcher reports whatever the OS hands it; the key has to absorb that.""" | ||
| photo = tmp_path / "a.jpg" | ||
| photo.write_bytes(b"x" * 100) | ||
| path, size, mtime = _observe(str(photo)) | ||
| db_record_self_write(path.upper(), size, mtime) | ||
|
|
||
| assert db_take_matching_self_writes([(path, size, mtime)]) == {path} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
This test only passes on Windows; CI fails on Linux.
os.path.normcase lowercases the path on Windows and returns it unchanged on POSIX. The test records path.upper() and expects a match, so the keys differ on the Linux runner and db_take_matching_self_writes returns an empty set. The pipeline reports exactly this failure at line 100.
The production code is correct. Assert the normalization that os.path.abspath performs on every platform, and keep the case-folding assertion behind a Windows guard.
💚 Proposed fix
def test_lookup_survives_a_differently_spelled_path(self, test_db, tmp_path):
"""The watcher reports whatever the OS hands it; the key has to absorb that."""
photo = tmp_path / "a.jpg"
photo.write_bytes(b"x" * 100)
path, size, mtime = _observe(str(photo))
- db_record_self_write(path.upper(), size, mtime)
+ # Redundant "." and separator components resolve the same way on every
+ # platform; case folding only applies on Windows.
+ noisy = os.path.join(str(tmp_path), ".", "", "a.jpg")
+ db_record_self_write(noisy, size, mtime)
assert db_take_matching_self_writes([(path, size, mtime)]) == {path}
+
+ `@pytest.mark.skipif`(os.name != "nt", reason="normcase folds case only on Windows")
+ def test_lookup_survives_a_differently_cased_path(self, test_db, tmp_path):
+ photo = tmp_path / "a.jpg"
+ photo.write_bytes(b"x" * 100)
+ path, size, mtime = _observe(str(photo))
+ db_record_self_write(path.upper(), size, mtime)
+
+ assert db_take_matching_self_writes([(path, size, mtime)]) == {path}As per pipeline failures: "Expected db_take_matching_self_writes([(path, size, mtime)]) to return the observed path, but it returned an empty set when the recorded path was uppercase".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_lookup_survives_a_differently_spelled_path(self, test_db, tmp_path): | |
| """The watcher reports whatever the OS hands it; the key has to absorb that.""" | |
| photo = tmp_path / "a.jpg" | |
| photo.write_bytes(b"x" * 100) | |
| path, size, mtime = _observe(str(photo)) | |
| db_record_self_write(path.upper(), size, mtime) | |
| assert db_take_matching_self_writes([(path, size, mtime)]) == {path} | |
| def test_lookup_survives_a_differently_spelled_path(self, test_db, tmp_path): | |
| """The watcher reports whatever the OS hands it; the key has to absorb that.""" | |
| photo = tmp_path / "a.jpg" | |
| photo.write_bytes(b"x" * 100) | |
| path, size, mtime = _observe(str(photo)) | |
| # Redundant "." and separator components resolve the same way on every | |
| # platform; case folding only applies on Windows. | |
| noisy = os.path.join(str(tmp_path), ".", "", "a.jpg") | |
| db_record_self_write(noisy, size, mtime) | |
| assert db_take_matching_self_writes([(path, size, mtime)]) == {path} | |
| `@pytest.mark.skipif`(os.name != "nt", reason="normcase folds case only on Windows") | |
| def test_lookup_survives_a_differently_cased_path(self, test_db, tmp_path): | |
| photo = tmp_path / "a.jpg" | |
| photo.write_bytes(b"x" * 100) | |
| path, size, mtime = _observe(str(photo)) | |
| db_record_self_write(path.upper(), size, mtime) | |
| assert db_take_matching_self_writes([(path, size, mtime)]) == {path} |
🧰 Tools
🪛 GitHub Actions: PR Check / 1_Backend Tests.txt
[error] 100-100: Pytest test_lookup_survives_a_differently_spelled_path failed: db_take_matching_self_writes returned an empty set instead of the expected path when the recorded path used different casing. Command 'cd backend && pytest' failed with exit code 1.
🪛 GitHub Actions: PR Check / Backend Tests
[error] 100-100: pytest failed in tests/test_self_write.py::TestLedger::test_lookup_survives_a_differently_spelled_path. Expected db_take_matching_self_writes([(path, size, mtime)]) to return the observed path, but it returned an empty set when the recorded path was uppercase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_self_write.py` around lines 93 - 100, Update
test_lookup_survives_a_differently_spelled_path to assert the
platform-independent abspath normalization on all systems, and guard the
path.upper() case-folding scenario with a Windows-only condition. Keep the
db_take_matching_self_writes expectation for the normalized path, while avoiding
the uppercase comparison on POSIX.
Source: Pipeline failures
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '=== Changed files ==='
git diff --name-status "$(git merge-base HEAD origin/main)" HEAD 2>/dev/null || git diff --name-status HEAD~1 HEAD
printf '%s\n' '=== Python outlines ==='
fd -a 'self_writes\.py|self_write\.py|watcher\.py|test_self_write\.py|main\.py|conftest\.py' . \
| while IFS= read -r file; do
printf '\n--- %s ---\n' "$file"
ast-grep outline "$file" --items all 2>/dev/null || true
done
printf '%s\n' '=== Relevant source ==='
fd -a 'self_writes\.py|self_write\.py|watcher\.py|test_self_write\.py|main\.py|conftest\.py' . \
| while IFS= read -r file; do
printf '\n--- %s ---\n' "$file"
cat -n "$file"
doneLength of output: 68973
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '=== self_write_util_replace definitions and calls ==='
ast-grep run \
--pattern 'self_write_util_replace($$$ARGS)' \
--lang python \
.
printf '%s\n' '=== Imports of self_write ==='
rg -n -C 3 --type py \
'^\s*(from\s+app\.utils\.self_write\s+import|import\s+app\.utils\.self_write\b)' \
.
printf '%s\n' '=== Candidate watched-file mutation operations ==='
rg -n -C 4 --type py \
'(piexif\.insert\s*\(|exiftool|Image\.save\s*\(|\.save\s*\(|open\s*\(.*["'\''](?:wb|ab|\+b)["'\'']|write_bytes\s*\()' \
backend sync-microservice 2>/dev/null || true
printf '%s\n' '=== Watcher and self-write test coverage ==='
fd -a -e py . backend sync-microservice \
| xargs -r rg -n -C 3 \
'(watcher_util_handle_file_changes\s*\(|watcher_util_drop_self_writes\s*\(|watcher_util_is_own_temp_file\s*\(|SELF_WRITE_TEMP_PREFIX)' \
|| trueLength of output: 37623
@rohan-pandeyy, I found one blocking issue.
🔴 backend/app/utils/self_write.py — Line 32
self_write_util_replace() has no production caller. The only imports and calls are in backend/tests/test_self_write.py.
As a result, PictoPy’s existing writes do not create ledger entries and do not use the atomic replacement path. The watcher therefore cannot identify those writes as self-writes. The PR does not yet prevent production file writes from triggering folder synchronization.
Call self_write_util_replace() from each production path that rewrites a file inside a watched folder. Add an integration test that performs that production write path and verifies that watcher_util_handle_file_changes() does not call the folder-sync API.
The prior path-normalization test finding is fixed. The test now verifies absolute-path normalization without assuming case folding on POSIX.
You are interacting with an AI system.
normcase folds case on Windows and is the identity on Linux, where paths really are case-sensitive, so the test asserted Windows behaviour as if it were universal. Absolute-vs-relative is the part that holds everywhere.
The table stands alone, but every other _connect() in this package sets the pragma and there is no reason for this one to differ.
|
|
1 similar comment
|
|
|
@coderabbitai review |
|
1 participant
The sync microservice treats any change under a watched folder as a user edit. That's fine today, because nothing in PictoPy writes to a photo — but the moment something does, the write comes straight back as a
sync-foldercall and re-runs the whole pipeline on the folder it lives in.This adds the mechanism to tell our own writes apart, so that work can land safely.
How it works
self_write_util_replace()writes to a sibling temp file, records that file's size and mtime, then renames it into place. A rename carries both across unchanged, so the ledger row is committed before the new bytes are visible at the watched path — there's no window where our own write can read as a user edit.The watcher claims matching entries and drops those changes before any folder is marked affected.
Notes
normcase(abspath(path))and both files say so.Verification
15 new tests on the backend side, including that the ledger row survives a rename that fails midway — that's the ordering guarantee, so it's worth pinning down.
The watcher half was checked end to end against a real database: our own write plus its temp file produced no sync call; a genuine edit produced one; the two mixed in a single batch still let the real edit through; and a repeat event for a claimed write correctly fell back to syncing.
Worth flagging for review: CI doesn't run tests for
sync-microservice(there's no suite there), so that side is covered by the manual check above rather than automated tests. Happy to add a suite and a CI job separately if that's wanted.Full backend suite passes (1103),
pre-commitclean, both services import cleanly.Summary by CodeRabbit
Bug Fixes
Reliability