Repository navigation
Conversation
`/tree` lists every file in a revision with its git blob ID, and `/read` returns the bytes at given paths, paging past a 32 MiB ceiling through `remaining`. realm-server's recovery uses them to compare a realm's disk with HEAD and to bring disk up to HEAD. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnysKbB8Y5FgNf4nQuAd6M
A caller that journals revisions needs each one's real parent. A replay answered with only the revision left the caller to assume its parent was the HEAD it asked against, which is wrong once other commits have landed since. /commit and /outcome now answer with the revision's parent, and with whether the operation wrote that revision or found the tree unchanged, which a replay could not tell apart before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnysKbB8Y5FgNf4nQuAd6M
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review covered correctness of the new /tree and /read routes against what realm-server recovery will rely on: entry order, the absent meaning, and paging. It also covered the parent / created semantics on replays and in /outcome. I read jj-lib 0.46's tree iteration to check the ordering claim. I did not run the test suite locally.
No blocking issues. The parent/created changes are sound. The two /tree and /read contract points below are worth settling before the recovery consumer builds on them.
/treedocuments "path order", but entries come back in jj's per-directory component order, which putsAuthor/jane.jsonbeforeAuthor.gts. Sort by full path or document the actual order, and add a test that tells the two apart. (See the comment on README.mdPOST /tree.)/readreports symlinks, submodules, and conflicts asabsent, which a recovery caller will act on by deleting the file. Error on them the waytree()does. (See the comment on the_ => Nonearm inCommitService::read.)- Follow-up: bound the read of the file that trips the 32 MiB ceiling instead of reading it in full and discarding it. (See the comment on
read_to_end.)
| } | ||
| ``` | ||
|
|
||
| Every file in the revision's tree, in path order. `blob` is the git blob ID |
There was a problem hiding this comment.
[Claude Code 🤖] "In path order" doesn't describe the order /tree actually returns. A caller that merge-joins the list against a sorted disk listing will get mismatches. CommitService::tree passes along jj's MergedTree::entries(), which walks depth-first and sorts each directory by component name (jj-lib 0.46 TreeEntriesIterator; GitBackend::read_tree re-sorts git's entries by component). So a directory comes before a sibling file whose name it prefixes:
/treereturnsAuthor/jane.json, thenAuthor.gts- full-path byte order, git order, and JS
Array.prototype.sortall putAuthor.gtsfirst (.0x2e </0x2f)
Realms routinely have a Foo.gts next to a Foo/ instance directory, so recovery will hit this case.
Either sort entries by the full path string before returning (entries.sort_by(|a, b| a.path.cmp(&b.path)), then "byte order of path" is a precise contract), or document the order as it is. I'd sort, since a flat byte order is what a caller can reproduce. Either way, tree_lists_every_file_with_its_git_blob_id should cover a.txt + a/b.txt: the current b.json / dir/a.gts pair has the same order under both rules, so it can't tell them apart. The doc comment on CommitService::tree makes the same claim.
Regression (new contract in this PR), non-blocking unless the realm-server consumer relies on the order.
| .map_err(internal)?; | ||
| Some(bytes) | ||
| } | ||
| _ => None, |
There was a problem hiding this comment.
[Claude Code 🤖] This catch-all reports a symlink, submodule, or conflicted value as absent, while tree() treats exactly those as Internal ("a repo this service did not write"). For the recovery caller, absent is an instruction: it makes the on-disk copy match HEAD, which means deleting the file. So the one case this service calls corruption becomes a delete on disk instead of an error.
Narrow None to the two cases the README promises, and error on the rest the way tree() does:
Some(None) | Some(Some(TreeValue::Tree(_))) => None,
_ => {
return Err(CommitError::Internal(format!(
"{path:?} is not a regular file in revision {revision}"
)))
}Regression, non-blocking. It only matters for a repo something other than /commit has written to, but the cost when it does is data loss.
| let mut bytes = Vec::new(); | ||
| repo.store() | ||
| .read_file(&repo_path, id) | ||
| .await | ||
| .map_err(internal)? | ||
| .read_to_end(&mut bytes) | ||
| .await | ||
| .map_err(internal)?; | ||
| Some(bytes) |
There was a problem hiding this comment.
[Claude Code 🤖] The file that trips the ceiling is read in full and then thrown away, and the next call reads it again. With /commit accepting up to a 64 MiB body, that can be about 48 MiB read and discarded, on top of the 32 MiB already held. Cap the read so it stops one byte past what still fits:
let budget = if index == 0 { u64::MAX } else { (READ_MAX_BYTES - total) as u64 + 1 };
reader.take(budget).read_to_end(&mut bytes)Then bytes.len() > READ_MAX_BYTES - total means stop. Non-blocking, can be a follow-up.
Explicit-commit repos (
/commit,/outcome,/head,/tree-hash) could record a revision but not read one back. realm-server's recovery needs both: it compares a realm's disk with HEAD, and it writes HEAD's bytes to disk for a revision a crash left half-written. This adds two routes for those repos.POST /tree { repo, revision }answers every file in the revision, in path order, with its git blob ID and executable bit. A caller hashes a file on disk the way git does (git hash-object) and compares, without reading the revision's copy.POST /read { repo, revision, paths }answers each path's bytes (base64), orabsentwhere the revision has no file there, including a directory. One answer carries at most 32 MiB of content. Paths past that are listed inremaining, for the caller to ask for again. The first path is always read, so every file can be read.Both resolve the revision the way
/tree-hashdoes: they read without the repo locks, and take them only when this host's view doesn't show the revision yet. A revision outside the repo's history isrevision-not-found./readchecks paths against/commit's rules. The README documents both routes./commitand/outcomealso answer with the revision'sparent. A caller that journals revisions needs each one's real parent. Until now a replay answered with only the revision, so the caller had to assume its parent was the HEAD it asked against. That is wrong once other commits have landed on top.createdon a replay now says whether the operation wrote that revision or found the tree unchanged./outcome'scommittedanswer carries the same two fields.Tests in
history/commit.rs:/treematchesgit hash-object, and an empty revision lists nothing./readat two revisions covers a modified file, a deleted file, a directory and a path that never existed.cfg(test), splits a read and still returns a lone oversized file.created: false. The restart test checks/outcome's new fields.Consumed by cardstack/boxel's realm recovery, CS-14362.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AnysKbB8Y5FgNf4nQuAd6M