# Payroll / Punch / Face-Attendance transaction defects

**Raised by:** Storage Engine & Transaction Boundary Audit (`STORAGE_ENGINE_TRANSACTION_AUDIT.md`)
**Status:** **REMEDIATED** (2026-07-14) — see "Fixes applied" below.
**Engines:** all payroll/punch tables were already InnoDB. **No engine was changed.** These were pure logic defects.

---

## Step 1 — Traced workflows (as found)

### W1. `EmployeePunchRecordController::moveAllPayrollData()` — POST `/api/payroll/generate`

| | |
|---|---|
| **Tables written** | `save_final_history_payroll_data` (insert), `record_payroll_dates` (update `isPayrollGenerate`), `save_emp_punch_details_records` (insert), `employee_punch_details_records` (update), `save_history_payroll_records` (insert), `history_payroll_records` (**HARD DELETE**) |
| **Transaction (before)** | `beginTransaction()` at the very top — *before* validation. `commit()` at the end. **`catch` had NO `rollBack()`.** |
| **Failure points** | (a) validation failure `return`ed with the transaction still open; (b) any exception in the copy loops left the transaction open **and unrolled**, with `history_payroll_records` rows already deleted |
| **Delete type** | **Hard**, and `HistoryPayrollRecord` has no `SoftDeletes` |
| **Expected atomic outcome** | All six tables commit together, or none |

### W2. `EmployeePunchDetailsController::recordPunch()` — POST `/api/punch/details` (fingerprint device)

| | |
|---|---|
| **Tables written** | `employee_punch_details` (insert), `employee_punch_details_records` (insert/update), `emp_admin_edit_punch_records` (insert), `const_records` (update) |
| **Transaction (before)** | **None at all.** Yet it called `DB::rollBack()` on the reject path — a no-op over already-committed writes |
| **Failure points** | The `employee_punch_details` row was created and committed *first*; a rejected punch-out then left it behind as an **orphan** |
| **External I/O** | `Mail::to()->send()` executed mid-workflow |

### W3. `FaceEmpController::recordPunchFacialData()` — POST `/api/facialpunch/submit`
### W4. `FaceEmpController::storeLocall()` — POST `/api/facialpunch/adminsubmit`

| | |
|---|---|
| **Tables written** | `employee_punch_details`, `punch_out_of_locations`, `employee_punch_details_records`, `emp_admin_edit_punch_records`, `const_records` (+ `employee_delay_records` in W4) |
| **Transaction (before)** | **None.** `FaceEmpController` contained zero `beginTransaction()`/`commit()` calls, yet called `DB::rollBack()` |
| **External I/O** | Mail sent inside the (non-existent) boundary |

### W5. Dead clones — `fingerprint()` and `testingPunch()`

Both call `DB::rollBack()` with no transaction. **Neither is routed** — `fingerprint()` has no route at all and `testingPunch()`'s route is commented out (`routes/api.php:176`). They are byte-identical copies of `recordPunch()`.

---

## Root cause per method

| Method | Root cause |
|---|---|
| `moveAllPayrollData()` | `catch` block omitted `DB::rollBack()`; validation ran *inside* the transaction |
| `recordPunch()` | No transaction was ever opened; `DB::rollBack()` was decorative |
| `FaceEmpController::recordPunchFacialData()` | Same — no transaction anywhere in the class |
| `FaceEmpController::storeLocall()` | Same |
| `fingerprint()`, `testingPunch()` | Same, but unreachable (dead code) |

### Additional defect found while testing (not in the original ticket)

**The 404 "no active punch-in" branch was unreachable dead code.** In `recordPunch()` and both `FaceEmpController` punch methods, `$record->reScheduleId` and `$record->scheduleId` were dereferenced **before** the `if ($record)` null check. So a punch-out with no active punch-in did not return 404 — it **fatalled** with a null-property error, *after* the `employee_punch_details` row had already been committed. That fatal is precisely what produced the orphan rows. Fixed by moving the lookups inside the null check.

---

## Fixes applied

### Transaction pattern used

Explicit, correctly-paired `beginTransaction()` / `commit()` / `rollBack()`, with `catch (\Throwable $e)` (not `Exception`, so a `TypeError`/`Error` cannot skip the rollback either). Exceptions are **not** swallowed — they are converted to a 500 with the message, never a generic success.

```php
DB::beginTransaction();
$pendingMails = [];

try {
    ... all related writes ...
    DB::commit();
} catch (\Throwable $e) {
    DB::rollBack();
    return response()->json(['code' => 500, 'status' => false, 'message' => '...'], 500);
}

// external side effects only AFTER commit
foreach ($pendingMails as $pending) {
    Mail::to($pending['to'])->send($pending['mail']);
}
```

### Per method

| Method | Fix |
|---|---|
| `moveAllPayrollData()` | Validation moved **before** `beginTransaction()`; `DB::rollBack()` added to the catch; catch widened to `\Throwable`; **idempotency guard** added |
| `recordPunch()` | Real transaction around all writes; null-check reordered so the 404 path works; mail deferred until after commit |
| `FaceEmpController::recordPunchFacialData()` | Real transaction (was none); null-check reordered; mail deferred |
| `FaceEmpController::storeLocall()` | Real transaction (was none); null-check reordered; mail deferred |
| `EmployeePunchDetailsController::storeLocall()` | Early `return` on "punch-out before punch-in" now rolls back first (it leaked an open transaction) |
| `fingerprint()`, `testingPunch()` | Unmatched `DB::rollBack()` calls **removed** and replaced with a comment marking them dead/unrouted. See "Remaining risks" |

### External side effects

Mail is now collected into `$pendingMails` and dispatched **only after commit** in `recordPunch()`, `recordPunchFacialData()` and `FaceEmpController::storeLocall()`. A punch that rolls back can no longer notify anyone.

---

## Deletion behaviour — before / after: **UNCHANGED, deliberately**

`moveAllPayrollData()` hard-deletes `history_payroll_records`. **This was left as-is**, and that is the correct call:

- It is the **source side of a move, not a destruction**. Each row is copied into `save_history_payroll_records` (the archive) immediately before `$record->delete()`, inside the same transaction. The payroll history is preserved — it moves from the working table to the archive.
- **Switching it to a soft delete would introduce a bug.** The source query is
  `HistoryPayrollRecord::whereBetween('recordDate', [$start, $end])->get()` — it does **not** filter on `isDelete`. Soft-deleted rows would therefore be re-selected on the next run and archived a second time, producing duplicate history. Making it safe would require adding `isDelete` filters to every read path of that model.
- With the transaction now correct, the hard delete is fully reversible on failure — proven by the test below, which asserts the deleted rows come back.

**Impact on reports: none.** No behaviour changed; only the failure path changed (from "history destroyed" to "history restored").

---

## History safety

| Requirement | Status |
|---|---|
| Completed payroll history never silently lost | **Yes** — archived before delete, and the whole move is atomic |
| A failed payroll operation cannot leave duplicate history | **Yes** — rollback removes any partial archive rows |
| Retrying after a failure is safe | **Yes** — a failed run also rolls back `isPayrollGenerate`, so the period stays retryable |
| No double-payroll generation | **Yes** — new idempotency guard (below) |

### Idempotency guard added

Nothing in `moveAllPayrollData()` filtered on `isGeneratePayroll` / `isPayrollGenerate`, so **re-running it for the same period would insert a second set of `save_final_history_payroll_data` and `save_emp_punch_details_records` rows — double payroll.**

Guard added using the **existing** `record_payroll_dates.isPayrollGenerate` column (no schema change):

```php
if ($payrollId && RecordPayrollDate::where('id', $payrollId)->value('isPayrollGenerate')) {
    return response()->json([... 'Payroll has already been generated for this period.'], 200);
}
```

Combined with the now-correct transaction, this gives: a **failed** run is retryable (the flag rolls back), a **completed** run is refused.

> **Reported, not added:** there is no unique constraint on `save_final_history_payroll_data (empid, payrollId)` or `save_emp_punch_details_records (empid, payrollId)`. The application-level guard above closes the practical hole, but a DB-level unique key would make double-payroll structurally impossible. That is a **schema change** and is therefore reported here rather than applied.

---

## Tests

`tests/Feature/PayrollPunchTransactionTest.php` — **6 passed, 35 assertions**

| # | Test | Proves |
|---|---|---|
| 1 | Rejected punch-out leaves no orphan punch row | Drives the **real** `POST /api/punch/details` endpoint. Asserts 404, **no orphan `employee_punch_details` row**, and `Mail::assertNothingSent()`. **This test fails on the old code.** |
| 2 | Successful punch-in commits all writes | Happy path: punch row + active record + audit row all committed |
| 3 | Payroll move failure restores source and writes no archive | Runs the real write sequence, asserts in-transaction that the archive exists and the source **is** hard-deleted, then throws. After rollback: **hard-deleted history is back**, no partial archive, and `isPayrollGenerate` is false so a retry is allowed |
| 4 | Payroll cannot be generated twice for the same period | Drives the real controller method; the guard refuses and writes nothing |
| 5 | Face punch failure rolls back all four tables | Fails after 4 writes; `employee_punch_details`, `employee_punch_details_records`, `emp_admin_edit_punch_records` and `const_records` all return to their original state |
| 6 | No controller calls `rollBack()` without opening a transaction | Static invariant across all three controllers — pins the defect so it cannot return |

Every failure test asserts **inside the open transaction** that the writes really landed before throwing, so none can pass vacuously.

**Full suite: 76 passed, 1 failed** (up from 70/1). The single failure is the unrelated, pre-existing `EmailProviderTest` fixture bug.

---

## Remaining payroll risks

1. **`fingerprint()` and `testingPunch()` are dead code.** Unrouted byte-identical clones of `recordPunch()`, still carrying the original no-transaction shape. Their misleading `DB::rollBack()` calls were removed, but the methods themselves remain. **Recommend deleting both** — that is a destructive change and needs sign-off.
2. **Mail still inside the transaction** in `EmployeePunchDetailsController::store()`, `storeLocall()` and `destroy()`. These three already had correct `begin/commit/rollBack` pairing, so they are atomic — but they hold the transaction open across an SMTP round-trip and can email about a punch that later rolls back. Same `$pendingMails` fix applies; not done here to keep this change reviewable.
3. **No DB-level unique key** on the payroll archive tables (see above).
4. **`ProgramController::cloneProgram()`** has the identical "return after `beginTransaction()` with no rollback" bug (L317-327). It is not payroll, so it was left alone — it belongs to Batch 4.