Skip to content

feat: retention flag, narrow release - #305

Merged
spokvulcan merged 1 commit into
mainfrom
feat/retro-retention-narrow
Sep 4, 2026
Merged

feat: retention flag, narrow release#305
spokvulcan merged 1 commit into
mainfrom
feat/retro-retention-narrow

Conversation

@spokvulcan

@spokvulcan spokvulcan commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Closes #285. Narrow half of the rooms.retained migration (ADR-0019, spec §3 step 3).

Release ordering constraint

This must deploy separately from #284 and only after the backfill has run to completion in prod. Every merge to main deploys to production, so the merge is the ordering unit: #304 (widen) merged and deployed first, the backfill ran, and this PR merges second. Both may appear in the same release-please changelog; that is fine.

Backfill state before this merge (2026-09-04): backfillRoomsRetained stamped 78 legacy rooms and a follow-up dry run reported 0 unstamped. The Convex push validates data at rest, so the narrowed schema would refuse to deploy if any room lacked the field.

This PR removes the backfill migration (a required field makes its undefined index query a type error, and its job is done). If the narrow push were ever to abort on an unstamped row, recovery is a revert of this PR, not a re-run.

What changed

  • convex/schema.ts: retained is a required boolean. by_activity is dropped.
  • convex/model/cleanup.ts: the sweep reads by_retention_activity with retained = false and lastActivityAt before the cutoff. Batch size and scheduling are unchanged.
  • convex/migrations.ts: backfillRoomsRetained removed.
  • convex/retention.test.ts: a retained room outlives five quiet days, a non-retained room is scheduled for deletion, and retention is the only discriminator (non-retained rooms with roomType canvas and undefined are treated identically).
  • Thirteen raw room inserts across nine convex test files, plus two Doc<"rooms"> literals in src/ (demo provider and a hook test), gained retained: false.

https://claude.ai/code/session_01LTuViwbbbictkKbzyZCBaa

Narrow half of the rooms.retained migration (ADR-0019). The field becomes
required, by_activity is dropped, and the sweep reads by_retention_activity
for non-retained rooms past the cutoff. The completed backfill migration is
removed; every raw room insert in tests and the two Doc<"rooms"> literals
in src/ carry retained: false.

Must ship in a separate release from #284, after the backfill has run to
completion in prod.

Claude-Session: https://claude.ai/code/session_01LTuViwbbbictkKbzyZCBaa
@vercel

vercel Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
agilekit Error Error Sep 4, 2026 4:44pm UTC

@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown

Review: narrow half of rooms.retained migration

Clean, mechanical migration PR — matches ADR-0019 §3 step 3 exactly (widen → backfill → narrow, by_activity dropped only once the compound index is live). A few notes, nothing blocking:

Verified correct

  • convex/model/cleanup.ts: the sweep now reads by_retention_activity with q.eq("retained", false).lt("lastActivityAt", cutoffTime) — matches the ADR's spec verbatim, and by_activity is gone from the schema with no remaining references anywhere in convex/ or src/ (checked via grep).
  • convex/schema.ts: retained is required; comment accurately reflects that both new writes and the completed backfill now guarantee every row has it.
  • Removing backfillRoomsRetained from migrations.ts is correct now that the field is required — a query for q.eq("retained", undefined) would no longer type-check, and its job is done. No cron or other caller references it.
  • All raw ctx.db.insert("rooms", …) call sites across the test suite (and the two Doc<"rooms"> literals in src/) were updated with retained: false, so nothing will fail schema validation at runtime. I also checked convex/analytics.seeds.ts's seedRoom helper (used by the new retention.test.ts but not touched in this diff) — it already stamps retained: false, presumably from the widen release (Retro: retention flag, widen release and backfill migration #284), so it's consistent.
  • convex/retention.test.ts rewrite is a solid improvement: it now tests behavior (a retained room survives the sweep, a non-retained one is scheduled, roomType is not a discriminator) rather than migration mechanics that no longer exist. The old backfill-draining test helpers (pendingBackfills, drainScheduled) were correctly removed along with the migration they tested.

Worth double-checking before merge (not code issues)

  • The release-ordering constraint (must ship after 2.10.0/Retro: retention flag, widen release and backfill migration #284 is deployed) is entirely a manual/process guarantee — nothing in CI enforces it. The PR body already flags this prominently with a clear revert-based recovery path if the narrow push ever aborts on an unstamped row, so this seems well understood — just flagging it's easy to forget if this PR sits in a queue for a while.
  • Worth a final prod check right before merging that the "0 unstamped" dry-run result is still 0 — if any new rooms were created since 2026-09-04 through a path that doesn't stamp retained (shouldn't happen after Retro: retention flag, widen release and backfill migration #284, but it's cheap insurance given this is a required-field migration).

No security, performance, or test-coverage concerns — this is a narrow, well-scoped schema change with proportionate test updates.

@spokvulcan
spokvulcan merged commit e1cdd6c into main Sep 4, 2026
3 of 4 checks passed
@spokvulcan
spokvulcan deleted the feat/retro-retention-narrow branch September 4, 2026 16:47
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.

Retro: retention flag, narrow release

1 participant