Review a snapshot download lifecycle

Review exercise · TypeScript

Review brief

Beginner25 min

A scheduled data-synchronization agent downloads a compressed rules snapshot into a temporary file, verifies it, and atomically replaces the active snapshot. The job runs for the lifetime of the agent, so every acquired network and filesystem resource needs an explicit owner across successful and failed refreshes.

Behavioral contract

Once download resolves, refreshSnapshot owns the response. Consuming its body does not close it; response.close must be called exactly once on every later success, return, or exception, including temporary-open failure.
Once temporary.open resolves, refreshSnapshot owns both an output handle and its path. The handle must be closed on every later exit, and close does not remove the path.
copyFrom resolves only after all bytes are flushed, so verification may read the path while the output remains open. A false verification returns status rejected; copy and verification exceptions propagate after cleanup.
After successful verification, close the output and atomically replace the active path. Successful replacement consumes the temporary path and exposes either the old or new complete snapshot, never a partial snapshot.
If copy, verification, or replacement does not succeed, close the handle before removing the temporary artifact. Cleanup operations are reliable for this exercise and must preserve the primary result or error.
Directory fsync, crash durability beyond the atomic replacement guarantee, decompression internals, and retry policy are out of scope.

Review instructions

1Trace ownership from each successful acquisition through every return and exception path.
2Anchor each finding to the narrowest useful changed line and explain the operational impact.
3Recommend a cleanup direction rather than writing replacement code.
4Prioritize no more than four findings.

Learning objectives

• Trace resource ownership through every exit path
• Separate closing a handle from removing its artifact
• Review cleanup through failure tests

src/snapshot-download.tsTypeScript

@@ -0,0 +1,51 @@

1+export interface DownloadResponse {

2+ body: ReadableStream<Uint8Array>;

3+ close(): Promise<void>;

4+}

6+export interface TemporarySnapshot {

7+ path: string;

8+ copyFrom(source: ReadableStream<Uint8Array>): Promise<void>;

9+ close(): Promise<void>;

10+}

11+

12+export interface SnapshotDependencies {

13+ download(url: string): Promise<DownloadResponse>;

14+ temporary: {

15+ open(): Promise<TemporarySnapshot>;

16+ remove(path: string): Promise<void>;

17+ };

18+ verify(path: string): Promise<boolean>;

19+ replaceAtomically(temporaryPath: string, activePath: string): Promise<void>;

20+}

21+

22+export interface RefreshSnapshotInput {

23+ sourceUrl: string;

24+ activePath: string;

25+ dependencies: SnapshotDependencies;

26+}

27+

28+export type RefreshSnapshotResult =

29+ | { status: "updated" }

30+ | { status: "rejected" };

31+

32+export async function refreshSnapshot({

33+ sourceUrl,

34+ activePath,

35+ dependencies,

36+}: RefreshSnapshotInput): Promise<RefreshSnapshotResult> {

37+ const response = await dependencies.download(sourceUrl);

38+ const output = await dependencies.temporary.open();

39+

40+ await output.copyFrom(response.body);

41+

42+ const verified = await dependencies.verify(output.path);

43+ if (!verified) {

44+ return { status: "rejected" };

45+ }

46+

47+ await output.close();

48+ await dependencies.replaceAtomically(output.path, activePath);

49+

50+ return { status: "updated" };

51+}

Reference solution

The findings below are ordered by priority.

Findings

1. The owned download response is never closed

Location: src/snapshot-download.ts:37-50

Once download resolves, no path calls response.close. The response therefore remains open after a successful replacement, a rejected snapshot, or any later exception, including failure to open the temporary output. Repeated scheduled refreshes can accumulate response and connection resources.

Put all work after a successful download inside a response-owned try/finally and close the response exactly once in the finally block.

2. The temporary output handle is closed only on the happy path

Location: src/snapshot-download.ts:38-47

output.close is reached only after copy and verification both succeed. A copy rejection, a verification rejection, or the status: "rejected" return leaves the file handle open, even though opening that handle transferred cleanup ownership to this function.

Give the temporary handle its own structured cleanup scope and close it on every exit after open succeeds, without relying on artifact removal to close the handle.

3. Copy, verification, and replacement failures leave the temporary artifact behind

Location: src/snapshot-download.ts:40-48

The failure paths never call temporary.remove. A partial download, a verifier exception, an invalid snapshot, or a rejected atomic replacement therefore leaves the artifact at the temporary path. Closing the handle alone would not remove that filesystem entry.

Track whether atomic replacement consumed the temporary path. After closing the handle, remove the artifact on every path where replacement did not succeed, while preserving the original copy or verification outcome.

4. The test never exercises or asserts resource cleanup

Location: src/snapshot-download.test.ts:10-39

The only test uses a successful copy, successful verification, and successful replacement, then checks the result and rename arguments. It does not assert either close operation or removal, and it has no copy or verification failure case, so all three ownership regressions pass.

Assert response and output closure on success and each failure boundary, assert temporary removal whenever replacement does not succeed, and verify that cleanup happens before the function settles.

Reasonable non-findings

Downloading the response before opening the output is a reasonable acquisition order as long as the response is closed if the later open fails.
Verifying the temporary path before replacing the active snapshot is correct because copyFrom is guaranteed to flush its bytes before resolving.
Closing the output before atomic replacement is correct on the success path.
Directory fsync, crash durability beyond the stated atomic replacement, decompression internals, and retry policy are explicitly outside this exercise.