Skip to content

Revert "fix: reclaim dead message refs, reuse exhausted capacity (#176)" — ref slot reuse is unsafe - #191

Merged
ranxianglei merged 1 commit into
masterfrom
2026-09-03_revert-ref-reclamation
Sep 3, 2026
Merged

ranxianglei merged 1 commit into
masterfrom
2026-09-03_revert-ref-reclamation

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Reverts merge commit 4d0b61f (PR #176, released in 0.0.48/0.0.49).

Why revert

PR #176 introduced ref slot recycling: prune byRaw/byRef to view ∪ active-block effectiveMessageIds each turn, plus two reuse paths (wrap-around scan from MIN_INDEX after forward exhaustion; cursor drop via highestUsedIndex(pruned)+1). Maintenance review judged this design unsafe and it contradicts the architecture's own "GC-free" key design principle:

  1. Silent decompress misattribution. effectiveMessageIds keeps only anchor ids (observed: a 7-message folded range retained 2). Summary text cites tags (mNNNNN) whose raw ids are NOT anchors and NOT in view → their mappings get pruned → slot freed → a later message reuses the slot → the model decompresses the old tag and receives the WRONG message's content, silently. Wrong content is strictly worse than an error.
  2. Reuse is a new risk class. Pre-fix: reclaim dead message refs, reuse exhausted capacity, classify boundary failures (billion-context#386) #176 the map never shrank, so highestUsedIndex+1 was monotonic and slots were never reused; the model could trust any tag it has ever seen. fix: reclaim dead message refs, reuse exhausted capacity, classify boundary failures (billion-context#386) #176 breaks that trust contract for every host.
  3. The architectural invariant going forward (recorded in hosts' AGENTS.md): message ids / ref numbers are never reused or duplicated within a session — ever. Capacity is addressed by widening the ref space if ever needed, not by recycling.

Accepted trade-off

The original #386 leak (monotonic growth toward the 99999 cap → allocateFreeRef throw → host-side silent stall) is REOPENED by this revert. That is accepted for now; the proper fix is widening REF_WIDTH (5→7 digits) so monotonic growth can never reach the cap — proposed separately.

Impact

Verification

  • npm run typecheck ✓
  • npm test 558/558 ✓
  • grep -rn pruneDeadRefs src/ tests/ → empty; refs.ts wrap-around removed

Merge needs 2-agent review per AGENTS.md; human merges.

…mation"

This reverts commit 4d0b61f, reversing
changes made to fa38292.
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

📦 Built Package Artifact

Branch: 2026-09-03_revert-ref-reclamation (e8081c8)

Option A — Install from npm PR tag (recommended)

In your adapter project:

npm install acp-kernel@pr-191

Each push to this PR publishes a new version under the pr-191 npm tag.

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf acp-kernel-pr191.tgz
npm install ./package

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · qwen3.8-27b

[bot] 🏷 Agent review 1/2 — VERIFIED ✅ (LGTM to merge)

I checked out the PR commit (e8081c8) and independently re-ran every check rather than trusting the summary. Details below. Not merging — human-only per AGENTS.md.

Revert correctness (this is the load-bearing question for a revert)

CI gates (all green on the PR commit, real Node v22)

  • npm run typecheck → exit 0
  • npm test → 558/558 pass, exit 0 (matches your claim exactly)
  • npm run build (tsup + tsc emitDeclarationOnly) → exit 0
  • Formatting: for every file this PR touches, master and the PR have identical prettier state → this PR introduces zero formatting regressions. (The repo-wide format:check failure is pre-existing and not a release-CI gate.)

Safety argument — I agree, and here's why there's no cheaper fix

Confirmed the mechanism in code: sync.ts pruned byRaw/byRef down to view ∪ active-block effectiveMessageIds, and allocateFreeRef gained a wrap-around second scan (MIN_INDEX→from) after forward exhaustion. Since effectiveMessageIds retains only anchor ids, a summary citing a non-anchor tag has its mapping pruned → slot freed → reused by a later message → the model decompresses an old tag and silently gets the wrong message. Because tags are opaque strings the model copies verbatim, there is no session registry tying a cited tag to its generation — so any reuse path is inherently misattributable. The only correct fix is making tags unique-for-the-session (never reuse), which is exactly this invariant. Reverting is right; recycling was a new risk class with no safe mitigation short of global uniqueness.

Accepted trade-off — accurately characterized

Confirmed REF_WIDTH = 5, MAX_INDEX = 99999, with the allocateFreeRef throw at src/refs.ts:99. So reopening the monotonic-growth-toward-cap leak (#386) is real but bounded (hosts currently pin 0.0.47, which predates #176 — verified in history). Widening REF_WIDTH 5→7 (~10M slots) is the correct forward capacity fix.

One heads-up for the follow-up REF_WIDTH PR (non-blocking here)

Widening 5→7 digits changes the tag format width. Any host-side parsing that assumes a fixed m\d{5} (regexes, length checks) must be updated in the same change as the constant, or hosts will fail to parse 7-digit tags. Worth calling out explicitly in that PR's scope.

Verdict: clean, complete, well-scoped revert; all CI gates green; no regressions. Approving as agent review 1/2.

ranxianglei added a commit that referenced this pull request Oct 2, 2026
The monotonic ref map bricks sessions at m99999: refs are additive
(sync.ts), the cursor restarts at highestUsedIndex+1, and hosts that
pass the full log die on the FIRST turn after a restart. #191 named
widening REF_WIDTH as the proper fix; this is it, hybrid-style:

- indices ≤ 99,999 render byte-identical (5-digit zero-padded, the
  REF_WIDTH floor); padStart is a no-op above it, so m100000…m9999999
  widen naturally. Zero bytes change for sessions under the old cap.
- REF_PATTERN and boundaries' MESSAGE_REF_PATTERN widen to {1,7};
  leading-zero tolerance preserved (m1 ≡ m00001).
- CCR placeholder grammar accepts CANONICAL shapes only
  (5-digit padded | natural width ≥ 100000, never leading-zero):
  m100000 placeholders round-trip while near-example prose like
  "m004234" stays rejected (#1456 firewall intact).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant