test(mount): pin the .relay/state.json writer contract — DO NOT MERGE, branch needs re-cut (23 files, not 2) - #413
test(mount): pin the .relay/state.json writer contract — DO NOT MERGE, branch needs re-cut (23 files, not 2)#413khaliqgant wants to merge 3 commits into
Conversation
…surement The public "sub-200ms end-to-end including measurement overhead that exceeds the signal" was never a one-way measurement. It came from the 2026-07-26 evidence, which measured a round trip (median 315.5 ms, n=12, 5 ms polling echo) and halved it. This replaces the inference with a direct one-way measurement, sf-mini as receiver. Both halves of the old claim are wrong, in opposite directions. "sub-200ms" is false for realistic change sets. A single small file propagates in a median of 20.2 ms (p95 161.7 ms, n=20), but a repo-sized change set of 11 files / ~14 KB takes a median of 216.7 ms (p95 303.9 ms, n=20). The receive path fetches each file with its own server round trip (syncer.go:3310,3320,3339), so cost scales with file count, not bytes. Agents commit change sets; small single files were never the case that mattered. "measurement overhead that exceeds the signal" is false by more than an order of magnitude and is retired. The watcher's own detection delay was measured against a local control, not assumed: median 1.225 ms, max 2.455 ms over 25 pairs. Every figure holds only under the stated topology precondition, which is repeated beside each median in RESULTS.md: the server ran on the sender's own machine, so sender->server was loopback and the only network hop was server->receiver over a Tailscale LAN. These are LAN best cases and do not license any claim about the hosted product, faster or slower. RESULTS.md states the three prerequisites a product claim would need. No sub-100ms claim is made. Two things the method caught that a simpler one would have missed: - The hosts' clock offset drifted 8.323 ms across a ~21 minute run (-6.441 ms to -14.765 ms), comparable to the small-file signal itself. The offset is therefore interpolated to each trial's own send time rather than pinned. A single offset measurement would have looked reasonable and been quietly wrong. - sf-mini's status/live flipped online<->offline four times per gate window while its heartbeat advanced monotonically, and an MCP query_nodes call reported it flatly offline with a 38 s old, advancing heartbeat. Liveness is gated on heartbeat advance across >=90 s, before and after the trials. Trusting status would have abandoned the run against a healthy host. 52 trials, all HTTP 202, zero lost files, zero partial change sets, no sample near the ~30 s websocket-off fallback. The headline batches are the clean 20+20; an earlier 12-trial batch cut short by an interrupt is preserved as correctness evidence only and contributes to no percentile. 26/26 named assertions pass via harness/assertions.py. Isolation: fresh server on a separate port with a separate state dir, fresh workspace, distinct mount path. The pre-existing .dev-collab-stack/ and .salvaged-from-minis/ trees, their processes and ports, were not touched or reused. Teardown in CLEANUP.md. Test credentials were ephemeral, kept outside the repo, and are absent from every artifact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
<localDir>/.relay/state.json has two independent writers in the same binary, emitting two disjoint schemas to one path: - internal/mountsync/syncer.go:1608 derives the path; syncer.go:7200 writes a `publicState` (localRoot, states, files, counters, circuit, outbox, credExpiresInSecs). - cmd/relayfile-cli/main.go:10998 writes a `syncStateFile` (providers, daemon, guards, stallReason, incrementalReadNotReadySince). Both are handed the same directory by one closure at main.go:6852-6892, which passes scope.LocalDir as NewSyncer's LocalRoot and as runMountLoopWithAuthLock's localDir. Whichever writes last defines the document every consumer sees; live, they alternate about once a second. The test fails on this commit. It is red, not a regression guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Separates two claims about the failedWritebacks counter that are easy to conflate: - It is NOT zeroed by the schema alternation. Both writers read it back before writing (internal/mountsync/syncer.go:7020, and the max at cmd/relayfile-cli/main.go:10986). TestFailedWritebacksSurvivesAlternation passes and locks that hardening in. - It IS lost under concurrency. Neither writer holds a lock the other respects: failedWritebacksStateMu lives in cmd/relayfile-cli and internal/mountsync cannot take it. An increment landing between savePublicState's read (syncer.go:7020) and its write (syncer.go:7200) is overwritten with the stale value. TestFailedWritebacksLostUpdateUnderConcurrency loses 1-5 of 200 increments in 4 of 5 runs. It is a race demonstration, so it is inherently non-deterministic; it passed once in five runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request adds regression tests for shared relay state writers and a direct mount-latency measurement package. It includes executable harnesses, validation logic, raw JSONL evidence, cleanup procedures, and corrected latency documentation. ChangesRelay state writer regression tests
Mount latency measurement evidence
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Sender
participant Relay
participant Receiver
participant Analyzer
Sender->>Relay: Submit authenticated file or bulk write
Relay->>Receiver: Propagate files through websocket delivery
Receiver->>Analyzer: Record observed file arrivals
Analyzer->>Analyzer: Apply clock correction and calculate latency
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e6ada3164
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for _, field := range []string{"localRoot", "states", "counters", "files", "outbox", "credExpiresInSecs"} { | ||
| if _, ok := afterWriter2[field]; !ok { | ||
| t.Errorf("writer 2 clobbered writer 1: field %q is absent from .relay/state.json after the CLI mirror write (keys now: %v)", |
There was a problem hiding this comment.
Keep the default test suite from failing unconditionally
This assertion is guaranteed to fail with the implementation under test: writeMirrorStateFile serializes syncStateFile, which has neither localRoot nor states, so at least those iterations always call t.Errorf. Because .github/workflows/ci.yml runs go test ./..., the commit cannot produce a green test run even when the existing behavior is unchanged; quarantine this red contract test or include the production fix that makes the contract pass.
Useful? React with 👍 / 👎.
| case <-stop: | ||
| return | ||
| default: | ||
| _ = syncer.FlushOutboxOnce(t.Context()) |
There was a problem hiding this comment.
Synchronize the lost-update interleaving
The background flush is not synchronized with any point in the increment loop, so this test depends entirely on scheduler timing: if the goroutine only flushes before the increments or the increment loop completes after its final write, the counter remains 200 and the test passes; if a stale flush lands during the loop, it fails. Since this runs in the default Go suite, the same code can therefore produce intermittent CI outcomes; use barriers or a controllable writer hook to force the read-before-increment/write-after-increment sequence.
Useful? React with 👍 / 👎.
| disturbed = [line for line in status if not line.startswith("??")] | ||
| check( | ||
| "preexisting_untracked_dirs_untouched", | ||
| not disturbed, |
There was a problem hiding this comment.
Verify untracked directory contents instead of Git status
When these pre-existing directories are untracked, git status --porcelain cannot distinguish untouched contents from modified or replaced contents—it reports the same ?? <dir>/ entry—and deleting a directory produces no entry, which also passes this check. Consequently the named isolation assertion can report that the directories were untouched even when the experiment changed or removed them, undermining the committed evidence claim; compare a pre-run snapshot or hashes instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/relayfile-cli/relay_state_counter_test.go`:
- Around line 63-83: Register test cleanup immediately after starting the worker
goroutine, before the increment loop in the relay state counter test, so a
t.Fatalf from incrementFailedWritebacksInState still signals stop and waits for
the worker to exit. Reuse the existing stop channel and sync.WaitGroup cleanup
behavior, while preserving the normal close-and-wait path without duplicating
cleanup.
- Around line 3-6: Update TestFailedWritebacksLostUpdateUnderConcurrency to
create a Go 1.22-compatible test-owned context and pass it to FlushOutboxOnce,
and require an explicit opt-in before running this nondeterministic concurrency
probe. Register cleanup immediately to cancel the context, close stop, and wait
for the worker, ensuring cleanup still runs when t.Fatalf exits early.
- Around line 42-53: Update TestFailedWritebacksLostUpdateUnderConcurrency to
return immediately unless its opt-in environment variable is enabled, before
starting the worker; replace t.Context() with a Go 1.22-compatible context and
ensure worker shutdown is deferred before any t.Fatalf path can exit the test.
In `@docs/evidence/mount-latency-20260807/CLEANUP.md`:
- Around line 33-47: Update the cleanup commands in the measurement-run
documentation to avoid broad pkill patterns: record each process ID when
starting the receiver watcher, clock-offset server, relayfile server, and
dev-auth daemon, then terminate those specific IDs, or constrain matching to the
run-specific latency state directory. Preserve cleanup of only this run’s
scratch resources.
In `@docs/evidence/mount-latency-20260807/harness/analyse.py`:
- Around line 53-72: Update load_arrivals to retain each path’s observed size
alongside its earliest timestamp, and update the completion check to compare
those sizes against expected per-path sizes. Modify sender-trials.py to emit the
expected size for every path, and reject trials when any observed size is
missing or differs.
In `@docs/evidence/mount-latency-20260807/harness/assertions.py`:
- Around line 114-115: Update
docs/evidence/mount-latency-20260807/harness/assertions.py lines 114-115 to
validate published liveness sample counts and final timestamp bounds against the
raw evidence, or derive those result values from validated output; update
docs/evidence/mount-latency-20260807/RESULTS.md lines 167-170 to report 14
post-trial samples ending at the actual 11:31:47Z timestamp.
- Around line 41-45: Update the window assertion near the existing
`${label}_window_at_least_90s` check to parse each sample’s `sampledAtLocalUtc`,
calculate the duration between the first and last timestamps, and require that
duration to be at least 90 seconds. Retain the existing sample-count check only
if still needed separately, and report the measured duration in the assertion
message.
In `@docs/evidence/mount-latency-20260807/harness/receiver-watch.py`:
- Around line 121-123: Update the scan_ms_p95 calculation in the receiver
statistics output to use the same linear-interpolation percentile logic as
analyse.py, rather than selecting index int(len(loop_periods) * 0.95), which
returns the maximum for 20 scans. Preserve the existing empty-loop None
behavior.
In `@docs/evidence/mount-latency-20260807/METHODOLOGY.md`:
- Around line 39-41: Remove identifying hostnames, personal names, and Tailnet
addresses from the evidence: in
docs/evidence/mount-latency-20260807/METHODOLOGY.md lines 39-41, use only sender
and receiver role labels; in
docs/evidence/mount-latency-20260807/raw/mount-watch.jsonl lines 1-1, replace
the absolute watch path with a repository-safe placeholder; and in
docs/evidence/mount-latency-20260807/raw/trials-repo.jsonl lines 1-20 and
docs/evidence/mount-latency-20260807/raw/trials-small.jsonl lines 1-32, replace
sender host values with the same neutral role label.
In `@docs/evidence/mount-latency-20260807/raw/control-watch.jsonl`:
- Line 1: Replace the user-specific absolute value in the watcher_started
record’s watch_dir field with a stable logical directory name, removing the
local account name while preserving valid JSONL structure and the
analyser-required directory identity.
In `@docs/evidence/mount-latency-20260807/raw/heartbeat-gate-pre.jsonl`:
- Around line 1-14: Update every sampledAtLocalUtc value in the heartbeat
evidence records to use the producer’s valid UTC format, replacing the invalid
NZ suffix with +00:00 or Z. Preserve the existing timestamps and record contents
while ensuring standard RFC 3339/ISO 8601 parsers can read the sampling
chronology.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 98b48b27-227e-4209-8620-655a37c44562
📒 Files selected for processing (23)
cmd/relayfile-cli/relay_state_counter_test.gocmd/relayfile-cli/relay_state_two_writers_test.godocs/evidence/mount-latency-20260807/CLEANUP.mddocs/evidence/mount-latency-20260807/METHODOLOGY.mddocs/evidence/mount-latency-20260807/RESULTS.mddocs/evidence/mount-latency-20260807/harness/analyse.pydocs/evidence/mount-latency-20260807/harness/assertions.pydocs/evidence/mount-latency-20260807/harness/clock-offset.pydocs/evidence/mount-latency-20260807/harness/control-local.pydocs/evidence/mount-latency-20260807/harness/dev-authd.pydocs/evidence/mount-latency-20260807/harness/receiver-watch.pydocs/evidence/mount-latency-20260807/harness/sender-trials.pydocs/evidence/mount-latency-20260807/raw/clock-offset-post.jsonldocs/evidence/mount-latency-20260807/raw/clock-offset-pre.jsonldocs/evidence/mount-latency-20260807/raw/control-create.jsonldocs/evidence/mount-latency-20260807/raw/control-watch.jsonldocs/evidence/mount-latency-20260807/raw/heartbeat-gate-post.jsonldocs/evidence/mount-latency-20260807/raw/heartbeat-gate-pre.jsonldocs/evidence/mount-latency-20260807/raw/heartbeat-gate-pre.shdocs/evidence/mount-latency-20260807/raw/mount-watch.jsonldocs/evidence/mount-latency-20260807/raw/trials-repo.jsonldocs/evidence/mount-latency-20260807/raw/trials-small.jsonldocs/multi-agent-collaboration-assessment.md
| import ( | ||
| "sync" | ||
| "testing" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '^\s*go\s+[0-9.]+' go.mod
rg -n -C2 't\.Context\(' cmd/relayfile-cli/relay_state_counter_test.goRepository: AgentWorkforce/relayfile
Length of output: 263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test file outline ---'
ast-grep outline cmd/relayfile-cli/relay_state_counter_test.go
printf '%s\n' '--- test file ---'
cat -n cmd/relayfile-cli/relay_state_counter_test.go
printf '%s\n' '--- module and workspace settings ---'
cat -n go.mod
if [ -f go.work ]; then cat -n go.work; fi
printf '%s\n' '--- related test and API usage ---'
rg -n -C3 'TestFailedWritebacks|FlushOutboxOnce|t\.Context\(' --glob '*.go' .Repository: AgentWorkforce/relayfile
Length of output: 24599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
p = Path("cmd/relayfile-cli/relay_state_counter_test.go")
text = p.read_text()
print("t.Context call count:", len(re.findall(r'\bt\.Context\(\)', text)))
print("context import present:", bool(re.search(r'(?m)^\s*"context"\s*$', text)))
print("goroutine launches:", len(re.findall(r'\bgo\s+func\s*\(', text)))
print("explicit cancel calls:", len(re.findall(r'\bcancel\s*\(\)', text)))
PYRepository: AgentWorkforce/relayfile
Length of output: 263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- helper and writer definitions ---'
rg -n -C5 'func (incrementFailedWritebacksInState|readPersistedFailedWritebacks|newPublicStateWriter)|func \(s \*Syncer\) FlushOutboxOnce|func .*savePublicState' \
cmd/relayfile-cli internal/mountsync --glob '*.go'
printf '%s\n' '--- test gating and cleanup patterns ---'
rg -n -C3 'TestFailedWritebacksLostUpdateUnderConcurrency|testing.Short|RUN.*Test|Skip\(' \
cmd/relayfile-cli .github Makefile* --glob '*.go' --glob '*.yml' --glob '*.yaml' --glob 'Makefile*' 2>/dev/null || trueRepository: AgentWorkforce/relayfile
Length of output: 5773
Use a Go 1.22-compatible context and isolate the concurrency probe.
testing.T.Context()is unavailable withgo 1.22. Create a test-owned context and pass it toFlushOutboxOnce.- Gate
TestFailedWritebacksLostUpdateUnderConcurrencybehind explicit opt-in because its unsynchronized timing can produce nondeterministic results. - Ensure cleanup cancels the context, closes
stop, and waits for the worker.t.Fatalfat line 78 exits before the current shutdown code runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/relayfile-cli/relay_state_counter_test.go` around lines 3 - 6, Update
TestFailedWritebacksLostUpdateUnderConcurrency to create a Go 1.22-compatible
test-owned context and pass it to FlushOutboxOnce, and require an explicit
opt-in before running this nondeterministic concurrency probe. Register cleanup
immediately to cancel the context, close stop, and wait for the worker, ensuring
cleanup still runs when t.Fatalf exits early.
Source: Linters/SAST tools
| // TestFailedWritebacksLostUpdateUnderConcurrency probes the real hazard: both | ||
| // writers preserve the counter via read-modify-write, but neither holds a lock | ||
| // the other respects, so an increment landing between mountsync's read | ||
| // (syncer.go:7020) and its write (syncer.go:7200) is silently overwritten with | ||
| // the stale value. | ||
| // | ||
| // failedWritebacksStateMu (cmd/relayfile-cli/main.go) serializes the CLI-side | ||
| // increments against each other, but mountsync lives in another package and | ||
| // cannot take it. The mutex looks synchronized and is not. | ||
| // | ||
| // N increments must yield a counter of N. | ||
| func TestFailedWritebacksLostUpdateUnderConcurrency(t *testing.T) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -i -C2 'go\s+test|relay_state_counter|RELAYFILE_RUN_FLAKY_STATE_RACE' .github Makefile 2>/dev/null || trueRepository: AgentWorkforce/relayfile
Length of output: 1229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- module Go version ---'
sed -n '1,40p' go.mod
printf '%s\n' '--- test file outline ---'
ast-grep outline cmd/relayfile-cli/relay_state_counter_test.go 2>/dev/null || true
printf '%s\n' '--- relevant test source ---'
cat -n cmd/relayfile-cli/relay_state_counter_test.go | sed -n '1,180p'
printf '%s\n' '--- test and CI references ---'
rg -n -i -C3 'go test|relay_state_counter|TestFailedWritebacksLostUpdateUnderConcurrency|RELAYFILE_RUN_FLAKY_STATE_RACE|t\.Context\(\)' \
--glob '!vendor/**' --glob '!node_modules/**' .
printf '%s\n' '--- Go tool version, if available ---'
go version 2>/dev/null || trueRepository: AgentWorkforce/relayfile
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant package test declarations and imports ---'
rg -n -C8 '^func Test|^import \(' cmd/relayfile-cli/relay_state_counter_test.go
printf '%s\n' '--- worker lifecycle and fatal paths ---'
rg -n -C10 'WaitGroup|go func|Fatalf|Errorf|context\.|cancel|Done\(' cmd/relayfile-cli/relay_state_counter_test.go
printf '%s\n' '--- repository test targets and workflow commands ---'
rg -n -i -C5 'go test|test:' Makefile .github --glob '*.yml' --glob '*.yaml' 2>/dev/null || trueRepository: AgentWorkforce/relayfile
Length of output: 7412
🌐 Web query:
Official Go documentation: which Go release added testing.T.Context, and what is the Go 1.22 testing.T API?
💡 Result:
The testing.T.Context method was added in Go 1.24.0 [1][2][3]. Regarding the Go 1.22 testing.T API, it did not include the Context method. The testing.T API in Go 1.22 consisted of the following primary methods: ArtifactDir (added in later versions) Cleanup Error Errorf Fail FailNow Failed Fatal Fatalf Helper Log Logf Name Parallel Run Setenv Skip SkipNow Skipf Skipped TempDir Deadline Please note that the exact composition of the API can vary based on the minor release (such as the addition of Chdir or Setenv in earlier versions), but Context was definitively not part of the standard library testing package until the Go 1.24 release [1][2].
Citations:
- 1: https://pkg.go.dev/testing@latest
- 2: https://pkg.go.dev/testing@go1.26.5
- 3: https://boldlygo.tech/archive/2025-04-09-t.context/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- helper implementations used by the concurrent probe ---'
rg -n -C12 'func (newPublicStateWriter|incrementFailedWritebacksInState|readPersistedFailedWritebacks|writeMountsyncPublicState|writeCLIMirrorState)' cmd/relayfile-cli --glob '*.go'
printf '%s\n' '--- all direct uses of the test context API and Go version settings ---'
rg -n -C3 't\.Context\(\)|go-version|GO_VERSION|^go [0-9]' \
go.mod .github Makefile cmd/relayfile-cli --glob '*.go' --glob '*.yml' --glob '*.yaml' 2>/dev/nullRepository: AgentWorkforce/relayfile
Length of output: 11416
Gate the probe and make it compile with Go 1.22.
go test ./..., the Makefile test target, and both CI workflows discover this test. Add an opt-in environment guard before starting the worker. Replace t.Context() because testing.T.Context was added in Go 1.24, while CI uses Go 1.22. Ensure worker shutdown runs when t.Fatalf exits the test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/relayfile-cli/relay_state_counter_test.go` around lines 42 - 53, Update
TestFailedWritebacksLostUpdateUnderConcurrency to return immediately unless its
opt-in environment variable is enabled, before starting the worker; replace
t.Context() with a Go 1.22-compatible context and ensure worker shutdown is
deferred before any t.Fatalf path can exit the test.
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| for { | ||
| select { | ||
| case <-stop: | ||
| return | ||
| default: | ||
| _ = syncer.FlushOutboxOnce(t.Context()) | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| for i := 0; i < increments; i++ { | ||
| if err := incrementFailedWritebacksInState(localDir); err != nil { | ||
| t.Fatalf("increment %d: %v", i, err) | ||
| } | ||
| } | ||
|
|
||
| close(stop) | ||
| wg.Wait() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Always stop the worker after a fatal test exit.
If incrementFailedWritebacksInState fails at Line 77, t.Fatalf exits before Lines 82-83 run. The mountsync worker then continues to write state. Register cleanup immediately after starting the worker.
Proposed fix
go func() {
defer wg.Done()
for {
@@
}
}()
+ defer func() {
+ close(stop)
+ wg.Wait()
+ }()
for i := 0; i < increments; i++ {
@@
- close(stop)
- wg.Wait()
-
if got := readPersistedFailedWritebacks(localDir); got != increments {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wg.Add(1) | |
| go func() { | |
| defer wg.Done() | |
| for { | |
| select { | |
| case <-stop: | |
| return | |
| default: | |
| _ = syncer.FlushOutboxOnce(t.Context()) | |
| } | |
| } | |
| }() | |
| for i := 0; i < increments; i++ { | |
| if err := incrementFailedWritebacksInState(localDir); err != nil { | |
| t.Fatalf("increment %d: %v", i, err) | |
| } | |
| } | |
| close(stop) | |
| wg.Wait() | |
| wg.Add(1) | |
| go func() { | |
| defer wg.Done() | |
| for { | |
| select { | |
| case <-stop: | |
| return | |
| default: | |
| _ = syncer.FlushOutboxOnce(t.Context()) | |
| } | |
| } | |
| }() | |
| defer func() { | |
| close(stop) | |
| wg.Wait() | |
| }() | |
| for i := 0; i < increments; i++ { | |
| if err := incrementFailedWritebacksInState(localDir); err != nil { | |
| t.Fatalf("increment %d: %v", i, err) | |
| } | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 71-71: stdversion: testing.Context requires go1.24 or later (module is go1.22)
(govet)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/relayfile-cli/relay_state_counter_test.go` around lines 63 - 83, Register
test cleanup immediately after starting the worker goroutine, before the
increment loop in the relay state counter test, so a t.Fatalf from
incrementFailedWritebacksInState still signals stop and waits for the worker to
exit. Reuse the existing stop channel and sync.WaitGroup cleanup behavior, while
preserving the normal close-and-wait path without duplicating cleanup.
| ssh sf-mini ' | ||
| pkill -f "relayfile-cli mount ws_latency_20260807" | ||
| pkill -f receiver-watch.py | ||
| pkill -f "clock-offset.py server" | ||
| rm -rf ~/relayfile-latency-mount-20260807 | ||
| rm -rf ~/.relayfile-latency-harness | ||
| ' | ||
| ``` | ||
|
|
||
| Sender: | ||
|
|
||
| ```sh | ||
| pkill -f "bin/relayfile-server" | ||
| pkill -f "dev-authd.py serve" | ||
| rm -rf <scratch>/latency-run |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Scope teardown commands to this measurement run.
pkill -f receiver-watch.py, pkill -f "clock-offset.py server", pkill -f "bin/relayfile-server", and pkill -f "dev-authd.py serve" can terminate unrelated processes.
Record each started process ID and terminate that ID during cleanup. Alternatively, match a unique run-specific argument such as the latency-run state directory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/evidence/mount-latency-20260807/CLEANUP.md` around lines 33 - 47, Update
the cleanup commands in the measurement-run documentation to avoid broad pkill
patterns: record each process ID when starting the receiver watcher,
clock-offset server, relayfile server, and dev-auth daemon, then terminate those
specific IDs, or constrain matching to the run-specific latency state directory.
Preserve cleanup of only this run’s scratch resources.
| def load_arrivals(path): | ||
| """Map workspace path -> earliest observation, ignoring daemon temp files.""" | ||
| arrivals = {} | ||
| for line in open(path): | ||
| line = line.strip() | ||
| if not line: | ||
| continue | ||
| record = json.loads(line) | ||
| if record.get("kind"): | ||
| continue | ||
| relative = record["path"] | ||
| base = os.path.basename(relative) | ||
| # writeFileAtomic publishes through `.<name>.tmp-<pid>`; the temp | ||
| # becoming visible is not the file arriving. | ||
| if ".tmp-" in base or base.endswith(".tmp"): | ||
| continue | ||
| key = "/" + relative | ||
| if key not in arrivals or record["observed_ns"] < arrivals[key]: | ||
| arrivals[key] = record["observed_ns"] | ||
| return arrivals |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate file sizes before marking a trial complete.
load_arrivals discards the recorded size. The completion check only verifies that each path was observed. A zero-byte or truncated file can therefore count as a successful arrival.
Retain the arrival sizes. Emit expected per-path sizes from sender-trials.py. Reject a trial when any observed size differs.
Also applies to: 127-134
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 55-55: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/evidence/mount-latency-20260807/harness/analyse.py` around lines 53 -
72, Update load_arrivals to retain each path’s observed size alongside its
earliest timestamp, and update the completion check to compare those sizes
against expected per-path sizes. Modify sender-trials.py to emit the expected
size for every path, and reject trials when any observed size is missing or
differs.
| summary = json.loads(output.stdout) | ||
| summaries[shape] = summary |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Keep published liveness facts synchronized with raw evidence.
The post-trial raw file contains 14 samples through 11:31:47Z. The result table reports 13 samples through 11:31:36Z. The assertion harness does not detect this drift.
docs/evidence/mount-latency-20260807/harness/assertions.py#L114-L115: validate the published liveness counts and time bounds, or generate the result values from validated output.docs/evidence/mount-latency-20260807/RESULTS.md#L167-L170: update the post-trial row to report 14 samples and the actual final timestamp.
📍 Affects 2 files
docs/evidence/mount-latency-20260807/harness/assertions.py#L114-L115(this comment)docs/evidence/mount-latency-20260807/RESULTS.md#L167-L170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/evidence/mount-latency-20260807/harness/assertions.py` around lines 114
- 115, Update docs/evidence/mount-latency-20260807/harness/assertions.py lines
114-115 to validate published liveness sample counts and final timestamp bounds
against the raw evidence, or derive those result values from validated output;
update docs/evidence/mount-latency-20260807/RESULTS.md lines 167-170 to report
14 post-trial samples ending at the actual 11:31:47Z timestamp.
| "scan_ms_p95": loop_periods[int(len(loop_periods) * 0.95)] * 1e3 | ||
| if loop_periods | ||
| else None, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Compute scan_ms_p95 as p95.
For 20 scans, this expression selects index 19. That value is the maximum, not p95. Use linear interpolation, as analyse.py does, or rename the field to scan_ms_max.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/evidence/mount-latency-20260807/harness/receiver-watch.py` around lines
121 - 123, Update the scan_ms_p95 calculation in the receiver statistics output
to use the same linear-interpolation percentile logic as analyse.py, rather than
selecting index int(len(loop_periods) * 0.95), which returns the maximum for 20
scans. Preserve the existing empty-loop None behavior.
| The receiver is `sf-mac-mini` (Tailscale `100.102.30.76`), as required by the | ||
| brief. The sender and the relayfile server are `khaliqs-macbook-pro` | ||
| (Tailscale `100.89.219.17`). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove identifying host and network metadata from committed evidence.
The measurement needs abstract sender and receiver roles. It does not need personal account names, machine names, or private Tailnet addresses. This metadata enables user identification and internal-topology reconnaissance.
docs/evidence/mount-latency-20260807/METHODOLOGY.md#L39-L41: replace host names and Tailnet addresses withsenderandreceiver.docs/evidence/mount-latency-20260807/raw/mount-watch.jsonl#L1-L1: replace the absolute watch path with a repository-safe placeholder.docs/evidence/mount-latency-20260807/raw/trials-repo.jsonl#L1-L20: replace the sender host value with a neutral role label.docs/evidence/mount-latency-20260807/raw/trials-small.jsonl#L1-L32: replace the sender host value with the same neutral role label.
📍 Affects 4 files
docs/evidence/mount-latency-20260807/METHODOLOGY.md#L39-L41(this comment)docs/evidence/mount-latency-20260807/raw/mount-watch.jsonl#L1-L1docs/evidence/mount-latency-20260807/raw/trials-repo.jsonl#L1-L20docs/evidence/mount-latency-20260807/raw/trials-small.jsonl#L1-L32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/evidence/mount-latency-20260807/METHODOLOGY.md` around lines 39 - 41,
Remove identifying hostnames, personal names, and Tailnet addresses from the
evidence: in docs/evidence/mount-latency-20260807/METHODOLOGY.md lines 39-41,
use only sender and receiver role labels; in
docs/evidence/mount-latency-20260807/raw/mount-watch.jsonl lines 1-1, replace
the absolute watch path with a repository-safe placeholder; and in
docs/evidence/mount-latency-20260807/raw/trials-repo.jsonl lines 1-20 and
docs/evidence/mount-latency-20260807/raw/trials-small.jsonl lines 1-32, replace
sender host values with the same neutral role label.
| @@ -0,0 +1,29 @@ | |||
| {"kind": "watcher_started", "watch_dir": "/Users/khaliqgant/.relayfile-latency-harness/control-dir", "poll_seconds": 0.001, "primed_paths": 0, "started_ns": 1786101797649995000} | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Remove the local account name from committed evidence.
watch_dir exposes the local account name in /Users/khaliqgant/. The analyser does not require this absolute path. Redact it from the committed JSONL, and record a stable logical directory name instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/evidence/mount-latency-20260807/raw/control-watch.jsonl` at line 1,
Replace the user-specific absolute value in the watcher_started record’s
watch_dir field with a stable logical directory name, removing the local account
name while preserving valid JSONL structure and the analyser-required directory
identity.
| {"sampledAtLocalUtc":"2026-08-07T11:10:48.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:09:54.000Z", "status": "offline", "live": false, "activeAgents": 7}} | ||
| {"sampledAtLocalUtc":"2026-08-07T11:10:59.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:11:00.000Z", "status": "online", "live": true, "activeAgents": 7}} | ||
| {"sampledAtLocalUtc":"2026-08-07T11:11:11.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:11:00.000Z", "status": "online", "live": true, "activeAgents": 7}} | ||
| {"sampledAtLocalUtc":"2026-08-07T11:11:22.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:11:00.000Z", "status": "online", "live": true, "activeAgents": 7}} | ||
| {"sampledAtLocalUtc":"2026-08-07T11:11:33.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:11:00.000Z", "status": "online", "live": true, "activeAgents": 7}} | ||
| {"sampledAtLocalUtc":"2026-08-07T11:11:44.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:11:00.000Z", "status": "offline", "live": false, "activeAgents": 7}} | ||
| {"sampledAtLocalUtc":"2026-08-07T11:11:56.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:11:00.000Z", "status": "offline", "live": false, "activeAgents": 7}} | ||
| {"sampledAtLocalUtc":"2026-08-07T11:12:07.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:12:00.000Z", "status": "online", "live": true, "activeAgents": 7}} | ||
| {"sampledAtLocalUtc":"2026-08-07T11:12:18.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:12:00.000Z", "status": "online", "live": true, "activeAgents": 7}} | ||
| {"sampledAtLocalUtc":"2026-08-07T11:12:30.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:12:00.000Z", "status": "online", "live": true, "activeAgents": 7}} | ||
| {"sampledAtLocalUtc":"2026-08-07T11:12:42.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:12:00.000Z", "status": "online", "live": true, "activeAgents": 7}} | ||
| {"sampledAtLocalUtc":"2026-08-07T11:12:53.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:12:00.000Z", "status": "offline", "live": false, "activeAgents": 7}} | ||
| {"sampledAtLocalUtc":"2026-08-07T11:13:04.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:13:00.000Z", "status": "online", "live": true, "activeAgents": 7}} | ||
| {"sampledAtLocalUtc":"2026-08-07T11:13:16.3NZ","node":{"present": true, "lastHeartbeatAt": "2026-08-07T11:13:00.000Z", "status": "online", "live": true, "activeAgents": 7}} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use valid UTC timestamps in the evidence.
sampledAtLocalUtc ends with NZ, which is not a valid RFC 3339 or ISO 8601 UTC designator. The producer script emits +00:00, so this artifact does not match its stated producer contract. Replace NZ with Z or regenerate the records. Invalid timestamps prevent standard tooling from parsing the sampling chronology.
Proposed fix
-{"sampledAtLocalUtc":"2026-08-07T11:10:48.3NZ", ...}
+{"sampledAtLocalUtc":"2026-08-07T11:10:48.3Z", ...}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/evidence/mount-latency-20260807/raw/heartbeat-gate-pre.jsonl` around
lines 1 - 14, Update every sampledAtLocalUtc value in the heartbeat evidence
records to use the producer’s valid UTC format, replacing the invalid NZ suffix
with +00:00 or Z. Preserve the existing timestamps and record contents while
ensuring standard RFC 3339/ISO 8601 parsers can read the sampling chronology.
There was a problem hiding this comment.
31 issues found across 23 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/evidence/mount-latency-20260807/harness/clock-offset.py">
<violation number="1" location="docs/evidence/mount-latency-20260807/harness/clock-offset.py:73">
P2: `SAMPLES=0` or a negative value crashes after creating/appending the output file because no observations exist for summary generation; validate a positive count before the loop.</violation>
</file>
<file name="docs/evidence/mount-latency-20260807/harness/dev-authd.py">
<violation number="1" location="docs/evidence/mount-latency-20260807/harness/dev-authd.py:27">
P2: Clean measurement hosts fail at startup with `ModuleNotFoundError: cryptography`; add a pinned `cryptography>=46.0.7` harness requirement and provision it in an isolated environment before invoking this script.
(Based on your team's feedback about provisioning and pinning the harness cryptography dependency.)</violation>
<violation number="2" location="docs/evidence/mount-latency-20260807/harness/dev-authd.py:51">
P2: Concurrent first startup can mint a token signed by a different key than the JWKS server publishes because this check-then-create sequence is racy; create/read the key under an exclusive lock or atomic create protocol before either command uses it.</violation>
</file>
<file name="docs/evidence/mount-latency-20260807/harness/control-local.py">
<violation number="1" location="docs/evidence/mount-latency-20260807/harness/control-local.py:34">
P2: Control runs with different RUN_IDs reuse the same probe paths, so reruns can be silently excluded by the watcher and contaminate appended evidence; place each run beneath a validated, previously empty run-ID directory before creating trials.
(Based on your team's feedback about control experiment run isolation.)</violation>
<violation number="2" location="docs/evidence/mount-latency-20260807/harness/control-local.py:47">
P2: Published control delay is not a pure watcher delay because `t_create_ns` precedes the visibility rename; record timestamps immediately before and after `os.rename`, then report watcher delay as the resulting interval.
(Based on your team's feedback about bounding control publish timing.)</violation>
</file>
<file name="docs/evidence/mount-latency-20260807/raw/mount-watch.jsonl">
<violation number="1" location="docs/evidence/mount-latency-20260807/raw/mount-watch.jsonl:1">
P3: The watcher_started record leaks the username and absolute home path in watch_dir (`/Users/khaliqgant/relayfile-latency-mount-20260807`). This is committed raw watcher metadata; replace the absolute home path/username with a stable non-identifying alias (consistent with the existing `host: "sf-mini"` alias convention), keeping the measurement sample lines unchanged.</violation>
</file>
<file name="docs/evidence/mount-latency-20260807/harness/receiver-watch.py">
<violation number="1" location="docs/evidence/mount-latency-20260807/harness/receiver-watch.py:50">
P1: A path that vanishes between listing and `stat()` is permanently suppressed because it enters `seen` first; only mark it seen after a successful stat so a later scan can record the arrival.</violation>
</file>
<file name="docs/evidence/mount-latency-20260807/harness/assertions.py">
<violation number="1" location="docs/evidence/mount-latency-20260807/harness/assertions.py:43">
P2: Heartbeat liveness can pass with malformed timestamps or a short real-time window because this checks sample count, not normalized timestamps and elapsed duration; parse both endpoints and require valid fixed-microsecond `+00:00` values at least 90 seconds apart.
(Based on your team's feedback about heartbeat timestamp and duration validation.)</violation>
<violation number="2" location="docs/evidence/mount-latency-20260807/harness/assertions.py:114">
P2: Analyzer failures are not recorded as failed gates: invalid JSON or a non-zero subprocess exit raises here and suppresses the assertion report; convert both outcomes into a failed per-shape check before continuing.
(Based on your team's feedback about hardening analysis assertion gates.)</violation>
<violation number="3" location="docs/evidence/mount-latency-20260807/harness/assertions.py:123">
P1: The no-loss gate can pass ambiguous or stale rerun pairings because it only checks missing arrivals; make analysis expose non-unique run/shape/path pairings and fail an explicit ambiguity gate.
(Based on your team's feedback about unambiguous trial pairing.)</violation>
<violation number="4" location="docs/evidence/mount-latency-20260807/harness/assertions.py:139">
P2: An empty latency population crashes this gate while formatting `None` as a float, and the `or 0` comparison would otherwise accept it; require a non-null maximum and format null diagnostics safely.
(Based on your team's feedback about hardening analysis assertion gates.)</violation>
<violation number="5" location="docs/evidence/mount-latency-20260807/harness/assertions.py:145">
P2: The published n=25 watcher-overhead claim can remain ungated when either control file is missing, and can pass with only 20 pairs; always add a missing-evidence failure and require all 25 selected creates to pair.
(Based on your team's feedback about complete control pairing and missing overhead evidence.)</violation>
<violation number="6" location="docs/evidence/mount-latency-20260807/harness/assertions.py:180">
P2: Isolation can pass after preexisting untracked directories are deleted or their contents changed because status has no baseline; compare a recorded before/after content snapshot, or limit the assertion to current Git status.
(Based on your team's feedback about isolation content snapshots.)</violation>
</file>
<file name="docs/evidence/mount-latency-20260807/harness/analyse.py">
<violation number="1" location="docs/evidence/mount-latency-20260807/harness/analyse.py:70">
P1: A rerun or duplicate send path can be credited with its stale earliest arrival, producing a false complete trial and invalid latency; retain all observations and pair exactly one arrival at/after the modeled receiver-clock send instant, reporting non-unique or before-send matches as ambiguous rather than complete.
(Based on your team's feedback about unambiguous append-only arrival pairing.)</violation>
<violation number="2" location="docs/evidence/mount-latency-20260807/harness/analyse.py:175">
P3: The generated n=20 summary omits the promised `p95_note`, so downstream evidence can present p95 without disclosing that linear interpolation rests on the two largest observations; emit the caveat with the summary.</violation>
</file>
<file name="docs/evidence/mount-latency-20260807/harness/sender-trials.py">
<violation number="1" location="docs/evidence/mount-latency-20260807/harness/sender-trials.py:51">
P2: Unsupported shapes silently execute the repo workload and produce mislabeled evidence; accept only `small` and `repo` before token loading, with `body_for` also rejecting any other value.
(Based on your team's feedback about early trial-shape validation.)</violation>
<violation number="2" location="docs/evidence/mount-latency-20260807/harness/sender-trials.py:101">
P1: RUN_ID is never validated before being interpolated into workspace paths. A run_id containing `/` or `..` would escape the `/trials/<run_id>/` namespace and write outside the intended trial area (or collide across runs), breaking run isolation in the raw evidence. Validate run_id as a safe slug (e.g. `[A-Za-z0-9._-]+`) and reject `.` and `..` before building any paths.</violation>
<violation number="3" location="docs/evidence/mount-latency-20260807/harness/sender-trials.py:114">
P2: A malformed or unreadable HTTP error body aborts the handler before its failed-trial JSONL record is flushed; guard the read and decode with replacement so the record is always appended.
(Based on your team's feedback about preserving failed trial records.)</violation>
<violation number="4" location="docs/evidence/mount-latency-20260807/harness/sender-trials.py:132">
P3: Raw evidence leaks a personal workstation name and cannot be reused on another sender; require an explicit non-identifying `HOST_ALIAS` and emit that value.
(Based on your team's feedback about sender host aliases.)</violation>
</file>
<file name="docs/evidence/mount-latency-20260807/RESULTS.md">
<violation number="1" location="docs/evidence/mount-latency-20260807/RESULTS.md:21">
P3: The n=20 p95 figures (161.7 ms small-file, 303.9 ms change-set, plus the leg-decomposition p95s) are presented in the headline, quotable, and leg-decomposition sections without the caveat that a p95 over n=20 rests on the two largest observations. analyse.py's own docstring flags this, but RESULTS.md never repeats the caveat, so the most-quoted numbers carry no estimate of their uncertainty.</violation>
<violation number="2" location="docs/evidence/mount-latency-20260807/RESULTS.md:213">
P2: The Reproducing section only documents the `analyse.py` command for the `repo r2` batch. But this report quotes *two* headline populations — `r2` small file (median 20.2 ms, n=20) and `r2` repo-sized (216.7 ms) — plus the `run20260807` small-file batch (n=12) held as correctness evidence. `raw/trials-small.jsonl` is listed in the raw-file table but no `analyse.py` invocation is shown for it, so the small-file headline number in this document is not reproducible from the documented steps. `analyse.py` supports a run/label filter (argv[5]/argv[6]) precisely for this. Add the small-file commands (e.g. `python3 harness/analyse.py raw/clock-offset-pre.jsonl raw/clock-offset-post.jsonl raw/trials-small.jsonl raw/mount-watch.jsonl small r2` and the `run20260807` variant) so every reported population can be regenerated. (Based on your team's feedback about reproducing every reported population.)</violation>
</file>
<file name="docs/evidence/mount-latency-20260807/raw/heartbeat-gate-pre.sh">
<violation number="1" location="docs/evidence/mount-latency-20260807/raw/heartbeat-gate-pre.sh:9">
P2: `datetime.now(timezone.utc).isoformat()` does not emit fixed microsecond precision: when the sample lands on a whole second (microseconds==0) the fractional part is omitted, yielding `2026-08-07T11:10:48+00:00` instead of `...48.000000+00:00` (verified in Python 3.11). This makes sampledAtLocalUtc shapes inconsistent and will fail any strict RFC 3339 fixed-microsecond parser, so emit an explicit fixed-precision string.</violation>
<violation number="2" location="docs/evidence/mount-latency-20260807/raw/heartbeat-gate-pre.sh:11">
P3: The `if True else ""` guard is dead code: the condition is a literal True, so the else branch can never run (the empty-list `.get(...) if n else None` guards already handle `n=[]`). It also does not serve as an error fallback, because `json.load` failure still aborts the command and only the `[ -z "$HB" ]` check below catches empty output. Remove the dead ternary.</violation>
</file>
<file name="docs/evidence/mount-latency-20260807/METHODOLOGY.md">
<violation number="1" location="docs/evidence/mount-latency-20260807/METHODOLOGY.md:49">
P3: The methodology anchors numerous claims to source line numbers (`cmd/relayfile-cli/main.go:58`, `internal/mountsync/syncer.go:3251-3260,3310,3320,3339,6033-6051`, `internal/relayfile/store.go:883-885,3605`, etc.) but names no inspected commit, so the line references rot as the code moves. RESULTS.md pins `Base commit: ea67a73`; pin the methodology's source references to that same commit (or name the symbols too) so the anchored claims stay verifiable.</violation>
<violation number="2" location="docs/evidence/mount-latency-20260807/METHODOLOGY.md:124">
P3: The methodology promises the reported latency is "quoted both raw and with this control subtracted," but the emitted analysis never produces a control-subtracted figure: analyse.py computes latency only from sender/arrival records and does not load or subtract the control, and RESULTS.md reports raw medians (20.2/216.7 ms) alongside a separate control table (1.225 ms), never an adjusted number. Make the methodology describe what is actually reported (raw latency plus a separately-bound measurement-overhead control) so the doc matches the emitted results and doesn't misstate the method.</violation>
</file>
<file name="docs/evidence/mount-latency-20260807/CLEANUP.md">
<violation number="1" location="docs/evidence/mount-latency-20260807/CLEANUP.md:37">
P2: The receiver teardown runs `rm -rf ~/relayfile-latency-mount-20260807` immediately after `pkill` without confirming the FUSE mount process actually exited and that the mount table no longer lists the path, so a slow-to-exit or respawned process can leave a stale mounted path alongside a partially deleted run directory. Wait for the exact process to exit and check the mount table before removing the run directory.</violation>
<violation number="2" location="docs/evidence/mount-latency-20260807/CLEANUP.md:45">
P2: The sender teardown kills by broad process name (`pkill -f "bin/relayfile-server"`, `pkill -f "dev-authd.py serve"`), which also matches the pre-existing dev-collab server that this very doc says must stay untouched (`lsof -nP -iTCP:8299 ... expected untouched`, port 8299 on the sender). Since `bin/relayfile-server` is exactly this repo's server binary name (Makefile `SERVER_BIN := relayfile-server`, built to `bin/`), a pre-existing dev-collab deployment on the same host matches the pattern and would be killed along with the run's own server. Scope the sender teardown by the run's recorded PID(s) or by port 18299 (e.g. `lsof -tiTCP:18299 | xargs kill`) the way the receiver side already scopes on the full workspace name.</violation>
<violation number="3" location="docs/evidence/mount-latency-20260807/CLEANUP.md:59">
P3: The verification step claims `.dev-collab-stack/` and `.salvaged-from-minis/` are "still untracked, unmodified", but `git status --short` only proves the dirs remain untracked (`??`); it cannot detect content changes inside untracked directories, so "unmodified" is unverified. Either take a before/after content snapshot or hash, or reword to "still untracked" and drop the unchanged-content claim (the harness assertion in assertions.py already limits itself to the untracked status).</violation>
</file>
<file name="cmd/relayfile-cli/relay_state_two_writers_test.go">
<violation number="1" location="cmd/relayfile-cli/relay_state_two_writers_test.go:27">
P3: The variable `websocketDisabled := false` is passed as `WebSocket: &websocketDisabled`, but NewSyncer reads `websocketEnabled = *opts.WebSocket`, so the value `false` actually DISABLES the websocket — the name reads as "websocket is not disabled / enabled", contradicting its effect (and the `disableWebSocket := false` convention in internal/mountsync/syncer_test.go where `true` means disabled). The field is inert in these tests (FlushOutboxOnce never dials), so there's no behavioral bug, but the inverted naming will mislead a future reader about whether the websocket path is on.</violation>
</file>
<file name="docs/evidence/mount-latency-20260807/raw/control-watch.jsonl">
<violation number="1" location="docs/evidence/mount-latency-20260807/raw/control-watch.jsonl:1">
P2: The committed `watcher_started` record embeds a personal absolute home path and username in `watch_dir` (`/Users/khaliqgant/.relayfile-latency-harness/control-dir`). This is the same for the paired mount-watch file (`/Users/khaliqgant/relayfile-latency-mount-20260807`). Since these raw control/mount watch files are committed to the repo, the home-path/username metadata should be redacted to a stable alias so the evidence doesn't expose the contributor's local username, while keeping the measurement samples intact.</violation>
</file>
<file name="docs/evidence/mount-latency-20260807/raw/heartbeat-gate-pre.jsonl">
<violation number="1" location="docs/evidence/mount-latency-20260807/raw/heartbeat-gate-pre.jsonl:1">
P2: The `sampledAtLocalUtc` timestamps in this file are not valid RFC 3339 and do not match the format used by its paired `heartbeat-gate-post.jsonl`. Every row here uses a malformed shape like `2026-08-07T11:10:48.3NZ` (single-digit fraction `.3` plus a trailing `N` after the `Z`), whereas the post file uses fixed-microsecond precision with a `+00:00` offset: `2026-08-07T11:29:21.412747+00:00`. The pre/post files are consumed together by the heartbeat liveness gate, whose parser validates this normalized format (and relies on parsing the first/last timestamps to require a 90s span), so these rows will fail that validation and are inconsistent with their counterpart. Please re-export this file with RFC 3339 `+00:00` nanosecond/microsecond precision timestamps, matching the post file, and confirm the 90s elapsed span still holds (currently ~11:10:48 → 11:13:16).</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| if entry.path in seen: | ||
| continue | ||
| observed_ns = time.time_ns() | ||
| seen.add(entry.path) |
There was a problem hiding this comment.
P1: A path that vanishes between listing and stat() is permanently suppressed because it enters seen first; only mark it seen after a successful stat so a later scan can record the arrival.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/evidence/mount-latency-20260807/harness/receiver-watch.py, line 50:
<comment>A path that vanishes between listing and `stat()` is permanently suppressed because it enters `seen` first; only mark it seen after a successful stat so a later scan can record the arrival.</comment>
<file context>
@@ -0,0 +1,134 @@
+ if entry.path in seen:
+ continue
+ observed_ns = time.time_ns()
+ seen.add(entry.path)
+ try:
+ size = entry.stat(follow_symlinks=False).st_size
</file context>
| ) | ||
| check( | ||
| f"{shape}_no_lost_changes", | ||
| summary["trials_incomplete"] == 0, |
There was a problem hiding this comment.
P1: The no-loss gate can pass ambiguous or stale rerun pairings because it only checks missing arrivals; make analysis expose non-unique run/shape/path pairings and fail an explicit ambiguity gate.
(Based on your team's feedback about unambiguous trial pairing.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/evidence/mount-latency-20260807/harness/assertions.py, line 123:
<comment>The no-loss gate can pass ambiguous or stale rerun pairings because it only checks missing arrivals; make analysis expose non-unique run/shape/path pairings and fail an explicit ambiguity gate.
(Based on your team's feedback about unambiguous trial pairing.) </comment>
<file context>
@@ -0,0 +1,197 @@
+ )
+ check(
+ f"{shape}_no_lost_changes",
+ summary["trials_incomplete"] == 0,
+ f"incomplete: {summary['incomplete_detail']}",
+ )
</file context>
| if ".tmp-" in base or base.endswith(".tmp"): | ||
| continue | ||
| key = "/" + relative | ||
| if key not in arrivals or record["observed_ns"] < arrivals[key]: |
There was a problem hiding this comment.
P1: A rerun or duplicate send path can be credited with its stale earliest arrival, producing a false complete trial and invalid latency; retain all observations and pair exactly one arrival at/after the modeled receiver-clock send instant, reporting non-unique or before-send matches as ambiguous rather than complete.
(Based on your team's feedback about unambiguous append-only arrival pairing.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/evidence/mount-latency-20260807/harness/analyse.py, line 70:
<comment>A rerun or duplicate send path can be credited with its stale earliest arrival, producing a false complete trial and invalid latency; retain all observations and pair exactly one arrival at/after the modeled receiver-clock send instant, reporting non-unique or before-send matches as ambiguous rather than complete.
(Based on your team's feedback about unambiguous append-only arrival pairing.) </comment>
<file context>
@@ -0,0 +1,192 @@
+ if ".tmp-" in base or base.endswith(".tmp"):
+ continue
+ key = "/" + relative
+ if key not in arrivals or record["observed_ns"] < arrivals[key]:
+ arrivals[key] = record["observed_ns"]
+ return arrivals
</file context>
|
|
||
| with open(out_path, "a") as raw: | ||
| for trial in range(1, count + 1): | ||
| paths, endpoint, payload = body_for(shape, run_id, trial) |
There was a problem hiding this comment.
P1: RUN_ID is never validated before being interpolated into workspace paths. A run_id containing / or .. would escape the /trials/<run_id>/ namespace and write outside the intended trial area (or collide across runs), breaking run isolation in the raw evidence. Validate run_id as a safe slug (e.g. [A-Za-z0-9._-]+) and reject . and .. before building any paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/evidence/mount-latency-20260807/harness/sender-trials.py, line 101:
<comment>RUN_ID is never validated before being interpolated into workspace paths. A run_id containing `/` or `..` would escape the `/trials/<run_id>/` namespace and write outside the intended trial area (or collide across runs), breaking run isolation in the raw evidence. Validate run_id as a safe slug (e.g. `[A-Za-z0-9._-]+`) and reject `.` and `..` before building any paths.</comment>
<file context>
@@ -0,0 +1,146 @@
+
+ with open(out_path, "a") as raw:
+ for trial in range(1, count + 1):
+ paths, endpoint, payload = body_for(shape, run_id, trial)
+ correlation_id = f"{run_id}-{shape}-{trial:03d}"
+ expected_bytes = sum(
</file context>
|
|
||
| observations = [] | ||
| with open(output_path, "a") as raw: | ||
| for index in range(samples): |
There was a problem hiding this comment.
P2: SAMPLES=0 or a negative value crashes after creating/appending the output file because no observations exist for summary generation; validate a positive count before the loop.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/evidence/mount-latency-20260807/harness/clock-offset.py, line 73:
<comment>`SAMPLES=0` or a negative value crashes after creating/appending the output file because no observations exist for summary generation; validate a positive count before the loop.</comment>
<file context>
@@ -0,0 +1,140 @@
+
+ observations = []
+ with open(output_path, "a") as raw:
+ for index in range(samples):
+ t0 = time.time_ns()
+ stream.write(b"PROBE\n")
</file context>
| while [ "$(date +%s)" -lt "$END" ]; do | ||
| TS=$(python3 -c "import datetime;print(datetime.datetime.now(datetime.timezone.utc).isoformat())") | ||
| HB=$(timeout 25 agent-relay fleet nodes --name sf-mini --all 2>/dev/null \ | ||
| | python3 -c 'import sys,json;d=json.load(sys.stdin);n=[x for x in d.get("nodes",[]) if x.get("name")=="sf-mini"];print(json.dumps({"present":bool(n),"lastHeartbeatAt":n[0].get("lastHeartbeatAt") if n else None,"status":n[0].get("status") if n else None,"live":n[0].get("live") if n else None,"activeAgents":n[0].get("activeAgents") if n else None}) if True else "")' 2>/dev/null) |
There was a problem hiding this comment.
P3: The if True else "" guard is dead code: the condition is a literal True, so the else branch can never run (the empty-list .get(...) if n else None guards already handle n=[]). It also does not serve as an error fallback, because json.load failure still aborts the command and only the [ -z "$HB" ] check below catches empty output. Remove the dead ternary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/evidence/mount-latency-20260807/raw/heartbeat-gate-pre.sh, line 11:
<comment>The `if True else ""` guard is dead code: the condition is a literal True, so the else branch can never run (the empty-list `.get(...) if n else None` guards already handle `n=[]`). It also does not serve as an error fallback, because `json.load` failure still aborts the command and only the `[ -z "$HB" ]` check below catches empty output. Remove the dead ternary.</comment>
<file context>
@@ -0,0 +1,15 @@
+while [ "$(date +%s)" -lt "$END" ]; do
+ TS=$(python3 -c "import datetime;print(datetime.datetime.now(datetime.timezone.utc).isoformat())")
+ HB=$(timeout 25 agent-relay fleet nodes --name sf-mini --all 2>/dev/null \
+ | python3 -c 'import sys,json;d=json.load(sys.stdin);n=[x for x in d.get("nodes",[]) if x.get("name")=="sf-mini"];print(json.dumps({"present":bool(n),"lastHeartbeatAt":n[0].get("lastHeartbeatAt") if n else None,"status":n[0].get("status") if n else None,"live":n[0].get("live") if n else None,"activeAgents":n[0].get("activeAgents") if n else None}) if True else "")' 2>/dev/null)
+ [ -z "$HB" ] && HB='{"present":false,"lastHeartbeatAt":null,"status":null,"live":null,"error":"query_failed"}'
+ echo "{\"sampledAtLocalUtc\":\"$TS\",\"node\":$HB}" >> "$OUT"
</file context>
| was run. | ||
|
|
||
| `relayfile-cli mount` runs in `poll` mode by default | ||
| (`cmd/relayfile-cli/main.go:58`). Despite the name, `poll` does **not** mean |
There was a problem hiding this comment.
P3: The methodology anchors numerous claims to source line numbers (cmd/relayfile-cli/main.go:58, internal/mountsync/syncer.go:3251-3260,3310,3320,3339,6033-6051, internal/relayfile/store.go:883-885,3605, etc.) but names no inspected commit, so the line references rot as the code moves. RESULTS.md pins Base commit: ea67a73; pin the methodology's source references to that same commit (or name the symbols too) so the anchored claims stay verifiable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/evidence/mount-latency-20260807/METHODOLOGY.md, line 49:
<comment>The methodology anchors numerous claims to source line numbers (`cmd/relayfile-cli/main.go:58`, `internal/mountsync/syncer.go:3251-3260,3310,3320,3339,6033-6051`, `internal/relayfile/store.go:883-885,3605`, etc.) but names no inspected commit, so the line references rot as the code moves. RESULTS.md pins `Base commit: ea67a73`; pin the methodology's source references to that same commit (or name the symbols too) so the anchored claims stay verifiable.</comment>
<file context>
@@ -0,0 +1,187 @@
+was run.
+
+`relayfile-cli mount` runs in `poll` mode by default
+(`cmd/relayfile-cli/main.go:58`). Despite the name, `poll` does **not** mean
+"poll the server for changes" — it means "materialise a mirror of real files
+on local disk", as opposed to `fuse` (which this build hard-refuses,
</file context>
| **Measurement overhead is itself measured, not assumed.** A control experiment | ||
| creates files locally on sf-mini — same directory, same watcher, no network | ||
| involved — and records the watcher's own detection delay distribution. The | ||
| reported latency is quoted both raw and with this control subtracted, so the |
There was a problem hiding this comment.
P3: The methodology promises the reported latency is "quoted both raw and with this control subtracted," but the emitted analysis never produces a control-subtracted figure: analyse.py computes latency only from sender/arrival records and does not load or subtract the control, and RESULTS.md reports raw medians (20.2/216.7 ms) alongside a separate control table (1.225 ms), never an adjusted number. Make the methodology describe what is actually reported (raw latency plus a separately-bound measurement-overhead control) so the doc matches the emitted results and doesn't misstate the method.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/evidence/mount-latency-20260807/METHODOLOGY.md, line 124:
<comment>The methodology promises the reported latency is "quoted both raw and with this control subtracted," but the emitted analysis never produces a control-subtracted figure: analyse.py computes latency only from sender/arrival records and does not load or subtract the control, and RESULTS.md reports raw medians (20.2/216.7 ms) alongside a separate control table (1.225 ms), never an adjusted number. Make the methodology describe what is actually reported (raw latency plus a separately-bound measurement-overhead control) so the doc matches the emitted results and doesn't misstate the method.</comment>
<file context>
@@ -0,0 +1,187 @@
+**Measurement overhead is itself measured, not assumed.** A control experiment
+creates files locally on sf-mini — same directory, same watcher, no network
+involved — and records the watcher's own detection delay distribution. The
+reported latency is quoted both raw and with this control subtracted, so the
+"measurement overhead exceeds the signal" hedge can be either retired or
+confirmed with a number instead of a guess.
</file context>
| ```sh | ||
| lsof -nP -iTCP:8299 -sTCP:LISTEN # dev-collab server port: expected untouched | ||
| ssh sf-mini 'pgrep -fl "relayfile-cli.*dev-collab"' # pre-existing mounts still up | ||
| git -C <repo> status --short # .dev-collab-stack/ and .salvaged-from-minis/ still untracked, unmodified |
There was a problem hiding this comment.
P3: The verification step claims .dev-collab-stack/ and .salvaged-from-minis/ are "still untracked, unmodified", but git status --short only proves the dirs remain untracked (??); it cannot detect content changes inside untracked directories, so "unmodified" is unverified. Either take a before/after content snapshot or hash, or reword to "still untracked" and drop the unchanged-content claim (the harness assertion in assertions.py already limits itself to the untracked status).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/evidence/mount-latency-20260807/CLEANUP.md, line 59:
<comment>The verification step claims `.dev-collab-stack/` and `.salvaged-from-minis/` are "still untracked, unmodified", but `git status --short` only proves the dirs remain untracked (`??`); it cannot detect content changes inside untracked directories, so "unmodified" is unverified. Either take a before/after content snapshot or hash, or reword to "still untracked" and drop the unchanged-content claim (the harness assertion in assertions.py already limits itself to the untracked status).</comment>
<file context>
@@ -0,0 +1,74 @@
+```sh
+lsof -nP -iTCP:8299 -sTCP:LISTEN # dev-collab server port: expected untouched
+ssh sf-mini 'pgrep -fl "relayfile-cli.*dev-collab"' # pre-existing mounts still up
+git -C <repo> status --short # .dev-collab-stack/ and .salvaged-from-minis/ still untracked, unmodified
+```
+
</file context>
| client := mountsync.NewHTTPClient("http://127.0.0.1:1", "test-token", &http.Client{ | ||
| Timeout: time.Second, | ||
| }) | ||
| websocketDisabled := false |
There was a problem hiding this comment.
P3: The variable websocketDisabled := false is passed as WebSocket: &websocketDisabled, but NewSyncer reads websocketEnabled = *opts.WebSocket, so the value false actually DISABLES the websocket — the name reads as "websocket is not disabled / enabled", contradicting its effect (and the disableWebSocket := false convention in internal/mountsync/syncer_test.go where true means disabled). The field is inert in these tests (FlushOutboxOnce never dials), so there's no behavioral bug, but the inverted naming will mislead a future reader about whether the websocket path is on.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/relayfile-cli/relay_state_two_writers_test.go, line 27:
<comment>The variable `websocketDisabled := false` is passed as `WebSocket: &websocketDisabled`, but NewSyncer reads `websocketEnabled = *opts.WebSocket`, so the value `false` actually DISABLES the websocket — the name reads as "websocket is not disabled / enabled", contradicting its effect (and the `disableWebSocket := false` convention in internal/mountsync/syncer_test.go where `true` means disabled). The field is inert in these tests (FlushOutboxOnce never dials), so there's no behavioral bug, but the inverted naming will mislead a future reader about whether the websocket path is on.</comment>
<file context>
@@ -0,0 +1,209 @@
+ client := mountsync.NewHTTPClient("http://127.0.0.1:1", "test-token", &http.Client{
+ Timeout: time.Second,
+ })
+ websocketDisabled := false
+ syncer, err := mountsync.NewSyncer(client, mountsync.SyncerOptions{
+ WorkspaceID: "rw_twowriters",
</file context>
What this PR is actually meant to be
Two Go tests pinning the
.relay/state.jsonwriter contract:cmd/relayfile-cli/relay_state_two_writers_test.gocmd/relayfile-cli/relay_state_counter_test.goZero production changes. That claim is still true and is unaffected by the
contamination below — every extra file is docs or evidence.
Why the tests exist
.relay/state.jsonhas two writers racing one path, and the file flaps between twodifferent schemas. That is not a cosmetic problem: in one schema the
providerskey isabsent, and a reader that samples it three times in a row gets three identical
answers and concludes
providersis an empty array. It is not empty — the key is notthere.
internal/mountsync/syncer.gowrites viapublicState, which has noProvidersfield, while the docs describe a
providers[].An absent key is not an empty store, and a bimodal file gives you the same wrong
answer as many times as you care to ask. These tests pin the contract so the next reader
does not have to rediscover that.
relay_state_two_writers_test.gois timing-dependent by construction — it races twowriters on purpose. It can fail spuriously. That is disclosed deliberately rather
than papered over: a flaky test that is documented is a known cost, and one that is
quietly retried until green is a lie. If it goes red in CI, reproduce locally before
assuming a regression.
The contamination — what else is in this diff
Twenty files under
docs/evidence/mount-latency-20260807/(methodology, results,cleanup, a 7-script Python harness, and 10 raw
.jsonlcapture files), plus 17 changedlines in
docs/multi-agent-collaboration-assessment.md.That bundle is real work and it deserves its own review — it is the one-way mount
latency measurement, including the clock-offset captures taken before and after the
trials. It should not land on
mainas an unreviewed side effect of a test PR, andnobody has reviewed it here.
Required before merge
maincarrying only the twocmd/relayfile-cli/test files.
Provenance
Base
main, headfix/relay-state-json-two-writers(3e6ada31). Opened by @chiefbecause the specialist that would have opened it had been released 24 minutes before the
ruling to open it was issued — a decision that arrives after its executor is dismissed
has no executor.