feat(crawler): discard oversized full-page archives via CRAWLER_FULL_PAGE_ARCHIVE_MAX_SIZE_MB - #3054
feat(crawler): discard oversized full-page archives via CRAWLER_FULL_PAGE_ARCHIVE_MAX_SIZE_MB#3054NoiceHax wants to merge 1 commit into
Conversation
…PAGE_ARCHIVE_MAX_SIZE_MB Media-rich pages can produce 1GB+ monolith archives that never render and only hang/crash browsers when opened. There was no way to cap them short of the overall storage quota. Adds an opt-in limit (MB, 0 = disabled default so current behavior is unchanged): after monolith runs, archives over the limit are discarded with a warning instead of saved. Mirrors the existing CRAWLER_VIDEO_DOWNLOAD_MAX_SIZE precedent, including the docs-table row. Fixes karakeep-app#1687
Greptile SummaryThis PR adds an opt-in maximum size for crawler-generated full-page archives. The worker measures Monolith output before quota and storage operations, removes archives exceeding the configured threshold, and documents the new environment variable.
Confidence Score: 4/5The PR appears safe to merge, with non-blocking hardening needed around temporary-file cleanup, configuration validation, and regression coverage. The size-cap flow is correctly placed before quota checks and storage, and returning Files Needing Attention: apps/workers/workers/crawler/assetStorage.ts, packages/shared/config.ts Important Files Changed
Prompt To Fix All With AI### Issue 1
apps/workers/workers/crawler/assetStorage.ts:457
**Failed cleanup is hidden**
If deleting an oversized archive fails, `tryCatch` suppresses the error and the function still returns `null`. The caller has no other cleanup path or access to the temporary filename, so the potentially multi-gigabyte file remains in the temporary directory. Repeated failures could exhaust local storage. Please inspect the unlink result and surface or retry failed cleanup.
### Issue 2
packages/shared/config.ts:142
**Negative values disable limit**
The new setting accepts negative numbers, but the worker treats every non-positive value as disabling the cap. An operator typo such as `-100` therefore silently defeats the storage safeguard even though only `0` is documented as disabled. Constrain the parsed value to be nonnegative.
```suggestion
CRAWLER_FULL_PAGE_ARCHIVE_MAX_SIZE_MB: z.coerce.number().min(0).default(0),
```
### Issue 3
apps/workers/workers/crawler/assetStorage.ts:452-458
**Discard path lacks tests**
The new configuration-dependent filesystem behavior has no focused automated coverage. Add tests for the disabled setting, files at and above the boundary, deletion of oversized files, and prevention of quota or storage calls after a discard. Without these checks, a later change could silently restore the multi-gigabyte archive problem.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(crawler): discard oversized full-pa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| logger.warn( | ||
| `[Crawler][${jobId}] Discarding page archive of ${fileSize} bytes as it exceeds CRAWLER_FULL_PAGE_ARCHIVE_MAX_SIZE_MB=${maxArchiveSizeMb}.`, | ||
| ); | ||
| await tryCatch(fs.unlink(assetPath)); |
There was a problem hiding this comment.
If deleting an oversized archive fails, tryCatch suppresses the error and the function still returns null. The caller has no other cleanup path or access to the temporary filename, so the potentially multi-gigabyte file remains in the temporary directory. Repeated failures could exhaust local storage. Please inspect the unlink result and surface or retry failed cleanup.
Knowledge Base Used: Content ingestion and enrichment jobs
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/workers/workers/crawler/assetStorage.ts
Line: 457
Comment:
**Failed cleanup is hidden**
If deleting an oversized archive fails, `tryCatch` suppresses the error and the function still returns `null`. The caller has no other cleanup path or access to the temporary filename, so the potentially multi-gigabyte file remains in the temporary directory. Repeated failures could exhaust local storage. Please inspect the unlink result and surface or retry failed cleanup.
**Knowledge Base Used:** [Content ingestion and enrichment jobs](https://app.greptile.com/karakeep/-/custom-context/knowledge-base/karakeep-app/karakeep/-/docs/content-ingestion-enrichment-jobs.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| CRAWLER_FULL_PAGE_SCREENSHOT: stringBool("false"), | ||
| CRAWLER_STORE_PDF: stringBool("false"), | ||
| CRAWLER_FULL_PAGE_ARCHIVE: stringBool("false"), | ||
| CRAWLER_FULL_PAGE_ARCHIVE_MAX_SIZE_MB: z.coerce.number().default(0), |
There was a problem hiding this comment.
The new setting accepts negative numbers, but the worker treats every non-positive value as disabling the cap. An operator typo such as -100 therefore silently defeats the storage safeguard even though only 0 is documented as disabled. Constrain the parsed value to be nonnegative.
| CRAWLER_FULL_PAGE_ARCHIVE_MAX_SIZE_MB: z.coerce.number().default(0), | |
| CRAWLER_FULL_PAGE_ARCHIVE_MAX_SIZE_MB: z.coerce.number().min(0).default(0), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/shared/config.ts
Line: 142
Comment:
**Negative values disable limit**
The new setting accepts negative numbers, but the worker treats every non-positive value as disabling the cap. An operator typo such as `-100` therefore silently defeats the storage safeguard even though only `0` is documented as disabled. Constrain the parsed value to be nonnegative.
```suggestion
CRAWLER_FULL_PAGE_ARCHIVE_MAX_SIZE_MB: z.coerce.number().min(0).default(0),
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const maxArchiveSizeMb = serverConfig.crawler.fullPageArchiveMaxSizeMb; | ||
| if (maxArchiveSizeMb > 0 && fileSize > maxArchiveSizeMb * 1024 * 1024) { | ||
| logger.warn( | ||
| `[Crawler][${jobId}] Discarding page archive of ${fileSize} bytes as it exceeds CRAWLER_FULL_PAGE_ARCHIVE_MAX_SIZE_MB=${maxArchiveSizeMb}.`, | ||
| ); | ||
| await tryCatch(fs.unlink(assetPath)); | ||
| return null; |
There was a problem hiding this comment.
The new configuration-dependent filesystem behavior has no focused automated coverage. Add tests for the disabled setting, files at and above the boundary, deletion of oversized files, and prevention of quota or storage calls after a discard. Without these checks, a later change could silently restore the multi-gigabyte archive problem.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/workers/workers/crawler/assetStorage.ts
Line: 452-458
Comment:
**Discard path lacks tests**
The new configuration-dependent filesystem behavior has no focused automated coverage. Add tests for the disabled setting, files at and above the boundary, deletion of oversized files, and prevention of quota or storage calls after a discard. Without these checks, a later change could silently restore the multi-gigabyte archive problem.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
What does this PR do?
Adds an opt-in size cap for monolith full-page archives. Media-rich pages can produce 1GB+ archive files that never render and only hang/crash browsers when opened; now archives over the configured limit are discarded with a warning instead of saved.
Changes
CRAWLER_FULL_PAGE_ARCHIVE_MAX_SIZE_MBenv var (MB, default0= disabled, so current behavior is unchanged)CRAWLER_*varsCRAWLER_VIDEO_DOWNLOAD_MAX_SIZEprecedentFixes #1687