Skip to content

fix(persistence): correlate 5xx responses with request ids - #1601

Merged
wyuc merged 2 commits into
THU-MAIC:mainfrom
Ai-Eastern:fix/persistence-request-id-1574
Sep 20, 2026
Merged

wyuc merged 2 commits into
THU-MAIC:mainfrom
Ai-Eastern:fix/persistence-request-id-1574

Conversation

@Ai-Eastern

@Ai-Eastern Ai-Eastern commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Add correlation IDs to embedded persistence responses and log every returned 5xx with the HTTP method, request path, status, error code, and request ID.

This PR is AI-assisted. I reviewed the complete diff and verified the behavior locally before submission.

Related Issues

Fixes #1574

Changes

  • Reuse a valid incoming x-request-id or generate a UUID, then return it on every persistence response.
  • Emit one structured persistence error log for each 5xx response without consuming the client response body.
  • Add route-level coverage for 503 correlation logging and invalid request-ID replacement on a non-5xx response.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Refactoring (no functional changes)
  • CI/CD or build changes

Verification

Steps to reproduce / test

  1. Run pnpm vitest run tests/persistence/route.test.ts.
  2. Send a persistence request that returns 5xx and inspect the server log plus the response x-request-id header.
  3. Send a non-5xx request with an invalid x-request-id and confirm it is replaced without an error log.

What you personally verified

  • Confirmed the new tests failed before the route change because responses had no x-request-id.
  • Confirmed all 42 persistence route tests pass after the change.
  • Confirmed valid incoming IDs are echoed, invalid IDs are replaced with UUIDs, and 4xx responses do not produce 5xx logs.
  • Reviewed the diff for response-body consumption, route coverage, and unrelated file changes.

Evidence

  • CI passes (pnpm check && pnpm lint && npx tsc --noEmit)
  • Manually tested locally
  • Screenshots / recordings attached (if UI changes)

Local checks:

  • pnpm check — passed
  • pnpm lint — 0 errors (existing warnings remain in unchanged files)
  • pnpm exec tsc --noEmit — passed
  • pnpm vitest run tests/persistence/route.test.ts — 42 passed

No live server or UI was started; the route handlers were verified directly through route-level tests. Screenshots are not applicable.

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have added/updated documentation as needed
  • My changes do not introduce new warnings

@LING-6150
LING-6150 requested a review from wyuc September 19, 2026 21:37

@wyuc wyuc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — thanks @Ai-Eastern, this is exactly what #1574 asked for. Validating the inbound x-request-id against a bounded charset (and falling back to a UUID) keeps log injection out, only 5xx is logged with method/path/error-code/id, and reading the code via response.clone().json() is confined to the error path. Both behaviours are covered by tests.

Non-blocking: the echoed id is client-supplied when it passes the pattern, so without a trusted proxy in front two clients could pick the same value. Fine for correlation, worth keeping in mind if it ever feeds something stricter.

@wyuc
wyuc merged commit 33ff189 into THU-MAIC:main Sep 20, 2026
6 checks passed
looper1126-code pushed a commit to looper1126-code/OpenMAIC that referenced this pull request Nov 2, 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.

Persistence route answers 5xx (503 on PUT/PATCH/DELETE) with no server-side trace: log them with a request id

2 participants