Write the key to a scratch file in the same directory and hard-link it
onto the final path. The bytes are durable before any name points at
them, so a crash cannot leave a partial key that fails to load on the
next start and takes the node off the network until someone reads the
logs. A concurrent reader can no longer observe a half-written file
either, since the final name appears complete or not at all.
hard_link rather than rename: rename replaces its destination silently,
so refusing to clobber an existing key would depend on a check followed
by a separate rename, and a concurrent start can land in that gap.
hard_link is atomic and refuses an occupied path, including a symlink,
which it does not follow.
Create the key directory 0700 and tighten it when an existing one grants
group or other access. A 0600 key under a writable directory can still be
replaced or unlinked. Tightening rather than refusing to start, because
existing installs already have 0755 there and refusing would take p2p
down on all of them through a path that only warns.
Formatting on the branch is swept up here; it was already failing
cargo fmt --check before this change.
Open the key file with create_new and the mode set at creation, then
fsync, instead of writing it and narrowing the mode afterwards. The
secret is never on disk under a wider mode, an interrupted start cannot
leave it readable, and the exclusive open also refuses a pre-existing
entry at the path and makes a concurrent start take the key that landed
rather than clobber it.
Refuse to load a key file whose mode grants group or other access, and
name the observed mode so the operator can fix it. Report an empty key
file as empty rather than surfacing a protobuf decode error that blames
a missing rsa feature.
Pin GITLAWB_P2P_KEY onto the mounted volume in the Docker and fly
configs and document it, so the key does not depend on home-directory
resolution to land on persistent storage.
p2p::start now takes the Ed25519 keypair loaded by
load_or_create_p2p_keypair instead of computing one from the node DID,
so a node's network identity is generated once from the OS RNG and kept
on disk rather than recomputed from a public value on every start.
The node DID parameter is gone from start; the call site loads the key
first and continues without p2p if the key file cannot be read, matching
how a swarm-start failure is already handled.
The gossipsub message_id_fn is untouched and keeps its own hasher.
Add p2p_key_path (--p2p-key-path / GITLAWB_P2P_KEY, default
~/.gitlawb/p2p.key) with a resolver mirroring resolved_key_path, and
load_or_create_p2p_keypair, which generates an Ed25519 keypair on first
start, persists it 0600, and loads it thereafter. Mirrors the existing
load_or_create_keypair idiom for the node identity PEM.
A corrupt or unreadable key file is a hard error naming the path rather
than a silent regeneration, so a disk problem cannot quietly rotate the
node's network identity.
Not yet wired into p2p::start; that follows.
The domain separation owner_slug + ':' + repo_name has no length prefix, so
it is injective only while owner_slug cannot contain a colon. State that on
the pub(crate) function and guard it with a debug_assert, since a raw DID
would collide (('did:key:abc','x') and ('did','key:abc:x') hash the same).
Spelling the escape character as `ESCAPE '\'` puts a backslash inside an
ordinary SQL string literal. Under the legacy `standard_conforming_strings=off`
setting that backslash escapes the closing quote, so the literal runs on and the
statement fails to parse. The failure happens before the bound parameters are
considered, so every prefix lookup breaks, including prefixes that contain no
metacharacter at all, and short certificate IDs stop resolving entirely.
Connection settings come from outside the process, so that mode can arrive from
a database- or role-level setting.
Use `!` as the LIKE escape character instead, escaping `%`, `_`, and `!` in the
bound pattern. The statement then holds no backslash and parses the same either
way, without needing a second layer of backslash escaping. A backslash in the
prefix becomes an ordinary character, which is the literal match the search
wants anyway.
Cover it with a test that runs the production query through a second pool that
requests the legacy mode at connection startup, so a future return to a
parser-mode-dependent literal is caught rather than only wildcard handling under
the default configuration.
Refs #319
The deadline test hand-copied the advisory-lock key derivation, and that
copy drifted once the key moved to SHA-256, silently breaking the lock
collision the test is built around. Import the production function
instead so the key cannot diverge again.
Short-ID resolution passes a caller-provided prefix into a LIKE pattern. Any '%', '_' or '\' in that prefix is interpreted as a LIKE wildcard rather than a literal character, so lookups can match certificates outside the requested prefix.
Escape the three metacharacters in the prefix and add an explicit ESCAPE '\' clause so prefix matching is always literal.
Add a regression test covering literal '%', '_' and '\' prefixes.
Co-Authored-By: cairn-code <[email protected]>
Cross-model review round, findings corroborated by the in-process reliability
reviewer.
Both channel waits were unbounded. If the reader thread never reached its read,
or never returned after the peer close, the test wedged the binary instead of
failing, and cargo test has no per-test timeout to rescue it. Both are now
recv_timeout with bounds far above any real scheduling delay, so hitting one is
a genuine defect report rather than a flake, and the failure names which wait
stalled.
The handshake paragraph also overstated what the handshake buys. It listed "the
reader thread not yet scheduled to reach the read at all" among the states the
handshake removes, which reads as though the whole scheduling window is closed.
It is not: the handshake removes thread spawn from the window, and a reader
descheduled between its own Started send and its read is still a false-green
path. That path is now stated plainly instead of being left to the following
paragraph to correct.
Re-verified after the change: probe RED 50/50 under a deleted
set_nonblocking(false) at full-suite parallelism, RED on the named message under
a deleted set_read_timeout, 50/50 green unmutated, fmt and clippy clean.
Two findings from review of the probe test.
The helper has two postconditions and only one was guarded. Deleting
set_read_timeout left every test green while a stalled peer would park the
shim's only accept-loop thread forever, since handle_conn runs inline in that
loop. std exposes read_timeout() as a getter, so this half is checkable exactly
with no timing component. Confirmed load-bearing: deleting the line reds the
probe on the new message.
The probe also normalized one socket and then read from a try_clone of it, so
it only proved anything if blocking mode is shared across the two. On Linux it
is, because try_clone is dup(2) and O_NONBLOCK lives on the shared open file
description. On Windows try_clone goes through WSADuplicateSocketW and WSASocketW
(std os/windows/io/socket.rs), which mints a new socket descriptor, and whether
FIONBIO follows it is a question this test would have depended on without being
able to answer. It would have failed silently and green, on the one platform
where the underlying defect is real. The clone was unnecessary, so the reader
now owns the socket the helper normalized and the question does not arise.
Also correct two overstatements in the probe's own doc comment. It called
itself deterministic while its verdict is a 250ms window, and it described the
residual false-green path as "the few instructions between the signal and the
call" when the window is a full cross-thread round trip: signal, read, return,
channel send, and this thread waking. Both now say what the code does.
Re-verified after the change: probe RED 50/50 under a deleted
set_nonblocking(false) at full-suite parallelism, RED on the named message under
a deleted set_read_timeout, and 50/50 green unmutated. The end-to-end test came
in at 39/50 on this run against the first mutation, a third sample alongside 43
and 47, so its doc comment now reports the spread instead of two numbers.
The regression test added alongside the shim fix could pass with the fix
removed. It synchronized on a channel send that happens after accept() but
before handle_conn runs, so the request bytes could already be buffered by
the time the shim read them, and the read then succeeded on a non-blocking
socket. Measured at 43 and 47 of 50 full-suite runs catching a deleted
set_nonblocking(false), so it went false-green roughly one run in eight.
Extract the normalization into normalize_accepted_stream and assert its
postcondition directly, with no client and no request-arrival race. The new
probe puts a connected socket into non-blocking mode, calls the helper, and
checks that a reader parks instead of returning WouldBlock. Blocking mode has
no getter to query: std exposes none and Winsock has no FIONBIO getter at all,
so the property has to be observed through behavior.
The reader signals Started immediately before its read and the main thread
waits for that signal unbounded before opening the 250ms window. Without the
handshake the window would also be satisfied by a thread that has not been
scheduled yet, which would reintroduce the same silent hole one level down.
Peer close comes after the assertion, never before; closing first unblocks the
read and fails the test even when the helper is correct.
Also correct a platform claim this PR had in five places. The bug is not
Windows-only. An accepted socket inherits the listener's flag on BSD-derived
stacks, so macOS and the BSDs are exposed too; Linux is the exception because
std uses accept4 there, which does not inherit file status flags
(rust-lang/rust#67027). A macOS contributor could hit this today while the
comments said it was impossible. The macOS half is documented rather than
executed, and the comments now say so.
Verification, full-suite runs at default parallelism:
helper body mutated (set_nonblocking(false) deleted): probe RED 50/50
call site mutated (helper call deleted): probe GREEN 0/50,
integration test 45
and 46 of 50
unmutated: 50/50 green
The call-site row is the probe's blind spot, measured rather than assumed: it
calls the helper directly, so only the integration test notices a deleted
call, and it does so at a sampled rate rather than reliably. That residual is
recorded in the integration test's doc comment.
The test reproduces acquire_write's advisory-lock key derivation locally so
a held session lock can stall the handler. repo_store's advisory_lock_key
now uses SHA-256, so the DefaultHasher copy no longer collided and the
stall never happened. Update the local copy to the SHA-256 derivation.
Resolve conflicts: keep the SHA-256 advisory-lock key change alongside
upstream's connection-affine RepoWriteGuard rework and its F1/F3b/F4/U8
test suite; RUN-A-NODE.md keeps both the updated Public URL row and the
rolling-upgrade advisory-lock row.
start_shim sets the listener non-blocking so its accept loop can poll the
stop flag. On unix accept() hands back a blocking socket, so that never
showed; on Windows the accepted socket inherits the listener's FIONBIO
state and std does not reset it, so every connection arrived non-blocking.
handle_conn reads with read_line(..).unwrap_or(0), which turns the
resulting WouldBlock into "peer closed" and returns without writing a
response, dropping the connection and aborting the client mid-request. The
shim accepts as soon as the handshake completes, strictly before the
request bytes arrive, so the window was open on every connection and
closed only because the 5ms accept poll usually lagged the client. That is
what made real_git_withheld_shaped_first_post fail on windows-latest with
os error 10053 and zero POSTs recorded.
Normalizing at handle_conn entry also restores the 30s read timeout on the
next line, which a non-blocking socket silently made meaningless.
The regression test forces the Windows socket shape explicitly, so it is
load-bearing on every platform rather than only the one that exhibits the
bug, and it establishes the accept-before-request interleaving with a
handshake rather than a sleep: a timing-based version cannot flake red but
can flake green under load, which would make the guard vacuous.
Rebased onto current main. Keep budget-timeout wrappers on get_by_cid;
convert sqlx failures with Into<AppError> (not AppError::Internal) so
pool closed/timeouts become 503 db_unavailable. Same for list_pins and
arweave anchors, with closed-pool regressions.
Add a regression test for verify_strict: a signature forged under a weak (small-order) public key satisfies the verification equation but must be rejected. The non-strict verify path accepts it.
Co-Authored-By: cairn-code <[email protected]>
Rebased onto current main. Keep Overloaded/UnresolvableDid arms; make
Internal and non-unavailable Db responses opaque with server-side logs,
and pin exact JSON in unit + anonymous route tests.
Resolves jatmn's #192 P2. run_bounded's clean-exit teardown only terminated
the job when one had been established; when assign_job returned None it did
nothing, so a git-remote-gitlawb helper descendant that outlived the `git
fetch` leader while holding the inherited stdout/stderr pipe write-ends stalled
the reader joins on the helper's ~300s HTTP timeout, past the bound run_bounded
promises. The timeout path's reap_tree (taskkill /T) fallback cannot rescue the
clean path: once the leader has cleanly exited it no longer roots the
descendant in the parent-PID tree, so taskkill /T cannot reach the orphan.
The Job Object is the only reliable Windows teardown, so make it mandatory:
assign_job now returns JobHandle and panics per-step if the job cannot be
established, and both teardown paths unconditionally terminate it. Nested
unnamed jobs with KILL_ON_JOB_CLOSE are standard on the shipped
x86_64-pc-windows-msvc / CI runners, so this costs no coverage; it converts a
silent ~300s hang into a loud, actionable failure and strengthens the bound
(KILL_ON_JOB_CLOSE kills the descendant at latest when JobHandle drops).
reap_tree, whose only Windows use was the removed None arm, is narrowed to
cfg(all(not(unix), not(windows))).
The change is entirely cfg(windows). Verified by cross-compiling the tests for
x86_64-pc-windows-gnu (compiles clean, clippy -D warnings clean, no dead_code
from the reap_tree narrowing) and by the unchanged Linux git-remote-gitlawb
suite (green). The Windows runtime teardown is reasoned-not-run here; CI's
Windows job is the runtime gate.
run_bounded's clean-exit descendant reap was Unix-only: on Windows a
git fetch leader that exits cleanly leaves the git-remote-gitlawb helper
blocked in its ~300s HTTP request holding the inherited pipes, and
taskkill /T can't reach a descendant once the leader is gone, so the
reader joins blew past the promised bound. The child is now assigned to
a Job Object at spawn (the analog of the Unix process_group(0)), and the
job is terminated on both the timeout and clean-exit paths, which kills
the whole tree regardless of leader liveness. KILL_ON_JOB_CLOSE is the
safety net. The clean-exit regression is now cross-platform so it
exercises this on a future Windows CI lane (#228).
Unix tests unchanged and green; the Windows path is compile-verified for
x86_64-pc-windows-msvc (runtime coverage lands with #228's CI lane).
The test accepted any nonzero git exit (with posts>=1) as the recorded #191
withheld rejection, so a truncated response or a later HTTP failure after the
first POST could pass green. Assert the specific `expected ACK/NAK` signature so
only that break passes, and pin LC_ALL=C so the diagnostic stays the untranslated
English string on a git build with l10n installed. (#192 F6)
The gitlawb_fetches_total gate keyed on whether the request carried `done`,
which both undercounts a no-done completion (a client can finish a flush-
terminated have round with `ACK <oid> ready` + pack and no `done` -> counted 0)
and, on the filtered path, counts a rejected response per POST (the pre-#191
two-POST negotiation the client refuses was counted twice).
Derive completion from the response instead. upload_pack / upload_pack_excluding
now return whether they served a pack; response_served_pack detects the PACK
magic in both side-band-64k and raw framing. The plain path counts when the
response delivered a pack (catches no-done, skips negotiation-only rounds so an
N-round fetch counts once); the filtered path stays gated on the finalizing
`done` round until #191 makes that negotiation valid. Also tighten
request_finalizes_fetch to reject 0003 framing and trailing bytes after the
`done` pkt-line. (#192 F1/F2 + CodeRabbit repos.rs:761)
init() did an unsynchronized REGISTRY.get() check then OnceLock::set(...).
expect(...) for each metric, so two concurrent callers both passed the check
and the loser panicked at 'set INFO once'. Two tests call init(), so the
parallel test runner could hit this. Guard the body with std::sync::Once so it
runs exactly once. Verified: a 32-thread barrier stress-test panics before the
fix and passes after (run in isolation; not committed since the process-global
registry makes the race non-deterministic in the shared-binary suite). (#192)
A non-UTF-8 or non-hex 4-byte length prefix was collapsed to 0000 via
unwrap_or, so a garbled header was handled exactly like a flush -- masking
corrupted/desynchronized protocol input and letting accumulated have lines
go out as a synthesized flush round. Propagate the parse error instead;
a real 0000 still parses as a valid flush. (#192)
run_bounded only tore down the process group on the deadline path. When the git
leader exits on its own but a descendant (the git-remote-gitlawb helper mid-HTTP
request) outlives it holding the inherited stdout/stderr write-ends, the reader
joins blocked on the helper's ~300s timeout, past the deadline run_bounded
promises. Close the group on the clean-exit path too, guarded by a kill(-pgid, 0)
liveness probe so a recycled pid is never signalled (the process-group ID stays
reserved while any member is alive). Adds a leader-exits-first fixture and a
regression test.
Verified by execution: with the clean-path reap removed the new test blocks ~10s
on the descendant (RED); with the reap it returns promptly.
git_upload_pack recorded a fetch and observed a pack size on every successful
POST. In HTTP stateless-RPC the plain serve path streams a pack only on the
finalizing `done` round, so an N-round incremental fetch was reported as N
completed fetches. The filtered path (upload_pack_excluding) instead serves a
self-contained full pack on the single POST that reaches it, with no `done`, so
gating purely on `done` would drop those private (path-scoped) fetches to zero.
Count once on the POST that actually completes a fetch: the `done` round on the
plain path (request_finalizes_fetch parses pkt-lines and fails closed on a
malformed body), or the one filtered-serve POST (served_filtered_pack).
should_count_fetch captures the decision; a completed fetch is exactly one such
POST, so a multi-round negotiation is never double-counted.
The observe_pack_size call still measures the request body, not the served pack;
that mislabel predates this change and is left as a follow-up.
Verified by execution: a three-round plain fetch and a no-`done` filtered serve
each record exactly one completion; reverting the gate to count per POST makes
the plain fetch record 3, and breaking the decision to AND drops both to 0.
The large-output and descendants tests re-invoke the test binary as their
fixtures instead of spawning sh/seq, so they run on x86_64-pc-windows-msvc;
the non-unix timeout path tears down the whole fetch tree with taskkill /T
before joining the drainers instead of killing only the leader. Also folds
the receive-pack POST block into post_pack_round (review nitpick), removing
the duplicated status-check/forward logic.
Address jatmn's two P2 review findings on real_git_fetch.rs.
F1 (INV-22): run_bounded killed only the top-level `git fetch` on the
timeout path, so the git-remote-gitlawb helper descendant kept the
inherited stdout/stderr pipe write-ends open and the reader joins blocked
on the helper's ~300s HTTP timeout instead of the 30s deadline. Spawn the
child with process_group(0) and, on timeout, tear the whole group down
(SIGTERM, ESRCH poll, SIGKILL escalation, bounded cap, then reap) before
joining, mirroring gitlawb-node/src/git/smart_http.rs. libc is added as a
cfg(unix) dev-dep; non-unix keeps the child.kill() fallback so the
always-on test still compiles. New regression run_bounded_reaps_descendants_holding_the_pipe:
a backgrounded sleep inherits the pipe and outlives a 1s-deadline leader;
reverting to a leader-only kill turns it RED (elapsed ~10s).
F2 (INV-21): the non-success branch of real_git_withheld_shaped_first_post
accepted any `git fetch` failure and only printed the POST count, so a
failure before the first POST (broken advertisement, helper lookup,
connection) passed as the expected withheld rejection. Assert posts >= 1
before accepting the nonzero exit. Forcing a pre-POST failure (0 POSTs)
turns the assertion RED.
F1: a nonempty have-batch terminated by EOF means git aborted mid-negotiation, but
the fetch loop folded RoundEnd::Eof into the Done arm, appended a synthetic
0009done, and POSTed it — so a cancelled private fetch still issued an authorized,
signed upload-pack request and made the node generate and stream a full pack to a
client that had gone away. Split the arm: only a real done pkt finalizes; a
nonempty EOF terminates without a POST. The nonempty_eof_batch_posts_as_final_round
test that locked in the wrong behavior is flipped to assert no POST.
F2: fetch_with_helper piped stdout+stderr but only polled try_wait, so a fetch
emitting more than an OS pipe buffer (~64 KiB) blocked git before it exited and the
harness reported a false 30s deadlock. Drain both streams on reader threads while
enforcing the deadline, so the deadline measures a real protocol hang only.
F3: the integration repos were removed only on the success path (predictable
pid/counter names), so a timeout/panic leaked real git repos and could poison a
later run. Root them in a RAII tempfile::TempDir that drops on unwind.
RED->GREEN: nonempty_eof_batch_aborts_without_posting (pre-fix POSTs a synthetic
done → the aborted-negotiation POST fails against the single-response server; fixed
makes one request, no POST). repos_are_cleaned_up_on_unwind (temp dir gone after a
panic unwinds past the guard). Full git-remote-gitlawb suite 42 unit + 3 integration
green, fmt + clippy clean.
Adds executed coverage for the negotiation branches the first pass left
unrun: an invalid pkt-line length (rejected, not underflowed), a non-EOF
read error (propagated, not swallowed as end-of-stream), an immediately
empty upload-pack request (skips the POST), a nonempty have-batch
terminated by EOF (POSTed as a final done round), and an empty
receive-pack body (skips the POST). Each was mutation-checked to fail
without its guard, so the coverage is load-bearing rather than vacuous.
A code-review pass found the negotiation loop would POST once per bare `0000`
flush that carried no haves, so a malformed `wants + N*0000 + EOF` stream
amplified into N signed POSTs. Real git never sends a content-free mid-negotiation
flush and the peer is upstream of the local git process, so this was not
reachable in practice, but it left the loop unbounded per input. Skip a round
that carries no new haves (an empty flush) and only POST rounds with have content
or a terminator, which also makes the loop provably bounded by git's finite have
set. The fresh-clone done-with-no-haves round still POSTs.
A committed integration test drives a real `git fetch` through the built helper
against a real `git upload-pack --stateless-rpc`, so the stateful-to-stateless
ACK bridging is proven by execution rather than reasoned. The main.rs mock tests
cannot falsify it: a pre-scripted Cursor never reacts to the server's ACKs. An
in-test shim replicates the node's v0 serving; the fetch is forced to at least
two negotiation rounds (a fixture that resolved in one round fails the test) and
the resulting object graph is verified.
The second scenario drives the withheld-blob shape (a full pack on the first
POST, as upload_pack_excluding does) through real git and records the outcome:
real git rejects a pack where it expected an ACK continuation ("expected
ACK/NAK, got ..."), so a multi-round fetch of a withheld repo does not complete.
The helper forwards and terminates cleanly rather than hanging, so this is a
node-side concern, not a helper defect, left as a follow-up per the plan's
Withheld-Path Decision. The test guards the helper's non-hang behavior and
surfaces the break if the assumption ever changes.
git-remote-gitlawb advertised `connect`, so git spoke the stateful native
protocol, but handle_connect collapsed the exchange into one GET plus one POST.
A multi-round fetch (more than ~32 overlapping commits) deadlocked: git sent a
flush-terminated have-batch and blocked for an ACK/NAK the helper never sent,
while the helper blocked reading for a `done` git never sent.
Turn Phase 2 into a per-round stateless-RPC client loop. read_upload_pack_round
returns one round at a time (at a flush, done, or EOF) instead of buffering past
flushes waiting for `done`; negotiate_upload_pack captures the wants once and
POSTs a self-contained request per flush-terminated batch: wants, one flush,
every have accumulated so far, and exactly one terminator. Each ACK/NAK is
streamed back to git so it advances, until the done round returns the pack. The
node's `git upload-pack --stateless-rpc` keeps no state between POSTs, so wants
and all prior haves are re-sent every round and no intermediate flush survives
into a body (which would truncate the negotiation server-side).
Signing is preserved per round: every POST carries the Phase-1 decision (signed
after the 404 escalation for a private repo, anonymous for a public one), and a
mid-negotiation denial surfaces through the sanitized error path rather than
reading as an empty or successful fetch. The receive-pack path is unchanged.
Covers the deadlock repro, multi-round accumulation on the wire, per-round
signing for private and public fetches, mid-negotiation denial surfacing, the
withheld-shaped forwarding path, and single-round non-regression.
beardthelion