fix(brain-repo): mirror propagates source deletions, safely - #127
Open
mt-alarcon wants to merge 1 commit into
Open
fix(brain-repo): mirror propagates source deletions, safely#127mt-alarcon wants to merge 1 commit into
mt-alarcon wants to merge 1 commit into
Conversation
The workspace mirror used `shutil.copytree(dirs_exist_ok=True)`, which only ever adds or overwrites. A file deleted from a watched path stayed in the brain repo forever, so the backup drifted further from the workspace with every sync and a "restore" would resurrect files that were removed on purpose. This adds a reconciliation pass that removes destination files whose source counterpart is gone. Everything else here exists to keep that pass from becoming a data-loss engine, since it is the only code in the project whose job is to delete backup content. The invariant: absence of evidence is never evidence of deletion. Ask the source directly, and keep the file on any uncertainty. - Reconciliation runs OUTSIDE the copytree try block. copytree accumulates per-file errors and raises `shutil.Error` once at the end, so a single unreadable file used to skip deletion propagation for the ENTIRE watch path while logging only a warning. - Deletion is authorised by `_source_may_still_exist`, not by the "was it copied this round" set. That set is a proxy with a silent hole: a subdirectory that fails to scan leaves its whole subtree unvisited, and reconciling on the proxy would erase that subtree from the backup. - `lstat`, not `exists()`: `exists()` follows symlinks, so a broken symlink (a file that plainly exists) reports False. Only FileNotFoundError counts as proof of deletion; any other OSError means "cannot tell". - No `Path.is_file()` probe on a possibly unreadable path — pathlib's `_ignore_error` does not cover EACCES, so it raised straight out of the mirror. - A cancel mid-walk leaves the copied set partial; reconciliation is skipped in that case. - Mass-deletion circuit breaker: a pass that would erase at least half of a watch path's backup, and at least 50 files, is refused and logged. At that scale the likeliest cause is the source being unavailable (an unmounted volume usually looks like an empty directory) rather than a real deletion. - Credential caches written by integration skills are never copied, and any stale copy already in the backup is purged. Known limitation, accepted deliberately: content already mirrored under a watch path whose source directory disappears entirely is left untouched and therefore stays in the backup indefinitely. Reconciling it under the breaker was tried and rejected — a small watch path falls under any sane threshold, so a momentarily unavailable mount would take its backup with it. There is no local signal separating "unused" from "unavailable right now", and for a backup the ambiguous answer has to be "keep". Tests: 18 covering each guard. The permission-based ones carry a positive control and skip loudly rather than pass vacuously when run as root. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reviewer's GuideAdds a post-copy reconciliation pass to the brain repo workspace mirror so that source deletions are propagated safely to the backup, with multiple guards against unintended data loss and special handling for credential caches. Sequence diagram for workspace mirror with post-copy deletion reconciliationsequenceDiagram
participant MirrorJobRunner as _mirror_workspace
participant CopyTree as shutil_copytree
participant Reconcile as _reconcile_deletions
participant SourceCheck as _source_may_still_exist
MirrorJobRunner->>CopyTree: copytree(src, dst, dirs_exist_ok=True, ignore=_ignore)
alt walk_completed
MirrorJobRunner->>Reconcile: _reconcile_deletions(workspace, brain_dir, watch, copied_this_watch)
loop each_backup_file
Reconcile->>SourceCheck: _source_may_still_exist(src_file)
alt source_may_still_exist and not _is_credential_cache
Reconcile-->>MirrorJobRunner: keep_backup_file
else source_absent or is_credential_cache
Reconcile-->>MirrorJobRunner: mark_file_doomed
end
end
Reconcile-->>MirrorJobRunner: deleted_count
MirrorJobRunner->>MirrorJobRunner: log reconciled deletions
else walk_aborted_or_cancel_flag
MirrorJobRunner-->>MirrorJobRunner: skip_reconciliation
end
Flow diagram for deletion reconciliation and mass-deletion circuit breakerflowchart TD
A[start_reconcile_deletions] --> B[scan_dst_root_with_rglob]
B --> C[for_each_backup_file]
C --> D{rel_in_copied_set}
D -->|yes| C
D -->|no| E[compute_src_file_from_workspace]
E --> F[_source_may_still_exist]
F -->|true and not _is_credential_cache| C
F -->|false or is_credential_cache| G[_is_policy_excluded]
G -->|true| C
G -->|false| H[add_to_doomed_list]
H --> C
C --> I{all_files_scanned}
I -->|yes| J["_is_mass_deletion(len_doomed, backup_files)"]
J -->|true| K[log_refuse_and_return_0]
J -->|false| L[unlink_doomed_files_and_prune_empty_dirs]
L --> M[return_deleted_count]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The reconciliation and guarding logic (_is_policy_excluded, _reconcile_deletions, _source_may_still_exist, mass‑deletion breaker, credential cache handling) has grown quite large inside job_runner.py; consider extracting it into a dedicated module or class to reduce cognitive load and make the mirroring pipeline easier to follow.
- In _mirror_workspace, you traverse dst twice via dst.rglob('*') (once to count files and once inside _reconcile_deletions); if this starts to hurt on large trees, consider aggregating file counting and reconciliation in a single walk to avoid repeated filesystem traversal.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The reconciliation and guarding logic (_is_policy_excluded, _reconcile_deletions, _source_may_still_exist, mass‑deletion breaker, credential cache handling) has grown quite large inside job_runner.py; consider extracting it into a dedicated module or class to reduce cognitive load and make the mirroring pipeline easier to follow.
- In _mirror_workspace, you traverse dst twice via dst.rglob('*') (once to count files and once inside _reconcile_deletions); if this starts to hurt on large trees, consider aggregating file counting and reconciliation in a single walk to avoid repeated filesystem traversal.
## Individual Comments
### Comment 1
<location path="dashboard/backend/brain_repo/tests/test_job_runner_mirror_reconcile.py" line_range="139-148" />
<code_context>
+
+ self._mirror()
+
+ self.assertFalse(
+ (self.workspace / watch / ".gitignore").exists() and False,
+ "sanity: the source has no .gitignore",
+ )
+ self.assertTrue(
</code_context>
<issue_to_address>
**issue (testing):** Sanity assertion for source .gitignore is ineffective and will always pass
In `test_gitignore_never_copied_and_never_deletes_stale_copy`, this assertion:
```python
self.assertFalse(
(self.workspace / watch / ".gitignore").exists() and False,
"sanity: the source has no .gitignore",
)
```
will always pass, because `X and False` is always `False`. It never actually checks whether `.gitignore` exists, so it can mask accidental fixture changes.
Please change it to a real absence check, e.g.:
```python
self.assertFalse(
(self.workspace / watch / ".gitignore").exists(),
"sanity: the source has no .gitignore",
)
```
or remove the assertion entirely if this precondition isn’t required.
</issue_to_address>
### Comment 2
<location path="dashboard/backend/brain_repo/tests/test_job_runner_mirror_reconcile.py" line_range="166-175" />
<code_context>
+ def test_gitignore_never_copied_and_never_deletes_stale_copy(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Add a case where a source .gitignore actually exists to prove it is not copied
The current test only covers a stale `.gitignore` already in the destination and ensures reconciliation doesn’t delete it. To fully exercise the guard, add coverage for a `.gitignore` that actually exists in the source watch path: create the file in the source, run `_mirror_workspace`, and assert that it does not appear under `brain_dir`. This will validate both that source `.gitignore` files are never copied and that existing destination `.gitignore` files are preserved.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+139
to
+148
| self.assertFalse( | ||
| dst_file.exists(), | ||
| "a file deleted from the source should have been removed from the destination (original bug)", | ||
| ) | ||
|
|
||
| # ── 2. A policy-excluded file stays in the destination ───────────────── | ||
| def test_policy_excluded_file_is_never_deleted(self): | ||
| watch = _WATCH_PATHS[0] | ||
| (self.workspace / watch).mkdir() | ||
| small = self.workspace / watch / "big.bin" |
There was a problem hiding this comment.
issue (testing): Sanity assertion for source .gitignore is ineffective and will always pass
In test_gitignore_never_copied_and_never_deletes_stale_copy, this assertion:
self.assertFalse(
(self.workspace / watch / ".gitignore").exists() and False,
"sanity: the source has no .gitignore",
)will always pass, because X and False is always False. It never actually checks whether .gitignore exists, so it can mask accidental fixture changes.
Please change it to a real absence check, e.g.:
self.assertFalse(
(self.workspace / watch / ".gitignore").exists(),
"sanity: the source has no .gitignore",
)or remove the assertion entirely if this precondition isn’t required.
Comment on lines
+166
to
+175
| def test_gitignore_never_copied_and_never_deletes_stale_copy(self): | ||
| watch = _WATCH_PATHS[0] | ||
| (self.workspace / watch).mkdir() | ||
| (self.workspace / watch / "keep.md").write_text("k") | ||
|
|
||
| # Simulate a residue from an old sync (before the .gitignore rule | ||
| # existed): a .gitignore already present in the destination. | ||
| dst_dir = self.brain_dir / watch | ||
| dst_dir.mkdir(parents=True) | ||
| stale_gitignore = dst_dir / ".gitignore" |
There was a problem hiding this comment.
suggestion (testing): Add a case where a source .gitignore actually exists to prove it is not copied
The current test only covers a stale .gitignore already in the destination and ensures reconciliation doesn’t delete it. To fully exercise the guard, add coverage for a .gitignore that actually exists in the source watch path: create the file in the source, run _mirror_workspace, and assert that it does not appear under brain_dir. This will validate both that source .gitignore files are never copied and that existing destination .gitignore files are preserved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The workspace mirror uses
shutil.copytree(dirs_exist_ok=True), which only ever adds or overwrites. A file deleted from a watched path stays in the brain repo forever, so the backup drifts further from the workspace with every sync — and a restore resurrects files that were removed on purpose.What this does
Adds a reconciliation pass that removes destination files whose source counterpart is gone. Everything else in the diff exists to keep that pass from becoming a data-loss engine, since it is the only code here whose job is to delete backup content.
The invariant: absence of evidence is never evidence of deletion. Ask the source directly, and keep the file on any uncertainty.
copytreetry blockcopytreeaccumulates per-file errors and raisesshutil.Erroronce at the end, so one unreadable file skipped deletion propagation for the entire watch path, logging only a warning_source_may_still_exist, not by the "copied this round" setlstat, notexists()exists()follows symlinks, so a broken symlink — a file that plainly exists — reports False. OnlyFileNotFoundErrorproves deletionPath.is_file()probe on a possibly unreadable path_ignore_errordoes not coverEACCES, so it raised straight out of the mirrorKnown limitation (deliberate)
Content already mirrored under a watch path whose source directory disappears entirely is left untouched, so it stays in the backup indefinitely.
Reconciling it under the breaker was tried and rejected: the breaker needs a file count to judge, and a small watch path falls under any sane threshold — so a momentarily unavailable mount would silently take its backup with it. There is no local signal separating "this path is unused" from "this path is unavailable right now", and for a backup the ambiguous answer has to be "keep". Purging that residue should stay a deliberate operator action.
Tests
18 tests, one per guard. The permission-based ones carry a positive control and call
skipTestloudly rather than passing vacuously when run as root.Each guard was verified to be load-bearing by neutralising it and confirming the corresponding test fails — not merely by observing a green suite.
Summary by Sourcery
Propagate deletions from workspace watch paths to the brain repo mirror while protecting against unintended data loss and excluding credential caches.
New Features:
Bug Fixes:
Enhancements:
Tests: