Skip to content

Commit c929386

Browse files
authored
Record policy-run failures as durable, actionable events (Review Flow PR 1) (#7269)
# Description of Changes PR 1 of the failure-notification work: a durable, team-scoped record of **why a policy run failed**, surfaced in the portal with the triage actions each failure allows. Today a failed policy run is not quite invisible, but it is unusable: the ledger marks the file `ERROR`, and the audit aspect keeps the exception message and status code. Nothing classifies either one, nothing surfaces them, and neither offers a next step. If the file came from a folder, bucket or webhook there is also no user watching, so nobody learns it never made it through. This adds the record and the read surface; the remediation that acts on documents comes later (see below). ## What this does **A failure kind registry as data.** `FailureKind` describes what can go wrong: a stable wire id, i18n keys, an English fallback, and four facets the review surface needs (`Stage`, `Severity`, `Remedy`, `Scope`). It is shaped like the existing `ExceptionUtils.ErrorCode` and *links* to that vocabulary rather than replacing it. **Classification off structured codes, not message matching.** Policy steps dispatch over loopback HTTP, so a tool's 4xx arrives as a `RestClientResponseException` whose body is the Problem Details document carrying `errorCode`. `FailureClassifier` reads that. Anything unrecognised becomes `UNKNOWN`, which is the point: every failed run gets an addressable record from day one, and which kinds to promote next is answered by production frequency rather than guesswork. **Actions declared by a kind, implemented as beans.** A kind lists the `FailureActionId`s it offers; behaviour lives in `FailureAction` beans resolved by id — the idiom this codebase already uses for `InputSource`, `PolicyOutputSink` and `PolicyTrigger`. A kind cannot be sent an action it never declared (400), so an incoherent pairing is unreachable rather than merely unrendered. A new kind ships as a registry entry plus copy: no new endpoint, no UI change. **Repeat folding.** Recording folds a genuine repeat into the existing incident instead of inserting again, keyed on `(team_id, dedup_key)`. That matters for a snapshot-mode source that re-lists every file on each sweep: the same broken file is one incident, not one per sweep. Distinct files keep distinct rows. The unique constraint is enforced by the database, and a writer that loses the insert race folds into the winner's row. One granularity caveat worth naming: nothing populates `file_id` in this PR, so every row has it NULL. A FILE-scoped kind therefore dedups on `policy + run` rather than `policy + file`. That still yields one row per document for the sources shipped here, because the folder, S3 and webhook sources each start one run per file; it stops holding as soon as a single run carries several documents, which is why editor-origin reporting (item 3 below) populates `file_id`. **No document identity is stored.** No file name, no content. `fileId` is an opaque reference only the owner's own client can resolve locally. `detail` keeps the raw message (the only diagnostic an `UNKNOWN` failure has) with anything path- or filename-shaped stripped on the way in, capped at 2,000 characters. `PolicyExecutor`'s type-mismatch message now reports the *extension* rather than the filename, since that message becomes the stored `detail`. **Access.** Reads and triage are leader-only, gated exactly the way `PolicyController` gates policy editing, with the single-user carve-out when login is disabled. Every read and write is scoped to the caller's own team from the authenticated principal — there is no team parameter on the API. Self-hosted needs no migration: the table is created from the entity by `ddl-auto=update`, as with every other table. ## What this does not do yet - **Actions are incident dispositions, not document dispositions.** Acknowledge and Dismiss change how a failure is displayed and touch nothing else — not the document, not the processed-file ledger, not the run, not any output destination. That is what makes them safe to offer against `UNKNOWN`, and why there is no Approve/Release yet. - **Two kinds only.** `INPUT_PASSWORD_PROTECTED` and `UNKNOWN`. Everything else classifies as `UNKNOWN` and shows its raw message. - **Editor-origin failures are not reported.** Every row is `PROCESSOR`. `FailureOrigin.EDITOR` and `API` exist in the enum but nothing writes them. - **The list is dev-only for now.** The section renders behind `import.meta.env.DEV`, so it ships in no production bundle. The endpoints are live and gated. - **No retention or per-team cap** on `file_run_events`. Tracked separately. - **No suspend-and-prompt.** `PolicyInputRequiredException` and the engine's `suspend()` exist but nothing throws it, so a run cannot pause to ask for a password today. - **SaaS needs a migration** in `Stirling-PDF-SaaS` (`CREATE TABLE IF NOT EXISTS stirling_pdf.file_run_events`), per the convention documented at `app/saas/src/main/resources/application-saas.properties:21`. ## What follows in later PRs 1. **Map the remaining error codes to specific kinds** — corrupted file, OCR unavailable, output destination unreachable, entitlement refusals, and so on — each with its own copy and its own action set, replacing today's `UNKNOWN` catch-all with a named notification in the review UI. 2. **Real remediation actions** attached to those kinds: fix (supply a password and resume), skip (drop this file, continue the batch), and decline (reject an incoming file outright), acting on the held document rather than only on the incident row. This is where the suspend-and-prompt path gets wired. 3. **Editor-origin reporting**, so a failure a user hits in the editor lands in the same queue as one from a bucket. 4. **The user-facing review surface**: notifications with a sticky review section, per-file badges, and an export gate, with the dev-only list here replaced by the real thing. ## How to test Needs a SaaS or proprietary build with login enabled, and an account that leads a team. 1. Create a policy in the Processor with any step (Auto-redact is fine) and a source you can drop files into. 2. Upload two files that will fail it: **a password-protected PDF**, and **a corrupted PDF** (truncate a valid one, or rename a `.csv` to `.pdf`). 3. Let the policy run and fail on both. 4. Go to the portal's **Documents** view and scroll to **Failures** (dev builds only). Expect two rows: - **Password-protected document** — classified from `E004`, with the kind's own labels **"I'll unlock this"** and **"Skip this file"** rather than generic wording. - **Unrecognised failure** — the corrupted file, classified `UNKNOWN` (`E001` is not claimed by a kind yet), showing its raw message with generic **Acknowledge** / **Dismiss**. Neither row contains a file name anywhere, including in the raw message. Press **Show raw JSON** to read exactly what the server returned. Acting on a row transitions it and comes back with both buttons disabled and a reason. Re-running the same batch increments the occurrence count on the existing rows rather than adding new ones; two *different* password-protected files produce two separate rows. --- ## Checklist ### General - [ ] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [ ] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md) (if applicable) - [ ] I have read the [How to add new languages to Stirling-PDF](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md) (if applicable) - [ ] I have performed a self-review of my own code - [ ] My changes generate no new warnings ### Documentation - [ ] I have updated relevant docs on [Stirling-PDF's doc repo](https://github.com/Stirling-Tools/Stirling-Tools.github.io/blob/main/docs/) (if functionality has heavily changed) - [ ] I have read the section [Add New Translation Tags](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md#add-new-translation-tags) (for new translation tags only) ### Translations (if applicable) - [ ] I ran [`scripts/counter_translation.py`](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/docs/counter_translation.md) ### UI Changes (if applicable) - [ ] Screenshots or videos demonstrating the UI changes are attached (e.g., as comments or direct attachments in the PR) ### Testing (if applicable) - [ ] I have run `task check` to verify linters, typechecks, and tests pass
1 parent 0ff4ef6 commit c929386

71 files changed

Lines changed: 6500 additions & 46 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
package stirling.software.proprietary.failure;
2+
3+
import java.util.List;
4+
import java.util.Map;
5+
6+
import org.springframework.stereotype.Component;
7+
8+
import lombok.RequiredArgsConstructor;
9+
10+
/**
11+
* "Seen, and I own it." Moves {@code NEW} to {@code ACKNOWLEDGED} so the row stops counting as
12+
* unread while staying in the open list. Touches only the status, not the document, ledger or run.
13+
*
14+
* <p>Re-acknowledging is a no-op that keeps the original actor and timestamp, so the first person
15+
* to pick it up stays credited.
16+
*/
17+
@Component
18+
@RequiredArgsConstructor
19+
public class AcknowledgeAction implements FailureAction {
20+
21+
private final FileRunEventStore store;
22+
23+
@Override
24+
public FailureActionId id() {
25+
return FailureActionId.ACKNOWLEDGE;
26+
}
27+
28+
@Override
29+
public FileRunEvent execute(FileRunEvent event, Map<String, String> inputs, String actor) {
30+
// Guarded on NEW so a racing acknowledger cannot re-stamp the row, and so re-acknowledging
31+
// returns the first actor's row rather than taking their credit.
32+
return store.applyStatusOnce(
33+
event.id(),
34+
event.teamId(),
35+
FileRunEventStatus.ACKNOWLEDGED,
36+
actor,
37+
List.of(FileRunEventStatus.NEW));
38+
}
39+
}
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
package stirling.software.proprietary.failure;
2+
3+
import java.util.Map;
4+
5+
import org.springframework.stereotype.Component;
6+
7+
import lombok.RequiredArgsConstructor;
8+
9+
/**
10+
* "No remediation will happen; close it." Terminal. Named Dismiss rather than Reject because it
11+
* closes the incident, not the document: nothing is deleted or delivered, the ledger row is left
12+
* alone, and a different file failing the same way still opens its own incident.
13+
*
14+
* <p>Recurrences of this exact failure fold onto the dismissed row, which is what makes "stop
15+
* showing me this" hold for a source that re-lists the same failing file each sweep.
16+
*/
17+
@Component
18+
@RequiredArgsConstructor
19+
public class DismissAction implements FailureAction {
20+
21+
private final FileRunEventStore store;
22+
23+
@Override
24+
public FailureActionId id() {
25+
return FailureActionId.DISMISS;
26+
}
27+
28+
@Override
29+
public FileRunEvent execute(FileRunEvent event, Map<String, String> inputs, String actor) {
30+
return store.applyStatus(event.id(), event.teamId(), FileRunEventStatus.DISMISSED, actor);
31+
}
32+
}
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
package stirling.software.proprietary.failure;
2+
3+
import java.util.Map;
4+
5+
/**
6+
* The behaviour behind one {@link FailureActionId}. Implementations are Spring beans injected as a
7+
* {@code List} and resolved by id, the pattern already used for {@code InputSource}, {@code
8+
* PolicyOutputSink} and {@code PolicyTrigger}.
9+
*
10+
* <p>Keeping behaviour out of the registry keeps that pure data, so a new kind ships by declaring
11+
* an action id that already has a handler.
12+
*/
13+
public interface FailureAction {
14+
15+
FailureActionId id();
16+
17+
/**
18+
* Apply the action and return the updated event. {@code inputs} carries whatever the action
19+
* declared it needs, which is nothing for the two that exist today. Implementations leave the
20+
* document, ledger, run and output destinations alone unless that is the action's purpose.
21+
*/
22+
FileRunEvent execute(FileRunEvent event, Map<String, String> inputs, String actor);
23+
}
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
package stirling.software.proprietary.failure;
2+
3+
import lombok.Getter;
4+
5+
/**
6+
* Why an action could not be dispatched. Carries a {@link Reason} rather than an HTTP status, so
7+
* the service stays web-agnostic and the controller owns the mapping.
8+
*/
9+
@Getter
10+
public class FailureActionException extends RuntimeException {
11+
12+
public enum Reason {
13+
/**
14+
* No such event, it belongs to another team, or the caller's team did not resolve. One
15+
* reason for all three, so the response does not vary with which it was. Unrelated to
16+
* {@link FailureKind#UNKNOWN}, which is an unclassified failure rather than a refused
17+
* action.
18+
*/
19+
EVENT_NOT_FOUND,
20+
21+
/** The action id is not in the vocabulary at all, or has no registered handler. */
22+
ACTION_NOT_RECOGNISED,
23+
24+
/**
25+
* The action exists but this kind does not declare it, so an incoherent pairing (releasing
26+
* a document whose destination is what failed) cannot be dispatched even by hand.
27+
*
28+
* <p>Unreachable today: both kinds declare both actions, so no request can trip this guard
29+
* until a kind ships with a restricted action set. Declared now because the guard must
30+
* exist before that kind does, not after.
31+
*/
32+
ACTION_NOT_DECLARED,
33+
34+
/** The event is already closed, so no further transition is possible. */
35+
ALREADY_CLOSED
36+
}
37+
38+
private final Reason reason;
39+
40+
public FailureActionException(Reason reason, String message) {
41+
this(reason, message, null);
42+
}
43+
44+
/** For a refusal that follows from a lower-level failure, so its stack is not dropped. */
45+
public FailureActionException(Reason reason, String message, Throwable cause) {
46+
super(message, cause);
47+
this.reason = reason;
48+
}
49+
}
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
package stirling.software.proprietary.failure;
2+
3+
/**
4+
* The actions a {@link FailureKind} may declare. Both are incident dispositions: they change how
5+
* the event is shown and touch nothing else, which is what makes them valid for every kind
6+
* including {@link FailureKind#UNKNOWN}, and why there is no {@code APPROVE} yet.
7+
*/
8+
public enum FailureActionId {
9+
ACKNOWLEDGE,
10+
DISMISS
11+
}
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
package stirling.software.proprietary.failure;
2+
3+
import java.util.Arrays;
4+
import java.util.EnumMap;
5+
import java.util.List;
6+
import java.util.Map;
7+
import java.util.Optional;
8+
9+
import org.springframework.stereotype.Service;
10+
11+
import jakarta.annotation.PostConstruct;
12+
13+
import lombok.extern.slf4j.Slf4j;
14+
15+
/**
16+
* Resolves a {@link FailureActionId} to the bean that implements it. The startup check is the
17+
* point: because kinds declare action ids as data, one could name an action nobody implements,
18+
* which would otherwise show up as a button that 400s rather than as a failed boot.
19+
*/
20+
@Slf4j
21+
@Service
22+
public class FailureActionRegistry {
23+
24+
private final Map<FailureActionId, FailureAction> byId = new EnumMap<>(FailureActionId.class);
25+
26+
public FailureActionRegistry(List<FailureAction> actions) {
27+
for (FailureAction action : actions) {
28+
FailureAction clash = byId.put(action.id(), action);
29+
if (clash != null) {
30+
throw new IllegalStateException(
31+
"Two handlers registered for action "
32+
+ action.id()
33+
+ ": "
34+
+ clash.getClass().getName()
35+
+ " and "
36+
+ action.getClass().getName());
37+
}
38+
}
39+
}
40+
41+
/**
42+
* Fail fast if any kind declares an action with no handler, naming every gap rather than the
43+
* first, so one boot tells you everything that is missing.
44+
*/
45+
@PostConstruct
46+
void verifyEveryDeclaredActionHasAHandler() {
47+
List<String> gaps =
48+
Arrays.stream(FailureKind.values())
49+
.flatMap(
50+
kind ->
51+
kind.getActions().stream()
52+
.filter(action -> !byId.containsKey(action))
53+
.map(action -> kind.getId() + " -> " + action))
54+
.toList();
55+
if (!gaps.isEmpty()) {
56+
throw new IllegalStateException(
57+
"Failure kinds declare actions with no registered handler: " + gaps);
58+
}
59+
log.debug(
60+
"Failure action registry initialised with {} handler(s) for {} kind(s)",
61+
byId.size(),
62+
FailureKind.values().length);
63+
}
64+
65+
public Optional<FailureAction> find(FailureActionId id) {
66+
return Optional.ofNullable(byId.get(id));
67+
}
68+
}
Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
package stirling.software.proprietary.failure;
2+
3+
import java.util.List;
4+
5+
import org.springframework.stereotype.Service;
6+
import org.springframework.web.client.RestClientResponseException;
7+
8+
import jakarta.annotation.PostConstruct;
9+
10+
import lombok.RequiredArgsConstructor;
11+
import lombok.extern.slf4j.Slf4j;
12+
13+
import stirling.software.common.util.ExceptionUtils;
14+
15+
import tools.jackson.core.JacksonException;
16+
import tools.jackson.databind.JsonNode;
17+
import tools.jackson.databind.ObjectMapper;
18+
19+
/**
20+
* Maps a thrown failure onto a {@link FailureKind}. A tool's 4xx arrives as a {@link
21+
* RestClientResponseException} whose body is the Problem Details document carrying {@code
22+
* errorCode}, so this matches on codes rather than exception messages.
23+
*
24+
* <p>Anything unrecognised becomes {@link FailureKind#UNKNOWN}, so every failed run still gets a
25+
* record.
26+
*/
27+
@Slf4j
28+
@Service
29+
@RequiredArgsConstructor
30+
public class FailureClassifier {
31+
32+
/** Set by {@code GlobalExceptionHandler#createProblemDetailResponse}. */
33+
private static final String ERROR_CODE_PROPERTY = "errorCode";
34+
35+
/**
36+
* Depth bound on the cause chain. The JDK forbids self-causation but not a longer cycle (A
37+
* caused by B caused by A), which an unbounded walk would spin on. Real chains are a handful
38+
* deep.
39+
*/
40+
private static final int MAX_CAUSE_DEPTH = 16;
41+
42+
private final ObjectMapper objectMapper;
43+
44+
/**
45+
* Refuse to start on an ambiguous registry: two kinds claiming one code would make {@link
46+
* #classify} depend on declaration order. Checked here because this is what resolves codes to
47+
* kinds, and at boot so the message names the codes rather than arriving as a class-init error.
48+
*/
49+
@PostConstruct
50+
void verifyNoErrorCodeIsClaimedTwice() {
51+
List<String> duplicates = FailureKind.duplicateErrorCodes();
52+
if (!duplicates.isEmpty()) {
53+
throw new IllegalStateException(
54+
"Error codes claimed by more than one failure kind: " + duplicates);
55+
}
56+
}
57+
58+
/** Never null, never throws. A classifier that can fail would lose the failure it describes. */
59+
public FailureKind classify(Throwable throwable) {
60+
Throwable current = throwable;
61+
for (int depth = 0; current != null && depth < MAX_CAUSE_DEPTH; depth++) {
62+
FailureKind resolved = classifyOne(current);
63+
if (resolved != FailureKind.UNKNOWN) {
64+
return resolved;
65+
}
66+
Throwable cause = current.getCause();
67+
current = cause == current ? null : cause;
68+
}
69+
return FailureKind.UNKNOWN;
70+
}
71+
72+
private FailureKind classifyOne(Throwable throwable) {
73+
// A tool step's 4xx/5xx: the Problem Details body names the error code.
74+
if (throwable instanceof RestClientResponseException responseException) {
75+
String code = errorCodeFromBody(responseException);
76+
if (code != null) {
77+
return FailureKind.byErrorCode(code).orElse(FailureKind.UNKNOWN);
78+
}
79+
}
80+
// Thrown in-process (not over loopback): the exception carries its own code.
81+
if (throwable instanceof ExceptionUtils.ErrorCodeProvider provider) {
82+
return FailureKind.byErrorCode(provider.getErrorCode()).orElse(FailureKind.UNKNOWN);
83+
}
84+
return FailureKind.UNKNOWN;
85+
}
86+
87+
/**
88+
* Pull {@code errorCode} from a Problem Details body, or null when the body is absent, not
89+
* JSON, or has no such property (an entitlement sentinel has {@code error} instead).
90+
*/
91+
private String errorCodeFromBody(RestClientResponseException exception) {
92+
String body = exception.getResponseBodyAsString();
93+
if (body == null || body.isBlank()) {
94+
return null;
95+
}
96+
try {
97+
JsonNode root = objectMapper.readTree(body);
98+
JsonNode code = root.get(ERROR_CODE_PROPERTY);
99+
if (code == null || !code.isTextual()) {
100+
return null;
101+
}
102+
String text = code.asString();
103+
return text.isBlank() ? null : text;
104+
} catch (JacksonException e) {
105+
log.debug("Downstream error body was not JSON; classifying as UNKNOWN");
106+
return null;
107+
}
108+
}
109+
}

0 commit comments

Comments
 (0)