Conversation
… any depth + rate-limit Drive API by query
Two root causes made a large tree (e.g. node_modules) never finish syncing, so
every restart re-attempted the whole unsynced backlog (looking like "everything
re-uploads"; already-synced files were in fact correctly skipped):
1. Ignore matching only skipped .git/.gcrypt at the sync ROOT, and node_modules
wasn't ignored at all — so 17k+ dependency/VCS files flooded the queue.
- Built-in ignores are now matched on ANY path component: .git, .gcrypt,
.svn, .hg, node_modules are skipped at any depth and on every pair
(EffectiveIgnorePatterns returns a pair's custom patterns *instead of* the
defaults, so this had to live in the non-overridable built-in matcher).
- DefaultIgnorePatterns also gains the common build/cache trees (vendor,
dist, build, target, bin, obj, __pycache__, .venv, .idea, .vscode, ...)
for pairs without custom patterns.
2. The rate limiter counted operations, not Drive API queries — but each upload
issues several queries (dedup search + encrypted folder-chain creation +
upload), so the real query rate was several times the cap and blew Google's
per-minute per-user quota (logs full of "Quota exceeded"/"rate limit").
- Enforce the cap at the HTTP transport (rateLimitedRoundTripper), so EVERY
request across all pairs, upload chunks and token refreshes counts —
process-global since the quota is per-user. Default 100 q/s (~6,000/min,
well under the ~12,000/min limit, with headroom for retries).
Note: already-synced node_modules/.git files become "ignored" and are removed
from Drive (trashed, recoverable) on the next scan, as intended.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR makes two independent improvements: it adds a process-global token-bucket rate limiter that gates all Drive HTTP requests (including OAuth refresh) through a custom ChangesDrive API Rate Limiting
Ignore Pattern Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (4)
internal/drive/client.go (1)
47-52: 💤 Low valueConsider adding a cleanup mechanism for the ticker.
The ticker created here runs indefinitely without a stop mechanism. While this is acceptable for the singleton
apiLimiter, it prevents clean shutdown and could complicate testing scenarios where the rate limiter might need to be recreated or stopped.♻️ Optional: Add Stop method for graceful cleanup
type apiRateLimiter struct{ ticker *time.Ticker } func newAPIRateLimiter(qps int) *apiRateLimiter { if qps < 1 { qps = 1 } return &apiRateLimiter{ticker: time.NewTicker(time.Second / time.Duration(qps))} } +// Stop stops the rate limiter's ticker. Call this only during shutdown or tests. +func (l *apiRateLimiter) Stop() { + l.ticker.Stop() +} + // wait blocks until the next token is available or ctx is cancelled. func (l *apiRateLimiter) wait(ctx context.Context) error {🤖 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 `@internal/drive/client.go` around lines 47 - 52, The ticker in the apiRateLimiter struct created by newAPIRateLimiter function runs indefinitely without a stop mechanism, which prevents graceful shutdown and complicates testing. Add a Stop method to the apiRateLimiter type that calls the Stop() method on the ticker field to allow callers to cleanly shut down the rate limiter and reclaim resources when needed.internal/sync/ignore.go (2)
96-101: 💤 Low valueConsider clarifying that files matching these names are also ignored.
The comment states "Directory names that are never worth syncing," but the implementation ignores paths where ANY component matches these names, including files (e.g., a file literally named
node_modulesat the root). This is likely acceptable since such filenames are unusual, but the comment could be more precise about this behavior to prevent future confusion.🤖 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 `@internal/sync/ignore.go` around lines 96 - 101, Update the comment block starting with "Directory names that are never worth syncing" to clarify that the implementation ignores not just directory names but any path component (including files) that match these names, since the behavior applies to ANY component of the file path at any depth. Modify the comment to be more precise about this behavior to prevent future confusion about whether files with these names are also ignored.
264-279: Consolidate overlapping ignore patterns betweensyncandconfigpackages.The
DefaultIgnorePatterns()function ininternal/sync/ignore.goand its counterpart ininternal/config/config.goshare 9 overlapping patterns:Thumbs.db,.DS_Store,desktop.ini,*.tmp,~$*,*.swp,*~,.~lock.*#,*.lock. Although the functions serve different purposes—sync patterns are always applied (including.gcrypt/,.git/) while config patterns are overridable defaults—consolidating the shared artifact patterns would reduce maintenance burden. Consider extracting common patterns into a shared constant or having one function compose the other.🤖 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 `@internal/sync/ignore.go` around lines 264 - 279, The DefaultIgnorePatterns() function in sync/ignore.go shares 9 overlapping patterns with a counterpart function in the config package (Thumbs.db, .DS_Store, desktop.ini, *.tmp, ~$*, *.swp, *~, .~lock.*#, *.lock). To fix this, extract these 9 common artifact patterns into a shared constant in a common location accessible to both packages. Then modify DefaultIgnorePatterns() to build its return value by combining this shared constant with sync-specific patterns (.gcrypt/, .gcrypt-ignore, .git/), and update the config package counterpart to use the same shared constant for its defaults. This ensures a single source of truth for artifact patterns while preserving the distinct purposes of each function.internal/config/config.go (1)
144-159: 🏗️ Heavy liftFunction name collision with
sync.DefaultIgnorePatternscreates maintainability risk.Both
internal/config/config.goandinternal/sync/ignore.goexport a function namedDefaultIgnorePatterns()with overlapping pattern content. This creates confusion about:
- Which version should be called in different contexts
- Which version to update when adding new patterns
- Why patterns are duplicated across both
The comment on line 145 acknowledges the separate built-in ignores but doesn't explain why two functions with the same name and overlapping patterns should coexist. Consider renaming one (e.g.,
DefaultConfigIgnorePatterns) or consolidating them to prevent future maintainability issues.🤖 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 `@internal/config/config.go` around lines 144 - 159, The DefaultIgnorePatterns function in internal/config/config.go has a name collision with another DefaultIgnorePatterns function in internal/sync/ignore.go, causing confusion about which function to use and maintain. Rename the DefaultIgnorePatterns function in internal/config/config.go to a more specific name like DefaultConfigIgnorePatterns to differentiate it from the sync package version and clarify its purpose in the config context.
🤖 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.
Nitpick comments:
In `@internal/config/config.go`:
- Around line 144-159: The DefaultIgnorePatterns function in
internal/config/config.go has a name collision with another
DefaultIgnorePatterns function in internal/sync/ignore.go, causing confusion
about which function to use and maintain. Rename the DefaultIgnorePatterns
function in internal/config/config.go to a more specific name like
DefaultConfigIgnorePatterns to differentiate it from the sync package version
and clarify its purpose in the config context.
In `@internal/drive/client.go`:
- Around line 47-52: The ticker in the apiRateLimiter struct created by
newAPIRateLimiter function runs indefinitely without a stop mechanism, which
prevents graceful shutdown and complicates testing. Add a Stop method to the
apiRateLimiter type that calls the Stop() method on the ticker field to allow
callers to cleanly shut down the rate limiter and reclaim resources when needed.
In `@internal/sync/ignore.go`:
- Around line 96-101: Update the comment block starting with "Directory names
that are never worth syncing" to clarify that the implementation ignores not
just directory names but any path component (including files) that match these
names, since the behavior applies to ANY component of the file path at any
depth. Modify the comment to be more precise about this behavior to prevent
future confusion about whether files with these names are also ignored.
- Around line 264-279: The DefaultIgnorePatterns() function in sync/ignore.go
shares 9 overlapping patterns with a counterpart function in the config package
(Thumbs.db, .DS_Store, desktop.ini, *.tmp, ~$*, *.swp, *~, .~lock.*#, *.lock).
To fix this, extract these 9 common artifact patterns into a shared constant in
a common location accessible to both packages. Then modify
DefaultIgnorePatterns() to build its return value by combining this shared
constant with sync-specific patterns (.gcrypt/, .gcrypt-ignore, .git/), and
update the config package counterpart to use the same shared constant for its
defaults. This ensures a single source of truth for artifact patterns while
preserving the distinct purposes of each function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f61f92c6-85b5-4a3b-b89c-c3629deeb9a3
📒 Files selected for processing (5)
internal/config/config.gointernal/config/config_test.gointernal/drive/client.gointernal/sync/ignore.gointernal/sync/ignore_test.go
1 participant
Problem
On restart the client appeared to re-upload everything. Investigation of the live DB/logs showed already-synced files are correctly skipped (zero overlap between the 1,612 synced rows and the upload log). The real issue: a large tree (
node_modules, 18,620 files vs 1,612 synced) never finished syncing, so each restart re-attempted the unsynced backlog — which never completes because of Google Drive quota errors (logs full of Quota exceeded / rate limit exceeded).Root causes & fixes
node_modulessynced; nested.gitsynced. Built-in ignores only matched.git/.gcryptat the sync root, andnode_moduleswasn't ignored at all. SinceEffectiveIgnorePatterns()returns a pair's custom patterns instead of the defaults, the fix had to go in the non-overridable built-in matcher:.git,.gcrypt,.svn,.hg,node_modulesare skipped at any depth, on every pair.DefaultIgnorePatternsalso gains common build/cache trees (vendor, dist, build, target, bin, obj,__pycache__,.venv,.idea,.vscode, …) for pairs without custom patterns.rateLimitedRoundTripper) so every request — across all pairs, upload chunks and token refreshes — counts; process-global since the quota is per-user. Default 100 q/s (~6,000/min, well under ~12,000/min).Note: already-synced
node_modules/.gitfiles become ignored and are removed from Drive (trashed, recoverable) on the next scan, as intended.Verified locally
build,
go test ./..., golangci-lint (0), gosec (0), govulncheck (clean). Added a regression test for nestednode_modules/.git/.svn/.hgignoring.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes