combined branch for Improve performance, stability and memory use for large databases#280
Draft
Abhinegi2 wants to merge 16 commits into
Draft
combined branch for Improve performance, stability and memory use for large databases#280Abhinegi2 wants to merge 16 commits into
Abhinegi2 wants to merge 16 commits into
Conversation
BasicAuthStrategy called CouchDB _session for every request — an extra round trip plus a password hash verification inside CouchDB, amplified by replication bursts of hundreds of requests. Successful logins are now cached for 60s, keyed by a SHA-256 digest of the credentials (plaintext is never stored). Failed logins are never cached, and a wrong password never matches a cached entry since the digest differs. A max-entries cap bounds memory. Part of #261 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
clearLocal fetched all _local docs at once (one checkpoint doc exists per client replication, so this can be large) and fired every DELETE in parallel via Promise.all - momentarily flooding CouchDB on big deployments, and rejecting wholesale on the first failure while the remaining deletes kept running detached. Now: - _local_docs is fetched in pages of 500 (deleted docs disappear from the index, so re-fetching pages through the remainder) - deletes run with bounded concurrency (10 in flight) - failures are logged, all docs are still attempted, and a summary error is thrown at the end (endpoint still reports failure) Part of #261 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
No axios timeout was configured anywhere, so a hung CouchDB connection (e.g. half-open socket) would hang client requests - and the internal changes feed - indefinitely. There was also no explicit http agent, so request bursts could open an unbounded number of connections. The shared CouchDB HTTP client is now configured with: - a 60s request timeout (DATABASE_TIMEOUT_MS), safely above the 50s longpoll window of the internal changes feed - keep-alive agents capped at 50 sockets (DATABASE_MAX_SOCKETS) Both are documented in README and .env.template. Part of #261 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds the compression middleware so that large JSON responses (e.g. _bulk_get / _all_docs payloads during initial sync) are gzipped for clients on slow links. Responses that already carry a Content-Encoding (proxied CouchDB responses) and non-compressible content types (attachments) are skipped by the middleware's default filter, as is anything below the 1kb threshold. Registered in RestrictedEndpointsModule so the behavior is covered by the e2e suite. Part of #261 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- filterBulkDocsRequest: index existing docs in a Map instead of
scanning all response rows per requested doc (was O(n²) — ~10⁶
comparisons for a 1000-doc _bulk_docs push)
- _changes: drop doc bodies per fetch iteration when include_docs is
not requested, instead of accumulating all doc bodies in memory and
copying every result object again in a final mapping pass
- filterAllDocsResponse: pass through error rows for unknown keys
({key, error: "not_found"} rows have no id) instead of crashing with
a TypeError that turned the whole request into a 500
Part of #261
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The shared longpoll _changes feed (DocumentChangesService) requested include_docs=true, so every document written to the database was transferred to and parsed by the backend continuously — although its only consumers just compare document IDs: - UserIdentityService only invalidates cache entries by id - RulesService only needs the body of the single Config:Permissions doc The feed now emits a narrowed DocumentChangeEvent (id/seq/deleted) so the compiler enforces that no consumer relies on doc bodies, and RulesService fetches the permission document on demand when its ID appears in the feed (serialized via concatMap; fetch failures keep the previous in-memory permissions and are logged). This removes constant O(write volume × doc size) overhead on systems with large, active databases. Part of #261 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Building a DocumentAbility runs on every request: all rules are deep-cloned via JSON.parse(JSON.stringify(...)) for user variable injection and then compiled by CASL. The result only changes when the permission config or the user's identity changes. Abilities are now cached per user identity (id, name, roles, projects - everything that influences computed rules), guarded by: - a new RulesService.configVersion counter that increments whenever the in-memory permission config is swapped (initial load, bootstrap mode, live config updates) - a 5-minute TTL aligned with the user identity cache - a max-entries cap as memory bound for very many distinct users Part of #261 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Implements #109 for the read-heavy replication endpoints: _all_docs (GET/POST), _bulk_get, _find and _changes. Previously every response was fully buffered by axios, JSON.parsed into an object graph, filtered into a copy and re-serialized as one giant string - a peak memory cost of roughly 4-5x the raw payload per request, multiplied by concurrent syncing clients. An unbounded GET /db/_all_docs?include_docs=true could pull the whole database into heap. Now: - CouchdbService gains getStream/postStream (axios responseType stream, Accept-Encoding identity on the internal hop, errors mapped to the same HttpExceptions as buffered methods) - JsonArrayFilterTransform incrementally re-serializes a JSON object from stream-json tokens, filtering/transforming the items of one top-level array while passing envelope fields through - without holding the document or the array in memory - _all_docs/_bulk_get/_find pipe CouchDB responses through this filter directly into the client response with backpressure (stream.pipeline) - _changes keeps its iterative fetch loop and time budget but writes each batch's permitted results to the client immediately instead of accumulating them - per-item permission predicates are shared between the buffered service methods and the streaming endpoints Error semantics: failures before the first byte yield regular error responses; failures mid-stream abort the connection so clients see a truncated response (PouchDB retries) instead of valid-looking partial JSON. Truncated upstream bodies are detected by the transform and never produce complete-looking output. Behavior change: the streamed POST endpoints now return 200 (matching CouchDB) instead of NestJS's default 201. Closes #109 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # src/restricted-endpoints/replication/bulk-document/bulk-document.service.ts # src/restricted-endpoints/replication/changes/changes.controller.ts
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR converts bulk-document and changes replication endpoints from Observable-based full-response buffering to Node stream pipelines using a new ChangesStreaming Replication, Caching, and CouchDB Infrastructure
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Abhinegi2
changed the title
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
Abhinegi2
marked this pull request as ready for review
June 29, 2026 05:28
Abhinegi2
changed the title
Member
Author
Manual UI testing steps (dev site)A. Initial sync / load — streaming (#270, #263, #267)
B. Permission filtering — must not regress
C. Write path —
|
2. There is mismatch in the participant list shown under recurring Attendance and Record Attendance- check for driving class. under record attendance, attendance is being captured for 36 participants
3. In import, in step 4- review and edit data, the data takes some time to load
|
Member
Author
|
# Conflicts: # package-lock.json # package.json # src/restricted-endpoints/replication/bulk-document/bulk-doc-endpoints.controller.ts
Member
Author
|
also investigated the import loading issue and found that it wasn't actually a bug. the csv I was importing contained the Address field, and while mapping the columns, I didn't check the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/restricted-endpoints/replication/changes/changes.controller.ts (1)
67-75: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the
_changesdoc-body contract comment.Line 70 now contradicts Line 120:
include_docs=truereturnsdoc, while other requests strip it. Please align the comment with the actual streaming behavior.Proposed comment fix
- * Even if `include_docs: true` is set, the stream will not return the document content. + * Document bodies are included only when `include_docs=true`; otherwise `doc` + * is stripped after permission filtering.🤖 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 `@src/restricted-endpoints/replication/changes/changes.controller.ts` around lines 67 - 75, The `_changes` doc-body contract comment in `ChangesController` is outdated and contradicts the actual behavior handled later in the stream logic. Update the comment near the changes feed description to match the current `include_docs` behavior in `changes.controller.ts`, where `include_docs=true` can return `doc` and other requests strip it, and keep the wording aligned with the streaming flow in the `getChanges`/changes feed handler.
🧹 Nitpick comments (1)
src/restricted-endpoints/replication/changes/changes.controller.spec.ts (1)
79-89: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd one test path where
write()returnsfalse.The response mock always takes the fast path, so the new
writeChunk()backpressure branch (drain/closelistener cleanup) can regress unnoticed. Consider adding a focused mock variant that returnsfalseonce and invokes the registereddraincallback.🤖 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 `@src/restricted-endpoints/replication/changes/changes.controller.spec.ts` around lines 79 - 89, Add a focused test case for the backpressure path in the response mock used by changes.controller.spec.ts so `writeChunk()` exercises the `drain`/`close` listener cleanup branch. Keep the existing mock for the fast path, but add a variant where `write()` in the response mock returns false once, then trigger the registered `drain` callback and assert the listeners are cleaned up via the `writeChunk` flow.
🤖 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 `@src/admin/admin.service.ts`:
- Around line 44-64: In AdminService’s `_local_docs` scan loop, the current
`ids.length === 0` break can stop pagination on a page that only contains
CouchDB-internal or already-attempted docs, so later checkpoints are missed.
Update the `firstValueFrom(this.couchdbService.get(...))` call in `AdminService`
to use `_local_docs` cursor pagination with `startkey_docid` plus `skip: 1`, and
keep advancing the cursor even when a page yields no usable IDs so the loop
continues through all documents.
In `@src/auth/guards/basic-auth/basic-auth.strategy.ts`:
- Around line 19-22: The Basic Auth cache is reusing completed logins for too
long, which effectively turns successful auth into a short-lived session and
delays password/role revocations. Update the logic in BasicAuthStrategy around
LOGIN_CACHE_TTL_MS, LOGIN_CACHE_MAX_ENTRIES, and validate() so it does not cache
authenticated results for 60 seconds; instead, cache only in-flight login
attempts or remove the completed-login reuse entirely. Keep
couchdbService.login() and the roles passed to PermissionService.getAbilityFor()
in sync with the latest CouchDB state so authorization changes take effect
immediately.
In `@src/couchdb/couchdb.module.ts`:
- Around line 33-35: The timeout parsing in the CouchDB module currently accepts
any numeric value from DATABASE_TIMEOUT_ENV, which can break longpoll _changes
requests used by document-changes.service. Update the timeout handling in the
module initialization so undersized values are either rejected or clamped to the
documented minimum before being passed into the CouchDB client, and keep
DEFAULT_DATABASE_TIMEOUT_MS as the fallback when the config is missing or
invalid.
In `@src/permissions/permission/permission.service.ts`:
- Around line 93-102: The ability cache key in abilityCacheKey() only covers a
subset of user fields, so cached CASL abilities can be reused across users whose
other rule-injectable properties differ. Update the key derivation in
PermissionService to include every user field that RulesService can interpolate
into `${user...}` conditions, not just id, name, roles, and projects, and keep
the anonymous fallback unchanged so each distinct user state maps to a unique
cache entry.
In `@src/permissions/rules/rules.service.ts`:
- Around line 188-197: The permission refresh flow in RulesService currently
swallows a 404 from couchdbService.get and leaves stale in-memory rules active.
Update the observable chain around the Permission.DOC_ID fetch in RulesService
to inspect the lightweight event’s deleted flag before calling get, and when a
Config:Permissions doc is deleted switch the in-memory state to
bootstrap/fail-closed permissions instead of returning EMPTY. Also invalidate
any permission caches in the same deletion path so the previous rules are not
retained.
In
`@src/restricted-endpoints/replication/bulk-document/bulk-doc-endpoints.controller.ts`:
- Around line 169-199: The restricted `_all_docs` handlers in `allDocs` and
`allDocsGet` currently allow requests without `include_docs=true`, which lets
`streamAllDocs` leak metadata when `BulkDocumentService.allDocsRowFilter()` sees
rows without `doc`. Add a guard in these endpoints before calling
`couchdbService.postStream`/`getStream` to require `include_docs=true` for
restricted access, or otherwise fetch documents needed for permission checks;
use the `allDocs`, `allDocsGet`, and `streamAllDocs` flow as the entry point for
the fix.
---
Outside diff comments:
In `@src/restricted-endpoints/replication/changes/changes.controller.ts`:
- Around line 67-75: The `_changes` doc-body contract comment in
`ChangesController` is outdated and contradicts the actual behavior handled
later in the stream logic. Update the comment near the changes feed description
to match the current `include_docs` behavior in `changes.controller.ts`, where
`include_docs=true` can return `doc` and other requests strip it, and keep the
wording aligned with the streaming flow in the `getChanges`/changes feed
handler.
---
Nitpick comments:
In `@src/restricted-endpoints/replication/changes/changes.controller.spec.ts`:
- Around line 79-89: Add a focused test case for the backpressure path in the
response mock used by changes.controller.spec.ts so `writeChunk()` exercises the
`drain`/`close` listener cleanup branch. Keep the existing mock for the fast
path, but add a variant where `write()` in the response mock returns false once,
then trigger the registered `drain` callback and assert the listeners are
cleaned up via the `writeChunk` flow.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa88144b-9290-4452-8f97-dcfda31c47f7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
.env.templateREADME.mdpackage.jsonsrc/admin/admin.service.spec.tssrc/admin/admin.service.tssrc/auth/guards/basic-auth/basic-auth.strategy.spec.tssrc/auth/guards/basic-auth/basic-auth.strategy.tssrc/common/json-array-filter.spec.tssrc/common/json-array-filter.tssrc/couchdb/couchdb.module.spec.tssrc/couchdb/couchdb.module.tssrc/couchdb/couchdb.service.tssrc/couchdb/document-changes.service.spec.tssrc/couchdb/document-changes.service.tssrc/permissions/permission/permission.service.spec.tssrc/permissions/permission/permission.service.tssrc/permissions/rules/rules.service.spec.tssrc/permissions/rules/rules.service.tssrc/permissions/user-identity/user-identity.service.spec.tssrc/restricted-endpoints/replication/bulk-document/bulk-doc-endpoints.controller.spec.tssrc/restricted-endpoints/replication/bulk-document/bulk-doc-endpoints.controller.tssrc/restricted-endpoints/replication/bulk-document/bulk-document.service.spec.tssrc/restricted-endpoints/replication/bulk-document/bulk-document.service.tssrc/restricted-endpoints/replication/changes/changes.controller.spec.tssrc/restricted-endpoints/replication/changes/changes.controller.tssrc/restricted-endpoints/restricted-endpoints.module.tstest/compression.e2e-spec.tstest/replication.e2e-spec.tstest/session-documents.e2e-spec.tstest/utils/mock-couchdb.ts
- extract shared TtlCache (LRU + expiry) for the ability and basic-auth
login caches instead of duplicating TTL-Map logic; key ability cache on
the whole user to avoid collisions on injectable ${user.*} fields
- enforce a MIN_DATABASE_TIMEOUT_MS floor so a too-low DATABASE_TIMEOUT_MS
cannot abort the 50s longpoll changes feed
- retry the on-demand permission-document fetch on transient errors, and
fail closed to bootstrap permissions when the permission doc is deleted
- harden _changes writeChunk with an 'error' listener + listener cleanup
- paginate clearLocal by startkey_docid so undeletable docs cannot block
later pages (drops the unbounded attemptedIds set)
- remove dead filter*Response wrappers (retarget specs to the mappers) and
the unused inTargetArray field
Member
|
@Abhinegi2 , sorry for being picky 😊 But could you apply the cleanups back to the individual branches, please? Replication backend is mission-critical and clear, isolated git commits are super helpful in case we need to troubleshoot regressions or undo any changes later. |
Abhinegi2
marked this pull request as draft
July 3, 2026 06:23
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
3 participants
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.
Combines all PRs from the #261 perf stack onto one branch so the integrated behavior can be tested in CI and on staging (Tier-3 real-replication / memory test). The actual changes merge via the individual PRs #263–#270.
Config:Permissionsfinishes loading at startup (self-corrects); likely pre-existing.Summary by CodeRabbit
New Features
Bug Fixes