feat: expose daemon artifact inventory#1020
Conversation
fe3e36b to
0bca02f
Compare
thymikee
left a comment
There was a problem hiding this comment.
I found one artifact lifecycle regression around normal downloads of retained-on-disk artifacts.
| } | ||
| return { | ||
| ...payload, | ||
| deleteAfterDownload: entry.deleteAfterDownload && options.consume !== false, |
There was a problem hiding this comment.
This changes the old one-shot tracking behavior for artifacts registered with deleteAfterDownload: false. Previously a normal GET /artifacts/:id always called cleanupDownloadableArtifact, which removed the pending registry entry and only skipped deleting the file when entry.deleteAfterDownload was false. With this return value, the HTTP layer skips cleanup entirely for those entries, so a normal non-retain download remains listed and can be downloaded repeatedly until the timeout. I think the consume decision needs to be separate from the file-delete decision, e.g. normal downloads still remove the pending entry while deleteAfterDownload only controls removal of the backing path.
There was a problem hiding this comment.
Good catch, thanks. You were right: this was conflating two separate lifecycles.
I updated the implementation so completed HTTP downloads use consumeAfterDownload to decide whether to remove the artifact from the pending inventory, while deleteAfterDownload only controls whether cleanup removes the backing source path.
I also added a regression test for deleteAfterDownload: false: a normal download now consumes the inventory entry but leaves the source file in place. For hosted/EAS sessions, AGENT_DEVICE_RETAIN_ARTIFACT_DOWNLOADS=1 sets consumeAfterDownload: false, so user downloads and the EAS poller can both fetch the same artifact.
e949769 to
40234df
Compare
|
|
||
| const artifact = prepareDownloadableArtifact(artifactId, auth.tenantId); | ||
| const artifact = await prepareDownloadableArtifact(artifactId, auth.tenantId, { | ||
| consumeAfterDownload: options.retainArtifactDownloads !== true, |
There was a problem hiding this comment.
feels kinda weird we thread "retain" through, only to change it to "consume" later
There was a problem hiding this comment.
also, there's deleteAfterDownload already?
There was a problem hiding this comment.
Agreed, this was weird. I changed it so retainArtifacts stays as an HTTP/server concern, and prepareDownloadableArtifact is back to only preparing the downloadable payload.
deleteAfterDownload keeps its existing meaning: when cleanup runs, should it remove the backing file path? retainArtifacts is separate: should a successful HTTP download skip cleanup entirely so the pending artifact remains available for another consumer.
does it make sense?
| fileName, | ||
| sizeBytes: fs.statSync(archivePath).size, | ||
| }; | ||
| if (pendingArtifacts.get(artifactId) !== entry) { |
There was a problem hiding this comment.
this compares objects? is this expected?
There was a problem hiding this comment.
Yes, intentional. entry is the exact map value captured before the async tar; if the artifact was consumed, expired, or re-tracked while archiving, pendingArtifacts.get(artifactId) will no longer be that same object, so we discard the archive instead of attaching it to a stale entry.
d8fef14 to
34b3b5a
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds an authenticated daemon HTTP artifact inventory endpoint (GET /artifacts) and extends artifact handling so directory artifacts can be downloaded as cached .tar.gz archives. It also updates the remote daemon proxy to forward inventory requests and introduces an opt-in retention mode (AGENT_DEVICE_RETAIN_ARTIFACTS=1) so downloads don’t consume artifact entries in hosted-session scenarios.
Changes:
- Add
GET /artifactsinventory endpoint (tenant-filtered) and enrich artifact download responses with MIME type + size. - Archive directory artifacts as
.tar.gzwith caching and shared in-flight archive creation. - Extend daemon proxy + client tests to support inventory forwarding and remote artifact downloads; add unit tests for inventory/download semantics and retention mode.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/cli-command-overrides.ts | Updates CLI help text to mention /artifacts inventory route support in the proxy. |
| src/utils/tests/daemon-client.test.ts | Adds a test verifying downloadRemoteArtifact hits the correct URL and uses Bearer auth. |
| src/remote/daemon-proxy.ts | Allows forwarding GET /artifacts (and /artifacts/) through the proxy. |
| src/remote/daemon-artifacts.ts | Refactors downloadRemoteArtifact params into a named type (no behavior change). |
| src/daemon/server/http-server.ts | Implements GET /artifacts inventory route; enhances download handling (MIME type/length + retention option). |
| src/daemon/server/daemon-runtime.ts | Threads AGENT_DEVICE_RETAIN_ARTIFACTS into the HTTP server creation options. |
| src/daemon/artifact-tracking.ts | Adds inventory listing, timestamps/expiry, and directory-archive creation/caching/cleanup. |
| src/daemon/tests/http-server-artifacts.test.ts | New test coverage for inventory filtering, directory archiving behavior, consumption semantics, and retention mode. |
| src/tests/daemon-proxy.test.ts | Adds proxy test coverage for forwarding /artifacts inventory responses. |
| src/tests/daemon-entrypoint.test.ts | Extends runtime test to validate retain-artifacts behavior via env var. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function readArtifactId(requestUrl: string | undefined): string { | ||
| const encoded = readRequestPathname(requestUrl).slice('/artifacts/'.length); | ||
| if (!encoded || encoded.includes('/')) return ''; | ||
| try { | ||
| return decodeURIComponent(encoded); | ||
| } catch { | ||
| return ''; | ||
| } | ||
| } |
| async function waitFor(condition: () => boolean): Promise<void> { | ||
| for (let attempt = 0; attempt < 20; attempt++) { | ||
| if (condition()) return; | ||
| await new Promise((resolve) => setTimeout(resolve, 10)); | ||
| } | ||
| } |
| try { | ||
| await runCmd( | ||
| 'tar', | ||
| [ | ||
| 'czf', | ||
| archivePath, | ||
| '-C', | ||
| path.dirname(entry.artifactPath), | ||
| '--', | ||
| path.basename(entry.artifactPath), | ||
| ], |
a5521ce to
e527d3b
Compare
|
Re-check complete: latest head is clean, CI is green, and the only post-approval delta is a test-complexity refactor in |
Summary
GET /artifactsdaemon endpoint that lists pending downloadable artifacts with filename, MIME type, size, creation time, and expiry timeAGENT_DEVICE_RETAIN_ARTIFACTS=1.tar.gzdownloads, including.tracebundles, while preserving the existing one-shot consume behavior for normal downloadsDetails
The inventory endpoint is tenant-filtered and best-effort: missing artifacts are cleaned up, and directory artifacts that fail to archive are skipped from inventory without blocking other artifacts. Direct downloads still surface artifact-specific errors.
Directory archives are cached per tracked artifact and concurrent callers share the same in-flight archive operation. Cached archives are cleaned up with the tracked artifact.
Normal downloads still consume their artifact inventory entry even when the tracked source path was registered with
deleteAfterDownload: false; that flag only controls whether cleanup removes the backing path. When the daemon starts withAGENT_DEVICE_RETAIN_ARTIFACTS=1, every directGET /artifacts/:iddownload stays retained instead. This lets EAS-style hosted sessions keep tunneling the raw daemon port while preventing user downloads from consuming artifacts before a live upload poller can upload them.Test plan
pnpm vitest run --project unit src/daemon/__tests__/http-server-artifacts.test.ts src/__tests__/daemon-entrypoint.test.ts src/__tests__/daemon-proxy.test.ts src/utils/__tests__/args.test.ts src/utils/__tests__/daemon-client.test.tspnpm vitest run --project provider-integration test/integration/provider-scenarios/daemon-http-server.test.ts test/integration/provider-scenarios/remote-daemon-client.test.tspnpm typecheckpnpm lintpnpm format:checkgit diff --check