fix(dashboard): audit_log never written + hero blind to annotation-only work #280

New Issue

Closed

opened 2026-04-19 19:10:47 +02:00 by marcel · 8 comments

marcel commented 2026-04-19 19:10:47 +02:00

Owner

Copy Link

Problem

Two bugs found while testing the dashboard after adding annotation segments:

Bug 1 — audit_log is silently never written

Symptom: The audit_log table is completely empty despite annotations and transcription blocks being saved successfully. The activity feed and pulse stats therefore show nothing.

Root cause: AuditService.logAfterCommit() registers a TransactionSynchronization.afterCommit() callback. Inside that callback, writeLog() calls auditLogRepository.save(), which is @Transactional (from SimpleJpaRepository). Spring tries to start a new transaction, which calls TransactionSynchronizationManager.initSynchronization(). But the outer transaction's synchronizations are still active at afterCommit() time — they are only cleared in afterCompletion(), which fires later. This throws:

IllegalStateException: Cannot activate transaction synchronization - already active

The catch (Exception e) in writeLog() silently swallows this and calls log.error(), so no row is ever written and no visible stack trace appears unless you watch stdout.

Fix: In the afterCommit() callback, submit the work to the auditExecutor thread pool directly instead of calling writeLog() inline. A fresh thread has no existing synchronization context and SimpleJpaRepository.save() can start a clean transaction normally.

// AuditService — inject the executor bean directly
@Autowired
@Qualifier("auditExecutor")
private TaskExecutor auditExecutor;

public void logAfterCommit(AuditKind kind, UUID actorId, UUID documentId, Map<String, Object> payload) {
    if (TransactionSynchronizationManager.isActualTransactionActive()) {
        TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
            @Override
            public void afterCommit() {
                // Run on a separate thread — avoids the "synchronization already active" conflict
                auditExecutor.execute(() -> writeLog(kind, actorId, documentId, payload));
            }
        });
    } else {
        writeLog(kind, actorId, documentId, payload);
    }
}

Note: calling this.log() (the @Async method) from within the same class won't work due to Spring AOP self-invocation. Injecting the executor directly avoids that limitation.

---

Bug 2 — Dashboard hero is blind to annotation-only work

Symptom: After adding annotation segments without saving transcription text, the hero ("resume" card) stays in empty state.

Root cause: AuditLogQueryRepository.findMostRecentDocumentIdByActor queries:

WHERE a.kind = 'TEXT_SAVED' AND a.actor_id = :userId

Annotations write ANNOTATION_CREATED events, not TEXT_SAVED. A document where the user only drew bounding boxes (no text typed yet) never appears in the hero.

Fix: Extend the IN clause to include ANNOTATION_CREATED:

WHERE a.kind IN ('TEXT_SAVED', 'ANNOTATION_CREATED') AND a.actor_id = :userId

The hero's progress calculation and excerpt already come from transcription blocks, so documents with only annotations will correctly show 0 % and no excerpt — that's the right behaviour for documents not yet transcribed.

---

Side note — REVOKE on audit_log had no effect

V46__add_audit_log.sql runs REVOKE UPDATE, DELETE ON audit_log FROM CURRENT_USER to enforce append-only at the DB layer. But archive_user is the table owner, and PostgreSQL owners always retain all privileges regardless of REVOKE. The statement ran without error but had no effect. A separate superuser role would be needed to enforce this properly. Not blocking, but the append-only guarantee is currently not enforced at the DB level.

---

Acceptance criteria

• Adding an annotation writes an ANNOTATION_CREATED row to audit_log
• The dashboard activity feed shows the annotation event
• After annotating a document (with no text saved), the hero resume card shows that document
• Existing tests pass; new integration tests cover both fixes

## Problem Two bugs found while testing the dashboard after adding annotation segments: ### Bug 1 — `audit_log` is silently never written **Symptom:** The `audit_log` table is completely empty despite annotations and transcription blocks being saved successfully. The activity feed and pulse stats therefore show nothing. **Root cause:** `AuditService.logAfterCommit()` registers a `TransactionSynchronization.afterCommit()` callback. Inside that callback, `writeLog()` calls `auditLogRepository.save()`, which is `@Transactional` (from `SimpleJpaRepository`). Spring tries to start a new transaction, which calls `TransactionSynchronizationManager.initSynchronization()`. But the outer transaction's synchronizations are still active at `afterCommit()` time — they are only cleared in `afterCompletion()`, which fires later. This throws: ``` IllegalStateException: Cannot activate transaction synchronization - already active ``` The `catch (Exception e)` in `writeLog()` silently swallows this and calls `log.error()`, so no row is ever written and no visible stack trace appears unless you watch stdout. **Fix:** In the `afterCommit()` callback, submit the work to the `auditExecutor` thread pool directly instead of calling `writeLog()` inline. A fresh thread has no existing synchronization context and `SimpleJpaRepository.save()` can start a clean transaction normally. ```java // AuditService — inject the executor bean directly @Autowired @Qualifier("auditExecutor") private TaskExecutor auditExecutor; public void logAfterCommit(AuditKind kind, UUID actorId, UUID documentId, Map<String, Object> payload) { if (TransactionSynchronizationManager.isActualTransactionActive()) { TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { @Override public void afterCommit() { // Run on a separate thread — avoids the "synchronization already active" conflict auditExecutor.execute(() -> writeLog(kind, actorId, documentId, payload)); } }); } else { writeLog(kind, actorId, documentId, payload); } } ``` Note: calling `this.log()` (the `@Async` method) from within the same class won't work due to Spring AOP self-invocation. Injecting the executor directly avoids that limitation. --- ### Bug 2 — Dashboard hero is blind to annotation-only work **Symptom:** After adding annotation segments without saving transcription text, the hero ("resume" card) stays in empty state. **Root cause:** `AuditLogQueryRepository.findMostRecentDocumentIdByActor` queries: ```sql WHERE a.kind = 'TEXT_SAVED' AND a.actor_id = :userId ``` Annotations write `ANNOTATION_CREATED` events, not `TEXT_SAVED`. A document where the user only drew bounding boxes (no text typed yet) never appears in the hero. **Fix:** Extend the `IN` clause to include `ANNOTATION_CREATED`: ```sql WHERE a.kind IN ('TEXT_SAVED', 'ANNOTATION_CREATED') AND a.actor_id = :userId ``` The hero's progress calculation and excerpt already come from transcription blocks, so documents with only annotations will correctly show 0 % and no excerpt — that's the right behaviour for documents not yet transcribed. --- ## Side note — REVOKE on `audit_log` had no effect `V46__add_audit_log.sql` runs `REVOKE UPDATE, DELETE ON audit_log FROM CURRENT_USER` to enforce append-only at the DB layer. But `archive_user` is the table **owner**, and PostgreSQL owners always retain all privileges regardless of REVOKE. The statement ran without error but had no effect. A separate superuser role would be needed to enforce this properly. Not blocking, but the append-only guarantee is currently not enforced at the DB level. --- ## Acceptance criteria - [ ] Adding an annotation writes an `ANNOTATION_CREATED` row to `audit_log` - [ ] The dashboard activity feed shows the annotation event - [ ] After annotating a document (with no text saved), the hero resume card shows that document - [ ] Existing tests pass; new integration tests cover both fixes

marcel added the bug label 2026-04-19 19:10:52 +02:00

marcel commented 2026-04-19 19:17:01 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Senior Application Architect

Observations

• The root cause is correctly identified. afterCommit() fires before afterCompletion(), so Spring's transaction synchronizations are still registered on the current thread when writeLog() tries to start a new @Transactional context via SimpleJpaRepository.save(). The silent catch (Exception e) buries the resulting IllegalStateException entirely.
• The fix correctly offloads the write to a new thread from auditExecutor. A fresh thread has no registered synchronizations, so save() can start a clean transaction normally.
• CallerRunsPolicy is a latent risk with the proposed fix. The auditExecutor is configured with core=1, max=2, queue=50, and CallerRunsPolicy. If the queue fills under load, auditExecutor.execute(() -> writeLog(...)) runs the lambda synchronously on the calling thread — which is the afterCommit() callback thread, and still has live synchronizations. Bug 1 would silently resurface under sustained write load at exactly the moment of highest audit activity.
• The proposed fix uses @Autowired @Qualifier("auditExecutor") private TaskExecutor auditExecutor — field injection. Every service in this codebase uses @RequiredArgsConstructor with final fields. This would be the first field-injected dependency in the service layer, inconsistent with the established pattern.
• The DB REVOKE side note is correctly identified. PostgreSQL table owners bypass REVOKE unconditionally. Non-blocking for this issue, but the migration comment should reflect reality.

Recommendations

• Fix the injection style. Declare private final TaskExecutor auditExecutor; with @Qualifier("auditExecutor") on the field. @RequiredArgsConstructor generates the constructor. No @Autowired needed.
• Change auditExecutor rejection policy from CallerRunsPolicy to AbortPolicy. Catch RejectedExecutionException in the afterCommit() callback and emit a distinct log.warn(). This prevents the fix from regressing under load and makes capacity exhaustion visible instead of silently running on the wrong thread.
• Update V46__add_audit_log.sql with a comment noting that the REVOKE is a no-op for table owners, so future developers don't rely on this guarantee.
• No package structure or service topology changes needed — the audit and dashboard packages are correctly feature-packaged and boundaries are clean.

## 🏛️ Markus Keller — Senior Application Architect ### Observations - The root cause is correctly identified. `afterCommit()` fires before `afterCompletion()`, so Spring's transaction synchronizations are still registered on the current thread when `writeLog()` tries to start a new `@Transactional` context via `SimpleJpaRepository.save()`. The silent `catch (Exception e)` buries the resulting `IllegalStateException` entirely. - The fix correctly offloads the write to a new thread from `auditExecutor`. A fresh thread has no registered synchronizations, so `save()` can start a clean transaction normally. - **`CallerRunsPolicy` is a latent risk with the proposed fix.** The `auditExecutor` is configured with core=1, max=2, queue=50, and `CallerRunsPolicy`. If the queue fills under load, `auditExecutor.execute(() -> writeLog(...))` runs the lambda synchronously on the calling thread — which is the `afterCommit()` callback thread, and still has live synchronizations. Bug 1 would silently resurface under sustained write load at exactly the moment of highest audit activity. - The proposed fix uses `@Autowired @Qualifier("auditExecutor") private TaskExecutor auditExecutor` — field injection. Every service in this codebase uses `@RequiredArgsConstructor` with `final` fields. This would be the first field-injected dependency in the service layer, inconsistent with the established pattern. - The DB REVOKE side note is correctly identified. PostgreSQL table owners bypass `REVOKE` unconditionally. Non-blocking for this issue, but the migration comment should reflect reality. ### Recommendations - **Fix the injection style.** Declare `private final TaskExecutor auditExecutor;` with `@Qualifier("auditExecutor")` on the field. `@RequiredArgsConstructor` generates the constructor. No `@Autowired` needed. - **Change `auditExecutor` rejection policy from `CallerRunsPolicy` to `AbortPolicy`.** Catch `RejectedExecutionException` in the `afterCommit()` callback and emit a distinct `log.warn()`. This prevents the fix from regressing under load and makes capacity exhaustion visible instead of silently running on the wrong thread. - **Update `V46__add_audit_log.sql`** with a comment noting that the REVOKE is a no-op for table owners, so future developers don't rely on this guarantee. - No package structure or service topology changes needed — the `audit` and `dashboard` packages are correctly feature-packaged and boundaries are clean.

marcel commented 2026-04-19 19:17:12 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

• The root cause diagnosis is correct. The bug is real and the fix is conceptually right. The existing AuditServiceTest.java mocks auditLogRepository via @ExtendWith(MockitoExtension.class) — the mock's save() never calls SimpleJpaRepository.save(), so the TransactionSynchronizationManager.initSynchronization() conflict is never triggered. All existing tests pass while the bug is live in production. This is the expected failure mode of mocking at the wrong boundary.
• The proposed fix uses field injection (@Autowired @Qualifier("auditExecutor") private TaskExecutor auditExecutor). Every service in this codebase uses @RequiredArgsConstructor — this would be an inconsistency. The correct pattern exists right next to it.
• Bug 2 is a one-line SQL fix: change WHERE a.kind = 'TEXT_SAVED' to WHERE a.kind IN ('TEXT_SAVED', 'ANNOTATION_CREATED') in AuditLogQueryRepository.findMostRecentDocumentIdByActor. Straightforward.
• AnnotationService.createOcrAnnotation() does not fire an ANNOTATION_CREATED event. The AC says "Adding an annotation writes an ANNOTATION_CREATED row." OCR annotations are system-generated, not user-initiated — scope should be explicitly resolved before implementation starts.
• AuditService.log() is annotated @Async("auditExecutor"). After injecting the executor directly, there will be two ways to submit work to auditExecutor — @Async proxy and direct execute(). Rationalise one path.

Recommendations

• Use constructor injection: declare private final TaskExecutor auditExecutor; with @Qualifier("auditExecutor") on the field. Lombok handles the rest. The @Async("auditExecutor") annotation on log() can stay if that method has separate callers, but document the two patterns explicitly.
• TDD sequence for Bug 1: write a red @SpringBootTest integration test that calls annotationService.createAnnotation() inside a @Transactional wrapper, waits with Awaitility, then asserts an audit_log row exists. The test must be red on the current code, green after the fix.

  @Test
  void logAfterCommit_writes_row_after_transactional_method_commits() {
      // call a @Transactional service that triggers logAfterCommit(...)
      await().atMost(5, SECONDS)
             .until(() -> auditLogRepository.count() > 0);
      assertThat(auditLogRepository.findAll())
          .extracting(AuditLog::getKind)
          .containsExactly(AuditKind.ANNOTATION_CREATED);
  }
  

• Rollback guard: write a companion test confirming no row is written when the outer transaction rolls back. This makes the after-commit semantics an explicit tested contract, not an assumption.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - The root cause diagnosis is correct. The bug is real and the fix is conceptually right. The existing `AuditServiceTest.java` mocks `auditLogRepository` via `@ExtendWith(MockitoExtension.class)` — the mock's `save()` never calls `SimpleJpaRepository.save()`, so the `TransactionSynchronizationManager.initSynchronization()` conflict is never triggered. All existing tests pass while the bug is live in production. This is the expected failure mode of mocking at the wrong boundary. - The proposed fix uses field injection (`@Autowired @Qualifier("auditExecutor") private TaskExecutor auditExecutor`). Every service in this codebase uses `@RequiredArgsConstructor` — this would be an inconsistency. The correct pattern exists right next to it. - Bug 2 is a one-line SQL fix: change `WHERE a.kind = 'TEXT_SAVED'` to `WHERE a.kind IN ('TEXT_SAVED', 'ANNOTATION_CREATED')` in `AuditLogQueryRepository.findMostRecentDocumentIdByActor`. Straightforward. - `AnnotationService.createOcrAnnotation()` does not fire an `ANNOTATION_CREATED` event. The AC says "Adding an annotation writes an `ANNOTATION_CREATED` row." OCR annotations are system-generated, not user-initiated — scope should be explicitly resolved before implementation starts. - `AuditService.log()` is annotated `@Async("auditExecutor")`. After injecting the executor directly, there will be two ways to submit work to `auditExecutor` — `@Async` proxy and direct `execute()`. Rationalise one path. ### Recommendations - **Use constructor injection:** declare `private final TaskExecutor auditExecutor;` with `@Qualifier("auditExecutor")` on the field. Lombok handles the rest. The `@Async("auditExecutor")` annotation on `log()` can stay if that method has separate callers, but document the two patterns explicitly. - **TDD sequence for Bug 1:** write a red `@SpringBootTest` integration test that calls `annotationService.createAnnotation()` inside a `@Transactional` wrapper, waits with Awaitility, then asserts an `audit_log` row exists. The test must be red on the current code, green after the fix. ```java @Test void logAfterCommit_writes_row_after_transactional_method_commits() { // call a @Transactional service that triggers logAfterCommit(...) await().atMost(5, SECONDS) .until(() -> auditLogRepository.count() > 0); assertThat(auditLogRepository.findAll()) .extracting(AuditLog::getKind) .containsExactly(AuditKind.ANNOTATION_CREATED); } ``` - **Rollback guard:** write a companion test confirming no row is written when the outer transaction rolls back. This makes the after-commit semantics an explicit tested contract, not an assumption.

marcel commented 2026-04-19 19:17:24 +02:00

Author

Owner

Copy Link

🔐 Nora "NullX" Steiner — Application Security Engineer

Observations

• Audit log failures are invisible by design — and that's a security problem. catch (Exception e) { log.error(...); } in writeLog() means dropped events produce only a log line. There is no counter, no alert, and no structured signal. An attacker who triggers load conditions that exhaust the executor queue produces no audit trail and no alarm. Audit logs only have value if their failure mode is itself observable.
• The CallerRunsPolicy is a regression vector for the proposed fix. The auditExecutor uses CallerRunsPolicy with a queue of 50. When the queue fills, auditExecutor.execute(() -> writeLog(...)) runs the lambda synchronously on the afterCommit() thread — which still has TransactionSynchronizationManager synchronizations registered. The original IllegalStateException returns, and is silently swallowed again. Under sustained write load, audit logging fails exactly when activity is highest.
• The REVOKE is a documented non-op. The append-only guarantee in V46__add_audit_log.sql does not exist at the database layer. archive_user owns the table and PostgreSQL owners cannot be stripped of privileges by REVOKE. Any application bug or compromised credential that can write to the database can also delete audit rows.
• Positive: The GDPR design is sound. ON DELETE SET NULL on actor_id means user deletion does not erase audit history, only attribution. The JSONB payload structure is flexible without schema migration cost.

Recommendations

• Change auditExecutor rejection policy to AbortPolicy. Catch RejectedExecutionException in the afterCommit() callback with a distinct log.warn("Audit event dropped — executor queue full: kind={}, documentId={}"). This makes capacity exhaustion an explicit observable signal rather than a silent bug.
• Add a Micrometer counter in writeLog()'s catch block: auditWriteFailures.increment(). Wire it to Prometheus. Dropped audit events then show up on the Grafana dashboard without log scraping.
• Add a comment to V46__add_audit_log.sql documenting the REVOKE limitation: -- NOTE: REVOKE has no effect for table owner. Append-only is enforced at application layer only. Future developers should not rely on a guarantee that doesn't exist.

## 🔐 Nora "NullX" Steiner — Application Security Engineer ### Observations - **Audit log failures are invisible by design — and that's a security problem.** `catch (Exception e) { log.error(...); }` in `writeLog()` means dropped events produce only a log line. There is no counter, no alert, and no structured signal. An attacker who triggers load conditions that exhaust the executor queue produces no audit trail and no alarm. Audit logs only have value if their failure mode is itself observable. - **The `CallerRunsPolicy` is a regression vector for the proposed fix.** The `auditExecutor` uses `CallerRunsPolicy` with a queue of 50. When the queue fills, `auditExecutor.execute(() -> writeLog(...))` runs the lambda synchronously on the `afterCommit()` thread — which still has `TransactionSynchronizationManager` synchronizations registered. The original `IllegalStateException` returns, and is silently swallowed again. Under sustained write load, audit logging fails exactly when activity is highest. - **The REVOKE is a documented non-op.** The append-only guarantee in `V46__add_audit_log.sql` does not exist at the database layer. `archive_user` owns the table and PostgreSQL owners cannot be stripped of privileges by `REVOKE`. Any application bug or compromised credential that can write to the database can also delete audit rows. - **Positive:** The GDPR design is sound. `ON DELETE SET NULL` on `actor_id` means user deletion does not erase audit history, only attribution. The JSONB payload structure is flexible without schema migration cost. ### Recommendations - Change `auditExecutor` rejection policy to `AbortPolicy`. Catch `RejectedExecutionException` in the `afterCommit()` callback with a distinct `log.warn("Audit event dropped — executor queue full: kind={}, documentId={}")`. This makes capacity exhaustion an explicit observable signal rather than a silent bug. - Add a Micrometer counter in `writeLog()`'s catch block: `auditWriteFailures.increment()`. Wire it to Prometheus. Dropped audit events then show up on the Grafana dashboard without log scraping. - Add a comment to `V46__add_audit_log.sql` documenting the REVOKE limitation: `-- NOTE: REVOKE has no effect for table owner. Append-only is enforced at application layer only.` Future developers should not rely on a guarantee that doesn't exist.

marcel commented 2026-04-19 19:17:37 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — Senior QA Engineer

Observations

• The existing test suite structurally cannot detect Bug 1. AuditServiceTest.java uses @ExtendWith(MockitoExtension.class) and mocks auditLogRepository. The mock's save() never invokes SimpleJpaRepository.save(), so Spring's transaction synchronization conflict is never triggered. All tests pass while the bug is live. This is a test pyramid gap: transaction lifecycle behavior requires a real Spring context and a real database — mocks cannot reproduce it.
• AC #4 requires "new integration tests cover both fixes." These tests must use @SpringBootTest with Testcontainers on postgres:16-alpine. H2 would not reproduce this bug either — the issue is in Spring's TransactionSynchronizationManager, which is framework-level and independent of the underlying database. But the SQL fix for Bug 2 uses native PostgreSQL (DISTINCT ON, date_trunc) and must be tested on real Postgres.
• Async timing: the fix makes writeLog() execute on the executor pool. Integration tests must use Awaitility — not Thread.sleep() — to wait for the async write to complete before asserting.
• Missing rollback coverage: there is no test confirming that a rolled-back transaction does NOT produce an audit row. The afterCommit() pattern should guarantee this, but it's an explicit contract that deserves a permanent regression test.

Recommendations

• Required test names:

  • logAfterCommit_writes_ANNOTATION_CREATED_row_after_transaction_commits()
  • logAfterCommit_writes_no_row_when_transaction_rolls_back()
  • findMostRecentDocumentIdByActor_returns_document_with_annotation_only_events()
  • dashboard_hero_resume_shows_annotation_only_document_with_zero_progress()

• Integration test structure for Bug 1 (must be red before the fix, green after):

  @SpringBootTest
  @Import(PostgresContainerConfig.class)  // real Postgres via Testcontainers
  class AuditServiceIntegrationTest {
      @Autowired AnnotationService annotationService;
      @Autowired AuditLogRepository auditLogRepository;
  
      @Test
      void logAfterCommit_writes_ANNOTATION_CREATED_row_after_transaction_commits() {
          annotationService.createAnnotation(/* test data */);
          await().atMost(5, SECONDS)
                 .until(() -> auditLogRepository.count() > 0);
          assertThat(auditLogRepository.findAll())
              .extracting(AuditLog::getKind)
              .containsExactly(AuditKind.ANNOTATION_CREATED);
      }
  }
  

• Consider adding DashboardQueryIntegrationTest for the SQL query changes in AuditLogQueryRepository. The existing DashboardControllerTest uses @WebMvcTest with a mocked service — it cannot verify that the native SQL query returns correct results for annotation-only documents.

## 🧪 Sara Holt — Senior QA Engineer ### Observations - **The existing test suite structurally cannot detect Bug 1.** `AuditServiceTest.java` uses `@ExtendWith(MockitoExtension.class)` and mocks `auditLogRepository`. The mock's `save()` never invokes `SimpleJpaRepository.save()`, so Spring's transaction synchronization conflict is never triggered. All tests pass while the bug is live. This is a test pyramid gap: transaction lifecycle behavior requires a real Spring context and a real database — mocks cannot reproduce it. - **AC #4 requires "new integration tests cover both fixes."** These tests must use `@SpringBootTest` with Testcontainers on `postgres:16-alpine`. H2 would not reproduce this bug either — the issue is in Spring's `TransactionSynchronizationManager`, which is framework-level and independent of the underlying database. But the SQL fix for Bug 2 uses native PostgreSQL (`DISTINCT ON`, `date_trunc`) and must be tested on real Postgres. - **Async timing:** the fix makes `writeLog()` execute on the executor pool. Integration tests must use Awaitility — not `Thread.sleep()` — to wait for the async write to complete before asserting. - **Missing rollback coverage:** there is no test confirming that a rolled-back transaction does NOT produce an audit row. The `afterCommit()` pattern should guarantee this, but it's an explicit contract that deserves a permanent regression test. ### Recommendations - **Required test names:** - `logAfterCommit_writes_ANNOTATION_CREATED_row_after_transaction_commits()` - `logAfterCommit_writes_no_row_when_transaction_rolls_back()` - `findMostRecentDocumentIdByActor_returns_document_with_annotation_only_events()` - `dashboard_hero_resume_shows_annotation_only_document_with_zero_progress()` - **Integration test structure for Bug 1** (must be red before the fix, green after): ```java @SpringBootTest @Import(PostgresContainerConfig.class) // real Postgres via Testcontainers class AuditServiceIntegrationTest { @Autowired AnnotationService annotationService; @Autowired AuditLogRepository auditLogRepository; @Test void logAfterCommit_writes_ANNOTATION_CREATED_row_after_transaction_commits() { annotationService.createAnnotation(/* test data */); await().atMost(5, SECONDS) .until(() -> auditLogRepository.count() > 0); assertThat(auditLogRepository.findAll()) .extracting(AuditLog::getKind) .containsExactly(AuditKind.ANNOTATION_CREATED); } } ``` - **Consider adding `DashboardQueryIntegrationTest`** for the SQL query changes in `AuditLogQueryRepository`. The existing `DashboardControllerTest` uses `@WebMvcTest` with a mocked service — it cannot verify that the native SQL query returns correct results for annotation-only documents.

marcel commented 2026-04-19 19:17:45 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

• This is a backend bug fix with no UI component changes. I checked DashboardService.getResume() and DashboardResumeDTO: the hero card's progress is calculated from transcription blocks (reviewed / total). For an annotation-only document, total = 0. The issue correctly anticipates this and calls 0% the right behaviour — but a zero denominator must be explicitly handled in the frontend progress calculation or the hero will display either a division-by-zero result or a blank progress bar.

Recommendations

• After the fix, manually verify the hero card with an annotation-only document:
  1. Progress bar shows 0% — not NaN, not invisible, not a broken element.
  2. The excerpt area renders a graceful empty state rather than an empty string creating layout collapse.
  3. The card title and document metadata still display correctly without transcription data present.
• Add a guard in the frontend progress computation (wherever the hero card calculates percentage): const progress = $derived(totalBlocks > 0 ? Math.round((reviewedBlocks / totalBlocks) * 100) : 0). This is a $derived expression — no $effect needed.

No accessibility, brand, or layout concerns with the backend changes themselves. The annotation-only edge case just needs a visual smoke-test after the fix ships.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations - This is a backend bug fix with no UI component changes. I checked `DashboardService.getResume()` and `DashboardResumeDTO`: the hero card's progress is calculated from transcription blocks (`reviewed / total`). For an annotation-only document, `total = 0`. The issue correctly anticipates this and calls 0% the right behaviour — but a zero denominator must be explicitly handled in the frontend progress calculation or the hero will display either a division-by-zero result or a blank progress bar. ### Recommendations - After the fix, manually verify the hero card with an annotation-only document: 1. Progress bar shows 0% — not NaN, not invisible, not a broken element. 2. The excerpt area renders a graceful empty state rather than an empty string creating layout collapse. 3. The card title and document metadata still display correctly without transcription data present. - Add a guard in the frontend progress computation (wherever the hero card calculates percentage): `const progress = $derived(totalBlocks > 0 ? Math.round((reviewedBlocks / totalBlocks) * 100) : 0)`. This is a `$derived` expression — no `$effect` needed. _No accessibility, brand, or layout concerns with the backend changes themselves. The annotation-only edge case just needs a visual smoke-test after the fix ships._

marcel commented 2026-04-19 19:17:53 +02:00

Author

Owner

Copy Link

🔧 Tobias Wendt — DevOps & Platform Engineer

Observations

• No Docker Compose, CI, or deployment changes required. This is a pure application code fix.
• CallerRunsPolicy is a code-level risk worth calling out from an operational perspective. The auditExecutor (core=1, max=2, queue=50, CallerRunsPolicy) means that under sustained write load — 50+ queued audit events — auditExecutor.execute(...) runs its lambda synchronously on the calling thread. If that thread is the afterCommit() callback thread (which still has transaction synchronizations registered), Bug 1 silently returns. This is not a DevOps fix, but it belongs in the AsyncConfig.java change accompanying this PR.
• The only current signal of audit failures is a log.error() line. If the application is running under Loki log aggregation, a saved search on "Failed to write audit log" would surface this. Currently, persistent audit log failures produce no alert and no dashboard signal. Under the current setup, this bug could have been silently failing for weeks with no visibility.

Recommendations

• After the fix ships, add a Loki alert rule on level=error msg=~"Failed to write audit log" so persistent failures trigger an alert. This is a 5-minute rule addition and gives the team early warning if the fix regresses.
• Change auditExecutor to AbortPolicy in AsyncConfig.java. Document in the config why CallerRunsPolicy is unsafe for this specific executor (it would reintroduce the synchronization conflict under load). A brief inline comment is justified here.
• No VPS sizing, backup, or CI pipeline changes needed for this issue.

## 🔧 Tobias Wendt — DevOps & Platform Engineer ### Observations - **No Docker Compose, CI, or deployment changes required.** This is a pure application code fix. - **`CallerRunsPolicy` is a code-level risk worth calling out from an operational perspective.** The `auditExecutor` (core=1, max=2, queue=50, `CallerRunsPolicy`) means that under sustained write load — 50+ queued audit events — `auditExecutor.execute(...)` runs its lambda synchronously on the calling thread. If that thread is the `afterCommit()` callback thread (which still has transaction synchronizations registered), Bug 1 silently returns. This is not a DevOps fix, but it belongs in the `AsyncConfig.java` change accompanying this PR. - **The only current signal of audit failures is a `log.error()` line.** If the application is running under Loki log aggregation, a saved search on `"Failed to write audit log"` would surface this. Currently, persistent audit log failures produce no alert and no dashboard signal. Under the current setup, this bug could have been silently failing for weeks with no visibility. ### Recommendations - After the fix ships, add a Loki alert rule on `level=error msg=~"Failed to write audit log"` so persistent failures trigger an alert. This is a 5-minute rule addition and gives the team early warning if the fix regresses. - Change `auditExecutor` to `AbortPolicy` in `AsyncConfig.java`. Document in the config why `CallerRunsPolicy` is unsafe for this specific executor (it would reintroduce the synchronization conflict under load). A brief inline comment is justified here. - No VPS sizing, backup, or CI pipeline changes needed for this issue.

marcel commented 2026-04-19 19:18:04 +02:00

Author

Owner

Copy Link

🗳️ Decision Queue — Action Required

1 decision needs your input before implementation starts.

Scope

• Should createOcrAnnotation() fire ANNOTATION_CREATED events? — AnnotationService.createOcrAnnotation() (the OCR-pipeline path) does not call auditService.logAfterCommit(), unlike the user-initiated createAnnotation(). The AC says "Adding an annotation writes an ANNOTATION_CREATED row" — this is ambiguous about system-generated annotations. If OCR-created annotations count as activity: include them and they'll appear in the activity feed and hero. If they don't count (they are system output, not user work): exclude them and the current gap is intentional. The right answer depends on how you want the activity feed to represent OCR work. (Raised by: Felix)

## 🗳️ Decision Queue — Action Required _1 decision needs your input before implementation starts._ ### Scope - **Should `createOcrAnnotation()` fire `ANNOTATION_CREATED` events?** — `AnnotationService.createOcrAnnotation()` (the OCR-pipeline path) does not call `auditService.logAfterCommit()`, unlike the user-initiated `createAnnotation()`. The AC says "Adding an annotation writes an `ANNOTATION_CREATED` row" — this is ambiguous about system-generated annotations. If OCR-created annotations count as activity: include them and they'll appear in the activity feed and hero. If they don't count (they are system output, not user work): exclude them and the current gap is intentional. The right answer depends on how you want the activity feed to represent OCR work. _(Raised by: Felix)_

marcel commented 2026-04-19 19:20:21 +02:00

Author

Owner

Copy Link

Decision closed: createOcrAnnotation() will NOT fire ANNOTATION_CREATED events. OCR-generated annotations are system output, not user activity. The current gap is intentional — exclude from scope.

**Decision closed:** `createOcrAnnotation()` will NOT fire `ANNOTATION_CREATED` events. OCR-generated annotations are system output, not user activity. The current gap is intentional — exclude from scope.

marcel closed this issue 2026-04-20 13:04:27 +02:00

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

main

feat/issue-533-mass-import-ux

worktree-feat+issue-557-upload-artifact-v3-pin

worktree-chore+issue-556-drop-client-branches-coverage-gate

fix/issue-514-prerender-crawl-bakes-redirects

fix/issue-472-prerender-entries

feat/issue-395-readme

feat/issue-345-bulk-mark-reviewed

feat/issue-344-bell-tooltip

feat/issue-341-annotieren-contrast

feat/issue-225-bulk-metadata-edit

feat/issue-317-bulk-upload

feat/issue-271-dashboard-redesign

docs/issue-240-mission-control-spec

refactor/issues-193-200

feat/issue-162-korrespondenz-redesign

feature/68-new-document-file-first

feat/81-discussion-discoverability

feat/62-document-bottom-panel

feature/56-backfill-file-hashes

feat/38-document-edit-history

fix/svelte5-test-delegation-and-login-auth

No results found.

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label bug

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#280