Skip to content

[#1555] Add observability state machine for deterministic, atomic, and recoverable request lifecycle - #1584

Merged
1nonlypiece merged 1 commit into
Disciplr-Org:mainfrom
Mhidesav:fix/1555-observability-state-machine
Aug 30, 2026
Merged

1nonlypiece merged 1 commit into
Disciplr-Org:mainfrom
Mhidesav:fix/1555-observability-state-machine

Conversation

@Mhidesav

Copy link
Copy Markdown
Contributor

What it fixes

Refs #1555

The three privacy-safe observability modules (privacy-logger, tracingMiddleware, httpMetrics) previously operated independently with no coordination. Each attached its own res.on('finish', …) handler, creating three concrete production-quality risk areas:

  1. Duplicate event recording — if the finish event fires more than once (retry, pipeline re-entry), the same request would produce duplicate log lines, metrics, or span completions, inflating observability signals and confusing dashboards.
  2. No atomic state transitions — a partial failure (e.g. serialization error in the privacy-logger) left tracing/metrics in an inconsistent state with no recovery path.
  3. No recovery from interrupted operations — if one component failed, downstream components had no signal that an earlier phase had already completed or failed, making retries opaque.

Additionally, httpMetrics.test.ts and privacy-logger.test.ts had missing jest imports that caused every test in those suites to fail (pre-existing breakage).

Root cause

The three middleware modules shared no state. Each independently attached a res.on('finish', …) handler and had no awareness of whether the others had already recorded their observability event for the same request. The redact() function's WeakSet for cycle detection was also created fresh per-call rather than shared, and the privacy-logger's catch block could itself throw (e.g. if new Date().toISOString() failed during error logging).

The fix and why

Introduced a shared, per-request state machine (src/observability/observabilityState.ts) that enforces deterministic transitions for logging, tracing, and metrics:

    PENDING → IN_PROGRESS → DONE
                     ↓
                   FAILED

Design decisions

Decision Rationale
WeakMap keyed on Request State is per-request and automatically reclaimed by GC when the request is collected. No explicit cleanup needed.
Terminal-state idempotency Once an operation reaches DONE or FAILED, subsequent transitionOperation calls are no-ops (return false). This is the core deduplication mechanism — if finish fires twice, the second invocation is skipped.
transitionOperation returns boolean Callers can distinguish "transition applied" from "already terminal" without exceptions, keeping the hot path exception-free.
Independent operation tracking Logging, tracing, and metrics each have independent state. A failure in one doesn't block or corrupt the others.
Defensive catch blocks The privacy-logger's catch block wraps transitionOperation and new Date().toISOString() in nested try-catch to prevent the error handler itself from throwing. This was the actual root cause of the serialization-failure test failing.
No changes to the public API privacyLogger, tracingMiddleware, and httpMetricsMiddleware retain their existing Express middleware signatures. The state machine is an internal coordination layer.

Why not alternatives

  • Shared WeakMap vs. request property — Using req.observabilityState would pollute the Express Request type and risk conflicts with other middleware. WeakMap keeps it invisible to the rest of the stack.
  • Event-emitter coordination — Would add complexity and another dependency for a problem that a simple state machine solves.
  • Per-component deduplication (e.g. boolean flag) — Would duplicate the same logic three times. The shared module centralizes it.

How it was tested

node --experimental-vm-modules node_modules/jest/bin/jest.js \
  --forceExit --testTimeout=30000 \
  --testPathPattern="privacy-logger|tracing|httpMetrics|observabilityState"

Results: 7 suites, 154 tests, 0 failures, 1 snapshot.

Test coverage added (30 new tests)

Category Tests
State machine unit tests getObservabilityState idempotency and isolation; transitionOperation normal paths (PENDING→IN_PROGRESS→DONE, PENDING→FAILED, IN_PROGRESS→FAILED), invalid transitions (PENDING→DONE rejected), terminal state rejection (DONE and FAILED are terminal)
isTerminal / isFullyResolved Returns correct values for all states; mixed DONE+FAILED resolves fully
Middleware integration Privacy-logger: duplicate finish events produce only one log line; state transitions to DONE on success and FAILED with error message on serialization failure; catch block doesn't throw when Date mock is active
Tracing middleware State transitions to DONE on response finish; duplicate finish events export only one span
httpMetrics middleware State transitions to DONE on response finish; duplicate finish events call inc only once; excluded paths leave state as PENDING
Cleanup _resetObservabilityStateForTesting creates fresh state

Verification commands

# Tests
node --experimental-vm-modules node_modules/jest/bin/jest.js \
  --forceExit --testTimeout=30000 \
  --testPathPattern="privacy-logger|tracing|httpMetrics|observabilityState"

# Type check (new files pass; pre-existing errors in webhookVerify.ts are unrelated)
npx tsc --noEmit

# Lint (new files pass; pre-existing warnings in privacy-logger.ts are unrelated)
npx eslint src/observability/observabilityState.ts \
  src/observability/httpMetrics.ts \
  src/observability/tracingMiddleware.ts

Design tradeoffs

Tradeoff Mitigation
WeakMap is not enumerable — can't inspect all active request states at runtime Acceptable for production; _resetObservabilityStateForTesting provides test-time access
transitionOperation is synchronous — if future state transitions need async work (e.g. persisting audit logs) The current design is intentionally synchronous for the hot path. Async recovery hooks can be added as a follow-up without changing the state machine
No automatic retry/recovery — the state machine records failures but doesn't retry Retries should be driven by the caller (route handler) based on the state, not by the observability layer. Automatic retry in observability would risk duplicate side effects

What could break

  • Tests that directly call res.emit('finish') multiple times — these will now see the idempotency guard in action (which is the correct behavior). Existing tests already expect single invocation.
  • Code that reads req.observabilityState directly — no such code exists; the state is accessed only through the exported API.
  • Middleware ordering in app.ts — the three observability middlewares are already mounted in a consistent order (httpMetricstracing → … → privacyLogger). The state machine doesn't depend on mount order; each operation tracks independently.

Follow-up worth filing separately

  1. Persist observability state to structured audit logs — currently state is in-memory only. A follow-up could write DONE/FAILED outcomes to the audit log for compliance.
  2. Add a /api/observability/state debug endpoint — expose isFullyResolved for debugging stuck requests (disabled by default, behind auth).
  3. Port remaining bun:test / node:test suites to Jest — the testPathIgnorePatterns list in jest.config.cjs still excludes ~20 test suites that target other runners.

…, atomic, and recoverable request lifecycle

The three privacy-safe observability modules (privacy-logger, tracingMiddleware,
httpMetrics) previously operated independently with no coordination. Each
attached its own `res.on('finish', …)` handler, creating three concrete
failure modes:

1. Duplicate event recording when `finish` fires more than once (retry,
   pipeline re-entry), inflating observability signals.
2. No atomic state transitions — a partial failure (e.g. serialization error
   in the privacy-logger) left tracing/metrics in an inconsistent state.
3. No recovery from interrupted operations — downstream components had no
   signal that an earlier phase had already completed or failed.

This introduces a shared, per-request state machine (`observabilityState.ts`)
that enforces deterministic transitions for logging, tracing, and metrics:

    PENDING → IN_PROGRESS → DONE
                     ↓
                   FAILED

Key changes:
- New `src/observability/observabilityState.ts`: WeakMap-based per-request
  state with idempotent transitions and terminal-state guards.
- `privacy-logger.ts`: Registers `in_progress` on mount; `done`/`failed` on
  finish. Idempotency guard prevents duplicate log lines. Catch block is
  defensive against state-transition failures and Date construction errors.
- `tracingMiddleware.ts`: Same pattern — `in_progress` on mount, `done` on
  finish, idempotent guard.
- `httpMetrics.ts`: Same pattern — `in_progress` on mount, `done` on finish,
  idempotent guard. Excluded paths remain unaffected (state stays PENDING).
- 30 new tests covering: normal transitions, idempotency guards, terminal
  state rejection, independent operation tracking, mixed terminal states,
  middleware integration (duplicate finish events), state on serialization
  failure, and cleanup helpers.
- Fixed pre-existing missing `jest` imports in `httpMetrics.test.ts` and
  `privacy-logger.test.ts` that caused all tests in those suites to fail.

Refs Disciplr-Org#1555

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
@drips-wave

drips-wave Bot commented Aug 30, 2026

Copy link
Copy Markdown

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., Closes #123), or by clicking a button below:

Issue Title
#1555 [Quality][High] Improve privacy-safe observability and tracing: transactional invariants and recovery Link to this issue

ℹ️ Learn more about linking PRs to issues

@1nonlypiece
1nonlypiece merged commit 2573a9d into Disciplr-Org:main Aug 30, 2026
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.

2 participants