Harden stage-2 enrichment: durable sidecar, enforceable timeout, tests

Address code-review findings on the metadata pipeline:

- finalize_known! now fsyncs the sidecar bytes before the rename and
  fsyncs done/ after, so the "file in done/ implies sidecar present"
  invariant holds across power loss, not just process crashes. The
  docstring previously claimed an fsync the code never performed.
- run_exiftool's timeout escalates SIGTERM -> (2s grace) -> SIGKILL, so
  an exiftool that ignores SIGTERM can't pin a worker forever on
  wait(proc). Previously the timeout sent only SIGTERM.
- Add test/ (48 tests) covering the correctness-critical paths:
  sanitize_filename, normalize_metadata, degraded build_metadata,
  real exiftool extraction, finalize_known! end-to-end, recover_dir!.

Project.toml

@@ -14,6 +14,13 @@ Oxygen = "df9a0d86-3283-4920-82dc-4555fc0d1d8b"
 UUIDs = "cf7118a7-6976-5b1a-9a39-7adc72f591a4"
 Zygote = "e88e6eb3-aa80-5325-afca-941959d7151f"
 
+[extras]
+JSON3 = "0f8b85d8-7281-11e9-16c2-39a750bddbf1"
+Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
+
+[targets]
+test = ["Test", "JSON3"]
+
 [compat]
 HTTP = "1.11.0"
 JLD2 = "0.6.4"

src/metadata.jl

@@ -34,6 +34,24 @@ const AUTHOR_TAGS      = ["Author", "Artist", "By-line", "Owner", "Artist"]
 const CREATED_DATE_TAGS  = ["DateTimeOriginal", "CreateDate", "MediaCreateDate", "CreationDate"]
 const MODIFIED_DATE_TAGS = ["ModifyDate", "FileModifyDate"]
 
+"fsync an open file descriptor, throwing on failure — used to make a write durable before a rename commits it."
+function fsync_fd(fd)
+    ccall(:fsync, Cint, (Cint,), fd) == 0 || error("fsync failed: $(Base.Libc.strerror())")
+    return nothing
+end
+
+"fsync a directory so a rename into it survives a crash (the rename, not just the file bytes, must be persisted)."
+function fsync_dir(dir::AbstractString)
+    dfd = ccall(:open, Cint, (Cstring, Cint), dir, 0)   # O_RDONLY
+    dfd < 0 && error("cannot open dir for fsync: $dir ($(Base.Libc.strerror()))")
+    try
+        fsync_fd(dfd)
+    finally
+        ccall(:close, Cint, (Cint,), dfd)
+    end
+    return nothing
+end
+
 "Strip exiftool's `-G` group prefix (`EXIF:Software` → `Software`) so lookups are group-agnostic."
 strip_group(tag::AbstractString) = String(last(split(tag, ':')))
 
@@ -72,7 +90,14 @@ function run_exiftool(path::AbstractString, timeout::Integer)
         end
         if process_running(proc)
             killed[] = true
-            kill(proc)
+            kill(proc, Base.SIGTERM)
+            # Escalate: a process that ignores/defers SIGTERM would otherwise pin
+            # the worker forever on the wait(proc) below, defeating the timeout.
+            grace = 0.0
+            while process_running(proc) && grace < 2.0
+                sleep(0.1); grace += 0.1
+            end
+            process_running(proc) && kill(proc, Base.SIGKILL)
         end
     end
     wait(proc)
@@ -154,10 +179,12 @@ end
 Commit an enriched known file to `done/` with the sidecar-first ordering so the
 invariant *"a file in done/ implies its sidecar is already there"* always holds.
 
-Sequence: write `<name>.meta.json` directly into `done/`, fsync-close it, THEN
+Sequence: write `<name>.meta.json` to a temp name, fsync its bytes, rename it
-move the file into `done/`. A crash between the two leaves only a harmless orphan
+into place, fsync `done/` so the rename itself is durable, THEN move the file
-sidecar in `done/` while the file stays in `known/`, so stage-aware recovery
+into `done/`. A crash between the two leaves only a harmless orphan sidecar in
-re-drives it and overwrites the sidecar — idempotent.
+`done/` while the file stays in `known/`, so stage-aware recovery re-drives it
+and overwrites the sidecar — idempotent. The fsyncs make the ordering hold
+across power loss, not just process crashes.
 """
 function finalize_known!(cfg::Config, job::Job, meta)
     base        = basename(job.path)
@@ -168,8 +195,11 @@ function finalize_known!(cfg::Config, job::Job, meta)
     # sidecar and a crash mid-write can't masquerade as a committed one.
     open(tmp_sidecar, "w") do io
         write(io, JSON3.write(meta))
+        flush(io)
+        fsync_fd(fd(io))                   # durably persist bytes before the rename
     end
     mv(tmp_sidecar, sidecar; force=true)   # sidecar committed first
+    fsync_dir(cfg.done_dir)                # persist the rename itself, not just the bytes
 
     file_dest = move_to(cfg.done_dir, job) # file arrival = commit point
     return (file_dest, sidecar)

test/runtests.jl

@@ -0,0 +1,166 @@
+using Test
+using FileServer
+using JSON3
+
+# Pull internals into scope. These aren't exported (only `run` is), but the
+# whole risk profile of this pipeline lives in these functions, so we test them
+# directly rather than only through the HTTP surface.
+using FileServer: Job, Config, ChannelQueue, enqueue!, dequeue!, length,
+                  sanitize_filename, recover_dir!, normalize_metadata,
+                  build_metadata, finalize_known!, run_exiftool
+
+# A minimal, valid 1×1 PNG. Lets the real-exiftool tests assert stable facts
+# (FileType == "PNG", 1×1 dimensions) that don't drift across exiftool versions.
+const PNG_1x1 = UInt8[137,80,78,71,13,10,26,10,0,0,0,13,73,72,68,82,0,0,0,1,0,
+    0,0,1,8,6,0,0,0,31,21,196,137,0,0,0,11,73,68,65,84,120,218,99,100,248,255,
+    191,30,0,5,132,2,127,194,91,30,42,0,0,0,0,73,69,78,68,174,66,96,130]
+
+"Build a Config whose data dirs all live under a fresh temp directory."
+function tmp_config(root; kwargs...)
+    cfg = Config(;
+        spool_dir   = joinpath(root, "spool"),
+        known_dir   = joinpath(root, "known"),
+        unknown_dir = joinpath(root, "unknown"),
+        done_dir    = joinpath(root, "done"),
+        failed_dir  = joinpath(root, "failed"),
+        kwargs...,
+    )
+    FileServer.ensure_dirs(cfg)
+    return cfg
+end
+
+@testset "FileServer" begin
+
+    @testset "sanitize_filename" begin
+        @test sanitize_filename("report.pdf") == "report.pdf"
+        # Directory components and traversal are stripped, not preserved.
+        @test sanitize_filename("../../etc/passwd") == "passwd"
+        @test sanitize_filename("/abs/path/x.txt") == "x.txt"
+        # Leading dots removed so "..", ".hidden" can't sneak through.
+        @test sanitize_filename("..") == "unnamed"
+        @test sanitize_filename(".hidden") == "hidden"
+        # Unsafe chars collapse to underscores; empty falls back to "unnamed".
+        @test sanitize_filename("a b&c*.d") == "a_b_c_.d"
+        @test sanitize_filename("") == "unnamed"
+        # Length is capped.
+        @test Base.length(sanitize_filename("a"^500)) == FileServer.MAX_NAME_LEN
+    end
+
+    @testset "normalize_metadata" begin
+        job = Job("id-1", "photo.jpg", "/data/known/id-1-photo.jpg", 4242, 0.0)
+        # Group-prefixed tags as exiftool -G emits them are already group-stripped
+        # by run_exiftool before reaching normalize_metadata, so keys are bare.
+        bytag = Dict{String,Any}(
+            "FileType"   => "JPEG",
+            "MIMEType"   => "image/jpeg",
+            "ImageWidth" => 800,
+            "ImageHeight"=> 600,
+            "Author"     => "Ada Lovelace",
+            "Creator"    => "Acrobat",        # feeds created_by, not author
+            "CreateDate" => "2020:01:02 03:04:05",
+            "ModifyDate" => "2020:01:02 03:04:06",
+            "PageCount"  => 12,
+        )
+        m = normalize_metadata(job, bytag)
+        @test m.file_type == "JPEG"
+        @test m.mime_type == "image/jpeg"
+        @test m.dimensions == (width = 800, height = 600)
+        @test m.author == "Ada Lovelace"
+        @test m.created_by == "Acrobat"
+        @test m.created_date == "2020:01:02 03:04:05"
+        @test m.page_count == 12
+        @test m.error === nothing
+        @test m.raw === bytag
+        # file_size is authoritative from the Job, never from exiftool.
+        @test m.file_size == 4242
+    end
+
+    @testset "normalize_metadata: missing tags degrade to nothing" begin
+        job = Job("id-2", "blob.bin", "/data/known/id-2-blob.bin", 7, 0.0)
+        m = normalize_metadata(job, Dict{String,Any}())
+        @test m.file_type === nothing
+        @test m.dimensions === nothing   # neither width nor height present
+        @test m.author === nothing
+        @test m.file_size == 7
+        @test m.error === nothing        # empty-but-present dict is still "success"
+    end
+
+    @testset "build_metadata: degraded on extraction failure" begin
+        mktempdir() do root
+            cfg = tmp_config(root; exiftool_timeout=5)
+            # Point at a nonexistent file → exiftool exits non-zero → degraded.
+            job = Job("id-3", "gone.dat", joinpath(cfg.known_dir, "id-3-gone.dat"), 99, 0.0)
+            m = build_metadata(job, cfg)
+            @test m.error !== nothing
+            @test m.file_type === nothing
+            @test m.raw === nothing
+            @test m.file_size == 99      # still authoritative from the Job
+            @test m.id == "id-3"
+        end
+    end
+
+    @testset "run_exiftool: real extraction on a PNG" begin
+        mktempdir() do root
+            p = joinpath(root, "pixel.png")
+            write(p, PNG_1x1)
+            bytag = run_exiftool(p, 30)
+            @test bytag !== nothing
+            @test bytag["FileType"] == "PNG"
+            @test bytag["ImageWidth"] == 1
+            @test bytag["ImageHeight"] == 1
+        end
+    end
+
+    @testset "finalize_known!: sidecar-first commit, end to end" begin
+        mktempdir() do root
+            cfg = tmp_config(root)
+            # A real known-stage file to enrich.
+            src = joinpath(cfg.known_dir, "id-9-pixel.png")
+            write(src, PNG_1x1)
+            job = Job("id-9", "pixel.png", src, Base.length(PNG_1x1), 0.0)
+
+            meta = build_metadata(job, cfg)
+            file_dest, sidecar = finalize_known!(cfg, job, meta)
+
+            # File moved into done/, original gone from known/.
+            @test isfile(file_dest)
+            @test dirname(file_dest) == cfg.done_dir
+            @test !isfile(src)
+
+            # Sidecar committed alongside it, valid JSON, no leftover .tmp.
+            @test isfile(sidecar)
+            @test endswith(sidecar, ".meta.json")
+            @test !isfile(string(sidecar, ".tmp"))
+            parsed = JSON3.read(read(sidecar, String))
+            @test parsed.file_type == "PNG"
+            @test parsed.file_size == Base.length(PNG_1x1)
+            @test parsed.error === nothing
+        end
+    end
+
+    @testset "recover_dir!: re-enqueues work, skips sidecars" begin
+        mktempdir() do root
+            dir = joinpath(root, "known"); mkpath(dir)
+            uuid = "0123456789abcdef0123456789abcdef0123"   # 36 chars
+            work = joinpath(dir, string(uuid, "-report.pdf"))
+            write(work, "x")
+            write(joinpath(dir, string(uuid, "-report.pdf.meta.json")), "{}")  # sidecar
+            write(joinpath(dir, "shortname"), "y")          # no uuid prefix
+
+            q = ChannelQueue(10)
+            n = recover_dir!(dir, q)
+            @test n == 2                       # the two real files, not the sidecar
+            @test length(q) == 2
+
+            jobs = [dequeue!(q), dequeue!(q)]   # sorted by filename on recovery
+            # "0123...-report.pdf" sorts before "shortname".
+            @test jobs[1].id == uuid
+            @test jobs[1].original_name == "report.pdf"
+            @test jobs[1].path == work
+            # File with no uuid prefix keeps its whole name; gets a minted id.
+            @test jobs[2].original_name == "shortname"
+            @test !isempty(jobs[2].id)
+        end
+    end
+
+end