feat: data deletion retention#363
Merged
tipusinghaw merged 6 commits intofeat/oidc-main-syncfrom Apr 28, 2026
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a comprehensive data purge system enabling automatic record deletion via two independent scheduling mechanisms: NATS JetStream-based event-driven purging and cron-based periodic scanning. It adds environment configuration, decorator-driven purge task scheduling on DIDComm/OpenID4VC endpoints, worker-based message processing, webhook notifications, and configuration validation with NATS connectivity checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant SchedulePurge as `@SchedulePurge`<br/>Decorator
participant NatsPurgeScheduler
participant JetStream
participant PurgeWorker
participant DeleteService as DeleteRecord<br/>Service
participant WebhookService
Controller->>Controller: Handle request
Controller-->>SchedulePurge: Method returns result
SchedulePurge->>NatsPurgeScheduler: Extract recordId, get scheduler
SchedulePurge->>NatsPurgeScheduler: schedulePurge(recordType, recordId, tenantId)
NatsPurgeScheduler->>NatsPurgeScheduler: Compute scheduledAt from TTL
NatsPurgeScheduler->>JetStream: Publish PurgeJob with schedule headers
JetStream->>JetStream: Queue scheduled message
par Worker Message Processing
PurgeWorker->>JetStream: Consume messages
JetStream-->>PurgeWorker: Deliver scheduled PurgeJob
PurgeWorker->>PurgeWorker: Parse and validate job
PurgeWorker->>DeleteService: deletePurgeRecord(recordType, recordId)
DeleteService->>DeleteService: Route to module backend
DeleteService-->>PurgeWorker: Record deleted / not found
PurgeWorker->>WebhookService: sendPurgeWebhook(status)
WebhookService->>WebhookService: Retry with backoff
WebhookService-->>PurgeWorker: Notification sent
PurgeWorker->>JetStream: Acknowledge message
end
SchedulePurge-->>Controller: Return original result
sequenceDiagram
participant CronScheduler as CronPurgeScheduler
participant CronJob
participant Agent
participant TenantModule
participant Storage as Storage Backend
participant DeleteService as DeleteRecord<br/>Service
participant WebhookService
CronScheduler->>CronJob: Schedule on cron expression
par Periodic Scan Loop
CronJob->>CronScheduler: Invoke runScan()
alt Shared Mode (Multi-tenant)
CronScheduler->>TenantModule: Get all tenants
TenantModule-->>CronScheduler: Tenant list
loop For each tenant
CronScheduler->>CronScheduler: Get tenant agent
CronScheduler->>CronScheduler: Scan each record type
end
else Dedicated Mode
CronScheduler->>Agent: Scan using provided agent
end
loop For each expired record
CronScheduler->>Storage: Query by recordType & TTL cutoff
Storage-->>CronScheduler: Record IDs
CronScheduler->>DeleteService: deletePurgeRecord(recordType, recordId)
DeleteService-->>CronScheduler: Success / NotFound
CronScheduler->>WebhookService: sendPurgeWebhook(status)
WebhookService-->>CronScheduler: Webhook sent
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Author
|
Review @coderabbitai |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (15)
src/purge/PurgeConfigValidator.ts-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorFix the import ordering lint error.
The reported
import/orderviolation can blockyarn validate. Move the local type import into the expected import group/order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/purge/PurgeConfigValidator.ts` at line 3, The import ordering lint failed because the local type import "PurgeConfig" is in the wrong group; move the line "import type { PurgeConfig } from './PurgeTypes'" into the local/project imports group so it follows external package imports and precedes sibling/relative imports per the project's import/order rules (update the imports in PurgeConfigValidator.ts accordingly).src/purge/PurgeConstants.ts-50-53 (1)
50-53:⚠️ Potential issue | 🟡 MinorRun Prettier on the backoff array.
Line 51 has the reported spacing issue and can fail the formatter check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/purge/PurgeConstants.ts` around lines 50 - 53, The backoff array constant PURGE_CONSUMER_BACKOFF_NS has inconsistent spacing that triggers Prettier; reformat the array to match the project's Prettier rules (remove the extra spaces before the inline comment and normalize spacing around commas/entries) so the file passes the formatter check — update the PURGE_CONSUMER_BACKOFF_NS array definition accordingly.src/purge/PurgeWebhook.ts-1-5 (1)
1-5:⚠️ Potential issue | 🟡 MinorFix the import grouping/order lint errors.
The reported
import/orderissues can blockyarn validate; reorder the type/local imports according to the ESLint config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/purge/PurgeWebhook.ts` around lines 1 - 5, Reorder the imports to satisfy import/order: place the external package import first (import type { Logger } from '@credo-ts/core'), then local non-type imports (import { sleep } from '../utils/webhook' and import { PURGE_WEBHOOK_PATHS, PURGE_WEBHOOK_RETRY_DELAYS_MS } from './PurgeConstants'), and finally group local type-only imports together (import type { PurgeRecordType } from './PurgeTypes'); adjust spacing so type imports are grouped and the import blocks follow external → local non-type → local type ordering.src/controllers/openid4vc/verifier-sessions/verification-sessions.Controller.ts-8-9 (1)
8-9:⚠️ Potential issue | 🟡 MinorFix the import ordering lint error.
Move
../../../purge/PurgeTypesbefore../../../purge/decorators/SchedulePurgeto satisfy the reportedimport/orderrule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controllers/openid4vc/verifier-sessions/verification-sessions.Controller.ts` around lines 8 - 9, The import ordering in verification-sessions.Controller.ts is wrong: swap the two imports so PurgeRecordType is imported before SchedulePurge; specifically reorder the import lines that reference PurgeRecordType and SchedulePurge so the import for PurgeRecordType appears above the import for SchedulePurge (symbols: PurgeRecordType, SchedulePurge) to satisfy the import/order lint rule.src/purge/PurgeTypes.ts-16-18 (1)
16-18:⚠️ Potential issue | 🟡 MinorRun Prettier on the inline comments.
The current spacing triggers the reported
prettier/prettiererrors and can blockyarn validate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/purge/PurgeTypes.ts` around lines 16 - 18, The inline trailing comments on the fields tenantId, agentMode, and scheduledAt in PurgeTypes.ts are mis-spaced and causing prettier/prettier errors; run Prettier (or manually adjust) so trailing comments follow Prettier's spacing rules (normalize spaces before the // and a single space after //) for those fields and reformat the file, ensuring tenantId, agentMode and scheduledAt comment alignment no longer triggers lint errors.package.json-79-80 (1)
79-80:⚠️ Potential issue | 🟡 MinorRemove
@types/node-cronas it is incompatible withnode-cron@4.x.
node-cron@4.2.1bundles its own TypeScript declarations, making@types/node-cron@3.0.11redundant and incompatible. This can cause type conflicts for consumers. Remove the dev dependency:Proposed fix
- "@types/node-cron": "^3.0.11",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 79 - 80, The devDependency `@types/node-cron` should be removed because node-cron@4.x includes its own TypeScript types; open package.json and delete the "@types/node-cron" entry from devDependencies (and any mentions in package-lock or shrinkwrap if present), then run npm install (or yarn) to update lockfiles and ensure TypeScript resolves types from the bundled node-cron package rather than the removed `@types` package.src/controllers/didcomm/proofs/ProofController.ts-19-20 (1)
19-20:⚠️ Potential issue | 🟡 MinorFix the changed import order.
ESLint flags this ordering, so CI may fail until
PurgeTypesis imported before the decorator.Proposed fix
-import { SchedulePurge } from '../../../purge/decorators/SchedulePurge' import { PurgeRecordType } from '../../../purge/PurgeTypes' +import { SchedulePurge } from '../../../purge/decorators/SchedulePurge'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controllers/didcomm/proofs/ProofController.ts` around lines 19 - 20, Reorder the two imports in ProofController.ts so the PurgeTypes import comes before the decorator import: move "import { PurgeRecordType } from '../../../purge/PurgeTypes'" above "import { SchedulePurge } from '../../../purge/decorators/SchedulePurge'"; this will satisfy ESLint's import ordering rule while keeping the same symbols (SchedulePurge and PurgeRecordType) and behavior.src/controllers/didcomm/credentials/CredentialController.ts-17-18 (1)
17-18:⚠️ Potential issue | 🟡 MinorFix the changed import order.
ESLint flags this ordering, so CI may fail until
PurgeTypesis imported before the decorator.Proposed fix
-import { SchedulePurge } from '../../../purge/decorators/SchedulePurge' import { PurgeRecordType } from '../../../purge/PurgeTypes' +import { SchedulePurge } from '../../../purge/decorators/SchedulePurge'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controllers/didcomm/credentials/CredentialController.ts` around lines 17 - 18, Reorder the two imports in CredentialController.ts so that PurgeRecordType (from '../../../purge/PurgeTypes') is imported before the SchedulePurge decorator (from '../../../purge/decorators/SchedulePurge'); update the import statement order to satisfy ESLint ordering rules and ensure symbols PurgeRecordType and SchedulePurge remain unchanged.src/controllers/openid4vc/issuance-sessions/issuance-sessions.Controller.ts-9-10 (1)
9-10:⚠️ Potential issue | 🟡 MinorFix the changed import order.
ESLint flags this ordering, so CI may fail until
PurgeTypesis imported before the decorator.Proposed fix
-import { SchedulePurge } from '../../../purge/decorators/SchedulePurge' import { PurgeRecordType } from '../../../purge/PurgeTypes' +import { SchedulePurge } from '../../../purge/decorators/SchedulePurge'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controllers/openid4vc/issuance-sessions/issuance-sessions.Controller.ts` around lines 9 - 10, The import order is reversed and causing an ESLint ordering error: move the PurgeRecordType import before the SchedulePurge import so that PurgeTypes are imported prior to the decorator; specifically, ensure the import for PurgeRecordType (from '../../../purge/PurgeTypes') appears above the import for SchedulePurge (from '../../../purge/decorators/SchedulePurge') in issuance-sessions.Controller.ts to satisfy the linter..env.sample-90-98 (1)
90-98:⚠️ Potential issue | 🟡 MinorMove inline examples out of empty env assignments.
These lines are linted as values, not comments. Keep assignments empty or provide real defaults, with examples on the preceding comment lines.
Proposed fix
# ── Flow A: NATS (requires NATS_SERVERS below) ──────────────────────────────── PURGE_NATS_ENABLED= -PURGE_NATS_TTL_SECONDS= # seconds from offer creation to deletion (e.g. 2592000 = 30 days) +# Seconds from offer creation to deletion (e.g. 2592000 = 30 days) +PURGE_NATS_TTL_SECONDS= # ── Flow B: Cron (DB scan, no NATS required) ────────────────────────────────── PURGE_CRON_ENABLED= -PURGE_CRON_TTL_SECONDS= # records older than this (createdAt) are deleted (e.g. 2592000 = 30 days) -PURGE_CRON_SCHEDULE= # cron expression for scan frequency (e.g. 0 * * * * = hourly) +# Cron expression for scan frequency (e.g. 0 * * * * = hourly) +PURGE_CRON_SCHEDULE= +# Records older than this (createdAt) are deleted (e.g. 2592000 = 30 days) +PURGE_CRON_TTL_SECONDS=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.sample around lines 90 - 98, The env sample currently places inline examples after empty assignments so they are parsed as values; update the PURGE_* variables (PURGE_NATS_ENABLED, PURGE_NATS_TTL_SECONDS, PURGE_CRON_ENABLED, PURGE_CRON_TTL_SECONDS, PURGE_CRON_SCHEDULE) so the assignments remain empty (e.g. PURGE_NATS_TTL_SECONDS=) or set real sensible defaults, and move any illustrative examples (e.g. "2592000 = 30 days", "0 * * * * = hourly") into comment lines immediately above each variable so the examples are not treated as values.src/cliAgent.ts-67-70 (1)
67-70:⚠️ Potential issue | 🟡 MinorFix the changed purge import ordering and formatting.
ESLint/Prettier flags these imports; move purge imports before
setupServerand format the multi-import.Proposed fix
-import { setupServer } from './server' import { buildPurgeConfig } from './purge/PurgeTypes' import { validatePurgeConfig } from './purge/PurgeConfigValidator' -import { initPurgeSchedulers, stopPurgeSchedulers, getNatsPurgeScheduler, getCronPurgeScheduler } from './purge/PurgeSchedulerFactory' +import { + initPurgeSchedulers, + stopPurgeSchedulers, + getNatsPurgeScheduler, + getCronPurgeScheduler, +} from './purge/PurgeSchedulerFactory' +import { setupServer } from './server'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cliAgent.ts` around lines 67 - 70, Move the purge-related imports so they appear before setupServer and normalize their formatting: place the imports for buildPurgeConfig (from './purge/PurgeTypes'), validatePurgeConfig (from './purge/PurgeConfigValidator') and the multi-import of initPurgeSchedulers, stopPurgeSchedulers, getNatsPurgeScheduler, getCronPurgeScheduler (from './purge/PurgeSchedulerFactory') together as a single, consistently formatted import block above the import of setupServer; keep the same symbol names but ensure the PurgeSchedulerFactory import lists its exported names on one line (comma-separated) to satisfy ESLint/Prettier.src/purge/schedulers/CronPurgeScheduler.ts-8-11 (1)
8-11:⚠️ Potential issue | 🟡 MinorFix the CI lint failures in this file.
ESLint/Prettier is flagging import ordering, explicit public modifiers, and formatting around
withTenantAgent()andqueryExpiredRecords(). These need cleanup before merge.Also applies to: 16-16, 32-32, 50-56, 98-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/purge/schedulers/CronPurgeScheduler.ts` around lines 8 - 11, Imports are misordered and there are ESLint/prettier style issues: reorder imports to follow project ordering (external packages first, then internal modules) and combine related PurgeTypes imports into a single line (replace separate imports of PurgeConfig and PurgeRecordType with one import), remove unnecessary explicit "public" modifiers from class members, and fix spacing/formatting around calls to withTenantAgent() and queryExpiredRecords() (ensure consistent spacing, line breaks and parentheses per project style). Update references in the CronPurgeScheduler class methods (including the constructor and methods that call withTenantAgent() and queryExpiredRecords()) to match the corrected import names and formatting so lint/Prettier errors are resolved.src/purge/PurgeWorker.ts-9-11 (1)
9-11:⚠️ Potential issue | 🟡 MinorFix the CI lint failures in this file.
ESLint/Prettier is currently flagging import grouping/type-only imports, missing explicit public modifiers, and wrapped call formatting. These will block the PR even if runtime behavior is correct.
Also applies to: 20-26, 68-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/purge/PurgeWorker.ts` around lines 9 - 11, Adjust imports to satisfy ESLint/Prettier: make the PurgeJob import a type-only import (use "import type { PurgeJob } from './PurgeTypes'"), keep value imports (sendPurgeWebhook, PurgeDeletionStatus, PurgeRecordType) as regular imports and group them according to your project's import-order rule (values then types); add explicit public modifiers to any class properties and methods in PurgeWorker (e.g., mark methods like process(), start(), stop() or properties used publicly with "public"), and fix wrapped call formatting by collapsing or properly aligning multiline calls so they match the project's call-wrapping rule (ensure opening parenthesis/first argument placement conforms to linter). Ensure you update usages to match the type-only import where needed.src/purge/schedulers/NatsPurgeScheduler.ts-84-84 (1)
84-84:⚠️ Potential issue | 🟡 MinorReplace
console.*with the agent logger.The scheduler otherwise loses structured logs and currently violates the project’s
no-consolelint rule.Also applies to: 125-125, 142-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/purge/schedulers/NatsPurgeScheduler.ts` at line 84, In NatsPurgeScheduler replace all console.* calls with the agent logger used in this class (e.g., agent.logger.info/warn/error) so logs are structured and comply with lint rules; specifically change the console.info at the scheduled message to agent.logger.info and include the same structured fields (recordType, recordId, fireAt), and likewise replace the console.* calls at the other occurrences (lines noted in the review) with agent.logger.warn or agent.logger.error as appropriate so the messages and metadata remain the same but use the class's agent logger.src/purge/schedulers/CronPurgeScheduler.ts-160-173 (1)
160-173:⚠️ Potential issue | 🟡 MinorDo not count already-absent records as deleted.
RecordNotFoundErrorsetsALREADY_ABSENT, but the method still returnstrue, sototalDeletedincludes records that were not deleted by this scan.📊 Proposed metric fix
- return true + return status === PurgeDeletionStatus.DELETED🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/purge/schedulers/CronPurgeScheduler.ts` around lines 160 - 173, The method in CronPurgeScheduler.ts currently sets status = PurgeDeletionStatus.ALREADY_ABSENT on RecordNotFoundError but still returns true; change the final return logic so already-absent records are not counted as deleted — e.g. after the catch or before the final return, return false when status === PurgeDeletionStatus.ALREADY_ABSENT (while still calling sendPurgeWebhook(webhookUrl, recordId, recordType, tenantId, status, logger) if webhookUrl is present), or equivalently return status === PurgeDeletionStatus.DELETED; reference RecordNotFoundError, PurgeDeletionStatus, sendPurgeWebhook and the surrounding logger calls to locate the code to modify.
🧹 Nitpick comments (2)
src/tracer.ts (1)
30-63: Gate the shutdown handler withotelEnabledtoo.Lines 49-51 conditionally register the log processor, but lines 61-62 unconditionally register a SIGTERM handler that calls
otelSDK.shutdown(). Sincesrc/server.tsskipsotelSDK.start()whenOTEL_ENABLEDis not 'true', the shutdown handler attempts to shut down an SDK that was never started. Gate the handler to match the startup pattern.♻️ Proposed fix
-if (typeof process?.on === 'function') { +if (otelEnabled && typeof process?.on === 'function') { process.on('SIGTERM', () => { Promise.all([otelSDK.shutdown(), logProvider.shutdown()])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tracer.ts` around lines 30 - 63, The SIGTERM shutdown handler is registered unconditionally even when OTEL is disabled; wrap the registration of process.on('SIGTERM', ...) with the same otelEnabled check used for the log processor so you only call otelSDK.shutdown() and logProvider.shutdown() when otelEnabled is true. Locate otelEnabled, otelSDK and logProvider in this file and gate the process.on registration (the block that calls Promise.all([otelSDK.shutdown(), logProvider.shutdown()])) behind if (otelEnabled) so the handler is not added when the SDK was never started.src/controllers/openid4vc/verifier-sessions/verification-sessions.Controller.ts (1)
26-26: The decorator already logs when ID extraction fails—revisit the type-safety concern separately.The
SchedulePurgedecorator (lines 24–26 insrc/purge/decorators/SchedulePurge.ts) includes explicit handling: whenidExtractorreturnsundefined, it logs a warning (console.warn) with the result and returns early. This is not a silent failure.However, relying on
anytype casts and optional chaining on an untyped result ((r as any)?.verificationSession?.id) remains a type-safety concern—consider typing the response shape or creating a stricter type-safe extractor to prevent accidental mismatches at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controllers/openid4vc/verifier-sessions/verification-sessions.Controller.ts` at line 26, The decorator usage is masking a type-safety issue by casting to any in the id extractor; replace the loose extractor "(r as any)?.verificationSession?.id" used on the `@SchedulePurge`(PurgeRecordType.OID4VC_VERIFICATION, ...) decorator with a typed extractor function or an interface for the controller response (e.g., define an IVerificationSessionResponse { verificationSession?: { id?: string } }) and use that type in the extractor to safely access verificationSession.id; update the idExtractor reference in SchedulePurge invocation in verification-sessions.Controller.ts to use that typed shape so the compiler enforces correctness instead of relying on any/optional chaining.
🤖 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/cliAgent.ts`:
- Around line 561-578: The server is started with app.listen(adminPort) before
purge setup, allowing requests to create records before purge schedulers exist;
move the purge initialization block (calls to buildPurgeConfig,
validatePurgeConfig, initPurgeSchedulers and the conditional awaits of
getNatsPurgeScheduler().start and getCronPurgeScheduler().start) to run before
calling app.listen, or alternatively delay readiness/listening until those
promises complete so decorated endpoints cannot run before purge schedulers are
initialized.
In `@src/purge/decorators/SchedulePurge.ts`:
- Around line 40-42: The code currently fires
scheduler.schedulePurge(recordType, recordId, tenantId, agentMode) and only logs
errors, which can leave records without enforced purge; change the flow so
schedulePurge is awaited and failures propagate or are compensated: either await
scheduler.schedulePurge(...) and rethrow the error from the surrounding function
(so the create operation fails), or implement a durable outbox fallback by
persisting the pending purge action (e.g., add a record to the purge_outbox
table/queue) when schedulePurge rejects and ensure a retry/cron job will publish
it; update the code paths that call scheduler.schedulePurge in SchedulePurge.ts
to use the chosen strategy and adjust error handling to avoid returning success
when scheduling fails.
- Around line 24-25: Replace the direct console.warn that prints the full
`result` in the `SchedulePurge` decorator when `recordId` is missing: do not log
the full `result` object (it may contain sensitive issuance/proof payloads);
instead use the agent's logger (e.g., `agent.logger.warn`) and emit only
minimal, sanitized metadata such as `recordType`, an indication that `recordId`
was not found, and any non-sensitive identifiers (e.g., `result.id` or
`result.metadata` if those are safe), leaving out exchange payloads; update the
warning in the `SchedulePurge` code path that currently references `recordType`,
`recordId`, and `result` to follow this sanitized logging approach.
In `@src/purge/PurgeConstants.ts`:
- Around line 40-41: The hardcoded PURGE_STREAM_MAX_AGE_NS (35 days) can be
smaller than the configured PURGE_NATS_TTL_SECONDS causing scheduled JetStream
messages to expire before execution; update initialization to either validate
that configured PURGE_NATS_TTL_SECONDS <= PURGE_STREAM_MAX_AGE_NS and throw/log
a clear error if not, or compute PURGE_STREAM_MAX_AGE_NS dynamically as
(PURGE_NATS_TTL_SECONDS + bufferSeconds) converted to nanoseconds so the stream
max age always exceeds the TTL (reference symbols: PURGE_STREAM_MAX_AGE_NS,
PURGE_NATS_TTL_SECONDS); implement the chosen approach where
PURGE_STREAM_MAX_AGE_NS is set/checked at startup before schedules are created.
In `@src/purge/PurgeDeleteRecord.ts`:
- Around line 8-28: The switch over recordType
(PurgeRecordType.DIDCOMM_CREDENTIAL, DIDCOMM_PROOF, OID4VC_ISSUANCE,
OID4VC_VERIFICATION) lacks a default/exhaustiveness guard and will silently
succeed for new/unsupported types; add a final default case (or an
exhaustiveness check like assertNever) that throws an Error including the
unexpected recordType value so the job fails visibly (e.g., throw new
Error(`Unsupported PurgeRecordType: ${recordType}`)), updating the switch in
PurgeDeleteRecord.ts where recordType is handled to ensure unsupported values do
not pass silently.
In `@src/purge/PurgeTypes.ts`:
- Around line 75-81: If any PURGE_* env flag is defined (anyEnvSet true) but the
filtered result yields an empty array, fail fast instead of returning []: inside
the block that uses Object.entries(envFlags).filter(...).map(...), check if no
types were selected and either throw a descriptive Error (e.g., "PURGE_* flags
present but none set to 'true' - explicitly disable purge or set a flag to
'true'") or return a sentinel (null/undefined) that the caller treats as
“disabled”; update callers accordingly. Reference the anyEnvSet variable and the
Object.entries(envFlags).filter(([key]) => process.env[key] === 'true').map(([,
type]) => type) logic to implement this guard.
- Around line 51-61: Replace the loose Number(...) || default pattern with
explicit validation: add a DEFAULT_PURGE_TTL_SECONDS constant and a helper like
parsePositiveTtlSeconds(value, envName) and use it when setting ttlSeconds for
both the top-level purge config and cronConfig (reference buildPurgeConfig,
ttlSeconds fields) so invalid, non-finite, zero or negative values throw a clear
error; also normalize NATS servers by splitting process.env.NATS_SERVERS,
trimming each entry and filtering out empty strings before assigning to
nats.servers so only valid server URLs are passed to connect (reference the
nats.servers assignment and NATS_CREDENTIALS_FILE).
In `@src/purge/PurgeWebhook.ts`:
- Around line 38-45: The webhook sender in PurgeWebhook uses fetch but treats
any resolved response as success; update the send logic (in the method that
performs await fetch(...) in PurgeWebhook) to inspect the Response object and
only mark delivery as successful when response.ok is true — for non-2xx
responses log the status and body (or statusText) and throw an error (or return
a rejected promise) so existing retry logic can run; ensure you still pass the
AbortSignal.timeout and preserve the logger.debug call for actual successful
deliveries.
In `@src/purge/PurgeWorker.ts`:
- Around line 49-62: Validate that the incoming job's recordType equals the
worker's expected this.recordType before calling deletePurgeRecord; if they
differ, log an error with both values, ack/discard the job and return early. Add
the check in the handler just after extracting const { recordId, recordType,
tenantId, agentMode } and before the agentMode branch (and inside the
withTenantAgent callback path if needed), referencing recordType,
this.recordType, deletePurgeRecord, agentMode and the tenants.withTenantAgent
call so the deletion only runs when types match.
In `@src/purge/schedulers/CronPurgeScheduler.ts`:
- Around line 19-23: Cron ticks can start overlapping runs because each
scheduled callback calls this.runScan regardless of current activity; add a
guard on the CronPurgeScheduler instance (e.g., a boolean like isRunning) and
check it inside the cron.schedule callback before calling runScan — if isRunning
is true, log and skip this tick. When starting a scan set isRunning = true, call
this.runScan(agent, config, webhookUrl) and ensure isRunning is reset to false
in a finally block so it always releases the lock even on errors; update the
cron.schedule callback and references around this.job, runScan, and
agent.config.logger accordingly.
- Around line 98-142: The queryExpiredRecords method loads all records into
memory then filters by createdAt, causing high memory/latency; update each case
in queryExpiredRecords to push the createdAt < cutoffMs predicate into the
repository query instead of filtering in-app — i.e., call (agent as
any).modules.didcomm.credentials.findAllByQuery and .profs.findAllByQuery with a
query object that filters createdAt < cutoff (or use the repository's date-query
API) and call OpenId4VcIssuanceSessionRepository.findByQuery and
OpenId4VcVerificationSessionRepository.findByQuery with the same date filter; if
the repository cannot accept such queries, add pagination to those repo methods
and iterate pages until no more records, collecting IDs as you go (update
queryExpiredRecords to use the repo query/pagination changes).
In `@src/purge/schedulers/NatsPurgeScheduler.ts`:
- Around line 177-179: The handler currently only logs failures from
worker.start and doesn't restart the worker; add a supervised loop helper named
runWorker(agent: Agent, worker: PurgeWorker, consumer: Consumer, consumerName:
string): Promise<void> that repeatedly calls await worker.start(agent, consumer)
inside a while (this.nc) loop, catches errors, logs via
agent.config.logger.error('[Purge] Worker crashed — restarting', { consumerName,
error: err?.message }), waits (e.g. 5s) and retries; then replace the existing
one-off worker.start(...).catch(...) call with a call to this.runWorker(agent,
worker, consumer, consumerName) so workers are restarted after unexpected
failures.
- Around line 134-144: deleteStaleStreams currently treats any subject with '>'
as automatically overlapping and can delete unrelated streams; update the
overlap check in deleteStaleStreams to perform explicit overlap matching: for
each stream subject s and each purge subject newS, consider them overlapping if
s === newS, or if s.endsWith('>') and newS.startsWith(s.slice(0, -1)), or if
newS.endsWith('>') and s.startsWith(newS.slice(0, -1)); use
stream.config.subjects and the incoming subjects array for these checks, and
only call this.jsm.streams.delete(stream.config.name) when that explicit overlap
is true and stream.config.name !== PURGE_STREAM.
- Around line 40-45: The connect call in NatsPurgeScheduler is using a
non-existent ConnectionOptions property credentialsFile, so authentication will
fail; replace that spread with the authenticator option using credsAuthenticator
by reading the credentials file into a Uint8Array (e.g., via fs.readFileSync or
an async read) and passing it to credsAuthenticator, i.e. when
natsConfig.nats.credentialsFile is present set authenticator:
credsAuthenticator(<Uint8Array of file contents>), leaving other connect options
(servers, maxReconnectAttempts, reconnectTimeWait) unchanged, and update imports
to include credsAuthenticator and fs as needed.
- Around line 73-80: scheduleSubject and the Nats-Msg-Id header currently only
use recordType and recordId which can collide across tenants; update the code
that builds scheduleSubject and the 'Nats-Msg-Id' header (the block where
scheduleSubject is set and headers() h are populated) to include tenantId (or
the string 'dedicated' when appropriate) and agentMode so identifiers are unique
per tenant and mode, and add a helper subjectToken(value: string) that
URI-encodes and percent-escapes problematic characters to sanitize tokens before
concatenation; use subjectToken(recordType), subjectToken(recordId),
subjectToken(tenantIdOrDedicated), and subjectToken(agentMode) when composing
scheduleSubject and the message id, and ensure the Nats-Schedule-Target
(PURGE_EXECUTION_SUBJECTS) remains unchanged.
---
Minor comments:
In @.env.sample:
- Around line 90-98: The env sample currently places inline examples after empty
assignments so they are parsed as values; update the PURGE_* variables
(PURGE_NATS_ENABLED, PURGE_NATS_TTL_SECONDS, PURGE_CRON_ENABLED,
PURGE_CRON_TTL_SECONDS, PURGE_CRON_SCHEDULE) so the assignments remain empty
(e.g. PURGE_NATS_TTL_SECONDS=) or set real sensible defaults, and move any
illustrative examples (e.g. "2592000 = 30 days", "0 * * * * = hourly") into
comment lines immediately above each variable so the examples are not treated as
values.
In `@package.json`:
- Around line 79-80: The devDependency `@types/node-cron` should be removed
because node-cron@4.x includes its own TypeScript types; open package.json and
delete the "@types/node-cron" entry from devDependencies (and any mentions in
package-lock or shrinkwrap if present), then run npm install (or yarn) to update
lockfiles and ensure TypeScript resolves types from the bundled node-cron
package rather than the removed `@types` package.
In `@src/cliAgent.ts`:
- Around line 67-70: Move the purge-related imports so they appear before
setupServer and normalize their formatting: place the imports for
buildPurgeConfig (from './purge/PurgeTypes'), validatePurgeConfig (from
'./purge/PurgeConfigValidator') and the multi-import of initPurgeSchedulers,
stopPurgeSchedulers, getNatsPurgeScheduler, getCronPurgeScheduler (from
'./purge/PurgeSchedulerFactory') together as a single, consistently formatted
import block above the import of setupServer; keep the same symbol names but
ensure the PurgeSchedulerFactory import lists its exported names on one line
(comma-separated) to satisfy ESLint/Prettier.
In `@src/controllers/didcomm/credentials/CredentialController.ts`:
- Around line 17-18: Reorder the two imports in CredentialController.ts so that
PurgeRecordType (from '../../../purge/PurgeTypes') is imported before the
SchedulePurge decorator (from '../../../purge/decorators/SchedulePurge'); update
the import statement order to satisfy ESLint ordering rules and ensure symbols
PurgeRecordType and SchedulePurge remain unchanged.
In `@src/controllers/didcomm/proofs/ProofController.ts`:
- Around line 19-20: Reorder the two imports in ProofController.ts so the
PurgeTypes import comes before the decorator import: move "import {
PurgeRecordType } from '../../../purge/PurgeTypes'" above "import {
SchedulePurge } from '../../../purge/decorators/SchedulePurge'"; this will
satisfy ESLint's import ordering rule while keeping the same symbols
(SchedulePurge and PurgeRecordType) and behavior.
In `@src/controllers/openid4vc/issuance-sessions/issuance-sessions.Controller.ts`:
- Around line 9-10: The import order is reversed and causing an ESLint ordering
error: move the PurgeRecordType import before the SchedulePurge import so that
PurgeTypes are imported prior to the decorator; specifically, ensure the import
for PurgeRecordType (from '../../../purge/PurgeTypes') appears above the import
for SchedulePurge (from '../../../purge/decorators/SchedulePurge') in
issuance-sessions.Controller.ts to satisfy the linter.
In
`@src/controllers/openid4vc/verifier-sessions/verification-sessions.Controller.ts`:
- Around line 8-9: The import ordering in verification-sessions.Controller.ts is
wrong: swap the two imports so PurgeRecordType is imported before SchedulePurge;
specifically reorder the import lines that reference PurgeRecordType and
SchedulePurge so the import for PurgeRecordType appears above the import for
SchedulePurge (symbols: PurgeRecordType, SchedulePurge) to satisfy the
import/order lint rule.
In `@src/purge/PurgeConfigValidator.ts`:
- Line 3: The import ordering lint failed because the local type import
"PurgeConfig" is in the wrong group; move the line "import type { PurgeConfig }
from './PurgeTypes'" into the local/project imports group so it follows external
package imports and precedes sibling/relative imports per the project's
import/order rules (update the imports in PurgeConfigValidator.ts accordingly).
In `@src/purge/PurgeConstants.ts`:
- Around line 50-53: The backoff array constant PURGE_CONSUMER_BACKOFF_NS has
inconsistent spacing that triggers Prettier; reformat the array to match the
project's Prettier rules (remove the extra spaces before the inline comment and
normalize spacing around commas/entries) so the file passes the formatter check
— update the PURGE_CONSUMER_BACKOFF_NS array definition accordingly.
In `@src/purge/PurgeTypes.ts`:
- Around line 16-18: The inline trailing comments on the fields tenantId,
agentMode, and scheduledAt in PurgeTypes.ts are mis-spaced and causing
prettier/prettier errors; run Prettier (or manually adjust) so trailing comments
follow Prettier's spacing rules (normalize spaces before the // and a single
space after //) for those fields and reformat the file, ensuring tenantId,
agentMode and scheduledAt comment alignment no longer triggers lint errors.
In `@src/purge/PurgeWebhook.ts`:
- Around line 1-5: Reorder the imports to satisfy import/order: place the
external package import first (import type { Logger } from '@credo-ts/core'),
then local non-type imports (import { sleep } from '../utils/webhook' and import
{ PURGE_WEBHOOK_PATHS, PURGE_WEBHOOK_RETRY_DELAYS_MS } from './PurgeConstants'),
and finally group local type-only imports together (import type {
PurgeRecordType } from './PurgeTypes'); adjust spacing so type imports are
grouped and the import blocks follow external → local non-type → local type
ordering.
In `@src/purge/PurgeWorker.ts`:
- Around line 9-11: Adjust imports to satisfy ESLint/Prettier: make the PurgeJob
import a type-only import (use "import type { PurgeJob } from './PurgeTypes'"),
keep value imports (sendPurgeWebhook, PurgeDeletionStatus, PurgeRecordType) as
regular imports and group them according to your project's import-order rule
(values then types); add explicit public modifiers to any class properties and
methods in PurgeWorker (e.g., mark methods like process(), start(), stop() or
properties used publicly with "public"), and fix wrapped call formatting by
collapsing or properly aligning multiline calls so they match the project's
call-wrapping rule (ensure opening parenthesis/first argument placement conforms
to linter). Ensure you update usages to match the type-only import where needed.
In `@src/purge/schedulers/CronPurgeScheduler.ts`:
- Around line 8-11: Imports are misordered and there are ESLint/prettier style
issues: reorder imports to follow project ordering (external packages first,
then internal modules) and combine related PurgeTypes imports into a single line
(replace separate imports of PurgeConfig and PurgeRecordType with one import),
remove unnecessary explicit "public" modifiers from class members, and fix
spacing/formatting around calls to withTenantAgent() and queryExpiredRecords()
(ensure consistent spacing, line breaks and parentheses per project style).
Update references in the CronPurgeScheduler class methods (including the
constructor and methods that call withTenantAgent() and queryExpiredRecords())
to match the corrected import names and formatting so lint/Prettier errors are
resolved.
- Around line 160-173: The method in CronPurgeScheduler.ts currently sets status
= PurgeDeletionStatus.ALREADY_ABSENT on RecordNotFoundError but still returns
true; change the final return logic so already-absent records are not counted as
deleted — e.g. after the catch or before the final return, return false when
status === PurgeDeletionStatus.ALREADY_ABSENT (while still calling
sendPurgeWebhook(webhookUrl, recordId, recordType, tenantId, status, logger) if
webhookUrl is present), or equivalently return status ===
PurgeDeletionStatus.DELETED; reference RecordNotFoundError, PurgeDeletionStatus,
sendPurgeWebhook and the surrounding logger calls to locate the code to modify.
In `@src/purge/schedulers/NatsPurgeScheduler.ts`:
- Line 84: In NatsPurgeScheduler replace all console.* calls with the agent
logger used in this class (e.g., agent.logger.info/warn/error) so logs are
structured and comply with lint rules; specifically change the console.info at
the scheduled message to agent.logger.info and include the same structured
fields (recordType, recordId, fireAt), and likewise replace the console.* calls
at the other occurrences (lines noted in the review) with agent.logger.warn or
agent.logger.error as appropriate so the messages and metadata remain the same
but use the class's agent logger.
---
Nitpick comments:
In
`@src/controllers/openid4vc/verifier-sessions/verification-sessions.Controller.ts`:
- Line 26: The decorator usage is masking a type-safety issue by casting to any
in the id extractor; replace the loose extractor "(r as
any)?.verificationSession?.id" used on the
`@SchedulePurge`(PurgeRecordType.OID4VC_VERIFICATION, ...) decorator with a typed
extractor function or an interface for the controller response (e.g., define an
IVerificationSessionResponse { verificationSession?: { id?: string } }) and use
that type in the extractor to safely access verificationSession.id; update the
idExtractor reference in SchedulePurge invocation in
verification-sessions.Controller.ts to use that typed shape so the compiler
enforces correctness instead of relying on any/optional chaining.
In `@src/tracer.ts`:
- Around line 30-63: The SIGTERM shutdown handler is registered unconditionally
even when OTEL is disabled; wrap the registration of process.on('SIGTERM', ...)
with the same otelEnabled check used for the log processor so you only call
otelSDK.shutdown() and logProvider.shutdown() when otelEnabled is true. Locate
otelEnabled, otelSDK and logProvider in this file and gate the process.on
registration (the block that calls Promise.all([otelSDK.shutdown(),
logProvider.shutdown()])) behind if (otelEnabled) so the handler is not added
when the SDK was never started.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 156ee1ad-adc2-40b5-bb2d-1b9633062ad4
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
.env.samplepackage.jsonsrc/cliAgent.tssrc/controllers/didcomm/credentials/CredentialController.tssrc/controllers/didcomm/proofs/ProofController.tssrc/controllers/openid4vc/issuance-sessions/issuance-sessions.Controller.tssrc/controllers/openid4vc/verifier-sessions/verification-sessions.Controller.tssrc/purge/PurgeConfigValidator.tssrc/purge/PurgeConstants.tssrc/purge/PurgeDeleteRecord.tssrc/purge/PurgeSchedulerFactory.tssrc/purge/PurgeTypes.tssrc/purge/PurgeWebhook.tssrc/purge/PurgeWorker.tssrc/purge/decorators/SchedulePurge.tssrc/purge/schedulers/CronPurgeScheduler.tssrc/purge/schedulers/NatsPurgeScheduler.tssrc/routes/swagger.jsonsrc/server.tssrc/tracer.tssrc/utils/NatsConstants.ts
tipusinghaw
force-pushed
the
feat/data-deletion-retention
branch
from
867dd67 to
9d3ca75
Compare
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
tipusinghaw
force-pushed
the
feat/data-deletion-retention
branch
from
5fbd551 to
8510def
Compare
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
|
RinkalBhojani
approved these changes
Apr 28, 2026
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by CodeRabbit
New Features
Chores