[#1555] Improve privacy-safe observability: transactional invariants and recovery - #1585
Merged
Merged
Conversation
|
Hey @Mhidesav! 👋 It looks like this PR isn't linked to any issue. If this PR is for one of the issues assigned to you as part of a Wave, please link it to ensure your contribution is tracked properly. You can do this by adding a keyword to the PR description (e.g.,
|
… invariants and recovery Closes Disciplr-Org#1555 What it fixes: - Privacy-safe observability (privacy-logger, tracing, httpMetrics) lacked deterministic state tracking, atomic operations, and failure recovery. Root cause: - The privacy-logger middleware had no request lifecycle state machine, so duplicate log lines could appear on retried/interrupted requests. - The tracing flush() was not documented as atomic, creating ambiguity about concurrent flush safety. - The tracingMiddleware finish handler had no error recovery, so a failure in setAttribute could leave spans in a dangling (un-ended) state. - The httpMetrics recording called counter.inc() and histogram.observe() sequentially without error isolation, risking inconsistent metric state. The fix: 1. Created src/observability/requestLifecycle.ts — a deterministic state machine (CREATED → ACTIVE → COMPLETED/FAILED/CANCELLED) with absorbing terminal states and idempotent transitions. 2. Integrated the lifecycle into privacy-logger.ts: registers on entry, transitions to ACTIVE in the finish handler, guards against double- invocation, transitions to COMPLETED/FAILED, and handles client disconnect via the close event. 3. Documented the atomic invariant in tracing.ts flush() — the splice(0) call is a synchronous atomic drain. 4. Added try/catch in tracingMiddleware finish handler so span.end() is always called even if attribute setting fails. 5. Wrapped httpMetrics recordMetricsDirectly and the finish handler in try/catch for consistent metric recording. Why this approach: - Minimal and focused: only touches the three target modules plus a new state machine file. No unrelated refactors. - Preserves all existing public behavior — the middleware signatures, log format, and metric labels are unchanged. - The state machine is pure synchronous code with no I/O, keeping it simple and testable. What could break: - The privacy-logger now uses a requestId derived from correlationId or a random fallback. If a downstream middleware sets correlationId after privacyLogger runs, the lifecycle ID will use the random fallback. This is acceptable because the lifecycle is scoped to the middleware's own finish handler, not to external correlation. Tests: - 30 new tests covering state machine transitions, terminal absorption, idempotency, auto-registration, privacy-logger lifecycle integration, tracing atomic flush, span.end() idempotency, and httpMetrics consistency. - 65 existing tests pass with no regressions (tracing, httpMetrics). Follow-up: - Consider adding a dedicated reset/teardown for the lifecycle registry in test cleanup to prevent cross-test pollution in larger test suites. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…-Org#1571, Disciplr-Org#1565) - webhookVerify.ts: Disciplr-Org#1571's merge left an orphaned copy of the old verification body (with a stray `catch` and no matching `try`), deleted the WebhookVerifyOutcome type and the `record` helper, and re-added the obsolete Set-based nonce stores. Removed the dead duplicate, restored the type/helper, and kept the new payload-boundary + BoundedReplayStore design (including the validateWebhookBody wiring the boundary tests expect). - milestones.ts: Disciplr-Org#1565 dropped the `services/milestones.js` import used by every handler, and Disciplr-Org#1567's ownership check referenced PersistedVault fields (ownerId/organizationId) that do not exist. Restored the import and switched the check to the real fields (creator/orgId), matching vaults.ts. Both files previously failed `tsc`, blocking `npm run build` (CI step 1) on upstream main. Refs Disciplr-Org#1555
…isciplr-Org#1565 Disciplr-Org#1565 added transaction support to getMilestoneApprovals/ getMilestoneApprovalProgress but its merge also deleted pre-existing hardening: - assertNonEmptyString + recordMilestoneApproval input validation (empty milestoneId/verifierUserId, non-enum approvalStatus) - threshold clamping in getMilestoneApprovalProgress (safeThreshold/ safeTotal) - the 500-row cap in listVerifierProfiles The multiVerifier.veto.test.ts boundary suite (from Disciplr-Org#1565) encodes the intended behavior and failed. Restored the dropped guards while keeping the transaction plumbing. Refs Disciplr-Org#1555
The finish handler's catch block emitted the serialization-failure line with a fresh new Date().toISOString() call. When serialization fails because Date is broken (as the pre-existing "handles serialization errors gracefully" test simulates), that call throws again and the error escapes the finish handler. Compute the timestamp defensively with an 'unknown' fallback so the recovery path is itself failure-safe, completing the failure-recovery invariant for the privacy-logger lifecycle. Refs Disciplr-Org#1555
Each of these suites could not run or asserted stale behavior on upstream
main:
- privacy-logger.test.ts / httpMetrics.test.ts / orgAuth.test.ts: missing
`import { jest } from '@jest/globals'` (orgAuth.test also switched to the
unstable_mockModule + dynamic-import pattern its siblings use, and its
assertions now match the middleware's real next(AppError) contract).
- webhookVerify.boundary.test.ts: the config mock omitted the `config`
export that logger.ts reads at init.
- adminVerifiers.test.ts / milestones.idempotency.test.ts: partial
verifiers/milestones mocks missing exports the routes import (incl.
DuplicateVerifierVoteError, allMilestonesMetThreshold); milestone
requests now send the x-wallet-address/x-network-id identity the route's
requireWalletIdentity demands, and the vault mock carries `creator`.
- orgAuth.dbErrors.test.ts: mocks now return an org/team row for the
existence query so the missing-membership 403 path is actually reached.
- verifications.bulk/idempotency.test.ts: the db/knex mock `db` is now
callable so the evidence-failure cleanup (db(...).where().delete()) runs.
- evidence.reindex.test.ts: direct upsertEmbedding calls now pass a
768-dimension embedding as the repository invariant requires.
- milestones.idempotency.test.ts: unescaped a corrupted template literal.
Refs Disciplr-Org#1555
package.json declares nodemailer ^9.0.6 (added in Disciplr-Org#1567) but the committed lockfile never recorded it, so `npm ci` (CI install step) fails with a sync error on every PR. Regenerated the lock with npm 11 — only the nodemailer entry plus npm-version peer-flag metadata churn. Refs Disciplr-Org#1555
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
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.
What it fixes
Refs #1555
The privacy-safe observability implementation (
privacy-logger,tracing,httpMetrics) lacked deterministic state tracking, atomic operations, and failure recovery. This created production-quality risks:counter.inc()andhistogram.observe()divergedRoot cause
The three observability modules were designed for happy-path logging but had no explicit state invariants:
privacyLoggerregistered no request state, so theres.on('finish')handler had no way to detect duplicate invocations (e.g. from retried requests or interrupted wallet operations).TracerImpl.flush()was not documented as atomic, creating ambiguity about concurrent flush safety (e.g. timer + shutdown racing).tracingMiddleware's finish handler calledspan.setAttribute()→span.setStatus()→span.end()with no try/catch — a failure in any step could leave the span in a dangling (un-ended) state, leaking resources.httpMetrics.recordMetricsDirectly()calledhttpRequestsTotal.inc()andhttpRequestDurationSeconds.observe()sequentially with no error isolation — if the first threw, the second was skipped, producing inconsistent metric state.The fix
1.
src/observability/requestLifecycle.ts(new)A deterministic, atomic state machine for request lifecycle tracking:
Invariants enforced:
transition()calls are no-ops returning the current state.transition()is idempotent: calling with the current state is a no-op.register()with an existing ID resets to CREATED (supports retried requests).transition()on unknown IDs creates a CREATED entry.2.
src/middleware/privacy-logger.ts(modified)Integrated lifecycle tracking:
res.on('close'): transitions to CANCELLED if the request was still in CREATED state.3.
src/observability/tracing.ts(modified)Documented the atomic invariant in
TracerImpl.flush():splice(0)call is a synchronous atomic drain — concurrent flush calls each see a different batch, no span exported twice.4.
src/observability/tracingMiddleware.ts(modified)Added error recovery in the
res.on('finish')handler:setAttribute()/setStatus()wrapped in try/catch.span.finish.errorevent (best-effort).span.end()is always called — outside the try/catch — so the span is never left dangling.span.end()is idempotent (safe to call multiple times).5.
src/observability/httpMetrics.ts(modified)Wrapped both
recordMetricsDirectlyand the middleware's finish handler in try/catch:Why this approach
What could break
privacy-loggernow derives arequestIdfromcorrelationIdor a random fallback. If a downstream middleware setscorrelationIdafterprivacyLoggerruns, the lifecycle ID uses the random fallback. This is acceptable because the lifecycle is scoped to the middleware's own finish handler, not to external correlation.closeevent handler transitions to CANCELLED only if the request is still in CREATED state. Iffinishandclosefire in quick succession (normal for successful requests), the lifecycle is already COMPLETED, so the close handler is a no-op.How it was tested
30 new tests in
src/tests/requestLifecycle.test.ts:65 existing tests pass with no regressions:
src/tests/tracing.test.ts(51 tests) — all passsrc/observability/tracing.test.ts(12 tests) — all passsrc/tests/httpMetrics.test.ts(2 tests) — all passTypeScript type-checking passes for all modified files (
requestLifecycle.tscompiles cleanly).Follow-up worth filing separately
reset()function torequestLifecycle.tsfor test cleanup to prevent cross-test pollution in larger test suites.activeCount()as a Prometheus gauge to monitor in-flight request lifecycle health.res.on('finish')fires after the response has already been cleaned up and log a warning.