Repository navigation
Write small files in place in one APPLY round trip - #89
Conversation
Since yas-run#54 a command wait that runs out before any command it could report on has started answers TIMEOUT (NOT_FOUND stays for an exited terminal or an evicted index). The client_host test still expected NOT_FOUND, so it failed on main.
Since yas-run#54 the server answers TIMEOUT when a command wait runs out before any command it could report on has started; wait_terminal_command's documentation still said NOT_FOUND, which stays for an evicted index or an exited terminal.
APPLY WRITE_INLINE items take item flag APPLY_ITEM_IN_PLACE (2), offered
with CAPABILITY_APPLY_IN_PLACE (32): the item writes as COMMIT of a
STAGE_IN_PLACE stage does (open with create and truncate through a final
symlink, no temporary, no rename), through one shared write_in_place. An
in-place write onto a directory is CONFLICT with {EISDIR, open} in the
item's ApplyOsErrors entry, as COMMIT's. The flag is INVALID on other item
kinds and with APPLY_ITEM_CREATE_PARENTS; APPLY_ITEM_EXTENDED_FLAGS lists it
beside the v1 baseline's APPLY_ITEM_FLAGS.
yas-client's write_in_place sends it for content within the server's
inline limit and stages the rest, so a write tool call that already spends
a round trip resolving or reading the file takes two in all, not three.
TypeScript codec parity (inPlace) and golden vector
fs.apply.in_place.payload.
There was a problem hiding this comment.
Ultimator's review of 6e22625
Adds APPLY_ITEM_IN_PLACE (advertised by CAPABILITY_APPLY_IN_PLACE) so FsRoot::write_in_place can write content up to the inline limit in one APPLY round trip. COMMIT's in-place body becomes a shared write_in_place, so both paths follow symlinks, handle EISDIR, mark operations and observe entries the same way. The wire, codec, vector and server logic check out (cargo test -p yas-wire --lib and cargo xtask protocol --check pass locally). One issue breaks the Windows build: the new InPlace::mode is only read under #[cfg(unix)], and the Windows job builds with -D warnings.
2 issues: 1 critical, 1 nit
No issue is open since the review of bcd1948.
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
| /// the content's hash, the operation marking the entry, and COMMIT flags | ||
| /// (`COMMIT_SYNC_DATA`, `COMMIT_SYNC_DIRECTORY`; APPLY gives none). | ||
| struct InPlace { | ||
| mode: u32, |
There was a problem hiding this comment.
🔴 Critical · InPlace::mode is never read on Windows, so the -D warnings build fails
The only read of how.mode is inside the #[cfg(unix)] block at line 2606. On Windows the field is set but never read, so rustc raises dead_code ("field mode is never read"). _build-windows.yml runs with RUSTFLAGS: "-D warnings" (cargo build --release -p yas-cli, cargo test --workspace), and mod yas_fs is compiled there, so that job fails. The old code didn't hit this because Stage::mode is also read by atomic_replace. I reproduced the lint with a minimal struct whose field is read only in a cfg'd-out block (rustc -D warnings: error: field mode is never read).
| mode: u32, | |
| #[cfg_attr(not(unix), allow(dead_code))] | |
| mode: u32, |
Ultimator · issue #1 · reply @ultimator-agent fix 1 to have it fixed
There was a problem hiding this comment.
Fixed in bcd1948 with your suggestion, plus a line on the field saying why it's Unix-only (a file Windows creates takes its directory's ACL). I have no Windows target locally, so fork CI's build-windows (indent-com#43) is the check. I'll report it here.
| `CAPABILITY_APPLY_IN_PLACE` (32); `CAPABILITY_FLAGS` (63) is their union. Absent means zero and receivers ignore | ||
| unknown bits. A client sends an opt-in question kind or flag only when its bit | ||
| is set; an older server rejects them as INVALID. Nothing changes for a client | ||
| that sends none of them. |
There was a problem hiding this comment.
⚪ Nit · Paragraph left unwrapped at 112 columns
The rest of this RFC wraps prose at 80 columns. Adding the new capability left line 2865 at 112 characters, which prettier won't fix because its prose wrap setting preserves lines.
| `CAPABILITY_APPLY_IN_PLACE` (32); `CAPABILITY_FLAGS` (63) is their union. Absent means zero and receivers ignore | |
| unknown bits. A client sends an opt-in question kind or flag only when its bit | |
| is set; an older server rejects them as INVALID. Nothing changes for a client | |
| that sends none of them. | |
| `CAPABILITY_APPLY_IN_PLACE` (32); `CAPABILITY_FLAGS` (63) is their union. | |
| Absent means zero and receivers ignore unknown bits. A client sends an opt-in | |
| question kind or flag only when its bit is set; an older server rejects them as | |
| INVALID. Nothing changes for a client that sends none of them. |
Ultimator · issue #2 · reply @ultimator-agent fix 2 to have it fixed
There was a problem hiding this comment.
Rewrapped at 80 columns in bcd1948. The other lines I added in yas.md and fs-write.md are all within 80.
Review of yas-run#89: the mode an in-place write gives a file it creates is read under cfg(unix) alone, so Windows' -D warnings build failed on dead_code (Stage::mode had atomic_replace to read it). Windows files take their directory's ACL; say so on the field. Rewrap yas.md's opt-in paragraph at 80 columns.
|
Reviewed |
There was a problem hiding this comment.
Ultimator's review of bcd1948
bcd1948 fixes both open issues. InPlace::mode now has #[cfg_attr(not(unix), allow(dead_code))], which is how this file already handles fields used on one platform only, so the Windows -D warnings build no longer fails on it. The capabilities paragraph is rewrapped at 80 columns. Nothing new to report.
No issues found · 2 resolved
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
What
This PR lets a small in-place write take one round trip instead of two.
APPLYWRITE_INLINEitems can now carry item flagAPPLY_ITEM_IN_PLACE(2), which servers advertise withCAPABILITY_APPLY_IN_PLACE(32). An item with the flag writes its content exactly asCOMMITof aSTAGE_IN_PLACEstage does, the way Node'swriteFiledoes: open with create and truncate through a final symlink, no temporary file, no rename.yas-client's
FsRoot::write_in_placenow sends it for content within the server's inline limit (MAX_INLINE_BYTES, 32 KiB), and stages anything bigger as before.Why
Ultimator writes files on YAS computers with
write_in_place, because its tools must behave as they do on the machine they run on: keep the inode, the mode and hard links, write through symlinks. Staging costsSTAGE_WRITE, then the upload andCOMMIT: two round trips. A write or edit tool call already spends one round trip reading or resolving the file, so it takes 3 RT (93/303/602 ms at 30/100/200 ms RTT, measured over netem). With one-round-trip writes it takes 2 RT.Wire
fs.toml:APPLY_ITEM_IN_PLACE= 2,APPLY_ITEM_EXTENDED_FLAGS= 2 andCAPABILITY_APPLY_IN_PLACE= 32;CAPABILITY_FLAGSbecomes 63;apply_in_place, which describes the semantics.apply_itemlayout andAPPLY_ITEM_FLAGSare unchanged. Decoders acceptAPPLY_ITEM_FLAGS | APPLY_ITEM_EXTENDED_FLAGS, following the precedent ofSTAGE_EXTENDED_FLAGS(cargo xtask protocol --checkpasses).INVALIDon any item kind other thanWRITE_INLINE, and together withAPPLY_ITEM_CREATE_PARENTS. The same rule applies toSTAGE_IN_PLACEwithSTAGE_CREATE_PARENTS.INVALID. Clients send it only when the capability bit is set.fs.apply.in_place.payload. The Rust decode gate and both TypeScript vector tests consume it.Server
commit_in_place's body is nowwrite_in_place(root, path, target, InPlace { mode, content_hash, operation_id, flags }, write). Both COMMIT and APPLY use it, so the two paths share:apply_itemsnow reports an item's OsError whenever it has one, including on aCONFLICT. An in-place write onto a directory is thereforeCONFLICTplus{EISDIR, open}, as COMMIT's is. No existing item kind produced a conflict that carried an OS error, so nothing else changes.Clients
ApplyItem::WriteInlinehas a newin_place: boolfield; the CLI sets it to false;FsRoot::write_in_placetakes the APPLY path whenfs_capabilities()has the bit and the content fits the server's advertised inline limit.inPlacefor codec parity. Encode and decode enforce the same rules as Rust.Tests
yas-wire
an_in_place_apply_write_is_a_write_inline_flag_without_create_parents: round trip, every truncation, the flag's position on the wire, rejection with create-parents (on encode and decode), rejection on MKDIR.yas-server:
stage_in_place_keeps_the_inode_and_writes_through_symlinksbody is now shared by it and a newapply_in_place_writes_as_commit_in_place_does;an_apply_in_place_onto_a_directory_is_a_conflict_saying_eisdir: the ApplyOsErrors entry, a replacing write's bare conflict alongside, and create-parents refused.client_host
files_answer_as_the_os_does:write_in_placeof 11 bytes (APPLY) and ofMAX_INLINE_BYTES + 1bytes (staged) each answer:CONFLICT+ EISDIR;Negative control: a server whose in-place APPLY writes other bytes fails this test at the 11-byte write, so small writes do go through APPLY.
TypeScript:
yas.test.tsandyasFs.test.tsround-trip the new vector and check the decodedinPlace.Checks
Run locally at 6e22625:
cargo fmt --checkandcargo xtask protocol --checkpass;--workspace --all-targetsand yas-server--no-default-featuresgive no warnings;--include-ignored;tsc --noEmitpasses, and vitestyas.test.ts+yasFs.test.tspass 58/58;Review r1 (bcd1948):
InPlace::modeis read only undercfg(unix), so it is nowallow(dead_code)elsewhere, which fixes Windows-D warnings. The capabilities paragraph in yas.md is rewrapped. Fork CI (indent-com#43) checks the Windows build; results will be posted here.Based on #83's branch, so fork CI runs past the stale journal test. It is independent of #85–#88, but the generated files (
generated.rs,generated.ts,schema.json,vectors.json,wire.md) will conflict textually with #86/#88. Regenerate them withcargo xtask protocolon merge.