Repository navigation
fix: LatestFrameWriter reclaims a stale segment whose pid was reused - #5
rafalmaciag wants to merge 1 commit into
Conversation
platform::get_process_start_time (Linux) extracted field 3 of /proc/<pid>/stat (the state character, e.g. 'S') into a uint64_t, the extraction failed and the function returned 0 for every process. Every writer therefore recorded writer_start_time = 0, and reclaim_stale_or_throw treated start == 0 as "skip the pid-reuse check", so liveness was only kill(pid, 0). When a container holding a writer is killed without a clean close (docker rm -f) and /dev/shm is shared with the host, the next container reuses the same pids: the new writer found its OWN pid (or one of its threads' ids - kill(tid, 0) succeeds) in the stale header, judged the segment live, threw, and displaysink failed set_caps with not-negotiated. Reproduced 10/10 with the rocketwelder 2.59.0 image. - parse /proc/<pid>/stat fields after comm as text tokens; field 22 is the 20th token - a header with writer_start_time == 0 cannot prove a live owner (every pre-fix Linux writer wrote 0): treat it as stale - tests: start time equals /proc/self/stat field 22; a writer records it; stale segment with own pid + start 0 is reclaimed; stale segment with own pid + different start is reclaimed Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rafalmaciag
left a comment
There was a problem hiding this comment.
cpp-reviewer verdict: APPROVED (no BLOCKER/MAJOR; 3 MINOR)
What I ran (own clone /tmp/claude-1001/zb-review at 82d60b9, master worktree beside it; cpp/build.sh Debug in a throwaway Ubuntu 24.04 container rw144-review-zb)
- Before the fix: master
cpp/srcwith this PR'stests/test_latest_frame.cppgives 19 passed, 3 failed:ProcessStartTimeIsProcStatField22(got 0, expected 83843191)WriterRecordsRealStartTime(got 0)StaleSegmentWithOwnPidAndZeroStartTimeReclaimed(throws "owned by a live writer")StaleSegmentWithReusedPidReclaimedpasses on master too. That is expected: the old code compared a nonzero recorded start against the broken parser's 0.
- After the fix, built with ASan+UBSan (
libasan.so.8andlibubsan.so.1linked):test_latest_frame22/22, with no sanitizer reports.--gtest_repeat=20on the stale, start-time and second-writer tests: 0 failures. - No compiler warnings in the PR build.
- Full ctest: the non-generated tests (1–29) all pass. The generated Harmony scenario tests (30–97) fail the same way on master and on this branch ("Step not found: we run in harmony", because they need the Harmony harness). They have nothing to do with this PR.
Checked, no issue
/proc/<pid>/statparse (cpp/src/platform_linux.cpp:44-66):rfind(')')is right.commmay contain spaces and), but no field after it does, so the last)always closescomm.- The loop
field = 3..22reads 20 whitespace tokens, so the last token is field 22 (starttime). stoullsits inside try/catch, and any failure returns 0.- My own test run cross-checks the value against the test's own oracle, and the engineer's container evidence cross-checks it against
/proc(8 (lf_writer_maste) S 83812919).
- Self-pid case:
- A dead writer whose pid is now our own pid recorded a different start tick, so the segment is judged stale.
- A genuine second writer in the same process (same pid, same start) is still refused.
SecondWriterOnLiveSegmentRejectednow actually exercises the start-time comparison; before, it passed through thestart == 0shortcut.
- Thread-id case:
kill(tid, 0)still succeeds, but that no longer decides anything alone./proc/<tid>/statexists for non-leader threads (it is just not listed), and its field 22 is the thread's own creation tick.- Ticks count since host boot, and a reused pid or tid is always created after the dead writer started. A false "alive" therefore needs the reused id to be created in the same 10 ms tick as the dead writer. That cannot happen when a container is recreated.
- Without time namespaces (Docker does not use them), both sides read the same clock.
- Performance: both
/procreads are on the create path only: the constructor'sget_current_process_start_time()and thereclaim_stale_or_throw()fallback. Nothing changed in publish or read. - Cross-language layout:
LatestFrameSharedHeaderis untouched (still 128 B,static_assertintact). Only the value written towriter_start_time@80changes, from 0 to the real tick. No C# or Python implementation reads or writes that field. The reader's liveness is magic-only (latest_frame.cpp:377-394), so readers are not affected. - Upgrade path: an old-writer stale header (
start = 0) is reclaimed by a new writer, 0/5 failures (lib-upgrade-old-stale-new-writer-5x.txt).
Finding #1: "start 0 = stale" lets a new writer take a segment from a LIVE old-version writer
Severity: MINOR
Category: Design
Evidence: cpp/src/latest_frame.cpp:146-147, alive = pid != 0 && start != 0 && ...; release() at latest_frame.cpp:158-167 unlinks by name.
Issue: a live writer has start time 0 in two cases:
- (a) It runs a pre-fix library, for example the
gstzerobuffer.soin rocketwelder 2.59.0 (submodule 0d795e2). - (b)
/procis unreadable where it runs.
A new-version writer opening the same name on the same /dev/shm while one of these is alive now unlinks the name and creates its own segment. Before this PR it was refused, as long as the pids differed.
When this can happen: only when two writers with the same segment name run at the same time on a shared /dev/shm. That means an old-version container and a new-version container running the same pipeline id side by side (both are up during a manual blue/green, or two stations seeded from the same pipeline id share /dev/shm). A normal rw2 upgrade does not do this. docker compose stops the old container, which kills its gst children, before starting the new one. So the old writer is dead, and reclaiming its segment is exactly the observed fix.
Effect if it happens:
- The old writer keeps writing to its now-unlinked mapping. Readers already attached to it stay there.
- When the old writer later closes cleanly, its
release()callsshm_unlink(name), which removes the NEW writer's name. New readers then cannot attach until the new writer is recreated. - This is not a memory-safety problem.
Recommendation: no code change now. Nothing observed asks for it (happy-path-first). Add one line to the epic's "Deferred until observed" list: "mixed-version concurrent writers on one segment name: new writer reclaims; old writer's close unlinks the new name." If it is ever observed, the fix is to have release() unlink only when its fd's st_ino still matches /dev/shm/<name>.
Finding #2: process_exists is now redundant in the alive test
Severity: MINOR
Category: Standards (simplicity)
Evidence: cpp/src/latest_frame.cpp:146-147
Issue: with start != 0 required, get_process_start_time(pid) == start already implies the process exists, because a missing /proc/<pid> returns 0. The kill(pid, 0) call only adds a way to be wrong: with EPERM it reports a live process owned by another uid as dead. That outcome is still harmless today, because the sticky /dev/shm blocks the unlink and the retried create() throws.
Question: drop platform::process_exists(pid) from the expression? Optional.
Finding #3: the header contract does not mention the start-0 rule
Severity: MINOR
Category: Design (docs)
Evidence: cpp/include/zerobuffer/latest_frame.h:69 (writer_start_time; // writer process start time (pid-reuse guard)) and the class comment "Reclaims a stale segment left by a dead writer on create".
Issue: the rule "0 means unknown, so the segment is treated as stale and reclaimed" lives only in a .cpp comment, which also carries history ("epic-144, 2026-11-07"). Another-language writer would need this rule from the header. One clause on the field comment would cover it. Optional.
Not in scope (stated by the engineer, confirmed)
platform_windows.cpp has no get_process_start_time / get_current_process_start_time, and SharedMemory::remove is a no-op there, so this PR changes nothing on Windows.
🤖 Generated with Claude Code
Problem (observed, epic-144, 2026-11-07)
After the rocket-welder container was recreated, a camera pipeline failed to start with
not-negotiated. A shared/dev/shmstill held the segment that the killed container'sdisplaysinkwrote. The plugin logged:pid 8 was the new writer itself.
Root cause
platform::get_process_start_timeread field 3 of/proc/<pid>/statinto auint64_t. Field 3 is the state character (S,R), so the read failed and the function returned 0 for every process. As a result, everyLatestFrameWriterwrotewriter_start_time = 0(header offset 80).reclaim_stale_or_throwreadsstart == 0as "skip the pid-reuse check". Liveness then rested onkill(pid, 0)alone.kill(<thread id>, 0)also succeeds: on the epic-144 env, a stale header's pid 187 was a thread of the new gst-launch. In both cases the dead segment counts as live, the constructor throws, andset_capsfails.Fix
commas text tokens. Field 22 is the 20th token.writer_start_time == 0as stale. It cannot prove a live owner, because every Linux writer before this fix wrote 0. Without this, the first restart after upgrading still fails on the segments the old writers left.Evidence
test_latest_frame):Step not found: we run in harmonyand are unrelated./dev/shm,docker rm -fthe first, then start the same writer in a second container):displaysink(zerobuffer 0d795e2), same piddisplaysink, different pidLatestFrameWriteron masterLatestFrameWriteron this branchRelease path
The fix reaches rocket-welder only through streamer. streamer pins
src/zerobufferat 0d795e2 (v3.2.8), so rocket-welder needs:streamer:base image in rw2's Dockerfiles;The reader (native-player) does not need this change.
Not changed
platform_windows.cppdefines neitherget_process_start_timenorget_current_process_start_time. This PR does not touch Windows.🤖 Generated with Claude Code