Repository navigation
Make Git mirror recovery safe - #11
Merged
Merged
Conversation
Contributor
Reviewer's GuideThe PR makes failed Git mirror creation recoverable by deleting partial cache directories, keeps the standalone and Rust-embedded C implementations synchronized, and adds an integration test that verifies installation heals a mirror missing Sequence diagram for recovering an incomplete Git mirror cachesequenceDiagram
participant Flint
participant pkg_ensure_mirror
participant Git
participant Cache
Flint->>pkg_ensure_mirror: pkg_ensure_mirror(mirror, url)
pkg_ensure_mirror->>Cache: Check mirror HEAD
alt HEAD is missing
pkg_ensure_mirror->>Git: pkg_git(git clone --mirror)
alt Clone fails
Git-->>pkg_ensure_mirror: non-zero status
pkg_ensure_mirror->>Cache: pkg_remove_tree(mirror)
pkg_ensure_mirror-->>Flint: false
else Clone succeeds
Git-->>pkg_ensure_mirror: success
pkg_ensure_mirror-->>Flint: true
end
end
Flint->>pkg_ensure_mirror: Retry installation
pkg_ensure_mirror->>Git: pkg_git(git clone --mirror)
Git-->>pkg_ensure_mirror: success
Flow diagram for healing a corrupted package mirrorflowchart TD
A[flint pkg install] --> B{Mirror HEAD exists?}
B -->|Yes| C[Use cached mirror]
B -->|No| D[Attempt git clone --mirror]
D -->|Failure| E["pkg_remove_tree(mirror)"]
E --> F[Return false]
F --> G[Retry installation]
G --> D
D -->|Success| H[Install package]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/pkg.c" line_range="869" />
<code_context>
+ * Remove it so the next attempt can start cleanly rather
+ * than failing forever on a non-empty cache directory.
+ */
+ (void)pkg_remove_tree(mirror);
snprintf(error, error_size, "cannot clone '%s'", url);
- return NULL;
</code_context>
<issue_to_address>
**Symlink targets lose data**
When the mirror path is a symlink to a non-empty directory and cloning fails, `pkg_remove_tree` calls `opendir` on the mirror path, follows the symlink, and recursively deletes the target’s contents; the later `rmdir` fails on the symlink only after the files are gone. The failed clone can therefore delete data outside the cache.
Check whether the mirror path is a symlink before recursing, and remove the link itself without traversing its target.
Also at `rust/flintlang/c-src/src/pkg.c:869`.
</issue_to_address>
### Comment 2
<location path="src/pkg.c" line_range="869" />
<code_context>
+ * Remove it so the next attempt can start cleanly rather
+ * than failing forever on a non-empty cache directory.
+ */
+ (void)pkg_remove_tree(mirror);
snprintf(error, error_size, "cannot clone '%s'", url);
- return NULL;
</code_context>
<issue_to_address>
**Concurrent mirrors are deleted**
When two Flint processes initialize the same mirror concurrently and one clone fails while the other process is creating or using it, the failing process calls `pkg_remove_tree` on the shared mirror and deletes the other process’s in-progress or completed clone. The other install can then fail, and the cache must be cloned again.
Serialize operations per mirror or clone into isolated staging directories and publish only after a successful clone.
Also at `rust/flintlang/c-src/src/pkg.c:869`.
</issue_to_address>
### Comment 3
<location path="src/pkg.c" line_range="869-871" />
<code_context>
+ * Remove it so the next attempt can start cleanly rather
+ * than failing forever on a non-empty cache directory.
+ */
+ (void)pkg_remove_tree(mirror);
snprintf(error, error_size, "cannot clone '%s'", url);
- return NULL;
+ return false;
}
return true;
</code_context>
<issue_to_address>
**Mirror repair needs another run**
When an existing mirror is missing `HEAD` but still contains other files, `git clone --mirror` rejects the non-empty mirror, then `pkg_ensure_mirror` removes it and returns `false` without retrying; `pkg_resolve_git` aborts the current install, so the user must run it again to restore the cache.
Retry the clone after removing the damaged mirror, or remove the damaged mirror before starting the clone.
Also at `rust/flintlang/c-src/src/pkg.c:869-871`, `tests/pkg_test.sh:162-166`.
</issue_to_address>
### Comment 4
<location path="src/pkg.c" line_range="869" />
<code_context>
+ * Remove it so the next attempt can start cleanly rather
+ * than failing forever on a non-empty cache directory.
+ */
+ (void)pkg_remove_tree(mirror);
snprintf(error, error_size, "cannot clone '%s'", url);
- return NULL;
</code_context>
<issue_to_address>
**Cleanup failures go unnoticed**
When removing the partial mirror fails, `pkg_ensure_mirror` discards the result of `pkg_remove_tree`, leaving the non-empty mirror in place; later clone attempts fail against it again, and callers receive no indication that cleanup failed.
Check the cleanup result, report its failure, and do not retry cloning into the uncleared mirror.
Also at `rust/flintlang/c-src/src/pkg.c:869`.
</issue_to_address>
### Comment 5
<location path="tests/pkg_test.sh" line_range="165-167" />
<code_context>
+mirror=$(find "$HOME/.flint/git" -mindepth 1 -maxdepth 1 -type d | head -n 1)
+expect "git mirror cache exists" "yes" "$([ -n "$mirror" ] && echo yes || echo no)"
+rm -f "$mirror/HEAD"
+out=$("$FLINT" pkg install 2>&1) && code=0 || code=$?
+expect "install recovers a mirror missing HEAD" "0" "$code"
+expect "recovered mirror still imports" "HEY zed" "$("$FLINT" use.fl 2>&1)"
+
cat > "$WORK/gitsrc/main.fl" <<'EOF'
</code_context>
<issue_to_address>
**Recovery test skips the mirror**
When the installed module still matches the lockfile content hash, `pkg install` verifies the existing module and skips resolving the dependency, so it never checks the mirror after the test removes `HEAD`; the install and import assertions pass without exercising cache recovery.
Change the test setup so the install must resolve the Git dependency instead of reusing the already-installed module.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and if the cleanup is wrong, a failed clone can recursively remove the mirror cache, including any repository state already present in that directory. The cache is bounded and can be recreated by retrying the clone, but reverting the change would not restore data already removed.
Blocking findings: src/pkg.c:869, src/pkg.c:869, src/pkg.c:871, src/pkg.c:869, tests/pkg_test.sh:167
programmersd21
changed the title
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.
Recover incomplete mirrors in one install. Clone to staging, serialize per-mirror operations, and avoid following symlinks during cleanup. Add recovery, symlink-safety, and concurrent-install tests. GCC, Clang, macOS, Windows, and Rust checks pass.