fix(watcher): harden incremental auth sync and mirrored auth dir handling#1920
fix(watcher): harden incremental auth sync and mirrored auth dir handling#1920shenshuoyaoyouguang wants to merge 2 commits intorouter-for-me:devfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the watcher component's authentication synchronization process by transitioning from full snapshot rebuilds to incremental updates for individual auth file changes. It also strengthens the handling of mirrored authentication directories, ensuring the system consistently uses the correct auth paths. These changes aim to reduce unnecessary processing, enhance stability, and provide a more granular and efficient way to manage authentication state. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the watcher's authentication handling. The move to incremental auth synchronization for file events is a great enhancement that should reduce unnecessary full rescans and improve performance. The hardening of mirrored auth directory handling by using effectiveAuthDir and the more robust pathBelongsToDir check makes the watcher more reliable. The added tests provide good coverage for these new behaviors. I have one suggestion regarding case normalization for auth IDs on Windows to prevent potential bugs on case-insensitive filesystems.
| id := fullPath | ||
| if strings.TrimSpace(ctx.AuthDir) != "" { | ||
| if rel, errRel := filepath.Rel(ctx.AuthDir, fullPath); errRel == nil && rel != "" { | ||
| id = rel | ||
| } | ||
| // On Windows, normalize ID casing to avoid duplicate auth entries caused by case-insensitive paths. | ||
| if runtime.GOOS == "windows" { | ||
| id = strings.ToLower(id) | ||
| } | ||
| } |
There was a problem hiding this comment.
The case normalization for auth IDs on Windows has been removed. This could lead to duplicate auth entries on case-insensitive file systems like Windows, where file paths might be reported with different casing. To ensure consistency with sdk/auth/filestore.go and prevent duplicates, it's important to normalize the ID to lowercase on Windows.
This change will also require re-adding the runtime import to this file.
id := fullPath
if authDir := strings.TrimSpace(ctx.AuthDir); authDir != "" {
if rel, errRel := filepath.Rel(authDir, fullPath); errRel == nil && rel != "" {
id = rel
}
}
// On Windows, normalize ID casing to avoid duplicate auth entries caused by case-insensitive paths.
if runtime.GOOS == "windows" {
id = strings.ToLower(id)
}There was a problem hiding this comment.
Fixed in 803a275. I restored Windows auth ID case normalization in internal/watcher/synthesizer/file.go so synthesized file-backed auth IDs stay aligned with sdk/auth.NormalizeFileAuthID and do not duplicate on case-insensitive filesystems.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the watcher's authentication handling. The switch to incremental auth synchronization for file events is a great enhancement that will reduce unnecessary full rescans and improve performance. The logic for this appears sound, and I've only suggested a minor optimization to reduce object allocations. The hardening of mirrored auth directory handling through the effectiveAuthDir and pathBelongsToDir functions is also a solid improvement, making path resolution more robust. The addition of comprehensive tests for both incremental sync and mirrored directory behavior is excellent and ensures these new features are well-covered. Overall, this is a high-quality change that delivers on its goals.
| if !ok { | ||
| w.currentAuths[id] = newAuth.Clone() | ||
| updates = append(updates, AuthUpdate{Action: AuthUpdateActionAdd, ID: id, Auth: newAuth.Clone()}) | ||
| continue | ||
| } | ||
| if !authEqual(existing, newAuth) { | ||
| w.currentAuths[id] = newAuth.Clone() | ||
| updates = append(updates, AuthUpdate{Action: AuthUpdateActionModify, ID: id, Auth: newAuth.Clone()}) | ||
| } |
There was a problem hiding this comment.
There's an opportunity to optimize object allocations here. The newAuth object is freshly synthesized and not shared with other parts of the system at this point. Therefore, it can be stored directly in w.currentAuths without cloning. A clone is only necessary for the AuthUpdate struct that is sent over the channel to consumers, to prevent concurrent modification. This change avoids one Clone() call per auth update, improving efficiency.
| if !ok { | |
| w.currentAuths[id] = newAuth.Clone() | |
| updates = append(updates, AuthUpdate{Action: AuthUpdateActionAdd, ID: id, Auth: newAuth.Clone()}) | |
| continue | |
| } | |
| if !authEqual(existing, newAuth) { | |
| w.currentAuths[id] = newAuth.Clone() | |
| updates = append(updates, AuthUpdate{Action: AuthUpdateActionModify, ID: id, Auth: newAuth.Clone()}) | |
| } | |
| if !ok { | |
| w.currentAuths[id] = newAuth | |
| updates = append(updates, AuthUpdate{Action: AuthUpdateActionAdd, ID: id, Auth: newAuth.Clone()}) | |
| continue | |
| } | |
| if !authEqual(existing, newAuth) { | |
| w.currentAuths[id] = newAuth | |
| updates = append(updates, AuthUpdate{Action: AuthUpdateActionModify, ID: id, Auth: newAuth.Clone()}) | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the watcher's authentication handling by introducing an incremental synchronization mechanism, which avoids costly full state rebuilds on file events. The changes also harden the handling of mirrored authentication directories, making the system more robust. The new pathBelongsToDir function provides a more reliable way to check file paths. The added tests provide good coverage for the new functionality. The overall code quality is high, and the refactoring improves modularity and maintainability. I have one minor suggestion regarding code style in a test file.
| t.Fatalf("expected mirroredAuthDir %s, got %s", tmp, w.mirroredAuthDir) | ||
| } | ||
| if w.authDir != tmp { | ||
| t.Fatalf("expected runtime authDir to switch to mirrored path %s, got %s", tmp, w.authDir) |
There was a problem hiding this comment.
This line has inconsistent indentation. While the current indentation (8 spaces) might align visually, it uses spaces instead of tabs, which is inconsistent with the surrounding code and the standard Go formatting (gofmt). Using tabs would ensure consistent style across the project.
| t.Fatalf("expected runtime authDir to switch to mirrored path %s, got %s", tmp, w.authDir) | |
| t.Fatalf("expected runtime authDir to switch to mirrored path %s, got %s", tmp, w.authDir) |
|
Review Navigation: This PR is now suitable for priority review. Scope focuses on watcher incremental sync and mirrored auth dir handling. CI has passed, automated review only has minor optimization suggestions remaining, no known high-risk blockers. |
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary
This change moves auth-file handling to the incremental path, but I found a blocking regression in remove handling, and the branch is currently not mergeable into dev.
Key findings
- Blocking: handleEvent now filters auth events through pathBelongsToDir(...). For deleted files, normalizeAbsolutePath resolves the existing auth dir through symlinks but cannot resolve the removed file path the same way, so the comparison can fail on macOS (/var/... vs /private/var/...). That causes remove/rename events to be dropped before removeClient runs, leaving stale auth hashes/runtime auth state behind.
- Evidence: go test ./internal/watcher/... fails in TestHandleEventRemovesAuthFile and TestHandleEventRemoveKnownFileDeletes.
- Blocking: this PR is currently in a conflicting state against dev. I reproduced the merge locally and hit unresolved conflicts in:
- internal/watcher/clients.go
- internal/watcher/dispatcher.go
- internal/watcher/watcher_test.go
Test plan
- go build -o /tmp/test-output-pr1920 ./cmd/server && rm /tmp/test-output-pr1920
- go test ./internal/watcher/... ❌
Follow-up
- Please fix the delete-path ownership check so removed auth files are still recognized correctly, resolve the merge conflicts against dev, and rerun gofmt on the touched watcher files.
2 participants
Summary
Business value
Tests