Lane 3: instance retention gets an absolute cap, not just a default (D24, #729) #763
Loading…
Reference in a new issue
No description provided.
Delete branch "build/lane3-retention-cap"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Lane 3's first PR: D24, rows
IS-311andIS-312. This is about what actually gets deleted and when, so it is worth reading slowly.What was wrong
get_effective_retentiontook a campaign's override verbatim, floored bymax(1, …)and bounded above by nothing. So a campaign could keep audio longer than the instance, and the instance setting was a default in name and nothing in practice.D24: instance retention becomes two numbers — a default every campaign starts from and an absolute cap no campaign may pass, with the campaign's own form naming both so a GM can see the room they have.
Retention is a pair, not a number
This is the part a one-line summary hides. A campaign can keep audio longer by changing the mode rather than the number, so a cap expressed as a number is not a cap. Both sides order by how long the thing is kept:
One comparison helper per side, used by all three checks, so the ordering is stated once. An unknown mode raises rather than being given an invented position.
Two rules that protect people's recordings
An absent cap means no cap. The
max_*keys are omitted entirely until an administrator sets one, so an instance that never does keeps the blob it has today byte for byte and behaves exactly as it does now. A cap that appeared retroactively would start deleting people's audio, which is the one outcome this must not have.A stored over-cap override is read as the cap and never rewritten. The clamp is in
get_effective_retention, which both consumers already go through, so the nightly sweep obeys it. The row keeps the GM's stated preference for the day the cap is raised again.The test that matters drives this against the real task, over the same committed rows: with no cap, a campaign asking to keep for good keeps its audio; an administrator then sets a one-day cap, nothing about the campaign changes, and the same sweep trashes it — while
Campaign.audio_retention_modeis stillretain_indefinitelyon the row and the file is still on disk, because an expiring window is never itself destructive (#427).A subtlety worth calling out
trash_after_processingcarries a live day count: the sweep usesaudio_daysas the window for a run whose processing failed, in every mode butretain_indefinitely. So a campaign binning on success with a ninety-day debug grace would outlive a thirty-day cap precisely when the audio is the only record of the session. The audio clamp therefore narrows the number as well as the mode. Transcripts need no counterpart, and the docstrings say why.Refusals rather than silent reinterpretation
PATCH /api/campaigns/{id}refuses an over-cap override and names the cap;PUT /api/admin/settings/retentionrefuses a default above its own cap, validating both sides before writing either, since the two settings are separate rows. The Pydantic schema stays unbounded on purpose — the cap is a runtime setting, soField(le=…)cannot express it.CampaignResponsegains read-onlyretention_capsso CAMP-13's form can name the ceiling.A clone arrives already capped
Added after review: "from one of yours" copies the four retention columns straight off the source and never meets the
PATCHcheck, so a template could hand over-cap numbers to a new campaign. Nothing was ever kept longer — the read clamp holds — but the new campaign's form would have shown a span the server refuses, which is this PR's own fault reappearing one path over. A null is left null, because writing the cap into an inheriting field would turn an inheriting campaign into an overriding one.The privacy page, and one deliberate departure from my own instruction
I told the delegate to restore the drawing's "A GM can keep less than this. Nobody can keep more." unconditionally. It is conditional instead, and that is the right call.
That sentence sits under two bullets that print the default. The guarantee this PR creates is about the cap. They coincide only when cap equals default — and D24's own model, "a campaign lands anywhere between them", makes cap above default the normal configuration. On every instance the day this ships, no cap is set at all, so the unconditional sentence would have been false everywhere immediately. The file's existing comment warned about exactly this hazard, and that argument does not weaken because enforcement now exists.
So the drawn sentence renders when the cap is the default, and the honest one stays otherwise.
/public/policiescarries the cap to make the distinction possible — four additive fields, no more sensitive than the defaults already beside them, and arguably the reader's business, since it is the longest anyone on that server can keep their voice.Checks
Ruff 0.4.4 clean (
format --checkandcheck). Backend 3976 passed, 13 skipped; the newtest_retention_cap.pyis 94 of them, including both comparators exhaustively across every mode pair in both directions. Frontend 1398 tests, lint clean. No migration — the caps are keys inside the existingapp_settingsJSON rows.🤖 Generated with Claude Code
`get_effective_retention` took a campaign's override verbatim, floored by `max(1, …)` and bounded above by nothing, and `CampaignUpdate` only enforced `ge=1`. A campaign could therefore keep audio *longer* than the instance did, which made the instance setting a default in name and nothing in practice. D24 (`IS-311`, `IS-312`) splits it in two: a default every campaign starts from, and an absolute cap no campaign may pass. The crux is that retention is a `(mode, quantity)` pair, not a number. A campaign can keep audio longer by changing the mode rather than the number, so a cap of "30 days" alone is not a cap at all. Both sides are ordered by how long the thing is actually kept — audio: trash_after_processing < retain_days(N) < retain_indefinitely transcript: retain_months(N) < retain_indefinitely — by exactly one comparator per side, used by all three callers: the read clamp, the campaign write check and the admin write check. Three open-coded copies of an ordering is how two of them end up disagreeing, and the copy that matters is the one the nightly sweep reads. **An absent cap means `retain_indefinitely`**, so every existing instance behaves precisely as it does today until an administrator sets one. A cap that appeared retroactively would start binning people's recordings on the next Beat tick, which is the one outcome this must not have. The cap lives in new keys inside the two `app_settings` JSON blobs, so there is no migration. The clamp is on read, in `get_effective_retention`, because that is what both consumers obey — `apply_post_processing_audio_retention` and the `enforce_retention` sweep, which is the code that actually deletes. A campaign row that already exceeds a newly-set cap is *read* as the cap and is not rewritten: the GM's stated preference is worth keeping for the day the cap is raised again. One subtlety the ordering alone would miss is handled beside it: `trash_after_processing` still carries a day count, which the sweep uses as the window for a run whose processing *failed*, so that number is narrowed to a day-shaped cap too. Writes refuse rather than reinterpret. `PATCH /api/campaigns/{id}` answers 400 with a sentence naming the cap, checked against the pair the campaign would end up with (a PATCH is partial, so a lone day count over a stored mode is caught); `PUT /api/admin/settings/retention` answers 422 for a default above its own cap, naming both. The Pydantic schemas stay unbounded on purpose — the cap is a runtime setting, so a static `Field(le=…)` cannot express it. The admin request takes the cap fields optionally, so a client that predates D24 keeps whatever cap is configured rather than clearing it by omission, and both halves are validated before either row is written. `CampaignResponse.retention_caps` carries the cap read-only, which is what CAMP-13 needs to name both numbers so a GM can see the room they have. It is not redacted for players: it is the rule the GM is being held to. `Privacy.jsx` gets the drawing's "A GM can keep less than this. Nobody can keep more." back, and the comment explaining why it could not be said goes. It is printed where it is true rather than unconditionally — the guarantee is about the cap, and on a server that has set none, or that has left room above its default, a GM can still keep more than the lines above say. Printing it anyway would put a promise the server does not enforce on the one page whose whole purpose is not to drift from what the server does, which is the failure `IS-387` exists to prevent. `GET /public/policies` carries the cap for that reason; it is the longest anyone here can keep a reader's voice, which is the reader's business. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>