Fault-tolerant step: `retry(Class)` traverses exception causes, `skip(Class)` does not

[kzander91]

Bug description
skip(Class) and retry(Class) behave inconsistently in that skip(SkippableException.class) does not cause throw new RuntimeException(new SkippableException()) to be skipped, but retry(SkippableException.class) does inspect the cause and causes the same expression to be retried.

The expectation would be that exception matching in both RetryPolicy and SkipPolicy behave consistently (ideally aligned with RetryPolicy, in that causes are traversed).

The underlying reason for that is the switch to the new retry support from Framework, which always traverses causes (as it happens, a feature that I have requested myself 🙃): spring-projects/spring-framework#35583

Environment
Spring Batch 6.0.0

Steps to reproduce

1. Configure fault-tolerant step that skips and retries the same exception type.
2. Throw another exception with a skippable exception as the cause.

Expected behavior
The exception is both retried and then skipped (after retry exhaustion).

Minimal Complete Reproducible example
Reproducer: demo14.zip
Run with ./mvnw test

The project has a step like this:

@Bean
Step step() {
    return new StepBuilder("step", jobRepository)
            .chunk(5)
            .transactionManager(transactionManager)
            .faultTolerant()
            .retry(SkippableException.class)
            .retryLimit(1)
            .skip(SkippableException.class)
            .skipLimit(1)
            .reader(new ListItemReader<>(List.of("item")))
            .writer(_ -> {
                throw new RuntimeException(new SkippableException());
            })
            .build();
}

static class SkippableException extends RuntimeException {

}

A test then launches and expects successful completion and a skip count of 1.

---

With Spring Batch 5, I was using a LimitCheckingItemSkipPolicy with a BinaryExceptionClassifier (from spring-retry) that was configured to traverse causes. However this is now deprecated and no equivalent replacement exists (apart from fully reimplementing my own SkipPolicy.

---

While debugging this, I found another, likely related issue:

spring-batch/spring-batch-core/src/main/java/org/springframework/batch/core/step/skip/LimitCheckingExceptionHierarchySkipPolicy.java

Lines 50 to 54 in fa73e01

	@Override
	public boolean shouldSkip(Throwable t, long skipCount) throws SkipLimitExceededException {
	if (skipCount < 0) {
	return !isSkippable(t);
	}

Note how the logic is inverted in the case of skipCount < 0 (which is the case when the SkipPolicy is queried directly after a retryable exception happend). In that case, non-skippable exceptions are classified as skippable, due to !isSkippable(t).

[kzander91]

After more debugging, I'm getting more confused, perhaps the logic here is inverted as well?

spring-batch/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/ChunkOrientedStep.java

Lines 688 to 702 in fa73e01

	if (this.faultTolerant && exception instanceof RetryException retryException
	&& !this.skipPolicy.shouldSkip(retryException.getCause(), -1)) {
	logger.info("Retry exhausted while attempting to write items, scanning the chunk", retryException);
	ChunkScanEvent chunkScanEvent = new ChunkScanEvent(contribution.getStepExecution().getStepName(),
	contribution.getStepExecution().getId());
	chunkScanEvent.begin();
	scan(chunk, contribution);
	chunkScanEvent.skipCount = contribution.getSkipCount();
	chunkScanEvent.commit();
	logger.info("Chunk scan completed");
	}
	else {
	logger.error("Retry exhausted after last attempt in recovery path, but exception is not skippable");
	throw exception;
	}

Why is this.skipPolicy.shouldSkip() negated? This error, which is logged when the SkipPolicy does indicate a skip, also indicates that the inverse was intended:

spring-batch/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/ChunkOrientedStep.java

Line 700 in fa73e01

logger.error("Retry exhausted after last attempt in recovery path, but exception is not skippable");