Skip to content

deckd: commit staged content idempotently by operation ID with HEAD compare-and-swap - #5

Merged
lukemelia merged 3 commits into
mainfrom
cs-14358-deckd-commit-staged-content-idempotently-by-operation-id
Oct 27, 2026
Merged

lukemelia merged 3 commits into
mainfrom
cs-14358-deckd-commit-staged-content-idempotently-by-operation-id

Conversation

@lukemelia

@lukemelia lukemelia commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

A writer such as realm-server can now record each write in a realm's history exactly once, even when deckd's response is lost. A revision never contains another request's unrecorded bytes.

New endpoints work on a bare jj repo (internal git backend, no working copy) addressed by an absolute repo path. deckd creates the repo on first use. When DECKD_REPO_ROOT is set, repo must resolve inside it.

  • POST /commit builds one revision from an explicit {path, contentBase64 | delete} list on top of parent.
    • When parent isn't HEAD, it refuses with 409 parent-mismatch and the current head.
    • Retrying an operation ID that already committed returns the original revision with replayed: true. A retry with a different request (anything but the parent) gets 409 operation-mismatch.
    • Changes that leave the tree as it was create no revision; the response returns HEAD with created: false.
    • It refuses a change that would turn a file into a directory or back, and any path with a . or .git segment. Rewriting a file keeps its executable bit.
  • POST /outcome reports committed (with its revision) or absent. Reporting absent fences the ID, so a later commit with it gets 409 operation-fenced.
  • POST /head reports HEAD; POST /tree-hash reports the git tree ID of a revision in history. Identical content has the same tree ID in any repo.
  • Every error body is { code, error } on every route, including requests that can't be parsed.

The README documents the API, with examples and error codes. docs/deck-daemon.md lists the routes.

Key decisions

  • State lives in files, not in deckd's memory. Outcomes and fences are records under <repo>/deckd/operations/, so they survive restarts and are shared by every deckd on the same filesystem.
    • Whether a record exists is only learned from an O_EXCL create, which an NFS server answers.
    • Records and their directory entries are synced before a call returns. Each record keeps a digest of the request that claimed it, which is what a replay is checked against.
  • One lock file, never deleted. commit, outcome and head lock <repo>/deckd/HEAD with File::try_lock, behind a per-repo in-process mutex. jj-lib's FileLock isn't used: it deletes its file on release, which lets two NFS clients each hold the lock.
    • The mutex is keyed by the repo directory's device and inode, so every spelling of a repo shares it. Gates no call holds are pruned.
    • Both locks share one 30 s deadline, then 503 lock-timeout.
  • A recorded HEAD checks jj's. deckd/HEAD holds the revision about to be published. It is written after the commit object and before jj publishes, and read through the locked descriptor, so an NFS lock keeps it current even when this host's listing of jj's operations is stale. jj's HEAD must match it before anything is written.
    • When jj is one commit behind (a crash between recording and publishing, or a stale listing), the recorded commit is published. Publishing it twice merges to the same single head.
    • Any other mismatch is refused with 503 history-unsettled after a short wait, with nothing written.
    • The file can't be replaced by rename, since it is the lock. It holds two checksummed slots, and each write goes to the slot without the latest value, so a torn write leaves the previous revision readable.
    • A revision is recorded only after its git objects are fsynced. git doesn't sync loose objects.
  • Lost operation records are repaired from HEAD. Every commit carries a Deckd-Operation-Id trailer, and each locked call first records HEAD's operation as committed. A fenced record for HEAD's operation means the lock failed to exclude a writer. It is reported as an error and never overwritten.
  • /outcome is a POST because it can change state.
  • /commit bodies are capped at 64 MiB, since the body and its decoded copy are held in memory.
  • jj-lib goes from 0.43 to 0.46. 0.46 needs rustc 1.97.1, so packages/deckd/rust-toolchain.toml pins 1.99.1. CI's deckd steps run from that directory so rustup reads the pin, and CI now runs cargo test --locked.
    • 0.46 skips snapshot files whose names aren't valid UTF-8, where 0.43 failed the whole snapshot. snapshot_wc logs each skipped file.

Test plan

  • cargo test --locked: 35 unit tests, plus 4 integration tests against the real binary.
    • Integration:
      • a commit's outcome and a fence each survive a restart;
      • unparseable and oversized requests get typed errors.
    • Unit coverage:
      • CAS refusal carries the current HEAD;
      • idempotent replay works even with a stale parent;
      • fences hold across instances;
      • unlisted files on disk are excluded;
      • file/directory crossings and ./.git paths are refused;
      • deletes remove the path;
      • no-op commits create no revision;
      • tree hashes are equal across repos;
      • a recorded but unpublished revision is published by the next call;
      • a recorded HEAD jj can't reach is refused;
      • a fenced record for HEAD is reported, not overwritten;
      • a lost race to initialize keeps the existing repo;
      • a torn deckd/HEAD write keeps the previous revision;
      • new objects sit where the sync looks for them;
      • a reused operation ID with a different request is refused, including after a crash repair;
      • spellings of one repo share a gate, which is pruned afterwards;
      • a held gate times out;
      • repos outside DECKD_REPO_ROOT are refused;
      • an unpublished commit has no tree hash;
      • rewriting an executable file keeps it executable.
    • Race: eight independent writers from one parent: exactly one wins, and retries keep history linear.
    • Mutation checks: each crash-window, lock, path, durability and replay test fails with its fix removed.
  • scripts/smoke.sh against the new build (ensure, seal, list, file-list-at, watch plane).
  • pnpm test:collaboration with DECKD_BIN pointing at the new build.
  • pnpm lint, and cargo clippy --all-targets with no new warnings.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Tg2ZC6YEPfRcNZSoWKq2HS

deckd can now record a write exactly once, from content the caller
staged, against a bare history repo addressed by path:

- POST /commit builds a revision from an explicit {path, bytes | delete}
  list on top of `parent`, refusing with `parent-mismatch` (and the
  current HEAD) unless `parent` is HEAD. Retrying an operation ID that
  already committed returns the original revision.
- POST /outcome reports `committed` or `absent`; `absent` fences the ID
  so it can never commit later.
- POST /head and POST /tree-hash report HEAD and a revision's git tree ID.

Operation records are files under the repo, tested with O_EXCL creates,
and every commit carries a Deckd-Operation-Id trailer that repairs a
record lost to a crash. A flock serializes deckd processes.

Also bumps jj-lib to 0.46, pins the Rust toolchain it needs, and runs
cargo test in CI.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Tg2ZC6YEPfRcNZSoWKq2HS

@lukemelia lukemelia left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] This review covered correctness on the explicit-commit path: path handling, crash windows, cross-process locking on NFS, and the HTTP error contract. It didn't measure performance, and it looked at the existing seal/watch endpoints only where the jj-lib bump touches them.

Bottom line: three blocking issues. Listed paths can silently remove unlisted ones, . and .git segments land in permanent history, and the commit lock can be held twice on NFS. There are also four non-blocking fixes.

  1. File/directory overlaps change paths the request didn't list. Blocking; see build_tree.
  2. Reject . and .git segments on writes. Blocking; see the path check in validate_commit_request.
  3. Use a persistent lock file, don't let repair overwrite a fence, and answer the HEAD-freshness question. Blocking; see lock_repo.
  4. A stale lookup of jj/ causes a 500 or a false 404. Non-blocking; see load_or_init.
  5. Request-parsing errors bypass the {code, error} body. Non-blocking; see the commit handler.
  6. docs/deck-daemon.md §2, "RPC surface", lists only the older routes. Add /commit, /outcome, /head and /tree-hash. Non-blocking.
  7. jj-lib 0.46 changes snapshots on the existing seal path. A file whose name isn't valid UTF-8 used to fail the snapshot (SnapshotError::InvalidUtf8Path in 0.43). Now it's skipped and reported in the snapshot stats, which snapshot_wc discards as _stats, so such a file silently never enters history. Log invalid_utf8_paths there. Non-blocking.

}
None => Merge::absent(),
};
builder.set_or_remove(path, value);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] A listed path that crosses a file/directory boundary silently changes paths the request didn't list. MergedTreeBuilder replaces whatever is in the way. Against the built binary, each of these returned 200 with created: true:

  • a is a file, request writes a/b: a is gone.
  • d/x and d/y exist, request writes d: both files are gone.
  • One request writes e and e/f: only e/f lands.
  • a/ is a directory, request deletes a: the whole subtree is gone.

That breaks the "exactly the listed paths" guarantee in the README. Refuse these with invalid-request:

  • In validate_commit_request: any listed path that is a directory prefix of another listed path.
  • Here, against parent.tree():
    • a write whose ancestor is a file;
    • a write onto an existing directory;
    • a delete whose target is a directory.

Add a test for each case.

Regression, blocking.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in c8ccedb. These are now refused with invalid-request:

  • a listed path inside another listed path;
  • a path inside a file in HEAD;
  • a path that names a directory in HEAD.

A refused request leaves no claim, so the operation ID stays usable. changes_may_not_cross_a_file_directory_boundary covers all four cases from this thread, plus delete a with write a/b in the same request. It fails with the checks removed.

Comment thread packages/deckd/src/history/commit.rs Outdated
}
let mut seen = HashSet::new();
for change in &request.changes {
if !is_valid_history_path(&change.path) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] is_valid_history_path lets . segments and .git through, and on this path they become permanent tree entries. x/./y and .git/config both committed, and git fsck on the store reports hasDot and hasDotgit.

History only ever grows, so a bad tree can't be cleaned up later. a/./b and a/b also pass the duplicate check as two different paths.

Reject . segments and .git (case-insensitively) on writes, either by tightening the shared helper or with a check here.

is_valid_history_path predates this change and the read endpoints use it too. This change makes it the guard for writes, so it's blocking for the write path.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in c8ccedb. Writes go through is_valid_commit_path, which also rejects . segments and .git in any case. Reads keep is_valid_history_path unchanged. dot_and_git_segments_are_refused covers ., x/./y, .git/config and x/.GIT/hooks, and checks that .gitignore and .github/ still commit.

Comment thread packages/deckd/src/history/commit.rs Outdated
let lock_path = deckd_dir.join("commit.lock");
let started = Instant::now();
loop {
match FileLock::try_lock(lock_path.clone()).map_err(internal)? {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] The CAS and the fence both assume this lock excludes other processes. jj-lib's FileLock deletes commit.lock when dropped, before unlocking, and that isn't safe on NFS:

  • Unlinking a file this client still has open becomes a silly-rename, so the old inode stays alive under a hidden name.
  • A peer that opened commit.lock before the delete wins the flock on that renamed inode. It sees st_nlink == 1 and proceeds.
  • Meanwhile a third process creates and locks a fresh commit.lock.

I found this by reading jj-lib's lock/unix.rs; it can't be reproduced on local disk. Two lock holders cause two failures, and nothing recovers from either today:

  • Two heads: both writers publish on the same HEAD. jj merges the op heads, and head_commit then fails with "history has 2 heads" on every later call to that repo.
  • Fence overwritten: one process runs /outcome while the other is mid-commit, and fences an ID that then publishes anyway. repair_head_record rewrites fenced to committed, so an ID reported absent ends up committed.

Suggested fix:

  • Lock a persistent deckd/commit.lock that is never deleted, using std::fs::File::try_lock (stable since Rust 1.89).
  • Have repair_head_record return an error and log when it finds a fenced record, rather than overwriting it.

A related question about the same invariant: HEAD comes from jj's op_heads directory listing, and holding a flock doesn't revalidate a client's directory cache. What stops a replica from reading a stale HEAD here right after a peer commits? If nothing does, a deckd-owned deckd/HEAD file read through open() (which is close-to-open consistent) could serve as the CAS source, checked against jj's head.

Regression, blocking for the lock fix, which is small. The HEAD-freshness question can be answered here or deferred to the EFS feasibility work.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in c8ccedb, including the HEAD-freshness question.

  • The lock: commit, outcome and head lock deckd/HEAD with File::try_lock, and that file is never deleted.
  • HEAD: the same file records the revision about to be published. It is written after the commit object and before jj publishes. It's read through the locked descriptor, and taking an NFS lock revalidates that file's cached data.
    • jj's HEAD must equal it before anything is written.
    • When jj is exactly one commit behind (a crash between recording and publishing, or a stale op_heads listing), the recorded commit is published. Publishing it twice merges to the same single head.
    • Any other mismatch reloads for up to 5 s, then returns 503 history-unsettled with nothing written.
  • The fence: repair_head_record now returns an error and logs when HEAD's operation has a fenced record. It never overwrites one.

New tests, each failing with its fix removed:

  • commit_recorded_but_not_published_is_published_by_the_next_call;
  • head_that_disagrees_with_the_record_is_refused;
  • fenced_record_for_head_is_reported_not_overwritten;
  • lock_file_records_head_and_persists.

racing_writers_stay_linear fails with the new lock bypassed.

/// crash mid-init never leaves a half-made `jj/`.
async fn load_or_init(repo_dir: &Path) -> CommitResult<Arc<ReadonlyRepo>> {
let jj_dir = repo_dir.join("jj");
if !jj_dir.is_dir() {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] jj_dir.is_dir() trusts a cached negative lookup, which is exactly what the stale-NFS rule guards against. If a peer has just created jj/, this builds a second repo in staging. The rename into jj then fails with ENOTEMPTY, so the call returns 500 and leaves a deckd/init-* directory behind.

tree_hash and the fast path in head make the same check, so tree_hash can return 404 for a revision that exists.

Suggested fix:

  • Treat AlreadyExists or DirectoryNotEmpty from the rename as "already initialized": remove the staging directory, then load the repo.
  • In head and tree_hash, try loading the repo before falling back.

Regression, non-blocking (the window lasts seconds).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in c8ccedb. install_repo treats AlreadyExists or DirectoryNotEmpty from the rename as already initialized: it removes the staging directory and loads the existing repo. tree_hash loads the repo first, and returns revision-not-found only when that fails and jj/ is absent. head now takes the lock and goes through the same init path as commit. initializing_over_an_existing_repo_keeps_it covers the collision.

Comment thread packages/deckd/src/api.rs Outdated

async fn commit(
State(st): State<AppState>,
Json(body): Json<CommitBody>,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Malformed requests bypass ApiError. axum's Json rejection answers in plain text:

  • 400 for bad JSON;
  • 422 for a missing field;
  • 415 without the JSON content type;
  • 413 over the size limit.

I checked the first three against the binary. So the README's "every error body is { code, error }" doesn't hold, and a client that reads code on every non-2xx response will fail to parse these.

Wrap the extractor on every route: either axum_extra::extract::WithRejection, or a small FromRequest that maps JsonRejection to invalid-request and payload-too-large.

While there: COMMIT_BODY_LIMIT lets one request buffer 512 MiB in the sidecar, plus its decoded copy. A limit near the realm batch size would stop one bad request from taking the task down.

Regression (the README claim is new), non-blocking.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in c8ccedb. Every route takes its body through a Body<T> extractor that maps JsonRejection to ApiError. The status stays the same (400, 415 or 422), with invalid-request, or 413 with payload-too-large. COMMIT_BODY_LIMIT is now 64 MiB. tests/http_errors.rs checks all four shapes against the real binary, including on /list, and the README's error table lists them.

- Refuse changes that cross a file/directory boundary (a listed path
  inside another listed path, inside a file in HEAD, or naming a
  directory in HEAD) and paths with `.` or `.git` segments, so a
  revision holds exactly the listed paths and only valid git trees.
- Lock a persistent deckd/HEAD instead of jj-lib's FileLock, which
  deletes its file on release and so can be held twice over NFS. The
  file records the revision about to be published; jj's HEAD must match
  it before a write, a recorded-but-unpublished revision is published by
  the next call, and any other mismatch is refused as history-unsettled.
- Never overwrite a fenced record for HEAD's operation; report it.
- Treat a lost race to initialize jj/ as already initialized, and let
  /tree-hash load before deciding a repo is missing.
- Answer unparseable requests with typed {code, error} bodies on every
  route, and lower the /commit body limit to 64 MiB.
- Warn when a snapshot skips a file whose name isn't valid UTF-8.
- List the explicit-commit routes in docs/deck-daemon.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Tg2ZC6YEPfRcNZSoWKq2HS
@lukemelia

Copy link
Copy Markdown
Collaborator Author

[Claude Code 🤖] The two review-body items are fixed in c8ccedb:

  • 6. docs/deck-daemon.md §2 has a table for /commit, /outcome, /head and /tree-hash, linking to the README for the full contract.
  • 7. snapshot_wc logs a warning for each file the snapshot skips because its name isn't valid UTF-8. macOS won't create such a name, so this has no local test.

@lukemelia
lukemelia marked this pull request as ready for review October 26, 2026 20:36

/// Overwrite `deckd/HEAD` in place. Every revision has the same length, so
/// the file is never truncated and a reader never sees it empty.
fn write_recorded_head(head_file: &mut File, revision: &str) -> CommitResult<()> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

deckd/HEAD is rewritten in place (seek+write_all+fsync), which is not atomic; a crash or power loss mid-write can leave a torn mix of old and new hex bytes.

Power loss during write_recorded_head leaves 40 hex chars that are half the old revision and half the new one. Every later commit/outcome/head call then fails: either read_recorded_head returns 'unreadable deckd/HEAD' (Internal), or settled_head can't find the commit and loops to HistoryUnsettled. The repo is wedged until an operator steps in. The file can't be replaced by rename because it is the lock, so the recorded HEAD needs a separate atomically renamed file, or a checksummed or double-buffered format.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 812d3e3. deckd/HEAD now holds two fixed-size slots, each <seq> <revision> <checksum>. A write goes to the slot that doesn't hold the latest value, and a reader takes the intact slot with the highest sequence. A torn write therefore leaves the previous revision readable. That is the right answer, because the commit point hadn't been reached. The file stays the lock and is still read through the locked descriptor. torn_head_write_keeps_the_previous_revision tears a real write in half (half new bytes, half old) and checks that HEAD and the next commit both work. It fails against the single-slot format.

let revision = commit.id().hex();
// The commit point: from here a crash leaves this revision as HEAD, and
// the next locked call publishes it if jj hasn't.
write_recorded_head(head_file, &revision)?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The recorded HEAD is fsynced before the new commit's git objects are known to be durable, so after a power loss deckd/HEAD can name a commit the store doesn't have.

commit.write() writes loose git objects without an fsync. write_recorded_head fsyncs deckd/HEAD. Then the machine loses power before the page cache is flushed. On restart recorded != jj head and store().get_commit(recorded) fails, so settled_head returns HistoryUnsettled on every call and the repo is permanently wedged.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 812d3e3. Before deckd/HEAD is written, sync_new_objects fsyncs the new commit's loose objects: the commit, the root tree, every tree on the way to a changed path, and each written blob. It then syncs their fan-out directories and objects/. An object with no loose file was already packed. jj's extras table doesn't need the same treatment: read_commit re-imports a commit whose extras are missing. new_objects_are_loose_where_they_are_synced checks that each of those objects is at the path the sync opens, so a wrong path fails the test instead of skipping the sync.

Comment thread packages/deckd/src/history/commit.rs Outdated

/// Create the record with `create_new`. Returns false when it already exists.
/// An empty record (a crash between create and write) reads as `pending`.
fn claim_record(path: &Path, record: Record) -> CommitResult<bool> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

claim_record fsyncs the new record file but not the operations/ directory, so the creation of a 'fenced' (or 'pending') record can be lost on crash.

/outcome answers 'absent' for op X after claim_record(Fenced) creates the file, but the directory entry is never fsynced. A power loss drops the entry, and the caller's retried /commit with op X then claims it fresh and commits. That breaks the guarantee that an operation reported absent can never commit. write_record syncs the dir, but claim_record (and the remove_file paths) don't.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 812d3e3. claim_record syncs the operations directory after the record itself, so a fenced or pending claim survives a crash once the call returns. Removing a claim (after a parent mismatch or a refused change) syncs it too. A removal that is lost anyway is harmless: the claim comes back as pending, which reads as never published.

Comment thread packages/deckd/src/history/commit.rs Outdated
})
}

fn gate(&self, repo_dir: &Path) -> Arc<Mutex<()>> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The in-process gate is keyed on the raw request path, so two spellings of one repo (a symlink, a bind mount, different case on a case-insensitive FS) get separate mutexes and the per-process exclusion the module docs rely on is lost.

Requests for /realms/a/.history and /mnt/realms/a/.history (a symlink to it) run at the same time in one process. On NFS, flock is emulated with POSIX locks that don't exclude threads of one process, so both hold 'the' lock. Both see the same HEAD and both commit, giving two heads (head_commit then errors 'history has 2 heads' forever). Also, closing either fd releases the POSIX lock for both. Canonicalize repo_dir (after create_repo_dir) before keying the gate.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 812d3e3. The gate is keyed by the repo directory's (dev, ino) after the path is resolved, so symlinked, bind-mounted and differently-cased spellings share one mutex. Every call resolves the path first and uses the resolved path from then on. spellings_of_one_repo_share_a_gate_that_is_pruned reaches one repo through a symlinked parent and checks that both spellings get the same gate.

}
}

fn validate_repo_dir(repo_dir: &Path) -> CommitResult<()> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

validate_repo_dir accepts any absolute path, so any HTTP caller can make deckd create directories, a jj repo and lock files anywhere the daemon user can write.

POST /head {"repo":"/home/deck/.ssh/x"}, or any path whose parent exists, creates /jj and /deckd/ outside any realm. /commit can then write arbitrary git objects there. Nothing restricts repo to a configured root such as the realms directory.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 812d3e3. When DECKD_REPO_ROOT is set, repo must resolve, with symlinks followed, to a directory strictly inside it. The check runs before anything is created, and /tree-hash applies it too. Unset, any absolute path is accepted as before. That keeps local dev and the existing smoke and collaboration setups working; the README says deployments should set it to the realms directory. repos_outside_the_root_are_refused covers a sibling directory, a symlink inside the root that points out of it, and the root itself, and checks that nothing was created outside.


let record_path = operation_record_path(repo_dir, &request.operation_id);
if !claim_record(&record_path, Record::Pending)? {
match read_record(&record_path)? {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A replay of an already-committed operation ID returns success without checking that the request (parent, changes, message) matches the original, so a reused or colliding operation ID silently drops the new write.

A caller bug reuses op ID 'abc' for a different write on a newer parent. deckd returns {replayed:true, revision:} with 200, and the caller believes its new content was recorded. It wasn't, and no parent-mismatch is reported. Storing a request digest in the record and refusing a mismatched replay would surface this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 812d3e3. A claim now records a SHA-256 digest of the request: author, message, trailers and changes, with changes order-independent since paths are unique. A committed record keeps that digest. Replaying the ID with a different request is a 409 operation-mismatch whose body carries the original revision. The parent is left out on purpose: a retry may legitimately name a newer parent. The HEAD repair carries a pending claim's digest into the committed record, so a crash between publishing and recording doesn't lose the check. Taking over a stale pending claim rewrites it with the new request's digest. Tests:

  • reused_operation_id_with_a_different_request_is_refused covers different content, a dropped change and a different message, plus a same-request replay from a newer parent in another order;
  • unchanged_content_replay_is_checked_too covers no-op commits;
  • the roll-forward test covers the repaired record.

Comment thread packages/deckd/src/history/commit.rs Outdated
validate_operation_id(&request.operation_id)?;
validate_commit_request(&request)?;
let gate = self.gate(repo_dir);
let _in_process = gate.lock().unwrap_or_else(|e| e.into_inner());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The in-process gate mutex is acquired with no timeout, while only the cross-process file lock honors LOCK_TIMEOUT, so requests to one busy or stuck repo queue without bound on blocking threads.

A slow NFS commit holds the gate. Every other /commit, /outcome or /head for that repo parks a spawn_blocking thread on gate.lock() indefinitely, with no lock-timeout 503. Under load this exhausts tokio's blocking pool (512), which also stalls the unrelated history endpoints that use spawn_blocking.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 812d3e3. The gate is taken with try_lock against the same deadline as the file lock, so both locks together give up after LOCK_TIMEOUT with a 503 lock-timeout. A queued call parks a blocking thread for at most that long. busy_gate_times_out holds the gate and checks that /head times out.

}
}

fn get_commit(repo: &Arc<ReadonlyRepo>, revision: &str) -> CommitResult<Commit> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

get_commit reads straight from the git store, so /tree-hash and the replay path accept commits that were never published to history (for example, objects left by a crashed attempt).

An attempt writes its commit object, then fails before write_recorded_head, and the record is taken over. /tree-hash with that orphan revision returns 200 and a tree, though the revision is not in the repo's history. The README's contract is 404 revision-not-found for a revision the repo doesn't have.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 812d3e3. /tree-hash answers only for a revision in the index that is an ancestor of, or equal to, a head. The fast path checks the unlocked view. On a miss it takes the lock and checks against the settled HEAD, so a peer's just-published commit isn't a false 404. unpublished_commit_has_no_tree_hash writes a commit object without publishing it and gets revision-not-found. The replay path can't see an orphan: a committed record is written only after publishing, by the repair from HEAD, or for a no-op as HEAD itself.

.write_file(&path, &mut bytes.as_slice())
.await
.map_err(internal)?;
Merge::normal(TreeValue::File {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overwriting an existing path always writes executable: false, so changing the content of an executable file (or replacing a symlink) silently drops its mode or type.

The parent tree has scripts/run.sh as executable. A commit that updates its bytes produces a non-executable file in the new revision, a mode change the request never asked for. Carry over the existing value's executable flag when the path already holds a file.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 812d3e3. Writing over an existing file keeps its executable bit. A write over a symlink or submodule produces a regular file with the given bytes, since those bytes are what the request states. Explicit commits only ever write regular files, so both cases come only from history imported some other way. rewriting_an_executable_file_keeps_it_executable seeds an executable file directly through jj, then rewrites it.

Comment thread packages/deckd/src/history/commit.rs Outdated
/// state that correctness depends on; a fresh instance behaves exactly like
/// one that has been running.
pub struct CommitService {
gates: Mutex<HashMap<PathBuf, Arc<Mutex<()>>>>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The gates map gains an entry for every distinct repo path ever requested and never removes one, so it grows for the life of the process.

A long-running deckd serving many realms, or callers sending varied or bogus absolute paths (each is inserted before any validation that the repo exists), keeps a PathBuf plus an Arc per path forever. Remove entries when the Arc is unique after use, or use a weak-value map.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 812d3e3. The map holds Weak gates keyed by (dev, ino), and each lookup drops entries no call holds. It is therefore bounded by the repos in use at once. A key is only computed after the path resolves to an existing directory, so a bogus path never adds an entry. spellings_of_one_repo_share_a_gate_that_is_pruned checks that a finished repo's gate is gone after the next call.

- deckd/HEAD holds two checksummed slots; a write goes to the slot without
  the latest value, so a torn write leaves the previous revision readable.
- The new commit's git objects are synced before deckd/HEAD records it.
- Claiming or removing an operation record syncs its directory entry.
- Operation records keep a digest of the claiming request (everything but
  the parent); replaying a committed ID with a different request is a 409
  operation-mismatch. The HEAD repair keeps a pending claim's digest.
- The in-process gate is keyed by the repo directory's device and inode,
  is taken with the same deadline as the file lock, and is pruned once no
  call holds it.
- DECKD_REPO_ROOT, when set, confines explicit-commit repos to a directory.
- /tree-hash only answers for revisions in history; an unpublished commit
  object is revision-not-found.
- Rewriting an existing file keeps its executable bit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Tg2ZC6YEPfRcNZSoWKq2HS
@lukemelia
lukemelia requested a review from habdelra October 26, 2026 21:57
@lukemelia
lukemelia merged commit 95bfe7a into main Oct 27, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants