Add expires_at timestamp to Firestore salt documents

[cmraible]

closes https://linear.app/ghost/issue/NY-1083/add-an-expire-at-timestamp-to-future-firestore-keys

Summary

• Adds an expires_at field to new Firestore salt documents, computed as the key's date + 2 days at midnight UTC
• This is a stepping stone toward using Firestore's native TTL deletion, which will eventually replace the manual cleanup scheduler added in Updated Firestore cleanup job to fetch and delete in batches of 500 #582
• expire_at is a Firestore-only storage concern — not added to SaltRecord or ISaltStore

Changes

• FirestoreSaltStore.ts: Added private getExpireAt(key, fallbackDate) helper that parses the date from the key format salt:{date}:{siteUuid} and returns that date + 2 days. Falls back to fallbackDate + 2 days for unexpected key formats. Modified set() to include expires_at when writing documents.
• FirestoreSaltStore.test.ts: Added 6 integration tests covering set(), getOrCreate(), fallback behavior, and verifying get()/getAll() don't leak expires_at into SaltRecord.

Test plan

• expires_at written correctly via set() (key date + 2 days)
• expires_at written correctly via getOrCreate()
• Fallback to created_at + 2 days for unexpected key formats
• get() does not include expires_at in returned SaltRecord
• getAll() does not include expires_at in returned records
• All 414 unit tests pass
• All 108 integration tests pass
• Lint clean

[coderabbitai]

Walkthrough

This pull request adds expiration handling to FirestoreSaltStore. It introduces EXPIRE_AT_BUFFER_DAYS = 2 and a private getExpireAt(key, fallbackDate) that parses keys of the form salt:{YYYY-MM-DD}:{siteUuid} to compute an expire_at timestamp (UTC midnight plus two days), falling back to fallbackDate + 2 days when parsing fails. The set() method now persists expire_at alongside created_at and salt. Integration tests were added to cover set(), getOrCreate(), malformed-key fallback behavior, and to verify expire_at is not exposed by get()/getAll().

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: adding an expire_at timestamp to Firestore salt documents, which aligns perfectly with the primary objective and changeset.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, providing context, implementation details, test coverage, and risk assessment that all align with the code changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch chris-ny-1083-add-an-expire_at-timestamp-to-future-firestore-keys

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

❌ Staging deployment failed (tree: f8571daa77594d03840e589c05dd0e74dde14ee7)

[ErisDS]

🤖 Velo CI Failure Analysis

Classification: 🔴 HARD FAIL

• Workflow: Deploy Staging
• Failed Step: Set up job
• Run: View failed run
  What failed: Failed to resolve action download info due to Service Unavailable errors
  Why: The failure is caused by repeated Service Unavailable errors when trying to download the required actions, which indicates an infrastructure issue with the service providing the action downloads. This is not a code-related problem, so it is classified as a HARD failure.
  Action:
  This is likely a temporary issue with the service providing the action downloads. Try rerunning the workflow, and if the problem persists, the infrastructure team should investigate the root cause of the Service Unavailable errors.

[github-actions]

✅ Deployed to staging (tree: f8571daa77594d03840e589c05dd0e74dde14ee7)

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/services/salt-store/FirestoreSaltStore.ts (1)

196-200: Treat the TTL switchover as a small schema migration.

This only populates expires_at on newly created documents. Before retiring cleanup(), make sure the Firestore TTL policy targets the renamed expires_at field and backfill older salt docs, otherwise pre-existing records will never age out automatically.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/salt-store/FirestoreSaltStore.ts` around lines 196 - 200, The
current write in FirestoreSaltStore (docRef.create with expires_at from
getExpireAt) only sets expires_at for new documents; treat this as a small
schema migration: update the Firestore TTL policy to target the renamed
expires_at field and add a backfill step to populate expires_at for existing
salt documents (e.g., run a one-time migration job inside the FirestoreSaltStore
migration path that reads existing records, computes getExpireAt(key, now) or
appropriate original-creation-based TTL, and writes expires_at back to those
docs), and keep cleanup() until backfill is complete and verified so older
records will age out automatically.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/services/salt-store/FirestoreSaltStore.ts`:
- Line 28: The getExpireAt method in FirestoreSaltStore currently accepts
non-canonical date strings via new Date(dateStr); tighten validation by first
matching the key-derived date segment against a strict /^\d{4}-\d{2}-\d{2}$/
regex, then parse year/month/day integers and construct a canonical Date using
new Date(Date.UTC(year, month-1, day)) (or new Date(year, month-1, day)) and
verify the constructed date's year/month/day equal the parsed values to reject
invalid dates like 2026-02-30; if validation fails, fall back to the existing
behavior (use current date minus EXPIRE_AT_BUFFER_DAYS) so getExpireAt preserves
the documented YYYY-MM-DD contract.

---

Nitpick comments:
In `@src/services/salt-store/FirestoreSaltStore.ts`:
- Around line 196-200: The current write in FirestoreSaltStore (docRef.create
with expires_at from getExpireAt) only sets expires_at for new documents; treat
this as a small schema migration: update the Firestore TTL policy to target the
renamed expires_at field and add a backfill step to populate expires_at for
existing salt documents (e.g., run a one-time migration job inside the
FirestoreSaltStore migration path that reads existing records, computes
getExpireAt(key, now) or appropriate original-creation-based TTL, and writes
expires_at back to those docs), and keep cleanup() until backfill is complete
and verified so older records will age out automatically.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0e996f34-1102-4776-b95d-3c2cac436e4b

📥 Commits

Reviewing files that changed from the base of the PR and between 13f5b16 and 1fcadb5.

📒 Files selected for processing (2)

• src/services/salt-store/FirestoreSaltStore.ts
• test/integration/services/salt-store/FirestoreSaltStore.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• test/integration/services/salt-store/FirestoreSaltStore.test.ts

[troyciesco]

🔥 🧂

[troyciesco]

thought: my personal preference is to infer return types whenever possible, though when i just when to search for an article to explain my reasoning i was surprised to find a suggestion that they should be explicit to help ai understand better.

another thought: it'd be neat if we could type the key in the expected format instead of just string

(neither of these thoughts need any action)

[troyciesco]

(fwiw, i personally still like inferred return types even if ai doesn't lol)