Skip to content

fix(platform): restore three dropped retention behaviours - #3143

Merged
Israeltheminer merged 1 commit into
mainfrom
fix/retention-dropped-sweeps
Sep 3, 2026
Merged

fix(platform): restore three dropped retention behaviours#3143
Israeltheminer merged 1 commit into
mainfrom
fix/retention-dropped-sweeps

Conversation

@Israeltheminer

Copy link
Copy Markdown
Collaborator

An administrator could set an externalConversations retention window and nothing ever deleted. Email bodies were kept forever. Two smaller windows were also wrong.

Refs #3142.

Why

The category is exposed and validated in three places — domains/retention/routes.ts:206, domains/governance/settings-tail.ts:317, lib/shared/schemas/retention.ts:20 — and domains/retention/service.ts never read app.conversations. app.conversation_messages.content is NOT NULL text holding inbound and outbound email bodies, plus a metadata jsonb of envelope detail, so this is the heaviest correspondent-PII surface retention governs.

Two windows were also off, neither with a note:

  • login_block_counters aged at 90 days where 0.4 used one 30-day cutoff for attempts and counters together. Those rows hold email and last_ip.
  • two_factor_attempts lost its sweep entirely. 0.4 aged it alongside login attempts, added deliberately because "users who failed 2FA and never came back left a permanently-stuck row".

What changed

sweepExternalConversations ages by app.conversations.last_message_at_ms, served by the existing conversations_org_last_message index. Same shape as the sweeps beside it: BATCH_LIMIT bound, the org-hold skip, the two-pass grace through lifecycle_status/status_changed_at_ms, wired into Phase2Stats and sweepOrgPhase2.

A conversation that never received a message has no timestamp to age against and is deliberately not a candidate.

jobs/task-list.ts now uses one 30-day cutoff for all three auth tables.

Beyond the brief, because leaving it would have made the sweep worse than not having it: deleting a conversation would have orphaned its mail attachments permanently. file_metadata.conversation_id has no foreign key, and the mail lane stamps source with the connector slug, so sweepTempFiles — which takes only source user/agent — can never collect them. The window would have destroyed the email body and left the attachment bytes live behind a dangling pointer. The index and the blob helper both already existed. A file promoted into a Document is left alone, under the same document_id IS NULL guard sweepTempFiles uses.

Decisions

two_factor_grace gets no sweep. An absent row reads as "no anchor yet", and evaluateTwoFactorEnforcement then mints a fresh now + gracePeriodDays — so ageing it out would hand a user who already burned their grace a new window for staying away. It carries no PII beyond user_id, and member removal already deletes it. A mutation locks this in, so nobody adds the sweep later by symmetry.

30 days for counters, not 90. listBlockCounters has no time window, so nothing reads a counter beyond 30 days, and the lockout itself lives on login_attempts.locked_until rather than a counter bucket — so the shorter window releases no lockout early.

Risk

The retention sweeps delete blobs best-effort, so a failed blob delete is logged and the row is destroyed anyway: the file row goes while the bytes may remain. I kept that idiom to match the neighbouring sweeps rather than diverge inside one file, but it is knowingly weaker than the strict posture #3135 gives erasure. Observed directly — with no object store configured the sweep warns and continues. Worth a deliberate decision rather than inheriting it.

Tests

Three assertions in checkRetention, and eleven mutations, each named with the assertion that went red:

Mutation Went red
both hold guards disabled frozenConv=0 (want 1)
only the outer sweep guard disabled stayed green — the in-function guard is load-bearing
age by created_at_ms destroyed the never-messaged conversation
foreign key re-created without ON DELETE CASCADE sweep failed, bodies survived
attachment cascade removed attachment=1 (want 0)
counters back to 90 days both counter rows survived
2FA delete neutered both 2FA rows survived
a two_factor_grace age delete added grace=0 (want 1)
attempts window widened to 90 days both attempt rows survived
pass-one flip disabled firstPass=unmarked
grace clock dropped from the delete firstPass=ROW GONE — deleted inside the window

The grace path had no coverage before this — the existing checkRetention runs deletionGraceDays: 0 throughout, so sweepContacts's grace path is untested too. Rather than ship an untested path, there is now an assertion for it.

Proven with a driver importing the real runRetentionCleanup and the real task-list handler against a throwaway database, because re-running the full suite eleven times was not viable. Throwaway role and databases dropped afterwards.

Scope

No per-row audit rows. 0.4's deleteExpiredExternalConversation emitted external_conversation.retention_deleted, and no 0.5 sweep emits per-row audit events, so this matches its neighbours instead of introducing a lone pattern. Retention emitting no audit rows at all is tracked in #3142.

chatFilterEvents is the same defect shape and is left alone: it has no production writer yet, so it should land with one.

Gate: typecheck, oxlint --type-aware, oxfmt --check green.

The Postgres port left three data-retention behaviours behind. An
administrator could configure a window and nothing enforced it.

Sweep external conversations. The `externalConversations` category was
validated and exposed but no sweep ever read `app.conversations`, so
inbound and outbound email bodies were kept forever. The new phase-2
sweep ages conversations by `last_message_at_ms`, honours the org legal
hold and the two-pass deletion grace, and stays inside `BATCH_LIMIT`.
Message rows ride the parent's `ON DELETE CASCADE`. Stored mail
attachments do not, and no other sweep can reach them — the mail lane
stamps `file_metadata.source` with the connector slug — so the sweep
removes them too, leaving files promoted into a Document to the
`documents` category.

Sweep two-factor attempts. `app.two_factor_attempts` is cleared on a
successful verify and on member removal, so a user who failed 2FA and
never came back left a permanently stuck row. It now ages out on the
same 30-day window as login attempts. `app.two_factor_grace` is left
alone on purpose: an absent row mints a fresh grace window, so ageing
it out would reward staying away.

Restore the 30-day window for login block counters. The port used 90
days for counters and 30 for attempts, holding `email` and `last_ip`
three times longer than the attempts they summarise. Nothing needed the
longer window: the admin view reads the most recent 200 counters with no
time bound, and the lockout lives on `login_attempts.locked_until`.

`integration-check.ts` asserts all three against real Postgres, plus the
two-pass grace path.
@Israeltheminer
Israeltheminer force-pushed the fix/retention-dropped-sweeps branch from aaa0ca0 to c333bd0 Compare September 3, 2026 12:46
@Israeltheminer

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main, and the rebase needed real work rather than conflict resolution.

deleteBlobBestEffort is gone. #3140 replaced it with a refcounted releaseRefs, so the attachment cascade is re-pointed at that, following how sweepTempFiles now uses it. Three things come with it: an indexed attachment's corpus rows die with the file, a blob another holder still references survives, and a failed delete keeps the row for the next sweep instead of leaking the bytes.

That changed one behaviour worth calling out. A conversation whose attachment could not be released is now held back too — deleting the parent would orphan the file row behind a dangling pointer, which is the exact failure this cascade was added to prevent.

A test-isolation bug of mine, caught by comparing against a baseline. The run showed a governance tail: retention catalog failure. Rather than assume it was pre-existing, I ran unmodified main on the same box: 370/376, and that check passed. So it was mine — the two-pass grace fixture writes deletionGraceDays: 30 and left it there, so a later governance check found its own policy summary listing a reduction it never set. The fixture now restores the policy it inherited, with a comment saying why.

After the fix, on a real Postgres and MinIO:

result
this branch 373/379
unmodified main 370/376

Same six failures on both — the warm-MinIO bucket collision, two yt-dlp-dependent probes and three agent-lane probes. +3 checks, +3 passes, no new failures.

All three assertions pass:

PASS  retention: external conversations age by last message, org hold freezes
PASS  retention: external conversations honour the two-pass deletion grace
PASS  retention: sign-in throttling TTL is one 30-day window, grace kept

@Israeltheminer
Israeltheminer merged commit 899fcc0 into main Sep 3, 2026
43 checks passed
@Israeltheminer
Israeltheminer deleted the fix/retention-dropped-sweeps branch September 3, 2026 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant