Skip to content

fix(apps-engine/deno): use std mkdtemp for ephemeral files#41411

Closed
debdutdeb wants to merge 2 commits into
developfrom
fix-apps-engine-ephemeral-dir-ownership
Closed

fix(apps-engine/deno): use std mkdtemp for ephemeral files#41411
debdutdeb wants to merge 2 commits into
developfrom
fix-apps-engine-ephemeral-dir-ownership

Conversation

@debdutdeb

@debdutdeb debdutdeb commented Jul 16, 2026

Copy link
Copy Markdown
Member

Proposed changes (including videos or screenshots)

Issue(s)

CORE-2428

Steps to test or reproduce

Further comments

this will respect posix TMPDIR environment variable. app shouldn't write to /app/bundle at runtime. it breaks change in runtime uid changes. separating where data is written to, we can bind an emptyDir at TMPDIR and call it a day

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Improved Deno runtime configuration generation to write the ephemeral runtime config into an isolated temporary directory rather than alongside the static config.
    • Reduces the likelihood of filename/location collisions by deriving the runtime config path from the original config’s basename, improving reliability for concurrent runs.

@debdutdeb
debdutdeb requested a review from a team as a code owner July 16, 2026 06:56
@dionisio-bot

dionisio-bot Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot

changeset-bot Bot commented Jul 16, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 6ef47e0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The Deno runtime now constructs its ephemeral configuration path inside tempFilePath, using the static configuration filename with a .runtime.jsonc suffix.

Changes

Deno runtime configuration

Layer / File(s) Summary
Construct temporary config path
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
Updates ephemeral configuration path construction to join tempFilePath with the renamed static configuration basename.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

Suggested labels: type: bug

Suggested reviewers: d-gubert

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the change: ephemeral Deno files are moved under the temp path to respect TMPDIR.

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.

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts`:
- Around line 20-22: Update getDenoEphemeralConfigPath to create the temporary
directory beneath the system temporary directory by joining tmpdir() with the
deno-runtime- prefix before calling fs.mkdtempSync, rather than using a relative
prefix.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 10960043-58f3-44f4-83f1-4d374446fc93

📥 Commits

Reviewing files that changed from the base of the PR and between 8bbd1c6 and 987a1b0.

📒 Files selected for processing (1)
  • packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
  • GitHub Check: 🔎 Code Check / Code Lint
  • GitHub Check: 🔎 Code Check / TypeScript
  • GitHub Check: 🔨 Test Storybook / Test Storybook
  • GitHub Check: 🔨 Test Unit / Unit Tests
  • GitHub Check: 📦 Meteor Build (coverage)
  • GitHub Check: cubic · AI code reviewer
  • GitHub Check: Hacktron Security Check
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation

Files:

  • packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
🧠 Learnings (4)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.

Applied to files:

  • packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.

Applied to files:

  • packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.

Applied to files:

  • packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📚 Learning: 2026-05-11T21:46:23.471Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40463
File: packages/apps/src/lib/SecureFields.ts:17-19
Timestamp: 2026-05-11T21:46:23.471Z
Learning: In Rocket.Chat’s `packages/apps/tsconfig.json`, TypeScript `"strict"` is set to `false`, which disables strict type-checking (including `noImplicitAny`) for `packages/apps`. When reviewing, do not flag TS7053 (and similar strict-mode indexing/type errors) in files under `packages/apps/src/` that are a consequence of this relaxed strictness—e.g., patterns like indexing an `unknown`/`object` via optional chaining such as `object?.[kSecureFields]`.

Applied to files:

  • packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts

Comment thread packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts Outdated
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.45%. Comparing base (8bbd1c6) to head (987a1b0).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop   #41411   +/-   ##
========================================
  Coverage    68.45%   68.45%           
========================================
  Files         4092     4092           
  Lines       158212   158212           
  Branches     28611    28642   +31     
========================================
  Hits        108298   108298           
+ Misses       44874    44867    -7     
- Partials      5040     5047    +7     
Flag Coverage Δ
unit 70.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@debdutdeb
debdutdeb marked this pull request as draft July 16, 2026 07:06
@debdutdeb

Copy link
Copy Markdown
Member Author

in favor of #41338

@debdutdeb debdutdeb closed this Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant