Skip to content

loadSpooledBodies' shared read narrows the unparseable-body miscount race but does not close it: release the entry from the classify-and-delete arm #2053

Closed

Description

@philcunliffe

Source: #2042 at f77424a
Location: hypaware-core/plugins-workspace/claude/src/telemetry/bodies.js:141
Severity: minor

Evidence

Independently reproduced by the adjudicator at f77424a: two loadSpooledBodies calls naming one unparseable ref, second call delayed by setImmediate ticks, 400 trials per separation. Same-tick overlap: 0/400 miscounts and one readFile per pair (2000 calls for 2000 Promise.all pairs, against 4000 at parent c35411c), so the CI-flake interleaving is structurally gone. But at 20-tick separation the head still miscounts 103/398 genuinely-overlapping trials (parent 113/398) as unparseable=1, missing=1 instead of unparseable=2, missing=0, because pending.finally at :141 drops the map entry in the first microtask after the read settles, before the first caller's unlink is issued. Byte accounting was correct in every trial at both commits (badBytes=0 in 2400 trials), and the miscounted counters feed only diagnostics: state.bodiesMissing/bodiesUnparseable (telemetry/source.js:650-651) are not even published in the status() details block, and spool_bytes moves on the byte fields, which stay correct.

Expected behavior

Callers of loadSpooledBodies that overlap an in-flight read-classify-delete of one unparseable ref all classify it identically: unparseable sum equals the caller count that saw it, missing sum is 0, bytes reported once.

Why deferral is safe

The head is measurably no worse than the parent at every separation and structurally better at the one that broke CI, the residual miscount reaches only diagnostic counters (verified: not user data, not the spool_bytes gauge, and unparseable content is by design recoverable from transcript backfill per LLP 0253), and the in-code comment at the head now states the narrowing honestly. Landing the full fix changes concurrency semantics on a PR about verb views, and it must release the entry on every exit path including the 'if (removing.has(file)) continue' early return at :169 or the map leaks on exactly that branch, which is more than an unrelated PR should carry after two exhausted review rounds.

Acceptance condition

Release the reading entry from the classify-and-delete arm (every exit path, including the removing.has(file) continue branch) instead of pending.finally, with a macrotask-separation regression test showing 0 miscounts over hundreds of genuinely-overlapping trials (round 2 measured the mechanism at 0/400 twice) and a map-residency assertion showing reading returns to size 0 on all paths.

Evidenced behavioral defect; eligible for automatic repair.

Activity

  1. added
    neutral:fixDelegate this issue to neutral for an autonomous fix attempt (reproduce -> fix -> PR)
    on Sep 22, 2026
  2. HypForge commented on Oct 18, 2026

    @HypForge
    Contributor

    Fixed by #2717 (merged as c5bdc85).

    The shared reading entry in loadSpooledBodies now counts its users and is released in a finally on every exit path. The owner therefore holds it through its unlink, and a caller that arrives mid-delete shares the read and counts the body as unparseable, not missing.

    Evidence:

    • A deterministic regression drives the overlap with gated promises. It fails on master (unparseable 1, expected 2) and passes on the fix.
    • Removing the release fails both new tests, which shows the entry is dropped after the last caller.
    • The single-caller base-vs-fix differential is identical across all arms, so the capture-health line is unchanged; it reads timestamps only.
    • claude_telemetry_capture smoke: ok.
    • npm test: 8653 pass, 0 fail, 3 skipped.
    • Review R1: CLEAN/LOW.

    status_capture_health fails the same way on master and on this branch ("Daemon is not running" at its first step), so that failure is pre-existing and unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    neutral:fixDelegate this issue to neutral for an autonomous fix attempt (reproduce -> fix -> PR)

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions