feat: add filesystem payload offloader and retries - #681
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Error
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Thread.currentThread().interrupt(); | ||
| var interrupted = new RetryablePayloadOffloadException( | ||
| String.format("Interrupted while waiting to retry payload %s after attempt %d", action, attempt), | ||
| e); | ||
| interrupted.addSuppressed(failure); | ||
| throw interrupted; |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_5vmint2jktj7b53xftxnhr7dib
[P1] An interrupted backoff is converted to RetryablePayloadOffloadException. DurableExecutor treats this subtype as RETRYING, so cancellation can trigger a fresh Lambda invocation and repeat storage work. The filesystem classifier similarly marks actual I/O interruptions retryable. Restore the interrupt flag but throw a non-retryable PayloadOffloadException, and classify ClosedByInterruptException or an interrupt-signaled InterruptedIOException as permanent interruption.
| var inlinePayload = OffloadedPayload.inline( | ||
| serializedPayload, context.durableExecutionArn(), context.entityId(), payloadDigest) | ||
| .bindProducer(context, payloadDigest); | ||
| if (fitsCheckpoint(inlinePayload)) { |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_6olmknqlyp3zhgezq3cu4y2oso
[P1] OVERFLOW sizing materializes the complete escaped envelope and then another UTF-8 byte array through PayloadCodec.envelopeSizeBytes. For the multi-megabyte payloads this offloader is intended to handle, these extra full-size copies can exhaust Lambda memory before the code chooses filesystem storage. Short-circuit when a bounded UTF-8 count already exceeds the limit, and perform exact envelope encoding only for near-boundary payloads.
| var expectedPrefix = payloadOwnerPrefix(payload.ownerDurableExecutionArn(), payload.ownerEntityId()) | ||
| + "-" | ||
| + payload.payloadDigest() | ||
| + "-"; |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_2cuyghirtpc6zjugxs55p2ygo2
[P2] Loading recomputes the expected filename using the current pathEncoding. An in-flight execution that wrote a URI-form reference will therefore fail replay after configuration changes to HASH, despite the persisted reference, owner, and digest remaining valid. Make encoding a write-only choice by accepting both versioned filename formats during load, or persist the encoding with the reference, and add a cross-configuration replay test.
| import software.amazon.lambda.durable.offload.PayloadStorageMode; | ||
| import software.amazon.lambda.durable.offload.SerDesPayloadKind; | ||
|
|
||
| class FileSystemPayloadOffloaderTest { |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_nlr5gumo4w2iw45unbkquvbkep
[P2] The new public offloader is tested only through direct method calls. That does not exercise DurableConfig, PayloadCodec, executor routing, checkpoint persistence, or replay, and project rules require integration coverage for public API changes. Add a LocalDurableTestRunner test that configures this offloader and verifies checkpoint/suspend/replay loading, including an OVERFLOW payload and retryable storage failure.
Codex AI reviewFound four actionable issues affecting interruption handling, large-payload reliability, replay compatibility, and required integration coverage. Reviewed commit |
578208f to
d3f3e32
Compare
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Failure
| private void waitForRetry(String action, RetryablePayloadOffloadException failure, int attempt, Duration delay) { | ||
| if (delay == null || delay.isNegative()) { | ||
| throw new PayloadOffloadException(String.format( | ||
| "Retry strategy returned an invalid delay for payload %s attempt %d", action, attempt)); | ||
| } | ||
| if (delay.isZero()) { | ||
| return; | ||
| } | ||
| try { | ||
| sleeper.sleep(delay); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| var interrupted = new RetryablePayloadOffloadException( | ||
| String.format("Interrupted while waiting to retry payload %s after attempt %d", action, attempt), | ||
| e); | ||
| interrupted.addSuppressed(failure); | ||
| throw interrupted; | ||
| } | ||
| } |
There was a problem hiding this comment.
Claude AI review · Finding arf_v1_b5hgr2vtoilx2bumojiyfm4bsc
waitForRetry blocks the calling thread with TimeUnit.SECONDS.sleep/TimeUnit.NANOSECONDS.sleep for the entire retry delay returned by the caller-supplied RetryStrategy, with no upper bound and no awareness of the Lambda invocation's remaining time budget. PayloadCodec.runOffloadTask can invoke offloader calls inline on the current thread (its Javadoc says the executor may be null "to execute inline"), so a realistic backoff (seconds to minutes) burns billed compute time and can push the invocation past its timeout — turning a transient, retryable storage failure into a hard failure. This is inconsistent with every other retry path in the SDK: StepOperation.handleStepFailure always converts a retry delay into a checkpointed RETRY + poll-for-READY cycle instead of a blocking sleep, and AGENTS.md explicitly says not to use Thread.sleep()/blocking timers to model waits. Recommend either capping the cumulative retry delay this executor will block for (failing fast past the cap) or prominently documenting on RetryPayloadOffloader/PayloadOffloadRetryExecutor that supplied RetryStrategy delays must be small/bounded because they block the calling thread and count as billed compute time.
| @Test | ||
| void overflowModeKeepsSmallPayloadInlineAndOffloadsLargePayload() { | ||
| var offloader = FileSystemPayloadOffloader.builder(temporaryDirectory) | ||
| .storageMode(PayloadOffloadMode.OVERFLOW) | ||
| .build(); | ||
|
|
||
| var inline = offloader.offload("small", context()); | ||
| var reference = offloader.offload("x".repeat(256 * 1024), context()); | ||
|
|
||
| assertEquals(PayloadStorageMode.INLINE, inline.mode()); | ||
| assertEquals("small", inline.data()); | ||
| assertEquals(PayloadStorageMode.REFERENCE, reference.mode()); | ||
| } |
There was a problem hiding this comment.
Claude AI review · Finding arf_v1_xm7cj2gaddq2zlhqpxsmwmpbvb
This test only checks inline.mode()/inline.data() but never calls offloader.load(inline, context()). FileSystemPayloadOffloader's OVERFLOW branch builds the inline payload via OffloadedPayload.inline(data, arn, entityId, digest), which always sets requiresLoad=true; consequently PayloadCodec.resolve() will call this offloader's load() on every subsequent read of that payload (e.g. on replay), exercising the untested PayloadStorageMode.INLINE branch of load() (integrity-metadata check, owner validation, digest verification). A regression in that branch (e.g. in requireIntegrityMetadata, validateOwner, or verifyDigest for inline data) would go undetected. Add assertEquals("small", offloader.load(inline, context())) to close the gap.
| private java.nio.channels.SeekableByteChannel openPayloadForRead( | ||
| SecureDirectoryStream<Path> directory, Path fileName, PayloadOffloadContext context) throws IOException { | ||
| var attributes = readAttributes(directory, fileName); | ||
| if (attributes.isSymbolicLink() || !attributes.isRegularFile()) { | ||
| throw new PayloadOffloadException( | ||
| "Filesystem payload must be a regular file for entity '" + context.entityId() + "'"); | ||
| } | ||
| return directory.newByteChannel(fileName, Set.of(StandardOpenOption.READ, LinkOption.NOFOLLOW_LINKS)); | ||
| } | ||
|
|
||
| private static java.nio.file.attribute.BasicFileAttributes readAttributes( |
There was a problem hiding this comment.
Claude AI review · Finding arf_v1_lpu6fjilm7ok6xrupvoce2d45m
openPayloadForRead returns java.nio.channels.SeekableByteChannel and readAttributes returns java.nio.file.attribute.BasicFileAttributes, both using fully qualified names inline instead of imports. AGENTS.md's Java style rules explicitly say "ALWAYS use proper imports, NEVER use fully qualified class names in code." Add import java.nio.channels.SeekableByteChannel; and import java.nio.file.attribute.BasicFileAttributes; at the top of the file and use the simple type names in both signatures.
Claude AI reviewReviewed the new filesystem payload offloader ( Overall the filesystem implementation is careful: Three actionable issues remain, ordered by impact:
No correctness bugs were found in the digest/ownership validation, path containment, checkpoint-size accounting (OVERFLOW/ALWAYS modes, envelope-size math), or the preview truncation/budget algorithm. Residual risk: this PR intentionally excludes testing-runner/history integration and cloud/local integration scenarios (deferred to a later PR in the stack), so end-to-end checkpoint/replay coverage of this offloader is not yet exercised anywhere in the repo. Reviewed commit |
1 participant
Stack
Scope
FileSystemPayloadOffloaderin the core SDKCREATE_NEWSecureDirectoryStream, no-follow access, direct-child confinement, ownership binding, and SHA-256 verificationALWAYS/OVERFLOW, URI/hash path encodings, envelope-size limits, and structured/custom previewsRetryPayloadOffloaderfor explicitly retryable storage failuresIntentionally excluded:
Validation
git diff --checkmvn spotless:checkRelated to #463.