fix(0.6.0): establish safe persistence failure boundaries - #225
fix(0.6.0): establish safe persistence failure boundaries#225GionaGranchelli wants to merge 3 commits into
Conversation
Persistence stores now expose fixed cause-free failure text; raw paths, SQL text, persisted payloads, and arbitrary exception messages flow only to an explicitly configured PersistenceFailureDiagnosticObserver. - Add PersistenceFailureCode / PersistenceResourceKind / PersistenceOperation, PersistenceFailureDiagnosticObserver + event, and WorkflowPersistenceFailureException. - Add persistenceBoundary() with cancellation-first execution, fail-open observer delivery, trusted safe-factory pass-through, and re-sanitization of untrusted caller-constructed domain exceptions. - Wrap checkpoint (file/markdown/JDBC/in-memory), lease (file/JDBC/in-memory), step-attempt (file/JDBC/in-memory) and worker-registry operations; corrupt payload/path/field values ride only on internal Corrupt*Exception carriers to the observer. - Preserve exception and store JVM descriptors: class-body failureCode and safeFactoryTrusted props with internal setters, additive observer constructor overloads, existing primary constructors unchanged (api dump + v0.5.0 binary fixture extended to 17 markers). - Worker observers receive safe terminal failures; LoggingTramaiWorkerObserver renders exception class names only. - Add PersistenceSafeFailureBoundaryTest (14 adversarial leak scenarios) and update contract tests to the fixed-text public contract without weakening cancellation, CAS, durability, or recovery assertions. - Mark Epic 1.2 Safe Error Boundaries complete in roadmap, characterization matrix, safe-error-boundaries concept, orchestration-persistence guide, and changelog.
There was a problem hiding this comment.
Pull request overview
Establishes a safe persistence failure boundary across all built-in persistence implementations in tramai-orchestration, ensuring filesystem paths, SQL text, persisted payloads, and arbitrary exception messages do not cross into caller-visible exceptions, ordinary worker observers, or default logs. The original failure is retained only via an explicitly configured PersistenceFailureDiagnosticObserver, aligning persistence with the previously-merged safe-boundary slices for tools, providers, workflow steps, and structured output.
Changes:
- Adds a typed persistence failure model (
PersistenceFailureCode,PersistenceResourceKind,PersistenceOperation) plus a single cancellation-firstpersistenceBoundary(...)implementation and safe fixed-message factories. - Wires the boundary into all built-in checkpoint / lease / step-attempt / worker-registry persistence families, and ensures worker observer/logging paths receive only safe failures.
- Extends binary-compat fixture coverage and adds adversarial leak tests + doc/ADR-style updates describing the persistence boundary contract.
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/PersistenceFailures.kt | New central persistence safe-boundary implementation, typed codes, diagnostic observer, and fixed-message safe factories. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/WorkflowPersistence.kt | Updates checkpoint APIs/exceptions and in-memory store to use the new persistence boundary + safe failures. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/WorkflowLease.kt | Updates lease exceptions and in-memory lease/worker-registry operations to route through the persistence boundary. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/FileWorkflowCheckpointStore.kt | Wraps file-backed checkpoint operations in the safe boundary; corruption routes via internal carriers. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/MarkdownWorkflowCheckpointStore.kt | Wraps markdown checkpoint operations in the safe boundary; fail-closed corruption handling via internal carrier. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/JdbcWorkflowCheckpointStore.kt | Wraps JDBC checkpoint operations in the safe boundary; fail-closed corruption handling during row decoding. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/FileWorkflowLeaseStore.kt | Wraps file-backed lease operations in the safe boundary; fixed-text fencing/stale lease failures. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/JdbcWorkflowLeaseStore.kt | Wraps JDBC lease operations in the safe boundary; replaces detailed conflicts with safe fixed-text failures. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/FileStepAttemptRecordStore.kt | Wraps file-backed step-attempt operations in the safe boundary; corruption uses internal carrier exceptions. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/JdbcStepAttemptRecordStore.kt | Wraps JDBC step-attempt operations in the safe boundary; corruption uses internal carrier exceptions. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/StepAttemptRecord.kt | Extends step-attempt corruption exception with safe-factory trust metadata. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/StepAttemptRecordCodec.kt | Converts codec failures to internal corruption carriers to avoid leaking raw values in public exceptions. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/TramaiWorker.kt | Sanitizes persistence failures before sending them to ordinary worker observers. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/LoggingTramaiWorkerObserver.kt | Stops logging Throwable.message; logs exception class names only. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/PersistenceSafeFailureBoundaryTest.kt | New adversarial tests proving secrets don’t leak across persistence boundaries and observer behavior is fail-open + cancellation-correct. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/WorkflowTest.kt | Updates assertions to fixed persistence conflict messages + cause-free failures. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/WorkflowRecoveryContractTest.kt | Updates internal codec expectations to use corruption carrier exceptions (public fixed text proven at store boundary). |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/WorkflowLeaseStoreTest.kt | Updates lease conflict assertions to fixed messages + cause-free failures. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/WorkflowCheckpointStoreTest.kt | Updates checkpoint conflict assertions to fixed messages + cause-free failures. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/StepAttemptRecordStoreContractTest.kt | Updates step-attempt persistence failure expectations to safe fixed-text failures and internal corruption carrier usage. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/JdbcWorkflowPersistenceCancellationContractTest.kt | Updates lease-conflict assertion to fixed message + cause-free failure. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/BinaryCompatibilityFixtureTest.kt | Extends binary fixture marker coverage to validate additional ABI-preservation cases. |
| tramai-orchestration/src/test/resources/binary-compat/BinaryCompatFixture.kt | Extends fixture code to exercise additional persistence exception/store constructors. |
| tramai-orchestration/src/test/resources/binary-compat/README.md | Documents updated fixture scope and exercised constructors. |
| tramai-orchestration/api/tramai-orchestration.api | API dump updates for newly-added persistence failure types/observers and additive constructors/getters. |
| docs/concepts/safe-error-boundaries.md | Documents persistence safe-failure boundary as implemented and completes Epic 1.2 scope. |
| docs/guides/orchestration-persistence.md | Adds usage guidance for PersistenceFailureDiagnosticObserver and fixed public failure behavior. |
| docs/ROADMAP-0.6.0.md | Marks Epic 1.2 as complete and records persistence boundary completion. |
| docs/releases/0.6.0-characterization-matrix.md | Updates characterization entries to include persistence safe-failure boundary coverage. |
| CHANGELOG.md | Adds release note entry describing safe persistence failure boundaries and Epic 1.2 completion. |
Suppressed comments (1)
tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/WorkflowPersistence.kt:145
clearRecoverywraps the initialloadin apersistenceBoundarylabeled asPersistenceOperation.SAVE. If a custom store’sloadthrows a raw exception, it will be classified/emitted as a write failure instead of a read failure. Consider splitting into a LOAD boundary (for the read) followed by a SAVE boundary (for the write).
return persistenceBoundary(
PersistenceResourceKind.CHECKPOINT,
PersistenceOperation.SAVE,
checkpointDiagnosticObserver(this),
) {
val current = load(workflowName, workflowId)
?: throw safePersistenceFailure(
PersistenceResourceKind.CHECKPOINT,
PersistenceOperation.SAVE,
PersistenceFailureCode.CONFLICT,
)
save(
checkpoint = current.copy(recoveryState = WorkflowRecoveryState.Normal),
expectedRevision = expectedRevision,
)
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return persistenceBoundary( | ||
| PersistenceResourceKind.CHECKPOINT, | ||
| PersistenceOperation.SAVE, | ||
| checkpointDiagnosticObserver(this), | ||
| ) { | ||
| val current = load(workflowName, workflowId) | ||
| ?: throw safePersistenceFailure( | ||
| PersistenceResourceKind.CHECKPOINT, | ||
| PersistenceOperation.SAVE, | ||
| PersistenceFailureCode.CONFLICT, | ||
| ) | ||
| save( | ||
| checkpoint = current.copy(recoveryState = WorkflowRecoveryState.Required(record)), | ||
| expectedRevision = expectedRevision, | ||
| ) | ||
| return save( | ||
| checkpoint = current.copy( | ||
| recoveryState = WorkflowRecoveryState.Required(record), | ||
| ), | ||
| expectedRevision = expectedRevision, | ||
| ) | ||
| } |
…ize attempt summaries - persistenceBoundary now ends with currentCoroutineContext().ensureActive() so a parent cancellation arriving while the block completes normally still wins over a normal return (the #223/#224 post-callback rule applied to the boundary itself). Fixes the CI-flaky genuine-parent-cancellation boundary test: release.complete(Unit) could race the parent cancel, making the block return normally and the test observe null instead of CancellationException. - ExecutionTracker.failAttempt sanitizes persistence-family failures for both the ordinary observer and the persisted outputSummary, while user step-execution errors keep their real message. Adds Throwable.isPersistenceFamilyFailure() and a boundary test locking the split (secret stripped from persistence summaries, user messages preserved).
…d cleanup leaks Round-2 review fixes (safe persistence boundaries): - persistenceBoundary no longer trusts throwable identity: trusted failures are reconstructed as fresh cause-free/suppressed-free fixed-text instances of the same semantic class (JDBC cleanup mutates the primary after construction via addSuppressed). Contaminated trusted throwables are delivered to the observer before reconstruction; clean trusted failures emit no event. - Cancellation with raw JDBC children (Statement.cancel, rollback, auto-commit restore, close) is sanitized: raw graph goes to the observer, the caller-visible CancellationException is a fresh fixed-text instance with only a SanitizedCleanupDiagnosticException marker. Genuine framework cancellation passes through unchanged and emits no diagnostic. Fixed CE message text. - FileWorkflowLeaseStore.release/deleteCheckpointIfLeaseOwner resolve the path inside persistenceBoundary so a throwing WorkflowCheckpointPathStrategy cannot escape the boundary. - JdbcWorkflowLeaseStore fenced save/delete route checkpoint DML failures to the checkpoint store's observer with resourceKind=CHECKPOINT via a CheckpointDmlFailure marker; lease-row/fence failures stay on the lease channel (exactly one event per failing phase). - requireRecovery/clearRecovery split into LOAD- and SAVE-phase boundaries so a load failure is READ_FAILED, not mislabeled WRITE_FAILED. - defaultPersistenceFailureCode drops the class-name string heuristic; CLAIM/RENEW -> WRITE_FAILED, RELEASE -> DELETE_FAILED. - safeWorkerObservableFailure reconstructs trusted failures instead of passing the throwable identity to observers. - PersistenceSafeFailureBoundaryTest extended to 25 adversarial scenarios: cancellation suppressed-graph sanitization, trusted contamination, throwing path strategy, phase-aware classification, checkpoint observer routing, and cancellation precedence over trusted failures.
fix(0.6.0): establish safe persistence failure boundaries
Summary
Final slice of Epic 1.2 (Safe Error Boundaries). Persistence internals — filesystem paths, SQL text, JDBC driver messages, persisted payloads, metadata, and arbitrary exception messages — no longer cross into caller-visible exceptions, ordinary worker observers, or logs. The original failure remains available only through an explicitly configured
PersistenceFailureDiagnosticObserver.This follows the exact pattern established by PRs #219 (tools), #222 (providers), #223 (workflow steps), and #224 (structured output). It is a safe-boundary PR, not a persistence redesign: concurrency, CAS, optimistic locking, recovery, durability, and cancellation semantics are unchanged.
What
Failure model (new
PersistenceFailures.kt)PersistenceFailureCode(READ_FAILED,WRITE_FAILED,DELETE_FAILED,LIST_FAILED,CONFLICT,CORRUPTED_DATA) — backend-agnostic, noJDBC_*/FILE_*variants.PersistenceResourceKind(CHECKPOINT,LEASE,STEP_ATTEMPT,WORKER_REGISTRY) andPersistenceOperation(LOAD,SAVE,DELETE,LIST,CLAIM,RENEW,RELEASE,COMPARE_AND_SET).PersistenceFailureDiagnosticObserver+PersistenceFailureDiagnosticEvent+NoOpPersistenceFailureDiagnosticObserver.WorkflowPersistenceFailureException(general-purpose safe persistence failure).internal fun persistenceBoundary(...): single authoritative implementation of cancellation-first execution (rethrowIfCancellation()first in the broad catch), classification, fail-open observer delivery (synthetic observerCancellationExceptionswallowed while active; genuine parent cancellation wins; post-observerensureActive()), trusted safe-factory pass-through, and re-sanitization of untrusted caller-constructed domain exceptions.internal fun safePersistenceFailure(...): fixed enum-derived public text only; takes no caller text (the fix(0.6.0): establish safe structured-output failure boundaries #224 rule — a public factory accepting caller text would let extension code mint "trusted" exceptions).Coverage — all built-in persistence families
FileWorkflowCheckpointStore,MarkdownWorkflowCheckpointStore,JdbcWorkflowCheckpointStore,InMemoryWorkflowCheckpointStore, catalog,decodeRecoveryState/decodeCheckpoint/decodeMetadata/validateExpectedRevision/validateDeleteExpectedRevision.FileWorkflowLeaseStore,JdbcWorkflowLeaseStore,InMemoryWorkflowLeaseStore, fence operations.FileStepAttemptRecordStore,JdbcStepAttemptRecordStore,StepAttemptRecordCodec, in-memory methods.CorruptCheckpointException/CorruptStepAttemptExceptioncarriers to the observer — never in public messages.Workflow checkpoint conflict,Workflow lease conflict,Persisted workflow checkpoint is invalid,Persisted step-attempt record is invalid,Workflow persistence read/write/delete/list failed.StaleWorkflowLeaseExceptionkeeps its semantic class (the worker depends on it) with fixed textWorkflow lease is no longer active.Worker ordinary observers
onPollFailed,onLeaseRenewalFailed,onLeaseReleaseFailed,onStepAttemptFailedreceive safe terminal persistence failures viasafeWorkerObservableFailure(...).LoggingTramaiWorkerObserverrenders exception class names only — nevererror.message.ABI preservation
WorkflowResumeException(String),WorkflowCheckpointConflictException(String),WorkflowCheckpointCorruptionException(String),WorkflowLeaseConflictException(String),StepAttemptRecordCorruptionException(String, Throwable?)incl. the default-arg marker synthetic).persistenceFailureDiagnosticObserveris a class-body property withinternal set; observer-taking overloads are new additive constructors.WorkflowCheckpointCorruptionException/StepAttemptRecordCorruptionException/step-attempt stores post-date v0.5.0 and are proven by api-dump descriptor comparison).Tests
PersistenceSafeFailureBoundaryTest— 14 adversarial leak scenarios: raw JDBC secret SQL, file secret path, corrupt checkpoint secret payload, corrupt step-attempt secret value, untrusted caller-constructed exception re-sanitization, trusted pass-through, fail-open observer, synthetic observer cancellation, genuine parent cancellation (terminalCancellationException, zero diagnostics), conflict semantic class, unchanged CAS/revision, workeronPollFailedsafe-only, logging observer class-name-only, fixed-message stability. Every secret assertion scans message + cause + all suppressed throwables + observer events.StepAttemptRecordStoreContractTest,WorkflowCheckpointStoreTest,WorkflowLeaseStoreTest,WorkflowRecoveryContractTest,TramaiWorkerTest, and both persistence cancellation contract tests pass unchanged in semantic content.Files
31 files, 1716 insertions / 474 deletions (production + tests + api dump + fixture + docs).
PersistenceFailures.kt(new),FileWorkflowCheckpointStore.kt,MarkdownWorkflowCheckpointStore.kt,JdbcWorkflowCheckpointStore.kt,FileWorkflowLeaseStore.kt,JdbcWorkflowLeaseStore.kt,FileStepAttemptRecordStore.kt,JdbcStepAttemptRecordStore.kt,StepAttemptRecord.kt,StepAttemptRecordCodec.kt,WorkflowLease.kt,WorkflowPersistence.kt,TramaiWorker.kt,LoggingTramaiWorkerObserver.kt.PersistenceSafeFailureBoundaryTest.kt(new, 14 tests),BinaryCompatibilityFixtureTest.kt,BinaryCompatFixture.kt,fixture-v0.5.0.jar,README.md,StepAttemptRecordStoreContractTest.kt,WorkflowCheckpointStoreTest.kt,WorkflowLeaseStoreTest.kt,WorkflowRecoveryContractTest.kt,WorkflowTest.kt,JdbcWorkflowPersistenceCancellationContractTest.kt.tramai-orchestration.api(+98, additive).safe-error-boundaries.md,ROADMAP-0.6.0.md(Epic 1.2 ✅ Complete),0.6.0-characterization-matrix.md,orchestration-persistence.md,CHANGELOG.md.Verification
./gradlew test --rerun-tasks— BUILD SUCCESSFUL (all modules, 212 tasks; orchestration 391 tests incl. 14 new boundary tests + 23-test contract suite)../gradlew apiCheck— PASSED../gradlew verifyCancellationSafety -PtramaiCancellationBaseSha=<merge-base>— PASSED (282 current / 275 base, no new critical/high findings)../gradlew verifyPr -PchangeClass=public-api— PASSED (maintainability baseline verified; 4 pre-existing warnings: API hash change expected for a public-api PR, and two pre-existingFILE_GROWTH_EXCEEDED/NEW_TOP_FIVE_HOTSPOTfindings onTramaiWorker.kt/Tramai.ktthat exist on master before this PR).unzip -l fixture-v0.5.0.jar— all entries underdev/tramai/orchestration/.Non-claims
WorkflowCheckpointCorruptionExceptionandStepAttemptRecordCorruptionExceptionpost-date v0.5.0, so their ABI preservation is proven by api-dump descriptor comparison (marker synthetics present in base and current), not by the fixture — documented in the fixture README.Epic 1.2
Safe Error Boundaries is now ✅ Complete after this PR. Next roadmap work moves to Epic 1.3 Runtime Lifecycle Ownership.
This PR needs review before merge.