fix: implement actual data collection for backup security manager#333
fix: implement actual data collection for backup security manager#333daggerstuff wants to merge 5 commits intostagingfrom
Conversation
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideImplements real backup data collection in BackupSecurityManager by querying all Mongoose models (optionally filtered by last backup time), introduces a helper to derive the last backup timestamp from stored backup metadata, and tightens error logging to use error.message. Sequence diagram for backup data collection using Mongoose and metadata lookupsequenceDiagram
actor Admin
participant BackupSecurityManager
participant StorageProvider as StorageProvider(s)
participant Mongoose
Admin->>BackupSecurityManager: getDataForBackup(type)
activate BackupSecurityManager
alt type is DIFFERENTIAL or INCREMENTAL
BackupSecurityManager->>BackupSecurityManager: getLastBackupTime(requireFull)
activate BackupSecurityManager
loop each storageProvider
BackupSecurityManager->>StorageProvider: listFiles(backups/)
StorageProvider-->>BackupSecurityManager: files
BackupSecurityManager->>StorageProvider: getFile(metaFile)
StorageProvider-->>BackupSecurityManager: metadataBuffer
BackupSecurityManager->>BackupSecurityManager: parse BackupMetadata
BackupSecurityManager->>BackupSecurityManager: update latestTimestamp if eligible
end
deactivate BackupSecurityManager
BackupSecurityManager->>BackupSecurityManager: build query with updatedAt >= baselineTime
else type is FULL
BackupSecurityManager->>BackupSecurityManager: query = {}
end
BackupSecurityManager->>Mongoose: modelNames()
Mongoose-->>BackupSecurityManager: [modelName]
loop each modelName
BackupSecurityManager->>Mongoose: model(modelName)
Mongoose-->>BackupSecurityManager: Model
BackupSecurityManager->>Mongoose: Model.find(query).lean().exec()
Mongoose-->>BackupSecurityManager: docs
BackupSecurityManager->>BackupSecurityManager: add docs to backupData.data[modelName]
end
BackupSecurityManager-->>Admin: Uint8Array(JSON.stringify(backupData))
deactivate BackupSecurityManager
Updated class diagram for BackupSecurityManager backup data collectionclassDiagram
class BackupSecurityManager {
- storageProviders Map<StorageLocation, StorageProvider>
- logger Logger
+ getLastBackupTime(requireFull boolean) Promise~Date|null~
+ getDataForBackup(type BackupType) Promise~Uint8Array~
}
class BackupType {
<<enumeration>>
FULL
DIFFERENTIAL
INCREMENTAL
}
class BackupStatus {
<<enumeration>>
COMPLETED
FAILED
IN_PROGRESS
}
class BackupMetadata {
+ timestamp string
+ type BackupType
+ status BackupStatus
}
class StorageProvider {
<<interface>>
+ listFiles(prefix string) Promise~string[]~
+ getFile(path string) Promise~Uint8Array~
}
class Mongoose {
+ modelNames() string[]
+ model(name string) MongooseModel
}
class MongooseModel {
+ find(query any) MongooseQuery
}
class MongooseQuery {
+ lean() MongooseQuery
+ exec() Promise~any[]~
}
BackupSecurityManager --> StorageProvider : uses
BackupSecurityManager --> BackupMetadata : reads
BackupMetadata --> BackupType
BackupMetadata --> BackupStatus
BackupSecurityManager --> Mongoose : queries
Mongoose --> MongooseModel
MongooseModel --> MongooseQuery
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe backup module now collects actual database data instead of generating dummy payloads. A new private method retrieves the latest backup timestamp from storage providers by reading metadata files. Differential backups filter documents based on the previous backup's timestamp to capture only recent changes. Changes
Sequence DiagramsequenceDiagram
participant BackupManager as BackupSecurityManager
participant StorageProvider as Storage Provider
participant MetadataFile as Metadata File
participant Database as Database/Models
participant Cursor as Document Cursor
BackupManager->>StorageManager: getDataForBackup(type)
activate BackupManager
alt if differential/incremental backup
BackupManager->>BackupManager: getLastBackupTime(requireFull=false)
activate BackupManager
loop for each storage provider
BackupManager->>StorageProvider: list(backups/)
StorageProvider-->>BackupManager: file list
loop for each *.meta.json file
BackupManager->>MetadataFile: read & parse
MetadataFile-->>BackupManager: BackupMetadata
alt status === COMPLETED
BackupManager->>BackupManager: track timestamp
end
end
end
BackupManager-->>BackupManager: return latest timestamp
deactivate BackupManager
else full backup
BackupManager->>BackupManager: baselineTime = null
end
BackupManager->>Database: import mongoose & enumerate models
loop for each model
BackupManager->>Database: Model.find(query).lean().cursor()
activate Database
Database-->>Cursor: document cursor
loop for each document
Cursor-->>BackupManager: document
BackupManager->>BackupManager: JSON.stringify & append
end
deactivate Database
end
BackupManager-->>BackupManager: return JSON payload
deactivate BackupManager
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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 1 issue, and left some high level feedback:
- In
getDataForBackup,Promise.allover all Mongoose models can still create a large in-memory spike; consider iterating models sequentially or in small batches to better control memory usage during backup. - The query that filters by
updatedAtassumes all models have anupdatedAtfield; it may be safer to either restrict to known backup-eligible models or detect whether a model supports that field before applying the filter. - In
getLastBackupTime, individual file parse errors are silently swallowed; adding at least a debug-level log with the filename would make it easier to diagnose corrupted or malformed metadata files.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `getDataForBackup`, `Promise.all` over all Mongoose models can still create a large in-memory spike; consider iterating models sequentially or in small batches to better control memory usage during backup.
- The query that filters by `updatedAt` assumes all models have an `updatedAt` field; it may be safer to either restrict to known backup-eligible models or detect whether a model supports that field before applying the filter.
- In `getLastBackupTime`, individual file parse errors are silently swallowed; adding at least a debug-level log with the filename would make it easier to diagnose corrupted or malformed metadata files.
## Individual Comments
### Comment 1
<location path="src/lib/security/backup/index.ts" line_range="870-877" />
<code_context>
+ query = { updatedAt: { $gte: baselineTime } }
+ }
+
+ const models = mongoose.modelNames()
+ const dataPromises = models.map(async (modelName) => {
+ const Model = mongoose.model(modelName)
+ const docs = await Model.find(query).lean().exec()
+ return { modelName, docs }
+ })
+
+ const results = await Promise.all(dataPromises)
+
+ for (const result of results) {
</code_context>
<issue_to_address>
**issue (performance):** Fetching all documents from all Mongoose models at once can be very expensive and memory-intensive.
For full backups this builds `{}` and runs `Model.find({}).lean().exec()` for every registered model in parallel, which can load a very large number of documents into memory at once. Consider batching per model using pagination/streaming (e.g., cursors) instead of a single `find()`, and/or limiting backups to an explicit subset of models rather than all `mongoose.modelNames()` to reduce memory usage and long-running queries.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new backup collection logic has several correctness and scalability problems: it runs all model queries concurrently (high memory/DB load), uses a potentially invalid now - 24h baseline when no prior backup exists (can produce incomplete restore chains), and assumes updatedAt exists/reliably updates for every collection. getLastBackupTime() is also potentially very expensive because it reads/parses every *.meta.json on each run, and it silently ignores individual metadata file errors. There’s also an avoidable Record<string, any> that reduces type-safety.
Summary of changes
What changed
- Added a Mongoose-based implementation for
getDataForBackup()that:- Builds a
backupDatapayload withtimestamp,type, and per-model documents. - Uses
mongoose.modelNames()andModel.find(query).lean().exec()to collect documents. - Applies an
updatedAt >= baselineTimefilter forDIFFERENTIAL/INCREMENTALbackups.
- Builds a
- Introduced
getLastBackupTime(requireFull?: boolean)to scanbackups/for*.meta.jsonfiles and compute the latest completed backup timestamp. - Tweaked error logging to prefer
error.messagewhen available. - Added
import mongoose from "mongoose".
src/lib/security/backup/index.ts
Outdated
| private async getLastBackupTime(requireFull: boolean = false): Promise<Date | null> { | ||
| let latestTimestamp = 0; | ||
|
|
||
| const storageEntries = Array.from(this.storageProviders.entries()) | ||
| for (let i = 0; i < storageEntries.length; i++) { | ||
| const entry = storageEntries[i] | ||
| if (!entry || !Array.isArray(entry) || entry.length < 2) continue | ||
|
|
||
| const [location, provider] = entry | ||
| if (!provider) continue | ||
|
|
||
| try { | ||
| const files = await provider.listFiles("backups/") // Fallback to list all and filter if pattern not supported | ||
| const metaFiles = files.filter(f => f.endsWith(".meta.json")) | ||
| for (const file of metaFiles) { | ||
| try { | ||
| const metadataBuffer = await provider.getFile(file) | ||
| const metadata = JSON.parse(new TextDecoder().decode(metadataBuffer)) as BackupMetadata | ||
|
|
||
| if (metadata.status === BackupStatus.COMPLETED) { | ||
| if (requireFull && metadata.type !== BackupType.FULL) continue | ||
| if (!requireFull && metadata.type !== BackupType.FULL && metadata.type !== BackupType.DIFFERENTIAL && metadata.type !== BackupType.INCREMENTAL) continue | ||
|
|
||
| const timestamp = new Date(metadata.timestamp).getTime() | ||
| if (timestamp > latestTimestamp) { | ||
| latestTimestamp = timestamp | ||
| } | ||
| } | ||
| } catch (e) { | ||
| // Ignore individual file parsing errors | ||
| } | ||
| } | ||
| } catch (error: unknown) { | ||
| logger.error(`Error searching for latest backup metadata: ${error instanceof Error ? error.message : String(error)}`) | ||
| } | ||
| } | ||
|
|
||
| return latestTimestamp > 0 ? new Date(latestTimestamp) : null; | ||
| } |
There was a problem hiding this comment.
getLastBackupTime() lists all files under backups/ and then fetches/parses every *.meta.json to find the newest timestamp. This can become extremely expensive on large repositories/backends (many metadata files, network round-trips per file) and will run on every incremental/differential backup.
Suggestion
Avoid reading/parsing every metadata file on each run. Options:
- If filenames contain sortable timestamps/IDs, derive the latest candidate from the name and only fetch that file.
- Maintain a small index/"latest" pointer file per storage location (e.g.
backups/latest.meta.json) written at backup completion. - If your provider supports server-side filtering/sorting, add/extend an API to query only the latest
*.meta.json.
Reply with "@CharlieHelps yes please" if you want me to add a commit that introduces a latest.meta.json pointer approach.
src/lib/security/backup/index.ts
Outdated
| for (const file of metaFiles) { | ||
| try { | ||
| const metadataBuffer = await provider.getFile(file) | ||
| const metadata = JSON.parse(new TextDecoder().decode(metadataBuffer)) as BackupMetadata | ||
|
|
||
| if (metadata.status === BackupStatus.COMPLETED) { | ||
| if (requireFull && metadata.type !== BackupType.FULL) continue | ||
| if (!requireFull && metadata.type !== BackupType.FULL && metadata.type !== BackupType.DIFFERENTIAL && metadata.type !== BackupType.INCREMENTAL) continue | ||
|
|
||
| const timestamp = new Date(metadata.timestamp).getTime() | ||
| if (timestamp > latestTimestamp) { | ||
| latestTimestamp = timestamp | ||
| } | ||
| } | ||
| } catch (e) { | ||
| // Ignore individual file parsing errors | ||
| } | ||
| } |
There was a problem hiding this comment.
The inner catch (e) silently ignores metadata parse/read errors for individual files. That can mask corruption/partial uploads and lead to choosing an incorrect baseline timestamp without any observability.
Suggestion
At minimum, log (ideally at debug/warn) which metadata file failed to parse and why, so operators can detect corruption.
Example:
} catch (e: unknown) {
logger.warn(
`Skipping invalid backup metadata file ${file}: ${e instanceof Error ? e.message : String(e)}`,
)
}Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this logging change.
src/lib/security/backup/index.ts
Outdated
| const models = mongoose.modelNames() | ||
| const dataPromises = models.map(async (modelName) => { | ||
| const Model = mongoose.model(modelName) | ||
| const docs = await Model.find(query).lean().exec() | ||
| return { modelName, docs } | ||
| }) | ||
|
|
||
| const results = await Promise.all(dataPromises) | ||
|
|
||
| for (const result of results) { | ||
| if (result.docs.length > 0) { | ||
| backupData.data[result.modelName] = result.docs | ||
| } | ||
| } | ||
| } catch (error: unknown) { |
There was a problem hiding this comment.
getDataForBackup() uses Promise.all() over all models, which runs all collection queries concurrently and materializes all results at once. This contradicts the PR description (“parsing them sequentially for memory safety”) and can spike DB load and memory usage (especially combined with JSON.stringify of a large in-memory object).
Suggestion
Collect model data sequentially (or with a small concurrency limit) and consider streaming/pagination for large collections.
Sequential example:
for (const modelName of mongoose.modelNames()) {
const Model = mongoose.model(modelName)
const docs = await Model.find(query).lean().exec()
if (docs.length) backupData.data[modelName] = docs
}If collections can be large, prefer cursors/pagination rather than loading everything into memory.
Reply with "@CharlieHelps yes please" if you’d like me to add a commit that switches to sequential collection (and optionally adds a simple concurrency limiter).
| if (type === BackupType.DIFFERENTIAL || type === BackupType.INCREMENTAL) { | ||
| const requireFull = type === BackupType.DIFFERENTIAL | ||
| const lastBackupTime = await this.getLastBackupTime(requireFull) | ||
| const baselineTime = lastBackupTime || new Date(Date.now() - 24 * 60 * 60 * 1000) | ||
| query = { updatedAt: { $gte: baselineTime } } | ||
| } |
There was a problem hiding this comment.
Differential/incremental backups fall back to baselineTime = now - 24h when no prior baseline is found. That can produce invalid backup chains (missing older documents), making restores incomplete. For DIFFERENTIAL, if there’s no prior FULL, the correct behavior is usually to fail or force a FULL backup.
Suggestion
Treat “no baseline found” as a hard error (or automatically behave as FULL) to avoid silently producing incomplete backups.
Example:
const lastBackupTime = await this.getLastBackupTime(requireFull)
if (!lastBackupTime) {
if (type === BackupType.DIFFERENTIAL) {
throw new Error('No FULL backup found; cannot create DIFFERENTIAL backup')
}
// INCREMENTAL: either throw or treat as FULL
query = {}
} else {
query = { updatedAt: { $gte: lastBackupTime } }
}Reply with "@CharlieHelps yes please" if you want me to add a commit implementing this baseline behavior.
src/lib/security/backup/index.ts
Outdated
| const backupData: Record<string, any> = { | ||
| timestamp: new Date().toISOString(), | ||
| type, | ||
| data: {} | ||
| } |
There was a problem hiding this comment.
backupData is typed as Record<string, any>, which weakens type-safety and makes it easier to accidentally serialize unsupported shapes (and can hide mistakes in backupData.data[...]). This is a concrete use of any, which is a long-term maintainability footgun.
Suggestion
Replace any with a minimal explicit type for the backup payload.
Example:
type BackupPayload = {
timestamp: string
type: BackupType
data: Record<string, unknown[]> // or a stricter document type if available
}
const backupData: BackupPayload = {
timestamp: new Date().toISOString(),
type,
data: {},
}Reply with "@CharlieHelps yes please" if you’d like me to add a commit that removes the any here.
src/lib/security/backup/index.ts
Outdated
| let query = {} | ||
|
|
||
| if (type === BackupType.DIFFERENTIAL || type === BackupType.INCREMENTAL) { | ||
| const requireFull = type === BackupType.DIFFERENTIAL | ||
| const lastBackupTime = await this.getLastBackupTime(requireFull) | ||
| const baselineTime = lastBackupTime || new Date(Date.now() - 24 * 60 * 60 * 1000) | ||
| query = { updatedAt: { $gte: baselineTime } } | ||
| } |
There was a problem hiding this comment.
Incremental/differential logic assumes every collection supports an updatedAt field. For models without timestamps (or where updatedAt isn’t reliably updated), this will silently drop records from non-full backups, breaking restore correctness for those collections.
Suggestion
Handle models without updatedAt deterministically (e.g., treat those models as full-copy even during incremental/differential, or enforce timestamps across all schemas and fail fast if missing).
One pragmatic approach:
- Detect timestamp support per model (schema timestamps option or presence of
updatedAtpath). - If missing, either:
- query without the
updatedAtfilter for that model, or - skip with a warning and document that such collections are not incrementally backed up.
- query without the
Reply with "@CharlieHelps yes please" if you want me to add a commit that detects missing updatedAt and falls back to full-copy per model.
src/lib/security/backup/index.ts
Outdated
| private async getLastBackupTime(requireFull: boolean = false): Promise<Date | null> { | ||
| let latestTimestamp = 0; | ||
|
|
||
| const storageEntries = Array.from(this.storageProviders.entries()) | ||
| for (let i = 0; i < storageEntries.length; i++) { | ||
| const entry = storageEntries[i] | ||
| if (!entry || !Array.isArray(entry) || entry.length < 2) continue | ||
|
|
||
| const [location, provider] = entry | ||
| if (!provider) continue | ||
|
|
||
| try { | ||
| const files = await provider.listFiles("backups/") // Fallback to list all and filter if pattern not supported | ||
| const metaFiles = files.filter(f => f.endsWith(".meta.json")) | ||
| for (const file of metaFiles) { | ||
| try { | ||
| const metadataBuffer = await provider.getFile(file) | ||
| const metadata = JSON.parse(new TextDecoder().decode(metadataBuffer)) as BackupMetadata | ||
|
|
||
| if (metadata.status === BackupStatus.COMPLETED) { | ||
| if (requireFull && metadata.type !== BackupType.FULL) continue | ||
| if (!requireFull && metadata.type !== BackupType.FULL && metadata.type !== BackupType.DIFFERENTIAL && metadata.type !== BackupType.INCREMENTAL) continue | ||
|
|
||
| const timestamp = new Date(metadata.timestamp).getTime() | ||
| if (timestamp > latestTimestamp) { | ||
| latestTimestamp = timestamp | ||
| } | ||
| } | ||
| } catch (e) { | ||
| // Ignore individual file parsing errors | ||
| } | ||
| } | ||
| } catch (error: unknown) { | ||
| logger.error(`Error searching for latest backup metadata: ${error instanceof Error ? error.message : String(error)}`) | ||
| } | ||
| } | ||
|
|
||
| return latestTimestamp > 0 ? new Date(latestTimestamp) : null; | ||
| } |
There was a problem hiding this comment.
getLastBackupTime() hard-codes the prefix "backups/". If other parts of this module write metadata under a different base path (or per-StorageLocation root), this will silently fail to find baselines and cause incorrect differential/incremental behavior.
Suggestion
Reuse the same directory/prefix constants or path-building helpers used by the code that writes backups/metadata (and/or pass the base prefix in) instead of hard-coding "backups/".
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this refactor.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/lib/security/backup/index.ts">
<violation number="1" location="src/lib/security/backup/index.ts:825">
P1: `listFiles("backups/")` uses a non-matching pattern, so previous metadata files are not found.</violation>
<violation number="2" location="src/lib/security/backup/index.ts:832">
P1: Incremental/differential baseline selection only considers COMPLETED backups, but created backups remain PENDING in normal flow, causing repeated fallback windows and potential data omission.</violation>
<violation number="3" location="src/lib/security/backup/index.ts:866">
P1: Using a 24-hour fallback baseline can omit older records and produce incomplete differential/incremental backups.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const metadataBuffer = await provider.getFile(file) | ||
| const metadata = JSON.parse(new TextDecoder().decode(metadataBuffer)) as BackupMetadata | ||
|
|
||
| if (metadata.status === BackupStatus.COMPLETED) { |
There was a problem hiding this comment.
P1: Incremental/differential baseline selection only considers COMPLETED backups, but created backups remain PENDING in normal flow, causing repeated fallback windows and potential data omission.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/security/backup/index.ts, line 832:
<comment>Incremental/differential baseline selection only considers COMPLETED backups, but created backups remain PENDING in normal flow, causing repeated fallback windows and potential data omission.</comment>
<file context>
@@ -809,16 +810,83 @@ export class BackupSecurityManager {
+ const metadataBuffer = await provider.getFile(file)
+ const metadata = JSON.parse(new TextDecoder().decode(metadataBuffer)) as BackupMetadata
+
+ if (metadata.status === BackupStatus.COMPLETED) {
+ if (requireFull && metadata.type !== BackupType.FULL) continue
+ if (!requireFull && metadata.type !== BackupType.FULL && metadata.type !== BackupType.DIFFERENTIAL && metadata.type !== BackupType.INCREMENTAL) continue
</file context>
src/lib/security/backup/index.ts
Outdated
| if (type === BackupType.DIFFERENTIAL || type === BackupType.INCREMENTAL) { | ||
| const requireFull = type === BackupType.DIFFERENTIAL | ||
| const lastBackupTime = await this.getLastBackupTime(requireFull) | ||
| const baselineTime = lastBackupTime || new Date(Date.now() - 24 * 60 * 60 * 1000) |
There was a problem hiding this comment.
P1: Using a 24-hour fallback baseline can omit older records and produce incomplete differential/incremental backups.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/security/backup/index.ts, line 866:
<comment>Using a 24-hour fallback baseline can omit older records and produce incomplete differential/incremental backups.</comment>
<file context>
@@ -809,16 +810,83 @@ export class BackupSecurityManager {
+ if (type === BackupType.DIFFERENTIAL || type === BackupType.INCREMENTAL) {
+ const requireFull = type === BackupType.DIFFERENTIAL
+ const lastBackupTime = await this.getLastBackupTime(requireFull)
+ const baselineTime = lastBackupTime || new Date(Date.now() - 24 * 60 * 60 * 1000)
+ query = { updatedAt: { $gte: baselineTime } }
+ }
</file context>
| const baselineTime = lastBackupTime || new Date(Date.now() - 24 * 60 * 60 * 1000) | |
| const baselineTime = lastBackupTime || new Date(0) |
src/lib/security/backup/index.ts
Outdated
| if (!provider) continue | ||
|
|
||
| try { | ||
| const files = await provider.listFiles("backups/") // Fallback to list all and filter if pattern not supported |
There was a problem hiding this comment.
P1: listFiles("backups/") uses a non-matching pattern, so previous metadata files are not found.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/security/backup/index.ts, line 825:
<comment>`listFiles("backups/")` uses a non-matching pattern, so previous metadata files are not found.</comment>
<file context>
@@ -809,16 +810,83 @@ export class BackupSecurityManager {
+ if (!provider) continue
+
+ try {
+ const files = await provider.listFiles("backups/") // Fallback to list all and filter if pattern not supported
+ const metaFiles = files.filter(f => f.endsWith(".meta.json"))
+ for (const file of metaFiles) {
</file context>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/lib/security/backup/index.ts (2)
859-864:⚠️ Potential issue | 🟠 Major24-hour fallback for DIFFERENTIAL backups risks incomplete backup chains.
When no prior FULL backup is found for a DIFFERENTIAL backup (line 862-863), the code falls back to
Date.now() - 24h. This silently produces an incomplete backup—documents older than 24 hours will be missing, making restore incomplete. A DIFFERENTIAL backup by definition requires a FULL baseline.🐛 Proposed fix to fail fast when no FULL baseline exists
let baselineTime: Date | null = null; if (type === BackupType.DIFFERENTIAL || type === BackupType.INCREMENTAL) { const requireFull = type === BackupType.DIFFERENTIAL; const lastBackupTime = await this.getLastBackupTime(requireFull); - baselineTime = lastBackupTime || new Date(Date.now() - 24 * 60 * 60 * 1000); + if (!lastBackupTime) { + if (type === BackupType.DIFFERENTIAL) { + throw new Error('Cannot create DIFFERENTIAL backup: no prior FULL backup found'); + } + // INCREMENTAL without any prior backup: treat as FULL (no filter) + baselineTime = null; + } else { + baselineTime = lastBackupTime; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/security/backup/index.ts` around lines 859 - 864, The current fallback to Date.now()-24h for baselineTime when type === BackupType.DIFFERENTIAL can create incomplete backups; instead, after calling this.getLastBackupTime(requireFull) check if requireFull is true and lastBackupTime is null/undefined and fail fast by throwing a descriptive error (e.g., "No FULL baseline found for DIFFERENTIAL backup") rather than assigning a 24-hour fallback; update the logic around baselineTime (the block using BackupType.DIFFERENTIAL / BackupType.INCREMENTAL and this.getLastBackupTime(requireFull)) to throw when requireFull && !lastBackupTime so callers can handle the missing baseline.
837-839:⚠️ Potential issue | 🟡 MinorSilent catch masks metadata corruption and hinders debugging.
The inner
catchblock silently discards parse/read errors. Per coding guidelines, failures should be logged rather than silently ignored. This can mask corrupted or partially uploaded metadata files.🔧 Proposed fix to add warning log
- } catch { - // Ignore individual file parsing errors - } + } catch (e: unknown) { + logger.warn( + `Skipping invalid backup metadata file ${metaFile}: ${e instanceof Error ? e.message : String(e)}`, + ); + }As per coding guidelines: "Always log failures rather than silently failing; implement proper error handling and logging".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/security/backup/index.ts` around lines 837 - 839, The inner catch in src/lib/security/backup/index.ts currently swallows parsing/read errors; change it to log a warning with the error and the metadata file identifier before continuing so corruption is visible. Locate the try/catch around the metadata parsing (the anonymous catch that currently has "// Ignore individual file parsing errors") and replace the silent swallow with a logger call (e.g., processLogger.warn or the module's logger) that includes a descriptive message, the caught error object, and the filename/metadata key being processed so callers can trace corrupted or partially uploaded metadata. Ensure the code still continues processing other files after logging.
🧹 Nitpick comments (1)
src/lib/security/backup/index.ts (1)
816-818: Unused variablelocationfrom destructuring.The
locationvariable is destructured but never used. This was flagged by static analysis.♻️ Proposed fix
- const [, provider] = storageEntries[i]; + const entry = storageEntries[i]; + if (!entry) continue; + const provider = entry[1];Or simply use
_convention if your linter allows it:- const [, provider] = storageEntries[i]; + const [_location, provider] = storageEntries[i];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/security/backup/index.ts` around lines 816 - 818, The destructuring in the loop over storageEntries creates an unused variable `location` (for example in the for loop that does "const [, provider] = storageEntries[i]"), causing lint warnings; fix it by either removing the unused destructured element (only extract `provider`) or replace it with a throwaway name like `_location` or `_` according to linter rules so only `provider` is used in the loop body (update the destructuring in that loop where storageEntries is iterated).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/security/backup/index.ts`:
- Around line 859-864: The current fallback to Date.now()-24h for baselineTime
when type === BackupType.DIFFERENTIAL can create incomplete backups; instead,
after calling this.getLastBackupTime(requireFull) check if requireFull is true
and lastBackupTime is null/undefined and fail fast by throwing a descriptive
error (e.g., "No FULL baseline found for DIFFERENTIAL backup") rather than
assigning a 24-hour fallback; update the logic around baselineTime (the block
using BackupType.DIFFERENTIAL / BackupType.INCREMENTAL and
this.getLastBackupTime(requireFull)) to throw when requireFull &&
!lastBackupTime so callers can handle the missing baseline.
- Around line 837-839: The inner catch in src/lib/security/backup/index.ts
currently swallows parsing/read errors; change it to log a warning with the
error and the metadata file identifier before continuing so corruption is
visible. Locate the try/catch around the metadata parsing (the anonymous catch
that currently has "// Ignore individual file parsing errors") and replace the
silent swallow with a logger call (e.g., processLogger.warn or the module's
logger) that includes a descriptive message, the caught error object, and the
filename/metadata key being processed so callers can trace corrupted or
partially uploaded metadata. Ensure the code still continues processing other
files after logging.
---
Nitpick comments:
In `@src/lib/security/backup/index.ts`:
- Around line 816-818: The destructuring in the loop over storageEntries creates
an unused variable `location` (for example in the for loop that does "const [,
provider] = storageEntries[i]"), causing lint warnings; fix it by either
removing the unused destructured element (only extract `provider`) or replace it
with a throwaway name like `_location` or `_` according to linter rules so only
`provider` is used in the loop body (update the destructuring in that loop where
storageEntries is iterated).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d0f3fcd-ecf8-4c73-b2f1-a6117637a010
📒 Files selected for processing (1)
src/lib/security/backup/index.ts
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/lib/security/backup/index.ts">
<violation number="1" location="src/lib/security/backup/index.ts:821">
P2: `getLastBackupTime` uses `listFiles("backups/")`, but storage providers treat the argument as a full glob pattern (not a recursive prefix). For providers like `LocalFileSystemProvider`, the pattern is anchored, so `backups/` never matches nested metadata files and the last-backup scan always returns null.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (!provider) continue; | ||
|
|
||
| try { | ||
| const files = await provider.listFiles("backups/"); |
There was a problem hiding this comment.
P2: getLastBackupTime uses listFiles("backups/"), but storage providers treat the argument as a full glob pattern (not a recursive prefix). For providers like LocalFileSystemProvider, the pattern is anchored, so backups/ never matches nested metadata files and the last-backup scan always returns null.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/security/backup/index.ts, line 821:
<comment>`getLastBackupTime` uses `listFiles("backups/")`, but storage providers treat the argument as a full glob pattern (not a recursive prefix). For providers like `LocalFileSystemProvider`, the pattern is anchored, so `backups/` never matches nested metadata files and the last-backup scan always returns null.</comment>
<file context>
@@ -810,83 +809,95 @@ export class BackupSecurityManager {
- const files = await provider.listFiles("backups/") // Fallback to list all and filter if pattern not supported
- const metaFiles = files.filter(f => f.endsWith(".meta.json"))
- for (const file of metaFiles) {
+ const files = await provider.listFiles("backups/");
+ const metaFiles = files.filter((f) => f.endsWith(".meta.json"));
+ for (const metaFile of metaFiles) {
</file context>
| const files = await provider.listFiles("backups/"); | |
| const files = await provider.listFiles("backups/*/*/*/*.meta.json"); |
There was a problem hiding this comment.
Pull request overview
Implements real backup payload collection by querying Mongoose models and adds metadata scanning to establish a baseline timestamp for differential/incremental backups.
Changes:
- Added
getLastBackupTime()to scanbackups/*.meta.jsonacross storage providers and compute a baseline timestamp. - Replaced dummy
getDataForBackup()data with a Mongoose-backed collection sweep using.lean().cursor()and optionalupdatedAtfiltering. - Standardized some error logging to prefer
error.message.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| try { | ||
| const mongooseModule = "mongoose"; | ||
| const mongoose = (await import(/* @vite-ignore */ mongooseModule)).default || await import(/* @vite-ignore */ mongooseModule); |
There was a problem hiding this comment.
This does two dynamic imports and can yield inconsistent types: if .default is missing, mongoose becomes the module namespace object, so mongoose.modelNames() may not exist. Import once, then resolve default vs namespace from that single result (e.g., store the import result in a variable and use mod.default ?? mod). This also avoids the extra module load.
| const mongoose = (await import(/* @vite-ignore */ mongooseModule)).default || await import(/* @vite-ignore */ mongooseModule); | |
| const mongooseImport = await import(/* @vite-ignore */ mongooseModule); | |
| const mongoose = (mongooseImport as any).default ?? mongooseImport; |
| // [PIX-44] TODO: No more fucking cop-outs | ||
| const dummyData = { | ||
| message: `This is a ${type} backup created at ${new Date().toISOString()}`, | ||
| let appDataJson = '{"timestamp":"' + new Date().toISOString() + '","type":"' + type + '","data":{'; |
There was a problem hiding this comment.
Building a large JSON payload via repeated string += ... inside nested loops can become very costly (frequent reallocations/copies) as the backup grows. Prefer accumulating into an array of string chunks and join('') once, or write into a dedicated buffer/stream abstraction; it keeps the same sequential read pattern without the quadratic-ish string concatenation overhead.
| if (!isFirstModel) { | ||
| appDataJson += ','; | ||
| } | ||
| appDataJson += '"' + modelName + '":['; |
There was a problem hiding this comment.
Building a large JSON payload via repeated string += ... inside nested loops can become very costly (frequent reallocations/copies) as the backup grows. Prefer accumulating into an array of string chunks and join('') once, or write into a dedicated buffer/stream abstraction; it keeps the same sequential read pattern without the quadratic-ish string concatenation overhead.
| for await (const doc of cursor) { | ||
| if (!isFirstDoc) { | ||
| appDataJson += ','; | ||
| } | ||
| appDataJson += JSON.stringify(doc); | ||
| isFirstDoc = false; | ||
| } |
There was a problem hiding this comment.
Building a large JSON payload via repeated string += ... inside nested loops can become very costly (frequent reallocations/copies) as the backup grows. Prefer accumulating into an array of string chunks and join('') once, or write into a dedicated buffer/stream abstraction; it keeps the same sequential read pattern without the quadratic-ish string concatenation overhead.
| appDataJson += ']'; | ||
| } | ||
|
|
||
| appDataJson += '}}'; |
There was a problem hiding this comment.
Building a large JSON payload via repeated string += ... inside nested loops can become very costly (frequent reallocations/copies) as the backup grows. Prefer accumulating into an array of string chunks and join('') once, or write into a dedicated buffer/stream abstraction; it keeps the same sequential read pattern without the quadratic-ish string concatenation overhead.
|
|
||
| // Use TextEncoder for cross-environment compatibility | ||
| return new TextEncoder().encode(JSON.stringify(dummyData)) | ||
| return new TextEncoder().encode(appDataJson); |
There was a problem hiding this comment.
Building a large JSON payload via repeated string += ... inside nested loops can become very costly (frequent reallocations/copies) as the backup grows. Prefer accumulating into an array of string chunks and join('') once, or write into a dedicated buffer/stream abstraction; it keeps the same sequential read pattern without the quadratic-ish string concatenation overhead.
| if (type === BackupType.DIFFERENTIAL || type === BackupType.INCREMENTAL) { | ||
| const requireFull = type === BackupType.DIFFERENTIAL; | ||
| const lastBackupTime = await this.getLastBackupTime(requireFull); | ||
| baselineTime = lastBackupTime || new Date(Date.now() - 24 * 60 * 60 * 1000); |
There was a problem hiding this comment.
The 24 * 60 * 60 * 1000 fallback is a magic number and the rationale isn’t encoded in the code. Extract this to a named constant (e.g., DEFAULT_BASELINE_LOOKBACK_MS) and add a short comment explaining why 24h is the chosen fallback.
| } catch { | ||
| // Ignore individual file parsing errors |
There was a problem hiding this comment.
Swallowing all per-file parsing errors makes it hard to diagnose corrupted/partial metadata and can silently degrade baseline discovery (e.g., always falling back). Consider at least logging at debug/warn with the metaFile name (and the caught error message), or explicitly handling expected parse failures vs unexpected ones.
| } catch { | |
| // Ignore individual file parsing errors | |
| } catch (error: unknown) { | |
| logger.warn( | |
| `Failed to parse backup metadata file "${metaFile}": ${ | |
| error instanceof Error ? error.message : String(error) | |
| }`, | |
| ); |
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
Replaced the placeholder dummy data implementation in
getDataForBackup(PIX-44) with an actual implementation that uses Mongoose to collect application data for backups based on the backup type.Changes include:
getDataForBackupnow fetches data from all registered mongoose collections, parsing them sequentially for memory safety using.lean().getLastBackupTimeto search through previous backup metadata files (.meta.json) using standard directory prefixes to establish valid baseline timestamps forDIFFERENTIALandINCREMENTALbackups.error.message.PR created automatically by Jules for task 7376318658770257015 started by @daggerstuff
Summary by Sourcery
Replace the placeholder backup data implementation with a real Mongoose-based data collection mechanism and introduce backup metadata scanning to support differential and incremental backups.
New Features:
Enhancements:
Summary by cubic
Implemented real backup data collection using
mongooseand metadata scanning to set baselines for differential and incremental backups (PIX-44). This enables accurate backups across all storage providers.New Features
getDataForBackup: builds one JSON payload with timestamp/type and per-model arrays; streams allmongoosemodels via.lean().cursor(); for differential/incremental, filters byupdatedAt(if present) using the last valid backup time or a 24h fallback.getLastBackupTime: scansbackups/*.meta.jsonacross providers for the latest COMPLETED backup; requires a FULL baseline for differential, otherwise allows FULL/DIFFERENTIAL/INCREMENTAL; ignores malformed files.Bug Fixes
error.message; fail loudly on data collection errors.Written for commit cac6640. Summary will update on new commits.
Summary by CodeRabbit