Skip to content

Conversation

@kevalyq
Copy link
Contributor

@kevalyq kevalyq commented Nov 1, 2025

Problem

Issue #62 reported intermittent test failures (~1-10% failure rate) when running tests with php artisan test --parallel. The error was:

RuntimeException: Failed to unwrap idx_key

Root Cause

All tests shared the same KEK (Key Encryption Key) file at storage/app/keys/kek.key. When multiple test processes ran in parallel:

  • Each process would generate and save its own KEK file
  • Processes overwrote each other's KEK files
  • This caused decryption failures when a process tried to unwrap keys encrypted with a different KEK

Solution

Implemented Option 1 from Issue #62 (Per-Process KEK Files) by making the KEK file path configurable:

Changes

  1. app/Models/TenantKey.php:

    • Added protected static ?string $kekPath = null property
    • Modified getKekPath() to check the static property before returning default path
    • Added public static function setKekPath(?string $path): void to allow tests to set custom KEK paths
  2. tests/Feature/TenantKeyTest.php:

    • Added getProcessKekPath() helper returning storage/app/keys/kek-test-{pid}.key
    • Updated beforeEach() to set process-specific KEK path
    • Updated afterEach() to cleanup KEK file and reset path
  3. tests/Feature/PersonTest.php:

    • Added getPersonTestKekPath() helper returning storage/app/keys/kek-test-{pid}.key
    • Updated beforeEach() to set process-specific KEK path
    • Updated afterEach() to cleanup KEK file and reset path

Each test process now uses its own unique KEK file identified by process ID, completely eliminating race conditions.

Testing

Validated the fix with 10 consecutive parallel test runs:

for i in {1..10}; do 
  echo "=== Run $i/10 ==="
  ddev exec php artisan test --parallel
done

Results:

  • 10/10 runs successful (79 tests, 186 assertions each)
  • Zero intermittent failures (eliminated the ~1-10% failure rate)
  • ✅ Duration: ~7-8 seconds per run with 2 parallel processes
  • ✅ PHPStan Level 9: Clean (0 errors)
  • ✅ Pint PSR-12: Compliant (50 files)

Acceptance Criteria from Issue #62

  • ✅ Tests pass consistently with php artisan test --parallel
  • ✅ No test isolation violations (process-specific KEK files ensure isolation)
  • ✅ Parallel execution enabled (no need to disable)

Fixes #62

Implemented process-specific KEK files to eliminate race conditions
in parallel test execution.

Root cause:
- All tests shared the same KEK file (storage/app/keys/kek.key)
- Parallel processes overwrote each other's KEK files
- Led to 'Failed to unwrap idx_key' errors (~1-10% failure rate)

Solution:
- Added TenantKey::setKekPath() to support custom KEK paths
- Modified getKekPath() to use static $kekPath property when set
- Updated TenantKeyTest and PersonTest to use process-specific paths:
  storage/app/keys/kek-test-{pid}.key
- Added cleanup in afterEach hooks

Testing:
- Verified 10 consecutive successful parallel test runs (79/79 pass)
- Tests run in 7-8s with 2 parallel processes
- PHPStan Level 9: Clean
- Pint PSR-12: Compliant

Fixes #62
Copilot AI review requested due to automatic review settings November 1, 2025 22:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for parallel test execution by introducing process-specific KEK (Key Encryption Key) file paths to prevent file conflicts when running tests concurrently.

  • Adds a configurable KEK path to the TenantKey model via setKekPath() method
  • Implements process-specific KEK file paths in tests using getmypid() to ensure isolation
  • Updates test setup/teardown hooks to manage process-specific KEK files

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
app/Models/TenantKey.php Adds a static $kekPath property and setKekPath() method to allow overriding the default KEK file path for testing
tests/Feature/TenantKeyTest.php Implements process-specific KEK paths using getProcessKekPath() helper function and updates beforeEach/afterEach hooks to set and reset the KEK path
tests/Feature/PersonTest.php Implements process-specific KEK paths using getPersonTestKekPath() helper function, includes directory creation logic, and adds afterEach cleanup hook
Comments suppressed due to low confidence (1)

tests/Feature/TenantKeyTest.php:50

  • This hardcoded path should use getProcessKekPath() instead to match the process-specific KEK path that was set in beforeEach. The current code will check the wrong file location and cause test failures in parallel execution.
    $kekPath = storage_path('app/keys/kek.key');

TenantKey::generateKek() already handles directory creation,
so the manual mkdir in PersonTest.php beforeEach is unnecessary.
This simplifies the code and reduces duplication.

Addresses Copilot review comment on PR #63.
@kevalyq kevalyq merged commit 50dd446 into main Nov 1, 2025
12 checks passed
@kevalyq kevalyq deleted the fix/issue-62-parallel-test-failures branch November 1, 2025 23:03
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.

Parallel test execution causes intermittent TenantKey unwrap failures

2 participants