Skip to content

fix: unblock Modal image builds and function deployment - #1837

Merged
ColeMurray merged 2 commits into
mainfrom
fix/modal-image-deployment-blockers
Sep 8, 2026
Merged

fix: unblock Modal image builds and function deployment#1837
ColeMurray merged 2 commits into
mainfrom
fix/modal-image-deployment-blockers

Conversation

@ColeMurray

@ColeMurray ColeMurray commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Summary

Port the two Modal deployment fixes validated in production after the shared sandbox-image consolidation (#1816):

  • Restore /tmp to mode 1777 before Debian/Ubuntu APT operations. The staged Modal image can leave it owned by root with mode 0755, preventing APT's unprivileged _apt user from creating temporary files and causing misleading repository-signature errors.
  • Set copy=True when adding sandbox_runtime to the Modal function image. This bakes the source into the image so the subsequent .env(...) build step is valid; Modal rejects build steps after a runtime-only add_local_dir mount.
  • Include the regression guards from production for both image contracts.

No provider selection, deployment configuration, credentials, or unrelated production changes are included.

Validation

  • Full sandbox-images test suite.
  • Full Modal infrastructure test suite.
  • Ruff lint and format checks on changed Python files.
  • Bash syntax check for the Debian installer and git diff --check.

These same runtime changes were previously verified through a successful production Modal image build, fresh-sandbox smoke verification, and function deployment. This public port does not trigger a new manual production deployment.

Summary by CodeRabbit

  • Bug Fixes

    • Runtime files are now copied into built images, ensuring builds use a fresh bundled copy.
    • Debian and Ubuntu sandbox images now restore /tmp with shared, secure permissions before package installation.
  • Tests

    • Added coverage verifying runtime copying and /tmp setup occur in the correct build order.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Terraform Validation Results

Step Status
Format
Init
Validate
Tests

Note: Terraform plan was skipped because secrets are not configured. This is expected for external contributors. See docs/GETTING_STARTED.md for setup instructions.

Pushed by: @ColeMurray, Action: pull_request

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 9c40d970-0bc1-421b-bd73-238f0360b823

📥 Commits

Reviewing files that changed from the base of the PR and between 9dbbb03 and 34d1175.

📒 Files selected for processing (4)
  • packages/modal-infra/src/app.py
  • packages/modal-infra/tests/test_deploy.py
  • packages/sandbox-images/install/os/debian.sh
  • packages/sandbox-images/tests/test_bundle.py

Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

The change copies sandbox_runtime into Modal images and creates a sticky-bit /tmp directory in Debian images before package installation. Tests verify both command placement and ordering.

Changes

Sandbox runtime image copying

Layer / File(s) Summary
Copy sandbox runtime into function images
packages/modal-infra/src/app.py, packages/modal-infra/tests/test_deploy.py
function_image passes copy=True to .add_local_dir. The test verifies this occurs before .env.

Debian temporary directory setup

Layer / File(s) Summary
Restore temporary directory before apt
packages/sandbox-images/install/os/debian.sh, packages/sandbox-images/tests/test_bundle.py
The install script creates /tmp with mode 1777 before apt-get update. The test verifies the ordering.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 34d11

This restores runtime inclusion in Modal function images and sticky-bit /tmp permissions before Debian package installation. The targeted safeguards cover both build contracts, with no remaining merge-blocking risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the two main changes: fixing Modal image builds and unblocking function deployment.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/modal-image-deployment-blockers

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@open-inspect open-inspect Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thermo-nuclear code quality audit complete. No blocking findings.

Both fixes are located at the narrow canonical boundaries: the shared Debian installer restores the system /tmp invariant immediately before APT, and the Modal function image explicitly bakes local runtime source before the subsequent image layer. They add no branching, wrapper, optionality, cross-layer leakage, or file-size regression. The source-level regression guards are focused and proportionate to these declarative build contracts.

Validation performed:

  • uv run --extra dev pytest tests/test_deploy.py -q (13 passed)
  • uv run --extra dev pytest tests/test_bundle.py -q (26 passed)
  • Ruff lint and format checks for all changed Python files
  • bash -n install/os/debian.sh
  • git diff --check
  • Confirmed the installed Modal API requires copy=True before subsequent image build steps
  • All completed GitHub CI checks pass

@ColeMurray
ColeMurray merged commit b1a585b into main Sep 8, 2026
22 checks passed
@ColeMurray
ColeMurray deleted the fix/modal-image-deployment-blockers branch September 8, 2026 01:07

@open-inspect open-inspect Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

PR #1837, fix: unblock Modal image builds and function deployment, by @ColeMurray updates 4 files (+13/-1). The changes correctly restore the shared /tmp permissions required by APT and bake sandbox_runtime into the Modal function image before the subsequent image step. No blocking or non-blocking defects were found.

Critical Issues

None.

Suggestions

None.

Nitpicks

None.

Positive Feedback

  • install -d -m 1777 /tmp is a minimal, idempotent repair that runs before both APT update operations.
  • copy=True matches Modal 1.4.3's image API contract and preserves the intended runtime import path while allowing the following .env(...) operation.
  • Focused regression guards cover both deployment contracts. All 238 Modal infrastructure tests and all 55 sandbox-image tests pass; Ruff lint/format, Bash syntax, and diff whitespace checks also pass.

Questions

None.

Verdict

Approve: Ready to merge.

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