feat: add --session-ttl flag to marimo edit#7863
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
39ac055 to
ae05eed
Compare
dmadisetti
left a comment
There was a problem hiding this comment.
Nice clean change! Just one comment on the test
| _check_contents(p, b'"mode": "home"', contents) | ||
| finally: | ||
| p.kill() | ||
| p.wait(timeout=5) |
There was a problem hiding this comment.
Hmm, this is super long, how long do we wait in other cases?
There was a problem hiding this comment.
Thanks! I also questioned this initially, but went with the same value used elsewhere for consistency. The timeout=5 is just a max wait/upper bound that should rarely get hit after p.kill(), the process should exit instantly, so wait() returns almost immediately. I guess the use of timeout is to prevent hanging forever if something goes wrong.
See other tests using the same pattern:
Line 935 in eb98a06
Line 1410 in eb98a06
|
looks like a single test is failign: |
|
Should be passing now, since RUN mode can only take in integer values for session-ttl, I updated the test to reflect the default value of 120. |
## 📝 Summary Fix marimo-team#7834 ## 🔍 Description of Changes This PR adds the `--session-ttl` flag to `marimo edit` (extending the functionality from the run mode), enabling automatic cleanup of inactive sessions on multi-user edit servers. When set, sessions are automatically closed after the specified number of seconds following a websocket disconnect. Usage: ```bash marimo edit --session-ttl 300 # Close sessions 5 minutes after disconnect ``` ## 📋 Checklist - [x] I have read the [contributor guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md). - [ ] For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on [Discord](https://marimo.io/discord?ref=pr), or the community [discussions](https://github.com/marimo-team/marimo/discussions) (Please provide a link if applicable). - [x] Tests have been added for the changes made. - [x] Documentation has been updated where applicable, including docstrings for API changes. - [x] Pull request title is a good summary of the changes - it will be used in the [release notes](https://github.com/marimo-team/marimo/releases).
## 📝 Summary Fix #7834 ## 🔍 Description of Changes This PR adds the `--session-ttl` flag to `marimo edit` (extending the functionality from the run mode), enabling automatic cleanup of inactive sessions on multi-user edit servers. When set, sessions are automatically closed after the specified number of seconds following a websocket disconnect. Usage: ```bash marimo edit --session-ttl 300 # Close sessions 5 minutes after disconnect ``` ## 📋 Checklist - [x] I have read the [contributor guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md). - [ ] For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on [Discord](https://marimo.io/discord?ref=pr), or the community [discussions](https://github.com/marimo-team/marimo/discussions) (Please provide a link if applicable). - [x] Tests have been added for the changes made. - [x] Documentation has been updated where applicable, including docstrings for API changes. - [x] Pull request title is a good summary of the changes - it will be used in the [release notes](https://github.com/marimo-team/marimo/releases).
Session cleanup for ASGI apps was broken after #7863 refactored session TTL handling. The `_on_disconnect` handler checked `manager.ttl_seconds` which is None by default for `create_asgi_app()`, causing sessions to never be closed on WebSocket disconnect. This also forces a non-deafult for `create_asgi_app`'s session_ttl parameter, for another layer of safety.
📝 Summary
Fix #7834
🔍 Description of Changes
This PR adds the
--session-ttlflag tomarimo edit(extending the functionality from the run mode), enabling automatic cleanup of inactive sessions on multi-user edit servers.When set, sessions are automatically closed after the specified number of seconds
following a websocket disconnect.
Usage:
marimo edit --session-ttl 300 # Close sessions 5 minutes after disconnect📋 Checklist