Access logging and session replay: the two features left behind at the flip - #137
Open
corrin wants to merge 6 commits into
Open
Access logging and session replay: the two features left behind at the flip#137corrin wants to merge 6 commits into
corrin wants to merge 6 commits into
Conversation
One line per authenticated request on its own `access` logger, carrying the X-Session-Replay-Id that joins a request to its recording. It goes to the console, not v1's rotating access.log: journald already rotates, retains and greps that stream. Two v1 constructs are deliberately not ported. AccessLoggingMiddleware now reads the principal AFTER get_response — v1 checked it on the way in and returned early when anonymous, which under ninja auth (which sets request.user during operation dispatch, after middleware) would log nothing for any /api/** request while a v1-shaped test still passed. And DisallowedHostMiddleware never did anything: process_exception fires only for view exceptions, while DisallowedHost comes out of CommonMiddleware.process_request above it. Django was always returning that 400; only the traceback was the complaint, so a logging filter keeps the record of the probe and drops its traceback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012KsKjSzg9umSt7ad4NHEg7
Chunk payloads go to a private disk root (SESSION_REPLAY_STORAGE_ROOT, which instance.sh has been provisioning at 700 all along); the rows index the store rather than being it, so browsing sessions stay out of every pg_dump. The store itself is shared, not a second copy. Phone-call recordings already kept a metadata row plus a payload on a private root, and v1 wrote that logic twice; apps/core/file_store.py now owns the path-escape guard, the atomic write and the refuse-to-overwrite, and phone_call_service uses it too. Access follows what the business actually had: a staff member may write to their own recording and read none of them. v1 gated the API at office staff but hung the page behind a superuser route, so reads are superuser-only here — a replay is an unredacted video of somebody's screen. Three things v1 lacked or got wrong: - CompanyDefaults.session_replay_enabled, an off-switch that is not a deploy. - The purge is scheduled and now deletes payloads, not just rows. Retention is this feature's only privacy control, so it ships with capture. - The scrubber truncates recordings. v1 blanked storage_path and sha256, which left the unscrubbed payloads on disk AND left the purge unable to find them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012KsKjSzg9umSt7ad4NHEg7
rrweb records every authenticated page and flushes batches every ten seconds. The upload rules are the parts v1 iterated on and they are about not losing a session: a transient failure puts the events back at the FRONT of the buffer (behind later events they would replay out of order), a 409 means the chunk already landed so the sequence advances and recording continues, and only 401/403/404 discard the recording. X-Session-Replay-Id now leaves the client for real — src/api/client.ts had carried a STUB comment where the header belonged, which is why every AppError persisted since the port had a replay column it could never fill. Capture stays off under Playwright-over-ngrok: the E2E run is already bottlenecked on that tunnel and recording every spec would add chunk uploads to it. The new spec clears that opt-out for itself, because the cross-layer path — browser capture to gzip on disk to the events endpoint to the player — is the one no unit test on either side can reach. Two v1 defects not carried over: the admin page destroyed its player by clearing innerHTML, leaking the previous instance's timers and listeners on every reselect; and the wire's opaque JSON is now checked against rrweb's event shape rather than trusted, so an unplayable recording fails visibly instead of showing a blank frame. The one docs line is the spec-count row the status-table gate regenerates because this commit adds a spec file; the rest of that file is another workstream's and is deliberately untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012KsKjSzg9umSt7ad4NHEg7
Both rows named session replay as their blocker, and both blockers are now answered, so the disposition rule applies: the code arrives with the feature. pull_prod_files.sh is ported. Its blocker was the storage decision — payloads live on disk under SESSION_REPLAY_STORAGE_ROOT, so all three destinations exist and the script has somewhere to put them. Two changes from v1: host and instance-user are required arguments rather than defaulting to MSM production, and instance-user is validated as a plain unix account name, because it is interpolated into the remote --rsync-path and is the same escalation pull_prod_backup.sh already guards. Destinations are all resolved before the first byte moves, since a missing root found halfway through leaves a partial pull that looks complete on the directories it did reach. purge_old_session_replays_daily is ported: scheduled 01:30 NZT, and deleting chunk payloads and the recording directory rather than only rows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012KsKjSzg9umSt7ad4NHEg7
Authored by a concurrent session (update-rewrite-status-postrelease), which exited before it could commit. I reviewed the diffs and committed them rather than leave the work uncommitted in a shared worktree; I did not write them. What it does: rewrite-status.md becomes forward-looking, holding tasks only, with what shipped moved to rewrite-history.md; cutover-checklist.md becomes cutover-record.md in all but filename, because the flip ran on 2026-08-29 and a checklist nobody ticked on the night would lie in a new way; CLAUDE.md now says work is tracked in both Jira and rewrite-status.md, and points at release-process.md rather than the cutover file. What I checked, since I did not author it: every redirected comment resolves to content that really moved — the DRAFT/WIP divergence, the unweighted billable_percentage and total_revenue quirks, the public-holiday working-day split and the quote-event double count are all in rewrite-history.md now; the two renamed doc targets (development_session.md, release-process.md) exist; and the test docstring's "a client error IS an AppError" item is really in rewrite-status.md. The five apps/accounting and two test/spec edits are comment-only. One line is mine: I deleted the "Re-dispose the two blocked-by: rows that named session replays" task, which 7ff50da completed — that file only shrinks, and finished work is deleted the moment it is finished. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012KsKjSzg9umSt7ad4NHEg7
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 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 |
Links rather than restates: the ticket is the authority, this file just says enough for a session to pick it up cold and to know not to start it yet — it overturns the layout CLAUDE.md documents, so its owner-ratified ADR gates everything after it. The scoping caution is worth carrying here because it changes how big the work looks. The draft justified the epic as breaking seven ORM cycles; traversing every concrete cross-app relation in the app registry finds exactly one (accounts <-> job, via Staff.default_labour_subtype). The other six are one-way, and purchasing -> job, accounting -> job and quoting -> company already point where the target architecture wants them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012KsKjSzg9umSt7ad4NHEg7
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Ports the last two features that were left behind at the flip: the per-request
access log, and session replay end to end. Both were recorded as deferred in
docs/rewrite-status.md, and access logging was the first item on thepost-cutover queue.
Access logging
One line per authenticated request on its own
accesslogger, carrying theX-Session-Replay-Idthat joins a request to its recording.It goes to the console, not v1's rotating
access.log.config/settings.pyalready states that journald is the sink, and journald gives the rotation and
retention v1 got from
ConcurrentRotatingFileHandler— a file handler wouldonly put a second copy on disk for an operator to find and prune.
Two v1 constructs are deliberately not ported, both verified broken rather than
assumed:
request.userbefore calling the view. Under ninja auth — whichsets
request.userduring operation dispatch, after every middleware — thatlogs nothing for any
/api/**request, i.e. for the whole application, whilea v1-shaped test still passes. v2 reads the principal after
get_response.DisallowedHostMiddlewarenever did anything.process_exceptionfiresonly for exceptions raised by the view, but
DisallowedHostcomes out ofCommonMiddleware.process_requestabove it. Django was always returning that400; only the traceback was ever the complaint.
apps/core/logging_filters.pykeeps the record of the probe and strips its traceback. The test fails without
the filter — I checked.
v1's JWT re-authentication block went with it (v2 is cookie-authenticated), and
with it a bare
except Exception: pass.Session replay
Chunk payloads go to a private disk root (
SESSION_REPLAY_STORAGE_ROOT, whichinstance.shhas been provisioning at 700 all along). The rows index the storerather than being it, so browsing sessions stay out of every
pg_dump.The store is shared, not a second copy. Phone-call recordings already kept a
metadata row plus a payload on a private root, and v1 wrote that logic twice.
apps/core/file_store.pynow owns the path-escape guard, the atomic write andthe refuse-to-overwrite, and
phone_call_serviceuses it too (ADR 0039).Access follows what the business actually had. A staff member may write to
their own recording and read none of them. v1 gated the API at office staff but
hung the page behind a superuser route; a replay is an unredacted video of
somebody's screen, so reads are superuser-only here.
Upload rules are about not losing a session, and are the part v1 iterated on:
a transient failure puts events back at the front of the buffer (behind later
events they would replay out of order), a 409 means the chunk already landed so
the sequence advances and recording continues, and only 401/403/404 discard the
recording.
Three things v1 lacked or got wrong:
CompanyDefaults.session_replay_enabled— an off-switch that is not a deploy.v1 had none outside a DEV-only E2E flag.
Retention is this feature's only privacy control, so it ships with capture.
storage_pathandsha256,which left the unscrubbed payloads on disk and left the purge unable to
ever find them again.
Also fixed on the way past:
src/api/client.tscarried a STUB comment where theX-Session-Replay-Idheader belonged, which is why everyAppErrorpersistedsince the port had a replay column it could never fill. And v1's admin page
"destroyed" its player with
innerHTML = '', leaking the previous instance'stimers and listeners on every reselect.
Capture stays off under Playwright-over-ngrok — the E2E run is already
bottlenecked on that tunnel. The new spec clears that opt-out for itself.
Operational assets unblocked
Both
blocked-by:rows inv1-disposition.mdthat named session replay are nowported: the purge beat entry, andpull_prod_files.sh, whose only blocker wasthe storage decision. It takes host and instance-user as required arguments
rather than defaulting to MSM production, and validates instance-user as a plain
unix account name — it is interpolated into the remote
--rsync-path, the sameescalation
pull_prod_backup.shalready guards.Docs commit authored by another session
80b7d48is the work of a concurrent session that exited before it could commit;I reviewed and committed it rather than leave it in a shared worktree. It makes
rewrite-status.mdforward-looking and turnscutover-checklist.mdinto arecord now the flip has run. I verified every redirected comment resolves to
content that really moved before committing it.
Verification
Commit and push tiers green (ruff, mypy strict, import-linter, find-duplicates,
deptry, exported schema, status table, code-quality, delta goldens, frontend
lint/format/boundary/type-check/audit, generated-client-current, server suites,
makemigrations). Python suite green; 613 frontend unit tests green, 6 of them new
and covering the upload rules above.
Not yet run:
frontend/tests/e2e/admin/session-replay.spec.ts. It drives thecross-layer path no unit test on either side can reach — browser capture, chunk
upload, gzip on disk, events endpoint, player mount — plus a second test toggling
session_replay_enabledoff. Per CLAUDE.md this slice is not done until thatpasses, so please treat this PR as unverified end-to-end until it does.
🤖 Generated with Claude Code
https://claude.ai/code/session_012KsKjSzg9umSt7ad4NHEg7