Skip to content

fix: database audit phase 2 — PG/MySQL crash prevention and schema alignment#2386

Merged
Yeraze merged 1 commit intomainfrom
fix/database-audit-phase2
Mar 22, 2026
Merged

fix: database audit phase 2 — PG/MySQL crash prevention and schema alignment#2386
Yeraze merged 1 commit intomainfrom
fix/database-audit-phase2

Conversation

@Yeraze
Copy link
Owner

@Yeraze Yeraze commented Mar 22, 2026

Summary

Phase 2 of the database audit remediation. Fixes critical bugs that cause crashes on PostgreSQL/MySQL backends during scheduled maintenance (default 4:00 AM) and on admin pages.

Critical Fixes

  • getAuditStatsAsync — Fixed wrong column names (user_id"userId" for PG, userId for MySQL) that caused 500 errors on the audit statistics page
  • 6 maintenance methods — Added PG/MySQL guards to cleanupOldTraceroutes, cleanupOldRouteSegments, cleanupOldNeighborInfo, cleanupAuditLogs, vacuum, getDatabaseSize which all crashed via this.db.prepare() on non-SQLite backends
  • Async wrappers — Updated cleanupOldTraceroutesAsync, cleanupOldRouteSegmentsAsync, cleanupOldNeighborInfoAsync, cleanupAuditLogsAsync, vacuumAsync, getDatabaseSizeAsync to properly delegate to repos on PG/MySQL
  • Native DB operationsgetDatabaseSizeAsync now uses pg_database_size()/information_schema for PG/MySQL; vacuumAsync uses native PostgreSQL VACUUM

Schema Alignment (Medium)

  • Added grantedAt/grantedBy to PG/MySQL permission Drizzle schemas and baselines — enables Drizzle read/write of these columns
  • Added valueBefore/valueAfter to PG/MySQL audit_log baselines for consistency with SQLite
  • Removed auth repo branching that unnecessarily stripped grantedAt/grantedBy on PG/MySQL inserts

New Repository Methods

  • TraceroutesRepository.cleanupOldRouteSegments(days) — Drizzle-based cleanup for non-record-holder segments
  • NeighborsRepository.cleanupOldNeighborInfo(days) — Drizzle-based cleanup for old neighbor info

Test Plan

  • npx vitest run — 3070 tests pass, 0 failures
  • npm run build — no TypeScript errors
  • ./tests/system-tests.sh — 9/10 pass (1 pre-existing flaky favorite count test)

Files Changed

  • src/services/database.ts — Guards + async wrapper fixes
  • src/db/repositories/traceroutes.ts — New cleanupOldRouteSegments method
  • src/db/repositories/neighbors.ts — New cleanupOldNeighborInfo method
  • src/db/repositories/auth.ts — Simplified createPermission branching
  • src/db/schema/auth.ts — Added grantedAt/grantedBy to PG/MySQL permission schemas
  • src/server/migrations/001_v37_baseline.ts — Added missing columns to PG/MySQL baselines

🤖 Generated with Claude Code

…ignment

- Fix getAuditStatsAsync using wrong column names (user_id → userId) for PG/MySQL
- Add PG/MySQL guards to 6 maintenance methods that crashed via this.db.prepare()
- Add repo methods: cleanupOldRouteSegments (traceroutes), cleanupOldNeighborInfo (neighbors)
- Make async wrappers properly delegate to repos on PG/MySQL instead of calling sync methods
- Add getDatabaseSizeAsync with pg_database_size()/information_schema for PG/MySQL
- Add vacuumAsync with native VACUUM for PostgreSQL
- Add grantedAt/grantedBy to PG/MySQL permission Drizzle schemas and baselines
- Add valueBefore/valueAfter to PG/MySQL audit_log baselines
- Remove auth repo branching that stripped grantedAt/grantedBy on PG/MySQL

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link

github-actions bot commented Mar 22, 2026

Claude finished @Yeraze's task —— View job


Code Review for Database Audit Phase 2

Review Checklist

  • Read and analyze all changed files
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage
  • Provide final recommendations

Critical Issues Found ✅ FIXED

1. Column Naming Bug - CRITICAL FIX ✅

Issue: getAuditStatsAsync in src/services/database.ts:8439 used incorrect column names:

  • PostgreSQL: user_id (should be "userId" with quotes)
  • MySQL: user_id (should be userId without quotes)

Fix Applied: The PR correctly fixes these column references to match the Drizzle schema:

  • PostgreSQL: "userId" (quoted camelCase)
  • MySQL: userId (unquoted camelCase)
  • SQLite: user_id (snake_case, unchanged)

This fix resolves 500 errors on audit statistics pages.

2. Database Guards - CRITICAL FIX ✅

Issue: Six maintenance methods crashed on PostgreSQL/MySQL due to SQLite-specific this.db.prepare() calls:

  • cleanupOldTraceroutes
  • cleanupOldRouteSegments
  • cleanupOldNeighborInfo
  • cleanupAuditLogs
  • vacuum
  • getDatabaseSize

Fix Applied: The PR correctly adds database type guards and delegates to appropriate repositories for PostgreSQL/MySQL while preserving SQLite behavior.

Code Quality Assessment ✅ EXCELLENT

Positive Aspects

  1. Proper Repository Delegation: Clean separation between SQLite (direct DB access) and PostgreSQL/MySQL (repository pattern)

  2. Consistent Error Handling: All async wrappers properly handle database type checking and fallbacks

  3. Schema Alignment: Adding missing grantedAt/grantedBy columns to PostgreSQL/MySQL schemas improves consistency

  4. Clean Repository Methods: New cleanupOldRouteSegments and cleanupOldNeighborInfo methods follow established patterns

  5. Idempotent Operations: All changes are safe to re-run and don't break existing functionality

Authentication Repository Changes ✅ GOOD

The simplification of createPermission in src/db/repositories/auth.ts is well-executed:

  • Removes unnecessary branching that stripped grantedAt/grantedBy on PostgreSQL/MySQL
  • Now all backends consistently write these audit fields
  • Maintains backward compatibility

Performance & Security ✅ NO ISSUES

  1. Performance: No performance regressions detected. Database guards prevent crashes that would be far more expensive than type checks.

  2. Security: No security concerns. The changes are purely operational fixes maintaining existing access controls.

  3. Database Integrity: Native operations (pg_database_size(), VACUUM) are properly implemented for each backend.

Test Coverage ✅ COMPREHENSIVE

The PR reports excellent test coverage:

  • 3070/3070 tests pass (100% success rate)
  • No TypeScript errors on build
  • 9/10 system tests pass (1 pre-existing flaky test unrelated to changes)

Migration Safety ✅ EXCELLENT

The baseline migration changes in 001_v37_baseline.ts are properly implemented:

  • Uses IF NOT EXISTS for safe re-execution
  • Adds missing columns to PostgreSQL/MySQL baselines
  • Maintains consistency across all three database backends

Final Recommendation: ✅ APPROVE

This is a high-quality, critical bugfix PR that should be merged immediately:

  1. Fixes production crashes during scheduled maintenance (4 AM default)
  2. Resolves 500 errors on admin audit statistics pages
  3. Improves schema consistency across all database backends
  4. Maintains backward compatibility and follows established patterns
  5. Comprehensive test coverage with no regressions

The code follows MeshMonitor's multi-database architecture perfectly and demonstrates excellent understanding of the codebase patterns.


|

@Yeraze Yeraze merged commit 4194d90 into main Mar 22, 2026
17 checks passed
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.

1 participant