feat: add contents directory poller by lndmanh · Pull Request #598 · googlecolab/colab-vscode

lndmanh · 2026-05-12T04:10:15Z

Summary

Adds DirectoryPoller, the low-level polling unit for future Colab filesystem watch support. It provides support for working on #589 .

Why this matters

Colab's Jupyter Contents API does not provide native filesystem watch events. To support VS Code file watching safely, we need a small, testable polling primitive before adding orchestration or provider integration.

Changes

DirectoryPoller (src/jupyter/contents/directory-poller.ts)

Adds a ref-counted poller for one watched directory URI
Polls direct directory contents with client.get({ path, type: 'directory' }, { signal })
Uses SequentialTaskRunner with OverrunPolicy.AbandonAndRun
Emits Created, Changed, and Deleted events by diffing snapshots
Treats type flips as Deleted followed by Created
Detects changes from mtime or size differences
Uses exponential backoff on transient failures, starting at 5 seconds and capped at 5 minutes
Resets backoff after a successful poll
Emits Deleted for the watched root and disposes on terminal 404
Supports suspend() / resume(), with immediate refresh on resume
Treats missing existing clients as no-op ticks

Tests

Added unit coverage for initial snapshot behavior, created/changed/deleted diffs, type flips, size and mtime changes, exponential backoff, terminal 404 handling, suspend/resume, refcounting, and zero-ref state cleanup

…ctory-poller

kevineger · 2026-05-19T16:42:06Z

+      this.options.onDidChangeFile([
+        { type: this.options.vs.FileChangeType.Deleted, uri: this.options.uri },
+      ]);
+      this.dispose();


Self-disposing here makes the ref-counted API awkward for callers. After this runs, any subsequent release(), addRef(), start(), suspend(), or resume() call from the owner throws via guardDisposed(). The orchestration layer that holds the addRef() will, in the natural cleanup path, want to call release() on the watch handle and instead has to special-case isDisposed.

Two options worth considering:

Don't auto-dispose. Set an internal isTerminated flag, stop the runner, and let release() continue to be safe (no-op past 0).

(Preferred) Make release()/addRef() tolerant of the disposed state (return current refCount instead of throwing), and clearly document the lifecycle.

kevineger · 2026-05-19T16:42:06Z

+  private async poll(signal: AbortSignal): Promise<void> {
+    if (this.isSuspended || Date.now() < this.nextPollTimeMs) {
+      return;
+    }


The backoff is implemented by short-circuiting the poll method, but the runner is still ticking at intervalMs and invoking poll() on every interval. A couple of consequences:

The effective wake-up cadence stays at intervalMs even when backoff is e.g. 5 minutes. The runner does work (albeit, cheap) every interval, ticks the abort signal lifecycle, etc.

The actual delay between retries is intervalMs-rounded. With intervalMs=5000 and first-failure currentBackoffMs=5000, the next retry happens at the next interval tick (5s) regardless. The exponential growth only manifests at intervals of intervalMs.

For real exponential backoff with quiet idle periods, consider stopping the runner on failure and using a setTimeout to restart it after currentBackoffMs.

kevineger · 2026-05-19T16:42:06Z

+    }
+
+    log.warn(`Unable to poll ${this.options.uri.toString()}`, error);
+    this.nextPollTimeMs = Date.now() + this.currentBackoffMs;


Most exponential backoff implementations double before scheduling so the first retry already has a longer delay than the baseline. It's also probably overkill here, but most have jitter.

I don’t think jitter is necessary for this single-user VS Code polling path, but I’ll make the retry sequence explicit in the tests and use the revised scheduling model consistently.

kevineger · 2026-05-19T16:42:06Z

+    }
+    this.refCountValue -= 1;
+    if (this.refCountValue === 0) {
+      this.runner.stop();


If a poll is in flight when release() brings the count to 0, the abort propagates to the request and the task rejects with an abort-type error. That error is not a ResponseError 404 so it hits the warn/backoff branch in handlePollError. The result: after the user has released all watches, the poller still mutates nextPollTimeMs and currentBackoffMs via the in-flight task that resolves after the synchronous reset on lines 131-132. If start() is called again later, the poller starts in a backed-off state with no obvious cause.

Severity is low (the abort error path runs only once and the values get re-clobbered on the next successful poll), but the snapshot/backoff reset here isn't actually authoritative.

I verified this race. An in-flight request can still complete after release()/stop resets local state. I’ll add an in-flight dispose/abort test and guard state mutation after awaits with signal.aborted / disposed checks.

kevineger · 2026-05-19T16:42:06Z

+      continue;
+    }
+    seen.add(key);
+    coalesced.push(event);


Sorry if I'm tracing this wrong, but what case does this defend against? I'm strugling to see how duplicates can be produced. Each path appears at most once in previous and at most once in current, and the type-flip branch produces (Deleted, Created) pairs which differ by type.

No you are not wrong. I don’t see a real duplicate case inside one directory diff either. I’ll remove coalesceEvents() from DirectoryPoller.

kevineger · 2026-05-19T16:42:06Z

+      mtime: contents.lastModified
+        ? new Date(contents.lastModified).getTime()
+        : 0,
+      size: contents.size ?? 0,


Should we key on the URI string (after normalization)?

In my opinion, the current Jupyter path key is already sufficient for one poller. But keying by normalized emitted URI string should be clearer and better aligned with the events. I am considering to switch the snapshot map key to the child URI string. Do you have any thoughts on this?

kevineger · 2026-05-19T16:42:06Z

+      return;
+    }
+    this.isSuspended = false;
+    this.nextPollTimeMs = 0;


resume() unconditionally resets nextPollTimeMs = 0, which means a resume during a long backoff window discards the backoff. If suspend() was called while we were backing off after repeated failures, resume() triggers an immediate poll attempt instead of respecting the remaining backoff. Probably fine for a user-initiated resume ("I want to see the latest now"), but is this the intended behavior?

If resume is ever triggered programmatically in response to a network/state event, you could thrash a failing endpoint. No action required if this is by design, if so just leave a comment in the docblock.

Current behavior intentionally probes immediately on resume, but that is not documented. Since focus regain is user-visible and the user likely wants a fresh check, I’d keep the immediate refresh behavior and document that it bypasses the pending retry delay while preserving the current backoff level for subsequent failures.

kevineger · 2026-05-19T16:42:06Z

+    }
+    this.isStarted = true;
+    this.runner.start(StartMode.Immediately);
+  }


Nit: start() silently no-ops when refCountValue === 0. That's reasonable, but consider whether start() should call addRef() implicitly, or whether the contract is that addRef() is always called by an outer factory. If the latter, document on the class that start() requires at least one addRef() first. Otherwise debugging "why is my poller not polling?" can get confusing.

kevineger · 2026-05-19T16:42:06Z

+
+    await advance();
+    sinon.assert.callCount(client.get, 5);
+  });


This test confirms that the backoff throttles polls but doesn't make obvious the timing-vs-interval interaction. With intervalMs=5000 and the first failure setting nextPollTimeMs to t+5000, the next runner tick at t+5000 actually executes the poll (because Date.now() < nextPollTimeMs is false at equality). The doubling kicks in only after the second failure. The expectations match the implementation, but a quick comment in the test labeling the wall-clock state at each advance() (e.g., // t=10000: nextPoll=10000, executes) would make this much easier to maintain when the backoff math changes.

kevineger · 2026-05-19T17:02:27Z

+    client.get.resolves(listing([file('a.txt')]));
+
+    poller.start();
+    await advance(0);


This is effectively just flushign the micro-task queue. Are there any proper async mechanisms we can block on instead?

I’ll replace raw advance(0) calls with named helpers such as flushInitialPoll() / flushMicrotasks() so the async intent is explicit in each test.

kevineger · 2026-05-19T17:11:58Z

Thanks for sending this first PR!

Sorry it's taken me a while to get to, been pretty busy with some other work and its taken me a while to tease apart the lifecycle here.

I think we could simplify things a bit if the DirectoryPoller wasn't having to do three things at once:

A polling engine (snapshot --> diff --> events, with backoff)
A ref-counted shared resource (multiple watch() calls coalesce into one poll loop)
Its own lifecycle controller (start/suspend/resume/dispose)

What do you think of this split? I think it may align a bit closer along the VS Code seams we need to integrate with:

ContentsFileSystemProvider.watch(uri, opts): VS Code contract, returns Disposable
DirectoryWatcherRegistry: owns Map<authority+path, DirectoryPoller>
- watch(uri): Disposable: does the ref-counting; consumer never sees it
- on auth/assignment events: suspend/resume/drop the entire map
DirectoryPoller: just polls. Start in constructor, stop in dispose().
- no addRef/release
- no start/suspend/resume on the public surface (or keep suspend/resume but no ref logic)
- one job: tick, diff, emit, backoff

Sorry if this back and forth is more churn than you'd like to take on. If at any point you'd prefer I push commits and make the changes directly let me know.

lndmanh · 2026-05-22T14:37:24Z

I reviewed the lifecycle feedback against the current implementation. The main point is valid: DirectoryPoller is carrying polling, refcount sharing, and lifecycle ownership at once, and that makes the future provider wiring more awkward than it needs to be.

I revised this PR so DirectoryPoller only owns polling one directory and emitting diffs. Refcounting and map ownership would move to a follow-up DirectoryWatcherRegistry layer. For backoff, I’ll switch away from interval short-circuiting so failure backoff creates quiet idle periods instead of waking every base interval.

Kindly take a look at my newly changes and if anytime you want, just make the changes directly. Thanks for your kindly support again!

kevineger · 2026-06-04T16:19:18Z

Please feel free to re-request review (under the Reviewers section) when I should take another look!

kevineger · 2026-06-17T17:35:25Z

+    }
+
+    const client = await this.options.getClient();
+    if (!client || this.shouldIgnorePoll(signal)) {


The "missing client is a no-op tick" behavior from the PR description doesn't look covered by a test. Worth one where getClient resolves undefined and we assert no events fire and the backoff stays put? It's easy to silently regress this into a spurious backoff otherwise.

Agreed. I traced it: when getClient() resolves undefined, poll() returns before the try/catch, so nothing emits, currentBackoffMs is untouched, no retry is armed, and the base interval keeps ticking - but nothing locks that in. Adding two tests that prove it via cadence (matching the existing backoff tests rather than reaching into private state):

a no-op tick followed by a successful poll on the next base interval (proves no spurious backoff delay)

a no-op tick after a prior failure, asserting the elevated backoff is preserved (not reset, not re-armed).

kevineger · 2026-06-17T17:35:25Z

+      { path: this.options.uri.path, type: ContentsGetTypeEnum.Directory },
+      { signal },
+    );
+    if (!isDirectoryContents(contents)) {


A non-directory response throws here and lands in the backoff branch. In that case (e.g. a path that's actually a file or a transient wrong-type response), would we retry forever (capped at 5 min) and warn each cycle?

Yes, and it's actually reachable via two paths, only one of which is this line:

Stock Jupyter (the common case): get({ type: 'directory' }) on a path that is now a file rejects with ResponseError 400, reason "bad type" before this isDirectoryContents guard is even reached. handlePollError only treats 404 as terminal, so the 400 falls through to log.warn + scheduleRetry → infinite retry capped at 5 min, warning every cycle.

This line: a non-compliant custom ContentsManager could instead resolve a coerced file model, which throws the generic Error and lands in the same backoff branch.

A file-where-a-directory-was is a stable state, not a transient one, so backoff can never recover. I'll treat it like the existing 404 terminal path: emit Deleted(rootUri), fire onDidTerminate(), and stop (renaming handleTerminalDelete → handleTerminalGone since it now covers "deleted or no longer a directory"). I'll route the 400 in handlePollError and the coerced-model case here. Since this poller only ever sends type=directory with no format, the only 400 it can produce is "bad type", so I'll scope the terminal check to 400/404 and leave 401/403/5xx on the transient backoff path. Adding tests for both routes mirrored on the existing 404 test.

kevineger · 2026-06-17T20:26:57Z

+  async function flushInitialPoll(): Promise<void> {
+    await clock.tickAsync(0);
+  }
+
+  async function flushMicrotasks(): Promise<void> {
+    await clock.tickAsync(0);
+  }


Do you see any way around needing this microtask flushing? It can be very brittle and it's way better to rely on proper async mechanisms (e.g. deferred promises we control) where possible.

ConsumptionPoller's test already has the pattern I should be using here (same SequentialTaskRunner + immediate poll). I'll drop the tickAsync(0) pumping and synchronize on observable side-effects with our Deferred helper: resolve a deferred at the top of the client.get callsFake (poll started) and inside the change listener (diff emitted), then await those. Genuine time advancement stays as tickAsync(INTERVAL) (which also drains the synchronous tail), so there's no bare tick(0) left as a load-bearing synchronizer.

When a watched directory is replaced by a file, polling can never recover, so the poller should stop rather than retry forever. Previously only HTTP 404 was terminal; a wrong-type response fell through to the warn + exponential-backoff branch and retried indefinitely (capped at 5 minutes), logging a warning every cycle. This happens two ways: stock Jupyter rejects `get({ type: 'directory' })` on a file path with HTTP 400 "bad type", and a non-compliant contents manager may instead resolve a coerced file model. Both now route to the terminal handler (renamed handleTerminalDelete -> handleTerminalGone), which emits Deleted for the watched root and notifies the owner via onDidTerminate, exactly like the existing 404 path. The 400 check is scoped to this request shape (type=directory, no format), whose only possible 400 is "bad type"; transient 401/403/5xx failures keep backing off.

Addresses the review feedback on the poller tests: - Replace `clock.tickAsync(0)` microtask pumping (flushInitialPoll / flushMicrotasks) with deterministic synchronization on observable side-effects, mirroring the ConsumptionPoller unit test: `pollStarted(n)` resolves when the n-th client.get begins, and `nextEvents` resolves with the emitted events when the change listener fires. Genuine time advancement still uses `tickAsync(interval)`; only the zero-tick microtask pumping is removed. - Cover the missing-client no-op tick: getClient() resolving undefined must emit nothing, leave the backoff untouched, and keep the base interval running (proven via cadence, including across an elevated backoff). - Cover terminal wrong-type responses (HTTP 400 "bad type" and a coerced non-directory model).

lndmanh added 2 commits May 8, 2026 13:10

feat: add contents directory poller

99e6bb1

Merge remote-tracking branch 'upstream/main' into contents-watch-dire…

ebb1748

…ctory-poller

kevineger reviewed May 19, 2026

View reviewed changes

refactor: simplify directory poller lifecycle

d744c3e

Merge branch 'main' into contents-watch-directory-poller

b5ba3bb

Merge branch 'main' into contents-watch-directory-poller

0131bb5

lndmanh requested a review from kevineger June 4, 2026 16:53

lndmanh added 2 commits June 13, 2026 11:53

Merge branch 'main' into contents-watch-directory-poller

824000b

Merge branch 'main' into contents-watch-directory-poller

b284fc1

kevineger requested changes Jun 17, 2026

View reviewed changes

lndmanh added 2 commits June 18, 2026 11:20

lndmanh requested a review from kevineger June 18, 2026 04:28

Merge branch 'main' into contents-watch-directory-poller

68dde5d

Conversation

lndmanh commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why this matters

Changes

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevineger commented May 19, 2026

Uh oh!

lndmanh commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevineger commented Jun 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lndmanh commented May 12, 2026 •

edited

Loading

lndmanh commented May 22, 2026 •

edited

Loading