Skip to content

Conversation

@dansamsara
Copy link
Contributor

@dansamsara dansamsara commented Nov 7, 2025

Add bounds checking in FSEventsWatcher to handle malformed fswatch
output lines that lack event flags. This prevents index out of range
panics when running taskrunner.

Here's an example of the error:

PANIC: runtime error: index out of range [1] with length 1

STACK TRACE:
┌─ goroutine 41 [running]
│
├─ github.com/samsarahq/taskrunner/watcher.(*FSEventsWatcher).Run.func1()
│  └─ ***/go/pkg/mod/github.com/samsarahq/taskrunner@v0.0.0-20251003152358-bc40d5853054/watcher/watcher.go:87 +0x2b4
│
├─ golang.org/x/sync/errgroup.(*Group).Go.func1()
│  └─ ***/go/pkg/mod/golang**.*org/x/sync@v0.17.0/errgroup/errgroup.go:93 +0x54
│
└─ created by golang.org/x/sync/errgroup.(*Group).Go in goroutine 31
   └─ ***/go/pkg/mod/golang**.*org/x/sync@v0.17.0/errgroup/errgroup.go:78 +0x94

The second commit was added to fixe the tests in CI which were broken on the
master branch before this change. See https://github.com/samsarahq/taskrunner/tree/refs/heads/master

@dansamsara dansamsara self-assigned this Nov 7, 2025
@dansamsara dansamsara force-pushed the danhansen/fix-watcher-panic branch from b1539a2 to 221dcf8 Compare November 7, 2025 00:08
Add bounds checking in FSEventsWatcher to handle malformed fswatch
output lines that lack event flags. This prevents index out of range
panics when running taskrunner.
@dansamsara dansamsara force-pushed the danhansen/fix-watcher-panic branch from 221dcf8 to a0b8367 Compare November 7, 2025 00:09
@samloop
Copy link
Contributor

samloop commented Nov 7, 2025

Do you mean to have both of these commits in this pull request?

@dansamsara
Copy link
Contributor Author

dansamsara commented Nov 7, 2025

Do you mean to have both of these commits in this pull request?

Yeah, the tests were broken on master so the unrelated commit is to fix them so they pass on this branch. See https://github.com/samsarahq/taskrunner/actions/runs/18226498829

Prior to this change, the test were failing on CI on the master branch
and thus on this branch since it was created from master.

Update vendor directory to include github.com/stretchr/testify/require
which is imported by goextensions/builder_test.go but was missing from
the vendor directory, causing CI tests to fail.
@dansamsara dansamsara force-pushed the danhansen/fix-watcher-panic branch from 3e2cb54 to bd44b54 Compare November 7, 2025 19:16
@dansamsara
Copy link
Contributor Author

dansamsara commented Nov 7, 2025

Do you mean to have both of these commits in this pull request?

Yeah, the tests were broken on master so the unrelated commit is to fix them so they pass on this branch. See samsarahq/taskrunner/actions/runs/18226498829

I updated the commit message and the PR description to help clarify

Copy link

@ran-arigur ran-arigur left a comment

Choose a reason for hiding this comment

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

Looks good to me! 🫐

@dansamsara dansamsara merged commit b7f09d6 into master Nov 7, 2025
3 checks passed
@dansamsara dansamsara deleted the danhansen/fix-watcher-panic branch November 7, 2025 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants