Conversation
Value resources reported an empty canonical id to every client. The field the server puts on the wire was written only by deduplication, which never runs for a resource without input fields. The defect shipped twice and was found by hand during release validation; nothing in the monorepo checked it. The test creates a value resource and a final structural resource, commits, then reads both back in a separate transaction and requires a non-empty canonical id. It lives in pl-client, which both monorepo suites of the backend CI run on every pull request.
🦋 Changeset detectedLatest commit: 9920229 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+88
to
+134
| test("pure resources expose a canonical id to clients after commit", async () => { | ||
| await withTempRoot(async (pl) => { | ||
| const [valueId, structId] = await pl.withWriteTx( | ||
| "createPureResources", | ||
| async (tx) => { | ||
| const value = tx.createValue(ValueTestResource, Buffer.from("canonical id test value")); | ||
| // A structural resource with no fields reaches its final state as soon | ||
| // as both of its field sets are locked. | ||
| const struct = tx.createStruct(StructTestResource); | ||
| tx.lock(struct); | ||
|
|
||
| // Keep both reachable from the client root, so neither is collected | ||
| // before the reading transaction below. | ||
| tx.createField(field(tx.clientRoot, "value"), "Dynamic", value); | ||
| tx.createField(field(tx.clientRoot, "struct"), "Dynamic", struct); | ||
|
|
||
| await tx.commit(); | ||
| return [await value.globalId, await struct.globalId]; | ||
| }, | ||
| { sync: true }, | ||
| ); | ||
|
|
||
| // Confirm the resources are what this test claims they are, then check the | ||
| // canonical id of each one. | ||
| const [valueData, structData] = await pl.withReadTx("checkResourceKinds", async (tx) => { | ||
| return await Promise.all([ | ||
| tx.getResourceData(valueId, false), | ||
| tx.getResourceData(structId, false), | ||
| ]); | ||
| }); | ||
|
|
||
| expect(valueData.kind).toEqual("Value"); | ||
| expect(structData.kind).toEqual("Structural"); | ||
| expect(structData.final).toBe(true); | ||
|
|
||
| const [valueCid, structCid] = await readExposedCanonicalIds([valueId, structId]); | ||
|
|
||
| expect( | ||
| valueCid.length, | ||
| `value resource ${valueId} exposes an empty canonical id`, | ||
| ).toBeGreaterThan(0); | ||
| expect( | ||
| structCid.length, | ||
| `structural resource ${structId} exposes an empty canonical id`, | ||
| ).toBeGreaterThan(0); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
If the loaded CI backend takes more than Vitest's default timeout to establish the second client or answer a streaming request, Vitest terminates this test before the intentionally configured 10-second client deadline, making the regression check flaky. Set an explicit test timeout that accommodates the client deadline and transaction cleanup.
Suggested change
| test("pure resources expose a canonical id to clients after commit", async () => { | |
| await withTempRoot(async (pl) => { | |
| const [valueId, structId] = await pl.withWriteTx( | |
| "createPureResources", | |
| async (tx) => { | |
| const value = tx.createValue(ValueTestResource, Buffer.from("canonical id test value")); | |
| // A structural resource with no fields reaches its final state as soon | |
| // as both of its field sets are locked. | |
| const struct = tx.createStruct(StructTestResource); | |
| tx.lock(struct); | |
| // Keep both reachable from the client root, so neither is collected | |
| // before the reading transaction below. | |
| tx.createField(field(tx.clientRoot, "value"), "Dynamic", value); | |
| tx.createField(field(tx.clientRoot, "struct"), "Dynamic", struct); | |
| await tx.commit(); | |
| return [await value.globalId, await struct.globalId]; | |
| }, | |
| { sync: true }, | |
| ); | |
| // Confirm the resources are what this test claims they are, then check the | |
| // canonical id of each one. | |
| const [valueData, structData] = await pl.withReadTx("checkResourceKinds", async (tx) => { | |
| return await Promise.all([ | |
| tx.getResourceData(valueId, false), | |
| tx.getResourceData(structId, false), | |
| ]); | |
| }); | |
| expect(valueData.kind).toEqual("Value"); | |
| expect(structData.kind).toEqual("Structural"); | |
| expect(structData.final).toBe(true); | |
| const [valueCid, structCid] = await readExposedCanonicalIds([valueId, structId]); | |
| expect( | |
| valueCid.length, | |
| `value resource ${valueId} exposes an empty canonical id`, | |
| ).toBeGreaterThan(0); | |
| expect( | |
| structCid.length, | |
| `structural resource ${structId} exposes an empty canonical id`, | |
| ).toBeGreaterThan(0); | |
| }); | |
| }); | |
| test("pure resources expose a canonical id to clients after commit", async () => { | |
| await withTempRoot(async (pl) => { | |
| const [valueId, structId] = await pl.withWriteTx( | |
| "createPureResources", | |
| async (tx) => { | |
| const value = tx.createValue(ValueTestResource, Buffer.from("canonical id test value")); | |
| // A structural resource with no fields reaches its final state as soon | |
| // as both of its field sets are locked. | |
| const struct = tx.createStruct(StructTestResource); | |
| tx.lock(struct); | |
| // Keep both reachable from the client root, so neither is collected | |
| // before the reading transaction below. | |
| tx.createField(field(tx.clientRoot, "value"), "Dynamic", value); | |
| tx.createField(field(tx.clientRoot, "struct"), "Dynamic", struct); | |
| await tx.commit(); | |
| return [await value.globalId, await struct.globalId]; | |
| }, | |
| { sync: true }, | |
| ); | |
| // Confirm the resources are what this test claims they are, then check the | |
| // canonical id of each one. | |
| const [valueData, structData] = await pl.withReadTx("checkResourceKinds", async (tx) => { | |
| return await Promise.all([ | |
| tx.getResourceData(valueId, false), | |
| tx.getResourceData(structId, false), | |
| ]); | |
| }); | |
| expect(valueData.kind).toEqual("Value"); | |
| expect(structData.kind).toEqual("Structural"); | |
| expect(structData.final).toBe(true); | |
| const [valueCid, structCid] = await readExposedCanonicalIds([valueId, structId]); | |
| expect( | |
| valueCid.length, | |
| `value resource ${valueId} exposes an empty canonical id`, | |
| ).toBeGreaterThan(0); | |
| expect( | |
| structCid.length, | |
| `structural resource ${structId} exposes an empty canonical id`, | |
| ).toBeGreaterThan(0); | |
| }); | |
| }, 20_000); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/node/pl-client/src/core/canonical_id.test.ts
Line: 88-134
Comment:
**Test timeout undercuts client deadline**
If the loaded CI backend takes more than Vitest's default timeout to establish the second client or answer a streaming request, Vitest terminates this test before the intentionally configured 10-second client deadline, making the regression check flaky. Set an explicit test timeout that accommodates the client deadline and transaction cleanup.
```suggestion
test("pure resources expose a canonical id to clients after commit", async () => {
await withTempRoot(async (pl) => {
const [valueId, structId] = await pl.withWriteTx(
"createPureResources",
async (tx) => {
const value = tx.createValue(ValueTestResource, Buffer.from("canonical id test value"));
// A structural resource with no fields reaches its final state as soon
// as both of its field sets are locked.
const struct = tx.createStruct(StructTestResource);
tx.lock(struct);
// Keep both reachable from the client root, so neither is collected
// before the reading transaction below.
tx.createField(field(tx.clientRoot, "value"), "Dynamic", value);
tx.createField(field(tx.clientRoot, "struct"), "Dynamic", struct);
await tx.commit();
return [await value.globalId, await struct.globalId];
},
{ sync: true },
);
// Confirm the resources are what this test claims they are, then check the
// canonical id of each one.
const [valueData, structData] = await pl.withReadTx("checkResourceKinds", async (tx) => {
return await Promise.all([
tx.getResourceData(valueId, false),
tx.getResourceData(structId, false),
]);
});
expect(valueData.kind).toEqual("Value");
expect(structData.kind).toEqual("Structural");
expect(structData.final).toBe(true);
const [valueCid, structCid] = await readExposedCanonicalIds([valueId, structId]);
expect(
valueCid.length,
`value resource ${valueId} exposes an empty canonical id`,
).toBeGreaterThan(0);
expect(
structCid.length,
`structural resource ${structId} exposes an empty canonical id`,
).toBeGreaterThan(0);
});
}, 20_000);
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1791 +/- ##
==========================================
+ Coverage 54.60% 54.75% +0.15%
==========================================
Files 429 429
Lines 22535 22539 +4
Branches 5075 5075
==========================================
+ Hits 12305 12341 +36
+ Misses 8733 8705 -28
+ Partials 1497 1493 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The test read the canonical id through a second low-level client. Resource signatures are bound to the session that minted them, and a user-role client may not replay a signature from another session, so the read failed with "invalid resource signature" wherever the two clients did not happen to share a cached token. CI hit it on every run; a warm token cache hid it locally. PlTransaction now exposes getResourceCanonicalId(), the only accessor for a value that getResourceData() drops, and the test reads through it in a second transaction on the same client. The subject is unchanged: the read still happens after the commit, in a transaction of its own.
…re-regression-test
Main made changedSinceToken required on ResourceAPI_Get_Request. The merge was textually clean, so the new getResourceCanonicalId built the request without it and only tsc caught the gap. Empty, like every other on-demand read in this file: the accessor caches nothing and every call asks the server, so a token here could only suppress the one body it came for.
Replaces the getResourceCanonicalId accessor, which added public API for the sake of one test. The canonical id belongs with originalResourceId: neither is populated from the start, and both are fixed once a value has been observed, which is what the readonly group on BasicResourceData actually means. pl-tree carries the field through the mirror and guards the same transition it guards for originalResourceId. That reaches the persisted snapshot, so the schema goes to 2; an older file already loads as unknown-schema and is refetched, so no migration is needed. Reader.bytes now returns a real Uint8Array. It sliced through Buffer's species, so a round-tripped value came back a Buffer and compared unequal to the Uint8Array it was written from.
DenKoren
enabled auto-merge
September 24, 2026 12:44
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this pins
ResourceState.ExposedCID()is the only canonical ID a client ever sees over the API. It returnseffectiveCID, which used to be written only by deduplication. Deduplication waits forAllInputsFinal, which is never true for a resource with no input fields at all — a value or singleton resource. Every such resource therefore reported an empty canonical ID to every client, forever, with no state it could reach to fix itself.The defect shipped undetected and was fixed twice: 4.3.1 special-cased the getter (
if !r.features.IO()), 4.3.2 replaced that by settingeffectiveCIDat creation time so the getter is unconditional again. It was found by hand during release validation; nothing on the client side checked it.The test
lib/node/pl-client/src/core/canonical_id.test.ts—pure resources expose a canonical id to clients after commit:The read runs in its own transaction on purpose: the subject is what a client observes after the commit, not in-transaction state. It stays on the same client, because resource signatures are bound to the session that minted them and a user-role client may not replay one from another session.
One API addition
PlTransaction.getResourceCanonicalId()— the canonical id had no accessor at all.getResourceData()drops the field, because everything it returns keeps its value for the whole life of a resource and the canonical id does not (it appears at creation for resources without input fields, at deduplication for the rest). Not being able to see the value from a client is part of why the defect hid.Singleton resources are not covered here
The brief for this work also asked for a singleton resource. A normal API client cannot create one:
access_requirements.gomapsResourceCreateSingletontomisecurity.AccessControllerOnly, and the test user getsPermissionDenied(role "u" has no access to this method). Singletons take the same code path as value resources and are covered by the backend unit testplatform/core/coretest/value_resource_canonical_id_test.go.Which CI runs it
@milaboratories/pl-clientis in both monorepo suites of the backend test workflow (core/pl/.github/workflows/test.yaml:monorepo-localfsandmonorepo-k8s-s3both pass--filter=@milaboratories/pl-client), so this check now gates every backend pull request as well as this repo's own PR suite.Verification
Against backends built from source, with the test user in the User role (
role: "u"in the JWT — the role CI uses, which enforces resource signatures):core/pl@release/4.3(4.3.2-1-g60d616, contains the fix)pl-clientsuite green (101 passed, 1 skipped) on 3 consecutive runs from a cold auth-token cachecore/pl@ tagv4.3.0(pre-fix)So the test detects the regression it pins.
The first revision of this test read through a second low-level client, which broke on session-scoped resource signatures — reproduced locally from a cold token cache as the same
Unauthenticated desc = invalid resource signatureCI reported, then fixed by keeping the read on the committing client.pl-clientformatter:check,linter:checkandtypes:checkpass; the repo-widepnpm checkpre-push hook passed (293/293 tasks).Note on the generated protobuf comment
canonicalIdinsrc/proto-grpc/.../api_types.tscarries the comment// could be empty; it depends on the resource lifecycle state, which is part of why the defect hid. That file is generated from the backend.proto, so tightening the wording belongs incore/pl, not here.Why
test / testis red on this PRThe job runs the suite against the ECR image
pl:main. That image is a snapshot ofcore/plfrom 10 Aug 2026 (4.2.14-266-main, commit2bee67977, per the run's ownplatforma-dumpartifact), while the fix merged intoplmainon 18 Aug 2026 (3922d3ad7). The image predates the fix, so the new test correctly reports the defect it pins:The same test passes against backends built from
core/plmain(0ba12964e) andrelease/4.3.pl's image-building workflow has no push-to-maintrigger (pull_request+workflow_dispatchonly) and the lastmain-ref run, from 10 Aug, is stillwaiting— so nothing has refreshedpl:mainsince. This job goes green once that image is rebuilt from currentmain; it needs no change to this branch.Note this also means every monorepo pull request is currently tested against a backend more than a week old.