Skip to content

GET /files and /tree take the exclusive write lock, serializing all reads #171

Description

@Hazzng

Summary

GET /v1/sandboxes/:id/files/* (routes/files.ts:151) and GET /:id/tree (:550) acquire the exclusive write lock via withOwnedSessionOrRehydrate. withOwnedSessionRead exists (src/api/ownership.ts:69) and no HTTP route uses it — only the MCP read tools do.

Root cause: chronology, not a safety decision

  • Both GET handlers were last touched by ffa3fe8 (2026-04-25).
  • withOwnedSessionRead did not exist until 34228c1 (2026-05-10, parallel readOnly exec). That commit wired the shared path into exec.ts and mcp/tools.ts only; the April handlers were never retrofitted.
  • Confirming it is an oversight: 5a8a29e added MCP file_read, doing byte-for-byte the same work (stat + readFileBuffer), and it uses withOwnedSessionRead (mcp/tools.ts:213). Two implementations of the same read, two lock modes.
  • No comment, changeset, or design record gives a coherence reason. ensureFreshCache is called identically on both paths.

Important correction to expectations

This is not what causes a GET to wait behind a running exec. A shared reader still excludes an exclusive writer — that is what an RW lock does. Measured against one in-flight 5 s writer:

GET /files (exclusive) 4.330 s
exec-sync readOnly (shared) 4.343 s
GET /tree (exclusive) 4.389 s

The shared path waited 13 ms longer. Anyone expecting this change to fix "GET blocked 4.5 s by an exec" will be disappointed — that belongs to the exec-disconnect and acquire-timeout issues.

The real win: reader-reader parallelism

Same file, warm session, no writer:

Concurrency exclusive (GET /files) shared (MCP file_read, identical work)
1 5 ms 5 ms
12 wall 147 ms, max 112 ms wall 12 ms, max 10 ms
32 wall 197 ms, p50 50 ms wall 61 ms, p50 13 ms

~12x lower wall time and ~11x lower tail at 12-way concurrency. Secondary win: an in-flight GET currently holds the writer flag, which by writer-priority also blocks queued readers and forces a real writer to wait for a drain cycle.

Is switching safe? Yes, with one caveat

  • Writers and readers remain mutually exclusive under the shared path (ACQUIRE_SHARED_SCRIPT refuses while the writer flag exists; a writer sets the flag then drains readers).
  • The per-session RWLock is writer-priority (src/api/rw-lock.ts), so a stream of GETs cannot starve an exec.
  • The read-only FS scope only guards the 11 mutating methods; stat/readFileBuffer/readdir/getAllPaths are untouched.
  • Zero torn reads is not owed to the exclusive lock: 25 sequential shared-path reads against a writer creating 300 files in one script-tx returned exactly 300 every time, never a partial count. Atomicity comes from RW-lock exclusion plus the script-tx, both of which the shared path keeps.

Caveat: ensureFreshCache runs outside the in-process lock on both paths. On the shared path, reader B can reload() while reader A is mid-stat, swapping the pathCache under A. It needs the Redis version GET to throw or the version to have moved, yields a consistent-but-newer snapshot rather than corruption, and already ships today on every MCP read tool and every readOnly exec. Switching two HTTP routes widens exposure rather than introducing it. Worth a follow-up (take the probe under the shared lock), not a blocker.

Fix

Change routes/files.ts:151 and :550 to withOwnedSessionRead, update the import. Identical signature and rehydrate fallback. Set expectations in the changeset: this buys read throughput, not freedom from writers.

Provenance

Found during pre-merge load testing of #162. Not introduced by that PR.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:distributedDistributed locking / caching / coherencebugSomething isn't workingseverity:mediumMedium severity

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions