Skip to content

fix(sof-export): re-sign S3 download URLs on each manifest poll (#145)#181

Open
mauripunzueta wants to merge 2 commits into
fix/144-sof-export-cleanup-cancelledfrom
fix/145-resign-s3-download-urls
Open

fix(sof-export): re-sign S3 download URLs on each manifest poll (#145)#181
mauripunzueta wants to merge 2 commits into
fix/144-sof-export-cleanup-cancelledfrom
fix/145-resign-s3-download-urls

Conversation

@mauripunzueta

Copy link
Copy Markdown
Contributor

Summary

Closes #145.

⚠️ Stacked on #180. Base branch is fix/144-sof-export-cleanup-cancelled, so this PR's diff shows only the #145 changes. GitHub will auto-retarget the base to main once #180 merges. Review/merge #180 first.

For S3-backed SoF exports ($viewdefinition-export / $sqlquery-export), pre-signed GET URLs were generated once at shard-write time and baked into CompletedFile.url. Every later status poll re-served those same URLs, so the presign TTL counted down from write time, not completion — a client re-polling the manifest hours in could receive URLs about to expire (or already expired, under a shorter configured TTL). The spec requires output.location URLs to be valid for ≥24h after completion.

What changed

  • CompletedFile stores the shard's filename, not a baked URL.
  • ExportSink::write_shard returns the filename; new ExportSink::download_url(job_id, filename) resolves it to a public URL:
    • FilesystemSink / InMemorySink → stable {base}/export/{job_id}/{filename} route (unchanged behavior).
    • S3Sink → a freshly pre-signed GET URL; the presign moves out of the write path into download_url. Presigning is a local signature computation (no network round trip), so re-signing per poll is cheap.
  • ExportJobController::download_url (tenant-gated, mirrors read_shard). The result handler re-resolves every shard's URL on each poll, so each handout carries a full TTL window. A resolution failure surfaces as 500 rather than a manifest with an empty location.
  • build_completion_manifest now takes pre-resolved (view_name, location) pairs.

Notes / deliberate choices

  • Expires header stays tied to the result-retention window (24h, now aligned with the output-retention reaper from fix(sof-export): clean up partial results on cancel, plus a TTL reaper (#144) #180), not to a single pre-signature's lifetime — re-polling always yields a fresh URL. Documented inline.
  • STS / temporary credentials can cap a pre-signed URL's effective lifetime below the requested TTL; noted on S3Sink::download_url for operators.

Testing

  • New unit tests: in_memory_sink_writes_filename_and_resolves_url, controller_download_url_is_fresh_and_tenant_gated (proves per-poll re-resolution returns distinct URLs + tenant gating).
  • helios-rest lib suite (283) and sof_export integration suite (44) pass; FS-sink manifest URLs unchanged. cargo fmt clean; cargo clippy --features s3 --all-targets -D warnings clean.

🤖 Generated with Claude Code

For S3-backed SoF exports, pre-signed GET URLs were generated once at
shard-write time and baked into CompletedFile, so the TTL clock started at
write rather than completion — a client re-polling the manifest hours later
received URLs already counting down (or expired, under a shorter configured
TTL). Closes #145.

- CompletedFile stores the shard's filename, not a baked URL.
- ExportSink::write_shard now returns that filename; new ExportSink::download_url
  resolves a filename to a public URL — a stable server route for the
  filesystem/in-memory sinks, a freshly pre-signed GET URL for S3 (the presign
  moves out of the write path).
- New ExportJobController::download_url (tenant-gated, mirrors read_shard); the
  result handler re-resolves every shard's URL on each poll, so each handout
  carries a full TTL window. A resolution failure yields 500 rather than a
  manifest with an empty location.
- Expires stays tied to the result-retention window (now aligned with the
  output-retention reaper); re-polling always yields a fresh URL. STS/temporary
  credential lifetime caveat documented on S3Sink::download_url.

Tests: in_memory_sink_writes_filename_and_resolves_url,
controller_download_url_is_fresh_and_tenant_gated (proves per-poll re-resolution
+ tenant gating). helios-rest lib (283) + sof_export integration (44) pass;
clippy clean with --features s3.
@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

…2026-0185

Two ambient CI breakages on origin/main, unrelated to this PR's changes:

- clippy 1.91 newly flags collapsible_else_if in sof/emit.rs; collapse the
  nested else { if .. } into else if.
- cargo audit fails on RUSTSEC-2026-0185 (quinn-proto remote memory
  exhaustion), a transitive reqwest QUIC dep. We never accept inbound QUIC,
  so the reassembly path is unreachable; ignore it with justification.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant