Recording stops at 2 GB because the binary, not the card, says so - #124
Conversation
PR Summary by QodoEnable large-file recording and report size-limit stops
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
Code Review by Qodo
1. Status selects stale recorder
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| if (sr == RECORDER_STOP_MANUAL) { | ||
| sr = rec_snap.last_stop_reason; | ||
| last = &rec_snap; | ||
| } |
There was a problem hiding this comment.
1. Status selects stale recorder 📘 Rule violation ≡ Correctness
Inactive status chooses between independently retained recorder snapshots using persistent stop reasons rather than session recency, so a manual TS stop can select an older raw-recorder snapshot and an old TS failure can override a more recent manually stopped HEVC recording. Failed starts can also set disk_full before retained metadata is reset, causing all three backends to return stale paths, counters, and stop reasons for an earlier recording or attribute them to an attempt that never started.
Agent Prompt
## Issue description
Inactive recording status infers snapshot ownership and recency from persistent `last_stop_reason` values. This can select stale metadata from an older TS or raw/HEVC recording, or expose retained metadata as belonging to a failed start, instead of reporting the recording that most recently ended.
## Issue Context
The API contract requires inactive `path`, `frames`, `bytes`, and `segments` to describe the recording that ended. TS and HEVC/raw recorder state is retained independently, and a manual or non-manual stop reason is not a reliable recency signal; track which recorder owns the current or most recently ended session by adding explicit stop sequencing/generation metadata or maintaining one authoritative last-completed-recording snapshot. Ensure failed starts do not associate old counters and paths with a new stop reason, and apply the correction consistently across all three runtime callbacks.
## Fix Focus Areas
- src/star6e_runtime.c[829-861]
- src/maruko_runtime.c[101-132]
- src/cv610_runtime.c[752-783]
- src/star6e_ts_recorder.c[212-264]
- src/star6e_recorder.c[150-203]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if ((uint64_t)ts_len > RECORDER_OFF_T_CEILING - state->segment_bytes) { | ||
| fprintf(stderr, | ||
| "[ts_recorder] segment at %llu bytes cannot grow past the " | ||
| "32-bit file ceiling and the stream produced no IRAP to " | ||
| "rotate on; stopping\n", | ||
| (unsigned long long)state->segment_bytes); | ||
| stop_with_reason(state, RECORDER_STOP_SIZE_LIMIT); | ||
| return -1; |
There was a problem hiding this comment.
2. write_video() exceeds size guideline 📘 Rule violation ⚙ Maintainability
The new ceiling guard and error-classification logic expand star6e_ts_recorder_write_video() to approximately 115 lines. The function now combines muxing, ceiling enforcement, error reporting, counter updates, and synchronization instead of delegating focused operations to helpers.
Agent Prompt
## Issue description
`star6e_ts_recorder_write_video()` substantially exceeds the approximately 80-line function guideline after adding inline ceiling enforcement and `EFBIG` handling.
## Issue Context
Keep frame muxing as the function's main responsibility. Extract the file-ceiling guard and classified write/error handling into focused static helpers without changing rotation or stop-reason behavior.
## Fix Focus Areas
- src/star6e_ts_recorder.c[401-515]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } else if (errno == EFBIG) { | ||
| /* Only reachable if the guard above was defeated -- | ||
| * a shrinking off_t, or a filesystem cap below it | ||
| * (FAT32's 4 GB). Name it: "write error" sent the |
There was a problem hiding this comment.
3. Filesystem limit truncates access unit 🐞 Bug ≡ Correctness
The new EFBIG path labels the TS file intact, but write_all() may already have written part of the access unit before encountering a filesystem ceiling below the off_t ceiling. Because this path does not truncate back to the pre-write offset, recordings exceeding limits such as FAT32's can end with a partial TS access unit while reporting size_limit.
Agent Prompt
## Issue description
A TS write can partially succeed before returning `EFBIG` at a filesystem-specific size ceiling. The recorder reports `size_limit` without truncating the partial access unit, contradicting the API guarantee that the file ends on a frame boundary.
## Issue Context
`RECORDER_OFF_T_CEILING` only covers the ABI offset limit and is `UINT64_MAX` in the shipped build. The raw recorder already records the frame offset and calls `ftruncate()` after failed writes; the TS recorder needs equivalent behavior.
## Fix Focus Areas
- src/star6e_ts_recorder.c[444-484]
- src/star6e_recorder.c[251-296]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
A 32-bit glibc build leaves off_t 32-bit and open() without O_LARGEFILE, so
the kernel refuses any write crossing 2^31-1 with EFBIG no matter what the
filesystem allows. A recording on exFAT died at exactly 2147483647 bytes,
twice, and reported "write_error" -- which reads as bad media and sent the
diagnosis to the SD card. `record.maxMB` above ~2047 was therefore a knob
that validated, applied over MUT_RESTART, and then silently could not work.
Build with -D_FILE_OFFSET_BITS=64. That is the whole fix: it removes the
ceiling from open(), and equally from the stat()/fstat() calls in
venc_recordings.c and venc_httpd.c that return EOVERFLOW for a >2 GB file
and would otherwise hide such a recording from the listing and the download
path. The flag has to be on every translation unit or the link silently
mixes two off_t layouts, so it goes in COMMON_CFLAGS and is mirrored into
HOST_CFLAGS to keep the tested ABI equal to the shipped one. Note that a
CFLAGS change does not invalidate objects: this needs a clean build.
The rest is so the knob cannot lie again if that flag is ever lost:
- check_rotation() rotates on whichever binds first, the operator's limit or
the one off_t can reach. A configured max_mb above the ceiling now yields
more segments instead of a dead recorder. Inert at 64-bit off_t.
- A segment can only be cut on an IRAP, and the IDR request is bounded, so a
GDR stream can carry one past any threshold with no cut coming. Rather
than walk into an EFBIG that truncates mid-AU, stop on the frame boundary
with the file intact.
- EFBIG gets its own arm in all three recorders and its own stop reason,
RECORDER_STOP_SIZE_LIMIT ("size_limit"), because nothing failed -- a
ceiling is not an I/O error and should not read as one.
- The inactive branch of the record status filled in only stop_reason, so a
recorder that stopped by itself answered {path:"", frames:0, bytes:0}. It
now carries the path, bytes, frames and segments from the snapshot the
reason came from -- for a non-manual stop that is the entire diagnosis.
contract_version 0.29.0 -> 0.30.0: the stop_reason enum gains a value and the
inactive status payload changes meaning. Both are additive under the
contract's own governance rules. VERSION 0.82.0 -> 0.83.0.
Docs: maxSeconds/maxMB of 0 were documented as "no limit" but the runtime
only overrides its compiled-in default when the value is > 0, so 0 means
300 s / 500 MB. Also state that rotation is TS-only; format "hevc" ignores
both thresholds, which is filed separately.
Device-verified on SSC338Q (.232) against the real recorder core -- three
arms of one harness, same 58 GB FAT32 card, same deterministic input, only
the build differing:
stock master, no flag EFBIG "File too large" at 34084 frames
file on disk exactly 2147483647 -- the report,
reproduced byte for byte
patched, no flag segment cut at 2147212672, second segment opened,
2306867536 bytes over 2 segments, exit 0
patched + flag 2306867536 bytes in ONE segment, exit 0
The last two wrote identical totals from identical input, so segmentation is
the only variable between them. Note the stock arm's counter stopped at
2147464968 while the file is 2147483647: the failing write was partial, which
is the mid-AU truncation the new frame-boundary stop exists to prevent.
Separately verified on .232 that a 32-bit off_t build cannot stat() or fstat()
a 3 GB file at all (EOVERFLOW), so fixing only open() would have produced
recordings the API then hides.
CV610 was checked and is not affected: measured on .181, musl is 64-bit off_t
and forces O_LARGEFILE regardless of the flag. That negative comes from a
probe whose raw-openat control DID fail with EFBIG on the same kernel, so the
instrument was shown able to detect the positive before its negative was
trusted.
Fixes the root cause reported in OpenIPC#118.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1w7gRc6ASpa7P7HRCBQEw
bbd1c88 to
ef7119d
Compare
|
Qodo review addressed — all three findings were real, and two were bugs I 1. Status selects stale recorder — confirmed, fixed. The rule was "take Fixed by selecting on 2. 3. Filesystem limit truncates the access unit — confirmed, and I had Fixed with a rollback to New coverage:
On the two alternatives in the summary — agreed, and for the reason given: |
1 participant
Summary
Fixes the root cause in #118. Recordings stopped at exactly 2147483647
bytes with
stop_reason: "write_error", on exFAT, which has no such limit.The ceiling was never in the media — it was in the binary. A 32-bit glibc
build leaves
off_t32-bit andopen()withoutO_LARGEFILE, so the kernelreturns
EFBIGfor any write crossing 2^31-1 whatever the filesystem allows(
src/star6e_ts_recorder.c:160,src/star6e_recorder.c:176).record.maxMBabove ~2047 was therefore a knob that validated, applied over
MUT_RESTART,and then silently could not work.
This matters for how the setting is framed: an integrator cannot fix a
32-bit
off_tby choosing a better filesystem or a bigger card. The segmentsize is theirs to choose; the ceiling is ours to remove.
1. Build with
-D_FILE_OFFSET_BITS=64That is the fix (
Makefile:62). It removes the ceiling fromopen()andequally from the
stat()/fstat()calls atsrc/venc_recordings.c:162,248and
src/venc_httpd.c:268, which returnEOVERFLOWfor a file over 2 GB —so fixing only the write would have produced recordings the listing and the
download then hide. Measured on Star6E against a sparse 3 GB file:
off_toff_tstat()EOVERFLOW— invisible to/api/v1/recordingsfstat()EOVERFLOW— undownloadableThe flag must reach every translation unit or the link silently mixes two
off_tlayouts, so it is set once inCOMMON_CFLAGSand mirrored intoHOST_CFLAGSto keep the tested ABI equal to the shipped one. A CFLAGSchange does not invalidate objects, so this needs a clean build; the flag
was confirmed at the artifact, not the command line —
nm -uonstar6e_ts_recorder.oresolvesopen64.2. Rotation is bounded by what
off_tcan reachcheck_rotation()now rotates on whichever limit binds first: theoperator's, or the one the binary can actually write. A
max_mbabove theceiling yields more segments instead of a dead recorder. On every shipped
build the ceiling is
UINT64_MAXand this is inert — it is insuranceagainst the flag being lost, not live behaviour.
3. A segment that cannot be cut stops cleanly
A segment can only open on an IRAP, and the IDR ask is bounded by
TS_RECORDER_MAX_IDR_REQUESTS, so a GDR stream can carry one past anythreshold with no cut coming — the clamp in (2) alone is defeatable exactly
in the case that motivated the IDR-request machinery. Rather than walk into
an
EFBIGthat truncates mid-access-unit, the recorder stops on the frameboundary with the file intact and reports
size_limit.That truncation is not hypothetical: in the reproduction below, the
unpatched recorder's own counter stopped at 2147464968 while its file is
2147483647. The failing write was partial.
A ceiling below the
off_tone — FAT32 stops at 4 GB — cannot beanticipated by that guard, since it derives from
sizeof(off_t). There thefailure lands inside
write_all()mid-access-unit, so the write is now rolledback to
segment_bytes, which is already the offset of the last completeframe and so costs no
lseek. The raw recorder always had this; the TS onedid not, which made
size_limitclaiming an intact file wrong on exactly thatpath.
4.
EFBIGis no longer reported aswrite_errorAll three recorders name it, with a new stop reason
RECORDER_STOP_SIZE_LIMIT/"size_limit". Nothing failed — the file isintact and closed on a frame boundary — and a ceiling calls for lowering
record.maxMB, while an I/O error calls for looking at the card. Reportingboth as
write_erroris what sent #118 to the SD card.5. The record status keeps its counters after a self-stop
The inactive branch of the status callback filled in only
stop_reason, soa recorder that stopped by itself answered
{"path":"","frames":0, "bytes":0,"segments":0}— losing both the file that was cut short and howfar it got, which is the entire diagnosis for a non-manual stop. It now
carries path, bytes, frames and segments of the recording that ended, on all
three backends.
Which recorder that is comes from
record.format, not from guessing. Anearlier revision picked "the TS reason unless it is manual", which says
nothing about which session ended last — a retained
write_errorwould haveoutranked a later, cleanly stopped recording and, with the counters now
following the reason, named that older file too.
record.formatisrestart_required, so only one recorder can have run in a given process andthe ambiguity is removed rather than arbitrated.
6. Docs
maxSeconds/maxMBof0were documented as "no limit", but the runtimeoverrides its compiled-in default only when the value is
> 0— so0means 300 s / 500 MB, not unlimited. Corrected, along with a statement that
rotation is
format: "ts"only.Evidence
Three arms of one harness driving the real recorder core on Star6E
(SSC338Q), same 58 GB FAT32 card, same deterministic input,
max_mb=2500throughout — only the build differs.
sizeof(off_t)EFBIG"File too large"The control arm reproduces #118 byte for byte, so the instrument is known
to detect the defect before either negative is trusted. The two patched arms
wrote identical totals from identical input, so segmentation is the only
variable between them.
CV610 and Maruko are not affected and are not changed by this. musl is
64-bit
off_tand forcesO_LARGEFILEinto everyopen()with or withoutthe flag — measured on CV610, not assumed. That negative is only trustworthy
because the same probe's second arm opened the same file through a raw
openatsyscall, bypassing the libc wrapper and guaranteed to lackO_LARGEFILE, and did fail withEFBIGon that kernel. A probe whosenegative arm also passes proves nothing.
Method note: file sizes are read from the filesystem after each run, not from
the recorder's counters — the control arm shows those two disagreeing, so the
counter alone would have hidden the partial write.
Validation
make buildclean on all three backends frommake clean; no new warnings.make test— 3028 passed, 0 failed. New coverage:test_ts_size_limit_rolls_back_partial_writeusesRLIMIT_FSIZEto pose abelow-
off_tceiling, which is what makes this reachable from a host test atall — the
off_tceiling itself is unreachable whereoff_tis 64-bit. Itasserts the stop is classified
size_limitand that the file ends on a wholenumber of TS packets. Mutation-checked both ways: removing the rollback fails
2 assertions, misclassifying
EFBIGfails 1. The repo's owndoc_app_version_matches_VERSION/doc_contract_version_matches_codegatesalso caught an incomplete version bump mid-change.
AGENTS.md:553(~80-line functions): the ceiling guard and errorclassification moved into
ts_write_muxed(), leaving muxing aswrite_video()'s job; it is now 77 lines.the
stat()probe; CV610 for theO_LARGEFILEprobe. The harness drivesthe SoC-independent recorder entry points and links only
-lpthread, so itran alongside a live
waybeamwithout disturbing it.off_tis32-bit; a host test would have to fake the ceiling to reach them, and one
that can only pass proves nothing. The 21 existing TS-recorder tests cover
the rotation logic this touches.
contract_version0.29.0→0.30.0— thestop_reasonenum gains a valueand the inactive status payload changes meaning, both non-breaking under the
contract's own governance rules.
VERSION0.82.0→0.83.0.Review
Qodo's review found three issues, all real and all fixed in the current head:
the stale-recorder selection above (a bug this PR introduced by making the
counters follow the reason),
write_video()exceeding the ~80-line guideline,and the partial-write truncation above — which this PR's own evidence table had
already recorded as the 18679-byte gap between 2147464968 and 2147483647
without my noticing what it implied.
Not in this change
format: "hevc"ignoresmaxSeconds/maxMBentirely — the raw recorder hasno rotation code at all. That is why the "lower
maxMB" workaround in #118does not generalise. Filed separately as #123: it is new behaviour in a path
with no segment state, needing its own cut boundary and tests. The docs here
state the current behaviour so it is no longer a doc/code mismatch either way.
🤖 Generated with Claude Code
https://claude.ai/code/session_01U1w7gRc6ASpa7P7HRCBQEw