Prevent Goggles memory amplification and contain worker pressure#230
Conversation
WalkthroughThe PR adds configurable upload, export, pagination, and web runtime limits; bounds JSONL ingestion and ORM hydration; adds truncation metadata and notices; and documents deployment procedures and operational safety behavior. ChangesOperational limits and deployment wiring
Bounded upload ingestion
Lightweight projection and evidence queries
Bounded APIs, tabs, exports, and notices
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Ready to review this PR? Stage has broken it down into 7 individual chapters for you: 
Chapters generated by Stage for commit 5fd6e04 on Jul 10, 2026 8:58am UTC. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
forensics/tests.py (1)
98-112: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
HEAVY_BULK_SELECT_COLUMNSmisses columns the app itself treats as heavy.
HEAVY_EVENT_FIELDSinforensics/views.py(raw_kind, raw_context, context_human_action, context_transport, context_engine, context_group, context_convergence, context_source) are all deliberately excluded fromEVENT_ROW_FIELDSfor bulk queries, but this test helper only checks forraw_text/raw_line/raw_event. A future regression that reintroduces one of the other heavy JSON columns into a bulk queryset would pass everyheavy_bulk_selects(...) == []assertion in this file undetected.♻️ Broaden the safety net
HEAVY_BULK_SELECT_COLUMNS = ( '"forensics_auditfile"."raw_text"', '"forensics_auditevent"."raw_line"', '"forensics_auditevent"."raw_event"', + '"forensics_auditevent"."raw_kind"', + '"forensics_auditevent"."raw_context"', + '"forensics_auditevent"."context_human_action"', )🤖 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 `@forensics/tests.py` around lines 98 - 112, Expand HEAVY_BULK_SELECT_COLUMNS in heavy_bulk_selects to include every field listed in HEAVY_EVENT_FIELDS from forensics/views.py: raw_kind, raw_context, context_human_action, context_transport, context_engine, context_group, context_convergence, and context_source, while retaining the existing columns.
🤖 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 `@docker-compose.yml`:
- Around line 43-44: The Docker Compose operational limits are hard-coded
despite the configuration contract. Update the service resource and logging
settings associated with cpus, pids_limit, max-size, and max-file to use
documented GOGGLES_WEB_* environment variables with appropriate defaults,
matching the existing memory and swap configuration; alternatively, narrow the
documented contract to exclude these limits.
- Around line 40-41: Update the memory-limit configuration used by the Compose
service: the ${GOGGLES_WEB_MEMORY_LIMIT:-16g} interpolation in mem_limit and
memswap_limit only reads Compose-time environment sources, not GOGGLES_ENV_FILE.
Either document that this variable must be defined in the shell or Compose .env,
or change the deployment configuration so both fields read the limit from the
same source as the other settings.
In `@docs/deployment.md`:
- Around line 66-68: Update the deployment step to avoid hard-coding
`goggles-web-1`: first resolve the running web container ID using `docker
compose ps -q web`, then pass that ID to `docker inspect` when verifying the
memory/no-swap limit and health check.
- Around line 63-71: The deployment runbook’s upload test occurs while uploads
are disabled. Update the numbered procedure so the representative upload is
performed only after setting GOGGLES_UPLOADS_ENABLED=1 and recreating the web
service, while keeping the authenticated group overview, delivery, and evidence
checks in the disabled-upload phase.
In `@forensics/ingest.py`:
- Around line 413-427: Move the per-line size validation in parse_jsonl before
the logic that strips and skips blank lines, so whitespace-only lines are
checked against GOGGLES_MAX_JSONL_LINE_BYTES using their raw UTF-8 length.
Preserve the existing AuditLogComplexityError and message, and avoid decoding or
parsing oversized lines.
---
Nitpick comments:
In `@forensics/tests.py`:
- Around line 98-112: Expand HEAVY_BULK_SELECT_COLUMNS in heavy_bulk_selects to
include every field listed in HEAVY_EVENT_FIELDS from forensics/views.py:
raw_kind, raw_context, context_human_action, context_transport, context_engine,
context_group, context_convergence, and context_source, while retaining the
existing columns.
🪄 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: f7907020-e34e-4aca-a117-3385d701c929
📒 Files selected for processing (15)
.env.exampleREADME.mdconfig/settings.pydocker-compose.ymldocs/api-v1.mddocs/deployment.mdforensics/admin.pyforensics/analysis.pyforensics/ingest.pyforensics/management/commands/rebuild_audit_projections.pyforensics/projections.pyforensics/templates/forensics/partials/group_files.htmlforensics/templates/forensics/partials/group_overview.htmlforensics/tests.pyforensics/views.py
| mem_limit: ${GOGGLES_WEB_MEMORY_LIMIT:-16g} | ||
| memswap_limit: ${GOGGLES_WEB_MEMORY_LIMIT:-16g} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the compose file around the cited lines.
file="docker-compose.yml"
wc -l "$file"
cat -n "$file" | sed -n '1,140p'
# Show only the service block if present.
rg -n "mem_limit|memswap_limit|env_file|GOGGLES_WEB_MEMORY_LIMIT|GOGGLES_ENV_FILE" "$file"Repository: marmot-protocol/goggles
Length of output: 3742
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect repository references for the environment-file mechanism and memory limit variable.
rg -n "GOGGLES_ENV_FILE|GOGGLES_WEB_MEMORY_LIMIT|docker compose|docker-compose" .
# Look for docs explaining how the env file is meant to be supplied to Compose.
fd -a -t f 'README*|*.md' .Repository: marmot-protocol/goggles
Length of output: 2126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the documentation around the memory limit configuration and env-file usage.
sed -n '120,150p' README.md
printf '\n---\n'
sed -n '60,90p' docs/deployment.md
printf '\n---\n'
sed -n '1,80p' .env.example 2>/dev/null || trueRepository: marmot-protocol/goggles
Length of output: 4029
Use a Compose-time source for the memory limit. env_file only sets container environment, so changing GOGGLES_WEB_MEMORY_LIMIT inside GOGGLES_ENV_FILE will not update mem_limit or memswap_limit; Compose will keep the default unless the value is present in the shell or Compose .env. Either document that requirement or read the limit from the same config path as the rest of the deployment settings.
🤖 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 `@docker-compose.yml` around lines 40 - 41, Update the memory-limit
configuration used by the Compose service: the ${GOGGLES_WEB_MEMORY_LIMIT:-16g}
interpolation in mem_limit and memswap_limit only reads Compose-time environment
sources, not GOGGLES_ENV_FILE. Either document that this variable must be
defined in the shell or Compose .env, or change the deployment configuration so
both fields read the limit from the same source as the other settings.
| cpus: 2.0 | ||
| pids_limit: 256 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Expose the promised CPU, PID, and log limits as settings.
Only memory and swap are environment-driven; cpus, pids_limit, max-size, and max-file remain hard-coded. This contradicts the stated objective that these operational limits are configurable and prevents incident tuning without editing Compose. Add documented GOGGLES_WEB_* values or narrow the operational contract.
Also applies to: 63-67
🤖 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 `@docker-compose.yml` around lines 43 - 44, The Docker Compose operational
limits are hard-coded despite the configuration contract. Update the service
resource and logging settings associated with cpus, pids_limit, max-size, and
max-file to use documented GOGGLES_WEB_* environment variables with appropriate
defaults, matching the existing memory and swap configuration; alternatively,
narrow the documented contract to exclude these limits.
| 1. Set `GOGGLES_UPLOADS_ENABLED=0` in the production environment. | ||
| 2. Recreate the web service so the changed environment and Compose resource | ||
| limits take effect: `docker compose up -d --build --force-recreate web`. | ||
| 3. Confirm the container has the expected 16 GiB memory/no-swap boundary (or | ||
| the value set in `GOGGLES_WEB_MEMORY_LIMIT`) with | ||
| `docker inspect goggles-web-1` and wait for the health check. | ||
| 4. Exercise an authenticated group overview, delivery tab, evidence tab, and a | ||
| representative upload while watching `docker stats`. | ||
| 5. Set `GOGGLES_UPLOADS_ENABLED=1` and recreate the web service again. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not test ingestion while uploads are disabled.
Step 1 disables uploads, but step 4 still asks operators to exercise a representative upload before step 5 re-enables them. That upload will be rejected and will not exercise bounded ingestion. Move the representative upload after re-enabling uploads, or explicitly label it as an expected rejection test.
🤖 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 `@docs/deployment.md` around lines 63 - 71, The deployment runbook’s upload
test occurs while uploads are disabled. Update the numbered procedure so the
representative upload is performed only after setting GOGGLES_UPLOADS_ENABLED=1
and recreating the web service, while keeping the authenticated group overview,
delivery, and evidence checks in the disabled-upload phase.
| 3. Confirm the container has the expected 16 GiB memory/no-swap boundary (or | ||
| the value set in `GOGGLES_WEB_MEMORY_LIMIT`) with | ||
| `docker inspect goggles-web-1` and wait for the health check. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid hard-coding the Compose container name.
goggles-web-1 depends on the Compose project name and can differ across deployments. Resolve the actual container with docker compose ps -q web before calling docker inspect.
🤖 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 `@docs/deployment.md` around lines 66 - 68, Update the deployment step to avoid
hard-coding `goggles-web-1`: first resolve the running web container ID using
`docker compose ps -q web`, then pass that ID to `docker inspect` when verifying
the memory/no-swap limit and health check.
| if len(parsed_lines) >= settings.GOGGLES_MAX_DUMP_RECORDS: | ||
| raise AuditLogComplexityError( | ||
| f"record count exceeds maximum of {settings.GOGGLES_MAX_DUMP_RECORDS}" | ||
| ) | ||
| # UTF-8 can use more than one byte per character. Avoid encoding an | ||
| # already-obviously-oversized line, then enforce the exact byte ceiling | ||
| # for non-ASCII input before json.loads expands it into Python objects. | ||
| if ( | ||
| len(raw_line) > settings.GOGGLES_MAX_JSONL_LINE_BYTES | ||
| or len(raw_line.encode("utf-8")) > settings.GOGGLES_MAX_JSONL_LINE_BYTES | ||
| ): | ||
| raise AuditLogComplexityError( | ||
| f"line {line_number} exceeds maximum of " | ||
| f"{settings.GOGGLES_MAX_JSONL_LINE_BYTES} UTF-8 bytes" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check raw line size before skipping blank lines.
Because parse_jsonl() strips and continues on empty lines before reaching this block, a whitespace-only line larger than GOGGLES_MAX_JSONL_LINE_BYTES bypasses the advertised per-line bound. Measure the raw line’s UTF-8 size before stripping, or explicitly define whitespace-only lines as exempt.
🤖 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 `@forensics/ingest.py` around lines 413 - 427, Move the per-line size
validation in parse_jsonl before the logic that strips and skips blank lines, so
whitespace-only lines are checked against GOGGLES_MAX_JSONL_LINE_BYTES using
their raw UTF-8 length. Preserve the existing AuditLogComplexityError and
message, and avoid decoding or parsing oversized lines.
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
Adversarial review
The core fix is solid and well-diagnosed. I read every changed file, cross-checked the new PROJECTION_EVENT_FIELDS / EVENT_ROW_FIELDS / EVIDENCE_REF_EVENT_FIELDS field-selection lists against every attribute actually accessed downstream (no missing-field lazy-load regressions found), and ran the full local verification suite: manage.py test forensics.tests (252/252 passing on SQLite), ruff check, manage.py check, and makemigrations --check — all clean, no migration needed as claimed.
Two issues were reproduced empirically rather than just read from the code — a docker-compose.yml variable-scoping gap that silently ignores the documented GOGGLES_WEB_MEMORY_LIMIT override, and an admin change page that still renders an unbounded raw blob. Details inline, along with four smaller suggestions.
What's done well
- The
.only()/.defer()refactor is unusually careful: everyevent.<attr>access inprojections.pyand the query-builder functions inviews.py/analysis.pyis covered by the field lists — no reintroduced lazy-load hydration of the heavy columns, which is easy to get subtly wrong in a refactor this wide. - Locking the "no heavy columns in bulk queries" invariant in with
heavy_bulk_selects()+CaptureQueriesContextacross ingestion, projection, admin, and every group tab/API is a strong regression guard — it tests the actual SQL, not just behavior. - The upload-complexity guardrails are nicely layered (record count → per-line byte size before
json.loads→ nesting depth insidejson.loads) and correctly preserve raw evidence via the existingsave_invalid_uploadquarantine path rather than dropping data. read_upload_bytes's switch to a single bounded.read(max_dump_bytes + 1)is a legitimate simplification — verified it can't under-read for the file-backed/BytesIO-backed uploaded-file types Django uses here.- Docs were updated in lockstep with the actual defaults — I checked every documented number against
config/settings.py/views.pyand they all match.
| mem_limit: ${GOGGLES_WEB_MEMORY_LIMIT:-16g} | ||
| memswap_limit: ${GOGGLES_WEB_MEMORY_LIMIT:-16g} | ||
| mem_swappiness: 0 | ||
| cpus: 2.0 | ||
| pids_limit: 256 | ||
| env_file: ${GOGGLES_ENV_FILE:-.env} |
There was a problem hiding this comment.
Issue: GOGGLES_WEB_MEMORY_LIMIT is silently ignored when set via a custom GOGGLES_ENV_FILE.
mem_limit/memswap_limit use single-$ interpolation, which Compose resolves at compose-file-parse time from the invoking shell's environment or the literal ./.env file Compose auto-loads next to the compose file. The gunicorn flags below use $$-escaped interpolation, resolved inside the container at runtime from whatever file env_file: ${GOGGLES_ENV_FILE:-.env} points to. These are two different resolution mechanisms, and GOGGLES_ENV_FILE only participates in the second one.
Verified with docker compose config (with GOGGLES_WEB_MEMORY_LIMIT=24g set only inside a custom GOGGLES_ENV_FILE target):
mem_limit: "17179869184" # 16 GiB fallback — the 24g override was NOT applied
memswap_limit: "17179869184"
...
--workers $${GOGGLES_WEB_WORKERS:-3} # correctly threads through to the container
Moving the same value into a top-level .env (or exporting it in the shell) resolves correctly to 24 GiB. So the memory ceiling — the headline container-hardening feature of this PR — is only configurable through the file Compose auto-loads, not through the pre-existing GOGGLES_ENV_FILE override (this repo's own CI sets GOGGLES_ENV_FILE: .env.example). An operator who follows .env.example/README and puts GOGGLES_WEB_MEMORY_LIMIT=24g in a custom env file will see the gunicorn knobs take effect while the actual cgroup ceiling silently stays at 16 GiB.
Worth either (a) documenting the constraint explicitly ("GOGGLES_WEB_MEMORY_LIMIT must be exported in the shell or the top-level .env, not merely placed in the GOGGLES_ENV_FILE target"), or (b) restructuring so all six new tunables resolve the same way (e.g. document docker compose --env-file "$GOGGLES_ENV_FILE" as the required invocation).
| def get_queryset(self, request): | ||
| return ( | ||
| super() | ||
| .get_queryset(request) | ||
| .defer( | ||
| "raw_line", | ||
| "raw_event", | ||
| "raw_kind", | ||
| "raw_context", | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Issue: AuditEventAdmin change page still renders an unbounded raw blob, unlike its AuditFileAdmin sibling in this same PR.
This defer() correctly keeps the heavy columns out of the paginated changelist (verified by the new test_admin_changelists_do_not_select_verbatim_evidence_columns). But there's no equivalent of AuditFileAdmin.raw_text_preview (which caps rendering at RAW_TEXT_PREVIEW_CHARS = 2000 specifically because "a single upload can be tens of megabytes"). AuditEventAdmin has no exclude/custom readonly_fields for raw_line, so the auto-generated ModelForm resolves the deferred field and renders it verbatim into an editable <textarea>.
This matters more after this PR than before it: save_invalid_upload() stores the entire raw upload body (up to GOGGLES_MAX_DUMP_BYTES, 50 MiB default) into a single AuditEvent.raw_line for a quarantined file, and this PR adds two new triggers for that path (GOGGLES_MAX_DUMP_RECORDS / GOGGLES_MAX_JSONL_LINE_BYTES violations) on top of the pre-existing ones (bad UTF-8, generic ingest exceptions).
Confirmed directly: opening the admin change page for a quarantined event with a 2 MiB raw_line returns a 2.17 MB HTML response containing the blob verbatim; with a full 50 MiB quarantined upload this would be a ~50 MB single-request response, one .../audit_event/<pk>/change/ click away for any admin operator.
Given AuditFileAdmin was given exactly this treatment in this same PR for exactly this reason, AuditEventAdmin should probably get a bounded raw_line_preview/raw_event_preview readonly display too — or at minimum exclude those fields from the form and link out to audit_file_raw_text/the JSON evidence API the way AuditFileAdmin.events_link links out instead of inlining.
| GROUP_PROJECTION_API_DEFAULT_LIMIT = 100 | ||
| GROUP_PROJECTION_API_MAX_LIMIT = 500 |
There was a problem hiding this comment.
Suggestion: dropping GROUP_PROJECTION_API_DEFAULT_LIMIT/MAX_LIMIT from 500/5000 to 100/500 is a real behavioral change for any existing API consumer that requests limit=5000 (now silently clamped to 500) or relies on the 500-row default. The docs in docs/api-v1.md are updated correctly, but the PR description frames this purely as internal memory-safety — worth a one-line heads-up flagging it as a client-facing breaking change so consumers know to adjust their pagination.
| return NetworkObservation.objects.prefetch_related( | ||
| Prefetch( | ||
| "audit_event", | ||
| queryset=evidence_ref_event_queryset("audit_data_mode"), |
There was a problem hiding this comment.
Suggestion: "audit_data_mode" is already a member of the base EVIDENCE_REF_EVENT_FIELDS tuple that evidence_ref_event_queryset() always includes. Harmless (the dict.fromkeys dedupe absorbs it), but the redundant arg reads as if audit_data_mode is not already covered, which is misleading during future maintenance. Suggest dropping the arg: evidence_ref_event_queryset().
| GOGGLES_MAX_ACTION_EVENTS_PER_REQUEST=50000 | ||
| GOGGLES_AGENT_EXPORT_MAX_EVENTS=50000 | ||
| GOGGLES_FILE_UPLOAD_MEMORY_BYTES=1048576 | ||
| GOGGLES_UPLOADS_ENABLED=1 |
There was a problem hiding this comment.
Suggestion: this PR documents the pre-existing GOGGLES_UPLOADS_ENABLED kill switch for the first time but leaves its sibling GOGGLES_EXPORTS_ENABLED (streaming group export) undocumented in .env.example. Since this section is already being touched, worth adding it too.
| def action_groups_for_api(group: AuditGroup, filters: dict) -> ActionGroupCollection: | ||
| max_events = settings.GOGGLES_MAX_ACTION_EVENTS_PER_REQUEST | ||
| # Pull the newest bounded window, then restore chronological order for the | ||
| # existing grouping semantics. This prevents a single group overview/API | ||
| # request from materializing an unbounded action history. | ||
| events = list( | ||
| filtered_action_events(group, filters).order_by( | ||
| "-wall_time_ms", "-engine_id", "-line_number", "-id" | ||
| )[: max_events + 1] | ||
| ) | ||
| truncated = len(events) > max_events | ||
| if truncated: | ||
| events = events[:max_events] | ||
| events.reverse() |
There was a problem hiding this comment.
Suggestion: bounding the scan to the newest GOGGLES_MAX_ACTION_EVENTS_PER_REQUEST events is a reasonable, documented trade-off — but note it can silently split a single operation_id action group across the truncation boundary. A group with events both inside and outside the newest window will report only the in-window events, with no per-group indicator (only the request-level scan_truncated flag). Not a bug given the documented semantics, but worth a code comment near the human_action_groups_for_group(events) call so a future reader doesn't assume a group's event_count is always complete.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
1 participant
Summary
AuditFile.raw_textand raw event JSON out of bulk delivery, projection, evidence, state, convergence, export-metadata, and admin querysetsAPI compatibility
This intentionally changes projection endpoint pagination from a 500-row default / 5,000-row maximum to a 100-row default / 500-row maximum. Existing consumers must follow
pagination.has_moreand advanceoffsetin batches of at most 500.Root cause
Bulk event querysets used
select_related("audit_file"). BecauseAuditFilecontains the complete uploaded JSONL inraw_text, SQL and Django materialized that large value once for every joined event. A 50 MiB upload joined to roughly 1,000 events can therefore approach 50 GiB before other allocations; the delivery path performed two affected evidence prefetches.Impact
The raw forensic evidence remains available through dedicated detail/download paths. No schema migration or audit-data purge is required. Pathological requests are now bounded in application code, and any future regression is contained within the Goggles cgroup rather than exhausting host RAM and swap.
The representative 5 MiB / 3,000-event stress case improved from approximately 673 MiB RSS and 31 seconds to 166 MiB RSS and 1.9 seconds. The delivery-evidence stress case peaked around 110 MiB RSS after the change.
Validation
just cigit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Performance