Skip to content

chore(tab5): trace transport-start allocation failures - #50

Merged
vincentkoc merged 6 commits into
mainfrom
diag/tab5-allocation-failures
Sep 12, 2026
Merged

chore(tab5): trace transport-start allocation failures#50
vincentkoc merged 6 commits into
mainfrom
diag/tab5-allocation-failures

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Sep 10, 2026

Copy link
Copy Markdown
Member

Problem

The existing transport-start failure reports preserve the SDK error but cannot
establish whether a matching allocation failed, which capabilities were requested,
or how much matching-capability memory remains after the SDK returns. A generic
start error alone is not evidence of task-stack allocation failure.

Stacked on #49
(b05b470fabf54fbc9aac687488eafb76e5fc2814).

Changes

  • Add two private, weak default-no-op observation hooks around only
    client_start; preserve its original result and existing cleanup ordering.
  • Install the Tab5 failed-allocation callback before board initialization. Two
    fixed internal-memory slots capture the first matching node/operator calling
    task failure. ISR and unrelated-task failures are ignored.
  • Keep callback work to lock-free publication of request size, capabilities, and
    the SDK function pointer. No allocation, logging, heap walk, name dereference,
    formatting, or waiting occurs there.
  • Disarm and copy evidence before releasing slot ownership. Resolve only an
    allowlisted allocator enum afterward; raw pointers and names are never logged.
  • Emit bounded numeric evidence with
    phase=after_sdk_return_before_owner_cleanup. Heap values are explicitly
    post-return samples, not fault-time availability or proof that an aligned
    allocation would fit. Allocator identity remains distinct from unknown caller
    origin.
  • Report SDK failure without a matching callback as captured=0; also preserve
    a captured allocation followed by SDK success. Uncaptured success is silent.
  • Document the temporary diagnostic contract and add focused host execution plus
    final Tab5 ELF strong-hook checks to existing CI.

No stack sizes, PSRAM policy, SDK/component patches, dependencies, retry policy,
or storage behavior change. Other boards retain the no-op defaults. The SDK
registration API has no ownership getter; this profile requires no other
failed-allocation callback registration.

Validation

  • Seven focused host tests passed with ASan/UBSan. They compile the actual Tab5
    owner separately from the weak defaults and execute the actual transport-start
    function against bounded SDK stubs.
  • Coverage includes first capture, role/task/ISR exclusion, two-role concurrency,
    rearming, post-return observation, exact error/cleanup behavior, callback
    restrictions, absent capture, captured success, registration ordering, and
    nonfatal registration failure.
  • Host nm verified strong Tab5 definitions and weak standalone defaults.
  • A name-lookup regression failed on the prior implementation at the intended
    callback-context assertion and passed after lookup moved outside the callback.
  • actionlint and git diff --check passed.
  • Fresh P2 code review and separate independent functional and implementation
    reviews completed without findings on the frozen eight-file diff. The subsequent
    one-line CI repair also passed scoped review. Those reviews do not by
    themselves authorize hardware testing.
  • The initial CI run
    passed the host test steps but failed all five Build steps before compilation:
    an inline awk expression broke the IDF action's single-quoted command wrapper.
    The one-line repair keeps the strong-symbol gate using quote-safe grep, without
    early pipe termination. Offline wrapper proof is red before/green after for
    all five matrix entries; strong symbols pass and weak/missing symbols fail.
  • All five jobs passed in CI run 34436645624, attempt 1,
    including the final Tab5 strong-symbol gate. PR head is
    04cea90c98c14a357b74fba331a1198242e388f4; the compiled test merge is
    6be43e8ae825d94db4d35d2555f99816a7838cff, with the expected base/head parents
    and a tree matching the PR head.
  • Downloaded and verified Tab5 firmware artifact 10136593429 only:
    2,898,645 bytes, API/archive SHA256
    f37b903d4f3e6d99a25deb4c802273c6ac782c9d41d6b66ae0b173805a3bf0cf.
    All 11 internal checksums, 10 manifest file records, and four offset-mapped
    images passed; protected data partitions do not overlap those image writes.
  • Actual SDK remains 362a1776ec212788fda95f75b733bfdde3a0c394,
    v5.5.5-648-g362a1776ec2-dirty, with the existing tracked patches. SDK,
    component-patch, BSP, and tool records, generated configuration, and the full
    parsed dependency lock match the verified 2e75363 baseline. No new dependency
    or configuration drift was found.
  • Matching ELF/map artifacts remain retained in CI; they were not downloaded.
    The native strong-hook result is the enforced CI gate, not a claimed local
    inspection of those symbol files.

Bounded Hardware Proof

  • The exact compiled image 6be43e8ae825d94db4d35d2555f99816a7838cff
    was flashed to M5Stack Tab5. All four writes and independent readbacks passed;
    protected data was preserved.
  • An observed boot and one keyless Gateway restart completed with both node and
    operator roles reconnected. Observation windows were A 60.044 seconds and
    B 50.104 seconds. The B budget was 60 seconds including startup, not a second
    full 60-second observation window.
  • During that initial keyless run, no monitored fault or allocation diagnostic
    record was observed. Fault capture was not exercised in that run.
  • Two baseline console probes, status and diagnostics status, produced one
    heap sample. No node RPC, Talk, or provider request was started. No provider
    key was used and no media call was made.
  • The application was observed running before Gateway teardown. No post-run
    reset was performed; connection state after host release was not probed.
    Owned host/hardware processes and serial/port handles were released.

Subsequent Allocation Capture

A later preflight on the same compiled image captured a node-role allocation
failure around WebSocket SDK startup: requested=8192, caps=0x804,
sdk_err=-1, and origin=0 (unknown). The matching-capability heap sample was
free=45187, largest=7680, explicitly in
phase=after_sdk_return_before_owner_cleanup, not at the allocation-failure
instant. The diagnostic callback therefore now has live failure-capture evidence.

The accompanying SDK task-creation error and pinned startup path are consistent
with a WebSocket task-stack allocation failure; the callback itself records an
allocator identity, not the C caller, and its origin remains unknown. This does
not identify the allocation history that exhausted a suitable contiguous block.
The trial stopped before allowance claim, provider-key loading, node RPC, Talk,
or provider execution. It supplies no audio or voice proof.

This remains instrumentation, not a memory or reconnect root-cause fix. The
bounded boot/reconnect result does not establish voice, camera, audiovisual,
or production qualification.

Landing

Accept the bounded diagnostics and observed failure capture without claiming
a memory repair. Land after #49
using the verified cumulative tree and a merge commit that retains the signed
source history and accepted predecessor repairs. Historical image proof is not
new live qualification of the cumulative tree. No release is authorized.

@clawsweeper

clawsweeper Bot commented Sep 10, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 10, 2026
@clawsweeper

clawsweeper Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 12, 2026, 12:13 PM ET / 16:13 UTC (Revision 6).

ClawSweeper review

What this changes

The branch adds Tab5 transport-start allocation diagnostics, together with stacked camera format and geometry repairs, firmware provenance checks, and host tests.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked before merge - 3 items remain

The diagnostics remain useful and have bounded hardware evidence. The cumulative branch still contains a packaging compatibility defect already repaired in its predecessor; it is not ready to land as currently checked out.

Priority: P2
Reviewed head: 04cea90c98c14a357b74fba331a1198242e388f4

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Strong bounded hardware evidence supports the diagnostics, but the cumulative patch retains a concrete packaging defect.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (logs): The captured body reports the actual Tab5 callback observing an 8192-byte failure during WebSocket startup on the identified flashed image, followed by the intended post-return heap record. Predecessor evidence also records successful camera capture and corrected JPEG geometry; these bounded observations do not qualify the future cumulative tree or complete media behavior.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (logs): The captured body reports the actual Tab5 callback observing an 8192-byte failure during WebSocket startup on the identified flashed image, followed by the intended post-return heap record. Predecessor evidence also records successful camera capture and corrected JPEG geometry; these bounded observations do not qualify the future cumulative tree or complete media behavior.
Evidence reviewed 9 items Current main still lacks the diagnostics: The fetched default branch initializes the board and room node without the allocation callback; its transport startup also calls client_start without the observation hooks. No merged fixing PR was established.
Introduced packaging trigger: Ordinary Tab5 packaging calls the camera patch verifier even without camera_compat. That verifier unconditionally requires esp_video 2.4.1, its fixed component and manifest hashes, and its mapper layout, despite the application's existing esp_video ^2.1 constraint.
Accepted predecessor repair: #48 remains open and explicitly documents its packaging correction at 27fc464. Its current patch separates ordinary resolved-lock integrity verification from the opt-in camera repair. This PR's landing disposition already requires retaining accepted predecessor repairs.
Findings 1 actionable finding [P1] Carry forward the ordinary-packaging compatibility repair
Security None None.

How this fits together

Tab5 firmware connects separate node and operator sessions to the OpenClaw Gateway. The new observation hooks capture allocation failures during WebSocket startup and emit numeric diagnostics after the SDK returns; stacked camera changes affect captured JPEGs and firmware packaging.

flowchart TD
  A[Node or operator session] --> B[WebSocket startup]
  B --> C[Task-scoped allocation capture]
  C --> D[Post-return diagnostic log]
  B --> E[Existing success or cleanup path]
  F[Camera frame] --> G[Format and geometry repair]
  G --> H[JPEG output]
Loading

Before merge

  • Carry forward the ordinary-packaging compatibility repair (P1) - With camera_compat=False, this new branch still calls verify_camera_patch, whose identity and mapper checks require the exact 2.4.1 repair inputs. The application permits esp_video ^2.1, so a valid ordinary build with another allowed resolution now fails packaging. The corrected path already exists in fix(tab5): align CSI input with ISP output #48: ordinary builds validate their resolved lock, while explicit repair builds retain strict pins. Preserve that correction in the cumulative landing tree. This concern applies to the expanded delta after retargeting to main.
  • Resolve merge risk (P1) - The conflict-resolved cumulative landing tree remains unverified; historical hardware observations establish behavior of the recorded images, not the future integrated tree.
  • Complete next step (P2) - Integrate after fix(tab5): match camera geometry to PPA scale #49, resolve conflicts while preserving accepted predecessor repairs and signed history, then verify the cumulative tree and run CI on the resulting head before merge.

Findings

  • [P1] Carry forward the ordinary-packaging compatibility repair — scripts/package_firmware.py:225-230
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Cumulative net line growth Production and build tools +426; tests +1,365; dependency patch +18; CI +16; docs +98 The growth includes justified camera compatibility work from the predecessor stack as well as the allocation instrumentation.

Merge-risk options

Maintainer options:

  1. Preserve the accepted packaging repair (recommended)
    Integrate the corrected predecessor stack and verify that ordinary builds use resolved-lock validation while the explicit repair retains its strict pins.

Technical review

Best possible solution:

Retain the bounded diagnostics while ordinary firmware packaging follows its resolved dependency lock and only explicit camera-repair builds enforce fixed repair pins.

Do we have a high-confidence way to reproduce the issue?

Yes, by source inspection of the pinned PR: ordinary Tab5 packaging with a valid non-2.4.1 esp_video resolution reaches unconditional 2.4.1 checks even when camera compatibility is disabled. This review did not execute that path.

Is this the best way to solve the issue?

The allocation instrumentation is appropriately bounded, but the cumulative packaging path needs the already-accepted predecessor correction rather than a new implementation.

Full review comments:

  • [P1] Carry forward the ordinary-packaging compatibility repair — scripts/package_firmware.py:225-230
    With camera_compat=False, this new branch still calls verify_camera_patch, whose identity and mapper checks require the exact 2.4.1 repair inputs. The application permits esp_video ^2.1, so a valid ordinary build with another allowed resolution now fails packaging. The corrected path already exists in fix(tab5): align CSI input with ISP output #48: ordinary builds validate their resolved lock, while explicit repair builds retain strict pins. Preserve that correction in the cumulative landing tree. This concern applies to the expanded delta after retargeting to main.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 6b63951f0ac5.

Labels

Label changes:

  • add P2: The diagnostics are bounded, but the cumulative branch has a concrete ordinary-packaging compatibility defect.
  • add merge-risk: 🚨 compatibility: Without the accepted predecessor correction, valid unpatched Tab5 builds using another permitted camera component version can no longer be packaged.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (logs): The captured body reports the actual Tab5 callback observing an 8192-byte failure during WebSocket startup on the identified flashed image, followed by the intended post-return heap record. Predecessor evidence also records successful camera capture and corrected JPEG geometry; these bounded observations do not qualify the future cumulative tree or complete media behavior.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.
  • remove P3: Current review triage priority is P2, so this older priority label is no longer current.

Label justifications:

  • P2: The diagnostics are bounded, but the cumulative branch has a concrete ordinary-packaging compatibility defect.
  • merge-risk: 🚨 compatibility: Without the accepted predecessor correction, valid unpatched Tab5 builds using another permitted camera component version can no longer be packaged.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (logs): The captured body reports the actual Tab5 callback observing an 8192-byte failure during WebSocket startup on the identified flashed image, followed by the intended post-return heap record. Predecessor evidence also records successful camera capture and corrected JPEG geometry; these bounded observations do not qualify the future cumulative tree or complete media behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured body reports the actual Tab5 callback observing an 8192-byte failure during WebSocket startup on the identified flashed image, followed by the intended post-return heap record. Predecessor evidence also records successful camera capture and corrected JPEG geometry; these bounded observations do not qualify the future cumulative tree or complete media behavior.

Evidence

What I checked:

  • Current main still lacks the diagnostics: The fetched default branch initializes the board and room node without the allocation callback; its transport startup also calls client_start without the observation hooks. No merged fixing PR was established. (examples/m5stack-tab5-room-node/main/main.c:4, 6b63951f0ac5)
  • Introduced packaging trigger: Ordinary Tab5 packaging calls the camera patch verifier even without camera_compat. That verifier unconditionally requires esp_video 2.4.1, its fixed component and manifest hashes, and its mapper layout, despite the application's existing esp_video ^2.1 constraint. (scripts/package_firmware.py:225, 04cea90c98c1)
  • Accepted predecessor repair: fix(tab5): align CSI input with ISP output #48 remains open and explicitly documents its packaging correction at 27fc464. Its current patch separates ordinary resolved-lock integrity verification from the opt-in camera repair. This PR's landing disposition already requires retaining accepted predecessor repairs. (scripts/package_firmware.py, 6b63951f0ac5)
  • Recorded hardware failure capture: The complete supplied PR body, captured under sourceRevision 449ced3edf5b8dbb3550c31e1bdc43c53bd0ffcb42191f79b0bb56d30aebc261, reports a later allocation capture on historical compiled image 6be43e8: requested=8192, caps=0x804, sdk_err=-1, free=45187, largest=7680, and unknown origin. This directly exercises the new Tab5 observation owner. The earlier successful boot/reconnect run alone did not exercise failure capture. (examples/m5stack-tab5-room-node/main/main.c:56, 04cea90c98c1)
  • Allocator dependency contract: The patch directly registers an ESP-IDF failed-allocation callback. The pinned SDK invokes it synchronously with allocator arguments and static function names; registration replaces a single global callback. This supports the task-scoped capture and documented exclusive-registration requirement. (components/heap/heap_caps.c:50, 362a1776ec21)
  • Camera dependency contract: The introduced tracked patch directly changes this upstream mapper. The pinned source selects sensor format as CSI input below IDF 6 while retaining the requested ISP output; the branch changes that input to the post-ISP format. (esp_video/src/device/esp_video_csi_format.c:509, 67a555a517b7)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Carry the accepted predecessor packaging correction into the conflict-resolved cumulative tree and validate both ordinary and explicitly patched packaging.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (5 earlier review cycles)
  • reviewed 2026-09-10T04:07:01.307Z sha 2b42b4e :: needs maintainer review before merge. :: none
  • reviewed 2026-09-10T04:23:37.050Z sha 04cea90 :: needs changes before merge. :: none
  • reviewed 2026-09-10T04:39:59.624Z sha 04cea90 :: needs changes before merge. :: none
  • reviewed 2026-09-10T05:00:43.999Z sha 04cea90 :: blocked before merge. :: none
  • reviewed 2026-09-10T14:06:44.767Z sha 04cea90 :: needs changes before merge. :: none

@clawsweeper clawsweeper Bot added the proof: sufficient Contributor real behavior proof is sufficient. label Sep 10, 2026
@vincentkoc
vincentkoc changed the base branch from fix/tab5-camera-quantized-geometry to main September 12, 2026 16:07
@vincentkoc
vincentkoc marked this pull request as ready for review September 12, 2026 16:07
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Sep 12, 2026
@vincentkoc
vincentkoc merged commit 976b609 into main Sep 12, 2026
17 of 18 checks passed
@vincentkoc
vincentkoc deleted the diag/tab5-allocation-failures branch September 12, 2026 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant