Skip to content

A concurrent download() destroys what a live sync() is writing (deletes *and* overwrites) #12

Description

@vertix

download(delete=True) silently deletes files a concurrent sync() is still writing. A reader and a writer of the same remote path share one local mirror directory (the cache path is derived from the S3 URL), and download defaults to mirror semantics — "delete local files NOT in S3". Anything not yet uploaded is not in S3, so an in-progress write is exactly what gets deleted.

What it cost us

A robot run recording to s3://runway/inference/030826/rollouts/ via pos3.sync(output_dir, sync_on_error=True):

20:14:30  run starts, sync registers the output dir
20:15:35  episode 14 starts recording into <mirror>/000000000000/000000000014/
20:16:44  a separate analysis process calls
          pos3.download('s3://runway/inference/030826/rollouts/')
          -> deletes 000000000014/, which is mid-recording and not yet uploaded

The recorder then died flushing into a directory that no longer existed:

FileNotFoundError: [Errno 2] Failed to open local file
  '.../030826/rollouts/000000000000/000000000014/grip.parquet'

One episode of robot time lost, and the run had to be restarted. Nothing warned; from the reader's side this is a completely ordinary read.

Why the caller can't reasonably avoid it

  • The two are separate processes, so no in-process registry can see the conflict.
  • The reader has no way to know the path is live. Both sides just name an S3 URL; the shared local directory is an implementation detail of the cache layout.
  • The destructive behaviour is the defaultdelete: bool = True — so the dangerous call is the one you write without thinking about it. delete=False is correct here but you only learn that after losing data.

This isn't limited to ad-hoc scripts: any dataset viewer or analysis job pointed at a live run's output directory does the same thing.

Proposed fix: a liveness marker with a heartbeat

sync() marks the local mirror it owns; download() refuses to delete inside a directory that is actively owned.

# sync(), when it takes a local mirror for read-write
(local / '.pos3-live').write_text(json.dumps({
    'pid': os.getpid(), 'host': socket.gethostname(), 'ts': time.time()}))
# refreshed on each periodic upload; removed on clean exit

# download(), before deleting anything
if delete and (m := _fresh_marker(local)):
    raise RuntimeError(
        f'{local} is being written by a live run (pid {m["pid"]} on {m["host"]}); '
        f'pass delete=False to read it')

Details that matter:

  • Heartbeat, not a plain lock. A crashed run must not wedge the path forever. Refresh ts on each periodic upload and treat a marker older than ~2x the sync interval as stale and ignorable.
  • Raise, don't skip silently. The failure being prevented is silent data loss; a loud error naming the owning pid is more useful than quietly retaining files. The caller's fix is one keyword.
  • Scope the check to the subtree being deleted, so an unrelated sibling path is unaffected.
  • Marker excluded from upload so it never reaches S3.

Alternatives considered

  • Flip the default to delete=False. Safer, but it silently degrades mirror fidelity everywhere — a file dropped remotely would linger locally and readers would quietly see a stale set. Worse failure mode, just less visible.
  • Caller discipline (readers always pass delete=False). No code change, but it fails silently for whoever forgets, which is the situation that produced this report.

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

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions