Skip to content

fix(runtimed): ephemeral notebook review cleanups#1671

Merged
rgbkrk merged 2 commits intomainfrom
fix/ephemeral-review-cleanups
Apr 9, 2026
Merged

fix(runtimed): ephemeral notebook review cleanups#1671
rgbkrk merged 2 commits intomainfrom
fix/ephemeral-review-cleanups

Conversation

@quillaid
Copy link
Copy Markdown
Collaborator

@quillaid quillaid commented Apr 9, 2026

Summary

Follow-up from #1669 code review — three small cleanups:

  1. Cap redirect map at 1000 entries — prevents unbounded growth under extreme agent churn (thousands of save-and-disconnect cycles within an hour). Evicts oldest entries when the cap is hit.

  2. Add delete_metadata() to NotebookDoc — replaces set_metadata("ephemeral", "") with a clean key deletion. No more empty string keys left in the CRDT.

  3. Document promoted room persistence — clarifies that persist_tx remains None after ephemeral→persistent promotion. The .ipynb autosave (via file watcher) is the source of truth for file-backed notebooks, not .automerge persistence.

Test plan

  • 224 lib tests pass (cargo test -p runtimed --lib)
  • Tokio mutex lint passes
  • Ephemeral room tests pass
  • Lint clean (cargo xtask lint)

Three follow-ups from the ephemeral notebooks PR (#1669) code review:

1. Cap redirect map at 1000 entries to bound memory under extreme agent
   churn (previously only TTL pruning was implemented)

2. Add delete_metadata() to NotebookDoc and use it to cleanly remove the
   ephemeral flag on save instead of setting it to an empty string

3. Document that promoted rooms intentionally lack .automerge persistence
   since .ipynb autosave is the source of truth for file-backed notebooks
@github-actions github-actions bot added sync Automerge CRDT sync protocol daemon runtimed daemon, kernel management, sync server labels Apr 9, 2026
The autosave debouncer is spawned above the ephemeral flag clearing,
not below. Self-review catch.
@rgbkrk rgbkrk marked this pull request as ready for review April 9, 2026 23:19
Ok(true)
}
None => Ok(false),
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have to wonder if there's a pattern to "delete in" with automerge

@rgbkrk rgbkrk merged commit 3359216 into main Apr 9, 2026
27 checks passed
@rgbkrk rgbkrk deleted the fix/ephemeral-review-cleanups branch April 9, 2026 23:24
@quillaid
Copy link
Copy Markdown
Collaborator Author

quillaid commented Apr 9, 2026

Re: the delete_metadata pattern — Automerge doc.delete(obj, key) errors if the key does not exist (no "delete if present" variant). The get-then-delete is necessary to return Ok(false) instead of propagating an error for missing keys. The error variant for "key not found" is not well-documented in automerge 0.7, so I went with the explicit check rather than matching on error types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

daemon runtimed daemon, kernel management, sync server sync Automerge CRDT sync protocol

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants