Skip to content

Conversation

ChumpChief
Copy link
Contributor

@ChumpChief ChumpChief commented Aug 26, 2025

Second attempt at #25171 that was reverted due to failures in the Loop integration pipeline. Old documents rely on the identity entries of the redirect table, because handles created after attach used to directly reference the storage ID rather than a local ID. I've restored the identity mappings, plus added a regression test. Also added a lot of comments to detail this historical context.

AB#45943

This PR changes how we track blobs that are created while the container is detached.

  1. Currently we store them in the redirect table as pseudoId -> undefined and at attach we update them to pseudoId -> storageId where pseudoId is their index in the MemoryBlobStorage array.
  2. With this PR they will instead be tracked as localId -> pseudoId and at attach updated to localId -> storageId.
  3. This allows getBlob() to be less-special-cased, and allows us to tighten up the typing of the redirectTable. In several places we can stop passing/checking the attachState.

@ChumpChief ChumpChief requested review from Copilot and dannimad and removed request for Copilot August 26, 2025 23:12
@github-actions github-actions bot added area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc labels Aug 26, 2025
@github-actions github-actions bot added the base: main PRs targeted against main branch label Aug 26, 2025
@Copilot Copilot AI review requested due to automatic review settings August 28, 2025 15:19
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR modifies how detached blobs are tracked in the blob manager's redirect table to improve consistency and reduce special cases. Instead of storing detached blobs as pseudoId -> undefined, they are now tracked as localId -> pseudoId and updated to localId -> storageId at attach time.

  • Changes redirect table type from Map<string, string | undefined> to Map<string, string>
  • Updates blob manager methods to work with the new redirect table structure
  • Adds regression test for blob storage ID compatibility

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
gcSweepAttachmentBlobs.spec.ts Fixes test assertion logic
blobs.spec.ts Updates test to compare full summary tree structure
blobManager.spec.ts Updates redirect table references and test expectations
blobHandles.spec.ts Renames localBlobIdGenerator to localIdGenerator
containerRuntime.ts Updates method call to patchRedirectTable
blobManagerSnapSum.ts Removes undefined handling from redirect table type
blobManager.ts Core implementation changes for new redirect table handling
Comments suppressed due to low confidence (1)

packages/runtime/container-runtime/src/test/blobManager.spec.ts:1

  • The test comment indicates this is only 'to appease an assert', but the test should verify the actual behavior being tested rather than just satisfying preconditions.
/*!

@ChumpChief ChumpChief enabled auto-merge (squash) August 29, 2025 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant